Re: [PATCH v7 02/14] iommu/arm-smmu-v3: Start building a generic PASID layer

From: Nicolin Chen <nicolinc@nvidia.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: <iommu@lists.linux.dev>, Joerg Roedel <joro@8bytes.org>,
	<linux-arm-kernel@lists.infradead.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Eric Auger <eric.auger@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Moritz Fischer <mdf@kernel.org>,
	Michael Shavit <mshavit@google.com>, <patches@lists.linux.dev>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v7 02/14] iommu/arm-smmu-v3: Start building a generic PASID layer
Date: Sun, 12 May 2024 13:54:10 -0700	[thread overview]
Message-ID: <ZkEscmhh4I6PeP/S@nvidia.com> (raw)
In-Reply-To: <ZkDAL+TX93QfTFMc@nvidia.com>

On Sun, May 12, 2024 at 10:12:15AM -0300, Jason Gunthorpe wrote:
> On Fri, May 10, 2024 at 07:32:16PM -0700, Nicolin Chen wrote:
> > On Wed, May 08, 2024 at 03:57:10PM -0300, Jason Gunthorpe wrote:
> > > @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
> > >  	struct arm_smmu_bond *bond = NULL, *t;
> > >  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > >  
> > > +	arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
> > > +
> > >  	mutex_lock(&sva_lock);
> > > -
> > > -	arm_smmu_clear_cd(master, id);
> > > -
> > 
> > Should the new arm_smmu_remove_pasid() be inside the sva_lock as
> > the arm_smmu_clear_cd() previously? This would also seem to match
> > with the arm_smmu_set_pasid() that's inside the sva_lock too.
> 
> Not needed, the sva_lock primarily protects the bond/etc stuff. The CD
> is locked by the group mutex. By the end of the series the sva_lock is
> just protecting the set_feature stuff, none of the attach flow.

I see. That lock in the end product does look cleaner.

> > > +int arm_smmu_set_pasid(struct arm_smmu_master *master,
> > > +		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
> > > +		       const struct arm_smmu_cd *cd)
> > > +{
> > > +	struct arm_smmu_cd *cdptr;
> > > +
> > > +	/* The core code validates pasid */
> > > +
> > > +	if (!master->cd_table.in_ste)
> > > +		return -ENODEV;
> > > +
> > > +	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);
> > > +	if (!cdptr)
> > > +		return -ENOMEM;
> > 
> > Though I might be missing some piece, the in_ste is set in the
> > arm_smmu_install_ste_for_dev() when STE.Config=S1_TRANS, in which
> > case cdptr is already allocated? 
> 
> This only means that at least the first level of a two level (or
> entire linear) table is allocated.
> 
> alloc_cd_ptr() is sill needed to allocate any second level. For
> instance if the PASID is > 1024 then we will need a new second level
> page as only SSID=0 will have been allocated.

Oh, I had misread that v.s. arm_smmu_alloc_cd_tables, while a
cdptr here is a CD entry.

Thanks
Nicolin

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