Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Feb 26, 2025 at 03:40:22AM -0800, 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_group_replace_domain_handle() and it is expected to be used
> only by IOMMUFD.
  
> +static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
> +		struct iommu_attach_handle *handle;
> +
> +		handle = xa_untag_pointer(entry);
> +		domain = handle->domain;
> +	} else {
> +		domain = xa_untag_pointer(entry);
> +	}
> +
> +	return domain;
> +}
> +

Might be a bit compact with

static inline struct iommu_domain *pasid_array_entry_to_domain(void *entry)
{
	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_DOMAIN)
		return xa_untag_pointer(entry);
	return ((struct iommu_attach_handle *)xa_untag_pointer(entry))->domain;
}

> +/**
> + * iommu_replace_device_pasid_handle - Replace the domain that a pasid
> + *                                     is attached to
> + * @domain: the new iommu domain
> + * @dev: the attached device.
> + * @pasid: the pasid of the device.
> + * @handle: the attach handle.
> + *
> + * This API allows the pasid to switch domains. The @pasid should have been
> + * attached. Otherwise, this fails.
> + * The pasid will keep the old configuration if replacement failed.
> + * Return 0 on success, or an error.
> + */
> +int iommu_replace_device_pasid_handle(struct iommu_domain *domain,
> +				      struct device *dev, ioasid_t pasid,
> +				      struct iommu_attach_handle *handle)
> +{
> +	/* Caller must be a probed driver on dev */
> +	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);
> +	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;
> +	}
> +
> +	/* Not a replace case */
> +	if (!curr) {

Mind elaborating this?

> +		xa_release(&group->pasid_array, pasid);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	curr_domain = pasid_array_entry_to_domain(curr);
> +	ret = 0;

Can we set the ret = 0 at the top?

Thanks
Nicolin