[PATCH 0/9] Consolidate the error handling around device attachment

[PATCH 0/9] Consolidate the error handling around device attachment

[Jason Gunthorpe]

Device attachment has a bunch of different flows open coded in different
ways throughout the code.

One of the things that became apparently recently is that error handling
is important and we do need to consistently treat errors during attach and
have some strategy to unwind back to a safe state.

Implement a single algorithm for this in one function. It will call each
device's attach, if it fails it will try to go back to the prior domain or
as a contingency against a UAF crash try to go to a blocking domain.

As part of this we consolidate how the default domain is created and
attached as well into one place with a consistent flow.

The new worker functions are called __iommu_device_set_domain() and
__iommu_group_set_domain_internal(), each has sensible error handling
internally. At the end __iommu_group_set_domain_internal() is the only
function that stores to group->domain, and must be called to change this
value.

Some flags tell the intent of the caller, if the caller cannot accept a
failure, or if the caller is a first attach and wants to do the deferred
logic.

Several of the confusing workflows where we store things in group->domain
or group->default_domain before they are fully setup are removed.

Cc: Nicolin Chen <nicolinc@nvidia.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason Gunthorpe (9):
  iommu: Make __iommu_group_set_domain() handle error unwind
  iommu: Use __iommu_group_set_domain() for __iommu_attach_group()
  iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()
  iommu: Replace __iommu_group_dma_first_attach() with set_domain
  iommu: Make iommu_group_do_dma_first_attach() simpler
  iommu: Fix iommu_probe_device() to attach the right domain
  iommu: Remove the assignment of group->domain during default domain
    alloc
  iommu: Consolidate the default_domain setup to one function
  iommu: Remove __iommu_group_for_each_dev()

 drivers/iommu/iommu.c | 444 +++++++++++++++++++++---------------------
 1 file changed, 222 insertions(+), 222 deletions(-)


base-commit: 771ad306397838108f264e180e31188136907616
-- 
2.40.0

[PATCH 1/9] iommu: Make __iommu_group_set_domain() handle error unwind

[Jason Gunthorpe]

Let's try to have a consistent and clear strategy for error handling
during domain attach failures.

There are two broad categories, the first is callers doing destruction and
trying to set the domain back to a previously good domain. These cases
cannot handle failure during destruction flows and must succeed, or at
least avoid a UAF on the current group->domain which is likely about to be
freed.

Many of the drivers are well behaved here and will not hit the WARN_ON's
or a UAF, but some are doing hypercalls/etc that can fail unpredictably
and don't meet the expectations.

The second case is attaching a domain for the first time in a failable
context, failure should restore the attachment back to group->domain using
the above unfailable operation.

Have __iommu_group_set_domain_internal() execute a common algorithm that
tries to achieve this, and in the worst case, would leave a device
"detached" or assigned to a global blocking domain. This relies on some
existing common driver behaviors where attach failure will also do detatch
and true IOMMU_DOMAIN_BLOCK implementations that are not allowed to ever
fail.

Name the first case with __iommu_group_set_domain_nofail() to make it
clear.

Pull all the error handling and WARN_ON generation into
__iommu_group_set_domain_internal().

