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

[Clément Léger]

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.

Thanks,

Clément

> 
> Thanks,
> drew