| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZixV3lCe3jYQN3Qx@debug.ba.rivosinc.com>
Date: Fri, 26 Apr 2024 18:33:18 -0700
From: Deepak Gupta <debug@...osinc.com>
To: Clément Léger <cleger@...osinc.com>
Cc: Conor Dooley <conor@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Anup Patel <anup@...infault.org>,
Atish Patra <atishp@...shpatra.org>,
linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
kvm-riscv@...ts.infradead.org, Ved Shanbhogue <ved@...osinc.com>
Subject: Re: [RFC PATCH 7/7] RISC-V: KVM: add support for double trap
exception
On Thu, Apr 18, 2024 at 04:26:46PM +0200, Clément Léger wrote:
>When a double trap exception is generated from VS-mode, it should be
>delivered to M-mode which might redirect it to S-mode. Currently, the
>kvm double trap exception handling simply prints an error and returns
>-EOPNOTSUPP to stop VM execution. In future, this might use KVM SBI SSE
>extension implementation to actually send an SSE event to the guest VM.
>
>Signed-off-by: Clément Léger <cleger@...osinc.com>
>---
> arch/riscv/include/asm/kvm_host.h | 7 ++++---
> arch/riscv/include/uapi/asm/kvm.h | 1 +
> arch/riscv/kvm/vcpu.c | 23 +++++++++------------
> arch/riscv/kvm/vcpu_exit.c | 33 +++++++++++++++++++++++++------
> arch/riscv/kvm/vcpu_insn.c | 15 +++++---------
> arch/riscv/kvm/vcpu_onereg.c | 2 ++
> arch/riscv/kvm/vcpu_sbi.c | 4 +---
> arch/riscv/kvm/vcpu_switch.S | 19 +++++++++++++++---
> 8 files changed, 65 insertions(+), 39 deletions(-)
>
>diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
>index be60aaa07f57..1d699bf44c45 100644
>--- a/arch/riscv/include/asm/kvm_host.h
>+++ b/arch/riscv/include/asm/kvm_host.h
>@@ -358,12 +358,13 @@ unsigned long kvm_riscv_vcpu_unpriv_read(struct kvm_vcpu *vcpu,
> bool read_insn,
> unsigned long guest_addr,
> struct kvm_cpu_trap *trap);
>-void kvm_riscv_vcpu_trap_redirect(struct kvm_vcpu *vcpu,
>- struct kvm_cpu_trap *trap);
>+int kvm_riscv_vcpu_trap_redirect(struct kvm_vcpu *vcpu,
>+ struct kvm_cpu_trap *trap);
> int kvm_riscv_vcpu_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
> struct kvm_cpu_trap *trap);
>
>-void __kvm_riscv_switch_to(struct kvm_vcpu_arch *vcpu_arch);
>+void __kvm_riscv_switch_to(struct kvm_vcpu_arch *vcpu_arch,
>+ struct kvm_cpu_trap *trap);
>
> void kvm_riscv_vcpu_setup_isa(struct kvm_vcpu *vcpu);
> unsigned long kvm_riscv_vcpu_num_regs(struct kvm_vcpu *vcpu);
>diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
>index fa3097da91c0..323f4e8380d2 100644
>--- a/arch/riscv/include/uapi/asm/kvm.h
>+++ b/arch/riscv/include/uapi/asm/kvm.h
>@@ -166,6 +166,7 @@ enum KVM_RISCV_ISA_EXT_ID {
> KVM_RISCV_ISA_EXT_ZVFH,
> KVM_RISCV_ISA_EXT_ZVFHMIN,
> KVM_RISCV_ISA_EXT_ZFA,
>+ KVM_RISCV_ISA_EXT_SSDBLTRP,
> KVM_RISCV_ISA_EXT_MAX,
> };
>
>diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
>index 461ef60d4eda..89e663defe14 100644
>--- a/arch/riscv/kvm/vcpu.c
>+++ b/arch/riscv/kvm/vcpu.c
>@@ -121,6 +121,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> /* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
> cntx = &vcpu->arch.guest_reset_context;
> cntx->sstatus = SR_SPP | SR_SPIE;
>+ if (riscv_isa_extension_available(vcpu->arch.isa, SSDBLTRP))
>+ cntx->sstatus |= SR_SDT;
> cntx->hstatus = 0;
> cntx->hstatus |= HSTATUS_VTW;
> cntx->hstatus |= HSTATUS_SPVP;
>@@ -579,6 +581,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> csr->hvip = csr_read(CSR_HVIP);
> csr->vsatp = csr_read(CSR_VSATP);
> cfg->hedeleg = csr_read(CSR_HEDELEG);
>+ cfg->henvcfg = csr_read(CSR_HENVCFG);
>+ if (IS_ENABLED(CONFIG_32BIT))
>+ cfg->henvcfg = csr_read(CSR_HENVCFGH) << 32;
> }
>
> static void kvm_riscv_check_vcpu_requests(struct kvm_vcpu *vcpu)
>@@ -670,11 +675,12 @@ static __always_inline void kvm_riscv_vcpu_swap_in_host_state(struct kvm_vcpu *v
> * This must be noinstr as instrumentation may make use of RCU, and this is not
> * safe during the EQS.
> */
>-static void noinstr kvm_riscv_vcpu_enter_exit(struct kvm_vcpu *vcpu)
>+static void noinstr kvm_riscv_vcpu_enter_exit(struct kvm_vcpu *vcpu,
>+ struct kvm_cpu_trap *trap)
> {
> kvm_riscv_vcpu_swap_in_guest_state(vcpu);
> guest_state_enter_irqoff();
>- __kvm_riscv_switch_to(&vcpu->arch);
>+ __kvm_riscv_switch_to(&vcpu->arch, trap);
> vcpu->arch.last_exit_cpu = vcpu->cpu;
> guest_state_exit_irqoff();
> kvm_riscv_vcpu_swap_in_host_state(vcpu);
>@@ -789,22 +795,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>
> guest_timing_enter_irqoff();
>
>- kvm_riscv_vcpu_enter_exit(vcpu);
>+ kvm_riscv_vcpu_enter_exit(vcpu, &trap);
>
> vcpu->mode = OUTSIDE_GUEST_MODE;
> vcpu->stat.exits++;
>
>- /*
>- * Save SCAUSE, STVAL, HTVAL, and HTINST because we might
>- * get an interrupt between __kvm_riscv_switch_to() and
>- * local_irq_enable() which can potentially change CSRs.
>- */
>- trap.sepc = vcpu->arch.guest_context.sepc;
>- trap.scause = csr_read(CSR_SCAUSE);
>- trap.stval = csr_read(CSR_STVAL);
>- trap.htval = csr_read(CSR_HTVAL);
>- trap.htinst = csr_read(CSR_HTINST);
>-
> /* Syncup interrupts state with HW */
> kvm_riscv_vcpu_sync_interrupts(vcpu);
>
>diff --git a/arch/riscv/kvm/vcpu_exit.c b/arch/riscv/kvm/vcpu_exit.c
>index 2415722c01b8..892c6df97eaf 100644
>--- a/arch/riscv/kvm/vcpu_exit.c
>+++ b/arch/riscv/kvm/vcpu_exit.c
>@@ -126,17 +126,34 @@ unsigned long kvm_riscv_vcpu_unpriv_read(struct kvm_vcpu *vcpu,
> return val;
> }
>
>+static int kvm_riscv_double_trap(struct kvm_vcpu *vcpu,
>+ struct kvm_cpu_trap *trap)
>+{
>+ pr_err("Guest double trap");
>+ /* TODO: Implement SSE support */
>+
>+ return -EOPNOTSUPP;
>+}
>+
> /**
> * kvm_riscv_vcpu_trap_redirect -- Redirect trap to Guest
> *
> * @vcpu: The VCPU pointer
> * @trap: Trap details
> */
>-void kvm_riscv_vcpu_trap_redirect(struct kvm_vcpu *vcpu,
>- struct kvm_cpu_trap *trap)
>+int kvm_riscv_vcpu_trap_redirect(struct kvm_vcpu *vcpu,
>+ struct kvm_cpu_trap *trap)
> {
> unsigned long vsstatus = csr_read(CSR_VSSTATUS);
>
>+ if (riscv_isa_extension_available(vcpu->arch.isa, SSDBLTRP)) {
>+ if (vsstatus & SR_SDT)
>+ return kvm_riscv_double_trap(vcpu, trap);
>+
>+ /* Set Double Trap bit to enable double trap detection */
>+ vsstatus |= SR_SDT;
I didn't get it.
Why do this without checking if current config allows us to do so ?
I am imagining we do this only when henvcfg for current vcpu says that DTE=1
for this this guest, right?
>+ }
>+
> /* Change Guest SSTATUS.SPP bit */
> vsstatus &= ~SR_SPP;
> if (vcpu->arch.guest_context.sstatus & SR_SPP)
>@@ -163,6 +180,8 @@ void kvm_riscv_vcpu_trap_redirect(struct kvm_vcpu *vcpu,
>
> /* Set Guest privilege mode to supervisor */
> vcpu->arch.guest_context.sstatus |= SR_SPP;
>+
>+ return 1;
> }
>
> /*
>@@ -185,10 +204,8 @@ int kvm_riscv_vcpu_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
> case EXC_INST_ILLEGAL:
> case EXC_LOAD_MISALIGNED:
> case EXC_STORE_MISALIGNED:
>- if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV) {
>- kvm_riscv_vcpu_trap_redirect(vcpu, trap);
>- ret = 1;
>- }
>+ if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV)
>+ ret = kvm_riscv_vcpu_trap_redirect(vcpu, trap);
> break;
> case EXC_VIRTUAL_INST_FAULT:
> if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV)
>@@ -204,6 +221,10 @@ int kvm_riscv_vcpu_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
> if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV)
> ret = kvm_riscv_vcpu_sbi_ecall(vcpu, run);
> break;
>+ case EXC_DOUBLE_TRAP:
>+ if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV)
>+ ret = kvm_riscv_double_trap(vcpu, trap);
>+ break;
> default:
> break;
> }
>diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
>index 7a6abed41bc1..050e811204f2 100644
>--- a/arch/riscv/kvm/vcpu_insn.c
>+++ b/arch/riscv/kvm/vcpu_insn.c
>@@ -159,9 +159,8 @@ static int truly_illegal_insn(struct kvm_vcpu *vcpu, struct kvm_run *run,
> utrap.stval = insn;
> utrap.htval = 0;
> utrap.htinst = 0;
>- kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
>
>- return 1;
>+ return kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
> }
>
> static int truly_virtual_insn(struct kvm_vcpu *vcpu, struct kvm_run *run,
>@@ -175,9 +174,8 @@ static int truly_virtual_insn(struct kvm_vcpu *vcpu, struct kvm_run *run,
> utrap.stval = insn;
> utrap.htval = 0;
> utrap.htinst = 0;
>- kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
>
>- return 1;
>+ return kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
> }
>
> /**
>@@ -422,8 +420,7 @@ int kvm_riscv_vcpu_virtual_insn(struct kvm_vcpu *vcpu, struct kvm_run *run,
> &utrap);
> if (utrap.scause) {
> utrap.sepc = ct->sepc;
>- kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
>- return 1;
>+ return kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
> }
> }
> if (INSN_IS_16BIT(insn))
>@@ -478,8 +475,7 @@ int kvm_riscv_vcpu_mmio_load(struct kvm_vcpu *vcpu, struct kvm_run *run,
> if (utrap.scause) {
> /* Redirect trap if we failed to read instruction */
> utrap.sepc = ct->sepc;
>- kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
>- return 1;
>+ return kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
> }
> insn_len = INSN_LEN(insn);
> }
>@@ -604,8 +600,7 @@ int kvm_riscv_vcpu_mmio_store(struct kvm_vcpu *vcpu, struct kvm_run *run,
> if (utrap.scause) {
> /* Redirect trap if we failed to read instruction */
> utrap.sepc = ct->sepc;
>- kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
>- return 1;
>+ return kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
> }
> insn_len = INSN_LEN(insn);
> }
>diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
>index 5f7355e96008..fece0043871c 100644
>--- a/arch/riscv/kvm/vcpu_onereg.c
>+++ b/arch/riscv/kvm/vcpu_onereg.c
>@@ -36,6 +36,7 @@ static const unsigned long kvm_isa_ext_arr[] = {
> /* Multi letter extensions (alphabetically sorted) */
> KVM_ISA_EXT_ARR(SMSTATEEN),
> KVM_ISA_EXT_ARR(SSAIA),
>+ KVM_ISA_EXT_ARR(SSDBLTRP),
> KVM_ISA_EXT_ARR(SSTC),
> KVM_ISA_EXT_ARR(SVINVAL),
> KVM_ISA_EXT_ARR(SVNAPOT),
>@@ -153,6 +154,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext)
> case KVM_RISCV_ISA_EXT_ZVKSED:
> case KVM_RISCV_ISA_EXT_ZVKSH:
> case KVM_RISCV_ISA_EXT_ZVKT:
>+ case KVM_RISCV_ISA_EXT_SSDBLTRP:
> return false;
> /* Extensions which can be disabled using Smstateen */
> case KVM_RISCV_ISA_EXT_SSAIA:
>diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
>index 76901f0f34b7..b839d578dc26 100644
>--- a/arch/riscv/kvm/vcpu_sbi.c
>+++ b/arch/riscv/kvm/vcpu_sbi.c
>@@ -456,10 +456,8 @@ int kvm_riscv_vcpu_sbi_ecall(struct kvm_vcpu *vcpu, struct kvm_run *run)
>
> /* Handle special error cases i.e trap, exit or userspace forward */
> if (sbi_ret.utrap->scause) {
>- /* No need to increment sepc or exit ioctl loop */
>- ret = 1;
> sbi_ret.utrap->sepc = cp->sepc;
>- kvm_riscv_vcpu_trap_redirect(vcpu, sbi_ret.utrap);
>+ ret = kvm_riscv_vcpu_trap_redirect(vcpu, sbi_ret.utrap);
> next_sepc = false;
> goto ecall_done;
> }
>diff --git a/arch/riscv/kvm/vcpu_switch.S b/arch/riscv/kvm/vcpu_switch.S
>index 0c26189aa01c..94d5eb9da788 100644
>--- a/arch/riscv/kvm/vcpu_switch.S
>+++ b/arch/riscv/kvm/vcpu_switch.S
>@@ -154,7 +154,6 @@ SYM_FUNC_START(__kvm_riscv_switch_to)
> REG_L t2, (KVM_ARCH_HOST_SSCRATCH)(a0)
> REG_L t3, (KVM_ARCH_HOST_SCOUNTEREN)(a0)
> REG_L t4, (KVM_ARCH_HOST_HSTATUS)(a0)
>- REG_L t5, (KVM_ARCH_HOST_SSTATUS)(a0)
>
> /* Save Guest SEPC */
> csrr t0, CSR_SEPC
>@@ -171,8 +170,8 @@ SYM_FUNC_START(__kvm_riscv_switch_to)
> /* Save Guest and Restore Host HSTATUS */
> csrrw t4, CSR_HSTATUS, t4
>
>- /* Save Guest and Restore Host SSTATUS */
>- csrrw t5, CSR_SSTATUS, t5
>+ /* Save Guest SSTATUS */
>+ csrr t5, CSR_SSTATUS
>
> /* Store Guest CSR values */
> REG_S t0, (KVM_ARCH_GUEST_SEPC)(a0)
>@@ -206,6 +205,20 @@ SYM_FUNC_START(__kvm_riscv_switch_to)
> REG_L s10, (KVM_ARCH_HOST_S10)(a0)
> REG_L s11, (KVM_ARCH_HOST_S11)(a0)
>
>+ csrr t1, CSR_SCAUSE
>+ csrr t2, CSR_STVAL
>+ csrr t3, CSR_HTVAL
>+ csrr t4, CSR_HTINST
>+ REG_S t0, (KVM_ARCH_TRAP_SEPC)(a1)
>+ REG_S t1, (KVM_ARCH_TRAP_SCAUSE)(a1)
>+ REG_S t2, (KVM_ARCH_TRAP_STVAL)(a1)
>+ REG_S t3, (KVM_ARCH_TRAP_HTVAL)(a1)
>+ REG_S t4, (KVM_ARCH_TRAP_HTINST)(a1)
>+
>+ /* Restore Host SSTATUS */
>+ REG_L t5, (KVM_ARCH_HOST_SSTATUS)(a0)
>+ csrw CSR_SSTATUS, t5
>+
> /* Return to C code */
> ret
> SYM_FUNC_END(__kvm_riscv_switch_to)
>--
>2.43.0
>
>
Powered by blists - more mailing lists