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

[Nicolin Chen <nicolinc@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