[PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register

[PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register

[Nutty Liu]

The original implementation incorrectly performed a bitwise AND
operation between the PPN of iova and PPN Mask, leading to an
incorrect PPN field in Translation-reponse register.

The PPN of iova should be set entirely in the PPN field of
Translation-reponse register.

Signed-off-by: Nutty Liu <liujingqi@lanxincomputing.com>
---
 hw/riscv/riscv-iommu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index a877e5da84..f529a6a3d7 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -1935,8 +1935,7 @@ static void riscv_iommu_process_dbg(RISCVIOMMUState *s)
             iova = RISCV_IOMMU_TR_RESPONSE_FAULT | (((uint64_t) fault) << 10);
         } else {
             iova = iotlb.translated_addr & ~iotlb.addr_mask;
-            iova >>= TARGET_PAGE_BITS;
-            iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
+            iova = set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));
 
             /* We do not support superpages (> 4kbs) for now */
             iova &= ~RISCV_IOMMU_TR_RESPONSE_S;
-- 
2.49.0.windows.1

Re: [PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register

[Alistair Francis]

On Thu, May 29, 2025 at 10:52 PM Nutty Liu
<liujingqi@lanxincomputing.com> wrote:
>
> The original implementation incorrectly performed a bitwise AND
> operation between the PPN of iova and PPN Mask, leading to an
> incorrect PPN field in Translation-reponse register.
>
> The PPN of iova should be set entirely in the PPN field of
> Translation-reponse register.
>
> Signed-off-by: Nutty Liu <liujingqi@lanxincomputing.com>
> ---
>  hw/riscv/riscv-iommu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index a877e5da84..f529a6a3d7 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -1935,8 +1935,7 @@ static void riscv_iommu_process_dbg(RISCVIOMMUState *s)
>              iova = RISCV_IOMMU_TR_RESPONSE_FAULT | (((uint64_t) fault) << 10);
>          } else {
>              iova = iotlb.translated_addr & ~iotlb.addr_mask;
> -            iova >>= TARGET_PAGE_BITS;
> -            iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
> +            iova = set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));

I don't see how this is different.

PPN_DOWN(iova)
is the same as
iova >> TARGET_PAGE_BITS

and

set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova))
should just return
(0 & ~RISCV_IOMMU_TR_RESPONSE_PPN) |
    (RISCV_IOMMU_TR_RESPONSE_PPN & PPN_DOWN(iova))

Can you describe the issue with the original implementation and why
this fixes it in the commit message?

Alistair

>
>              /* We do not support superpages (> 4kbs) for now */
>              iova &= ~RISCV_IOMMU_TR_RESPONSE_S;
> --
> 2.49.0.windows.1
>

