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@xxxxxxxxxxxxxxx>
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
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
