diff mbox series

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

Message ID 20250321165403.57859-8-andrew.jones@linux.dev (mailing list archive)
State New
Headers show
Series riscv: sbi: Ensure we can pass with any opensbi | expand

Commit Message

Andrew Jones March 21, 2025, 4:54 p.m. UTC 
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(-)

Comments

Clément Léger March 21, 2025, 8:22 p.m. UTC | #1 
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 ?

>  
>  	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@rivosinc.com>

Thanks,

Clément
Andrew Jones March 22, 2025, 7:38 a.m. UTC | #2 
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
diff mbox series

Patch

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);
 
 	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};
+
 	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();