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

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

[Yi Liu]

PASID (Process Address Space ID) is a PCIe extension that tags the DMA
transactions from a physical device. Most modern IOMMU hardware supports
PASID-granular address translation. This allows a PASID-capable device
to be attached to multiple hardware page tables (hwpts, also known as
domains), with each attachment tagged by a PASID.

This series builds on previous series [1] [2]. It begins by adding a missing
IOMMU API to replace the domain for a PASID. Utilizing the IOMMU PASID
attach/replace/detach APIs, this series introduces iommufd APIs for device
drivers to attach, replace, or detach PASIDs to/from hwpts at the request
of userspace. It also enforces PASID compatibility with domain requirements,
allocates PASID-compatible hwpts in iommufd, and includes self-tests to
validate the iommufd APIs.

The complete code is available at the following link [3]. Please note that
the existing iommufd self-test was broken, and a temporary fix patch is at
the top of the branch [3]. If you wish to run the iommufd self-test, please
apply that fix. We apologize for any inconvenience.

[1] https://lore.kernel.org/linux-iommu/20250226011849.5102-1-yi.l.liu@intel.com/
[2] https://lore.kernel.org/linux-iommu/20250226050130.5814-1-yi.l.liu@intel.com/
[3] https://github.com/yiliu1765/iommufd/tree/iommufd_pasid

Change log:

v8:
 - Rebase on top of the dependency patch series
 - Check both handle and domain in the iommu_replace_device_pasid_handle()
   to support replace between domain and handle.
 - Fix a typo in patch 02 of v7 (Kevin)
 - Dropped patch 11 0f v7 as it is included in a series to add iommufd
   selftest to cover replace between domain and handle.
 - r-b tag on patch 01 of v7 (Kevin)
 - Add selftest for replace between handle and non-handle case

v7: https://lore.kernel.org/linux-iommu/20250216035228.23831-1-yi.l.liu@intel.com/
 - Remove the iommu_attach_handle related refactors, as they have been addressed
   by Nic's series.
 - Address the comments on patch 01 of v6 in a separate series [1]. Store either
   the domain or handle in group->pasid_array, and swap the order of setting
   group->pasid_array and invoking the attach operation of IOMMU drivers.
 - Introduce iommu_attach_device_pasid_handle() and iommu_replace_device_pasid_handle(),
   and remove iommu_replace_device_pasid() since iommufd consistently uses the
   _handle() API.
 - Add patch 04 to include reserved_iova only for the RID path, as the underlying
   helpers are shared by both the RID and PASID paths, but only the RID path needs
   to add reserved_iova.
 - Remove the iommu_dev.max_pasids check in patch 11 of v6. The mock IOMMU always
   supports 20-bit PASIDs, so there is no need to verify PASID support in the mock
   IOMMU driver. Additionally, the IOMMU_HWPT_ALLOC_PASID flag does not imply PASID
   support, so it should be removed to avoid misleading IOMMU driver programming.

v6: https://lore.kernel.org/linux-iommu/20241219132746.16193-1-yi.l.liu@intel.com/
 - Add kdoc to iommufd_device_get_attach_handle() to note the returned handle
   should be used with care. (Baolu)
 - Reworked the patch 07 and 08 of v5 to avoid domain allocation failure on VT-d
   after applying patch 07 of v5.
     1) Split out the intel iommu driver IOMMU_HWPT_ALLOC_PASID support out of
	patch 08
     2) Rework the PASID-compatible domain enforcement by checking the RID domain
	and idev->pasid_hwpts under the idev->igroup->lock.
 - iommufd_device_pasid_do_attach() returns -EINVAL if there is old hwpt and it's
   not the same with new hwpt. This aligns with how the iommufd_device_do_attach()
   deals it. Otherwise, attaching the same pasid to the same ioas is going to fail
   before the auto_domain loop goes to the correct hwpt. Thsi is not reasonable. So
   make this change.
 - Enhanced the pasid selftest to have non-pasid-capable device and pasid-capable
   device.
 - The order of the series is tweaked to be prepare the iommufd for pasid attach,
   add pasid attach, add PASID-compat domain enforcement and then add the PASID-compat
   hwpt allocation.
 - Rebased on top of 6.13-rc3 and some already applied patches.

v5: https://lore.kernel.org/linux-iommu/20241104132513.15890-1-yi.l.liu@intel.com/
 - 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: Add iommu_attach_device_pasid_handle()
  iommu: Introduce a replace API for device pasid
  iommufd: Pass @pasid through the device attach/replace path
  iommufd/device: Only add reserved_iova in non-pasid path
  iommufd: Mark PASID-compatible domain
  iommufd: Support pasid attach/replace
  iommufd: Enforce PASID-compatible domain for RID
  iommu/vt-d: Add IOMMU_HWPT_ALLOC_PASID support
  iommufd: Allow allocating PASID-compatible domain
  iommufd/selftest: Add set_dev_pasid in mock iommu
  iommufd/selftest: Add test ops to test pasid attach/detach
  iommufd/selftest: Add coverage for iommufd pasid attach/detach

 drivers/dma/idxd/init.c                       |   2 +-
 drivers/iommu/intel/iommu.c                   |   3 +-
 drivers/iommu/intel/nested.c                  |   2 +-
 drivers/iommu/iommu-priv.h                    |   4 +
 drivers/iommu/iommu-sva.c                     |  10 +-
 drivers/iommu/iommu.c                         | 121 +++++-
 drivers/iommu/iommufd/Makefile                |   1 +
 drivers/iommu/iommufd/device.c                | 139 ++++---
 drivers/iommu/iommufd/hw_pagetable.c          |  15 +-
 drivers/iommu/iommufd/iommufd_private.h       |  32 +-
 drivers/iommu/iommufd/iommufd_test.h          |  37 ++
 drivers/iommu/iommufd/pasid.c                 | 155 ++++++++
 drivers/iommu/iommufd/selftest.c              | 238 +++++++++++-
 include/linux/iommu.h                         |  34 +-
 include/linux/iommufd.h                       |   7 +
 tools/testing/selftests/iommu/iommufd.c       | 350 ++++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  46 ++-
 tools/testing/selftests/iommu/iommufd_utils.h | 123 ++++++
 18 files changed, 1230 insertions(+), 89 deletions(-)
 create mode 100644 drivers/iommu/iommufd/pasid.c

-- 
2.34.1

[PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu]

The existing iommu_attach_device_pasid() function allows both a valid
handle and a NULL handle, which is not consistent with the RID path where
iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
it, this adds iommu_attach_device_pasid_handle() to cover the case with
valid handle, while let the iommu_attach_device_pasid() only deals with
the case with NULL handle.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/dma/idxd/init.c   |  2 +-
 drivers/iommu/iommu-sva.c | 10 ++++++----
 drivers/iommu/iommu.c     |  8 ++++----
 include/linux/iommu.h     | 34 +++++++++++++++++++++++++++++-----
 4 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index b946f78f85e1..d11a763ef124 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -593,7 +593,7 @@ static int idxd_enable_system_pasid(struct idxd_device *idxd)
 	 * DMA domain is owned by the driver, it should support all valid
 	 * types such as DMA-FQ, identity, etc.
 	 */
-	ret = iommu_attach_device_pasid(domain, dev, pasid, NULL);
+	ret = iommu_attach_device_pasid(domain, dev, pasid);
 	if (ret) {
 		dev_err(dev, "failed to attach device pasid %d, domain type %d",
 			pasid, domain->type);
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 503c5d23c1ea..09d676ddf15e 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -115,8 +115,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 
 	/* Search for an existing domain. */
 	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
-		ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid,
-						&handle->handle);
+		ret = iommu_attach_device_pasid_handle(domain, dev,
+						       iommu_mm->pasid,
+						       &handle->handle);
 		if (!ret) {
 			domain->users++;
 			goto out;
@@ -130,8 +131,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 		goto out_free_handle;
 	}
 
-	ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid,
-					&handle->handle);
+	ret = iommu_attach_device_pasid_handle(domain, dev,
+					       iommu_mm->pasid,
+					       &handle->handle);
 	if (ret)
 		goto out_free_domain;
 	domain->users = 1;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 73a3b20b2ef9..f6dbb60ef948 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3354,9 +3354,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
  *
  * Return: 0 on success, or an error.
  */
-int iommu_attach_device_pasid(struct iommu_domain *domain,
-			      struct device *dev, ioasid_t pasid,
-			      struct iommu_attach_handle *handle)
+int __iommu_attach_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;
@@ -3415,7 +3415,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	mutex_unlock(&group->mutex);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
+EXPORT_SYMBOL_GPL(__iommu_attach_device_pasid);
 
 /*
  * iommu_detach_device_pasid() - Detach the domain from pasid of device
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 38c65e92ecd0..d795de7bad8f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1122,9 +1122,24 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
 int iommu_device_claim_dma_owner(struct device *dev, void *owner);
 void iommu_device_release_dma_owner(struct device *dev);
 
-int iommu_attach_device_pasid(struct iommu_domain *domain,
-			      struct device *dev, ioasid_t pasid,
-			      struct iommu_attach_handle *handle);
+int __iommu_attach_device_pasid(struct iommu_domain *domain,
+				struct device *dev, ioasid_t pasid,
+				struct iommu_attach_handle *handle);
+
+static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
+					    struct device *dev, ioasid_t pasid)
+{
+	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
+}
+
+static inline int
+iommu_attach_device_pasid_handle(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t pasid,
+				 struct iommu_attach_handle *handle)
+{
+	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
+}
+
 void iommu_detach_device_pasid(struct iommu_domain *domain,
 			       struct device *dev, ioasid_t pasid);
 ioasid_t iommu_alloc_global_pasid(struct device *dev);
@@ -1139,6 +1154,8 @@ struct iommu_fault_param {};
 struct iommu_iotlb_gather {};
 struct iommu_dirty_bitmap {};
 struct iommu_dirty_ops {};
+struct iommu_attach_handle {};
+
 
 static inline bool device_iommu_capable(struct device *dev, enum iommu_cap cap)
 {
@@ -1451,8 +1468,15 @@ static inline int iommu_device_claim_dma_owner(struct device *dev, void *owner)
 }
 
 static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
-					    struct device *dev, ioasid_t pasid,
-					    struct iommu_attach_handle *handle)
+					    struct device *dev, ioasid_t pasid)
+{
+	return -ENODEV;
+}
+
+static inline int
+iommu_attach_device_pasid_handle(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t pasid,
+				 struct iommu_attach_handle *handle)
 {
 	return -ENODEV;
 }
-- 
2.34.1

[PATCH v8 02/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_handle() 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      | 113 +++++++++++++++++++++++++++++++++++--
 2 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index b4508423e13b..e8e34be8140d 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -43,4 +43,8 @@ void iommu_detach_group_handle(struct iommu_domain *domain,
 int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle);
+
+int iommu_replace_device_pasid_handle(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_attach_handle *handle);
 #endif /* __LINUX_IOMMU_PRIV_H */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f6dbb60ef948..129f32303fc5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -513,6 +513,22 @@ static void iommu_deinit_device(struct device *dev)
 	dev_iommu_free(dev);
 }
 
+static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
+{
+	struct iommu_domain *domain;
+
+	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
+		struct iommu_attach_handle *handle;
+
+		handle = xa_untag_pointer(entry);
+		domain = handle->domain;
+	} else {
+		domain = xa_untag_pointer(entry);
+	}
+
+	return domain;
+}
+
 DEFINE_MUTEX(iommu_probe_device_lock);
 
 static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
@@ -3311,14 +3327,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;
 	}
@@ -3330,7 +3347,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;
 }
@@ -3396,7 +3426,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_release(&group->pasid_array, pasid);
 		goto out_unlock;
@@ -3417,6 +3447,81 @@ int __iommu_attach_device_pasid(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(__iommu_attach_device_pasid);
 
+/**
+ * iommu_replace_device_pasid_handle - 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. The @pasid should have been
+ * attached. Otherwise, this fails.
+ * The pasid will keep the old configuration if replacement failed.
+ * Return 0 on success, or an error.
+ */
+int iommu_replace_device_pasid_handle(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 *entry;
+	struct iommu_domain *curr_domain;
+	void *curr;
+	int ret;
+
+	if (!group)
+		return -ENODEV;
+
+	if (!domain->ops->set_dev_pasid)
+		return -EOPNOTSUPP;
+
+	if (dev_iommu_ops(dev) != domain->owner ||
+	    pasid == IOMMU_NO_PASID || !handle)
+		return -EINVAL;
+
+	mutex_lock(&group->mutex);
+	entry = iommu_make_pasid_array_entry(domain, handle);
+	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
+			  XA_ZERO_ENTRY, GFP_KERNEL);
+	if (xa_is_err(curr)) {
+		ret = xa_err(curr);
+		goto out_unlock;
+	}
+
+	/* Not a replace case */
+	if (!curr) {
+		xa_release(&group->pasid_array, pasid);
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	curr_domain = pasid_array_entry_to_domain(curr);
+	ret = 0;
+
+	if (curr_domain != domain) {
+		ret = __iommu_set_group_pasid(domain, group,
+					      pasid, curr_domain);
+		if (ret)
+			goto out_unlock;
+	}
+
+	if (curr != entry) {
+		/*
+		 * The above xa_cmpxchg() reserved the memory, and the
+		 * group->mutex is held, this cannot fail.
+		 */
+		WARN_ON(xa_is_err(xa_store(&group->pasid_array,
+					   pasid, entry, GFP_KERNEL)));
+	}
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid_handle, "IOMMUFD_INTERNAL");
+
 /*
  * iommu_detach_device_pasid() - Detach the domain from pasid of device
  * @domain: the iommu domain.
-- 
2.34.1

[PATCH v8 03/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          | 67 +++++++++++++++----------
 drivers/iommu/iommufd/hw_pagetable.c    |  5 +-
 drivers/iommu/iommufd/iommufd_private.h |  5 +-
 3 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 0786290b4056..6ec7f6935115 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -355,7 +355,8 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
 /* The device attach/detach/replace helpers for attach_handle */
 
 static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
-				      struct iommufd_device *idev)
+				      struct iommufd_device *idev,
+				      ioasid_t pasid)
 {
 	struct iommufd_attach_handle *handle;
 	int rc;
@@ -373,6 +374,7 @@ static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 	}
 
 	handle->idev = idev;
+	WARN_ON(pasid != IOMMU_NO_PASID);
 	rc = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,
 				       &handle->handle);
 	if (rc)
@@ -389,25 +391,28 @@ static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 }
 
 static 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;
 
 	lockdep_assert_held(&idev->igroup->lock);
 
 	handle =
-		iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0);
+		iommu_attach_handle_get(idev->igroup->group, pasid, 0);
 	if (IS_ERR(handle))
 		return NULL;
 	return to_iommufd_handle(handle);
 }
 
 static 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;
 
-	handle = iommufd_device_get_attach_handle(idev);
+	WARN_ON(pasid != IOMMU_NO_PASID);
+
+	handle = iommufd_device_get_attach_handle(idev, pasid);
 	iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
 	if (hwpt->fault) {
 		iommufd_auto_response_faults(hwpt, handle);
@@ -417,13 +422,17 @@ static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
 }
 
 static int iommufd_hwpt_replace_device(struct iommufd_device *idev,
+				       ioasid_t pasid,
 				       struct iommufd_hw_pagetable *hwpt,
 				       struct iommufd_hw_pagetable *old)
 {
-	struct iommufd_attach_handle *handle, *old_handle =
-		iommufd_device_get_attach_handle(idev);
+	struct iommufd_attach_handle *handle, *old_handle;
 	int rc;
 
+	WARN_ON(pasid != IOMMU_NO_PASID);
+
+	old_handle = iommufd_device_get_attach_handle(idev, pasid);
+
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
 	if (!handle)
 		return -ENOMEM;
@@ -458,7 +467,8 @@ static int iommufd_hwpt_replace_device(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;
@@ -484,7 +494,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;
@@ -502,7 +512,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);
@@ -510,7 +520,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)
@@ -522,12 +532,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;
@@ -576,7 +586,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);
@@ -605,7 +615,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;
 
