Re: [RFC PATCH v4 08/10] iommu/riscv: Pre-enable GADE for second-stage domains

[fangyu . yu]

>Hi Fangyu,
>
>On 9/20/2026 11:12 AM, fangyu.yu@xxxxxxxxxxxxxxxxx wrote:
>>>Hi Fangyu,
>>>
>>>On 9/15/2026 11:28 AM, fangyu.yu@xxxxxxxxxxxxxxxxx wrote:
>>>> From: Fangyu Yu <fangyu.yu@xxxxxxxxxxxxxxxxx>
>>>>
>>>> Pre-enable RISCV_IOMMU_DC_TC_GADE in the device context when
>>>> attaching a second-stage domain, if the IOMMU supports AMO_HWAD.
>>>>
>>>> Software pre-populates second-stage page tables with D set, so
>>>> enabling GADE by default does not change normal behavior. When
>>>> dirty tracking is enabled, iommufd clears the pre-set D bits and
>>>> GADE becomes necessary for hardware to update the dirty bit on
>>>> write access.
>>>>
>>>> This avoids toggling GADE dynamically and keeps device context
>>>> setup consistent with second-stage domain attachment.
>>>>
>>>> Signed-off-by: Fangyu Yu <fangyu.yu@xxxxxxxxxxxxxxxxx>
>>>> ---
>>>>   drivers/iommu/riscv/iommu.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
>>>> index 16779877351b..f01fd4e2f840 100644
>>>> --- a/drivers/iommu/riscv/iommu.c
>>>> +++ b/drivers/iommu/riscv/iommu.c
>>>> @@ -1330,6 +1330,8 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
>>>>   			FIELD_PREP(RISCV_IOMMU_DC_IOHGATP_MODE, pt_info.iohgatp_mode) |
>>>>   			FIELD_PREP(RISCV_IOMMU_DC_IOHGATP_GSCID, domain->gscid) |
>>>>   			FIELD_PREP(RISCV_IOMMU_DC_IOHGATP_PPN, pt_info.ppn);
>>>> +		if (iommu->caps & RISCV_IOMMU_CAPABILITIES_AMO_HWAD)
>>>> +			dc.tc |= RISCV_IOMMU_DC_TC_GADE;
>>
>> Hi Shuai:
>>
>>>
>>>I might be missing something, but I don't see how the GADE bit set
>>>here reaches the hardware IOMMU DC. In riscv_iommu_iodir_update()
>>>(in patch 04/10), it looks like only the V bit is taken:
>>>
>>>	tc = READ_ONCE(dc->tc);
>>>	tc |= new_dc->ta & RISCV_IOMMU_DC_TC_V;
>>>	...
>>>	WRITE_ONCE(dc->tc, tc);
>>>
>>
>> You’re right — this is a real issue.
>>
>> GADE was being set in the software-side dc, but it was not
>> propagated into the hardware DC in riscv_iommu_iodir_update().
>>
>> I have already fixed this in my local tree, and I’ll include the
>> change in the next version of the patchset:
>>      tc &= ~RISCV_IOMMU_DC_TC_GADE;
>>      tc |= new_dc->tc & RISCV_IOMMU_DC_TC_GADE;
>>      tc |= new_dc->ta & RISCV_IOMMU_DC_TC_V;
>>
>
>One thought here, instead of updating individual TC bits one by one
>in riscv_iommu_iodir_update(), could we transfer all software-configurable
>bits from the incoming DC to the hardware DC in one go?
>
>Something like:
>	...
>	tc = (READ_ONCE(dc->tc) & ~GENMASK_ULL(11, 0)) |
>		 (new_dc->tc & GENMASK_ULL(11, 0));
>	...
>	WRITE_ONCE(dc->tc, tc);
>
>I’m not sure whether this could have negative side effects.
>

Hi Shuai:

That is a reasonable idea in principle.

However, TC bits may have different lifetimes and
update rules, and several features such as ATS and
PRI are still not implemented yet.

For now, I would prefer to keep the change minimal
and only handle GADE here.

Once the remaining features are in place, we can
consider a broader cleanup and unify the handling
of these bits.

Thanks,
Fangyu

>>>I saw that you mentioned you tested it on QEMU. As far as I know,
>>>mainline QEMU has not yet declared the AMO_HWAD capability. Could
>>>you clarify whether you used a modified QEMU for this test?
>>>
>>
>> I used mainline QEMU, not a modified one.
>>
>> In QEMU, I only tested the second-stage path with a
>> VM using device passthrough.
>>
>> I did not test the dirty-log related part there,
>> so that path cannot be covered there.
>>
>> That is why I noted in the TODO that I will build a
>> dedicated test case for dirty log handling later.
>
>Sorry, I missed that TODO.
>
>
>Thanks,
>Shuai
>