Re: [PATCH v4 01/10] iommu: Introduce a replace API for device pasid

On 9/12/24 9:12 PM, 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() 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 Signed-off-by: Yi Liu yi.l.liu@intel.com

---

drivers/iommu/iommu-priv.h | 4 ++ drivers/iommu/iommu.c | 90 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h index de5b54eaa8bf..90b367de267e 100644 --- a/drivers/iommu/iommu-priv.h +++ b/drivers/iommu/iommu-priv.h @@ -27,6 +27,10 @@ static inline const struct iommu_ops *iommu_fwspec_ops(struct iommu_fwspec *fwsp int iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *new_domain); +int iommu_replace_device_pasid(struct iommu_domain *domain,

• 	       struct device *dev, ioasid_t pasid,
  

• 	       struct iommu_attach_handle *handle);
  

• int iommu_device_register_bus(struct iommu_device *iommu, const struct iommu_ops *ops, const struct bus_type *bus,

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index b6b44b184004..066f659018a5 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3347,14 +3347,15 @@ static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, } static int __iommu_set_group_pasid(struct iommu_domain *domain,

• 		   struct iommu_group *group, ioasid_t pasid)
  

• 		   struct iommu_group *group, ioasid_t pasid,
  

• 		   struct iommu_domain *old)
  

  { struct group_device *device, *last_gdev; int ret;

for_each_group_device(group, device) { ret = domain->ops->set_dev_pasid(domain, device->dev,

• 				 pasid, NULL);
  

• 				 pasid, old);
  

  if (ret) goto err_revert; }

@@ -3366,7 +3367,20 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain, for_each_group_device(group, device) { if (device == last_gdev) break;

• iommu_remove_dev_pasid(device->dev, pasid, domain);
  

• /* If no old domain, undo the succeeded devices/pasid */
  

• if (!old) {
  

• 	iommu_remove_dev_pasid(device->dev, pasid, domain);
  

• 	continue;
  

• }
  

• /*
  

•  * Rollback the succeeded devices/pasid to the old domain.
  

•  * And it is a driver bug to fail attaching with a previously
  

•  * good domain.
  

•  */
  

• if (WARN_ON(old->ops->set_dev_pasid(old, device->dev,
  

• 				    pasid, domain)))
  

• 	iommu_remove_dev_pasid(device->dev, pasid, domain);
  

You want to rollback to the 'old' domain, right? So, %s/domain/old/ ?

} return ret; } @@ -3425,7 +3439,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain, if (ret) goto out_unlock;

• ret = __iommu_set_group_pasid(domain, group, pasid);

• ret = __iommu_set_group_pasid(domain, group, pasid, NULL); if (ret) xa_erase(&group->pasid_array, pasid); out_unlock:

@@ -3434,6 +3448,74 @@ 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
• • @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)
  

How about passing the old domain as a parameter?

+{

• /* 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)
  

dev_has_iommu() check is duplicate with above if (!group) check.

By the way, why do you require a non-NULL attach handle? In the current design, attach handles are only used for domains with iopf capability.

• 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 function
  

• * 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);
  

• if (!curr) {

• xa_erase(&group->pasid_array, pasid);
  

• ret = -EINVAL;
  

• goto out_unlock;
  

• }

This seems to be broken as explained above. The attach handle is currently only for iopf-capable domains.

If I understand it correctly, you just want the previous attached domain here, right? If so, why not just passing it to this helper from callers?

• 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.

Thanks, baolu