@@ -638,7 +648,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
@@ -646,7 +657,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)
 {
@@ -675,7 +686,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);
 			/*
@@ -702,7 +713,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 {
@@ -723,8 +734,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;
@@ -739,7 +751,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;
@@ -748,8 +760,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;
@@ -786,7 +798,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;
 
@@ -816,7 +829,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");
@@ -832,7 +845,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/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 9b5b0b852229..5bdcf1f23305 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -187,7 +187,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;
 	}
@@ -200,7 +201,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 8e0e3ab64747..193ee8a3d5f9 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -350,9 +350,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);
-- 
2.34.1

[PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Yi Liu]

As the pasid is passed through the attach/replace/detach helpers, it is
necessary to ensure only the non-pasid path adds reserved_iova.

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

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 6ec7f6935115..72f6195e32f2 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -471,6 +471,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
 				ioasid_t pasid)
 {
 	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
+	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);
 	int rc;
 
 	mutex_lock(&idev->igroup->lock);
@@ -480,7 +481,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
 		goto err_unlock;
 	}
 
-	if (hwpt_paging) {
+	if (add_reserved) {
 		rc = iommufd_device_attach_reserved_iova(idev, hwpt_paging);
 		if (rc)
 			goto err_unlock;
@@ -504,7 +505,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
 	mutex_unlock(&idev->igroup->lock);
 	return 0;
 err_unresv:
-	if (hwpt_paging)
+	if (add_reserved)
 		iopt_remove_reserved_iova(&hwpt_paging->ioas->iopt, idev->dev);
 err_unlock:
 	mutex_unlock(&idev->igroup->lock);
@@ -523,7 +524,7 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev, ioasid_t pasid)
 		iommufd_hwpt_detach_device(hwpt, idev, pasid);
 		idev->igroup->hwpt = NULL;
 	}
-	if (hwpt_paging)
+	if (hwpt_paging && pasid == IOMMU_NO_PASID)
 		iopt_remove_reserved_iova(&hwpt_paging->ioas->iopt, idev->dev);
 	mutex_unlock(&idev->igroup->lock);
 
@@ -590,6 +591,7 @@ 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);
+	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);
 	struct iommufd_hwpt_paging *old_hwpt_paging;
 	struct iommufd_group *igroup = idev->igroup;
 	struct iommufd_hw_pagetable *old_hwpt;
@@ -609,7 +611,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
 	}
 
 	old_hwpt = igroup->hwpt;
-	if (hwpt_paging) {
+	if (add_reserved) {
 		rc = iommufd_group_do_replace_reserved_iova(igroup, hwpt_paging);
 		if (rc)
 			goto err_unlock;
@@ -620,7 +622,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
 		goto err_unresv;
 
 	old_hwpt_paging = find_hwpt_paging(old_hwpt);
-	if (old_hwpt_paging &&
+	if (old_hwpt_paging && pasid == IOMMU_NO_PASID &&
 	    (!hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas))
 		iommufd_group_remove_reserved_iova(igroup, old_hwpt_paging);
 
@@ -640,7 +642,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
 	/* Caller must destroy old_hwpt */
 	return old_hwpt;
 err_unresv:
-	if (hwpt_paging)
+	if (add_reserved)
 		iommufd_group_remove_reserved_iova(igroup, hwpt_paging);
 err_unlock:
 	mutex_unlock(&idev->igroup->lock);
-- 
2.34.1

[PATCH v8 05/12] iommufd: Mark PASID-compatible domain

[Yi Liu]

AMD IOMMU requires attaching PASID-compatible domains to PASID-capable
devices. This includes the domains attached to RID and PASIDs. Related
discussions in link [1] and [2]. ARM also has such a requirement, Intel
does not need it, but can live up with it. Hence, iommufd is going to
enforce this requirement as it is not harmful to vendors that do not
need it.

Mark the PASID-capable domains to prepare for adding this enforcement
when iommufd PASID support is added.

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

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/hw_pagetable.c    | 3 +++
 drivers/iommu/iommufd/iommufd_private.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 5bdcf1f23305..5df837388419 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -135,6 +135,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() */
@@ -242,6 +243,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;
@@ -296,6 +298,7 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	if (IS_ERR(hwpt_nested))
 		return ERR_CAST(hwpt_nested);
 	hwpt = &hwpt_nested->common;
+	hwpt->pasid_compat = flags & IOMMU_HWPT_ALLOC_PASID;
 
 	hwpt_nested->viommu = viommu;
 	refcount_inc(&viommu->obj.users);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 193ee8a3d5f9..cfb6a0767a30 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -276,6 +276,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 {
-- 
2.34.1

[PATCH v8 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);

The pasid operations share underlying attach/replace/detach infrastructure
with the device operations, but still have some different implications:

 - 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;

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.

AMD requires using PASID-compatible domains for PASIDs, hence the hwpts
directing the PASID path requires to flagged with IOMMU_HWPT_ALLOC_PASID.

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          |  64 ++++++----
 drivers/iommu/iommufd/iommufd_private.h |  26 ++++
 drivers/iommu/iommufd/pasid.c           | 155 ++++++++++++++++++++++++
 include/linux/iommufd.h                 |   7 ++
 5 files changed, 227 insertions(+), 26 deletions(-)
 create mode 100644 drivers/iommu/iommufd/pasid.c

diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
index cb784da6cddc..a64a67b502ae 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 \
 	viommu.o
 
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 72f6195e32f2..30dd2f79491a 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.
@@ -354,15 +357,18 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
 
 /* The device attach/detach/replace helpers for attach_handle */
 
-static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
-				      struct iommufd_device *idev,
-				      ioasid_t pasid)
+int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
+			       struct iommufd_device *idev,
+			       ioasid_t pasid)
 {
 	struct iommufd_attach_handle *handle;
 	int rc;
 
 	lockdep_assert_held(&idev->igroup->lock);
 
+	if (pasid != IOMMU_NO_PASID && !hwpt->pasid_compat)
+		return -EINVAL;
+
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
 	if (!handle)
 		return -ENOMEM;
@@ -374,9 +380,12 @@ static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 	}
 
 	handle->idev = idev;
-	WARN_ON(pasid != IOMMU_NO_PASID);
-	rc = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,
-				       &handle->handle);
+	if (pasid == IOMMU_NO_PASID)
+		rc = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,
+					       &handle->handle);
+	else
+		rc = iommu_attach_device_pasid_handle(hwpt->domain, idev->dev,
+						      pasid, &handle->handle);
 	if (rc)
 		goto out_disable_iopf;
 
@@ -404,16 +413,19 @@ iommufd_device_get_attach_handle(struct iommufd_device *idev, ioasid_t pasid)
 	return to_iommufd_handle(handle);
 }
 
-static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
-				       struct iommufd_device *idev,
-				       ioasid_t pasid)
+void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
+				struct iommufd_device *idev,
+				ioasid_t pasid)
 {
 	struct iommufd_attach_handle *handle;
 
-	WARN_ON(pasid != IOMMU_NO_PASID);
-
 	handle = iommufd_device_get_attach_handle(idev, 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);
+
 	if (hwpt->fault) {
 		iommufd_auto_response_faults(hwpt, handle);
 		iommufd_fault_iopf_disable(idev);
@@ -421,15 +433,16 @@ static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
 	kfree(handle);
 }
 
-static int iommufd_hwpt_replace_device(struct iommufd_device *idev,
-				       ioasid_t pasid,
-				       struct iommufd_hw_pagetable *hwpt,
-				       struct iommufd_hw_pagetable *old)
+int iommufd_hwpt_replace_device(struct iommufd_device *idev,
+				ioasid_t pasid,
+				struct iommufd_hw_pagetable *hwpt,
+				struct iommufd_hw_pagetable *old)
 {
 	struct iommufd_attach_handle *handle, *old_handle;
 	int rc;
 
-	WARN_ON(pasid != IOMMU_NO_PASID);
+	if (pasid != IOMMU_NO_PASID && !hwpt->pasid_compat)
+		return -EINVAL;
 
 	old_handle = iommufd_device_get_attach_handle(idev, pasid);
 
@@ -444,8 +457,12 @@ static int iommufd_hwpt_replace_device(struct iommufd_device *idev,
 	}
 
 	handle->idev = idev;
-	rc = iommu_replace_group_handle(idev->igroup->group, hwpt->domain,
-					&handle->handle);
+	if (pasid == IOMMU_NO_PASID)
+		rc = iommu_replace_group_handle(idev->igroup->group, hwpt->domain,
+						&handle->handle);
+	else
+		rc = iommu_replace_device_pasid_handle(hwpt->domain, idev->dev,
+						       pasid, &handle->handle);
 	if (rc)
 		goto out_disable_iopf;
 
@@ -649,10 +666,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.
@@ -736,9 +749,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/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index cfb6a0767a30..d533171d28de 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -400,6 +400,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;
@@ -417,6 +418,31 @@ 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_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
+			       struct iommufd_device *idev,
+			       ioasid_t pasid);
+void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
+				struct iommufd_device *idev,
+				ioasid_t pasid);
+int iommufd_hwpt_replace_device(struct iommufd_device *idev,
+				ioasid_t pasid,
+				struct iommufd_hw_pagetable *hwpt,
+				struct iommufd_hw_pagetable *old);
+
+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;
diff --git a/drivers/iommu/iommufd/pasid.c b/drivers/iommu/iommufd/pasid.c
new file mode 100644
index 000000000000..4450c101e557
--- /dev/null
+++ b/drivers/iommu/iommufd/pasid.c
@@ -0,0 +1,155 @@
+// 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;
+
+	mutex_lock(&idev->igroup->lock);
+	curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL);
+	if (curr) {
+		if (curr == hwpt)
+			rc = 0;
+		else
+			rc = xa_err(curr) ? : -EINVAL;
+		goto out_unlock;
+	}
+
+	rc = iommufd_hwpt_attach_device(hwpt, idev, pasid);
+	if (rc)
+		goto out_erase;
+
+	mutex_unlock(&idev->igroup->lock);
+	refcount_inc(&hwpt->obj.users);
+	return NULL;
+
+out_erase:
+	xa_erase(&idev->pasid_hwpts, pasid);
+out_unlock:
+	mutex_unlock(&idev->igroup->lock);
+	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;
+
+	mutex_lock(&idev->igroup->lock);
+	curr = xa_store(&idev->pasid_hwpts, pasid, hwpt, GFP_KERNEL);
+	rc = xa_err(curr);
+	if (rc)
+		goto out_unlock;
+
+	if (curr == hwpt)
+		goto out_unlock;
+
+	/* Not replace case */
+	if (!curr) {
+		xa_erase(&idev->pasid_hwpts, pasid);
+		rc = -EINVAL;
+		goto out_unlock;
+	}
+
+	/*
+	 * 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_unlock;
+	}
+
+	mutex_unlock(&idev->igroup->lock);
+	refcount_inc(&hwpt->obj.users);
+	/* Caller must destroy old_hwpt */
+	return curr;
+
+out_unlock:
+	mutex_unlock(&idev->igroup->lock);
+	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().
+ *
+ * Return 0 for success, otherwise errno.
+ */
+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.
+ *
+ * Return 0 for success, otherwise errno.
+ */
+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.
+ */
+void iommufd_device_pasid_detach(struct iommufd_device *idev, ioasid_t pasid)
+{
+	struct iommufd_hw_pagetable *hwpt;
+
+	mutex_lock(&idev->igroup->lock);
+	hwpt = xa_erase(&idev->pasid_hwpts, pasid);
+	if (WARN_ON(!hwpt)) {
+		mutex_unlock(&idev->igroup->lock);
+		return;
+	}
+	iommufd_hwpt_detach_device(hwpt, idev, pasid);
+	mutex_unlock(&idev->igroup->lock);
+	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 11110c749200..af7e5a4bfcf2 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/refcount.h>
 #include <linux/types.h>
 #include <linux/xarray.h>
@@ -56,6 +57,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 v8 07/12] iommufd: Enforce PASID-compatible domain for RID

[Yi Liu]

Per the definition of IOMMU_HWPT_ALLOC_PASID, iommufd needs to enforce
the RID to use PASID-compatible domain if PASID has been attached, and
vice versa. The PASID path has already enforced it. This adds the
enforcement in the RID path.

This enforcement requires a lock across the RID and PASID attach path,
the idev->igroup->lock is used as both the RID and the PASID path holds
it.

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

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 30dd2f79491a..e0f097b04467 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -357,6 +357,22 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
 
 /* The device attach/detach/replace helpers for attach_handle */
 
+static int iommufd_hwpt_pasid_compat(struct iommufd_hw_pagetable *hwpt,
+				     struct iommufd_device *idev,
+				     ioasid_t pasid)
+{
+	lockdep_assert_held(&idev->igroup->lock);
+
+	if (pasid == IOMMU_NO_PASID &&
+	    !xa_empty(&idev->pasid_hwpts) && !hwpt->pasid_compat)
+		return -EINVAL;
+
+	if (pasid != IOMMU_NO_PASID &&
+	    (!idev->igroup->hwpt->pasid_compat || !hwpt->pasid_compat))
+		return -EINVAL;
+	return 0;
+}
+
 int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 			       struct iommufd_device *idev,
 			       ioasid_t pasid)
@@ -364,10 +380,9 @@ int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 	struct iommufd_attach_handle *handle;
 	int rc;
 
-	lockdep_assert_held(&idev->igroup->lock);
-
-	if (pasid != IOMMU_NO_PASID && !hwpt->pasid_compat)
-		return -EINVAL;
+	rc = iommufd_hwpt_pasid_compat(hwpt, idev, pasid);
+	if (rc)
+		return rc;
 
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
 	if (!handle)
@@ -441,8 +456,9 @@ int iommufd_hwpt_replace_device(struct iommufd_device *idev,
 	struct iommufd_attach_handle *handle, *old_handle;
 	int rc;
 
-	if (pasid != IOMMU_NO_PASID && !hwpt->pasid_compat)
-		return -EINVAL;
+	rc = iommufd_hwpt_pasid_compat(hwpt, idev, pasid);
+	if (rc)
+		return rc;
 
 	old_handle = iommufd_device_get_attach_handle(idev, pasid);
 
-- 
2.34.1

[PATCH v8 08/12] iommu/vt-d: Add IOMMU_HWPT_ALLOC_PASID support

[Yi Liu]

Intel iommu driver just treats it as a nop since Intel VT-d does not have
special requirement on domains attached to either the PASID or RID of a
PASID-capable device.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 3 ++-
 drivers/iommu/intel/nested.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cc46098f875b..7bc890609b90 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3338,7 +3338,8 @@ intel_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 	bool first_stage;
 
 	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/intel/nested.c b/drivers/iommu/intel/nested.c
index aba92c00b427..6ac5c534bef4 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -198,7 +198,7 @@ intel_iommu_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
 	struct dmar_domain *domain;
 	int ret;
 
-	if (!nested_supported(iommu) || flags)
+	if (!nested_supported(iommu) || flags & ~IOMMU_HWPT_ALLOC_PASID)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	/* Must be nested domain */
-- 
2.34.1

[PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Yi Liu]

The underlying infrastructure has supported the PASID attach and related
enforcement per the requirement of the IOMMU_HWPT_ALLOC_PASID flag. This
extends iommufd to support PASID compatible domain requested by userspace
or the PASID compatible domain allocated in the auto_domain path.

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

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index e0f097b04467..afba66211b11 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -735,7 +735,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,
+						pasid != IOMMU_NO_PASID ?
+						IOMMU_HWPT_ALLOC_PASID : 0,
 						immediate_attach, NULL);
 	if (IS_ERR(hwpt_paging)) {
 		destroy_hwpt = ERR_CAST(hwpt_paging);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 5df837388419..36da52311893 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -111,7 +111,8 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 {
 	const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
 				IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
-				IOMMU_HWPT_FAULT_ID_VALID;
+				IOMMU_HWPT_FAULT_ID_VALID |
+				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;
@@ -231,7 +232,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_nested)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (parent->auto_domain || !parent->nest_parent ||
@@ -286,7 +287,7 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	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))
 		return ERR_PTR(-EOPNOTSUPP);
 	if (!user_data->len)
 		return ERR_PTR(-EOPNOTSUPP);
