[PATCH v5 00/12] iommufd support pasid attach/replace

[PATCH v5 00/12] iommufd support pasid attach/replace

[Yi Liu]

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.

The completed code can be found in the below link [3]. Heads up! The existing
iommufd selftest was broken, there was a fix [4] 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/20241104131842.13303-1-yi.l.liu@intel.com/
[2] https://lore.kernel.org/linux-iommu/20241104132033.14027-1-yi.l.liu@intel.com/
[3] https://github.com/yiliu1765/iommufd/tree/iommufd_pasid
[4] https://lore.kernel.org/linux-iommu/20240111073213.180020-1-baolu.lu@linux.intel.com/

Change log:

v5:
 - Fix a mistake in patch 02 of v4 (Kevin)
 - Move the iommufd_handle helpers to device.c
 - Add IOMMU_HWPT_ALLOC_PASID check to enforce pasid-compatible domain for pasid
   capable device in iommufd
 - Update the iommufd selftest to use IOMMU_HWPT_ALLOC_PASID

v4: https://lore.kernel.org/linux-iommu/20240912131255.13305-1-yi.l.liu@intel.com/
 - 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@BN9PR11MB5276.namprd11.prod.outlook.com/
   *) 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.com/
 - 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.com/

Regards,
	Yi Liu

Yi Liu (12):
  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 device.c
  iommufd: Always pass iommu_attach_handle to iommu core
  iommufd: Pass pasid through the device attach/replace path
  iommufd: Support pasid attach/replace
  iommufd: Allocate auto_domain with IOMMU_HWPT_ALLOC_PASID flag if
    device is PASID-capable
  iommufd: Enforce pasid compatible domain for PASID-capable device
  iommufd/selftest: Add set_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/intel/iommu.c                   |   6 +-
 drivers/iommu/iommu-priv.h                    |   4 +
 drivers/iommu/iommu.c                         |  90 +++++-
 drivers/iommu/iommufd/Makefile                |   1 +
 drivers/iommu/iommufd/device.c                | 111 +++++--
 drivers/iommu/iommufd/fault.c                 |  88 ++----
 drivers/iommu/iommufd/hw_pagetable.c          |  12 +-
 drivers/iommu/iommufd/iommufd_private.h       |  81 ++++-
 drivers/iommu/iommufd/iommufd_test.h          |  31 ++
 drivers/iommu/iommufd/pasid.c                 | 157 ++++++++++
 drivers/iommu/iommufd/selftest.c              | 210 ++++++++++++-
 include/linux/iommufd.h                       |   7 +
 tools/testing/selftests/iommu/iommufd.c       | 277 ++++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  35 ++-
 tools/testing/selftests/iommu/iommufd_utils.h |  78 +++++
 15 files changed, 1054 insertions(+), 134 deletions(-)
 create mode 100644 drivers/iommu/iommufd/pasid.c

-- 
2.34.1

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

[Yi Liu]

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 53f8e60acf30..a441ba0a6733 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3411,14 +3411,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;
 	}
@@ -3430,7 +3431,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;
 }
@@ -3492,7 +3506,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:
@@ -3501,6 +3515,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 functional
+	 * impact. The PRI path would eventually become consistent when the
+	 * replacement is done.
+	 */
+	curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
+						      pasid, handle,
+						      GFP_KERNEL);
+	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.
-- 
2.34.1

[PATCH v5 02/12] iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle()

[Yi Liu]

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 | 50 ++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index e590973ce5cf..230d37c17102 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -146,33 +146,23 @@ 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)
+static int
+__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;
 	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;
-
-		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 -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 ret;
 }
@@ -183,6 +173,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;
 	int ret;
 
 	if (iopf_on) {
@@ -191,13 +182,24 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 			return ret;
 	}
 
-	ret = __fault_domain_replace_dev(idev, hwpt, old);
+	curr = iommufd_device_get_attach_handle(idev);
+
+	if (hwpt->fault)
+		ret = __fault_domain_replace_dev(idev, hwpt, old);
+	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);
 
-- 
2.34.1

[PATCH v5 03/12] iommufd: Move the iommufd_handle helpers to device.c

[Yi Liu]

The iommu_attach_handle is now only passed when attaching iopf-capable
domain, while it is not convenient for the iommu core to track the
attached domain of pasids. To address it, the iommu_attach_handle will
be passed to iommu core for non-fault-able domain as well. Hence the
iommufd_handle related helpers are no longer fault specific, it makes
more sense to move it out of fault.c.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c          | 51 ++++++++++++++++++++++
 drivers/iommu/iommufd/fault.c           | 56 +------------------------
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++
 3 files changed, 61 insertions(+), 54 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 5fd3dd420290..823c81145214 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -293,6 +293,57 @@ u32 iommufd_device_to_id(struct iommufd_device *idev)
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD);
 
+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);
+}
+
+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;
+}
+
+int iommufd_dev_replace_handle(struct iommufd_device *idev,
+			       struct iommufd_hw_pagetable *hwpt,
+			       struct iommufd_hw_pagetable *old)
+{
+	struct iommufd_attach_handle *handle;
+	int ret;
+
+	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+	if (!handle)
+		return -ENOMEM;
+
+	handle->idev = idev;
+	ret = iommu_replace_group_handle(idev->igroup->group,
+					 hwpt->domain, &handle->handle);
+	if (ret)
+		kfree(handle);
+
+	return ret;
+}
+
 static int iommufd_group_setup_msi(struct iommufd_group *igroup,
 				   struct iommufd_hwpt_paging *hwpt_paging)
 {
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 230d37c17102..add94b044dc6 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,27 +115,6 @@ 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)
-{
-	struct iommufd_attach_handle *handle;
-	int ret;
-
-	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (!handle)
-		return -ENOMEM;
-
-	handle->idev = idev;
-	ret = iommu_replace_group_handle(idev->igroup->group,
-					 hwpt->domain, &handle->handle);
-	if (ret)
-		kfree(handle);
-
-	return ret;
-}
-
 int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 				     struct iommufd_hw_pagetable *hwpt,
 				     struct iommufd_hw_pagetable *old)
@@ -185,7 +133,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 	curr = iommufd_device_get_attach_handle(idev);
 
 	if (hwpt->fault)
-		ret = __fault_domain_replace_dev(idev, hwpt, old);
+		ret = iommufd_dev_replace_handle(idev, hwpt, old);
 	else
 		ret = iommu_replace_group_handle(idev->igroup->group,
 						 hwpt->domain, NULL);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index f1d865e6fab6..66eb95063068 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -478,6 +478,14 @@ 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)
 
+struct iommufd_attach_handle *
+iommufd_device_get_attach_handle(struct iommufd_device *idev);
+int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
+			      struct iommufd_device *idev);
+int iommufd_dev_replace_handle(struct iommufd_device *idev,
+			       struct iommufd_hw_pagetable *hwpt,
+			       struct iommufd_hw_pagetable *old);
+
 static inline struct iommufd_fault *
 iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id)
 {
-- 
2.34.1

[PATCH v5 04/12] iommufd: Always pass iommu_attach_handle to iommu core

[Yi Liu]

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           | 12 ++++--------
 drivers/iommu/iommufd/iommufd_private.h | 20 +++++++++++++++++---
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index add94b044dc6..55418a067869 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -132,21 +132,17 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 
 	curr = iommufd_device_get_attach_handle(idev);
 
-	if (hwpt->fault)
-		ret = iommufd_dev_replace_handle(idev, hwpt, old);
-	else
-		ret = iommu_replace_group_handle(idev->igroup->group,
-						 hwpt->domain, NULL);
+	ret = iommufd_dev_replace_handle(idev, hwpt, old);
 	if (ret) {
 		if (iopf_on)
 			iommufd_fault_iopf_disable(idev);
 		return ret;
 	}
 
-	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 66eb95063068..19870b08056e 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -512,28 +512,42 @@ 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;
+	int ret;
+
 	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_device_get_attach_handle(idev);
+
+	ret = iommufd_dev_replace_handle(idev, hwpt, old);
+	if (ret)
+		return ret;
+
+	kfree(curr);
+	return 0;
 }
 
 #ifdef CONFIG_IOMMUFD_TEST
-- 
2.34.1

[PATCH v5 05/12] iommufd: Pass pasid through the device attach/replace path

[Yi Liu]

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          | 55 ++++++++++++++-----------
 drivers/iommu/iommufd/fault.c           | 16 ++++---
 drivers/iommu/iommufd/hw_pagetable.c    |  5 ++-
 drivers/iommu/iommufd/iommufd_private.h | 41 +++++++++++-------
 4 files changed, 71 insertions(+), 46 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 823c81145214..0b3f2094af4a 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -294,11 +294,12 @@ u32 iommufd_device_to_id(struct iommufd_device *idev)
 EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD);
 
 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;
 
@@ -306,7 +307,8 @@ iommufd_device_get_attach_handle(struct iommufd_device *idev)
 }
 
 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;
@@ -316,6 +318,7 @@ 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)
@@ -325,6 +328,7 @@ int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
 }
 
 int iommufd_dev_replace_handle(struct iommufd_device *idev,
+			       ioasid_t pasid,
 			       struct iommufd_hw_pagetable *hwpt,
 			       struct iommufd_hw_pagetable *old)
 {
@@ -336,6 +340,7 @@ int iommufd_dev_replace_handle(struct iommufd_device *idev,
 		return -ENOMEM;
 
 	handle->idev = idev;
+	WARN_ON(pasid != IOMMU_NO_PASID);
 	ret = iommu_replace_group_handle(idev->igroup->group,
 					 hwpt->domain, &handle->handle);
 	if (ret)
@@ -404,7 +409,8 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
 }
 
 int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
-				struct iommufd_device *idev)
+				struct iommufd_device *idev,
+				ioasid_t pasid)
 {
 	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
 	int rc;
@@ -430,7 +436,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;
@@ -448,7 +454,7 @@ 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;
 	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
@@ -456,7 +462,7 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev)
 	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_paging)
