[kvm-unit-tests PATCH 00/10] riscv: sbi: Test improvements and a couple new

[kvm-unit-tests PATCH 00/10] riscv: sbi: Test improvements and a couple new

[Andrew Jones]

Improvements:
 - Ensure system suspend test won't hang
 - Ensure HSM suspend tests won't hang
 - Ensure state is cleaned up at the end of one extension's test before
   starting another
 - Probe SUSP and skip cleanly
 - Minor SUSP test output improvement

New tests:
 - Check bad FIDs for all extensions
 - Check SUSP sleep_type upper bits are ignored on RV64

Other:
 - Use kfail for some FWFT fails to make CI happy

Based and available here:
https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi

Andrew Jones (10):
  riscv: sbi: Mark known fwft failures as kfails
  riscv: sbi: Ensure we have IPIs enabled for HSM suspend tests
  riscv: sbi: Ensure SUSP test gets an interrupt
  riscv: sbi: Improve susp expected error output
  riscv: sbi: Improve interrupt handling cleanup
  lib/cpumask: Add some operators
  riscv: sbi: HSM suspend may not be supported
  riscv: sbi: Probe/skip SUSP
  riscv: sbi: susp: Check upper bits of sleep_type are ignored
  riscv: sbi: Add bad fid tests

 lib/cpumask.h     |  56 +++++++++
 riscv/sbi-fwft.c  |  17 ++-
 riscv/sbi-tests.h |   4 +
 riscv/sbi.c       | 292 +++++++++++++++++++++++++++++++++-------------
 4 files changed, 284 insertions(+), 85 deletions(-)

-- 
2.48.1


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

[kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Andrew Jones]

Until we fix opensbi mark these known failures as kfails so we can
pass CI.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi-fwft.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index b10c147f22dd..19340d6bb48c 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -63,11 +63,17 @@ static void fwft_check_base(void)
 		struct sbiret ret;
 
 		ret = fwft_get_raw(BIT(32));
-		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
+		if (ret.error == 0)
+			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
+		else
+			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
 				    "get feature with bit 32 set error");
 
 		ret = fwft_set_raw(BIT(32), 0, 0);
-		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
+		if (ret.error == 0)
+			report_kfail(true, false, "set feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
+		else
+			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
 				    "set feature with bit 32 set error");
 	}
 #endif
@@ -167,7 +173,10 @@ static void fwft_check_misaligned_exc_deleg(void)
 	ret = fwft_misaligned_exc_set(0, SBI_FWFT_SET_FLAG_LOCK);
 	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0 and lock");
 	ret = fwft_misaligned_exc_set(1, 0);
-	sbiret_report_error(&ret, SBI_ERR_LOCKED,
+	if (ret.error == SBI_ERR_DENIED)
+		report_kfail(true, false, "Set locked misaligned deleg feature to new value: SBI_ERR_LOCKED");
+	else
+		sbiret_report_error(&ret, SBI_ERR_LOCKED,
 			    "Set locked misaligned deleg feature to new value");
 	ret = fwft_misaligned_exc_get();
 	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg locked value 0");
-- 
2.48.1


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

[kvm-unit-tests PATCH 02/10] riscv: sbi: Ensure we have IPIs enabled for HSM suspend tests

[Andrew Jones]

We'll start cleaning up after each test, so the HSM tests need to
ensure they have IPIs setup themselves since they plan to use them.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi.c | 62 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/riscv/sbi.c b/riscv/sbi.c
index 7c7a2d2ddddb..d7bf758e23fb 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -62,7 +62,7 @@ static struct sbiret sbi_dbcn_write_byte(uint8_t byte)
 	return sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, byte, 0, 0, 0, 0, 0);
 }
 
-static struct sbiret sbi_hart_suspend(uint32_t suspend_type, unsigned long resume_addr, unsigned long opaque)
+static struct sbiret sbi_hart_suspend_raw(unsigned long suspend_type, unsigned long resume_addr, unsigned long opaque)
 {
 	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
 }
@@ -568,59 +568,71 @@ static void hart_start_invalid_hartid(void *data)
 		sbi_hsm_invalid_hartid_check = true;
 }
 
-static void hart_retentive_suspend(void *data)
+static void ipi_nop(struct pt_regs *regs)
+{
+	ipi_ack();
+}
+
+static void hart_suspend_and_wait_ipi(unsigned long suspend_type, unsigned long resume_addr,
+				      unsigned long opaque, bool returns, const char *typestr)
 {
 	unsigned long hartid = current_thread_info()->hartid;
-	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, 0, 0);
+	struct sbiret ret;
 
+	install_irq_handler(IRQ_S_SOFT, ipi_nop);
+	local_ipi_enable();
+	local_irq_enable();
+
+	ret = sbi_hart_suspend_raw(suspend_type, resume_addr, opaque);
 	if (ret.error)
-		report_fail("failed to retentive suspend cpu%d (hartid = %lx) (error=%ld)",
-			    smp_processor_id(), hartid, ret.error);
+		report_fail("failed to %s cpu%d (hartid = %lx) (error=%ld)",
+			    typestr, smp_processor_id(), hartid, ret.error);
+	else if (!returns)
+		report_fail("failed to %s cpu%d (hartid = %lx) (call should not return)",
+			    typestr, smp_processor_id(), hartid);
+
+	local_irq_disable();
+	local_ipi_disable();
+	install_irq_handler(IRQ_S_SOFT, NULL);
+}
+
+static void hart_retentive_suspend(void *data)
+{
+	hart_suspend_and_wait_ipi(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, 0, 0, true, "retentive suspend");
 }
 
 static void hart_non_retentive_suspend(void *data)
 {
-	unsigned long hartid = current_thread_info()->hartid;
 	unsigned long params[] = {
 		[SBI_HSM_MAGIC_IDX] = SBI_HSM_MAGIC,
-		[SBI_HSM_HARTID_IDX] = hartid,
+		[SBI_HSM_HARTID_IDX] = current_thread_info()->hartid,
 	};
-	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE,
-					     virt_to_phys(&sbi_hsm_check_non_retentive_suspend),
-					     virt_to_phys(params));
 
-	report_fail("failed to non-retentive suspend cpu%d (hartid = %lx) (error=%ld)",
-		    smp_processor_id(), hartid, ret.error);
+	hart_suspend_and_wait_ipi(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE,
+				  virt_to_phys(&sbi_hsm_check_non_retentive_suspend), virt_to_phys(params),
+				  false, "non-retentive suspend");
 }
 
 /* This test function is only being run on RV64 to verify that upper bits of suspend_type are ignored */
 static void hart_retentive_suspend_with_msb_set(void *data)
 {
-	unsigned long hartid = current_thread_info()->hartid;
 	unsigned long suspend_type = SBI_EXT_HSM_HART_SUSPEND_RETENTIVE | (_AC(1, UL) << (__riscv_xlen - 1));
-	struct sbiret ret = sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, 0, 0, 0, 0, 0);
 
-	if (ret.error)
-		report_fail("failed to retentive suspend cpu%d (hartid = %lx) with MSB set (error=%ld)",
-			    smp_processor_id(), hartid, ret.error);
+	hart_suspend_and_wait_ipi(suspend_type, 0, 0, true, "retentive suspend with MSB set");
 }
 
 /* This test function is only being run on RV64 to verify that upper bits of suspend_type are ignored */
 static void hart_non_retentive_suspend_with_msb_set(void *data)
 {
-	unsigned long hartid = current_thread_info()->hartid;
 	unsigned long suspend_type = SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE | (_AC(1, UL) << (__riscv_xlen - 1));
 	unsigned long params[] = {
 		[SBI_HSM_MAGIC_IDX] = SBI_HSM_MAGIC,
-		[SBI_HSM_HARTID_IDX] = hartid,
+		[SBI_HSM_HARTID_IDX] = current_thread_info()->hartid,
 	};
 
-	struct sbiret ret = sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type,
-				      virt_to_phys(&sbi_hsm_check_non_retentive_suspend), virt_to_phys(params),
-				      0, 0, 0);
-
-	report_fail("failed to non-retentive suspend cpu%d (hartid = %lx) with MSB set (error=%ld)",
-		    smp_processor_id(), hartid, ret.error);
+	hart_suspend_and_wait_ipi(suspend_type,
+				  virt_to_phys(&sbi_hsm_check_non_retentive_suspend), virt_to_phys(params),
+				  false, "non-retentive suspend with MSB set");
 }
 
 static bool hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status, unsigned long duration)
-- 
2.48.1


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

[kvm-unit-tests PATCH 03/10] riscv: sbi: Ensure SUSP test gets an interrupt

[Andrew Jones]

A system suspend may call WFI. Set a periodic timer to ensure we get
an interrupt and don't hang forever.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi-tests.h |  2 ++
 riscv/sbi.c       | 70 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
index 7a24e083a7b8..7c7fe30541e4 100644
--- a/riscv/sbi-tests.h
+++ b/riscv/sbi-tests.h
@@ -19,12 +19,14 @@
 #define SBI_SUSP_HARTID_IDX	2
 #define SBI_SUSP_TESTNUM_IDX	3
 #define SBI_SUSP_RESULTS_IDX	4
+#define SBI_SUSP_NR_IDX		5
 
 #define SBI_CSR_SSTATUS_IDX	0
 #define SBI_CSR_SIE_IDX		1
 #define SBI_CSR_STVEC_IDX	2
 #define SBI_CSR_SSCRATCH_IDX	3
 #define SBI_CSR_SATP_IDX	4
+#define SBI_CSR_NR_IDX		5
 
 #define SBI_SUSP_MAGIC		0x505b
 
diff --git a/riscv/sbi.c b/riscv/sbi.c
index d7bf758e23fb..dd5cb6fa26bf 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -158,6 +158,19 @@ static bool get_invalid_addr(phys_addr_t *paddr, bool allow_default)
 	return false;
 }
 
