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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2025/2/22 9:59, Yi Liu wrote:
> On 2025/2/22 01:27, Nicolin Chen wrote:
>> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>>> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
>>>
>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>
>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>
>> A nit:
>>
>>> ---
>>>   drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index eff5f678883b..73555e1cf016 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct 
>>> iommu_domain *domain,
>>>       entry = iommu_make_pasid_array_entry(domain, handle);
>>> -    ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>>> +    ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, 
>>> GFP_KERNEL);
>>
>> Maybe xa_reserve() pairing xa_release()? Same thing though..
> 
> there is slight difference. xa_reserve() will return 0 if there is already
> a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
> failures here. You may notice that the replace function uses xa_reserve()
> as it intend to replace existing entry.

Do you mind adding some comments around the code explaining why
xa_insert() is used instead of xa_reserve() here? This will make the
code easier to understand without needing to refer to this discussion.

Thanks,
baolu