@@ -468,12 +474,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;
@@ -522,7 +528,7 @@ iommufd_group_do_replace_reserved_iova(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_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
@@ -551,7 +557,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;
 
@@ -584,7 +590,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
@@ -592,7 +599,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)
 {
@@ -621,7 +628,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);
 			/*
@@ -648,7 +655,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 {
@@ -669,8 +676,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;
@@ -685,7 +693,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;
@@ -694,8 +702,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;
@@ -732,7 +740,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;
 
@@ -762,7 +771,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);
@@ -778,7 +787,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 55418a067869..3b60349e2913 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,9 +134,9 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 			return ret;
 	}
 
-	curr = iommufd_device_get_attach_handle(idev);
+	curr = iommufd_device_get_attach_handle(idev, pasid);
 
-	ret = iommufd_dev_replace_handle(idev, hwpt, old);
+	ret = iommufd_dev_replace_handle(idev, pasid, hwpt, old);
 	if (ret) {
 		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 d06bf6e6c19f..48639427749b 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 19870b08056e..8e7265885f36 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -369,9 +369,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);
@@ -479,10 +480,12 @@ struct iommufd_attach_handle {
 #define to_iommufd_handle(hdl)	container_of(hdl, struct iommufd_attach_handle, handle)
 
 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);
 int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
-			      struct iommufd_device *idev);
+			      struct iommufd_device *idev,
+			      ioasid_t pasid);
 int iommufd_dev_replace_handle(struct iommufd_device *idev,
+			       ioasid_t pasid,
 			       struct iommufd_hw_pagetable *hwpt,
 			       struct iommufd_hw_pagetable *old);
 
@@ -499,38 +502,45 @@ 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)
 {
@@ -538,11 +548,12 @@ static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
 	int ret;
 
 	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_device_get_attach_handle(idev);
+	curr = iommufd_device_get_attach_handle(idev, pasid);
 
-	ret = iommufd_dev_replace_handle(idev, hwpt, old);
+	ret = iommufd_dev_replace_handle(idev, pasid, hwpt, old);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH v5 06/12] iommufd: Support pasid attach/replace

[Yi Liu]

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          |  31 ++---
 drivers/iommu/iommufd/fault.c           |   6 +-
 drivers/iommu/iommufd/iommufd_private.h |  21 +++-
 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 0b3f2094af4a..4e7a473d0dd0 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.
@@ -298,7 +301,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;
@@ -318,9 +320,12 @@ 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);
 
@@ -340,9 +345,12 @@ int iommufd_dev_replace_handle(struct iommufd_device *idev,
 		return -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);
 
@@ -589,10 +597,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.
@@ -676,9 +680,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 3b60349e2913..2d7590ede715 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 8e7265885f36..11773cef5acc 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -418,6 +418,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;
@@ -435,6 +436,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;
@@ -534,8 +549,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 rc ? ERR_PTR(rc) : NULL;
+}
+
+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 rc ? ERR_PTR(rc) : NULL;
+}
+
+/**
+ * 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 30f832a60ccb..f18472cbf688 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -8,6 +8,7 @@
 
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/iommu.h>
 #include <linux/types.h>
 
 struct 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);
 
-- 
2.34.1

[PATCH v5 07/12] iommufd: Allocate auto_domain with IOMMU_HWPT_ALLOC_PASID flag if device is PASID-capable

[Yi Liu]

iommufd cannot predict if user will use pasid or not, but if user is
going to use it, the domains used by PASID-capable device should be
allocated with IOMMU_HWPT_ALLOC_PASID flag.

For domain allocated per userspace request, user should provide this
flag. While for the auto_domain, iommufd needs to make it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 4e7a473d0dd0..a52937fba366 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -650,7 +650,9 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev, ioasid_t pasid,
 		goto out_unlock;
 	}
 
-	hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev, 0,
+	hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev,
+						idev->dev->iommu->max_pasids ?
+						IOMMU_HWPT_ALLOC_PASID : 0,
 						immediate_attach, NULL);
 	if (IS_ERR(hwpt_paging)) {
 		destroy_hwpt = ERR_CAST(hwpt_paging);
-- 
2.34.1

[PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

iommu hw may have special requirement on the domain attached to PASID
capable device. e.g. AMD IOMMU requires the domain allocated with the
IOMMU_HWPT_ALLOC_PASID flag. Hence, iommufd should enforce it when the
domain is used by PASID-capable device.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c             | 6 ++++--
 drivers/iommu/iommufd/hw_pagetable.c    | 7 +++++--
 drivers/iommu/iommufd/iommufd_private.h | 7 +++++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a1341078b962..d24e21a757ff 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3545,13 +3545,15 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 
 	/* Must be NESTING domain */
 	if (parent) {
-		if (!nested_supported(iommu) || flags)
+		if (!nested_supported(iommu) ||
+		    flags & ~IOMMU_HWPT_ALLOC_PASID)
 			return ERR_PTR(-EOPNOTSUPP);
 		return intel_nested_domain_alloc(parent, user_data);
 	}
 
 	if (flags &
-	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
+	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
+	       IOMMU_HWPT_ALLOC_PASID)))
 		return ERR_PTR(-EOPNOTSUPP);
 	if (nested_parent && !nested_supported(iommu))
 		return ERR_PTR(-EOPNOTSUPP);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 48639427749b..e4932a5a87ea 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -107,7 +107,8 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 			  const struct iommu_user_data *user_data)
 {
 	const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
-				IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+				IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
+				IOMMU_HWPT_ALLOC_PASID;
 	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
 	struct iommufd_hwpt_paging *hwpt_paging;
 	struct iommufd_hw_pagetable *hwpt;
@@ -128,6 +129,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 	if (IS_ERR(hwpt_paging))
 		return ERR_CAST(hwpt_paging);
 	hwpt = &hwpt_paging->common;
+	hwpt->pasid_compat = flags & IOMMU_HWPT_ALLOC_PASID;
 
 	INIT_LIST_HEAD(&hwpt_paging->hwpt_item);
 	/* Pairs with iommufd_hw_pagetable_destroy() */
@@ -223,7 +225,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	struct iommufd_hw_pagetable *hwpt;
 	int rc;
 
-	if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) ||
+	if ((flags & ~(IOMMU_HWPT_FAULT_ID_VALID | IOMMU_HWPT_ALLOC_PASID)) ||
 	    !user_data->len || !ops->domain_alloc_user)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (parent->auto_domain || !parent->nest_parent ||
@@ -235,6 +237,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	if (IS_ERR(hwpt_nested))
 		return ERR_CAST(hwpt_nested);
 	hwpt = &hwpt_nested->common;
+	hwpt->pasid_compat = flags & IOMMU_HWPT_ALLOC_PASID;
 
 	refcount_inc(&parent->common.obj.users);
 	hwpt_nested->parent = parent;
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 11773cef5acc..81a95f869e10 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -296,6 +296,7 @@ struct iommufd_hw_pagetable {
 	struct iommufd_object obj;
 	struct iommu_domain *domain;
 	struct iommufd_fault *fault;
+	bool pasid_compat : 1;
 };
 
 struct iommufd_hwpt_paging {
@@ -531,6 +532,9 @@ static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 					     struct iommufd_device *idev,
 					     ioasid_t pasid)
 {
+	if (idev->dev->iommu->max_pasids && !hwpt->pasid_compat)
+		return -EINVAL;
+
 	if (hwpt->fault)
 		return iommufd_fault_domain_attach_dev(hwpt, idev, pasid);
 
@@ -564,6 +568,9 @@ static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
 	struct iommufd_attach_handle *curr;
 	int ret;
 
+	if (idev->dev->iommu->max_pasids && !hwpt->pasid_compat)
+		return -EINVAL;
+
 	if (old->fault || hwpt->fault)
 		return iommufd_fault_domain_replace_dev(idev, pasid,
 							hwpt, old);
-- 
2.34.1

[PATCH v5 09/12] iommufd/selftest: Add set_dev_pasid in mock iommu

[Yi Liu]

The callback is needed to make pasid_attach/detach path complete for mock
device. A nop is enough for set_dev_pasid.

A MOCK_FLAGS_DEVICE_PASID is added to indicate a pasid-capable mock device
for the pasid test cases. Other test cases will still create a non-pasid
mock device. While the mock iommu always pretends to be pasid-capable.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/iommufd_test.h |  1 +
 drivers/iommu/iommufd/selftest.c     | 39 +++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index f4bc23a92f9a..6d532e25b78e 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -47,6 +47,7 @@ enum {
 enum {
 	MOCK_FLAGS_DEVICE_NO_DIRTY = 1 << 0,
 	MOCK_FLAGS_DEVICE_HUGE_IOVA = 1 << 1,
+	MOCK_FLAGS_DEVICE_PASID = 1 << 2,
 };
 
 enum {
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 540437be168a..635d8246691d 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -166,8 +166,16 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
 	return 0;
 }
 
+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_domain_ops mock_blocking_ops = {
 	.attach_dev = mock_domain_nop_attach,
+	.set_dev_pasid = mock_domain_set_dev_pasid_nop
 };
 
 static struct iommu_domain mock_blocking_domain = {
@@ -321,19 +329,25 @@ mock_domain_alloc_user(struct device *dev, u32 flags,
 		       struct iommu_domain *parent,
 		       const struct iommu_user_data *user_data)
 {
+	struct mock_dev *mdev = container_of(dev, struct mock_dev, dev);
 	struct mock_iommu_domain *mock_parent;
 	struct iommu_hwpt_selftest user_cfg;
 	int rc;
 
+	if ((flags & IOMMU_HWPT_ALLOC_PASID) &&
+	    (!(mdev->flags & MOCK_FLAGS_DEVICE_PASID) ||
+	     !dev->iommu->iommu_dev->max_pasids))
+		return ERR_PTR(-EOPNOTSUPP);
+
 	/* must be mock_domain */
 	if (!parent) {
-		struct mock_dev *mdev = container_of(dev, struct mock_dev, dev);
 		bool has_dirty_flag = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 		bool no_dirty_ops = mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY;
 		struct iommu_domain *domain;
 
 		if (flags & (~(IOMMU_HWPT_ALLOC_NEST_PARENT |
-			       IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
+			       IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
+			       IOMMU_HWPT_ALLOC_PASID)))
 			return ERR_PTR(-EOPNOTSUPP);
 		if (user_data || (has_dirty_flag && no_dirty_ops))
 			return ERR_PTR(-EOPNOTSUPP);
@@ -347,7 +361,8 @@ mock_domain_alloc_user(struct device *dev, u32 flags,
 	}
 
 	/* must be mock_domain_nested */
-	if (user_data->type != IOMMU_HWPT_DATA_SELFTEST || flags)
+	if (user_data->type != IOMMU_HWPT_DATA_SELFTEST ||
+	    flags & ~IOMMU_HWPT_ALLOC_PASID)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (!parent || parent->ops != mock_ops.default_domain_ops)
 		return ERR_PTR(-EINVAL);
@@ -566,6 +581,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,
 		},
 };
 
@@ -630,6 +646,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 *
@@ -690,11 +707,16 @@ 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;
 
 	if (dev_flags &
-	    ~(MOCK_FLAGS_DEVICE_NO_DIRTY | MOCK_FLAGS_DEVICE_HUGE_IOVA))
+	    ~(MOCK_FLAGS_DEVICE_NO_DIRTY |
+		    MOCK_FLAGS_DEVICE_HUGE_IOVA | MOCK_FLAGS_DEVICE_PASID))
 		return ERR_PTR(-EINVAL);
 
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
@@ -715,6 +737,14 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags)
 	if (rc)
 		goto err_put;
 
+	if (dev_flags & MOCK_FLAGS_DEVICE_PASID) {
+		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 +1579,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;
 
-- 
2.34.1

[PATCH v5 10/12] iommufd/selftest: Add a helper to get test device

[Yi Liu]

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 635d8246691d..bfd2c50f64ec 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -825,29 +825,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)
@@ -857,7 +867,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;
 }
 
-- 
2.34.1

[PATCH v5 11/12] iommufd/selftest: Add test ops to test pasid attach/detach

[Yi Liu]

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     | 139 +++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 6d532e25b78e..a9cc0154fba1 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 {
@@ -136,6 +140,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 bfd2c50f64ec..2c8d1c6ecef6 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -166,10 +166,29 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
 	return 0;
 }
 
+static bool pasid_1024_attached;
+
 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.
+	 * This is helpful to test the case in which the iommu core needs
+	 * to rollback to old domain due to driver failure.
+	 */
+	if (pasid == 1024) {
+		if (domain->type == IOMMU_DOMAIN_BLOCKED) {
+			pasid_1024_attached = false;
+		} else if (pasid_1024_attached) {
+			pasid_1024_attached = false;
+			// Fake an error to fail the replacement
+			return -ENOMEM;
+		} else {
+			pasid_1024_attached = true;
+		}
+	}
+
 	return 0;
 }
 
@@ -1469,6 +1488,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);
@@ -1546,6 +1676,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;
 	}
@@ -1590,6 +1728,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;
 
-- 
2.34.1

[PATCH v5 12/12] iommufd/selftest: Add coverage for iommufd pasid attach/detach

[Yi Liu]

This tests iommufd pasid attach/replace/detach.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 tools/testing/selftests/iommu/iommufd.c       | 277 ++++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  35 ++-
 tools/testing/selftests/iommu/iommufd_utils.h |  78 +++++
 3 files changed, 384 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 4927b9add5ad..f44d55f53f15 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -2386,4 +2386,281 @@ 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_flags(self->ioas_id,
+				   MOCK_FLAGS_DEVICE_PASID,
+				   &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 no_pasid_compat_hwpt;
+		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 |
+				    IOMMU_HWPT_ALLOC_PASID,
+				    &parent_hwpt_id);
+
+		test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id,
+					   IOMMU_HWPT_ALLOC_PASID,
+					   &nested_hwpt_id[0],
+					   IOMMU_HWPT_DATA_SELFTEST,
+					   &data, sizeof(data));
+		test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id,
+					   IOMMU_HWPT_ALLOC_PASID,
+					   &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 | IOMMU_HWPT_ALLOC_PASID,
+					 &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);
+
+		test_ioctl_destroy(iopf_hwpt_id);
+		close(fault_fd);
+		test_ioctl_destroy(fault_id);
+
+		/* Negative */
+		/*
+		 * Attach non pasid compat hwpt to pasid-capable device, should
+		 * fail, and have null domain.
+		 */
+		test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
+				    IOMMU_HWPT_ALLOC_NEST_PARENT,
+				    &no_pasid_compat_hwpt);
+		test_err_cmd_pasid_attach(EINVAL, pasid, no_pasid_compat_hwpt);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, 0, &result));
+		EXPECT_EQ(1, result);
+		test_ioctl_destroy(no_pasid_compat_hwpt);
+
+		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..72b87ed82a6b 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,22 +613,40 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 
 	fail_nth_enable();
 
-	if (_test_cmd_mock_domain(self->fd, ioas_id, &stdev_id, NULL,
-				  &idev_id))
+	if (_test_cmd_mock_domain_flags(self->fd, ioas_id,
+					MOCK_FLAGS_DEVICE_PASID,
+					&self->stdev_id, NULL, &idev_id))
 		return -1;
 
 	if (_test_cmd_get_hw_info(self->fd, idev_id, &info, sizeof(info), NULL))
 		return -1;
 
