diff mbox series

[kvm-unit-tests] riscv: Refactor SBI FWFT lock tests

Message ID 20250316123209.100561-1-akshaybehl231@gmail.com (mailing list archive)
State New
Headers show
Series [kvm-unit-tests] riscv: Refactor SBI FWFT lock tests | expand

Commit Message

Akshay Behl March 16, 2025, 12:32 p.m. UTC 
This patch adds a generic function for lock tests for all
the sbi fwft features. It expects the feature is already
locked before being called and tests the locked feature.

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

Comments

Clément Léger March 17, 2025, 8:20 a.m. UTC | #1 
On 16/03/2025 13:32, Akshay Behl wrote:
> This patch adds a generic function for lock tests for all
> the sbi fwft features. It expects the feature is already
> locked before being called and tests the locked feature.
> 
> Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
> ---
>  riscv/sbi-fwft.c | 48 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index 581cbf6b..5c0a7f6f 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -74,6 +74,33 @@ static void fwft_check_reset(uint32_t feature, unsigned long reset)
>  	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
>  }
>  
> +/* Must be called after locking the feature using SBI_FWFT_SET_FLAG_LOCK */
> +static void fwft_feature_lock_test(int32_t feature, unsigned long locked_value)
> +{
> +    struct sbiret ret;
> +    unsigned long alt_value = locked_value ? 0 : 1;

Hi Akshay,

This will work for boolean FWFT features but might not work for PMLEN
for instance. It could be good to pass the alt value (or values for
PMLEN) as an argument to this function.

Thanks,

Clément

> +
> +    ret = fwft_set(feature, locked_value, 0);
> +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> +        "Set locked feature to %lu without lock", locked_value);
> +
> +    ret = fwft_set(feature, locked_value, SBI_FWFT_SET_FLAG_LOCK);
> +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> +        "Set locked feature to %lu with lock", locked_value);
> +
> +    ret = fwft_set(feature, alt_value, 0);
> +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> +        "Set locked feature to %lu without lock", alt_value);
> +
> +    ret = fwft_set(feature, alt_value, SBI_FWFT_SET_FLAG_LOCK);
> +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> +        "Set locked feature to %lu with lock", alt_value);
> +
> +    ret = fwft_get(feature);
> +    sbiret_report(&ret, SBI_SUCCESS, locked_value,
> +        "Get locked feature value %lu", locked_value);
> +}
> +
>  static void fwft_check_base(void)
>  {
>  	report_prefix_push("base");
> @@ -181,11 +208,9 @@ static void fwft_check_misaligned_exc_deleg(void)
>  	/* Lock the feature */
>  	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_DENIED_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");
> +
> +	/* Test feature lock */
> +	fwft_feature_lock_test(SBI_FWFT_MISALIGNED_EXC_DELEG, 0);
>  
>  	report_prefix_pop();
>  }
> @@ -326,17 +351,8 @@ adue_inval_tests:
>  	else
>  		enabled = !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_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
> -	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
> +	/* Test the feature lock */
> +	fwft_feature_lock_test(SBI_FWFT_PTE_AD_HW_UPDATING, enabled);
>  
>  adue_done:
>  	install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);
Andrew Jones March 17, 2025, 2:06 p.m. UTC | #2 
On Mon, Mar 17, 2025 at 09:20:27AM +0100, Clément Léger wrote:
> 
> 
> On 16/03/2025 13:32, Akshay Behl wrote:
> > This patch adds a generic function for lock tests for all
> > the sbi fwft features. It expects the feature is already
> > locked before being called and tests the locked feature.
> > 
> > Signed-off-by: Akshay Behl <akshaybehl231@gmail.com>
> > ---
> >  riscv/sbi-fwft.c | 48 ++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 32 insertions(+), 16 deletions(-)
> > 
> > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> > index 581cbf6b..5c0a7f6f 100644
> > --- a/riscv/sbi-fwft.c
> > +++ b/riscv/sbi-fwft.c
> > @@ -74,6 +74,33 @@ static void fwft_check_reset(uint32_t feature, unsigned long reset)
> >  	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
> >  }
> >  
> > +/* Must be called after locking the feature using SBI_FWFT_SET_FLAG_LOCK */
> > +static void fwft_feature_lock_test(int32_t feature, unsigned long locked_value)
                                        ^
                                        ^ uint32_t