Avoid the obfuscating use of __iommu_group_for_each_dev() and be more
careful about what should happen during failures by only touching devices
we've already touched.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 132 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 107 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f7a5f166b58512..6f52df534decc0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -96,8 +96,26 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group);
+
+enum {
+	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
+};
+
+static int __iommu_group_set_domain_internal(struct iommu_group *group,
+					     struct iommu_domain *new_domain,
+					     unsigned int flags);
 static int __iommu_group_set_domain(struct iommu_group *group,
-				    struct iommu_domain *new_domain);
+				    struct iommu_domain *new_domain)
+{
+	return __iommu_group_set_domain_internal(group, new_domain, 0);
+}
+static void __iommu_group_set_domain_nofail(struct iommu_group *group,
+					    struct iommu_domain *new_domain)
+{
+	WARN_ON(__iommu_group_set_domain_internal(
+		group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED));
+}
+
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -1999,15 +2017,13 @@ EXPORT_SYMBOL_GPL(iommu_domain_free);
 static void __iommu_group_set_core_domain(struct iommu_group *group)
 {
 	struct iommu_domain *new_domain;
-	int ret;
 
 	if (group->owner)
 		new_domain = group->blocking_domain;
 	else
 		new_domain = group->default_domain;
 
-	ret = __iommu_group_set_domain(group, new_domain);
-	WARN(ret, "iommu driver failed to attach the default/blocking domain");
+	__iommu_group_set_domain_nofail(group, new_domain);
 }
 
 static int __iommu_attach_device(struct iommu_domain *domain,
@@ -2213,29 +2229,60 @@ int iommu_group_replace_domain(struct iommu_group *group,
 
 	mutex_lock(&group->mutex);
 	ret = __iommu_group_set_domain(group, new_domain);
-	if (ret)
-		__iommu_group_for_each_dev(group, group->domain,
-					   iommu_group_do_attach_device);
 	mutex_unlock(&group->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, IOMMUFD_INTERNAL);
 
-static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
+static int __iommu_device_set_domain(struct iommu_group *group,
+				     struct device *dev,
+				     struct iommu_domain *new_domain,
+				     unsigned int flags)
 {
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-	if (!WARN_ON(!ops->set_platform_dma_ops))
-		ops->set_platform_dma_ops(dev);
+	int ret;
 
+	ret = __iommu_attach_device(new_domain, dev);
+	if (ret) {
+		/*
+		 * If we have a blocking domain then try to attach that in hopes
+		 * of avoiding a UAF. Modern drivers should implement blocking
+		 * domains as global statics that cannot fail.
+		 */
+		if ((flags & IOMMU_SET_DOMAIN_MUST_SUCCEED) &&
+		    group->blocking_domain &&
+		    group->blocking_domain != new_domain)
+			__iommu_attach_device(group->blocking_domain, dev);
+		return ret;
+	}
 	return 0;
 }
 
-static int __iommu_group_set_domain(struct iommu_group *group,
-				    struct iommu_domain *new_domain)
+/*
+ * If 0 is returned the group's domain is new_domain. If an error is returned
+ * then the group's domain will be set back to the existing domain unless
+ * IOMMU_SET_DOMAIN_MUST_SUCCEED, otherwise an error is returned and the group's
+ * domains is left inconsistent. This is a driver bug to fail attach with a
+ * previously good domain. We try to avoid a kernel UAF because of this.
+ *
+ * IOMMU groups are really the natural working unit of the IOMMU, but the IOMMU
+ * API works on domains and devices.  Bridge that gap by iterating over the
+ * devices in a group.  Ideally we'd have a single device which represents the
+ * requestor ID of the group, but we also allow IOMMU drivers to create policy
+ * defined minimum sets, where the physical hardware may be able to distiguish
+ * members, but we wish to group them at a higher level (ex. untrusted
+ * multi-function PCI devices).  Thus we attach each device.
+ */
+static int __iommu_group_set_domain_internal(struct iommu_group *group,
+					     struct iommu_domain *new_domain,
+					     unsigned int flags)
 {
+	struct group_device *last_gdev;
+	struct group_device *gdev;
+	int result;
 	int ret;
 
+	lockdep_assert_held(&group->mutex);
+
 	if (group->domain == new_domain)
 		return 0;
 
@@ -2245,8 +2292,12 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 	 * platform specific behavior.
 	 */
 	if (!new_domain) {
-		__iommu_group_for_each_dev(group, NULL,
-					   iommu_group_do_set_platform_dma);
+		list_for_each_entry(gdev, &group->devices, list) {
+			const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
+
+			if (!WARN_ON(!ops->set_platform_dma_ops))
+				ops->set_platform_dma_ops(gdev->dev);
+		}
 		group->domain = NULL;
 		return 0;
 	}
@@ -2260,12 +2311,46 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 	 * Note that this is called in error unwind paths, attaching to a
 	 * domain that has already been attached cannot fail.
 	 */
-	ret = __iommu_group_for_each_dev(group, new_domain,
-					 iommu_group_do_attach_device);
-	if (ret)
-		return ret;
+	result = 0;
+	list_for_each_entry(gdev, &group->devices, list) {
+		ret = __iommu_device_set_domain(group, gdev->dev, new_domain,
+						flags);
+		if (ret) {
+			result = ret;
+			/*
+			 * Keep trying the other devices in the group. If a
+			 * driver fails attach to an otherwise good domain, and
+			 * does not support blocking domains, it should at least
+			 * drop its reference on the current domain so we don't
+			 * UAF.
+			 */
+			if (flags & IOMMU_SET_DOMAIN_MUST_SUCCEED)
+				continue;
+			goto err_revert;
+		}
+	}
 	group->domain = new_domain;
-	return 0;
+	return result;
+
+err_revert:
+	last_gdev = gdev;
+	list_for_each_entry(gdev, &group->devices, list) {
+		const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
+
+		/*
+		 * If set_platform_dma_ops is not present a NULL domain can
+		 * happen only for first probe, in which case we leave
+		 * group->domain as NULL and let release clean everything up.
+		 */
+		if (group->domain)
+			WARN_ON(__iommu_device_set_domain(group, gdev->dev,
+							  group->domain, true));
+		else if (ops->set_platform_dma_ops)
+			ops->set_platform_dma_ops(gdev->dev);
+		if (gdev == last_gdev)
+			break;
+	}
+	return ret;
 }
 
 void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
@@ -3278,16 +3363,13 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
 
 static void __iommu_release_dma_ownership(struct iommu_group *group)
 {
-	int ret;
-
 	if (WARN_ON(!group->owner_cnt || !group->owner ||
 		    !xa_empty(&group->pasid_array)))
 		return;
 
 	group->owner_cnt = 0;
 	group->owner = NULL;
-	ret = __iommu_group_set_domain(group, group->default_domain);
-	WARN(ret, "iommu driver failed to attach the default domain");
+	__iommu_group_set_domain_nofail(group, group->default_domain);
 }
 
 /**
-- 
2.40.0

RE: [PATCH 1/9] iommu: Make __iommu_group_set_domain() handle error unwind

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> +
> +		/*
> +		 * If set_platform_dma_ops is not present a NULL domain
> can
> +		 * happen only for first probe, in which case we leave
> +		 * group->domain as NULL and let release clean everything up.
> +		 */
> +		if (group->domain)
> +			WARN_ON(__iommu_device_set_domain(group,
> gdev->dev,
> +							  group->domain,
> true));

s/true/IOMMU_SET_DOMAIN_MUST_SUCCEED/

otherwise looks good.

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

Re: [PATCH 1/9] iommu: Make __iommu_group_set_domain() handle error unwind

[Jason Gunthorpe]

On Fri, Mar 24, 2023 at 07:17:57AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 22, 2023 3:53 AM
> > +
> > +		/*
> > +		 * If set_platform_dma_ops is not present a NULL domain
> > can
> > +		 * happen only for first probe, in which case we leave
> > +		 * group->domain as NULL and let release clean everything up.
> > +		 */
> > +		if (group->domain)
> > +			WARN_ON(__iommu_device_set_domain(group,
> > gdev->dev,
> > +							  group->domain,
> > true));
> 
> s/true/IOMMU_SET_DOMAIN_MUST_SUCCEED/

Woops, yes missed that

Thanks,
Jason

[PATCH 2/9] iommu: Use __iommu_group_set_domain() for __iommu_attach_group()

[Jason Gunthorpe]

The error recovery here matches the recovery inside
__iommu_group_set_domain(), so just use it directly.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6f52df534decc0..96323c8cff0168 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2136,52 +2136,14 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
 	return dev->iommu_group->default_domain;
 }
 
-/*
- * IOMMU groups are really the natural working unit of the IOMMU, but
- * the IOMMU API works on domains and devices.  Bridge that gap by
- * iterating over the devices in a group.  Ideally we'd have a single
- * device which represents the requestor ID of the group, but we also
- * allow IOMMU drivers to create policy defined minimum sets, where
- * the physical hardware may be able to distiguish members, but we
- * wish to group them at a higher level (ex. untrusted multi-function
- * PCI devices).  Thus we attach each device.
- */
-static int iommu_group_do_attach_device(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-
-	return __iommu_attach_device(domain, dev);
-}
-
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group)
 {
-	int ret;
-
 	if (group->domain && group->domain != group->default_domain &&
 	    group->domain != group->blocking_domain)
 		return -EBUSY;
 
-	ret = __iommu_group_for_each_dev(group, domain,
-					 iommu_group_do_attach_device);
-	if (ret == 0) {
-		group->domain = domain;
-	} else {
-		/*
-		 * To recover from the case when certain device within the
-		 * group fails to attach to the new domain, we need force
-		 * attaching all devices back to the old domain. The old
-		 * domain is compatible for all devices in the group,
-		 * hence the iommu driver should always return success.
-		 */
-		struct iommu_domain *old_domain = group->domain;
-
-		group->domain = NULL;
-		WARN(__iommu_group_set_domain(group, old_domain),
-		     "iommu driver failed to attach a compatible domain");
-	}
-
-	return ret;
+	return __iommu_group_set_domain(group, domain);
 }
 
 /**
-- 
2.40.0

RE: [PATCH 2/9] iommu: Use __iommu_group_set_domain() for __iommu_attach_group()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> 
> The error recovery here matches the recovery inside
> __iommu_group_set_domain(), so just use it directly.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH 3/9] iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()

[Jason Gunthorpe]

This is missing re-attach error handling if the attach fails, use the
common code.

The ugly "group->domain = prev_domain" will be cleaned in a later patch.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 96323c8cff0168..ed4d792f4bdb68 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3005,16 +3005,15 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	if (ret)
 		goto out;
 
+	group->domain = prev_dom;
 	ret = iommu_create_device_direct_mappings(group, dev);
 	if (ret)
 		goto free_new_domain;
 
-	ret = __iommu_attach_device(group->default_domain, dev);
+	ret = __iommu_group_set_domain(group, group->default_domain);
 	if (ret)
 		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
@@ -3031,7 +3030,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 free_new_domain:
 	iommu_domain_free(group->default_domain);
 	group->default_domain = prev_dom;
-	group->domain = prev_dom;
 
 out:
 	mutex_unlock(&group->mutex);
-- 
2.40.0

RE: [PATCH 3/9] iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> 
> This is missing re-attach error handling if the attach fails, use the
> common code.
> 
> The ugly "group->domain = prev_domain" will be cleaned in a later patch.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Joerg just picked Baolu's series [1]. this should be rebased atop.

[1] https://lore.kernel.org/linux-iommu/20230322064956.263419-1-baolu.lu@linux.intel.com/#t

Re: [PATCH 3/9] iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()

[Jason Gunthorpe]

On Fri, Mar 24, 2023 at 07:19:35AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 22, 2023 3:53 AM
> > 
> > This is missing re-attach error handling if the attach fails, use the
> > common code.
> > 
> > The ugly "group->domain = prev_domain" will be cleaned in a later patch.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Joerg just picked Baolu's series [1]. this should be rebased atop.
> 
> [1] https://lore.kernel.org/linux-iommu/20230322064956.263419-1-baolu.lu@linux.intel.com/#t

Yeah, I just did it, and made a few more adjustments. The default
domain setup stuff looks really clean and straightfoward in the v2

Jason

[PATCH 4/9] iommu: Replace __iommu_group_dma_first_attach() with set_domain

[Jason Gunthorpe]

Prepare for removing the group->domain set from
iommu_group_alloc_default_domain() by calling __iommu_group_set_domain_internal()
to set the group->domain.

Add IOMMU_SET_DOMAIN_WITH_DEFERRED to allow it to do the attach_deferred
logic.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ed4d792f4bdb68..3e4f9ab89d38ec 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -99,6 +99,7 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 
 enum {
 	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
+	IOMMU_SET_DOMAIN_WITH_DEFERRED = 1 << 1,
 };
 
 static int __iommu_group_set_domain_internal(struct iommu_group *group,
@@ -1814,12 +1815,6 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 
 }
 
-static int __iommu_group_dma_first_attach(struct iommu_group *group)
-{
-	return __iommu_group_for_each_dev(group, group->default_domain,
-					  iommu_group_do_dma_first_attach);
-}
-
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -1882,7 +1877,10 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		ret = __iommu_group_dma_first_attach(group);
+		group->domain = NULL;
+		ret = __iommu_group_set_domain_internal(
+			group, group->default_domain,
+			IOMMU_SET_DOMAIN_WITH_DEFERRED);
 
 		mutex_unlock(&group->mutex);
 
@@ -2203,6 +2201,12 @@ static int __iommu_device_set_domain(struct iommu_group *group,
 {
 	int ret;
 
+	if ((flags & IOMMU_SET_DOMAIN_WITH_DEFERRED) &&
+	    iommu_is_attach_deferred(dev)) {
+		dev->iommu->attach_deferred = 1;
+		return 0;
+	}
+
 	ret = __iommu_attach_device(new_domain, dev);
 	if (ret) {
 		/*
-- 
2.40.0

RE: [PATCH 4/9] iommu: Replace __iommu_group_dma_first_attach() with set_domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> 
> Prepare for removing the group->domain set from
> iommu_group_alloc_default_domain() by calling
> __iommu_group_set_domain_internal()
> to set the group->domain.
> 
> Add IOMMU_SET_DOMAIN_WITH_DEFERRED to allow it to do the
> attach_deferred
> logic.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH 5/9] iommu: Make iommu_group_do_dma_first_attach() simpler

[Jason Gunthorpe]

It should always attach to the current group->domain, so don't take in a
domain parameter. Use the __iommu_device_set_domain() common code to
handle the attach.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3e4f9ab89d38ec..ea7700bbcf3399 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -102,6 +102,10 @@ enum {
 	IOMMU_SET_DOMAIN_WITH_DEFERRED = 1 << 1,
 };
 
+static int __iommu_device_set_domain(struct iommu_group *group,
+				     struct device *dev,
+				     struct iommu_domain *new_domain,
+				     unsigned int flags);
 static int __iommu_group_set_domain_internal(struct iommu_group *group,
 					     struct iommu_domain *new_domain,
 					     unsigned int flags);
@@ -402,18 +406,11 @@ static bool iommu_is_attach_deferred(struct device *dev)
 	return false;
 }
 
-static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
+static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
 {
-	struct iommu_domain *domain = data;
-
-	lockdep_assert_held(&dev->iommu_group->mutex);
-
-	if (iommu_is_attach_deferred(dev)) {
-		dev->iommu->attach_deferred = 1;
-		return 0;
-	}
-
-	return __iommu_attach_device(domain, dev);
+	return __iommu_device_set_domain(
+		group, dev, group->domain,
+		group->owner ? 0 : IOMMU_SET_DOMAIN_WITH_DEFERRED);
 }
 
 int iommu_probe_device(struct device *dev)
@@ -446,7 +443,7 @@ int iommu_probe_device(struct device *dev)
 	 * attach the default domain.
 	 */
 	if (group->default_domain && !group->owner) {
-		ret = iommu_group_do_dma_first_attach(dev, group->default_domain);
+		ret = iommu_group_do_dma_first_attach(group, dev);
 		if (ret) {
 			mutex_unlock(&group->mutex);
 			iommu_group_put(group);
@@ -1049,7 +1046,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
 	if (group->domain)
-		ret = iommu_group_do_dma_first_attach(dev, group->domain);
+		ret = iommu_group_do_dma_first_attach(group, dev);
 	mutex_unlock(&group->mutex);
 	if (ret)
 		goto err_put_group;
-- 
2.40.0

RE: [PATCH 5/9] iommu: Make iommu_group_do_dma_first_attach() simpler

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> 
> -static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
> +static int iommu_group_do_dma_first_attach(struct iommu_group *group,
> struct device *dev)
>  {
> -	struct iommu_domain *domain = data;
> -
> -	lockdep_assert_held(&dev->iommu_group->mutex);
> -
> -	if (iommu_is_attach_deferred(dev)) {
> -		dev->iommu->attach_deferred = 1;
> -		return 0;
> -	}
> -
> -	return __iommu_attach_device(domain, dev);
> +	return __iommu_device_set_domain(
> +		group, dev, group->domain,
> +		group->owner ? 0 : IOMMU_SET_DOMAIN_WITH_DEFERRED);
>  }

This changes semantics in iommu_group_add_device().

Previously it always does deferred attach no matter whether the group
has been claimed.

While this change makes sense given deferred attach only applies to
default domain, it's worthy of an explanation or probably even made
into a separate patch.

Re: [PATCH 5/9] iommu: Make iommu_group_do_dma_first_attach() simpler

[Jason Gunthorpe]

On Fri, Mar 24, 2023 at 07:23:30AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 22, 2023 3:53 AM
> > 
> > -static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
> > +static int iommu_group_do_dma_first_attach(struct iommu_group *group,
> > struct device *dev)
> >  {
> > -	struct iommu_domain *domain = data;
> > -
> > -	lockdep_assert_held(&dev->iommu_group->mutex);
> > -
> > -	if (iommu_is_attach_deferred(dev)) {
> > -		dev->iommu->attach_deferred = 1;
> > -		return 0;
> > -	}
> > -
> > -	return __iommu_attach_device(domain, dev);
> > +	return __iommu_device_set_domain(
> > +		group, dev, group->domain,
> > +		group->owner ? 0 : IOMMU_SET_DOMAIN_WITH_DEFERRED);
> >  }
> 
> This changes semantics in iommu_group_add_device().
> 
> Previously it always does deferred attach no matter whether the group
> has been claimed.
> 
> While this change makes sense given deferred attach only applies to
> default domain, it's worthy of an explanation or probably even made
> into a separate patch.

I added another patch

Thanks,
Jason

[PATCH 6/9] iommu: Fix iommu_probe_device() to attach the right domain

[Jason Gunthorpe]

The general invariant is that all devices in an iommu_group are attached
to group->domain. We missed some cases here where an owned group would not
get the device attached.

Rework this logic so it follows the default domain flow of the
bus_iommu_probe() - call iommu_alloc_default_domain(), then use
__iommu_group_set_domain_internal() to set up all the devices.

Finally always attach the device to the current domain if it is already
set.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 45 ++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ea7700bbcf3399..d58570f361bb92 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -429,27 +429,32 @@ int iommu_probe_device(struct device *dev)
 		goto err_release;
 	}
 
-	/*
-	 * Try to allocate a default domain - needs support from the
-	 * IOMMU driver. There are still some drivers which don't
-	 * support default domains, so the return value is not yet
-	 * checked.
-	 */
 	mutex_lock(&group->mutex);
-	iommu_alloc_default_domain(group, dev);
 
-	/*
-	 * If device joined an existing group which has been claimed, don't
-	 * attach the default domain.
-	 */
-	if (group->default_domain && !group->owner) {
+	if (group->domain) {
 		ret = iommu_group_do_dma_first_attach(group, dev);
-		if (ret) {
-			mutex_unlock(&group->mutex);
-			iommu_group_put(group);
-			goto err_release;
-		}
+	} else if (!group->default_domain) {
+		/*
+		 * Try to allocate a default domain - needs support from the
+		 * IOMMU driver. There are still some drivers which don't
+		 * support default domains, so the return value is not yet
+		 * checked.
+		 */
+		iommu_alloc_default_domain(group, dev);
+		group->domain = NULL;
+		if (group->default_domain)
+			ret = __iommu_group_set_domain_internal(
+				group, group->default_domain,
+				IOMMU_SET_DOMAIN_WITH_DEFERRED);
+
+		/*
+		 * We assume that the iommu driver starts up the device in
+		 * 'set_platform_dma_ops' mode if it does not support default
+		 * domains.
+		 */
 	}
+	if (ret)
+		goto err_unlock;
 
 	iommu_create_device_direct_mappings(group, dev);
 
@@ -462,6 +467,9 @@ int iommu_probe_device(struct device *dev)
 
 	return 0;
 
+err_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
 err_release:
 	iommu_release_device(dev);
 
@@ -1676,9 +1684,6 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
 {
 	unsigned int type;
 
-	if (group->default_domain)
-		return 0;
-
 	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
 
 	return iommu_group_alloc_default_domain(dev->bus, group, type);
-- 
2.40.0

RE: [PATCH 6/9] iommu: Fix iommu_probe_device() to attach the right domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> 
> -	/*
> -	 * If device joined an existing group which has been claimed, don't
> -	 * attach the default domain.
> -	 */
> -	if (group->default_domain && !group->owner) {
> +	if (group->domain) {
>  		ret = iommu_group_do_dma_first_attach(group, dev);

emm this duplicates with the attach in iommu_group_add_device().

It sounds cleaner to remove it from iommu_group_add_device() and just
do attach consistently in bus_iommu_probe() and iommu_probe_device().

Re: [PATCH 6/9] iommu: Fix iommu_probe_device() to attach the right domain

[Jason Gunthorpe]

On Fri, Mar 24, 2023 at 07:29:59AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 22, 2023 3:53 AM
> > 
> > -	/*
> > -	 * If device joined an existing group which has been claimed, don't
> > -	 * attach the default domain.
> > -	 */
> > -	if (group->default_domain && !group->owner) {
> > +	if (group->domain) {
> >  		ret = iommu_group_do_dma_first_attach(group, dev);
> 
> emm this duplicates with the attach in iommu_group_add_device().

Yes, it looks like that code in iommu_group_add_device() is
nonsensical.

I added a patch to remove it, and maybe a few more patches to make
this less duplicated..

Thanks,
Jason

Re: [PATCH 6/9] iommu: Fix iommu_probe_device() to attach the right domain

[Jason Gunthorpe]

On Fri, Mar 24, 2023 at 07:29:59AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 22, 2023 3:53 AM
> > 
> > -	/*
> > -	 * If device joined an existing group which has been claimed, don't
> > -	 * attach the default domain.
> > -	 */
> > -	if (group->default_domain && !group->owner) {
> > +	if (group->domain) {
> >  		ret = iommu_group_do_dma_first_attach(group, dev);
> 
> emm this duplicates with the attach in iommu_group_add_device().
> 
> It sounds cleaner to remove it from iommu_group_add_device() and just
> do attach consistently in bus_iommu_probe() and iommu_probe_device().

This is what I came up with, I will keep it as a followup series:

https://github.com/jgunthorpe/linux/commits/iommu_err_unwind

Jason

[PATCH 7/9] iommu: Remove the assignment of group->domain during default domain alloc

[Jason Gunthorpe]

group->domain should only be set once all the device's drivers have
had their ops->attach_dev() called. iommu_group_alloc_default_domain()
doesn't do this, so it shouldn't set the value.

The previous patches organized things so that each caller of
iommu_group_alloc_default_domain() follows up with calling
__iommu_group_set_domain_internal() that does set the group->domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d58570f361bb92..9973d1eac18cc9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -441,7 +441,6 @@ int iommu_probe_device(struct device *dev)
 		 * checked.
 		 */
 		iommu_alloc_default_domain(group, dev);
-		group->domain = NULL;
 		if (group->default_domain)
 			ret = __iommu_group_set_domain_internal(
 				group, group->default_domain,
@@ -1674,8 +1673,6 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus,
 		return -ENOMEM;
 
 	group->default_domain = dom;
-	if (!group->domain)
-		group->domain = dom;
 	return 0;
 }
 
@@ -1879,7 +1876,6 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		group->domain = NULL;
 		ret = __iommu_group_set_domain_internal(
 			group, group->default_domain,
 			IOMMU_SET_DOMAIN_WITH_DEFERRED);
-- 
2.40.0

RE: [PATCH 7/9] iommu: Remove the assignment of group->domain during default domain alloc

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 22, 2023 3:53 AM
> 
> group->domain should only be set once all the device's drivers have
> had their ops->attach_dev() called. iommu_group_alloc_default_domain()
> doesn't do this, so it shouldn't set the value.
> 
> The previous patches organized things so that each caller of
> iommu_group_alloc_default_domain() follows up with calling
> __iommu_group_set_domain_internal() that does set the group->domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH 8/9] iommu: Consolidate the default_domain setup to one function

[Jason Gunthorpe]

The default_domain setup flow requires
 - Determining the default type
 - Allocating a default_domain
 - Attaching it to devices
 - Doing iommu_create_device_direct_mappings()

This is sprinkled around three places written in two different ways. Make
it all the same in one function.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 179 +++++++++++++++++-------------------------
 1 file changed, 71 insertions(+), 108 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9973d1eac18cc9..e129f55587def2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -88,8 +88,6 @@ static const char * const iommu_group_resv_type_string[] = {
 
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
-static int iommu_alloc_default_domain(struct iommu_group *group,
-				      struct device *dev);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
@@ -121,6 +119,7 @@ static void __iommu_group_set_domain_nofail(struct iommu_group *group,
 		group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED));
 }
 
+static int iommu_setup_default_domain(struct iommu_group *group);
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -433,29 +432,14 @@ int iommu_probe_device(struct device *dev)
 
 	if (group->domain) {
 		ret = iommu_group_do_dma_first_attach(group, dev);
+		if (ret)
+			goto err_unlock;
+		iommu_create_device_direct_mappings(group, dev);
 	} else if (!group->default_domain) {
-		/*
-		 * Try to allocate a default domain - needs support from the
-		 * IOMMU driver. There are still some drivers which don't
-		 * support default domains, so the return value is not yet
-		 * checked.
-		 */
-		iommu_alloc_default_domain(group, dev);
-		if (group->default_domain)
-			ret = __iommu_group_set_domain_internal(
-				group, group->default_domain,
-				IOMMU_SET_DOMAIN_WITH_DEFERRED);
-
-		/*
-		 * We assume that the iommu driver starts up the device in
-		 * 'set_platform_dma_ops' mode if it does not support default
-		 * domains.
-		 */
+		ret = iommu_setup_default_domain(group);
+		if (ret)
+			goto err_unlock;
 	}
-	if (ret)
-		goto err_unlock;
-
-	iommu_create_device_direct_mappings(group, dev);
 
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
@@ -1655,37 +1639,6 @@ static int iommu_get_def_domain_type(struct device *dev)
 	return 0;
 }
 
-static int iommu_group_alloc_default_domain(struct bus_type *bus,
-					    struct iommu_group *group,
-					    unsigned int type)
-{
-	struct iommu_domain *dom;
-
-	dom = __iommu_domain_alloc(bus, type);
-	if (!dom && type != IOMMU_DOMAIN_DMA) {
-		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
-		if (dom)
-			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
-				type, group->name);
-	}
-
-	if (!dom)
-		return -ENOMEM;
-
-	group->default_domain = dom;
-	return 0;
-}
-
-static int iommu_alloc_default_domain(struct iommu_group *group,
-				      struct device *dev)
-{
-	unsigned int type;
-
-	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
-
-	return iommu_group_alloc_default_domain(dev->bus, group, type);
-}
-
 /**
  * iommu_group_get_for_dev - Find or create the IOMMU group for a device
  * @dev: target device
@@ -1771,6 +1724,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 struct __group_domain_type {
 	struct device *dev;
 	unsigned int type;
+	unsigned int count;
 };
 
 static int probe_get_default_domain_type(struct device *dev, void *data)
@@ -1778,6 +1732,10 @@ static int probe_get_default_domain_type(struct device *dev, void *data)
 	struct __group_domain_type *gtype = data;
 	unsigned int type = iommu_get_def_domain_type(dev);
 
+	gtype->count++;
+	if (!gtype->dev)
+		gtype->dev = dev;
+
 	if (type) {
 		if (gtype->type && gtype->type != type) {
 			dev_warn(dev, "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
@@ -1796,12 +1754,18 @@ static int probe_get_default_domain_type(struct device *dev, void *data)
 	return 0;
 }
 
-static void probe_alloc_default_domain(struct bus_type *bus,
-				       struct iommu_group *group)
+static int iommu_setup_default_domain(struct iommu_group *group)
 {
-	struct __group_domain_type gtype;
+	struct __group_domain_type gtype = {};
+	struct group_device *gdev;
+	struct iommu_domain *dom;
+	struct bus_type *bus;
+	int ret;
+
+	lockdep_assert_held(&group->mutex);
 
-	memset(&gtype, 0, sizeof(gtype));
+	if (group->default_domain)
+		return 0;
 
 	/* Ask for default domain requirements of all devices in the group */
 	__iommu_group_for_each_dev(group, &gtype,
@@ -1810,8 +1774,48 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 	if (!gtype.type)
 		gtype.type = iommu_def_domain_type;
 
-	iommu_group_alloc_default_domain(bus, group, gtype.type);
+	bus = gtype.dev->bus;
+	dom = __iommu_domain_alloc(bus, gtype.type);
+	if (!dom && gtype.type != IOMMU_DOMAIN_DMA) {
+		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
+		if (dom)
+			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
+				gtype.type, group->name);
+	}
 
+	/*
+	 * There are still some drivers which don't support default domains, so
+	 * we ignore the failure and leave group->default_domain NULL.
+	 *
+	 * We assume that the iommu driver starts up the device in
+	 * 'set_platform_dma_ops' mode if it does not support default domains.
+	 */
+	if (!dom)
+		return 0;
+
+	ret = __iommu_group_set_domain_internal(group, dom,
+						IOMMU_SET_DOMAIN_WITH_DEFERRED);
+	if (ret) {
+		/*
+		 * An attach_dev failure may result in some devices being left
+		 * attached to dom. This is not cleaned up until release_device
+		 * is called. Thus we can't always free dom on failure, we have
+		 * no choice but to stick the broken domain into
+		 * group->default_domain to defer the free and try to continue.
+		 */
+		if (gtype.count > 1)
+			group->default_domain = dom;
+		else
+			iommu_domain_free(dom);
+		return ret;
+	}
+
+	group->default_domain = dom;
+
+	/* The domain must be attached before we can establish any mappings */
+	list_for_each_entry(gdev, &group->devices, list)
+		iommu_create_device_direct_mappings(group, gdev->dev);
+	return 0;
 }
 
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
@@ -1830,21 +1834,6 @@ static void __iommu_group_dma_finalize(struct iommu_group *group)
 				   iommu_group_do_probe_finalize);
 }
 
-static int iommu_do_create_direct_mappings(struct device *dev, void *data)
-{
-	struct iommu_group *group = data;
-
-	iommu_create_device_direct_mappings(group, dev);
-
-	return 0;
-}
-
-static int iommu_group_create_direct_mappings(struct iommu_group *group)
-{
-	return __iommu_group_for_each_dev(group, group,
-					  iommu_do_create_direct_mappings);
-}
-
 int bus_iommu_probe(struct bus_type *bus)
 {
 	struct iommu_group *group, *next;
@@ -1866,29 +1855,14 @@ int bus_iommu_probe(struct bus_type *bus)
 		/* Remove item from the list */
 		list_del_init(&group->entry);
 
-		/* Try to allocate default domain */
-		probe_alloc_default_domain(bus, group);
-
-		if (!group->default_domain) {
-			mutex_unlock(&group->mutex);
-			continue;
-		}
-
-		iommu_group_create_direct_mappings(group);
-
-		ret = __iommu_group_set_domain_internal(
-			group, group->default_domain,
-			IOMMU_SET_DOMAIN_WITH_DEFERRED);
-
+		ret = iommu_setup_default_domain(group);
 		mutex_unlock(&group->mutex);
-
 		if (ret)
-			break;
-
+			return ret;
 		__iommu_group_dma_finalize(group);
 	}
 
-	return ret;
+	return 0;
 }
 
 bool iommu_present(struct bus_type *bus)
@@ -3002,19 +2976,12 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 		goto out;
 	}
 