-	if (_test_cmd_hwpt_alloc(self->fd, idev_id, ioas_id, 0, 0, &hwpt_id,
+	if (_test_cmd_hwpt_alloc(self->fd, idev_id, ioas_id, 0,
+				 IOMMU_HWPT_ALLOC_PASID, &hwpt_id,
 				 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);
+}
-- 
2.34.1

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

[Baolu Lu]

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

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

	__iommu_set_group_pasid();
	switch_attach_handle();

Or anything I overlooked?

> +	if (!curr) {
> +		xa_erase(&group->pasid_array, pasid);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	ret = xa_err(curr);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (curr->domain == domain)
> +		goto out_unlock;
> +
> +	ret = __iommu_set_group_pasid(domain, group, pasid, curr->domain);
> +	if (ret)
> +		WARN_ON(handle != xa_store(&group->pasid_array, pasid,
> +					   curr, GFP_KERNEL));
> +out_unlock:
> +	mutex_unlock(&group->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid, IOMMUFD_INTERNAL);
> +
>   /*
>    * iommu_detach_device_pasid() - Detach the domain from pasid of device
>    * @domain: the iommu domain.

--
baolu

Re: [PATCH v5 02/12] iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle()

[Baolu Lu]

On 11/4/24 21:25, Yi Liu wrote:
> 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.

This patch is not a simple, non-functional refactoring. It allocates
attach_handle for all devices in domain attach/replace interfaces,
regardless of whether the domain is iopf-capable. Therefore, the commit
message should be rephrased to accurately reflect the patch's purpose
and rationale.

--
baolu

Re: [PATCH v5 03/12] iommufd: Move the iommufd_handle helpers to device.c

[Baolu Lu]

On 11/4/24 21:25, Yi Liu wrote:
> The iommu_attach_handle is now only passed when attaching iopf-capable
> domain, while it is not convenient for the iommu core to track the
> attached domain of pasids. To address it, the iommu_attach_handle will
> be passed to iommu core for non-fault-able domain as well. Hence the
> iommufd_handle related helpers are no longer fault specific, it makes
> more sense to move it out of fault.c.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommufd/device.c          | 51 ++++++++++++++++++++++
>   drivers/iommu/iommufd/fault.c           | 56 +------------------------
>   drivers/iommu/iommufd/iommufd_private.h |  8 ++++
>   3 files changed, 61 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 5fd3dd420290..823c81145214 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -293,6 +293,57 @@ u32 iommufd_device_to_id(struct iommufd_device *idev)
>   }
>   EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD);
>   
> +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);
> +}

I would suggest placing this helper closer to where it is used. Because,
there is currently no locking mechanism to synchronize threads that
access the returned handle with those attaching or replacing the domain.
This lack of synchronization could potentially lead to use-after-free
issue.

By placing the helper near its callers and perhaps adding comments
explaining this limitation, we can improve maintainability and prevent
misuse in the future.

--
baolu

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

[Yi Liu]

On 2024/11/5 11:58, Baolu Lu wrote:
> On 11/4/24 21:25, Yi Liu wrote:
>> +/**
>> + * iommu_replace_device_pasid - Replace the domain that a pasid is 
>> attached to
>> + * @domain: the new iommu domain
>> + * @dev: the attached device.
>> + * @pasid: the pasid of the device.
>> + * @handle: the attach handle.
>> + *
>> + * This API allows the pasid to switch domains. Return 0 on success, or an
>> + * error. The pasid will keep the old configuration if replacement failed.
>> + * This is supposed to be used by iommufd, and iommufd can guarantee that
>> + * both iommu_attach_device_pasid() and iommu_replace_device_pasid() would
>> + * pass in a valid @handle.
>> + */
>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +                   struct device *dev, ioasid_t pasid,
>> +                   struct iommu_attach_handle *handle)
>> +{
>> +    /* Caller must be a probed driver on dev */
>> +    struct iommu_group *group = dev->iommu_group;
>> +    struct iommu_attach_handle *curr;
>> +    int ret;
>> +
>> +    if (!domain->ops->set_dev_pasid)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (!group)
>> +        return -ENODEV;
>> +
>> +    if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
>> +        pasid == IOMMU_NO_PASID || !handle)
>> +        return -EINVAL;
>> +
>> +    handle->domain = domain;
>> +
>> +    mutex_lock(&group->mutex);
>> +    /*
>> +     * The iommu_attach_handle of the pasid becomes inconsistent with the
>> +     * actual handle per the below operation. The concurrent PRI path will
>> +     * deliver the PRQs per the new handle, this does not have a functional
>> +     * impact. The PRI path would eventually become consistent when the
>> +     * replacement is done.
>> +     */
>> +    curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
>> +                              pasid, handle,
>> +                              GFP_KERNEL);
> 
> The iommu drivers can only flush pending PRs in the hardware queue when
> __iommu_set_group_pasid() is called. So, it appears more reasonable to
> reorder things like this:
> 
>      __iommu_set_group_pasid();
>      switch_attach_handle();
> 
> Or anything I overlooked?

not quite get why this handle is related to iommu driver flushing PRs.
Before __iommu_set_group_pasid(), the pasid is still attached with the
old domain, so is the hw configuration.

>> +    if (!curr) {
>> +        xa_erase(&group->pasid_array, pasid);
>> +        ret = -EINVAL;
>> +        goto out_unlock;
>> +    }
>> +
>> +    ret = xa_err(curr);
>> +    if (ret)
>> +        goto out_unlock;
>> +
>> +    if (curr->domain == domain)
>> +        goto out_unlock;
>> +
>> +    ret = __iommu_set_group_pasid(domain, group, pasid, curr->domain);
>> +    if (ret)
>> +        WARN_ON(handle != xa_store(&group->pasid_array, pasid,
>> +                       curr, GFP_KERNEL));
>> +out_unlock:
>> +    mutex_unlock(&group->mutex);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid, IOMMUFD_INTERNAL);
>> +
>>   /*
>>    * iommu_detach_device_pasid() - Detach the domain from pasid of device
>>    * @domain: the iommu domain.
> 
> -- 
> baolu

-- 
Regards,
Yi Liu

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

[Baolu Lu]

On 2024/11/5 15:49, Yi Liu wrote:
> On 2024/11/5 11:58, Baolu Lu wrote:
>> On 11/4/24 21:25, Yi Liu wrote:
>>> +/**
>>> + * iommu_replace_device_pasid - Replace the domain that a pasid is 
>>> attached to
>>> + * @domain: the new iommu domain
>>> + * @dev: the attached device.
>>> + * @pasid: the pasid of the device.
>>> + * @handle: the attach handle.
>>> + *
>>> + * This API allows the pasid to switch domains. Return 0 on success, 
>>> or an
>>> + * error. The pasid will keep the old configuration if replacement 
>>> failed.
>>> + * This is supposed to be used by iommufd, and iommufd can guarantee 
>>> that
>>> + * both iommu_attach_device_pasid() and iommu_replace_device_pasid() 
>>> would
>>> + * pass in a valid @handle.
>>> + */
>>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>>> +                   struct device *dev, ioasid_t pasid,
>>> +                   struct iommu_attach_handle *handle)
>>> +{
>>> +    /* Caller must be a probed driver on dev */
>>> +    struct iommu_group *group = dev->iommu_group;
>>> +    struct iommu_attach_handle *curr;
>>> +    int ret;
>>> +
>>> +    if (!domain->ops->set_dev_pasid)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (!group)
>>> +        return -ENODEV;
>>> +
>>> +    if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
>>> +        pasid == IOMMU_NO_PASID || !handle)
>>> +        return -EINVAL;
>>> +
>>> +    handle->domain = domain;
>>> +
>>> +    mutex_lock(&group->mutex);
>>> +    /*
>>> +     * The iommu_attach_handle of the pasid becomes inconsistent 
>>> with the
>>> +     * actual handle per the below operation. The concurrent PRI 
>>> path will
>>> +     * deliver the PRQs per the new handle, this does not have a 
>>> functional
>>> +     * impact. The PRI path would eventually become consistent when the
>>> +     * replacement is done.
>>> +     */
>>> +    curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
>>> +                              pasid, handle,
>>> +                              GFP_KERNEL);
>>
>> The iommu drivers can only flush pending PRs in the hardware queue when
>> __iommu_set_group_pasid() is called. So, it appears more reasonable to
>> reorder things like this:
>>
>>      __iommu_set_group_pasid();
>>      switch_attach_handle();
>>
>> Or anything I overlooked?
> 
> not quite get why this handle is related to iommu driver flushing PRs.
> Before __iommu_set_group_pasid(), the pasid is still attached with the
> old domain, so is the hw configuration.

I meant that in the path of __iommu_set_group_pasid(), the iommu drivers
have the opportunity to flush the PRs pending in the hardware queue. If
the attach_handle is switched (by calling xa_store()) before
__iommu_set_group_pasid(), the pending PRs will be routed to iopf
handler of the new domain, which is not desirable.

--
baolu

Re: [PATCH v5 03/12] iommufd: Move the iommufd_handle helpers to device.c

[Yi Liu]

On 2024/11/5 13:21, Baolu Lu wrote:
> On 11/4/24 21:25, Yi Liu wrote:
>> The iommu_attach_handle is now only passed when attaching iopf-capable
>> domain, while it is not convenient for the iommu core to track the
>> attached domain of pasids. To address it, the iommu_attach_handle will
>> be passed to iommu core for non-fault-able domain as well. Hence the
>> iommufd_handle related helpers are no longer fault specific, it makes
>> more sense to move it out of fault.c.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommufd/device.c          | 51 ++++++++++++++++++++++
>>   drivers/iommu/iommufd/fault.c           | 56 +------------------------
>>   drivers/iommu/iommufd/iommufd_private.h |  8 ++++
>>   3 files changed, 61 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index 5fd3dd420290..823c81145214 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -293,6 +293,57 @@ u32 iommufd_device_to_id(struct iommufd_device *idev)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD);
>> +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);
>> +}
> 
> I would suggest placing this helper closer to where it is used. Because,
> there is currently no locking mechanism to synchronize threads that
> access the returned handle with those attaching or replacing the domain.
> This lack of synchronization could potentially lead to use-after-free
> issue.
> 
> By placing the helper near its callers and perhaps adding comments
> explaining this limitation, we can improve maintainability and prevent
> misuse in the future.

with this comment, it seems better to put it in the header file. There are
two files that has referred this helper. the fault.c and iommufd_private.h.

-- 
Regards,
Yi Liu

Re: [PATCH v5 02/12] iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle()

[Yi Liu]

On 2024/11/5 13:06, Baolu Lu wrote:
> On 11/4/24 21:25, Yi Liu wrote:
>> 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.
> 
> This patch is not a simple, non-functional refactoring. It allocates
> attach_handle for all devices in domain attach/replace interfaces,
> regardless of whether the domain is iopf-capable. Therefore, the commit
> message should be rephrased to accurately reflect the patch's purpose
> and rationale.

This patch splits the __fault_domain_replace_dev() a lot, the else branch 
of the below code was lifted to the iommufd_fault_domain_replace_dev().
While the new __fault_domain_replace_dev() will only be called when the
hwpt->fault is valid. So the iommu_attach_handle is still allocated only
for the iopf-capable path. When the hwpt->fault is invalid, the
iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with
a null iommu_attach_handle. What you described is done in the patch 04 of
this series. :)

-	if (hwpt->fault) {
-		handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-		if (!handle)
-			return -ENOMEM;
-
-		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);
-	}

-- 
Regards,
Yi Liu

Re: [PATCH v5 02/12] iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle()

[Baolu Lu]

On 2024/11/5 16:01, Yi Liu wrote:
> On 2024/11/5 13:06, Baolu Lu wrote:
>> On 11/4/24 21:25, Yi Liu wrote:
>>> 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.
>>
>> This patch is not a simple, non-functional refactoring. It allocates
>> attach_handle for all devices in domain attach/replace interfaces,
>> regardless of whether the domain is iopf-capable. Therefore, the commit
>> message should be rephrased to accurately reflect the patch's purpose
>> and rationale.
> 
> This patch splits the __fault_domain_replace_dev() a lot, the else 
> branch of the below code was lifted to the 
> iommufd_fault_domain_replace_dev().
> While the new __fault_domain_replace_dev() will only be called when the
> hwpt->fault is valid. So the iommu_attach_handle is still allocated only
> for the iopf-capable path. When the hwpt->fault is invalid, the
> iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with
> a null iommu_attach_handle. What you described is done in the patch 04 of
> this series. 🙂
> 
> -    if (hwpt->fault) {
> -        handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> -        if (!handle)
> -            return -ENOMEM;
> -
> -        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);
> -    }

Okay, I overlooked that part.

Below change caused me to think that attach handle is always allocated
in this patch no matter ...

-	if (hwpt->fault) {
-		handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-		if (!handle)
-			return -ENOMEM;
-
-		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 -ENOMEM;

If no functional change, please just ignore this comment.

--
baolu

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

[Yi Liu]

On 2024/11/5 15:57, Baolu Lu wrote:
> On 2024/11/5 15:49, Yi Liu wrote:
>> On 2024/11/5 11:58, Baolu Lu wrote:
>>> On 11/4/24 21:25, Yi Liu wrote:
>>>> +/**
>>>> + * iommu_replace_device_pasid - Replace the domain that a pasid is 
>>>> attached to
>>>> + * @domain: the new iommu domain
>>>> + * @dev: the attached device.
>>>> + * @pasid: the pasid of the device.
>>>> + * @handle: the attach handle.
>>>> + *
>>>> + * This API allows the pasid to switch domains. Return 0 on success, 
>>>> or an
>>>> + * error. The pasid will keep the old configuration if replacement 
>>>> failed.
>>>> + * This is supposed to be used by iommufd, and iommufd can guarantee that
>>>> + * both iommu_attach_device_pasid() and iommu_replace_device_pasid() 
>>>> would
>>>> + * pass in a valid @handle.
>>>> + */
>>>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>>>> +                   struct device *dev, ioasid_t pasid,
>>>> +                   struct iommu_attach_handle *handle)
>>>> +{
>>>> +    /* Caller must be a probed driver on dev */
>>>> +    struct iommu_group *group = dev->iommu_group;
>>>> +    struct iommu_attach_handle *curr;
>>>> +    int ret;
>>>> +
>>>> +    if (!domain->ops->set_dev_pasid)
>>>> +        return -EOPNOTSUPP;
>>>> +
>>>> +    if (!group)
>>>> +        return -ENODEV;
>>>> +
>>>> +    if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
>>>> +        pasid == IOMMU_NO_PASID || !handle)
>>>> +        return -EINVAL;
>>>> +
>>>> +    handle->domain = domain;
>>>> +
>>>> +    mutex_lock(&group->mutex);
>>>> +    /*
>>>> +     * The iommu_attach_handle of the pasid becomes inconsistent with the
>>>> +     * actual handle per the below operation. The concurrent PRI path 
>>>> will
>>>> +     * deliver the PRQs per the new handle, this does not have a 
>>>> functional
>>>> +     * impact. The PRI path would eventually become consistent when the
>>>> +     * replacement is done.
>>>> +     */
>>>> +    curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
>>>> +                              pasid, handle,
>>>> +                              GFP_KERNEL);
>>>
>>> The iommu drivers can only flush pending PRs in the hardware queue when
>>> __iommu_set_group_pasid() is called. So, it appears more reasonable to
>>> reorder things like this:
>>>
>>>      __iommu_set_group_pasid();
>>>      switch_attach_handle();
>>>
>>> Or anything I overlooked?
>>
>> not quite get why this handle is related to iommu driver flushing PRs.
>> Before __iommu_set_group_pasid(), the pasid is still attached with the
>> old domain, so is the hw configuration.
> 
> I meant that in the path of __iommu_set_group_pasid(), the iommu drivers
> have the opportunity to flush the PRs pending in the hardware queue. If
> the attach_handle is switched (by calling xa_store()) before
> __iommu_set_group_pasid(), the pending PRs will be routed to iopf
> handler of the new domain, which is not desirable.

