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

[Gong Shuai]

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.

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