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

[Tina Zhang <tina.zhang@intel.com>]

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