I see. You mean the handling of PRQs. I was interpreting you are talking
about PRQ draining.

yet, what you described was discussed before [1]. Forwarding PRQs to the
new domain looks to be ok.

But you reminded me one thing. What I cared about more is the case
replacing an iopf-capable domain to non-capable domain. This means the new
coming PRQs would be responded by iopf_error_response(). Do you see an
issue here?

[1] https://lore.kernel.org/linux-iommu/20240429135512.GC941030@nvidia.com/

-- 
Regards,
Yi Liu

Re: [PATCH v5 02/12] iommufd: Refactor __fault_domain_replace_dev() to be a wrapper of iommu_replace_group_handle()

[Yi Liu]

On 2024/11/5 16:03, Baolu Lu wrote:
> On 2024/11/5 16:01, Yi Liu wrote:
>> On 2024/11/5 13:06, Baolu Lu wrote:
>>> On 11/4/24 21:25, Yi Liu wrote:
>>>> 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.
>>>
>>> This patch is not a simple, non-functional refactoring. It allocates
>>> attach_handle for all devices in domain attach/replace interfaces,
>>> regardless of whether the domain is iopf-capable. Therefore, the commit
>>> message should be rephrased to accurately reflect the patch's purpose
>>> and rationale.
>>
>> This patch splits the __fault_domain_replace_dev() a lot, the else branch 
>> of the below code was lifted to the iommufd_fault_domain_replace_dev().
>> While the new __fault_domain_replace_dev() will only be called when the
>> hwpt->fault is valid. So the iommu_attach_handle is still allocated only
>> for the iopf-capable path. When the hwpt->fault is invalid, the
>> iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with
>> a null iommu_attach_handle. What you described is done in the patch 04 of
>> this series. 🙂
>>
>> -    if (hwpt->fault) {
>> -        handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>> -        if (!handle)
>> -            return -ENOMEM;
>> -
>> -        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);
>> -    }
> 
> Okay, I overlooked that part.
> 
> Below change caused me to think that attach handle is always allocated
> in this patch no matter ...

aha, because the caller of this chunk would check hwpt->fault first.

> 
> -    if (hwpt->fault) {
> -        handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> -        if (!handle)
> -            return -ENOMEM;
> -
> -        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 -ENOMEM;
> 
> If no functional change, please just ignore this comment.

yep, no functional change is intended in this patch. But do let me know if
you find one. :)

-- 
Regards,
Yi Liu

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

[Baolu Lu]

On 2024/11/5 16:10, Yi Liu wrote:
> On 2024/11/5 15:57, Baolu Lu wrote:
>> On 2024/11/5 15:49, Yi Liu wrote:
>>> On 2024/11/5 11:58, Baolu Lu wrote:
>>>> On 11/4/24 21:25, Yi Liu wrote:
>>>>> +/**
>>>>> + * iommu_replace_device_pasid - Replace the domain that a pasid is 
>>>>> attached to
>>>>> + * @domain: the new iommu domain
>>>>> + * @dev: the attached device.
>>>>> + * @pasid: the pasid of the device.
>>>>> + * @handle: the attach handle.
>>>>> + *
>>>>> + * This API allows the pasid to switch domains. Return 0 on 
>>>>> success, or an
>>>>> + * error. The pasid will keep the old configuration if replacement 
>>>>> failed.
>>>>> + * This is supposed to be used by iommufd, and iommufd can 
>>>>> guarantee that
>>>>> + * both iommu_attach_device_pasid() and 
>>>>> iommu_replace_device_pasid() would
>>>>> + * pass in a valid @handle.
>>>>> + */
>>>>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>>>>> +                   struct device *dev, ioasid_t pasid,
>>>>> +                   struct iommu_attach_handle *handle)
>>>>> +{
>>>>> +    /* Caller must be a probed driver on dev */
>>>>> +    struct iommu_group *group = dev->iommu_group;
>>>>> +    struct iommu_attach_handle *curr;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (!domain->ops->set_dev_pasid)
>>>>> +        return -EOPNOTSUPP;
>>>>> +
>>>>> +    if (!group)
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
>>>>> +        pasid == IOMMU_NO_PASID || !handle)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    handle->domain = domain;
>>>>> +
>>>>> +    mutex_lock(&group->mutex);
>>>>> +    /*
>>>>> +     * The iommu_attach_handle of the pasid becomes inconsistent 
>>>>> with the
>>>>> +     * actual handle per the below operation. The concurrent PRI 
>>>>> path will
>>>>> +     * deliver the PRQs per the new handle, this does not have a 
>>>>> functional
>>>>> +     * impact. The PRI path would eventually become consistent 
>>>>> when the
>>>>> +     * replacement is done.
>>>>> +     */
>>>>> +    curr = (struct iommu_attach_handle *)xa_store(&group- 
>>>>> >pasid_array,
>>>>> +                              pasid, handle,
>>>>> +                              GFP_KERNEL);
>>>>
>>>> The iommu drivers can only flush pending PRs in the hardware queue when
>>>> __iommu_set_group_pasid() is called. So, it appears more reasonable to
>>>> reorder things like this:
>>>>
>>>>      __iommu_set_group_pasid();
>>>>      switch_attach_handle();
>>>>
>>>> Or anything I overlooked?
>>>
>>> not quite get why this handle is related to iommu driver flushing PRs.
>>> Before __iommu_set_group_pasid(), the pasid is still attached with the
>>> old domain, so is the hw configuration.
>>
>> I meant that in the path of __iommu_set_group_pasid(), the iommu drivers
>> have the opportunity to flush the PRs pending in the hardware queue. If
>> the attach_handle is switched (by calling xa_store()) before
>> __iommu_set_group_pasid(), the pending PRs will be routed to iopf
>> handler of the new domain, which is not desirable.
> 
> I see. You mean the handling of PRQs. I was interpreting you are talking
> about PRQ draining.
> 
> yet, what you described was discussed before [1]. Forwarding PRQs to the
> new domain looks to be ok.
> 
> But you reminded me one thing. What I cared about more is the case
> replacing an iopf-capable domain to non-capable domain. This means the new
> coming PRQs would be responded by iopf_error_response(). Do you see an
> issue here?

I am not sure, but it will be more reasonable if you can make it in the
right order. If that's impossible, then add some comments to explain it.

--
baolu

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

[Jason Gunthorpe]

On Tue, Nov 05, 2024 at 04:10:59PM +0800, Yi Liu wrote:

> > > not quite get why this handle is related to iommu driver flushing PRs.
> > > Before __iommu_set_group_pasid(), the pasid is still attached with the
> > > old domain, so is the hw configuration.
> > 
> > I meant that in the path of __iommu_set_group_pasid(), the iommu drivers
> > have the opportunity to flush the PRs pending in the hardware queue. If
> > the attach_handle is switched (by calling xa_store()) before
> > __iommu_set_group_pasid(), the pending PRs will be routed to iopf
> > handler of the new domain, which is not desirable.
> 
> I see. You mean the handling of PRQs. I was interpreting you are talking
> about PRQ draining.

I don't think we need to worry about this race, and certainly you
shouldn't be making the domain replacement path non-hitless just to
fence the page requests.

If a page request comes in during the race window of domain change
there are only three outcomes:

  1) The old domain handles it and it translates on the old domain
  2) The new domain handles it and it translates on the new domain
  3) The old domain handles it and it translates on the new domain.
     a) The page request is ack'd and the device retries and loads the
       new domain - OK - at best it will use the new translation, at
       worst it will retry.
     b) the page request fails and the device sees the failure. This
        is the same as #1 - OK

All are correct. We don't need to do more here than just let the race
resolve itself.

Once the domains are switched in HW we do have to flush everything
queued due to the fault path locking scheme on the domain.

Jason

Re: [PATCH v5 03/12] iommufd: Move the iommufd_handle helpers to device.c

[Jason Gunthorpe]

On Tue, Nov 05, 2024 at 04:01:24PM +0800, Yi Liu wrote:

> > By placing the helper near its callers and perhaps adding comments
> > explaining this limitation, we can improve maintainability and prevent
> > misuse in the future.
> 
> with this comment, it seems better to put it in the header file. There are
> two files that has referred this helper. the fault.c and iommufd_private.h.

Put kdoc comments at the function body in the .c file please

Jason

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

[Baolu Lu]

On 2024/11/5 23:10, Jason Gunthorpe wrote:
> On Tue, Nov 05, 2024 at 04:10:59PM +0800, Yi Liu wrote:
> 
>>>> not quite get why this handle is related to iommu driver flushing PRs.
>>>> Before __iommu_set_group_pasid(), the pasid is still attached with the
>>>> old domain, so is the hw configuration.
>>> I meant that in the path of __iommu_set_group_pasid(), the iommu drivers
>>> have the opportunity to flush the PRs pending in the hardware queue. If
>>> the attach_handle is switched (by calling xa_store()) before
>>> __iommu_set_group_pasid(), the pending PRs will be routed to iopf
>>> handler of the new domain, which is not desirable.
>> I see. You mean the handling of PRQs. I was interpreting you are talking
>> about PRQ draining.
> I don't think we need to worry about this race, and certainly you
> shouldn't be making the domain replacement path non-hitless just to
> fence the page requests.
> 
> If a page request comes in during the race window of domain change
> there are only three outcomes:
> 
>    1) The old domain handles it and it translates on the old domain
>    2) The new domain handles it and it translates on the new domain
>    3) The old domain handles it and it translates on the new domain.
>       a) The page request is ack'd and the device retries and loads the
>         new domain - OK - at best it will use the new translation, at
>         worst it will retry.
>       b) the page request fails and the device sees the failure. This
>          is the same as #1 - OK
> 
> All are correct. We don't need to do more here than just let the race
> resolve itself.
> 
> Once the domains are switched in HW we do have to flush everything
> queued due to the fault path locking scheme on the domain.

Agreed. To my understanding, the worst case is that the device retries
the transaction which might result in another page fault, which will set
up the translation in the new domain.

--
baolu

Re: [PATCH v5 00/12] iommufd support pasid attach/replace

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:25:01AM -0800, Yi Liu wrote:

> This series is based on the preparation series [1] [2], it first adds a
> missing iommu API to replace the domain for a pasid.

Let's try hard to get some of these dependencies merged this cycle..

Thanks,
Jason

Re: [PATCH v5 00/12] iommufd support pasid attach/replace

[Baolu Lu]

On 11/13/24 09:37, Jason Gunthorpe wrote:
> On Mon, Nov 04, 2024 at 05:25:01AM -0800, Yi Liu wrote:
> 
>> This series is based on the preparation series [1] [2], it first adds a
>> missing iommu API to replace the domain for a pasid.
> Let's try hard to get some of these dependencies merged this cycle..

The pasid replace has been merged in the iommu tree.

Yi, did I overlook anything?

--
baolu

Re: [PATCH v5 00/12] iommufd support pasid attach/replace

[Yi Liu]

On 2024/11/13 11:01, Baolu Lu wrote:
> On 11/13/24 09:37, Jason Gunthorpe wrote:
>> On Mon, Nov 04, 2024 at 05:25:01AM -0800, Yi Liu wrote:
>>
>>> This series is based on the preparation series [1] [2], it first adds a
>>> missing iommu API to replace the domain for a pasid.
>> Let's try hard to get some of these dependencies merged this cycle..
> 
> The pasid replace has been merged in the iommu tree.
> 
> Yi, did I overlook anything?

I think Jason means the two series I listed. The first one has already been
merged by you and Joerg [1]. While the second one [2] is not yet. It might
not be a hard dependency of the iommufd pasid series, but as it was
originated from the iommufd pasid series, so it is listed here as well. I
think it is already in good shape except one nit spotted by you. Perhaps I
can update a version and see what we can do for it.

[1] 
https://lore.kernel.org/linux-iommu/20241108021406.173972-1-baolu.lu@linux.intel.com/
[2] 
https://lore.kernel.org/linux-iommu/20241108021406.173972-1-baolu.lu@linux.intel.com/

-- 
Regards,
Yi Liu

Re: [PATCH v5 00/12] iommufd support pasid attach/replace

[Yi Liu]