-- 
2.34.1

[PATCH v8 10/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     | 33 ++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 87dd123d1ff0..a4306d5f4b7e 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -49,6 +49,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 c779d0283f9b..8ed1ee129584 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -200,8 +200,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 = {
@@ -343,7 +351,7 @@ mock_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
 	struct mock_iommu_domain_nested *mock_nested;
 	struct mock_iommu_domain *mock_parent;
 
-	if (flags)
+	if (flags & ~IOMMU_HWPT_ALLOC_PASID)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (!parent || parent->ops != mock_ops.default_domain_ops)
 		return ERR_PTR(-EINVAL);
@@ -365,7 +373,8 @@ mock_domain_alloc_paging_flags(struct device *dev, u32 flags,
 {
 	bool has_dirty_flag = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
-				 IOMMU_HWPT_ALLOC_NEST_PARENT;
+				 IOMMU_HWPT_ALLOC_NEST_PARENT |
+				 IOMMU_HWPT_ALLOC_PASID;
 	struct mock_dev *mdev = to_mock_dev(dev);
 	bool no_dirty_ops = mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY;
 	struct mock_iommu_domain *mock;
@@ -585,7 +594,7 @@ mock_viommu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
 	struct mock_viommu *mock_viommu = to_mock_viommu(viommu);
 	struct mock_iommu_domain_nested *mock_nested;
 
-	if (flags)
+	if (flags & ~IOMMU_HWPT_ALLOC_PASID)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	mock_nested = __mock_domain_alloc_nested(user_data);
@@ -720,6 +729,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,
 		},
 };
 
@@ -780,6 +790,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 *
@@ -839,11 +850,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, i;
 
 	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);
@@ -866,6 +882,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;
@@ -1836,6 +1860,7 @@ int __init iommufd_test_init(void)
 	init_completion(&mock_iommu.complete);
 
 	mock_iommu_iopf_queue = iopf_queue_alloc("mock-iopfq");
+	mock_iommu.iommu_dev.max_pasids = (1 << 20);
 
 	return 0;
 
-- 
2.34.1

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

[Yi Liu]

This adds 5 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 |  36 +++++
 drivers/iommu/iommufd/selftest.c     | 205 ++++++++++++++++++++++++++-
 2 files changed, 238 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index a4306d5f4b7e..85715c17b2df 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -25,6 +25,11 @@ enum {
 	IOMMU_TEST_OP_TRIGGER_IOPF,
 	IOMMU_TEST_OP_DEV_CHECK_CACHE,
 	IOMMU_TEST_OP_MIX_REPLACE_HANDLE,
+	IOMMU_TEST_OP_PASID_ATTACH,
+	IOMMU_TEST_OP_PASID_REPLACE,
+	IOMMU_TEST_OP_PASID_MIX_REPLACE_HANDLE,
+	IOMMU_TEST_OP_PASID_DETACH,
+	IOMMU_TEST_OP_PASID_CHECK_DOMAIN,
 };
 
 enum {
@@ -151,6 +156,37 @@ struct iommu_test_cmd {
 			__u32 pt_id;
 			/* @id is stdev_id */
 		} mix_replace_handle;
+		struct {
+			__u32 pasid;
+			__u32 pt_id;
+			/* @id is stdev_id
+			 * pasid#1024 is for special test, do not use it
+			 * in normal case.
+			 */
+		} pasid_attach;
+		struct {
+			__u32 pasid;
+			__u32 pt_id;
+			/* @id is stdev_id
+			 * pasid#1024 is for special test, do not use it
+			 * in normal case.
+			 */
+		} pasid_replace;
+		struct {
+			__u32 pasid;
+			__u32 pt_id;
+			/* @id is stdev_id */
+		} pasid_mix_replace_handle;
+		struct {
+			__u32 pasid;
+			/* @id is stdev_id */
+		} pasid_detach;
+		struct {
+			__u32 pasid;
+			__u32 hwpt_id;
+			__u64 out_result_ptr;
+			/* @id is stdev_id */
+		} pasid_check;
 	};
 	__u32 last;
 };
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 8ed1ee129584..056a97a0b50a 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -200,10 +200,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;
 }
 
@@ -1697,9 +1716,61 @@ static int iommufd_test_mixed_group_replace(struct iommufd_device *idev,
 	return ret;
 }
 
+static int iommufd_test_mixed_pasid_replace(struct iommu_domain *domain,
+					    struct iommufd_device *idev,
+					    ioasid_t pasid)
+{
+	struct iommu_group *group = idev->igroup->group;
+	struct iommu_attach_handle *handle;
+	struct device *dev = idev->dev;
+	int ret;
+
+	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+	if (!handle)
+		return -ENOMEM;
+
+	mutex_lock(&idev->igroup->lock);
+	/* should fail as no previous attached handle or domain */
+	ret = iommu_replace_device_pasid_handle(domain, dev, pasid, handle);
+	if (ret != -EINVAL)
+		goto out_unlock;
+
+	ret = iommu_attach_device_pasid(domain, dev, pasid);
+	if (ret)
+		goto out_unlock;
+
+	if (PTR_ERR(iommu_attach_handle_get(group, pasid, 0)) != -ENOENT) {
+		ret = -ENODEV;
+		goto out_detach;
+	}
+
+	ret = iommu_replace_device_pasid_handle(domain, dev, pasid, handle);
+	if (ret)
+		goto out_detach;
+
+	if (handle != iommu_attach_handle_get(group, pasid, 0)) {
+		ret = -ENOENT;
+		goto out_detach;
+	}
+
+	ret = iommu_replace_device_pasid_handle(domain, dev, pasid, handle);
+	if (ret)
+		goto out_detach;
+
+	if (handle != iommu_attach_handle_get(group, pasid, 0))
+		ret = -ENOENT;
+
+out_detach:
+	iommu_detach_device_pasid(domain, dev, pasid);
+out_unlock:
+	mutex_unlock(&idev->igroup->lock);
+	kfree(handle);
+	return ret;
+}
+
 static int iommufd_test_mixed_handle_replace(struct iommufd_ucmd *ucmd,
 					     unsigned int device_id,
-					     u32 pt_id)
+					     ioasid_t pasid, u32 pt_id)
 {
 	struct iommufd_hw_pagetable *hwpt;
 	struct iommufd_device *idev;
@@ -1722,14 +1793,127 @@ static int iommufd_test_mixed_handle_replace(struct iommufd_ucmd *ucmd,
 	domain = hwpt->domain;
 
 	rc = iommufd_test_mixed_group_replace(idev, domain);
+	if (rc)
+		goto out_put_hwpt;
+
+	if (pasid != IOMMU_NO_PASID)
+		rc = iommufd_test_mixed_pasid_replace(domain, idev, pasid);
 
+out_put_hwpt:
 	iommufd_put_object(ucmd->ictx, &hwpt->obj);
+out_dev_obj:
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
+	return rc;
+}
+
+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;
+}
+
+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);
+	if (rc)
+		goto out_dev_obj;
+
+	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+	if (rc)
+		iommufd_device_pasid_detach(sobj->idev.idev,
+					    cmd->pasid_attach.pasid);
+
+out_dev_obj:
+	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);
+	if (rc)
+		goto out_dev_obj;
+
+	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 
 out_dev_obj:
 	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;
+}
+
 void iommufd_selftest_destroy(struct iommufd_object *obj)
 {
 	struct selftest_obj *sobj = to_selftest_obj(obj);
@@ -1812,8 +1996,22 @@ int iommufd_test(struct iommufd_ucmd *ucmd)
 	case IOMMU_TEST_OP_TRIGGER_IOPF:
 		return iommufd_test_trigger_iopf(ucmd, cmd);
 	case IOMMU_TEST_OP_MIX_REPLACE_HANDLE:
-		return iommufd_test_mixed_handle_replace(ucmd, cmd->id,
-						cmd->mix_replace_handle.pt_id);
+		return iommufd_test_mixed_handle_replace(
+			ucmd, cmd->id, IOMMU_NO_PASID,
+			cmd->mix_replace_handle.pt_id);
+	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_MIX_REPLACE_HANDLE:
+		return iommufd_test_mixed_handle_replace(
+			ucmd, cmd->id,
+			cmd->pasid_mix_replace_handle.pasid,
+			cmd->pasid_mix_replace_handle.pt_id);
+	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;
 	}
@@ -1861,6 +2059,7 @@ int __init iommufd_test_init(void)
 
 	mock_iommu_iopf_queue = iopf_queue_alloc("mock-iopfq");
 	mock_iommu.iommu_dev.max_pasids = (1 << 20);
+	pasid_1024_attached = false;
 
 	return 0;
 
-- 
2.34.1

[PATCH v8 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       | 350 ++++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  46 ++-
 tools/testing/selftests/iommu/iommufd_utils.h | 123 ++++++
 3 files changed, 511 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 35c5f1ad482a..0ddcf31ae0e4 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -2962,4 +2962,354 @@ TEST_F(iommufd_viommu, vdevice_cache)
 	}
 }
 
+FIXTURE(iommufd_device_pasid)
+{
+	int fd;
+	uint32_t ioas_id;
+	uint32_t hwpt_id;
+	uint32_t stdev_id;
+	uint32_t device_id;
+	uint32_t no_pasid_stdev_id;
+	uint32_t no_pasid_device_id;
+};
+
+FIXTURE_VARIANT(iommufd_device_pasid)
+{
+	bool pasid_capable;
+};
+
+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);
+	if (!variant->pasid_capable)
+		test_cmd_mock_domain_flags(self->ioas_id, 0,
+					   &self->no_pasid_stdev_id, NULL,
+					   &self->no_pasid_device_id);
+}
+
+FIXTURE_TEARDOWN(iommufd_device_pasid)
+{
+	teardown_iommufd(self->fd, _metadata);
+}
+
+FIXTURE_VARIANT_ADD(iommufd_device_pasid, no_pasid)
+{
+	.pasid_capable = false,
+};
+
+FIXTURE_VARIANT_ADD(iommufd_device_pasid, has_pasid)
+{
+	.pasid_capable = true,
+};
+
+TEST_F(iommufd_device_pasid, pasid_attach)
+{
+	struct iommu_hwpt_selftest data = {
+		.iotlb =  IOMMU_TEST_IOTLB_DEFAULT,
+	};
+	uint32_t nested_hwpt_id[3] = {};
+	uint32_t parent_hwpt_id = 0;
+	uint32_t fault_id, fault_fd;
+	uint32_t s2_hwpt_id = 0;
+	uint32_t iopf_hwpt_id;
+	uint32_t pasid = 100;
+	uint32_t auto_hwpt;
+	uint32_t viommu_id;
+	bool result;
+
+	/* Allocate two nested hwpts sharing one common parent hwpt */
+	test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
+			    IOMMU_HWPT_ALLOC_NEST_PARENT,
+			    &parent_hwpt_id);
+	test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id,
+				   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));
+
+	/* Faulte related preparation */
+	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));
+
+	/* Allocate a regular nested hwpt based on viommu */
+	test_cmd_viommu_alloc(self->device_id, parent_hwpt_id,
+			      IOMMU_VIOMMU_TYPE_SELFTEST,
+			      &viommu_id);
+	test_cmd_hwpt_alloc_nested(self->device_id, viommu_id,
+				   IOMMU_HWPT_ALLOC_PASID,
+				   &nested_hwpt_id[2],
+				   IOMMU_HWPT_DATA_SELFTEST, &data,
+				   sizeof(data));
+
+	test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
+			    IOMMU_HWPT_ALLOC_PASID,
+			    &s2_hwpt_id);
+
+	/* Attach RID to non-pasid compat domain, */
+	test_cmd_mock_domain_replace(self->stdev_id, parent_hwpt_id);
+	/* then attach to pasid should fail */
+	test_err_pasid_attach(EINVAL, pasid, s2_hwpt_id, NULL);
+
+	/* Attach RID to pasid compat domain, */
+	test_cmd_mock_domain_replace(self->stdev_id, s2_hwpt_id);
+	/* then attach to pasid should succeed, */
+	test_cmd_pasid_attach(pasid, nested_hwpt_id[0], NULL);
+	/* but attach RID to non-pasid compat domain should fail now. */
+	test_err_mock_domain_replace(EINVAL, self->stdev_id, parent_hwpt_id);
+	test_cmd_pasid_detach(pasid);
+
+	if (!variant->pasid_capable) {
+		/*
+		 * PASID-compatible domain can be used by non-PASID-capable
+		 * device.
+		 */
+		test_cmd_mock_domain_replace(self->no_pasid_stdev_id, nested_hwpt_id[0]);
+		test_cmd_mock_domain_replace(self->no_pasid_stdev_id, self->ioas_id);
+		/*
+		 * Attach hwpt to pasid#100 of non-PASID-capable device,
+		 * should fail, no matter domain is pasid-comapt or not.
+		 */
+		EXPECT_ERRNO(EINVAL,
+			     _test_cmd_pasid_attach(self->fd, self->no_pasid_stdev_id,
+						    pasid, parent_hwpt_id, NULL));
+		EXPECT_ERRNO(EINVAL,
+			     _test_cmd_pasid_attach(self->fd, self->no_pasid_stdev_id,
+						    pasid, s2_hwpt_id, NULL));
+	}
+
+	/*
+	 * Attach non pasid compat hwpt to pasid-capable device, should
+	 * fail, and have null domain.
+	 */
+	test_err_pasid_attach(EINVAL, pasid, parent_hwpt_id, NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, 0, &result));
+	EXPECT_EQ(1, result);
+
+	/*
+	 * Attach ioas to pasid 100, should succeed, domain should
+	 * be valid.
+	 */
+	test_cmd_pasid_attach(pasid, self->ioas_id, &auto_hwpt);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, auto_hwpt, &result));
+	EXPECT_EQ(1, result);
+
+	/*
+	 * Attach same ioas to pasid 100, should succeed.
+	 */
+	test_cmd_pasid_attach(pasid, self->ioas_id, &auto_hwpt);
+
+	/*
+	 * Try attach pasid 100 with another hwpt, should FAIL
+	 * as attach does not allow overwrite, use REPLACE instead.
+	 */
+	test_err_pasid_attach(EINVAL, pasid, nested_hwpt_id[0], NULL);
+
+	/*
+	 * 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], NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, nested_hwpt_id[0],
+					      &result));
+	EXPECT_EQ(1, result);
+
+	/*
+	 * Try attach pasid 100 to same nested_hwpt_id[0], should succeed.
+	 */
+	test_cmd_pasid_attach(pasid, nested_hwpt_id[0], NULL);
+
+	/*
+	 * 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, s2_hwpt_id, NULL);
+
+	/*
+	 * Attach a s2 hwpt to pasid 200, should succeed, domain should
+	 * be valid.
+	 */
+	test_cmd_pasid_attach(pasid, s2_hwpt_id, NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, s2_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, &auto_hwpt);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, auto_hwpt,
+					      &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], NULL);
+	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], NULL);
+	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, s2_hwpt_id, NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, s2_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, NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, s2_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, NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, iopf_hwpt_id,
+					      &result));
+	EXPECT_EQ(1, result);
+
+	/*
+	 * Replace with s2_hwpt_id for pasid 2048, should
+	 * succeed, and have valid domain.
+	 */
+	test_cmd_pasid_replace(pasid, s2_hwpt_id, NULL);
+	ASSERT_EQ(0,
+		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+					      pasid, s2_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);
+
+	test_cmd_pasid_mixed_replace(4096, s2_hwpt_id);
+
+	/* Detach the s2_hwpt_id from RID */
+	test_cmd_mock_domain_replace(self->stdev_id, self->ioas_id);
+
+	test_ioctl_destroy(nested_hwpt_id[0]);
+	test_ioctl_destroy(nested_hwpt_id[1]);
+	test_ioctl_destroy(nested_hwpt_id[2]);
+	test_ioctl_destroy(viommu_id);
+	test_ioctl_destroy(parent_hwpt_id);
+	test_ioctl_destroy(s2_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 fa6d8cbbe6e1..a08b8012120c 100644
--- a/tools/testing/selftests/iommu/iommufd_fail_nth.c
+++ b/tools/testing/selftests/iommu/iommufd_fail_nth.c
@@ -209,12 +209,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)
@@ -226,6 +230,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);
 }
 
