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

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

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