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
> <liujin...@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 <liujin...@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
>>