On 2024/11/13 11:24, Yi Liu wrote:
> On 2024/11/13 11:01, Baolu Lu wrote:
>> On 11/13/24 09:37, Jason Gunthorpe wrote:
>>> On Mon, Nov 04, 2024 at 05:25:01AM -0800, Yi Liu wrote:
>>>
>>>> This series is based on the preparation series [1] [2], it first adds a
>>>> missing iommu API to replace the domain for a pasid.
>>> Let's try hard to get some of these dependencies merged this cycle..
>>
>> The pasid replace has been merged in the iommu tree.
>>
>> Yi, did I overlook anything?
> 
> I think Jason means the two series I listed. The first one has already been
> merged by you and Joerg [1]. While the second one [2] is not yet. It might
> not be a hard dependency of the iommufd pasid series, but as it was
> originated from the iommufd pasid series, so it is listed here as well. I
> think it is already in good shape except one nit spotted by you. Perhaps I
> can update a version and see what we can do for it.
> 
> [1] 
> https://lore.kernel.org/linux-iommu/20241108021406.173972-1-baolu.lu@linux.intel.com/
> [2] https://lore.kernel.org/linux-iommu/20241108120427.13562-1-yi.l.liu@intel.com/
> 
corrected the link [2].

-- 
Regards,
Yi Liu

Re: [PATCH v5 00/12] iommufd support pasid attach/replace

[Yi Liu]

On 2024/11/13 11:26, Yi Liu wrote:
> On 2024/11/13 11:24, Yi Liu wrote:
>> On 2024/11/13 11:01, Baolu Lu wrote:
>>> On 11/13/24 09:37, Jason Gunthorpe wrote:
>>>> On Mon, Nov 04, 2024 at 05:25:01AM -0800, Yi Liu wrote:
>>>>
>>>>> This series is based on the preparation series [1] [2], it first adds a
>>>>> missing iommu API to replace the domain for a pasid.
>>>> Let's try hard to get some of these dependencies merged this cycle..

The second dependency has a new version in the below.

https://lore.kernel.org/linux-iommu/20241113134613.7173-1-yi.l.liu@intel.com/

-- 
Regards,
Yi Liu

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

Hi Jason, Vasant,

When cooking new version, I got three opens on enforcing using
pasid-compatible domains to pasid-capable device in iommufd as suggested
in [1].

- Concept problem when considering nested domain
   IIUC. pasid-compatible domain means the domain can be attached to PASID.
   e.g. AMD requires using V2 page table hence it can be configed to GCR3.
   However, the nested domain uses both V1 and V2 page table, I don't think
   it can be attached to PASID on AMD. So nested domain can not be
   considered as pasid-compatible. Based on this, this enforcement only
   applies to paging-domains. If so, do we still need to enforce it in
   iommufd? Will it simpler to let the AMD iommu driver to deal it?

- PASID-capable device v.s. PASID-enabled device
   We keep saying PASID-capable, but system may not enable it. Would it
   better enforce the pasid-compatible domain for PASID-enabled device?
   Seems all iommu vendor will enable PASID if it's supported. But
   conceptly, it is be more accurate if only do it when PASID is enabled.
   For PCI devices, we can check if the pasid cap is enabled from device
   config space. But for non-PCI PASID support (e.g. some ARM platform), I
   don't know if there is any way to check the PASID enabled or not. Or, to
   cover both, we need an iommu API to check PASID enabled or not?

- Nest parent domain should never be pasid-compatible?
   I think the AMD iommu uses the V1 page table format for the parent
   domain. Hence parent domain should not be allocated with the
   IOMMU_HWPT_ALLOC_PASID flag. Otherwise, it does not work. Should this
   be enforced in iommufd?

Thoughts?

[1] 
https://lore.kernel.org/linux-iommu/5a6c2676-256a-4fa5-b9a0-e433d4e933c9@intel.com/

Regards,
Yi Liu

On 2024/11/4 21:25, Yi Liu wrote:
> iommu hw may have special requirement on the domain attached to PASID
> capable device. e.g. AMD IOMMU requires the domain allocated with the
> IOMMU_HWPT_ALLOC_PASID flag. Hence, iommufd should enforce it when the
> domain is used by PASID-capable device.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c             | 6 ++++--
>   drivers/iommu/iommufd/hw_pagetable.c    | 7 +++++--
>   drivers/iommu/iommufd/iommufd_private.h | 7 +++++++
>   3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index a1341078b962..d24e21a757ff 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3545,13 +3545,15 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
>   
>   	/* Must be NESTING domain */
>   	if (parent) {
> -		if (!nested_supported(iommu) || flags)
> +		if (!nested_supported(iommu) ||
> +		    flags & ~IOMMU_HWPT_ALLOC_PASID)
>   			return ERR_PTR(-EOPNOTSUPP);
>   		return intel_nested_domain_alloc(parent, user_data);
>   	}
>   
>   	if (flags &
> -	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
> +	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> +	       IOMMU_HWPT_ALLOC_PASID)))
>   		return ERR_PTR(-EOPNOTSUPP);
>   	if (nested_parent && !nested_supported(iommu))
>   		return ERR_PTR(-EOPNOTSUPP);
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index 48639427749b..e4932a5a87ea 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -107,7 +107,8 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   			  const struct iommu_user_data *user_data)
>   {
>   	const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
> -				IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +				IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> +				IOMMU_HWPT_ALLOC_PASID;
>   	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
>   	struct iommufd_hwpt_paging *hwpt_paging;
>   	struct iommufd_hw_pagetable *hwpt;
> @@ -128,6 +129,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   	if (IS_ERR(hwpt_paging))
>   		return ERR_CAST(hwpt_paging);
>   	hwpt = &hwpt_paging->common;
> +	hwpt->pasid_compat = flags & IOMMU_HWPT_ALLOC_PASID;
>   
>   	INIT_LIST_HEAD(&hwpt_paging->hwpt_item);
>   	/* Pairs with iommufd_hw_pagetable_destroy() */
> @@ -223,7 +225,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
>   	struct iommufd_hw_pagetable *hwpt;
>   	int rc;
>   
> -	if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) ||
> +	if ((flags & ~(IOMMU_HWPT_FAULT_ID_VALID | IOMMU_HWPT_ALLOC_PASID)) ||
>   	    !user_data->len || !ops->domain_alloc_user)
>   		return ERR_PTR(-EOPNOTSUPP);
>   	if (parent->auto_domain || !parent->nest_parent ||
> @@ -235,6 +237,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
>   	if (IS_ERR(hwpt_nested))
>   		return ERR_CAST(hwpt_nested);
>   	hwpt = &hwpt_nested->common;
> +	hwpt->pasid_compat = flags & IOMMU_HWPT_ALLOC_PASID;
>   
>   	refcount_inc(&parent->common.obj.users);
>   	hwpt_nested->parent = parent;
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 11773cef5acc..81a95f869e10 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -296,6 +296,7 @@ struct iommufd_hw_pagetable {
>   	struct iommufd_object obj;
>   	struct iommu_domain *domain;
>   	struct iommufd_fault *fault;
> +	bool pasid_compat : 1;
>   };
>   
>   struct iommufd_hwpt_paging {
> @@ -531,6 +532,9 @@ static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
>   					     struct iommufd_device *idev,
>   					     ioasid_t pasid)
>   {
> +	if (idev->dev->iommu->max_pasids && !hwpt->pasid_compat)
> +		return -EINVAL;
> +
>   	if (hwpt->fault)
>   		return iommufd_fault_domain_attach_dev(hwpt, idev, pasid);
>   
> @@ -564,6 +568,9 @@ static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
>   	struct iommufd_attach_handle *curr;
>   	int ret;
>   
> +	if (idev->dev->iommu->max_pasids && !hwpt->pasid_compat)
> +		return -EINVAL;
> +
>   	if (old->fault || hwpt->fault)
>   		return iommufd_fault_domain_replace_dev(idev, pasid,
>   							hwpt, old);

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Jason Gunthorpe]

On Fri, Dec 06, 2024 at 03:57:39PM +0800, Yi Liu wrote:
> Hi Jason, Vasant,
> 
> When cooking new version, I got three opens on enforcing using
> pasid-compatible domains to pasid-capable device in iommufd as suggested
> in [1].
> 
> - Concept problem when considering nested domain
>   IIUC. pasid-compatible domain means the domain can be attached to PASID.
>   e.g. AMD requires using V2 page table hence it can be configed to GCR3.
>   However, the nested domain uses both V1 and V2 page table, I don't think
>   it can be attached to PASID on AMD. So nested domain can not be
>   considered as pasid-compatible. 

Yes.

>   Based on this, this enforcement only
>   applies to paging-domains. If so, do we still need to enforce it in
>   iommufd? Will it simpler to let the AMD iommu driver to deal it?

I think driver should deal with it, Intel doesn't have that
limitation. I sent patches to fix that detection for AMD and ARM
already.

> - PASID-capable device v.s. PASID-enabled device
>   We keep saying PASID-capable, but system may not enable it. Would it
>   better enforce the pasid-compatible domain for PASID-enabled device?
>   Seems all iommu vendor will enable PASID if it's supported. But
>   conceptly, it is be more accurate if only do it when PASID is
>   enabled.

If we want to do more here we should put the core code in charge of
deciding of a device will be PASID enabled and the IOMMU driver only
indicates if it can be PASID supported.

>   For PCI devices, we can check if the pasid cap is enabled from device
>   config space. But for non-PCI PASID support (e.g. some ARM platform), I
>   don't know if there is any way to check the PASID enabled or not. Or, to
>   cover both, we need an iommu API to check PASID enabled or not?

Yes, some iommu API, I suggest a flag in the common iommu_device. We
already have max_pasids there, it may already be nearly enough.
 
> - Nest parent domain should never be pasid-compatible?

Up to the driver.

>   I think the AMD iommu uses the V1 page table format for the parent
>   domain. Hence parent domain should not be allocated with the
>   IOMMU_HWPT_ALLOC_PASID flag. Otherwise, it does not work. Should this
>   be enforced in iommufd?

Enforced in the driver.

iommufd should enforce that the domain was created with
IOMMU_HWPT_ALLOC_PASID before passing the HWPT to any pasid
attach/replace function.

Jason

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/7 01:58, Jason Gunthorpe wrote:
> On Fri, Dec 06, 2024 at 03:57:39PM +0800, Yi Liu wrote:
>> Hi Jason, Vasant,
>>
>> When cooking new version, I got three opens on enforcing using
>> pasid-compatible domains to pasid-capable device in iommufd as suggested
>> in [1].
>>
>> - Concept problem when considering nested domain
>>    IIUC. pasid-compatible domain means the domain can be attached to PASID.
>>    e.g. AMD requires using V2 page table hence it can be configed to GCR3.
>>    However, the nested domain uses both V1 and V2 page table, I don't think
>>    it can be attached to PASID on AMD. So nested domain can not be
>>    considered as pasid-compatible.
> 
> Yes.
> 
>>    Based on this, this enforcement only
>>    applies to paging-domains. If so, do we still need to enforce it in
>>    iommufd? Will it simpler to let the AMD iommu driver to deal it?
> 
> I think driver should deal with it, Intel doesn't have that
> limitation. I sent patches to fix that detection for AMD and ARM
> already.

ok.

> 
>> - PASID-capable device v.s. PASID-enabled device
>>    We keep saying PASID-capable, but system may not enable it. Would it
>>    better enforce the pasid-compatible domain for PASID-enabled device?
>>    Seems all iommu vendor will enable PASID if it's supported. But
>>    conceptly, it is be more accurate if only do it when PASID is
>>    enabled.
> 
> If we want to do more here we should put the core code in charge of
> deciding of a device will be PASID enabled and the IOMMU driver only
> indicates if it can be PASID supported.
> 
>>    For PCI devices, we can check if the pasid cap is enabled from device
>>    config space. But for non-PCI PASID support (e.g. some ARM platform), I
>>    don't know if there is any way to check the PASID enabled or not. Or, to
>>    cover both, we need an iommu API to check PASID enabled or not?
> 
> Yes, some iommu API, I suggest a flag in the common iommu_device. We
> already have max_pasids there, it may already be nearly enough.

yeah, the dev->iommu->max_pasids indicates if a device can enable pasid
or not. It already counts in the iommu support. Since all known iommu
drivers will enable it once it is supported, can we say
dev->iommu->max_pasids also means enabled? If so, in the HW_INFO path[1],
we only need check it instead of checking pci config space.

[1] https://lore.kernel.org/kvm/20241108121742.18889-6-yi.l.liu@intel.com/

>> - Nest parent domain should never be pasid-compatible?
> 
> Up to the driver.
> 
>>    I think the AMD iommu uses the V1 page table format for the parent
>>    domain. Hence parent domain should not be allocated with the
>>    IOMMU_HWPT_ALLOC_PASID flag. Otherwise, it does not work. Should this
>>    be enforced in iommufd?
> 
> Enforced in the driver.

ok. BTW. Should we update the below description to be "the rule is only
applied to the domains that will be attached to pasid-capable device"?
Otherwise, a 'poor' userspace might consider any domains allocated for
pasid-capable device must use this flag.

  * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
  *                          domain can be attached to any PASID on the device.
  *                          Any domain attached to the non-PASID part of the
  *                          device must also be flaged, otherwise attaching a
  *                          PASID will blocked.
  *                          If IOMMU does not support PASID it will return
  *                          error (-EOPNOTSUPP).

> iommufd should enforce that the domain was created with
> IOMMU_HWPT_ALLOC_PASID before passing the HWPT to any pasid
> attach/replace function.

This seems much simpler enforcement than I did in this patch. I even
enforced the domains used in the non-pasid path to be flagged with
_ALLOC_PASID. But just as the beginning of this email, the nested domains
on AMD is not able to be used by pasid, a sane userspace won't do it. So
such a domain won't appear in the pasid path on AMD platform. But it can be
on Intel as we support attaching nested domain to pasid. So we would need
the nested domain be flagged in order to pass this check. Looks like we
cannot do this enforcement in iommufd. Put it in the iommu drivers would be
better. Is it?

Regards,
Yi Liu

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Jason Gunthorpe]