-	/* Sets group->default_domain to the newly allocated domain */
-	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
-	if (ret)
+	group->default_domain = NULL;
+	ret = iommu_setup_default_domain(group);
+	if (ret) {
+		group->default_domain = prev_dom;
 		goto out;
-
-	group->domain = prev_dom;
-	ret = iommu_create_device_direct_mappings(group, dev);
-	if (ret)
-		goto free_new_domain;
-
-	ret = __iommu_group_set_domain(group, group->default_domain);
-	if (ret)
-		goto free_new_domain;
+	}
 
 	/*
 	 * Release the mutex here because ops->probe_finalize() call-back of
@@ -3029,10 +2996,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	iommu_domain_free(prev_dom);
 	return 0;
 
-free_new_domain:
-	iommu_domain_free(group->default_domain);
-	group->default_domain = prev_dom;
-
 out:
 	mutex_unlock(&group->mutex);
 
-- 
2.40.0

[PATCH 9/9] iommu: Remove __iommu_group_for_each_dev()

[Jason Gunthorpe]

The last two users of it are quite trivial, just open code the one line
loop.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 55 +++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e129f55587def2..bb6140067353e7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1119,20 +1119,6 @@ static int iommu_group_device_count(struct iommu_group *group)
 	return ret;
 }
 
-static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
-				      int (*fn)(struct device *, void *))
-{
-	struct group_device *device;
-	int ret = 0;
-
-	list_for_each_entry(device, &group->devices, list) {
-		ret = fn(device->dev, data);
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
 /**
  * iommu_group_for_each_dev - iterate over each device in the group
  * @group: the group
@@ -1147,10 +1133,15 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
 int iommu_group_for_each_dev(struct iommu_group *group, void *data,
 			     int (*fn)(struct device *, void *))
 {
-	int ret;
+	struct group_device *device;
+	int ret = 0;
 
 	mutex_lock(&group->mutex);
-	ret = __iommu_group_for_each_dev(group, data, fn);
+	list_for_each_entry(device, &group->devices, list) {
+		ret = fn(device->dev, data);
+		if (ret)
+			break;
+	}
 	mutex_unlock(&group->mutex);
 
 	return ret;
@@ -1727,9 +1718,9 @@ struct __group_domain_type {
 	unsigned int count;
 };
 
-static int probe_get_default_domain_type(struct device *dev, void *data)
+static int probe_get_default_domain_type(struct device *dev,
+					 struct __group_domain_type *gtype)
 {
-	struct __group_domain_type *gtype = data;
 	unsigned int type = iommu_get_def_domain_type(dev);
 
 	gtype->count++;
@@ -1768,8 +1759,8 @@ static int iommu_setup_default_domain(struct iommu_group *group)
 		return 0;
 
 	/* Ask for default domain requirements of all devices in the group */
