Re: [PATCH v5 01/12] iommu: Introduce a replace API for device pasid

Baolu Lu
2024-11-05T03:59:25Z

On 11/4/24 21:25, Yi Liu wrote:
> +/**
> + * iommu_replace_device_pasid - 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. Return 0 on success, or an
> + * error. The pasid will keep the old configuration if replacement failed.
> + * This is supposed to be used by iommufd, and iommufd can guarantee that
> + * both iommu_attach_device_pasid() and iommu_replace_device_pasid() would
> + * pass in a valid @handle.
> + */
> +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 */
> + struct iommu_group *group = dev->iommu_group;
> + struct iommu_attach_handle *curr;
> + int ret;
> +
> + if (!domain->ops->set_dev_pasid)
> + return -EOPNOTSUPP;
> +
> + if (!group)
> + return -ENODEV;
> +
> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> + pasid == IOMMU_NO_PASID || !handle)
> + return -EINVAL;
> +
> + handle->domain = domain;
> +
> + mutex_lock(&group->mutex);
> + /*
> + * The iommu_attach_handle of the pasid becomes inconsistent with the
> + * actual handle per the below operation. The concurrent PRI path will
> + * deliver the PRQs per the new handle, this does not have a functional
> + * impact. The PRI path would eventually become consistent when the
> + * replacement is done.
> + */
> + curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
> + pasid, handle,
> + GFP_KERNEL);

The iommu drivers can only flush pending PRs in the hardware queue when
__iommu_set_group_pasid() is called. So, it appears more reasonable to
reorder things like this:

__iommu_set_group_pasid();
switch_attach_handle();

Or anything I overlooked?

> + if (!curr) {
> + xa_erase(&group->pasid_array, pasid);
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> + ret = xa_err(curr);
> + if (ret)
> + goto out_unlock;
> +
> + if (curr->domain == domain)
> + goto out_unlock;
> +
> + ret = __iommu_set_group_pasid(domain, group, pasid, curr->domain);
> + if (ret)
> + WARN_ON(handle != xa_store(&group->pasid_array, pasid,
> + curr, 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.

--
baolu