+static void timer_setup(void (*handler)(struct pt_regs *))
+{
+	install_irq_handler(IRQ_S_TIMER, handler);
+	timer_irq_enable();
+}
+
+static void timer_teardown(void)
+{
+	timer_irq_disable();
+	timer_stop();
+	install_irq_handler(IRQ_S_TIMER, NULL);
+}
+
 static void check_base(void)
 {
 	struct sbiret ret;
@@ -534,18 +547,6 @@ static void hsm_timer_irq_handler(struct pt_regs *regs)
 	sbi_hsm_timer_fired = true;
 }
 
-static void hsm_timer_setup(void)
-{
-	install_irq_handler(IRQ_S_TIMER, hsm_timer_irq_handler);
-	timer_irq_enable();
-}
-
-static void hsm_timer_teardown(void)
-{
-	timer_irq_disable();
-	install_irq_handler(IRQ_S_TIMER, NULL);
-}
-
 static void hart_check_already_started(void *data)
 {
 	struct sbiret ret;
@@ -753,7 +754,7 @@ static void check_hsm(void)
 
 	cpumask_copy(&secondary_cpus_mask, &cpu_present_mask);
 	cpumask_clear_cpu(me, &secondary_cpus_mask);
-	hsm_timer_setup();
+	timer_setup(hsm_timer_irq_handler);
 	local_irq_enable();
 
 	/* Assume that previous tests have not cleaned up and stopped the secondary harts */
@@ -975,7 +976,7 @@ sbi_hsm_hart_stop_tests:
 
 	if (__riscv_xlen == 32 || ipi_unavailable) {
 		local_irq_disable();
-		hsm_timer_teardown();
+		timer_teardown();
 		report_prefix_pop();
 		return;
 	}
@@ -1084,7 +1085,7 @@ sbi_hsm_hart_stop_tests:
 	report(count, "secondary hart stopped after suspension tests with MSB set");
 
 	local_irq_disable();
-	hsm_timer_teardown();
+	timer_teardown();
 	report_prefix_popn(2);
 }
 
@@ -1215,6 +1216,12 @@ static void check_dbcn(void)
 void sbi_susp_resume(unsigned long hartid, unsigned long opaque);
 jmp_buf sbi_susp_jmp;
 
