[PATCH v2 0/3] hw/riscv/riscv-iommu.c: additional PTE checks

[PATCH v2 0/3] hw/riscv/riscv-iommu.c: additional PTE checks

[Daniel Henrique Barboza]

Hi,

I'm sending a v2 because I just ended up fixed another iommu bug that
happened to be in the same code, and it has a test dependency with patch
2.  Sending it standalone will cause conflicts so I'm adding it in the
batch.

Patches 1 and 2 didn't change.  R-bs from Chao and Nutty were added in
both.

Patches based on alistair/riscv-to-apply.next.


Changes from v1:
- patch 3 (new):
  - add another PTE_U fault that we're missing (gitlab 3555)
- v1 link: https://lore.kernel.org/qemu-devel/20260629121334.567587-1-daniel.barboza@oss.qualcomm.com/

Daniel Henrique Barboza (3):
  hw/riscv/riscv-iommu.c: check for reserved PTE bits
  hw/riscv/riscv-iommu.c: fault when !PTE_U and no priv access
  hw/riscv/riscv-iommu.c: fault for non-user PTE in G_STAGE

 hw/riscv/riscv-iommu.c               | 20 ++++++++++++++++++++
 tests/qtest/libqos/qos-riscv-iommu.h |  4 ++--
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] hw/riscv/riscv-iommu.c: check for reserved PTE bits

[Daniel Henrique Barboza]

We need to fault if reserved PTE bits (60:54) are set.

Fixes: 0c54acb8243d ("hw/riscv: add RISC-V IOMMU base emulation")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3554
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>
---
 hw/riscv/riscv-iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index 974042d017..2c2273e1d2 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -468,6 +468,8 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
 
         if (!(pte & PTE_V)) {
             break;                /* Invalid PTE */
+        } else if (pte & PTE_RESERVED(false)) {
+            break;                /* Reserved PTE bits set */
         } else if (!(pte & (PTE_R | PTE_W | PTE_X))) {
             base = PPN_PHYS(ppn); /* Inner PTE, continue walking */
         } else if ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W) {
-- 
2.43.0

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

[Daniel Henrique Barboza]

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>
---
 hw/riscv/riscv-iommu.c               | 8 ++++++++
 tests/qtest/libqos/qos-riscv-iommu.h | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index 2c2273e1d2..3b165348ca 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -297,6 +297,7 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
         G_STAGE = 1,
     } pass;
     MemTxResult ret;
+    bool pv = !!ctx->process_id;
 
     satp = get_field(ctx->satp, RISCV_IOMMU_ATP_MODE_FIELD);
     gatp = get_field(ctx->gatp, RISCV_IOMMU_ATP_MODE_FIELD);
@@ -470,6 +471,13 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
             break;                /* Invalid PTE */
         } else if (pte & PTE_RESERVED(false)) {
             break;                /* Reserved PTE bits set */
+        } else if (!(pte & PTE_U) && !pv) {
+            /*
+             * All accesses are assumed to be User mode unless
+             * process_id is valid (pv).  In case we have a
+             * non-user mode PTE and !pv we need to fault.
+             */
+            break;
         } else if (!(pte & (PTE_R | PTE_W | PTE_X))) {
             base = PPN_PHYS(ppn); /* Inner PTE, continue walking */
         } else if ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W) {
diff --git a/tests/qtest/libqos/qos-riscv-iommu.h b/tests/qtest/libqos/qos-riscv-iommu.h
index 90e69a5d73..c218e9d66d 100644
--- 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 */
-- 
2.43.0

[PATCH v2 3/3] hw/riscv/riscv-iommu.c: fault for non-user PTE in G_STAGE

[Daniel Henrique Barboza]

riscv-iommu spec 1.0 says:

"When checking the U bit in a second-stage PTE, the transaction
 is treated as not requesting supervisor privilege."

We need to *always* fault in case we're on G_STAGE and PTE_U is cleared
since we can't be on supervisor mode at this point.

Fixes: 0c54acb8243d ("hw/riscv: add RISC-V IOMMU base emulation")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3555
Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
---
 hw/riscv/riscv-iommu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index 3b165348ca..29eec3133e 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -494,6 +494,16 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
             break;                /* Access bit not set */
         } else if ((iotlb->perm & IOMMU_WO) && !ade && !(pte & PTE_D)) {
             break;                /* Dirty bit not set */
+        } else if (pass == G_STAGE && !(pte & PTE_U)) {
+            /*
+             * riscv-iommu spec 1.0: "When checking the U bit in a
+             * second-stage PTE, the transaction is treated as
+             * not requesting supervisor privilege."
+             *
+             * I.e. we need to fault if this is a non-user PTE since
+             * we are always in user mode at this point.
+             */
+            break;
         } else {
             /* Leaf PTE, translation completed. */
             sc[pass].step = sc[pass].levels;
-- 
2.43.0

Re: [PATCH v2 3/3] hw/riscv/riscv-iommu.c: fault for non-user PTE in G_STAGE

[Alistair Francis]

On Wed, Jul 1, 2026 at 10:12 PM Daniel Henrique Barboza
<daniel.barboza@oss.qualcomm.com> wrote:
>
> riscv-iommu spec 1.0 says:
>
> "When checking the U bit in a second-stage PTE, the transaction
>  is treated as not requesting supervisor privilege."
>
> We need to *always* fault in case we're on G_STAGE and PTE_U is cleared
> since we can't be on supervisor mode at this point.
>
> Fixes: 0c54acb8243d ("hw/riscv: add RISC-V IOMMU base emulation")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3555
> Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/riscv-iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index 3b165348ca..29eec3133e 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -494,6 +494,16 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
>              break;                /* Access bit not set */
>          } else if ((iotlb->perm & IOMMU_WO) && !ade && !(pte & PTE_D)) {
>              break;                /* Dirty bit not set */
> +        } else if (pass == G_STAGE && !(pte & PTE_U)) {
> +            /*
> +             * riscv-iommu spec 1.0: "When checking the U bit in a
> +             * second-stage PTE, the transaction is treated as
> +             * not requesting supervisor privilege."
> +             *
> +             * I.e. we need to fault if this is a non-user PTE since
> +             * we are always in user mode at this point.
> +             */
> +            break;
>          } else {
>              /* Leaf PTE, translation completed. */
>              sc[pass].step = sc[pass].levels;
> --
> 2.43.0
>
>

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

[Tao Tang]

On 7/1/2026 8:11 PM, 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>

Reviewed-by: Tao Tang <tangtao1634@phytium.com.cn>

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

[Michael Tokarev]

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?

Thanks,

/mjt

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

[Michael Tokarev]

On 7/11/26 11:30, Michael Tokarev wrote:
> 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've dropped this patch from 10.0.x (lts) series for now,
while picking up other patches - in order to go with this
at least somewhere.  But I really don't know what to do
with all this :)  Lemme sleep on that for some time :)

Thanks,

/mjt

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

[Fabiano Rosas]

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

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

[Michael Tokarev]

On 7/13/26 16:00, Fabiano Rosas wrote:
> Michael Tokarev <mjt@tls.msk.ru> 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.

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.

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 :)

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

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

[Fabiano Rosas]

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

> On 7/13/26 16:00, Fabiano Rosas wrote:
>> Michael Tokarev <mjt@tls.msk.ru> 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