[kvm-unit-tests PATCH v2 0/2] Add support for SBI FWFT extension testing

[kvm-unit-tests PATCH v2 0/2] Add support for SBI FWFT extension testing

[Clément Léger]

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.

---

V2:
 - Added fwft_{get/set}_raw() to test invalid > 32 bits ids
 - Added test for invalid flags/value > 32 bits
 - Added test for lock feature
 - Use and enum for FWFT functions
 - Replace hardcoded 1 << with BIT()
 - Fix fwft_get/set return value
 - Split set/get tests for reserved ranges
 - Added push/pop to arch -c option
 - Remove leftover of manual probing code

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 |  34 ++++++++
 riscv/sbi-fwft.c    | 189 ++++++++++++++++++++++++++++++++++++++++++++
 riscv/sbi.c         |   3 +
 4 files changed, 231 insertions(+), 3 deletions(-)
 create mode 100644 riscv/sbi-fwft.c

-- 
2.47.1

[kvm-unit-tests PATCH v2 1/2] riscv: Add "-deps" handling for tests

[Clément Léger]

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

[kvm-unit-tests PATCH v2 2/2] riscv: Add tests for SBI FWFT extension

[Clément Léger]

Add tests for the FWFT SBI extension. Currently, only the reserved range
as well as the misaligned exception delegation are used.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 riscv/Makefile      |   2 +-
 lib/riscv/asm/sbi.h |  34 ++++++++
 riscv/sbi-fwft.c    | 189 ++++++++++++++++++++++++++++++++++++++++++++
 riscv/sbi.c         |   3 +
 4 files changed, 227 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..397400f2 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -11,6 +11,12 @@
 #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
+#define SBI_ERR_TIMEOUT			-12
+#define SBI_ERR_IO			-13
+#define SBI_ERR_LOCKED			-14
 
 #ifndef __ASSEMBLY__
 #include <cpumask.h>
@@ -23,6 +29,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 +78,33 @@ enum sbi_ext_dbcn_fid {
 	SBI_EXT_DBCN_CONSOLE_WRITE_BYTE,
 };
 
