[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 at 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 at rivosinc.com>

Thanks,

Clément