@@ -623,7 +629,6 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 	uint32_t fault_hwpt_id;
 	uint32_t ioas_id;
 	uint32_t ioas_id2;
-	uint32_t stdev_id;
 	uint32_t idev_id;
 	uint32_t hwpt_id;
 	uint32_t viommu_id;
@@ -654,28 +659,32 @@ 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_mixed_replace(self->fd, stdev_id, hwpt_id))
+	if (_test_cmd_mixed_replace(self->fd, self->stdev_id, hwpt_id))
 		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, stdev_id, hwpt_id, NULL))
+	if (_test_cmd_mock_domain_replace(self->fd, self->stdev_id, hwpt_id, NULL))
 		return -1;
 
 	if (_test_cmd_hwpt_alloc(self->fd, idev_id, ioas_id, 0,
-				 IOMMU_HWPT_ALLOC_NEST_PARENT, &hwpt_id,
+				 IOMMU_HWPT_ALLOC_NEST_PARENT |
+						IOMMU_HWPT_ALLOC_PASID,
+				 &hwpt_id,
 				 IOMMU_HWPT_DATA_NONE, 0, 0))
 		return -1;
 
@@ -695,6 +704,27 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 				 IOMMU_HWPT_DATA_SELFTEST, &data, sizeof(data)))
 		return -1;
 
+	self->pasid = 200;
+
+	/* Tests for pasid attach/replace/detach */
+	if (_test_cmd_pasid_attach(self->fd, self->stdev_id,
+				   self->pasid, ioas_id, NULL)) {
+		self->pasid = 0;
+		return -1;
+	}
+
+	if (_test_cmd_pasid_replace(self->fd, self->stdev_id,
+				    self->pasid, ioas_id2, NULL))
+		return -1;
+
+	if (_test_cmd_pasid_detach(self->fd, self->stdev_id, self->pasid))
+		return -1;
+
+	if (_test_cmd_pasid_mixed_replace(self->fd, self->stdev_id, 10, hwpt_id))
+		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 27d51f880369..b7a39fe12873 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -954,3 +954,126 @@ static int _test_cmd_mixed_replace(int fd, __u32 stdev_id, __u32 pt_id)
 
 #define test_cmd_mixed_replace(stdev_id, hwpt_id)                         \
 	ASSERT_EQ(0, _test_cmd_mixed_replace(self->fd, stdev_id, hwpt_id))
+
+static int _test_cmd_pasid_attach(int fd, __u32 stdev_id, __u32 pasid,
+				  __u32 pt_id, __u32 *out_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,
+		},
+	};
+	int ret;
+
+	ret = ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_ATTACH),
+		    &test_attach);
+	if (ret)
+		return ret;
+
+	if (out_pt_id)
+		*out_pt_id = test_attach.pasid_attach.pt_id;
+	return 0;
+}
+
+#define test_cmd_pasid_attach(pasid, hwpt_id, out_pt_id) \
+	ASSERT_EQ(0, _test_cmd_pasid_attach(self->fd, self->stdev_id, \
+					    pasid, hwpt_id, out_pt_id))
+
+#define test_err_pasid_attach(_errno, pasid, hwpt_id, out_pt_id) \
+	EXPECT_ERRNO(_errno, \
+		     _test_cmd_pasid_attach(self->fd, self->stdev_id, \
+					    pasid, hwpt_id, out_pt_id))
+
+static int _test_cmd_pasid_replace(int fd, __u32 stdev_id, __u32 pasid,
+				   __u32 pt_id, __u32 *out_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,
+		},
+	};
+	int ret;
+
+	ret = ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_REPLACE),
+		    &test_replace);
+	if (ret)
+		return ret;
+
+	if (out_pt_id)
+		*out_pt_id = test_replace.pasid_replace.pt_id;
+	return 0;
+}
+
+static int _test_cmd_pasid_mixed_replace(int fd, __u32 stdev_id,
+					 __u32 pasid, __u32 pt_id)
+{
+	struct iommu_test_cmd test_mixed_replace = {
+		.size = sizeof(test_mixed_replace),
+		.op = IOMMU_TEST_OP_PASID_MIX_REPLACE_HANDLE,
+		.id = stdev_id,
+		.pasid_mix_replace_handle = {
+			.pasid = pasid,
+			.pt_id = pt_id,
+		},
+	};
+
+	return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_MIX_REPLACE_HANDLE),
+		     &test_mixed_replace);
+}
+
+#define test_cmd_pasid_replace(pasid, hwpt_id, out_pt_id) \
+	ASSERT_EQ(0, _test_cmd_pasid_replace(self->fd, self->stdev_id, \
+					     pasid, hwpt_id, out_pt_id))
+
+#define test_err_cmd_pasid_replace(_errno, pasid, hwpt_id, out_pt_id) \
+	EXPECT_ERRNO(_errno, \
+		     _test_cmd_pasid_replace(self->fd, self->stdev_id, \
+					     pasid, hwpt_id, out_pt_id))
+
+#define test_cmd_pasid_mixed_replace(pasid, hwpt_id)                          \
+	ASSERT_EQ(0, _test_cmd_pasid_mixed_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 v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Baolu Lu]

On 2025/2/26 19:40, Yi Liu wrote:
> The existing iommu_attach_device_pasid() function allows both a valid
> handle and a NULL handle, which is not consistent with the RID path where
> iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
> it, this adds iommu_attach_device_pasid_handle() to cover the case with
> valid handle, while let the iommu_attach_device_pasid() only deals with
> the case with NULL handle.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Nicolin Chen]

On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
> The existing iommu_attach_device_pasid() function allows both a valid
> handle and a NULL handle, which is not consistent with the RID path where
> iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
> it, this adds iommu_attach_device_pasid_handle() to cover the case with
> valid handle, while let the iommu_attach_device_pasid() only deals with
> the case with NULL handle.

Hmm, I am not very sure about the necessity of this change. The
underlying function being called from those two helpers (with/
without handle) is still taking a NULL handle, which looks very
straightforward to me already, so this extra layer feels a bit
redundant..

> @@ -130,8 +131,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>  		goto out_free_handle;
>  	}
>  
> -	ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid,
> -					&handle->handle);
[..]
> +	ret = iommu_attach_device_pasid_handle(domain, dev,
> +					       iommu_mm->pasid,

These two could fit in one line.

>  	if (ret)
>  		goto out_free_domain;
>  	domain->users = 1;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 73a3b20b2ef9..f6dbb60ef948 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3354,9 +3354,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>   *
>   * Return: 0 on success, or an error.
>   */
> -int iommu_attach_device_pasid(struct iommu_domain *domain,
> -			      struct device *dev, ioasid_t pasid,
> -			      struct iommu_attach_handle *handle)
> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
> +				struct device *dev, ioasid_t pasid,
> +				struct iommu_attach_handle *handle)

Should it be just iommu_attach_device_pasid_handle?

> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
> +					    struct device *dev, ioasid_t pasid)
> +{
> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);

Then, here:
	return iommu_attach_device_pasid_handle(domain, dev, pasid, NULL);
?

> +static inline int
> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
> +				 struct device *dev, ioasid_t pasid,
> +				 struct iommu_attach_handle *handle)
> +{
> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);

And drop this?

> @@ -1139,6 +1154,8 @@ struct iommu_fault_param {};
>  struct iommu_iotlb_gather {};
>  struct iommu_dirty_bitmap {};
>  struct iommu_dirty_ops {};
> +struct iommu_attach_handle {};
> +

Nit: no need of the extra line

Thanks
Nicolin

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Nicolin Chen]

On Wed, Feb 26, 2025 at 03:40:22AM -0800, Yi Liu wrote:
> Provide a high-level API to allow replacements of one domain with
> another for specific pasid of a device. This is similar to
> iommu_group_replace_domain_handle() and it is expected to be used
> only by IOMMUFD.
  
> +static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
> +		struct iommu_attach_handle *handle;
> +
> +		handle = xa_untag_pointer(entry);
> +		domain = handle->domain;
> +	} else {
> +		domain = xa_untag_pointer(entry);
> +	}
> +
> +	return domain;
> +}
> +

Might be a bit compact with

static inline struct iommu_domain *pasid_array_entry_to_domain(void *entry)
{
	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_DOMAIN)
		return xa_untag_pointer(entry);
	return ((struct iommu_attach_handle *)xa_untag_pointer(entry))->domain;
}

> +/**
> + * iommu_replace_device_pasid_handle - 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. The @pasid should have been
> + * attached. Otherwise, this fails.
> + * The pasid will keep the old configuration if replacement failed.
> + * Return 0 on success, or an error.
> + */
> +int iommu_replace_device_pasid_handle(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 *entry;
> +	struct iommu_domain *curr_domain;
> +	void *curr;
> +	int ret;
> +
> +	if (!group)
> +		return -ENODEV;
> +
> +	if (!domain->ops->set_dev_pasid)
> +		return -EOPNOTSUPP;
> +
> +	if (dev_iommu_ops(dev) != domain->owner ||
> +	    pasid == IOMMU_NO_PASID || !handle)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +	entry = iommu_make_pasid_array_entry(domain, handle);
> +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
> +			  XA_ZERO_ENTRY, GFP_KERNEL);
> +	if (xa_is_err(curr)) {
> +		ret = xa_err(curr);
> +		goto out_unlock;
> +	}
> +
> +	/* Not a replace case */
> +	if (!curr) {

Mind elaborating this?

> +		xa_release(&group->pasid_array, pasid);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	curr_domain = pasid_array_entry_to_domain(curr);
> +	ret = 0;

Can we set the ret = 0 at the top?

Thanks
Nicolin

Re: [PATCH v8 03/12] iommufd: Pass @pasid through the device attach/replace path

[Nicolin Chen]

On Wed, Feb 26, 2025 at 03:40:23AM -0800, Yi Liu wrote:
>  int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> -				struct iommufd_device *idev)
> +				struct iommufd_device *idev,
> +				ioasid_t pasid)

I think these two could fit in one line.

> @@ -187,7 +187,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 */

"Since"

>  int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> -				struct iommufd_device *idev);
> +				struct iommufd_device *idev,
> +				ioasid_t pasid);

Here too.

Otherwise,
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Nicolin Chen]

On Wed, Feb 26, 2025 at 03:40:24AM -0800, Yi Liu wrote:
> As the pasid is passed through the attach/replace/detach helpers, it is
> necessary to ensure only the non-pasid path adds reserved_iova.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/device.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 6ec7f6935115..72f6195e32f2 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -471,6 +471,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>  				ioasid_t pasid)
>  {
>  	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
> +	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);

"hwpt_paging && pasid == IOMMU_NO_PASID" is already good?

"attach_resv" sounds better to me.

> @@ -620,7 +622,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
>  		goto err_unresv;
>  
>  	old_hwpt_paging = find_hwpt_paging(old_hwpt);
> -	if (old_hwpt_paging &&
> +	if (old_hwpt_paging && pasid == IOMMU_NO_PASID &&
>  	    (!hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas))
>  		iommufd_group_remove_reserved_iova(igroup, old_hwpt_paging);

I am a bit unclear about the use case with pasid and replace. If
we replace the old_hwpt_paging with pasid != IOMMU_NO_PASID, what
would happen those reserved iova on the old_hwpt_paging?

Or does that mean a device can multi-attach to one hwpt_paging via
different pasids so that only down the IOMMU_NO_PASID path would
it remove the reserved iova?

Thanks
Nicolin

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu]

On 2025/2/27 06:16, Nicolin Chen wrote:
> On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
>> The existing iommu_attach_device_pasid() function allows both a valid
>> handle and a NULL handle, which is not consistent with the RID path where
>> iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
>> it, this adds iommu_attach_device_pasid_handle() to cover the case with
>> valid handle, while let the iommu_attach_device_pasid() only deals with
>> the case with NULL handle.
> 
> Hmm, I am not very sure about the necessity of this change. The
> underlying function being called from those two helpers (with/
> without handle) is still taking a NULL handle, which looks very
> straightforward to me already, so this extra layer feels a bit
> redundant..

I think I should have added a if (!handle) check in the beginning of
iommu_attach_device_pasid_handle() just like the other _handle() APIs.
If so, this should be clearer. is it?

> 
>> @@ -130,8 +131,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>>   		goto out_free_handle;
>>   	}
>>   
>> -	ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid,
>> -					&handle->handle);
> [..]
>> +	ret = iommu_attach_device_pasid_handle(domain, dev,
>> +					       iommu_mm->pasid,
> 
> These two could fit in one line.

got it.

> 
>>   	if (ret)
>>   		goto out_free_domain;
>>   	domain->users = 1;
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 73a3b20b2ef9..f6dbb60ef948 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3354,9 +3354,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>>    *
>>    * Return: 0 on success, or an error.
>>    */
>> -int iommu_attach_device_pasid(struct iommu_domain *domain,
>> -			      struct device *dev, ioasid_t pasid,
>> -			      struct iommu_attach_handle *handle)
>> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
>> +				struct device *dev, ioasid_t pasid,
>> +				struct iommu_attach_handle *handle)
> 
> Should it be just iommu_attach_device_pasid_handle?
> 
>> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
>> +					    struct device *dev, ioasid_t pasid)
>> +{
>> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
> 
> Then, here:
> 	return iommu_attach_device_pasid_handle(domain, dev, pasid, NULL);
> ?
> 
>> +static inline int
>> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
>> +				 struct device *dev, ioasid_t pasid,
>> +				 struct iommu_attach_handle *handle)
>> +{
>> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
> 
> And drop this?

this needs to check the handle before calling __iommu_attach_device_pasid().

> 
>> @@ -1139,6 +1154,8 @@ struct iommu_fault_param {};
>>   struct iommu_iotlb_gather {};
>>   struct iommu_dirty_bitmap {};
>>   struct iommu_dirty_ops {};
>> +struct iommu_attach_handle {};
>> +
> 
> Nit: no need of the extra line
> 
got it.

-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Baolu Lu]

