Re: [PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register
On 6/4/2025 1:52 AM, Tomasz Jeznach wrote:
> On Thu, May 29, 2025 at 2:14 AM Nutty Liu
> 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
>> ---
>> 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
>
> Best,
> - Tomasz
Thanks,
Nutty
Re: [PATCH] hw/riscv/riscv-iommu: Fix PPN field of Translation-reponse register
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
> 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
>> ---
>> 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 0x3ffc00.
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 & 0x3ffc00;
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) & 0x3ffc00 = 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
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
> 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
>> ---
>> 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 & 0x3ffc00;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) & 0x3ffc00 = 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
On Thu, May 29, 2025 at 10:52 PM Nutty Liu
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
> ---
> 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
>
