kvm-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [kvm-unit-tests v3 0/2] riscv: Add double trap testing
@ 2025-06-16 11:58 Clément Léger
  2025-06-16 11:58 ` [kvm-unit-tests v3 1/2] lib/riscv: export FWFT functions Clément Léger
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Clément Léger @ 2025-06-16 11:58 UTC (permalink / raw)
  To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones

Add a test that triggers double trap and verify that it's behavior
conforms to the spec. Also use SSE to verify that an SSE event is
correctly sent upon double trap.

In order to run this test, one can use the following command using an
upstream version of OpenSBI:

$ QEMU=qemu-system-riscv64 \
  FIRMWARE_OVERRIDE=<opensbi>/fw_dynamic.bin \
  ./riscv-run riscv/isa-dbltrp.flat

---

v3:
 - Return an error only if SSE event wasn't unregistered successfully

v2:
 - Use WRITE_ONCE/READ_ONCE for shared variables
 - Remove locking flag for last test
 - Fix a few typos
 - Skip crash test if env var DOUBLE_TRAP_TEST_CRASH isn't set
 - Skip crash test if SSE event unregistering failed
 - Remove SDT clearing patch
 - Fix wrong check using ret.value nstead of ret.error

Clément Léger (2):
  lib/riscv: export FWFT functions
  riscv: Add ISA double trap extension testing

 riscv/Makefile            |   1 +
 lib/riscv/asm/csr.h       |   1 +
 lib/riscv/asm/processor.h |  10 ++
 lib/riscv/asm/sbi.h       |   5 +
 lib/riscv/sbi.c           |  20 ++++
 riscv/isa-dbltrp.c        | 210 ++++++++++++++++++++++++++++++++++++++
 riscv/sbi-fwft.c          |  49 +++------
 riscv/unittests.cfg       |   4 +
 8 files changed, 265 insertions(+), 35 deletions(-)
 create mode 100644 riscv/isa-dbltrp.c

-- 
2.49.0


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [kvm-unit-tests v3 1/2] lib/riscv: export FWFT functions
  2025-06-16 11:58 [kvm-unit-tests v3 0/2] riscv: Add double trap testing Clément Léger
@ 2025-06-16 11:58 ` Clément Léger
  2025-06-16 11:59 ` [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing Clément Léger
  2025-07-02 14:48 ` [kvm-unit-tests v3 0/2] riscv: Add double trap testing Andrew Jones
  2 siblings, 0 replies; 6+ messages in thread
From: Clément Léger @ 2025-06-16 11:58 UTC (permalink / raw)
  To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Andrew Jones

These functions will be needed by other tests as well, expose them.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/sbi.h |  5 +++++
 lib/riscv/sbi.c     | 20 ++++++++++++++++++
 riscv/sbi-fwft.c    | 49 +++++++++++++--------------------------------
 3 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index a5738a5c..08146260 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -302,5 +302,10 @@ struct sbiret sbi_sse_hart_mask(void);
 struct sbiret sbi_sse_hart_unmask(void);
 struct sbiret sbi_sse_inject(unsigned long event_id, unsigned long hart_id);
 
+struct sbiret sbi_fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags);
+struct sbiret sbi_fwft_set(uint32_t feature, unsigned long value, unsigned long flags);
+struct sbiret sbi_fwft_get_raw(unsigned long feature);
+struct sbiret sbi_fwft_get(uint32_t feature);
+
 #endif /* !__ASSEMBLER__ */
 #endif /* _ASMRISCV_SBI_H_ */
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 2959378f..0b547d42 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -107,6 +107,26 @@ struct sbiret sbi_sse_inject(unsigned long event_id, unsigned long hart_id)
 	return sbi_ecall(SBI_EXT_SSE, SBI_EXT_SSE_INJECT, event_id, hart_id, 0, 0, 0, 0);
 }
 