On 2/26/25 19:40, Yi Liu wrote:
> +/**
> + * iommu_replace_device_pasid_handle - 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. The @pasid should have been
> + * attached. Otherwise, this fails.
> + * The pasid will keep the old configuration if replacement failed.
> + * Return 0 on success, or an error.
> + */
> +int iommu_replace_device_pasid_handle(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 *entry;
> +	struct iommu_domain *curr_domain;
> +	void *curr;
> +	int ret;
> +
> +	if (!group)
> +		return -ENODEV;
> +
> +	if (!domain->ops->set_dev_pasid)
> +		return -EOPNOTSUPP;
> +
> +	if (dev_iommu_ops(dev) != domain->owner ||
> +	    pasid == IOMMU_NO_PASID || !handle)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +	entry = iommu_make_pasid_array_entry(domain, handle);
> +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
> +			  XA_ZERO_ENTRY, GFP_KERNEL);
> +	if (xa_is_err(curr)) {
> +		ret = xa_err(curr);
> +		goto out_unlock;
> +	}
> +
> +	/* Not a replace case */
> +	if (!curr) {
> +		xa_release(&group->pasid_array, pasid);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	curr_domain = pasid_array_entry_to_domain(curr);
> +	ret = 0;
> +
> +	if (curr_domain != domain) {
> +		ret = __iommu_set_group_pasid(domain, group,
> +					      pasid, curr_domain);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +
> +	if (curr != entry) {
> +		/*
> +		 * The above xa_cmpxchg() reserved the memory, and the
> +		 * group->mutex is held, this cannot fail.
> +		 */
> +		WARN_ON(xa_is_err(xa_store(&group->pasid_array,
> +					   pasid, entry, GFP_KERNEL)));
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&group->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid_handle, "IOMMUFD_INTERNAL");

Perhaps you can compact your code with guard(muext). Something like
below:

         guard(mutex)(&group->mutex);
         entry = iommu_make_pasid_array_entry(domain, handle);
         curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
                           XA_ZERO_ENTRY, GFP_KERNEL);
         if (xa_is_err(curr))
                 return xa_err(curr);

         /* Not a replace case */
         if (!curr) {
                 xa_release(&group->pasid_array, pasid);
                 return -EINVAL;
         }

         curr_domain = pasid_array_entry_to_domain(curr);
         if (curr_domain == domain)
                 return 0;

         ret = __iommu_set_group_pasid(domain, group, pasid, curr_domain);
         if (ret)
                 return ret;

         /*
          * The above xa_cmpxchg() reserved the memory, and the
          * group->mutex is held, this cannot fail.
          */
         WARN_ON(xa_is_err(xa_store(&group->pasid_array, pasid, entry, 
GFP_KERNEL)));

         return 0;

Thanks,
baolu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Yi Liu]

On 2025/2/27 07:11, Nicolin Chen wrote:
> On Wed, Feb 26, 2025 at 03:40:22AM -0800, Yi Liu wrote:
>> Provide a high-level API to allow replacements of one domain with
>> another for specific pasid of a device. This is similar to
>> iommu_group_replace_domain_handle() and it is expected to be used
>> only by IOMMUFD.
>    
>> +static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
>> +{
>> +	struct iommu_domain *domain;
>> +
>> +	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
>> +		struct iommu_attach_handle *handle;
>> +
>> +		handle = xa_untag_pointer(entry);
>> +		domain = handle->domain;
>> +	} else {
>> +		domain = xa_untag_pointer(entry);
>> +	}
>> +
>> +	return domain;
>> +}
>> +
> 
> Might be a bit compact with
> 
> static inline struct iommu_domain *pasid_array_entry_to_domain(void *entry)
> {
> 	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_DOMAIN)
> 		return xa_untag_pointer(entry);
> 	return ((struct iommu_attach_handle *)xa_untag_pointer(entry))->domain;
> }

yep.

> 
>> +/**
>> + * iommu_replace_device_pasid_handle - 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. The @pasid should have been
>> + * attached. Otherwise, this fails.
>> + * The pasid will keep the old configuration if replacement failed.
>> + * Return 0 on success, or an error.
>> + */
>> +int iommu_replace_device_pasid_handle(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 *entry;
>> +	struct iommu_domain *curr_domain;
>> +	void *curr;
>> +	int ret;
>> +
>> +	if (!group)
>> +		return -ENODEV;
>> +
>> +	if (!domain->ops->set_dev_pasid)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (dev_iommu_ops(dev) != domain->owner ||
>> +	    pasid == IOMMU_NO_PASID || !handle)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&group->mutex);
>> +	entry = iommu_make_pasid_array_entry(domain, handle);
>> +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
>> +			  XA_ZERO_ENTRY, GFP_KERNEL);
>> +	if (xa_is_err(curr)) {
>> +		ret = xa_err(curr);
>> +		goto out_unlock;
>> +	}
>> +
>> +	/* Not a replace case */
>> +	if (!curr) {
> 
> Mind elaborating this?

If curr is null, this is more an attach op. In my opinion, this should not
be allowed to mix the usage of attach and replace.

>> +		xa_release(&group->pasid_array, pasid);
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
>> +	curr_domain = pasid_array_entry_to_domain(curr);
>> +	ret = 0;
> 
> Can we set the ret = 0 at the top?

yes.

-- 
Regards,
Yi Liu

Re: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Yi Liu]

On 2025/2/27 08:05, Nicolin Chen wrote:
> On Wed, Feb 26, 2025 at 03:40:24AM -0800, Yi Liu wrote:
>> As the pasid is passed through the attach/replace/detach helpers, it is
>> necessary to ensure only the non-pasid path adds reserved_iova.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommufd/device.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index 6ec7f6935115..72f6195e32f2 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -471,6 +471,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>>   				ioasid_t pasid)
>>   {
>>   	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
>> +	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);
> 
> "hwpt_paging && pasid == IOMMU_NO_PASID" is already good?
> 
> "attach_resv" sounds better to me.

got it.

>> @@ -620,7 +622,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
>>   		goto err_unresv;
>>   
>>   	old_hwpt_paging = find_hwpt_paging(old_hwpt);
>> -	if (old_hwpt_paging &&
>> +	if (old_hwpt_paging && pasid == IOMMU_NO_PASID &&
>>   	    (!hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas))
>>   		iommufd_group_remove_reserved_iova(igroup, old_hwpt_paging);
> 
> I am a bit unclear about the use case with pasid and replace. If
> we replace the old_hwpt_paging with pasid != IOMMU_NO_PASID, what
> would happen those reserved iova on the old_hwpt_paging?
> 
> Or does that mean a device can multi-attach to one hwpt_paging via
> different pasids so that only down the IOMMU_NO_PASID path would
> it remove the reserved iova?

yes, and only the IOMMU_NO_PASID path adds it. So only this path removes
it when the old_hwpt_paging is replaced or detached.

-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Yi Liu]

On 2025/2/27 09:31, Baolu Lu wrote:
> On 2/26/25 19:40, Yi Liu wrote:
>> +/**
>> + * iommu_replace_device_pasid_handle - 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. The @pasid should have been
>> + * attached. Otherwise, this fails.
>> + * The pasid will keep the old configuration if replacement failed.
>> + * Return 0 on success, or an error.
>> + */
>> +int iommu_replace_device_pasid_handle(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 *entry;
>> +    struct iommu_domain *curr_domain;
>> +    void *curr;
>> +    int ret;
>> +
>> +    if (!group)
>> +        return -ENODEV;
>> +
>> +    if (!domain->ops->set_dev_pasid)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (dev_iommu_ops(dev) != domain->owner ||
>> +        pasid == IOMMU_NO_PASID || !handle)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&group->mutex);
>> +    entry = iommu_make_pasid_array_entry(domain, handle);
>> +    curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
>> +              XA_ZERO_ENTRY, GFP_KERNEL);
>> +    if (xa_is_err(curr)) {
>> +        ret = xa_err(curr);
>> +        goto out_unlock;
>> +    }
>> +
>> +    /* Not a replace case */
>> +    if (!curr) {
>> +        xa_release(&group->pasid_array, pasid);
>> +        ret = -EINVAL;
>> +        goto out_unlock;
>> +    }
>> +
>> +    curr_domain = pasid_array_entry_to_domain(curr);
>> +    ret = 0;
>> +
>> +    if (curr_domain != domain) {
>> +        ret = __iommu_set_group_pasid(domain, group,
>> +                          pasid, curr_domain);
>> +        if (ret)
>> +            goto out_unlock;
>> +    }
>> +
>> +    if (curr != entry) {
>> +        /*
>> +         * The above xa_cmpxchg() reserved the memory, and the
>> +         * group->mutex is held, this cannot fail.
>> +         */
>> +        WARN_ON(xa_is_err(xa_store(&group->pasid_array,
>> +                       pasid, entry, GFP_KERNEL)));
>> +    }
>> +
>> +out_unlock:
>> +    mutex_unlock(&group->mutex);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid_handle, 
>> "IOMMUFD_INTERNAL");
> 
> Perhaps you can compact your code with guard(muext). Something like
> below:
> 
>          guard(mutex)(&group->mutex);

this is interesting.

>          entry = iommu_make_pasid_array_entry(domain, handle);
>          curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
>                            XA_ZERO_ENTRY, GFP_KERNEL);
>          if (xa_is_err(curr))
>                  return xa_err(curr);
> 
>          /* Not a replace case */
>          if (!curr) {
>                  xa_release(&group->pasid_array, pasid);
>                  return -EINVAL;
>          }
> 
>          curr_domain = pasid_array_entry_to_domain(curr);
>          if (curr_domain == domain)
>                  return 0;

even if domains are the same, we cannot assume the handles are the same. If
not the same, we still need to update the xa_array. Otherwise, UAF of the
old handle may occur although I don't think current caller does it. Or if
no old handle, handle needs to be stored.

>          ret = __iommu_set_group_pasid(domain, group, pasid, curr_domain);
>          if (ret)
>                  return ret;
> 
>          /*
>           * The above xa_cmpxchg() reserved the memory, and the
>           * group->mutex is held, this cannot fail.
>           */
>          WARN_ON(xa_is_err(xa_store(&group->pasid_array, pasid, entry, 
> GFP_KERNEL)));
> 
>          return 0;


-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Baolu Lu]

On 2/27/25 10:29, Yi Liu wrote:
>>          entry = iommu_make_pasid_array_entry(domain, handle);
>>          curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
>>                            XA_ZERO_ENTRY, GFP_KERNEL);
>>          if (xa_is_err(curr))
>>                  return xa_err(curr);
>>
>>          /* Not a replace case */
>>          if (!curr) {
>>                  xa_release(&group->pasid_array, pasid);
>>                  return -EINVAL;
>>          }
>>
>>          curr_domain = pasid_array_entry_to_domain(curr);
>>          if (curr_domain == domain)
>>                  return 0;
> 
> even if domains are the same, we cannot assume the handles are the same. If
> not the same, we still need to update the xa_array. Otherwise, UAF of the
> old handle may occur although I don't think current caller does it. Or if
> no old handle, handle needs to be stored.

Okay, fair enough.

Re: [PATCH v8 05/12] iommufd: Mark PASID-compatible domain

[Baolu Lu]

On 2/26/25 19:40, Yi Liu wrote:
> AMD IOMMU requires attaching PASID-compatible domains to PASID-capable
> devices. This includes the domains attached to RID and PASIDs. Related
> discussions in link [1] and [2]. ARM also has such a requirement, Intel
> does not need it, but can live up with it. Hence, iommufd is going to
> enforce this requirement as it is not harmful to vendors that do not
> need it.
> 
> Mark the PASID-capable domains to prepare for adding this enforcement
> when iommufd PASID support is added.
> 
> [1]https://lore.kernel.org/linux-iommu/20240709182303.GK14050@ziepe.ca/
> [2]https://lore.kernel.org/linux-iommu/20240822124433.GD3468552@ziepe.ca/
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

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

[Baolu Lu]

On 2/26/25 19:40, Yi Liu wrote:
> 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);
> 
> The pasid operations share underlying attach/replace/detach infrastructure
> with the device operations, but still have some different implications:
> 
>   - 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;
> 
> 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.

I feel that this is somewhat redundant with the pasid xarray in
iommu_group. If iommu_replace_device_pasid_handle() were modified to
include the old domain that was assigned to the PASID, the following
patch would be unnecessary?

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

> 
> AMD requires using PASID-compatible domains for PASIDs, hence the hwpts
> directing the PASID path requires to flagged with IOMMU_HWPT_ALLOC_PASID.
> 
> Signed-off-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>

Either way,

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Re: [PATCH v8 07/12] iommufd: Enforce PASID-compatible domain for RID

[Baolu Lu]

On 2/26/25 19:40, Yi Liu wrote:
> Per the definition of IOMMU_HWPT_ALLOC_PASID, iommufd needs to enforce
> the RID to use PASID-compatible domain if PASID has been attached, and
> vice versa. The PASID path has already enforced it. This adds the
> enforcement in the RID path.
> 
> This enforcement requires a lock across the RID and PASID attach path,
> the idev->igroup->lock is used as both the RID and the PASID path holds
> it.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommufd/device.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 30dd2f79491a..e0f097b04467 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -357,6 +357,22 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
>   
>   /* The device attach/detach/replace helpers for attach_handle */
>   
> +static int iommufd_hwpt_pasid_compat(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev,
> +				     ioasid_t pasid)
> +{
> +	lockdep_assert_held(&idev->igroup->lock);
> +
> +	if (pasid == IOMMU_NO_PASID &&
> +	    !xa_empty(&idev->pasid_hwpts) && !hwpt->pasid_compat)
> +		return -EINVAL;
> +
> +	if (pasid != IOMMU_NO_PASID &&
> +	    (!idev->igroup->hwpt->pasid_compat || !hwpt->pasid_compat))
> +		return -EINVAL;
> +	return 0;
> +}

I'm not following this. Patch 05/12, which introduced the concept of
PASID-compatible domain, states, "AMD IOMMU requires attaching PASID-
compatible domains to PASID-capable devices." My understanding was that
if PASID is enabled on a device, the RID or PASID of that device should
only be configured with PASID-compatible domains.

However, the logic here differs. It seems that even if a device is
PASID-capable, it's allowed to attach a non-PASID-compatible domain to
the RID, if no domain is currently attached to the device's PASID. This
doesn't seem like a generic property.

Thanks,
baolu

Re: [PATCH v8 08/12] iommu/vt-d: Add IOMMU_HWPT_ALLOC_PASID support

[Baolu Lu]

On 2/26/25 19:40, Yi Liu wrote:
> Intel iommu driver just treats it as a nop since Intel VT-d does not have
> special requirement on domains attached to either the PASID or RID of a
> PASID-capable device.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Baolu Lu]

On 2/26/25 19:40, Yi Liu wrote:
> The underlying infrastructure has supported the PASID attach and related
> enforcement per the requirement of the IOMMU_HWPT_ALLOC_PASID flag. This
> extends iommufd to support PASID compatible domain requested by userspace
> or the PASID compatible domain allocated in the auto_domain path.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommufd/device.c       | 4 +++-
>   drivers/iommu/iommufd/hw_pagetable.c | 7 ++++---
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index e0f097b04467..afba66211b11 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -735,7 +735,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,
> +						pasid != IOMMU_NO_PASID ?
> +						IOMMU_HWPT_ALLOC_PASID : 0,
>   						immediate_attach, NULL);

I still don't have a complete understanding of PASID-compatible domains
and the appropriate policies for attach/detach/replacing. I understand
that it is for the AMD IOMMU, but the AMD iommu driver doesn't yet
support attaching domains other than SVA to a device's PASID.

If that's the case, wouldn't the PASID-compatible domain logic
introduced here a dead code? Why not placing it in a separate series and
test it on real hardware before merging?

Thanks,
baolu

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

[Yi Liu]

On 2025/2/27 11:27, Baolu Lu wrote:
> On 2/26/25 19:40, Yi Liu wrote:
>> 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);
>>
>> The pasid operations share underlying attach/replace/detach infrastructure
>> with the device operations, but still have some different implications:
>>
>>   - 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;
>>
>> 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.
> 
> I feel that this is somewhat redundant with the pasid xarray in
> iommu_group. If iommu_replace_device_pasid_handle() were modified to
> include the old domain that was assigned to the PASID, the following
> patch would be unnecessary?
> 
> https://lore.kernel.org/linux-iommu/20250226011849.5102-4-yi.l.liu@intel.com/

yes, I've considered it as well. The array in core should be necessary.
Otherwise, the PRI forwarding path will have problem to support pasid.
So the question is if the pasid_hwpts is necessary in iommufd. I kept it as
below considerations.

The usages of the pasid_hwpts here are:
1) check if any prior attachment in the attach path
2) retrieve the attached hwpt in attach/replace/detach path.

The first usage can possibly be done by querying the pasid_array of the
core.

However, the second one seems not trivial. iommu core stores domain/handle,
while the iommufd needs hwpt to check if curr_hwpt is the same with new
hwpt and the detach path needs hwpt as well. I've considered the
possibility of a reverse mapping from domain to hwpt. This might be
possible since iommufd does not share domain across hwpts. But it appears
to me this is more complicated than adding pasid_hwpts array.

