[RFC kvm-unit-tests PATCH] riscv: Refactoring sbi fwft tests

[RFC kvm-unit-tests PATCH] riscv: Refactoring sbi fwft tests

[Akshay Behl]

This patch refactors the current sbi fwft tests
(pte_ad_hw_updating, misaligned_exc_deleg)

Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
---
 riscv/sbi-fwft.c | 58 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index ac2e3486..bf735f62 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -19,6 +19,15 @@
 
 void check_fwft(void);
 
+static bool env_or_skip(const char *env)
+{
+	if (!getenv(env)) {
+		report_skip("missing %s environment variable", env);
+		return false;
+	}
+
+	return true;
+}
 
 static struct sbiret fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags)
 {
@@ -66,6 +75,13 @@ static void fwft_check_reserved(unsigned long id)
 	sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
 }
 
+static void fwft_check_reset(uint32_t feature, unsigned long reset)
+{
+	struct sbiret ret = fwft_get(feature);
+
+	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
+}
+
 static void fwft_check_base(void)
 {
 	report_prefix_push("base");
@@ -99,18 +115,32 @@ static struct sbiret fwft_misaligned_exc_get(void)
 static void fwft_check_misaligned_exc_deleg(void)
 {
 	struct sbiret ret;
+	unsigned long expected;
 
 	report_prefix_push("misaligned_exc_deleg");
 
 	ret = fwft_misaligned_exc_get();
-	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
-		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
+	if (ret.error != SBI_SUCCESS) {
+		if (env_or_skip("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) {
+			expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG"), NULL, 0);
+			if (expected == 1)
+			{
+				report_fail("not supported by platform");
+				return;
+			}
+		}
+		report_skip("not supported by platform");
 		return;
 	}
 
 	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Get misaligned deleg feature"))
 		return;
 
+	if (env_or_skip("MISALIGNED_EXC_DELEG_RESET")) {
+		expected = (unsigned long)strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0);
+		fwft_check_reset(SBI_FWFT_MISALIGNED_EXC_DELEG, expected);
+	}
+
 	ret = fwft_misaligned_exc_set(2, 0);
 	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
 			    "Set misaligned deleg feature invalid value 2");
@@ -129,16 +159,10 @@ static void fwft_check_misaligned_exc_deleg(void)
 #endif
 
 	/* Set to 0 and check after with get */
-	ret = fwft_misaligned_exc_set(0, 0);
-	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0");
-	ret = fwft_misaligned_exc_get();
-	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg feature expected value 0");
+	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
 
 	/* Set to 1 and check after with get */
-	ret = fwft_misaligned_exc_set(1, 0);
-	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 1");
-	ret = fwft_misaligned_exc_get();
-	sbiret_report(&ret, SBI_SUCCESS, 1, "Get misaligned deleg feature expected value 1");
+	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
 
 	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
 
@@ -257,11 +281,20 @@ static void fwft_check_pte_ad_hw_updating(void)
 {
 	struct sbiret ret;
 	bool enabled;
+	unsigned long expected;
 
 	report_prefix_push("pte_ad_hw_updating");
 
 	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
-	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
+	if (ret.error != SBI_SUCCESS) {
+		if (env_or_skip("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
+			expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING"), NULL, 0);
+			if (expected == 1)
+			{
+				report_fail("not supported by platform");
+				return;
+			}
+		}
 		report_skip("not supported by platform");
 		return;
 	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
@@ -269,10 +302,9 @@ static void fwft_check_pte_ad_hw_updating(void)
 		return;
 	}
 
-	report(ret.value == 0 || ret.value == 1, "first get value is 0/1");
+	fwft_check_reset(SBI_FWFT_PTE_AD_HW_UPDATING, 1);
 
 	enabled = ret.value;
-	report_kfail(true, !enabled, "resets to 0");
 
 	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
 	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
-- 
2.34.1

Re: [RFC kvm-unit-tests PATCH] riscv: Refactoring sbi fwft tests

[Andrew Jones]

On Thu, Mar 13, 2025 at 01:28:45PM +0530, Akshay Behl wrote:
> This patch refactors the current sbi fwft tests
> (pte_ad_hw_updating, misaligned_exc_deleg)
> 
> Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
> ---
>  riscv/sbi-fwft.c | 58 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index ac2e3486..bf735f62 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -19,6 +19,15 @@
>  
>  void check_fwft(void);
>  
> +static bool env_or_skip(const char *env)
> +{
> +	if (!getenv(env)) {
> +		report_skip("missing %s environment variable", env);
> +		return false;
> +	}
> +
> +	return true;
> +}

We already have this in riscv/sbi.c so we should share it. We can just
make it a static inline in sbi-tests.h

>  
>  static struct sbiret fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags)
>  {
> @@ -66,6 +75,13 @@ static void fwft_check_reserved(unsigned long id)
>  	sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
>  }
>  
> +static void fwft_check_reset(uint32_t feature, unsigned long reset)
> +{
> +	struct sbiret ret = fwft_get(feature);
> +
> +	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
> +}

Yes, this is the function I suggested we add, but you neglected to add the
comment above it that I also suggested.

 /* Must be called before any fwft_set() call is made for @feature */

> +
>  static void fwft_check_base(void)
>  {
>  	report_prefix_push("base");
> @@ -99,18 +115,32 @@ static struct sbiret fwft_misaligned_exc_get(void)
>  static void fwft_check_misaligned_exc_deleg(void)
>  {
>  	struct sbiret ret;
> +	unsigned long expected;
>  
>  	report_prefix_push("misaligned_exc_deleg");
>  
>  	ret = fwft_misaligned_exc_get();
> -	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> -		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> +	if (ret.error != SBI_SUCCESS) {
> +		if (env_or_skip("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) {

We don't want env_or_skip() here since we don't want to output SKIP
logs for _HAVE_ environment variables. env_or_skip() is for getting
expected values for tests. In those cases, without the env, there's
no expected value to compare with, so we must skip the test.

> +			expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG"), NULL, 0);
> +			if (expected == 1)

We have env_enabled() in riscv/sbi.c which we can move to sbi-tests.h as a
static inline. That's what we should use here. (It doesn't use strtoul,
since there's no need to in order to compare with the string "1".)

> +			{

This { should be on the same line as the if.

> +				report_fail("not supported by platform");

This should be sbiret_report_error(&ret, SBI_SUCCESS, "supported") since
SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG says it's supported and we want to see
what error code we get when it's not success.

> +				return;
> +			}
> +		}
> +		report_skip("not supported by platform");
>  		return;
>  	}
>  
>  	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Get misaligned deleg feature"))
>  		return;
>  
> +	if (env_or_skip("MISALIGNED_EXC_DELEG_RESET")) {
> +		expected = (unsigned long)strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0);

No need for the (unsigned long) cast. strtoul() returns an unsigned long
(it even says it will in its name).

> +		fwft_check_reset(SBI_FWFT_MISALIGNED_EXC_DELEG, expected);
> +	}
> +
>  	ret = fwft_misaligned_exc_set(2, 0);
>  	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>  			    "Set misaligned deleg feature invalid value 2");
> @@ -129,16 +159,10 @@ static void fwft_check_misaligned_exc_deleg(void)
>  #endif
>  
>  	/* Set to 0 and check after with get */
> -	ret = fwft_misaligned_exc_set(0, 0);
> -	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0");
> -	ret = fwft_misaligned_exc_get();
> -	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg feature expected value 0");
> +	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
>  
>  	/* Set to 1 and check after with get */
> -	ret = fwft_misaligned_exc_set(1, 0);
> -	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 1");
> -	ret = fwft_misaligned_exc_get();
> -	sbiret_report(&ret, SBI_SUCCESS, 1, "Get misaligned deleg feature expected value 1");
> +	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
>  
>  	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
>  
> @@ -257,11 +281,20 @@ static void fwft_check_pte_ad_hw_updating(void)
>  {
>  	struct sbiret ret;
>  	bool enabled;
> +	unsigned long expected;
>  
>  	report_prefix_push("pte_ad_hw_updating");
>  
>  	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> -	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> +	if (ret.error != SBI_SUCCESS) {
> +		if (env_or_skip("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
> +			expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING"), NULL, 0);
> +			if (expected == 1)
> +			{
> +				report_fail("not supported by platform");
> +				return;
> +			}
> +		}

same comments as above

>  		report_skip("not supported by platform");
>  		return;
>  	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> @@ -269,10 +302,9 @@ static void fwft_check_pte_ad_hw_updating(void)
>  		return;
>  	}
>  
> -	report(ret.value == 0 || ret.value == 1, "first get value is 0/1");
> +	fwft_check_reset(SBI_FWFT_PTE_AD_HW_UPDATING, 1);

The reset value of PTE_AD_HW_UPDATING isn't 1. It's 0. See the spec.

>  
>  	enabled = ret.value;
> -	report_kfail(true, !enabled, "resets to 0");

This removes a test case that proved opensbi is broken. Always refer
to the spec when writing tests. We don't want to write tests (and
especially not change tests) just to output PASS.

Until we've fixed opensbi this test needs to stay as it is, i.e. with
the check for 0 or 1 for the get and then the kfail report for when
it's 1.

>  
>  	install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
>  	install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
> -- 
> 2.34.1
> 

Thanks,
drew

[RFC kvm-unit-tests PATCH v2] riscv: Refactoring sbi fwft tests

[Akshay Behl]

This patch refactors the current sbi fwft tests
(pte_ad_hw_updating, misaligned_exc_deleg)

v2:
 - Made env_or_skip and env_enabled methods shared by adding
   them to sbi-tests.h
 - Used env_enabled check instead of env_or_skip for
   platform support
 - Added the reset to 0/1 test back for pte_ad_hw_updating
 - Made other suggested changes

Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
---
 riscv/sbi-tests.h | 22 ++++++++++++++++++++++
 riscv/sbi-fwft.c  | 38 +++++++++++++++++++++++++++-----------
 riscv/sbi.c       | 17 -----------------
 3 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
index b081464d..91eba7b7 100644
--- a/riscv/sbi-tests.h
+++ b/riscv/sbi-tests.h
@@ -70,6 +70,28 @@
 #define sbiret_check(ret, expected_error, expected_value) \
 	sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
 
+/**
+ * Check if environment variable exists, skip test if missing
+ *
+ * @param env The environment variable name to check
+ * @return true if environment variable exists, false otherwise
+ */
+static inline bool env_or_skip(const char *env)
+{
+	if (!getenv(env)) {
+		report_skip("missing %s environment variable", env);
+		return false;
+	}
+	return true;
+}
+
+static inline bool env_enabled(const char *env)
+{
+	char *s = getenv(env);
+
+	return s && (*s == '1' || *s == 'y' || *s == 'Y');
+}
+
 void sbi_bad_fid(int ext);
 
 #endif /* __ASSEMBLER__ */
diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index ac2e3486..581cbf6b 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -66,6 +66,14 @@ static void fwft_check_reserved(unsigned long id)
 	sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
 }
 
+/* Must be called before any fwft_set() call is made for @feature */
+static void fwft_check_reset(uint32_t feature, unsigned long reset)
+{
+	struct sbiret ret = fwft_get(feature);
+
+	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
+}
+
 static void fwft_check_base(void)
 {
 	report_prefix_push("base");
@@ -99,18 +107,28 @@ static struct sbiret fwft_misaligned_exc_get(void)
 static void fwft_check_misaligned_exc_deleg(void)
 {
 	struct sbiret ret;
+	unsigned long expected;
 
 	report_prefix_push("misaligned_exc_deleg");
 
 	ret = fwft_misaligned_exc_get();
-	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
-		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
+	if (ret.error != SBI_SUCCESS) {
+		if (env_enabled("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) {
+			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
+			return;
+		}
+		report_skip("not supported by platform");
 		return;
 	}
 
 	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Get misaligned deleg feature"))
 		return;
 
+	if (env_or_skip("MISALIGNED_EXC_DELEG_RESET")) {
+		expected = strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0);
+		fwft_check_reset(SBI_FWFT_MISALIGNED_EXC_DELEG, expected);
+	}
+
 	ret = fwft_misaligned_exc_set(2, 0);
 	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
 			    "Set misaligned deleg feature invalid value 2");
@@ -129,16 +147,10 @@ static void fwft_check_misaligned_exc_deleg(void)
 #endif
 
 	/* Set to 0 and check after with get */
-	ret = fwft_misaligned_exc_set(0, 0);
-	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0");
-	ret = fwft_misaligned_exc_get();
-	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg feature expected value 0");
+	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
 
 	/* Set to 1 and check after with get */
-	ret = fwft_misaligned_exc_set(1, 0);
-	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 1");
-	ret = fwft_misaligned_exc_get();
-	sbiret_report(&ret, SBI_SUCCESS, 1, "Get misaligned deleg feature expected value 1");
+	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
 
 	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
 
@@ -261,7 +273,11 @@ static void fwft_check_pte_ad_hw_updating(void)
 	report_prefix_push("pte_ad_hw_updating");
 
 	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
-	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
+	if (ret.error != SBI_SUCCESS) {
+		if (env_enabled("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
+			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
+			return;
+		}
 		report_skip("not supported by platform");
 		return;
 	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 0404bb81..219f7187 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -131,23 +131,6 @@ static phys_addr_t get_highest_addr(void)
 	return highest_end - 1;
 }
 
-static bool env_enabled(const char *env)
-{
-	char *s = getenv(env);
-
-	return s && (*s == '1' || *s == 'y' || *s == 'Y');
-}
-
-static bool env_or_skip(const char *env)
-{
-	if (!getenv(env)) {
-		report_skip("missing %s environment variable", env);
-		return false;
-	}
-
-	return true;
-}
-
 static bool get_invalid_addr(phys_addr_t *paddr, bool allow_default)
 {
 	if (env_enabled("INVALID_ADDR_AUTO")) {
-- 
2.34.1

Re: [RFC kvm-unit-tests PATCH v2] riscv: Refactoring sbi fwft tests

[Andrew Jones]

On Thu, Mar 13, 2025 at 10:42:23PM +0530, Akshay Behl wrote:
> This patch refactors the current sbi fwft tests
> (pte_ad_hw_updating, misaligned_exc_deleg)
> 
> v2:
>  - Made env_or_skip and env_enabled methods shared by adding
>    them to sbi-tests.h
>  - Used env_enabled check instead of env_or_skip for
>    platform support
>  - Added the reset to 0/1 test back for pte_ad_hw_updating
>  - Made other suggested changes

The v2 changelog should go under the '---' below to keep it out of the
file commit message.

> 
> Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
> ---
>  riscv/sbi-tests.h | 22 ++++++++++++++++++++++
>  riscv/sbi-fwft.c  | 38 +++++++++++++++++++++++++++-----------
>  riscv/sbi.c       | 17 -----------------
>  3 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
> index b081464d..91eba7b7 100644
> --- a/riscv/sbi-tests.h
> +++ b/riscv/sbi-tests.h
> @@ -70,6 +70,28 @@
>  #define sbiret_check(ret, expected_error, expected_value) \
>  	sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
>  
> +/**
> + * Check if environment variable exists, skip test if missing
> + *
> + * @param env The environment variable name to check
> + * @return true if environment variable exists, false otherwise
> + */
> +static inline bool env_or_skip(const char *env)
> +{
> +	if (!getenv(env)) {
> +		report_skip("missing %s environment variable", env);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static inline bool env_enabled(const char *env)
> +{
> +	char *s = getenv(env);
> +
> +	return s && (*s == '1' || *s == 'y' || *s == 'Y');
> +}

We should include libcflat.h now that we've added these functions. Make
sure the include is under the '#ifndef __ASSEMBLER__' (and above the
'#include <asm/sbi.h>')

> +
>  void sbi_bad_fid(int ext);
>  
>  #endif /* __ASSEMBLER__ */
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index ac2e3486..581cbf6b 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -66,6 +66,14 @@ static void fwft_check_reserved(unsigned long id)
>  	sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
>  }
>  
> +/* Must be called before any fwft_set() call is made for @feature */
> +static void fwft_check_reset(uint32_t feature, unsigned long reset)
> +{
> +	struct sbiret ret = fwft_get(feature);
> +
> +	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
> +}
> +
>  static void fwft_check_base(void)
>  {
>  	report_prefix_push("base");
> @@ -99,18 +107,28 @@ static struct sbiret fwft_misaligned_exc_get(void)
>  static void fwft_check_misaligned_exc_deleg(void)
>  {
>  	struct sbiret ret;
> +	unsigned long expected;
>  
>  	report_prefix_push("misaligned_exc_deleg");
>  
>  	ret = fwft_misaligned_exc_get();
> -	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> -		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> +	if (ret.error != SBI_SUCCESS) {
> +		if (env_enabled("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) {
> +			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
> +			return;
> +		}
> +		report_skip("not supported by platform");
>  		return;
>  	}
>  
>  	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Get misaligned deleg feature"))
>  		return;
>  
> +	if (env_or_skip("MISALIGNED_EXC_DELEG_RESET")) {
> +		expected = strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0);
> +		fwft_check_reset(SBI_FWFT_MISALIGNED_EXC_DELEG, expected);
> +	}
> +
>  	ret = fwft_misaligned_exc_set(2, 0);
>  	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
>  			    "Set misaligned deleg feature invalid value 2");
> @@ -129,16 +147,10 @@ static void fwft_check_misaligned_exc_deleg(void)
>  #endif
>  
>  	/* Set to 0 and check after with get */
> -	ret = fwft_misaligned_exc_set(0, 0);
> -	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0");
> -	ret = fwft_misaligned_exc_get();
> -	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg feature expected value 0");
> +	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
>  
>  	/* Set to 1 and check after with get */
> -	ret = fwft_misaligned_exc_set(1, 0);
> -	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 1");
> -	ret = fwft_misaligned_exc_get();
> -	sbiret_report(&ret, SBI_SUCCESS, 1, "Get misaligned deleg feature expected value 1");
> +	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
>  
>  	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
>  
> @@ -261,7 +273,11 @@ static void fwft_check_pte_ad_hw_updating(void)
>  	report_prefix_push("pte_ad_hw_updating");
>  
>  	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> -	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> +	if (ret.error != SBI_SUCCESS) {
> +		if (env_enabled("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
> +			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
> +			return;
> +		}
>  		report_skip("not supported by platform");
>  		return;
>  	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 0404bb81..219f7187 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -131,23 +131,6 @@ static phys_addr_t get_highest_addr(void)
>  	return highest_end - 1;
>  }
>  
> -static bool env_enabled(const char *env)
> -{
> -	char *s = getenv(env);
> -
> -	return s && (*s == '1' || *s == 'y' || *s == 'Y');
> -}
> -
> -static bool env_or_skip(const char *env)
> -{
> -	if (!getenv(env)) {
> -		report_skip("missing %s environment variable", env);
> -		return false;
> -	}
> -
> -	return true;
> -}
> -
>  static bool get_invalid_addr(phys_addr_t *paddr, bool allow_default)
>  {
>  	if (env_enabled("INVALID_ADDR_AUTO")) {
> -- 
> 2.34.1
>

Other than the two comments above, it looks good. I've made the changes
myself while applying to riscv/sbi

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

Thanks,
drew

Re: [RFC kvm-unit-tests PATCH v2] riscv: Refactoring sbi fwft tests

[Andrew Jones]

On Fri, Mar 14, 2025 at 01:57:45PM +0100, Andrew Jones wrote:
> On Thu, Mar 13, 2025 at 10:42:23PM +0530, Akshay Behl wrote:
> > This patch refactors the current sbi fwft tests
> > (pte_ad_hw_updating, misaligned_exc_deleg)
> > 
> > v2:
> >  - Made env_or_skip and env_enabled methods shared by adding
> >    them to sbi-tests.h
> >  - Used env_enabled check instead of env_or_skip for
> >    platform support
> >  - Added the reset to 0/1 test back for pte_ad_hw_updating
> >  - Made other suggested changes
> 
> The v2 changelog should go under the '---' below to keep it out of the
> file commit message.
> 
> > 
> > Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
> > ---
> >  riscv/sbi-tests.h | 22 ++++++++++++++++++++++
> >  riscv/sbi-fwft.c  | 38 +++++++++++++++++++++++++++-----------
> >  riscv/sbi.c       | 17 -----------------
> >  3 files changed, 49 insertions(+), 28 deletions(-)
> > 
> > diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
> > index b081464d..91eba7b7 100644
> > --- a/riscv/sbi-tests.h
> > +++ b/riscv/sbi-tests.h
> > @@ -70,6 +70,28 @@
> >  #define sbiret_check(ret, expected_error, expected_value) \
> >  	sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
> >  
> > +/**
> > + * Check if environment variable exists, skip test if missing
> > + *
> > + * @param env The environment variable name to check
> > + * @return true if environment variable exists, false otherwise
> > + */

I also dropped this comment block. The '/**' made check-kerneldoc complain
and the comment didn't tell us anything that the four lines of code that
the function has was already easily telling us.

Thanks,
drew

> > +static inline bool env_or_skip(const char *env)
> > +{
> > +	if (!getenv(env)) {
> > +		report_skip("missing %s environment variable", env);
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> > +static inline bool env_enabled(const char *env)
> > +{
> > +	char *s = getenv(env);
> > +
> > +	return s && (*s == '1' || *s == 'y' || *s == 'Y');
> > +}
> 
> We should include libcflat.h now that we've added these functions. Make
> sure the include is under the '#ifndef __ASSEMBLER__' (and above the
> '#include <asm/sbi.h>')
> 
> > +
> >  void sbi_bad_fid(int ext);
> >  
> >  #endif /* __ASSEMBLER__ */
> > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> > index ac2e3486..581cbf6b 100644
> > --- a/riscv/sbi-fwft.c
> > +++ b/riscv/sbi-fwft.c
> > @@ -66,6 +66,14 @@ static void fwft_check_reserved(unsigned long id)
> >  	sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
> >  }
> >  
> > +/* Must be called before any fwft_set() call is made for @feature */
> > +static void fwft_check_reset(uint32_t feature, unsigned long reset)
> > +{
> > +	struct sbiret ret = fwft_get(feature);
> > +
> > +	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
> > +}
> > +
> >  static void fwft_check_base(void)
> >  {
> >  	report_prefix_push("base");
> > @@ -99,18 +107,28 @@ static struct sbiret fwft_misaligned_exc_get(void)
> >  static void fwft_check_misaligned_exc_deleg(void)
> >  {
> >  	struct sbiret ret;
> > +	unsigned long expected;
> >  
> >  	report_prefix_push("misaligned_exc_deleg");
> >  
> >  	ret = fwft_misaligned_exc_get();
> > -	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> > -		report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> > +	if (ret.error != SBI_SUCCESS) {
> > +		if (env_enabled("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) {
> > +			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
> > +			return;
> > +		}
> > +		report_skip("not supported by platform");
> >  		return;
> >  	}
> >  
> >  	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Get misaligned deleg feature"))
> >  		return;
> >  
> > +	if (env_or_skip("MISALIGNED_EXC_DELEG_RESET")) {
> > +		expected = strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0);
> > +		fwft_check_reset(SBI_FWFT_MISALIGNED_EXC_DELEG, expected);
> > +	}
> > +
> >  	ret = fwft_misaligned_exc_set(2, 0);
> >  	sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >  			    "Set misaligned deleg feature invalid value 2");
> > @@ -129,16 +147,10 @@ static void fwft_check_misaligned_exc_deleg(void)
> >  #endif
> >  
> >  	/* Set to 0 and check after with get */
> > -	ret = fwft_misaligned_exc_set(0, 0);
> > -	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0");
> > -	ret = fwft_misaligned_exc_get();
> > -	sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg feature expected value 0");
> > +	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
> >  
> >  	/* Set to 1 and check after with get */
> > -	ret = fwft_misaligned_exc_set(1, 0);
> > -	sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 1");
> > -	ret = fwft_misaligned_exc_get();
> > -	sbiret_report(&ret, SBI_SUCCESS, 1, "Get misaligned deleg feature expected value 1");
> > +	fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
> >  
> >  	install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
> >  
> > @@ -261,7 +273,11 @@ static void fwft_check_pte_ad_hw_updating(void)
> >  	report_prefix_push("pte_ad_hw_updating");
> >  
> >  	ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > -	if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> > +	if (ret.error != SBI_SUCCESS) {
> > +		if (env_enabled("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
> > +			sbiret_report_error(&ret, SBI_SUCCESS, "supported");
> > +			return;
> > +		}
> >  		report_skip("not supported by platform");
> >  		return;
> >  	} else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index 0404bb81..219f7187 100644
> > --- a/riscv/sbi.c
> > +++ b/riscv/sbi.c
> > @@ -131,23 +131,6 @@ static phys_addr_t get_highest_addr(void)
> >  	return highest_end - 1;
> >  }
> >  
> > -static bool env_enabled(const char *env)
> > -{
> > -	char *s = getenv(env);
> > -
> > -	return s && (*s == '1' || *s == 'y' || *s == 'Y');
> > -}
> > -
> > -static bool env_or_skip(const char *env)
> > -{
> > -	if (!getenv(env)) {
> > -		report_skip("missing %s environment variable", env);
> > -		return false;
> > -	}
> > -
> > -	return true;
> > -}
> > -
> >  static bool get_invalid_addr(phys_addr_t *paddr, bool allow_default)
> >  {
> >  	if (env_enabled("INVALID_ADDR_AUTO")) {
> > -- 
> > 2.34.1
> >
> 
> Other than the two comments above, it looks good. I've made the changes
> myself while applying to riscv/sbi
> 
> https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi
> 
> Thanks,
> drew

Re: [RFC kvm-unit-tests PATCH v2] riscv: Refactoring sbi fwft tests

[Andrew Jones]

On Thu, Mar 13, 2025 at 10:42:23PM +0530, Akshay Behl wrote:
> This patch refactors the current sbi fwft tests
> (pte_ad_hw_updating, misaligned_exc_deleg)
> 
> v2:
>  - Made env_or_skip and env_enabled methods shared by adding
>    them to sbi-tests.h
>  - Used env_enabled check instead of env_or_skip for
>    platform support
>  - Added the reset to 0/1 test back for pte_ad_hw_updating
>  - Made other suggested changes
> 
> Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
> ---
>  riscv/sbi-tests.h | 22 ++++++++++++++++++++++
>  riscv/sbi-fwft.c  | 38 +++++++++++++++++++++++++++-----------
>  riscv/sbi.c       | 17 -----------------
>  3 files changed, 49 insertions(+), 28 deletions(-)

Merged.

Thanks,
drew