+#define SBI_SUSP_TIMER_DURATION_US 500000
+static void susp_timer(struct pt_regs *regs)
+{
+	timer_start(SBI_SUSP_TIMER_DURATION_US);
+}
+
 struct susp_params {
 	unsigned long sleep_type;
 	unsigned long resume_addr;
@@ -1226,9 +1233,17 @@ struct susp_params {
 static bool susp_basic_prep(unsigned long ctx[], struct susp_params *params)
 {
 	int cpu, me = smp_processor_id();
+	unsigned long *csrs;
 	struct sbiret ret;
 	cpumask_t mask;
 
+	csrs = (unsigned long *)ctx[SBI_SUSP_CSRS_IDX];
+	csrs[SBI_CSR_SSTATUS_IDX] = csr_read(CSR_SSTATUS);
+	csrs[SBI_CSR_SIE_IDX] = csr_read(CSR_SIE);
+	csrs[SBI_CSR_STVEC_IDX] = csr_read(CSR_STVEC);
+	csrs[SBI_CSR_SSCRATCH_IDX] = csr_read(CSR_SSCRATCH);
+	csrs[SBI_CSR_SATP_IDX] = csr_read(CSR_SATP);
+
 	memset(params, 0, sizeof(*params));
 	params->sleep_type = 0; /* suspend-to-ram */
 	params->resume_addr = virt_to_phys(sbi_susp_resume);
@@ -1352,19 +1367,11 @@ static bool susp_one_prep(unsigned long ctx[], struct susp_params *params)
 
 static void check_susp(void)
 {
-	unsigned long csrs[] = {
-		[SBI_CSR_SSTATUS_IDX] = csr_read(CSR_SSTATUS),
-		[SBI_CSR_SIE_IDX] = csr_read(CSR_SIE),
-		[SBI_CSR_STVEC_IDX] = csr_read(CSR_STVEC),
-		[SBI_CSR_SSCRATCH_IDX] = csr_read(CSR_SSCRATCH),
-		[SBI_CSR_SATP_IDX] = csr_read(CSR_SATP),
-	};
-	unsigned long ctx[] = {
+	unsigned long csrs[SBI_CSR_NR_IDX];
+	unsigned long ctx[SBI_SUSP_NR_IDX] = {
 		[SBI_SUSP_MAGIC_IDX] = SBI_SUSP_MAGIC,
 		[SBI_SUSP_CSRS_IDX] = (unsigned long)csrs,
 		[SBI_SUSP_HARTID_IDX] = current_thread_info()->hartid,
-		[SBI_SUSP_TESTNUM_IDX] = 0,
-		[SBI_SUSP_RESULTS_IDX] = 0,
 	};
 	enum {
 #define SUSP_FIRST_TESTNUM 1
@@ -1393,6 +1400,10 @@ static void check_susp(void)
 
 	report_prefix_push("susp");
 
+	timer_setup(susp_timer);
+	local_irq_enable();
+	timer_start(SBI_SUSP_TIMER_DURATION_US);
+
 	ret = sbi_ecall(SBI_EXT_SUSP, 1, 0, 0, 0, 0, 0, 0);
 	report(ret.error == SBI_ERR_NOT_SUPPORTED, "funcid != 0 not supported");
 
@@ -1402,6 +1413,8 @@ static void check_susp(void)
 		ctx[SBI_SUSP_TESTNUM_IDX] = i;
 		ctx[SBI_SUSP_RESULTS_IDX] = 0;
 
+		local_irq_disable();
+
 		assert(susp_tests[i].prep);
 		if (!susp_tests[i].prep(ctx, &params)) {
 			report_prefix_pop();
@@ -1411,6 +1424,8 @@ static void check_susp(void)
 		if ((testnum = setjmp(sbi_susp_jmp)) == 0) {
 			ret = sbi_system_suspend(params.sleep_type, params.resume_addr, params.opaque);
 
+			local_irq_enable();
+
 			if (!params.returns && ret.error == SBI_ERR_NOT_SUPPORTED) {
 				report_skip("SUSP not supported?");
 				report_prefix_popn(2);
@@ -1428,12 +1443,17 @@ static void check_susp(void)
 		}
 		assert(testnum == i);
 
+		local_irq_enable();
+
 		if (susp_tests[i].check)
 			susp_tests[i].check(ctx, &params);
 
 		report_prefix_pop();
 	}
 
+	local_irq_disable();
+	timer_teardown();
+
 	report_prefix_pop();
 }
 
-- 
2.48.1


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

[kvm-unit-tests PATCH 04/10] riscv: sbi: Improve susp expected error output

[Andrew Jones]

Improve the output of expected error tests. We don't use
sbiret_report_error() because we're not passing in an SBI_ERR_*
name to stringify.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/riscv/sbi.c b/riscv/sbi.c
index dd5cb6fa26bf..ceeb0d8d2779 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -1433,9 +1433,8 @@ static void check_susp(void)
 			} else if (!params.returns) {
 				report_fail("unexpected return with error: %ld, value: %ld", ret.error, ret.value);
 			} else {
-				report(ret.error == params.ret.error, "expected sbi.error");
-				if (ret.error != params.ret.error)
-					report_info("expected error %ld, received %ld", params.ret.error, ret.error);
+				if (!report(ret.error == params.ret.error, "got expected sbi.error (%ld)", params.ret.error))
+					report_info("expected sbi.error %ld, received %ld", params.ret.error, ret.error);
 			}
 
 			report_prefix_pop();
-- 
2.48.1


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

[kvm-unit-tests PATCH 05/10] riscv: sbi: Improve interrupt handling cleanup

[Andrew Jones]

Each test should clean up after itself so following tests don't
need to worry about the current state and can just do its own
prep.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/riscv/sbi.c b/riscv/sbi.c
index ceeb0d8d2779..7bffaac07283 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -394,6 +394,8 @@ static void ipi_hart_wait(void *data)
 	timer_stop();
 	local_ipi_disable();
 	timer_irq_disable();
+	install_irq_handler(IRQ_S_SOFT, NULL);
+	install_irq_handler(IRQ_S_TIMER, NULL);
 
 	cpumask_set_cpu(me, &ipi_done);
 }
@@ -1395,9 +1397,6 @@ static void check_susp(void)
 	struct sbiret ret;
 	int testnum, i;
 
-	local_irq_disable();
-	timer_stop();
-
 	report_prefix_push("susp");
 
 	timer_setup(susp_timer);
@@ -1428,8 +1427,8 @@ static void check_susp(void)
 
 			if (!params.returns && ret.error == SBI_ERR_NOT_SUPPORTED) {
 				report_skip("SUSP not supported?");
-				report_prefix_popn(2);
-				return;
+				report_prefix_pop();
+				goto out;
 			} else if (!params.returns) {
 				report_fail("unexpected return with error: %ld, value: %ld", ret.error, ret.value);
 			} else {
@@ -1450,6 +1449,7 @@ static void check_susp(void)
 		report_prefix_pop();
 	}
 
+out:
 	local_irq_disable();
 	timer_teardown();
 
-- 
2.48.1


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

[kvm-unit-tests PATCH 06/10] lib/cpumask: Add some operators

[Andrew Jones]

We have a need for cpumask_andnot(). Add that and a few friends.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/cpumask.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/lib/cpumask.h b/lib/cpumask.h
index b4cd83a66a56..3f03d3370f85 100644
--- a/lib/cpumask.h
+++ b/lib/cpumask.h
@@ -72,6 +72,62 @@ static inline bool cpumask_subset(const struct cpumask *src1, const struct cpuma
 	return !lastmask || !((cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i]) & lastmask);
 }
 
+/* false if dst is empty */
+static inline bool cpumask_and(cpumask_t *dst, const cpumask_t *src1, const cpumask_t *src2)
+{
+	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
+	unsigned long ret = 0;
+	int i;
+
+	for (i = 0; i < BIT_WORD(nr_cpus); ++i) {
+		cpumask_bits(dst)[i] = cpumask_bits(src1)[i] & cpumask_bits(src2)[i];
+		ret |= cpumask_bits(dst)[i];
+	}
+
+	cpumask_bits(dst)[i] = (cpumask_bits(src1)[i] & cpumask_bits(src2)[i]) & lastmask;
+
+	return ret | cpumask_bits(dst)[i];
+}
+
+static inline void cpumask_or(cpumask_t *dst, const cpumask_t *src1, const cpumask_t *src2)
+{
+	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
+	int i;
+
+	for (i = 0; i < BIT_WORD(nr_cpus); ++i)
+		cpumask_bits(dst)[i] = cpumask_bits(src1)[i] | cpumask_bits(src2)[i];
+
+	cpumask_bits(dst)[i] = (cpumask_bits(src1)[i] | cpumask_bits(src2)[i]) & lastmask;
+}
+
+static inline void cpumask_xor(cpumask_t *dst, const cpumask_t *src1, const cpumask_t *src2)
+{
+	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
+	int i;
+
+	for (i = 0; i < BIT_WORD(nr_cpus); ++i)
+		cpumask_bits(dst)[i] = cpumask_bits(src1)[i] ^ cpumask_bits(src2)[i];
+
+	cpumask_bits(dst)[i] = (cpumask_bits(src1)[i] ^ cpumask_bits(src2)[i]) & lastmask;
+}
+
+/* false if dst is empty */
+static inline bool cpumask_andnot(cpumask_t *dst, const cpumask_t *src1, const cpumask_t *src2)
+{
+	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
+	unsigned long ret = 0;
+	int i;
+
+	for (i = 0; i < BIT_WORD(nr_cpus); ++i) {
+		cpumask_bits(dst)[i] = cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i];
+		ret |= cpumask_bits(dst)[i];
+	}
+
+	cpumask_bits(dst)[i] = (cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i]) & lastmask;
+
+	return ret | cpumask_bits(dst)[i];
+}
+
 static inline bool cpumask_equal(const struct cpumask *src1, const struct cpumask *src2)
 {
 	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
-- 
2.48.1


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

[kvm-unit-tests PATCH 07/10] riscv: sbi: HSM suspend may not be supported

[Andrew Jones]

The spec doesn't require that any suspend types are supported and
indeed KVM doesn't support non-retentive suspend. Tolerate
SBI_ERR_NOT_SUPPORTED.

TODO: The HSM tests have lots of duplicated code and this patch
adds even more. We need to refactor them and we should break them
out into their own riscv/sbi-hsm.c file too.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi.c | 87 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 70 insertions(+), 17 deletions(-)

diff --git a/riscv/sbi.c b/riscv/sbi.c
index 7bffaac07283..a0a8331b04bd 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -571,6 +571,8 @@ static void hart_start_invalid_hartid(void *data)
 		sbi_hsm_invalid_hartid_check = true;
 }
 
+static cpumask_t hsm_suspend_not_supported;
+
 static void ipi_nop(struct pt_regs *regs)
 {
 	ipi_ack();
@@ -587,7 +589,9 @@ static void hart_suspend_and_wait_ipi(unsigned long suspend_type, unsigned long
 	local_irq_enable();
 
 	ret = sbi_hart_suspend_raw(suspend_type, resume_addr, opaque);
-	if (ret.error)
+	if (ret.error == SBI_ERR_NOT_SUPPORTED)
+		cpumask_set_cpu(smp_processor_id(), &hsm_suspend_not_supported);
+	else if (ret.error)
 		report_fail("failed to %s cpu%d (hartid = %lx) (error=%ld)",
 			    typestr, smp_processor_id(), hartid, ret.error);
 	else if (!returns)
@@ -707,7 +711,7 @@ static void check_hsm(void)
 {
 	struct sbiret ret;
 	unsigned long hartid;
-	cpumask_t secondary_cpus_mask, mask;
+	cpumask_t secondary_cpus_mask, mask, resume_mask;
 	struct hart_state_transition_info transition_states;
 	bool ipi_unavailable = false;
 	int cpu, me = smp_processor_id();
@@ -715,7 +719,7 @@ static void check_hsm(void)
 	unsigned long hsm_timer_duration = getenv("SBI_HSM_TIMER_DURATION")
 					 ? strtol(getenv("SBI_HSM_TIMER_DURATION"), NULL, 0) : 200000;
 	unsigned long sbi_hsm_hart_start_params[NR_CPUS * SBI_HSM_NUM_OF_PARAMS];
-	int count, check;
+	int count, check, expected_count, resume_count;
 
 	max_cpus = MIN(MIN(max_cpus, nr_cpus), cpumask_weight(&cpu_present_mask));
 
@@ -879,6 +883,7 @@ static void check_hsm(void)
 		goto sbi_hsm_hart_stop_tests;
 	}
 
+	cpumask_clear(&hsm_suspend_not_supported);
 	on_cpumask_async(&secondary_cpus_mask, hart_retentive_suspend, NULL);
 
 	transition_states = (struct hart_state_transition_info) {
@@ -888,22 +893,36 @@ static void check_hsm(void)
 	};
 	count = hart_wait_state_transition(&secondary_cpus_mask, hsm_timer_duration, &transition_states);
 
-	report(count == max_cpus - 1, "all secondary harts retentive suspended");
+	expected_count = max_cpus - 1 - cpumask_weight(&hsm_suspend_not_supported);
+
+	if (expected_count != 0) {
+		if (expected_count != max_cpus - 1)
+			report_info("not all harts support retentive suspend");
+		report(count == expected_count, "supporting secondary harts retentive suspended");
+	} else {
+		report_skip("retentive suspend not supported by any harts");
+		goto nonret_suspend_tests;
+	}
+
+	cpumask_andnot(&resume_mask, &secondary_cpus_mask, &hsm_suspend_not_supported);
+	resume_count = cpumask_weight(&resume_mask);
 
 	/* Ignore the return value since we check the status of each hart anyway */
-	sbi_send_ipi_cpumask(&secondary_cpus_mask);
+	sbi_send_ipi_cpumask(&resume_mask);
 
 	transition_states = (struct hart_state_transition_info) {
 		.initial_state = SBI_EXT_HSM_SUSPENDED,
 		.intermediate_state = SBI_EXT_HSM_RESUME_PENDING,
 		.final_state = SBI_EXT_HSM_STARTED,
 	};
-	count = hart_wait_state_transition(&secondary_cpus_mask, hsm_timer_duration, &transition_states);
+	count = hart_wait_state_transition(&resume_mask, hsm_timer_duration, &transition_states);
 
-	report(count == max_cpus - 1, "all secondary harts retentive resumed");
+	report(count == resume_count, "supporting secondary harts retentive resumed");
 
+nonret_suspend_tests:
 	hart_wait_until_idle(&secondary_cpus_mask, hsm_timer_duration);
 
+	cpumask_clear(&hsm_suspend_not_supported);
 	on_cpumask_async(&secondary_cpus_mask, hart_non_retentive_suspend, NULL);
 
 	transition_states = (struct hart_state_transition_info) {
@@ -913,20 +932,32 @@ static void check_hsm(void)
 	};
 	count = hart_wait_state_transition(&secondary_cpus_mask, hsm_timer_duration, &transition_states);
 
-	report(count == max_cpus - 1, "all secondary harts non-retentive suspended");
+	expected_count = max_cpus - 1 - cpumask_weight(&hsm_suspend_not_supported);
+
+	if (expected_count != 0) {
+		if (expected_count != max_cpus - 1)
+			report_info("not all harts support non-retentive suspend");
+		report(count == expected_count, "supporting secondary harts non-retentive suspended");
+	} else {
+		report_skip("non-retentive suspend not supported by any harts");
+		goto hsm_suspend_tests_done;
+	}
+
+	cpumask_andnot(&resume_mask, &secondary_cpus_mask, &hsm_suspend_not_supported);
+	resume_count = cpumask_weight(&resume_mask);
 
 	/* Ignore the return value since we check the status of each hart anyway */
-	sbi_send_ipi_cpumask(&secondary_cpus_mask);
+	sbi_send_ipi_cpumask(&resume_mask);
 
 	transition_states = (struct hart_state_transition_info) {
 		.initial_state = SBI_EXT_HSM_SUSPENDED,
 		.intermediate_state = SBI_EXT_HSM_RESUME_PENDING,
 		.final_state = SBI_EXT_HSM_STARTED,
 	};
-	count = hart_wait_state_transition(&secondary_cpus_mask, hsm_timer_duration, &transition_states);
+	count = hart_wait_state_transition(&resume_mask, hsm_timer_duration, &transition_states);
 	check = 0;
 
-	for_each_cpu(cpu, &secondary_cpus_mask) {
+	for_each_cpu(cpu, &resume_mask) {
 		sbi_hsm_timer_fired = false;
 		timer_start(hsm_timer_duration);
 
@@ -952,15 +983,16 @@ static void check_hsm(void)
 			check++;
 	}
 
-	report(count == max_cpus - 1, "all secondary harts non-retentive resumed");
-	report(check == max_cpus - 1, "all secondary harts have expected register values after non-retentive resume");
+	report(count == resume_count, "supporting secondary harts non-retentive resumed");
+	report(check == resume_count, "supporting secondary harts have expected register values after non-retentive resume");
 
+hsm_suspend_tests_done:
 	report_prefix_pop();
 
 sbi_hsm_hart_stop_tests:
 	report_prefix_push("hart_stop");
 
-	if (ipi_unavailable)
+	if (ipi_unavailable || expected_count == 0)
 		on_cpumask_async(&secondary_cpus_mask, stop_cpu, NULL);
 	else
 		memset(sbi_hsm_stop_hart, 1, sizeof(sbi_hsm_stop_hart));
@@ -994,6 +1026,7 @@ sbi_hsm_hart_stop_tests:
 	/* Boot up the secondary cpu and let it proceed to the idle loop */
 	on_cpu(cpu, start_cpu, NULL);
 
+	cpumask_clear(&hsm_suspend_not_supported);
 	on_cpu_async(cpu, hart_retentive_suspend_with_msb_set, NULL);
 
 	transition_states = (struct hart_state_transition_info) {
@@ -1003,7 +1036,14 @@ sbi_hsm_hart_stop_tests:
 	};
 	count = hart_wait_state_transition(&mask, hsm_timer_duration, &transition_states);
 
-	report(count, "secondary hart retentive suspended with MSB set");
+	expected_count = 1 - cpumask_weight(&hsm_suspend_not_supported);
+
+	if (expected_count) {
+		report(count == expected_count, "retentive suspend with MSB set");
+	} else {
+		report_skip("retentive suspend not supported by cpu%d", cpu);
+		goto nonret_suspend_with_msb;
+	}
 
 	/* Ignore the return value since we manually validate the status of the hart anyway */
 	sbi_send_ipi_cpu(cpu);
@@ -1017,10 +1057,12 @@ sbi_hsm_hart_stop_tests:
 
 	report(count, "secondary hart retentive resumed with MSB set");
 
+nonret_suspend_with_msb:
 	/* Reset these flags so that we can reuse them for the non-retentive suspension test */
 	sbi_hsm_stop_hart[cpu] = 0;
 	sbi_hsm_non_retentive_hart_suspend_checks[cpu] = 0;
 
+	cpumask_clear(&hsm_suspend_not_supported);
 	on_cpu_async(cpu, hart_non_retentive_suspend_with_msb_set, NULL);
 
 	transition_states = (struct hart_state_transition_info) {
@@ -1030,7 +1072,14 @@ sbi_hsm_hart_stop_tests:
 	};
 	count = hart_wait_state_transition(&mask, hsm_timer_duration, &transition_states);
 
-	report(count, "secondary hart non-retentive suspended with MSB set");
+	expected_count = 1 - cpumask_weight(&hsm_suspend_not_supported);
+
+	if (expected_count) {
+		report(count == expected_count, "non-retentive suspend with MSB set");
+	} else {
+		report_skip("non-retentive suspend not supported by cpu%d", cpu);
+		goto hsm_hart_stop_test;
+	}
 
 	/* Ignore the return value since we manually validate the status of the hart anyway */
 	sbi_send_ipi_cpu(cpu);
@@ -1071,11 +1120,15 @@ sbi_hsm_hart_stop_tests:
 	report(count, "secondary hart non-retentive resumed with MSB set");
 	report(check, "secondary hart has expected register values after non-retentive resume with MSB set");
 
+hsm_hart_stop_test:
 	report_prefix_pop();
 
 	report_prefix_push("hart_stop");
 
-	sbi_hsm_stop_hart[cpu] = 1;
+	if (expected_count == 0)
+		on_cpu_async(cpu, stop_cpu, NULL);
+	else
+		sbi_hsm_stop_hart[cpu] = 1;
 
 	transition_states = (struct hart_state_transition_info) {
 		.initial_state = SBI_EXT_HSM_STARTED,
-- 
2.48.1


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

[kvm-unit-tests PATCH 08/10] riscv: sbi: Probe/skip SUSP

[Andrew Jones]

Cleanly skip testing suspend when the SUSP extension isn't available.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/riscv/sbi.c b/riscv/sbi.c
index a0a8331b04bd..e5f6e1c72ae6 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -1452,6 +1452,12 @@ static void check_susp(void)
 
 	report_prefix_push("susp");
 
+	if (!sbi_probe(SBI_EXT_SUSP)) {
+		report_skip("SUSP extension not available");
+		report_prefix_pop();
+		return;
+	}
+
 	timer_setup(susp_timer);
 	local_irq_enable();
 	timer_start(SBI_SUSP_TIMER_DURATION_US);
@@ -1479,7 +1485,7 @@ static void check_susp(void)
 			local_irq_enable();
 
 			if (!params.returns && ret.error == SBI_ERR_NOT_SUPPORTED) {
-				report_skip("SUSP not supported?");
+				report_fail("probing claims support, but it's not?");
 				report_prefix_pop();
 				goto out;
 			} else if (!params.returns) {
-- 
2.48.1


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

[kvm-unit-tests PATCH 09/10] riscv: sbi: susp: Check upper bits of sleep_type are ignored

[Andrew Jones]

When an SBI function parameter has a specific bit width which is
less than that of a long, then the upper bits should be ignored
by the SBI implementation. I.e. since sleep_type is a uint32_t,
then on rv64 both 0 and 0x1_0000_0000 are valid and mean suspend
to ram.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/riscv/sbi.c b/riscv/sbi.c
index e5f6e1c72ae6..1d9d1871a28b 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -67,7 +67,7 @@ static struct sbiret sbi_hart_suspend_raw(unsigned long suspend_type, unsigned l
 	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
 }
 
-static struct sbiret sbi_system_suspend(uint32_t sleep_type, unsigned long resume_addr, unsigned long opaque)
+static struct sbiret sbi_system_suspend_raw(unsigned long sleep_type, unsigned long resume_addr, unsigned long opaque)
 {
 	return sbi_ecall(SBI_EXT_SUSP, 0, sleep_type, resume_addr, opaque, 0, 0, 0);
 }
@@ -1367,6 +1367,19 @@ static bool susp_type_prep(unsigned long ctx[], struct susp_params *params)
 	return true;
 }
 
+#if __riscv_xlen != 32
+static bool susp_type_prep2(unsigned long ctx[], struct susp_params *params)
+{
+	bool r;
+
+	r = susp_basic_prep(ctx, params);
+	assert(r);
+	params->sleep_type = BIT(32);
+
+	return true;
+}
+#endif
+
 static bool susp_badaddr_prep(unsigned long ctx[], struct susp_params *params)
 {
 	phys_addr_t badaddr;
@@ -1432,6 +1445,7 @@ static void check_susp(void)
 #define SUSP_FIRST_TESTNUM 1
 		SUSP_BASIC = SUSP_FIRST_TESTNUM,
 		SUSP_TYPE,
+		SUSP_TYPE2,
 		SUSP_BAD_ADDR,
 		SUSP_ONE_ONLINE,
 		NR_SUSP_TESTS,
@@ -1441,10 +1455,13 @@ static void check_susp(void)
 		bool (*prep)(unsigned long ctx[], struct susp_params *params);
 		void (*check)(unsigned long ctx[], struct susp_params *params);
 	} susp_tests[] = {
-		[SUSP_BASIC]		= { "basic",		susp_basic_prep,	susp_basic_check,	},
-		[SUSP_TYPE]		= { "sleep_type",	susp_type_prep,					},
-		[SUSP_BAD_ADDR]		= { "bad addr",		susp_badaddr_prep,				},
-		[SUSP_ONE_ONLINE]	= { "one cpu online",	susp_one_prep,					},
+		[SUSP_BASIC]		= { "basic",			susp_basic_prep,	susp_basic_check,	},
+		[SUSP_TYPE]		= { "sleep_type",		susp_type_prep,					},
+#if __riscv_xlen != 32
+		[SUSP_TYPE2]		= { "sleep_type upper bits",	susp_type_prep2,	susp_basic_check	},
+#endif
+		[SUSP_BAD_ADDR]		= { "bad addr",			susp_badaddr_prep,				},
+		[SUSP_ONE_ONLINE]	= { "one cpu online",		susp_one_prep,					},
 	};
 	struct susp_params params;
 	struct sbiret ret;
@@ -1466,6 +1483,9 @@ static void check_susp(void)
 	report(ret.error == SBI_ERR_NOT_SUPPORTED, "funcid != 0 not supported");
 
 	for (i = SUSP_FIRST_TESTNUM; i < NR_SUSP_TESTS; i++) {
+		if (!susp_tests[i].name)
+			continue;
+
 		report_prefix_push(susp_tests[i].name);
 
 		ctx[SBI_SUSP_TESTNUM_IDX] = i;
@@ -1480,7 +1500,7 @@ static void check_susp(void)
 		}
 
 		if ((testnum = setjmp(sbi_susp_jmp)) == 0) {
-			ret = sbi_system_suspend(params.sleep_type, params.resume_addr, params.opaque);
+			ret = sbi_system_suspend_raw(params.sleep_type, params.resume_addr, params.opaque);
 
 			local_irq_enable();
 
-- 
2.48.1


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

[kvm-unit-tests PATCH 10/10] riscv: sbi: Add bad fid tests

[Andrew Jones]

Ensure we get the correct error code when attempting to use an
invalid FID.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi-fwft.c  |  2 ++
 riscv/sbi-tests.h |  2 ++
 riscv/sbi.c       | 18 ++++++++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index 19340d6bb48c..005edec43403 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -194,6 +194,8 @@ void check_fwft(void)
 		return;
 	}
 
+	sbi_bad_fid(SBI_EXT_FWFT);
+
 	fwft_check_base();
 	fwft_check_misaligned_exc_deleg();
 
diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
index 7c7fe30541e4..f8ce3c269d48 100644
--- a/riscv/sbi-tests.h
+++ b/riscv/sbi-tests.h
@@ -70,5 +70,7 @@
 #define sbiret_check(ret, expected_error, expected_value) \
 	sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
 
+void sbi_bad_fid(int ext);
+
 #endif /* __ASSEMBLY__ */
 #endif /* _RISCV_SBI_TESTS_H_ */
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 1d9d1871a28b..0404bb81317d 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -72,6 +72,12 @@ static struct sbiret sbi_system_suspend_raw(unsigned long sleep_type, unsigned l
 	return sbi_ecall(SBI_EXT_SUSP, 0, sleep_type, resume_addr, opaque, 0, 0, 0);
 }
 
+void sbi_bad_fid(int ext)
+{
+	struct sbiret ret = sbi_ecall(ext, 0xbad, 0, 0, 0, 0, 0, 0);
+	sbiret_report_error(&ret, SBI_ERR_NOT_SUPPORTED, "Bad FID");
+}
+
 static void start_cpu(void *data)
 {
 	/* nothing to do */
@@ -178,6 +184,8 @@ static void check_base(void)
 
 	report_prefix_push("base");
 
+	sbi_bad_fid(SBI_EXT_BASE);
+
 	ret = sbi_base(SBI_EXT_BASE_GET_SPEC_VERSION, 0);
 
 	report_prefix_push("spec_version");
@@ -331,6 +339,8 @@ static void check_time(void)
 		return;
 	}
 
+	sbi_bad_fid(SBI_EXT_TIME);
+
 	report_prefix_push("set_timer");
 
 	install_irq_handler(IRQ_S_TIMER, timer_irq_handler);
@@ -438,6 +448,8 @@ static void check_ipi(void)
 		return;
 	}
 
+	sbi_bad_fid(SBI_EXT_IPI);
+
 	if (nr_cpus_present < 2) {
 		report_skip("At least 2 cpus required");
 		report_prefix_pop();
@@ -731,6 +743,8 @@ static void check_hsm(void)
 		return;
 	}
 
+	sbi_bad_fid(SBI_EXT_HSM);
+
 	report_prefix_push("hart_get_status");
 
 	hartid = current_thread_info()->hartid;
@@ -1210,6 +1224,8 @@ static void check_dbcn(void)
 		return;
 	}
 
+	sbi_bad_fid(SBI_EXT_DBCN);
+
 	report_prefix_push("write");
 
 	dbcn_write_test(DBCN_WRITE_TEST_STRING, num_bytes, false);
@@ -1475,6 +1491,8 @@ static void check_susp(void)
 		return;
 	}
 
+	sbi_bad_fid(SBI_EXT_SUSP);
+
 	timer_setup(susp_timer);
 	local_irq_enable();
 	timer_start(SBI_SUSP_TIMER_DURATION_US);
-- 
2.48.1


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

Re: [kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Clément Léger]

On 21/02/2025 16:55, Andrew Jones wrote:
> Until we fix opensbi mark these known failures as kfails so we can
> pass CI.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  riscv/sbi-fwft.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index b10c147f22dd..19340d6bb48c 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -63,11 +63,17 @@ static void fwft_check_base(void)
>  		struct sbiret ret;
>  
>  		ret = fwft_get_raw(BIT(32));
> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> +		if (ret.error == 0)
> +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> +		else
> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>  				    "get feature with bit 32 set error");
>  
>  		ret = fwft_set_raw(BIT(32), 0, 0);
> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> +		if (ret.error == 0)
> +			report_kfail(true, false, "set feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> +		else
> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>  				    "set feature with bit 32 set error");
>  	}
>  #endif
> @@ -167,7 +173,10 @@ static void fwft_check_misaligned_exc_deleg(void)
>  	ret = fwft_misaligned_exc_set(0, SBI_FWFT_SET_FLAG_LOCK);
>  	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0 and lock");
>  	ret = fwft_misaligned_exc_set(1, 0);
> -	sbiret_report_error(&ret, SBI_ERR_LOCKED,
> +	if (ret.error == SBI_ERR_DENIED)
> +		report_kfail(true, false, "Set locked misaligned deleg feature to new value: SBI_ERR_LOCKED");
> +	else
> +		sbiret_report_error(&ret, SBI_ERR_LOCKED,
>  			    "Set locked misaligned deleg feature to new value");
>  	ret = fwft_misaligned_exc_get();
>  	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg locked value 0");

Hi Andrew,

Reviewed-by: Clément Léger <cleger@rivosinc.com>

Thanks,

Clément

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

Re: [kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Clément Léger]

On 21/02/2025 16:55, Andrew Jones wrote:
> Until we fix opensbi mark these known failures as kfails so we can
> pass CI.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  riscv/sbi-fwft.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index b10c147f22dd..19340d6bb48c 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -63,11 +63,17 @@ static void fwft_check_base(void)
>  		struct sbiret ret;
>  
>  		ret = fwft_get_raw(BIT(32));
> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> +		if (ret.error == 0)
> +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> +		else
> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>  				    "get feature with bit 32 set error");