+struct sbiret sbi_fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags)
+{
+	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, feature, value, flags, 0, 0, 0);
+}
+
+struct sbiret sbi_fwft_set(uint32_t feature, unsigned long value, unsigned long flags)
+{
+	return sbi_fwft_set_raw(feature, value, flags);
+}
+
+struct sbiret sbi_fwft_get_raw(unsigned long feature)
+{
+	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, feature, 0, 0, 0, 0, 0);
+}
+
+struct sbiret sbi_fwft_get(uint32_t feature)
+{
+	return sbi_fwft_get_raw(feature);
+}
+
 void sbi_shutdown(void)
 {
 	sbi_ecall(SBI_EXT_SRST, 0, 0, 0, 0, 0, 0, 0);
diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index c52fbd6e..8920bcb5 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -19,37 +19,16 @@
 
 void check_fwft(void);
 
-
-static struct sbiret fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags)
-{
-	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, feature, value, flags, 0, 0, 0);
-}
-
-static struct sbiret fwft_set(uint32_t feature, unsigned long value, unsigned long flags)
-{
-	return fwft_set_raw(feature, value, flags);
-}
-
-static struct sbiret fwft_get_raw(unsigned long feature)
-{
-	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, feature, 0, 0, 0, 0, 0);
-}
-
-static struct sbiret fwft_get(uint32_t feature)
-{
-	return fwft_get_raw(feature);
-}
-
 static struct sbiret fwft_set_and_check_raw(const char *str, unsigned long feature,
 					    unsigned long value, unsigned long flags)
 {
 	struct sbiret ret;
 
-	ret = fwft_set_raw(feature, value, flags);
+	ret = sbi_fwft_set_raw(feature, value, flags);
 	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %ld%s", value, str))
 		return ret;
 
-	ret = fwft_get_raw(feature);
+	ret = sbi_fwft_get_raw(feature);
 	sbiret_report(&ret, SBI_SUCCESS, value, "get %ld after set%s", value, str);
 
 	return ret;
@@ -59,17 +38,17 @@ static void fwft_check_reserved(unsigned long id)
 {
 	struct sbiret ret;
 
-	ret = fwft_get(id);
+	ret = sbi_fwft_get(id);
 	sbiret_report_error(&ret, SBI_ERR_DENIED, "get reserved feature 0x%lx", id);
 
-	ret = fwft_set(id, 1, 0);
+	ret = sbi_fwft_set(id, 1, 0);
 	sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
 }
 
-/* Must be called before any fwft_set() call is made for @feature */
+/* Must be called before any sbi_fwft_set() call is made for @feature */
 static void fwft_check_reset(uint32_t feature, unsigned long reset)
 {
-	struct sbiret ret = fwft_get(feature);
+	struct sbiret ret = sbi_fwft_get(feature);
 
 	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
 }
@@ -87,16 +66,16 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
 		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
 
 	for (int i = 0; i < nr_values; ++i) {
-		ret = fwft_set(feature, test_values[i], 0);
+		ret = sbi_fwft_set(feature, test_values[i], 0);
 		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
 				   "Set to %lu without lock flag", test_values[i]);
 
-		ret = fwft_set(feature, test_values[i], SBI_FWFT_SET_FLAG_LOCK);
+		ret = sbi_fwft_set(feature, test_values[i], SBI_FWFT_SET_FLAG_LOCK);
 		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
 				   "Set to %lu with lock flag", test_values[i]);
 	}
 
-	ret = fwft_get(feature);
+	ret = sbi_fwft_get(feature);
 	sbiret_report(&ret, SBI_SUCCESS, locked_value, "Get value %lu", locked_value);
 
 	report_prefix_pop();
@@ -131,12 +110,12 @@ static void misaligned_handler(struct pt_regs *regs)
 
 static struct sbiret fwft_misaligned_exc_set(unsigned long value, unsigned long flags)
 {
-	return fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, value, flags);
+	return sbi_fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, value, flags);
 }
 
 static struct sbiret fwft_misaligned_exc_get(void)
 {
-	return fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
+	return sbi_fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
 }
 
 static void fwft_check_misaligned_exc_deleg(void)
@@ -304,7 +283,7 @@ static void fwft_check_pte_ad_hw_updating(void)
 
 	report_prefix_push("pte_ad_hw_updating");
 
-	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	ret = sbi_fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
 	if (ret.error != SBI_SUCCESS) {
 		if (env_enabled("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
 			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
@@ -350,10 +329,10 @@ static void fwft_check_pte_ad_hw_updating(void)
 #endif
 
 adue_inval_tests:
-	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, 2, 0);
+	ret = sbi_fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, 2, 0);
 	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to 2");
 
-	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 2);
+	ret = sbi_fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 2);
 	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to %d with flags=2", !enabled);
 
 	if (!adue_toggle_and_check(" with lock", !enabled, 1))
-- 
2.49.0


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing
  2025-06-16 11:58 [kvm-unit-tests v3 0/2] riscv: Add double trap testing Clément Léger
  2025-06-16 11:58 ` [kvm-unit-tests v3 1/2] lib/riscv: export FWFT functions Clément Léger
@ 2025-06-16 11:59 ` Clément Léger
  2025-06-19  7:59   ` Clément Léger
  2025-07-02 14:48 ` [kvm-unit-tests v3 0/2] riscv: Add double trap testing Andrew Jones
  2 siblings, 1 reply; 6+ messages in thread
From: Clément Léger @ 2025-06-16 11:59 UTC (permalink / raw)
  To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones

This test allows to test the double trap implementation of hardware as
well as the SBI FWFT and SSE support for double trap. The tests will try
to trigger double trap using various sequences and will test to receive
the SSE double trap event if supported.

It is provided as a separate test from the SBI one for two reasons:
- It isn't specifically testing SBI "per se".
- It ends up by trying to crash into in M-mode.

Currently, the test uses a page fault to raise a trap programatically.
Some concern was raised by a github user on the original branch [1]
saying that the spec doesn't mandate any trap to be delegatable and that
we would need a way to detect which ones are delegatable. I think we can
safely assume that PAGE FAULT is delegatable and if a hardware that does
not have support comes up then it will probably be the vendor
responsibility to provide a way to do so.