@Jason, your thoughts?

>>
>> AMD requires using PASID-compatible domains for PASIDs, hence the hwpts
>> directing the PASID path requires to flagged with IOMMU_HWPT_ALLOC_PASID.
>>
>> Signed-off-by: Kevin Tian<kevin.tian@intel.com>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> 
> Either way,
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

-- 
Regards,
Yi Liu

Re: [PATCH v8 07/12] iommufd: Enforce PASID-compatible domain for RID

[Yi Liu]

On 2025/2/27 11:43, Baolu Lu wrote:
> On 2/26/25 19:40, Yi Liu wrote:
>> Per the definition of IOMMU_HWPT_ALLOC_PASID, iommufd needs to enforce
>> the RID to use PASID-compatible domain if PASID has been attached, and
>> vice versa. The PASID path has already enforced it. This adds the
>> enforcement in the RID path.
>>
>> This enforcement requires a lock across the RID and PASID attach path,
>> the idev->igroup->lock is used as both the RID and the PASID path holds
>> it.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommufd/device.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index 30dd2f79491a..e0f097b04467 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -357,6 +357,22 @@ iommufd_device_attach_reserved_iova(struct 
>> iommufd_device *idev,
>>   /* The device attach/detach/replace helpers for attach_handle */
>> +static int iommufd_hwpt_pasid_compat(struct iommufd_hw_pagetable *hwpt,
>> +                     struct iommufd_device *idev,
>> +                     ioasid_t pasid)
>> +{
>> +    lockdep_assert_held(&idev->igroup->lock);
>> +
>> +    if (pasid == IOMMU_NO_PASID &&
>> +        !xa_empty(&idev->pasid_hwpts) && !hwpt->pasid_compat)
>> +        return -EINVAL;
>> +
>> +    if (pasid != IOMMU_NO_PASID &&
>> +        (!idev->igroup->hwpt->pasid_compat || !hwpt->pasid_compat))
>> +        return -EINVAL;
>> +    return 0;
>> +}
> 
> I'm not following this. Patch 05/12, which introduced the concept of
> PASID-compatible domain, states, "AMD IOMMU requires attaching PASID-
> compatible domains to PASID-capable devices." My understanding was that
> if PASID is enabled on a device, the RID or PASID of that device should
> only be configured with PASID-compatible domains.

that word may need a bit refine. If we do fail the attachment if the pasid
is enabled, the old userspace that does not aware of this enforcement but
do not use pasid will be failed. Such old userspaces do not use pasid at
all, so they shall still be workable. And it is allowed. Let me enhance
that commit message in patch 05.

> However, the logic here differs. It seems that even if a device is
> PASID-capable, it's allowed to attach a non-PASID-compatible domain to
> the RID, if no domain is currently attached to the device's PASID. This
> doesn't seem like a generic property.

this shall be allowed. The key point is if pasid is ever used or going to
be used, the rid path should have been or should been attached to pasid
compat domain.

-- 
Regards,
Yi Liu

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Yi Liu]

On 2025/2/27 12:00, Baolu Lu wrote:
> On 2/26/25 19:40, Yi Liu wrote:
>> The underlying infrastructure has supported the PASID attach and related
>> enforcement per the requirement of the IOMMU_HWPT_ALLOC_PASID flag. This
>> extends iommufd to support PASID compatible domain requested by userspace
>> or the PASID compatible domain allocated in the auto_domain path.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommufd/device.c       | 4 +++-
>>   drivers/iommu/iommufd/hw_pagetable.c | 7 ++++---
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index e0f097b04467..afba66211b11 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -735,7 +735,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,
>> +                        pasid != IOMMU_NO_PASID ?
>> +                        IOMMU_HWPT_ALLOC_PASID : 0,
>>                           immediate_attach, NULL);
> 
> I still don't have a complete understanding of PASID-compatible domains
> and the appropriate policies for attach/detach/replacing. I understand
> that it is for the AMD IOMMU, but the AMD iommu driver doesn't yet
> support attaching domains other than SVA to a device's PASID.
> 
> If that's the case, wouldn't the PASID-compatible domain logic
> introduced here a dead code? Why not placing it in a separate series and
> test it on real hardware before merging?

it was added per one for Jason's remark. I didn't see a problem to include
it given the rule is agreed.

https://lore.kernel.org/linux-iommu/20240807135915.GF8473@ziepe.ca/

-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Nicolin Chen]

On Thu, Feb 27, 2025 at 09:43:01AM +0800, Yi Liu wrote:
> On 2025/2/27 07:11, Nicolin Chen wrote:
> > > +/**
> > > + * iommu_replace_device_pasid_handle - 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. The @pasid should have been
> > > + * attached. Otherwise, this fails.
> > > + * The pasid will keep the old configuration if replacement failed.
> > > + * Return 0 on success, or an error.
> > > + */
> > > +int iommu_replace_device_pasid_handle(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 *entry;
> > > +	struct iommu_domain *curr_domain;
> > > +	void *curr;
> > > +	int ret;
> > > +
> > > +	if (!group)
> > > +		return -ENODEV;
> > > +
> > > +	if (!domain->ops->set_dev_pasid)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (dev_iommu_ops(dev) != domain->owner ||
> > > +	    pasid == IOMMU_NO_PASID || !handle)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&group->mutex);
> > > +	entry = iommu_make_pasid_array_entry(domain, handle);
> > > +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
> > > +			  XA_ZERO_ENTRY, GFP_KERNEL);
> > > +	if (xa_is_err(curr)) {
> > > +		ret = xa_err(curr);
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	/* Not a replace case */
> > > +	if (!curr) {
> > 
> > Mind elaborating this?
> 
> If curr is null, this is more an attach op.

Does it mean that this function doesn't allow to replace a domain
that has no handle? But how can that be an attach op? I mean there
still could be a use case of replacing a domain (no handle) with
another domain (no handle or has handle)?

Thanks
Nic

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Nicolin Chen]

On Thu, Feb 27, 2025 at 09:27:12AM +0800, Yi Liu wrote:
> On 2025/2/27 06:16, Nicolin Chen wrote:
> > On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
> > > The existing iommu_attach_device_pasid() function allows both a valid
> > > handle and a NULL handle, which is not consistent with the RID path where
> > > iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
> > > it, this adds iommu_attach_device_pasid_handle() to cover the case with
> > > valid handle, while let the iommu_attach_device_pasid() only deals with
> > > the case with NULL handle.
> > 
> > Hmm, I am not very sure about the necessity of this change. The
> > underlying function being called from those two helpers (with/
> > without handle) is still taking a NULL handle, which looks very
> > straightforward to me already, so this extra layer feels a bit
> > redundant..
> 
> I think I should have added a if (!handle) check in the beginning of
> iommu_attach_device_pasid_handle() just like the other _handle() APIs.
> If so, this should be clearer. is it?

My point is that the extra layer of these two helpers really do
nothing but be two wrappers around the exact same function with
just different "handle" inputs which only introduces some extra
complicity to the core code, not to mention that the underlying
function __iommu_attach_device_pasid is even in the header :-/

So, overall it doesn't feel very necessary to me...

The iommu_attach_group/iommu_attach_group_handle() case, on the
other hand, does take care of some underlying difference. Thus,
having two separate functions could make things clear.

Thanks
Nic

Re: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Nicolin Chen]

On Thu, Feb 27, 2025 at 09:50:29AM +0800, Yi Liu wrote:
> On 2025/2/27 08:05, Nicolin Chen wrote:
> > On Wed, Feb 26, 2025 at 03:40:24AM -0800, Yi Liu wrote:
> > > @@ -620,7 +622,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
> > >   		goto err_unresv;
> > >   	old_hwpt_paging = find_hwpt_paging(old_hwpt);
> > > -	if (old_hwpt_paging &&
> > > +	if (old_hwpt_paging && pasid == IOMMU_NO_PASID &&
> > >   	    (!hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas))
> > >   		iommufd_group_remove_reserved_iova(igroup, old_hwpt_paging);
> > 
> > I am a bit unclear about the use case with pasid and replace. If
> > we replace the old_hwpt_paging with pasid != IOMMU_NO_PASID, what
> > would happen those reserved iova on the old_hwpt_paging?
> > 
> > Or does that mean a device can multi-attach to one hwpt_paging via
> > different pasids so that only down the IOMMU_NO_PASID path would
> > it remove the reserved iova?
> 
> yes, and only the IOMMU_NO_PASID path adds it. So only this path removes
> it when the old_hwpt_paging is replaced or detached.

I see. This is replacing one of the domain in the per-device PASID
table, and only the master domain (PASID=0) owns the reserved_iova:

PASID=0 -> domainA -> attach_resv/remove_resv
PASID=1 -> domainB
PASID=2 -> domainC

?

Thanks
Nic

Re: [PATCH v8 05/12] iommufd: Mark PASID-compatible domain

[Nicolin Chen]

On Wed, Feb 26, 2025 at 03:40:25AM -0800, Yi Liu wrote:
> AMD IOMMU requires attaching PASID-compatible domains to PASID-capable
> devices. This includes the domains attached to RID and PASIDs. Related
> discussions in link [1] and [2]. ARM also has such a requirement, Intel
> does not need it, but can live up with it. Hence, iommufd is going to
> enforce this requirement as it is not harmful to vendors that do not
> need it.
> 
> Mark the PASID-capable domains to prepare for adding this enforcement
> when iommufd PASID support is added.
> 
> [1] https://lore.kernel.org/linux-iommu/20240709182303.GK14050@ziepe.ca/
> [2] https://lore.kernel.org/linux-iommu/20240822124433.GD3468552@ziepe.ca/
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
 
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

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

[Jason Gunthorpe]

On Thu, Feb 27, 2025 at 12:19:24PM +0800, Yi Liu wrote:
> However, the second one seems not trivial. iommu core stores domain/handle,
> while the iommufd needs hwpt to check if curr_hwpt is the same with new
> hwpt and the detach path needs hwpt as well.

Nicolin's series stores the hwpt in the domain:

struct iommu_domain {
        union { /* Pointer usable by owner of the domain */
                struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
        };

So it is possible

However, you should also to check that iommufd is the domain owner
which we don't have a thing for yet.

Maybe leave it like this for now and we can revisit the xarray when
more parts are done?

Jason

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Jason Gunthorpe]

On Thu, Feb 27, 2025 at 12:00:18PM +0800, Baolu Lu wrote:

> If that's the case, wouldn't the PASID-compatible domain logic
> introduced here a dead code? Why not placing it in a separate series and
> test it on real hardware before merging?

ARM requires it too and does support general pasids.

Jason

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu]

On 2025/2/28 00:26, Nicolin Chen wrote:
> On Thu, Feb 27, 2025 at 09:27:12AM +0800, Yi Liu wrote:
>> On 2025/2/27 06:16, Nicolin Chen wrote:
>>> On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
>>>> The existing iommu_attach_device_pasid() function allows both a valid
>>>> handle and a NULL handle, which is not consistent with the RID path where
>>>> iommu_attach_group() and iommu_attach_group_handle() coexist. To refine
>>>> it, this adds iommu_attach_device_pasid_handle() to cover the case with
>>>> valid handle, while let the iommu_attach_device_pasid() only deals with
>>>> the case with NULL handle.
>>>
>>> Hmm, I am not very sure about the necessity of this change. The
>>> underlying function being called from those two helpers (with/
>>> without handle) is still taking a NULL handle, which looks very
>>> straightforward to me already, so this extra layer feels a bit
>>> redundant..
>>
>> I think I should have added a if (!handle) check in the beginning of
>> iommu_attach_device_pasid_handle() just like the other _handle() APIs.
>> If so, this should be clearer. is it?
> 
> My point is that the extra layer of these two helpers really do
> nothing but be two wrappers around the exact same function with
> just different "handle" inputs which only introduces some extra
> complicity to the core code, not to mention that the underlying
> function __iommu_attach_device_pasid is even in the header :-/

hmmm. I see your point. It's really strange that most caller of
iommu_attach_device_pasid() input a NULL in the place of @handle.
I thought it was done in that way as caller of iommu_attach_device_pasid()
is not huge. Having a dedicate helper that don't accept handle looks
better.

Actually, we have many examples that two APIs call into the
same code but with some parameters be static.

e.g. iommu_paging_domain_alloc_flags() and iommu_paging_domain_alloc().

But you indeed reminds me. It's improper to have the
__iommu_attach_device_pasid() in header.

> So, overall it doesn't feel very necessary to me...
> 
> The iommu_attach_group/iommu_attach_group_handle() case, on the
> other hand, does take care of some underlying difference. Thus,
> having two separate functions could make things clear.

actually, they can share code as well. The below patch actually has started
it. Though dropped to not blocking the pasid series.

https://lore.kernel.org/linux-iommu/20250218195756.GG4183890@nvidia.com/

-- 
Regards,
Yi Liu

Re: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Yi Liu]

On 2025/2/28 00:31, Nicolin Chen wrote:
> On Thu, Feb 27, 2025 at 09:50:29AM +0800, Yi Liu wrote:
>> On 2025/2/27 08:05, Nicolin Chen wrote:
>>> On Wed, Feb 26, 2025 at 03:40:24AM -0800, Yi Liu wrote:
>>>> @@ -620,7 +622,7 @@ iommufd_device_do_replace(struct iommufd_device *idev, ioasid_t pasid,
>>>>    		goto err_unresv;
>>>>    	old_hwpt_paging = find_hwpt_paging(old_hwpt);
>>>> -	if (old_hwpt_paging &&
>>>> +	if (old_hwpt_paging && pasid == IOMMU_NO_PASID &&
>>>>    	    (!hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas))
>>>>    		iommufd_group_remove_reserved_iova(igroup, old_hwpt_paging);
>>>
>>> I am a bit unclear about the use case with pasid and replace. If
>>> we replace the old_hwpt_paging with pasid != IOMMU_NO_PASID, what
>>> would happen those reserved iova on the old_hwpt_paging?
>>>
>>> Or does that mean a device can multi-attach to one hwpt_paging via
>>> different pasids so that only down the IOMMU_NO_PASID path would
>>> it remove the reserved iova?
>>
>> yes, and only the IOMMU_NO_PASID path adds it. So only this path removes
>> it when the old_hwpt_paging is replaced or detached.
> 
> I see. This is replacing one of the domain in the per-device PASID
> table, and only the master domain (PASID=0) owns the reserved_iova:
> 
> PASID=0 -> domainA -> attach_resv/remove_resv
> PASID=1 -> domainB
> PASID=2 -> domainC
> 
> ?
yes. it is.

-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Yi Liu]

On 2025/2/28 00:04, Nicolin Chen wrote:
> On Thu, Feb 27, 2025 at 09:43:01AM +0800, Yi Liu wrote:
>> On 2025/2/27 07:11, Nicolin Chen wrote:
>>>> +/**
>>>> + * iommu_replace_device_pasid_handle - 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. The @pasid should have been
>>>> + * attached. Otherwise, this fails.
>>>> + * The pasid will keep the old configuration if replacement failed.
>>>> + * Return 0 on success, or an error.
>>>> + */
>>>> +int iommu_replace_device_pasid_handle(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 *entry;
>>>> +	struct iommu_domain *curr_domain;
>>>> +	void *curr;
>>>> +	int ret;
>>>> +
>>>> +	if (!group)
>>>> +		return -ENODEV;
>>>> +
>>>> +	if (!domain->ops->set_dev_pasid)
>>>> +		return -EOPNOTSUPP;
>>>> +
>>>> +	if (dev_iommu_ops(dev) != domain->owner ||
>>>> +	    pasid == IOMMU_NO_PASID || !handle)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&group->mutex);
>>>> +	entry = iommu_make_pasid_array_entry(domain, handle);
>>>> +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
>>>> +			  XA_ZERO_ENTRY, GFP_KERNEL);
>>>> +	if (xa_is_err(curr)) {
>>>> +		ret = xa_err(curr);
>>>> +		goto out_unlock;
>>>> +	}
>>>> +
>>>> +	/* Not a replace case */
>>>> +	if (!curr) {
>>>
>>> Mind elaborating this?
>>
>> If curr is null, this is more an attach op.
> 
> Does it mean that this function doesn't allow to replace a domain
> that has no handle? But how can that be an attach op? I mean there
> still could be a use case of replacing a domain (no handle) with
> another domain (no handle or has handle)?

I think you misunderstood me here. The core stores either domain or handle
now after the handle series. So curr==NULL means no prior attached
domain(either with or w/o handle). That's why I said it is more like an 
attach and should use attach op.

replace a domain w/o handle to another domain with handle is supported.
vice versa.

But replace a domain w/o handle to another domain w/o handle is not
supported as I didn't add it. But it can be supported if there is
use case.

-- 
Regards,
Yi Liu

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

[Yi Liu]

On 2025/2/28 04:15, Jason Gunthorpe wrote:
> On Thu, Feb 27, 2025 at 12:19:24PM +0800, Yi Liu wrote:
>> However, the second one seems not trivial. iommu core stores domain/handle,
>> while the iommufd needs hwpt to check if curr_hwpt is the same with new
>> hwpt and the detach path needs hwpt as well.
> 
> Nicolin's series stores the hwpt in the domain:
> 
> struct iommu_domain {
>          union { /* Pointer usable by owner of the domain */
>                  struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
>          };
> 
> So it is possible
> 
> However, you should also to check that iommufd is the domain owner
> which we don't have a thing for yet.
> 
> Maybe leave it like this for now and we can revisit the xarray when
> more parts are done?

sure.

-- 
Regards,
Yi Liu

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Jason Gunthorpe]

On Fri, Feb 28, 2025 at 02:48:22PM +0800, Yi Liu wrote:
> But you indeed reminds me. It's improper to have the
> __iommu_attach_device_pasid() in header.

If you do it would be wrapped in an inline for users.

Ie you'd have  iommu_attach_device_pasid() &
iommu_attach_device_pasid_handle() with the !NULL validation just call
out to __iommu_attach_device_pasid()

> > So, overall it doesn't feel very necessary to me...
> > 
> > The iommu_attach_group/iommu_attach_group_handle() case, on the
> > other hand, does take care of some underlying difference. Thus,
> > having two separate functions could make things clear.
> 
> actually, they can share code as well. The below patch actually has started
> it. Though dropped to not blocking the pasid series.

Yeah, I think we can get to the point where all domain attach,
handle/nohandle/pasid/nopasid just calls one function that manipulates
the xarray. You got pretty close in that series

The PASID0 special cases can be handled with flags.

Jason

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:

> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
> +				struct device *dev, ioasid_t pasid,
> +				struct iommu_attach_handle *handle);
> +
> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
> +					    struct device *dev, ioasid_t pasid)
> +{
> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
> +}
> +
> +static inline int
> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
> +				 struct device *dev, ioasid_t pasid,
> +				 struct iommu_attach_handle *handle)
> +{
> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
> +}