On Sat, Dec 07, 2024 at 06:49:04PM +0800, Yi Liu wrote:
> yeah, the dev->iommu->max_pasids indicates if a device can enable pasid
> or not. It already counts in the iommu support. Since all known iommu
> drivers will enable it once it is supported, can we say
> dev->iommu->max_pasids also means enabled? If so, in the HW_INFO path[1],
> we only need check it instead of checking pci config space.

That would be nice, and it is better than checking config space

> 
> > > - Nest parent domain should never be pasid-compatible?
> > 
> > Up to the driver.
> > 
> > >    I think the AMD iommu uses the V1 page table format for the parent
> > >    domain. Hence parent domain should not be allocated with the
> > >    IOMMU_HWPT_ALLOC_PASID flag. Otherwise, it does not work. Should this
> > >    be enforced in iommufd?
> > 
> > Enforced in the driver.
> 
> ok. BTW. Should we update the below description to be "the rule is only
> applied to the domains that will be attached to pasid-capable device"?
> Otherwise, a 'poor' userspace might consider any domains allocated for
> pasid-capable device must use this flag.
> 
>  * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
>  *                          domain can be attached to any PASID on the device.
>  *                          Any domain attached to the non-PASID part of the
>  *                          device must also be flaged, otherwise attaching a
>  *                          PASID will blocked.
>  *                          If IOMMU does not support PASID it will return
>  *                          error (-EOPNOTSUPP).

I'm not sure, I think we should not make it dependent on the device
capability. There may be multiple devices in the iommufd and some of
them may be PASID capable. The PASID capable domains should interwork
with all of the devices. Thus I'd also expect to be able to allocate a
PASID capable domain on a non-pasid capable device. Even though that
would be pointless on its own.

> > iommufd should enforce that the domain was created with
> > IOMMU_HWPT_ALLOC_PASID before passing the HWPT to any pasid
> > attach/replace function.
> 
> This seems much simpler enforcement than I did in this patch. I even
> enforced the domains used in the non-pasid path to be flagged with
> _ALLOC_PASID.

That seems good too, if it isn't too hard to do.

> on AMD is not able to be used by pasid, a sane userspace won't do it. So
> such a domain won't appear in the pasid path on AMD platform. But it can be
> on Intel as we support attaching nested domain to pasid. So we would need
> the nested domain be flagged in order to pass this check. Looks like we
> cannot do this enforcement in iommufd. Put it in the iommu drivers would be
> better. Is it?

Why can't it be in iommufd? A PASID domain should be a hwpt_paging
with the ALLOC_PASID flag, just put a bit in the hwpt_paging struct
and be done with it. That automatically rejects nested domains from
pasid.

I would like to keep this detail out of the drivers as I think drivers
will have a hard time getting it right. Drivers should implement their
HW restrictions and if they are more permissive than the iommufd model
that is OK.

We want some reasonable compromise to encourage applications to use
IOMMU_HWPT_ALLOC_PASID properly, but not build too much complexity to
reject driver-specific behavior.

Jason

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/9 22:57, Jason Gunthorpe wrote:
> On Sat, Dec 07, 2024 at 06:49:04PM +0800, Yi Liu wrote:
>> yeah, the dev->iommu->max_pasids indicates if a device can enable pasid
>> or not. It already counts in the iommu support. Since all known iommu
>> drivers will enable it once it is supported, can we say
>> dev->iommu->max_pasids also means enabled? If so, in the HW_INFO path[1],
>> we only need check it instead of checking pci config space.
> 
> That would be nice, and it is better than checking config space
> 
>>
>>>> - Nest parent domain should never be pasid-compatible?
>>>
>>> Up to the driver.
>>>
>>>>     I think the AMD iommu uses the V1 page table format for the parent
>>>>     domain. Hence parent domain should not be allocated with the
>>>>     IOMMU_HWPT_ALLOC_PASID flag. Otherwise, it does not work. Should this
>>>>     be enforced in iommufd?
>>>
>>> Enforced in the driver.
>>
>> ok. BTW. Should we update the below description to be "the rule is only
>> applied to the domains that will be attached to pasid-capable device"?
>> Otherwise, a 'poor' userspace might consider any domains allocated for
>> pasid-capable device must use this flag.
>>
>>   * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
>>   *                          domain can be attached to any PASID on the device.
>>   *                          Any domain attached to the non-PASID part of the
>>   *                          device must also be flaged, otherwise attaching a
>>   *                          PASID will blocked.
>>   *                          If IOMMU does not support PASID it will return
>>   *                          error (-EOPNOTSUPP).
> 
> I'm not sure, I think we should not make it dependent on the device
> capability. There may be multiple devices in the iommufd and some of
> them may be PASID capable. The PASID capable domains should interwork
> with all of the devices. Thus I'd also expect to be able to allocate a
> PASID capable domain on a non-pasid capable device. Even though that
> would be pointless on its own.

