[kvm-unit-tests PATCH 0/3] riscv: sbi: Ensure we can pass with any opensbi

[kvm-unit-tests PATCH 0/3] riscv: sbi: Ensure we can pass with any opensbi

[Andrew Jones]

At some point CI will update the version of QEMU it uses. If the version
selected includes an opensbi that doesn't have all the fixes for current
tests, then CI will start failing for riscv. Guard against that by using
kfail for known opensbi failures. We only kfail for opensbi and for its
versions less than 1.7, though, as we expect everything to be fixed then.

Andrew Jones (3):
  lib/riscv: Also provide sbiret impl functions
  riscv: sbi: Add kfail versions of sbiret_report functions
  riscv: sbi: Use kfail for known opensbi failures

 lib/riscv/asm/sbi.h |  6 ++++--
 lib/riscv/sbi.c     | 18 ++++++++++++++----
 riscv/sbi-fwft.c    | 20 +++++++++++++-------
 riscv/sbi-sse.c     |  4 ++--
 riscv/sbi-tests.h   | 20 +++++++++++++++-----
 riscv/sbi.c         |  6 ++++--
 6 files changed, 52 insertions(+), 22 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 1/3] lib/riscv: Also provide sbiret impl functions

[Andrew Jones]

We almost always return sbiret from sbi wrapper functions so
do that for sbi_get_imp_version() and sbi_get_imp_id(), but
asserting no error and returning the value is also useful,
so continue to provide those functions too, just with a slightly
different name.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/sbi.h |  6 ++++--
 lib/riscv/sbi.c     | 18 ++++++++++++++----
 riscv/sbi-sse.c     |  4 ++--
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index edaee462c3fa..a5738a5ce209 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -260,9 +260,11 @@ struct sbiret sbi_send_ipi_cpumask(const cpumask_t *mask);
 struct sbiret sbi_send_ipi_broadcast(void);
 struct sbiret sbi_set_timer(unsigned long stime_value);
 struct sbiret sbi_get_spec_version(void);
-unsigned long sbi_get_imp_version(void);
-unsigned long sbi_get_imp_id(void);
+struct sbiret sbi_get_imp_version(void);
+struct sbiret sbi_get_imp_id(void);
 long sbi_probe(int ext);
+unsigned long __sbi_get_imp_version(void);
+unsigned long __sbi_get_imp_id(void);
 
 typedef void (*sbi_sse_handler_fn)(void *data, struct pt_regs *regs, unsigned int hartid);
 
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 53d25489f905..2959378f64bb 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -183,21 +183,31 @@ struct sbiret sbi_set_timer(unsigned long stime_value)
 	return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0);
 }
 
-unsigned long sbi_get_imp_version(void)
+struct sbiret sbi_get_imp_version(void)
+{
+	return sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_VERSION, 0, 0, 0, 0, 0, 0);
+}
+
+struct sbiret sbi_get_imp_id(void)
+{
+	return sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID, 0, 0, 0, 0, 0, 0);
+}
+
+unsigned long __sbi_get_imp_version(void)
 {
 	struct sbiret ret;
 
-	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_VERSION, 0, 0, 0, 0, 0, 0);
+	ret = sbi_get_imp_version();
 	assert(!ret.error);
 
 	return ret.value;
 }
 
-unsigned long sbi_get_imp_id(void)
+unsigned long __sbi_get_imp_id(void)
 {
 	struct sbiret ret;
 
-	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID, 0, 0, 0, 0, 0, 0);
+	ret = sbi_get_imp_id();
 	assert(!ret.error);
 
 	return ret.value;
diff --git a/riscv/sbi-sse.c b/riscv/sbi-sse.c
index 97e07725c359..bc6afaf5481e 100644
--- a/riscv/sbi-sse.c
+++ b/riscv/sbi-sse.c
@@ -1232,8 +1232,8 @@ void check_sse(void)
 		return;
 	}
 
