Re: [PATCH 4/5] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

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

On 2025/2/19 03:27, Jason Gunthorpe wrote:
> On Tue, Feb 11, 2025 at 10:05:39PM -0800, Yi Liu wrote:
>> The current implementation stores group->pasid_array before the underlying
>> iommu driver has successfully set the new domain. This can lead to issues
>> where PRIs are received on the new domain before the attach operation is
>> completed. This patch swaps the order of operations to ensure that the
>> domain is set in the iommu driver before updating group->pasid_array.
> 
> I think I would drop this note in a comment since it is subtle and
> important..
> 
>> -	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
>> -	if (ret)
>> +	if (xa_load(&group->pasid_array, pasid)) {
>> +		ret = -EBUSY;
>>   		goto out_unlock;
>> +	}
> 
> Let's write it like this:
> 
> 	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
> 	if (ret)
> 		goto out_unlock;
> 
> 	ret = __iommu_set_group_pasid(domain, group, pasid);
> 	if (ret) {
> 		xa_release(&group->pasid_array, pasid);
> 		goto out_unlock;
> 	}
> 
> 	/*
> 	 * The xa_insert() above reserved the memory, and the group->mutex is
> 	 * held, this cannot fail. The new domain cannot be visible until the
> 	 * operation succeeds as we cannot tolerate PRIs becoming concurrently
> 	 * queued and then failing attach.
> 	 */
> 	ret = xa_err(
> 		xa_store(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL));
> 	WARN_ON(ret);
> 	ret = 0;

perhaps we can save the above line by WARN_ON(xa_is_err(xa_store())); :)

> 
>> @@ -3510,19 +3518,26 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>>   	pasid_entry = iommu_make_pasid_entry(domain, handle);
>>   
>>   	mutex_lock(&group->mutex);
>> -	ret = xa_insert(&group->pasid_array,
>> -			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
>> -	if (ret)
>> +	if (xa_load(&group->pasid_array, IOMMU_NO_PASID)) {
>> +		ret = -EBUSY;
>>   		goto err_unlock;
>> +	}
> 
> Same here

yes.

-- 
Regards,
Yi Liu