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

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

[Yi Liu]

PASID (Process Address Space ID) is a PCIe extension to tag the DMA
transactions out of a physical device, and most modern IOMMU hardwares
have supported PASID granular address translation. So a PASID-capable
device can be attached to multiple hwpts (a.k.a. domains), each attachment
is tagged with a pasid.

This series first fixes the gap of existing set_dev_pasid() callback for
supporting domain replacement, then a missing iommu API to replace domain
for a pasid. Based on the iommu pasid attach/replace/detach APIs, this
series adds iommufd APIs for device drivers to attach/replace/detach pasid
to/from hwpt per userspace's request, and adds selftest to validate the
iommufd APIs.

pasid attach/replace is mandatory on Intel VT-d given the PASID table
locates in the physical address space hence must be managed by the kernel,
both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
which allow configuring the PASID/CD table either in host physical address
space or nested on top of an GPA address space. This series only add VT-d
support as the minimal requirement.

The completed code can be found in below link [1]. Heads up! The existing
iommufd selftest was broken, there was a fix [2] to it, but not upstreamed
yet. If wants to run iommufd selftest, please apply that fix. Sorry for the
inconvenience.

[1] https://github.com/yiliu1765/iommufd/tree/iommufd_pasid
[2] https://lore.kernel.org/linux-iommu/20240111073213.180020-1-baolu.lu@linux.intel.com/

Change log:

v2:
 - 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

Lu Baolu (1):
  iommu/vt-d: Add set_dev_pasid callback for nested domain

Yi Liu (11):
  iommu: Pass old domain to set_dev_pasid op
  iommu: Introduce a replace API for device pasid
  iommufd: replace attach_fn with a structure
  iommufd: Support attach/replace hwpt per pasid
  iommu: Allow iommu driver to populate the max_pasids
  iommufd/selftest: Add set_dev_pasid and remove_dev_pasid in mock iommu
  iommufd/selftest: Add a helper to get test device
  iommufd/selftest: Add test ops to test pasid attach/detach
  iommufd/selftest: Add coverage for iommufd pasid attach/detach
  iommu/vt-d: Return if no dev_pasid is found in domain
  iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain
    replacement

 drivers/iommu/intel/iommu.c                   |  29 ++-
 drivers/iommu/intel/iommu.h                   |   3 +
 drivers/iommu/intel/nested.c                  |  15 ++
 drivers/iommu/intel/svm.c                     |   3 +-
 drivers/iommu/iommu-priv.h                    |   3 +
 drivers/iommu/iommu.c                         | 102 ++++++++-
 drivers/iommu/iommufd/Makefile                |   1 +
 drivers/iommu/iommufd/device.c                |  43 ++--
 drivers/iommu/iommufd/iommufd_private.h       |  23 ++
 drivers/iommu/iommufd/iommufd_test.h          |  30 +++
 drivers/iommu/iommufd/pasid.c                 | 161 ++++++++++++++
 drivers/iommu/iommufd/selftest.c              | 198 ++++++++++++++++-
 include/linux/iommu.h                         |   2 +-
 include/linux/iommufd.h                       |   6 +
 tools/testing/selftests/iommu/iommufd.c       | 207 ++++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  28 ++-
 tools/testing/selftests/iommu/iommufd_utils.h |  78 +++++++
 17 files changed, 888 insertions(+), 44 deletions(-)
 create mode 100644 drivers/iommu/iommufd/pasid.c

-- 
2.34.1

[PATCH v2 01/12] iommu: Pass old domain to set_dev_pasid op

[Yi Liu]

To support domain replacement for pasid, the underlying iommu driver needs
to know the old domain hence be able to clean up the existing attachment.
It would be much convenient for iommu layer to pass down the old domain.
Otherwise, iommu drivers would need to track domain for pasids by themselves,
this would duplicate code among the iommu drivers. Or iommu drivers would
rely group->pasid_array to get domain, which may not always the correct
one.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 3 ++-
 drivers/iommu/intel/svm.c   | 3 ++-
 drivers/iommu/iommu.c       | 3 ++-
 include/linux/iommu.h       | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 45c75a8a0ef5..df49aed3df5e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4626,7 +4626,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 }
 
 static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid)
+				     struct device *dev, ioasid_t pasid,
+				     struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index c1bed89b1026..ac8733b61470 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -315,7 +315,8 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
 }
 
 static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
-				   struct device *dev, ioasid_t pasid)
+				   struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3183b0ed4cdb..701b02a118db 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3321,7 +3321,8 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 	int ret;
 
 	for_each_group_device(group, device) {
-		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
+		ret = domain->ops->set_dev_pasid(domain, device->dev,
+						 pasid, NULL);
 		if (ret)
 			goto err_revert;
 	}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 40dd439307e8..1e5e9249c93f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -631,7 +631,7 @@ struct iommu_ops {
 struct iommu_domain_ops {
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
-			     ioasid_t pasid);
+			     ioasid_t pasid, struct iommu_domain *old);
 
 	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
-- 
2.34.1

[PATCH v2 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() and it is expected to be used only by
IOMMUFD.

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 |  3 ++
 drivers/iommu/iommu.c      | 92 +++++++++++++++++++++++++++++++++++---
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index 5f731d994803..0949c02cee93 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -20,6 +20,9 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 int iommu_group_replace_domain(struct iommu_group *group,
 			       struct iommu_domain *new_domain);
 
+int iommu_replace_device_pasid(struct iommu_domain *domain,
+			       struct device *dev, ioasid_t pasid);
+
 int iommu_device_register_bus(struct iommu_device *iommu,
 			      const struct iommu_ops *ops,
 			      const struct bus_type *bus,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 701b02a118db..343683e646e0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3315,14 +3315,15 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 
 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;
 	}
@@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 err_revert:
 	last_gdev = device;
 	for_each_group_device(group, device) {
-		const struct iommu_ops *ops = dev_iommu_ops(device->dev);
+		/*
+		 * If no old domain, just undo all the devices/pasid that
+		 * have attached to the new domain.
+		 */
+		if (!old) {
+			const struct iommu_ops *ops =
+						dev_iommu_ops(device->dev);
+
+			if (device == last_gdev)
+				break;
+			ops = dev_iommu_ops(device->dev);
+			ops->remove_dev_pasid(device->dev, pasid, domain);
+			continue;
+		}
 
-		if (device == last_gdev)
+		/*
+		 * Rollback the devices/pasid that have attached to the new
+		 * domain. And it is a driver bug to fail attaching with a
+		 * previously good domain.
+		 */
+		if (device == last_gdev) {
+			WARN_ON(old->ops->set_dev_pasid(old, device->dev,
+							pasid, NULL));
 			break;
-		ops->remove_dev_pasid(device->dev, pasid, domain);
+		}
+
+		WARN_ON(old->ops->set_dev_pasid(old, device->dev,
+						pasid, domain));
 	}
 	return ret;
 }
@@ -3395,7 +3419,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		goto out_unlock;
 	}
 
-	ret = __iommu_set_group_pasid(domain, group, pasid);
+	ret = __iommu_set_group_pasid(domain, group, pasid, NULL);
 	if (ret)
 		xa_erase(&group->pasid_array, pasid);
 out_unlock:
@@ -3404,6 +3428,62 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
 