-	if (sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
-	    sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7)) {
+	if (__sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
+	    __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7)) {
 		report_skip("OpenSBI < v1.7 detected, skipping tests");
 		report_prefix_pop();
 		return;
-- 
2.48.1


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

[kvm-unit-tests PATCH 2/3] riscv: sbi: Add kfail versions of sbiret_report functions

[Andrew Jones]

report_kfail is useful for SBI testing to allowing CI to PASS even
when SBI implementations have known failures. Since sbiret_report
functions are frequently used by SBI tests, make kfail versions of
them too.

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

diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
index ddfad7fef293..d5c4ae709632 100644
--- a/riscv/sbi-tests.h
+++ b/riscv/sbi-tests.h
@@ -39,7 +39,8 @@
 #include <libcflat.h>
 #include <asm/sbi.h>
 
-#define __sbiret_report(ret, expected_error, expected_value, has_value, expected_error_name, fmt, ...) ({	\
+#define __sbiret_report(kfail, ret, expected_error, expected_value,						\
+			has_value, expected_error_name, fmt, ...) ({						\
 	long ex_err = expected_error;										\
 	long ex_val = expected_value;										\
 	bool has_val = !!(has_value);										\
@@ -48,9 +49,9 @@
 	bool pass;												\
 														\
 	if (has_val)												\
-		pass = report(ch_err && ch_val, fmt, ##__VA_ARGS__);						\
+		pass = report_kfail(kfail, ch_err && ch_val, fmt, ##__VA_ARGS__);				\
 	else													\
-		pass = report(ch_err, fmt ": %s", ##__VA_ARGS__, expected_error_name);				\
+		pass = report_kfail(kfail, ch_err, fmt ": %s", ##__VA_ARGS__, expected_error_name);		\
 														\
 	if (!pass && has_val)											\
 		report_info(fmt ": expected (error: %ld, value: %ld), received: (error: %ld, value %ld)",	\
@@ -63,14 +64,23 @@
 })
 
 #define sbiret_report(ret, expected_error, expected_value, ...) \
-	__sbiret_report(ret, expected_error, expected_value, true, #expected_error, __VA_ARGS__)
+	__sbiret_report(false, ret, expected_error, expected_value, true, #expected_error, __VA_ARGS__)
 
 #define sbiret_report_error(ret, expected_error, ...) \
-	__sbiret_report(ret, expected_error, 0, false, #expected_error, __VA_ARGS__)
+	__sbiret_report(false, ret, expected_error, 0, false, #expected_error, __VA_ARGS__)
 
 #define sbiret_check(ret, expected_error, expected_value) \
 	sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
 
+#define sbiret_kfail(kfail, ret, expected_error, expected_value, ...) \
+	__sbiret_report(kfail, ret, expected_error, expected_value, true, #expected_error, __VA_ARGS__)
+
+#define sbiret_kfail_error(kfail, ret, expected_error, ...) \
+	__sbiret_report(kfail, ret, expected_error, 0, false, #expected_error, __VA_ARGS__)
+
+#define sbiret_check_kfail(kfail, ret, expected_error, expected_value) \
+	sbiret_kfail(kfail, ret, expected_error, expected_value, "check sbi.error and sbi.value")
+
 static inline bool env_or_skip(const char *env)
 {
 	if (!getenv(env)) {
-- 
2.48.1


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

[kvm-unit-tests PATCH 3/3] riscv: sbi: Use kfail for known opensbi failures

[Andrew Jones]

Use kfail for the opensbi s/SBI_ERR_DENIED/SBI_ERR_DENIED_LOCKED/
change. We expect it to be fixed in 1.7, so only kfail for opensbi
which has a version less than that. Also change the other uses of
kfail to only kfail for opensbi versions less than 1.7.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/sbi-fwft.c | 20 +++++++++++++-------
 riscv/sbi.c      |  6 ++++--
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index 3d225997c0ec..c52fbd6e77a6 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -83,19 +83,21 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
 
 	report_prefix_push("locked");
 
+	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
+		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
+
 	for (int i = 0; i < nr_values; ++i) {
 		ret = fwft_set(feature, test_values[i], 0);
-		sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
-			"Set to %lu without lock flag", test_values[i]);
+		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
+				   "Set to %lu without lock flag", test_values[i]);
 
 		ret = fwft_set(feature, test_values[i], SBI_FWFT_SET_FLAG_LOCK);
-		sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
-			"Set to %lu with lock flag", test_values[i]);
+		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
+				   "Set to %lu with lock flag", test_values[i]);
 	}
 
 	ret = fwft_get(feature);
-	sbiret_report(&ret, SBI_SUCCESS, locked_value,
-		"Get value %lu", locked_value);
+	sbiret_report(&ret, SBI_SUCCESS, locked_value, "Get value %lu", locked_value);
 
 	report_prefix_pop();
 }
@@ -103,6 +105,7 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
 static void fwft_feature_lock_test(uint32_t feature, unsigned long locked_value)
 {
 	unsigned long values[] = {0, 1};
+
 	fwft_feature_lock_test_values(feature, 2, values, locked_value);
 }
 
@@ -317,7 +320,10 @@ static void fwft_check_pte_ad_hw_updating(void)
 	report(ret.value == 0 || ret.value == 1, "first get value is 0/1");
 
 	enabled = ret.value;
-	report_kfail(true, !enabled, "resets to 0");
+
+	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
+		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
+	report_kfail(kfail, !enabled, "resets to 0");
 
 	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
 	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 83bc55125d46..edb1a6bef1ac 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -515,10 +515,12 @@ end_two:
 	sbiret_report_error(&ret, SBI_SUCCESS, "no targets, hart_mask_base is 1");
 
 	/* Try the next higher hartid than the max */
+	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
+		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
 	ret = sbi_send_ipi(2, max_hartid);
-	report_kfail(true, ret.error == SBI_ERR_INVALID_PARAM, "hart_mask got expected error (%ld)", ret.error);
+	sbiret_kfail_error(kfail, &ret, SBI_ERR_INVALID_PARAM, "hart_mask");
 	ret = sbi_send_ipi(1, max_hartid + 1);
-	report_kfail(true, ret.error == SBI_ERR_INVALID_PARAM, "hart_mask_base got expected error (%ld)", ret.error);
+	sbiret_kfail_error(kfail, &ret, SBI_ERR_INVALID_PARAM, "hart_mask_base");
 
 	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 1/3] lib/riscv: Also provide sbiret impl functions

[Clément Léger]

On 21/03/2025 17:54, Andrew Jones wrote:
> We almost always return sbiret from sbi wrapper functions so
> do that for sbi_get_imp_version() and sbi_get_imp_id(), but
> asserting no error and returning the value is also useful,
> so continue to provide those functions too, just with a slightly
> different name.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/riscv/asm/sbi.h |  6 ++++--
>  lib/riscv/sbi.c     | 18 ++++++++++++++----
>  riscv/sbi-sse.c     |  4 ++--
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index edaee462c3fa..a5738a5ce209 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -260,9 +260,11 @@ struct sbiret sbi_send_ipi_cpumask(const cpumask_t *mask);
>  struct sbiret sbi_send_ipi_broadcast(void);
>  struct sbiret sbi_set_timer(unsigned long stime_value);
>  struct sbiret sbi_get_spec_version(void);
> -unsigned long sbi_get_imp_version(void);
> -unsigned long sbi_get_imp_id(void);
> +struct sbiret sbi_get_imp_version(void);
> +struct sbiret sbi_get_imp_id(void);
>  long sbi_probe(int ext);
> +unsigned long __sbi_get_imp_version(void);
> +unsigned long __sbi_get_imp_id(void);
>  
>  typedef void (*sbi_sse_handler_fn)(void *data, struct pt_regs *regs, unsigned int hartid);
>  
> diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
> index 53d25489f905..2959378f64bb 100644
> --- a/lib/riscv/sbi.c
> +++ b/lib/riscv/sbi.c
> @@ -183,21 +183,31 @@ struct sbiret sbi_set_timer(unsigned long stime_value)
>  	return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0);
>  }
>  
> -unsigned long sbi_get_imp_version(void)
> +struct sbiret sbi_get_imp_version(void)
> +{
> +	return sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_VERSION, 0, 0, 0, 0, 0, 0);
> +}
> +
> +struct sbiret sbi_get_imp_id(void)
> +{
> +	return sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID, 0, 0, 0, 0, 0, 0);
> +}
> +
> +unsigned long __sbi_get_imp_version(void)
>  {
>  	struct sbiret ret;
>  
> -	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_VERSION, 0, 0, 0, 0, 0, 0);
> +	ret = sbi_get_imp_version();
>  	assert(!ret.error);
>  
>  	return ret.value;
>  }
>  
> -unsigned long sbi_get_imp_id(void)
> +unsigned long __sbi_get_imp_id(void)
>  {
>  	struct sbiret ret;
>  
> -	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID, 0, 0, 0, 0, 0, 0);
> +	ret = sbi_get_imp_id();
>  	assert(!ret.error);
>  
>  	return ret.value;
> diff --git a/riscv/sbi-sse.c b/riscv/sbi-sse.c
> index 97e07725c359..bc6afaf5481e 100644
> --- a/riscv/sbi-sse.c
> +++ b/riscv/sbi-sse.c
> @@ -1232,8 +1232,8 @@ void check_sse(void)
>  		return;
>  	}
>  
> -	if (sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> -	    sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7)) {
> +	if (__sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> +	    __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7)) {
>  		report_skip("OpenSBI < v1.7 detected, skipping tests");
>  		report_prefix_pop();
>  		return;

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 2/3] riscv: sbi: Add kfail versions of sbiret_report functions

