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

[Jason Gunthorpe <jgg@nvidia.com>]

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?

> 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()) {

> @@ -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;
      }

Jason