Re: [PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register

[liu]

Hi Alistair,

Thanks for your review.

On 6/2/2025 1:11 PM, Alistair Francis wrote:
> On Thu, May 29, 2025 at 10:52 PM Nutty Liu
> <liujingqi@lanxincomputing.com> wrote:
>> The original implementation incorrectly performed a bitwise AND
>> operation between the PPN of iova and PPN Mask, leading to an
>> incorrect PPN field in Translation-reponse register.
>>
>> The PPN of iova should be set entirely in the PPN field of
>> Translation-reponse register.
>>
>> Signed-off-by: Nutty Liu <liujingqi@lanxincomputing.com>
>> ---
>>   hw/riscv/riscv-iommu.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>> index a877e5da84..f529a6a3d7 100644
>> --- a/hw/riscv/riscv-iommu.c
>> +++ b/hw/riscv/riscv-iommu.c
>> @@ -1935,8 +1935,7 @@ static void riscv_iommu_process_dbg(RISCVIOMMUState *s)
>>               iova = RISCV_IOMMU_TR_RESPONSE_FAULT | (((uint64_t) fault) << 10);
>>           } else {
>>               iova = iotlb.translated_addr & ~iotlb.addr_mask;
>> -            iova >>= TARGET_PAGE_BITS;
>> -            iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
>> +            iova = set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));
> I don't see how this is different.
>
> PPN_DOWN(iova)
> is the same as
> iova >> TARGET_PAGE_BITS
Yes. The results of above operations are the same.
>
> and
>
> set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova))
> should just return
> (0 & ~RISCV_IOMMU_TR_RESPONSE_PPN) |
>      (RISCV_IOMMU_TR_RESPONSE_PPN & PPN_DOWN(iova))
It's not correct.Let me show the definitions of the related macros as 
follows.
1) The definition of 'set_field' in 'target/riscv/cpu_bits.h' is as 
below:#define set_field(reg, mask, val) (((reg) & ~(uint64_t)(mask)) | 
\(((uint64_t)(val) * ((mask) & ~((mask) << 1))) & \(uint64_t)(mask)))2) 
The definition of 'RISCV_IOMMU_TR_RESPONSE_PPN' 
in'hw/riscv/riscv-iommu-bits.h' is as follows:#define 
RISCV_IOMMU_PPN_FIELD GENMASK_ULL(53, 10)#define 
RISCV_IOMMU_TR_RESPONSE_PPN RISCV_IOMMU_PPN_FIELD
Let me give you an example as follows.
With the original implementation,1) Assume:iova = 0x12345678;
2) iova >>= TARGET_PAGE_BITS；The result is as follows:iova = 
0x12345;It's the same as PPN_DOWN(iova). This is the correct value of PPN.
3) iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
After substituting with 'iova = 0x12345', the result is as follows:iova 
= 0x12345 & 0x3ffffffffffc00;The final 'iova' is :iova = 0x12000;
This 'iova' will be set to Translation-reponse register finally by 
thebelow sentence.riscv_iommu_reg_set64(s, RISCV_IOMMU_REG_TR_RESPONSE, 
iova);
Then the value of Translation-reponse register will be set to 
'0x12000'.The PPN field is between bit 10 and bit 53. So the PPN is 
0x48(calculated by 0x12000 >> 10 = 0x48).
It's incorrect. The PPN field should be 0x12345.It should be calculated 
as follows:iova = (iova << 10) & RISCV_IOMMU_TR_RESPONSE_PPN;
After substituting with 'iova = 0x12345', and the result is as 
follows:iova = (0x12345 << 10) & 0x3ffffffffffc00 = 0x48D1400The correct 
value of Translation-reponse register should be '0x48D1400'.
4) With the new implementation,iova = set_field(0, 
RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));
It will set the bits 10 to 53 of 'iova' to PPN_DOWN(iova).After 
substituting with 'iova = 0x12345', the final 'iova' is as follows:iova 
= 0x48D1400;
And then the correct 'iova' will be set to Translation-reponse register.
>
> Can you describe the issue with the original implementation and why
> this fixes it in the commit message?
Hope the above description and the example can help you understand.
Nutty,
Thanks
>
> Alistair
>
>>               /* We do not support superpages (> 4kbs) for now */
>>               iova &= ~RISCV_IOMMU_TR_RESPONSE_S;
>> --
>> 2.49.0.windows.1
>>

Re: [PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register

[Nutty Liu]

Hi Alistair,

Thanks for your review.

(Sorry, just resend this email due to the incorrect line break in the
previous reply email.)

On 6/2/2025 1:11 PM, Alistair Francis wrote:
> On Thu, May 29, 2025 at 10:52 PM Nutty Liu
> <liujingqi@lanxincomputing.com> wrote:
>> The original implementation incorrectly performed a bitwise AND
>> operation between the PPN of iova and PPN Mask, leading to an
>> incorrect PPN field in Translation-reponse register.
>>
>> The PPN of iova should be set entirely in the PPN field of
>> Translation-reponse register.
>>
>> Signed-off-by: Nutty Liu <liujingqi@lanxincomputing.com>
>> ---
>>   hw/riscv/riscv-iommu.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>> index a877e5da84..f529a6a3d7 100644
>> --- a/hw/riscv/riscv-iommu.c
>> +++ b/hw/riscv/riscv-iommu.c
>> @@ -1935,8 +1935,7 @@ static void riscv_iommu_process_dbg(RISCVIOMMUState *s)
>>               iova = RISCV_IOMMU_TR_RESPONSE_FAULT | (((uint64_t) fault) << 10);
>>           } else {
>>               iova = iotlb.translated_addr & ~iotlb.addr_mask;
>> -            iova >>= TARGET_PAGE_BITS;
>> -            iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
>> +            iova = set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));
> I don't see how this is different.
>
> PPN_DOWN(iova)
> is the same as
> iova >> TARGET_PAGE_BITS

Yes. The results of above operations are the same.

>
> and
>
> set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova))
> should just return
> (0 & ~RISCV_IOMMU_TR_RESPONSE_PPN) |
>      (RISCV_IOMMU_TR_RESPONSE_PPN & PPN_DOWN(iova))

It's not correct.
Let me show the definitions of the related macros as follows.

1) The definition of 'set_field' in 'target/riscv/cpu_bits.h' is as below:

     #define set_field(reg, mask, val) (((reg) & ~(uint64_t)(mask)) | \
                                       (((uint64_t)(val) * ((mask) & ~((mask) << 1))) & \
                                       (uint64_t)(mask)))