Link: https://github.com/clementleger/kvm-unit-tests/issues/1 [1]
Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 riscv/Makefile            |   1 +
 lib/riscv/asm/csr.h       |   1 +
 lib/riscv/asm/processor.h |  10 ++
 riscv/isa-dbltrp.c        | 210 ++++++++++++++++++++++++++++++++++++++
 riscv/unittests.cfg       |   4 +
 5 files changed, 226 insertions(+)
 create mode 100644 riscv/isa-dbltrp.c

diff --git a/riscv/Makefile b/riscv/Makefile
index 11e68eae..d71c9d2e 100644
--- a/riscv/Makefile
+++ b/riscv/Makefile
@@ -14,6 +14,7 @@ tests =
 tests += $(TEST_DIR)/sbi.$(exe)
 tests += $(TEST_DIR)/selftest.$(exe)
 tests += $(TEST_DIR)/sieve.$(exe)
+tests += $(TEST_DIR)/isa-dbltrp.$(exe)
 
 all: $(tests)
 
diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index 3e4b5fca..6a8e0578 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -18,6 +18,7 @@
 
 #define SR_SIE			_AC(0x00000002, UL)
 #define SR_SPP			_AC(0x00000100, UL)
+#define SR_SDT			_AC(0x01000000, UL) /* Supervisor Double Trap */
 
 /* Exception cause high bit - is an interrupt if set */
 #define CAUSE_IRQ_FLAG		(_AC(1, UL) << (__riscv_xlen - 1))
diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
index 631ce226..a3dab064 100644
--- a/lib/riscv/asm/processor.h
+++ b/lib/riscv/asm/processor.h
@@ -50,6 +50,16 @@ static inline void ipi_ack(void)
 	csr_clear(CSR_SIP, IE_SSIE);
 }
 
+static inline void local_dlbtrp_enable(void)
+{
+	csr_set(CSR_SSTATUS, SR_SDT);
+}
+
+static inline void local_dlbtrp_disable(void)
+{
+	csr_clear(CSR_SSTATUS, SR_SDT);
+}
+
 void install_exception_handler(unsigned long cause, void (*handler)(struct pt_regs *));
 void install_irq_handler(unsigned long cause, void (*handler)(struct pt_regs *));
 void do_handle_exception(struct pt_regs *regs);