Hey Andrew,

I'm actually a bit confused here. As raised by Anup, the spec states that:

"Every SBI function should prefer unsigned long as the data type. It
keeps the specification simple and easily adaptable for all RISC-V ISA
types. In case the data is defined as 32bit wide, higher privilege
software must ensure that it only uses 32 bit data."

Does this means 64 bits value are truncated to 32 bits ?

Thanks,

Clément

>  
>  		ret = fwft_set_raw(BIT(32), 0, 0);
> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> +		if (ret.error == 0)
> +			report_kfail(true, false, "set feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> +		else
> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>  				    "set feature with bit 32 set error");
>  	}
>  #endif
> @@ -167,7 +173,10 @@ static void fwft_check_misaligned_exc_deleg(void)
>  	ret = fwft_misaligned_exc_set(0, SBI_FWFT_SET_FLAG_LOCK);
>  	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0 and lock");
>  	ret = fwft_misaligned_exc_set(1, 0);
> -	sbiret_report_error(&ret, SBI_ERR_LOCKED,
> +	if (ret.error == SBI_ERR_DENIED)
> +		report_kfail(true, false, "Set locked misaligned deleg feature to new value: SBI_ERR_LOCKED");
> +	else
> +		sbiret_report_error(&ret, SBI_ERR_LOCKED,
>  			    "Set locked misaligned deleg feature to new value");
>  	ret = fwft_misaligned_exc_get();
>  	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg locked value 0");


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

Re: [kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Andrew Jones]

On Tue, Feb 25, 2025 at 11:14:13AM +0100, Clément Léger wrote:
> 
> 
> On 21/02/2025 16:55, Andrew Jones wrote:
> > Until we fix opensbi mark these known failures as kfails so we can
> > pass CI.
> > 
> > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> > ---
> >  riscv/sbi-fwft.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> > index b10c147f22dd..19340d6bb48c 100644
> > --- a/riscv/sbi-fwft.c
> > +++ b/riscv/sbi-fwft.c
> > @@ -63,11 +63,17 @@ static void fwft_check_base(void)
> >  		struct sbiret ret;
> >  
> >  		ret = fwft_get_raw(BIT(32));
> > -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> > +		if (ret.error == 0)
> > +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> > +		else
> > +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >  				    "get feature with bit 32 set error");
> 
> 
> Hey Andrew,
> 
> I'm actually a bit confused here. As raised by Anup, the spec states that:
> 
> "Every SBI function should prefer unsigned long as the data type. It
> keeps the specification simple and easily adaptable for all RISC-V ISA
> types. In case the data is defined as 32bit wide, higher privilege
> software must ensure that it only uses 32 bit data."
> 
> Does this means 64 bits value are truncated to 32 bits ?