+/**
+ * iommu_replace_device_pasid - replace the domain that a pasid is attached to
+ * @domain: new IOMMU domain to replace with
+ * @dev: the physical device
+ * @pasid: pasid that will be attached to the new domain
+ *
+ * This API allows the pasid to switch domains. Return 0 on success, or an
+ * error. The pasid will roll back to use the old domain if failure. The
+ * caller could call iommu_detach_device_pasid() before free the old domain
+ * in order to avoid use-after-free case.
+ */
+int iommu_replace_device_pasid(struct iommu_domain *domain,
+			       struct device *dev, ioasid_t pasid)
+{
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
+	void *curr;
+	int ret;
+
+	if (!domain)
+		return -EINVAL;
+
+	if (!domain->ops->set_dev_pasid)
+		return -EOPNOTSUPP;
+
+	if (!group)
+		return -ENODEV;
+
+	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+		return -EINVAL;
+
+	mutex_lock(&group->mutex);
+	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
+	if (!curr) {
+		xa_erase(&group->pasid_array, pasid);
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	ret = xa_err(curr);
+	if (ret)
+		goto out_unlock;
+
+	if (curr == domain)
+		goto out_unlock;
+
+	ret = __iommu_set_group_pasid(domain, group, pasid, curr);
+	if (ret)
+		WARN_ON(xa_err(xa_store(&group->pasid_array, pasid,
+					curr, GFP_KERNEL)));
+out_unlock:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid, IOMMUFD_INTERNAL);
+
 /*
  * iommu_detach_device_pasid() - Detach the domain from pasid of device
  * @domain: the iommu domain.
-- 
2.34.1

[PATCH v2 03/12] iommufd: replace attach_fn with a structure

[Yi Liu]

Most of the core logic before conducting the actual device attach/
replace operation can be shared with pasid attach/replace. Create
a new structure so more information (e.g. pasid) can be later added
along with the attach_fn.

Signed-off-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c          | 33 ++++++++++++++++---------
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++++
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 873630c111c1..56e4b8e776c9 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -531,8 +531,11 @@ iommufd_device_do_replace(struct iommufd_device *idev,
 	return ERR_PTR(rc);
 }
 
-typedef struct iommufd_hw_pagetable *(*attach_fn)(
-	struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt);
+static struct iommufd_hw_pagetable *do_attach(struct iommufd_device *idev,
+		struct iommufd_hw_pagetable *hwpt, struct attach_data *data)
+{
+	return data->attach_fn(idev, hwpt);
+}
 
 /*
  * When automatically managing the domains we search for a compatible domain in
@@ -542,7 +545,7 @@ typedef struct iommufd_hw_pagetable *(*attach_fn)(
 static struct iommufd_hw_pagetable *
 iommufd_device_auto_get_domain(struct iommufd_device *idev,
 			       struct iommufd_ioas *ioas, u32 *pt_id,
-			       attach_fn do_attach)
+			       struct attach_data *data)
 {
 	/*
 	 * iommufd_hw_pagetable_attach() is called by
@@ -551,7 +554,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	 * to use the immediate_attach path as it supports drivers that can't
 	 * directly allocate a domain.
 	 */
-	bool immediate_attach = do_attach == iommufd_device_do_attach;
+	bool immediate_attach = data->attach_fn == iommufd_device_do_attach;
 	struct iommufd_hw_pagetable *destroy_hwpt;
 	struct iommufd_hwpt_paging *hwpt_paging;
 	struct iommufd_hw_pagetable *hwpt;
@@ -569,7 +572,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
 		hwpt = &hwpt_paging->common;
 		if (!iommufd_lock_obj(&hwpt->obj))
 			continue;
-		destroy_hwpt = (*do_attach)(idev, hwpt);
+		destroy_hwpt = do_attach(idev, hwpt, data);
 		if (IS_ERR(destroy_hwpt)) {
 			iommufd_put_object(idev->ictx, &hwpt->obj);
 			/*
@@ -596,7 +599,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	hwpt = &hwpt_paging->common;
 
 	if (!immediate_attach) {
-		destroy_hwpt = (*do_attach)(idev, hwpt);
+		destroy_hwpt = do_attach(idev, hwpt, data);
 		if (IS_ERR(destroy_hwpt))
 			goto out_abort;
 	} else {
@@ -618,7 +621,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
 }
 
 static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
-				    attach_fn do_attach)
+				    struct attach_data *data)
 {
 	struct iommufd_hw_pagetable *destroy_hwpt;
 	struct iommufd_object *pt_obj;
@@ -633,7 +636,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, hwpt, data);
 		if (IS_ERR(destroy_hwpt))
 			goto out_put_pt_obj;
 		break;
@@ -643,7 +646,7 @@ static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
 			container_of(pt_obj, struct iommufd_ioas, obj);
 
 		destroy_hwpt = iommufd_device_auto_get_domain(idev, ioas, pt_id,
-							      do_attach);
+							      data);
 		if (IS_ERR(destroy_hwpt))
 			goto out_put_pt_obj;
 		break;
@@ -679,8 +682,11 @@ static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
 int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 {
 	int rc;
+	struct attach_data data = {
+		.attach_fn = &iommufd_device_do_attach,
+	};
 
-	rc = iommufd_device_change_pt(idev, pt_id, &iommufd_device_do_attach);
+	rc = iommufd_device_change_pt(idev, pt_id, &data);
 	if (rc)
 		return rc;
 
@@ -710,8 +716,11 @@ 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,
-					&iommufd_device_do_replace);
+	struct attach_data data = {
+		.attach_fn = &iommufd_device_do_replace,
+	};
+
+	return iommufd_device_change_pt(idev, pt_id, &data);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_replace, IOMMUFD);
 
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 991f864d1f9b..22f0b9a3df36 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -408,6 +408,14 @@ 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);
 
+struct attach_data {
+	union {
+		struct iommufd_hw_pagetable *(*attach_fn)(
+				struct iommufd_device *idev,
+				struct iommufd_hw_pagetable *hwpt);
+	};
+};
+
 struct iommufd_access {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
-- 
2.34.1

[PATCH v2 04/12] iommufd: Support attach/replace hwpt per pasid

[Yi Liu]

This introduces three APIs for device drivers to manage pasid attach/
replace/detach.

    int iommufd_device_pasid_attach(struct iommufd_device *idev,
				    u32 pasid, u32 *pt_id);
    int iommufd_device_pasid_replace(struct iommufd_device *idev,
				     u32 pasid, u32 *pt_id);
    void iommufd_device_pasid_detach(struct iommufd_device *idev,
				     u32 pasid);

pasid operations have different implications when comparing to device
operations:

 - No connection to iommufd_group since pasid is a device capability
   and can be enabled only in singleton group;

 - no reserved region per pasid otherwise SVA architecture is already
   broken (CPU address space doesn't count device reserved regions);

 - accordingly no sw_msi trick;

 - immediated_attach is not supported, expecting that arm-smmu driver
   will already remove that requirement before supporting this pasid
   operation. This avoids unnecessary change in iommufd_hw_pagetable_alloc()
   to carry the pasid from device.c.

With above differences, this puts all pasid related logics into a new
pasid.c file.

Cache coherency enforcement is still applied to pasid operations since
it is about memory accesses post page table walking (no matter the walk
is per RID or per PASID).

Since the attach is per PASID, this introduces a pasid_hwpts xarray to
track the per-pasid attach data.

Signed-off-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/Makefile          |   1 +
 drivers/iommu/iommufd/device.c          |  14 ++-
 drivers/iommu/iommufd/iommufd_private.h |  15 +++
 drivers/iommu/iommufd/pasid.c           | 161 ++++++++++++++++++++++++
 include/linux/iommufd.h                 |   6 +
 5 files changed, 194 insertions(+), 3 deletions(-)
 create mode 100644 drivers/iommu/iommufd/pasid.c

diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
index 34b446146961..4b4d516b025c 100644
--- a/drivers/iommu/iommufd/Makefile
+++ b/drivers/iommu/iommufd/Makefile
@@ -6,6 +6,7 @@ iommufd-y := \
 	ioas.o \
 	main.o \
 	pages.o \
+	pasid.o \
 	vfio_compat.o
 
 iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 56e4b8e776c9..442169f8b460 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))
@@ -216,6 +217,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 	/* igroup refcount moves into iommufd_device */
 	idev->igroup = igroup;
 
+	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.
@@ -534,7 +537,10 @@ iommufd_device_do_replace(struct iommufd_device *idev,
 static struct iommufd_hw_pagetable *do_attach(struct iommufd_device *idev,
 		struct iommufd_hw_pagetable *hwpt, struct attach_data *data)
 {
-	return data->attach_fn(idev, hwpt);
+	if (data->pasid == IOMMU_PASID_INVALID)
+		return data->attach_fn(idev, hwpt);
+	else
+		return data->pasid_attach_fn(idev, data->pasid, hwpt);
 }
 
 /*
@@ -620,8 +626,8 @@ 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,
-				    struct attach_data *data)
+int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
+			     struct attach_data *data)
 {
 	struct iommufd_hw_pagetable *destroy_hwpt;
 	struct iommufd_object *pt_obj;
@@ -684,6 +690,7 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 	int rc;
 	struct attach_data data = {
 		.attach_fn = &iommufd_device_do_attach,
+		.pasid = IOMMU_PASID_INVALID,
 	};
 
 	rc = iommufd_device_change_pt(idev, pt_id, &data);
@@ -718,6 +725,7 @@ int iommufd_device_replace(struct iommufd_device *idev, u32 *pt_id)
 {
 	struct attach_data data = {
 		.attach_fn = &iommufd_device_do_replace,
+		.pasid = IOMMU_PASID_INVALID,
 	};
 
 	return iommufd_device_change_pt(idev, pt_id, &data);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 22f0b9a3df36..bf42775fa1c1 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -394,6 +394,7 @@ struct iommufd_device {
 	struct list_head group_item;
 	/* always the physical device */
 	struct device *dev;
+	struct xarray pasid_hwpts;
 	bool enforce_cache_coherency;
 };
 
@@ -413,9 +414,23 @@ struct attach_data {
 		struct iommufd_hw_pagetable *(*attach_fn)(
 				struct iommufd_device *idev,
 				struct iommufd_hw_pagetable *hwpt);
+		struct iommufd_hw_pagetable *(*pasid_attach_fn)(
+				struct iommufd_device *idev, u32 pasid,
+				struct iommufd_hw_pagetable *hwpt);
 	};
+	u32 pasid;
 };
 
+int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
+			     struct attach_data *data);
+
+struct iommufd_hw_pagetable *
+iommufd_device_pasid_do_attach(struct iommufd_device *idev, u32 pasid,
+			       struct iommufd_hw_pagetable *hwpt);
+struct iommufd_hw_pagetable *
+iommufd_device_pasid_do_replace(struct iommufd_device *idev, u32 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..ee063fdb75c3
--- /dev/null
+++ b/drivers/iommu/iommufd/pasid.c
@@ -0,0 +1,161 @@
+// 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, u32 pasid,
+			       struct iommufd_hw_pagetable *hwpt)
+{
+	void *curr;
+	int rc;
+
+	refcount_inc(&hwpt->obj.users);
+	curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL);
+	if (curr) {
+		if (curr == hwpt)
+			rc = 0;
+		else
+			rc = xa_err(curr) ? : -EBUSY;
+		goto err_put_hwpt;
+	}
+
+	rc = iommu_attach_device_pasid(hwpt->domain, idev->dev, pasid);
+	if (rc) {
+		xa_erase(&idev->pasid_hwpts, pasid);
+		goto err_put_hwpt;
+	}
+
+	return NULL;
+
+err_put_hwpt:
+	refcount_dec(&hwpt->obj.users);
+	return ERR_PTR(rc);
+}
+
+struct iommufd_hw_pagetable *
+iommufd_device_pasid_do_replace(struct iommufd_device *idev, u32 pasid,
+				struct iommufd_hw_pagetable *hwpt)
+{
+	void *curr;
+	int rc;
+
+	refcount_inc(&hwpt->obj.users);
+	curr = xa_store(&idev->pasid_hwpts, pasid, hwpt, GFP_KERNEL);
+	rc = xa_err(curr);
+	if (rc)
+		goto out_put_hwpt;
+
+	if (!curr) {
+		xa_erase(&idev->pasid_hwpts, pasid);
+		rc = -EINVAL;
+		goto out_put_hwpt;
+	}
+
+	if (curr == hwpt)
+		goto out_put_hwpt;
+
+	rc = iommu_replace_device_pasid(hwpt->domain, idev->dev, pasid);
+	if (rc) {
+		WARN_ON(xa_err(xa_store(&idev->pasid_hwpts, pasid,
+					curr, GFP_KERNEL)));
+		goto out_put_hwpt;
+	}
+
+	/* Caller must destroy old_hwpt */
+	return curr;
+
+out_put_hwpt:
+	refcount_dec(&hwpt->obj.users);
+	return ERR_PTR(rc);
+}
+
+/**
+ * iommufd_device_pasid_attach - Connect a {device, pasid} to an iommu_domain
+ * @idev: device to attach
+ * @pasid: pasid to attach
+ * @pt_id: Input a IOMMUFD_OBJ_IOAS, or IOMMUFD_OBJ_HW_PAGETABLE
+ *         Output the IOMMUFD_OBJ_HW_PAGETABLE ID
+ *
+ * This connects a pasid of the device to an iommu_domain. Once this
+ * completes the device could do DMA with the pasid.
+ *
+ * This function is undone by calling iommufd_device_detach_pasid().
+ *
+ * iommufd does not handle race between iommufd_device_pasid_attach(),
+ * iommufd_device_pasid_replace() and iommufd_device_pasid_detach().
+ * So caller of them should guarantee no concurrent call on the same
+ * device and pasid.
+ */
+int iommufd_device_pasid_attach(struct iommufd_device *idev,
+				u32 pasid, u32 *pt_id)
+{
+	struct attach_data data = {
+		.pasid_attach_fn = &iommufd_device_pasid_do_attach,
+		.pasid = pasid,
+	};
+
+	return iommufd_device_change_pt(idev, pt_id, &data);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_device_pasid_attach, IOMMUFD);
+
+/**
+ * iommufd_device_pasid_replace - Change the {device, pasid}'s iommu_domain
+ * @idev: device to change
+ * @pasid: pasid to change
+ * @pt_id: Input a IOMMUFD_OBJ_IOAS, or IOMMUFD_OBJ_HW_PAGETABLE
+ *         Output the IOMMUFD_OBJ_HW_PAGETABLE ID
+ *
+ * This is the same as
+ *   iommufd_device_pasid_detach();
+ *   iommufd_device_pasid_attach();
+ *
+ * If it fails then no change is made to the attachment. The iommu driver may
+ * implement this so there is no disruption in translation. This can only be
+ * called if iommufd_device_pasid_attach() has already succeeded.
+ *
+ * iommufd does not handle race between iommufd_device_pasid_replace(),
+ * iommufd_device_pasid_attach() and iommufd_device_pasid_detach().
+ * So caller of them should guarantee no concurrent call on the same
+ * device and pasid.
+ */
+int iommufd_device_pasid_replace(struct iommufd_device *idev,
+				 u32 pasid, u32 *pt_id)
+{
+	struct attach_data data = {
+		.pasid_attach_fn = &iommufd_device_pasid_do_replace,
+		.pasid = pasid,
+	};
+
+	return iommufd_device_change_pt(idev, pt_id, &data);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_device_pasid_replace, IOMMUFD);
+
+/**
+ * iommufd_device_pasid_detach - Disconnect a {device, pasid} to an iommu_domain
+ * @idev: device to detach
+ * @pasid: pasid to detach
+ *
+ * Undo iommufd_device_pasid_attach(). This disconnects the idev/pasid from
+ * the previously attached pt_id.
+ *
+ * iommufd does not handle race between iommufd_device_pasid_detach(),
+ * iommufd_device_pasid_attach() and iommufd_device_pasid_replace().
+ * So caller of them should guarantee no concurrent call on the same
+ * device and pasid.
+ */
+void iommufd_device_pasid_detach(struct iommufd_device *idev, u32 pasid)
+{
+	struct iommufd_hw_pagetable *hwpt;
+
+	hwpt = xa_erase(&idev->pasid_hwpts, pasid);
+	if (WARN_ON(!hwpt))
+		return;
+	iommu_detach_device_pasid(hwpt->domain, idev->dev, pasid);
+	iommufd_hw_pagetable_put(idev->ictx, hwpt);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_device_pasid_detach, IOMMUFD);
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index ffc3a949f837..0b007c376306 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -26,6 +26,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,
+				u32 pasid, u32 *pt_id);
+int iommufd_device_pasid_replace(struct iommufd_device *idev,
+				 u32 pasid, u32 *pt_id);
+void iommufd_device_pasid_detach(struct iommufd_device *idev, u32 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 v2 05/12] iommu: Allow iommu driver to populate the max_pasids

[Yi Liu]

Today, the iommu layer gets the max_pasids by pci_max_pasids() or reading
the "pasid-num-bits" property. This requires the non-PCI devices to have a
"pasid-num-bits" property. Like the mock device used in iommufd selftest,
otherwise the max_pasids check would fail in iommu layer.

While there is an alternative, the iommu layer can allow the iommu driver
to set the max_pasids in its probe_device() callback and populate it. This
is simpler and has no impact on the existing cases.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 343683e646e0..dc85c251237f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -368,9 +368,9 @@ static bool dev_has_iommu(struct device *dev)
 	return dev->iommu && dev->iommu->iommu_dev;
 }
 
-static u32 dev_iommu_get_max_pasids(struct device *dev)
+static void dev_iommu_set_max_pasids(struct device *dev)
 {
-	u32 max_pasids = 0, bits = 0;
+	u32 max_pasids = dev->iommu->max_pasids, bits = 0;
 	int ret;
 
 	if (dev_is_pci(dev)) {
@@ -383,7 +383,8 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
 			max_pasids = 1UL << bits;
 	}
 
-	return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
+	dev->iommu->max_pasids = min_t(u32, max_pasids,
+				       dev->iommu->iommu_dev->max_pasids);
 }
 
 void dev_iommu_priv_set(struct device *dev, void *priv)
@@ -433,7 +434,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	}
 	dev->iommu_group = group;
 
-	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
+	dev_iommu_set_max_pasids(dev);
 	if (ops->is_attach_deferred)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
 	return 0;
-- 
2.34.1

[PATCH v2 06/12] iommufd/selftest: Add set_dev_pasid and remove_dev_pasid in mock iommu

[Yi Liu]

The two callbacks are needed to make pasid_attach/detach path complete for
mock device. A nop is enough for set_dev_pasid, a domain type check in the
remove_dev_pasid is also helpful.

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

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 7a2199470f31..45e1328f0f5d 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -511,9 +511,35 @@ static struct iommu_device *mock_probe_device(struct device *dev)
 {
 	if (dev->bus != &iommufd_mock_bus_type.bus)
 		return ERR_PTR(-ENODEV);
+
+	dev->iommu->max_pasids = 1 << 20;//20 bits
 	return &mock_iommu_device;
 }
 
+static void mock_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+					struct iommu_domain *domain)
+{
+	/* Domain type specific cleanup: */
+	if (domain) {
+		switch (domain->type) {
+		case IOMMU_DOMAIN_NESTED:
+		case IOMMU_DOMAIN_UNMANAGED:
+			break;
+		default:
+			/* should never reach here */
+			WARN_ON(1);
+			break;
+		}
+	}
+}
+
+static int mock_domain_set_dev_pasid_nop(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old)
+{
+	return 0;
+}
+
 static const struct iommu_ops mock_ops = {
 	/*
 	 * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type()
@@ -529,6 +555,7 @@ static const struct iommu_ops mock_ops = {
 	.capable = mock_domain_capable,
 	.device_group = generic_device_group,
 	.probe_device = mock_probe_device,
+	.remove_dev_pasid = mock_iommu_remove_dev_pasid,
 	.default_domain_ops =
 		&(struct iommu_domain_ops){
 			.free = mock_domain_free,
@@ -536,6 +563,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,
 		},
 };
 
@@ -600,6 +628,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 *
@@ -1491,6 +1520,8 @@ int __init iommufd_test_init(void)
 				  &iommufd_mock_bus_type.nb);
 	if (rc)
 		goto err_sysfs;
+
+	mock_iommu_device.max_pasids = (1 << 20);//20 bits
 	return 0;
 
 err_sysfs:
-- 
2.34.1

[PATCH v2 07/12] iommufd/selftest: Add a helper to get test device

[Yi Liu]

There is need to get the selftest device (sobj->type == TYPE_IDEV) in
multiple places, so have a helper to for it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/selftest.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 45e1328f0f5d..e21b076b2223 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -794,29 +794,39 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
 	return rc;
 }
 
-/* Replace the mock domain with a manually allocated hw_pagetable */
-static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
-					    unsigned int device_id, u32 pt_id,
-					    struct iommu_test_cmd *cmd)
+static struct selftest_obj *
+iommufd_test_get_self_test_device(struct iommufd_ctx *ictx, u32 id)
 {
 	struct iommufd_object *dev_obj;
 	struct selftest_obj *sobj;
-	int rc;
 
 	/*
 	 * Prefer to use the OBJ_SELFTEST because the destroy_rwsem will ensure
 	 * it doesn't race with detach, which is not allowed.
 	 */
-	dev_obj =
-		iommufd_get_object(ucmd->ictx, device_id, IOMMUFD_OBJ_SELFTEST);
+	dev_obj = iommufd_get_object(ictx, id, IOMMUFD_OBJ_SELFTEST);
 	if (IS_ERR(dev_obj))
-		return PTR_ERR(dev_obj);
+		return (struct selftest_obj *)dev_obj;
 
 	sobj = container_of(dev_obj, struct selftest_obj, obj);
 	if (sobj->type != TYPE_IDEV) {
-		rc = -EINVAL;
-		goto out_dev_obj;
+		iommufd_put_object(ictx, dev_obj);
+		return ERR_PTR(-EINVAL);
 	}
+	return sobj;
+}
+
+/* Replace the mock domain with a manually allocated hw_pagetable */
+static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
+					    unsigned int device_id, u32 pt_id,
+					    struct iommu_test_cmd *cmd)
+{
+	struct selftest_obj *sobj;
+	int rc;
+
+	sobj = iommufd_test_get_self_test_device(ucmd->ictx, device_id);
+	if (IS_ERR(sobj))
+		return PTR_ERR(sobj);
 
 	rc = iommufd_device_replace(sobj->idev.idev, &pt_id);
 	if (rc)
@@ -826,7 +836,7 @@ static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 
 out_dev_obj:
-	iommufd_put_object(ucmd->ictx, dev_obj);
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
 	return rc;
 }
 
-- 
2.34.1

[PATCH v2 08/12] iommufd/selftest: Add test ops to test pasid attach/detach

[Yi Liu]

This adds 4 test ops for pasid attach/replace/detach testing. There are
ops to attach/detach pasid, and also op to check the attached domain of
a pasid.

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

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index e854d3f67205..ee6310f07749 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -22,6 +22,10 @@ enum {
 	IOMMU_TEST_OP_MOCK_DOMAIN_FLAGS,
 	IOMMU_TEST_OP_DIRTY,
 	IOMMU_TEST_OP_MD_CHECK_IOTLB,
+	IOMMU_TEST_OP_PASID_ATTACH,
+	IOMMU_TEST_OP_PASID_REPLACE,
+	IOMMU_TEST_OP_PASID_DETACH,
+	IOMMU_TEST_OP_PASID_CHECK_DOMAIN,
 };
 
 enum {
@@ -127,6 +131,32 @@ struct iommu_test_cmd {
 			__u32 id;
 			__u32 iotlb;
 		} check_iotlb;
+		struct {
+			__u32 pasid;
+			__u32 pt_id;
+			/* @id is stdev_id for IOMMU_TEST_OP_PASID_ATTACH
+			 * pasid#1024 is for special test, avoid use it
+			 * in normal case.
+			 */
+		} pasid_attach;
+		struct {
+			__u32 pasid;
+			__u32 pt_id;
+			/* @id is stdev_id for IOMMU_TEST_OP_PASID_ATTACH
+			 * pasid#1024 is for special test, avoid use it
+			 * in normal case.
+			 */
+		} pasid_replace;
+		struct {
+			__u32 pasid;
+			/* @id is stdev_id for IOMMU_TEST_OP_PASID_DETACH */
+		} pasid_detach;
+		struct {
+			__u32 pasid;
+			__u32 hwpt_id;
+			__u64 out_result_ptr;
+			/* @id is stdev_id for IOMMU_TEST_OP_HWPT_GET_DOMAIN */
+		} pasid_check;
 	};
 	__u32 last;
 };
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index e21b076b2223..45469213f2f9 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -516,6 +516,8 @@ static struct iommu_device *mock_probe_device(struct device *dev)
 	return &mock_iommu_device;
 }
 
+static bool pasid_1024_attached;
+
 static void mock_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 					struct iommu_domain *domain)
 {
@@ -524,6 +526,8 @@ static void mock_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 		switch (domain->type) {
 		case IOMMU_DOMAIN_NESTED:
 		case IOMMU_DOMAIN_UNMANAGED:
+			if (pasid == 1024)
+				pasid_1024_attached = false;
 			break;
 		default:
 			/* should never reach here */
@@ -537,6 +541,20 @@ static int mock_domain_set_dev_pasid_nop(struct iommu_domain *domain,
 					 struct device *dev, ioasid_t pasid,
 					 struct iommu_domain *old)
 {
+	/*
+	 * First attach with pasid 1024 succ, second attach would fail,
+	 * and so on. This is helpful to test the case in which the iommu
+	 * layer needs to rollback to old domain due to driver failure.
+	 */
+	if (pasid == 1024) {
+		if (pasid_1024_attached) {
+			pasid_1024_attached = false;
+			// Fake an error to fail the replacement
+			return -ENOMEM;
+		}
+		pasid_1024_attached = true;
+	}
+
 	return 0;
 }
 
@@ -1414,6 +1432,114 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 	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);
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
+	return rc;
+}
+
+static int iommufd_test_pasid_replace(struct iommufd_ucmd *ucmd,
+				      struct iommu_test_cmd *cmd)
+{
+	struct selftest_obj *sobj;
+	int rc;
+
+	sobj = iommufd_test_get_self_test_device(ucmd->ictx, cmd->id);
+	if (IS_ERR(sobj))
+		return PTR_ERR(sobj);
+
+	rc = iommufd_device_pasid_replace(sobj->idev.idev,
+					  cmd->pasid_attach.pasid,
+					  &cmd->pasid_attach.pt_id);
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
+	return rc;
+}
+
+static int iommufd_test_pasid_detach(struct iommufd_ucmd *ucmd,
+				     struct iommu_test_cmd *cmd)
+{
+	struct selftest_obj *sobj;
+
+	sobj = iommufd_test_get_self_test_device(ucmd->ictx, cmd->id);
+	if (IS_ERR(sobj))
+		return PTR_ERR(sobj);
+
+	iommufd_device_pasid_detach(sobj->idev.idev,
+				    cmd->pasid_detach.pasid);
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
+	return 0;
+}
+
+static inline struct iommufd_hw_pagetable *
+iommufd_get_hwpt(struct iommufd_ucmd *ucmd, u32 id)
+{
+	struct iommufd_object *pt_obj;
+
+	pt_obj = iommufd_get_object(ucmd->ictx, id, IOMMUFD_OBJ_ANY);
+	if (IS_ERR(pt_obj))
+		return ERR_CAST(pt_obj);
+
+	if (pt_obj->type != IOMMUFD_OBJ_HWPT_NESTED &&
+	    pt_obj->type != IOMMUFD_OBJ_HWPT_PAGING) {
+		iommufd_put_object(ucmd->ictx, pt_obj);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return container_of(pt_obj, struct iommufd_hw_pagetable, obj);
+}
+
+static int iommufd_test_pasid_check_domain(struct iommufd_ucmd *ucmd,
+					   struct iommu_test_cmd *cmd)
+{
+	struct iommu_domain *attached_domain, *expect_domain = NULL;
+	struct iommufd_hw_pagetable *hwpt = NULL;
+	struct 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;
+
+	attached_domain = iommu_get_domain_for_dev_pasid(&mdev->dev,
+							 cmd->pasid_check.pasid, 0);
+	if (IS_ERR(attached_domain))
+		attached_domain = NULL;
+
+	if (cmd->pasid_check.hwpt_id) {
+		hwpt = iommufd_get_hwpt(ucmd, cmd->pasid_check.hwpt_id);
+		if (IS_ERR(hwpt)) {
+			rc = PTR_ERR(hwpt);
+			goto out_put_dev;
+		}
+		expect_domain = hwpt->domain;
+	}
+
+	result = (attached_domain == expect_domain) ? 1 : 0;
+	if (copy_to_user(u64_to_user_ptr(cmd->pasid_check.out_result_ptr),
+			 &result, sizeof(result)))
+		rc = -EFAULT;
+	if (hwpt)
+		iommufd_put_object(ucmd->ictx, &hwpt->obj);
+out_put_dev:
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
+	return rc;
+}
+
 void iommufd_selftest_destroy(struct iommufd_object *obj)
 {
 	struct selftest_obj *sobj = container_of(obj, struct selftest_obj, obj);
@@ -1489,6 +1615,14 @@ int iommufd_test(struct iommufd_ucmd *ucmd)
 					  cmd->dirty.page_size,
 					  u64_to_user_ptr(cmd->dirty.uptr),
 					  cmd->dirty.flags);
+	case IOMMU_TEST_OP_PASID_ATTACH:
+		return iommufd_test_pasid_attach(ucmd, cmd);
+	case IOMMU_TEST_OP_PASID_REPLACE:
+		return iommufd_test_pasid_replace(ucmd, cmd);
+	case IOMMU_TEST_OP_PASID_DETACH:
+		return iommufd_test_pasid_detach(ucmd, cmd);
+	case IOMMU_TEST_OP_PASID_CHECK_DOMAIN:
+		return iommufd_test_pasid_check_domain(ucmd, cmd);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -1532,6 +1666,7 @@ int __init iommufd_test_init(void)
 		goto err_sysfs;
 
 	mock_iommu_device.max_pasids = (1 << 20);//20 bits
+	pasid_1024_attached = false;
 	return 0;
 
 err_sysfs:
-- 
2.34.1

[PATCH v2 09/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       | 207 ++++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  28 ++-
 tools/testing/selftests/iommu/iommufd_utils.h |  78 +++++++
 3 files changed, 309 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index edf1c99c9936..d18b4d24fe65 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -2346,4 +2346,211 @@ TEST_F(vfio_compat_mock_domain, huge_map)
 	}
 }
 
+FIXTURE(iommufd_device_pasid)
+{
+	int fd;
+	uint32_t ioas_id;
+	uint32_t hwpt_id;
+	uint32_t stdev_id;
+	uint32_t device_id;
+};
+
+FIXTURE_SETUP(iommufd_device_pasid)
+{
+	self->fd = open("/dev/iommu", O_RDWR);
+	ASSERT_NE(-1, self->fd);
+	test_ioctl_ioas_alloc(&self->ioas_id);
+
+	test_cmd_mock_domain(self->ioas_id, &self->stdev_id,
+			     &self->hwpt_id, &self->device_id);
+}
+
+FIXTURE_TEARDOWN(iommufd_device_pasid)
+{
+	teardown_iommufd(self->fd, _metadata);
+}
+
+TEST_F(iommufd_device_pasid, pasid_attach)
+{
+	if (self->device_id) {
+		struct iommu_hwpt_selftest data = {
+			.iotlb =  IOMMU_TEST_IOTLB_DEFAULT,
+		};
+		uint32_t nested_hwpt_id[2] = {};
+		uint32_t parent_hwpt_id = 0;
+		uint32_t pasid = 100;
+		bool result;
+
+		/* Allocate two nested hwpts sharing one common parent hwpt */
+		test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
+				    IOMMU_HWPT_ALLOC_NEST_PARENT,
+				    &parent_hwpt_id);
+
+		test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id, 0,
+					   &nested_hwpt_id[0],
+					   IOMMU_HWPT_DATA_SELFTEST,
+					   &data, sizeof(data));
+		test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id, 0,
+					   &nested_hwpt_id[1],
+					   IOMMU_HWPT_DATA_SELFTEST,
+					   &data, sizeof(data));
+
+		/*
+		 * Attach ioas to pasid 100, should succeed, domain should
+		 * be valid.
+		 */
+		test_cmd_pasid_attach(pasid, self->ioas_id);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, self->hwpt_id,
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Try attach pasid 100 with self->ioas_id, should succeed
+		 * as it is the same with existing hwpt.
+		 */
+		test_cmd_pasid_attach(pasid, self->ioas_id);
+
+		/*
+		 * Try attach pasid 100 with another hwpt, should FAIL
+		 * as attach does not allow overwrite, use REPLACE instead.
+		 */
+		test_err_cmd_pasid_attach(EBUSY, pasid, nested_hwpt_id[0]);
+
+		/*
+		 * Detach hwpt from pasid 100, and check if the pasid 100
+		 * has null domain. Should be done before the next attach.
+		 */
+		test_cmd_pasid_detach(pasid);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, 0, &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Attach nested hwpt to pasid 100, should succeed, domain
+		 * should be valid.
+		 */
+		test_cmd_pasid_attach(pasid, nested_hwpt_id[0]);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, nested_hwpt_id[0],
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Detach hwpt from pasid 100, and check if the pasid 100
+		 * has null domain
+		 */
+		test_cmd_pasid_detach(pasid);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, 0, &result));
+		EXPECT_EQ(1, result);
+
+		/* Replace tests */
+		pasid = 200;
+
+		/*
+		 * Replace pasid 200 without attaching it first, should
+		 * fail with -EINVAL.
+		 */
+		test_err_cmd_pasid_replace(EINVAL, pasid, parent_hwpt_id);
+
+		/*
+		 * Attach a s2 hwpt to pasid 200, should succeed, domain should
+		 * be valid.
+		 */
+		test_cmd_pasid_attach(pasid, parent_hwpt_id);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, parent_hwpt_id,
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Replace pasid 200 with self->ioas_id, should succeed,
+		 * and have valid domain.
+		 */
+		test_cmd_pasid_replace(pasid, self->ioas_id);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, self->hwpt_id,
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Replace a nested hwpt for pasid 200, should succeed,
+		 * and have valid domain.
+		 */
+		test_cmd_pasid_replace(pasid, nested_hwpt_id[0]);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, nested_hwpt_id[0],
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Replace with another nested hwpt for pasid 200, should
+		 * succeed, and have valid domain.
+		 */
+		test_cmd_pasid_replace(pasid, nested_hwpt_id[1]);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, nested_hwpt_id[1],
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Detach hwpt from pasid 200, and check if the pasid 200
+		 * has null domain.
+		 */
+		test_cmd_pasid_detach(pasid);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, 0, &result));
+		EXPECT_EQ(1, result);
+
+		/* Negative Tests for pasid replace, use pasid 1024 */
+
+		/*
+		 * Attach a s2 hwpt to pasid 1024, should succeed, domain should
+		 * be valid.
+		 */
+		pasid = 1024;
+		test_cmd_pasid_attach(pasid, parent_hwpt_id);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, parent_hwpt_id,
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Replace pasid 1024 with self->ioas_id, should fail.
+		 * but have the old valid domain.
+		 */
+		test_err_cmd_pasid_replace(ENOMEM, pasid, self->ioas_id);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, parent_hwpt_id,
+						      &result));
+		EXPECT_EQ(1, result);
+
+		/*
+		 * Detach hwpt from pasid 1024, and check if the pasid 1024
+		 * has null domain.
+		 */
+		test_cmd_pasid_detach(pasid);
+		ASSERT_EQ(0,
+			  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
+						      pasid, 0, &result));
+		EXPECT_EQ(1, result);
+
+		test_ioctl_destroy(nested_hwpt_id[0]);
+		test_ioctl_destroy(nested_hwpt_id[1]);
+		test_ioctl_destroy(parent_hwpt_id);
+	}
+}
+
 TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/iommu/iommufd_fail_nth.c b/tools/testing/selftests/iommu/iommufd_fail_nth.c
index f590417cd67a..6d1b03e73b9d 100644
--- a/tools/testing/selftests/iommu/iommufd_fail_nth.c
+++ b/tools/testing/selftests/iommu/iommufd_fail_nth.c
@@ -206,12 +206,16 @@ FIXTURE(basic_fail_nth)
 {
 	int fd;
 	uint32_t access_id;
+	uint32_t stdev_id;
+	uint32_t pasid;
 };
 
 FIXTURE_SETUP(basic_fail_nth)
 {
 	self->fd = -1;
 	self->access_id = 0;
+	self->stdev_id = 0;
+	self->pasid = 0; //test should use a non-zero value
 }
 
 FIXTURE_TEARDOWN(basic_fail_nth)
@@ -223,6 +227,8 @@ FIXTURE_TEARDOWN(basic_fail_nth)
 		rc = _test_cmd_destroy_access(self->access_id);
 		assert(rc == 0);
 	}
+	if (self->pasid && self->stdev_id)
+		_test_cmd_pasid_detach(self->fd, self->stdev_id, self->pasid);
 	teardown_iommufd(self->fd, _metadata);
 }
 
@@ -579,7 +585,6 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 	struct iommu_test_hw_info info;
 	uint32_t ioas_id;
 	uint32_t ioas_id2;
-	uint32_t stdev_id;
 	uint32_t idev_id;
 	uint32_t hwpt_id;
 	__u64 iova;
@@ -608,7 +613,7 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 
 	fail_nth_enable();
 
-	if (_test_cmd_mock_domain(self->fd, ioas_id, &stdev_id, NULL,
+	if (_test_cmd_mock_domain(self->fd, ioas_id, &self->stdev_id, NULL,
 				  &idev_id))
 		return -1;
 
@@ -619,11 +624,26 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 				 IOMMU_HWPT_DATA_NONE, 0, 0))
 		return -1;
 
-	if (_test_cmd_mock_domain_replace(self->fd, stdev_id, ioas_id2, NULL))
+	if (_test_cmd_mock_domain_replace(self->fd, self->stdev_id, ioas_id2, NULL))
+		return -1;
+
+	if (_test_cmd_mock_domain_replace(self->fd, self->stdev_id, hwpt_id, NULL))
 		return -1;
 
-	if (_test_cmd_mock_domain_replace(self->fd, stdev_id, hwpt_id, NULL))
+	self->pasid = 200;
+
+	/* Tests for pasid attach/replace/detach */
+	if (_test_cmd_pasid_attach(self->fd, self->stdev_id, self->pasid, ioas_id))
 		return -1;
+
+	if (_test_cmd_pasid_replace(self->fd, self->stdev_id, self->pasid, ioas_id2))
+		return -1;
+
+	if (_test_cmd_pasid_detach(self->fd, self->stdev_id, self->pasid))
+		return -1;
+
+	self->pasid = 0;
+
 	return 0;
 }
 
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 8d2b46b2114d..9b112b108670 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -684,3 +684,81 @@ static int _test_cmd_get_hw_info(int fd, __u32 device_id, void *data,
 
 #define test_cmd_get_hw_capabilities(device_id, caps, mask) \
 	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, NULL, 0, &caps))
+
+static int _test_cmd_pasid_attach(int fd, __u32 stdev_id, __u32 pasid, __u32 pt_id)
+{
+	struct iommu_test_cmd test_attach = {
+		.size = sizeof(test_attach),
+		.op = IOMMU_TEST_OP_PASID_ATTACH,
+		.id = stdev_id,
+		.pasid_attach = {
+			.pasid = pasid,
+			.pt_id = pt_id,
+		},
+	};
+
+	return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_ATTACH), &test_attach);
+}
+
+#define test_cmd_pasid_attach(pasid, hwpt_id) \
+	ASSERT_EQ(0, _test_cmd_pasid_attach(self->fd, self->stdev_id, pasid, hwpt_id))
+
+#define test_err_cmd_pasid_attach(_errno, pasid, hwpt_id) \
+	EXPECT_ERRNO(_errno, \
+		     _test_cmd_pasid_attach(self->fd, self->stdev_id, pasid, hwpt_id))
+
+static int _test_cmd_pasid_replace(int fd, __u32 stdev_id, __u32 pasid, __u32 pt_id)
+{
+	struct iommu_test_cmd test_replace = {
+		.size = sizeof(test_replace),
+		.op = IOMMU_TEST_OP_PASID_REPLACE,
+		.id = stdev_id,
+		.pasid_replace = {
+			.pasid = pasid,
+			.pt_id = pt_id,
+		},
+	};
+
+	return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_REPLACE), &test_replace);
+}
+
+#define test_cmd_pasid_replace(pasid, hwpt_id) \
+	ASSERT_EQ(0, _test_cmd_pasid_replace(self->fd, self->stdev_id, pasid, hwpt_id))
+
+#define test_err_cmd_pasid_replace(_errno, pasid, hwpt_id) \
+	EXPECT_ERRNO(_errno, \
+		     _test_cmd_pasid_replace(self->fd, self->stdev_id, pasid, hwpt_id))
+
+static int _test_cmd_pasid_detach(int fd, __u32 stdev_id, __u32 pasid)
+{
+	struct iommu_test_cmd test_detach = {
+		.size = sizeof(test_detach),
+		.op = IOMMU_TEST_OP_PASID_DETACH,
+		.id = stdev_id,
+		.pasid_detach = {
+			.pasid = pasid,
+		},
+	};
+
+	return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_DETACH), &test_detach);
+}
+
+#define test_cmd_pasid_detach(pasid) \
+	ASSERT_EQ(0, _test_cmd_pasid_detach(self->fd, self->stdev_id, pasid))
+
+static int test_cmd_pasid_check_domain(int fd, __u32 stdev_id, __u32 pasid,
+				       __u32 hwpt_id, bool *result)
+{
+	struct iommu_test_cmd test_pasid_check = {
+		.size = sizeof(test_pasid_check),
+		.op = IOMMU_TEST_OP_PASID_CHECK_DOMAIN,
+		.id = stdev_id,
+		.pasid_check = {
+			.pasid = pasid,
+			.hwpt_id = hwpt_id,
+			.out_result_ptr = (__u64)result,
+		},
+	};
+
+	return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_PASID_CHECK_DOMAIN), &test_pasid_check);
+}
-- 
2.34.1

[PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Yi Liu]

If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
is fine, but no need to go further as nothing to be cleanup and also it
may hit unknown issue.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index df49aed3df5e..fff7dea012a7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4614,8 +4614,9 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 			break;
 		}
 	}
-	WARN_ON_ONCE(!dev_pasid);
 	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+	if (WARN_ON_ONCE(!dev_pasid))
+		return;
 
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
-- 
2.34.1

[PATCH v2 11/12] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

Existing intel_iommu_set_dev_pasid() does not support domain replacement.
However, iommu layer requires set_dev_pasid() to handle domain replacement.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index fff7dea012a7..9e79ffdd47db 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4646,6 +4646,10 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (context_copied(iommu, info->bus, info->devfn))
 		return -EBUSY;
 
+	/* Block old translation */
+	if (old)
+		intel_iommu_remove_dev_pasid(dev, pasid, old);
+
 	ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

From: Lu Baolu <baolu.lu@linux.intel.com>

This allows the upper layers to set a nested type domain to a PASID of a
device if the PASID feature is supported by the IOMMU hardware.

The set_dev_pasid callback for non-nested domain has already be there, so
this only needs to add it for nested domains. Note that the S2 domain with
dirty tracking capability is not supported yet as no user for now.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 23 +++++++++++++++++++----
 drivers/iommu/intel/iommu.h  |  3 +++
 drivers/iommu/intel/nested.c | 15 +++++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9e79ffdd47db..052b90917ced 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -319,6 +319,11 @@ static int domain_type_is_si(struct dmar_domain *domain)
 	return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
 }
 
+static int domain_type_is_nested(struct dmar_domain *domain)
+{
+	return domain->domain.type == IOMMU_DOMAIN_NESTED;
+}
+
 static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn)
 {
 	int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
@@ -4626,9 +4631,9 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	intel_drain_pasid_prq(dev, pasid);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4650,7 +4655,15 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (old)
 		intel_iommu_remove_dev_pasid(dev, pasid, old);
 
-	ret = prepare_domain_attach_device(domain, dev);
+	/*
+	 * Nested type domain should adjust its parent domain according
+	 * to iommu capability.
+	 */
+	if (domain_type_is_nested(dmar_domain))
+		ret = prepare_domain_attach_device(
+				&dmar_domain->s2_domain->domain, dev);
+	else
+		ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
 		return ret;
 
@@ -4664,6 +4677,8 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 
 	if (domain_type_is_si(dmar_domain))
 		ret = intel_pasid_setup_pass_through(iommu, dev, pasid);
+	else if (domain_type_is_nested(dmar_domain))
+		ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain);
 	else if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 404d2476a877..3dfd183c9736 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1082,6 +1082,9 @@ void device_block_translation(struct device *dev);
 int prepare_domain_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 void domain_update_iommu_cap(struct dmar_domain *domain);
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old);
 
 int dmar_ir_support(void);
 
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index a7d68f3d518a..7cb124cc0ca0 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -70,6 +70,20 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
 	return 0;
 }
 
+static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+
+	if (iommu->agaw < dmar_domain->s2_domain->agaw)
+		return -EINVAL;
+
+	return intel_iommu_set_dev_pasid(domain, dev, pasid, old);
+}
+
 static void intel_nested_domain_free(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -170,6 +184,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.attach_dev		= intel_nested_attach_dev,
+	.set_dev_pasid		= intel_nested_set_dev_pasid,
 	.free			= intel_nested_domain_free,
 	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
 };
-- 
2.34.1

Re: [PATCH v2 01/12] iommu: Pass old domain to set_dev_pasid op

[Baolu Lu]

On 4/12/24 4:15 PM, Yi Liu wrote:
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 40dd439307e8..1e5e9249c93f 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -631,7 +631,7 @@ struct iommu_ops {
>   struct iommu_domain_ops {
>   	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>   	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
> -			     ioasid_t pasid);
> +			     ioasid_t pasid, struct iommu_domain *old);

Is it possible to add another op to replace a domain for pasid? For
example,

	int (*replace_dev_pasid)(domain, dev, pasid, old_domain)

Best regards,
baolu

Re: [PATCH v2 05/12] iommu: Allow iommu driver to populate the max_pasids

[Baolu Lu]

On 4/12/24 4:15 PM, Yi Liu wrote:
> Today, the iommu layer gets the max_pasids by pci_max_pasids() or reading
> the "pasid-num-bits" property. This requires the non-PCI devices to have a
> "pasid-num-bits" property. Like the mock device used in iommufd selftest,
> otherwise the max_pasids check would fail in iommu layer.
> 
> While there is an alternative, the iommu layer can allow the iommu driver
> to set the max_pasids in its probe_device() callback and populate it. This
> is simpler and has no impact on the existing cases.
> 
> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommu.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

The code does not appear to match the commit message here.

The code in change is a refactoring by folding the max_pasid assignment
into its helper. However, the commit message suggests a desire to expose
some kind of kAPI for device drivers.

Best regards,
baolu

Re: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Baolu Lu]

On 4/12/24 4:15 PM, Yi Liu wrote:
> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
> is fine, but no need to go further as nothing to be cleanup and also it
> may hit unknown issue.

If "... it should be a problem of caller ...", then the check and WARN()
should be added in the caller instead of individual drivers.

> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index df49aed3df5e..fff7dea012a7 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4614,8 +4614,9 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>   			break;
>   		}
>   	}
> -	WARN_ON_ONCE(!dev_pasid);
>   	spin_unlock_irqrestore(&dmar_domain->lock, flags);
> +	if (WARN_ON_ONCE(!dev_pasid))
> +		return;

The iommu core calls remove_dev_pasid op to tear down the translation on
a pasid and park it in a BLOCKED state. Since this is a must-be-
successful callback, it makes no sense to return before tearing down the
pasid table entry.

 From the Intel iommu driver's perspective, the pasid devices have
already been tracked in the core, hence the dev_pasid is a duplicate and
will be removed later, so don't use it for other purposes.

In the end, perhaps we just need to remove the WARN_ON() from the code.

>   
>   	domain_detach_iommu(dmar_domain, iommu);
>   	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);

Best regards,
baolu

Re: [PATCH v2 01/12] iommu: Pass old domain to set_dev_pasid op

[Jason Gunthorpe]

On Mon, Apr 15, 2024 at 01:32:03PM +0800, Baolu Lu wrote:
> On 4/12/24 4:15 PM, Yi Liu wrote:
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 40dd439307e8..1e5e9249c93f 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -631,7 +631,7 @@ struct iommu_ops {
> >   struct iommu_domain_ops {
> >   	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> >   	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
> > -			     ioasid_t pasid);
> > +			     ioasid_t pasid, struct iommu_domain *old);
> 
> Is it possible to add another op to replace a domain for pasid? For
> example,
> 
> 	int (*replace_dev_pasid)(domain, dev, pasid, old_domain)

We haven't needed that in the normal case, what would motivate it
here?

Jason

Re: [PATCH v2 01/12] iommu: Pass old domain to set_dev_pasid op

[Baolu Lu]

On 4/15/24 7:54 PM, Jason Gunthorpe wrote:
> On Mon, Apr 15, 2024 at 01:32:03PM +0800, Baolu Lu wrote:
>> On 4/12/24 4:15 PM, Yi Liu wrote:
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 40dd439307e8..1e5e9249c93f 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -631,7 +631,7 @@ struct iommu_ops {
>>>    struct iommu_domain_ops {
>>>    	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>>>    	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
>>> -			     ioasid_t pasid);
>>> +			     ioasid_t pasid, struct iommu_domain *old);
>> Is it possible to add another op to replace a domain for pasid? For
>> example,
>>
>> 	int (*replace_dev_pasid)(domain, dev, pasid, old_domain)
> We haven't needed that in the normal case, what would motivate it
> here?

My understanding of the difference between set_dev_pasid and
replace_dev_pasid is that the former assumes that there is no domain
attached to the pasid yet, so it sets the passed domain to it. For the
latter, it simply replaces the existing domain with a new one.

The set_dev_pasid doesn't need an old domain because it's assumed that
the existing domain is NULL. The replace_dev_pasid could have an
existing domain as its input.

Replace also implies an atomic switch between different domains. This
makes it subtly different from a set operation.

Best regards,
baolu

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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: [PATCH v2 02/12] iommu: Introduce a replace API for device pasid
>
>Provide a high-level API to allow replacements of one domain with
>another for specific pasid of a device. This is similar to
>iommu_group_replace_domain() and it is expected to be used only by
>IOMMUFD.
>
>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 |  3 ++
> drivers/iommu/iommu.c      | 92
>+++++++++++++++++++++++++++++++++++---
> 2 files changed, 89 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
>index 5f731d994803..0949c02cee93 100644
>--- a/drivers/iommu/iommu-priv.h
>+++ b/drivers/iommu/iommu-priv.h
>@@ -20,6 +20,9 @@ static inline const struct iommu_ops
>*dev_iommu_ops(struct device *dev)
> int iommu_group_replace_domain(struct iommu_group *group,
> 			       struct iommu_domain *new_domain);
>
>+int iommu_replace_device_pasid(struct iommu_domain *domain,
>+			       struct device *dev, ioasid_t pasid);
>+
> int iommu_device_register_bus(struct iommu_device *iommu,
> 			      const struct iommu_ops *ops,
> 			      const struct bus_type *bus,
>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>index 701b02a118db..343683e646e0 100644
>--- a/drivers/iommu/iommu.c
>+++ b/drivers/iommu/iommu.c
>@@ -3315,14 +3315,15 @@ bool
>iommu_group_dma_owner_claimed(struct iommu_group *group)
> EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
>
> 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;
> 	}
>@@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct
>iommu_domain *domain,
> err_revert:
> 	last_gdev = device;
> 	for_each_group_device(group, device) {
>-		const struct iommu_ops *ops = dev_iommu_ops(device-
>>dev);
>+		/*
>+		 * If no old domain, just undo all the devices/pasid that
>+		 * have attached to the new domain.
>+		 */
>+		if (!old) {
>+			const struct iommu_ops *ops =
>+						dev_iommu_ops(device->dev);
>+
>+			if (device == last_gdev)

Maybe this check can be moved to beginning of the for loop,

>+				break;
>+			ops = dev_iommu_ops(device->dev);
>+			ops->remove_dev_pasid(device->dev, pasid, domain);
>+			continue;
>+		}
>
>-		if (device == last_gdev)
>+		/*
>+		 * Rollback the devices/pasid that have attached to the new
>+		 * domain. And it is a driver bug to fail attaching with a
>+		 * previously good domain.
>+		 */
>+		if (device == last_gdev) {

then this check can be removed.

>+			WARN_ON(old->ops->set_dev_pasid(old, device-
>>dev,
>+							pasid, NULL));

Is this call necessary? last_gdev is the first device failed.

Thanks
Zhenzhong

> 			break;
>-		ops->remove_dev_pasid(device->dev, pasid, domain);
>+		}
>+
>+		WARN_ON(old->ops->set_dev_pasid(old, device->dev,
>+						pasid, domain));
> 	}
> 	return ret;
> }
>@@ -3395,7 +3419,7 @@ int iommu_attach_device_pasid(struct
>iommu_domain *domain,
> 		goto out_unlock;
> 	}
>
>-	ret = __iommu_set_group_pasid(domain, group, pasid);
>+	ret = __iommu_set_group_pasid(domain, group, pasid, NULL);
> 	if (ret)
> 		xa_erase(&group->pasid_array, pasid);
> out_unlock:
>@@ -3404,6 +3428,62 @@ int iommu_attach_device_pasid(struct
>iommu_domain *domain,
> }
> EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
>
>+/**
>+ * iommu_replace_device_pasid - replace the domain that a pasid is
>attached to
>+ * @domain: new IOMMU domain to replace with
>+ * @dev: the physical device
>+ * @pasid: pasid that will be attached to the new domain
>+ *
>+ * This API allows the pasid to switch domains. Return 0 on success, or an
>+ * error. The pasid will roll back to use the old domain if failure. The
>+ * caller could call iommu_detach_device_pasid() before free the old
>domain
>+ * in order to avoid use-after-free case.
>+ */
>+int iommu_replace_device_pasid(struct iommu_domain *domain,
>+			       struct device *dev, ioasid_t pasid)
>+{
>+	/* Caller must be a probed driver on dev */
>+	struct iommu_group *group = dev->iommu_group;
>+	void *curr;
>+	int ret;
>+
>+	if (!domain)
>+		return -EINVAL;
>+
>+	if (!domain->ops->set_dev_pasid)
>+		return -EOPNOTSUPP;
>+
>+	if (!group)
>+		return -ENODEV;
>+
>+	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain-
>>owner)
>+		return -EINVAL;
>+
>+	mutex_lock(&group->mutex);
>+	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
>+	if (!curr) {
>+		xa_erase(&group->pasid_array, pasid);
>+		ret = -EINVAL;
>+		goto out_unlock;
>+	}
>+
>+	ret = xa_err(curr);
>+	if (ret)
>+		goto out_unlock;
>+
>+	if (curr == domain)
>+		goto out_unlock;
>+
>+	ret = __iommu_set_group_pasid(domain, group, pasid, curr);
>+	if (ret)
>+		WARN_ON(xa_err(xa_store(&group->pasid_array, pasid,
>+					curr, GFP_KERNEL)));
>+out_unlock:
>+	mutex_unlock(&group->mutex);
>+	return ret;
>+}
>+EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid,
>IOMMUFD_INTERNAL);
>+
> /*
>  * iommu_detach_device_pasid() - Detach the domain from pasid of device
>  * @domain: the iommu domain.
>--
>2.34.1

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

[Yi Liu]

On 2024/4/16 11:01, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Subject: [PATCH v2 02/12] iommu: Introduce a replace API for device pasid
>>
>> Provide a high-level API to allow replacements of one domain with
>> another for specific pasid of a device. This is similar to
>> iommu_group_replace_domain() and it is expected to be used only by
>> IOMMUFD.
>>
>> 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 |  3 ++
>> drivers/iommu/iommu.c      | 92
>> +++++++++++++++++++++++++++++++++++---
>> 2 files changed, 89 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
>> index 5f731d994803..0949c02cee93 100644
>> --- a/drivers/iommu/iommu-priv.h
>> +++ b/drivers/iommu/iommu-priv.h
>> @@ -20,6 +20,9 @@ static inline const struct iommu_ops
>> *dev_iommu_ops(struct device *dev)
>> int iommu_group_replace_domain(struct iommu_group *group,
>> 			       struct iommu_domain *new_domain);
>>
>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +			       struct device *dev, ioasid_t pasid);
>> +
>> int iommu_device_register_bus(struct iommu_device *iommu,
>> 			      const struct iommu_ops *ops,
>> 			      const struct bus_type *bus,
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 701b02a118db..343683e646e0 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3315,14 +3315,15 @@ bool
>> iommu_group_dma_owner_claimed(struct iommu_group *group)
>> EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
>>
>> 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;
>> 	}
>> @@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct
>> iommu_domain *domain,
>> err_revert:
>> 	last_gdev = device;
>> 	for_each_group_device(group, device) {
>> -		const struct iommu_ops *ops = dev_iommu_ops(device-
>>> dev);
>> +		/*
>> +		 * If no old domain, just undo all the devices/pasid that
>> +		 * have attached to the new domain.
>> +		 */
>> +		if (!old) {
>> +			const struct iommu_ops *ops =
>> +						dev_iommu_ops(device->dev);
>> +
>> +			if (device == last_gdev)
> 
> Maybe this check can be moved to beginning of the for loop,
> 
>> +				break;
>> +			ops = dev_iommu_ops(device->dev);
>> +			ops->remove_dev_pasid(device->dev, pasid, domain);
>> +			continue;
>> +		}
>>
>> -		if (device == last_gdev)
>> +		/*
>> +		 * Rollback the devices/pasid that have attached to the new
>> +		 * domain. And it is a driver bug to fail attaching with a
>> +		 * previously good domain.
>> +		 */
>> +		if (device == last_gdev) {
> 
> then this check can be removed.
> 
>> +			WARN_ON(old->ops->set_dev_pasid(old, device-
>>> dev,
>> +							pasid, NULL));
> 
> Is this call necessary? last_gdev is the first device failed.


It is since we claim replacement failure would leave the pasid to be
attached with the prior attached domain. That's why old is checked
in the above. If no old domain, it would exit the loop if it is the
first failed device. Nothing need to be done for the failed device.

> Thanks
> Zhenzhong
> 
>> 			break;
>> -		ops->remove_dev_pasid(device->dev, pasid, domain);
>> +		}
>> +
>> +		WARN_ON(old->ops->set_dev_pasid(old, device->dev,
>> +						pasid, domain));
>> 	}
>> 	return ret;
>> }
>> @@ -3395,7 +3419,7 @@ int iommu_attach_device_pasid(struct
>> iommu_domain *domain,
>> 		goto out_unlock;
>> 	}
>>
>> -	ret = __iommu_set_group_pasid(domain, group, pasid);
>> +	ret = __iommu_set_group_pasid(domain, group, pasid, NULL);
>> 	if (ret)
>> 		xa_erase(&group->pasid_array, pasid);
>> out_unlock:
>> @@ -3404,6 +3428,62 @@ int iommu_attach_device_pasid(struct
>> iommu_domain *domain,
>> }
>> EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
>>
>> +/**
>> + * iommu_replace_device_pasid - replace the domain that a pasid is
>> attached to
>> + * @domain: new IOMMU domain to replace with
>> + * @dev: the physical device
>> + * @pasid: pasid that will be attached to the new domain
>> + *
>> + * This API allows the pasid to switch domains. Return 0 on success, or an
>> + * error. The pasid will roll back to use the old domain if failure. The
>> + * caller could call iommu_detach_device_pasid() before free the old
>> domain
>> + * in order to avoid use-after-free case.
>> + */
>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +			       struct device *dev, ioasid_t pasid)
>> +{
>> +	/* Caller must be a probed driver on dev */
>> +	struct iommu_group *group = dev->iommu_group;
>> +	void *curr;
>> +	int ret;
>> +
>> +	if (!domain)
>> +		return -EINVAL;
>> +
>> +	if (!domain->ops->set_dev_pasid)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!group)
>> +		return -ENODEV;
>> +
>> +	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain-
>>> owner)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&group->mutex);
>> +	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
>> +	if (!curr) {
>> +		xa_erase(&group->pasid_array, pasid);
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
>> +	ret = xa_err(curr);
>> +	if (ret)
>> +		goto out_unlock;
>> +
>> +	if (curr == domain)
>> +		goto out_unlock;
>> +
>> +	ret = __iommu_set_group_pasid(domain, group, pasid, curr);
>> +	if (ret)
>> +		WARN_ON(xa_err(xa_store(&group->pasid_array, pasid,
>> +					curr, GFP_KERNEL)));
>> +out_unlock:
>> +	mutex_unlock(&group->mutex);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid,
>> IOMMUFD_INTERNAL);
>> +
>> /*
>>   * iommu_detach_device_pasid() - Detach the domain from pasid of device
>>   * @domain: the iommu domain.
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu

Re: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Yi Liu]

n 2024/4/15 14:04, Baolu Lu wrote:
> On 4/12/24 4:15 PM, Yi Liu wrote:
>> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
>> is fine, but no need to go further as nothing to be cleanup and also it
>> may hit unknown issue.
> 
> If "... it should be a problem of caller ...", then the check and WARN()
> should be added in the caller instead of individual drivers.
> 
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index df49aed3df5e..fff7dea012a7 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4614,8 +4614,9 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid,
>>               break;
>>           }
>>       }
>> -    WARN_ON_ONCE(!dev_pasid);
>>       spin_unlock_irqrestore(&dmar_domain->lock, flags);
>> +    if (WARN_ON_ONCE(!dev_pasid))
>> +        return;
> 
> The iommu core calls remove_dev_pasid op to tear down the translation on
> a pasid and park it in a BLOCKED state. Since this is a must-be-
> successful callback, it makes no sense to return before tearing down the
> pasid table entry.

but if no dev_pasid is found, does it mean there is no pasid table entry
to be destroyed? That's why I think it deserves a warn, but no need to
continue.

> 
>  From the Intel iommu driver's perspective, the pasid devices have
> already been tracked in the core, hence the dev_pasid is a duplicate and
> will be removed later, so don't use it for other purposes.


good to know it. But for the current code, if we continue, it would hit
call trace in the end in the intel_iommu_debugfs_remove_dev_pasid().

> In the end, perhaps we just need to remove the WARN_ON() from the code.
> 
>>       domain_detach_iommu(dmar_domain, iommu);
>>       intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
> 
> Best regards,
> baolu

-- 
Regards,
Yi Liu

Re: [PATCH v2 01/12] iommu: Pass old domain to set_dev_pasid op

[Jason Gunthorpe]

On Tue, Apr 16, 2024 at 10:07:49AM +0800, Baolu Lu wrote:
> On 4/15/24 7:54 PM, Jason Gunthorpe wrote:
> > On Mon, Apr 15, 2024 at 01:32:03PM +0800, Baolu Lu wrote:
> > > On 4/12/24 4:15 PM, Yi Liu wrote:
> > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > > index 40dd439307e8..1e5e9249c93f 100644
> > > > --- a/include/linux/iommu.h
> > > > +++ b/include/linux/iommu.h
> > > > @@ -631,7 +631,7 @@ struct iommu_ops {
> > > >    struct iommu_domain_ops {
> > > >    	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> > > >    	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
> > > > -			     ioasid_t pasid);
> > > > +			     ioasid_t pasid, struct iommu_domain *old);
> > > Is it possible to add another op to replace a domain for pasid? For
> > > example,
> > > 
> > > 	int (*replace_dev_pasid)(domain, dev, pasid, old_domain)
> > We haven't needed that in the normal case, what would motivate it
> > here?
> 
> My understanding of the difference between set_dev_pasid and
> replace_dev_pasid is that the former assumes that there is no domain
> attached to the pasid yet, so it sets the passed domain to it. For the
> latter, it simply replaces the existing domain with a new one.
> 
> The set_dev_pasid doesn't need an old domain because it's assumed that
> the existing domain is NULL. The replace_dev_pasid could have an
> existing domain as its input.

I would just pass in the NULL domain for set than make another
op. iommu drivers should be exactly the same implementation for both

> Replace also implies an atomic switch between different domains. This
> makes it subtly different from a set operation.

Well, not necessarily hitless, but at least old/new/blocked - not
something corrupt.

Jason

Re: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Baolu Lu]

On 4/16/24 5:21 PM, Yi Liu wrote:
> 
> n 2024/4/15 14:04, Baolu Lu wrote:
>> On 4/12/24 4:15 PM, Yi Liu wrote:
>>> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
>>> is fine, but no need to go further as nothing to be cleanup and also it
>>> may hit unknown issue.
>>
>> If "... it should be a problem of caller ...", then the check and WARN()
>> should be added in the caller instead of individual drivers.
>>
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/intel/iommu.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index df49aed3df5e..fff7dea012a7 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -4614,8 +4614,9 @@ static void intel_iommu_remove_dev_pasid(struct 
>>> device *dev, ioasid_t pasid,
>>>               break;
>>>           }
>>>       }
>>> -    WARN_ON_ONCE(!dev_pasid);
>>>       spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>> +    if (WARN_ON_ONCE(!dev_pasid))
>>> +        return;
>>
>> The iommu core calls remove_dev_pasid op to tear down the translation on
>> a pasid and park it in a BLOCKED state. Since this is a must-be-
>> successful callback, it makes no sense to return before tearing down the
>> pasid table entry.
> 
> but if no dev_pasid is found, does it mean there is no pasid table entry
> to be destroyed? That's why I think it deserves a warn, but no need to
> continue.

The pasid table is allocated in the iommu probe path, hence the entry is
*always* there. Teardown a pasid translation just means zeroing out all
fields of the entry.

> 
>>
>>  From the Intel iommu driver's perspective, the pasid devices have
>> already been tracked in the core, hence the dev_pasid is a duplicate and
>> will be removed later, so don't use it for other purposes.
> 
> 
> good to know it. But for the current code, if we continue, it would hit
> call trace in the end in the intel_iommu_debugfs_remove_dev_pasid().

The debugfs interface should be designed to be self-contained. That
means, even if one passes a NULL pointer to
intel_iommu_debugfs_remove_dev_pasid(), it should handle it gracefully.

Best regards,
baolu

Re: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Yi Liu]

On 2024/4/17 10:30, Baolu Lu wrote:
> On 4/16/24 5:21 PM, Yi Liu wrote:
>>
>> n 2024/4/15 14:04, Baolu Lu wrote:
>>> On 4/12/24 4:15 PM, Yi Liu wrote:
>>>> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
>>>> is fine, but no need to go further as nothing to be cleanup and also it
>>>> may hit unknown issue.
>>>
>>> If "... it should be a problem of caller ...", then the check and WARN()
>>> should be added in the caller instead of individual drivers.
>>>
>>>>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> ---
>>>>   drivers/iommu/intel/iommu.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index df49aed3df5e..fff7dea012a7 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -4614,8 +4614,9 @@ static void intel_iommu_remove_dev_pasid(struct 
>>>> device *dev, ioasid_t pasid,
>>>>               break;
>>>>           }
>>>>       }
>>>> -    WARN_ON_ONCE(!dev_pasid);
>>>>       spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>>> +    if (WARN_ON_ONCE(!dev_pasid))
>>>> +        return;
>>>
>>> The iommu core calls remove_dev_pasid op to tear down the translation on
>>> a pasid and park it in a BLOCKED state. Since this is a must-be-
>>> successful callback, it makes no sense to return before tearing down the
>>> pasid table entry.
>>
>> but if no dev_pasid is found, does it mean there is no pasid table entry
>> to be destroyed? That's why I think it deserves a warn, but no need to
>> continue.
> 
> The pasid table is allocated in the iommu probe path, hence the entry is
> *always* there. Teardown a pasid translation just means zeroing out all
> fields of the entry.

aha, I didn't make it clear. It should be no present pasid entry if no
dev_pasid. Anyhow, I noticed intel_pasid_tear_down_entry() checks present
first before going ahead. So keep going is ok to me now.

>>
>>>
>>>  From the Intel iommu driver's perspective, the pasid devices have
>>> already been tracked in the core, hence the dev_pasid is a duplicate and
>>> will be removed later, so don't use it for other purposes.
>>
>>
>> good to know it. But for the current code, if we continue, it would hit
>> call trace in the end in the intel_iommu_debugfs_remove_dev_pasid().
> 
> The debugfs interface should be designed to be self-contained. That
> means, even if one passes a NULL pointer to
> intel_iommu_debugfs_remove_dev_pasid(), it should handle it gracefully.

TBH. I don't know if it is necessary to make every internal helpers
self-contained. It looks to be more readable to avoid unnecessary
function calls in the caller side. :)

-- 
Regards,
Yi Liu

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

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, April 12, 2024 4:15 PM
>
> @@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct
> iommu_domain *domain,
>  err_revert:
>  	last_gdev = device;
>  	for_each_group_device(group, device) {
> -		const struct iommu_ops *ops = dev_iommu_ops(device-
> >dev);
> +		/*
> +		 * If no old domain, just undo all the devices/pasid that
> +		 * have attached to the new domain.
> +		 */
> +		if (!old) {
> +			const struct iommu_ops *ops =
> +						dev_iommu_ops(device-
> >dev);
> +
> +			if (device == last_gdev)
> +				break;
> +			ops = dev_iommu_ops(device->dev);

'ops' is already assigned

> +			ops->remove_dev_pasid(device->dev, pasid, domain);
> +			continue;
> +		}
> 
> -		if (device == last_gdev)
> +		/*
> +		 * Rollback the devices/pasid that have attached to the new
> +		 * domain. And it is a driver bug to fail attaching with a
> +		 * previously good domain.
> +		 */
> +		if (device == last_gdev) {
> +			WARN_ON(old->ops->set_dev_pasid(old, device-
> >dev,
> +							pasid, NULL));

do we have a clear definition that @set_dev_pasid callback should
leave the device detached (as 'NULL' indicates) or we just don't 
care the currently-attached domain at this point?

> 
> +/**
> + * iommu_replace_device_pasid - replace the domain that a pasid is
> attached to
> + * @domain: new IOMMU domain to replace with
> + * @dev: the physical device
> + * @pasid: pasid that will be attached to the new domain
> + *
> + * This API allows the pasid to switch domains. Return 0 on success, or an
> + * error. The pasid will roll back to use the old domain if failure. The
> + * caller could call iommu_detach_device_pasid() before free the old
> domain
> + * in order to avoid use-after-free case.

I didn't get what the last sentence tries to convey. Do you mean that
the old domain cannot be freed even after the replace operation has
been completed successfully? why does it require a detach before
the free?

> + */
> +int iommu_replace_device_pasid(struct iommu_domain *domain,
> +			       struct device *dev, ioasid_t pasid)
> +{
> +	/* Caller must be a probed driver on dev */
> +	struct iommu_group *group = dev->iommu_group;
> +	void *curr;
> +	int ret;
> +
> +	if (!domain)
> +		return -EINVAL;

this check can be skipped. Accessing a null pointer will hit
a call trace already.

> +
> +	if (!domain->ops->set_dev_pasid)
> +		return -EOPNOTSUPP;
> +
> +	if (!group)
> +		return -ENODEV;
> +
> +	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain-
> >owner)
> +		return -EINVAL;

and check it's not IOMMU_NO_PASID

> +
> +	mutex_lock(&group->mutex);
> +	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
> +	if (!curr) {
> +		xa_erase(&group->pasid_array, pasid);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	ret = xa_err(curr);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (curr == domain)
> +		goto out_unlock;

emmm then 'ret' is used uninitialized here.

> +
> +	ret = __iommu_set_group_pasid(domain, group, pasid, curr);
> +	if (ret)
> +		WARN_ON(xa_err(xa_store(&group->pasid_array, pasid,
> +					curr, GFP_KERNEL)));

split the line. WARN_ON() as long as the return value doesn't match 
'domain'.

> +out_unlock:
> +	mutex_unlock(&group->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid,
> IOMMUFD_INTERNAL);
> +
>  /*
>   * iommu_detach_device_pasid() - Detach the domain from pasid of device
>   * @domain: the iommu domain.
> --
> 2.34.1

RE: [PATCH v2 05/12] iommu: Allow iommu driver to populate the max_pasids

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, April 15, 2024 1:42 PM
> 
> On 4/12/24 4:15 PM, Yi Liu wrote:
> > Today, the iommu layer gets the max_pasids by pci_max_pasids() or
> reading
> > the "pasid-num-bits" property. This requires the non-PCI devices to have a
> > "pasid-num-bits" property. Like the mock device used in iommufd selftest,
> > otherwise the max_pasids check would fail in iommu layer.
> >
> > While there is an alternative, the iommu layer can allow the iommu driver
> > to set the max_pasids in its probe_device() callback and populate it. This
> > is simpler and has no impact on the existing cases.
> >
> > Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
> > Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> > ---
> >   drivers/iommu/iommu.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> The code does not appear to match the commit message here.
> 
> The code in change is a refactoring by folding the max_pasid assignment
> into its helper. However, the commit message suggests a desire to expose
> some kind of kAPI for device drivers.
> 

it's not about exposing a new kAPI. Instead it allows the driver to
manually set dev->iommu->max_pasids before calling
iommu_init_device(). kind of another contract to convey the
max pasid.

But as how you are confused, I prefer to defining a "pasid-num-bits"
property in the mock driver. It's easier to understand.

RE: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, April 12, 2024 4:15 PM
> 
> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
> is fine, but no need to go further as nothing to be cleanup and also it
> may hit unknown issue.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

with your discussions let's remove it from this series.

If there is anything to further cleanup it can be done alone.

RE: [PATCH v2 11/12] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, April 12, 2024 4:15 PM
>
> @@ -4646,6 +4646,10 @@ static int intel_iommu_set_dev_pasid(struct
> iommu_domain *domain,
>  	if (context_copied(iommu, info->bus, info->devfn))
>  		return -EBUSY;
> 
> +	/* Block old translation */
> +	if (old)
> +		intel_iommu_remove_dev_pasid(dev, pasid, old);
> +

let's talk about one scenario.

pasid#100 is currently attached to domain#1

the user requests to replace pasid#100 to domain#2, which enables
dirty tracking.

this function will return error before blocking the old translation:

	if (domain->dirty_ops)
		return -EINVAL;

pasid#100 is still attached to domain#1.

then the error unwinding in iommu core tries to attach pasid#100
back to domain#1:

		/*
		 * Rollback the devices/pasid that have attached to the new
		 * domain. And it is a driver bug to fail attaching with a
		 * previously good domain.
		 */
		if (device == last_gdev) {
			WARN_ON(old->ops->set_dev_pasid(old, device->dev,
							pasid, NULL));
 			break;
		}

but intel iommu driver doesn't expect duplicated attaches e.g.
domain_attach_iommu() will increase the refcnt of the existing
DID but later the user will only call detach once.

do we want to force iommu driver to block translation upon 
any error in @set_dev_pasid (then rely on the core to recover
it correctly) or tolerate duplicated attaches?

Thanks
Kevin

RE: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, April 12, 2024 4:15 PM
> 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> This allows the upper layers to set a nested type domain to a PASID of a
> device if the PASID feature is supported by the IOMMU hardware.
> 
> The set_dev_pasid callback for non-nested domain has already be there, so
> this only needs to add it for nested domains. Note that the S2 domain with
> dirty tracking capability is not supported yet as no user for now.

S2 domain does support dirty tracking. Do you mean the specific
check in intel_iommu_set_dev_pasid() i.e. pasid-granular dirty
tracking is not supported yet?

> +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
> +				      struct device *dev, ioasid_t pasid,
> +				      struct iommu_domain *old)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> +	struct intel_iommu *iommu = info->iommu;
> +
> +	if (iommu->agaw < dmar_domain->s2_domain->agaw)
> +		return -EINVAL;
> +

this check is covered by prepare_domain_attach_device() already.

Re: [PATCH v2 11/12] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

On 2024/4/17 17:19, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, April 12, 2024 4:15 PM
>>
>> @@ -4646,6 +4646,10 @@ static int intel_iommu_set_dev_pasid(struct
>> iommu_domain *domain,
>>   	if (context_copied(iommu, info->bus, info->devfn))
>>   		return -EBUSY;
>>
>> +	/* Block old translation */
>> +	if (old)
>> +		intel_iommu_remove_dev_pasid(dev, pasid, old);
>> +
> 
> let's talk about one scenario.
> 
> pasid#100 is currently attached to domain#1
> 
> the user requests to replace pasid#100 to domain#2, which enables
> dirty tracking.
> 
> this function will return error before blocking the old translation:
> 
> 	if (domain->dirty_ops)
> 		return -EINVAL;

yep. From what I learned from Joao, this check was added due to no actual
usage before SIOV. If only considering vSVM, the domain attached to pasids
won't have the dirty_ops. So I was planning to remove this check when
coming to SIOV series. But let me know if it's already the time to remove
it.

> 
> pasid#100 is still attached to domain#1.
> 
> then the error unwinding in iommu core tries to attach pasid#100
> back to domain#1:
> 
> 		/*
> 		 * Rollback the devices/pasid that have attached to the new
> 		 * domain. And it is a driver bug to fail attaching with a
> 		 * previously good domain.
> 		 */
> 		if (device == last_gdev) {
> 			WARN_ON(old->ops->set_dev_pasid(old, device->dev,
> 							pasid, NULL));
>   			break;
> 		}
> 
> but intel iommu driver doesn't expect duplicated attaches e.g.
> domain_attach_iommu() will increase the refcnt of the existing
> DID but later the user will only call detach once.

good catch!!! This is a problem...Even if the domain->dirty_ops check is
removed, the set_dev_pasid() callback can fail for other reasons.

> do we want to force iommu driver to block translation upon
> any error in @set_dev_pasid (then rely on the core to recover
> it correctly) or tolerate duplicated attaches?

The second one seems better? The first option looks a too heavy especially
considering the atomic requirement in certain scenarios.

-- 
Regards,
Yi Liu

Re: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

[Yi Liu]

On 2024/4/17 17:03, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, April 12, 2024 4:15 PM
>>
>> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
>> is fine, but no need to go further as nothing to be cleanup and also it
>> may hit unknown issue.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> with your discussions let's remove it from this series.
> 
> If there is anything to further cleanup it can be done alone.

ok. Would drop it from this series.

-- 
Regards,
Yi Liu

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

[Jason Gunthorpe]

On Wed, Apr 17, 2024 at 08:44:11AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Friday, April 12, 2024 4:15 PM
> >
> > @@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct
> > iommu_domain *domain,
> >  err_revert:
> >  	last_gdev = device;
> >  	for_each_group_device(group, device) {
> > -		const struct iommu_ops *ops = dev_iommu_ops(device-
> > >dev);
> > +		/*
> > +		 * If no old domain, just undo all the devices/pasid that
> > +		 * have attached to the new domain.
> > +		 */
> > +		if (!old) {
> > +			const struct iommu_ops *ops =
> > +						dev_iommu_ops(device-
> > >dev);
> > +
> > +			if (device == last_gdev)
> > +				break;
> > +			ops = dev_iommu_ops(device->dev);
> 
> 'ops' is already assigned
> 
> > +			ops->remove_dev_pasid(device->dev, pasid, domain);
> > +			continue;
> > +		}
> > 
> > -		if (device == last_gdev)
> > +		/*
> > +		 * Rollback the devices/pasid that have attached to the new
> > +		 * domain. And it is a driver bug to fail attaching with a
> > +		 * previously good domain.
> > +		 */
> > +		if (device == last_gdev) {
> > +			WARN_ON(old->ops->set_dev_pasid(old, device-
> > >dev,
> > +							pasid, NULL));
> 
> do we have a clear definition that @set_dev_pasid callback should
> leave the device detached (as 'NULL' indicates) or we just don't 
> care the currently-attached domain at this point?

If set_dev_pasid fails I would expect to to have done nothing so the
failing device should be left in the old config and we should just not
call it at all.

The RID path is wonky here because so many drivers don't do that, so
we poke them again to hopefully get it right. I think for PASID we
should try to make the drivers work properly from the start. Failure
means no change.

I would summarize this in a comment..

Jason

Re: [PATCH v2 11/12] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Jason Gunthorpe]

On Wed, Apr 17, 2024 at 05:35:47PM +0800, Yi Liu wrote:
> > do we want to force iommu driver to block translation upon
> > any error in @set_dev_pasid (then rely on the core to recover
> > it correctly) or tolerate duplicated attaches?
> 
> The second one seems better? The first option looks a too heavy especially
> considering the atomic requirement in certain scenarios.

On set_dev_pasid() failure the driver should make no change to the
translation.

It is the only sane answer.

Jason 

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

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, April 17, 2024 8:17 PM
> 
> On Wed, Apr 17, 2024 at 08:44:11AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Friday, April 12, 2024 4:15 PM
> > >
> > > -		if (device == last_gdev)
> > > +		/*
> > > +		 * Rollback the devices/pasid that have attached to the new
> > > +		 * domain. And it is a driver bug to fail attaching with a
> > > +		 * previously good domain.
> > > +		 */
> > > +		if (device == last_gdev) {
> > > +			WARN_ON(old->ops->set_dev_pasid(old, device-
> > > >dev,
> > > +							pasid, NULL));
> >
> > do we have a clear definition that @set_dev_pasid callback should
> > leave the device detached (as 'NULL' indicates) or we just don't
> > care the currently-attached domain at this point?
> 
> If set_dev_pasid fails I would expect to to have done nothing so the
> failing device should be left in the old config and we should just not
> call it at all.
> 
> The RID path is wonky here because so many drivers don't do that, so
> we poke them again to hopefully get it right. I think for PASID we
> should try to make the drivers work properly from the start. Failure
> means no change.
> 
> I would summarize this in a comment..

Ah yes, that's the simple sane way to do. 😊

Re: [PATCH v2 05/12] iommu: Allow iommu driver to populate the max_pasids

[Yi Liu]

On 2024/4/17 16:49, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Monday, April 15, 2024 1:42 PM
>>
>> On 4/12/24 4:15 PM, Yi Liu wrote:
>>> Today, the iommu layer gets the max_pasids by pci_max_pasids() or
>> reading
>>> the "pasid-num-bits" property. This requires the non-PCI devices to have a
>>> "pasid-num-bits" property. Like the mock device used in iommufd selftest,
>>> otherwise the max_pasids check would fail in iommu layer.
>>>
>>> While there is an alternative, the iommu layer can allow the iommu driver
>>> to set the max_pasids in its probe_device() callback and populate it. This
>>> is simpler and has no impact on the existing cases.
>>>
>>> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>> ---
>>>    drivers/iommu/iommu.c | 9 +++++----
>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> The code does not appear to match the commit message here.
>>
>> The code in change is a refactoring by folding the max_pasid assignment
>> into its helper. However, the commit message suggests a desire to expose
>> some kind of kAPI for device drivers.
>>
> 
> it's not about exposing a new kAPI. Instead it allows the driver to
> manually set dev->iommu->max_pasids before calling
> iommu_init_device(). kind of another contract to convey the
> max pasid.
> 
> But as how you are confused, I prefer to defining a "pasid-num-bits"
> property in the mock driver. It's easier to understand.

@Jason, how about your thought? :) Adding a "pasid-num-bits" may be
more straightforward.

-- 
Regards,
Yi Liu

Re: [PATCH v2 05/12] iommu: Allow iommu driver to populate the max_pasids

[Jason Gunthorpe]

On Sat, Apr 20, 2024 at 01:45:57PM +0800, Yi Liu wrote:
> On 2024/4/17 16:49, Tian, Kevin wrote:
> > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > Sent: Monday, April 15, 2024 1:42 PM
> > > 
> > > On 4/12/24 4:15 PM, Yi Liu wrote:
> > > > Today, the iommu layer gets the max_pasids by pci_max_pasids() or
> > > reading
> > > > the "pasid-num-bits" property. This requires the non-PCI devices to have a
> > > > "pasid-num-bits" property. Like the mock device used in iommufd selftest,
> > > > otherwise the max_pasids check would fail in iommu layer.
> > > > 
> > > > While there is an alternative, the iommu layer can allow the iommu driver
> > > > to set the max_pasids in its probe_device() callback and populate it. This
> > > > is simpler and has no impact on the existing cases.
> > > > 
> > > > Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
> > > > Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> > > > ---
> > > >    drivers/iommu/iommu.c | 9 +++++----
> > > >    1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > The code does not appear to match the commit message here.
> > > 
> > > The code in change is a refactoring by folding the max_pasid assignment
> > > into its helper. However, the commit message suggests a desire to expose
> > > some kind of kAPI for device drivers.
> > > 
> > 
> > it's not about exposing a new kAPI. Instead it allows the driver to
> > manually set dev->iommu->max_pasids before calling
> > iommu_init_device(). kind of another contract to convey the
> > max pasid.
> > 
> > But as how you are confused, I prefer to defining a "pasid-num-bits"
> > property in the mock driver. It's easier to understand.
> 
> @Jason, how about your thought? :) Adding a "pasid-num-bits" may be
> more straightforward.

Sure, just make sure it doesn't leak memory...

Jason 

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

[Jason Gunthorpe]

On Fri, Apr 12, 2024 at 01:15:06AM -0700, Yi Liu wrote:

> -		if (device == last_gdev)
> +		/*
> +		 * Rollback the devices/pasid that have attached to the new
> +		 * domain. And it is a driver bug to fail attaching with a
> +		 * previously good domain.
> +		 */
> +		if (device == last_gdev) {
> +			WARN_ON(old->ops->set_dev_pasid(old, device->dev,
> +							pasid, NULL));
>  			break;
> -		ops->remove_dev_pasid(device->dev, pasid, domain);

Suggest writing this as 

if (WARN_ON(old->ops->set_dev_pasid(old, device->dev, pasid, curr)))
    ops->remove_dev_pasid(device->dev, pasid, domain);

As we may as well try to bring the system back to some kind of safe
state before we continue on.

Also NULL doesn't seem right, if we here then the new domain was
attached successfully and we are put it back to old.

> +	mutex_lock(&group->mutex);
> +	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
> +	if (!curr) {
> +		xa_erase(&group->pasid_array, pasid);

A comment here about order is likely a good idea..

At this point the pasid_array and the translation are not matched. If
we get a PRI at this instant it will deliver to the new domain until
the translation is updated.

There is nothing to do about this race, but lets note it and say the
concurrent PRI path will eventually become consistent and there is no
harm in directing PRI to the wrong domain.

Let's also check that receiving a PRI on a domain that is not PRI
capable doesn't explode in case someone uses replace to change from a
PRI to non PRI domain.

Jason

Re: [PATCH v2 04/12] iommufd: Support attach/replace hwpt per pasid

[Jason Gunthorpe]

On Fri, Apr 12, 2024 at 01:15:08AM -0700, Yi Liu wrote:

> @@ -413,9 +414,23 @@ struct attach_data {
>  		struct iommufd_hw_pagetable *(*attach_fn)(
>  				struct iommufd_device *idev,
>  				struct iommufd_hw_pagetable *hwpt);
> +		struct iommufd_hw_pagetable *(*pasid_attach_fn)(
> +				struct iommufd_device *idev, u32 pasid,
> +				struct iommufd_hw_pagetable *hwpt);
>  	};
> +	u32 pasid;
>  };

I suggest you change this around to pass the attach_data to the
attach_fn and just pass the pasid through that way instead of having
to function pointers.

Jason

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

[Yi Liu]

On 2024/4/29 21:55, Jason Gunthorpe wrote:
> On Fri, Apr 12, 2024 at 01:15:06AM -0700, Yi Liu wrote:
> 
>> -		if (device == last_gdev)
>> +		/*
>> +		 * Rollback the devices/pasid that have attached to the new
>> +		 * domain. And it is a driver bug to fail attaching with a
>> +		 * previously good domain.
>> +		 */
>> +		if (device == last_gdev) {
>> +			WARN_ON(old->ops->set_dev_pasid(old, device->dev,
>> +							pasid, NULL));
>>   			break;
>> -		ops->remove_dev_pasid(device->dev, pasid, domain);
> 
> Suggest writing this as
> 
> if (WARN_ON(old->ops->set_dev_pasid(old, device->dev, pasid, curr)))
>      ops->remove_dev_pasid(device->dev, pasid, domain);
> 
> As we may as well try to bring the system back to some kind of safe
> state before we continue on.
> 
> Also NULL doesn't seem right, if we here then the new domain was
> attached successfully and we are put it back to old.

ok, and given your another remark [1], there is no more need to do rollback
for the last_gdev, just need to break the loop when comes to the last_gdev.

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

>> +	mutex_lock(&group->mutex);
>> +	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
>> +	if (!curr) {
>> +		xa_erase(&group->pasid_array, pasid);
> 
> A comment here about order is likely a good idea..
> 
> At this point the pasid_array and the translation are not matched. If
> we get a PRI at this instant it will deliver to the new domain until
> the translation is updated.

yes.

> There is nothing to do about this race, but lets note it and say the
> concurrent PRI path will eventually become consistent and there is no
> harm in directing PRI to the wrong domain.

If the old and new domain points to the same address space, it is fine.
How about they point to different address spaces? Delivering the PRI to
new domain seems problematic. Or, do we allow such domain replacement
when there is still ongoing DMA?

> Let's also check that receiving a PRI on a domain that is not PRI
> capable doesn't explode in case someone uses replace to change from a
> PRI to non PRI domain.

Just need to refuse the receiving PRI, is it? BTW. Should the PRI cap
be disabled in the devices side and the translation structure (e.g.
PRI enable bit in pasid entry) when the replacement is done?

-- 
Regards,
Yi Liu

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/4/17 17:25, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, April 12, 2024 4:15 PM
>>
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>
>> This allows the upper layers to set a nested type domain to a PASID of a
>> device if the PASID feature is supported by the IOMMU hardware.
>>
>> The set_dev_pasid callback for non-nested domain has already be there, so
>> this only needs to add it for nested domains. Note that the S2 domain with
>> dirty tracking capability is not supported yet as no user for now.
> 
> S2 domain does support dirty tracking. Do you mean the specific
> check in intel_iommu_set_dev_pasid() i.e. pasid-granular dirty
> tracking is not supported yet?

yes. We may remove this check when real usage comes. e.g. SIOV.

>> +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
>> +				      struct device *dev, ioasid_t pasid,
>> +				      struct iommu_domain *old)
>> +{
>> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
>> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> +	struct intel_iommu *iommu = info->iommu;
>> +
>> +	if (iommu->agaw < dmar_domain->s2_domain->agaw)
>> +		return -EINVAL;
>> +
> 
> this check is covered by prepare_domain_attach_device() already.

This was added to avoid modifying the s2_domain's agaw. I'm fine to remove
it personally as the existing attach path also needs to update domain's
agaw per device attachment. @Baolu, how about your opinion?

-- 
Regards,
Yi Liu

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

[Jason Gunthorpe]

On Tue, Apr 30, 2024 at 01:00:57PM +0800, Yi Liu wrote:

> > There is nothing to do about this race, but lets note it and say the
> > concurrent PRI path will eventually become consistent and there is no
> > harm in directing PRI to the wrong domain.
> 
> If the old and new domain points to the same address space, it is fine.
> How about they point to different address spaces? Delivering the PRI to
> new domain seems problematic. Or, do we allow such domain replacement
> when there is still ongoing DMA?

New PRI could happen an instant later and hit the new domain, or an
instant before and hit the old domain. It is fine

> > Let's also check that receiving a PRI on a domain that is not PRI
> > capable doesn't explode in case someone uses replace to change from a
> > PRI to non PRI domain.
> 
> Just need to refuse the receiving PRI, is it?

Yes

> BTW. Should the PRI cap
> be disabled in the devices side and the translation structure (e.g.
> PRI enable bit in pasid entry) when the replacement is done?

Yes, after domain attachment completes

Jason

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Baolu Lu]

On 2024/4/30 17:19, Yi Liu wrote:
> On 2024/4/17 17:25, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Friday, April 12, 2024 4:15 PM
>>>
>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>
>>> This allows the upper layers to set a nested type domain to a PASID of a
>>> device if the PASID feature is supported by the IOMMU hardware.
>>>
>>> The set_dev_pasid callback for non-nested domain has already be 
>>> there, so
>>> this only needs to add it for nested domains. Note that the S2 domain 
>>> with
>>> dirty tracking capability is not supported yet as no user for now.
>>
>> S2 domain does support dirty tracking. Do you mean the specific
>> check in intel_iommu_set_dev_pasid() i.e. pasid-granular dirty
>> tracking is not supported yet?
> 
> yes. We may remove this check when real usage comes. e.g. SIOV.
> 
>>> +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
>>> +                      struct device *dev, ioasid_t pasid,
>>> +                      struct iommu_domain *old)
>>> +{
>>> +    struct device_domain_info *info = dev_iommu_priv_get(dev);
>>> +    struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>>> +    struct intel_iommu *iommu = info->iommu;
>>> +
>>> +    if (iommu->agaw < dmar_domain->s2_domain->agaw)
>>> +        return -EINVAL;
>>> +
>>
>> this check is covered by prepare_domain_attach_device() already.
> 
> This was added to avoid modifying the s2_domain's agaw. I'm fine to remove
> it personally as the existing attach path also needs to update domain's
> agaw per device attachment. @Baolu, how about your opinion?

We still need something to do before we can safely remove this check.
All the domain allocation interfaces should eventually have the device
pointer as the input, and all domain attributions could be initialized
during domain allocation. In the attach paths, it should return -EINVAL
directly if the domain is not compatible with the iommu for the device.

Best regards,
baolu

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Jason Gunthorpe]

On Mon, May 06, 2024 at 03:42:21PM +0800, Baolu Lu wrote:
> On 2024/4/30 17:19, Yi Liu wrote:
> > On 2024/4/17 17:25, Tian, Kevin wrote:
> > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > Sent: Friday, April 12, 2024 4:15 PM
> > > > 
> > > > From: Lu Baolu <baolu.lu@linux.intel.com>
> > > > 
> > > > This allows the upper layers to set a nested type domain to a PASID of a
> > > > device if the PASID feature is supported by the IOMMU hardware.
> > > > 
> > > > The set_dev_pasid callback for non-nested domain has already be
> > > > there, so
> > > > this only needs to add it for nested domains. Note that the S2
> > > > domain with
> > > > dirty tracking capability is not supported yet as no user for now.
> > > 
> > > S2 domain does support dirty tracking. Do you mean the specific
> > > check in intel_iommu_set_dev_pasid() i.e. pasid-granular dirty
> > > tracking is not supported yet?
> > 
> > yes. We may remove this check when real usage comes. e.g. SIOV.
> > 
> > > > +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
> > > > +                      struct device *dev, ioasid_t pasid,
> > > > +                      struct iommu_domain *old)
> > > > +{
> > > > +    struct device_domain_info *info = dev_iommu_priv_get(dev);
> > > > +    struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > > > +    struct intel_iommu *iommu = info->iommu;
> > > > +
> > > > +    if (iommu->agaw < dmar_domain->s2_domain->agaw)
> > > > +        return -EINVAL;
> > > > +
> > > 
> > > this check is covered by prepare_domain_attach_device() already.
> > 
> > This was added to avoid modifying the s2_domain's agaw. I'm fine to remove
> > it personally as the existing attach path also needs to update domain's
> > agaw per device attachment. @Baolu, how about your opinion?
> 
> We still need something to do before we can safely remove this check.
> All the domain allocation interfaces should eventually have the device
> pointer as the input, and all domain attributions could be initialized
> during domain allocation. In the attach paths, it should return -EINVAL
> directly if the domain is not compatible with the iommu for the device.

Yes, and this is already true for PASID.

I feel we could reasonably insist that domanis used with PASID are
allocated with a non-NULL dev.

If so it means we need to fixup the domain allocation in iommufd as
part of the pasid series, and Intel will have to implement
alloc_domain_paging().

Jason

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/5/6 21:36, Jason Gunthorpe wrote:
> On Mon, May 06, 2024 at 03:42:21PM +0800, Baolu Lu wrote:
>> On 2024/4/30 17:19, Yi Liu wrote:
>>> On 2024/4/17 17:25, Tian, Kevin wrote:
>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>> Sent: Friday, April 12, 2024 4:15 PM
>>>>>
>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>
>>>>> This allows the upper layers to set a nested type domain to a PASID of a
>>>>> device if the PASID feature is supported by the IOMMU hardware.
>>>>>
>>>>> The set_dev_pasid callback for non-nested domain has already be
>>>>> there, so
>>>>> this only needs to add it for nested domains. Note that the S2
>>>>> domain with
>>>>> dirty tracking capability is not supported yet as no user for now.
>>>>
>>>> S2 domain does support dirty tracking. Do you mean the specific
>>>> check in intel_iommu_set_dev_pasid() i.e. pasid-granular dirty
>>>> tracking is not supported yet?
>>>
>>> yes. We may remove this check when real usage comes. e.g. SIOV.
>>>
>>>>> +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
>>>>> +                      struct device *dev, ioasid_t pasid,
>>>>> +                      struct iommu_domain *old)
>>>>> +{
>>>>> +    struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>>> +    struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>>>>> +    struct intel_iommu *iommu = info->iommu;
>>>>> +
>>>>> +    if (iommu->agaw < dmar_domain->s2_domain->agaw)
>>>>> +        return -EINVAL;
>>>>> +
>>>>
>>>> this check is covered by prepare_domain_attach_device() already.
>>>
>>> This was added to avoid modifying the s2_domain's agaw. I'm fine to remove
>>> it personally as the existing attach path also needs to update domain's
>>> agaw per device attachment. @Baolu, how about your opinion?
>>
>> We still need something to do before we can safely remove this check.
>> All the domain allocation interfaces should eventually have the device
>> pointer as the input, and all domain attributions could be initialized
>> during domain allocation. In the attach paths, it should return -EINVAL
>> directly if the domain is not compatible with the iommu for the device.
> 
> Yes, and this is already true for PASID.

I'm not quite get why it is already true for PASID. I think Baolu's remark
is general to domains attached to either RID or PASID.

> I feel we could reasonably insist that domanis used with PASID are
> allocated with a non-NULL dev.

Any special reason for this disclaim?

> 
> If so it means we need to fixup the domain allocation in iommufd as
> part of the pasid series, and Intel will have to implement
> alloc_domain_paging().

I agree implementing alloc_domain_paging() is the final solution to avoid
such dynamic modifications to domain's caps. If it's really needed for
PASID series now, I can add it in next version. :)

-- 
Regards,
Yi Liu

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Jason Gunthorpe]

On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
> > > We still need something to do before we can safely remove this check.
> > > All the domain allocation interfaces should eventually have the device
> > > pointer as the input, and all domain attributions could be initialized
> > > during domain allocation. In the attach paths, it should return -EINVAL
> > > directly if the domain is not compatible with the iommu for the device.
> > 
> > Yes, and this is already true for PASID.
> 
> I'm not quite get why it is already true for PASID. I think Baolu's remark
> is general to domains attached to either RID or PASID.
> 
> > I feel we could reasonably insist that domanis used with PASID are
> > allocated with a non-NULL dev.
> 
> Any special reason for this disclaim?

If it makes the driver easier, why not?

PASID is special since PASID is barely used, we could insist that
new PASID users also use the new domian_alloc API.

> I agree implementing alloc_domain_paging() is the final solution to avoid
> such dynamic modifications to domain's caps. If it's really needed for
> PASID series now, I can add it in next version. :)

Well, if it is needed. If you can do this some other way that is
reasonable then sure

Jason
 

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/5/7 23:18, Jason Gunthorpe wrote:
> On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
>>>> We still need something to do before we can safely remove this check.
>>>> All the domain allocation interfaces should eventually have the device
>>>> pointer as the input, and all domain attributions could be initialized
>>>> during domain allocation. In the attach paths, it should return -EINVAL
>>>> directly if the domain is not compatible with the iommu for the device.
>>>
>>> Yes, and this is already true for PASID.
>>
>> I'm not quite get why it is already true for PASID. I think Baolu's remark
>> is general to domains attached to either RID or PASID.
>>
>>> I feel we could reasonably insist that domanis used with PASID are
>>> allocated with a non-NULL dev.
>>
>> Any special reason for this disclaim?
> 
> If it makes the driver easier, why not?

yep.

> PASID is special since PASID is barely used, we could insist that
> new PASID users also use the new domian_alloc API.

Ok. I have one doubt on how to make it in iommufd core. Shall the iommufd
core call ops->domain_alloc_paging() directly or still call
ops->domain_alloc_user() while ops->domain_alloc_user() flows into the
paging domain allocation with non-null dev?

> 
>> I agree implementing alloc_domain_paging() is the final solution to avoid
>> such dynamic modifications to domain's caps. If it's really needed for
>> PASID series now, I can add it in next version. :)
> 
> Well, if it is needed. If you can do this some other way that is
> reasonable then sure

At first, I'd like to keep this agaw check here and remove it after
implementing the ops->domain_alloc_paging() and retire domain_alloc op
in intel iommu driver side. I have such thoughts as the RID and PASID
attach path have the same concern w.r.t. agaw and other caps :)

-- 
Regards,
Yi Liu

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Jason Gunthorpe]

On Wed, May 08, 2024 at 02:10:05PM +0800, Yi Liu wrote:
> On 2024/5/7 23:18, Jason Gunthorpe wrote:
> > On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
> > > > > We still need something to do before we can safely remove this check.
> > > > > All the domain allocation interfaces should eventually have the device
> > > > > pointer as the input, and all domain attributions could be initialized
> > > > > during domain allocation. In the attach paths, it should return -EINVAL
> > > > > directly if the domain is not compatible with the iommu for the device.
> > > > 
> > > > Yes, and this is already true for PASID.
> > > 
> > > I'm not quite get why it is already true for PASID. I think Baolu's remark
> > > is general to domains attached to either RID or PASID.
> > > 
> > > > I feel we could reasonably insist that domanis used with PASID are
> > > > allocated with a non-NULL dev.
> > > 
> > > Any special reason for this disclaim?
> > 
> > If it makes the driver easier, why not?
> 
> yep.
> 
> > PASID is special since PASID is barely used, we could insist that
> > new PASID users also use the new domian_alloc API.
> 
> Ok. I have one doubt on how to make it in iommufd core. Shall the iommufd
> core call ops->domain_alloc_paging() directly or still call
> ops->domain_alloc_user() while ops->domain_alloc_user() flows into the
> paging domain allocation with non-null dev?

I mostly figured we'd need a new iommu_domain_alloc_dev() sort of
thing? VFIO should be changed over too.

Jason

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/5/8 20:25, Jason Gunthorpe wrote:
> On Wed, May 08, 2024 at 02:10:05PM +0800, Yi Liu wrote:
>> On 2024/5/7 23:18, Jason Gunthorpe wrote:
>>> On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
>>>>>> We still need something to do before we can safely remove this check.
>>>>>> All the domain allocation interfaces should eventually have the device
>>>>>> pointer as the input, and all domain attributions could be initialized
>>>>>> during domain allocation. In the attach paths, it should return -EINVAL
>>>>>> directly if the domain is not compatible with the iommu for the device.
>>>>>
>>>>> Yes, and this is already true for PASID.
>>>>
>>>> I'm not quite get why it is already true for PASID. I think Baolu's remark
>>>> is general to domains attached to either RID or PASID.
>>>>
>>>>> I feel we could reasonably insist that domanis used with PASID are
>>>>> allocated with a non-NULL dev.
>>>>
>>>> Any special reason for this disclaim?
>>>
>>> If it makes the driver easier, why not?
>>
>> yep.
>>
>>> PASID is special since PASID is barely used, we could insist that
>>> new PASID users also use the new domian_alloc API.
>>
>> Ok. I have one doubt on how to make it in iommufd core. Shall the iommufd
>> core call ops->domain_alloc_paging() directly or still call
>> ops->domain_alloc_user() while ops->domain_alloc_user() flows into the
>> paging domain allocation with non-null dev?
> 
> I mostly figured we'd need a new iommu_domain_alloc_dev() sort of
> thing? VFIO should be changed over too.

Would this new iommu-domain_alloc_dev() have flags and user_data input?
As below code snippet, the existing iommufd core uses domain_alloc_user
op to allocate the s2 domain (paging domain), and will fall back to
iommu_domain_alloc() only if the domain_alloc_user op does not exist. The
typical reason is to use domain_alloc_user op is to allocate a paging
domain with NESTED_PARENT flag. I suppose the new iommu_domain_alloc_dev()
shall allow allocating s2 domain with NESTED_PARENT as well. right?


struct iommufd_hwpt_paging *
iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
			  struct iommufd_device *idev, u32 flags,
			  bool immediate_attach,
			  const struct iommu_user_data *user_data)
{

	...
	if (ops->domain_alloc_user) {
		hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL,
						      user_data);
		if (IS_ERR(hwpt->domain)) {
			rc = PTR_ERR(hwpt->domain);
			hwpt->domain = NULL;
			goto out_abort;
		}
		hwpt->domain->owner = ops;
	} else {
		hwpt->domain = iommu_domain_alloc(idev->dev->bus);
		if (!hwpt->domain) {
			rc = -ENOMEM;
			goto out_abort;
		}
	}
	...
}

-- 
Regards,
Yi Liu

Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Jason Gunthorpe]

On Wed, May 08, 2024 at 09:26:47PM +0800, Yi Liu wrote:
> On 2024/5/8 20:25, Jason Gunthorpe wrote:
> > On Wed, May 08, 2024 at 02:10:05PM +0800, Yi Liu wrote:
> > > On 2024/5/7 23:18, Jason Gunthorpe wrote:
> > > > On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
> > > > > > > We still need something to do before we can safely remove this check.
> > > > > > > All the domain allocation interfaces should eventually have the device
> > > > > > > pointer as the input, and all domain attributions could be initialized
> > > > > > > during domain allocation. In the attach paths, it should return -EINVAL
> > > > > > > directly if the domain is not compatible with the iommu for the device.
> > > > > > 
> > > > > > Yes, and this is already true for PASID.
> > > > > 
> > > > > I'm not quite get why it is already true for PASID. I think Baolu's remark
> > > > > is general to domains attached to either RID or PASID.
> > > > > 
> > > > > > I feel we could reasonably insist that domanis used with PASID are
> > > > > > allocated with a non-NULL dev.
> > > > > 
> > > > > Any special reason for this disclaim?
> > > > 
> > > > If it makes the driver easier, why not?
> > > 
> > > yep.
> > > 
> > > > PASID is special since PASID is barely used, we could insist that
> > > > new PASID users also use the new domian_alloc API.
> > > 
> > > Ok. I have one doubt on how to make it in iommufd core. Shall the iommufd
> > > core call ops->domain_alloc_paging() directly or still call
> > > ops->domain_alloc_user() while ops->domain_alloc_user() flows into the
> > > paging domain allocation with non-null dev?
> > 
> > I mostly figured we'd need a new iommu_domain_alloc_dev() sort of
> > thing? VFIO should be changed over too.
> 
> Would this new iommu-domain_alloc_dev() have flags and user_data
> input?

No, it would be an in-kernel replacement for the existing API.

> As below code snippet, the existing iommufd core uses domain_alloc_user
> op to allocate the s2 domain (paging domain), and will fall back to
> iommu_domain_alloc() only if the domain_alloc_user op does not exist. The

Oh, right. Yeah we built it like that so that drivers would have
consistency that iommufd always uses the _user version if it exists.

> typical reason is to use domain_alloc_user op is to allocate a paging
> domain with NESTED_PARENT flag. I suppose the new iommu_domain_alloc_dev()
> shall allow allocating s2 domain with NESTED_PARENT as well. right?

No, it is just a simple replacement for iommu_domain_alloc() that does
exactly the same thing. We don't have any in-kernel use for anything
more fancy than a simple domain right now.

Jason

RE: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Liu, Yi L]

> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> On Wed, May 08, 2024 at 09:26:47PM +0800, Yi Liu wrote:
> > On 2024/5/8 20:25, Jason Gunthorpe wrote:
> > > On Wed, May 08, 2024 at 02:10:05PM +0800, Yi Liu wrote:
> > > > On 2024/5/7 23:18, Jason Gunthorpe wrote:
> > > > > On Tue, May 07, 2024 at 10:28:34AM +0800, Yi Liu wrote:
> > > > > > > > We still need something to do before we can safely remove this check.
> > > > > > > > All the domain allocation interfaces should eventually have the device
> > > > > > > > pointer as the input, and all domain attributions could be initialized
> > > > > > > > during domain allocation. In the attach paths, it should return -EINVAL
> > > > > > > > directly if the domain is not compatible with the iommu for the device.
> > > > > > >
> > > > > > > Yes, and this is already true for PASID.
> > > > > >
> > > > > > I'm not quite get why it is already true for PASID. I think Baolu's remark
> > > > > > is general to domains attached to either RID or PASID.
> > > > > >
> > > > > > > I feel we could reasonably insist that domanis used with PASID are
> > > > > > > allocated with a non-NULL dev.
> > > > > >
> > > > > > Any special reason for this disclaim?
> > > > >
> > > > > If it makes the driver easier, why not?
> > > >
> > > > yep.
> > > >
> > > > > PASID is special since PASID is barely used, we could insist that
> > > > > new PASID users also use the new domian_alloc API.
> > > >
> > > > Ok. I have one doubt on how to make it in iommufd core. Shall the iommufd
> > > > core call ops->domain_alloc_paging() directly or still call
> > > > ops->domain_alloc_user() while ops->domain_alloc_user() flows into the
> > > > paging domain allocation with non-null dev?
> > >
> > > I mostly figured we'd need a new iommu_domain_alloc_dev() sort of
> > > thing? VFIO should be changed over too.
> >
> > Would this new iommu-domain_alloc_dev() have flags and user_data
> > input?
> 
> No, it would be an in-kernel replacement for the existing API.

Ok.

> > As below code snippet, the existing iommufd core uses domain_alloc_user
> > op to allocate the s2 domain (paging domain), and will fall back to
> > iommu_domain_alloc() only if the domain_alloc_user op does not exist. The
> 
> Oh, right. Yeah we built it like that so that drivers would have
> consistency that iommufd always uses the _user version if it exists.

Yep. This means for the iommu drivers that have implemented
domain_alloc_user op, it would not call into the new iommu_domain_alloc_dev(). 
So I would need to make the intel domain_alloc_user op allocate paging domain
with non-dev as well.

> > typical reason is to use domain_alloc_user op is to allocate a paging
> > domain with NESTED_PARENT flag. I suppose the new iommu_domain_alloc_dev()
> > shall allow allocating s2 domain with NESTED_PARENT as well. right?
> 
> No, it is just a simple replacement for iommu_domain_alloc() that does
> exactly the same thing. We don't have any in-kernel use for anything
> more fancy than a simple domain right now.

Yes.

Regards,
Yi Liu