diff --git a/riscv/isa-dbltrp.c b/riscv/isa-dbltrp.c
new file mode 100644
index 00000000..dcfa66da
--- /dev/null
+++ b/riscv/isa-dbltrp.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SBI verification
+ *
+ * Copyright (C) 2025, Rivos Inc., Clément Léger <cleger@rivosinc.com>
+ */
+#include <alloc.h>
+#include <alloc_page.h>
+#include <libcflat.h>
+#include <stdlib.h>
+
+#include <asm/csr.h>
+#include <asm/page.h>
+#include <asm/processor.h>
+#include <asm/ptrace.h>
+#include <asm/sbi.h>
+
+#include <sbi-tests.h>
+
+static bool double_trap;
+static bool clear_sdt;
+
+#define INSN_LEN(insn)			((((insn) & 0x3) < 0x3) ? 2 : 4)
+
+#define GEN_TRAP()								\
+do {										\
+	void *ptr = NULL;							\
+	unsigned long value = 0;						\
+	asm volatile(								\
+	"	.option push\n"							\
+	"	.option arch,-c\n"						\
+	"	sw %0, 0(%1)\n"							\
+	"	.option pop\n"							\
+	: : "r" (value), "r" (ptr) : "memory");					\
+} while (0)
+
+static void pagefault_trap_handler(struct pt_regs *regs)
+{
+	if (READ_ONCE(clear_sdt))
+		local_dlbtrp_disable();
+
+	if (READ_ONCE(double_trap)) {
+		WRITE_ONCE(double_trap, false);
+		GEN_TRAP();
+	}
+
+	/* Skip trapping instruction */
+	regs->epc += 4;
+
+	local_dlbtrp_enable();
+}
+
+static bool sse_dbltrp_called;
+
+static void sse_dbltrp_handler(void *data, struct pt_regs *regs, unsigned int hartid)
+{
+	struct sbiret ret;
+	unsigned long flags;
+	unsigned long expected_flags = SBI_SSE_ATTR_INTERRUPTED_FLAGS_SSTATUS_SPP |
+				       SBI_SSE_ATTR_INTERRUPTED_FLAGS_SSTATUS_SDT;
+
+	ret = sbi_sse_read_attrs(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, SBI_SSE_ATTR_INTERRUPTED_FLAGS, 1,
+				 &flags);
+	sbiret_report_error(&ret, SBI_SUCCESS, "Get double trap event flags");
+	report(flags == expected_flags, "SSE flags == 0x%lx", expected_flags);
+
+	WRITE_ONCE(sse_dbltrp_called, true);
+
+	/* Skip trapping instruction */
+	regs->epc += 4;
+}
+
+static int sse_double_trap(void)
+{
+	struct sbiret ret;
+	int err = 0;
+
+	struct sbi_sse_handler_arg handler_arg = {
+		.handler = sse_dbltrp_handler,
+		.stack = alloc_page() + PAGE_SIZE,
+	};
+
+	report_prefix_push("sse");
+
+	ret = sbi_sse_hart_unmask();
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE hart unmask ok")) {
+		report_skip("Failed to unmask SSE events, skipping test");
+		goto out_free_page;
+	}
+
+	ret = sbi_sse_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, &handler_arg);
+	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
+		report_skip("SSE double trap event is not supported");
+		goto out_mask_sse;
+	}
+	sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap register");
+
+	ret = sbi_sse_enable(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP);
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap enable"))
+		goto out_unregister;
+
+	/*
+	 * Generate a double crash so that an SSE event should be generated. The SPEC (ISA nor SBI)
+	 * does not explicitly tell that if supported it should generate an SSE event but that's
+	 * a reasonable assumption to do so if both FWFT and SSE are supported.
+	 */
+	WRITE_ONCE(clear_sdt, false);
+	WRITE_ONCE(double_trap, true);
+	GEN_TRAP();
+
+	report(READ_ONCE(sse_dbltrp_called), "SSE double trap event generated");
+
+	ret = sbi_sse_disable(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP);
+	sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap disable");
+
+out_unregister:
+	ret = sbi_sse_unregister(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP);
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap unregister"))
+		err = ret.error;
+
+out_mask_sse:
+	sbi_sse_hart_mask();
+
+out_free_page:
+	free_page(handler_arg.stack - PAGE_SIZE);
+	report_prefix_pop();
+
+	return err;
+}
+
+static void check_double_trap(void)
+{
+	struct sbiret ret;
+
+	/* Disable double trap */
+	ret = sbi_fwft_set(SBI_FWFT_DOUBLE_TRAP, 0, 0);
+	sbiret_report_error(&ret, SBI_SUCCESS, "Set double trap enable feature value == 0");
+	ret = sbi_fwft_get(SBI_FWFT_DOUBLE_TRAP);
+	sbiret_report(&ret, SBI_SUCCESS, 0, "Get double trap enable feature value == 0");
+
+	install_exception_handler(EXC_STORE_PAGE_FAULT, pagefault_trap_handler);
+
+	WRITE_ONCE(clear_sdt, true);
+	WRITE_ONCE(double_trap, true);
+	GEN_TRAP();
+	report_pass("Double trap disabled, trap first time ok");
+
+	/* Enable double trap */
+	ret = sbi_fwft_set(SBI_FWFT_DOUBLE_TRAP, 1, 0);
+	sbiret_report_error(&ret, SBI_SUCCESS, "Set double trap enable feature value == 1");
+	ret = sbi_fwft_get(SBI_FWFT_DOUBLE_TRAP);
+	if (!sbiret_report(&ret, SBI_SUCCESS, 1, "Get double trap enable feature value == 1"))
+		return;
+
+	/* First time, clear the double trap flag (SDT) so that it doesn't generate a double trap */
+	WRITE_ONCE(clear_sdt, true);
+	WRITE_ONCE(double_trap, true);
+
+	GEN_TRAP();
+	report_pass("Trapped twice allowed ok");
+
+	if (sbi_probe(SBI_EXT_SSE)) {
+		if (sse_double_trap()) {
+			report_skip("Could not correctly unregister SSE event, skipping last test");
+			return;
+		}
+	} else {
+		report_skip("SSE double trap event will not be tested, extension is not available");
+	}
+
+	if (!env_or_skip("DOUBLE_TRAP_TEST_CRASH"))
+		return;
+
+	/*
+	 * Third time, keep the double trap flag (SDT) and generate another trap, this should
+	 * generate a double trap. Since there is no SSE handler registered, it should crash to
+	 * M-mode.
+	 */
+	WRITE_ONCE(clear_sdt, false);
+	WRITE_ONCE(double_trap, true);
+	report_info("Should generate a double trap and crash!");
+	GEN_TRAP();
+	report_fail("Should have crashed!");
+}
+
+int main(int argc, char **argv)
+{
+	struct sbiret ret;
+
+	report_prefix_push("dbltrp");
+
+	if (!sbi_probe(SBI_EXT_FWFT)) {
+		report_skip("FWFT extension is not available, can not enable double traps");
+		goto out;
+	}
+
+	ret = sbi_fwft_get(SBI_FWFT_DOUBLE_TRAP);
+	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
+		report_skip("SBI_FWFT_DOUBLE_TRAP is not supported!");
+		goto out;
+	}
+
+	if (sbiret_report_error(&ret, SBI_SUCCESS, "SBI_FWFT_DOUBLE_TRAP get value"))
+		check_double_trap();
+
+out:
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/riscv/unittests.cfg b/riscv/unittests.cfg
index 2eb760ec..286e1cc7 100644
--- a/riscv/unittests.cfg
+++ b/riscv/unittests.cfg
@@ -18,3 +18,7 @@ groups = selftest
 file = sbi.flat
 smp = $MAX_SMP
 groups = sbi