> > +{
> > +    struct sbiret ret;
> > +    unsigned long alt_value = locked_value ? 0 : 1;
> 
> Hi Akshay,
> 
> This will work for boolean FWFT features but might not work for PMLEN
> for instance. It could be good to pass the alt value (or values for
> PMLEN) as an argument to this function.

Yup,

 static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
                                           unsigned long test_values[])
 {
	for (i = 0; i < nr_values; ++i) {
	    ... test without lock flag ...
	    ... test with lock flag ...
	}

	... test get ...
 }
 
 static void fwft_feature_lock_test(uint32_t feature)
 {
    unsigned long values[] = { 0, 1 };

    fwft_feature_lock_test_values(feature, 2, values);
 }

So we have fwft_feature_lock_test() for boolean features (likely most of
them) and also fwft_feature_lock_test_values() for everything else.

Thanks,
drew

> 
> Thanks,
> 
> Clément
> 
> > +
> > +    ret = fwft_set(feature, locked_value, 0);
> > +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> > +        "Set locked feature to %lu without lock", locked_value);
> > +
> > +    ret = fwft_set(feature, locked_value, SBI_FWFT_SET_FLAG_LOCK);
> > +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> > +        "Set locked feature to %lu with lock", locked_value);
> > +
> > +    ret = fwft_set(feature, alt_value, 0);
> > +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> > +        "Set locked feature to %lu without lock", alt_value);
> > +
> > +    ret = fwft_set(feature, alt_value, SBI_FWFT_SET_FLAG_LOCK);
> > +    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> > +        "Set locked feature to %lu with lock", alt_value);
> > +
> > +    ret = fwft_get(feature);
> > +    sbiret_report(&ret, SBI_SUCCESS, locked_value,
> > +        "Get locked feature value %lu", locked_value);
> > +}
> > +
> >  static void fwft_check_base(void)
> >  {
> >  	report_prefix_push("base");
> > @@ -181,11 +208,9 @@ static void fwft_check_misaligned_exc_deleg(void)
> >  	/* Lock the feature */
> >  	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_DENIED_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");
> > +
> > +	/* Test feature lock */
> > +	fwft_feature_lock_test(SBI_FWFT_MISALIGNED_EXC_DELEG, 0);
> >  
> >  	report_prefix_pop();
> >  }
> > @@ -326,17 +351,8 @@ adue_inval_tests:
> >  	else
> >  		enabled = !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_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
> > -	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
> > +	/* Test the feature lock */
> > +	fwft_feature_lock_test(SBI_FWFT_PTE_AD_HW_UPDATING, enabled);
> >  
> >  adue_done:
> >  	install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);
>
diff mbox series

Patch

diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
index 581cbf6b..5c0a7f6f 100644
--- a/riscv/sbi-fwft.c
+++ b/riscv/sbi-fwft.c
@@ -74,6 +74,33 @@  static void fwft_check_reset(uint32_t feature, unsigned long reset)
 	sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
 }
 
+/* Must be called after locking the feature using SBI_FWFT_SET_FLAG_LOCK */
+static void fwft_feature_lock_test(int32_t feature, unsigned long locked_value)
+{
+    struct sbiret ret;
+    unsigned long alt_value = locked_value ? 0 : 1;
+
+    ret = fwft_set(feature, locked_value, 0);
+    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
+        "Set locked feature to %lu without lock", locked_value);
+
+    ret = fwft_set(feature, locked_value, SBI_FWFT_SET_FLAG_LOCK);
+    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
+        "Set locked feature to %lu with lock", locked_value);
+
+    ret = fwft_set(feature, alt_value, 0);
+    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
+        "Set locked feature to %lu without lock", alt_value);
+
+    ret = fwft_set(feature, alt_value, SBI_FWFT_SET_FLAG_LOCK);
+    sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
+        "Set locked feature to %lu with lock", alt_value);
+
+    ret = fwft_get(feature);
+    sbiret_report(&ret, SBI_SUCCESS, locked_value,
+        "Get locked feature value %lu", locked_value);
+}
+
 static void fwft_check_base(void)
 {
 	report_prefix_push("base");
@@ -181,11 +208,9 @@  static void fwft_check_misaligned_exc_deleg(void)
 	/* Lock the feature */
 	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_DENIED_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");
+
+	/* Test feature lock */
+	fwft_feature_lock_test(SBI_FWFT_MISALIGNED_EXC_DELEG, 0);
 
 	report_prefix_pop();
 }
@@ -326,17 +351,8 @@  adue_inval_tests:
 	else
 		enabled = !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_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
-	sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
+	/* Test the feature lock */
+	fwft_feature_lock_test(SBI_FWFT_PTE_AD_HW_UPDATING, enabled);
 
 adue_done:
 	install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);