Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu <yi.l.liu@intel.com>]

On 2025/2/27 06:16, Nicolin Chen wrote:
> On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
>> The existing iommu_attach_device_pasid() function allows both a valid
>> handle and a NULL handle, which is not consistent with the RID path where
>> iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
>> it, this adds iommu_attach_device_pasid_handle() to cover the case with
>> valid handle, while let the iommu_attach_device_pasid() only deals with
>> the case with NULL handle.
> 
> Hmm, I am not very sure about the necessity of this change. The
> underlying function being called from those two helpers (with/
> without handle) is still taking a NULL handle, which looks very
> straightforward to me already, so this extra layer feels a bit
> redundant..

I think I should have added a if (!handle) check in the beginning of
iommu_attach_device_pasid_handle() just like the other _handle() APIs.
If so, this should be clearer. is it?

> 
>> @@ -130,8 +131,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>>   		goto out_free_handle;
>>   	}
>>   
>> -	ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid,
>> -					&handle->handle);
> [..]
>> +	ret = iommu_attach_device_pasid_handle(domain, dev,
>> +					       iommu_mm->pasid,
> 
> These two could fit in one line.

got it.

> 
>>   	if (ret)
>>   		goto out_free_domain;
>>   	domain->users = 1;
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 73a3b20b2ef9..f6dbb60ef948 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3354,9 +3354,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>>    *
>>    * Return: 0 on success, or an error.
>>    */
>> -int iommu_attach_device_pasid(struct iommu_domain *domain,
>> -			      struct device *dev, ioasid_t pasid,
>> -			      struct iommu_attach_handle *handle)
>> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
>> +				struct device *dev, ioasid_t pasid,
>> +				struct iommu_attach_handle *handle)
> 
> Should it be just iommu_attach_device_pasid_handle?
> 
>> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
>> +					    struct device *dev, ioasid_t pasid)
>> +{
>> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
> 
> Then, here:
> 	return iommu_attach_device_pasid_handle(domain, dev, pasid, NULL);
> ?
> 
>> +static inline int
>> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
>> +				 struct device *dev, ioasid_t pasid,
>> +				 struct iommu_attach_handle *handle)
>> +{
>> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
> 
> And drop this?

this needs to check the handle before calling __iommu_attach_device_pasid().

> 
>> @@ -1139,6 +1154,8 @@ struct iommu_fault_param {};
>>   struct iommu_iotlb_gather {};
>>   struct iommu_dirty_bitmap {};
>>   struct iommu_dirty_ops {};
>> +struct iommu_attach_handle {};
>> +
> 
> Nit: no need of the extra line
> 
got it.

-- 
Regards,
Yi Liu