Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Thu, 21 Sep 2023 16:02:59 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Tina Zhang <tina.zhang@intel.com>
Cc:     Kevin Tian <kevin.tian@intel.com>,
        Lu Baolu <baolu.lu@linux.intel.com>,
        Michael Shavit <mshavit@google.com>,
        Vasant Hegde <vasant.hegde@amd.com>, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/6] iommu: Introduce mm_get_pasid() helper function
Message-ID: <20230921190259.GZ13795@ziepe.ca>
References: <20230912125936.722348-1-tina.zhang@intel.com>
 <20230912125936.722348-4-tina.zhang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230912125936.722348-4-tina.zhang@intel.com>
Precedence: bulk

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@linux.intel.com>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
> 
> 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.

> 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