Ah, yes. I had noticed that required truncation while doing some KVM fixes
and patch 9 of this series is even adding a test for it for SUSP. I just
didn't look closely enough at why this FWFT test was failing. The test
should get SBI_SUCCESS, as it does, but it should also check that
feature = BIT(32) behaves the same as features = 1.

Thanks,
drew

> 
> Thanks,
> 
> Clément
> 
> >  
> >  		ret = fwft_set_raw(BIT(32), 0, 0);
> > -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> > +		if (ret.error == 0)
> > +			report_kfail(true, false, "set feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> > +		else
> > +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >  				    "set feature with bit 32 set error");
> >  	}
> >  #endif
> > @@ -167,7 +173,10 @@ static void fwft_check_misaligned_exc_deleg(void)
> >  	ret = fwft_misaligned_exc_set(0, SBI_FWFT_SET_FLAG_LOCK);
> >  	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0 and lock");
> >  	ret = fwft_misaligned_exc_set(1, 0);
> > -	sbiret_report_error(&ret, SBI_ERR_LOCKED,
> > +	if (ret.error == SBI_ERR_DENIED)
> > +		report_kfail(true, false, "Set locked misaligned deleg feature to new value: SBI_ERR_LOCKED");
> > +	else
> > +		sbiret_report_error(&ret, SBI_ERR_LOCKED,
> >  			    "Set locked misaligned deleg feature to new value");
> >  	ret = fwft_misaligned_exc_get();
> >  	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg locked value 0");
> 

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

Re: [kvm-unit-tests PATCH 08/10] riscv: sbi: Probe/skip SUSP

[Clément Léger]

On 21/02/2025 16:55, Andrew Jones wrote:
> Cleanly skip testing suspend when the SUSP extension isn't available.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  riscv/sbi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index a0a8331b04bd..e5f6e1c72ae6 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -1452,6 +1452,12 @@ static void check_susp(void)
>  
>  	report_prefix_push("susp");
>  
> +	if (!sbi_probe(SBI_EXT_SUSP)) {
> +		report_skip("SUSP extension not available");
> +		report_prefix_pop();
> +		return;
> +	}
> +
>  	timer_setup(susp_timer);
>  	local_irq_enable();
>  	timer_start(SBI_SUSP_TIMER_DURATION_US);
> @@ -1479,7 +1485,7 @@ static void check_susp(void)
>  			local_irq_enable();
>  
>  			if (!params.returns && ret.error == SBI_ERR_NOT_SUPPORTED) {
> -				report_skip("SUSP not supported?");
> +				report_fail("probing claims support, but it's not?");
>  				report_prefix_pop();
>  				goto out;
>  			} else if (!params.returns) {

Hi Andrew,

Looks good to me,

Reviewed-by: Clément Léger <cleger@rivosinc.com>

Thanks,

Clément

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

Re: [kvm-unit-tests PATCH 09/10] riscv: sbi: susp: Check upper bits of sleep_type are ignored

[Clément Léger]

On 21/02/2025 16:55, Andrew Jones wrote:
> When an SBI function parameter has a specific bit width which is
> less than that of a long, then the upper bits should be ignored
> by the SBI implementation. I.e. since sleep_type is a uint32_t,
> then on rv64 both 0 and 0x1_0000_0000 are valid and mean suspend
> to ram.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  riscv/sbi.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index e5f6e1c72ae6..1d9d1871a28b 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -67,7 +67,7 @@ static struct sbiret sbi_hart_suspend_raw(unsigned long suspend_type, unsigned l
>  	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
>  }
>  
> -static struct sbiret sbi_system_suspend(uint32_t sleep_type, unsigned long resume_addr, unsigned long opaque)
> +static struct sbiret sbi_system_suspend_raw(unsigned long sleep_type, unsigned long resume_addr, unsigned long opaque)
>  {
>  	return sbi_ecall(SBI_EXT_SUSP, 0, sleep_type, resume_addr, opaque, 0, 0, 0);
>  }
> @@ -1367,6 +1367,19 @@ static bool susp_type_prep(unsigned long ctx[], struct susp_params *params)
>  	return true;
>  }
>  
> +#if __riscv_xlen != 32
> +static bool susp_type_prep2(unsigned long ctx[], struct susp_params *params)
> +{
> +	bool r;
> +
> +	r = susp_basic_prep(ctx, params);
> +	assert(r);
> +	params->sleep_type = BIT(32);
> +
> +	return true;
> +}
> +#endif
> +
>  static bool susp_badaddr_prep(unsigned long ctx[], struct susp_params *params)
>  {
>  	phys_addr_t badaddr;
> @@ -1432,6 +1445,7 @@ static void check_susp(void)
>  #define SUSP_FIRST_TESTNUM 1
>  		SUSP_BASIC = SUSP_FIRST_TESTNUM,
>  		SUSP_TYPE,
> +		SUSP_TYPE2,
>  		SUSP_BAD_ADDR,
>  		SUSP_ONE_ONLINE,
>  		NR_SUSP_TESTS,
> @@ -1441,10 +1455,13 @@ static void check_susp(void)
>  		bool (*prep)(unsigned long ctx[], struct susp_params *params);
>  		void (*check)(unsigned long ctx[], struct susp_params *params);
>  	} susp_tests[] = {
> -		[SUSP_BASIC]		= { "basic",		susp_basic_prep,	susp_basic_check,	},
> -		[SUSP_TYPE]		= { "sleep_type",	susp_type_prep,					},
> -		[SUSP_BAD_ADDR]		= { "bad addr",		susp_badaddr_prep,				},
> -		[SUSP_ONE_ONLINE]	= { "one cpu online",	susp_one_prep,					},
> +		[SUSP_BASIC]		= { "basic",			susp_basic_prep,	susp_basic_check,	},
> +		[SUSP_TYPE]		= { "sleep_type",		susp_type_prep,					},
> +#if __riscv_xlen != 32
> +		[SUSP_TYPE2]		= { "sleep_type upper bits",	susp_type_prep2,	susp_basic_check	},
> +#endif
> +		[SUSP_BAD_ADDR]		= { "bad addr",			susp_badaddr_prep,				},
> +		[SUSP_ONE_ONLINE]	= { "one cpu online",		susp_one_prep,					},
>  	};
>  	struct susp_params params;
>  	struct sbiret ret;
> @@ -1466,6 +1483,9 @@ static void check_susp(void)
>  	report(ret.error == SBI_ERR_NOT_SUPPORTED, "funcid != 0 not supported");
>  
>  	for (i = SUSP_FIRST_TESTNUM; i < NR_SUSP_TESTS; i++) {
> +		if (!susp_tests[i].name)
> +			continue;
> +

Hi Andrew,

You could have as well put an #ifdef around SUSP_TYPE2 definition in the
enum rather than relying on name to be != NULL. But I'm ok either way ;)

Reviewed-by: Clément Léger <cleger@rivosinc.com>

Thanks,

Clément

>  		report_prefix_push(susp_tests[i].name);
>  
>  		ctx[SBI_SUSP_TESTNUM_IDX] = i;
> @@ -1480,7 +1500,7 @@ static void check_susp(void)
>  		}
>  
>  		if ((testnum = setjmp(sbi_susp_jmp)) == 0) {
> -			ret = sbi_system_suspend(params.sleep_type, params.resume_addr, params.opaque);
> +			ret = sbi_system_suspend_raw(params.sleep_type, params.resume_addr, params.opaque);
>  
>  			local_irq_enable();
>  


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

Re: [kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Andrew Jones]

On Tue, Feb 25, 2025 at 11:25:13AM +0100, Andrew Jones wrote:
> On Tue, Feb 25, 2025 at 11:14:13AM +0100, Clément Léger wrote:
> > 
> > 
> > On 21/02/2025 16:55, Andrew Jones wrote:
> > > Until we fix opensbi mark these known failures as kfails so we can
> > > pass CI.
> > > 
> > > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> > > ---
> > >  riscv/sbi-fwft.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> > > index b10c147f22dd..19340d6bb48c 100644
> > > --- a/riscv/sbi-fwft.c
> > > +++ b/riscv/sbi-fwft.c
> > > @@ -63,11 +63,17 @@ static void fwft_check_base(void)
> > >  		struct sbiret ret;
> > >  
> > >  		ret = fwft_get_raw(BIT(32));
> > > -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> > > +		if (ret.error == 0)
> > > +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> > > +		else
> > > +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> > >  				    "get feature with bit 32 set error");
> > 
> > 
> > Hey Andrew,
> > 
> > I'm actually a bit confused here. As raised by Anup, the spec states that:
> > 
> > "Every SBI function should prefer unsigned long as the data type. It
> > keeps the specification simple and easily adaptable for all RISC-V ISA
> > types. In case the data is defined as 32bit wide, higher privilege
> > software must ensure that it only uses 32 bit data."
> > 
> > Does this means 64 bits value are truncated to 32 bits ?
> 
> Ah, yes. I had noticed that required truncation while doing some KVM fixes
> and patch 9 of this series is even adding a test for it for SUSP. I just
> didn't look closely enough at why this FWFT test was failing. The test
> should get SBI_SUCCESS, as it does, but it should also check that
> feature = BIT(32) behaves the same as features = 1.

Eh, '...as feature = 0 (MISALIGNED_EXC_DELEG)'. For now, I think we should
just drop the test, though, because we need a feature that, when present,
always allows enable/disable in order to test for a change in behavior.
While we may be able to toggle 0/1 for MISALIGNED_EXC_DELEG, we can't
count on traps getting enabled. I'll add ADUE tests and do a truncated
feature ID test for it.

Thanks,
drew

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

Re: [kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Clément Léger]

On 25/02/2025 16:32, Andrew Jones wrote:
> On Tue, Feb 25, 2025 at 11:25:13AM +0100, Andrew Jones wrote:
>> On Tue, Feb 25, 2025 at 11:14:13AM +0100, Clément Léger wrote:
>>>
>>>
>>> On 21/02/2025 16:55, Andrew Jones wrote:
>>>> Until we fix opensbi mark these known failures as kfails so we can
>>>> pass CI.
>>>>
>>>> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
>>>> ---
>>>>  riscv/sbi-fwft.c | 15 ++++++++++++---
>>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
>>>> index b10c147f22dd..19340d6bb48c 100644
>>>> --- a/riscv/sbi-fwft.c
>>>> +++ b/riscv/sbi-fwft.c
>>>> @@ -63,11 +63,17 @@ static void fwft_check_base(void)
>>>>  		struct sbiret ret;
>>>>  
>>>>  		ret = fwft_get_raw(BIT(32));
>>>> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>>>> +		if (ret.error == 0)
>>>> +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
>>>> +		else
>>>> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>>>>  				    "get feature with bit 32 set error");
>>>
>>>
>>> Hey Andrew,
>>>
>>> I'm actually a bit confused here. As raised by Anup, the spec states that:
>>>
>>> "Every SBI function should prefer unsigned long as the data type. It
>>> keeps the specification simple and easily adaptable for all RISC-V ISA
>>> types. In case the data is defined as 32bit wide, higher privilege
>>> software must ensure that it only uses 32 bit data."
>>>
>>> Does this means 64 bits value are truncated to 32 bits ?
>>
>> Ah, yes. I had noticed that required truncation while doing some KVM fixes
>> and patch 9 of this series is even adding a test for it for SUSP. I just
>> didn't look closely enough at why this FWFT test was failing. The test
>> should get SBI_SUCCESS, as it does, but it should also check that
>> feature = BIT(32) behaves the same as features = 1.
> 
> Eh, '...as feature = 0 (MISALIGNED_EXC_DELEG)'. For now, I think we should
> just drop the test, though, because we need a feature that, when present,
> always allows enable/disable in order to test for a change in behavior.
> While we may be able to toggle 0/1 for MISALIGNED_EXC_DELEG, we can't
> count on traps getting enabled. I'll add ADUE tests and do a truncated
> feature ID test for it.

