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

[Andrew Jones <andrew.jones@linux.dev>]

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