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

[Jason Gunthorpe]

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.

> With that, arm_smmu_remove_pasid() seems to be removed entirely
> by a following change, though its function declare still exists
> in arm-smmu-v3.h (I probably should find what following patch and
> comment there..)

Woops, yep, I missed that.

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

When we get further along in the series the above changes to:

	if (!master->cd_table.in_ste &&
	    sid_domain->type != IOMMU_DOMAIN_IDENTITY &&
	    sid_domain->type != IOMMU_DOMAIN_BLOCKED)
		return -EINVAL;

	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);

In those additional cases the entire allocation can be needed.

Jason