+
+[dbltrp]
+file = isa-dbltrp.flat
+groups = isa sbi
-- 
2.49.0


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing
  2025-06-16 11:59 ` [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing Clément Léger
@ 2025-06-19  7:59   ` Clément Léger
  2025-06-23 16:29     ` Andrew Jones
  0 siblings, 1 reply; 6+ messages in thread
From: Clément Léger @ 2025-06-19  7:59 UTC (permalink / raw)
  To: kvm, kvm-riscv; +Cc: Andrew Jones



On 16/06/2025 13:59, Clément Léger wrote:
> This test allows to test the double trap implementation of hardware as
> well as the SBI FWFT and SSE support for double trap. The tests will try
> to trigger double trap using various sequences and will test to receive
> the SSE double trap event if supported.
> 
> It is provided as a separate test from the SBI one for two reasons:
> - It isn't specifically testing SBI "per se".
> - It ends up by trying to crash into in M-mode.
> 
> Currently, the test uses a page fault to raise a trap programatically.
> Some concern was raised by a github user on the original branch [1]
> saying that the spec doesn't mandate any trap to be delegatable and that
> we would need a way to detect which ones are delegatable. I think we can
> safely assume that PAGE FAULT is delegatable and if a hardware that does
> not have support comes up then it will probably be the vendor
> responsibility to provide a way to do so.
> 
> Link: https://github.com/clementleger/kvm-unit-tests/issues/1 [1]
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  riscv/Makefile            |   1 +
>  lib/riscv/asm/csr.h       |   1 +
>  lib/riscv/asm/processor.h |  10 ++
>  riscv/isa-dbltrp.c        | 210 ++++++++++++++++++++++++++++++++++++++
>  riscv/unittests.cfg       |   4 +
>  5 files changed, 226 insertions(+)
>  create mode 100644 riscv/isa-dbltrp.c
> 
> diff --git a/riscv/Makefile b/riscv/Makefile
> index 11e68eae..d71c9d2e 100644
> --- a/riscv/Makefile
> +++ b/riscv/Makefile
> @@ -14,6 +14,7 @@ tests =
>  tests += $(TEST_DIR)/sbi.$(exe)
>  tests += $(TEST_DIR)/selftest.$(exe)
>  tests += $(TEST_DIR)/sieve.$(exe)
> +tests += $(TEST_DIR)/isa-dbltrp.$(exe)
>  
>  all: $(tests)
>  
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index 3e4b5fca..6a8e0578 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -18,6 +18,7 @@
>  
>  #define SR_SIE			_AC(0x00000002, UL)
>  #define SR_SPP			_AC(0x00000100, UL)
> +#define SR_SDT			_AC(0x01000000, UL) /* Supervisor Double Trap */
>  
>  /* Exception cause high bit - is an interrupt if set */
>  #define CAUSE_IRQ_FLAG		(_AC(1, UL) << (__riscv_xlen - 1))
> diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
> index 631ce226..a3dab064 100644
> --- a/lib/riscv/asm/processor.h
> +++ b/lib/riscv/asm/processor.h
> @@ -50,6 +50,16 @@ static inline void ipi_ack(void)
>  	csr_clear(CSR_SIP, IE_SSIE);
>  }
>  
> +static inline void local_dlbtrp_enable(void)
> +{
> +	csr_set(CSR_SSTATUS, SR_SDT);
> +}
> +
> +static inline void local_dlbtrp_disable(void)
> +{
> +	csr_clear(CSR_SSTATUS, SR_SDT);
> +}
> +
>  void install_exception_handler(unsigned long cause, void (*handler)(struct pt_regs *));
>  void install_irq_handler(unsigned long cause, void (*handler)(struct pt_regs *));
>  void do_handle_exception(struct pt_regs *regs);
> diff --git a/riscv/isa-dbltrp.c b/riscv/isa-dbltrp.c
> new file mode 100644
> index 00000000..dcfa66da
> --- /dev/null
> +++ b/riscv/isa-dbltrp.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SBI verification
> + *
> + * Copyright (C) 2025, Rivos Inc., Clément Léger <cleger@rivosinc.com>
> + */
> +#include <alloc.h>
> +#include <alloc_page.h>
> +#include <libcflat.h>
> +#include <stdlib.h>
> +
> +#include <asm/csr.h>
> +#include <asm/page.h>
> +#include <asm/processor.h>
> +#include <asm/ptrace.h>
> +#include <asm/sbi.h>
> +
> +#include <sbi-tests.h>
> +
> +static bool double_trap;
> +static bool clear_sdt;
> +
> +#define INSN_LEN(insn)			((((insn) & 0x3) < 0x3) ? 2 : 4)

This macro should be removed since it was merged in another file.

Clément

> +
> +#define GEN_TRAP()								\
> +do {										\
> +	void *ptr = NULL;							\
> +	unsigned long value = 0;						\
> +	asm volatile(								\
> +	"	.option push\n"							\
> +	"	.option arch,-c\n"						\
> +	"	sw %0, 0(%1)\n"							\
> +	"	.option pop\n"							\
> +	: : "r" (value), "r" (ptr) : "memory");					\
> +} while (0)
> +
> +static void pagefault_trap_handler(struct pt_regs *regs)
> +{
> +	if (READ_ONCE(clear_sdt))
> +		local_dlbtrp_disable();
> +
> +	if (READ_ONCE(double_trap)) {
> +		WRITE_ONCE(double_trap, false);
> +		GEN_TRAP();
> +	}
> +
> +	/* Skip trapping instruction */
> +	regs->epc += 4;
> +
> +	local_dlbtrp_enable();
> +}
> +
> +static bool sse_dbltrp_called;
> +
> +static void sse_dbltrp_handler(void *data, struct pt_regs *regs, unsigned int hartid)
> +{
> +	struct sbiret ret;
> +	unsigned long flags;
> +	unsigned long expected_flags = SBI_SSE_ATTR_INTERRUPTED_FLAGS_SSTATUS_SPP |
> +				       SBI_SSE_ATTR_INTERRUPTED_FLAGS_SSTATUS_SDT;
> +
> +	ret = sbi_sse_read_attrs(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, SBI_SSE_ATTR_INTERRUPTED_FLAGS, 1,
> +				 &flags);
> +	sbiret_report_error(&ret, SBI_SUCCESS, "Get double trap event flags");
> +	report(flags == expected_flags, "SSE flags == 0x%lx", expected_flags);
> +
> +	WRITE_ONCE(sse_dbltrp_called, true);
> +
> +	/* Skip trapping instruction */
> +	regs->epc += 4;
> +}
> +
> +static int sse_double_trap(void)
> +{
> +	struct sbiret ret;
> +	int err = 0;
> +
> +	struct sbi_sse_handler_arg handler_arg = {
> +		.handler = sse_dbltrp_handler,
> +		.stack = alloc_page() + PAGE_SIZE,
> +	};
> +
> +	report_prefix_push("sse");
> +
> +	ret = sbi_sse_hart_unmask();
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE hart unmask ok")) {
> +		report_skip("Failed to unmask SSE events, skipping test");
> +		goto out_free_page;
> +	}
> +
> +	ret = sbi_sse_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, &handler_arg);
> +	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> +		report_skip("SSE double trap event is not supported");
> +		goto out_mask_sse;
> +	}
> +	sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap register");
> +
> +	ret = sbi_sse_enable(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP);
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap enable"))
> +		goto out_unregister;
> +
> +	/*
> +	 * Generate a double crash so that an SSE event should be generated. The SPEC (ISA nor SBI)
> +	 * does not explicitly tell that if supported it should generate an SSE event but that's
> +	 * a reasonable assumption to do so if both FWFT and SSE are supported.
> +	 */
> +	WRITE_ONCE(clear_sdt, false);
> +	WRITE_ONCE(double_trap, true);
> +	GEN_TRAP();
> +
> +	report(READ_ONCE(sse_dbltrp_called), "SSE double trap event generated");
> +
> +	ret = sbi_sse_disable(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP);
> +	sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap disable");
> +
> +out_unregister:
> +	ret = sbi_sse_unregister(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP);
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap unregister"))
> +		err = ret.error;
> +
> +out_mask_sse:
> +	sbi_sse_hart_mask();
> +
> +out_free_page:
> +	free_page(handler_arg.stack - PAGE_SIZE);
> +	report_prefix_pop();
> +
> +	return err;
> +}
> +
> +static void check_double_trap(void)
> +{
> +	struct sbiret ret;
> +
> +	/* Disable double trap */
> +	ret = sbi_fwft_set(SBI_FWFT_DOUBLE_TRAP, 0, 0);
> +	sbiret_report_error(&ret, SBI_SUCCESS, "Set double trap enable feature value == 0");
> +	ret = sbi_fwft_get(SBI_FWFT_DOUBLE_TRAP);
> +	sbiret_report(&ret, SBI_SUCCESS, 0, "Get double trap enable feature value == 0");
> +
> +	install_exception_handler(EXC_STORE_PAGE_FAULT, pagefault_trap_handler);
> +
> +	WRITE_ONCE(clear_sdt, true);
> +	WRITE_ONCE(double_trap, true);
> +	GEN_TRAP();
> +	report_pass("Double trap disabled, trap first time ok");
> +
> +	/* Enable double trap */
> +	ret = sbi_fwft_set(SBI_FWFT_DOUBLE_TRAP, 1, 0);
> +	sbiret_report_error(&ret, SBI_SUCCESS, "Set double trap enable feature value == 1");
> +	ret = sbi_fwft_get(SBI_FWFT_DOUBLE_TRAP);
> +	if (!sbiret_report(&ret, SBI_SUCCESS, 1, "Get double trap enable feature value == 1"))
> +		return;
> +
> +	/* First time, clear the double trap flag (SDT) so that it doesn't generate a double trap */
> +	WRITE_ONCE(clear_sdt, true);
> +	WRITE_ONCE(double_trap, true);
> +
> +	GEN_TRAP();
> +	report_pass("Trapped twice allowed ok");
> +
> +	if (sbi_probe(SBI_EXT_SSE)) {
> +		if (sse_double_trap()) {
> +			report_skip("Could not correctly unregister SSE event, skipping last test");
> +			return;
> +		}
> +	} else {
> +		report_skip("SSE double trap event will not be tested, extension is not available");
> +	}
> +
> +	if (!env_or_skip("DOUBLE_TRAP_TEST_CRASH"))
> +		return;
> +
> +	/*
> +	 * Third time, keep the double trap flag (SDT) and generate another trap, this should
> +	 * generate a double trap. Since there is no SSE handler registered, it should crash to
> +	 * M-mode.
> +	 */
> +	WRITE_ONCE(clear_sdt, false);
> +	WRITE_ONCE(double_trap, true);
> +	report_info("Should generate a double trap and crash!");
> +	GEN_TRAP();
> +	report_fail("Should have crashed!");
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	struct sbiret ret;
> +
> +	report_prefix_push("dbltrp");
> +
> +	if (!sbi_probe(SBI_EXT_FWFT)) {
> +		report_skip("FWFT extension is not available, can not enable double traps");
> +		goto out;
> +	}
> +
> +	ret = sbi_fwft_get(SBI_FWFT_DOUBLE_TRAP);
> +	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> +		report_skip("SBI_FWFT_DOUBLE_TRAP is not supported!");
> +		goto out;
> +	}
> +
> +	if (sbiret_report_error(&ret, SBI_SUCCESS, "SBI_FWFT_DOUBLE_TRAP get value"))
> +		check_double_trap();
> +
> +out:
> +	report_prefix_pop();
> +
> +	return report_summary();
> +}
> diff --git a/riscv/unittests.cfg b/riscv/unittests.cfg
> index 2eb760ec..286e1cc7 100644
> --- a/riscv/unittests.cfg
> +++ b/riscv/unittests.cfg
> @@ -18,3 +18,7 @@ groups = selftest
>  file = sbi.flat
>  smp = $MAX_SMP
>  groups = sbi
> +
> +[dbltrp]
> +file = isa-dbltrp.flat
> +groups = isa sbi


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing
  2025-06-19  7:59   ` Clément Léger
