Re: [PATCH v10 02/18] iommu: Introduce a replace API for device pasid

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Mar 20, 2025 at 06:47:28AM -0700, Yi Liu wrote:
> Provide a high-level API to allow replacements of one domain with another
> for specific pasid of a device. This is similar to
> iommu_replace_group_handle() and it is expected to be used only by IOMMUFD.
> 
> Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-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>

Some nits:

> @@ -3420,6 +3436,99 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>  }
>  EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
>  
> +/**
> + * iommu_replace_device_pasid - Replace the domain that a pasid
> + *                              is attached to

How about "... a specific pasid of the device is attached to"
aligning with the clearer narrative in commit log?

> +int iommu_replace_device_pasid(struct iommu_domain *domain,
> +			       struct device *dev, ioasid_t pasid,
> +			       struct iommu_attach_handle *handle)
> +{
> +	/* Caller must be a probed driver on dev */

What's "a probed driver on dev"? Mind rephrasing this?

Also should it be placed outside this function?

> +	struct iommu_group *group = dev->iommu_group;
> +	struct iommu_attach_handle *entry;
> +	struct iommu_domain *curr_domain;
> +	void *curr;
> +	int ret;
> +
> +	if (!group)
> +		return -ENODEV;
> +
> +	if (!domain->ops->set_dev_pasid)
> +		return -EOPNOTSUPP;
> +
> +	if (dev_iommu_ops(dev) != domain->owner ||
> +	    pasid == IOMMU_NO_PASID || !handle)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);

How about guard(mutex)(&group->mutex)?

> +	entry = iommu_make_pasid_array_entry(domain, handle);
> +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
> +			  XA_ZERO_ENTRY, GFP_KERNEL);
> +	if (xa_is_err(curr)) {
> +		ret = xa_err(curr);
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * No domain (with or without handle) attached, hence not
> +	 * a replace case.
> +	 */
> +	if (!curr) {
> +		xa_release(&group->pasid_array, pasid);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * Reusing handle is problematic as there are paths that refers
> +	 * the handle without lock. To avoid race, reject the callers that
> +	 * attempt it.
> +	 */
> +	if (handle && curr == entry) {
> +		WARN_ON(1);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}

We rejected !handle and !curr cases. So it should be enough with:
	if (curr == entry) {
?

> +
> +	curr_domain = pasid_array_entry_to_domain(curr);
> +	ret = 0;
> +
> +	if (curr_domain != domain) {
> +		ret = __iommu_set_group_pasid(domain, group,
> +					      pasid, curr_domain);
> +		if (ret)
> +			goto out_unlock;
> +	}

Oh, does this mean that we can just use this function to replace
a handle if domain isn't changed? Maybe add this in the kdoc?

> +
> +	if (curr != entry) {

Hmm, since we rejected "curr == entry" already, we don't need to
double check any more?

> +		/*
> +		 * The above xa_cmpxchg() reserved the memory, and the
> +		 * group->mutex is held, this cannot fail.
> +		 */
> +		WARN_ON(xa_is_err(xa_store(&group->pasid_array,
> +					   pasid, entry, GFP_KERNEL)));
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&group->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid, "IOMMUFD_INTERNAL");
> +
>  /*
>   * iommu_detach_device_pasid() - Detach the domain from pasid of device
>   * @domain: the iommu domain.
> -- 
> 2.34.1
>