Re: [PATCH v2 2/3] hw/riscv/riscv-iommu.c: fault when !PTE_U and no priv access

[Fabiano Rosas <farosas@suse.de>]

Michael Tokarev <mjt@tls.msk.ru> writes:

> On 7/1/26 15:11, Daniel Henrique Barboza wrote:
>> All IOMMU accesses are assumed to be user mode unless told otherwise,
>> i.e. we have a process_id.  In case we have a non-user mode leaf PTE
>> (PTE_U isn't set) and we are running in user mode, we need to throw a
>> fault.
>> 
>> This also reflects on qos-riscv-iommu tests: the tests always run in
>> user mode so our PTEs must have PTE_U (bit 0x10) set.
>> 
>> Fixes: 0c54acb8243d ("hw/riscv: add RISC-V IOMMU base emulation")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3553
>> Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
>> Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>
>
> ...
>> --- a/tests/qtest/libqos/qos-riscv-iommu.h
>> +++ b/tests/qtest/libqos/qos-riscv-iommu.h
>> @@ -54,8 +54,8 @@
>>    * PTE masks for RISC-V IOMMU page tables.
>>    * Values match PTE_V, PTE_R, PTE_W, PTE_A, PTE_D in target/riscv/cpu_bits.h
>>    */
>> -#define QRIOMMU_NON_LEAF_PTE_MASK   0x001  /* PTE_V */
>> -#define QRIOMMU_LEAF_PTE_RW_MASK    0x0c7  /* V|R|W|A|D */
>> +#define QRIOMMU_NON_LEAF_PTE_MASK   0x011  /* PTE_V | PTE_U */
>> +#define QRIOMMU_LEAF_PTE_RW_MASK    0x0d7  /* V | R | W | A | D | PTE_U */
>>   #define QRIOMMU_PTE_PPN_MASK        0x003ffffffffffc00ull
>>   
>>   /* Address-space base offset for test tables */
>
> So, this patch adds a fix for the testing bits.
>
> When trying to pick this one up for 10.0.x (LTS) series, I've another doubt.
> This testing fix fixes v10.2.0-1299-g9d8ffbfc1d3 "tests/qtest/libqos: Add RISC-V
> IOMMU helper library".  Quite some tests were added based on that library.
> It doesn't exist in 10.0.x, and neither is v10.2.0-656-g489812e32df
> "tests/qtest/libqos: Add SMMUv3 helper library".
> Sure I can drop the parts of this change which touches the tests.  But this
> means we don't have tests to cover the issues being fixed, and I don't know
> if the result of these fixes actually works or not.
>
> Should we pick up some testing bits in this area for 10.0.x (especially
> 9d8ffbfc1d3 and some subsequent commits which use this library)?  Or just
> ignore all riscv iommu patches in there?
>

I worry picking tests for stable could become a development task,
requiring new code to make tests suitable for an older codebase/test
codebase.

The test frameworks and their supporting infrastructure (such as the
riscv iommu lib) don't ensure a stable abi so that a test from one
version will run without issues when backported to an earlier
version.

Also, the tests are usually not structured in a way that allow us to
pick just the part that tests the code fixes being backported, so we
risk trying to test in an earlier version features that don't even exist
at that point. There could also exist a complex graph of what's broken
vs. what is being tested (e.g. broken in v10, fixed partially in v11,
then test needs change, then fixed properly in v12, test needs change
again, etc).

> Thanks,
>
> /mjt