Re: [PATCH v3 06/17] iommu: Replace __iommu_group_dma_first_attach() with set_domain

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 06, 2023 at 04:58:27PM +0100, Robin Murphy wrote:
> On 06/04/2023 12:38 am, Jason Gunthorpe wrote:
> > 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.
> 
> This is way overcomplicated; if we evaluate iommu_is_attach_deferred()
> earlier in the probe_device path, then the rest of the handling at the point
> of attach should simply reduce to:
> 
> 	if (dev->iommu->attach_deferred && iommu_is_dma_domain(domain))
> 		return 0;

Ok, that seems to work without too much trouble elsewhere.
deferred domains currently work with IDENTITY domains too so the
iommu_is_dma_domain() needed a fix

Against the whole series it looks like this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c72aa3e81aae50..c5ed2d27a2da05 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -100,7 +100,6 @@ 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_device_set_domain(struct iommu_group *group,
@@ -365,6 +364,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 
 	dev->iommu->iommu_dev = iommu_dev;
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
+	if (ops->is_attach_deferred)
+		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
 
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group)) {
@@ -399,23 +400,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	return ret;
 }
 
-static bool iommu_is_attach_deferred(struct device *dev)
-{
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-	if (ops->is_attach_deferred)
-		return ops->is_attach_deferred(dev);
-
-	return false;
-}
-
-static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *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)
 {
 	const struct iommu_ops *ops;
@@ -434,9 +418,11 @@ int iommu_probe_device(struct device *dev)
 
 	mutex_lock(&group->mutex);
 
+	if (group->default_domain)
+		iommu_create_device_direct_mappings(group->default_domain, dev);
+
 	if (group->domain) {
-		iommu_create_device_direct_mappings(group->domain, dev);
-		ret = iommu_group_do_dma_first_attach(group, dev);
+		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
 		if (ret)
 			goto err_unlock;
 	} else if (!group->default_domain) {
@@ -1104,25 +1090,13 @@ 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(group, dev);
 	mutex_unlock(&group->mutex);
-	if (ret)
-		goto err_put_group;
-
 	trace_add_device_to_group(group->id, dev);
 
 	dev_info(dev, "Adding to iommu group %d\n", group->id);
 
 	return 0;
 
-err_put_group:
-	mutex_lock(&group->mutex);
-	list_del(&device->list);
-	mutex_unlock(&group->mutex);
-	dev->iommu_group = NULL;
-	kobject_put(group->devices_kobj);
-	sysfs_remove_link(group->devices_kobj, device->name);
 err_free_name:
 	kfree(device->name);
 err_remove_link:
@@ -2165,10 +2139,10 @@ 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;
+	if (dev->iommu->attach_deferred) {
+		if (new_domain == group->default_domain)
+			return 0;
+		dev->iommu->attach_deferred = 0;
 	}
 
 	ret = __iommu_attach_device(new_domain, dev);
@@ -2875,6 +2849,7 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 static int iommu_setup_default_domain(struct iommu_group *group,
 				      int target_type)
 {
+	struct iommu_domain *old_dom = group->default_domain;
 	struct group_device *gdev;
 	struct iommu_domain *dom;
 	bool direct_failed;
@@ -2899,8 +2874,8 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 		/* Once in default_domain mode we never leave */
 		if (group->default_domain)
 			return -ENODEV;
-		ret = 0;
-		goto out_set;
+		group->default_domain = NULL;
+		return 0;
 	}
 
 	if (group->default_domain == dom)
@@ -2917,8 +2892,10 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 			direct_failed = true;
 	}
 
-	ret = __iommu_group_set_domain_internal(group, dom,
-						IOMMU_SET_DOMAIN_WITH_DEFERRED);
+	/* We must set default_domain early for __iommu_device_set_domain */
+	group->default_domain = dom;
+	ret = __iommu_group_set_domain_internal(
+		group, dom, group->domain ? 0 : IOMMU_SET_DOMAIN_MUST_SUCCEED);
 	if (ret) {
 		/*
 		 * An attach_dev failure may result in some devices being left
@@ -2927,12 +2904,16 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 		 * no choice but to stick the broken domain into
 		 * group->default_domain to defer the free and try to continue.
 		 */
-		if (list_count_nodes(&group->devices) > 1)
+		if (!group->domain)
 			goto out_set;
 
+		/*
+		 * Otherwise assume that cleanup worked and the devices were put
+		 * back to old_dom.
+		 */
 		iommu_domain_free(dom);
-		dom = NULL;
-		goto out_set;
+		group->default_domain = old_dom;
+		return ret;
 	}
 
 	/*
@@ -2947,8 +2928,8 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	}
 
 out_set:
-	if (group->default_domain)
-		iommu_domain_free(group->default_domain);
+	if (old_dom)
+		iommu_domain_free(old_dom);
 	group->default_domain = dom;
 	return ret;
 }