I think this is the right pattern, you might add a 

if (WARN_ON(!handle))
   return -EINVAL;

But it also isn't really necessary since the API works with a NULL
handle just fine. We don't check for 0 flags in the alloc path for
instance.

However, I agree with Nicolin, this doesn't seem well justfied as I
see only one caller that passes in the NULL and really the only value
it brings is to save that boilerplate

So it is fine as is, but I'd also suggest dropping it unless the rest
of the series changes the reasoning.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:22AM -0800, Yi Liu wrote:
> Provide a high-level API to allow replacements of one domain with
> another for specific pasid of a device. This is similar to
> iommu_group_replace_domain_handle() 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      | 113 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 113 insertions(+), 4 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:24AM -0800, Yi Liu wrote:

> @@ -471,6 +471,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>  				ioasid_t pasid)
>  {
>  	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
> +	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);

The !! is unncessary. !! is a trick to booleanize a value:
 - Assiging or casting to bool already implicitly does !!
 - && is already a boolean operator

> @@ -590,6 +591,7 @@ 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);
> +	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);

Here too

Otherwise

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v8 03/12] iommufd: Pass @pasid through the device attach/replace path

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:23AM -0800, Yi Liu wrote:
> 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          | 67 +++++++++++++++----------
>  drivers/iommu/iommufd/hw_pagetable.c    |  5 +-
>  drivers/iommu/iommufd/iommufd_private.h |  5 +-
>  3 files changed, 46 insertions(+), 31 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v8 05/12] iommufd: Mark PASID-compatible domain

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:25AM -0800, Yi Liu wrote:
> AMD IOMMU requires attaching PASID-compatible domains to PASID-capable
> devices. This includes the domains attached to RID and PASIDs. Related
> discussions in link [1] and [2]. ARM also has such a requirement, Intel
> does not need it, but can live up with it. Hence, iommufd is going to
> enforce this requirement as it is not harmful to vendors that do not
> need it.
> 
> Mark the PASID-capable domains to prepare for adding this enforcement
> when iommufd PASID support is added.
> 
> [1] https://lore.kernel.org/linux-iommu/20240709182303.GK14050@ziepe.ca/
> [2] https://lore.kernel.org/linux-iommu/20240822124433.GD3468552@ziepe.ca/
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/hw_pagetable.c    | 3 +++
>  drivers/iommu/iommufd/iommufd_private.h | 1 +
>  2 files changed, 4 insertions(+)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

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

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:26AM -0800, Yi Liu wrote:
> 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);
> 
> The pasid operations share underlying attach/replace/detach infrastructure
> with the device operations, but still have some different implications:

You don't want to just add a PASID to the existing APIs and keep with
PASID=0 means no pasid?

> @@ -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));

Should this be in the igroup? That's what the core code does, so some
of the protections you have here won't match the core's version if we
ever see a multi-device pasid capable group.

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

This hunks could safely go in the prior patch adding the pasid_compat?

> @@ -417,6 +418,31 @@ 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_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
> +			       struct iommufd_device *idev,
> +			       ioasid_t pasid);
> +void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
> +				struct iommufd_device *idev,
> +				ioasid_t pasid);
> +int iommufd_hwpt_replace_device(struct iommufd_device *idev,
> +				ioasid_t pasid,
> +				struct iommufd_hw_pagetable *hwpt,
> +				struct iommufd_hw_pagetable *old);
> +
> +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);

Ugh is there going to be alot of stuf fin the pasid.c? Maybe it is
easier to just leave the new functions in device.

> +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;
> +
> +	mutex_lock(&idev->igroup->lock);
> +	curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL);
> +	if (curr) {

I guess you could do the same trick you wanted in the core where the
xarray in the group stores the normal domain too, then I think these
special functions just go away since the xa tests are folded into the
normal functions in place of their normal domain tests?

Jason

Re: [PATCH v8 07/12] iommufd: Enforce PASID-compatible domain for RID

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:27AM -0800, Yi Liu wrote:
> Per the definition of IOMMU_HWPT_ALLOC_PASID, iommufd needs to enforce
> the RID to use PASID-compatible domain if PASID has been attached, and
> vice versa. The PASID path has already enforced it. This adds the
> enforcement in the RID path.
> 
> This enforcement requires a lock across the RID and PASID attach path,
> the idev->igroup->lock is used as both the RID and the PASID path holds
> it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/device.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

> @@ -357,6 +357,22 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
>  
>  /* The device attach/detach/replace helpers for attach_handle */
>  
> +static int iommufd_hwpt_pasid_compat(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev,
> +				     ioasid_t pasid)
> +{

Though you could move the hunks adding this function earlier in the
series

Jason

Re: [PATCH v8 08/12] iommu/vt-d: Add IOMMU_HWPT_ALLOC_PASID support

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:28AM -0800, Yi Liu wrote:
> Intel iommu driver just treats it as a nop since Intel VT-d does not have
> special requirement on domains attached to either the PASID or RID of a
> PASID-capable device.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel/iommu.c  | 3 ++-
>  drivers/iommu/intel/nested.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 03:40:29AM -0800, Yi Liu wrote:
> The underlying infrastructure has supported the PASID attach and related
> enforcement per the requirement of the IOMMU_HWPT_ALLOC_PASID flag. This
> extends iommufd to support PASID compatible domain requested by userspace
> or the PASID compatible domain allocated in the auto_domain path.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/device.c       | 4 +++-
>  drivers/iommu/iommufd/hw_pagetable.c | 7 ++++---
>  2 files changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Nicolin Chen]

On Fri, Feb 28, 2025 at 10:12:32PM +0800, Yi Liu wrote:
> On 2025/2/28 00:04, Nicolin Chen wrote:
> > On Thu, Feb 27, 2025 at 09:43:01AM +0800, Yi Liu wrote:
> > > On 2025/2/27 07:11, Nicolin Chen wrote:
> > > > > +	/* Not a replace case */
> > > > > +	if (!curr) {
> > > > 
> > > > Mind elaborating this?
> > > 
> > > If curr is null, this is more an attach op.
> > 
> > Does it mean that this function doesn't allow to replace a domain
> > that has no handle? But how can that be an attach op? I mean there
> > still could be a use case of replacing a domain (no handle) with
> > another domain (no handle or has handle)?
> 
> I think you misunderstood me here. The core stores either domain or handle
> now after the handle series. So curr==NULL means no prior attached
> domain(either with or w/o handle). That's why I said it is more like an
> attach and should use attach op.
> 
> replace a domain w/o handle to another domain with handle is supported.
> vice versa.
> 
> But replace a domain w/o handle to another domain w/o handle is not
> supported as I didn't add it. But it can be supported if there is
> use case.

OK. Do you mind sparing a few more words in the comment line,
to clarify that !curr means the domain has not been attached
to anything so it shouldn't be a replace case?

Thanks
Nicolin

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu]

On 2025/2/28 23:12, Jason Gunthorpe wrote:
> On Fri, Feb 28, 2025 at 02:48:22PM +0800, Yi Liu wrote:
>> But you indeed reminds me. It's improper to have the
>> __iommu_attach_device_pasid() in header.
> 
> If you do it would be wrapped in an inline for users.
> 
> Ie you'd have  iommu_attach_device_pasid() &
> iommu_attach_device_pasid_handle() with the !NULL validation just call
> out to __iommu_attach_device_pasid()

oops,. yes. At first, I'd want iommu_attach_device_pasid() call
iommu_attach_device_pasid_handle(). However, it is not possible to add
the !NULL check in iommu_attach_device_pasid_handle(). Although I forget
to add the !NULL check in this version.

> 
>>> So, overall it doesn't feel very necessary to me...
>>>
>>> The iommu_attach_group/iommu_attach_group_handle() case, on the
>>> other hand, does take care of some underlying difference. Thus,
>>> having two separate functions could make things clear.
>>
>> actually, they can share code as well. The below patch actually has started
>> it. Though dropped to not blocking the pasid series.
> 
> Yeah, I think we can get to the point where all domain attach,
> handle/nohandle/pasid/nopasid just calls one function that manipulates
> the xarray. You got pretty close in that series
> 
> The PASID0 special cases can be handled with flags.

yeah, this might be possible.

-- 
Regards,
Yi Liu

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu]

On 2025/2/28 23:17, Jason Gunthorpe wrote:
> On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
> 
>> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
>> +				struct device *dev, ioasid_t pasid,
>> +				struct iommu_attach_handle *handle);
>> +
>> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
>> +					    struct device *dev, ioasid_t pasid)
>> +{
>> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
>> +}
>> +
>> +static inline int
>> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
>> +				 struct device *dev, ioasid_t pasid,
>> +				 struct iommu_attach_handle *handle)
>> +{
>> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
>> +}
> 
> I think this is the right pattern, you might add a
> 
> if (WARN_ON(!handle))
>     return -EINVAL;
> 
> But it also isn't really necessary since the API works with a NULL
> handle just fine. We don't check for 0 flags in the alloc path for
> instance.
> 
> However, I agree with Nicolin, this doesn't seem well justfied as I
> see only one caller that passes in the NULL and really the only value
> it brings is to save that boilerplate

not sure if there will be more such callers. I think idxd driver calls
it as it wants to use DMA API domain with pasid.

> 
> So it is fine as is, but I'd also suggest dropping it unless the rest
> of the series changes the reasoning.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>


-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Yi Liu]

On 2025/3/1 12:46, Nicolin Chen wrote:
> On Fri, Feb 28, 2025 at 10:12:32PM +0800, Yi Liu wrote:
>> On 2025/2/28 00:04, Nicolin Chen wrote:
>>> On Thu, Feb 27, 2025 at 09:43:01AM +0800, Yi Liu wrote:
>>>> On 2025/2/27 07:11, Nicolin Chen wrote:
>>>>>> +	/* Not a replace case */
>>>>>> +	if (!curr) {
>>>>>
>>>>> Mind elaborating this?
>>>>
>>>> If curr is null, this is more an attach op.
>>>
>>> Does it mean that this function doesn't allow to replace a domain
>>> that has no handle? But how can that be an attach op? I mean there
>>> still could be a use case of replacing a domain (no handle) with
>>> another domain (no handle or has handle)?
>>
>> I think you misunderstood me here. The core stores either domain or handle
>> now after the handle series. So curr==NULL means no prior attached
>> domain(either with or w/o handle). That's why I said it is more like an
>> attach and should use attach op.
>>
>> replace a domain w/o handle to another domain with handle is supported.
>> vice versa.
>>
>> But replace a domain w/o handle to another domain w/o handle is not
>> supported as I didn't add it. But it can be supported if there is
>> use case.
> 
> OK. Do you mind sparing a few more words in the comment line,
> to clarify that !curr means the domain has not been attached
> to anything so it shouldn't be a replace case?

sure.

-- 
Regards,
Yi Liu

Re: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Yi Liu]

On 2025/2/28 23:24, Jason Gunthorpe wrote:
> On Wed, Feb 26, 2025 at 03:40:24AM -0800, Yi Liu wrote:
> 
>> @@ -471,6 +471,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>>   				ioasid_t pasid)
>>   {
>>   	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
>> +	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);
> 
> The !! is unncessary. !! is a trick to booleanize a value:
>   - Assiging or casting to bool already implicitly does !!
>   - && is already a boolean operator
> 
>> @@ -590,6 +591,7 @@ 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);
>> +	bool add_reserved = !!(hwpt_paging && pasid == IOMMU_NO_PASID);
> 
> Here too

got it.

> Otherwise
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason

-- 
Regards,
Yi Liu

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

[Yi Liu]

On 2025/2/28 23:32, Jason Gunthorpe wrote:
> On Wed, Feb 26, 2025 at 03:40:26AM -0800, Yi Liu wrote:
>> 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);
>>
>> The pasid operations share underlying attach/replace/detach infrastructure
>> with the device operations, but still have some different implications:
> 
> You don't want to just add a PASID to the existing APIs and keep with
> PASID=0 means no pasid?

do you mean extending the iommufd_device_attach/detach/replace() APIs to
support a @pasid? Sounds ok to me. VFIO side reuses ioctl for rid attach
and pasid attach as well.

>> @@ -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));
> 
> Should this be in the igroup? That's what the core code does, so some
> of the protections you have here won't match the core's version if we
> ever see a multi-device pasid capable group.

conceptually, yes. But I had an impression that pasid is only supported on
singleton groups. Seems this does not stand since the SVA path dropped the
device count in below commit. If so, indeed iommufd should add pasid_array
per igroup.

https://lore.kernel.org/linux-iommu/20221031005917.45690-10-baolu.lu@linux.intel.com/

> 
>> +	if (pasid != IOMMU_NO_PASID && !hwpt->pasid_compat)
>> +		return -EINVAL;
> 
> This hunks could safely go in the prior patch adding the pasid_compat?

yes. And that patch can be renamed to be domains directing to pasid path
should be compat.

