RE: [PATCH v6 02/10] iommu: Remove sva handle list

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Monday, May 27, 2024 12:05 PM
> 
> @@ -69,11 +68,16 @@ static struct iommu_mm_data
> *iommu_alloc_mm_data(struct mm_struct *mm, struct de
>   */
>  struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct
> mm_struct *mm)
>  {
> +	struct iommu_group *group = dev->iommu_group;
> +	struct iommu_attach_handle *attach_handle;
>  	struct iommu_mm_data *iommu_mm;
>  	struct iommu_domain *domain;
>  	struct iommu_sva *handle;

it's confusing to have both 'handle' and 'attach_handle' in one function.

Clearer to rename 'handle' as 'sva'.

>  	int ret;
> 
> +	if (!group)
> +		return ERR_PTR(-ENODEV);
> +
>  	mutex_lock(&iommu_sva_lock);
> 
>  	/* Allocate mm->pasid if necessary. */
> @@ -83,12 +87,13 @@ struct iommu_sva *iommu_sva_bind_device(struct
> device *dev, struct mm_struct *mm
>  		goto out_unlock;
>  	}
> 
> -	list_for_each_entry(handle, &mm->iommu_mm->sva_handles,
> handle_item) {
> -		if (handle->dev == dev) {
> -			refcount_inc(&handle->users);
> -			mutex_unlock(&iommu_sva_lock);
> -			return handle;
> -		}
> +	/* A bond already exists, just take a reference`. */
> +	attach_handle = iommu_attach_handle_get(group, iommu_mm-
> >pasid, IOMMU_DOMAIN_SVA);
> +	if (!IS_ERR(attach_handle)) {
> +		handle = container_of(attach_handle, struct iommu_sva,
> handle);
> +		refcount_inc(&handle->users);
> +		mutex_unlock(&iommu_sva_lock);
> +		return handle;
>  	}

It's counter-intuitive to move forward when an error is returned.

e.g. if it's -EBUSY indicating the pasid already used for another type then
following attempts shouldn't been tried.

probably we should have iommu_attach_handle_get() return NULL
instead of -ENOENT when the entry is free? then:

	attach_handle = iommu_attach_handle_get();
	if (IS_ERR(attach_handle)) {
		ret = PTR_ERR(attach_handle);
		goto out_unlock;
	} else if (attach_handle) {
		/* matched and increase handle->users */
	}

	/* free entry falls through */

But then there is one potential issue with the design that 'handle'
can be optional in iommu_attach_device_pasid(). In that case
xa_load returns NULL then we cannot differentiate a real unused
PASID vs. one which has been attached w/o an handle.

Does it suggest that having the caller to always provide a handle
makes more sense?