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

[Andrew Jones <andrew.jones@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> >  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);
>