[Clément Léger]

On 21/03/2025 17:54, Andrew Jones wrote:
> report_kfail is useful for SBI testing to allowing CI to PASS even
> when SBI implementations have known failures. Since sbiret_report
> functions are frequently used by SBI tests, make kfail versions of
> them too.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  riscv/sbi-tests.h | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
> index ddfad7fef293..d5c4ae709632 100644
> --- a/riscv/sbi-tests.h
> +++ b/riscv/sbi-tests.h
> @@ -39,7 +39,8 @@
>  #include <libcflat.h>
>  #include <asm/sbi.h>
>  
> -#define __sbiret_report(ret, expected_error, expected_value, has_value, expected_error_name, fmt, ...) ({	\
> +#define __sbiret_report(kfail, ret, expected_error, expected_value,						\
> +			has_value, expected_error_name, fmt, ...) ({						\
>  	long ex_err = expected_error;										\
>  	long ex_val = expected_value;										\
>  	bool has_val = !!(has_value);										\
> @@ -48,9 +49,9 @@
>  	bool pass;												\
>  														\
>  	if (has_val)												\
> -		pass = report(ch_err && ch_val, fmt, ##__VA_ARGS__);						\
> +		pass = report_kfail(kfail, ch_err && ch_val, fmt, ##__VA_ARGS__);				\
>  	else													\
> -		pass = report(ch_err, fmt ": %s", ##__VA_ARGS__, expected_error_name);				\
> +		pass = report_kfail(kfail, ch_err, fmt ": %s", ##__VA_ARGS__, expected_error_name);		\
>  														\
>  	if (!pass && has_val)											\
>  		report_info(fmt ": expected (error: %ld, value: %ld), received: (error: %ld, value %ld)",	\
> @@ -63,14 +64,23 @@
>  })
>  
>  #define sbiret_report(ret, expected_error, expected_value, ...) \
> -	__sbiret_report(ret, expected_error, expected_value, true, #expected_error, __VA_ARGS__)
> +	__sbiret_report(false, ret, expected_error, expected_value, true, #expected_error, __VA_ARGS__)
>  
>  #define sbiret_report_error(ret, expected_error, ...) \
> -	__sbiret_report(ret, expected_error, 0, false, #expected_error, __VA_ARGS__)
> +	__sbiret_report(false, ret, expected_error, 0, false, #expected_error, __VA_ARGS__)
>  
>  #define sbiret_check(ret, expected_error, expected_value) \
>  	sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
>  
> +#define sbiret_kfail(kfail, ret, expected_error, expected_value, ...) \
> +	__sbiret_report(kfail, ret, expected_error, expected_value, true, #expected_error, __VA_ARGS__)
> +
> +#define sbiret_kfail_error(kfail, ret, expected_error, ...) \
> +	__sbiret_report(kfail, ret, expected_error, 0, false, #expected_error, __VA_ARGS__)
> +
> +#define sbiret_check_kfail(kfail, ret, expected_error, expected_value) \
> +	sbiret_kfail(kfail, ret, expected_error, expected_value, "check sbi.error and sbi.value")
> +
>  static inline bool env_or_skip(const char *env)
>  {
>  	if (!getenv(env)) {

Hi Andrew,

I needed that as well in another test so:

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 3/3] riscv: sbi: Use kfail for known opensbi failures

[Clément Léger]

On 21/03/2025 17:54, Andrew Jones wrote:
> Use kfail for the opensbi s/SBI_ERR_DENIED/SBI_ERR_DENIED_LOCKED/
> change. We expect it to be fixed in 1.7, so only kfail for opensbi
> which has a version less than that. Also change the other uses of
> kfail to only kfail for opensbi versions less than 1.7.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  riscv/sbi-fwft.c | 20 +++++++++++++-------
>  riscv/sbi.c      |  6 ++++--
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index 3d225997c0ec..c52fbd6e77a6 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -83,19 +83,21 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
>  
>  	report_prefix_push("locked");
>  
> +	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> +		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> +
>  	for (int i = 0; i < nr_values; ++i) {
>  		ret = fwft_set(feature, test_values[i], 0);
> -		sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> -			"Set to %lu without lock flag", test_values[i]);
> +		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
> +				   "Set to %lu without lock flag", test_values[i]);
>  
>  		ret = fwft_set(feature, test_values[i], SBI_FWFT_SET_FLAG_LOCK);
> -		sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> -			"Set to %lu with lock flag", test_values[i]);
> +		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
> +				   "Set to %lu with lock flag", test_values[i]);
>  	}
>  
>  	ret = fwft_get(feature);
> -	sbiret_report(&ret, SBI_SUCCESS, locked_value,
> -		"Get value %lu", locked_value);
> +	sbiret_report(&ret, SBI_SUCCESS, locked_value, "Get value %lu", locked_value);

Reformatting ?

>  
>  	report_prefix_pop();
>  }
> @@ -103,6 +105,7 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
>  static void fwft_feature_lock_test(uint32_t feature, unsigned long locked_value)
>  {
>  	unsigned long values[] = {0, 1};
> +

That's some spurious newline here.


>  	fwft_feature_lock_test_values(feature, 2, values, locked_value);
>  }
>  
> @@ -317,7 +320,10 @@ static void fwft_check_pte_ad_hw_updating(void)
>  	report(ret.value == 0 || ret.value == 1, "first get value is 0/1");
>  
>  	enabled = ret.value;
> -	report_kfail(true, !enabled, "resets to 0");
> +
> +	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> +		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> +	report_kfail(kfail, !enabled, "resets to 0");
>  
>  	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
>  	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 83bc55125d46..edb1a6bef1ac 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -515,10 +515,12 @@ end_two:
>  	sbiret_report_error(&ret, SBI_SUCCESS, "no targets, hart_mask_base is 1");
>  
>  	/* Try the next higher hartid than the max */
> +	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> +		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
>  	ret = sbi_send_ipi(2, max_hartid);> -	report_kfail(true, ret.error
== SBI_ERR_INVALID_PARAM, "hart_mask got expected error (%ld)", ret.error);
> +	sbiret_kfail_error(kfail, &ret, SBI_ERR_INVALID_PARAM, "hart_mask");
>  	ret = sbi_send_ipi(1, max_hartid + 1);
> -	report_kfail(true, ret.error == SBI_ERR_INVALID_PARAM, "hart_mask_base got expected error (%ld)", ret.error);
> +	sbiret_kfail_error(kfail, &ret, SBI_ERR_INVALID_PARAM, "hart_mask_base");
>  
>  	report_prefix_pop();
>  

Hi Andrew,

I tried thinking of some way to factorize the version check but can't
really find something elegant. Without the spurious newline:

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 3/3] riscv: sbi: Use kfail for known opensbi failures

[Andrew Jones]

On Fri, Mar 21, 2025 at 09:22:19PM +0100, Clément Léger wrote:
> 
> 
> On 21/03/2025 17:54, Andrew Jones wrote:
> > Use kfail for the opensbi s/SBI_ERR_DENIED/SBI_ERR_DENIED_LOCKED/
> > change. We expect it to be fixed in 1.7, so only kfail for opensbi
> > which has a version less than that. Also change the other uses of
> > kfail to only kfail for opensbi versions less than 1.7.
> > 
> > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> > ---
> >  riscv/sbi-fwft.c | 20 +++++++++++++-------
> >  riscv/sbi.c      |  6 ++++--
> >  2 files changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> > index 3d225997c0ec..c52fbd6e77a6 100644
> > --- a/riscv/sbi-fwft.c
> > +++ b/riscv/sbi-fwft.c
> > @@ -83,19 +83,21 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
> >  
> >  	report_prefix_push("locked");
> >  
> > +	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> > +		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> > +
> >  	for (int i = 0; i < nr_values; ++i) {
> >  		ret = fwft_set(feature, test_values[i], 0);
> > -		sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> > -			"Set to %lu without lock flag", test_values[i]);
> > +		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
> > +				   "Set to %lu without lock flag", test_values[i]);
> >  
> >  		ret = fwft_set(feature, test_values[i], SBI_FWFT_SET_FLAG_LOCK);
> > -		sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> > -			"Set to %lu with lock flag", test_values[i]);
> > +		sbiret_kfail_error(kfail, &ret, SBI_ERR_DENIED_LOCKED,
> > +				   "Set to %lu with lock flag", test_values[i]);
> >  	}
> >  
> >  	ret = fwft_get(feature);
> > -	sbiret_report(&ret, SBI_SUCCESS, locked_value,
> > -		"Get value %lu", locked_value);
> > +	sbiret_report(&ret, SBI_SUCCESS, locked_value, "Get value %lu", locked_value);
> 
> Reformatting ?

Yup, and the "Set..." strings above. I missed that the format was wrong
when I applied the fwft_feature_lock_test_values patch and just lazily
fixed it up with this patch. I still haven't merged to the master
branch yet, so I can still squash a formatting fix into the
fwft_feature_lock_test_values patch in order to make this patch cleaner.

> 
> >  
> >  	report_prefix_pop();
> >  }
> > @@ -103,6 +105,7 @@ static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
> >  static void fwft_feature_lock_test(uint32_t feature, unsigned long locked_value)
> >  {
> >  	unsigned long values[] = {0, 1};
> > +
> 
> That's some spurious newline here.

It's also reformatting.

> 
> 
> >  	fwft_feature_lock_test_values(feature, 2, values, locked_value);
> >  }
> >  
> > @@ -317,7 +320,10 @@ static void fwft_check_pte_ad_hw_updating(void)
> >  	report(ret.value == 0 || ret.value == 1, "first get value is 0/1");
> >  
> >  	enabled = ret.value;
> > -	report_kfail(true, !enabled, "resets to 0");
> > +
> > +	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> > +		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> > +	report_kfail(kfail, !enabled, "resets to 0");
> >  
> >  	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
> >  	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index 83bc55125d46..edb1a6bef1ac 100644
> > --- a/riscv/sbi.c
> > +++ b/riscv/sbi.c
> > @@ -515,10 +515,12 @@ end_two:
> >  	sbiret_report_error(&ret, SBI_SUCCESS, "no targets, hart_mask_base is 1");
> >  
> >  	/* Try the next higher hartid than the max */
> > +	bool kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> > +		     __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> >  	ret = sbi_send_ipi(2, max_hartid);> -	report_kfail(true, ret.error
> == SBI_ERR_INVALID_PARAM, "hart_mask got expected error (%ld)", ret.error);
> > +	sbiret_kfail_error(kfail, &ret, SBI_ERR_INVALID_PARAM, "hart_mask");
> >  	ret = sbi_send_ipi(1, max_hartid + 1);
> > -	report_kfail(true, ret.error == SBI_ERR_INVALID_PARAM, "hart_mask_base got expected error (%ld)", ret.error);
> > +	sbiret_kfail_error(kfail, &ret, SBI_ERR_INVALID_PARAM, "hart_mask_base");
> >  
> >  	report_prefix_pop();
> >  
> 
> Hi Andrew,
> 
> I tried thinking of some way to factorize the version check but can't
> really find something elegant. Without the spurious newline:

I'll move the reformatting to the fwft_feature_lock_test_values patch,
but I'm generally not overly opposed to sneaking a couple reformatting
fixes into patches when the reformatting is obvious enough.

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

Thanks,
drew

> 
> Thanks,
> 
> Clément
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

Re: [kvm-unit-tests PATCH 0/3] riscv: sbi: Ensure we can pass with any opensbi

[Andrew Jones]

On Fri, Mar 21, 2025 at 05:54:04PM +0100, Andrew Jones wrote:
> At some point CI will update the version of QEMU it uses. If the version
> selected includes an opensbi that doesn't have all the fixes for current
> tests, then CI will start failing for riscv. Guard against that by using
> kfail for known opensbi failures. We only kfail for opensbi and for its
> versions less than 1.7, though, as we expect everything to be fixed then.
> 
> Andrew Jones (3):
>   lib/riscv: Also provide sbiret impl functions
>   riscv: sbi: Add kfail versions of sbiret_report functions
>   riscv: sbi: Use kfail for known opensbi failures
> 
>  lib/riscv/asm/sbi.h |  6 ++++--
>  lib/riscv/sbi.c     | 18 ++++++++++++++----
>  riscv/sbi-fwft.c    | 20 +++++++++++++-------
>  riscv/sbi-sse.c     |  4 ++--
>  riscv/sbi-tests.h   | 20 +++++++++++++++-----
>  riscv/sbi.c         |  6 ++++--
>  6 files changed, 52 insertions(+), 22 deletions(-)
> 
> -- 
> 2.48.1
>

Merged.

Thanks,
drew

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