[v1] hw/riscv/riscv-iommu.c: check for misaligned IOHGATP_PPN

[PATCH] hw/riscv/riscv-iommu.c: check for misaligned IOHGATP_PPN

Posted by Daniel Henrique Barboza 3 weeks, 4 days ago

We must check if IOHGATP_PPN is 16kb aligned for non-bare GATP modes.

qos-riscv-iommu.h needs change too since G_IOHGATP must now be 16kb
aligned too.

Fixes: 69a9ae4836 ("hw/riscv/riscv-iommu: add ATS support")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3550
Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
---
 hw/riscv/riscv-iommu.c               | 14 ++++++++++++++
 tests/qtest/libqos/qos-riscv-iommu.h |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index 891a56e731..09990c640f 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -795,6 +795,20 @@ static bool riscv_iommu_validate_device_ctx(RISCVIOMMUState *s,
         return false;
     }
 
+    if (gatp != RISCV_IOMMU_DC_IOHGATP_MODE_BARE) {
+        uint64_t iohgatp_ppn = get_field(ctx->gatp,
+                                         RISCV_IOMMU_DC_IOHGATP_PPN);
+        /*
+         * One of the conditions for a misconfigured DDT entry
+         * according to the riscv-spec: "DC.iohgatp.MODE is not
+         * Bare and the root page table determined by DC.iohgatp.PPN
+         * is not aligned to a 16-KiB boundary."
+         */
+        if (iohgatp_ppn & ((1ULL << 14) - 1)) {
+            return false;
+        }
+    }
+
     fsc_mode = get_field(ctx->satp, RISCV_IOMMU_DC_FSC_MODE);
 
     if (ctx->tc & RISCV_IOMMU_DC_TC_PDTV) {
diff --git a/tests/qtest/libqos/qos-riscv-iommu.h b/tests/qtest/libqos/qos-riscv-iommu.h
index 90e69a5d73..4a972401f5 100644
--- a/tests/qtest/libqos/qos-riscv-iommu.h
+++ b/tests/qtest/libqos/qos-riscv-iommu.h
@@ -46,7 +46,7 @@
 #define QRIOMMU_L1_PTE_VAL          0x0000000000012000ull
 #define QRIOMMU_L2_PTE_VAL          0x0000000000013000ull
 
-#define QRIOMMU_G_IOHGATP           0x0000000000020000ull
+#define QRIOMMU_G_IOHGATP           0x0000000008000000ull
 #define QRIOMMU_G_L0_PTE_VAL        0x0000000000021000ull
 #define QRIOMMU_G_L1_PTE_VAL        0x0000000000022000ull
 
-- 
2.43.0

Re: [PATCH] hw/riscv/riscv-iommu.c: check for misaligned IOHGATP_PPN

Posted by Chao Liu 3 weeks, 2 days ago

On Tue, Jun 30, 2026 at 05:31:55PM +0800, Daniel Henrique Barboza wrote:
> We must check if IOHGATP_PPN is 16kb aligned for non-bare GATP modes.
> 
> qos-riscv-iommu.h needs change too since G_IOHGATP must now be 16kb
> aligned too.
> 
> Fixes: 69a9ae4836 ("hw/riscv/riscv-iommu: add ATS support")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3550
> Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> ---
>  hw/riscv/riscv-iommu.c               | 14 ++++++++++++++
>  tests/qtest/libqos/qos-riscv-iommu.h |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index 891a56e731..09990c640f 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -795,6 +795,20 @@ static bool riscv_iommu_validate_device_ctx(RISCVIOMMUState *s,
>          return false;
>      }
>  
> +    if (gatp != RISCV_IOMMU_DC_IOHGATP_MODE_BARE) {
> +        uint64_t iohgatp_ppn = get_field(ctx->gatp,
> +                                         RISCV_IOMMU_DC_IOHGATP_PPN);
> +        /*
> +         * One of the conditions for a misconfigured DDT entry
> +         * according to the riscv-spec: "DC.iohgatp.MODE is not
> +         * Bare and the root page table determined by DC.iohgatp.PPN
> +         * is not aligned to a 16-KiB boundary."
> +         */
> +        if (iohgatp_ppn & ((1ULL << 14) - 1)) {
I believe that if we are checking the PPN of this hgatp register, we should be
checking its lower 2 bits. According to the RISC-V specification, this is meant
to check the 16 KB alignment of the base address for the second-stage page
table. 

It's equal:

 ```text
   addr = PPN << 12
   16 KiB = 2^14
   so, PPN just need lower 14 - 12 = 2 bit is 0
 ```

Reference:

 1. iohgatp section：

 The iohgatp field holds the PPN of the root second-stage page table ...
 The root page table as determined by iohgatp.PPN is 16 KiB and must be aligned
 to a 16-KiB boundary.

 2. Device-context configuration checks：

 A DC with DC.tc.V=1 is considered as misconfigured if any of the following
 conditions are true. If misconfigured then, stop and report "DDT entry
 misconfigured" (cause = 259).
  ...
 DC.iohgatp.MODEis not Bare and the root page table determined by DC.iohgatp.PPN
 is not aligned to a 16-KiB boundary.

So, We can modify it this way.

 ```c
   if (iohgatp_ppn & ((1ULL << 2) - 1)) {
       return false;
   }
 ```

To better align with what you expressed in your comments, we are
obtaining the page table base address directly from the PPN.

 ```c
   if (PPN_PHYS(iohgatp_ppn) & ((1ULL << 14) - 1)) {
       return false;
   }
 ```
> +            return false;
> +        }
> +    }
> +
>      fsc_mode = get_field(ctx->satp, RISCV_IOMMU_DC_FSC_MODE);
>  
>      if (ctx->tc & RISCV_IOMMU_DC_TC_PDTV) {
> diff --git a/tests/qtest/libqos/qos-riscv-iommu.h b/tests/qtest/libqos/qos-riscv-iommu.h
> index 90e69a5d73..4a972401f5 100644
> --- a/tests/qtest/libqos/qos-riscv-iommu.h
> +++ b/tests/qtest/libqos/qos-riscv-iommu.h
> @@ -46,7 +46,7 @@
>  #define QRIOMMU_L1_PTE_VAL          0x0000000000012000ull
>  #define QRIOMMU_L2_PTE_VAL          0x0000000000013000ull
>  
> -#define QRIOMMU_G_IOHGATP           0x0000000000020000ull
> +#define QRIOMMU_G_IOHGATP           0x0000000008000000ull
I believe this isn't an adjustment. as the original value was specifically
set to perform the 16KB alignment check.

The iommu helper function will handle:

 ```c
   iohgatp = qriommu_apply_space_offs(QRIOMMU_G_IOHGATP);
   dc.iohgatp = (iohgatp >> 12) | (8ull << 60);
 ```

So, we can get:

 ```
   iohgatp addr = 0x80000000 + 0x20000 = 0x80020000
   PPN         = 0x80020000 >> 12 = 0x80020
 ```

This is a valid 16 KB byte alignment check:

```
   0x80020000 & 0x3fff == 0
   equal
   0x80020000 & ((1ULL << 14) - 1) == 0
```

But if we use old ppn check code, It just so happens that his check
will fail:

```
   0x80020 & 0x3fff != 0
```

Thanks,
Chao
>  #define QRIOMMU_G_L0_PTE_VAL        0x0000000000021000ull
>  #define QRIOMMU_G_L1_PTE_VAL        0x0000000000022000ull
>  
> -- 
> 2.43.0
>

Re: [PATCH] hw/riscv/riscv-iommu.c: check for misaligned IOHGATP_PPN

Posted by Nutty.Liu 3 weeks, 4 days ago

On 7/1/2026 4:31 AM, Daniel Henrique Barboza wrote:
> We must check if IOHGATP_PPN is 16kb aligned for non-bare GATP modes.
>
> qos-riscv-iommu.h needs change too since G_IOHGATP must now be 16kb
> aligned too.
>
> Fixes: 69a9ae4836 ("hw/riscv/riscv-iommu: add ATS support")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3550
> Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> ---
>   hw/riscv/riscv-iommu.c               | 14 ++++++++++++++
>   tests/qtest/libqos/qos-riscv-iommu.h |  2 +-
>   2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index 891a56e731..09990c640f 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -795,6 +795,20 @@ static bool riscv_iommu_validate_device_ctx(RISCVIOMMUState *s,
>           return false;
>       }
>   
> +    if (gatp != RISCV_IOMMU_DC_IOHGATP_MODE_BARE) {
> +        uint64_t iohgatp_ppn = get_field(ctx->gatp,
> +                                         RISCV_IOMMU_DC_IOHGATP_PPN);
Seems it should be like the following:

+        uint64_t iohgatp_ppn = PPN_PHYS(get_field(ctx->gatp,
+                                        RISCV_IOMMU_DC_IOHGATP_PPN));

Otherwise,
Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>

Thanks,
Nutty
> +        /*
> +         * One of the conditions for a misconfigured DDT entry
> +         * according to the riscv-spec: "DC.iohgatp.MODE is not
> +         * Bare and the root page table determined by DC.iohgatp.PPN
> +         * is not aligned to a 16-KiB boundary."
> +         */
> +        if (iohgatp_ppn & ((1ULL << 14) - 1)) {
> +            return false;
> +        }
> +    }
> +
>       fsc_mode = get_field(ctx->satp, RISCV_IOMMU_DC_FSC_MODE);
>   
>       if (ctx->tc & RISCV_IOMMU_DC_TC_PDTV) {
> diff --git a/tests/qtest/libqos/qos-riscv-iommu.h b/tests/qtest/libqos/qos-riscv-iommu.h
> index 90e69a5d73..4a972401f5 100644
> --- a/tests/qtest/libqos/qos-riscv-iommu.h
> +++ b/tests/qtest/libqos/qos-riscv-iommu.h
> @@ -46,7 +46,7 @@
>   #define QRIOMMU_L1_PTE_VAL          0x0000000000012000ull
>   #define QRIOMMU_L2_PTE_VAL          0x0000000000013000ull
>   
> -#define QRIOMMU_G_IOHGATP           0x0000000000020000ull
> +#define QRIOMMU_G_IOHGATP           0x0000000008000000ull
>   #define QRIOMMU_G_L0_PTE_VAL        0x0000000000021000ull
>   #define QRIOMMU_G_L1_PTE_VAL        0x0000000000022000ull
>

Re: [PATCH] hw/riscv/riscv-iommu.c: check for misaligned IOHGATP_PPN

Posted by Daniel Henrique Barboza 3 weeks, 2 days ago


On 7/1/2026 1:02 AM, Nutty.Liu wrote:
> 
> On 7/1/2026 4:31 AM, Daniel Henrique Barboza wrote:
>> We must check if IOHGATP_PPN is 16kb aligned for non-bare GATP modes.
>>
>> qos-riscv-iommu.h needs change too since G_IOHGATP must now be 16kb
>> aligned too.
>>
>> Fixes: 69a9ae4836 ("hw/riscv/riscv-iommu: add ATS support")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3550
>> Signed-off-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>> ---
>>   hw/riscv/riscv-iommu.c               | 14 ++++++++++++++
>>   tests/qtest/libqos/qos-riscv-iommu.h |  2 +-
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>> index 891a56e731..09990c640f 100644
>> --- a/hw/riscv/riscv-iommu.c
>> +++ b/hw/riscv/riscv-iommu.c
>> @@ -795,6 +795,20 @@ static bool riscv_iommu_validate_device_ctx(RISCVIOMMUState *s,
>>           return false;
>>       }
>> +    if (gatp != RISCV_IOMMU_DC_IOHGATP_MODE_BARE) {
>> +        uint64_t iohgatp_ppn = get_field(ctx->gatp,
>> +                                         RISCV_IOMMU_DC_IOHGATP_PPN);
> Seems it should be like the following:
> 
> +        uint64_t iohgatp_ppn = PPN_PHYS(get_field(ctx->gatp,
> +                                        RISCV_IOMMU_DC_IOHGATP_PPN));

Yes, I should.  No wonder I need to change the tests to make it work ... But
for some reason the bug reproducer was working.  Go figure ..


With this correction we're able to both fix the bug and keep the tests
untouched, meaning we're doing the right thing.

Thanks for both yours and Chao's comment in his reply.  I'll send a v2.


Daniel


> 
> Otherwise,
> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
> 
> Thanks,
> Nutty
>> +        /*
>> +         * One of the conditions for a misconfigured DDT entry
>> +         * according to the riscv-spec: "DC.iohgatp.MODE is not
>> +         * Bare and the root page table determined by DC.iohgatp.PPN
>> +         * is not aligned to a 16-KiB boundary."
>> +         */
>> +        if (iohgatp_ppn & ((1ULL << 14) - 1)) {
>> +            return false;
>> +        }
>> +    }
>> +
>>       fsc_mode = get_field(ctx->satp, RISCV_IOMMU_DC_FSC_MODE);
>>       if (ctx->tc & RISCV_IOMMU_DC_TC_PDTV) {
>> diff --git a/tests/qtest/libqos/qos-riscv-iommu.h b/tests/qtest/libqos/qos-riscv-iommu.h
>> index 90e69a5d73..4a972401f5 100644
>> --- a/tests/qtest/libqos/qos-riscv-iommu.h
>> +++ b/tests/qtest/libqos/qos-riscv-iommu.h
>> @@ -46,7 +46,7 @@
>>   #define QRIOMMU_L1_PTE_VAL          0x0000000000012000ull
>>   #define QRIOMMU_L2_PTE_VAL          0x0000000000013000ull
>> -#define QRIOMMU_G_IOHGATP           0x0000000000020000ull
>> +#define QRIOMMU_G_IOHGATP           0x0000000008000000ull
>>   #define QRIOMMU_G_L0_PTE_VAL        0x0000000000021000ull
>>   #define QRIOMMU_G_L1_PTE_VAL        0x0000000000022000ull