2) The definition of 'RISCV_IOMMU_TR_RESPONSE_PPN' in 'hw/riscv/riscv-iommu-bits.h' is as follows:

     #define RISCV_IOMMU_PPN_FIELD GENMASK_ULL(53, 10)
     #define RISCV_IOMMU_TR_RESPONSE_PPN RISCV_IOMMU_PPN_FIELD

So the 'RISCV_IOMMU_TR_RESPONSE_PPN' is 0x3ffffffffffc00.


Let me give you an example as follows.

With the original implementation,
1) Assume: iova = 0x12345678;

2) iova >>= TARGET_PAGE_BITS;
The result is as follows:
   iova = 0x12345;

It's the same as PPN_DOWN(iova).
This is the correct value of PPN.

3) iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
After substituting with 'iova = 0x12345', the result is as follows:
    iova = 0x12345 & 0x3ffffffffffc00;
The final 'iova' is as follows:
     iova = 0x12000;
This 'iova' will be set to Translation-reponse register finally by thebelow sentence.
    riscv_iommu_reg_set64(s, RISCV_IOMMU_REG_TR_RESPONSE, iova);

Then the value of Translation-reponse register will be set to '0x12000'.
The PPN field is between bit 10 and bit 53.
So the PPN is 0x48 which is calculated by '0x12000 >> 10 = 0x48'.

It's incorrect. Actually the PPN field should be 0x12345.

It should be calculated as follows:
     iova = (iova << 10) & RISCV_IOMMU_TR_RESPONSE_PPN;
After substituting with 'iova = 0x12345', and the result is as follows:
     iova = (0x12345 << 10) & 0x3ffffffffffc00 = 0x48D1400
The correct value of Translation-reponse register should be '0x48D1400'.

4) With the new implementation,
     iova = set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));

It will set the bit 10 to bit 53 of 'iova' to PPN_DOWN(iova).
After substituting with 'iova = 0x12345', the final 'iova' is as follows:
     iova = 0x48D1400;
And then the correct 'iova' will be set to Translation-reponse register.

> Can you describe the issue with the original implementation and why
> this fixes it in the commit message?

Hope the above description and the example can help you understand.

Nutty,
Thanks

>
> Alistair
>
>>               /* We do not support superpages (> 4kbs) for now */
>>               iova &= ~RISCV_IOMMU_TR_RESPONSE_S;
>> --
>> 2.49.0.windows.1
>>

Re: [PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register

[Nutty Liu]

On 6/4/2025 1:52 AM, Tomasz Jeznach wrote:
> On Thu, May 29, 2025 at 2:14 AM Nutty Liu <liujingqi@lanxincomputing.com> wrote:
>> The original implementation incorrectly performed a bitwise AND
>> operation between the PPN of iova and PPN Mask, leading to an
>> incorrect PPN field in Translation-reponse register.
>>
>> The PPN of iova should be set entirely in the PPN field of
>> Translation-reponse register.
>>
>> Signed-off-by: Nutty Liu <liujingqi@lanxincomputing.com>
>> ---
>>   hw/riscv/riscv-iommu.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>> index a877e5da84..f529a6a3d7 100644
>> --- a/hw/riscv/riscv-iommu.c
>> +++ b/hw/riscv/riscv-iommu.c
>> @@ -1935,8 +1935,7 @@ static void riscv_iommu_process_dbg(RISCVIOMMUState *s)
>>               iova = RISCV_IOMMU_TR_RESPONSE_FAULT | (((uint64_t) fault) << 10);
>>           } else {
>>               iova = iotlb.translated_addr & ~iotlb.addr_mask;
>> -            iova >>= TARGET_PAGE_BITS;
>> -            iova &= RISCV_IOMMU_TR_RESPONSE_PPN;
>> +            iova = set_field(0, RISCV_IOMMU_TR_RESPONSE_PPN, PPN_DOWN(iova));
>>
> Thanks for catching this. Yes, there is an issue here.
> Also, the line below, clearing _S bit, is no longer needed.

Thanks for your review.
Indeed. I will also remove the line below.

>>               /* We do not support superpages (> 4kbs) for now */
>>               iova &= ~RISCV_IOMMU_TR_RESPONSE_S;
>> --
>> 2.49.0.windows.1
> Reviewed-by: Tomasz Jeznach <tjeznach@rivosinc.com>
>
> Best,
> - Tomasz

Thanks,
Nutty