[kvm-unit-tests v2 2/2] riscv: Add ISA double trap extension testing
Andrew Jones
andrew.jones at linux.dev
Tue Jun 3 09:25:59 PDT 2025
On Tue, Jun 03, 2025 at 05:46:50PM +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 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 at rivosinc.com>
> ---
> riscv/Makefile | 1 +
> lib/riscv/asm/csr.h | 1 +
> lib/riscv/asm/processor.h | 10 ++
> riscv/isa-dbltrp.c | 211 ++++++++++++++++++++++++++++++++++++++
> riscv/unittests.cfg | 4 +
> 5 files changed, 227 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 40104272..87a41312 100644
> --- a/lib/riscv/asm/processor.h
> +++ b/lib/riscv/asm/processor.h
> @@ -48,6 +48,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..a4545096
> --- /dev/null
> +++ b/riscv/isa-dbltrp.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SBI verification
> + *
> + * Copyright (C) 2025, Rivos Inc., Clément Léger <cleger at 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")) {
> + err = ret.error;
I'm not sure we need to return an error for this case. We won't be leaving
an SSE event handler registered.
> + 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);
> + sbiret_report_error(&ret, SBI_SUCCESS, "SSE double trap unregister");
Needs to be
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");
Missing return
> + }
How about rearranging as
if (!sbi_probe(SBI_EXT_SSE)) {
report_skip("SSE double trap event will not be tested, extension is not available");
return;
}
if (sse_double_trap()) {
report_skip("Could not correctly unregister SSE event, skipping last test");
return;
}
> +
> + 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
>
Thanks,
drew
More information about the kvm-riscv
mailing list