On 2024/9/13 10:44, Baolu Lu wrote:
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...@linux.intel.com>
Signed-off-by: Lu Baolu <baolu...@linux.intel.com>
Signed-off-by: Yi Liu <yi.l....@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/ ?
this will be invoked if the rollback failed. Since the set_dev_pasid op
would keep the 'old' configure, so at this point, the 'old' domain is 'domain'.
}
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?
I suppose it was agreed in the below link.
https://lore.kernel.org/linux-iommu/20240816124707.gz2032...@nvidia.com/
+{
+ /* 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.
I was just referring to the iommu_attach_device_pasid(). So both the two
path could drop the dev_has_iommu() check, is it?
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.
yeah, but it looks fine to always pass in an attach handle. The iopf
path would require hwpt->domain->iopf_handler.
+ 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 attach handle is always passed, then this is not broken. is it?
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?
yeah, I'm open about it. :) @Jason, your opinion?
+
+ 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.
--
Regards,
Yi Liu