@ 2025-06-23 16:29     ` Andrew Jones
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2025-06-23 16:29 UTC (permalink / raw)
  To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones

On Thu, Jun 19, 2025 at 09:59:47AM +0200, Clément Léger wrote:
> 
> 
> On 16/06/2025 13:59, Clément Léger wrote:
> > This test allows to test the double trap implementation of hardware as
> > well as the SBI FWFT and SSE support for double trap. The tests will try
> > to trigger double trap using various sequences and will test to receive
> > the SSE double trap event if supported.
> > 
> > It is provided as a separate test from the SBI one for two reasons:
> > - It isn't specifically testing SBI "per se".
> > - It ends up by trying to crash into in M-mode.
> > 
> > Currently, the test uses a page fault to raise a trap programatically.
> > Some concern was raised by a github user on the original branch [1]
> > saying that the spec doesn't mandate any trap to be delegatable and that
> > we would need a way to detect which ones are delegatable. I think we can
> > safely assume that PAGE FAULT is delegatable and if a hardware that does
> > not have support comes up then it will probably be the vendor
> > responsibility to provide a way to do so.
> > 
> > Link: https://github.com/clementleger/kvm-unit-tests/issues/1 [1]
> > Signed-off-by: Clément Léger <cleger@rivosinc.com>
> > ---
> >  riscv/Makefile            |   1 +
> >  lib/riscv/asm/csr.h       |   1 +
> >  lib/riscv/asm/processor.h |  10 ++
> >  riscv/isa-dbltrp.c        | 210 ++++++++++++++++++++++++++++++++++++++
> >  riscv/unittests.cfg       |   4 +
> >  5 files changed, 226 insertions(+)
> >  create mode 100644 riscv/isa-dbltrp.c
> > 
> > diff --git a/riscv/Makefile b/riscv/Makefile
> > index 11e68eae..d71c9d2e 100644
> > --- a/riscv/Makefile
> > +++ b/riscv/Makefile
> > @@ -14,6 +14,7 @@ tests =
> >  tests += $(TEST_DIR)/sbi.$(exe)
> >  tests += $(TEST_DIR)/selftest.$(exe)
> >  tests += $(TEST_DIR)/sieve.$(exe)
> > +tests += $(TEST_DIR)/isa-dbltrp.$(exe)
> >  
> >  all: $(tests)
> >  
> > diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> > index 3e4b5fca..6a8e0578 100644
> > --- a/lib/riscv/asm/csr.h
> > +++ b/lib/riscv/asm/csr.h
> > @@ -18,6 +18,7 @@
> >  
> >  #define SR_SIE			_AC(0x00000002, UL)
> >  #define SR_SPP			_AC(0x00000100, UL)
> > +#define SR_SDT			_AC(0x01000000, UL) /* Supervisor Double Trap */
> >  
> >  /* Exception cause high bit - is an interrupt if set */
> >  #define CAUSE_IRQ_FLAG		(_AC(1, UL) << (__riscv_xlen - 1))
> > diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
> > index 631ce226..a3dab064 100644
> > --- a/lib/riscv/asm/processor.h
> > +++ b/lib/riscv/asm/processor.h
> > @@ -50,6 +50,16 @@ static inline void ipi_ack(void)
> >  	csr_clear(CSR_SIP, IE_SSIE);
> >  }
> >  
> > +static inline void local_dlbtrp_enable(void)
> > +{
> > +	csr_set(CSR_SSTATUS, SR_SDT);
> > +}
> > +
> > +static inline void local_dlbtrp_disable(void)
> > +{
> > +	csr_clear(CSR_SSTATUS, SR_SDT);
> > +}
> > +
> >  void install_exception_handler(unsigned long cause, void (*handler)(struct pt_regs *));
> >  void install_irq_handler(unsigned long cause, void (*handler)(struct pt_regs *));
> >  void do_handle_exception(struct pt_regs *regs);
> > diff --git a/riscv/isa-dbltrp.c b/riscv/isa-dbltrp.c
> > new file mode 100644
> > index 00000000..dcfa66da
> > --- /dev/null
> > +++ b/riscv/isa-dbltrp.c
> > @@ -0,0 +1,210 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SBI verification
> > + *
> > + * Copyright (C) 2025, Rivos Inc., Clément Léger <cleger@rivosinc.com>
> > + */
> > +#include <alloc.h>
> > +#include <alloc_page.h>
> > +#include <libcflat.h>
> > +#include <stdlib.h>
> > +
> > +#include <asm/csr.h>
> > +#include <asm/page.h>
> > +#include <asm/processor.h>
> > +#include <asm/ptrace.h>
> > +#include <asm/sbi.h>
> > +
> > +#include <sbi-tests.h>
> > +
> > +static bool double_trap;
> > +static bool clear_sdt;
> > +
> > +#define INSN_LEN(insn)			((((insn) & 0x3) < 0x3) ? 2 : 4)
> 
> This macro should be removed since it was merged in another file.

Actually it should be removed since it's unused. If it was used, then we'd
need to rename the callsites since we call it RV_INSN_LEN.

I've removed it while applying to riscv/sbi.

https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi

Thanks,
drew

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests v3 0/2] riscv: Add double trap testing
  2025-06-16 11:58 [kvm-unit-tests v3 0/2] riscv: Add double trap testing Clément Léger
  2025-06-16 11:58 ` [kvm-unit-tests v3 1/2] lib/riscv: export FWFT functions Clément Léger
  2025-06-16 11:59 ` [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing Clément Léger
@ 2025-07-02 14:48 ` Andrew Jones
  2 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2025-07-02 14:48 UTC (permalink / raw)
  To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones

On Mon, Jun 16, 2025 at 01:58:58PM +0200, Clément Léger wrote:
> Add a test that triggers double trap and verify that it's behavior
> conforms to the spec. Also use SSE to verify that an SSE event is
> correctly sent upon double trap.
> 
> In order to run this test, one can use the following command using an
> upstream version of OpenSBI:
> 
> $ QEMU=qemu-system-riscv64 \
>   FIRMWARE_OVERRIDE=<opensbi>/fw_dynamic.bin \
>   ./riscv-run riscv/isa-dbltrp.flat
> 
> ---
> 
> v3:
>  - Return an error only if SSE event wasn't unregistered successfully
> 
> v2:
>  - Use WRITE_ONCE/READ_ONCE for shared variables
>  - Remove locking flag for last test
>  - Fix a few typos
>  - Skip crash test if env var DOUBLE_TRAP_TEST_CRASH isn't set
>  - Skip crash test if SSE event unregistering failed
>  - Remove SDT clearing patch
>  - Fix wrong check using ret.value nstead of ret.error
> 
> Clément Léger (2):
>   lib/riscv: export FWFT functions
>   riscv: Add ISA double trap extension testing
> 
>  riscv/Makefile            |   1 +
>  lib/riscv/asm/csr.h       |   1 +
>  lib/riscv/asm/processor.h |  10 ++
>  lib/riscv/asm/sbi.h       |   5 +
>  lib/riscv/sbi.c           |  20 ++++
>  riscv/isa-dbltrp.c        | 210 ++++++++++++++++++++++++++++++++++++++
>  riscv/sbi-fwft.c          |  49 +++------
>  riscv/unittests.cfg       |   4 +
>  8 files changed, 265 insertions(+), 35 deletions(-)
>  create mode 100644 riscv/isa-dbltrp.c
> 
> -- 
> 2.49.0
>

Merged. Thanks

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-02 16:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-16 11:58 [kvm-unit-tests v3 0/2] riscv: Add double trap testing Clément Léger
2025-06-16 11:58 ` [kvm-unit-tests v3 1/2] lib/riscv: export FWFT functions Clément Léger
2025-06-16 11:59 ` [kvm-unit-tests v3 2/2] riscv: Add ISA double trap extension testing Clément Léger
2025-06-19  7:59   ` Clément Léger
2025-06-23 16:29     ` Andrew Jones
2025-07-02 14:48 ` [kvm-unit-tests v3 0/2] riscv: Add double trap testing Andrew Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).