Ah yeah totally make sense ! I'll modify SSE test as well to test that
64 bits identifier are correctly truncated.

Thanks,

Clément

> 
> Thanks,
> drew


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

[kvm-unit-tests PATCH 11/10] riscv: sbi: Add fwft pte_hw_ad_updating test

[Andrew Jones]

Add SBI FWFT tests for the PTE_HW_AD_UPDATING feature.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/mmu.h |   3 +
 lib/riscv/mmu.c     |   7 ++
 riscv/sbi-fwft.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)

diff --git a/lib/riscv/asm/mmu.h b/lib/riscv/asm/mmu.h
index 28c332f11496..4cdc1f77d884 100644
--- a/lib/riscv/asm/mmu.h
+++ b/lib/riscv/asm/mmu.h
@@ -32,4 +32,7 @@ static inline void local_flush_tlb_page(unsigned long addr)
  */
 pte_t *get_pte(pgd_t *pgtable, uintptr_t vaddr);
 
+/* Update the pgprot to @prot for the pte of the corresponding @addr */
+void pte_update_prot(unsigned long addr, pteval_t prot);
+
 #endif /* _ASMRISCV_MMU_H_ */
diff --git a/lib/riscv/mmu.c b/lib/riscv/mmu.c
index 577c66aa77ba..4438327f6ffe 100644
--- a/lib/riscv/mmu.c
+++ b/lib/riscv/mmu.c
@@ -54,6 +54,13 @@ pte_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
 	return ptep;
 }
 
