Re: [PATCH 2/2] iommu: Swap the order of setting group->pasid_array and __iommu_set_group_pasid()

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

On 2025/1/21 00:13, Jason Gunthorpe wrote:
> On Sun, Jan 19, 2025 at 07:08:40PM -0800, Yi Liu wrote:
>> Following the current order of setting group->pasid_array and
>> __iommu_set_group_pasid() in iommu_attach_device_pasid(), PRIs may be
>> forwarded to the domain before the attach succeeds. If the attach failed
>> in the end, the PRIs on the domain need to be flushed in the caller side.
>> Caller can do it, but it can be avoided by swapping the order. This is
>> more self-contained.
> 
> Can the caller do it? I thought only the driver could manage it?

I think yes. Take iommufd as example, if the hwpt is fault-able, the PRIs
would be forwarded to iommufd, and recorded in hwpt->fault->deliver list.
This cannot be done in driver alone, iommufd needs to flush it. Otherwise,
the PRIs would be kept until the hwpt is destroyed.

>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 278c4eb8f225..b69bcf559839 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3376,7 +3376,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   	struct iommu_group *group = dev->iommu_group;
>>   	struct group_device *device;
>>   	const struct iommu_ops *ops;
>> -	void *xa_entry;
>> +	void *curr, *xa_entry;
>>   	int ret;
>>   
>>   	if (!group)
>> @@ -3400,6 +3400,16 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		}
>>   	}
>>   
>> +	curr = xa_load(&group->pasid_array, pasid);
>> +	if (curr) {
> 
> No need for curr
> 
> if (xa_load()) {

got it.

>> @@ -3407,13 +3417,12 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
>>   	}
>>   
>> -	ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
>> -	if (ret)
>> -		goto out_unlock;
>> +	curr = xa_store(&group->pasid_array, pasid, handle, GFP_KERNEL);
>> +	if (curr) {
>> +		__iommu_remove_group_pasid(group, pasid, domain);
>> +		ret = xa_err(curr);
> 
> curr could be a valid non-error entry, but that
> would be a WARN_ON condition due to the above check. Like this:
> 
>        ret = xa_insert(&group->pasid_array, pasid, xa_entry,GFP_KERNEL);
>        if (ret) {
> 		WARN_ON(ret == -EBUSY);
> 		goto out_remove;
>        }

yes. it should be a warn_on after the above check.

-- 
Regards,
Yi Liu