A double trap typically arises during a sensitive phase in trap handling
operations — when an exception or interrupt occurs while the trap
handler (the component responsible for managing these events) is in a
non-reentrant state. This non-reentrancy usually occurs in the early
phase of trap handling, wherein the trap handler has not yet preserved
the necessary state to handle and resume from the trap. The occurrence
of such event is unlikely but can happen when dealing with hardware
errors.
This series adds support for Ssdbltrp[1]. It is based on SSE support as well as
firmware feature to enable double trap.
Ssdbltrp can be tested using qemu[1], opensbi[2], linux[3] and
kvm-unit-tests[5]. Assuming you have a riscv environment available and
configured (CROSS_COMPILE), it can be built for riscv64 using the
following instructions:
Qemu:
$ git clone https://github.com/rivosinc/qemu.git
$ cd qemu
$ git switch dev/cleger/dbltrp_rfc_v1
$ mkdir build && cd build
$ ../configure --target-list=riscv64-softmmu
$ make
OpenSBI:
$ git clone https://github.com/rivosinc/opensbi.git
$ cd opensbi
$ git switch dev/cleger/dbltrp_rfc_v1
$ make O=build PLATFORM_RISCV_XLEN=64 PLATFORM=generic
Linux:
$ git clone https://github.com/rivosinc/linux.git
$ cd linux
$ git switch dev/cleger/dbltrp_rfc_v1
$ export ARCH=riscv
$ make O=build defconfig
$ ./script/config --file build/.config --enable RISCV_DBLTRP
$ make O=build
kvm-unit-tests:
$ git clone https://github.com/clementleger/kvm-unit-tests.git
$ cd kvm-unit-tests
$ git switch dev/cleger/dbltrp_rfc_v1
$ ./configure --arch=riscv64 --cross-prefix=$CROSS_COMPILE
$ make
You will also need kvmtool in your rootfs. One can build a buildroot
rootfs using the buildroot provided at [6] (which contains an update
of kvmtool with riscv support).
Run with kvm-unit-test test as kernel:
$ qemu-system-riscv64 \
-M virt \
-cpu rv64,x-ssdbltrp=true,x-smdbltrp=true \
-nographic \
-serial mon:stdio \
-bios opensbi/build/platform/generic/firmware/fw_jump.bin \
-kernel kvm-unit-tests-dbltrp/riscv/sbi_dbltrp.flat
...
[OpenSBI boot partially elided]
Boot HART ISA Extensions : sscofpmf,sstc,zicntr,zihpm,zicboz,zicbom,sdtrig,svadu,ssdbltrp
...
##########################################################################
# kvm-unit-tests
##########################################################################
PASS: sbi: fwft: FWFT extension probing no error
PASS: sbi: fwft: FWFT extension is present
PASS: sbi: fwft: dbltrp: Get double trap enable feature value
PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 0
PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 0
PASS: sbi: fwft: dbltrp: Double trap disabled, trap first time ok
PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 1
PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 1
PASS: sbi: fwft: dbltrp: Trapped twice allowed ok
INFO: sbi: fwft: dbltrp: Should generate a double trap and crash !
sbi_trap_error: hart0: trap0: double trap handler failed (error -10)
sbi_trap_error: hart0: trap0: mcause=0x0000000000000010 mtval=0x0000000000000000
sbi_trap_error: hart0: trap0: mtval2=0x0000000000000003 mtinst=0x0000000000000000
sbi_trap_error: hart0: trap0: mepc=0x00000000802000d8 mstatus=0x8000000a01006900
sbi_trap_error: hart0: trap0: ra=0x00000000802001fc sp=0x0000000080213e70
sbi_trap_error: hart0: trap0: gp=0x0000000000000000 tp=0x0000000080088000
sbi_trap_error: hart0: trap0: s0=0x0000000080213e80 s1=0x0000000000000001
sbi_trap_error: hart0: trap0: a0=0x0000000080213e80 a1=0x0000000080208193
sbi_trap_error: hart0: trap0: a2=0x000000008020dc20 a3=0x000000000000000f
sbi_trap_error: hart0: trap0: a4=0x0000000080210cd8 a5=0x00000000802110d0
sbi_trap_error: hart0: trap0: a6=0x00000000802136e4 a7=0x0000000046574654
sbi_trap_error: hart0: trap0: s2=0x0000000080210cd9 s3=0x0000000000000000
sbi_trap_error: hart0: trap0: s4=0x0000000000000000 s5=0x0000000000000000
sbi_trap_error: hart0: trap0: s6=0x0000000000000000 s7=0x0000000000000001
sbi_trap_error: hart0: trap0: s8=0x0000000000002000 s9=0x0000000080083700
sbi_trap_error: hart0: trap0: s10=0x0000000000000000 s11=0x0000000000000000
sbi_trap_error: hart0: trap0: t0=0x0000000000000000 t1=0x0000000080213ed8
sbi_trap_error: hart0: trap0: t2=0x0000000000001000 t3=0x0000000080213ee0
sbi_trap_error: hart0: trap0: t4=0x0000000000000000 t5=0x000000008020f8d0
sbi_trap_error: hart0: trap0: t6=0x0000000000000000
Run with linux and kvm-unit-test test in kvm (testing VS-mode):
$ qemu-system-riscv64 \
-M virt \
-cpu rv64,x-ssdbltrp=true,x-smdbltrp=true \
-nographic \
-serial mon:stdio \
-bios opensbi/build/platform/generic/firmware/fw_jump.bin \
-kernel linux/build/arch/riscv/boot/Image
...
[Linux boot partially elided]
[ 0.735079] riscv-dbltrp: Double trap handling registered
...
$ lkvm run -k sbi_dbltrp.flat -m 128 -c 2
##########################################################################
# kvm-unit-tests
##########################################################################
PASS: sbi: fwft: FWFT extension probing no error
PASS: sbi: fwft: FWFT extension is present
PASS: sbi: fwft: dbltrp: Get double trap enable feature value
PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 0
PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 0
PASS: sbi: fwft: dbltrp: Double trap disabled, trap first time ok
PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 1
PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 1
PASS: sbi: fwft: dbltrp: Trapped twice allowed ok
INFO: sbi: fwft: dbltrp: Should generate a double trap and crash !
[ 51.939077] Guest double trap
[ 51.939323] kvm [93]: VCPU exit error -95
[ 51.939683] kvm [93]: SEPC=0x802000d8 SSTATUS=0x200004520 HSTATUS=0x200200180
[ 51.939947] kvm [93]: SCAUSE=0x10 STVAL=0x0 HTVAL=0x3 HTINST=0x0
KVM_RUN failed: Operation not supported
$
Link: https://github.com/riscv/riscv-double-trap/releases/download/v0.56/riscv-double-trap.pdf [1]
Link: https://github.com/rivosinc/qemu/tree/dev/cleger/dbltrp_rfc_v1 [2]
Link: https://github.com/rivosinc/opensbi/tree/dev/cleger/dbltrp_rfc_v1 [3]
Link: https://github.com/rivosinc/linux/tree/dev/cleger/dbltrp_rfc_v1 [4]
Link: https://github.com/clementleger/kvm-unit-tests/tree/dev/cleger/dbltrp_rfc_v1 [5]
Link: https://github.com/clementleger/buildroot/tree/dev/cleger/kvmtool [6]
---
Clément Léger (7):
riscv: kvm: add support for FWFT SBI extension
dt-bindings: riscv: add Ssdbltrp ISA extension description
riscv: add Ssdbltrp ISA extension parsing
riscv: handle Ssdbltrp mstatus SDT bit
riscv: add double trap driver
riscv: kvm: add SBI FWFT support for SBI_FWFT_DOUBLE_TRAP_ENABLE
RISC-V: KVM: add support for double trap exception
.../devicetree/bindings/riscv/extensions.yaml | 6 +
arch/riscv/include/asm/csr.h | 3 +
arch/riscv/include/asm/hwcap.h | 1 +
arch/riscv/include/asm/kvm_host.h | 12 +-
arch/riscv/include/asm/kvm_vcpu_sbi.h | 1 +
arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 37 ++++
arch/riscv/include/asm/sbi.h | 1 +
arch/riscv/include/uapi/asm/kvm.h | 2 +
arch/riscv/kernel/cpufeature.c | 1 +
arch/riscv/kernel/entry.S | 52 ++---
arch/riscv/kernel/head.S | 4 +
arch/riscv/kernel/sse_entry.S | 4 +-
arch/riscv/kvm/Makefile | 1 +
arch/riscv/kvm/vcpu.c | 28 +--
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 | 8 +-
arch/riscv/kvm/vcpu_sbi_fwft.c | 177 ++++++++++++++++++
arch/riscv/kvm/vcpu_switch.S | 19 +-
drivers/firmware/Kconfig | 7 +
drivers/firmware/Makefile | 1 +
drivers/firmware/riscv_dbltrp.c | 95 ++++++++++
include/linux/riscv_dbltrp.h | 19 ++
24 files changed, 466 insertions(+), 63 deletions(-)
create mode 100644 arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
create mode 100644 arch/riscv/kvm/vcpu_sbi_fwft.c
create mode 100644 drivers/firmware/riscv_dbltrp.c
create mode 100644 include/linux/riscv_dbltrp.h
--
2.43.0
Add support for FWFT extension in KVM
Signed-off-by: Clément Léger <[email protected]>
---
arch/riscv/include/asm/kvm_host.h | 5 +
arch/riscv/include/asm/kvm_vcpu_sbi.h | 1 +
arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 37 ++++++
arch/riscv/include/uapi/asm/kvm.h | 1 +
arch/riscv/kvm/Makefile | 1 +
arch/riscv/kvm/vcpu.c | 5 +
arch/riscv/kvm/vcpu_sbi.c | 4 +
arch/riscv/kvm/vcpu_sbi_fwft.c | 136 +++++++++++++++++++++
8 files changed, 190 insertions(+)
create mode 100644 arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
create mode 100644 arch/riscv/kvm/vcpu_sbi_fwft.c
diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 484d04a92fa6..be60aaa07f57 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -19,6 +19,7 @@
#include <asm/kvm_vcpu_fp.h>
#include <asm/kvm_vcpu_insn.h>
#include <asm/kvm_vcpu_sbi.h>
+#include <asm/kvm_vcpu_sbi_fwft.h>
#include <asm/kvm_vcpu_timer.h>
#include <asm/kvm_vcpu_pmu.h>
@@ -169,6 +170,7 @@ struct kvm_vcpu_csr {
struct kvm_vcpu_config {
u64 henvcfg;
u64 hstateen0;
+ u64 hedeleg;
};
struct kvm_vcpu_smstateen_csr {
@@ -261,6 +263,9 @@ struct kvm_vcpu_arch {
/* Performance monitoring context */
struct kvm_pmu pmu_context;
+ /* Firmware feature SBI extension context */
+ struct kvm_sbi_fwft fwft_context;
+
/* 'static' configurations which are set only once */
struct kvm_vcpu_config cfg;
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index b96705258cf9..3a33bbacc233 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -86,6 +86,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta;
+extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft;
extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
new file mode 100644
index 000000000000..7dc1b80c7e6c
--- /dev/null
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Rivos Inc
+ *
+ * Authors:
+ * Atish Patra <[email protected]>
+ */
+
+#ifndef __KVM_VCPU_RISCV_FWFT_H
+#define __KVM_VCPU_RISCV_FWFT_H
+
+#include <asm/sbi.h>
+
+#define KVM_SBI_FWFT_FEATURE_COUNT 1
+
+struct kvm_sbi_fwft_config;
+struct kvm_vcpu;
+
+struct kvm_sbi_fwft_feature {
+ enum sbi_fwft_feature_t id;
+ int (*set)(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long value);
+ int (*get)(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long *value);
+};
+
+struct kvm_sbi_fwft_config {
+ const struct kvm_sbi_fwft_feature *feature;
+ unsigned long flags;
+};
+
+/* FWFT data structure per vcpu */
+struct kvm_sbi_fwft {
+ struct kvm_sbi_fwft_config configs[KVM_SBI_FWFT_FEATURE_COUNT];
+};
+
+#define vcpu_to_fwft(vcpu) (&(vcpu)->arch.fwft_context)
+
+#endif /* !__KVM_VCPU_RISCV_FWFT_H */
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index 7499e88a947c..fa3097da91c0 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -185,6 +185,7 @@ enum KVM_RISCV_SBI_EXT_ID {
KVM_RISCV_SBI_EXT_VENDOR,
KVM_RISCV_SBI_EXT_DBCN,
KVM_RISCV_SBI_EXT_STA,
+ KVM_RISCV_SBI_EXT_FWFT,
KVM_RISCV_SBI_EXT_MAX,
};
diff --git a/arch/riscv/kvm/Makefile b/arch/riscv/kvm/Makefile
index c9646521f113..19175bd5b40a 100644
--- a/arch/riscv/kvm/Makefile
+++ b/arch/riscv/kvm/Makefile
@@ -27,6 +27,7 @@ kvm-y += vcpu_sbi_base.o
kvm-y += vcpu_sbi_replace.o
kvm-y += vcpu_sbi_hsm.o
kvm-y += vcpu_sbi_sta.o
+kvm-y += vcpu_sbi_fwft.o
kvm-y += vcpu_timer.o
kvm-$(CONFIG_RISCV_PMU_SBI) += vcpu_pmu.o vcpu_sbi_pmu.o
kvm-y += aia.o
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index b5ca9f2e98ac..461ef60d4eda 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -505,6 +505,8 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
if (riscv_isa_extension_available(isa, SMSTATEEN))
cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
}
+
+ cfg->hedeleg = csr_read(CSR_HEDELEG);
}
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
@@ -521,6 +523,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
csr_write(CSR_VSTVAL, csr->vstval);
csr_write(CSR_HVIP, csr->hvip);
csr_write(CSR_VSATP, csr->vsatp);
+ csr_write(CSR_HEDELEG, cfg->hedeleg);
csr_write(CSR_HENVCFG, cfg->henvcfg);
if (IS_ENABLED(CONFIG_32BIT))
csr_write(CSR_HENVCFGH, cfg->henvcfg >> 32);
@@ -551,6 +554,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
{
struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
+ struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
vcpu->cpu = -1;
@@ -574,6 +578,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
csr->vstval = csr_read(CSR_VSTVAL);
csr->hvip = csr_read(CSR_HVIP);
csr->vsatp = csr_read(CSR_VSATP);
+ cfg->hedeleg = csr_read(CSR_HEDELEG);
}
static void kvm_riscv_check_vcpu_requests(struct kvm_vcpu *vcpu)
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index 72a2ffb8dcd1..76901f0f34b7 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -74,6 +74,10 @@ static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = {
.ext_idx = KVM_RISCV_SBI_EXT_STA,
.ext_ptr = &vcpu_sbi_ext_sta,
},
+ {
+ .ext_idx = KVM_RISCV_SBI_EXT_FWFT,
+ .ext_ptr = &vcpu_sbi_ext_fwft,
+ },
{
.ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
.ext_ptr = &vcpu_sbi_ext_experimental,
diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
new file mode 100644
index 000000000000..b9b7f8fa6d22
--- /dev/null
+++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 Western Digital Corporation or its affiliates.
+ *
+ * Authors:
+ * Atish Patra <[email protected]>
+ */
+
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/kvm_host.h>
+#include <asm/sbi.h>
+#include <asm/kvm_vcpu_sbi.h>
+#include <asm/kvm_vcpu_sbi_fwft.h>
+
+#define MIS_DELEG (1UL << EXC_LOAD_MISALIGNED | 1UL << EXC_STORE_MISALIGNED)
+
+static int kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu,
+ struct kvm_sbi_fwft_config *conf,
+ unsigned long value)
+{
+ if (value)
+ csr_set(CSR_HEDELEG, MIS_DELEG);
+ else
+ csr_clear(CSR_HEDELEG, MIS_DELEG);
+
+ return SBI_SUCCESS;
+}
+
+static int kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu,
+ struct kvm_sbi_fwft_config *conf,
+ unsigned long *value)
+{
+ *value = (csr_read(CSR_HEDELEG) & MIS_DELEG) != 0;
+
+ return SBI_SUCCESS;
+}
+
+static struct kvm_sbi_fwft_config *
+kvm_sbi_fwft_get_config(struct kvm_vcpu *vcpu, enum sbi_fwft_feature_t feature)
+{
+ int i = 0;
+ struct kvm_sbi_fwft *fwft = vcpu_to_fwft(vcpu);
+
+ for (i = 0; i < KVM_SBI_FWFT_FEATURE_COUNT; i++) {
+ if (fwft->configs[i].feature->id == feature)
+ return &fwft->configs[i];
+ }
+
+ return NULL;
+}
+
+static int kvm_sbi_fwft_set(struct kvm_vcpu *vcpu,
+ enum sbi_fwft_feature_t feature,
+ unsigned long value, unsigned long flags)
+{
+ struct kvm_sbi_fwft_config *conf = kvm_sbi_fwft_get_config(vcpu,
+ feature);
+ if (!conf)
+ return SBI_ERR_DENIED;
+
+ if ((flags & ~SBI_FWFT_SET_FLAG_LOCK) != 0)
+ return SBI_ERR_INVALID_PARAM;
+
+ if (conf->flags & SBI_FWFT_SET_FLAG_LOCK)
+ return SBI_ERR_DENIED;
+
+ conf->flags = flags;
+
+ return conf->feature->set(vcpu, conf, value);
+}
+
+static int kvm_sbi_fwft_get(struct kvm_vcpu *vcpu,
+ enum sbi_fwft_feature_t feature,
+ unsigned long *value)
+{
+ struct kvm_sbi_fwft_config *conf = kvm_sbi_fwft_get_config(vcpu,
+ feature);
+ if (!conf)
+ return SBI_ERR_DENIED;
+
+ return conf->feature->get(vcpu, conf, value);
+}
+
+static int kvm_sbi_ext_fwft_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
+ struct kvm_vcpu_sbi_return *retdata)
+{
+ int ret = 0;
+ struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
+ unsigned long funcid = cp->a6;
+
+ switch (funcid) {
+ case SBI_EXT_FWFT_SET:
+ ret = kvm_sbi_fwft_set(vcpu, cp->a0, cp->a1, cp->a2);
+ break;
+ case SBI_EXT_FWFT_GET:
+ ret = kvm_sbi_fwft_get(vcpu, cp->a0, &retdata->out_val);
+ break;
+ default:
+ ret = SBI_ERR_NOT_SUPPORTED;
+ break;
+ }
+
+ retdata->err_val = ret;
+
+ return 0;
+}
+
+static const struct kvm_sbi_fwft_feature features[] = {
+ {
+ .id = SBI_FWFT_MISALIGNED_DELEG,
+ .set = kvm_sbi_fwft_set_misaligned_delegation,
+ .get = kvm_sbi_fwft_get_misaligned_delegation,
+ }
+};
+
+static_assert(ARRAY_SIZE(features) == KVM_SBI_FWFT_FEATURE_COUNT);
+
+
+static unsigned long kvm_sbi_ext_fwft_probe(struct kvm_vcpu *vcpu)
+{
+ struct kvm_sbi_fwft *fwft = vcpu_to_fwft(vcpu);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(features); i++)
+ fwft->configs[i].feature = &features[i];
+
+ return 1;
+}
+
+const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = {
+ .extid_start = SBI_EXT_FWFT,
+ .extid_end = SBI_EXT_FWFT,
+ .handler = kvm_sbi_ext_fwft_handler,
+ .probe = kvm_sbi_ext_fwft_probe,
+};
--
2.43.0
Add description for the Ssdbltrp ISA extension which is not yet
ratified.
Signed-off-by: Clément Léger <[email protected]>
---
Documentation/devicetree/bindings/riscv/extensions.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 63d81dc895e5..ce7021dbb556 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -147,6 +147,12 @@ properties:
and mode-based filtering as ratified at commit 01d1df0 ("Add ability
to manually trigger workflow. (#2)") of riscv-count-overflow.
+ - const: ssdbltrp
+ description: |
+ The standard Ssdbltrp supervisor-level extension for double trap
+ handling as currently defined by commit e85847b ("Merge pull request
+ #32 from ved-rivos/0415_1 ") of riscv-double-trap.
+
- const: sstc
description: |
The standard Sstc supervisor-level extension for time compare as
--
2.43.0
When Ssdbltrp is enabled, we must take care of clearing SDT after
sensitive phases are over to avoid generating a double trap. This
is mainly about exceptions handling so clear SDT once we have
saved enough information (critical CSRs) but also clears it during SATP
mode detection which generates an "inline" trap and thus sets SDT
implicitely.
Signed-off-by: Clément Léger <[email protected]>
---
arch/riscv/include/asm/csr.h | 1 +
arch/riscv/kernel/entry.S | 52 ++++++++++++++++++++---------------
arch/riscv/kernel/head.S | 4 +++
arch/riscv/kernel/sse_entry.S | 4 +--
4 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 5528159b3d5d..905cdf894a57 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -17,6 +17,7 @@
#define SR_SPP _AC(0x00000100, UL) /* Previously Supervisor */
#define SR_MPP _AC(0x00001800, UL) /* Previously Machine */
#define SR_SUM _AC(0x00040000, UL) /* Supervisor User Memory Access */
+#define SR_SDT _AC(0x01000000, UL) /* Supervisor Double Trap */
#define SR_FS _AC(0x00006000, UL) /* Floating-point Status */
#define SR_FS_OFF _AC(0x00000000, UL)
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 1591e0781569..07da91080839 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -49,23 +49,12 @@ SYM_CODE_START(handle_exception)
REG_S x5, PT_T0(sp)
save_from_x6_to_x31
- /*
- * Disable user-mode memory access as it should only be set in the
- * actual user copy routines.
- *
- * Disable the FPU/Vector to detect illegal usage of floating point
- * or vector in kernel space.
- */
- li t0, SR_SUM | SR_FS_VS
-
REG_L s0, TASK_TI_USER_SP(tp)
- csrrc s1, CSR_STATUS, t0
csrr s2, CSR_EPC
csrr s3, CSR_TVAL
csrr s4, CSR_CAUSE
csrr s5, CSR_SCRATCH
REG_S s0, PT_SP(sp)
- REG_S s1, PT_STATUS(sp)
REG_S s2, PT_EPC(sp)
REG_S s3, PT_BADADDR(sp)
REG_S s4, PT_CAUSE(sp)
@@ -77,6 +66,21 @@ SYM_CODE_START(handle_exception)
*/
csrw CSR_SCRATCH, x0
+ /*
+ * Disable user-mode memory access as it should only be set in the
+ * actual user copy routines.
+ *
+ * Disable the FPU/Vector to detect illegal usage of floating point
+ * or vector in kernel space.
+ *
+ * Clear supervisor double trap bit as all trap context is saved and we
+ * can handle another one
+ */
+ li t0, SR_SUM | SR_FS_VS | SR_SDT
+
+ csrrc s1, CSR_STATUS, t0
+ REG_S s1, PT_STATUS(sp)
+
/* Load the global pointer */
load_global_pointer
@@ -123,15 +127,25 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
#ifdef CONFIG_RISCV_M_MODE
/* the MPP value is too large to be used as an immediate arg for addi */
li t0, SR_MPP
- and s0, s0, t0
+ and t1, s0, t0
#else
- andi s0, s0, SR_SPP
+ andi t1, s0, SR_SPP
+#endif
+
+#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE
+ move a0, sp
+ call riscv_v_context_nesting_end
#endif
- bnez s0, 1f
+ /*
+ * Restore STATUS now to set supervisor double trap bit which means that
+ * from now on, we can not handle an exception up to "sret"
+ */
+ csrw CSR_STATUS, s0
+ bnez t1, 1f
/* Save unwound kernel stack pointer in thread_info */
- addi s0, sp, PT_SIZE_ON_STACK
- REG_S s0, TASK_TI_KERNEL_SP(tp)
+ addi t1, sp, PT_SIZE_ON_STACK
+ REG_S t1, TASK_TI_KERNEL_SP(tp)
/* Save the kernel shadow call stack pointer */
scs_save_current
@@ -142,11 +156,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
*/
csrw CSR_SCRATCH, tp
1:
-#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE
- move a0, sp
- call riscv_v_context_nesting_end
-#endif
- REG_L a0, PT_STATUS(sp)
/*
* The current load reservation is effectively part of the processor's
* state, in the sense that load reservations cannot be shared between
@@ -167,7 +176,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
REG_L a2, PT_EPC(sp)
REG_SC x0, a2, PT_EPC(sp)
- csrw CSR_STATUS, a0
csrw CSR_EPC, a2
REG_L x1, PT_RA(sp)
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 4236a69c35cb..bcc2b6678f40 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -106,6 +106,10 @@ relocate_enable_mmu:
csrw CSR_SATP, a0
.align 2
1:
+ /* A trap potentially set the SDT flag, clear it */
+ li t0, SR_SDT
+ csrc CSR_STATUS, t0
+
/* Set trap vector to spin forever to help debug */
la a0, .Lsecondary_park
csrw CSR_TVEC, a0
diff --git a/arch/riscv/kernel/sse_entry.S b/arch/riscv/kernel/sse_entry.S
index d3c7286f3372..e69d386e36e9 100644
--- a/arch/riscv/kernel/sse_entry.S
+++ b/arch/riscv/kernel/sse_entry.S
@@ -65,7 +65,7 @@ SYM_CODE_START(handle_sse)
REG_S a4, PT_SP(sp)
/* Disable user memory access and floating/vector computing */
- li t0, SR_SUM | SR_FS_VS
+ li t0, SR_SUM | SR_FS_VS | SR_SDT
csrc CSR_STATUS, t0
load_global_pointer
@@ -131,8 +131,8 @@ SYM_CODE_START(handle_sse)
SYM_INNER_LABEL(ret_from_sse, SYM_L_GLOBAL)
/* Restore saved CSRs */
- csrw CSR_SSCRATCH, s4
csrw CSR_SSTATUS, s5
+ csrw CSR_SSCRATCH, s4
#ifdef CONFIG_FRAME_POINTER
/* Frame pointer is created only when kernel is interrupted */
--
2.43.0
Add a small driver to request double trap enabling as well as
registering a SSE handler for double trap. This will also be used by KVM
SBI FWFT extension support to detect if it is possible to enable double
trap in VS-mode.
Signed-off-by: Clément Léger <[email protected]>
---
arch/riscv/include/asm/sbi.h | 1 +
drivers/firmware/Kconfig | 7 +++
drivers/firmware/Makefile | 1 +
drivers/firmware/riscv_dbltrp.c | 95 +++++++++++++++++++++++++++++++++
include/linux/riscv_dbltrp.h | 19 +++++++
5 files changed, 123 insertions(+)
create mode 100644 drivers/firmware/riscv_dbltrp.c
create mode 100644 include/linux/riscv_dbltrp.h
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 744aa1796c92..9cd4ca66487c 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -314,6 +314,7 @@ enum sbi_sse_attr_id {
#define SBI_SSE_ATTR_INTERRUPTED_FLAGS_SPIE (1 << 2)
#define SBI_SSE_EVENT_LOCAL_RAS 0x00000000
+#define SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP 0x00000001
#define SBI_SSE_EVENT_GLOBAL_RAS 0x00008000
#define SBI_SSE_EVENT_LOCAL_PMU 0x00010000
#define SBI_SSE_EVENT_LOCAL_SOFTWARE 0xffff0000
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 59f611288807..a037f6e89942 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -197,6 +197,13 @@ config RISCV_SSE_TEST
Select if you want to enable SSE extension testing at boot time.
This will run a series of test which verifies SSE sanity.
+config RISCV_DBLTRP
+ bool "Enable Double trap handling"
+ depends on RISCV_SSE && RISCV_SBI
+ default n
+ help
+ Select if you want to enable SSE double trap handler.
+
config SYSFB
bool
select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index fb7b0c08c56d..ad67a1738c0f 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
obj-$(CONFIG_RISCV_SSE) += riscv_sse.o
obj-$(CONFIG_RISCV_SSE_TEST) += riscv_sse_test.o
+obj-$(CONFIG_RISCV_DBLTRP) += riscv_dbltrp.o
obj-$(CONFIG_SYSFB) += sysfb.o
obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
diff --git a/drivers/firmware/riscv_dbltrp.c b/drivers/firmware/riscv_dbltrp.c
new file mode 100644
index 000000000000..72f9a067e87a
--- /dev/null
+++ b/drivers/firmware/riscv_dbltrp.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+#define pr_fmt(fmt) "riscv-dbltrp: " fmt
+
+#include <linux/cpu.h>
+#include <linux/init.h>
+#include <linux/riscv_dbltrp.h>
+#include <linux/riscv_sse.h>
+
+#include <asm/sbi.h>
+
+static bool double_trap_enabled;
+
+static int riscv_sse_dbltrp_handle(uint32_t evt, void *arg,
+ struct pt_regs *regs)
+{
+ __show_regs(regs);
+ panic("Double trap !\n");
+
+ return 0;
+}
+
+struct cpu_dbltrp_data {
+ int error;
+};
+
+static void
+sbi_cpu_enable_double_trap(void *data)
+{
+ struct sbiret ret;
+ struct cpu_dbltrp_data *cdd = data;
+
+ ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET,
+ SBI_FWFT_DOUBLE_TRAP_ENABLE, 1, 0, 0, 0, 0);
+
+ if (ret.error) {
+ cdd->error = 1;
+ pr_err("Failed to enable double trap on cpu %d\n", smp_processor_id());
+ }
+}
+
+static int sbi_enable_double_trap(void)
+{
+ struct cpu_dbltrp_data cdd = {0};
+
+ on_each_cpu(sbi_cpu_enable_double_trap, &cdd, 1);
+ if (cdd.error)
+ return -1;
+
+ double_trap_enabled = true;
+
+ return 0;
+}
+
+bool riscv_double_trap_enabled(void)
+{
+ return double_trap_enabled;
+}
+EXPORT_SYMBOL(riscv_double_trap_enabled);
+
+static int __init riscv_dbltrp(void)
+{
+ struct sse_event *evt;
+
+ if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSDBLTRP)) {
+ pr_err("Ssdbltrp extension not available\n");
+ return 1;
+ }
+
+ if (!sbi_probe_extension(SBI_EXT_FWFT)) {
+ pr_err("Can not enable double trap, SBI_EXT_FWFT is not available\n");
+ return 1;
+ }
+
+ if (sbi_enable_double_trap()) {
+ pr_err("Failed to enable double trap on all cpus\n");
+ return 1;
+ }
+
+ evt = sse_event_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, 0,
+ riscv_sse_dbltrp_handle, NULL);
+ if (IS_ERR(evt)) {
+ pr_err("SSE double trap register failed\n");
+ return PTR_ERR(evt);
+ }
+
+ sse_event_enable(evt);
+ pr_info("Double trap handling registered\n");
+
+ return 0;
+}
+device_initcall(riscv_dbltrp);
diff --git a/include/linux/riscv_dbltrp.h b/include/linux/riscv_dbltrp.h
new file mode 100644
index 000000000000..6de4f43fae6b
--- /dev/null
+++ b/include/linux/riscv_dbltrp.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+#ifndef __LINUX_RISCV_DBLTRP_H
+#define __LINUX_RISCV_DBLTRP_H
+
+#if defined(CONFIG_RISCV_DBLTRP)
+bool riscv_double_trap_enabled(void);
+#else
+
+static inline bool riscv_double_trap_enabled(void)
+{
+ return false;
+}
+#endif
+
+#endif /* __LINUX_RISCV_DBLTRP_H */
--
2.43.0
Handle Ssdbltrp extension at isa parsing level.
Signed-off-by: Clément Léger <[email protected]>
---
arch/riscv/include/asm/csr.h | 1 +
arch/riscv/include/asm/hwcap.h | 1 +
arch/riscv/kernel/cpufeature.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 510014051f5d..5528159b3d5d 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -102,6 +102,7 @@
#define EXC_INST_PAGE_FAULT 12
#define EXC_LOAD_PAGE_FAULT 13
#define EXC_STORE_PAGE_FAULT 15
+#define EXC_DOUBLE_TRAP 16
#define EXC_INST_GUEST_PAGE_FAULT 20
#define EXC_LOAD_GUEST_PAGE_FAULT 21
#define EXC_VIRTUAL_INST_FAULT 22
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 5340f818746b..16d2ad7ca9b2 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -80,6 +80,7 @@
#define RISCV_ISA_EXT_ZFA 71
#define RISCV_ISA_EXT_ZTSO 72
#define RISCV_ISA_EXT_ZACAS 73
+#define RISCV_ISA_EXT_SSDBLTRP 74
#define RISCV_ISA_EXT_MAX 128
#define RISCV_ISA_EXT_INVALID U32_MAX
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 89920f84d0a3..5cff21adf2c4 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
+ __RISCV_ISA_EXT_DATA(ssdbltrp, RISCV_ISA_EXT_SSDBLTRP),
__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
--
2.43.0
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 <[email protected]>
---
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;
+ }
+
/* 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
Add support in KVM SBI FWFT extension to allow VS-mode to request double
trap enabling. Double traps can then be generated by VS-mode, allowing
M-mode to redirect them to S-mode.
Signed-off-by: Clément Léger <[email protected]>
---
arch/riscv/include/asm/csr.h | 1 +
arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 2 +-
arch/riscv/kvm/vcpu_sbi_fwft.c | 41 ++++++++++++++++++++++
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 905cdf894a57..ee1b73655bec 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -196,6 +196,7 @@
/* xENVCFG flags */
#define ENVCFG_STCE (_AC(1, ULL) << 63)
#define ENVCFG_PBMTE (_AC(1, ULL) << 62)
+#define ENVCFG_DTE (_AC(1, ULL) << 59)
#define ENVCFG_CBZE (_AC(1, UL) << 7)
#define ENVCFG_CBCFE (_AC(1, UL) << 6)
#define ENVCFG_CBIE_SHIFT 4
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
index 7dc1b80c7e6c..a9e20d655126 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
@@ -11,7 +11,7 @@
#include <asm/sbi.h>
-#define KVM_SBI_FWFT_FEATURE_COUNT 1
+#define KVM_SBI_FWFT_FEATURE_COUNT 2
struct kvm_sbi_fwft_config;
struct kvm_vcpu;
diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
index b9b7f8fa6d22..9e8e397eb02f 100644
--- a/arch/riscv/kvm/vcpu_sbi_fwft.c
+++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
@@ -9,10 +9,19 @@
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/kvm_host.h>
+#include <linux/riscv_dbltrp.h>
#include <asm/sbi.h>
#include <asm/kvm_vcpu_sbi.h>
#include <asm/kvm_vcpu_sbi_fwft.h>
+#ifdef CONFIG_32BIT
+# define CSR_HENVCFG_DBLTRP CSR_HENVCFGH
+# define DBLTRP_DTE (ENVCFG_DTE >> 32)
+#else
+# define CSR_HENVCFG_DBLTRP CSR_HENVCFG
+# define DBLTRP_DTE ENVCFG_DTE
+#endif
+
#define MIS_DELEG (1UL << EXC_LOAD_MISALIGNED | 1UL << EXC_STORE_MISALIGNED)
static int kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu,
@@ -36,6 +45,33 @@ static int kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu,
return SBI_SUCCESS;
}
+static int kvm_sbi_fwft_set_double_trap(struct kvm_vcpu *vcpu,
+ struct kvm_sbi_fwft_config *conf,
+ unsigned long value)
+{
+ if (!riscv_double_trap_enabled())
+ return SBI_ERR_NOT_SUPPORTED;
+
+ if (value)
+ csr_set(CSR_HENVCFG_DBLTRP, DBLTRP_DTE);
+ else
+ csr_clear(CSR_HENVCFG_DBLTRP, DBLTRP_DTE);
+
+ return SBI_SUCCESS;
+}
+
+static int kvm_sbi_fwft_get_double_trap(struct kvm_vcpu *vcpu,
+ struct kvm_sbi_fwft_config *conf,
+ unsigned long *value)
+{
+ if (!riscv_double_trap_enabled())
+ return SBI_ERR_NOT_SUPPORTED;
+
+ *value = (csr_read(CSR_HENVCFG_DBLTRP) & DBLTRP_DTE) != 0;
+
+ return SBI_SUCCESS;
+}
+
static struct kvm_sbi_fwft_config *
kvm_sbi_fwft_get_config(struct kvm_vcpu *vcpu, enum sbi_fwft_feature_t feature)
{
@@ -111,6 +147,11 @@ static const struct kvm_sbi_fwft_feature features[] = {
.id = SBI_FWFT_MISALIGNED_DELEG,
.set = kvm_sbi_fwft_set_misaligned_delegation,
.get = kvm_sbi_fwft_get_misaligned_delegation,
+ },
+ {
+ .id = SBI_FWFT_DOUBLE_TRAP_ENABLE,
+ .set = kvm_sbi_fwft_set_double_trap,
+ .get = kvm_sbi_fwft_get_double_trap,
}
};
--
2.43.0
On Thu, Apr 18, 2024 at 04:26:41PM +0200, Cl?ment L?ger wrote:
> Add description for the Ssdbltrp ISA extension which is not yet
> ratified.
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> Documentation/devicetree/bindings/riscv/extensions.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 63d81dc895e5..ce7021dbb556 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -147,6 +147,12 @@ properties:
> and mode-based filtering as ratified at commit 01d1df0 ("Add ability
> to manually trigger workflow. (#2)") of riscv-count-overflow.
>
> + - const: ssdbltrp
> + description: |
> + The standard Ssdbltrp supervisor-level extension for double trap
> + handling as currently defined by commit e85847b ("Merge pull request
> + #32 from ved-rivos/0415_1 ") of riscv-double-trap.
I see the proposed ratification for this is Sept 2024, and is marked as
"Freeze Approved". Do you know when it is going to be frozen? Until
this, I can't ack this patch. I had a look in the RVI JIRA
https://jira.riscv.org/browse/RVS-2291?src=confmacro
and it looks imminent, but it's unclear to me whether it actually has
been or not.
> +
> - const: sstc
> description: |
> The standard Sstc supervisor-level extension for time compare as
> --
> 2.43.0
>
On Thu, Apr 18, 2024 at 04:26:39PM +0200, Cl?ment L?ger wrote:
> drivers/firmware/Kconfig | 7 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/riscv_dbltrp.c | 95 ++++++++++
From what we discussed with Arnd back when we started routing the dts
and soc driver stuff via the soc tree, riscv stuff in drivers/firmware
supposedly also falls under my remit, and gets merged via the soc tree,
although I would expect that this and the initial SSE support series
would go via the riscv tree since they touch a lot more than just
drivers/firmware.
Could you create a drivers/firmware/riscv directory for this dbltrp file
and whichever of this or the SSE patch that lands first add a maintainers
entry for drivers/firmware/riscv that links to my git tree?
It probably also makes sense to create per-driver entries for each of
the dbltrp and sse drivers that ensures you get CCed on any patches
for them.
On Thu, Apr 18, 2024 at 04:26:44PM +0200, Cl?ment L?ger wrote:
> Add a small driver to request double trap enabling as well as
> registering a SSE handler for double trap. This will also be used by KVM
> SBI FWFT extension support to detect if it is possible to enable double
> trap in VS-mode.
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> arch/riscv/include/asm/sbi.h | 1 +
> drivers/firmware/Kconfig | 7 +++
> drivers/firmware/Makefile | 1 +
> drivers/firmware/riscv_dbltrp.c | 95 +++++++++++++++++++++++++++++++++
> include/linux/riscv_dbltrp.h | 19 +++++++
> 5 files changed, 123 insertions(+)
> create mode 100644 drivers/firmware/riscv_dbltrp.c
> create mode 100644 include/linux/riscv_dbltrp.h
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 744aa1796c92..9cd4ca66487c 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -314,6 +314,7 @@ enum sbi_sse_attr_id {
> #define SBI_SSE_ATTR_INTERRUPTED_FLAGS_SPIE (1 << 2)
>
> #define SBI_SSE_EVENT_LOCAL_RAS 0x00000000
> +#define SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP 0x00000001
> #define SBI_SSE_EVENT_GLOBAL_RAS 0x00008000
> #define SBI_SSE_EVENT_LOCAL_PMU 0x00010000
> #define SBI_SSE_EVENT_LOCAL_SOFTWARE 0xffff0000
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 59f611288807..a037f6e89942 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -197,6 +197,13 @@ config RISCV_SSE_TEST
> Select if you want to enable SSE extension testing at boot time.
> This will run a series of test which verifies SSE sanity.
>
> +config RISCV_DBLTRP
> + bool "Enable Double trap handling"
> + depends on RISCV_SSE && RISCV_SBI
> + default n
> + help
> + Select if you want to enable SSE double trap handler.
> +
> config SYSFB
> bool
> select BOOT_VESA_SUPPORT
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index fb7b0c08c56d..ad67a1738c0f 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
> obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
> obj-$(CONFIG_RISCV_SSE) += riscv_sse.o
> obj-$(CONFIG_RISCV_SSE_TEST) += riscv_sse_test.o
> +obj-$(CONFIG_RISCV_DBLTRP) += riscv_dbltrp.o
As previously mentioned, I'd like to see all of these riscv specific
things in a riscv directory.
> obj-$(CONFIG_SYSFB) += sysfb.o
> obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
> obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
> diff --git a/drivers/firmware/riscv_dbltrp.c b/drivers/firmware/riscv_dbltrp.c
> new file mode 100644
> index 000000000000..72f9a067e87a
> --- /dev/null
> +++ b/drivers/firmware/riscv_dbltrp.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Rivos Inc.
> + */
> +
> +#define pr_fmt(fmt) "riscv-dbltrp: " fmt
> +
> +#include <linux/cpu.h>
> +#include <linux/init.h>
> +#include <linux/riscv_dbltrp.h>
> +#include <linux/riscv_sse.h>
> +
> +#include <asm/sbi.h>
> +
> +static bool double_trap_enabled;
> +
> +static int riscv_sse_dbltrp_handle(uint32_t evt, void *arg,
> + struct pt_regs *regs)
> +{
> + __show_regs(regs);
> + panic("Double trap !\n");
> +
> + return 0;
> +}
> +
> +struct cpu_dbltrp_data {
> + int error;
> +};
> +
> +static void
> +sbi_cpu_enable_double_trap(void *data)
This should easily fit on one line.
> +{
> + struct sbiret ret;
> + struct cpu_dbltrp_data *cdd = data;
> +
> + ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET,
> + SBI_FWFT_DOUBLE_TRAP_ENABLE, 1, 0, 0, 0, 0);
> +
> + if (ret.error) {
> + cdd->error = 1;
If this is a boolean, make it a boolean please. All the code in this
patch treats it as one.
> + pr_err("Failed to enable double trap on cpu %d\n", smp_processor_id());
> + }
> +}
> +
> +static int sbi_enable_double_trap(void)
> +{
> + struct cpu_dbltrp_data cdd = {0};
> +
> + on_each_cpu(sbi_cpu_enable_double_trap, &cdd, 1);
> + if (cdd.error)
> + return -1;
Can this be an errno please?
> +
> + double_trap_enabled = true;
> +
> + return 0;
> +}
> +
> +bool riscv_double_trap_enabled(void)
> +{
> + return double_trap_enabled;
> +}
> +EXPORT_SYMBOL(riscv_double_trap_enabled);
Can we just use double_trap everywhere? dbltrp reads like sound that a
beatboxer would make and this looks a lot nicer than the other functions
in the file...
> +
> +static int __init riscv_dbltrp(void)
I think this function is missing an action work - init or probe?
> +{
> + struct sse_event *evt;
> +
> + if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSDBLTRP)) {
> + pr_err("Ssdbltrp extension not available\n");
> + return 1;
> + }
> +
> + if (!sbi_probe_extension(SBI_EXT_FWFT)) {
> + pr_err("Can not enable double trap, SBI_EXT_FWFT is not available\n");
> + return 1;
> + }
> +
> + if (sbi_enable_double_trap()) {
> + pr_err("Failed to enable double trap on all cpus\n");
> + return 1;
Why do we return 1s here, but an errno via PTR_ERR() below?
Shouldn't all of these be returning a negative errono?
This particular one should probably propagate the error it got from
sbi_enable_double_trap().
Cheers,
Conor.
> + }
> +
> + evt = sse_event_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, 0,
> + riscv_sse_dbltrp_handle, NULL);
> + if (IS_ERR(evt)) {
> + pr_err("SSE double trap register failed\n");
> + return PTR_ERR(evt);
> + }
> +
> + sse_event_enable(evt);
> + pr_info("Double trap handling registered\n");
> +
> + return 0;
> +}
> +device_initcall(riscv_dbltrp);
> diff --git a/include/linux/riscv_dbltrp.h b/include/linux/riscv_dbltrp.h
> new file mode 100644
> index 000000000000..6de4f43fae6b
> --- /dev/null
> +++ b/include/linux/riscv_dbltrp.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 Rivos Inc.
> + */
> +
> +#ifndef __LINUX_RISCV_DBLTRP_H
> +#define __LINUX_RISCV_DBLTRP_H
> +
> +#if defined(CONFIG_RISCV_DBLTRP)
> +bool riscv_double_trap_enabled(void);
> +#else
> +
> +static inline bool riscv_double_trap_enabled(void)
> +{
> + return false;
> +}
> +#endif
> +
> +#endif /* __LINUX_RISCV_DBLTRP_H */
> --
> 2.43.0
>
On 23/04/2024 18:24, Conor Dooley wrote:
> On Thu, Apr 18, 2024 at 04:26:39PM +0200, Clément Léger wrote:
>
>> drivers/firmware/Kconfig | 7 +
>> drivers/firmware/Makefile | 1 +
>> drivers/firmware/riscv_dbltrp.c | 95 ++++++++++
>
> From what we discussed with Arnd back when we started routing the dts
> and soc driver stuff via the soc tree, riscv stuff in drivers/firmware
> supposedly also falls under my remit, and gets merged via the soc tree,
> although I would expect that this and the initial SSE support series
> would go via the riscv tree since they touch a lot more than just
> drivers/firmware.
Yes indeed.
>
> Could you create a drivers/firmware/riscv directory for this dbltrp file
> and whichever of this or the SSE patch that lands first add a maintainers
> entry for drivers/firmware/riscv that links to my git tree?
Yes sure, I'll modify this one as well as the SSE series.
> It probably also makes sense to create per-driver entries for each of
> the dbltrp and sse drivers that ensures you get CCed on any patches
> for them.
>
Indeed, missed that.
Thanks,
Clément
On 23/04/2024 18:30, Conor Dooley wrote:
> On Thu, Apr 18, 2024 at 04:26:41PM +0200, Clément Léger wrote:
>> Add description for the Ssdbltrp ISA extension which is not yet
>> ratified.
>>
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> Documentation/devicetree/bindings/riscv/extensions.yaml | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
>> index 63d81dc895e5..ce7021dbb556 100644
>> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
>> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
>> @@ -147,6 +147,12 @@ properties:
>> and mode-based filtering as ratified at commit 01d1df0 ("Add ability
>> to manually trigger workflow. (#2)") of riscv-count-overflow.
>>
>> + - const: ssdbltrp
>> + description: |
>> + The standard Ssdbltrp supervisor-level extension for double trap
>> + handling as currently defined by commit e85847b ("Merge pull request
>> + #32 from ved-rivos/0415_1 ") of riscv-double-trap.
>
> I see the proposed ratification for this is Sept 2024, and is marked as
> "Freeze Approved". Do you know when it is going to be frozen? Until
> this, I can't ack this patch. I had a look in the RVI JIRA
> https://jira.riscv.org/browse/RVS-2291?src=confmacro
> and it looks imminent, but it's unclear to me whether it actually has
> been or not.
Hi Conor,
Yeah, this series is a RFC since the spec is not yet ratified nor frozen
and its purpose is actually to get to a frozen state. As to when this
will be ratified, I guess Ved can probably answer that.
Clément
>
>> +
>> - const: sstc
>> description: |
>> The standard Sstc supervisor-level extension for time compare as
>> --
>> 2.43.0
>>
On Wed, Apr 24, 2024 at 09:20:35AM +0200, Cl?ment L?ger wrote:
>
>
> On 23/04/2024 18:30, Conor Dooley wrote:
> > On Thu, Apr 18, 2024 at 04:26:41PM +0200, Cl?ment L?ger wrote:
> >> Add description for the Ssdbltrp ISA extension which is not yet
> >> ratified.
> >>
> >> Signed-off-by: Cl?ment L?ger <[email protected]>
> >> ---
> >> Documentation/devicetree/bindings/riscv/extensions.yaml | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> >> index 63d81dc895e5..ce7021dbb556 100644
> >> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> >> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> >> @@ -147,6 +147,12 @@ properties:
> >> and mode-based filtering as ratified at commit 01d1df0 ("Add ability
> >> to manually trigger workflow. (#2)") of riscv-count-overflow.
> >>
> >> + - const: ssdbltrp
> >> + description: |
> >> + The standard Ssdbltrp supervisor-level extension for double trap
> >> + handling as currently defined by commit e85847b ("Merge pull request
> >> + #32 from ved-rivos/0415_1 ") of riscv-double-trap.
> >
> > I see the proposed ratification for this is Sept 2024, and is marked as
> > "Freeze Approved". Do you know when it is going to be frozen? Until
> > this, I can't ack this patch. I had a look in the RVI JIRA
> > https://jira.riscv.org/browse/RVS-2291?src=confmacro
> > and it looks imminent, but it's unclear to me whether it actually has
> > been or not.
>
> Hi Conor,
>
> Yeah, this series is a RFC since the spec is not yet ratified nor frozen
> and its purpose is actually to get to a frozen state. As to when this
> will be ratified, I guess Ved can probably answer that.
Usually I'd just not ack the RFC patches, but I do at least check how
far they might be from frozen before I move on, The jira for this one
says "Actual ARC Freeze Approval: 18/Apr/24", which made me think
a freeze was gonna happen soon.
On 23/04/2024 18:39, Conor Dooley wrote:
> On Thu, Apr 18, 2024 at 04:26:44PM +0200, Clément Léger wrote:
>> Add a small driver to request double trap enabling as well as
>> registering a SSE handler for double trap. This will also be used by KVM
>> SBI FWFT extension support to detect if it is possible to enable double
>> trap in VS-mode.
>>
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> arch/riscv/include/asm/sbi.h | 1 +
>> drivers/firmware/Kconfig | 7 +++
>> drivers/firmware/Makefile | 1 +
>> drivers/firmware/riscv_dbltrp.c | 95 +++++++++++++++++++++++++++++++++
>> include/linux/riscv_dbltrp.h | 19 +++++++
>> 5 files changed, 123 insertions(+)
>> create mode 100644 drivers/firmware/riscv_dbltrp.c
>> create mode 100644 include/linux/riscv_dbltrp.h
>>
>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>> index 744aa1796c92..9cd4ca66487c 100644
>> --- a/arch/riscv/include/asm/sbi.h
>> +++ b/arch/riscv/include/asm/sbi.h
>> @@ -314,6 +314,7 @@ enum sbi_sse_attr_id {
>> #define SBI_SSE_ATTR_INTERRUPTED_FLAGS_SPIE (1 << 2)
>>
>> #define SBI_SSE_EVENT_LOCAL_RAS 0x00000000
>> +#define SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP 0x00000001
>> #define SBI_SSE_EVENT_GLOBAL_RAS 0x00008000
>> #define SBI_SSE_EVENT_LOCAL_PMU 0x00010000
>> #define SBI_SSE_EVENT_LOCAL_SOFTWARE 0xffff0000
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 59f611288807..a037f6e89942 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -197,6 +197,13 @@ config RISCV_SSE_TEST
>> Select if you want to enable SSE extension testing at boot time.
>> This will run a series of test which verifies SSE sanity.
>>
>> +config RISCV_DBLTRP
>> + bool "Enable Double trap handling"
>> + depends on RISCV_SSE && RISCV_SBI
>> + default n
>> + help
>> + Select if you want to enable SSE double trap handler.
>> +
>> config SYSFB
>> bool
>> select BOOT_VESA_SUPPORT
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index fb7b0c08c56d..ad67a1738c0f 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>> obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
>> obj-$(CONFIG_RISCV_SSE) += riscv_sse.o
>> obj-$(CONFIG_RISCV_SSE_TEST) += riscv_sse_test.o
>> +obj-$(CONFIG_RISCV_DBLTRP) += riscv_dbltrp.o
>
> As previously mentioned, I'd like to see all of these riscv specific
> things in a riscv directory.
Acked, I was not aware of that.
>
>> obj-$(CONFIG_SYSFB) += sysfb.o
>> obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
>> obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
>> diff --git a/drivers/firmware/riscv_dbltrp.c b/drivers/firmware/riscv_dbltrp.c
>> new file mode 100644
>> index 000000000000..72f9a067e87a
>> --- /dev/null
>> +++ b/drivers/firmware/riscv_dbltrp.c
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Rivos Inc.
>> + */
>> +
>> +#define pr_fmt(fmt) "riscv-dbltrp: " fmt
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/init.h>
>> +#include <linux/riscv_dbltrp.h>
>> +#include <linux/riscv_sse.h>
>> +
>> +#include <asm/sbi.h>
>> +
>> +static bool double_trap_enabled;
>> +
>> +static int riscv_sse_dbltrp_handle(uint32_t evt, void *arg,
>> + struct pt_regs *regs)
>> +{
>> + __show_regs(regs);
>> + panic("Double trap !\n");
>> +
>> + return 0;
>> +}
>> +
>> +struct cpu_dbltrp_data {
>> + int error;
>> +};
>> +
>> +static void
>> +sbi_cpu_enable_double_trap(void *data)
>
> This should easily fit on one line.
>
>> +{
>> + struct sbiret ret;
>> + struct cpu_dbltrp_data *cdd = data;
>> +
>> + ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET,
>> + SBI_FWFT_DOUBLE_TRAP_ENABLE, 1, 0, 0, 0, 0);
>> +
>> + if (ret.error) {
>> + cdd->error = 1;
>
> If this is a boolean, make it a boolean please. All the code in this
> patch treats it as one.
>
>> + pr_err("Failed to enable double trap on cpu %d\n", smp_processor_id());
>> + }
>> +}
>> +
>> +static int sbi_enable_double_trap(void)
>> +{
>> + struct cpu_dbltrp_data cdd = {0};
>> +
>> + on_each_cpu(sbi_cpu_enable_double_trap, &cdd, 1);
>> + if (cdd.error)
>> + return -1;
>
> Can this be an errno please?
>
>> +
>> + double_trap_enabled = true;
>> +
>> + return 0;
>> +}
>> +
>> +bool riscv_double_trap_enabled(void)
>> +{
>> + return double_trap_enabled;
>> +}
>> +EXPORT_SYMBOL(riscv_double_trap_enabled);
>
> Can we just use double_trap everywhere? dbltrp reads like sound that a
> beatboxer would make and this looks a lot nicer than the other functions
> in the file...
Ahah you gave me a good laugh :D
>
>> +
>> +static int __init riscv_dbltrp(void)
>
> I think this function is missing an action work - init or probe?
>
>> +{
>> + struct sse_event *evt;
>> +
>> + if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSDBLTRP)) {
>> + pr_err("Ssdbltrp extension not available\n");
>> + return 1;
>> + }
>> +
>> + if (!sbi_probe_extension(SBI_EXT_FWFT)) {
>> + pr_err("Can not enable double trap, SBI_EXT_FWFT is not available\n");
>> + return 1;
>> + }
>> +
>> + if (sbi_enable_double_trap()) {
>> + pr_err("Failed to enable double trap on all cpus\n");
>> + return 1;
>
> Why do we return 1s here, but an errno via PTR_ERR() below?
> Shouldn't all of these be returning a negative errono?
> This particular one should probably propagate the error it got from
> sbi_enable_double_trap().
Indeed, I'll use some errno everywhere.
Thanks,
Clément
>
> Cheers,
> Conor.
>
>> + }
>> +
>> + evt = sse_event_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, 0,
>> + riscv_sse_dbltrp_handle, NULL);
>> + if (IS_ERR(evt)) {
>> + pr_err("SSE double trap register failed\n");
>> + return PTR_ERR(evt);
>> + }
>> +
>> + sse_event_enable(evt);
>> + pr_info("Double trap handling registered\n");
>> +
>> + return 0;
>> +}
>> +device_initcall(riscv_dbltrp);
>> diff --git a/include/linux/riscv_dbltrp.h b/include/linux/riscv_dbltrp.h
>> new file mode 100644
>> index 000000000000..6de4f43fae6b
>> --- /dev/null
>> +++ b/include/linux/riscv_dbltrp.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2023 Rivos Inc.
>> + */
>> +
>> +#ifndef __LINUX_RISCV_DBLTRP_H
>> +#define __LINUX_RISCV_DBLTRP_H
>> +
>> +#if defined(CONFIG_RISCV_DBLTRP)
>> +bool riscv_double_trap_enabled(void);
>> +#else
>> +
>> +static inline bool riscv_double_trap_enabled(void)
>> +{
>> + return false;
>> +}
>> +#endif
>> +
>> +#endif /* __LINUX_RISCV_DBLTRP_H */
>> --
>> 2.43.0
>>
Conor Dooley wrote:
>Usually I'd just not ack the RFC patches, but I do at least check how
>far they might be from frozen before I move on, The jira for this one
>says "Actual ARC Freeze Approval: 18/Apr/24", which made me think
>a freeze was gonna happen soon.
Yes, should happen soon - expected by 5/4.
regards
ved
On Thu, Apr 18, 2024 at 04:26:40PM +0200, Cl?ment L?ger wrote:
>Add support for FWFT extension in KVM
>
>Signed-off-by: Cl?ment L?ger <[email protected]>
>---
> arch/riscv/include/asm/kvm_host.h | 5 +
> arch/riscv/include/asm/kvm_vcpu_sbi.h | 1 +
> arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 37 ++++++
> arch/riscv/include/uapi/asm/kvm.h | 1 +
> arch/riscv/kvm/Makefile | 1 +
> arch/riscv/kvm/vcpu.c | 5 +
> arch/riscv/kvm/vcpu_sbi.c | 4 +
> arch/riscv/kvm/vcpu_sbi_fwft.c | 136 +++++++++++++++++++++
> 8 files changed, 190 insertions(+)
> create mode 100644 arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
> create mode 100644 arch/riscv/kvm/vcpu_sbi_fwft.c
>
>diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
>index 484d04a92fa6..be60aaa07f57 100644
>--- a/arch/riscv/include/asm/kvm_host.h
>+++ b/arch/riscv/include/asm/kvm_host.h
>@@ -19,6 +19,7 @@
> #include <asm/kvm_vcpu_fp.h>
> #include <asm/kvm_vcpu_insn.h>
> #include <asm/kvm_vcpu_sbi.h>
>+#include <asm/kvm_vcpu_sbi_fwft.h>
> #include <asm/kvm_vcpu_timer.h>
> #include <asm/kvm_vcpu_pmu.h>
>
>@@ -169,6 +170,7 @@ struct kvm_vcpu_csr {
> struct kvm_vcpu_config {
> u64 henvcfg;
> u64 hstateen0;
>+ u64 hedeleg;
> };
>
> struct kvm_vcpu_smstateen_csr {
>@@ -261,6 +263,9 @@ struct kvm_vcpu_arch {
> /* Performance monitoring context */
> struct kvm_pmu pmu_context;
>
>+ /* Firmware feature SBI extension context */
>+ struct kvm_sbi_fwft fwft_context;
>+
> /* 'static' configurations which are set only once */
> struct kvm_vcpu_config cfg;
>
>diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>index b96705258cf9..3a33bbacc233 100644
>--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
>+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>@@ -86,6 +86,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta;
>+extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft;
> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
>
>diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>new file mode 100644
>index 000000000000..7dc1b80c7e6c
>--- /dev/null
>+++ b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>@@ -0,0 +1,37 @@
>+/* SPDX-License-Identifier: GPL-2.0-only */
>+/*
>+ * Copyright (c) 2023 Rivos Inc
>+ *
>+ * Authors:
>+ * Atish Patra <[email protected]>
nit: probably need to fix Copyright year and Authors here :-)
Same in all new files being introduced.
>+ */
>+
>+#ifndef __KVM_VCPU_RISCV_FWFT_H
>+#define __KVM_VCPU_RISCV_FWFT_H
>+
>+#include <asm/sbi.h>
>+
>+#define KVM_SBI_FWFT_FEATURE_COUNT 1
>+
>+static int kvm_sbi_fwft_set(struct kvm_vcpu *vcpu,
>+ enum sbi_fwft_feature_t feature,
>+ unsigned long value, unsigned long flags)
>+{
>+ struct kvm_sbi_fwft_config *conf = kvm_sbi_fwft_get_config(vcpu,
>+ feature);
>+ if (!conf)
>+ return SBI_ERR_DENIED;
Curious,
Why denied and not something like NOT_SUPPORTED NOT_AVAILABLE here?
>+
>+ if ((flags & ~SBI_FWFT_SET_FLAG_LOCK) != 0)
>+ return SBI_ERR_INVALID_PARAM;
>+
>+ if (conf->flags & SBI_FWFT_SET_FLAG_LOCK)
>+ return SBI_ERR_DENIED;
>+
>+ conf->flags = flags;
>+
>+ return conf->feature->set(vcpu, conf, value);
>+}
>+
On Thu, Apr 18, 2024 at 04:26:44PM +0200, Cl?ment L?ger wrote:
>Add a small driver to request double trap enabling as well as
>registering a SSE handler for double trap. This will also be used by KVM
>SBI FWFT extension support to detect if it is possible to enable double
>trap in VS-mode.
>
>Signed-off-by: Cl?ment L?ger <[email protected]>
>---
> arch/riscv/include/asm/sbi.h | 1 +
> drivers/firmware/Kconfig | 7 +++
> drivers/firmware/Makefile | 1 +
> drivers/firmware/riscv_dbltrp.c | 95 +++++++++++++++++++++++++++++++++
> include/linux/riscv_dbltrp.h | 19 +++++++
> 5 files changed, 123 insertions(+)
> create mode 100644 drivers/firmware/riscv_dbltrp.c
> create mode 100644 include/linux/riscv_dbltrp.h
>
>diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>index 744aa1796c92..9cd4ca66487c 100644
>--- a/arch/riscv/include/asm/sbi.h
>+++ b/arch/riscv/include/asm/sbi.h
>@@ -314,6 +314,7 @@ enum sbi_sse_attr_id {
> #define SBI_SSE_ATTR_INTERRUPTED_FLAGS_SPIE (1 << 2)
>
> #define SBI_SSE_EVENT_LOCAL_RAS 0x00000000
>+#define SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP 0x00000001
> #define SBI_SSE_EVENT_GLOBAL_RAS 0x00008000
> #define SBI_SSE_EVENT_LOCAL_PMU 0x00010000
> #define SBI_SSE_EVENT_LOCAL_SOFTWARE 0xffff0000
>diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>index 59f611288807..a037f6e89942 100644
>--- a/drivers/firmware/Kconfig
>+++ b/drivers/firmware/Kconfig
>@@ -197,6 +197,13 @@ config RISCV_SSE_TEST
> Select if you want to enable SSE extension testing at boot time.
> This will run a series of test which verifies SSE sanity.
>
>+config RISCV_DBLTRP
>+ bool "Enable Double trap handling"
>+ depends on RISCV_SSE && RISCV_SBI
>+ default n
>+ help
>+ Select if you want to enable SSE double trap handler.
>+
> config SYSFB
> bool
> select BOOT_VESA_SUPPORT
>diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>index fb7b0c08c56d..ad67a1738c0f 100644
>--- a/drivers/firmware/Makefile
>+++ b/drivers/firmware/Makefile
>@@ -18,6 +18,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
> obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
> obj-$(CONFIG_RISCV_SSE) += riscv_sse.o
> obj-$(CONFIG_RISCV_SSE_TEST) += riscv_sse_test.o
>+obj-$(CONFIG_RISCV_DBLTRP) += riscv_dbltrp.o
> obj-$(CONFIG_SYSFB) += sysfb.o
> obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
> obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
>diff --git a/drivers/firmware/riscv_dbltrp.c b/drivers/firmware/riscv_dbltrp.c
>new file mode 100644
>index 000000000000..72f9a067e87a
>--- /dev/null
>+++ b/drivers/firmware/riscv_dbltrp.c
>@@ -0,0 +1,95 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * Copyright (C) 2023 Rivos Inc.
>+ */
nit: fix copyright year
>+
>+#define pr_fmt(fmt) "riscv-dbltrp: " fmt
>+
>+#include <linux/cpu.h>
>+#include <linux/init.h>
>+#include <linux/riscv_dbltrp.h>
>+#include <linux/riscv_sse.h>
>+
>+#include <asm/sbi.h>
>+
>+static bool double_trap_enabled;
>+
>+static int riscv_sse_dbltrp_handle(uint32_t evt, void *arg,
>+ struct pt_regs *regs)
>+{
>+ __show_regs(regs);
>+ panic("Double trap !\n");
>+
>+ return 0;
Curious:
Does panic return?
What's the point of returning from here?
>+}
>+
>+struct cpu_dbltrp_data {
>+ int error;
>+};
>+
>+static void
>+sbi_cpu_enable_double_trap(void *data)
>+{
>+ struct sbiret ret;
>+ struct cpu_dbltrp_data *cdd = data;
>+
>+ ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET,
>+ SBI_FWFT_DOUBLE_TRAP_ENABLE, 1, 0, 0, 0, 0);
>+
>+ if (ret.error) {
>+ cdd->error = 1;
>+ pr_err("Failed to enable double trap on cpu %d\n", smp_processor_id());
>+ }
>+}
>+
>+static int sbi_enable_double_trap(void)
>+{
>+ struct cpu_dbltrp_data cdd = {0};
>+
>+ on_each_cpu(sbi_cpu_enable_double_trap, &cdd, 1);
>+ if (cdd.error)
>+ return -1;
There is a bug here. If `sbi_cpu_enable_double_trap` failed on all cpus but last cpu.
Then cdd.error would not record error and will be reflect as if double trap was enabled.
Its less likely to happen that FW would return success for one cpu and fail for others.
But there is non-zero probablity here.
>+
>+ double_trap_enabled = true;
>+
>+ return 0;
>+}
>+
>+bool riscv_double_trap_enabled(void)
>+{
>+ return double_trap_enabled;
>+}
>+EXPORT_SYMBOL(riscv_double_trap_enabled);
>+
>+static int __init riscv_dbltrp(void)
>+{
>+ struct sse_event *evt;
>+
>+ if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSDBLTRP)) {
>+ pr_err("Ssdbltrp extension not available\n");
>+ return 1;
>+ }
>+
>+ if (!sbi_probe_extension(SBI_EXT_FWFT)) {
>+ pr_err("Can not enable double trap, SBI_EXT_FWFT is not available\n");
>+ return 1;
>+ }
>+
>+ if (sbi_enable_double_trap()) {
>+ pr_err("Failed to enable double trap on all cpus\n");
>+ return 1;
>+ }
>+
>+ evt = sse_event_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, 0,
>+ riscv_sse_dbltrp_handle, NULL);
>+ if (IS_ERR(evt)) {
>+ pr_err("SSE double trap register failed\n");
>+ return PTR_ERR(evt);
>+ }
>+
>+ sse_event_enable(evt);
>+ pr_info("Double trap handling registered\n");
>+
>+ return 0;
>+}
>+device_initcall(riscv_dbltrp);
>diff --git a/include/linux/riscv_dbltrp.h b/include/linux/riscv_dbltrp.h
>new file mode 100644
>index 000000000000..6de4f43fae6b
>--- /dev/null
>+++ b/include/linux/riscv_dbltrp.h
>@@ -0,0 +1,19 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ * Copyright (C) 2023 Rivos Inc.
>+ */
>+
>+#ifndef __LINUX_RISCV_DBLTRP_H
>+#define __LINUX_RISCV_DBLTRP_H
>+
>+#if defined(CONFIG_RISCV_DBLTRP)
>+bool riscv_double_trap_enabled(void);
>+#else
>+
>+static inline bool riscv_double_trap_enabled(void)
>+{
>+ return false;
>+}
>+#endif
>+
>+#endif /* __LINUX_RISCV_DBLTRP_H */
>--
>2.43.0
>
>
On Thu, Apr 18, 2024 at 04:26:45PM +0200, Cl?ment L?ger wrote:
>Add support in KVM SBI FWFT extension to allow VS-mode to request double
>trap enabling. Double traps can then be generated by VS-mode, allowing
>M-mode to redirect them to S-mode.
>
>Signed-off-by: Cl?ment L?ger <[email protected]>
>---
> arch/riscv/include/asm/csr.h | 1 +
> arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 2 +-
> arch/riscv/kvm/vcpu_sbi_fwft.c | 41 ++++++++++++++++++++++
> 3 files changed, 43 insertions(+), 1 deletion(-)
>
>diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>index 905cdf894a57..ee1b73655bec 100644
>--- a/arch/riscv/include/asm/csr.h
>+++ b/arch/riscv/include/asm/csr.h
>@@ -196,6 +196,7 @@
> /* xENVCFG flags */
> #define ENVCFG_STCE (_AC(1, ULL) << 63)
> #define ENVCFG_PBMTE (_AC(1, ULL) << 62)
>+#define ENVCFG_DTE (_AC(1, ULL) << 59)
> #define ENVCFG_CBZE (_AC(1, UL) << 7)
> #define ENVCFG_CBCFE (_AC(1, UL) << 6)
> #define ENVCFG_CBIE_SHIFT 4
>diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>index 7dc1b80c7e6c..a9e20d655126 100644
>--- a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>+++ b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>@@ -11,7 +11,7 @@
>
> #include <asm/sbi.h>
>
>-#define KVM_SBI_FWFT_FEATURE_COUNT 1
>+#define KVM_SBI_FWFT_FEATURE_COUNT 2
>
> struct kvm_sbi_fwft_config;
> struct kvm_vcpu;
>diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
>index b9b7f8fa6d22..9e8e397eb02f 100644
>--- a/arch/riscv/kvm/vcpu_sbi_fwft.c
>+++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
>@@ -9,10 +9,19 @@
> #include <linux/errno.h>
> #include <linux/err.h>
> #include <linux/kvm_host.h>
>+#include <linux/riscv_dbltrp.h>
> #include <asm/sbi.h>
> #include <asm/kvm_vcpu_sbi.h>
> #include <asm/kvm_vcpu_sbi_fwft.h>
>
>+#ifdef CONFIG_32BIT
>+# define CSR_HENVCFG_DBLTRP CSR_HENVCFGH
>+# define DBLTRP_DTE (ENVCFG_DTE >> 32)
>+#else
>+# define CSR_HENVCFG_DBLTRP CSR_HENVCFG
>+# define DBLTRP_DTE ENVCFG_DTE
>+#endif
>+
> #define MIS_DELEG (1UL << EXC_LOAD_MISALIGNED | 1UL << EXC_STORE_MISALIGNED)
>
> static int kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu,
>@@ -36,6 +45,33 @@ static int kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu,
> return SBI_SUCCESS;
> }
>
>+static int kvm_sbi_fwft_set_double_trap(struct kvm_vcpu *vcpu,
>+ struct kvm_sbi_fwft_config *conf,
>+ unsigned long value)
>+{
>+ if (!riscv_double_trap_enabled())
>+ return SBI_ERR_NOT_SUPPORTED;
Why its required to check whether host has enabled double trap for itself ?
It's orthogonal to guest asking hypervisor to enable double trap.
Probably you need a check here whether underlying FW supports handling double
trap.
Am I missing something here?
>+
>+ if (value)
>+ csr_set(CSR_HENVCFG_DBLTRP, DBLTRP_DTE);
>+ else
>+ csr_clear(CSR_HENVCFG_DBLTRP, DBLTRP_DTE);
I think vcpu->arch.cfg has `henvcfg` field. Can we reflect it there as well so that current
`henvcfg` copy in vcpu arch specifci config is consistent? Otherwise it'll be lost when vCPU
is scheduled out and later scheduled back in (on vcpu load)
Furthermore, lets not do feature specific alias names for CSR.
Instead let's keep consistent 64bit image of henvcfg in vcpu->arch.cfg.
And whenever it's time to pick up the setting, pick up logic either perform the writes in
henvcfg. And if required it'll perform henvcfgh too (as `kvm_arch_vcpu_load` already does)
>+
>+ return SBI_SUCCESS;
>+}
>+
>+static int kvm_sbi_fwft_get_double_trap(struct kvm_vcpu *vcpu,
>+ struct kvm_sbi_fwft_config *conf,
>+ unsigned long *value)
>+{
>+ if (!riscv_double_trap_enabled())
>+ return SBI_ERR_NOT_SUPPORTED;
>+
>+ *value = (csr_read(CSR_HENVCFG_DBLTRP) & DBLTRP_DTE) != 0;
>+
>+ return SBI_SUCCESS;
>+}
>+
> static struct kvm_sbi_fwft_config *
> kvm_sbi_fwft_get_config(struct kvm_vcpu *vcpu, enum sbi_fwft_feature_t feature)
> {
>@@ -111,6 +147,11 @@ static const struct kvm_sbi_fwft_feature features[] = {
> .id = SBI_FWFT_MISALIGNED_DELEG,
> .set = kvm_sbi_fwft_set_misaligned_delegation,
> .get = kvm_sbi_fwft_get_misaligned_delegation,
>+ },
>+ {
>+ .id = SBI_FWFT_DOUBLE_TRAP_ENABLE,
>+ .set = kvm_sbi_fwft_set_double_trap,
>+ .get = kvm_sbi_fwft_get_double_trap,
> }
> };
>
>--
>2.43.0
>
>
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 <[email protected]>
>---
> 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
>
>
On Fri, Apr 26, 2024 at 06:17:08PM -0700, Deepak Gupta wrote:
>On Thu, Apr 18, 2024 at 04:26:45PM +0200, Cl?ment L?ger wrote:
>>Add support in KVM SBI FWFT extension to allow VS-mode to request double
>>trap enabling. Double traps can then be generated by VS-mode, allowing
>>M-mode to redirect them to S-mode.
>>
>>Signed-off-by: Cl?ment L?ger <[email protected]>
>>---
>>arch/riscv/include/asm/csr.h | 1 +
>>arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 2 +-
>>arch/riscv/kvm/vcpu_sbi_fwft.c | 41 ++++++++++++++++++++++
>>3 files changed, 43 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>>index 905cdf894a57..ee1b73655bec 100644
>>--- a/arch/riscv/include/asm/csr.h
>>+++ b/arch/riscv/include/asm/csr.h
>>@@ -196,6 +196,7 @@
>>/* xENVCFG flags */
>>#define ENVCFG_STCE (_AC(1, ULL) << 63)
>>#define ENVCFG_PBMTE (_AC(1, ULL) << 62)
>>+#define ENVCFG_DTE (_AC(1, ULL) << 59)
>>#define ENVCFG_CBZE (_AC(1, UL) << 7)
>>#define ENVCFG_CBCFE (_AC(1, UL) << 6)
>>#define ENVCFG_CBIE_SHIFT 4
>>diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>>index 7dc1b80c7e6c..a9e20d655126 100644
>>--- a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>>+++ b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>>@@ -11,7 +11,7 @@
>>
>>#include <asm/sbi.h>
>>
>>-#define KVM_SBI_FWFT_FEATURE_COUNT 1
>>+#define KVM_SBI_FWFT_FEATURE_COUNT 2
>>
>>struct kvm_sbi_fwft_config;
>>struct kvm_vcpu;
>>diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
>>index b9b7f8fa6d22..9e8e397eb02f 100644
>>--- a/arch/riscv/kvm/vcpu_sbi_fwft.c
>>+++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
>>@@ -9,10 +9,19 @@
>>#include <linux/errno.h>
>>#include <linux/err.h>
>>#include <linux/kvm_host.h>
>>+#include <linux/riscv_dbltrp.h>
>>#include <asm/sbi.h>
>>#include <asm/kvm_vcpu_sbi.h>
>>#include <asm/kvm_vcpu_sbi_fwft.h>
>>
>>+#ifdef CONFIG_32BIT
>>+# define CSR_HENVCFG_DBLTRP CSR_HENVCFGH
>>+# define DBLTRP_DTE (ENVCFG_DTE >> 32)
>>+#else
>>+# define CSR_HENVCFG_DBLTRP CSR_HENVCFG
>>+# define DBLTRP_DTE ENVCFG_DTE
>>+#endif
>>+
>>#define MIS_DELEG (1UL << EXC_LOAD_MISALIGNED | 1UL << EXC_STORE_MISALIGNED)
>>
>>static int kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu,
>>@@ -36,6 +45,33 @@ static int kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu,
>> return SBI_SUCCESS;
>>}
>>
>>+static int kvm_sbi_fwft_set_double_trap(struct kvm_vcpu *vcpu,
>>+ struct kvm_sbi_fwft_config *conf,
>>+ unsigned long value)
>>+{
>>+ if (!riscv_double_trap_enabled())
>>+ return SBI_ERR_NOT_SUPPORTED;
>
>Why its required to check whether host has enabled double trap for itself ?
>It's orthogonal to guest asking hypervisor to enable double trap.
>
>Probably you need a check here whether underlying FW supports handling double
>trap.
>
>Am I missing something here?
>
On this I am indeed missing that menvcfg.DTE has to be 1 for any less priv.
So, nevermind on this comment. Sorry about that.
On 27/04/2024 01:44, Deepak Gupta wrote:
> On Thu, Apr 18, 2024 at 04:26:40PM +0200, Clément Léger wrote:
>> Add support for FWFT extension in KVM
>>
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> arch/riscv/include/asm/kvm_host.h | 5 +
>> arch/riscv/include/asm/kvm_vcpu_sbi.h | 1 +
>> arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h | 37 ++++++
>> arch/riscv/include/uapi/asm/kvm.h | 1 +
>> arch/riscv/kvm/Makefile | 1 +
>> arch/riscv/kvm/vcpu.c | 5 +
>> arch/riscv/kvm/vcpu_sbi.c | 4 +
>> arch/riscv/kvm/vcpu_sbi_fwft.c | 136 +++++++++++++++++++++
>> 8 files changed, 190 insertions(+)
>> create mode 100644 arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>> create mode 100644 arch/riscv/kvm/vcpu_sbi_fwft.c
>>
>> diff --git a/arch/riscv/include/asm/kvm_host.h
>> b/arch/riscv/include/asm/kvm_host.h
>> index 484d04a92fa6..be60aaa07f57 100644
>> --- a/arch/riscv/include/asm/kvm_host.h
>> +++ b/arch/riscv/include/asm/kvm_host.h
>> @@ -19,6 +19,7 @@
>> #include <asm/kvm_vcpu_fp.h>
>> #include <asm/kvm_vcpu_insn.h>
>> #include <asm/kvm_vcpu_sbi.h>
>> +#include <asm/kvm_vcpu_sbi_fwft.h>
>> #include <asm/kvm_vcpu_timer.h>
>> #include <asm/kvm_vcpu_pmu.h>
>>
>> @@ -169,6 +170,7 @@ struct kvm_vcpu_csr {
>> struct kvm_vcpu_config {
>> u64 henvcfg;
>> u64 hstateen0;
>> + u64 hedeleg;
>> };
>>
>> struct kvm_vcpu_smstateen_csr {
>> @@ -261,6 +263,9 @@ struct kvm_vcpu_arch {
>> /* Performance monitoring context */
>> struct kvm_pmu pmu_context;
>>
>> + /* Firmware feature SBI extension context */
>> + struct kvm_sbi_fwft fwft_context;
>> +
>> /* 'static' configurations which are set only once */
>> struct kvm_vcpu_config cfg;
>>
>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h
>> b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>> index b96705258cf9..3a33bbacc233 100644
>> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
>> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>> @@ -86,6 +86,7 @@ extern const struct kvm_vcpu_sbi_extension
>> vcpu_sbi_ext_srst;
>> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
>> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
>> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta;
>> +extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft;
>> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
>> extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
>>
>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>> b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>> new file mode 100644
>> index 000000000000..7dc1b80c7e6c
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi_fwft.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2023 Rivos Inc
>> + *
>> + * Authors:
>> + * Atish Patra <[email protected]>
>
> nit: probably need to fix Copyright year and Authors here :-)
> Same in all new files being introduced.
>
>> + */
>> +
>> +#ifndef __KVM_VCPU_RISCV_FWFT_H
>> +#define __KVM_VCPU_RISCV_FWFT_H
>> +
>> +#include <asm/sbi.h>
>> +
>> +#define KVM_SBI_FWFT_FEATURE_COUNT 1
>> +
>> +static int kvm_sbi_fwft_set(struct kvm_vcpu *vcpu,
>> + enum sbi_fwft_feature_t feature,
>> + unsigned long value, unsigned long flags)
>> +{
>> + struct kvm_sbi_fwft_config *conf = kvm_sbi_fwft_get_config(vcpu,
>> + feature);
>> + if (!conf)
>> + return SBI_ERR_DENIED;
>
> Curious,
> Why denied and not something like NOT_SUPPORTED NOT_AVAILABLE here?
Hey Deepak,
So indeed, the return value is not totally correct since the spec states
that we return EDENIED if feature is reserved or is platform-specific
and unimplemented. But in that case it dos not distinguish between
defined features and reserved one. I'll add a check for that.
Thanks,
Clément
>
>> +
>> + if ((flags & ~SBI_FWFT_SET_FLAG_LOCK) != 0)
>> + return SBI_ERR_INVALID_PARAM;
>> +
>> + if (conf->flags & SBI_FWFT_SET_FLAG_LOCK)
>> + return SBI_ERR_DENIED;
>> +
>> + conf->flags = flags;
>> +
>> + return conf->feature->set(vcpu, conf, value);
>> +}
>> +
On 27/04/2024 03:33, Deepak Gupta wrote:
>> /**
>> * 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?
We actually do not really care since if not enabled, this bit will be
masked by the hardware when writing it to the CSR. I could indeed add a
check for that but in the end, it will yield the same result. In that
case, just ORing it with the value seems more efficient than adding an
"if" to check if the extension and DTE is enabled.
Clément