RE: [PATCH v6 01/10] iommu: Introduce domain attachment handle

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Monday, May 27, 2024 12:05 PM
> 
> @@ -99,7 +99,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device
> *dev, struct mm_struct *mm
> 
>  	/* Search for an existing domain. */
>  	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
> -		ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
> >pasid);
> +		handle->handle.domain = domain;
> +		ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
> >pasid,
> +						&handle->handle);

move the setting of handle.domain into the helper?

> @@ -3382,11 +3383,9 @@ int iommu_attach_device_pasid(struct
> iommu_domain *domain,
>  		}
>  	}
> 
> -	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
> GFP_KERNEL);
> -	if (curr) {
> -		ret = xa_err(curr) ? : -EBUSY;
> +	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
> +	if (ret)
>  		goto out_unlock;
> -	}

this leads to a slightly different implication comparing to the old code.

the domain pointer was always tracked in the old code but now the handle
is optional.

if iommu core only needs to know whether a pasid has been attached (as
in this helper), it still works as xa_insert() will mark the entry as reserved
if handle==NULL and next xa_insert() at the same index will fail due to
the entry being reserved.

But if certain path (other than iopf) in the iommu core needs to know
the exact domain pointer then this change breaks it.

Anyway some explanation is welcomed here for why this change is safe.

> @@ -3414,7 +3413,7 @@ void iommu_detach_device_pasid(struct
> iommu_domain *domain, struct device *dev,
> 
>  	mutex_lock(&group->mutex);
>  	__iommu_remove_group_pasid(group, pasid, domain);
> -	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
> +	xa_erase(&group->pasid_array, pasid);

if the entry is valid do we want to keep the WARN_ON() upon handle->domain?