-	__iommu_group_for_each_dev(group, &gtype,
-				   probe_get_default_domain_type);
+	list_for_each_entry(gdev, &group->devices, list)
+		probe_get_default_domain_type(gdev->dev, &gtype);
 
 	if (!gtype.type)
 		gtype.type = iommu_def_domain_type;
@@ -1818,20 +1809,12 @@ static int iommu_setup_default_domain(struct iommu_group *group)
 	return 0;
 }
 
-static int iommu_group_do_probe_finalize(struct device *dev, void *data)
+static void iommu_group_do_probe_finalize(struct device *dev)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
-
-	return 0;
-}
-
-static void __iommu_group_dma_finalize(struct iommu_group *group)
-{
-	__iommu_group_for_each_dev(group, group->default_domain,
-				   iommu_group_do_probe_finalize);
 }
 
 int bus_iommu_probe(struct bus_type *bus)
@@ -1850,6 +1833,8 @@ int bus_iommu_probe(struct bus_type *bus)
 		return ret;
 
 	list_for_each_entry_safe(group, next, &group_list, entry) {
+		struct group_device *gdev;
+
 		mutex_lock(&group->mutex);
 
 		/* Remove item from the list */
@@ -1859,7 +1844,15 @@ int bus_iommu_probe(struct bus_type *bus)
 		mutex_unlock(&group->mutex);
 		if (ret)
 			return ret;
-		__iommu_group_dma_finalize(group);
+
+		/*
+		 * Mis-locked because the ops->probe_finalize() call-back of
+		 * some 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.
+		 */
+		list_for_each_entry(gdev, &group->devices, list)
+			iommu_group_do_probe_finalize(gdev->dev);
 	}
 
 	return 0;
