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

["Nutty Liu" <liujingqi@lanxincomputing.com>]

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