Re: [PATCH v4 3/6] iommu: Introduce mm_get_pasid() helper function

[Tina Zhang]

On 9/22/23 03:02, Jason Gunthorpe wrote:
> On Tue, Sep 12, 2023 at 08:59:33PM +0800, Tina Zhang wrote:
> > Use the helper function mm_get_pasid() to get a mm assigned pasid
> > value. The motivation is to replace mm->pasid with an iommu private
> > data structure that is introduced in a later patch.
> >
> > Reviewed-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Tina Zhang <tina.zhang@xxxxxxxxx>
> > ---
> >
> > Changes in v4:
> > - Rebase to v6.6-rc1.
> >
> > Changes in v2:
> > - Update commit message.
> > - Let mm_get_enqcmd_pasid() call mm_get_pasid() to get pasid.
> >
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 ++++++------
> >   drivers/iommu/iommu-sva.c                       | 12 ++++++------
> >   include/linux/iommu.h                           | 10 +++++++++-
> >   3 files changed, 21 insertions(+), 13 deletions(-)
>
> Michael already has a series to remove this mis-use from the ARM
> driver like you did for the Intel driver.
>
> I think for the interm it is fine to just use mm_get_enqcmd_pasid()
> and Michael can remove it.
>
> It helps point out this is just wrong anyhow and better discourages
> anyone else from using it.
Sure, I'll update the commit message.

Thanks,
-Tina
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index b78671a8a914..0f956ecd0c9b 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -23,7 +23,7 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
> >   	mutex_lock(&iommu_sva_lock);
> >   	/* Is a PASID already associated with this mm? */
> >   	if (mm_valid_pasid(mm)) {
> > -		if (mm->pasid >= dev->iommu->max_pasids)
> > +		if (mm_get_pasid(mm) >= dev->iommu->max_pasids)
> >   			ret = -EOVERFLOW;
> >
> >   		goto out;
> >   	}
> >
> > @@ -73,7 +73,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> >   
> >   	mutex_lock(&iommu_sva_lock);
> >   	/* Search for an existing domain. */
> > -	domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
> > +	domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm),
> >   						IOMMU_DOMAIN_SVA);
> >   	if (IS_ERR(domain)) {
> >   		ret = PTR_ERR(domain);
> > @@ -92,7 +92,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> >   		goto out_unlock;
> >   	}
> >   
> > -	ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
> > +	ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
> >   	if (ret)
> >   		goto out_free_domain;
> >   	domain->users = 1;
>
> These ones are all manipulating the enqcmd_pasid really..  If it
> wasn't for enqcmd then we'd simply make each device have its own PASID
> and then we wouldn't have the issue that a valid pasid is incompatible
> with a certain device.
>
> Jason