+
+enum sbi_ext_fwft_fid {
+	SBI_EXT_FWFT_SET = 0,
+	SBI_EXT_FWFT_GET,
+};
+
+#define SBI_FWFT_MISALIGNED_EXC_DELEG		0x0
+#define SBI_FWFT_LANDING_PAD			0x1
+#define SBI_FWFT_SHADOW_STACK			0x2
+#define SBI_FWFT_DOUBLE_TRAP			0x3
+#define SBI_FWFT_PTE_AD_HW_UPDATING		0x4
+#define SBI_FWFT_POINTER_MASKING_PMLEN		0x5
+#define SBI_FWFT_LOCAL_RESERVED_START		0x6
+#define SBI_FWFT_LOCAL_RESERVED_END		0x3fffffff
+#define SBI_FWFT_LOCAL_PLATFORM_START		0x40000000
+#define SBI_FWFT_LOCAL_PLATFORM_END		0x7fffffff
+
+#define SBI_FWFT_GLOBAL_RESERVED_START		0x80000000
+#define SBI_FWFT_GLOBAL_RESERVED_END		0xbfffffff
+#define SBI_FWFT_GLOBAL_PLATFORM_START		0xc0000000
+#define SBI_FWFT_GLOBAL_PLATFORM_END		0xffffffff
+
+#define SBI_FWFT_PLATFORM_FEATURE_BIT		BIT(30)
+#define SBI_FWFT_GLOBAL_FEATURE_BIT		BIT(31)
+
+#define SBI_FWFT_SET_FLAG_LOCK			BIT(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..7866a917
--- /dev/null
+++ b/riscv/sbi-fwft.c
@@ -0,0 +1,189 @@
+// 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 struct sbiret fwft_set_raw(unsigned long feature_id, unsigned long value,
+				  unsigned long flags)
+{
+	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, feature_id, value,
+			 flags, 0, 0, 0);
+}
+
+static struct sbiret fwft_set(uint32_t feature_id, unsigned long value,
+			      unsigned long flags)
+{
+	return fwft_set_raw(feature_id, value, flags);
+}
+
+static struct sbiret fwft_get_raw(unsigned long feature_id)
+{
+	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, feature_id, 0, 0, 0, 0,
+			 0);
+}
+
+static struct sbiret fwft_get(uint32_t feature_id)
+{
+	return fwft_get_raw(feature_id);
+}
+
+static void fwft_check_reserved(unsigned long id)
+{
+	struct sbiret ret;
+
+	ret = fwft_get(id);
+	report(ret.error == SBI_ERR_DENIED,
+	       "get reserved feature 0x%lx error == SBI_ERR_DENIED", id);
+
+	ret = fwft_set(id, 1, 0);
+	report(ret.error == SBI_ERR_DENIED,
+	       "set reserved feature 0x%lx error == SBI_ERR_DENIED", id);
+}
+
+static void fwft_check_base(void)
+{
+	struct sbiret ret;
+
+	report_prefix_push("base");
+
+	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);
+
+	/* Check id > 32 bits */
+	if (__riscv_xlen > 32) {
+		ret = fwft_get_raw(BIT(32));
+		report(ret.error == SBI_ERR_INVALID_PARAM,
+		"get feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));
+
+		ret = fwft_set_raw(BIT(32), 0, 0);
+		report(ret.error == SBI_ERR_INVALID_PARAM,
+		"set feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));
+	}
+
+	report_prefix_pop();
+}
+
+static bool misaligned_handled;
+
+static void misaligned_handler(struct pt_regs *regs)
+{
+	misaligned_handled = true;
+	regs->epc += 4;
+}
+
+static void fwft_check_misaligned_exc_deleg(void)
+{
+	struct sbiret ret;
+
+	report_prefix_push("misaligned_exc_deleg");
+
+	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
+	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
+		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
+		return;
+	}
+	report(!ret.error, "Get misaligned deleg feature no error: %lx",
+	       ret.error);
+	if (ret.error)
+		return;
+
+	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0);
+	report(ret.error == SBI_ERR_INVALID_PARAM,
+	       "Set misaligned deleg feature invalid value error");
+	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0);
+	report(ret.error == SBI_ERR_INVALID_PARAM,
+	       "Set misaligned deleg feature invalid value error");
+
+	if (__riscv_xlen > 32) {
+		ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, BIT(32), 0);
+		report(ret.error == SBI_ERR_INVALID_PARAM,
+		       "Set misaligned deleg with value > 32bits invalid param error");
+
+		ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, BIT(32));
+		report(ret.error == SBI_ERR_INVALID_PARAM,
+		       "Set misaligned deleg with flag > 32bits invalid param error");
+	}
+
+	/* Set to 0 and check after with get */
+	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
+	report(!ret.error, "Set misaligned deleg feature value no error");
+	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
+	if (ret.error)
+		report_fail("Get misaligned deleg feature after set: %lx",
+			    ret.error);
+	else
+		report(ret.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.error, "Set misaligned deleg feature value no error");
+	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
+	if (ret.error)
+		report_fail("Get misaligned deleg feature after set: %lx",
+			    ret.error);
+	else
+		report(ret.value == 1, "Set misaligned deleg feature value 1");
+
+	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
+
+	asm volatile (
+		".option push\n"
+		/*
+		 * Disable compression so the lw takes exactly 4 bytes and thus
+		 * can be skipped reliably from the exception handler.
+		 */
+		".option arch,-c\n"
+		"lw %[val], 1(%[val_addr])\n"
+		".option pop\n"
+		: [val] "+r" (ret.value)
+		: [val_addr] "r" (&ret.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);
+
+	/* Lock the feature */
+	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, SBI_FWFT_SET_FLAG_LOCK);
+	report(!ret.error, "Set misaligned deleg feature value 0 and lock no error");
+	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
+	report(ret.error == SBI_ERR_LOCKED,
+	       "Set misaligned deleg feature value 0 and lock no error");
+	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
+	report(!ret.error, "Get misaligned deleg locked value no error");
+
+	report_prefix_pop();
+}
+
+void check_fwft(void)
+{
+	report_prefix_push("fwft");
+
+	if (!sbi_probe(SBI_EXT_FWFT)) {
+		report_skip("FWFT extension not available");
+		report_prefix_pop();
+		return;
+	}
+
+	fwft_check_base();
+	fwft_check_misaligned_exc_deleg();
+
+	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

Re: [kvm-unit-tests PATCH v2 2/2] riscv: Add tests for SBI FWFT extension

[Andrew Jones]

On Thu, Jan 23, 2025 at 05:54:04PM +0100, Clément Léger wrote:
> Add tests for the FWFT SBI extension. Currently, only the reserved range
> as well as the misaligned exception delegation are used.
> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  riscv/Makefile      |   2 +-
>  lib/riscv/asm/sbi.h |  34 ++++++++
>  riscv/sbi-fwft.c    | 189 ++++++++++++++++++++++++++++++++++++++++++++
>  riscv/sbi.c         |   3 +
>  4 files changed, 227 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..397400f2 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -11,6 +11,12 @@
>  #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
> +#define SBI_ERR_TIMEOUT			-12
> +#define SBI_ERR_IO			-13
> +#define SBI_ERR_LOCKED			-14
>  
>  #ifndef __ASSEMBLY__
>  #include <cpumask.h>
> @@ -23,6 +29,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 +78,33 @@ enum sbi_ext_dbcn_fid {
>  	SBI_EXT_DBCN_CONSOLE_WRITE_BYTE,
>  };
>  
> +
> +enum sbi_ext_fwft_fid {
> +	SBI_EXT_FWFT_SET = 0,
> +	SBI_EXT_FWFT_GET,
> +};
> +
> +#define SBI_FWFT_MISALIGNED_EXC_DELEG		0x0
> +#define SBI_FWFT_LANDING_PAD			0x1
> +#define SBI_FWFT_SHADOW_STACK			0x2
> +#define SBI_FWFT_DOUBLE_TRAP			0x3
> +#define SBI_FWFT_PTE_AD_HW_UPDATING		0x4
> +#define SBI_FWFT_POINTER_MASKING_PMLEN		0x5
> +#define SBI_FWFT_LOCAL_RESERVED_START		0x6
> +#define SBI_FWFT_LOCAL_RESERVED_END		0x3fffffff
> +#define SBI_FWFT_LOCAL_PLATFORM_START		0x40000000
> +#define SBI_FWFT_LOCAL_PLATFORM_END		0x7fffffff
> +
> +#define SBI_FWFT_GLOBAL_RESERVED_START		0x80000000
> +#define SBI_FWFT_GLOBAL_RESERVED_END		0xbfffffff
> +#define SBI_FWFT_GLOBAL_PLATFORM_START		0xc0000000
> +#define SBI_FWFT_GLOBAL_PLATFORM_END		0xffffffff
> +
> +#define SBI_FWFT_PLATFORM_FEATURE_BIT		BIT(30)
> +#define SBI_FWFT_GLOBAL_FEATURE_BIT		BIT(31)
> +
> +#define SBI_FWFT_SET_FLAG_LOCK			BIT(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..7866a917
> --- /dev/null
> +++ b/riscv/sbi-fwft.c
> @@ -0,0 +1,189 @@
> +// 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 struct sbiret fwft_set_raw(unsigned long feature_id, unsigned long value,
> +				  unsigned long flags)
> +{
> +	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, feature_id, value,
> +			 flags, 0, 0, 0);
> +}
> +
> +static struct sbiret fwft_set(uint32_t feature_id, unsigned long value,
> +			      unsigned long flags)
> +{
> +	return fwft_set_raw(feature_id, value, flags);
> +}
> +
> +static struct sbiret fwft_get_raw(unsigned long feature_id)
> +{
> +	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, feature_id, 0, 0, 0, 0,
> +			 0);
> +}
> +
> +static struct sbiret fwft_get(uint32_t feature_id)
> +{
> +	return fwft_get_raw(feature_id);
> +}

nit: s/feature_id/feature/ for all the above, as that would exactly match
the spec, but no biggy.

> +
> +static void fwft_check_reserved(unsigned long id)
> +{
> +	struct sbiret ret;
> +
> +	ret = fwft_get(id);
> +	report(ret.error == SBI_ERR_DENIED,
> +	       "get reserved feature 0x%lx error == SBI_ERR_DENIED", id);
> +
> +	ret = fwft_set(id, 1, 0);
> +	report(ret.error == SBI_ERR_DENIED,
> +	       "set reserved feature 0x%lx error == SBI_ERR_DENIED", id);

I think we need something like gen_report(), which is in riscv/sbi.c, in
order to output the unexpected errors if we don't get what we expect. I've
pushed a few patches to my riscv/sbi branch[1] in order to provide
sbiret_report_error().

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

> +}
> +
> +static void fwft_check_base(void)
> +{
> +	struct sbiret ret;
> +
> +	report_prefix_push("base");
> +
> +	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);
> +
> +	/* Check id > 32 bits */
> +	if (__riscv_xlen > 32) {
> +		ret = fwft_get_raw(BIT(32));
> +		report(ret.error == SBI_ERR_INVALID_PARAM,
> +		"get feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));

Need to indent the string under ret.error.

> +
> +		ret = fwft_set_raw(BIT(32), 0, 0);
> +		report(ret.error == SBI_ERR_INVALID_PARAM,
> +		"set feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));

Need to indent the string under ret.error.

> +	}
> +
> +	report_prefix_pop();
> +}
> +
> +static bool misaligned_handled;
> +
> +static void misaligned_handler(struct pt_regs *regs)
> +{
> +	misaligned_handled = true;
> +	regs->epc += 4;
> +}
> +
> +static void fwft_check_misaligned_exc_deleg(void)
> +{
> +	struct sbiret ret;
> +
> +	report_prefix_push("misaligned_exc_deleg");
> +
> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> +	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> +		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> +		return;
> +	}
> +	report(!ret.error, "Get misaligned deleg feature no error: %lx",
> +	       ret.error);
> +	if (ret.error)
> +		return;

With the new sbiret_report_error() this can become

  if (!sbiret_report_error(ret, 0, "Get misaligned deleg feature no error"))
      return;

> +
> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0);
> +	report(ret.error == SBI_ERR_INVALID_PARAM,
> +	       "Set misaligned deleg feature invalid value error");
> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0);
> +	report(ret.error == SBI_ERR_INVALID_PARAM,
> +	       "Set misaligned deleg feature invalid value error");

Need sbiret_report_error() to ensure we can see what the returned error
was if it isn't what we expected.

We should have a different error message for each test so testers can
look up the one that failed by the message. Maybe just output the
invalid test value too.

> +
> +	if (__riscv_xlen > 32) {
> +		ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, BIT(32), 0);
> +		report(ret.error == SBI_ERR_INVALID_PARAM,
> +		       "Set misaligned deleg with value > 32bits invalid param error");
> +
> +		ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, BIT(32));
> +		report(ret.error == SBI_ERR_INVALID_PARAM,
> +		       "Set misaligned deleg with flag > 32bits invalid param error");

Same two comments as above.

> +	}
> +
> +	/* Set to 0 and check after with get */
> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
> +	report(!ret.error, "Set misaligned deleg feature value no error");

 sbiret_report_error()

> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> +	if (ret.error)
> +		report_fail("Get misaligned deleg feature after set: %lx",
> +			    ret.error);
> +	else
> +		report(ret.value == 0, "Set misaligned deleg feature value 0");

 sbiret_report(ret, 0, 0, "get/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.error, "Set misaligned deleg feature value no error");
> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> +	if (ret.error)
> +		report_fail("Get misaligned deleg feature after set: %lx",
> +			    ret.error);
> +	else
> +		report(ret.value == 1, "Set misaligned deleg feature value 1");

Same sbiret_report_error / sbiret_report comments as above.

We should get SBI_ERR_NOT_SUPPORTED if the platform doesn't support
delegating misaligned exceptions, right? If so, then we should tolerate
that and skip the trap test below.

> +
> +	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
> +
> +	asm volatile (
> +		".option push\n"
> +		/*
> +		 * Disable compression so the lw takes exactly 4 bytes and thus
> +		 * can be skipped reliably from the exception handler.
> +		 */
> +		".option arch,-c\n"

What's the difference between this and .option norvc ?

> +		"lw %[val], 1(%[val_addr])\n"
> +		".option pop\n"
> +		: [val] "+r" (ret.value)
> +		: [val_addr] "r" (&ret.value)
> +		: "memory");
> +
> +	if (!misaligned_handled)
> +		report_skip("Verify misaligned load exception trap in supervisor");

If we skip this test when we can't set SBI_FWFT_MISALIGNED_EXC_DELEG to 1,
then we should always fail this test when we don't get the exception.

> +	else
> +		report_pass("Verify misaligned load exception trap in supervisor");
> +
> +	install_exception_handler(EXC_LOAD_MISALIGNED, NULL);
> +
> +	/* Lock the feature */
> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, SBI_FWFT_SET_FLAG_LOCK);
> +	report(!ret.error, "Set misaligned deleg feature value 0 and lock no error");
> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
> +	report(ret.error == SBI_ERR_LOCKED,
> +	       "Set misaligned deleg feature value 0 and lock no error");
> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> +	report(!ret.error, "Get misaligned deleg locked value no error");

Need sbiret_report_error() for all these tests.

> +
> +	report_prefix_pop();
> +}
> +
> +void check_fwft(void)
> +{
> +	report_prefix_push("fwft");
> +
> +	if (!sbi_probe(SBI_EXT_FWFT)) {
> +		report_skip("FWFT extension not available");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	fwft_check_base();
> +	fwft_check_misaligned_exc_deleg();
> +
> +	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
>

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 2/2] riscv: Add tests for SBI FWFT extension

[Clément Léger]

On 24/01/2025 17:35, Andrew Jones wrote:
> On Thu, Jan 23, 2025 at 05:54:04PM +0100, Clément Léger wrote:
>> Add tests for the FWFT SBI extension. Currently, only the reserved range
>> as well as the misaligned exception delegation are used.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  riscv/Makefile      |   2 +-
>>  lib/riscv/asm/sbi.h |  34 ++++++++
>>  riscv/sbi-fwft.c    | 189 ++++++++++++++++++++++++++++++++++++++++++++
>>  riscv/sbi.c         |   3 +
>>  4 files changed, 227 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..397400f2 100644
>> --- a/lib/riscv/asm/sbi.h
>> +++ b/lib/riscv/asm/sbi.h
>> @@ -11,6 +11,12 @@
>>  #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
>> +#define SBI_ERR_TIMEOUT			-12
>> +#define SBI_ERR_IO			-13
>> +#define SBI_ERR_LOCKED			-14
>>  
>>  #ifndef __ASSEMBLY__
>>  #include <cpumask.h>
>> @@ -23,6 +29,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 +78,33 @@ enum sbi_ext_dbcn_fid {
>>  	SBI_EXT_DBCN_CONSOLE_WRITE_BYTE,
>>  };
>>  
>> +
>> +enum sbi_ext_fwft_fid {
>> +	SBI_EXT_FWFT_SET = 0,
>> +	SBI_EXT_FWFT_GET,
>> +};
>> +
>> +#define SBI_FWFT_MISALIGNED_EXC_DELEG		0x0
>> +#define SBI_FWFT_LANDING_PAD			0x1
>> +#define SBI_FWFT_SHADOW_STACK			0x2
>> +#define SBI_FWFT_DOUBLE_TRAP			0x3
>> +#define SBI_FWFT_PTE_AD_HW_UPDATING		0x4
>> +#define SBI_FWFT_POINTER_MASKING_PMLEN		0x5
>> +#define SBI_FWFT_LOCAL_RESERVED_START		0x6
>> +#define SBI_FWFT_LOCAL_RESERVED_END		0x3fffffff
>> +#define SBI_FWFT_LOCAL_PLATFORM_START		0x40000000
>> +#define SBI_FWFT_LOCAL_PLATFORM_END		0x7fffffff
>> +
>> +#define SBI_FWFT_GLOBAL_RESERVED_START		0x80000000
>> +#define SBI_FWFT_GLOBAL_RESERVED_END		0xbfffffff
>> +#define SBI_FWFT_GLOBAL_PLATFORM_START		0xc0000000
>> +#define SBI_FWFT_GLOBAL_PLATFORM_END		0xffffffff
>> +
>> +#define SBI_FWFT_PLATFORM_FEATURE_BIT		BIT(30)
>> +#define SBI_FWFT_GLOBAL_FEATURE_BIT		BIT(31)
>> +
>> +#define SBI_FWFT_SET_FLAG_LOCK			BIT(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..7866a917
>> --- /dev/null
>> +++ b/riscv/sbi-fwft.c
>> @@ -0,0 +1,189 @@
>> +// 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 struct sbiret fwft_set_raw(unsigned long feature_id, unsigned long value,
>> +				  unsigned long flags)
>> +{
>> +	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, feature_id, value,
>> +			 flags, 0, 0, 0);
>> +}
>> +
>> +static struct sbiret fwft_set(uint32_t feature_id, unsigned long value,
>> +			      unsigned long flags)
>> +{
>> +	return fwft_set_raw(feature_id, value, flags);
>> +}
>> +
>> +static struct sbiret fwft_get_raw(unsigned long feature_id)
>> +{
>> +	return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, feature_id, 0, 0, 0, 0,
>> +			 0);
>> +}
>> +
>> +static struct sbiret fwft_get(uint32_t feature_id)
>> +{
>> +	return fwft_get_raw(feature_id);
>> +}
> 
> nit: s/feature_id/feature/ for all the above, as that would exactly match
> the spec, but no biggy.
> 
>> +
>> +static void fwft_check_reserved(unsigned long id)
>> +{
>> +	struct sbiret ret;
>> +
>> +	ret = fwft_get(id);
>> +	report(ret.error == SBI_ERR_DENIED,
>> +	       "get reserved feature 0x%lx error == SBI_ERR_DENIED", id);
>> +
>> +	ret = fwft_set(id, 1, 0);
>> +	report(ret.error == SBI_ERR_DENIED,
>> +	       "set reserved feature 0x%lx error == SBI_ERR_DENIED", id);
> 
> I think we need something like gen_report(), which is in riscv/sbi.c, in
> order to output the unexpected errors if we don't get what we expect. I've
> pushed a few patches to my riscv/sbi branch[1] in order to provide
> sbiret_report_error().
> 
> [1] https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi

Ok sounds good, I'll use that function. And I'll do that on SSE testing
as well.

> 
>> +}
>> +
>> +static void fwft_check_base(void)
>> +{
>> +	struct sbiret ret;
>> +
>> +	report_prefix_push("base");
>> +
>> +	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);
>> +
>> +	/* Check id > 32 bits */
>> +	if (__riscv_xlen > 32) {
>> +		ret = fwft_get_raw(BIT(32));
>> +		report(ret.error == SBI_ERR_INVALID_PARAM,
>> +		"get feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));
> 
> Need to indent the string under ret.error.
> 
>> +
>> +		ret = fwft_set_raw(BIT(32), 0, 0);
>> +		report(ret.error == SBI_ERR_INVALID_PARAM,
>> +		"set feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));
> 
> Need to indent the string under ret.error.
> 
>> +	}
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +static bool misaligned_handled;
>> +
>> +static void misaligned_handler(struct pt_regs *regs)
>> +{
>> +	misaligned_handled = true;
>> +	regs->epc += 4;
>> +}
>> +
>> +static void fwft_check_misaligned_exc_deleg(void)
>> +{
>> +	struct sbiret ret;
>> +
>> +	report_prefix_push("misaligned_exc_deleg");
>> +
>> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
>> +	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
>> +		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
>> +		return;
>> +	}
>> +	report(!ret.error, "Get misaligned deleg feature no error: %lx",
>> +	       ret.error);
>> +	if (ret.error)
>> +		return;
> 
> With the new sbiret_report_error() this can become
> 
>   if (!sbiret_report_error(ret, 0, "Get misaligned deleg feature no error"))
>       return;
> 
>> +
>> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0);
>> +	report(ret.error == SBI_ERR_INVALID_PARAM,
>> +	       "Set misaligned deleg feature invalid value error");
>> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0);
>> +	report(ret.error == SBI_ERR_INVALID_PARAM,
>> +	       "Set misaligned deleg feature invalid value error");
> 
> Need sbiret_report_error() to ensure we can see what the returned error
> was if it isn't what we expected.
> 
> We should have a different error message for each test so testers can
> look up the one that failed by the message. Maybe just output the
> invalid test value too.