> 
>> @@ -417,6 +418,31 @@ 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_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
>> +			       struct iommufd_device *idev,
>> +			       ioasid_t pasid);
>> +void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
>> +				struct iommufd_device *idev,
>> +				ioasid_t pasid);
>> +int iommufd_hwpt_replace_device(struct iommufd_device *idev,
>> +				ioasid_t pasid,
>> +				struct iommufd_hw_pagetable *hwpt,
>> +				struct iommufd_hw_pagetable *old);
>> +
>> +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);
> 
> Ugh is there going to be alot of stuf fin the pasid.c? Maybe it is
> easier to just leave the new functions in device.

If your suggestion is to add @pasid to existing attach/detach/replace APIs,
put the new functions in device.c sounds good.

> 
>> +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;
>> +
>> +	mutex_lock(&idev->igroup->lock);
>> +	curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL);
>> +	if (curr) {
> 
> I guess you could do the same trick you wanted in the core where the
> xarray in the group stores the normal domain too, then I think these
> special functions just go away since the xa tests are folded into the
> normal functions in place of their normal domain tests?

do you mean storing the igroup->hwpt in the xarray?

-- 
Regards,
Yi Liu

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

[Jason Gunthorpe]

On Sat, Mar 01, 2025 at 07:44:26PM +0800, Yi Liu wrote:
> On 2025/2/28 23:32, Jason Gunthorpe wrote:
> > On Wed, Feb 26, 2025 at 03:40:26AM -0800, Yi Liu wrote:
> > > 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);
> > > 
> > > The pasid operations share underlying attach/replace/detach infrastructure
> > > with the device operations, but still have some different implications:
> > 
> > You don't want to just add a PASID to the existing APIs and keep with
> > PASID=0 means no pasid?
> 
> do you mean extending the iommufd_device_attach/detach/replace() APIs to
> support a @pasid? Sounds ok to me. VFIO side reuses ioctl for rid attach
> and pasid attach as well.

Yes

> > > @@ -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));
> > 
> > Should this be in the igroup? That's what the core code does, so some
> > of the protections you have here won't match the core's version if we
> > ever see a multi-device pasid capable group.
> 
> conceptually, yes. But I had an impression that pasid is only supported on
> singleton groups. Seems this does not stand since the SVA path dropped the
> device count in below commit. If so, indeed iommufd should add pasid_array
> per igroup.

I would keep it per igroup to match the code, even though we do expect
singleton groups.

> https://lore.kernel.org/linux-iommu/20221031005917.45690-10-baolu.lu@linux.intel.com/

PASID support is limited to single device groups directly currently,
so the SVA stuff was duplicate.

> > > +	mutex_lock(&idev->igroup->lock);
> > > +	curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL);
> > > +	if (curr) {
> > 
> > I guess you could do the same trick you wanted in the core where the
> > xarray in the group stores the normal domain too, then I think these
> > special functions just go away since the xa tests are folded into the
> > normal functions in place of their normal domain tests?
> 
> do you mean storing the igroup->hwpt in the xarray?

Yes

That would be appealing if it avoids all the code duplication

Jason 

RE: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Saturday, March 1, 2025 6:11 PM
> 
> On 2025/2/28 23:17, Jason Gunthorpe wrote:
> > On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
> >
> >> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
> >> +				struct device *dev, ioasid_t pasid,
> >> +				struct iommu_attach_handle *handle);
> >> +
> >> +static inline int iommu_attach_device_pasid(struct iommu_domain
> *domain,
> >> +					    struct device *dev, ioasid_t pasid)
> >> +{
> >> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
> >> +}
> >> +
> >> +static inline int
> >> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
> >> +				 struct device *dev, ioasid_t pasid,
> >> +				 struct iommu_attach_handle *handle)
> >> +{
> >> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
> >> +}
> >
> > I think this is the right pattern, you might add a
> >
> > if (WARN_ON(!handle))
> >     return -EINVAL;
> >
> > But it also isn't really necessary since the API works with a NULL
> > handle just fine. We don't check for 0 flags in the alloc path for
> > instance.
> >
> > However, I agree with Nicolin, this doesn't seem well justfied as I
> > see only one caller that passes in the NULL and really the only value
> > it brings is to save that boilerplate
> 
> not sure if there will be more such callers. I think idxd driver calls
> it as it wants to use DMA API domain with pasid.
> 

The existence of two flavors of APIs (w/o handle and w/ handle) for
RID comes from the reality that many drivers are using the old APIs
hence it makes sense to introduce new ones specific for limited 
users who requires handle.

But PASID-oriented APIs are relatively new. We don't necessarily
need to copy the RID flavors.

Actually even after this patch it's still not on par with the RID part,
e.g. for replace we don't have one w/o handle.

So we may go another way i.e. leaving pasid_attach() as is and then
make pasid_replace() accepting a handle parameter which could
be NULL too. Doing so make it consistent between attach/replace
instead of disallowing a corner case where one cannot replace 
between domains both w/o handle [1].

[1] https://lore.kernel.org/all/51eb28e7-0631-4965-81c1-cf9633e46019@intel.com/

RE: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:40 PM
> 
> -		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);
>  	}

My comment to v7 was not fixed. Above trunks can be merged into
one call.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v8 04/12] iommufd/device: Only add reserved_iova in non-pasid path

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:40 PM
> 
> As the pasid is passed through the attach/replace/detach helpers, it is
> necessary to ensure only the non-pasid path adds reserved_iova.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

with other comments fixed:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, March 4, 2025 1:49 AM
> 
> On Sat, Mar 01, 2025 at 07:44:26PM +0800, Yi Liu wrote:
> > On 2025/2/28 23:32, Jason Gunthorpe wrote:
> > > On Wed, Feb 26, 2025 at 03:40:26AM -0800, Yi Liu wrote:
> > > > @@ -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));
> > >
> > > Should this be in the igroup? That's what the core code does, so some
> > > of the protections you have here won't match the core's version if we
> > > ever see a multi-device pasid capable group.
> >
> > conceptually, yes. But I had an impression that pasid is only supported on
> > singleton groups. Seems this does not stand since the SVA path dropped
> the
> > device count in below commit. If so, indeed iommufd should add
> pasid_array
> > per igroup.
> 
> I would keep it per igroup to match the code, even though we do expect
> singleton groups.
> 
> > https://lore.kernel.org/linux-iommu/20221031005917.45690-10-
> baolu.lu@linux.intel.com/
> 
> PASID support is limited to single device groups directly currently,
> so the SVA stuff was duplicate.
> 

We may easily add an explicit check in iommufd given the existence
of igroup, so violation of singleton group can be detected in case
the iommu core starts to support non-singleton group for pasid
w/o notifying iommufd.

RE: [PATCH v8 07/12] iommufd: Enforce PASID-compatible domain for RID

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:40 PM
> 
> Per the definition of IOMMU_HWPT_ALLOC_PASID, iommufd needs to
> enforce
> the RID to use PASID-compatible domain if PASID has been attached, and
> vice versa. The PASID path has already enforced it. This adds the
> enforcement in the RID path.
> 
> This enforcement requires a lock across the RID and PASID attach path,
> the idev->igroup->lock is used as both the RID and the PASID path holds
> it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v8 08/12] iommu/vt-d: Add IOMMU_HWPT_ALLOC_PASID support

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:40 PM
> 
> Intel iommu driver just treats it as a nop since Intel VT-d does not have
> special requirement on domains attached to either the PASID or RID of a
> PASID-capable device.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:40 PM
>
> @@ -735,7 +735,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,
> +						pasid != IOMMU_NO_PASID ?
> +
> 	IOMMU_HWPT_ALLOC_PASID : 0,
>  						immediate_attach, NULL);

According to patch7 we require same ALLOC_PASID setting cross RID
and PASID's on a device. But above change attempts to break the
rule. Once RID has been attached to a pasid-incompatible hwpt, attaching
any PASID to a pasid-compatible auto domain always fails.

That essentially means that PASID cannot be attached to an auto domain.

RE: [PATCH v8 10/12] iommufd/selftest: Add set_dev_pasid in mock iommu

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:41 PM
> 
> 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>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 26, 2025 7:41 PM
> 
> This adds 5 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>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v8 01/12] iommu: Add iommu_attach_device_pasid_handle()

[Yi Liu]

On 2025/3/4 15:34, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Saturday, March 1, 2025 6:11 PM
>>
>> On 2025/2/28 23:17, Jason Gunthorpe wrote:
>>> On Wed, Feb 26, 2025 at 03:40:21AM -0800, Yi Liu wrote:
>>>
>>>> +int __iommu_attach_device_pasid(struct iommu_domain *domain,
>>>> +				struct device *dev, ioasid_t pasid,
>>>> +				struct iommu_attach_handle *handle);
>>>> +
>>>> +static inline int iommu_attach_device_pasid(struct iommu_domain
>> *domain,
>>>> +					    struct device *dev, ioasid_t pasid)
>>>> +{
>>>> +	return __iommu_attach_device_pasid(domain, dev, pasid, NULL);
>>>> +}
>>>> +
>>>> +static inline int
>>>> +iommu_attach_device_pasid_handle(struct iommu_domain *domain,
>>>> +				 struct device *dev, ioasid_t pasid,
>>>> +				 struct iommu_attach_handle *handle)
>>>> +{
>>>> +	return __iommu_attach_device_pasid(domain, dev, pasid, handle);
>>>> +}
>>>
>>> I think this is the right pattern, you might add a
>>>
>>> if (WARN_ON(!handle))
>>>      return -EINVAL;
>>>
>>> But it also isn't really necessary since the API works with a NULL
>>> handle just fine. We don't check for 0 flags in the alloc path for
>>> instance.
>>>
>>> However, I agree with Nicolin, this doesn't seem well justfied as I
>>> see only one caller that passes in the NULL and really the only value
>>> it brings is to save that boilerplate
>>
>> not sure if there will be more such callers. I think idxd driver calls
>> it as it wants to use DMA API domain with pasid.
>>
> 
> The existence of two flavors of APIs (w/o handle and w/ handle) for
> RID comes from the reality that many drivers are using the old APIs
> hence it makes sense to introduce new ones specific for limited
> users who requires handle.
> 
> But PASID-oriented APIs are relatively new. We don't necessarily
> need to copy the RID flavors.

got it.

> Actually even after this patch it's still not on par with the RID part,
> e.g. for replace we don't have one w/o handle.

this is because iommufd does not need it. But I got that pasid path
does not need to be complicated as rid path.

> So we may go another way i.e. leaving pasid_attach() as is and then
> make pasid_replace() accepting a handle parameter which could
> be NULL too. Doing so make it consistent between attach/replace
> instead of disallowing a corner case where one cannot replace
> between domains both w/o handle [1].
> 
> [1] https://lore.kernel.org/all/51eb28e7-0631-4965-81c1-cf9633e46019@intel.com/

then let me drop this patch.

-- 
Regards,
Yi Liu

Re: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Yi Liu]

On 2025/3/4 15:42, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, February 26, 2025 7:40 PM
>>
>> -		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);
>>   	}
> 
> My comment to v7 was not fixed. Above trunks can be merged into
> one call.

oops. yeah. it's missed.

I'm also thinking about if it is necessary to call iommu_remove_dev_pasid()
when WARN_ON happened. Ideally, we don't expect it to happen. If so, should
we just drop the iommu_remove_dev_pasid()?

> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

-- 
Regards,
Yi Liu

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Yi Liu]

On 2025/3/4 16:08, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, February 26, 2025 7:40 PM
>>
>> @@ -735,7 +735,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,
>> +						pasid != IOMMU_NO_PASID ?
>> +
>> 	IOMMU_HWPT_ALLOC_PASID : 0,
>>   						iommufd_hwpt_paging_allocimmediate_attach, NULL);
> 
> According to patch7 we require same ALLOC_PASID setting cross RID
> and PASID's on a device. But above change attempts to break the
> rule. Once RID has been attached to a pasid-incompatible hwpt, attaching
> any PASID to a pasid-compatible auto domain always fails.

yes.

> That essentially means that PASID cannot be attached to an auto domain.

PASID can still be attached to IOAS per this logic. If the first auto_hwpt
within the IOAS failed, iommufd would just allocate another one, and this 
would be pasid-compatiable. The always failing case is with hwpt..

how about making this part as the below? We may assume that a pasid
capable device have high chance to have pasid usage, hence use compatible
domain for rid path in advance. If user does not want it, user could
allocate an in-compatible domain and replace it. might doc it in uapi.

	flags = 0;
	if (pasid != IOMMU_NO_PASID || idev->dev->iommu->max_pasids)
		flags |= IOMMU_HWPT_ALLOC_PASID;
	hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev,
						flags, immediate_attach, NULL);


-- 
Regards,
Yi Liu

RE: [PATCH v8 02/12] iommu: Introduce a replace API for device pasid

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, March 4, 2025 4:49 PM
> 
> On 2025/3/4 15:42, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Wednesday, February 26, 2025 7:40 PM
> >>
> >> -		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);
> >>   	}
> >
> > My comment to v7 was not fixed. Above trunks can be merged into
> > one call.
> 
> oops. yeah. it's missed.
> 
> I'm also thinking about if it is necessary to call iommu_remove_dev_pasid()
> when WARN_ON happened. Ideally, we don't expect it to happen. If so,
> should
> we just drop the iommu_remove_dev_pasid()?
> 

keeping it avoids any undefined state left by a failed @set_dev_pasid().
attaching to blocking domain cannot fail.

RE: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, March 4, 2025 7:49 PM
> 
> how about making this part as the below? We may assume that a pasid
> capable device have high chance to have pasid usage, hence use compatible
> domain for rid path in advance. If user does not want it, user could
> allocate an in-compatible domain and replace it. might doc it in uapi.
> 
> 	flags = 0;
> 	if (pasid != IOMMU_NO_PASID || idev->dev->iommu->max_pasids)
> 		flags |= IOMMU_HWPT_ALLOC_PASID;
> 	hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev,
> 						flags, immediate_attach,
> NULL);
> 

yes that sounds better.

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Yi Liu]

On 2025/3/5 10:38, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, March 4, 2025 7:49 PM
>>
>> how about making this part as the below? We may assume that a pasid
>> capable device have high chance to have pasid usage, hence use compatible
>> domain for rid path in advance. If user does not want it, user could
>> allocate an in-compatible domain and replace it. might doc it in uapi.
>>
>> 	flags = 0;
>> 	if (pasid != IOMMU_NO_PASID || idev->dev->iommu->max_pasids)
>> 		flags |= IOMMU_HWPT_ALLOC_PASID;
>> 	hwpt_paging = iommufd_hwpt_paging_alloc(idev->ictx, ioas, idev,
>> 						flags, immediate_attach,
>> NULL);
>>
> 
> yes that sounds better.

after a second thinking, the original code should be better. It means
we keep the old users as they are. Old users might be using normal domains.
If we make it use the pasid-compat domain, it might potentially meet some
performance deduction? I think it's better to avoid it. While, we can add
a kdoc to mark that user that wants to attach PASID should first attach its
RID to pasid-compat domain. I think this should be a reasonable ask since
such users need to add extra code. I've made it in v9. Let me know if you
think it differently.

-- 
Regards,
Yi Liu

Re: [PATCH v8 09/12] iommufd: Allow allocating PASID-compatible domain

[Jason Gunthorpe]

On Thu, Mar 13, 2025 at 09:17:18PM +0800, Yi Liu wrote:
> after a second thinking, the original code should be better. It means
> we keep the old users as they are. Old users might be using normal domains.
> If we make it use the pasid-compat domain, it might potentially meet some
> performance deduction? I think it's better to avoid it. While, we can add
> a kdoc to mark that user that wants to attach PASID should first attach its
> RID to pasid-compat domain. I think this should be a reasonable ask since
> such users need to add extra code. I've made it in v9. Let me know if you
> think it differently.

That makes sense to me, there is a performance degredation on some HW
here.

The usage of pasid through vfio is going to be really rare, so better
to leave it fully opt in.

Jason