* [PATCH 0/3] riscv: Add double trap testing @ 2025-05-23 7:53 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue 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-system-riscv64 \ -M virt \ -cpu max \ -nographic -serial mon:stdio \ -bios <opensbi>/fw_dynamic.bin \ -kernel riscv/isa-dbltrp.flat Clément Léger (3): lib/riscv: export FWFT functions lib/riscv: clear SDT when entering exception handling riscv: Add ISA double trap extension testing riscv/Makefile | 1 + lib/riscv/asm/csr.h | 1 + lib/riscv/asm/sbi.h | 5 ++ lib/riscv/sbi.c | 20 +++++ riscv/cstart.S | 9 ++- riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ riscv/sbi-fwft.c | 49 ++++-------- riscv/unittests.cfg | 5 ++ 8 files changed, 240 insertions(+), 39 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] 22+ messages in thread
* [PATCH 0/3] riscv: Add double trap testing @ 2025-05-23 7:53 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue 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-system-riscv64 \ -M virt \ -cpu max \ -nographic -serial mon:stdio \ -bios <opensbi>/fw_dynamic.bin \ -kernel riscv/isa-dbltrp.flat Clément Léger (3): lib/riscv: export FWFT functions lib/riscv: clear SDT when entering exception handling riscv: Add ISA double trap extension testing riscv/Makefile | 1 + lib/riscv/asm/csr.h | 1 + lib/riscv/asm/sbi.h | 5 ++ lib/riscv/sbi.c | 20 +++++ riscv/cstart.S | 9 ++- riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ riscv/sbi-fwft.c | 49 ++++-------- riscv/unittests.cfg | 5 ++ 8 files changed, 240 insertions(+), 39 deletions(-) create mode 100644 riscv/isa-dbltrp.c -- 2.49.0 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] lib/riscv: export FWFT functions 2025-05-23 7:53 ` Clément Léger @ 2025-05-23 7:53 ` Clément Léger -1 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue These functions will probably be needed by other tests as well, expose them. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- 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] 22+ messages in thread
* [PATCH 1/3] lib/riscv: export FWFT functions @ 2025-05-23 7:53 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue These functions will probably be needed by other tests as well, expose them. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] lib/riscv: export FWFT functions 2025-05-23 7:53 ` Clément Léger @ 2025-06-03 7:14 ` Andrew Jones -1 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 7:14 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On Fri, May 23, 2025 at 09:53:08AM +0200, Clément Léger wrote: > These functions will probably be needed by other tests as well, expose > them. s/probably// > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > lib/riscv/asm/sbi.h | 5 +++++ > lib/riscv/sbi.c | 20 ++++++++++++++++++ > riscv/sbi-fwft.c | 49 +++++++++++++-------------------------------- > 3 files changed, 39 insertions(+), 35 deletions(-) Reviewed-by: Andrew Jones <andrew.jones@linux.dev> -- kvm-riscv mailing list kvm-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kvm-riscv ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] lib/riscv: export FWFT functions @ 2025-06-03 7:14 ` Andrew Jones 0 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 7:14 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On Fri, May 23, 2025 at 09:53:08AM +0200, Clément Léger wrote: > These functions will probably be needed by other tests as well, expose > them. s/probably// > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > lib/riscv/asm/sbi.h | 5 +++++ > lib/riscv/sbi.c | 20 ++++++++++++++++++ > riscv/sbi-fwft.c | 49 +++++++++++++-------------------------------- > 3 files changed, 39 insertions(+), 35 deletions(-) Reviewed-by: Andrew Jones <andrew.jones@linux.dev> ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] lib/riscv: clear SDT when entering exception handling 2025-05-23 7:53 ` Clément Léger @ 2025-05-23 7:53 ` Clément Léger -1 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue In order to avoid taking double trap once we have entered a trap and saved everything, clear SDT at the end of entry. This is not exactly required when double trap is disabled (probably most of the time), but that's not harmful. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- riscv/cstart.S | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/riscv/cstart.S b/riscv/cstart.S index 575f929b..a86f97f0 100644 --- a/riscv/cstart.S +++ b/riscv/cstart.S @@ -212,14 +212,15 @@ secondary_entry: REG_S t6, PT_T6(a0) // x31 csrr a1, CSR_SEPC REG_S a1, PT_EPC(a0) - csrr a1, CSR_SSTATUS - REG_S a1, PT_STATUS(a0) csrr a1, CSR_STVAL REG_S a1, PT_BADADDR(a0) csrr a1, CSR_SCAUSE REG_S a1, PT_CAUSE(a0) REG_L a1, PT_ORIG_A0(a0) REG_S a1, PT_A0(a0) + li t0, SR_SDT + csrrc a1, CSR_SSTATUS, t0 + REG_S a1, PT_STATUS(a0) .endm /* @@ -227,6 +228,8 @@ secondary_entry: * Also restores a0. */ .macro restore_context + REG_L a1, PT_STATUS(a0) + csrw CSR_SSTATUS, a1 REG_L ra, PT_RA(a0) // x1 REG_L sp, PT_SP(a0) // x2 REG_L gp, PT_GP(a0) // x3 @@ -260,8 +263,6 @@ secondary_entry: REG_L t6, PT_T6(a0) // x31 REG_L a1, PT_EPC(a0) csrw CSR_SEPC, a1 - REG_L a1, PT_STATUS(a0) - csrw CSR_SSTATUS, a1 REG_L a1, PT_BADADDR(a0) csrw CSR_STVAL, a1 REG_L a1, PT_CAUSE(a0) -- 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] 22+ messages in thread
* [PATCH 2/3] lib/riscv: clear SDT when entering exception handling @ 2025-05-23 7:53 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue In order to avoid taking double trap once we have entered a trap and saved everything, clear SDT at the end of entry. This is not exactly required when double trap is disabled (probably most of the time), but that's not harmful. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- riscv/cstart.S | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/riscv/cstart.S b/riscv/cstart.S index 575f929b..a86f97f0 100644 --- a/riscv/cstart.S +++ b/riscv/cstart.S @@ -212,14 +212,15 @@ secondary_entry: REG_S t6, PT_T6(a0) // x31 csrr a1, CSR_SEPC REG_S a1, PT_EPC(a0) - csrr a1, CSR_SSTATUS - REG_S a1, PT_STATUS(a0) csrr a1, CSR_STVAL REG_S a1, PT_BADADDR(a0) csrr a1, CSR_SCAUSE REG_S a1, PT_CAUSE(a0) REG_L a1, PT_ORIG_A0(a0) REG_S a1, PT_A0(a0) + li t0, SR_SDT + csrrc a1, CSR_SSTATUS, t0 + REG_S a1, PT_STATUS(a0) .endm /* @@ -227,6 +228,8 @@ secondary_entry: * Also restores a0. */ .macro restore_context + REG_L a1, PT_STATUS(a0) + csrw CSR_SSTATUS, a1 REG_L ra, PT_RA(a0) // x1 REG_L sp, PT_SP(a0) // x2 REG_L gp, PT_GP(a0) // x3 @@ -260,8 +263,6 @@ secondary_entry: REG_L t6, PT_T6(a0) // x31 REG_L a1, PT_EPC(a0) csrw CSR_SEPC, a1 - REG_L a1, PT_STATUS(a0) - csrw CSR_SSTATUS, a1 REG_L a1, PT_BADADDR(a0) csrw CSR_STVAL, a1 REG_L a1, PT_CAUSE(a0) -- 2.49.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] lib/riscv: clear SDT when entering exception handling 2025-05-23 7:53 ` Clément Léger @ 2025-06-03 8:16 ` Andrew Jones -1 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 8:16 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On Fri, May 23, 2025 at 09:53:09AM +0200, Clément Léger wrote: > In order to avoid taking double trap once we have entered a trap and > saved everything, clear SDT at the end of entry. This is not exactly > required when double trap is disabled (probably most of the time), but > that's not harmful. Hmm... I wonder if this shouldn't be left to the handlers. Maybe we should just provide a couple helpers in processor.h, such as local_dlbtrp_enable() local_dlbtrp_disable() If we do need to manage this at save_context time, then I have a couple comments below > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > riscv/cstart.S | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/riscv/cstart.S b/riscv/cstart.S > index 575f929b..a86f97f0 100644 > --- a/riscv/cstart.S > +++ b/riscv/cstart.S > @@ -212,14 +212,15 @@ secondary_entry: > REG_S t6, PT_T6(a0) // x31 > csrr a1, CSR_SEPC > REG_S a1, PT_EPC(a0) > - csrr a1, CSR_SSTATUS > - REG_S a1, PT_STATUS(a0) > csrr a1, CSR_STVAL > REG_S a1, PT_BADADDR(a0) > csrr a1, CSR_SCAUSE > REG_S a1, PT_CAUSE(a0) > REG_L a1, PT_ORIG_A0(a0) > REG_S a1, PT_A0(a0) > + li t0, SR_SDT ^ ^ should not be a tab ^ should be tabs SR_SDT isn't defined until the next patch so this breaks compiling at this point, which could break bisection. You can do a quick check of a series for this with git rebase -i -x 'make' <base> > + csrrc a1, CSR_SSTATUS, t0 > + REG_S a1, PT_STATUS(a0) > .endm > > /* > @@ -227,6 +228,8 @@ secondary_entry: > * Also restores a0. > */ > .macro restore_context > + REG_L a1, PT_STATUS(a0) > + csrw CSR_SSTATUS, a1 > REG_L ra, PT_RA(a0) // x1 > REG_L sp, PT_SP(a0) // x2 > REG_L gp, PT_GP(a0) // x3 > @@ -260,8 +263,6 @@ secondary_entry: > REG_L t6, PT_T6(a0) // x31 > REG_L a1, PT_EPC(a0) > csrw CSR_SEPC, a1 > - REG_L a1, PT_STATUS(a0) > - csrw CSR_SSTATUS, a1 > REG_L a1, PT_BADADDR(a0) > csrw CSR_STVAL, a1 > REG_L a1, PT_CAUSE(a0) > -- > 2.49.0 > Thanks, drew -- kvm-riscv mailing list kvm-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kvm-riscv ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] lib/riscv: clear SDT when entering exception handling @ 2025-06-03 8:16 ` Andrew Jones 0 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 8:16 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On Fri, May 23, 2025 at 09:53:09AM +0200, Clément Léger wrote: > In order to avoid taking double trap once we have entered a trap and > saved everything, clear SDT at the end of entry. This is not exactly > required when double trap is disabled (probably most of the time), but > that's not harmful. Hmm... I wonder if this shouldn't be left to the handlers. Maybe we should just provide a couple helpers in processor.h, such as local_dlbtrp_enable() local_dlbtrp_disable() If we do need to manage this at save_context time, then I have a couple comments below > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > riscv/cstart.S | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/riscv/cstart.S b/riscv/cstart.S > index 575f929b..a86f97f0 100644 > --- a/riscv/cstart.S > +++ b/riscv/cstart.S > @@ -212,14 +212,15 @@ secondary_entry: > REG_S t6, PT_T6(a0) // x31 > csrr a1, CSR_SEPC > REG_S a1, PT_EPC(a0) > - csrr a1, CSR_SSTATUS > - REG_S a1, PT_STATUS(a0) > csrr a1, CSR_STVAL > REG_S a1, PT_BADADDR(a0) > csrr a1, CSR_SCAUSE > REG_S a1, PT_CAUSE(a0) > REG_L a1, PT_ORIG_A0(a0) > REG_S a1, PT_A0(a0) > + li t0, SR_SDT ^ ^ should not be a tab ^ should be tabs SR_SDT isn't defined until the next patch so this breaks compiling at this point, which could break bisection. You can do a quick check of a series for this with git rebase -i -x 'make' <base> > + csrrc a1, CSR_SSTATUS, t0 > + REG_S a1, PT_STATUS(a0) > .endm > > /* > @@ -227,6 +228,8 @@ secondary_entry: > * Also restores a0. > */ > .macro restore_context > + REG_L a1, PT_STATUS(a0) > + csrw CSR_SSTATUS, a1 > REG_L ra, PT_RA(a0) // x1 > REG_L sp, PT_SP(a0) // x2 > REG_L gp, PT_GP(a0) // x3 > @@ -260,8 +263,6 @@ secondary_entry: > REG_L t6, PT_T6(a0) // x31 > REG_L a1, PT_EPC(a0) > csrw CSR_SEPC, a1 > - REG_L a1, PT_STATUS(a0) > - csrw CSR_SSTATUS, a1 > REG_L a1, PT_BADADDR(a0) > csrw CSR_STVAL, a1 > REG_L a1, PT_CAUSE(a0) > -- > 2.49.0 > Thanks, drew ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] lib/riscv: clear SDT when entering exception handling 2025-06-03 8:16 ` Andrew Jones @ 2025-06-03 8:22 ` Clément Léger -1 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-06-03 8:22 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On 03/06/2025 10:16, Andrew Jones wrote: > On Fri, May 23, 2025 at 09:53:09AM +0200, Clément Léger wrote: >> In order to avoid taking double trap once we have entered a trap and >> saved everything, clear SDT at the end of entry. This is not exactly >> required when double trap is disabled (probably most of the time), but >> that's not harmful. > > Hmm... I wonder if this shouldn't be left to the handlers. Maybe > we should just provide a couple helpers in processor.h, such as > > local_dlbtrp_enable() > local_dlbtrp_disable() Hi Drew, Yeah indeed, that makes sense, some of them might have to save more info before re-enabling trap in a near future. I'll drop this patch and use that proposal. > > If we do need to manage this at save_context time, then I have > a couple comments below > >> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> --- >> riscv/cstart.S | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/riscv/cstart.S b/riscv/cstart.S >> index 575f929b..a86f97f0 100644 >> --- a/riscv/cstart.S >> +++ b/riscv/cstart.S >> @@ -212,14 +212,15 @@ secondary_entry: >> REG_S t6, PT_T6(a0) // x31 >> csrr a1, CSR_SEPC >> REG_S a1, PT_EPC(a0) >> - csrr a1, CSR_SSTATUS >> - REG_S a1, PT_STATUS(a0) >> csrr a1, CSR_STVAL >> REG_S a1, PT_BADADDR(a0) >> csrr a1, CSR_SCAUSE >> REG_S a1, PT_CAUSE(a0) >> REG_L a1, PT_ORIG_A0(a0) >> REG_S a1, PT_A0(a0) >> + li t0, SR_SDT > ^ ^ should not be a tab > ^ should be tabs > > SR_SDT isn't defined until the next patch so this breaks compiling at this > point, which could break bisection. You can do a quick check of a series > for this with > > git rebase -i -x 'make' <base> Missed that, sorry, I'll check next time. Thanks, Clément > >> + csrrc a1, CSR_SSTATUS, t0 >> + REG_S a1, PT_STATUS(a0) >> .endm >> >> /* >> @@ -227,6 +228,8 @@ secondary_entry: >> * Also restores a0. >> */ >> .macro restore_context >> + REG_L a1, PT_STATUS(a0) >> + csrw CSR_SSTATUS, a1 >> REG_L ra, PT_RA(a0) // x1 >> REG_L sp, PT_SP(a0) // x2 >> REG_L gp, PT_GP(a0) // x3 >> @@ -260,8 +263,6 @@ secondary_entry: >> REG_L t6, PT_T6(a0) // x31 >> REG_L a1, PT_EPC(a0) >> csrw CSR_SEPC, a1 >> - REG_L a1, PT_STATUS(a0) >> - csrw CSR_SSTATUS, a1 >> REG_L a1, PT_BADADDR(a0) >> csrw CSR_STVAL, a1 >> REG_L a1, PT_CAUSE(a0) >> -- >> 2.49.0 >> > > Thanks, > drew -- kvm-riscv mailing list kvm-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kvm-riscv ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] lib/riscv: clear SDT when entering exception handling @ 2025-06-03 8:22 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-06-03 8:22 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On 03/06/2025 10:16, Andrew Jones wrote: > On Fri, May 23, 2025 at 09:53:09AM +0200, Clément Léger wrote: >> In order to avoid taking double trap once we have entered a trap and >> saved everything, clear SDT at the end of entry. This is not exactly >> required when double trap is disabled (probably most of the time), but >> that's not harmful. > > Hmm... I wonder if this shouldn't be left to the handlers. Maybe > we should just provide a couple helpers in processor.h, such as > > local_dlbtrp_enable() > local_dlbtrp_disable() Hi Drew, Yeah indeed, that makes sense, some of them might have to save more info before re-enabling trap in a near future. I'll drop this patch and use that proposal. > > If we do need to manage this at save_context time, then I have > a couple comments below > >> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> --- >> riscv/cstart.S | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/riscv/cstart.S b/riscv/cstart.S >> index 575f929b..a86f97f0 100644 >> --- a/riscv/cstart.S >> +++ b/riscv/cstart.S >> @@ -212,14 +212,15 @@ secondary_entry: >> REG_S t6, PT_T6(a0) // x31 >> csrr a1, CSR_SEPC >> REG_S a1, PT_EPC(a0) >> - csrr a1, CSR_SSTATUS >> - REG_S a1, PT_STATUS(a0) >> csrr a1, CSR_STVAL >> REG_S a1, PT_BADADDR(a0) >> csrr a1, CSR_SCAUSE >> REG_S a1, PT_CAUSE(a0) >> REG_L a1, PT_ORIG_A0(a0) >> REG_S a1, PT_A0(a0) >> + li t0, SR_SDT > ^ ^ should not be a tab > ^ should be tabs > > SR_SDT isn't defined until the next patch so this breaks compiling at this > point, which could break bisection. You can do a quick check of a series > for this with > > git rebase -i -x 'make' <base> Missed that, sorry, I'll check next time. Thanks, Clément > >> + csrrc a1, CSR_SSTATUS, t0 >> + REG_S a1, PT_STATUS(a0) >> .endm >> >> /* >> @@ -227,6 +228,8 @@ secondary_entry: >> * Also restores a0. >> */ >> .macro restore_context >> + REG_L a1, PT_STATUS(a0) >> + csrw CSR_SSTATUS, a1 >> REG_L ra, PT_RA(a0) // x1 >> REG_L sp, PT_SP(a0) // x2 >> REG_L gp, PT_GP(a0) // x3 >> @@ -260,8 +263,6 @@ secondary_entry: >> REG_L t6, PT_T6(a0) // x31 >> REG_L a1, PT_EPC(a0) >> csrw CSR_SEPC, a1 >> - REG_L a1, PT_STATUS(a0) >> - csrw CSR_SSTATUS, a1 >> REG_L a1, PT_BADADDR(a0) >> csrw CSR_STVAL, a1 >> REG_L a1, PT_CAUSE(a0) >> -- >> 2.49.0 >> > > Thanks, > drew ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] riscv: Add ISA double trap extension testing 2025-05-23 7:53 ` Clément Léger @ 2025-05-23 7:53 ` Clément Léger -1 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue 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@rivosinc.com> --- 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@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 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"); \ +} while (0) + +static void syscall_trap_handler(struct pt_regs *regs) +{ + 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; + + 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. + */ + set_sdt = true; + double_trap = true; + GEN_TRAP(); + + report(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"); + +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; + GEN_TRAP(); + report_pass("Double trap disabled, trap first time ok"); + + /* Enable double trap */ + ret = sbi_fwft_set(SBI_FWFT_DOUBLE_TRAP, 1, 1); + 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; + 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 + * generate a double trap. Since there is no SSE handler registered, it should crash to + * M-mode. + */ + set_sdt = true; + 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.value == 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..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 +groups = isa -- 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] 22+ messages in thread
* [PATCH 3/3] riscv: Add ISA double trap extension testing @ 2025-05-23 7:53 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-05-23 7:53 UTC (permalink / raw) To: kvm, kvm-riscv; +Cc: Clément Léger, Andrew Jones, Ved Shanbhogue 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@rivosinc.com> --- 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@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 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"); \ +} while (0) + +static void syscall_trap_handler(struct pt_regs *regs) +{ + 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; + + 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. + */ + set_sdt = true; + double_trap = true; + GEN_TRAP(); + + report(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"); + +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; + GEN_TRAP(); + report_pass("Double trap disabled, trap first time ok"); + + /* Enable double trap */ + ret = sbi_fwft_set(SBI_FWFT_DOUBLE_TRAP, 1, 1); + 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; + 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 + * generate a double trap. Since there is no SSE handler registered, it should crash to + * M-mode. + */ + set_sdt = true; + 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.value == 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..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 +groups = isa -- 2.49.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] riscv: Add ISA double trap extension testing 2025-05-23 7:53 ` Clément Léger @ 2025-06-03 9:09 ` Andrew Jones -1 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 9:09 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue 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@rivosinc.com> > --- > 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@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 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. > + > + 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). > + */ > + 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? > + 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. > + */ > + 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. > +} > + > +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 -- kvm-riscv mailing list kvm-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kvm-riscv ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] riscv: Add ISA double trap extension testing @ 2025-06-03 9:09 ` Andrew Jones 0 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 9:09 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue 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@rivosinc.com> > --- > 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@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 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. > + > + 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). > + */ > + 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? > + 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. > + */ > + 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. > +} > + > +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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] riscv: Add ISA double trap extension testing 2025-06-03 9:09 ` Andrew Jones @ 2025-06-03 9:39 ` Clément Léger -1 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-06-03 9:39 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue 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@rivosinc.com> >> --- >> 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@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 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 -- kvm-riscv mailing list kvm-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kvm-riscv ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] riscv: Add ISA double trap extension testing @ 2025-06-03 9:39 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-06-03 9:39 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue 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@rivosinc.com> >> --- >> 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@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 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] riscv: Add double trap testing 2025-05-23 7:53 ` Clément Léger @ 2025-06-03 7:10 ` Andrew Jones -1 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 7:10 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue Hi Clement, You may want to add format.subjectprefix = kvm-unit-tests to your git config, since it's missing from this series. On Fri, May 23, 2025 at 09:53:07AM +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-system-riscv64 \ > -M virt \ > -cpu max \ > -nographic -serial mon:stdio \ > -bios <opensbi>/fw_dynamic.bin \ > -kernel riscv/isa-dbltrp.flat You can also do $ QEMU=qemu-system-riscv64 FIRMWARE_OVERRIDE=<opensbi>/fw_dynamic.bin ./riscv-run riscv/isa-dbltrp.flat Thanks, drew > > Clément Léger (3): > lib/riscv: export FWFT functions > lib/riscv: clear SDT when entering exception handling > riscv: Add ISA double trap extension testing > > riscv/Makefile | 1 + > lib/riscv/asm/csr.h | 1 + > lib/riscv/asm/sbi.h | 5 ++ > lib/riscv/sbi.c | 20 +++++ > riscv/cstart.S | 9 ++- > riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ > riscv/sbi-fwft.c | 49 ++++-------- > riscv/unittests.cfg | 5 ++ > 8 files changed, 240 insertions(+), 39 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] 22+ messages in thread
* Re: [PATCH 0/3] riscv: Add double trap testing @ 2025-06-03 7:10 ` Andrew Jones 0 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2025-06-03 7:10 UTC (permalink / raw) To: Clément Léger; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue Hi Clement, You may want to add format.subjectprefix = kvm-unit-tests to your git config, since it's missing from this series. On Fri, May 23, 2025 at 09:53:07AM +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-system-riscv64 \ > -M virt \ > -cpu max \ > -nographic -serial mon:stdio \ > -bios <opensbi>/fw_dynamic.bin \ > -kernel riscv/isa-dbltrp.flat You can also do $ QEMU=qemu-system-riscv64 FIRMWARE_OVERRIDE=<opensbi>/fw_dynamic.bin ./riscv-run riscv/isa-dbltrp.flat Thanks, drew > > Clément Léger (3): > lib/riscv: export FWFT functions > lib/riscv: clear SDT when entering exception handling > riscv: Add ISA double trap extension testing > > riscv/Makefile | 1 + > lib/riscv/asm/csr.h | 1 + > lib/riscv/asm/sbi.h | 5 ++ > lib/riscv/sbi.c | 20 +++++ > riscv/cstart.S | 9 ++- > riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ > riscv/sbi-fwft.c | 49 ++++-------- > riscv/unittests.cfg | 5 ++ > 8 files changed, 240 insertions(+), 39 deletions(-) > create mode 100644 riscv/isa-dbltrp.c > > -- > 2.49.0 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] riscv: Add double trap testing 2025-06-03 7:10 ` Andrew Jones @ 2025-06-03 7:52 ` Clément Léger -1 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-06-03 7:52 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On 03/06/2025 09:10, Andrew Jones wrote: > > Hi Clement, > > You may want to add format.subjectprefix = kvm-unit-tests to your git > config, since it's missing from this series. Hi Drew, Yeah sorry forgot the prefix ! > > On Fri, May 23, 2025 at 09:53:07AM +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-system-riscv64 \ >> -M virt \ >> -cpu max \ >> -nographic -serial mon:stdio \ >> -bios <opensbi>/fw_dynamic.bin \ >> -kernel riscv/isa-dbltrp.flat > > You can also do > > $ QEMU=qemu-system-riscv64 FIRMWARE_OVERRIDE=<opensbi>/fw_dynamic.bin ./riscv-run riscv/isa-dbltrp.flat Oh yeah I even used that as well... Thanks, Clément > > Thanks, > drew > >> >> Clément Léger (3): >> lib/riscv: export FWFT functions >> lib/riscv: clear SDT when entering exception handling >> riscv: Add ISA double trap extension testing >> >> riscv/Makefile | 1 + >> lib/riscv/asm/csr.h | 1 + >> lib/riscv/asm/sbi.h | 5 ++ >> lib/riscv/sbi.c | 20 +++++ >> riscv/cstart.S | 9 ++- >> riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ >> riscv/sbi-fwft.c | 49 ++++-------- >> riscv/unittests.cfg | 5 ++ >> 8 files changed, 240 insertions(+), 39 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] 22+ messages in thread
* Re: [PATCH 0/3] riscv: Add double trap testing @ 2025-06-03 7:52 ` Clément Léger 0 siblings, 0 replies; 22+ messages in thread From: Clément Léger @ 2025-06-03 7:52 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Ved Shanbhogue On 03/06/2025 09:10, Andrew Jones wrote: > > Hi Clement, > > You may want to add format.subjectprefix = kvm-unit-tests to your git > config, since it's missing from this series. Hi Drew, Yeah sorry forgot the prefix ! > > On Fri, May 23, 2025 at 09:53:07AM +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-system-riscv64 \ >> -M virt \ >> -cpu max \ >> -nographic -serial mon:stdio \ >> -bios <opensbi>/fw_dynamic.bin \ >> -kernel riscv/isa-dbltrp.flat > > You can also do > > $ QEMU=qemu-system-riscv64 FIRMWARE_OVERRIDE=<opensbi>/fw_dynamic.bin ./riscv-run riscv/isa-dbltrp.flat Oh yeah I even used that as well... Thanks, Clément > > Thanks, > drew > >> >> Clément Léger (3): >> lib/riscv: export FWFT functions >> lib/riscv: clear SDT when entering exception handling >> riscv: Add ISA double trap extension testing >> >> riscv/Makefile | 1 + >> lib/riscv/asm/csr.h | 1 + >> lib/riscv/asm/sbi.h | 5 ++ >> lib/riscv/sbi.c | 20 +++++ >> riscv/cstart.S | 9 ++- >> riscv/isa-dbltrp.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ >> riscv/sbi-fwft.c | 49 ++++-------- >> riscv/unittests.cfg | 5 ++ >> 8 files changed, 240 insertions(+), 39 deletions(-) >> create mode 100644 riscv/isa-dbltrp.c >> >> -- >> 2.49.0 >> ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-06-03 9:41 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-23 7:53 [PATCH 0/3] riscv: Add double trap testing Clément Léger 2025-05-23 7:53 ` Clément Léger 2025-05-23 7:53 ` [PATCH 1/3] lib/riscv: export FWFT functions Clément Léger 2025-05-23 7:53 ` Clément Léger 2025-06-03 7:14 ` Andrew Jones 2025-06-03 7:14 ` Andrew Jones 2025-05-23 7:53 ` [PATCH 2/3] lib/riscv: clear SDT when entering exception handling Clément Léger 2025-05-23 7:53 ` Clément Léger 2025-06-03 8:16 ` Andrew Jones 2025-06-03 8:16 ` Andrew Jones 2025-06-03 8:22 ` Clément Léger 2025-06-03 8:22 ` Clément Léger 2025-05-23 7:53 ` [PATCH 3/3] riscv: Add ISA double trap extension testing Clément Léger 2025-05-23 7:53 ` Clément Léger 2025-06-03 9:09 ` Andrew Jones 2025-06-03 9:09 ` Andrew Jones 2025-06-03 9:39 ` Clément Léger 2025-06-03 9:39 ` Clément Léger 2025-06-03 7:10 ` [PATCH 0/3] riscv: Add double trap testing Andrew Jones 2025-06-03 7:10 ` Andrew Jones 2025-06-03 7:52 ` Clément Léger 2025-06-03 7:52 ` Clément Léger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.