Yes sure, I'll include the set value as well.

> 
>> +
>> +	if (__riscv_xlen > 32) {
>> +		ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, BIT(32), 0);
>> +		report(ret.error == SBI_ERR_INVALID_PARAM,
>> +		       "Set misaligned deleg with value > 32bits invalid param error");
>> +
>> +		ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, BIT(32));
>> +		report(ret.error == SBI_ERR_INVALID_PARAM,
>> +		       "Set misaligned deleg with flag > 32bits invalid param error");
> 
> Same two comments as above.
> 
>> +	}
>> +
>> +	/* Set to 0 and check after with get */
>> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
>> +	report(!ret.error, "Set misaligned deleg feature value no error");
> 
>  sbiret_report_error()
> 
>> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
>> +	if (ret.error)
>> +		report_fail("Get misaligned deleg feature after set: %lx",
>> +			    ret.error);
>> +	else
>> +		report(ret.value == 0, "Set misaligned deleg feature value 0");
> 
>  sbiret_report(ret, 0, 0, "get/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.error, "Set misaligned deleg feature value no error");
>> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
>> +	if (ret.error)
>> +		report_fail("Get misaligned deleg feature after set: %lx",
>> +			    ret.error);
>> +	else
>> +		report(ret.value == 1, "Set misaligned deleg feature value 1");
> 
> Same sbiret_report_error / sbiret_report comments as above.
> 
> We should get SBI_ERR_NOT_SUPPORTED if the platform doesn't support
> delegating misaligned exceptions, right? If so, then we should tolerate
> that and skip the trap test below.

I can't find/remember in which discussion it was discussed but the
consensus was that it is not an UNSUPPORTED error since the trap exist
on all implementations an can always be delegated. So this is actually
not and error, the trap is delegated but the platform does not generate
any misalign access traps. The S-mode can then check if the platform
misaligned accesses traps or not by itself.

> 
>> +
>> +	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
>> +
>> +	asm volatile (
>> +		".option push\n"
>> +		/*
>> +		 * Disable compression so the lw takes exactly 4 bytes and thus
>> +		 * can be skipped reliably from the exception handler.
>> +		 */
>> +		".option arch,-c\n"
> 
> What's the difference between this and .option norvc ?

I felt like this is mode generic than norvc/rvc. .arch works with all
extension AFAIK, norvc/rvc is purely relevant to C. Both of them works
the same way (at least to my trials with it).

> 
>> +		"lw %[val], 1(%[val_addr])\n"
>> +		".option pop\n"
>> +		: [val] "+r" (ret.value)
>> +		: [val_addr] "r" (&ret.value)
>> +		: "memory");
>> +
>> +	if (!misaligned_handled)
>> +		report_skip("Verify misaligned load exception trap in supervisor");
> 
> If we skip this test when we can't set SBI_FWFT_MISALIGNED_EXC_DELEG to 1,
> then we should always fail this test when we don't get the exception.

See my comment above. This is likely to be just an additional
information then. ie platform traps upon misaligned accesses. Probably
not what you expected though.

> 
>> +	else
>> +		report_pass("Verify misaligned load exception trap in supervisor");
>> +
>> +	install_exception_handler(EXC_LOAD_MISALIGNED, NULL);
>> +
>> +	/* Lock the feature */
>> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, SBI_FWFT_SET_FLAG_LOCK);
>> +	report(!ret.error, "Set misaligned deleg feature value 0 and lock no error");
>> +	ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
>> +	report(ret.error == SBI_ERR_LOCKED,
>> +	       "Set misaligned deleg feature value 0 and lock no error");
>> +	ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
>> +	report(!ret.error, "Get misaligned deleg locked value no error");
> 
> Need sbiret_report_error() for all these tests.
> 
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +void check_fwft(void)
>> +{
>> +	report_prefix_push("fwft");
>> +
>> +	if (!sbi_probe(SBI_EXT_FWFT)) {
>> +		report_skip("FWFT extension not available");
>> +		report_prefix_pop();
>> +		return;
>> +	}
>> +
>> +	fwft_check_base();
>> +	fwft_check_misaligned_exc_deleg();
>> +
>> +	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
>>
> 
> Thanks,
> drew