+void pte_update_prot(unsigned long addr, pteval_t prot)
+{
+	pte_t *ptep = get_pte(current_pgtable(), addr);
+	*ptep = __pte(pte_val(*ptep) | prot);
+	local_flush_tlb_page(addr);
+}
+
 static pteval_t *__install_page(pgd_t *pgtable, phys_addr_t paddr,
 				uintptr_t vaddr, pgprot_t prot, bool flush)
 {
diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index 0a70192b1da5..2b65285c82fc 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -5,9 +5,12 @@
  * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@rivosinc.com>
  */
 #include <libcflat.h>
+#include <alloc.h>
 #include <stdlib.h>
 
 #include <asm/csr.h>
+#include <asm/io.h>
+#include <asm/mmu.h>
 #include <asm/processor.h>
 #include <asm/ptrace.h>
 #include <asm/sbi.h>
@@ -160,6 +163,164 @@ static void fwft_check_misaligned_exc_deleg(void)
 	report_prefix_pop();
 }
 
+static bool adue_triggered_read, adue_triggered_write;
+
+static void adue_read_handler(struct pt_regs *regs)
+{
+	adue_triggered_read = true;
+	pte_update_prot(regs->badaddr, _PAGE_ACCESSED);
+}
+
+static void adue_write_handler(struct pt_regs *regs)
+{
+	adue_triggered_write = true;
+	pte_update_prot(regs->badaddr, _PAGE_ACCESSED | _PAGE_DIRTY);
+}
+
+static bool adue_check_pte(pteval_t pte, bool write)
+{
+	return (pte & (_PAGE_ACCESSED | _PAGE_DIRTY)) == (_PAGE_ACCESSED | (write ? _PAGE_DIRTY : 0));
+}
+
+static void __adue_check(bool hw_updating_enabled, bool write)
+{
+	unsigned long *ptr = malloc(sizeof(unsigned long));
+	pte_t *ptep = get_pte(current_pgtable(), (uintptr_t)ptr);
+	bool *triggered;
+	const char *op;
+
+	WRITE_ONCE(adue_triggered_read, false);
+	WRITE_ONCE(adue_triggered_write, false);
+
+	*ptep = __pte(pte_val(*ptep) & ~(_PAGE_ACCESSED | _PAGE_DIRTY));
+	local_flush_tlb_page((uintptr_t)ptr);
+
+	if (write) {
+		op = "write";
+		triggered = &adue_triggered_write;
+		writel(0xdeadbeef, ptr);
+	} else {
+		op = "read";
+		triggered = &adue_triggered_read;
+		readl(ptr);
+	}
+
+	ptep = get_pte(current_pgtable(), (uintptr_t)ptr);
+
+	report(hw_updating_enabled != *triggered &&
+	       adue_check_pte(pte_val(*ptep), write), "%s %s",
+	       hw_updating_enabled ? "hw updating enabled" : "hw updating disabled", op);
+
+	free(ptr);
+}
+
+static void adue_check(bool hw_updating_enabled)
+{
+	__adue_check(hw_updating_enabled, false);
+	__adue_check(hw_updating_enabled, true);
+}
+
+static void fwft_check_pte_ad_hw_updating(void)
+{
+	struct sbiret ret;
+	bool enabled;
+
+	report_prefix_push("pte_ad_hw_updating");
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
+		report_skip("not supported by platform");
+		return;
+	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
+		/* Not much we can do without a working get... */
+		return;
+	}
+
+	enabled = !!ret.value;
+	report(!enabled, "resets to 0");
+
+	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
+	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
+
+	adue_check(enabled);
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d", !enabled))
+		goto inval_tests;
+	else
+		enabled = !enabled;
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d", enabled);
+
+	adue_check(enabled);
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d again", !enabled))
+		goto inval_tests;
+	else
+		enabled = !enabled;
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d again", enabled);
+
+	adue_check(enabled);
+
+#if __riscv_xlen > 32
+	ret = fwft_set_raw(BIT(32) | SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with high feature bits set", !enabled))
+		goto inval_tests;
+	else
+		enabled = !enabled;
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with high feature bits set", enabled);
+
+	adue_check(enabled);
+#endif
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
+	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with lock", !enabled))
+		goto inval_tests;
+	else
+		enabled = !enabled;
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with lock", enabled);
+
+	adue_check(enabled);
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
+	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", !enabled);
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
+	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", !enabled);
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 0);
+	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", enabled);
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set without lock", enabled);
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
+	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
+
+	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
+	sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set with lock", enabled);
+
+inval_tests:
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, 2, 0);
+	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to 2");
+
+	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 2);
+	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to %d with flags=2", !enabled);
+
+	install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);
+	install_exception_handler(EXC_STORE_PAGE_FAULT, NULL);
+
+	report_prefix_pop();
+}
+
 void check_fwft(void)
 {
 	report_prefix_push("fwft");
@@ -174,6 +335,7 @@ void check_fwft(void)
 
 	fwft_check_base();
 	fwft_check_misaligned_exc_deleg();
+	fwft_check_pte_ad_hw_updating();
 
 	report_prefix_pop();
 }
-- 
2.48.1


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

Re: [kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Andrew Jones]

On Tue, Feb 25, 2025 at 04:48:42PM +0100, Clément Léger wrote:
> 
> 
> On 25/02/2025 16:32, Andrew Jones wrote:
> > On Tue, Feb 25, 2025 at 11:25:13AM +0100, Andrew Jones wrote:
> >> On Tue, Feb 25, 2025 at 11:14:13AM +0100, Clément Léger wrote:
> >>>
> >>>
> >>> On 21/02/2025 16:55, Andrew Jones wrote:
> >>>> Until we fix opensbi mark these known failures as kfails so we can
> >>>> pass CI.
> >>>>
> >>>> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> >>>> ---
> >>>>  riscv/sbi-fwft.c | 15 ++++++++++++---
> >>>>  1 file changed, 12 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> >>>> index b10c147f22dd..19340d6bb48c 100644
> >>>> --- a/riscv/sbi-fwft.c
> >>>> +++ b/riscv/sbi-fwft.c
> >>>> @@ -63,11 +63,17 @@ static void fwft_check_base(void)
> >>>>  		struct sbiret ret;
> >>>>  
> >>>>  		ret = fwft_get_raw(BIT(32));
> >>>> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >>>> +		if (ret.error == 0)
> >>>> +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> >>>> +		else
> >>>> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >>>>  				    "get feature with bit 32 set error");
> >>>
> >>>
> >>> Hey Andrew,
> >>>
> >>> I'm actually a bit confused here. As raised by Anup, the spec states that:
> >>>
> >>> "Every SBI function should prefer unsigned long as the data type. It
> >>> keeps the specification simple and easily adaptable for all RISC-V ISA
> >>> types. In case the data is defined as 32bit wide, higher privilege
> >>> software must ensure that it only uses 32 bit data."
> >>>
> >>> Does this means 64 bits value are truncated to 32 bits ?
> >>
> >> Ah, yes. I had noticed that required truncation while doing some KVM fixes
> >> and patch 9 of this series is even adding a test for it for SUSP. I just
> >> didn't look closely enough at why this FWFT test was failing. The test
> >> should get SBI_SUCCESS, as it does, but it should also check that
> >> feature = BIT(32) behaves the same as features = 1.
> > 
> > Eh, '...as feature = 0 (MISALIGNED_EXC_DELEG)'. For now, I think we should
> > just drop the test, though, because we need a feature that, when present,
> > always allows enable/disable in order to test for a change in behavior.
> > While we may be able to toggle 0/1 for MISALIGNED_EXC_DELEG, we can't
> > count on traps getting enabled. I'll add ADUE tests and do a truncated
> > feature ID test for it.
> 
> Ah yeah totally make sense ! I'll modify SSE test as well to test that
> 64 bits identifier are correctly truncated.
>

I sent a patch 11/10 to this series with the ADUE test including a
truncated feature id.

Thanks
drew

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

Re: [kvm-unit-tests PATCH 11/10] riscv: sbi: Add fwft pte_hw_ad_updating test

[Clément Léger]