yes. I also had an offline email to confirm with Vasant, and he confirmed
a non-pasid capable device should be able to use pasid-capable domain (V2
page table.

As far as I know, allocating PASID cpable domain depends on whether
the underlying IOMMU hw supports the page table format (AMD IOMMU V2, Intel
VT-d Stage-1, ARM SMMUv3 Stage-1 page table?) or not. So even for a
non-pasid capable device, allocating pasid-capable domain should succeed if
its IOMMU supports the page table format.

>>> iommufd should enforce that the domain was created with
>>> IOMMU_HWPT_ALLOC_PASID before passing the HWPT to any pasid
>>> attach/replace function.
>>
>> This seems much simpler enforcement than I did in this patch. I even
>> enforced the domains used in the non-pasid path to be flagged with
>> _ALLOC_PASID.
> 
> That seems good too, if it isn't too hard to do.

The non-pasid and pasid shares much code, I think it should be doable.

>> on AMD is not able to be used by pasid, a sane userspace won't do it. So
>> such a domain won't appear in the pasid path on AMD platform. But it can be
>> on Intel as we support attaching nested domain to pasid. So we would need
>> the nested domain be flagged in order to pass this check. Looks like we
>> cannot do this enforcement in iommufd. Put it in the iommu drivers would be
>> better. Is it?
> 
> Why can't it be in iommufd? A PASID domain should be a hwpt_paging
> with the ALLOC_PASID flag, just put a bit in the hwpt_paging struct
> and be done with it. That automatically rejects nested domains from
> pasid.

The problem is Intel side, we allow attaching nested domains to pasid. :(
That's why I'm asking for updating the description of ALLOC_PASID and
the enforcement to be only applicable to paging domains, not applicable for
nested domains.

> I would like to keep this detail out of the drivers as I think drivers
> will have a hard time getting it right. Drivers should implement their
> HW restrictions and if they are more permissive than the iommufd model
> that is OK.
> 
> We want some reasonable compromise to encourage applications to use
> IOMMU_HWPT_ALLOC_PASID properly, but not build too much complexity to
> reject driver-specific behavior.

I'm ok to do it in iommufd as long as it is only applicable to hwpt_paging.
Otherwise, attaching nested domain to pasid would be failed according to
the aforementioned enforcement.

-- 
Regards,
Yi Liu

RE: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, December 10, 2024 11:15 AM
> 
> On 2024/12/9 22:57, Jason Gunthorpe wrote:
> >
> > I'm not sure, I think we should not make it dependent on the device
> > capability. There may be multiple devices in the iommufd and some of
> > them may be PASID capable. The PASID capable domains should interwork
> > with all of the devices. Thus I'd also expect to be able to allocate a
> > PASID capable domain on a non-pasid capable device. Even though that
> > would be pointless on its own.
> 
> yes. I also had an offline email to confirm with Vasant, and he confirmed
> a non-pasid capable device should be able to use pasid-capable domain (V2
> page table.

It's hard to think any vendor would want to that type of restriction.
whatever format adopted is purely IOMMU internal thing.

> >
> > We want some reasonable compromise to encourage applications to use
> > IOMMU_HWPT_ALLOC_PASID properly, but not build too much complexity
> to
> > reject driver-specific behavior.
> 
> I'm ok to do it in iommufd as long as it is only applicable to hwpt_paging.
> Otherwise, attaching nested domain to pasid would be failed according to
> the aforementioned enforcement.
> 

IMHO we may want to have a general enforcement in IOMMUFD that
any domain (paging or nested) must have ALLOC_PASID set to be
used in pasid-oriented operations.

drivers can have more restrictions, e.g. for arm/amd allocating a nested
domain with that bit set will fail at the beginning.

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Jason Gunthorpe]

On Tue, Dec 10, 2024 at 11:15:13AM +0800, Yi Liu wrote:

> > Why can't it be in iommufd? A PASID domain should be a hwpt_paging
> > with the ALLOC_PASID flag, just put a bit in the hwpt_paging struct
> > and be done with it. That automatically rejects nested domains from
> > pasid.
> 
> The problem is Intel side, we allow attaching nested domains to pasid. :(
> That's why I'm asking for updating the description of ALLOC_PASID and
> the enforcement to be only applicable to paging domains, not applicable for
> nested domains.

Seems reasonable

Jason

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/11 16:46, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, December 10, 2024 11:15 AM
>>
>> On 2024/12/9 22:57, Jason Gunthorpe wrote:
>>>
>>> I'm not sure, I think we should not make it dependent on the device
>>> capability. There may be multiple devices in the iommufd and some of
>>> them may be PASID capable. The PASID capable domains should interwork
>>> with all of the devices. Thus I'd also expect to be able to allocate a
>>> PASID capable domain on a non-pasid capable device. Even though that
>>> would be pointless on its own.
>>
>> yes. I also had an offline email to confirm with Vasant, and he confirmed
>> a non-pasid capable device should be able to use pasid-capable domain (V2
>> page table.
> 
> It's hard to think any vendor would want to that type of restriction.
> whatever format adopted is purely IOMMU internal thing.
> 
>>>
>>> We want some reasonable compromise to encourage applications to use
>>> IOMMU_HWPT_ALLOC_PASID properly, but not build too much complexity
>> to
>>> reject driver-specific behavior.
>>
>> I'm ok to do it in iommufd as long as it is only applicable to hwpt_paging.
>> Otherwise, attaching nested domain to pasid would be failed according to
>> the aforementioned enforcement.
>>
> 
> IMHO we may want to have a general enforcement in IOMMUFD that
> any domain (paging or nested) must have ALLOC_PASID set to be
> used in pasid-oriented operations.
> 
> drivers can have more restrictions, e.g. for arm/amd allocating a nested
> domain with that bit set will fail at the beginning.

ARM/AMD should allow allocating nested domain with this flag. Otherwise,
it does not suit the ALLOC_PASID definition. It requires both the PASID
path and non-PASID path to use pasid-compat domain.

So maybe we should not stick with the initial purpose of ALLOC_PASID flag.
It actually means selecting V2 page table. But the definition of it allows
us to consider the nested domains to be pasid-compat as Intel allows it.
And, a sane userspace running on ARM/AMD will never attach nested domain
to PASIDs. Even it does, the ARM SMMU and AMD iommu driver can fail such
attempts. In this way, we can enforce the ALLOC_PASID flag for any domains
used by PASID-capable devices in iommufd. This suits the existing
ALLOC_PASID definition as well.

@Jason, your opinion? With this open closed, I can update this series.

Regards,
Yi Liu

RE: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, December 12, 2024 11:15 AM
> 
> On 2024/12/11 16:46, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Tuesday, December 10, 2024 11:15 AM
> >>
> >> On 2024/12/9 22:57, Jason Gunthorpe wrote:
> >>>
> >>> We want some reasonable compromise to encourage applications to use
> >>> IOMMU_HWPT_ALLOC_PASID properly, but not build too much
> complexity
> >> to
> >>> reject driver-specific behavior.
> >>
> >> I'm ok to do it in iommufd as long as it is only applicable to hwpt_paging.
> >> Otherwise, attaching nested domain to pasid would be failed according to
> >> the aforementioned enforcement.
> >>
> >
> > IMHO we may want to have a general enforcement in IOMMUFD that
> > any domain (paging or nested) must have ALLOC_PASID set to be
> > used in pasid-oriented operations.
> >
> > drivers can have more restrictions, e.g. for arm/amd allocating a nested
> > domain with that bit set will fail at the beginning.
> 
> ARM/AMD should allow allocating nested domain with this flag. Otherwise,
> it does not suit the ALLOC_PASID definition. It requires both the PASID
> path and non-PASID path to use pasid-compat domain.

hmm the main point you raised at the beginning was that ARM/AMD
doesn't support the flag on nested domain, given the CD/PASID table
is a per-RID thing.

> 
> So maybe we should not stick with the initial purpose of ALLOC_PASID flag.
> It actually means selecting V2 page table. But the definition of it allows
> us to consider the nested domains to be pasid-compat as Intel allows it.
> And, a sane userspace running on ARM/AMD will never attach nested
> domain
> to PASIDs. Even it does, the ARM SMMU and AMD iommu driver can fail such
> attempts. In this way, we can enforce the ALLOC_PASID flag for any domains
> used by PASID-capable devices in iommufd. This suits the existing
> ALLOC_PASID definition as well.

Isn't it what I was suggesting? IOMMUFD just enforces that flag must
be set if a domain will be attached to PASID, and drivers will do
additional restrictions e.g. AMD/ARM allows the flag only on paging
domain while VT-d allows it for any type.

> 
> @Jason, your opinion? With this open closed, I can update this series.
> 
> Regards,
> Yi Liu

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/12 13:51, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, December 12, 2024 11:15 AM
>>
>> On 2024/12/11 16:46, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Tuesday, December 10, 2024 11:15 AM
>>>>
>>>> On 2024/12/9 22:57, Jason Gunthorpe wrote:
>>>>>
>>>>> We want some reasonable compromise to encourage applications to use
>>>>> IOMMU_HWPT_ALLOC_PASID properly, but not build too much
>> complexity
>>>> to
>>>>> reject driver-specific behavior.
>>>>
>>>> I'm ok to do it in iommufd as long as it is only applicable to hwpt_paging.
>>>> Otherwise, attaching nested domain to pasid would be failed according to
>>>> the aforementioned enforcement.
>>>>
>>>
>>> IMHO we may want to have a general enforcement in IOMMUFD that
>>> any domain (paging or nested) must have ALLOC_PASID set to be
>>> used in pasid-oriented operations.
>>>
>>> drivers can have more restrictions, e.g. for arm/amd allocating a nested
>>> domain with that bit set will fail at the beginning.
>>
>> ARM/AMD should allow allocating nested domain with this flag. Otherwise,
>> it does not suit the ALLOC_PASID definition. It requires both the PASID
>> path and non-PASID path to use pasid-compat domain.
> 
> hmm the main point you raised at the beginning was that ARM/AMD
> doesn't support the flag on nested domain, given the CD/PASID table
> is a per-RID thing.

yes.

> 
>>
>> So maybe we should not stick with the initial purpose of ALLOC_PASID flag.
>> It actually means selecting V2 page table. But the definition of it allows
>> us to consider the nested domains to be pasid-compat as Intel allows it.
>> And, a sane userspace running on ARM/AMD will never attach nested
>> domain
>> to PASIDs. Even it does, the ARM SMMU and AMD iommu driver can fail such
>> attempts. In this way, we can enforce the ALLOC_PASID flag for any domains
>> used by PASID-capable devices in iommufd. This suits the existing
>> ALLOC_PASID definition as well.
> 
> Isn't it what I was suggesting? IOMMUFD just enforces that flag must
> be set if a domain will be attached to PASID, and drivers will do
> additional restrictions e.g. AMD/ARM allows the flag only on paging
> domain while VT-d allows it for any type.

A slight difference. :) I think we also need to enforce it for the
non-PASID path. If not, the PASID path cannot work according to the
ALLOC_PASID definition. But we are on the same page about the additional
restrictions in ARM/AMD drivers about the nested domain used on PASIDs.
This is supposed to be done in attach phase instead of domain allocation
time.

-- 
Regards,
Yi Liu

RE: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, December 12, 2024 3:13 PM
> 
> On 2024/12/12 13:51, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Thursday, December 12, 2024 11:15 AM
> >>
> >> So maybe we should not stick with the initial purpose of ALLOC_PASID flag.
> >> It actually means selecting V2 page table. But the definition of it allows
> >> us to consider the nested domains to be pasid-compat as Intel allows it.
> >> And, a sane userspace running on ARM/AMD will never attach nested
> >> domain
> >> to PASIDs. Even it does, the ARM SMMU and AMD iommu driver can fail
> such
> >> attempts. In this way, we can enforce the ALLOC_PASID flag for any
> domains
> >> used by PASID-capable devices in iommufd. This suits the existing
> >> ALLOC_PASID definition as well.
> >
> > Isn't it what I was suggesting? IOMMUFD just enforces that flag must
> > be set if a domain will be attached to PASID, and drivers will do
> > additional restrictions e.g. AMD/ARM allows the flag only on paging
> > domain while VT-d allows it for any type.
> 
> A slight difference. :) I think we also need to enforce it for the
> non-PASID path. If not, the PASID path cannot work according to the
> ALLOC_PASID definition. But we are on the same page about the additional
> restrictions in ARM/AMD drivers about the nested domain used on PASIDs.
> This is supposed to be done in attach phase instead of domain allocation
> time.
> 

Here is my full picture:

At domain allocation the driver should decide whether the setting of
ALLOC_PASID is compatible to the given domain type.

If paging and iommu supports pasid then ALLOC_PASID is allowed. This
applies to all drivers. AMD driver will further select V1 vs. V2 according
to the flag bit.

If nesting, AMR/ARM drivers will reject the bit as a CD/PASID table
cannot be attached to a PASID. Intel driver allows it if pasid is supported
by iommu.

At attach phase, a domain with ALLOC_PASID can be attached to RID
of any device no matter the device supports pasid or not. But a domain
must have ALLOC_PASID set for attaching to a PASID (if the device has
non-zero max_pasids), enforced by iommufd.

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/13 10:43, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, December 12, 2024 3:13 PM
>>
>> On 2024/12/12 13:51, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Thursday, December 12, 2024 11:15 AM
>>>>
>>>> So maybe we should not stick with the initial purpose of ALLOC_PASID flag.
>>>> It actually means selecting V2 page table. But the definition of it allows
>>>> us to consider the nested domains to be pasid-compat as Intel allows it.
>>>> And, a sane userspace running on ARM/AMD will never attach nested
>>>> domain
>>>> to PASIDs. Even it does, the ARM SMMU and AMD iommu driver can fail
>> such
>>>> attempts. In this way, we can enforce the ALLOC_PASID flag for any
>> domains
>>>> used by PASID-capable devices in iommufd. This suits the existing
>>>> ALLOC_PASID definition as well.
>>>
>>> Isn't it what I was suggesting? IOMMUFD just enforces that flag must
>>> be set if a domain will be attached to PASID, and drivers will do
>>> additional restrictions e.g. AMD/ARM allows the flag only on paging
>>> domain while VT-d allows it for any type.
>>
>> A slight difference. :) I think we also need to enforce it for the
>> non-PASID path. If not, the PASID path cannot work according to the
>> ALLOC_PASID definition. But we are on the same page about the additional
>> restrictions in ARM/AMD drivers about the nested domain used on PASIDs.
>> This is supposed to be done in attach phase instead of domain allocation
>> time.
>>
> 
> Here is my full picture:
> 
> At domain allocation the driver should decide whether the setting of
> ALLOC_PASID is compatible to the given domain type.
> 
> If paging and iommu supports pasid then ALLOC_PASID is allowed. This
> applies to all drivers. AMD driver will further select V1 vs. V2 according
> to the flag bit.
> 
> If nesting, AMR/ARM drivers will reject the bit as a CD/PASID table
> cannot be attached to a PASID. Intel driver allows it if pasid is supported
> by iommu.

Following your opinion, I think the enforcement is something like this,
it only checks pasid_compat for the PASID path.

+	if (idev->dev->iommu->max_pasids && pasid != IOMMU_NO_PASID && 
!hwpt->pasid_compat)
+		return -EINVAL;

This means the RID path is not surely be attached to pasid-comapt domain
or not. either iommufd or iommu driver should do across check between the
RID and PASID path. It is failing attaching non-pasid compat domain to RID
if PASID has been attached, and vice versa, attaching PASIDs should be
failed if RID has been attached to non pasid comapt domain. I doubt if this
can be done easily as there is no lock between RID and PASID paths. Maybe
it can still be done by enforcing pasid-comapt for pasid-capable device.
But this may be done in iommu drivers? If still done in iommufd. It would
be like:

+	if (idev->dev->iommu->max_pasids && !hwpt->pasid_compat)
+		return -EINVAL;

If so, ARM/AMD drivers need to allow allocating nested domain with
ALLOC_PASID flag. If not, attaching nested domain to RID would be failed.
That's why I intend to allow it, while let ARM/AMD drivers fail the
attempt of attaching nested domain to PASIDs.

> At attach phase, a domain with ALLOC_PASID can be attached to RID
> of any device no matter the device supports pasid or not. But a domain
> must have ALLOC_PASID set for attaching to a PASID (if the device has
> non-zero max_pasids), enforced by iommufd.


Regards,
Yi Liu

RE: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, December 13, 2024 3:20 PM
> 
> On 2024/12/13 10:43, Tian, Kevin wrote:
> >
> > Here is my full picture:
> >
> > At domain allocation the driver should decide whether the setting of
> > ALLOC_PASID is compatible to the given domain type.
> >
> > If paging and iommu supports pasid then ALLOC_PASID is allowed. This
> > applies to all drivers. AMD driver will further select V1 vs. V2 according
> > to the flag bit.
> >
> > If nesting, AMR/ARM drivers will reject the bit as a CD/PASID table
> > cannot be attached to a PASID. Intel driver allows it if pasid is supported
> > by iommu.
> 
> Following your opinion, I think the enforcement is something like this,
> it only checks pasid_compat for the PASID path.
> 
> +	if (idev->dev->iommu->max_pasids && pasid != IOMMU_NO_PASID
> &&
> !hwpt->pasid_compat)
> +		return -EINVAL;

shouldn't it be:

	if (!idev->dev->iommu->max_pasids ||
	     pasid == IOMMU_NO_PASID ||
	     !hwpt->pasid_compat)
		return -EINVAL;

?

> 
> This means the RID path is not surely be attached to pasid-comapt domain
> or not. either iommufd or iommu driver should do across check between the
> RID and PASID path. It is failing attaching non-pasid compat domain to RID
> if PASID has been attached, and vice versa, attaching PASIDs should be
> failed if RID has been attached to non pasid comapt domain. I doubt if this
> can be done easily as there is no lock between RID and PASID paths. 

I'm not sure where that requirement comes from. Does AMD require RID
and PASID to use the same format when nesting is disabled? If yes, that's
still a driver burden to handle, not iommufd's...

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/13 15:52, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, December 13, 2024 3:20 PM
>>
>> On 2024/12/13 10:43, Tian, Kevin wrote:
>>>
>>> Here is my full picture:
>>>
>>> At domain allocation the driver should decide whether the setting of
>>> ALLOC_PASID is compatible to the given domain type.
>>>
>>> If paging and iommu supports pasid then ALLOC_PASID is allowed. This
>>> applies to all drivers. AMD driver will further select V1 vs. V2 according
>>> to the flag bit.
>>>
>>> If nesting, AMR/ARM drivers will reject the bit as a CD/PASID table
>>> cannot be attached to a PASID. Intel driver allows it if pasid is supported
>>> by iommu.
>>
>> Following your opinion, I think the enforcement is something like this,
>> it only checks pasid_compat for the PASID path.
>>
>> +	if (idev->dev->iommu->max_pasids && pasid != IOMMU_NO_PASID
>> &&
>> !hwpt->pasid_compat)
>> +		return -EINVAL;
> 
> shouldn't it be:
> 
> 	if (!idev->dev->iommu->max_pasids ||
> 	     pasid == IOMMU_NO_PASID ||
> 	     !hwpt->pasid_compat)
> 		return -EINVAL;
> 
> ?

no, this check is added in a common place shared by RID and PASID path. If
it is added in place only for the PASID, it should be something like you
wrote.

>>
>> This means the RID path is not surely be attached to pasid-comapt domain
>> or not. either iommufd or iommu driver should do across check between the
>> RID and PASID path. It is failing attaching non-pasid compat domain to RID
>> if PASID has been attached, and vice versa, attaching PASIDs should be
>> failed if RID has been attached to non pasid comapt domain. I doubt if this
>> can be done easily as there is no lock between RID and PASID paths.
> 
> I'm not sure where that requirement comes from. Does AMD require RID
> and PASID to use the same format when nesting is disabled? If yes, that's
> still a driver burden to handle, not iommufd's...

yes, I've asked this question [1]. AMD requires the RID and PASID use the
same format. I agree it's a driver's burden but now it's defined in the
ALLOC_PASID. So, I doubt if it becomes a common requirement to all the
iommu drivers. Otherwise the ALLOC_PASID definition is broken. e.g. Intel
may have no need to enforce it, but it would be like Intel is breaking
it.

-- 
Regards,
Yi Liu

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/13 16:11, Yi Liu wrote:
> On 2024/12/13 15:52, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Friday, December 13, 2024 3:20 PM
>>>
>>> On 2024/12/13 10:43, Tian, Kevin wrote:
>>>>
>>>> Here is my full picture:
>>>>
>>>> At domain allocation the driver should decide whether the setting of
>>>> ALLOC_PASID is compatible to the given domain type.
>>>>
>>>> If paging and iommu supports pasid then ALLOC_PASID is allowed. This
>>>> applies to all drivers. AMD driver will further select V1 vs. V2 according
>>>> to the flag bit.
>>>>
>>>> If nesting, AMR/ARM drivers will reject the bit as a CD/PASID table
>>>> cannot be attached to a PASID. Intel driver allows it if pasid is 
>>>> supported
>>>> by iommu.
>>>
>>> Following your opinion, I think the enforcement is something like this,
>>> it only checks pasid_compat for the PASID path.
>>>
>>> +    if (idev->dev->iommu->max_pasids && pasid != IOMMU_NO_PASID
>>> &&
>>> !hwpt->pasid_compat)
>>> +        return -EINVAL;
>>
>> shouldn't it be:
>>
>>     if (!idev->dev->iommu->max_pasids ||
>>          pasid == IOMMU_NO_PASID ||
>>          !hwpt->pasid_compat)
>>         return -EINVAL;
>>
>> ?
> 
> no, this check is added in a common place shared by RID and PASID path. If
> it is added in place only for the PASID, it should be something like you
> wrote.
> 
>>>
>>> This means the RID path is not surely be attached to pasid-comapt domain
>>> or not. either iommufd or iommu driver should do across check between the
>>> RID and PASID path. It is failing attaching non-pasid compat domain to RID
>>> if PASID has been attached, and vice versa, attaching PASIDs should be
>>> failed if RID has been attached to non pasid comapt domain. I doubt if this
>>> can be done easily as there is no lock between RID and PASID paths.
>>
>> I'm not sure where that requirement comes from. Does AMD require RID
>> and PASID to use the same format when nesting is disabled? If yes, that's
>> still a driver burden to handle, not iommufd's...
> 
> yes, I've asked this question [1]. AMD requires the RID and PASID use the
> same format. I agree it's a driver's burden but now it's defined in the
> ALLOC_PASID. So, I doubt if it becomes a common requirement to all the
> iommu drivers. Otherwise the ALLOC_PASID definition is broken. e.g. Intel
> may have no need to enforce it, but it would be like Intel is breaking
> it.

aha, forgot the link.

[1] https://lore.kernel.org/linux-iommu/20240822124433.GD3468552@ziepe.ca/

-- 
Regards,
Yi Liu

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Jason Gunthorpe]

