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

[Nicolin Chen <nicolinc@nvidia.com>]

On Sat, Feb 22, 2025 at 09:59:05AM +0800, 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>

> > > @@ -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.

I see. There is an xa_zero_to_null down in the xa_reserve path.
So, they are different. And there's no need to change anything.

Thanks
Nic