@@ -2992,7 +2985,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	mutex_unlock(&group->mutex);
 
 	/* Make sure dma_ops is appropriatley set */
-	iommu_group_do_probe_finalize(dev, group->default_domain);
+	iommu_group_do_probe_finalize(dev);
 	iommu_domain_free(prev_dom);
 	return 0;
 
-- 
2.40.0

Re: [PATCH 9/9] iommu: Remove __iommu_group_for_each_dev()

[Joerg Roedel]

Hi Jason,

I like the general direction of this. Just a cosmetic nit below:

On Tue, Mar 21, 2023 at 04:53:21PM -0300, Jason Gunthorpe wrote:
> -	__iommu_group_for_each_dev(group, &gtype,
> -				   probe_get_default_domain_type);
> +	list_for_each_entry(gdev, &group->devices, list)
> +		probe_get_default_domain_type(gdev->dev, &gtype);

Can you introduce a macro instead? Something like for_each_group_dev()
or similar. This makes it easier to read in the future.

Regards,

	Joerg

Re: [PATCH 9/9] iommu: Remove __iommu_group_for_each_dev()

[Jason Gunthorpe]

On Wed, Mar 22, 2023 at 02:08:02PM +0100, Joerg Roedel wrote:
> Hi Jason,
> 
> I like the general direction of this. Just a cosmetic nit below:

Great, it would be good to get Lu's default domain series merged as
this needs to be rebased on top of it

> On Tue, Mar 21, 2023 at 04:53:21PM -0300, Jason Gunthorpe wrote:
> > -	__iommu_group_for_each_dev(group, &gtype,
> > -				   probe_get_default_domain_type);
> > +	list_for_each_entry(gdev, &group->devices, list)
> > +		probe_get_default_domain_type(gdev->dev, &gtype);
> 
> Can you introduce a macro instead? Something like for_each_group_dev()
> or similar. This makes it easier to read in the future.

Yeah, that is a good idea, I'll fold it into the v2

Jason