Re: [PATCH 1/2] iommu: Store either domain or handle in group->pasid_array

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

On 2025/1/20 23:55, Jason Gunthorpe wrote:
> On Sun, Jan 19, 2025 at 07:08:39PM -0800, Yi Liu wrote:
>> iommu_attach_device_pasid() only stores handle to group->pasid_array
>> when there is a valid handle input. However, it makes the
>> iommu_attach_device_pasid() unable to detect if the pasid has been
>> attached or not. To be complete, let the iommu_attach_device_pasid()
>> store the domain to group->pasid_array if no valid handle.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommu.c | 23 +++++++++++++++++------
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 28ffd836592b..278c4eb8f225 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -45,6 +45,8 @@ static unsigned int iommu_def_domain_type __read_mostly;
>>   static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
>>   static u32 iommu_cmd_line __read_mostly;
>>   
>> +enum { IOMMU_PASID_ARRAY_DOMAIN, IOMMU_PASID_ARRAY_HANDLE };
> 
> Since xarray says:
> 
>   * @tag: Tag value (0, 1 or 3).
> 
> I would initialize those enums and add a small comment

yes.

> /* tags used with xa_tag_pointer() */
> enum { IOMMU_PASID_ARRAY_DOMAIN = 0, IOMMU_PASID_ARRAY_HANDLE = 1 };
> 
>> @@ -3397,10 +3400,14 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		}
>>   	}
>>   
>> -	if (handle)
>> +	if (handle) {
>>   		handle->domain = domain;
>> +		xa_entry = xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
>> +	} else {
>> +		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
>> +	}
> 
> What about iommu_attach_group_handle() ?
> 
> 	mutex_lock(&group->mutex);
> 	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> 	if (ret)
> 
> And iommu_replace_group_handle() ?
> 
> 	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> 
> I think it should be consistent and it should call xa_tag_poiner() too
> (even though it implicitly always is since the domain is stored in the
> group)??

yes, it needs to be fixed as well. I asked a help from Baolu in prior
email, but I can enlarge this series to cover both RID and PASID path. :)

-- 
Regards,
Yi Liu