On Fri, Dec 13, 2024 at 07:52:40AM +0000, Tian, Kevin wrote:

> I'm not sure where that requirement comes from. Does AMD require RID
> and PASID to use the same format when nesting is disabled? If yes, that's
> still a driver burden to handle, not iommufd's...

Yes, ARM and AMD require this too

The point of the iommufd enforcement of ALLOC_PAGING is to try to
discourage bad apps - ie apps that only work on Intel. We can check
the rid attach too if it is easy to do

Jason 

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/13 20:40, Jason Gunthorpe wrote:
> On Fri, Dec 13, 2024 at 07:52:40AM +0000, Tian, Kevin wrote:
> 
>> I'm not sure where that requirement comes from. Does AMD require RID
>> and PASID to use the same format when nesting is disabled? If yes, that's
>> still a driver burden to handle, not iommufd's...
> 
> Yes, ARM and AMD require this too
> 
> The point of the iommufd enforcement of ALLOC_PAGING is to try to
> discourage bad apps - ie apps that only work on Intel. We can check
> the rid attach too if it is easy to do

I have an easy way to enforce RID attach. It is:

If the device is device capable, I would enforce all domains for this
device (either RID or PASID) be flagged. The device capable info is static,
so no need to add extra lock across the RID and PASID attach paths for the
page table format alignment. This has only one drawback. If userspace is
not going to use PASID, it still needs to allocated domain with this flag.
I think AMD may need to confirm if it is acceptable.

@Kevin, I'd like to echo the prior suggestion for nested domain. It looks
hard to apply the pasid enforcement on it. So I'd like to limit the
ALLOC_PASID flag to paging domains. Also, I doubt if the uapi needs to
mandate the RID part to use this flag. It appears to me it can be done
iommu drivers. If so, no need to mandate it in uapi. So I'm considering
to do below changes to IOMMU_HWPT_ALLOC_PASID. The new definition does not
mandate the RID part of devices, and leaves it to vendors. Hence, the
iommufd only needs to ensure the paging domains used by PASID should be
flagged. e.g. Intel won't fail PASID attach even its RID is using a domain
that is not flagged (e.g. nested domain, under the new definition, nested
domain does not use this flag). While, AMD would fail it if the RID domain
is not using this flag. This has one more benefit, it leaves the
flexibility of using pasid or not to user.

diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 0e27557fb86b..a1a11041d941 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -387,19 +387,20 @@ struct iommu_vfio_ioas {
   *                                   enforced on device attachment
   * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
   *                             valid.
- * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
- *                          domain can be attached to any PASID on the device.
- *                          Any domain attached to the non-PASID part of the
- *                          device must also be flagged, otherwise attaching a
- *                          PASID will blocked.
- *                          If IOMMU does not support PASID it will return
- *                          error (-EOPNOTSUPP).
+ * @IOMMU_HWPT_ALLOC_PAGING_PASID: Requests a paging domain that can be used
+ *                                 with PASID. The domain can be attached to
+ *                                 any PASID on the device. Vendors may 
require
+ *                                 the non-PASID part of the device use this
+ *                                 flag as well. If yes, attaching a PASID 
will
+ *                                 blocked if non-PASID part is not using it.
+ *                                 If IOMMU does not support PASID it will
+ *                                 return error (-EOPNOTSUPP).
   */
  enum iommufd_hwpt_alloc_flags {
  	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
  	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
  	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
-	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
+	IOMMU_HWPT_ALLOC_PAGING_PASID = 1 << 3,
  };

  /**
-- 
Regards,
Yi Liu

RE: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Saturday, December 14, 2024 5:04 PM
> 
> On 2024/12/13 20:40, Jason Gunthorpe wrote:
> > On Fri, Dec 13, 2024 at 07:52:40AM +0000, Tian, Kevin wrote:
> >
> >> I'm not sure where that requirement comes from. Does AMD require RID
> >> and PASID to use the same format when nesting is disabled? If yes, that's
> >> still a driver burden to handle, not iommufd's...
> >
> > Yes, ARM and AMD require this too
> >
> > The point of the iommufd enforcement of ALLOC_PAGING is to try to
> > discourage bad apps - ie apps that only work on Intel. We can check
> > the rid attach too if it is easy to do
> 
> I have an easy way to enforce RID attach. It is:
> 
> If the device is device capable, I would enforce all domains for this
> device (either RID or PASID) be flagged. The device capable info is static,
> so no need to add extra lock across the RID and PASID attach paths for the
> page table format alignment. This has only one drawback. If userspace is
> not going to use PASID, it still needs to allocated domain with this flag.
> I think AMD may need to confirm if it is acceptable.

It's simple but break applications which are not aware of PASID. Ideally
if the app is not interested in PASID it has no business to query the PASID
cap and then set the flag accordingly.

> 
> @Kevin, I'd like to echo the prior suggestion for nested domain. It looks
> hard to apply the pasid enforcement on it. So I'd like to limit the
> ALLOC_PASID flag to paging domains. Also, I doubt if the uapi needs to
> mandate the RID part to use this flag. It appears to me it can be done
> iommu drivers. If so, no need to mandate it in uapi. So I'm considering
> to do below changes to IOMMU_HWPT_ALLOC_PASID. The new definition
> does not
> mandate the RID part of devices, and leaves it to vendors. Hence, the
> iommufd only needs to ensure the paging domains used by PASID should be
> flagged. e.g. Intel won't fail PASID attach even its RID is using a domain
> that is not flagged (e.g. nested domain, under the new definition, nested
> domain does not use this flag). While, AMD would fail it if the RID domain
> is not using this flag. This has one more benefit, it leaves the
> flexibility of using pasid or not to user.
> 
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 0e27557fb86b..a1a11041d941 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -387,19 +387,20 @@ struct iommu_vfio_ioas {
>    *                                   enforced on device attachment
>    * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation
> data is
>    *                             valid.
> - * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used
> with PASID. The
> - *                          domain can be attached to any PASID on the device.
> - *                          Any domain attached to the non-PASID part of the
> - *                          device must also be flagged, otherwise attaching a
> - *                          PASID will blocked.
> - *                          If IOMMU does not support PASID it will return
> - *                          error (-EOPNOTSUPP).
> + * @IOMMU_HWPT_ALLOC_PAGING_PASID: Requests a paging domain that
> can be used
> + *                                 with PASID. The domain can be attached to
> + *                                 any PASID on the device. Vendors may
> require
> + *                                 the non-PASID part of the device use this
> + *                                 flag as well. If yes, attaching a PASID
> will
> + *                                 blocked if non-PASID part is not using it.
> + *                                 If IOMMU does not support PASID it will
> + *                                 return error (-EOPNOTSUPP).
>    */
>   enum iommufd_hwpt_alloc_flags {
>   	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>   	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
>   	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
> -	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
> +	IOMMU_HWPT_ALLOC_PAGING_PASID = 1 << 3,
>   };
> 

I'm afraid that doing so adds more confusion as one could easily
ask why such enforcement is only applied to the paging domain.

Please note the end result of nesting domain can still meet the
restriction.

For ARM/AMD the nesting domain attached to RID cannot set
ALLOC_PASID so it cannot be attached to PASID later.

For Intel a nesting domain attached to RID can have the flag
set or cleared. If the domain is intended to be attached to
a PASID later, then it must have the ALLOC_PASID set.

So I don't see a need of exempting the nesting domain here.

btw what about requiring to acquire &idev->igroup->lock
in the pasid path? It's not a performance critical path, and
by holding that lock in both RID/PASID attach, we can check
idev->pasid_hwpts to decide whether a domain attached to
RID must have the flag set and vice versa when doing pasid
attach whether idev->igroup->hwpt already has the flag set.

Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu]

On 2024/12/16 16:26, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Saturday, December 14, 2024 5:04 PM
>>
>> On 2024/12/13 20:40, Jason Gunthorpe wrote:
>>> On Fri, Dec 13, 2024 at 07:52:40AM +0000, Tian, Kevin wrote:
>>>
>>>> I'm not sure where that requirement comes from. Does AMD require RID
>>>> and PASID to use the same format when nesting is disabled? If yes, that's
>>>> still a driver burden to handle, not iommufd's...
>>>
>>> Yes, ARM and AMD require this too
>>>
>>> The point of the iommufd enforcement of ALLOC_PAGING is to try to
>>> discourage bad apps - ie apps that only work on Intel. We can check
>>> the rid attach too if it is easy to do
>>
>> I have an easy way to enforce RID attach. It is:
>>
>> If the device is device capable, I would enforce all domains for this
>> device (either RID or PASID) be flagged. The device capable info is static,
>> so no need to add extra lock across the RID and PASID attach paths for the
>> page table format alignment. This has only one drawback. If userspace is
>> not going to use PASID, it still needs to allocated domain with this flag.
>> I think AMD may need to confirm if it is acceptable.
> 
> It's simple but break applications which are not aware of PASID. Ideally
> if the app is not interested in PASID it has no business to query the PASID
> cap and then set the flag accordingly.

yes.

>>
>> @Kevin, I'd like to echo the prior suggestion for nested domain. It looks
>> hard to apply the pasid enforcement on it. So I'd like to limit the
>> ALLOC_PASID flag to paging domains. Also, I doubt if the uapi needs to
>> mandate the RID part to use this flag. It appears to me it can be done
>> iommu drivers. If so, no need to mandate it in uapi. So I'm considering
>> to do below changes to IOMMU_HWPT_ALLOC_PASID. The new definition
>> does not
>> mandate the RID part of devices, and leaves it to vendors. Hence, the
>> iommufd only needs to ensure the paging domains used by PASID should be
>> flagged. e.g. Intel won't fail PASID attach even its RID is using a domain
>> that is not flagged (e.g. nested domain, under the new definition, nested
>> domain does not use this flag). While, AMD would fail it if the RID domain
>> is not using this flag. This has one more benefit, it leaves the
>> flexibility of using pasid or not to user.
>>
>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>> index 0e27557fb86b..a1a11041d941 100644
>> --- a/include/uapi/linux/iommufd.h
>> +++ b/include/uapi/linux/iommufd.h
>> @@ -387,19 +387,20 @@ struct iommu_vfio_ioas {
>>     *                                   enforced on device attachment
>>     * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation
>> data is
>>     *                             valid.
>> - * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used
>> with PASID. The
>> - *                          domain can be attached to any PASID on the device.
>> - *                          Any domain attached to the non-PASID part of the
>> - *                          device must also be flagged, otherwise attaching a
>> - *                          PASID will blocked.
>> - *                          If IOMMU does not support PASID it will return
>> - *                          error (-EOPNOTSUPP).
>> + * @IOMMU_HWPT_ALLOC_PAGING_PASID: Requests a paging domain that
>> can be used
>> + *                                 with PASID. The domain can be attached to
>> + *                                 any PASID on the device. Vendors may
>> require
>> + *                                 the non-PASID part of the device use this
>> + *                                 flag as well. If yes, attaching a PASID
>> will
>> + *                                 blocked if non-PASID part is not using it.
>> + *                                 If IOMMU does not support PASID it will
>> + *                                 return error (-EOPNOTSUPP).
>>     */
>>    enum iommufd_hwpt_alloc_flags {
>>    	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>>    	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
>>    	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
>> -	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
>> +	IOMMU_HWPT_ALLOC_PAGING_PASID = 1 << 3,
>>    };
>>
> 
> I'm afraid that doing so adds more confusion as one could easily
> ask why such enforcement is only applied to the paging domain.
> 
> Please note the end result of nesting domain can still meet the
> restriction.
> 
> For ARM/AMD the nesting domain attached to RID cannot set
> ALLOC_PASID so it cannot be attached to PASID later.
> 
> For Intel a nesting domain attached to RID can have the flag
> set or cleared. If the domain is intended to be attached to
> a PASID later, then it must have the ALLOC_PASID set.
> 
> So I don't see a need of exempting the nesting domain here.
> 
> btw what about requiring to acquire &idev->igroup->lock
> in the pasid path? It's not a performance critical path, and
> by holding that lock in both RID/PASID attach, we can check
> idev->pasid_hwpts to decide whether a domain attached to
> RID must have the flag set and vice versa when doing pasid
> attach whether idev->igroup->hwpt already has the flag set.

seems workable although igroup->lock is supposed to protect the attach
of multi-device groups. Here we need to extend it to protect the
idev->pasid_hwpts as well. With this, I think we can apply the enforcement
to nested domains.

-- 
Regards,
Yi Liu