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

[Gong Shuai <gsh517025@gmail.com>]

Hi Fangyu,

On 9/20/2026 11:12 AM, fangyu.yu@linux.alibaba.com wrote:
>>Hi Fangyu,
>>
>>On 9/15/2026 11:28 AM, fangyu.yu@linux.alibaba.com wrote:
>>> From: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>>>
>>> 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@linux.alibaba.com>
>>> ---
>>>   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.

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

> 
> Thanks,
> Fangyu
> 
>>Thanks,
>>Shuai
>>
>>>   	} else {
>>>   		if (!riscv_iommu_fsc_supported(iommu, pt_info.fsc_iosatp_mode))
>>>   			return -ENODEV;