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 2/24/25 12:38, Yi Liu wrote:
> On 2025/2/23 21:32, Baolu Lu wrote:
>> 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.
> 
> hmmm. do you think it is enough when referring to the kdoc of xa_insert()?
> It notes it acts just like xa_reserve() when no stored or reserved entry.
> It fails if there is another present entry, and this suits the need here
> since it does not intend to replace existing entry.
> 
> 
>    * Inserting a NULL entry will store a reserved entry (like xa_reserve())
>    * if no entry is present.  Inserting will fail if a reserved entry is
>    * present, even though loading from this index will return NULL.
> 
>    * Return: 0 if the store succeeded.  -EBUSY if another entry was 
> present.
>    * -ENOMEM if memory could not be allocated.

I feel that it's enough if we add something like:

	/*
	 * Entry present is a failure case. Use xa_insert() instead of
	 * xa_reserve().
	 */

Thanks,
baolu