On 26/02/2025 18:59, Andrew Jones wrote:
> Add SBI FWFT tests for the PTE_HW_AD_UPDATING feature.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/riscv/asm/mmu.h |   3 +
>  lib/riscv/mmu.c     |   7 ++
>  riscv/sbi-fwft.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+)
> 
> diff --git a/lib/riscv/asm/mmu.h b/lib/riscv/asm/mmu.h
> index 28c332f11496..4cdc1f77d884 100644
> --- a/lib/riscv/asm/mmu.h
> +++ b/lib/riscv/asm/mmu.h
> @@ -32,4 +32,7 @@ static inline void local_flush_tlb_page(unsigned long addr)
>   */
>  pte_t *get_pte(pgd_t *pgtable, uintptr_t vaddr);
>  
> +/* Update the pgprot to @prot for the pte of the corresponding @addr */
> +void pte_update_prot(unsigned long addr, pteval_t prot);
> +
>  #endif /* _ASMRISCV_MMU_H_ */
> diff --git a/lib/riscv/mmu.c b/lib/riscv/mmu.c
> index 577c66aa77ba..4438327f6ffe 100644
> --- a/lib/riscv/mmu.c
> +++ b/lib/riscv/mmu.c
> @@ -54,6 +54,13 @@ pte_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
>  	return ptep;
>  }
>  
> +void pte_update_prot(unsigned long addr, pteval_t prot)
> +{
> +	pte_t *ptep = get_pte(current_pgtable(), addr);
> +	*ptep = __pte(pte_val(*ptep) | prot);
> +	local_flush_tlb_page(addr);
> +}
> +
>  static pteval_t *__install_page(pgd_t *pgtable, phys_addr_t paddr,
>  				uintptr_t vaddr, pgprot_t prot, bool flush)
>  {
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index 0a70192b1da5..2b65285c82fc 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -5,9 +5,12 @@
>   * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@rivosinc.com>
>   */
>  #include <libcflat.h>
> +#include <alloc.h>
>  #include <stdlib.h>
>  
>  #include <asm/csr.h>
> +#include <asm/io.h>
> +#include <asm/mmu.h>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
>  #include <asm/sbi.h>
> @@ -160,6 +163,164 @@ static void fwft_check_misaligned_exc_deleg(void)
>  	report_prefix_pop();
>  }
>  
> +static bool adue_triggered_read, adue_triggered_write;
> +
> +static void adue_read_handler(struct pt_regs *regs)
> +{
> +	adue_triggered_read = true;
> +	pte_update_prot(regs->badaddr, _PAGE_ACCESSED);
> +}
> +
> +static void adue_write_handler(struct pt_regs *regs)
> +{
> +	adue_triggered_write = true;
> +	pte_update_prot(regs->badaddr, _PAGE_ACCESSED | _PAGE_DIRTY);
> +}
> +
> +static bool adue_check_pte(pteval_t pte, bool write)
> +{
> +	return (pte & (_PAGE_ACCESSED | _PAGE_DIRTY)) == (_PAGE_ACCESSED | (write ? _PAGE_DIRTY : 0));
> +}
> +
> +static void __adue_check(bool hw_updating_enabled, bool write)
> +{
> +	unsigned long *ptr = malloc(sizeof(unsigned long));
> +	pte_t *ptep = get_pte(current_pgtable(), (uintptr_t)ptr);
> +	bool *triggered;
> +	const char *op;
> +
> +	WRITE_ONCE(adue_triggered_read, false);
> +	WRITE_ONCE(adue_triggered_write, false);
> +
> +	*ptep = __pte(pte_val(*ptep) & ~(_PAGE_ACCESSED | _PAGE_DIRTY));
> +	local_flush_tlb_page((uintptr_t)ptr);
> +
> +	if (write) {
> +		op = "write";
> +		triggered = &adue_triggered_write;
> +		writel(0xdeadbeef, ptr);
> +	} else {
> +		op = "read";
> +		triggered = &adue_triggered_read;
> +		readl(ptr);
> +	}
> +
> +	ptep = get_pte(current_pgtable(), (uintptr_t)ptr);
> +
> +	report(hw_updating_enabled != *triggered &&
> +	       adue_check_pte(pte_val(*ptep), write), "%s %s",
> +	       hw_updating_enabled ? "hw updating enabled" : "hw updating disabled", op);

Hi Andrew,

 adue_check_pte(pte_val(*ptep), write), "hw updating %s %s",
 hw_updating_enabled ? "enabled" : "disabled", op);

> +
> +	free(ptr);
> +}
> +
> +static void adue_check(bool hw_updating_enabled)
> +{
> +	__adue_check(hw_updating_enabled, false);
> +	__adue_check(hw_updating_enabled, true);
> +}
> +
> +static void fwft_check_pte_ad_hw_updating(void)
> +{
> +	struct sbiret ret;
> +	bool enabled;
> +
> +	report_prefix_push("pte_ad_hw_updating");
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> +		report_skip("not supported by platform");
> +		return;
> +	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> +		/* Not much we can do without a working get... */
> +		return;
> +	}
> +
> +	enabled = !!ret.value;

Is there a reason to normalize value to a boolean ? The spec states that
the values for ADUE is either exactly 0 or 1. So i'd expect value to
contain 0 or 1 and thus no need to normalize it. That would even hide
some invalid returned value from the SBI.

> +	report(!enabled, "resets to 0");
> +
> +	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
> +	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
> +
> +	adue_check(enabled);
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d", !enabled))
> +		goto inval_tests;
> +	else
> +		enabled = !enabled;
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d", enabled);
> +
> +	adue_check(enabled);
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d again", !enabled))
> +		goto inval_tests;
> +	else
> +		enabled = !enabled;
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d again", enabled);
> +
> +	adue_check(enabled);

That seems like this whole block was copy/pasted, it might be factorized.

> +
> +#if __riscv_xlen > 32
> +	ret = fwft_set_raw(BIT(32) | SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with high feature bits set", !enabled))
> +		goto inval_tests;
> +	else
> +		enabled = !enabled;
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with high feature bits set", enabled);
> +
> +	adue_check(enabled);
> +#endif
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with lock", !enabled))
> +		goto inval_tests;
> +	else
> +		enabled = !enabled;
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with lock", enabled);
> +
> +	adue_check(enabled);
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", !enabled);
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
> +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", !enabled);
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 0);
> +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", enabled);
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set without lock", enabled);
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
> +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
> +
> +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set with lock", enabled);

Should this be factorized with the misaligned LOCKED testing as well ?

> +
> +inval_tests:
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, 2, 0);
> +	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to 2");
> +
> +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 2);
> +	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to %d with flags=2", !enabled);

Ditto.

Otherwise, looks good to me.

Thanks,

Clément

> +
> +	install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);
> +	install_exception_handler(EXC_STORE_PAGE_FAULT, NULL);
> +
> +	report_prefix_pop();
> +}
> +
>  void check_fwft(void)
>  {
>  	report_prefix_push("fwft");
> @@ -174,6 +335,7 @@ void check_fwft(void)
>  
>  	fwft_check_base();
>  	fwft_check_misaligned_exc_deleg();
> +	fwft_check_pte_ad_hw_updating();
>  
>  	report_prefix_pop();
>  }


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

Re: [kvm-unit-tests PATCH 11/10] riscv: sbi: Add fwft pte_hw_ad_updating test

[Andrew Jones]

On Thu, Feb 27, 2025 at 10:09:42AM +0100, Clément Léger wrote:
...
> > +	report(hw_updating_enabled != *triggered &&
> > +	       adue_check_pte(pte_val(*ptep), write), "%s %s",
> > +	       hw_updating_enabled ? "hw updating enabled" : "hw updating disabled", op);
> 
> Hi Andrew,
> 
>  adue_check_pte(pte_val(*ptep), write), "hw updating %s %s",
>  hw_updating_enabled ? "enabled" : "disabled", op);

Will do

> 
> > +
> > +	free(ptr);
> > +}
> > +
> > +static void adue_check(bool hw_updating_enabled)
> > +{
> > +	__adue_check(hw_updating_enabled, false);
> > +	__adue_check(hw_updating_enabled, true);
> > +}
> > +
> > +static void fwft_check_pte_ad_hw_updating(void)
> > +{
> > +	struct sbiret ret;
> > +	bool enabled;
> > +
> > +	report_prefix_push("pte_ad_hw_updating");
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> > +		report_skip("not supported by platform");
> > +		return;
> > +	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> > +		/* Not much we can do without a working get... */
> > +		return;
> > +	}
> > +
> > +	enabled = !!ret.value;
> 
> Is there a reason to normalize value to a boolean ? The spec states that
> the values for ADUE is either exactly 0 or 1. So i'd expect value to
> contain 0 or 1 and thus no need to normalize it. That would even hide
> some invalid returned value from the SBI.

Good thought. We should test the exact 0/1.

> 
> > +	report(!enabled, "resets to 0");
> > +
> > +	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
> > +	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
> > +
> > +	adue_check(enabled);
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> > +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d", !enabled))
> > +		goto inval_tests;
> > +	else
> > +		enabled = !enabled;
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d", enabled);
> > +
> > +	adue_check(enabled);
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> > +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d again", !enabled))
> > +		goto inval_tests;
> > +	else
> > +		enabled = !enabled;
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d again", enabled);
> > +
> > +	adue_check(enabled);
> 
> That seems like this whole block was copy/pasted, it might be factorized.
> 
> > +
> > +#if __riscv_xlen > 32
> > +	ret = fwft_set_raw(BIT(32) | SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> > +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with high feature bits set", !enabled))
> > +		goto inval_tests;
> > +	else
> > +		enabled = !enabled;
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with high feature bits set", enabled);
> > +
> > +	adue_check(enabled);
> > +#endif
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
> > +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with lock", !enabled))
> > +		goto inval_tests;
> > +	else
> > +		enabled = !enabled;
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with lock", enabled);
> > +
> > +	adue_check(enabled);
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> > +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", !enabled);
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
> > +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", !enabled);
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 0);
> > +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", enabled);
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set without lock", enabled);
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
> > +	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
> > +
> > +	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > +	sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set with lock", enabled);
> 
> Should this be factorized with the misaligned LOCKED testing as well ?
> 
> > +
> > +inval_tests:
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, 2, 0);
> > +	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to 2");
> > +
> > +	ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 2);
> > +	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to %d with flags=2", !enabled);
> 
> Ditto.
> 
> Otherwise, looks good to me.

Thanks for the review. I'll refactor as suggested. I've also got a couple
other changes to make, such as marking the reset value test as kfail,
since opensbi currently enables svadu when it's present.

I'll send a v2 of this series with patch 1/10 dropped and this patch
added after fixing it up.

Thanks,
drew

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