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