[kvm-unit-tests PATCH 01/10] riscv: sbi: Mark known fwft failures as kfails

[Andrew Jones]

On Tue, Feb 25, 2025 at 04:48:42PM +0100, Clément Léger wrote:
> 
> 
> On 25/02/2025 16:32, Andrew Jones wrote:
> > On Tue, Feb 25, 2025 at 11:25:13AM +0100, Andrew Jones wrote:
> >> On Tue, Feb 25, 2025 at 11:14:13AM +0100, Clément Léger wrote:
> >>>
> >>>
> >>> On 21/02/2025 16:55, Andrew Jones wrote:
> >>>> Until we fix opensbi mark these known failures as kfails so we can
> >>>> pass CI.
> >>>>
> >>>> Signed-off-by: Andrew Jones <andrew.jones at linux.dev>
> >>>> ---
> >>>>  riscv/sbi-fwft.c | 15 ++++++++++++---
> >>>>  1 file changed, 12 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> >>>> index b10c147f22dd..19340d6bb48c 100644
> >>>> --- a/riscv/sbi-fwft.c
> >>>> +++ b/riscv/sbi-fwft.c
> >>>> @@ -63,11 +63,17 @@ static void fwft_check_base(void)
> >>>>  		struct sbiret ret;
> >>>>  
> >>>>  		ret = fwft_get_raw(BIT(32));
> >>>> -		sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >>>> +		if (ret.error == 0)
> >>>> +			report_kfail(true, false, "get feature with bit 32 set error: SBI_ERR_INVALID_PARAM");
> >>>> +		else
> >>>> +			sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> >>>>  				    "get feature with bit 32 set error");
> >>>
> >>>
> >>> Hey Andrew,
> >>>
> >>> I'm actually a bit confused here. As raised by Anup, the spec states that:
> >>>
> >>> "Every SBI function should prefer unsigned long as the data type. It
> >>> keeps the specification simple and easily adaptable for all RISC-V ISA
> >>> types. In case the data is defined as 32bit wide, higher privilege
> >>> software must ensure that it only uses 32 bit data."
> >>>
> >>> Does this means 64 bits value are truncated to 32 bits ?
> >>
> >> Ah, yes. I had noticed that required truncation while doing some KVM fixes
> >> and patch 9 of this series is even adding a test for it for SUSP. I just
> >> didn't look closely enough at why this FWFT test was failing. The test
> >> should get SBI_SUCCESS, as it does, but it should also check that
> >> feature = BIT(32) behaves the same as features = 1.
> > 
> > Eh, '...as feature = 0 (MISALIGNED_EXC_DELEG)'. For now, I think we should
> > just drop the test, though, because we need a feature that, when present,
> > always allows enable/disable in order to test for a change in behavior.
> > While we may be able to toggle 0/1 for MISALIGNED_EXC_DELEG, we can't
> > count on traps getting enabled. I'll add ADUE tests and do a truncated
> > feature ID test for it.
> 
> Ah yeah totally make sense ! I'll modify SSE test as well to test that
> 64 bits identifier are correctly truncated.
>

I sent a patch 11/10 to this series with the ADUE test including a
truncated feature id.

Thanks
drew