* [kvm-unit-tests PATCH 0/2] Add support for SBI FWFT extension testing @ 2025-01-06 15:53 Clément Léger 2025-01-06 15:53 ` [kvm-unit-tests PATCH 1/2] riscv: Add "-deps" handling for tests Clément Léger 2025-01-06 15:53 ` [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension Clément Léger 0 siblings, 2 replies; 5+ messages in thread From: Clément Léger @ 2025-01-06 15:53 UTC (permalink / raw) To: kvm, kvm-riscv Cc: Clément Léger, Andrew Jones, Anup Patel, Atish Patra This series adds a minimal set of tests for the FWFT extension. Reserved range as well as misaligned exception delegation. A commit coming from the SSE tests series is also included in this series to add -deps makefile notation. Clément Léger (2): riscv: Add "-deps" handling for tests riscv: Add tests for SBI FWFT extension riscv/Makefile | 8 ++- lib/riscv/asm/sbi.h | 31 +++++++++ riscv/sbi-fwft.c | 153 ++++++++++++++++++++++++++++++++++++++++++++ riscv/sbi.c | 3 + 4 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 riscv/sbi-fwft.c -- 2.47.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [kvm-unit-tests PATCH 1/2] riscv: Add "-deps" handling for tests 2025-01-06 15:53 [kvm-unit-tests PATCH 0/2] Add support for SBI FWFT extension testing Clément Léger @ 2025-01-06 15:53 ` Clément Léger 2025-01-06 15:53 ` [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension Clément Léger 1 sibling, 0 replies; 5+ messages in thread From: Clément Léger @ 2025-01-06 15:53 UTC (permalink / raw) To: kvm, kvm-riscv Cc: Clément Léger, Andrew Jones, Anup Patel, Atish Patra, Andrew Jones Some tests uses additional files that needs to be linked in the final binary. This is the case for asm-sbi.S which is only used by the sbi test. Add a "-deps" per test variable that allows to designate additional .o files. Reviewed-by: Andrew Jones <andrew.jones@linux.dev> Signed-off-by: Clément Léger <cleger@rivosinc.com> --- riscv/Makefile | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/riscv/Makefile b/riscv/Makefile index 28b04156..5b5e157c 100644 --- a/riscv/Makefile +++ b/riscv/Makefile @@ -17,6 +17,8 @@ tests += $(TEST_DIR)/sieve.$(exe) all: $(tests) +$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o + # When built for EFI sieve needs extra memory, run with e.g. '-m 256' on QEMU $(TEST_DIR)/sieve.$(exe): AUXFLAGS = 0x1 @@ -44,7 +46,6 @@ cflatobjs += lib/riscv/timer.o ifeq ($(ARCH),riscv32) cflatobjs += lib/ldiv32.o endif -cflatobjs += riscv/sbi-asm.o ######################################## @@ -93,6 +94,7 @@ include $(SRCDIR)/scripts/asm-offsets.mak $(CC) $(CFLAGS) -c -o $@ $< \ -DPROGNAME=\"$(notdir $(@:.aux.o=.$(exe)))\" -DAUXFLAGS=$(AUXFLAGS) +.SECONDEXPANSION: ifeq ($(CONFIG_EFI),y) # avoid jump tables before all relocations have been processed riscv/efi/reloc_riscv64.o: CFLAGS += -fno-jump-tables @@ -103,7 +105,7 @@ cflatobjs += lib/efi.o .PRECIOUS: %.so %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined -%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o +%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o $$($$*-deps) $(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \ $(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS) @@ -119,7 +121,7 @@ cflatobjs += lib/efi.o -O binary $^ $@ else %.elf: LDFLAGS += -pie -n -z notext -%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o +%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o $$($$*-deps) $(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \ $(filter %.o, $^) $(FLATLIBS) @chmod a-x $@ -- 2.47.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension 2025-01-06 15:53 [kvm-unit-tests PATCH 0/2] Add support for SBI FWFT extension testing Clément Léger 2025-01-06 15:53 ` [kvm-unit-tests PATCH 1/2] riscv: Add "-deps" handling for tests Clément Léger @ 2025-01-06 15:53 ` Clément Léger 2025-01-15 12:58 ` Andrew Jones 1 sibling, 1 reply; 5+ messages in thread From: Clément Léger @ 2025-01-06 15:53 UTC (permalink / raw) To: kvm, kvm-riscv Cc: Clément Léger, Andrew Jones, Anup Patel, Atish Patra This commit add tests for a the FWFT SBI extension. Currently, only the reserved range as well as the misaligned exception delegation. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- riscv/Makefile | 2 +- lib/riscv/asm/sbi.h | 31 +++++++++ riscv/sbi-fwft.c | 153 ++++++++++++++++++++++++++++++++++++++++++++ riscv/sbi.c | 3 + 4 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 riscv/sbi-fwft.c diff --git a/riscv/Makefile b/riscv/Makefile index 5b5e157c..52718f3f 100644 --- a/riscv/Makefile +++ b/riscv/Makefile @@ -17,7 +17,7 @@ tests += $(TEST_DIR)/sieve.$(exe) all: $(tests) -$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o +$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o $(TEST_DIR)/sbi-fwft.o # When built for EFI sieve needs extra memory, run with e.g. '-m 256' on QEMU $(TEST_DIR)/sieve.$(exe): AUXFLAGS = 0x1 diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h index 98a9b097..27e6fcdb 100644 --- a/lib/riscv/asm/sbi.h +++ b/lib/riscv/asm/sbi.h @@ -11,6 +11,9 @@ #define SBI_ERR_ALREADY_AVAILABLE -6 #define SBI_ERR_ALREADY_STARTED -7 #define SBI_ERR_ALREADY_STOPPED -8 +#define SBI_ERR_NO_SHMEM -9 +#define SBI_ERR_INVALID_STATE -10 +#define SBI_ERR_BAD_RANGE -11 #ifndef __ASSEMBLY__ #include <cpumask.h> @@ -23,6 +26,7 @@ enum sbi_ext_id { SBI_EXT_SRST = 0x53525354, SBI_EXT_DBCN = 0x4442434E, SBI_EXT_SUSP = 0x53555350, + SBI_EXT_FWFT = 0x46574654, }; enum sbi_ext_base_fid { @@ -71,6 +75,33 @@ enum sbi_ext_dbcn_fid { SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, }; +/* SBI function IDs for FW feature extension */ +#define SBI_EXT_FWFT_SET 0x0 +#define SBI_EXT_FWFT_GET 0x1 + +enum sbi_fwft_feature_t { + SBI_FWFT_MISALIGNED_EXC_DELEG = 0x0, + SBI_FWFT_LANDING_PAD = 0x1, + SBI_FWFT_SHADOW_STACK = 0x2, + SBI_FWFT_DOUBLE_TRAP = 0x3, + SBI_FWFT_PTE_AD_HARDWARE_UPDATE = 0x4, + SBI_FWFT_POINTER_MASKING_PMLEN = 0x5, + SBI_FWFT_LOCAL_RESERVED_START = 0x6, + SBI_FWFT_LOCAL_RESERVED_END = 0x3fffffff, + SBI_FWFT_LOCAL_PLATFORM_START = 0x40000000, + SBI_FWFT_LOCAL_PLATFORM_END = 0x7fffffff, + + SBI_FWFT_GLOBAL_RESERVED_START = 0x80000000, + SBI_FWFT_GLOBAL_RESERVED_END = 0xbfffffff, + SBI_FWFT_GLOBAL_PLATFORM_START = 0xc0000000, + SBI_FWFT_GLOBAL_PLATFORM_END = 0xffffffff, +}; + +#define SBI_FWFT_PLATFORM_FEATURE_BIT (1 << 30) +#define SBI_FWFT_GLOBAL_FEATURE_BIT (1 << 31) + +#define SBI_FWFT_SET_FLAG_LOCK (1 << 0) + struct sbiret { long error; long value; diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c new file mode 100644 index 00000000..8a7f2070 --- /dev/null +++ b/riscv/sbi-fwft.c @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * SBI verification + * + * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@rivosinc.com> + */ +#include <libcflat.h> +#include <stdlib.h> + +#include <asm/csr.h> +#include <asm/processor.h> +#include <asm/ptrace.h> +#include <asm/sbi.h> + +void check_fwft(void); + +static int fwft_set(unsigned long feature_id, unsigned long value, + unsigned long flags) +{ + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, + feature_id, value, flags, 0, 0, 0); + + return ret.error; +} + +static int fwft_get(unsigned long feature_id, unsigned long *value) +{ + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, + feature_id, 0, 0, 0, 0, 0); + + *value = ret.value; + + return ret.error; +} + +static void fwft_check_reserved(unsigned long id) +{ + int ret; + bool pass = true; + unsigned long value; + + ret = fwft_get(id, &value); + if (ret != SBI_ERR_DENIED) + pass = false; + + ret = fwft_set(id, 1, 0); + if (ret != SBI_ERR_DENIED) + pass = false; + + report(pass, "get/set reserved feature 0x%lx error == SBI_ERR_DENIED", id); +} + +static void fwft_check_denied(void) +{ + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_START); + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_END); + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_START); + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_END); +} + +static bool misaligned_handled; + +static void misaligned_handler(struct pt_regs *regs) +{ + misaligned_handled = true; + regs->epc += 4; +} + +static void fwft_check_misaligned(void) +{ + int ret; + unsigned long value; + + report_prefix_push("misaligned_deleg"); + + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); + if (ret == SBI_ERR_NOT_SUPPORTED) { + report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported"); + return; + } + report(!ret, "Get misaligned deleg feature no error"); + if (ret) + return; + + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0); + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error"); + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0); + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error"); + + /* Set to 0 and check after with get */ + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0); + report(!ret, "Set misaligned deleg feature value no error"); + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); + if (ret) + report_fail("Get misaligned deleg feature after set"); + else + report(value == 0, "Set misaligned deleg feature value 0"); + + /* Set to 1 and check after with get */ + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0); + report(!ret, "Set misaligned deleg feature value no error"); + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); + if (ret) + report_fail("Get misaligned deleg feature after set"); + else + report(value == 1, "Set misaligned deleg feature value 1"); + + install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler); + + asm volatile ( + ".option norvc\n" + "lw %[val], 1(%[val_addr])" + : [val] "+r" (value) + : [val_addr] "r" (&value) + : "memory"); + + if (!misaligned_handled) + report_skip("Verify misaligned load exception trap in supervisor"); + else + report_pass("Verify misaligned load exception trap in supervisor"); + + install_exception_handler(EXC_LOAD_MISALIGNED, NULL); + + report_prefix_pop(); +} + +void check_fwft(void) +{ + struct sbiret ret; + + report_prefix_push("fwft"); + + if (!sbi_probe(SBI_EXT_FWFT)) { + report_skip("FWFT extension not available"); + report_prefix_pop(); + return; + } + + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_FWFT, 0, 0, 0, 0, 0); + report(!ret.error, "FWFT extension probing no error"); + if (ret.error) + goto done; + + if (ret.value == 0) { + report_skip("FWFT extension is not present"); + goto done; + } + + fwft_check_denied(); + fwft_check_misaligned(); +done: + report_prefix_pop(); +} diff --git a/riscv/sbi.c b/riscv/sbi.c index 6f4ddaf1..8600e38e 100644 --- a/riscv/sbi.c +++ b/riscv/sbi.c @@ -32,6 +32,8 @@ #define HIGH_ADDR_BOUNDARY ((phys_addr_t)1 << 32) +void check_fwft(void); + static long __labs(long a) { return __builtin_labs(a); @@ -1451,6 +1453,7 @@ int main(int argc, char **argv) check_hsm(); check_dbcn(); check_susp(); + check_fwft(); return report_summary(); } -- 2.47.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension 2025-01-06 15:53 ` [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension Clément Léger @ 2025-01-15 12:58 ` Andrew Jones 2025-01-21 10:59 ` Clément Léger 0 siblings, 1 reply; 5+ messages in thread From: Andrew Jones @ 2025-01-15 12:58 UTC (permalink / raw) To: Clément Léger Cc: kvm, kvm-riscv, Andrew Jones, Anup Patel, Atish Patra On Mon, Jan 06, 2025 at 04:53:20PM +0100, Clément Léger wrote: > This commit add tests for a the FWFT SBI extension. Currently, only s/This commit// > the reserved range as well as the misaligned exception delegation. > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > riscv/Makefile | 2 +- > lib/riscv/asm/sbi.h | 31 +++++++++ > riscv/sbi-fwft.c | 153 ++++++++++++++++++++++++++++++++++++++++++++ > riscv/sbi.c | 3 + > 4 files changed, 188 insertions(+), 1 deletion(-) > create mode 100644 riscv/sbi-fwft.c > > diff --git a/riscv/Makefile b/riscv/Makefile > index 5b5e157c..52718f3f 100644 > --- a/riscv/Makefile > +++ b/riscv/Makefile > @@ -17,7 +17,7 @@ tests += $(TEST_DIR)/sieve.$(exe) > > all: $(tests) > > -$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o > +$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o $(TEST_DIR)/sbi-fwft.o > > # When built for EFI sieve needs extra memory, run with e.g. '-m 256' on QEMU > $(TEST_DIR)/sieve.$(exe): AUXFLAGS = 0x1 > diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h > index 98a9b097..27e6fcdb 100644 > --- a/lib/riscv/asm/sbi.h > +++ b/lib/riscv/asm/sbi.h > @@ -11,6 +11,9 @@ > #define SBI_ERR_ALREADY_AVAILABLE -6 > #define SBI_ERR_ALREADY_STARTED -7 > #define SBI_ERR_ALREADY_STOPPED -8 > +#define SBI_ERR_NO_SHMEM -9 > +#define SBI_ERR_INVALID_STATE -10 > +#define SBI_ERR_BAD_RANGE -11 Need SBI_ERR_DENIED_LOCKED (and TIMEOUT and IO) too > > #ifndef __ASSEMBLY__ > #include <cpumask.h> > @@ -23,6 +26,7 @@ enum sbi_ext_id { > SBI_EXT_SRST = 0x53525354, > SBI_EXT_DBCN = 0x4442434E, > SBI_EXT_SUSP = 0x53555350, > + SBI_EXT_FWFT = 0x46574654, > }; > > enum sbi_ext_base_fid { > @@ -71,6 +75,33 @@ enum sbi_ext_dbcn_fid { > SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, > }; > > +/* SBI function IDs for FW feature extension */ > +#define SBI_EXT_FWFT_SET 0x0 > +#define SBI_EXT_FWFT_GET 0x1 Use a _fid enum like the other extensions. > + > +enum sbi_fwft_feature_t { Use defines for the following, like SSE does for its ranges. > + SBI_FWFT_MISALIGNED_EXC_DELEG = 0x0, > + SBI_FWFT_LANDING_PAD = 0x1, > + SBI_FWFT_SHADOW_STACK = 0x2, > + SBI_FWFT_DOUBLE_TRAP = 0x3, > + SBI_FWFT_PTE_AD_HARDWARE_UPDATE = 0x4, SBI_FWFT_PTE_AD_HW_UPDATING > + SBI_FWFT_POINTER_MASKING_PMLEN = 0x5, > + SBI_FWFT_LOCAL_RESERVED_START = 0x6, > + SBI_FWFT_LOCAL_RESERVED_END = 0x3fffffff, Do we need the reserved start/end? SSE doesn't define its reserved ranges. > + SBI_FWFT_LOCAL_PLATFORM_START = 0x40000000, > + SBI_FWFT_LOCAL_PLATFORM_END = 0x7fffffff, > + > + SBI_FWFT_GLOBAL_RESERVED_START = 0x80000000, > + SBI_FWFT_GLOBAL_RESERVED_END = 0xbfffffff, Same reserved range question. > + SBI_FWFT_GLOBAL_PLATFORM_START = 0xc0000000, > + SBI_FWFT_GLOBAL_PLATFORM_END = 0xffffffff, > +}; > + > +#define SBI_FWFT_PLATFORM_FEATURE_BIT (1 << 30) > +#define SBI_FWFT_GLOBAL_FEATURE_BIT (1 << 31) > + > +#define SBI_FWFT_SET_FLAG_LOCK (1 << 0) BIT() for the above defines > + > struct sbiret { > long error; > long value; > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c > new file mode 100644 > index 00000000..8a7f2070 > --- /dev/null > +++ b/riscv/sbi-fwft.c > @@ -0,0 +1,153 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * SBI verification > + * > + * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@rivosinc.com> > + */ > +#include <libcflat.h> > +#include <stdlib.h> > + > +#include <asm/csr.h> > +#include <asm/processor.h> > +#include <asm/ptrace.h> > +#include <asm/sbi.h> > + > +void check_fwft(void); > + > +static int fwft_set(unsigned long feature_id, unsigned long value, returning an int is truncating sbiret.error s/unsigned long feature_id/uint32_t feature/ > + unsigned long flags) > +{ > + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, > + feature_id, value, flags, 0, 0, 0); > + > + return ret.error; > +} Probably need a fwft_set_raw() as well which takes an unsigned long for feature in order to test feature IDs that set bits >= 32 and returns an sbiret allowing sbiret.value to be checked. > + > +static int fwft_get(unsigned long feature_id, unsigned long *value) returning an int is truncating sbiret.error s/unsigned long feature_id/uint32_t feature/ > +{ > + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, > + feature_id, 0, 0, 0, 0, 0); > + > + *value = ret.value; > + > + return ret.error; Why not just return sbiret to return both value and error? As a separate patch we should update struct sbiret to match the latest spec which now has a union in it. Same comment about needing a _raw version too. > +} > + > +static void fwft_check_reserved(unsigned long id) > +{ > + int ret; > + bool pass = true; > + unsigned long value; > + > + ret = fwft_get(id, &value); > + if (ret != SBI_ERR_DENIED) > + pass = false; > + > + ret = fwft_set(id, 1, 0); > + if (ret != SBI_ERR_DENIED) > + pass = false; > + > + report(pass, "get/set reserved feature 0x%lx error == SBI_ERR_DENIED", id); The get and set should be split into two tests struct sbiret ret; ret = fwft_get(id); report(ret.error == SBI_ERR_DENIED, ...); ret = fwft_set(id, 1, 0); report(ret.error == SBI_ERR_DENIED, ...); > +} > + > +static void fwft_check_denied(void) > +{ > + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_START); > + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_END); > + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_START); > + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_END); I see why we have the reserved ranges defined now. Shouldn't we also have tests like these for SSE, which means we should define the reserved ranges for it too? > +} > + > +static bool misaligned_handled; > + > +static void misaligned_handler(struct pt_regs *regs) > +{ > + misaligned_handled = true; > + regs->epc += 4; > +} > + > +static void fwft_check_misaligned(void) > +{ > + int ret; > + unsigned long value; > + > + report_prefix_push("misaligned_deleg"); "misaligned_exc_deleg" > + > + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); > + if (ret == SBI_ERR_NOT_SUPPORTED) { > + report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported"); > + return; > + } > + report(!ret, "Get misaligned deleg feature no error"); Should output the error too > + if (ret) > + return; > + > + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0); > + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error"); > + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0); > + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error"); Something like if (__riscv_xlen > 32) { ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, (1ul << 32), 0); report(ret == SBI_ERR_INVALID_PARAM } would be a good test too (and also for the flags parameter) > + > + /* Set to 0 and check after with get */ > + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0); > + report(!ret, "Set misaligned deleg feature value no error"); > + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); > + if (ret) > + report_fail("Get misaligned deleg feature after set"); > + else > + report(value == 0, "Set misaligned deleg feature value 0"); > + > + /* Set to 1 and check after with get */ > + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0); > + report(!ret, "Set misaligned deleg feature value no error"); > + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); > + if (ret) > + report_fail("Get misaligned deleg feature after set"); > + else > + report(value == 1, "Set misaligned deleg feature value 1"); > + > + install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler); > + > + asm volatile ( > + ".option norvc\n" We also need push/pop otherwise from here on out we stop using compression instructions. > + "lw %[val], 1(%[val_addr])" > + : [val] "+r" (value) > + : [val_addr] "r" (&value) > + : "memory"); > + > + if (!misaligned_handled) > + report_skip("Verify misaligned load exception trap in supervisor"); Why is this report_skip()? Shouldn't we just do report(misaligned_handled, ...) > + else > + report_pass("Verify misaligned load exception trap in supervisor"); > + > + install_exception_handler(EXC_LOAD_MISALIGNED, NULL); > + > + report_prefix_pop(); > +} > + > +void check_fwft(void) > +{ > + struct sbiret ret; > + > + report_prefix_push("fwft"); > + > + if (!sbi_probe(SBI_EXT_FWFT)) { > + report_skip("FWFT extension not available"); > + report_prefix_pop(); > + return; > + } > + > + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_FWFT, 0, 0, 0, 0, 0); > + report(!ret.error, "FWFT extension probing no error"); > + if (ret.error) > + goto done; > + > + if (ret.value == 0) { > + report_skip("FWFT extension is not present"); > + goto done; > + } The above "raw" probing looks like it should have been removed when the sbi_probe() call was added. > + > + fwft_check_denied(); > + fwft_check_misaligned(); > +done: > + report_prefix_pop(); > +} > diff --git a/riscv/sbi.c b/riscv/sbi.c > index 6f4ddaf1..8600e38e 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -32,6 +32,8 @@ > > #define HIGH_ADDR_BOUNDARY ((phys_addr_t)1 << 32) > > +void check_fwft(void); > + > static long __labs(long a) > { > return __builtin_labs(a); > @@ -1451,6 +1453,7 @@ int main(int argc, char **argv) > check_hsm(); > check_dbcn(); > check_susp(); > + check_fwft(); > > return report_summary(); > } > -- > 2.47.1 > Nice start to the FWFT tests. After this is merged I'll add tests for PTE_AD_HW_UPDATING. We also should get LOCK and local/global tests in sooner than later. Thanks, drew ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension 2025-01-15 12:58 ` Andrew Jones @ 2025-01-21 10:59 ` Clément Léger 0 siblings, 0 replies; 5+ messages in thread From: Clément Léger @ 2025-01-21 10:59 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvm-riscv, Andrew Jones, Anup Patel, Atish Patra On 15/01/2025 13:58, Andrew Jones wrote: > On Mon, Jan 06, 2025 at 04:53:20PM +0100, Clément Léger wrote: >> This commit add tests for a the FWFT SBI extension. Currently, only > > s/This commit// > >> the reserved range as well as the misaligned exception delegation. >> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> --- >> riscv/Makefile | 2 +- >> lib/riscv/asm/sbi.h | 31 +++++++++ >> riscv/sbi-fwft.c | 153 ++++++++++++++++++++++++++++++++++++++++++++ >> riscv/sbi.c | 3 + >> 4 files changed, 188 insertions(+), 1 deletion(-) >> create mode 100644 riscv/sbi-fwft.c >> >> diff --git a/riscv/Makefile b/riscv/Makefile >> index 5b5e157c..52718f3f 100644 >> --- a/riscv/Makefile >> +++ b/riscv/Makefile >> @@ -17,7 +17,7 @@ tests += $(TEST_DIR)/sieve.$(exe) >> >> all: $(tests) >> >> -$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o >> +$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o $(TEST_DIR)/sbi-fwft.o >> >> # When built for EFI sieve needs extra memory, run with e.g. '-m 256' on QEMU >> $(TEST_DIR)/sieve.$(exe): AUXFLAGS = 0x1 >> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h >> index 98a9b097..27e6fcdb 100644 >> --- a/lib/riscv/asm/sbi.h >> +++ b/lib/riscv/asm/sbi.h >> @@ -11,6 +11,9 @@ >> #define SBI_ERR_ALREADY_AVAILABLE -6 >> #define SBI_ERR_ALREADY_STARTED -7 >> #define SBI_ERR_ALREADY_STOPPED -8 >> +#define SBI_ERR_NO_SHMEM -9 >> +#define SBI_ERR_INVALID_STATE -10 >> +#define SBI_ERR_BAD_RANGE -11 > > Need SBI_ERR_DENIED_LOCKED (and TIMEOUT and IO) too Indeed, i'll add that. > >> >> #ifndef __ASSEMBLY__ >> #include <cpumask.h> >> @@ -23,6 +26,7 @@ enum sbi_ext_id { >> SBI_EXT_SRST = 0x53525354, >> SBI_EXT_DBCN = 0x4442434E, >> SBI_EXT_SUSP = 0x53555350, >> + SBI_EXT_FWFT = 0x46574654, >> }; >> >> enum sbi_ext_base_fid { >> @@ -71,6 +75,33 @@ enum sbi_ext_dbcn_fid { >> SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, >> }; >> >> +/* SBI function IDs for FW feature extension */ >> +#define SBI_EXT_FWFT_SET 0x0 >> +#define SBI_EXT_FWFT_GET 0x1 > > Use a _fid enum like the other extensions. > >> + >> +enum sbi_fwft_feature_t { > > Use defines for the following, like SSE does for its ranges. > >> + SBI_FWFT_MISALIGNED_EXC_DELEG = 0x0, >> + SBI_FWFT_LANDING_PAD = 0x1, >> + SBI_FWFT_SHADOW_STACK = 0x2, >> + SBI_FWFT_DOUBLE_TRAP = 0x3, >> + SBI_FWFT_PTE_AD_HARDWARE_UPDATE = 0x4, > > SBI_FWFT_PTE_AD_HW_UPDATING > >> + SBI_FWFT_POINTER_MASKING_PMLEN = 0x5, >> + SBI_FWFT_LOCAL_RESERVED_START = 0x6, >> + SBI_FWFT_LOCAL_RESERVED_END = 0x3fffffff, > > Do we need the reserved start/end? SSE doesn't define its reserved > ranges. As seen below, it is used for reserved range testing. You are right about SSE, that would be nice to test that as well. > >> + SBI_FWFT_LOCAL_PLATFORM_START = 0x40000000, >> + SBI_FWFT_LOCAL_PLATFORM_END = 0x7fffffff, >> + >> + SBI_FWFT_GLOBAL_RESERVED_START = 0x80000000, >> + SBI_FWFT_GLOBAL_RESERVED_END = 0xbfffffff, > > Same reserved range question. > >> + SBI_FWFT_GLOBAL_PLATFORM_START = 0xc0000000, >> + SBI_FWFT_GLOBAL_PLATFORM_END = 0xffffffff, >> +}; >> + >> +#define SBI_FWFT_PLATFORM_FEATURE_BIT (1 << 30) >> +#define SBI_FWFT_GLOBAL_FEATURE_BIT (1 << 31) >> + >> +#define SBI_FWFT_SET_FLAG_LOCK (1 << 0) > > BIT() for the above defines > >> + >> struct sbiret { >> long error; >> long value; >> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c >> new file mode 100644 >> index 00000000..8a7f2070 >> --- /dev/null >> +++ b/riscv/sbi-fwft.c >> @@ -0,0 +1,153 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * SBI verification >> + * >> + * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@rivosinc.com> >> + */ >> +#include <libcflat.h> >> +#include <stdlib.h> >> + >> +#include <asm/csr.h> >> +#include <asm/processor.h> >> +#include <asm/ptrace.h> >> +#include <asm/sbi.h> >> + >> +void check_fwft(void); >> + >> +static int fwft_set(unsigned long feature_id, unsigned long value, > > returning an int is truncating sbiret.error > > s/unsigned long feature_id/uint32_t feature/ > >> + unsigned long flags) >> +{ >> + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, >> + feature_id, value, flags, 0, 0, 0); >> + >> + return ret.error; >> +} > > Probably need a fwft_set_raw() as well which takes an unsigned long for > feature in order to test feature IDs that set bits >= 32 and returns > an sbiret allowing sbiret.value to be checked. > >> + >> +static int fwft_get(unsigned long feature_id, unsigned long *value) > > returning an int is truncating sbiret.error > > s/unsigned long feature_id/uint32_t feature/ > >> +{ >> + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, >> + feature_id, 0, 0, 0, 0, 0); >> + >> + *value = ret.value; >> + >> + return ret.error; > > Why not just return sbiret to return both value and error? > > As a separate patch we should update struct sbiret to match the latest > spec which now has a union in it. > > Same comment about needing a _raw version too. Acked, I actually modified bith get/set to return an sbiret directly, that's easier to handle a unique return type in tests. > >> +} >> + >> +static void fwft_check_reserved(unsigned long id) >> +{ >> + int ret; >> + bool pass = true; >> + unsigned long value; >> + >> + ret = fwft_get(id, &value); >> + if (ret != SBI_ERR_DENIED) >> + pass = false; >> + >> + ret = fwft_set(id, 1, 0); >> + if (ret != SBI_ERR_DENIED) >> + pass = false; >> + >> + report(pass, "get/set reserved feature 0x%lx error == SBI_ERR_DENIED", id); > > The get and set should be split into two tests > > struct sbiret ret; > ret = fwft_get(id); > report(ret.error == SBI_ERR_DENIED, ...); > ret = fwft_set(id, 1, 0); > report(ret.error == SBI_ERR_DENIED, ...); > >> +} >> + >> +static void fwft_check_denied(void) >> +{ >> + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_START); >> + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_END); >> + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_START); >> + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_END); > > I see why we have the reserved ranges defined now. Shouldn't we also have > tests like these for SSE, which means we should define the reserved ranges > for it too? Yes, that should be tested in SSE as well. > >> +} >> + >> +static bool misaligned_handled; >> + >> +static void misaligned_handler(struct pt_regs *regs) >> +{ >> + misaligned_handled = true; >> + regs->epc += 4; >> +} >> + >> +static void fwft_check_misaligned(void) >> +{ >> + int ret; >> + unsigned long value; >> + >> + report_prefix_push("misaligned_deleg"); > > "misaligned_exc_deleg" > >> + >> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); >> + if (ret == SBI_ERR_NOT_SUPPORTED) { >> + report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported"); >> + return; >> + } >> + report(!ret, "Get misaligned deleg feature no error"); > > Should output the error too > >> + if (ret) >> + return; >> + >> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0); >> + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error"); >> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0); >> + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error"); > > Something like > > if (__riscv_xlen > 32) { > ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, (1ul << 32), 0); > report(ret == SBI_ERR_INVALID_PARAM > } > > would be a good test too (and also for the flags parameter) > Acked I'll add that. >> + >> + /* Set to 0 and check after with get */ >> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0); >> + report(!ret, "Set misaligned deleg feature value no error"); >> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); >> + if (ret) >> + report_fail("Get misaligned deleg feature after set"); >> + else >> + report(value == 0, "Set misaligned deleg feature value 0"); >> + >> + /* Set to 1 and check after with get */ >> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0); >> + report(!ret, "Set misaligned deleg feature value no error"); >> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value); >> + if (ret) >> + report_fail("Get misaligned deleg feature after set"); >> + else >> + report(value == 1, "Set misaligned deleg feature value 1"); >> + >> + install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler); >> + >> + asm volatile ( >> + ".option norvc\n" > > We also need push/pop otherwise from here on out we stop using compression > instructions. Yeah, nice catch, I'll add push/pop. > >> + "lw %[val], 1(%[val_addr])" >> + : [val] "+r" (value) >> + : [val_addr] "r" (&value) >> + : "memory"); >> + >> + if (!misaligned_handled) >> + report_skip("Verify misaligned load exception trap in supervisor"); > > Why is this report_skip()? Shouldn't we just do > > report(misaligned_handled, ...) Some platforms might actually allow you to delegate the misaligned access trap but handle scalar misaligned accesses in hardware but trap on vector misaligned. To be "more" complete, I should add vector testing but I haven't had time to check vector instructions. > >> + else >> + report_pass("Verify misaligned load exception trap in supervisor"); >> + >> + install_exception_handler(EXC_LOAD_MISALIGNED, NULL); >> + >> + report_prefix_pop(); >> +} >> + >> +void check_fwft(void) >> +{ >> + struct sbiret ret; >> + >> + report_prefix_push("fwft"); >> + >> + if (!sbi_probe(SBI_EXT_FWFT)) { >> + report_skip("FWFT extension not available"); >> + report_prefix_pop(); >> + return; >> + } >> + >> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_FWFT, 0, 0, 0, 0, 0); >> + report(!ret.error, "FWFT extension probing no error"); >> + if (ret.error) >> + goto done; >> + >> + if (ret.value == 0) { >> + report_skip("FWFT extension is not present"); >> + goto done; >> + } > > The above "raw" probing looks like it should have been removed when > the sbi_probe() call was added. Oups yes, that should have been removed. > >> + >> + fwft_check_denied(); >> + fwft_check_misaligned(); >> +done: >> + report_prefix_pop(); >> +} >> diff --git a/riscv/sbi.c b/riscv/sbi.c >> index 6f4ddaf1..8600e38e 100644 >> --- a/riscv/sbi.c >> +++ b/riscv/sbi.c >> @@ -32,6 +32,8 @@ >> >> #define HIGH_ADDR_BOUNDARY ((phys_addr_t)1 << 32) >> >> +void check_fwft(void); >> + >> static long __labs(long a) >> { >> return __builtin_labs(a); >> @@ -1451,6 +1453,7 @@ int main(int argc, char **argv) >> check_hsm(); >> check_dbcn(); >> check_susp(); >> + check_fwft(); >> >> return report_summary(); >> } >> -- >> 2.47.1 >> > > Nice start to the FWFT tests. After this is merged I'll add tests for > PTE_AD_HW_UPDATING. We also should get LOCK and local/global tests in > sooner than later. Acked, I'll add LOCK testing for misaligned as well. Thanks for the review, Clément > > Thanks, > drew ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-21 10:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-06 15:53 [kvm-unit-tests PATCH 0/2] Add support for SBI FWFT extension testing Clément Léger 2025-01-06 15:53 ` [kvm-unit-tests PATCH 1/2] riscv: Add "-deps" handling for tests Clément Léger 2025-01-06 15:53 ` [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension Clément Léger 2025-01-15 12:58 ` Andrew Jones 2025-01-21 10:59 ` Clément Léger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox