[PATCH v3 0/6] iommu: Extend changing default domain to normal group

[PATCH v3 0/6] iommu: Extend changing default domain to normal group

[Lu Baolu]

The IOMMU group sysfs interface allows users to change the default
domain of a group. The current implementation uses device_lock() to make
sure that the devices in the group are not bound to any driver and won't
be bound during the process of changing the default domain. In order to
avoid a possible deadlock caused by lock order of device_lock and
group->mutex, it limits the functionality to singleton groups only.

The recently implemented DMA ownership framework can be applied here to
replace device_lock(). In addition, use group->mutex to ensure that the
iommu ops of the device are always valid during the process of changing
default domain.

With above replacement and enhancement, the device_lock() could be
removed and the singleton-group-only limitation could be removed.

This series is based on v6.3-rc1 with below series from Robin applied,
https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/

The whole series is also available on github:
https://github.com/LuBaolu/intel-iommu/commits/iommu-sysfs-default-domain-extension-v3

Please help to review and suggest.

Change log:
v3:
 - "arm_iommu_detach_device() is a noop" is not entirely right. It is
   used to to make the iommu driver stop using the domain that it is
   about to free. It cannot be a NOP or it is a UAF. [Jason]
 - Use Jason's new arm_iommu_release_device() proposal instead.

v2:
 - https://lore.kernel.org/linux-iommu/20230217094736.159005-1-baolu.lu@linux.intel.com/
 - Use group->mutex instead of an additional rw lock.

v1: initial post
 - https://lore.kernel.org/linux-iommu/20230213074941.919324-1-baolu.lu@linux.intel.com/

Lu Baolu (6):
  ARM/dma-mapping: Add arm_iommu_release_device()
  iommu: Split iommu_group_remove_device() into helpers
  iommu: Same critical region for device release and removal
  iommu: Move lock from iommu_change_dev_def_domain() to its caller
  iommu: Replace device_lock() with group->mutex
  iommu: Cleanup iommu_change_dev_def_domain()

 arch/arm/include/asm/dma-iommu.h              |   1 +
 arch/arm/mm/dma-mapping.c                     |  25 ++
 drivers/iommu/iommu.c                         | 270 ++++++++----------
 drivers/iommu/ipmmu-vmsa.c                    |  15 +-
 .../ABI/testing/sysfs-kernel-iommu_groups     |   1 -
 5 files changed, 156 insertions(+), 156 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device()

[Lu Baolu]

It is like arm_iommu_detach_device() except it handles the special case
of being called under an iommu driver's release operation. In this case
the driver must have already detached the device from any attached
domain before calling this function.

Replace arm_iommu_detach_device() with arm_iommu_release_device() in the
release path of the ipmmu-vmsa driver.

The bonus is that it also removes a obstacle of arm_iommu_detach_device()
re-entering the iommu core during release_device. With this removed, the
iommu core code could be simplified a lot.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/arm/include/asm/dma-iommu.h |  1 +
 arch/arm/mm/dma-mapping.c        | 25 +++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c       | 15 +++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
index fe9ef6f79e9c..ea7198a17861 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -31,6 +31,7 @@ void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
 int arm_iommu_attach_device(struct device *dev,
 					struct dma_iommu_mapping *mapping);
 void arm_iommu_detach_device(struct device *dev);
+void arm_iommu_release_device(struct device *dev);
 
 #endif /* __KERNEL__ */
 #endif
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 8bc01071474a..96fa27f4a164 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1682,6 +1682,31 @@ int arm_iommu_attach_device(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
 
+/**
+ * arm_iommu_release_device
+ * @dev: valid struct device pointer
+ *
+ * This is like arm_iommu_detach_device() except it handles the special
+ * case of being called under an iommu driver's release operation. In this
+ * case the driver must have already detached the device from any attached
+ * domain before calling this function.
+ */
+void arm_iommu_release_device(struct device *dev)
+{
+	struct dma_iommu_mapping *mapping;
+
+	mapping = to_dma_iommu_mapping(dev);
+	if (!mapping) {
+		dev_warn(dev, "Not attached\n");
+		return;
+	}
+
+	kref_put(&mapping->kref, release_iommu_mapping);
+	to_dma_iommu_mapping(dev) = NULL;
+	set_dma_ops(dev, NULL);
+}
+EXPORT_SYMBOL_GPL(arm_iommu_release_device);
+
 /**
  * arm_iommu_detach_device
  * @dev: valid struct device pointer
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index bdf1a4e5eae0..de9c74cf61a4 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -30,7 +30,7 @@
 #define arm_iommu_create_mapping(...)	NULL
 #define arm_iommu_attach_device(...)	-ENODEV
 #define arm_iommu_release_mapping(...)	do {} while (0)
-#define arm_iommu_detach_device(...)	do {} while (0)
+#define arm_iommu_release_device(...)	do {} while (0)
 #endif
 
 #define IPMMU_CTX_MAX		16U
@@ -820,7 +820,18 @@ static void ipmmu_probe_finalize(struct device *dev)
 
 static void ipmmu_release_device(struct device *dev)
 {
-	arm_iommu_detach_device(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
+	unsigned int i;
+
+	for (i = 0; i < fwspec->num_ids; ++i) {
+		unsigned int utlb = fwspec->ids[i];
+
+		ipmmu_imuctr_write(mmu, utlb, 0);
+		mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
+	}
+
+	arm_iommu_release_device(dev);
 }
 
 static struct iommu_group *ipmmu_find_group(struct device *dev)
-- 
2.34.1

[PATCH v3 2/6] iommu: Split iommu_group_remove_device() into helpers

[Lu Baolu]

So that code could be re-used by iommu_release_device() in the subsequent
change. No intention for functionality change.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 64 +++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 80211bf08c0d..bd9b293e07a8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -465,6 +465,46 @@ int iommu_probe_device(struct device *dev)
 
 }
 
+/*
+ * Remove a device from a group's device list and return the group device
+ * if successful.
+ */
+static struct group_device *
+__iommu_group_remove_device(struct iommu_group *group, struct device *dev)
+{
+	struct group_device *device;
+
+	lockdep_assert_held(&group->mutex);
+	list_for_each_entry(device, &group->devices, list) {
+		if (device->dev == dev) {
+			list_del(&device->list);
+			return device;
+		}
+	}
+
+	return NULL;
+}
+
+/*
+ * Release a device from its group and decrements the iommu group reference
+ * count.
+ */
+static void __iommu_group_release_device(struct iommu_group *group,
+					 struct group_device *grp_dev)
+{
+	struct device *dev = grp_dev->dev;
+
+	sysfs_remove_link(group->devices_kobj, grp_dev->name);
+	sysfs_remove_link(&dev->kobj, "iommu_group");
+
+	trace_remove_device_from_group(group->id, dev);
+
+	kfree(grp_dev->name);
+	kfree(grp_dev);
+	dev->iommu_group = NULL;
+	kobject_put(group->devices_kobj);
+}
+
 void iommu_release_device(struct device *dev)
 {
 	const struct iommu_ops *ops;
@@ -1080,7 +1120,7 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device);
 void iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
-	struct group_device *tmp_device, *device = NULL;
+	struct group_device *device;
 
 	if (!group)
 		return;
@@ -1088,27 +1128,11 @@ void iommu_group_remove_device(struct device *dev)
 	dev_info(dev, "Removing from iommu group %d\n", group->id);
 
 	mutex_lock(&group->mutex);
-	list_for_each_entry(tmp_device, &group->devices, list) {
-		if (tmp_device->dev == dev) {
-			device = tmp_device;
-			list_del(&device->list);
-			break;
-		}
-	}
+	device = __iommu_group_remove_device(group, dev);
 	mutex_unlock(&group->mutex);
 
-	if (!device)
-		return;
-
-	sysfs_remove_link(group->devices_kobj, device->name);
-	sysfs_remove_link(&dev->kobj, "iommu_group");
-
-	trace_remove_device_from_group(group->id, dev);
-
-	kfree(device->name);
-	kfree(device);
-	dev->iommu_group = NULL;
-	kobject_put(group->devices_kobj);
+	if (device)
+		__iommu_group_release_device(group, device);
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-- 
2.34.1

[PATCH v3 3/6] iommu: Same critical region for device release and removal

[Lu Baolu]

In a non-driver context, it is crucial to ensure the consistency of a
device's iommu ops. Otherwise, it may result in a situation where a
device is released but it's iommu ops are still used.

Put the ops->release_device and __iommu_group_remove_device() in a some
group->mutext critical region, so that, as long as group->mutex is held
and the device is in its group's device list, its iommu ops are always
consistent. Add check of group ownership if the released device is the
last one.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bd9b293e07a8..0bcd9625090d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -507,18 +507,44 @@ static void __iommu_group_release_device(struct iommu_group *group,
 
 void iommu_release_device(struct device *dev)
 {
+	struct iommu_group *group = dev->iommu_group;
+	struct group_device *device;
 	const struct iommu_ops *ops;
 
-	if (!dev->iommu)
+	if (!dev->iommu || !group)
 		return;
 
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
+	mutex_lock(&group->mutex);
+	device = __iommu_group_remove_device(group, dev);
+
+	/*
+	 * If the group has become empty then ownership must have been released,
+	 * and the current domain must be set back to NULL or the default
+	 * domain.
+	 */
+	if (list_empty(&group->devices))
+		WARN_ON(group->owner_cnt ||
+			group->domain != group->default_domain);
+
+	/*
+	 * release_device() must stop using any attached domain on the device.
+	 * If there are still other devices in the group they are not effected
+	 * by this callback.
+	 *
+	 * The IOMMU driver must set the device to either an identity or
+	 * blocking translation and stop using any domain pointer, as it is
+	 * going to be freed.
+	 */
 	ops = dev_iommu_ops(dev);
 	if (ops->release_device)
 		ops->release_device(dev);
+	mutex_unlock(&group->mutex);
+
+	if (device)
+		__iommu_group_release_device(group, device);
 
-	iommu_group_remove_device(dev);
 	module_put(ops->owner);
 	dev_iommu_free(dev);
 }
-- 
2.34.1

[PATCH v3 4/6] iommu: Move lock from iommu_change_dev_def_domain() to its caller

[Lu Baolu]

The intention is to make it possible to put group ownership check and
default domain change in a same critical region protected by the group's
mutex lock. No intentional functional change.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0bcd9625090d..f8f400548a10 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2945,7 +2945,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	int ret, dev_def_dom;
 	struct device *dev;
 
-	mutex_lock(&group->mutex);
+	lockdep_assert_held(&group->mutex);
 
 	if (group->default_domain != group->domain) {
 		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
@@ -3033,28 +3033,15 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 		goto free_new_domain;
 
 	group->domain = group->default_domain;
-
-	/*
-	 * Release the mutex here because ops->probe_finalize() call-back of
-	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
-	 * in-turn might call back into IOMMU core code, where it tries to take
-	 * group->mutex, resulting in a deadlock.
-	 */
-	mutex_unlock(&group->mutex);
-
-	/* Make sure dma_ops is appropriatley set */
-	iommu_group_do_probe_finalize(dev, group->default_domain);
 	iommu_domain_free(prev_dom);
+
 	return 0;
 
 free_new_domain:
 	iommu_domain_free(group->default_domain);
 	group->default_domain = prev_dom;
 	group->domain = prev_dom;
-
 out:
-	mutex_unlock(&group->mutex);
-
 	return ret;
 }
 
@@ -3142,7 +3129,19 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 		goto out;
 	}
 
+	mutex_lock(&group->mutex);
 	ret = iommu_change_dev_def_domain(group, dev, req_type);
+	/*
+	 * Release the mutex here because ops->probe_finalize() call-back of
+	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
+	 * in-turn might call back into IOMMU core code, where it tries to take
+	 * group->mutex, resulting in a deadlock.
+	 */
+	mutex_unlock(&group->mutex);
+
+	/* Make sure dma_ops is appropriatley set */
+	if (!ret)
+		iommu_group_do_probe_finalize(dev, group->default_domain);
 	ret = ret ?: count;
 
 out:
-- 
2.34.1

[PATCH v3 5/6] iommu: Replace device_lock() with group->mutex

[Lu Baolu]

device_lock() was used in iommu_group_store_type() to prevent the
devices in an iommu group from being attached by any device driver.
On the other hand, in order to avoid lock race between group->mutex
and device_lock(), it limited the usage scenario to the singleton
groups.

We already have the DMA ownership scheme to avoid driver attachment
and group->mutex ensures that device ops are always valid, there's
no need for device_lock() anymore. Remove device_lock() and the
singleton group limitation.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 78 +++++++++----------------------------------
 1 file changed, 16 insertions(+), 62 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f8f400548a10..9fe6d149f281 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3011,14 +3011,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 		goto out;
 	}
 
-	/* We can bring up a flush queue without tearing down the domain */
-	if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
-		ret = iommu_dma_init_fq(prev_dom);
-		if (!ret)
-			prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
-		goto out;
-	}
-
 	/* Sets group->default_domain to the newly allocated domain */
 	ret = iommu_group_alloc_default_domain(group, dev, type);
 	if (ret)
@@ -3051,7 +3043,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
  * transition. Return failure if this isn't met.
  *
  * We need to consider the race between this and the device release path.
- * device_lock(dev) is used here to guarantee that the device release path
+ * group->mutex is used here to guarantee that the device release path
  * will not be entered at the same time.
  */
 static ssize_t iommu_group_store_type(struct iommu_group *group,
@@ -3077,60 +3069,27 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	else
 		return -EINVAL;
 
-	/*
-	 * Lock/Unlock the group mutex here before device lock to
-	 * 1. Make sure that the iommu group has only one device (this is a
-	 *    prerequisite for step 2)
-	 * 2. Get struct *dev which is needed to lock device
-	 */
 	mutex_lock(&group->mutex);
-	if (iommu_group_device_count(group) != 1) {
+	/* We can bring up a flush queue without tearing down the domain. */
+	if (req_type == IOMMU_DOMAIN_DMA_FQ &&
+	    group->default_domain->type == IOMMU_DOMAIN_DMA) {
+		ret = iommu_dma_init_fq(group->default_domain);
+		if (!ret)
+			group->default_domain->type = IOMMU_DOMAIN_DMA_FQ;
 		mutex_unlock(&group->mutex);
-		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
-		return -EINVAL;
+
+		return ret ?: count;
 	}
 
-	/* Since group has only one device */
+	/* Otherwise, ensure that device exists and no driver is bound. */
 	dev = iommu_group_first_dev(group);
-	get_device(dev);
-
-	/*
-	 * Don't hold the group mutex because taking group mutex first and then
-	 * the device lock could potentially cause a deadlock as below. Assume
-	 * two threads T1 and T2. T1 is trying to change default domain of an
-	 * iommu group and T2 is trying to hot unplug a device or release [1] VF
-	 * of a PCIe device which is in the same iommu group. T1 takes group
-	 * mutex and before it could take device lock assume T2 has taken device
-	 * lock and is yet to take group mutex. Now, both the threads will be
-	 * waiting for the other thread to release lock. Below, lock order was
-	 * suggested.
-	 * device_lock(dev);
-	 *	mutex_lock(&group->mutex);
-	 *		iommu_change_dev_def_domain();
-	 *	mutex_unlock(&group->mutex);
-	 * device_unlock(dev);
-	 *
-	 * [1] Typical device release path
-	 * device_lock() from device/driver core code
-	 *  -> bus_notifier()
-	 *   -> iommu_bus_notifier()
-	 *    -> iommu_release_device()
-	 *     -> ops->release_device() vendor driver calls back iommu core code
-	 *      -> mutex_lock() from iommu core code
-	 */
-	mutex_unlock(&group->mutex);
-
-	/* Check if the device in the group still has a driver bound to it */
-	device_lock(dev);
-	if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
-	    group->default_domain->type == IOMMU_DOMAIN_DMA)) {
-		pr_err_ratelimited("Device is still bound to driver\n");
-		ret = -EBUSY;
-		goto out;
+	if (!dev || group->owner_cnt) {
+		mutex_unlock(&group->mutex);
+		return -EPERM;
 	}
 
-	mutex_lock(&group->mutex);
 	ret = iommu_change_dev_def_domain(group, dev, req_type);
+
 	/*
 	 * Release the mutex here because ops->probe_finalize() call-back of
 	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
@@ -3141,14 +3100,9 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 
 	/* Make sure dma_ops is appropriatley set */
 	if (!ret)
-		iommu_group_do_probe_finalize(dev, group->default_domain);
-	ret = ret ?: count;
-
-out:
-	device_unlock(dev);
-	put_device(dev);
+		__iommu_group_dma_finalize(group);
 
-	return ret;
+	return ret ?: count;
 }
 
 static bool iommu_is_default_domain(struct iommu_group *group)
-- 
2.34.1

[PATCH v3 6/6] iommu: Cleanup iommu_change_dev_def_domain()

[Lu Baolu]

As the singleton group limitation has been removed, cleanup the code
in iommu_change_dev_def_domain() accordingly.

Documentation is also updated.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c                         | 81 +++++--------------
 .../ABI/testing/sysfs-kernel-iommu_groups     |  1 -
 2 files changed, 21 insertions(+), 61 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9fe6d149f281..735554ccc04b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2924,11 +2924,10 @@ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
 EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 
 /*
- * Changes the default domain of an iommu group that has *only* one device
+ * Changes the default domain of an iommu group
  *
  * @group: The group for which the default domain should be changed
- * @prev_dev: The device in the group (this is used to make sure that the device
- *	 hasn't changed after the caller has called this function)
+ * @dev: The first device in the group
  * @type: The type of the new default domain that gets associated with the group
  *
  * Returns 0 on success and error code on failure
@@ -2939,101 +2938,63 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
  *    Please take a closer look if intended to use for other purposes.
  */
 static int iommu_change_dev_def_domain(struct iommu_group *group,
-				       struct device *prev_dev, int type)
+				       struct device *dev, int type)
 {
+	struct __group_domain_type gtype = {NULL, 0};
 	struct iommu_domain *prev_dom;
-	int ret, dev_def_dom;
-	struct device *dev;
+	int ret;
 
 	lockdep_assert_held(&group->mutex);
 
-	if (group->default_domain != group->domain) {
-		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
-		ret = -EBUSY;
-		goto out;
-	}
-
-	/*
-	 * iommu group wasn't locked while acquiring device lock in
-	 * iommu_group_store_type(). So, make sure that the device count hasn't
-	 * changed while acquiring device lock.
-	 *
-	 * Changing default domain of an iommu group with two or more devices
-	 * isn't supported because there could be a potential deadlock. Consider
-	 * the following scenario. T1 is trying to acquire device locks of all
-	 * the devices in the group and before it could acquire all of them,
-	 * there could be another thread T2 (from different sub-system and use
-	 * case) that has already acquired some of the device locks and might be
-	 * waiting for T1 to release other device locks.
-	 */
-	if (iommu_group_device_count(group) != 1) {
-		dev_err_ratelimited(prev_dev, "Cannot change default domain: Group has more than one device\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/* Since group has only one device */
-	dev = iommu_group_first_dev(group);
-
-	if (prev_dev != dev) {
-		dev_err_ratelimited(prev_dev, "Cannot change default domain: Device has been changed\n");
-		ret = -EBUSY;
-		goto out;
-	}
-
 	prev_dom = group->default_domain;
-	if (!prev_dom) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	dev_def_dom = iommu_get_def_domain_type(dev);
+	__iommu_group_for_each_dev(group, &gtype,
+				   probe_get_default_domain_type);
 	if (!type) {
 		/*
 		 * If the user hasn't requested any specific type of domain and
 		 * if the device supports both the domains, then default to the
 		 * domain the device was booted with
 		 */
-		type = dev_def_dom ? : iommu_def_domain_type;
-	} else if (dev_def_dom && type != dev_def_dom) {
-		dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
+		type = gtype.type ? : iommu_def_domain_type;
+	} else if (gtype.type && type != gtype.type) {
+		dev_err_ratelimited(dev, "Device cannot be in %s domain\n",
 				    iommu_domain_type_str(type));
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	/*
 	 * Switch to a new domain only if the requested domain type is different
 	 * from the existing default domain type
 	 */
-	if (prev_dom->type == type) {
-		ret = 0;
-		goto out;
-	}
+	if (prev_dom->type == type)
+		return 0;
+
+	group->default_domain = NULL;
+	group->domain = NULL;
 
 	/* Sets group->default_domain to the newly allocated domain */
 	ret = iommu_group_alloc_default_domain(group, dev, type);
 	if (ret)
-		goto out;
+		goto restore_old_domain;
 
-	ret = iommu_create_device_direct_mappings(group, dev);
+	ret = iommu_group_create_direct_mappings(group);
 	if (ret)
 		goto free_new_domain;
 
-	ret = __iommu_attach_device(group->default_domain, dev);
+	ret = __iommu_attach_group(group->default_domain, group);
 	if (ret)
 		goto free_new_domain;
 
-	group->domain = group->default_domain;
 	iommu_domain_free(prev_dom);
 
 	return 0;
 
 free_new_domain:
 	iommu_domain_free(group->default_domain);
+restore_old_domain:
 	group->default_domain = prev_dom;
 	group->domain = prev_dom;
-out:
+
 	return ret;
 }
 
diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index b15af6a5bc08..a42d4383d999 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -53,7 +53,6 @@ Description:	/sys/kernel/iommu_groups/<grp_id>/type shows the type of default
 
 		The default domain type of a group may be modified only when
 
-		- The group has only one device.
 		- The device in the group is not bound to any device driver.
 		  So, the users must unbind the appropriate driver before
 		  changing the default domain type.
-- 
2.34.1

Re: [PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device()

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:57:59AM +0800, Lu Baolu wrote:
> It is like arm_iommu_detach_device() except it handles the special case
> of being called under an iommu driver's release operation. In this case
> the driver must have already detached the device from any attached
> domain before calling this function.
> 
> Replace arm_iommu_detach_device() with arm_iommu_release_device() in the
> release path of the ipmmu-vmsa driver.
> 
> The bonus is that it also removes a obstacle of arm_iommu_detach_device()
> re-entering the iommu core during release_device. With this removed, the
> iommu core code could be simplified a lot.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  arch/arm/include/asm/dma-iommu.h |  1 +
>  arch/arm/mm/dma-mapping.c        | 25 +++++++++++++++++++++++++
>  drivers/iommu/ipmmu-vmsa.c       | 15 +++++++++++++--
>  3 files changed, 39 insertions(+), 2 deletions(-)

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

Jason

Re: [PATCH v3 2/6] iommu: Split iommu_group_remove_device() into helpers

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:58:00AM +0800, Lu Baolu wrote:
> So that code could be re-used by iommu_release_device() in the subsequent
> change. No intention for functionality change.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 64 +++++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 20 deletions(-)

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

Jason

Re: [PATCH v3 3/6] iommu: Same critical region for device release and removal

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:58:01AM +0800, Lu Baolu wrote:
> In a non-driver context, it is crucial to ensure the consistency of a
> device's iommu ops. Otherwise, it may result in a situation where a
> device is released but it's iommu ops are still used.
> 
> Put the ops->release_device and __iommu_group_remove_device() in a some
> group->mutext critical region, so that, as long as group->mutex is held
> and the device is in its group's device list, its iommu ops are always
> consistent. Add check of group ownership if the released device is the
> last one.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)


> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bd9b293e07a8..0bcd9625090d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -507,18 +507,44 @@ static void __iommu_group_release_device(struct iommu_group *group,
>  
>  void iommu_release_device(struct device *dev)
>  {
> +	struct iommu_group *group = dev->iommu_group;
> +	struct group_device *device;
>  	const struct iommu_ops *ops;
>  
> -	if (!dev->iommu)
> +	if (!dev->iommu || !group)
>  		return;
>  
>  	iommu_device_unlink(dev->iommu->iommu_dev, dev);
>  
> +	mutex_lock(&group->mutex);
> +	device = __iommu_group_remove_device(group, dev);
> +
> +	/*
> +	 * If the group has become empty then ownership must have been released,
> +	 * and the current domain must be set back to NULL or the default
> +	 * domain.
> +	 */
> +	if (list_empty(&group->devices))
> +		WARN_ON(group->owner_cnt ||
> +			group->domain != group->default_domain);
> +
> +	/*
> +	 * release_device() must stop using any attached domain on the device.
> +	 * If there are still other devices in the group they are not effected
> +	 * by this callback.
> +	 *
> +	 * The IOMMU driver must set the device to either an identity or
> +	 * blocking translation and stop using any domain pointer, as it is
> +	 * going to be freed.
> +	 */
>  	ops = dev_iommu_ops(dev);
>  	if (ops->release_device)
>  		ops->release_device(dev);
> +	mutex_unlock(&group->mutex);

IMHO it is best to be locked like this

But for this series, if you run into problems with ARM and
release_device I think we could still safely unlock group->mutex
before calling this?

It would still be OK because the iommu_group_first_dev() will either
return NULL so iommu_group_store_type() wills top, or it will block
the ultimate call to release here which invalidate's ops.

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

Jason

Re: [PATCH v3 4/6] iommu: Move lock from iommu_change_dev_def_domain() to its caller

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:58:02AM +0800, Lu Baolu wrote:
> The intention is to make it possible to put group ownership check and
> default domain change in a same critical region protected by the group's
> mutex lock. No intentional functional change.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0bcd9625090d..f8f400548a10 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2945,7 +2945,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>  	int ret, dev_def_dom;
>  	struct device *dev;
>  
> -	mutex_lock(&group->mutex);
> +	lockdep_assert_held(&group->mutex);
>  
>  	if (group->default_domain != group->domain) {
>  		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
> @@ -3033,28 +3033,15 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>  		goto free_new_domain;
>  
>  	group->domain = group->default_domain;
> -
> -	/*
> -	 * Release the mutex here because ops->probe_finalize() call-back of
> -	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
> -	 * in-turn might call back into IOMMU core code, where it tries to take
> -	 * group->mutex, resulting in a deadlock.
> -	 */
> -	mutex_unlock(&group->mutex);
> -
> -	/* Make sure dma_ops is appropriatley set */
> -	iommu_group_do_probe_finalize(dev, group->default_domain);
>  	iommu_domain_free(prev_dom);
> +
>  	return 0;
>  
>  free_new_domain:
>  	iommu_domain_free(group->default_domain);
>  	group->default_domain = prev_dom;
>  	group->domain = prev_dom;
> -
>  out:
> -	mutex_unlock(&group->mutex);
> -
>  	return ret;
>  }
>  
> @@ -3142,7 +3129,19 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>  		goto out;
>  	}
>  
> +	mutex_lock(&group->mutex);
>  	ret = iommu_change_dev_def_domain(group, dev, req_type);
> +	/*
> +	 * Release the mutex here because ops->probe_finalize() call-back of
> +	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
> +	 * in-turn might call back into IOMMU core code, where it tries to take
> +	 * group->mutex, resulting in a deadlock.
> +	 */
> +	mutex_unlock(&group->mutex);
> +
> +	/* Make sure dma_ops is appropriatley set */
> +	if (!ret)
> +		iommu_group_do_probe_finalize(dev, group->default_domain);

Everything about iommu_group_do_probe_finalize() is still unsafe
against races with release. :(

Pre-existing bug so maybe leave it for this series :\

To fix it I'd suggest splitting probe_finalize ops into probe_finalize
and probe_finalized_unlocked.

Only have the "bad" deadlocky drivers use the unlocked variant and fix
intel and virtio to use the safe varient. 

We can decide which variant to use under the mutex and then at least
"good" drivers don't have this race.

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

Jason

Re: [PATCH v3 5/6] iommu: Replace device_lock() with group->mutex

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:58:03AM +0800, Lu Baolu wrote:
> device_lock() was used in iommu_group_store_type() to prevent the
> devices in an iommu group from being attached by any device driver.
> On the other hand, in order to avoid lock race between group->mutex
> and device_lock(), it limited the usage scenario to the singleton
> groups.
> 
> We already have the DMA ownership scheme to avoid driver attachment
> and group->mutex ensures that device ops are always valid, there's
> no need for device_lock() anymore. Remove device_lock() and the
> singleton group limitation.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 78 +++++++++----------------------------------
>  1 file changed, 16 insertions(+), 62 deletions(-)

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

Thanks,
Jason

Re: [PATCH v3 6/6] iommu: Cleanup iommu_change_dev_def_domain()

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:58:04AM +0800, Lu Baolu wrote:
> As the singleton group limitation has been removed, cleanup the code
> in iommu_change_dev_def_domain() accordingly.
> 
> Documentation is also updated.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c                         | 81 +++++--------------
>  .../ABI/testing/sysfs-kernel-iommu_groups     |  1 -
>  2 files changed, 21 insertions(+), 61 deletions(-)

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

I am going to post a series that conflicts with this, but it can go
after this one

Thanks for doing this, it looks great

Jason

Re: [PATCH v3 0/6] iommu: Extend changing default domain to normal group

[Jason Gunthorpe]

On Mon, Mar 06, 2023 at 10:57:58AM +0800, Lu Baolu wrote:
> The IOMMU group sysfs interface allows users to change the default
> domain of a group. The current implementation uses device_lock() to make
> sure that the devices in the group are not bound to any driver and won't
> be bound during the process of changing the default domain. In order to
> avoid a possible deadlock caused by lock order of device_lock and
> group->mutex, it limits the functionality to singleton groups only.
> 
> The recently implemented DMA ownership framework can be applied here to
> replace device_lock(). In addition, use group->mutex to ensure that the
> iommu ops of the device are always valid during the process of changing
> default domain.
> 
> With above replacement and enhancement, the device_lock() could be
> removed and the singleton-group-only limitation could be removed.
> 
> This series is based on v6.3-rc1 with below series from Robin applied,
> https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/

Joerg can you help Lu with patch planning here? Can we get Robin's
series, this one and a maybe few more that clash on this for the
cycle?

Should this one go before Robin's series? It didn't seem obviously
dependent beyond using the same helper function?

Thanks,
Jason

Re: [PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device()

[Robin Murphy]

On 2023-03-06 02:57, Lu Baolu wrote:
> It is like arm_iommu_detach_device() except it handles the special case
> of being called under an iommu driver's release operation. In this case
> the driver must have already detached the device from any attached
> domain before calling this function.
> 
> Replace arm_iommu_detach_device() with arm_iommu_release_device() in the
> release path of the ipmmu-vmsa driver.
> 
> The bonus is that it also removes a obstacle of arm_iommu_detach_device()
> re-entering the iommu core during release_device. With this removed, the
> iommu core code could be simplified a lot.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   arch/arm/include/asm/dma-iommu.h |  1 +
>   arch/arm/mm/dma-mapping.c        | 25 +++++++++++++++++++++++++
>   drivers/iommu/ipmmu-vmsa.c       | 15 +++++++++++++--
>   3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
> index fe9ef6f79e9c..ea7198a17861 100644
> --- a/arch/arm/include/asm/dma-iommu.h
> +++ b/arch/arm/include/asm/dma-iommu.h
> @@ -31,6 +31,7 @@ void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
>   int arm_iommu_attach_device(struct device *dev,
>   					struct dma_iommu_mapping *mapping);
>   void arm_iommu_detach_device(struct device *dev);
> +void arm_iommu_release_device(struct device *dev);
>   
>   #endif /* __KERNEL__ */
>   #endif
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 8bc01071474a..96fa27f4a164 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1682,6 +1682,31 @@ int arm_iommu_attach_device(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
>   
> +/**
> + * arm_iommu_release_device
> + * @dev: valid struct device pointer
> + *
> + * This is like arm_iommu_detach_device() except it handles the special
> + * case of being called under an iommu driver's release operation. In this
> + * case the driver must have already detached the device from any attached
> + * domain before calling this function.
> + */
> +void arm_iommu_release_device(struct device *dev)
> +{
> +	struct dma_iommu_mapping *mapping;
> +
> +	mapping = to_dma_iommu_mapping(dev);
> +	if (!mapping) {
> +		dev_warn(dev, "Not attached\n");
> +		return;
> +	}
> +
> +	kref_put(&mapping->kref, release_iommu_mapping);
> +	to_dma_iommu_mapping(dev) = NULL;
> +	set_dma_ops(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(arm_iommu_release_device);
> +
>   /**
>    * arm_iommu_detach_device
>    * @dev: valid struct device pointer
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index bdf1a4e5eae0..de9c74cf61a4 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -30,7 +30,7 @@
>   #define arm_iommu_create_mapping(...)	NULL
>   #define arm_iommu_attach_device(...)	-ENODEV
>   #define arm_iommu_release_mapping(...)	do {} while (0)
> -#define arm_iommu_detach_device(...)	do {} while (0)
> +#define arm_iommu_release_device(...)	do {} while (0)
>   #endif
>   
>   #define IPMMU_CTX_MAX		16U
> @@ -820,7 +820,18 @@ static void ipmmu_probe_finalize(struct device *dev)
>   
>   static void ipmmu_release_device(struct device *dev)
>   {
> -	arm_iommu_detach_device(dev);
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
> +	unsigned int i;
> +
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> +		unsigned int utlb = fwspec->ids[i];
> +
> +		ipmmu_imuctr_write(mmu, utlb, 0);
> +		mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> +	}
> +
> +	arm_iommu_release_device(dev);

Just call the existing arm_iommu_release_mapping(). Look at where the 
BUS_NOTIFY_REMOVED_DEVICE point is in device_del(); this device is not 
coming back. Zeroing out pointers and testing for a condition which 
cannot be true by construction is simply a waste of time and code.

Thanks,
Robin.

>   }
>   
>   static struct iommu_group *ipmmu_find_group(struct device *dev)

Re: [PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device()

[Baolu Lu]

On 3/11/23 6:04 AM, Robin Murphy wrote:
> On 2023-03-06 02:57, Lu Baolu wrote:
>> It is like arm_iommu_detach_device() except it handles the special case
>> of being called under an iommu driver's release operation. In this case
>> the driver must have already detached the device from any attached
>> domain before calling this function.
>>
>> Replace arm_iommu_detach_device() with arm_iommu_release_device() in the
>> release path of the ipmmu-vmsa driver.
>>
>> The bonus is that it also removes a obstacle of arm_iommu_detach_device()
>> re-entering the iommu core during release_device. With this removed, the
>> iommu core code could be simplified a lot.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   arch/arm/include/asm/dma-iommu.h |  1 +
>>   arch/arm/mm/dma-mapping.c        | 25 +++++++++++++++++++++++++
>>   drivers/iommu/ipmmu-vmsa.c       | 15 +++++++++++++--
>>   3 files changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/dma-iommu.h 
>> b/arch/arm/include/asm/dma-iommu.h
>> index fe9ef6f79e9c..ea7198a17861 100644
>> --- a/arch/arm/include/asm/dma-iommu.h
>> +++ b/arch/arm/include/asm/dma-iommu.h
>> @@ -31,6 +31,7 @@ void arm_iommu_release_mapping(struct 
>> dma_iommu_mapping *mapping);
>>   int arm_iommu_attach_device(struct device *dev,
>>                       struct dma_iommu_mapping *mapping);
>>   void arm_iommu_detach_device(struct device *dev);
>> +void arm_iommu_release_device(struct device *dev);
>>   #endif /* __KERNEL__ */
>>   #endif
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index 8bc01071474a..96fa27f4a164 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -1682,6 +1682,31 @@ int arm_iommu_attach_device(struct device *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
>> +/**
>> + * arm_iommu_release_device
>> + * @dev: valid struct device pointer
>> + *
>> + * This is like arm_iommu_detach_device() except it handles the special
>> + * case of being called under an iommu driver's release operation. In 
>> this
>> + * case the driver must have already detached the device from any 
>> attached
>> + * domain before calling this function.
>> + */
>> +void arm_iommu_release_device(struct device *dev)
>> +{
>> +    struct dma_iommu_mapping *mapping;
>> +
>> +    mapping = to_dma_iommu_mapping(dev);
>> +    if (!mapping) {
>> +        dev_warn(dev, "Not attached\n");
>> +        return;
>> +    }
>> +
>> +    kref_put(&mapping->kref, release_iommu_mapping);
>> +    to_dma_iommu_mapping(dev) = NULL;
>> +    set_dma_ops(dev, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(arm_iommu_release_device);
>> +
>>   /**
>>    * arm_iommu_detach_device
>>    * @dev: valid struct device pointer
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index bdf1a4e5eae0..de9c74cf61a4 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -30,7 +30,7 @@
>>   #define arm_iommu_create_mapping(...)    NULL
>>   #define arm_iommu_attach_device(...)    -ENODEV
>>   #define arm_iommu_release_mapping(...)    do {} while (0)
>> -#define arm_iommu_detach_device(...)    do {} while (0)
>> +#define arm_iommu_release_device(...)    do {} while (0)
>>   #endif
>>   #define IPMMU_CTX_MAX        16U
>> @@ -820,7 +820,18 @@ static void ipmmu_probe_finalize(struct device *dev)
>>   static void ipmmu_release_device(struct device *dev)
>>   {
>> -    arm_iommu_detach_device(dev);
>> +    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +    struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
>> +    unsigned int i;
>> +
>> +    for (i = 0; i < fwspec->num_ids; ++i) {
>> +        unsigned int utlb = fwspec->ids[i];
>> +
>> +        ipmmu_imuctr_write(mmu, utlb, 0);
>> +        mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
>> +    }
>> +
>> +    arm_iommu_release_device(dev);
> 
> Just call the existing arm_iommu_release_mapping(). Look at where the 
> BUS_NOTIFY_REMOVED_DEVICE point is in device_del(); this device is not 
> coming back. Zeroing out pointers and testing for a condition which 
> cannot be true by construction is simply a waste of time and code.

Fair enough. But the driver seems to have done things wrong.

It creates arm mappings in iommu probe path, but release it in the
driver's .remove path (see ipmmu_remove()). So perhaps we should move
calling arm_iommu_release_mapping() from ipmmu_remove() to
ipmmu_release_device()?

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index de9c74cf61a4..057050c28360 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -831,7 +831,7 @@ static void ipmmu_release_device(struct device *dev)
                 mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
         }

-       arm_iommu_release_device(dev);
+       arm_iommu_release_mapping(mmu->mapping);
  }

  static struct iommu_group *ipmmu_find_group(struct device *dev)
@@ -1091,8 +1091,6 @@ static int ipmmu_remove(struct platform_device *pdev)
         iommu_device_sysfs_remove(&mmu->iommu);
         iommu_device_unregister(&mmu->iommu);

-       arm_iommu_release_mapping(mmu->mapping);
-
         ipmmu_device_reset(mmu);

         return 0;

Best regards,
baolu