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

[kvm-unit-tests PATCH 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.

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


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

[kvm-unit-tests PATCH 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.

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

[kvm-unit-tests PATCH 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-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

[kvm-unit-tests PATCH 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 2/2] riscv: Add tests for SBI FWFT extension

[Clément Léger]

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


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Clément Léger]

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

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

[Andrew Jones]

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

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Andrew Jones]

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

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

[Clément Léger]

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


-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Clément Léger]

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