PASID (Process Address Space ID) is a PCIe extension to tag the DMA transactions out of a physical device, and most modern IOMMU hardware have supported PASID granular address translation. So a PASID-capable device can be attached to multiple hwpts (a.k.a. domains), and each attachment is tagged with a pasid.
This series is based on the preparation series [1] [2], it first adds a missing iommu API to replace the domain for a pasid. Based on the iommu pasid attach/ replace/detach APIs, this series adds iommufd APIs for device drivers to attach/replace/detach pasid to/from hwpt per userspace's request, and adds selftest to validate the iommufd APIs.
While this series has a missing part which is to enforce the domain allocation with special flag if it will be used by PASID [3]. This is due to special requirements by AMD. Since it is still in mailing discussion [4], so let's mark it here. Once it's finalized, this series needs to enforce the domain flag check to ensure the AMD pasid support is not broken from day-1.
The completed code can be found in the below link [5]. Heads up! The existing iommufd selftest was broken, there was a fix [6] to it, but not been upstreamed yet. If want to run the iommufd selftest, please apply that fix. Sorry for the inconvenience.
[1] https://lore.kernel.org/linux-iommu/20240912130427.10119-1-yi.l.liu@intel.co... [2] https://lore.kernel.org/linux-iommu/20240912130653.11028-1-yi.l.liu@intel.co... [3] https://lore.kernel.org/linux-iommu/20240822124433.GD3468552@ziepe.ca/ [4] https://lore.kernel.org/linux-iommu/20240911101911.6269-3-vasant.hegde@amd.c... [5] https://github.com/yiliu1765/iommufd/tree/iommufd_pasid [6] https://lore.kernel.org/linux-iommu/20240111073213.180020-1-baolu.lu@linux.i...
Change log:
v4: - Replace remove_dev_pasid() by supporting set_dev_pasid() for blocking domain (Kevin) - This is done by the preparation series "Support attaching PASID to the blocked_domain" - Misc tweaks to foil the merging of the iommufd iopf series. Three new patches are added: - iommufd: Always pass iommu_attach_handle to iommu core - iommufd: Move the iommufd_handle helpers to iommufd_private.h - iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle() - Renmae patch 03 of v3 to be "iommufd: Support pasid attach/replace" - Add test case for attaching/replacing iopf-capable hwpt to pasid
v3: https://lore.kernel.org/kvm/20240628090557.50898-1-yi.l.liu@intel.com/ - Split the set_dev_pasid op enhancements for domain replacement to be a separate series "Make set_dev_pasid op supportting domain replacement" [1]. The below changes are made in the separate series. *) set_dev_pasid() callback should keep the old config if failed to attach to a domain. This simplifies the caller a lot as caller does not need to attach it back to old domain explicitly. This also avoids some corner cases in which the core may do duplicated domain attachment as described in below link (Jason) https://lore.kernel.org/linux-iommu/BN9PR11MB52768C98314A95AFCD2FA6478C0F2@B... *) Drop patch 10 of v2 as it's a bug fix and can be submitted separately (Kevin) *) Rebase on top of Baolu's domain_alloc_paging refactor series (Jason) - Drop the attach_data which includes attach_fn and pasid, insteadly passing the pasid through the device attach path. (Jason) - Add a pasid-num-bits property to mock dev to make pasid selftest work (Kevin)
v2: https://lore.kernel.org/linux-iommu/20240412081516.31168-1-yi.l.liu@intel.co... - Domain replace for pasid should be handled in set_dev_pasid() callbacks instead of remove_dev_pasid and call set_dev_pasid afteward in iommu layer (Jason) - Make xarray operations more self-contained in iommufd pasid attach/replace/detach (Jason) - Tweak the dev_iommu_get_max_pasids() to allow iommu driver to populate the max_pasids. This makes the iommufd selftest simpler to meet the max_pasids check in iommu_attach_device_pasid() (Jason)
v1: https://lore.kernel.org/kvm/20231127063428.127436-1-yi.l.liu@intel.com/#r - Implemnet iommu_replace_device_pasid() to fall back to the original domain if this replacement failed (Kevin) - Add check in do_attach() to check corressponding attach_fn per the pasid value.
rfc: https://lore.kernel.org/linux-iommu/20230926092651.17041-1-yi.l.liu@intel.co...
Regards, Yi Liu
Yi Liu (10): iommu: Introduce a replace API for device pasid iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle() iommufd: Move the iommufd_handle helpers to iommufd_private.h iommufd: Always pass iommu_attach_handle to iommu core iommufd: Pass pasid through the device attach/replace path iommufd: Support pasid attach/replace iommufd/selftest: Add set_dev_pasid and remove_dev_pasid in mock iommu iommufd/selftest: Add a helper to get test device iommufd/selftest: Add test ops to test pasid attach/detach iommufd/selftest: Add coverage for iommufd pasid attach/detach
drivers/iommu/iommu-priv.h | 4 + drivers/iommu/iommu.c | 90 +++++- drivers/iommu/iommufd/Makefile | 1 + drivers/iommu/iommufd/device.c | 46 ++-- drivers/iommu/iommufd/fault.c | 90 ++---- drivers/iommu/iommufd/hw_pagetable.c | 5 +- drivers/iommu/iommufd/iommufd_private.h | 129 ++++++++- drivers/iommu/iommufd/iommufd_test.h | 30 ++ drivers/iommu/iommufd/pasid.c | 157 +++++++++++ drivers/iommu/iommufd/selftest.c | 208 +++++++++++++- include/linux/iommufd.h | 7 + tools/testing/selftests/iommu/iommufd.c | 256 ++++++++++++++++++ .../selftests/iommu/iommufd_fail_nth.c | 29 +- tools/testing/selftests/iommu/iommufd_utils.h | 78 ++++++ 14 files changed, 1005 insertions(+), 125 deletions(-) create mode 100644 drivers/iommu/iommufd/pasid.c
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); } 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) +{ + /* 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 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; + } + + 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.
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)for_each_group_device(group, device) { ret = domain->ops->set_dev_pasid(domain, device->dev,
pasid, NULL);
pasid, old);@@ -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
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.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/ ?
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.GZ2032816@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.
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
+/**
- 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.
this function assumes handle is always valid. So above comment makes it clear that iommufd is the only user and it will always pass in a valid handle.
but the code in iommu_attach_device_pasid() allows handle to be NULL. Then that comment is meaningless for it.
Also following patches are built on iommufd always passing in a valid handle as it's required by pasid operations but there is no detail explanation why it's mandatory or any alternative option exists. More explanation is welcomed.
- */
+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 withthe
* actual handle per the below operation. The concurrent PRI pathwill
* deliver the PRQs per the new handle, this does not have a function* impact. The PRI path would eventually become consistent when
s/function/functional/
the
* replacement is done.*/- curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
pasid, handle,GFP_KERNEL);
Could you elaborate why the PRI path will eventually becomes consistent with this path?
On 2024/9/30 15:38, Tian, Kevin wrote:
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
+/**
- 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.
this function assumes handle is always valid. So above comment makes it clear that iommufd is the only user and it will always pass in a valid handle.
but the code in iommu_attach_device_pasid() allows handle to be NULL. Then that comment is meaningless for it.
Actually, this is why I added the above comment. iommufd can ensure it would pass valid handle to both iommu_attach_device_pasid() and iommu_replace_device_pasid(), and iommu_replace_device_pasid() is only used by iommufd, so iommu_replace_device_pasid() can assume all the pasids have a valid handle stored in the pasid_array.
Also following patches are built on iommufd always passing in a valid handle as it's required by pasid operations but there is no detail explanation why it's mandatory or any alternative option exists. More explanation is welcomed.
There is more detail about it in the below link, but is it necessary to add them in the comment as well, or is it ok to add more explanation in commit message?
https://lore.kernel.org/linux-iommu/0bf383b7-ed96-49ca-b1da-d1fff48e161a@int...
- */
+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 withthe
* actual handle per the below operation. The concurrent PRI pathwill
* deliver the PRQs per the new handle, this does not have a function* impact. The PRI path would eventually become consistent whens/function/functional/
got it.
the
* replacement is done.*/- curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
pasid, handle,GFP_KERNEL);Could you elaborate why the PRI path will eventually becomes consistent with this path?
Because the handle stored in pasid_array would be consistent with the configuration of pasid. So the PRI would be forwarded to the correct domain.
There is a wrapper of iommu_attach_group_handle(), so making a wrapper for iommu_replace_group_handle() for further code refactor. No functional change intended.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/fault.c | 54 ++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index c4715261f2c7..cd56745f3003 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -146,35 +146,30 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, kfree(handle); }
-static int __fault_domain_replace_dev(struct iommufd_device *idev, - struct iommufd_hw_pagetable *hwpt, - struct iommufd_hw_pagetable *old) +/* Caller to free the old iommufd_attach_handle */ +static struct iommufd_attach_handle * +__fault_domain_replace_dev(struct iommufd_device *idev, + struct iommufd_hw_pagetable *hwpt, + struct iommufd_hw_pagetable *old) { - struct iommufd_attach_handle *handle, *curr = NULL; + struct iommufd_attach_handle *handle, *curr; int ret;
- if (old->fault) - curr = iommufd_device_get_attach_handle(idev); - - if (hwpt->fault) { - handle = kzalloc(sizeof(*handle), GFP_KERNEL); - if (!handle) - return -ENOMEM; + curr = iommufd_device_get_attach_handle(idev);
- handle->idev = idev; - ret = iommu_replace_group_handle(idev->igroup->group, - hwpt->domain, &handle->handle); - } else { - ret = iommu_replace_group_handle(idev->igroup->group, - hwpt->domain, NULL); - } + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return ERR_PTR(-ENOMEM);
- if (!ret && curr) { - iommufd_auto_response_faults(old, curr); - kfree(curr); + handle->idev = idev; + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, &handle->handle); + if (ret) { + kfree(handle); + return ERR_PTR(ret); }
- return ret; + return curr; }
int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, @@ -183,6 +178,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, { bool iopf_off = !hwpt->fault && old->fault; bool iopf_on = hwpt->fault && !old->fault; + struct iommufd_attach_handle *curr = NULL; int ret;
if (iopf_on) { @@ -191,13 +187,25 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, return ret; }
- ret = __fault_domain_replace_dev(idev, hwpt, old); + if (hwpt->fault) { + curr = __fault_domain_replace_dev(idev, hwpt, old); + ret = IS_ERR(curr) ? PTR_ERR(curr) : 0; + } else { + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, NULL); + } + if (ret) { if (iopf_on) iommufd_fault_iopf_disable(idev); return ret; }
+ if (curr) { + iommufd_auto_response_faults(old, curr); + kfree(curr); + } + if (iopf_off) iommufd_fault_iopf_disable(idev);
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
@@ -191,13 +187,25 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, return ret; }
- ret = __fault_domain_replace_dev(idev, hwpt, old);
if (hwpt->fault) {
curr = __fault_domain_replace_dev(idev, hwpt, old);ret = IS_ERR(curr) ? PTR_ERR(curr) : 0;} else {
ret = iommu_replace_group_handle(idev->igroup->group,hwpt->domain, NULL);}
if (ret) { if (iopf_on) iommufd_fault_iopf_disable(idev); return ret; }
if (curr) {
iommufd_auto_response_faults(old, curr);kfree(curr);}
this is incorrect. The old code does auto response as long as old->fault is true and replace succeeds. But now you make it an operation only when hwpt->fault is true.
On 2024/9/30 15:42, Tian, Kevin wrote:
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
@@ -191,13 +187,25 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, return ret; }
- ret = __fault_domain_replace_dev(idev, hwpt, old);
if (hwpt->fault) {
curr = __fault_domain_replace_dev(idev, hwpt, old);ret = IS_ERR(curr) ? PTR_ERR(curr) : 0;} else {
ret = iommu_replace_group_handle(idev->igroup->group,hwpt->domain, NULL);}
if (ret) { if (iopf_on) iommufd_fault_iopf_disable(idev); return ret; }
if (curr) {
iommufd_auto_response_faults(old, curr);kfree(curr);}
this is incorrect. The old code does auto response as long as old->fault is true and replace succeeds. But now you make it an operation only when hwpt->fault is true.
oops, yes it is. needs to get the curr handle in this function instead of getting it from the __fault_domain_replace_dev() helper.
iommufd plans to always pass in an iommu_attach_handle to the iommu core, so it's no longer fault specific, hence move the helpers out of the fault.c
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/fault.c | 61 +------------------------ drivers/iommu/iommufd/iommufd_private.h | 57 +++++++++++++++++++++++ 2 files changed, 59 insertions(+), 59 deletions(-)
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index cd56745f3003..50ce6c6e61be 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -55,25 +55,6 @@ static void iommufd_fault_iopf_disable(struct iommufd_device *idev) mutex_unlock(&idev->iopf_lock); }
-static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) -{ - struct iommufd_attach_handle *handle; - int ret; - - handle = kzalloc(sizeof(*handle), GFP_KERNEL); - if (!handle) - return -ENOMEM; - - handle->idev = idev; - ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, - &handle->handle); - if (ret) - kfree(handle); - - return ret; -} - int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { @@ -86,7 +67,7 @@ int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, if (ret) return ret;
- ret = __fault_domain_attach_dev(hwpt, idev); + ret = iommufd_dev_attach_handle(hwpt, idev); if (ret) iommufd_fault_iopf_disable(idev);
@@ -122,18 +103,6 @@ static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, mutex_unlock(&fault->mutex); }
-static struct iommufd_attach_handle * -iommufd_device_get_attach_handle(struct iommufd_device *idev) -{ - struct iommu_attach_handle *handle; - - handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0); - if (IS_ERR(handle)) - return NULL; - - return to_iommufd_handle(handle); -} - void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { @@ -146,32 +115,6 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, kfree(handle); }
-/* Caller to free the old iommufd_attach_handle */ -static struct iommufd_attach_handle * -__fault_domain_replace_dev(struct iommufd_device *idev, - struct iommufd_hw_pagetable *hwpt, - struct iommufd_hw_pagetable *old) -{ - struct iommufd_attach_handle *handle, *curr; - int ret; - - curr = iommufd_device_get_attach_handle(idev); - - handle = kzalloc(sizeof(*handle), GFP_KERNEL); - if (!handle) - return ERR_PTR(-ENOMEM); - - handle->idev = idev; - ret = iommu_replace_group_handle(idev->igroup->group, - hwpt->domain, &handle->handle); - if (ret) { - kfree(handle); - return ERR_PTR(ret); - } - - return curr; -} - int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) @@ -188,7 +131,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, }
if (hwpt->fault) { - curr = __fault_domain_replace_dev(idev, hwpt, old); + curr = iommufd_dev_replace_handle(idev, hwpt, old); ret = IS_ERR(curr) ? PTR_ERR(curr) : 0; } else { ret = iommu_replace_group_handle(idev->igroup->group, diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 1141c0633dc9..7039c86d9981 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -458,6 +458,63 @@ struct iommufd_attach_handle { /* Convert an iommu attach handle to iommufd handle. */ #define to_iommufd_handle(hdl) container_of(hdl, struct iommufd_attach_handle, handle)
+static inline struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{ + struct iommu_attach_handle *handle; + + handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0); + if (IS_ERR(handle)) + return NULL; + + return to_iommufd_handle(handle); +} + +static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt, + struct iommufd_device *idev) +{ + struct iommufd_attach_handle *handle; + int ret; + + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; + + handle->idev = idev; + ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, + &handle->handle); + if (ret) + kfree(handle); + + return ret; +} + +/* Caller to free the old iommufd_attach_handle */ +static inline struct iommufd_attach_handle * +iommufd_dev_replace_handle(struct iommufd_device *idev, + struct iommufd_hw_pagetable *hwpt, + struct iommufd_hw_pagetable *old) +{ + struct iommufd_attach_handle *handle, *curr; + int ret; + + curr = iommufd_device_get_attach_handle(idev); + + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return ERR_PTR(-ENOMEM); + + handle->idev = idev; + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, &handle->handle); + if (ret) { + kfree(handle); + return ERR_PTR(ret); + } + + return curr; +} + static inline struct iommufd_fault * iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id) {
From: Yi Liu yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
iommufd plans to always pass in an iommu_attach_handle to the iommu core, so it's no longer fault specific, hence move the helpers out of the fault.c
again please put the reason for why iommufd plans to do so.
--- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -458,6 +458,63 @@ struct iommufd_attach_handle { /* Convert an iommu attach handle to iommufd handle. */ #define to_iommufd_handle(hdl) container_of(hdl, struct iommufd_attach_handle, handle)
+static inline struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{
- struct iommu_attach_handle *handle;
- handle = iommu_attach_handle_get(idev->igroup->group,
IOMMU_NO_PASID, 0);
- if (IS_ERR(handle))
return NULL;- return to_iommufd_handle(handle);
+}
+static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
struct iommufd_device *idev)+{
- struct iommufd_attach_handle *handle;
- int ret;
- handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (!handle)
return -ENOMEM;- handle->idev = idev;
- ret = iommu_attach_group_handle(hwpt->domain, idev->igroup-
group,
&handle->handle);- if (ret)
kfree(handle);- return ret;
+}
+/* Caller to free the old iommufd_attach_handle */ +static inline struct iommufd_attach_handle * +iommufd_dev_replace_handle(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt,struct iommufd_hw_pagetable *old)+{
- struct iommufd_attach_handle *handle, *curr;
- int ret;
- curr = iommufd_device_get_attach_handle(idev);
- handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (!handle)
return ERR_PTR(-ENOMEM);- handle->idev = idev;
- ret = iommu_replace_group_handle(idev->igroup->group,
hwpt->domain, &handle->handle);- if (ret) {
kfree(handle);return ERR_PTR(ret);- }
- return curr;
+}
why putting them in header file instead of C file?
On 2024/9/30 15:44, Tian, Kevin wrote:
From: Yi Liu yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
iommufd plans to always pass in an iommu_attach_handle to the iommu core, so it's no longer fault specific, hence move the helpers out of the fault.c
again please put the reason for why iommufd plans to do so.
how about the below?
The iommu_attach_handle tracks the attached domains in the group->pasid_array, it is optional so far. But to support the domain replacement for a pasid, it needs to get the attached domain, so making iommufd always pass a handle is a choice. Before that, we would need to decouple the handle allocation and fault code as non-fault capable hwpt attach would also allocate handle.
--- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -458,6 +458,63 @@ struct iommufd_attach_handle { /* Convert an iommu attach handle to iommufd handle. */ #define to_iommufd_handle(hdl) container_of(hdl, struct iommufd_attach_handle, handle)
+static inline struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{
- struct iommu_attach_handle *handle;
- handle = iommu_attach_handle_get(idev->igroup->group,
IOMMU_NO_PASID, 0);
- if (IS_ERR(handle))
return NULL;- return to_iommufd_handle(handle);
+}
+static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
struct iommufd_device *idev)+{
- struct iommufd_attach_handle *handle;
- int ret;
- handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (!handle)
return -ENOMEM;- handle->idev = idev;
- ret = iommu_attach_group_handle(hwpt->domain, idev->igroup-
group,
&handle->handle);- if (ret)
kfree(handle);- return ret;
+}
+/* Caller to free the old iommufd_attach_handle */ +static inline struct iommufd_attach_handle * +iommufd_dev_replace_handle(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt,struct iommufd_hw_pagetable *old)+{
- struct iommufd_attach_handle *handle, *curr;
- int ret;
- curr = iommufd_device_get_attach_handle(idev);
- handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (!handle)
return ERR_PTR(-ENOMEM);- handle->idev = idev;
- ret = iommu_replace_group_handle(idev->igroup->group,
hwpt->domain, &handle->handle);- if (ret) {
kfree(handle);return ERR_PTR(ret);- }
- return curr;
+}
why putting them in header file instead of C file?
put it in device.c is also fine. :) but not in the fault.c as the non fault path also needs to use handle.
The iommu_attach_handle is optional in the RID attach/replace API and the PASID attach APIs. But it is a mandatory argument for the PASID replace API. Without it, the PASID replace path cannot get the old domain. Hence, the PASID path (attach/replace) requires the attach handle. As iommufd is the major user of the RID attach/replace with iommu_attach_handle, this also makes the iommufd always pass the attach handle for the RID path as well. This keeps the RID and PASID path much aligned.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/fault.c | 21 +++++++-------------- drivers/iommu/iommufd/iommufd_private.h | 19 ++++++++++++++++--- 2 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index 50ce6c6e61be..9fa56b3c2b7d 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -121,7 +121,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, { bool iopf_off = !hwpt->fault && old->fault; bool iopf_on = hwpt->fault && !old->fault; - struct iommufd_attach_handle *curr = NULL; + struct iommufd_attach_handle *curr; int ret;
if (iopf_on) { @@ -130,24 +130,17 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, return ret; }
- if (hwpt->fault) { - curr = iommufd_dev_replace_handle(idev, hwpt, old); - ret = IS_ERR(curr) ? PTR_ERR(curr) : 0; - } else { - ret = iommu_replace_group_handle(idev->igroup->group, - hwpt->domain, NULL); - } - - if (ret) { + curr = iommufd_dev_replace_handle(idev, hwpt, old); + if (IS_ERR(curr)) { if (iopf_on) iommufd_fault_iopf_disable(idev); - return ret; + return PTR_ERR(curr); }
- if (curr) { + if (old->fault) iommufd_auto_response_faults(old, curr); - kfree(curr); - } + + kfree(curr);
if (iopf_off) iommufd_fault_iopf_disable(idev); diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 7039c86d9981..30696936a926 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -499,6 +499,8 @@ iommufd_dev_replace_handle(struct iommufd_device *idev, int ret;
curr = iommufd_device_get_attach_handle(idev); + if (!curr) + return ERR_PTR(-EINVAL);
handle = kzalloc(sizeof(*handle), GFP_KERNEL); if (!handle) @@ -541,28 +543,39 @@ static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt, if (hwpt->fault) return iommufd_fault_domain_attach_dev(hwpt, idev);
- return iommu_attach_group(hwpt->domain, idev->igroup->group); + return iommufd_dev_attach_handle(hwpt, idev); }
static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { + struct iommufd_attach_handle *handle; + if (hwpt->fault) { iommufd_fault_domain_detach_dev(hwpt, idev); return; }
- iommu_detach_group(hwpt->domain, idev->igroup->group); + handle = iommufd_device_get_attach_handle(idev); + iommu_detach_group_handle(hwpt->domain, idev->igroup->group); + kfree(handle); }
static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) { + struct iommufd_attach_handle *curr; + if (old->fault || hwpt->fault) return iommufd_fault_domain_replace_dev(idev, hwpt, old);
- return iommu_group_replace_domain(idev->igroup->group, hwpt->domain); + curr = iommufd_dev_replace_handle(idev, hwpt, old); + if (IS_ERR(curr)) + return PTR_ERR(curr); + + kfree(curr); + return 0; }
#ifdef CONFIG_IOMMUFD_TEST
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
The iommu_attach_handle is optional in the RID attach/replace API and the PASID attach APIs. But it is a mandatory argument for the PASID replace API. Without it, the PASID replace path cannot get the old domain. Hence, the PASID path (attach/replace) requires the attach handle. As iommufd is the major user of the RID attach/replace with iommu_attach_handle, this also makes the iommufd always pass the attach handle for the RID path as well. This keeps the RID and PASID path much aligned.
hmm this looks more like a design choice instead of mandatory requirement, e.g. as Baolu commented you can also pass in the old domain. though I'm OK with what this patch does...
On 2024/9/30 15:45, Tian, Kevin wrote:
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
The iommu_attach_handle is optional in the RID attach/replace API and the PASID attach APIs. But it is a mandatory argument for the PASID replace API. Without it, the PASID replace path cannot get the old domain. Hence, the PASID path (attach/replace) requires the attach handle. As iommufd is the major user of the RID attach/replace with iommu_attach_handle, this also makes the iommufd always pass the attach handle for the RID path as well. This keeps the RID and PASID path much aligned.
hmm this looks more like a design choice instead of mandatory requirement, e.g. as Baolu commented you can also pass in the old domain. though I'm OK with what this patch does...
yes, it is a choice. I think it was agreed in the below link.
https://lore.kernel.org/linux-iommu/20240816124707.GZ2032816@nvidia.com/
Most of the core logic before conducting the actual device attach/ replace operation can be shared with pasid attach/replace. So pass pasid through the device attach/replace helpers to prepare adding pasid attach/replace.
So far the @pasid should only be IOMMU_NO_PASID. No functional change.
Signed-off-by: Kevin Tian kevin.tian@intel.com Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/device.c | 44 ++++++++++++----------- drivers/iommu/iommufd/fault.c | 14 +++++--- drivers/iommu/iommufd/hw_pagetable.c | 5 +-- drivers/iommu/iommufd/iommufd_private.h | 47 ++++++++++++++++--------- 4 files changed, 66 insertions(+), 44 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 3214a4c17c6b..953eb2349d59 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -352,7 +352,8 @@ static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging, }
int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) + struct iommufd_device *idev, + ioasid_t pasid) { int rc;
@@ -377,7 +378,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, * attachment. */ if (list_empty(&idev->igroup->device_list)) { - rc = iommufd_hwpt_attach_device(hwpt, idev); + rc = iommufd_hwpt_attach_device(hwpt, idev, pasid); if (rc) goto err_unresv; idev->igroup->hwpt = hwpt; @@ -396,14 +397,14 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, }
struct iommufd_hw_pagetable * -iommufd_hw_pagetable_detach(struct iommufd_device *idev) +iommufd_hw_pagetable_detach(struct iommufd_device *idev, ioasid_t pasid) { struct iommufd_hw_pagetable *hwpt = idev->igroup->hwpt;
mutex_lock(&idev->igroup->lock); list_del(&idev->group_item); if (list_empty(&idev->igroup->device_list)) { - iommufd_hwpt_detach_device(hwpt, idev); + iommufd_hwpt_detach_device(hwpt, idev, pasid); idev->igroup->hwpt = NULL; } if (hwpt_is_paging(hwpt)) @@ -416,12 +417,12 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev) }
static struct iommufd_hw_pagetable * -iommufd_device_do_attach(struct iommufd_device *idev, +iommufd_device_do_attach(struct iommufd_device *idev, ioasid_t pasid, struct iommufd_hw_pagetable *hwpt) { int rc;
- rc = iommufd_hw_pagetable_attach(hwpt, idev); + rc = iommufd_hw_pagetable_attach(hwpt, idev, pasid); if (rc) return ERR_PTR(rc); return NULL; @@ -470,7 +471,7 @@ iommufd_group_do_replace_paging(struct iommufd_group *igroup, }
static struct iommufd_hw_pagetable * -iommufd_device_do_replace(struct iommufd_device *idev, +iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid, struct iommufd_hw_pagetable *hwpt) { struct iommufd_group *igroup = idev->igroup; @@ -498,7 +499,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, goto err_unlock; }
- rc = iommufd_hwpt_replace_device(idev, hwpt, old_hwpt); + rc = iommufd_hwpt_replace_device(idev, pasid, hwpt, old_hwpt); if (rc) goto err_unresv;
@@ -533,7 +534,8 @@ iommufd_device_do_replace(struct iommufd_device *idev, }
typedef struct iommufd_hw_pagetable *(*attach_fn)( - struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt); + struct iommufd_device *idev, ioasid_t pasid, + struct iommufd_hw_pagetable *hwpt);
/* * When automatically managing the domains we search for a compatible domain in @@ -541,7 +543,7 @@ typedef struct iommufd_hw_pagetable *(*attach_fn)( * Automatic domain selection will never pick a manually created domain. */ static struct iommufd_hw_pagetable * -iommufd_device_auto_get_domain(struct iommufd_device *idev, +iommufd_device_auto_get_domain(struct iommufd_device *idev, ioasid_t pasid, struct iommufd_ioas *ioas, u32 *pt_id, attach_fn do_attach) { @@ -570,7 +572,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev, hwpt = &hwpt_paging->common; if (!iommufd_lock_obj(&hwpt->obj)) continue; - destroy_hwpt = (*do_attach)(idev, hwpt); + destroy_hwpt = (*do_attach)(idev, pasid, hwpt); if (IS_ERR(destroy_hwpt)) { iommufd_put_object(idev->ictx, &hwpt->obj); /* @@ -597,7 +599,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev, hwpt = &hwpt_paging->common;
if (!immediate_attach) { - destroy_hwpt = (*do_attach)(idev, hwpt); + destroy_hwpt = (*do_attach)(idev, pasid, hwpt); if (IS_ERR(destroy_hwpt)) goto out_abort; } else { @@ -618,8 +620,9 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev, return destroy_hwpt; }
-static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id, - attach_fn do_attach) +static int iommufd_device_change_pt(struct iommufd_device *idev, + ioasid_t pasid, + u32 *pt_id, attach_fn do_attach) { struct iommufd_hw_pagetable *destroy_hwpt; struct iommufd_object *pt_obj; @@ -634,7 +637,7 @@ static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id, struct iommufd_hw_pagetable *hwpt = container_of(pt_obj, struct iommufd_hw_pagetable, obj);
- destroy_hwpt = (*do_attach)(idev, hwpt); + destroy_hwpt = (*do_attach)(idev, pasid, hwpt); if (IS_ERR(destroy_hwpt)) goto out_put_pt_obj; break; @@ -643,8 +646,8 @@ static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id, struct iommufd_ioas *ioas = container_of(pt_obj, struct iommufd_ioas, obj);
- destroy_hwpt = iommufd_device_auto_get_domain(idev, ioas, pt_id, - do_attach); + destroy_hwpt = iommufd_device_auto_get_domain(idev, pasid, ioas, + pt_id, do_attach); if (IS_ERR(destroy_hwpt)) goto out_put_pt_obj; break; @@ -681,7 +684,8 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) { int rc;
- rc = iommufd_device_change_pt(idev, pt_id, &iommufd_device_do_attach); + rc = iommufd_device_change_pt(idev, IOMMU_NO_PASID, pt_id, + &iommufd_device_do_attach); if (rc) return rc;
@@ -711,7 +715,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_device_attach, IOMMUFD); */ int iommufd_device_replace(struct iommufd_device *idev, u32 *pt_id) { - return iommufd_device_change_pt(idev, pt_id, + return iommufd_device_change_pt(idev, IOMMU_NO_PASID, pt_id, &iommufd_device_do_replace); } EXPORT_SYMBOL_NS_GPL(iommufd_device_replace, IOMMUFD); @@ -727,7 +731,7 @@ void iommufd_device_detach(struct iommufd_device *idev) { struct iommufd_hw_pagetable *hwpt;
- hwpt = iommufd_hw_pagetable_detach(idev); + hwpt = iommufd_hw_pagetable_detach(idev, IOMMU_NO_PASID); iommufd_hw_pagetable_put(idev->ictx, hwpt); refcount_dec(&idev->obj.users); } diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index 9fa56b3c2b7d..d69849f3ed13 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -56,7 +56,8 @@ static void iommufd_fault_iopf_disable(struct iommufd_device *idev) }
int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) + struct iommufd_device *idev, + ioasid_t pasid) { int ret;
@@ -67,7 +68,7 @@ int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, if (ret) return ret;
- ret = iommufd_dev_attach_handle(hwpt, idev); + ret = iommufd_dev_attach_handle(hwpt, idev, pasid); if (ret) iommufd_fault_iopf_disable(idev);
@@ -104,11 +105,13 @@ static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, }
void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) + struct iommufd_device *idev, + ioasid_t pasid) { struct iommufd_attach_handle *handle;
- handle = iommufd_device_get_attach_handle(idev); + handle = iommufd_device_get_attach_handle(idev, pasid); + WARN_ON(pasid != IOMMU_NO_PASID); iommu_detach_group_handle(hwpt->domain, idev->igroup->group); iommufd_auto_response_faults(hwpt, handle); iommufd_fault_iopf_disable(idev); @@ -116,6 +119,7 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, }
int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, + ioasid_t pasid, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) { @@ -130,7 +134,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, return ret; }
- curr = iommufd_dev_replace_handle(idev, hwpt, old); + curr = iommufd_dev_replace_handle(idev, pasid, hwpt, old); if (IS_ERR(curr)) { if (iopf_on) iommufd_fault_iopf_disable(idev); diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index aefde4443671..9a346c59b5e5 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -180,7 +180,8 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, * sequence. Once those drivers are fixed this should be removed. */ if (immediate_attach) { - rc = iommufd_hw_pagetable_attach(hwpt, idev); + /* Sinc this is just a trick, so passing IOMMU_NO_PASID is enough */ + rc = iommufd_hw_pagetable_attach(hwpt, idev, IOMMU_NO_PASID); if (rc) goto out_abort; } @@ -193,7 +194,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
out_detach: if (immediate_attach) - iommufd_hw_pagetable_detach(idev); + iommufd_hw_pagetable_detach(idev, IOMMU_NO_PASID); out_abort: iommufd_object_abort_and_destroy(ictx, &hwpt->obj); return ERR_PTR(rc); diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 30696936a926..38aa9232f0f5 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -349,9 +349,10 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, bool immediate_attach, const struct iommu_user_data *user_data); int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev); + struct iommufd_device *idev, + ioasid_t pasid); struct iommufd_hw_pagetable * -iommufd_hw_pagetable_detach(struct iommufd_device *idev); +iommufd_hw_pagetable_detach(struct iommufd_device *idev, ioasid_t pasid); void iommufd_hwpt_paging_destroy(struct iommufd_object *obj); void iommufd_hwpt_paging_abort(struct iommufd_object *obj); void iommufd_hwpt_nested_destroy(struct iommufd_object *obj); @@ -459,11 +460,12 @@ struct iommufd_attach_handle { #define to_iommufd_handle(hdl) container_of(hdl, struct iommufd_attach_handle, handle)
static inline struct iommufd_attach_handle * -iommufd_device_get_attach_handle(struct iommufd_device *idev) +iommufd_device_get_attach_handle(struct iommufd_device *idev, ioasid_t pasid) { struct iommu_attach_handle *handle;
- handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0); + WARN_ON(pasid != IOMMU_NO_PASID); + handle = iommu_attach_handle_get(idev->igroup->group, pasid, 0); if (IS_ERR(handle)) return NULL;
@@ -471,7 +473,8 @@ iommufd_device_get_attach_handle(struct iommufd_device *idev) }
static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) + struct iommufd_device *idev, + ioasid_t pasid) { struct iommufd_attach_handle *handle; int ret; @@ -481,6 +484,7 @@ static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt, return -ENOMEM;
handle->idev = idev; + WARN_ON(pasid != IOMMU_NO_PASID); ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, &handle->handle); if (ret) @@ -491,14 +495,14 @@ static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
/* Caller to free the old iommufd_attach_handle */ static inline struct iommufd_attach_handle * -iommufd_dev_replace_handle(struct iommufd_device *idev, +iommufd_dev_replace_handle(struct iommufd_device *idev, ioasid_t pasid, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) { struct iommufd_attach_handle *handle, *curr; int ret;
- curr = iommufd_device_get_attach_handle(idev); + curr = iommufd_device_get_attach_handle(idev, pasid); if (!curr) return ERR_PTR(-EINVAL);
@@ -507,6 +511,7 @@ iommufd_dev_replace_handle(struct iommufd_device *idev, return ERR_PTR(-ENOMEM);
handle->idev = idev; + WARN_ON(pasid != IOMMU_NO_PASID); ret = iommu_replace_group_handle(idev->igroup->group, hwpt->domain, &handle->handle); if (ret) { @@ -530,47 +535,55 @@ void iommufd_fault_destroy(struct iommufd_object *obj); int iommufd_fault_iopf_handler(struct iopf_group *group);
int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev); + struct iommufd_device *idev, + ioasid_t pasid); void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev); + struct iommufd_device *idev, + ioasid_t pasid); int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, + ioasid_t pasid, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old);
static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) + struct iommufd_device *idev, + ioasid_t pasid) { if (hwpt->fault) - return iommufd_fault_domain_attach_dev(hwpt, idev); + return iommufd_fault_domain_attach_dev(hwpt, idev, pasid);
- return iommufd_dev_attach_handle(hwpt, idev); + return iommufd_dev_attach_handle(hwpt, idev, pasid); }
static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) + struct iommufd_device *idev, + ioasid_t pasid) { struct iommufd_attach_handle *handle;
if (hwpt->fault) { - iommufd_fault_domain_detach_dev(hwpt, idev); + iommufd_fault_domain_detach_dev(hwpt, idev, pasid); return; }
- handle = iommufd_device_get_attach_handle(idev); + handle = iommufd_device_get_attach_handle(idev, pasid); + WARN_ON(pasid != IOMMU_NO_PASID); iommu_detach_group_handle(hwpt->domain, idev->igroup->group); kfree(handle); }
static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev, + ioasid_t pasid, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) { struct iommufd_attach_handle *curr;
if (old->fault || hwpt->fault) - return iommufd_fault_domain_replace_dev(idev, hwpt, old); + return iommufd_fault_domain_replace_dev(idev, pasid, + hwpt, old);
- curr = iommufd_dev_replace_handle(idev, hwpt, old); + curr = iommufd_dev_replace_handle(idev, pasid, hwpt, old); if (IS_ERR(curr)) return PTR_ERR(curr);
This introduces three APIs for device drivers to manage pasid attach/ replace/detach.
int iommufd_device_pasid_attach(struct iommufd_device *idev, ioasid_t pasid, u32 *pt_id); int iommufd_device_pasid_replace(struct iommufd_device *idev, ioasid_t pasid, u32 *pt_id); void iommufd_device_pasid_detach(struct iommufd_device *idev, ioasid_t pasid);
pasid operations have different implications when comparing to device operations:
- No connection to iommufd_group since pasid is a device capability and can be enabled only in singleton group;
- no reserved region per pasid otherwise SVA architecture is already broken (CPU address space doesn't count device reserved regions);
- accordingly no sw_msi trick;
- immediated_attach is not supported, expecting that arm-smmu driver will already remove that requirement before supporting this pasid operation. This avoids unnecessary change in iommufd_hw_pagetable_alloc() to carry the pasid from device.c.
With above differences, this puts all pasid related logics into a new pasid.c file.
Cache coherency enforcement is still applied to pasid operations since it is about memory accesses post page table walking (no matter the walk is per RID or per PASID).
Since the attach is per PASID, this introduces a pasid_hwpts xarray to track the per-pasid attach data.
Signed-off-by: Kevin Tian kevin.tian@intel.com Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/Makefile | 1 + drivers/iommu/iommufd/device.c | 12 +- drivers/iommu/iommufd/fault.c | 6 +- drivers/iommu/iommufd/iommufd_private.h | 40 ++++-- drivers/iommu/iommufd/pasid.c | 157 ++++++++++++++++++++++++ include/linux/iommufd.h | 7 ++ 6 files changed, 205 insertions(+), 18 deletions(-) create mode 100644 drivers/iommu/iommufd/pasid.c
diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile index cf4605962bea..d791664ed05b 100644 --- a/drivers/iommu/iommufd/Makefile +++ b/drivers/iommu/iommufd/Makefile @@ -7,6 +7,7 @@ iommufd-y := \ ioas.o \ main.o \ pages.o \ + pasid.o \ vfio_compat.o
iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 953eb2349d59..18f94aa462ea 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -136,6 +136,7 @@ void iommufd_device_destroy(struct iommufd_object *obj) struct iommufd_device *idev = container_of(obj, struct iommufd_device, obj);
+ WARN_ON(!xa_empty(&idev->pasid_hwpts)); iommu_device_release_dma_owner(idev->dev); iommufd_put_group(idev->igroup); if (!iommufd_selftest_is_mock_dev(idev->dev)) @@ -217,6 +218,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx, idev->igroup = igroup; mutex_init(&idev->iopf_lock);
+ xa_init(&idev->pasid_hwpts); + /* * If the caller fails after this success it must call * iommufd_unbind_device() which is safe since we hold this refcount. @@ -533,10 +536,6 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid, return ERR_PTR(rc); }
-typedef struct iommufd_hw_pagetable *(*attach_fn)( - struct iommufd_device *idev, ioasid_t pasid, - struct iommufd_hw_pagetable *hwpt); - /* * When automatically managing the domains we search for a compatible domain in * the iopt and if one is found use it, otherwise create a new domain. @@ -620,9 +619,8 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev, ioasid_t pasid, return destroy_hwpt; }
-static int iommufd_device_change_pt(struct iommufd_device *idev, - ioasid_t pasid, - u32 *pt_id, attach_fn do_attach) +int iommufd_device_change_pt(struct iommufd_device *idev, ioasid_t pasid, + u32 *pt_id, attach_fn do_attach) { struct iommufd_hw_pagetable *destroy_hwpt; struct iommufd_object *pt_obj; diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index d69849f3ed13..5926785f240d 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -111,8 +111,10 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_attach_handle *handle;
handle = iommufd_device_get_attach_handle(idev, pasid); - WARN_ON(pasid != IOMMU_NO_PASID); - iommu_detach_group_handle(hwpt->domain, idev->igroup->group); + if (pasid == IOMMU_NO_PASID) + iommu_detach_group_handle(hwpt->domain, idev->igroup->group); + else + iommu_detach_device_pasid(hwpt->domain, idev->dev, pasid); iommufd_auto_response_faults(hwpt, handle); iommufd_fault_iopf_disable(idev); kfree(handle); diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 38aa9232f0f5..79e77a24ecea 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -398,6 +398,7 @@ struct iommufd_device { struct list_head group_item; /* always the physical device */ struct device *dev; + struct xarray pasid_hwpts; bool enforce_cache_coherency; /* protect iopf_enabled counter */ struct mutex iopf_lock; @@ -415,6 +416,20 @@ iommufd_get_device(struct iommufd_ucmd *ucmd, u32 id) void iommufd_device_destroy(struct iommufd_object *obj); int iommufd_get_hw_info(struct iommufd_ucmd *ucmd);
+typedef struct iommufd_hw_pagetable *(*attach_fn)( + struct iommufd_device *idev, ioasid_t pasid, + struct iommufd_hw_pagetable *hwpt); + +int iommufd_device_change_pt(struct iommufd_device *idev, ioasid_t pasid, + u32 *pt_id, attach_fn do_attach); + +struct iommufd_hw_pagetable * +iommufd_device_pasid_do_attach(struct iommufd_device *idev, ioasid_t pasid, + struct iommufd_hw_pagetable *hwpt); +struct iommufd_hw_pagetable * +iommufd_device_pasid_do_replace(struct iommufd_device *idev, ioasid_t pasid, + struct iommufd_hw_pagetable *hwpt); + struct iommufd_access { struct iommufd_object obj; struct iommufd_ctx *ictx; @@ -464,7 +479,6 @@ iommufd_device_get_attach_handle(struct iommufd_device *idev, ioasid_t pasid) { struct iommu_attach_handle *handle;
- WARN_ON(pasid != IOMMU_NO_PASID); handle = iommu_attach_handle_get(idev->igroup->group, pasid, 0); if (IS_ERR(handle)) return NULL; @@ -484,9 +498,12 @@ static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt, return -ENOMEM;
handle->idev = idev; - WARN_ON(pasid != IOMMU_NO_PASID); - ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, - &handle->handle); + if (pasid == IOMMU_NO_PASID) + ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, + &handle->handle); + else + ret = iommu_attach_device_pasid(hwpt->domain, idev->dev, pasid, + &handle->handle); if (ret) kfree(handle);
@@ -511,9 +528,12 @@ iommufd_dev_replace_handle(struct iommufd_device *idev, ioasid_t pasid, return ERR_PTR(-ENOMEM);
handle->idev = idev; - WARN_ON(pasid != IOMMU_NO_PASID); - ret = iommu_replace_group_handle(idev->igroup->group, - hwpt->domain, &handle->handle); + if (pasid == IOMMU_NO_PASID) + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, &handle->handle); + else + ret = iommu_replace_device_pasid(hwpt->domain, idev->dev, + pasid, &handle->handle); if (ret) { kfree(handle); return ERR_PTR(ret); @@ -567,8 +587,10 @@ static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt, }
handle = iommufd_device_get_attach_handle(idev, pasid); - WARN_ON(pasid != IOMMU_NO_PASID); - iommu_detach_group_handle(hwpt->domain, idev->igroup->group); + if (pasid == IOMMU_NO_PASID) + iommu_detach_group_handle(hwpt->domain, idev->igroup->group); + else + iommu_detach_device_pasid(hwpt->domain, idev->dev, pasid); kfree(handle); }
diff --git a/drivers/iommu/iommufd/pasid.c b/drivers/iommu/iommufd/pasid.c new file mode 100644 index 000000000000..5e8598f1846b --- /dev/null +++ b/drivers/iommu/iommufd/pasid.c @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2024, Intel Corporation + */ +#include <linux/iommufd.h> +#include <linux/iommu.h> +#include "../iommu-priv.h" + +#include "iommufd_private.h" + +struct iommufd_hw_pagetable * +iommufd_device_pasid_do_attach(struct iommufd_device *idev, ioasid_t pasid, + struct iommufd_hw_pagetable *hwpt) +{ + void *curr; + int rc; + + refcount_inc(&hwpt->obj.users); + curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL); + if (curr) { + if (curr == hwpt) + rc = 0; + else + rc = xa_err(curr) ? : -EBUSY; + goto err_put_hwpt; + } + + rc = iommufd_hwpt_attach_device(hwpt, idev, pasid); + if (rc) { + xa_erase(&idev->pasid_hwpts, pasid); + goto err_put_hwpt; + } + + return NULL; + +err_put_hwpt: + refcount_dec(&hwpt->obj.users); + return ERR_PTR(rc); +} + +struct iommufd_hw_pagetable * +iommufd_device_pasid_do_replace(struct iommufd_device *idev, ioasid_t pasid, + struct iommufd_hw_pagetable *hwpt) +{ + void *curr; + int rc; + + refcount_inc(&hwpt->obj.users); + curr = xa_store(&idev->pasid_hwpts, pasid, hwpt, GFP_KERNEL); + rc = xa_err(curr); + if (rc) + goto out_put_hwpt; + + if (!curr) { + xa_erase(&idev->pasid_hwpts, pasid); + rc = -EINVAL; + goto out_put_hwpt; + } + + if (curr == hwpt) + goto out_put_hwpt; + + /* + * After replacement, the reference on the old hwpt is retained + * in this thread as caller would free it. + */ + rc = iommufd_hwpt_replace_device(idev, pasid, hwpt, curr); + if (rc) { + WARN_ON(xa_err(xa_store(&idev->pasid_hwpts, pasid, + curr, GFP_KERNEL))); + goto out_put_hwpt; + } + + /* Caller must destroy old_hwpt */ + return curr; + +out_put_hwpt: + refcount_dec(&hwpt->obj.users); + return ERR_PTR(rc); +} + +/** + * iommufd_device_pasid_attach - Connect a {device, pasid} to an iommu_domain + * @idev: device to attach + * @pasid: pasid to attach + * @pt_id: Input a IOMMUFD_OBJ_IOAS, or IOMMUFD_OBJ_HW_PAGETABLE + * Output the IOMMUFD_OBJ_HW_PAGETABLE ID + * + * This connects a pasid of the device to an iommu_domain. Once this + * completes the device could do DMA with the pasid. + * + * This function is undone by calling iommufd_device_detach_pasid(). + * + * iommufd does not handle race between iommufd_device_pasid_attach(), + * iommufd_device_pasid_replace() and iommufd_device_pasid_detach(). + * So caller of them should guarantee no concurrent call on the same + * device and pasid. + */ +int iommufd_device_pasid_attach(struct iommufd_device *idev, + ioasid_t pasid, u32 *pt_id) +{ + return iommufd_device_change_pt(idev, pasid, pt_id, + &iommufd_device_pasid_do_attach); +} +EXPORT_SYMBOL_NS_GPL(iommufd_device_pasid_attach, IOMMUFD); + +/** + * iommufd_device_pasid_replace - Change the {device, pasid}'s iommu_domain + * @idev: device to change + * @pasid: pasid to change + * @pt_id: Input a IOMMUFD_OBJ_IOAS, or IOMMUFD_OBJ_HW_PAGETABLE + * Output the IOMMUFD_OBJ_HW_PAGETABLE ID + * + * This is the same as + * iommufd_device_pasid_detach(); + * iommufd_device_pasid_attach(); + * + * If it fails then no change is made to the attachment. The iommu driver may + * implement this so there is no disruption in translation. This can only be + * called if iommufd_device_pasid_attach() has already succeeded. + * + * iommufd does not handle race between iommufd_device_pasid_replace(), + * iommufd_device_pasid_attach() and iommufd_device_pasid_detach(). + * So caller of them should guarantee no concurrent call on the same + * device and pasid. + */ +int iommufd_device_pasid_replace(struct iommufd_device *idev, + ioasid_t pasid, u32 *pt_id) +{ + return iommufd_device_change_pt(idev, pasid, pt_id, + &iommufd_device_pasid_do_replace); +} +EXPORT_SYMBOL_NS_GPL(iommufd_device_pasid_replace, IOMMUFD); + +/** + * iommufd_device_pasid_detach - Disconnect a {device, pasid} to an iommu_domain + * @idev: device to detach + * @pasid: pasid to detach + * + * Undo iommufd_device_pasid_attach(). This disconnects the idev/pasid from + * the previously attached pt_id. + * + * iommufd does not handle race between iommufd_device_pasid_detach(), + * iommufd_device_pasid_attach() and iommufd_device_pasid_replace(). + * So caller of them should guarantee no concurrent call on the same + * device and pasid. + */ +void iommufd_device_pasid_detach(struct iommufd_device *idev, ioasid_t pasid) +{ + struct iommufd_hw_pagetable *hwpt; + + hwpt = xa_erase(&idev->pasid_hwpts, pasid); + if (WARN_ON(!hwpt)) + return; + iommufd_hwpt_detach_device(hwpt, idev, pasid); + iommufd_hw_pagetable_put(idev->ictx, hwpt); +} +EXPORT_SYMBOL_NS_GPL(iommufd_device_pasid_detach, IOMMUFD); diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index ffc3a949f837..5b79bf514f5d 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -9,6 +9,7 @@ #include <linux/types.h> #include <linux/errno.h> #include <linux/err.h> +#include <linux/iommu.h>
struct device; struct iommufd_device; @@ -26,6 +27,12 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id); int iommufd_device_replace(struct iommufd_device *idev, u32 *pt_id); void iommufd_device_detach(struct iommufd_device *idev);
+int iommufd_device_pasid_attach(struct iommufd_device *idev, + ioasid_t pasid, u32 *pt_id); +int iommufd_device_pasid_replace(struct iommufd_device *idev, + ioasid_t pasid, u32 *pt_id); +void iommufd_device_pasid_detach(struct iommufd_device *idev, ioasid_t pasid); + struct iommufd_ctx *iommufd_device_to_ictx(struct iommufd_device *idev); u32 iommufd_device_to_id(struct iommufd_device *idev);
The two callbacks are needed to make pasid_attach/detach path complete for mock device. A nop is enough for set_dev_pasid, a domain type check in the remove_dev_pasid is also helpful.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/selftest.c | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 222cfc11ebfd..7fd8dc5a667f 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -539,6 +539,30 @@ static int mock_dev_disable_feat(struct device *dev, enum iommu_dev_features fea return 0; }
+static void mock_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + /* Domain type specific cleanup: */ + if (domain) { + switch (domain->type) { + case IOMMU_DOMAIN_NESTED: + case IOMMU_DOMAIN_UNMANAGED: + break; + default: + /* should never reach here */ + WARN_ON(1); + break; + } + } +} + +static int mock_domain_set_dev_pasid_nop(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) +{ + return 0; +} + static const struct iommu_ops mock_ops = { /* * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type() @@ -558,6 +582,7 @@ static const struct iommu_ops mock_ops = { .dev_enable_feat = mock_dev_enable_feat, .dev_disable_feat = mock_dev_disable_feat, .user_pasid_table = true, + .remove_dev_pasid = mock_iommu_remove_dev_pasid, .default_domain_ops = &(struct iommu_domain_ops){ .free = mock_domain_free, @@ -565,6 +590,7 @@ static const struct iommu_ops mock_ops = { .map_pages = mock_domain_map_pages, .unmap_pages = mock_domain_unmap_pages, .iova_to_phys = mock_domain_iova_to_phys, + .set_dev_pasid = mock_domain_set_dev_pasid_nop, }, };
@@ -629,6 +655,7 @@ static struct iommu_domain_ops domain_nested_ops = { .free = mock_domain_free_nested, .attach_dev = mock_domain_nop_attach, .cache_invalidate_user = mock_domain_cache_invalidate_user, + .set_dev_pasid = mock_domain_set_dev_pasid_nop, };
static inline struct iommufd_hw_pagetable * @@ -689,6 +716,10 @@ static void mock_dev_release(struct device *dev)
static struct mock_dev *mock_dev_create(unsigned long dev_flags) { + struct property_entry prop[] = { + PROPERTY_ENTRY_U32("pasid-num-bits", 20), + {}, + }; struct mock_dev *mdev; int rc;
@@ -714,6 +745,12 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags) if (rc) goto err_put;
+ rc = device_create_managed_software_node(&mdev->dev, prop, NULL); + if (rc) { + dev_err(&mdev->dev, "add pasid-num-bits property failed, rc: %d", rc); + goto err_put; + } + rc = device_add(&mdev->dev); if (rc) goto err_put; @@ -1549,6 +1586,7 @@ int __init iommufd_test_init(void) goto err_sysfs;
mock_iommu_iopf_queue = iopf_queue_alloc("mock-iopfq"); + mock_iommu_device.max_pasids = (1 << 20);
return 0;
There is need to get the selftest device (sobj->type == TYPE_IDEV) in multiple places, so have a helper to for it.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/selftest.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 7fd8dc5a667f..b7e0cbb46f9f 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -831,29 +831,39 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd, return rc; }
-/* Replace the mock domain with a manually allocated hw_pagetable */ -static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd, - unsigned int device_id, u32 pt_id, - struct iommu_test_cmd *cmd) +static struct selftest_obj * +iommufd_test_get_self_test_device(struct iommufd_ctx *ictx, u32 id) { struct iommufd_object *dev_obj; struct selftest_obj *sobj; - int rc;
/* * Prefer to use the OBJ_SELFTEST because the destroy_rwsem will ensure * it doesn't race with detach, which is not allowed. */ - dev_obj = - iommufd_get_object(ucmd->ictx, device_id, IOMMUFD_OBJ_SELFTEST); + dev_obj = iommufd_get_object(ictx, id, IOMMUFD_OBJ_SELFTEST); if (IS_ERR(dev_obj)) - return PTR_ERR(dev_obj); + return ERR_CAST(dev_obj);
sobj = container_of(dev_obj, struct selftest_obj, obj); if (sobj->type != TYPE_IDEV) { - rc = -EINVAL; - goto out_dev_obj; + iommufd_put_object(ictx, dev_obj); + return ERR_PTR(-EINVAL); } + return sobj; +} + +/* Replace the mock domain with a manually allocated hw_pagetable */ +static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd, + unsigned int device_id, u32 pt_id, + struct iommu_test_cmd *cmd) +{ + struct selftest_obj *sobj; + int rc; + + sobj = iommufd_test_get_self_test_device(ucmd->ictx, device_id); + if (IS_ERR(sobj)) + return PTR_ERR(sobj);
rc = iommufd_device_replace(sobj->idev.idev, &pt_id); if (rc) @@ -863,7 +873,7 @@ static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd, rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
out_dev_obj: - iommufd_put_object(ucmd->ictx, dev_obj); + iommufd_put_object(ucmd->ictx, &sobj->obj); return rc; }
This adds 4 test ops for pasid attach/replace/detach testing. There are ops to attach/detach pasid, and also op to check the attached domain of a pasid.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/iommufd/iommufd_test.h | 30 ++++++ drivers/iommu/iommufd/selftest.c | 138 +++++++++++++++++++++++++++ 2 files changed, 168 insertions(+)
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h index acbbba1c6671..54f59141cc59 100644 --- a/drivers/iommu/iommufd/iommufd_test.h +++ b/drivers/iommu/iommufd/iommufd_test.h @@ -23,6 +23,10 @@ enum { IOMMU_TEST_OP_DIRTY, IOMMU_TEST_OP_MD_CHECK_IOTLB, IOMMU_TEST_OP_TRIGGER_IOPF, + IOMMU_TEST_OP_PASID_ATTACH, + IOMMU_TEST_OP_PASID_REPLACE, + IOMMU_TEST_OP_PASID_DETACH, + IOMMU_TEST_OP_PASID_CHECK_DOMAIN, };
enum { @@ -135,6 +139,32 @@ struct iommu_test_cmd { __u32 perm; __u64 addr; } trigger_iopf; + struct { + __u32 pasid; + __u32 pt_id; + /* @id is stdev_id for IOMMU_TEST_OP_PASID_ATTACH + * pasid#1024 is for special test, avoid use it + * in normal case. + */ + } pasid_attach; + struct { + __u32 pasid; + __u32 pt_id; + /* @id is stdev_id for IOMMU_TEST_OP_PASID_ATTACH + * pasid#1024 is for special test, avoid use it + * in normal case. + */ + } pasid_replace; + struct { + __u32 pasid; + /* @id is stdev_id for IOMMU_TEST_OP_PASID_DETACH */ + } pasid_detach; + struct { + __u32 pasid; + __u32 hwpt_id; + __u64 out_result_ptr; + /* @id is stdev_id for IOMMU_TEST_OP_HWPT_GET_DOMAIN */ + } pasid_check; }; __u32 last; }; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index b7e0cbb46f9f..421025d57626 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -539,6 +539,8 @@ static int mock_dev_disable_feat(struct device *dev, enum iommu_dev_features fea return 0; }
+static bool pasid_1024_attached; + static void mock_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct iommu_domain *domain) { @@ -547,6 +549,8 @@ static void mock_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, switch (domain->type) { case IOMMU_DOMAIN_NESTED: case IOMMU_DOMAIN_UNMANAGED: + if (pasid == 1024) + pasid_1024_attached = false; break; default: /* should never reach here */ @@ -560,6 +564,20 @@ static int mock_domain_set_dev_pasid_nop(struct iommu_domain *domain, struct device *dev, ioasid_t pasid, struct iommu_domain *old) { + /* + * First attach with pasid 1024 succ, second attach would fail, + * and so on. This is helpful to test the case in which the iommu + * layer needs to rollback to old domain due to driver failure. + */ + if (pasid == 1024) { + if (pasid_1024_attached) { + pasid_1024_attached = false; + // Fake an error to fail the replacement + return -ENOMEM; + } + pasid_1024_attached = true; + } + return 0; }
@@ -1476,6 +1494,117 @@ static int iommufd_test_trigger_iopf(struct iommufd_ucmd *ucmd, return 0; }
+static int iommufd_test_pasid_attach(struct iommufd_ucmd *ucmd, + struct iommu_test_cmd *cmd) +{ + struct selftest_obj *sobj; + int rc; + + sobj = iommufd_test_get_self_test_device(ucmd->ictx, cmd->id); + if (IS_ERR(sobj)) + return PTR_ERR(sobj); + + rc = iommufd_device_pasid_attach(sobj->idev.idev, + cmd->pasid_attach.pasid, + &cmd->pasid_attach.pt_id); + iommufd_put_object(ucmd->ictx, &sobj->obj); + return rc; +} + +static int iommufd_test_pasid_replace(struct iommufd_ucmd *ucmd, + struct iommu_test_cmd *cmd) +{ + struct selftest_obj *sobj; + int rc; + + sobj = iommufd_test_get_self_test_device(ucmd->ictx, cmd->id); + if (IS_ERR(sobj)) + return PTR_ERR(sobj); + + rc = iommufd_device_pasid_replace(sobj->idev.idev, + cmd->pasid_attach.pasid, + &cmd->pasid_attach.pt_id); + iommufd_put_object(ucmd->ictx, &sobj->obj); + return rc; +} + +static int iommufd_test_pasid_detach(struct iommufd_ucmd *ucmd, + struct iommu_test_cmd *cmd) +{ + struct selftest_obj *sobj; + + sobj = iommufd_test_get_self_test_device(ucmd->ictx, cmd->id); + if (IS_ERR(sobj)) + return PTR_ERR(sobj); + + iommufd_device_pasid_detach(sobj->idev.idev, + cmd->pasid_detach.pasid); + iommufd_put_object(ucmd->ictx, &sobj->obj); + return 0; +} + +static inline struct iommufd_hw_pagetable * +iommufd_get_hwpt(struct iommufd_ucmd *ucmd, u32 id) +{ + struct iommufd_object *pt_obj; + + pt_obj = iommufd_get_object(ucmd->ictx, id, IOMMUFD_OBJ_ANY); + if (IS_ERR(pt_obj)) + return ERR_CAST(pt_obj); + + if (pt_obj->type != IOMMUFD_OBJ_HWPT_NESTED && + pt_obj->type != IOMMUFD_OBJ_HWPT_PAGING) { + iommufd_put_object(ucmd->ictx, pt_obj); + return ERR_PTR(-EINVAL); + } + + return container_of(pt_obj, struct iommufd_hw_pagetable, obj); +} + +static int iommufd_test_pasid_check_domain(struct iommufd_ucmd *ucmd, + struct iommu_test_cmd *cmd) +{ + struct iommu_domain *attached_domain, *expect_domain = NULL; + struct iommufd_hw_pagetable *hwpt = NULL; + struct iommu_attach_handle *handle; + struct selftest_obj *sobj; + struct mock_dev *mdev; + bool result; + int rc = 0; + + sobj = iommufd_test_get_self_test_device(ucmd->ictx, cmd->id); + if (IS_ERR(sobj)) + return PTR_ERR(sobj); + + mdev = sobj->idev.mock_dev; + + handle = iommu_attach_handle_get(mdev->dev.iommu_group, + cmd->pasid_check.pasid, 0); + if (IS_ERR(handle)) + attached_domain = NULL; + else + attached_domain = handle->domain; + + if (cmd->pasid_check.hwpt_id) { + hwpt = iommufd_get_hwpt(ucmd, cmd->pasid_check.hwpt_id); + if (IS_ERR(hwpt)) { + rc = PTR_ERR(hwpt); + goto out_put_dev; + } + expect_domain = hwpt->domain; + } + + result = (attached_domain == expect_domain) ? 1 : 0; + if (copy_to_user(u64_to_user_ptr(cmd->pasid_check.out_result_ptr), + &result, sizeof(result))) + rc = -EFAULT; + if (hwpt) + iommufd_put_object(ucmd->ictx, &hwpt->obj); +out_put_dev: + iommufd_put_object(ucmd->ictx, &sobj->obj); + return rc; +} + void iommufd_selftest_destroy(struct iommufd_object *obj) { struct selftest_obj *sobj = container_of(obj, struct selftest_obj, obj); @@ -1553,6 +1682,14 @@ int iommufd_test(struct iommufd_ucmd *ucmd) cmd->dirty.flags); case IOMMU_TEST_OP_TRIGGER_IOPF: return iommufd_test_trigger_iopf(ucmd, cmd); + case IOMMU_TEST_OP_PASID_ATTACH: + return iommufd_test_pasid_attach(ucmd, cmd); + case IOMMU_TEST_OP_PASID_REPLACE: + return iommufd_test_pasid_replace(ucmd, cmd); + case IOMMU_TEST_OP_PASID_DETACH: + return iommufd_test_pasid_detach(ucmd, cmd); + case IOMMU_TEST_OP_PASID_CHECK_DOMAIN: + return iommufd_test_pasid_check_domain(ucmd, cmd); default: return -EOPNOTSUPP; } @@ -1597,6 +1734,7 @@ int __init iommufd_test_init(void)
mock_iommu_iopf_queue = iopf_queue_alloc("mock-iopfq"); mock_iommu_device.max_pasids = (1 << 20); + pasid_1024_attached = false;
return 0;
This tests iommufd pasid attach/replace/detach.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- tools/testing/selftests/iommu/iommufd.c | 256 ++++++++++++++++++ .../selftests/iommu/iommufd_fail_nth.c | 29 +- tools/testing/selftests/iommu/iommufd_utils.h | 78 ++++++ 3 files changed, 359 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index 4927b9add5ad..98f5beecca1b 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -2386,4 +2386,260 @@ TEST_F(vfio_compat_mock_domain, huge_map) } }
+FIXTURE(iommufd_device_pasid) +{ + int fd; + uint32_t ioas_id; + uint32_t hwpt_id; + uint32_t stdev_id; + uint32_t device_id; +}; + +FIXTURE_SETUP(iommufd_device_pasid) +{ + self->fd = open("/dev/iommu", O_RDWR); + ASSERT_NE(-1, self->fd); + test_ioctl_ioas_alloc(&self->ioas_id); + + test_cmd_mock_domain(self->ioas_id, &self->stdev_id, + &self->hwpt_id, &self->device_id); +} + +FIXTURE_TEARDOWN(iommufd_device_pasid) +{ + teardown_iommufd(self->fd, _metadata); +} + +TEST_F(iommufd_device_pasid, pasid_attach) +{ + if (self->device_id) { + struct iommu_hwpt_selftest data = { + .iotlb = IOMMU_TEST_IOTLB_DEFAULT, + }; + uint32_t nested_hwpt_id[2] = {}; + uint32_t parent_hwpt_id = 0; + uint32_t fault_id, fault_fd; + uint32_t iopf_hwpt_id; + uint32_t pasid = 100; + bool result; + + /* Allocate two nested hwpts sharing one common parent hwpt */ + test_cmd_hwpt_alloc(self->device_id, self->ioas_id, + IOMMU_HWPT_ALLOC_NEST_PARENT, + &parent_hwpt_id); + + test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id, 0, + &nested_hwpt_id[0], + IOMMU_HWPT_DATA_SELFTEST, + &data, sizeof(data)); + test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id, 0, + &nested_hwpt_id[1], + IOMMU_HWPT_DATA_SELFTEST, + &data, sizeof(data)); + + test_ioctl_fault_alloc(&fault_id, &fault_fd); + test_cmd_hwpt_alloc_iopf(self->device_id, parent_hwpt_id, fault_id, + IOMMU_HWPT_FAULT_ID_VALID, &iopf_hwpt_id, + IOMMU_HWPT_DATA_SELFTEST, &data, + sizeof(data)); + /* + * Attach ioas to pasid 100, should succeed, domain should + * be valid. + */ + test_cmd_pasid_attach(pasid, self->ioas_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, self->hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Try attach pasid 100 with self->ioas_id, should succeed + * as it is the same with existing hwpt. + */ + test_cmd_pasid_attach(pasid, self->ioas_id); + + /* + * Try attach pasid 100 with another hwpt, should FAIL + * as attach does not allow overwrite, use REPLACE instead. + */ + test_err_cmd_pasid_attach(EBUSY, pasid, nested_hwpt_id[0]); + + /* + * Detach hwpt from pasid 100, and check if the pasid 100 + * has null domain. Should be done before the next attach. + */ + test_cmd_pasid_detach(pasid); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, 0, &result)); + EXPECT_EQ(1, result); + + /* + * Attach nested hwpt to pasid 100, should succeed, domain + * should be valid. + */ + test_cmd_pasid_attach(pasid, nested_hwpt_id[0]); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, nested_hwpt_id[0], + &result)); + EXPECT_EQ(1, result); + + /* + * Detach hwpt from pasid 100, and check if the pasid 100 + * has null domain + */ + test_cmd_pasid_detach(pasid); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, 0, &result)); + EXPECT_EQ(1, result); + + /* Replace tests */ + pasid = 200; + + /* + * Replace pasid 200 without attaching it first, should + * fail with -EINVAL. + */ + test_err_cmd_pasid_replace(EINVAL, pasid, parent_hwpt_id); + + /* + * Attach a s2 hwpt to pasid 200, should succeed, domain should + * be valid. + */ + test_cmd_pasid_attach(pasid, parent_hwpt_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, parent_hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Replace pasid 200 with self->ioas_id, should succeed, + * and have valid domain. + */ + test_cmd_pasid_replace(pasid, self->ioas_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, self->hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Replace a nested hwpt for pasid 200, should succeed, + * and have valid domain. + */ + test_cmd_pasid_replace(pasid, nested_hwpt_id[0]); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, nested_hwpt_id[0], + &result)); + EXPECT_EQ(1, result); + + /* + * Replace with another nested hwpt for pasid 200, should + * succeed, and have valid domain. + */ + test_cmd_pasid_replace(pasid, nested_hwpt_id[1]); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, nested_hwpt_id[1], + &result)); + EXPECT_EQ(1, result); + + /* + * Detach hwpt from pasid 200, and check if the pasid 200 + * has null domain. + */ + test_cmd_pasid_detach(pasid); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, 0, &result)); + EXPECT_EQ(1, result); + + /* Negative Tests for pasid replace, use pasid 1024 */ + + /* + * Attach a s2 hwpt to pasid 1024, should succeed, domain should + * be valid. + */ + pasid = 1024; + test_cmd_pasid_attach(pasid, parent_hwpt_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, parent_hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Replace pasid 1024 with self->ioas_id, should fail, + * but have the old valid domain. This is a designed + * negative case, normally replace with self->ioas_id + * could succeed. + */ + test_err_cmd_pasid_replace(ENOMEM, pasid, self->ioas_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, parent_hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Detach hwpt from pasid 1024, and check if the pasid 1024 + * has null domain. + */ + test_cmd_pasid_detach(pasid); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, 0, &result)); + EXPECT_EQ(1, result); + + /* Attach to iopf-capable hwpt */ + + /* + * Attach an iopf hwpt to pasid 2048, should succeed, domain should + * be valid. + */ + pasid = 2048; + test_cmd_pasid_attach(pasid, iopf_hwpt_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, iopf_hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Replace with parent_hwpt_id for pasid 2048, should + * succeed, and have valid domain. + */ + test_cmd_pasid_replace(pasid, parent_hwpt_id); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, parent_hwpt_id, + &result)); + EXPECT_EQ(1, result); + + /* + * Detach hwpt from pasid 2048, and check if the pasid 2048 + * has null domain. + */ + test_cmd_pasid_detach(pasid); + ASSERT_EQ(0, + test_cmd_pasid_check_domain(self->fd, self->stdev_id, + pasid, 0, &result)); + EXPECT_EQ(1, result); + + /* Attach to iopf-capable hwpt */ + test_ioctl_destroy(iopf_hwpt_id); + close(fault_fd); + test_ioctl_destroy(fault_id); + + test_ioctl_destroy(nested_hwpt_id[0]); + test_ioctl_destroy(nested_hwpt_id[1]); + test_ioctl_destroy(parent_hwpt_id); + } +} + TEST_HARNESS_MAIN diff --git a/tools/testing/selftests/iommu/iommufd_fail_nth.c b/tools/testing/selftests/iommu/iommufd_fail_nth.c index c5d5e69452b0..749d3fe6b0fe 100644 --- a/tools/testing/selftests/iommu/iommufd_fail_nth.c +++ b/tools/testing/selftests/iommu/iommufd_fail_nth.c @@ -206,12 +206,16 @@ FIXTURE(basic_fail_nth) { int fd; uint32_t access_id; + uint32_t stdev_id; + uint32_t pasid; };
FIXTURE_SETUP(basic_fail_nth) { self->fd = -1; self->access_id = 0; + self->stdev_id = 0; + self->pasid = 0; //test should use a non-zero value }
FIXTURE_TEARDOWN(basic_fail_nth) @@ -223,6 +227,8 @@ FIXTURE_TEARDOWN(basic_fail_nth) rc = _test_cmd_destroy_access(self->access_id); assert(rc == 0); } + if (self->pasid && self->stdev_id) + _test_cmd_pasid_detach(self->fd, self->stdev_id, self->pasid); teardown_iommufd(self->fd, _metadata); }
@@ -579,7 +585,6 @@ TEST_FAIL_NTH(basic_fail_nth, device) struct iommu_test_hw_info info; uint32_t ioas_id; uint32_t ioas_id2; - uint32_t stdev_id; uint32_t idev_id; uint32_t hwpt_id; __u64 iova; @@ -608,7 +613,7 @@ TEST_FAIL_NTH(basic_fail_nth, device)
fail_nth_enable();
- if (_test_cmd_mock_domain(self->fd, ioas_id, &stdev_id, NULL, + if (_test_cmd_mock_domain(self->fd, ioas_id, &self->stdev_id, NULL, &idev_id)) return -1;
@@ -619,11 +624,27 @@ TEST_FAIL_NTH(basic_fail_nth, device) IOMMU_HWPT_DATA_NONE, 0, 0)) return -1;
- if (_test_cmd_mock_domain_replace(self->fd, stdev_id, ioas_id2, NULL)) + if (_test_cmd_mock_domain_replace(self->fd, self->stdev_id, ioas_id2, NULL)) + return -1; + + if (_test_cmd_mock_domain_replace(self->fd, self->stdev_id, hwpt_id, NULL)) return -1;
- if (_test_cmd_mock_domain_replace(self->fd, stdev_id, hwpt_id, NULL)) + self->pasid = 200; + + /* Tests for pasid attach/replace/detach */ + if (_test_cmd_pasid_attach(self->fd, self->stdev_id, self->pasid, ioas_id)) { + self->pasid = 0; return -1; + } + + _test_cmd_pasid_replace(self->fd, self->stdev_id, self->pasid, ioas_id2); + + if (_test_cmd_pasid_detach(self->fd, self->stdev_id, self->pasid)) + return -1; + + self->pasid = 0; + return 0; }
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h index 40f6f14ce136..399e023993fc 100644 --- a/tools/testing/selftests/iommu/iommufd_utils.h +++ b/tools/testing/selftests/iommu/iommufd_utils.h @@ -762,3 +762,81 @@ static int _test_cmd_trigger_iopf(int fd, __u32 device_id, __u32 fault_fd)
#define test_cmd_trigger_iopf(device_id, fault_fd) \ ASSERT_EQ(0, _test_cmd_trigger_iopf(self->fd, device_id, fault_fd)) + +static int _test_cmd_pasid_attach(int fd, __u32 stdev_id, __u32 pasid, __u32 pt_id) +{ + struct iommu_test_cmd test_attach = { + .size = sizeof(test_attach), + .op = IOMMU_TEST_OP_PASID_ATTACH, + .id = stdev_id, + .pasid_attach = { + .pasid = pasid, + .pt_id = pt_id, + }, + }; + + return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_ATTACH), &test_attach); +} + +#define test_cmd_pasid_attach(pasid, hwpt_id) \ + ASSERT_EQ(0, _test_cmd_pasid_attach(self->fd, self->stdev_id, pasid, hwpt_id)) + +#define test_err_cmd_pasid_attach(_errno, pasid, hwpt_id) \ + EXPECT_ERRNO(_errno, \ + _test_cmd_pasid_attach(self->fd, self->stdev_id, pasid, hwpt_id)) + +static int _test_cmd_pasid_replace(int fd, __u32 stdev_id, __u32 pasid, __u32 pt_id) +{ + struct iommu_test_cmd test_replace = { + .size = sizeof(test_replace), + .op = IOMMU_TEST_OP_PASID_REPLACE, + .id = stdev_id, + .pasid_replace = { + .pasid = pasid, + .pt_id = pt_id, + }, + }; + + return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_REPLACE), &test_replace); +} + +#define test_cmd_pasid_replace(pasid, hwpt_id) \ + ASSERT_EQ(0, _test_cmd_pasid_replace(self->fd, self->stdev_id, pasid, hwpt_id)) + +#define test_err_cmd_pasid_replace(_errno, pasid, hwpt_id) \ + EXPECT_ERRNO(_errno, \ + _test_cmd_pasid_replace(self->fd, self->stdev_id, pasid, hwpt_id)) + +static int _test_cmd_pasid_detach(int fd, __u32 stdev_id, __u32 pasid) +{ + struct iommu_test_cmd test_detach = { + .size = sizeof(test_detach), + .op = IOMMU_TEST_OP_PASID_DETACH, + .id = stdev_id, + .pasid_detach = { + .pasid = pasid, + }, + }; + + return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_DETACH), &test_detach); +} + +#define test_cmd_pasid_detach(pasid) \ + ASSERT_EQ(0, _test_cmd_pasid_detach(self->fd, self->stdev_id, pasid)) + +static int test_cmd_pasid_check_domain(int fd, __u32 stdev_id, __u32 pasid, + __u32 hwpt_id, bool *result) +{ + struct iommu_test_cmd test_pasid_check = { + .size = sizeof(test_pasid_check), + .op = IOMMU_TEST_OP_PASID_CHECK_DOMAIN, + .id = stdev_id, + .pasid_check = { + .pasid = pasid, + .hwpt_id = hwpt_id, + .out_result_ptr = (__u64)result, + }, + }; + + return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_CHECK_DOMAIN), &test_pasid_check); +}
linux-kselftest-mirror@lists.linaro.org
-
Baolu Lu -
Tian, Kevin -
Yi Liu