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

Michael Tokarev <[email protected]> writes:

> On 7/13/26 16:00, Fabiano Rosas wrote:
>> Michael Tokarev <[email protected]> writes:
>> 
>
> [..]
>>> 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).
>
> Things aren't that bad in practice.  Even a large new test lib with
> some significant amount of tests built on top mostly Just Works (tm).
> A bigger problem is to pick them up across various restructuring of
> code, moving and splitting files, etc.
>

Well, for qtest I rely a lot on the maintainers of the actual code being
tested knowing what they're doing, I can't keep up with every single
test that's added, so I think even somewhat trivial shuffling of code
would be disruptive. You're probably quite used to dealing with these
things during normal (non-test) backporting, so it's probably fine.

> Also, if a test fails due to old code, we can look and fix it, - it
> is just a test after all.  Or even mark it as "ok to fail" - if we know
> it's due to the old code.
>

Right, but this requires carrying stable-only patches, which makes me
confused. Again, my own lack of understanding.

> But having no tests at all - in my view - is worse.  Besides the tests
> which also needs some back-porting/reviewing work when picked up for
> older releases, the main code needs the same.  And without the tests
> in place -- no tests in place at all -- we just don't know if something
> is broken until users start reporting it.  Provided there *are* users
> of this code in the older releases to begin with :)
>

Sure, if you think the effort is not that big, having tests makes
everything easier, I agree.

> And actually, this last one is a good question, I think.  As in - do
> we really need the fixed iommu stuff in 10.0.x, when a lot of other
> development happened in this and other parts of riscv64 since then?
> 10.0.x is used in debian trixie (current stable), and probably that's
> it, - not even ubuntu uses it (which is derived from debian).  So, I
> wonder, if this stuff is really interesting to someone outside of the
> most active riscv community, who most likely uses the current qemu
> anyway?
>
> I'll pick this particular change to 10.0.x as the last missing one from
> this pull request, since all the others has been picked up (but without
> the change to tests).  Speaking of the future changes, lemme send a
> separate email summarizing all this in a single place.  It feels like
> we don't actually need many riscv changes in older 10.0.x at least.
>
> Thanks,
>
> /mjt