On 03/06/2025 11:09, Andrew Jones wrote: > On Fri, May 23, 2025 at 09:53:10AM +0200, 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 delagatable 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@xxxxxxxxxxxx> >> --- >> riscv/Makefile | 1 + >> lib/riscv/asm/csr.h | 1 + >> riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ >> riscv/unittests.cfg | 5 ++ >> 4 files changed, 196 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/riscv/isa-dbltrp.c b/riscv/isa-dbltrp.c >> new file mode 100644 >> index 00000000..174aee2a >> --- /dev/null >> +++ b/riscv/isa-dbltrp.c >> @@ -0,0 +1,189 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * SBI verification >> + * >> + * Copyright (C) 2025, Rivos Inc., Clément Léger <cleger@xxxxxxxxxxxx> >> + */ >> +#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 set_sdt = true; >> + >> +#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"); \ > > nit: need some spaces > > "r" (value), "r" (ptr) > >> +} while (0) >> + >> +static void syscall_trap_handler(struct pt_regs *regs) > > This is a page fault handler, not a syscall. > >> +{ >> + if (set_sdt) >> + csr_set(CSR_SSTATUS, SR_SDT); >> + >> + if (double_trap) { >> + double_trap = false; >> + GEN_TRAP(); >> + } >> + >> + /* Skip trapping instruction */ >> + regs->epc += 4; >> +} >> + >> +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); >> + >> + sse_dbltrp_called = true; >> + >> + /* Skip trapping instruction */ >> + regs->epc += 4; >> +} >> + >> +static void sse_double_trap(void) >> +{ >> + struct sbiret ret; >> + >> + 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")) >> + goto out; > > The unmasking failed, but the out label takes us to a mask. Hi Drew, Yeah masking it even if unmask wasn't so important since it's the boot state. I'll add a separate label though. > >> + >> + 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; >> + } >> + 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. > > This is something to raise in a tech-prs call, because it sounds like we > need another paragraph for FWFT which states when DOUBLE_TRAP is enabled > and SSE is supported that SSE local double trap events will be raised. Or, > we need another FWFT feature that allows S-mode to request that behavior > be enabled/disabled when SSE is supported (and M-mode can decide yes/no > to that request). That's a good point, I'll submit a patch to modify the SBI doc in that way. > >> + */ >> + set_sdt = true; >> + double_trap = true; > > WRITE_ONCE(set_sdt, true); > WRITE_ONCE(double_trap, true); > >> + GEN_TRAP(); >> + >> + report(sse_dbltrp_called, "SSE double trap event generated"); > > READ_ONCE(sse_dbltrp_called) > >> + >> + 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); >> + sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap unregister"); >> + >> +out: >> + sbi_sse_hart_mask(); >> + free_page(handler_arg.stack - PAGE_SIZE); >> + >> + report_prefix_pop(); >> +} >> + >> +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, syscall_trap_handler); >> + >> + double_trap = 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, 1); > > Why lock it? That's a mistake. > >> + 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 */ >> + set_sdt = false; >> + double_trap = true; > > WRITE_ONCE(set_sdt, true); > WRITE_ONCE(double_trap, true); > >> + GEN_TRAP(); >> + report_pass("Trapped twice allowed ok"); >> + >> + if (sbi_probe(SBI_EXT_SSE)) >> + sse_double_trap(); >> + else >> + report_skip("SSE double trap event will not be tested, extension is not available"); >> + >> + /* >> + * Second time, keep the double trap flag (SDT) and generate another trap, this should > > Third time if we did the SSE test. > >> + * generate a double trap. Since there is no SSE handler registered, it should crash to >> + * M-mode. > > No SSE handler that we know of... sse_double_trap() should return > some error if it fails to unregister and then we should skip this > test in that case. Indeed, good catch. > >> + */ >> + set_sdt = true; >> + double_trap = true; > > WRITE_ONCE(set_sdt, true); > WRITE_ONCE(double_trap, true); > >> + report_info("Should generate a double trap and crash !"); >> + GEN_TRAP(); >> + report_fail("Should have crashed !"); > > nit: Put the '!' next to the last word. > > I think this M-mode crash test should be optional. We can make it optional > on an environment variable since we already use environment variables for > other optional tests. We even have env_or_skip() in riscv/sbi-tests.h for > that purpose. Acked, I'll use env_or_skip(). Thanks, Clément > >> +} >> + >> +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.value == SBI_ERR_NOT_SUPPORTED) { >> + report_skip("SBI_FWFT_DOUBLE_TRAP is not supported !"); > > nit: Put the '!' next to the last word. > >> + 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..757e6027 100644 >> --- a/riscv/unittests.cfg >> +++ b/riscv/unittests.cfg >> @@ -18,3 +18,8 @@ groups = selftest >> file = sbi.flat >> smp = $MAX_SMP >> groups = sbi >> + >> +[dbltrp] >> +file = isa-dbltrp.flat >> +smp = $MAX_SMP > > The test doesn't appear to require multiple harts. > >> +groups = isa > > groups = isa sbi > >> -- >> 2.49.0 >> > > Thanks, > drew
