Re: [RFC PATCH v2 03/18] iommu/riscv: Use data structure instead of individual values

From: Andrew Jones <ajones@ventanamicro.com>
To: "Nutty.Liu" <nutty.liu@hotmail.com>
Cc: iommu@lists.linux.dev, kvm-riscv@lists.infradead.org,
	 kvm@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org,  jgg@nvidia.com,
	zong.li@sifive.com, tjeznach@rivosinc.com, joro@8bytes.org,
	 will@kernel.org, robin.murphy@arm.com, anup@brainfault.org,
	atish.patra@linux.dev,  tglx@linutronix.de,
	alex.williamson@redhat.com, paul.walmsley@sifive.com,
	 palmer@dabbelt.com, alex@ghiti.fr
Subject: Re: [RFC PATCH v2 03/18] iommu/riscv: Use data structure instead of individual values
Date: Wed, 24 Sep 2025 08:31:50 -0500	[thread overview]
Message-ID: <20250924-01f9a5207f8865555c839abd@orel> (raw)
In-Reply-To: <TY1PPFCDFFFA68A794163FFB7BFAAAC22BEF31CA@TY1PPFCDFFFA68A.apcprd02.prod.outlook.com>

On Wed, Sep 24, 2025 at 11:25:59AM +0800, Nutty.Liu wrote:
> On 9/21/2025 4:38 AM, Andrew Jones wrote:
> > From: Zong Li <zong.li@sifive.com>
> > 
> > The parameter will be increased when we need to set up more fields
> > in the device context. Use a data structure to wrap them up.
> > 
> > Signed-off-by: Zong Li <zong.li@sifive.com>
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >   drivers/iommu/riscv/iommu.c | 31 +++++++++++++++++++------------
> >   1 file changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> > index 901d02529a26..a44c67a848fa 100644
> > --- a/drivers/iommu/riscv/iommu.c
> > +++ b/drivers/iommu/riscv/iommu.c
> > @@ -988,7 +988,7 @@ static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
> >    * interim translation faults.
> >    */
> >   static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
> > -				     struct device *dev, u64 fsc, u64 ta)
> > +				     struct device *dev, struct riscv_iommu_dc *new_dc)
> >   {
> >   	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >   	struct riscv_iommu_dc *dc;
> > @@ -1022,10 +1022,10 @@ static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
> >   	for (i = 0; i < fwspec->num_ids; i++) {
> >   		dc = riscv_iommu_get_dc(iommu, fwspec->ids[i]);
> >   		tc = READ_ONCE(dc->tc);
> > -		tc |= ta & RISCV_IOMMU_DC_TC_V;
> > +		tc |= new_dc->ta & RISCV_IOMMU_DC_TC_V;
> > -		WRITE_ONCE(dc->fsc, fsc);
> > -		WRITE_ONCE(dc->ta, ta & RISCV_IOMMU_PC_TA_PSCID);
> > +		WRITE_ONCE(dc->fsc, new_dc->fsc);
> > +		WRITE_ONCE(dc->ta, new_dc->ta & RISCV_IOMMU_PC_TA_PSCID);
> Seems it will override all other fields in 'TA' except for the field of
> 'PSCID'.
> Should the other fields remain unchanged ?

The short answer is that the current implementation is doing the right
thing. The long answer is that riscv_iommu_iodir_update() and how it's
called from riscv_iommu_attach_paging_domain() could use some cleanup.

A more logical interface would be that new_dc would be completely written,
which means any fields left zero when creating new_dc will result in zeros
being written -- it doesn't do that right now. Also, rather than passing
DC_TC_V through new_dc->ta (as PC_TA_V, even though DC_TC_PDTV = 0), we
should probably just set it directly in new_dc->tc.

We can clean this up separately though, probably as work for adding SVA
support.

> Otherwise,
> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>

Thanks,
drew

> 
> Thanks,
> Nutty
> >   		/* Update device context, write TC.V as the last step. */
> >   		dma_wmb();
> >   		WRITE_ONCE(dc->tc, tc);
> > @@ -1304,20 +1304,20 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
> >   	struct riscv_iommu_domain *domain = iommu_domain_to_riscv(iommu_domain);
> >   	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
> >   	struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> > -	u64 fsc, ta;
> > +	struct riscv_iommu_dc dc = {0};
> >   	if (!riscv_iommu_pt_supported(iommu, domain->pgd_mode))
> >   		return -ENODEV;
> > -	fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, domain->pgd_mode) |
> > -	      FIELD_PREP(RISCV_IOMMU_PC_FSC_PPN, virt_to_pfn(domain->pgd_root));
> > -	ta = FIELD_PREP(RISCV_IOMMU_PC_TA_PSCID, domain->pscid) |
> > -	     RISCV_IOMMU_PC_TA_V;
> > +	dc.fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, domain->pgd_mode) |
> > +		 FIELD_PREP(RISCV_IOMMU_PC_FSC_PPN, virt_to_pfn(domain->pgd_root));
> > +	dc.ta = FIELD_PREP(RISCV_IOMMU_PC_TA_PSCID, domain->pscid) |
> > +			   RISCV_IOMMU_PC_TA_V;
> >   	if (riscv_iommu_bond_link(domain, dev))
> >   		return -ENOMEM;
> > -	riscv_iommu_iodir_update(iommu, dev, fsc, ta);
> > +	riscv_iommu_iodir_update(iommu, dev, &dc);
> >   	riscv_iommu_bond_unlink(info->domain, dev);
> >   	info->domain = domain;
> > @@ -1408,9 +1408,12 @@ static int riscv_iommu_attach_blocking_domain(struct iommu_domain *iommu_domain,
> >   {
> >   	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
> >   	struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> > +	struct riscv_iommu_dc dc = {0};
> > +
> > +	dc.fsc = RISCV_IOMMU_FSC_BARE;
> >   	/* Make device context invalid, translation requests will fault w/ #258 */
> > -	riscv_iommu_iodir_update(iommu, dev, RISCV_IOMMU_FSC_BARE, 0);
> > +	riscv_iommu_iodir_update(iommu, dev, &dc);
> >   	riscv_iommu_bond_unlink(info->domain, dev);
> >   	info->domain = NULL;
> > @@ -1429,8 +1432,12 @@ static int riscv_iommu_attach_identity_domain(struct iommu_domain *iommu_domain,
> >   {
> >   	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
> >   	struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> > +	struct riscv_iommu_dc dc = {0};
> > +
> > +	dc.fsc = RISCV_IOMMU_FSC_BARE;
> > +	dc.ta = RISCV_IOMMU_PC_TA_V;
> > -	riscv_iommu_iodir_update(iommu, dev, RISCV_IOMMU_FSC_BARE, RISCV_IOMMU_PC_TA_V);
> > +	riscv_iommu_iodir_update(iommu, dev, &dc);
> >   	riscv_iommu_bond_unlink(info->domain, dev);
> >   	info->domain = NULL;

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv