[PATCH v1 0/8] Scrap iommu_attach/detach_group() interfaces

[PATCH v1 0/8] Scrap iommu_attach/detach_group() interfaces

[Lu Baolu]

Hi folks,

The iommu_attach_device() added first by commit <fc2100eb4d096> ("add
frontend implementation for the IOMMU API") in 2008. At that time,
there was no concept of iommu group yet.

The iommu group was added by commit <d72e31c937462> ("iommu: IOMMU
Groups") four years later in 2012. The iommu_attach_group() was added
at the same time.

Then, people realized that iommu_attach_device() allowed different
device in a same group to attach different domain. This was not in
line with the concept of iommu group. The commit <426a273834eae>
("iommu: Limit iommu_attach/detach_device to device with their own
group") fixed this problem in 2015.

As the result, we have two coexisting interfaces for device drivers
to do the same thing. But neither is perfect:

  - iommu_attach_device() only works for singleton group.
  - iommu_attach_group() asks the device drivers to handle iommu group
    related staff which is beyond the role of a device driver.

Considering from the perspective of a device driver, its motivation is
very simple: "I want to manage my own I/O address space." Inspired by
the discussion [1], we consider heading in this direction:

Make the iommu_attach_device() the only and generic interface for the
device drivers to use their own private domain (I/O address space)
and replace all iommu_attach_group() uses with iommu_attach_device()
and deprecate the former.

This is a follow-up series of this discussion: 
[1] https://lore.kernel.org/linux-iommu/b4405a5e-c4cc-f44a-ab43-8cb62b888565@linux.intel.com/

It depends on the series of "Fix BUG_ON in vfio_iommu_group_notifier()".
The latest version was posted here:
https://lore.kernel.org/linux-iommu/20220104015644.2294354-1-baolu.lu@linux.intel.com/

and the whole patches are available on github:
https://github.com/LuBaolu/intel-iommu/commits/iommu-domain-attach-refactor-v1

Best regards,
baolu

Jason Gunthorpe (1):
  drm/tegra: Use iommu_attach/detatch_device()

Lu Baolu (7):
  iommu: Add iommu_group_replace_domain()
  vfio/type1: Use iommu_group_replace_domain()
  iommu: Extend iommu_at[de]tach_device() for multi-device groups
  iommu/amd: Use iommu_attach/detach_device()
  gpu/host1x: Use iommu_attach/detach_device()
  media: staging: media: tegra-vde: Use iommu_attach/detach_device()
  iommu: Remove iommu_attach/detach_group()

 include/linux/iommu.h                   |  25 ++---
 drivers/gpu/drm/tegra/dc.c              |   1 +
 drivers/gpu/drm/tegra/drm.c             |  47 +++-----
 drivers/gpu/drm/tegra/gr2d.c            |   1 +
 drivers/gpu/drm/tegra/gr3d.c            |   1 +
 drivers/gpu/drm/tegra/vic.c             |   1 +
 drivers/gpu/host1x/dev.c                |   4 +-
 drivers/iommu/amd/iommu_v2.c            |   4 +-
 drivers/iommu/iommu.c                   | 136 +++++++++++++++++-------
 drivers/staging/media/tegra-vde/iommu.c |   6 +-
 drivers/vfio/vfio_iommu_type1.c         |  22 ++--
 11 files changed, 146 insertions(+), 102 deletions(-)

-- 
2.25.1

[PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Lu Baolu]

Expose an interface to replace the domain of an iommu group for frameworks
like vfio which claims the ownership of the whole iommu group.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 10 ++++++++++
 drivers/iommu/iommu.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 408a6d2b3034..66ebce3d1e11 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -677,6 +677,9 @@ void iommu_device_unuse_dma_api(struct device *dev);
 int iommu_group_set_dma_owner(struct iommu_group *group, void *owner);
 void iommu_group_release_dma_owner(struct iommu_group *group);
 bool iommu_group_dma_owner_claimed(struct iommu_group *group);
+int iommu_group_replace_domain(struct iommu_group *group,
+			       struct iommu_domain *old,
+			       struct iommu_domain *new);
 
 #else /* CONFIG_IOMMU_API */
 
@@ -1090,6 +1093,13 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 {
 	return false;
 }
+
+static inline int
+iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *old,
+			   struct iommu_domain *new)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_IOMMU_API */
 
 /**
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 72a95dea688e..ab8ab95969f5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3431,3 +3431,40 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 	return user;
 }
 EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
+
+/**
+ * iommu_group_replace_domain() - Replace group's domain
+ * @group: The group.
+ * @old: The previous attached domain. NULL for none.
+ * @new: The new domain about to be attached.
+ *
+ * This is to support backward compatibility for vfio which manages the dma
+ * ownership in iommu_group level.
+ */
+int iommu_group_replace_domain(struct iommu_group *group,
+			       struct iommu_domain *old,
+			       struct iommu_domain *new)
+{
+	int ret = 0;
+
+	mutex_lock(&group->mutex);
+	if (!group->owner || group->domain != old) {
+		ret = -EPERM;
+		goto unlock_out;
+	}
+
+	if (old)
+		__iommu_detach_group(old, group);
+
+	if (new) {
+		ret = __iommu_attach_group(new, group);
+		if (ret && old)
+			__iommu_attach_group(old, group);
+	}
+
+unlock_out:
+	mutex_unlock(&group->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_group_replace_domain);
-- 
2.25.1

[PATCH v1 2/8] vfio/type1: Use iommu_group_replace_domain()

[Lu Baolu]

After an IOMMU group is placed in a vfio container, the domain attachment
may be deferred. During this process, other kernel modules can attach
another domain simply in the following way:

	group = iommu_group_get(dev);
	iommu_attach_group(domain, group);

Replace the iommu_attach/detach_group() with iommu_group_replace_domain()
and prohibit use of iommu_attach/detach_group() in other kernel drivers
can solve this problem.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/vfio/vfio_iommu_type1.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f17490ab238f..25276a5db737 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2213,7 +2213,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			goto out_domain;
 	}
 
-	ret = iommu_attach_group(domain->domain, group->iommu_group);
+	ret = iommu_group_replace_domain(group->iommu_group, NULL,
+					 domain->domain);
 	if (ret)
 		goto out_domain;
 
@@ -2280,19 +2281,14 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	list_for_each_entry(d, &iommu->domain_list, next) {
 		if (d->domain->ops == domain->domain->ops &&
 		    d->prot == domain->prot) {
-			iommu_detach_group(domain->domain, group->iommu_group);
-			if (!iommu_attach_group(d->domain,
-						group->iommu_group)) {
+			if (!iommu_group_replace_domain(group->iommu_group,
+							domain->domain,
+							d->domain)) {
 				list_add(&group->next, &d->group_list);
 				iommu_domain_free(domain->domain);
 				kfree(domain);
 				goto done;
 			}
-
-			ret = iommu_attach_group(domain->domain,
-						 group->iommu_group);
-			if (ret)
-				goto out_domain;
 		}
 	}
 
@@ -2327,7 +2323,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	return 0;
 
 out_detach:
-	iommu_detach_group(domain->domain, group->iommu_group);
+	iommu_group_replace_domain(group->iommu_group, domain->domain, NULL);
 out_domain:
 	iommu_domain_free(domain->domain);
 	vfio_iommu_iova_free(&iova_copy);
@@ -2488,7 +2484,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 		if (!group)
 			continue;
 
-		iommu_detach_group(domain->domain, group->iommu_group);
+		iommu_group_replace_domain(group->iommu_group,
+					   domain->domain, NULL);
 		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
 		kfree(group);
@@ -2577,7 +2574,8 @@ static void vfio_release_domain(struct vfio_domain *domain)
 
 	list_for_each_entry_safe(group, group_tmp,
 				 &domain->group_list, next) {
-		iommu_detach_group(domain->domain, group->iommu_group);
+		iommu_group_replace_domain(group->iommu_group,
+					   domain->domain, NULL);
 		list_del(&group->next);
 		kfree(group);
 	}
-- 
2.25.1

[PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Lu Baolu]

The iommu_attach/detach_device() interfaces were exposed for the device
drivers to attach/detach their own domains. The commit <426a273834eae>
("iommu: Limit iommu_attach/detach_device to device with their own group")
restricted them to singleton groups to avoid different device in a group
attaching different domain.

As we've introduced device DMA ownership into the iommu core. We can now
extend these interfaces for muliple-device groups, and "all devices are in
the same address space" is still guaranteed.

For multiple devices belonging to a same group, iommu_device_use_dma_api()
and iommu_attach_device() are exclusive. Therefore, when drivers decide to
use iommu_attach_domain(), they cannot call iommu_device_use_dma_api() at
the same time.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ab8ab95969f5..2c9efd85e447 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -47,6 +47,7 @@ struct iommu_group {
 	struct iommu_domain *domain;
 	struct list_head entry;
 	unsigned int owner_cnt;
+	unsigned int attach_cnt;
 	void *owner;
 };
 
@@ -1921,27 +1922,59 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_device() - attach external or UNMANAGED domain to device
+ * @domain: the domain about to attach
+ * @dev: the device about to be attached
+ *
+ * For devices belonging to the same group, iommu_device_use_dma_api() and
+ * iommu_attach_device() are exclusive. Therefore, when drivers decide to
+ * use iommu_attach_domain(), they cannot call iommu_device_use_dma_api()
+ * at the same time.
+ */
 int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
-	int ret;
+	int ret = 0;
+
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
 
 	group = iommu_group_get(dev);
 	if (!group)
 		return -ENODEV;
 
-	/*
-	 * Lock the group to make sure the device-count doesn't
-	 * change while we are attaching
-	 */
 	mutex_lock(&group->mutex);
-	ret = -EINVAL;
-	if (iommu_group_device_count(group) != 1)
-		goto out_unlock;
+	if (group->owner_cnt) {
+		/*
+		 * Group has been used for kernel-api dma or claimed explicitly
+		 * for exclusive occupation. For backward compatibility, device
+		 * in a singleton group is allowed to ignore setting the
+		 * drv.no_kernel_api_dma field.
+		 */
+		if ((group->domain == group->default_domain &&
+		     iommu_group_device_count(group) != 1) ||
+		    group->owner) {
+			ret = -EBUSY;
+			goto unlock_out;
+		}
+	}
 
-	ret = __iommu_attach_group(domain, group);
+	if (!group->attach_cnt) {
+		ret = __iommu_attach_group(domain, group);
+		if (ret)
+			goto unlock_out;
+	} else {
+		if (group->domain != domain) {
+			ret = -EPERM;
+			goto unlock_out;
+		}
+	}
 
-out_unlock:
+	group->owner_cnt++;
+	group->attach_cnt++;
+
+unlock_out:
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
 
@@ -2182,23 +2215,35 @@ static void __iommu_detach_device(struct iommu_domain *domain,
 	trace_detach_device_from_domain(dev);
 }
 
+/**
+ * iommu_detach_device() - detach external or UNMANAGED domain from device
+ * @domain: the domain about to detach
+ * @dev: the device about to be detached
+ *
+ * Paired with iommu_attach_device(), it detaches the domain from the device.
+ */
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
 
+	if (WARN_ON(domain->type != IOMMU_DOMAIN_UNMANAGED))
+		return;
+
 	group = iommu_group_get(dev);
-	if (!group)
+	if (WARN_ON(!group))
 		return;
 
 	mutex_lock(&group->mutex);
-	if (iommu_group_device_count(group) != 1) {
-		WARN_ON(1);
-		goto out_unlock;
-	}
+	if (WARN_ON(!group->attach_cnt || !group->owner_cnt ||
+		    group->domain != domain))
+		goto unlock_out;
 
-	__iommu_detach_group(domain, group);
+	group->attach_cnt--;
+	group->owner_cnt--;
+	if (!group->attach_cnt)
+		__iommu_detach_group(domain, group);
 
-out_unlock:
+unlock_out:
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
 }
-- 
2.25.1

[PATCH v1 4/8] drm/tegra: Use iommu_attach/detatch_device()

[Lu Baolu]

From: Jason Gunthorpe <jgg@nvidia.com>

Tegra joins many platform devices onto the same iommu_domain and builds
sort-of a DMA API on top of it.

Given that iommu_attach/detatch_device() has supported this usage model,
each device that wants to use the special domain could set the
no_kernel_api_dma field of the driver structure and call
iommu_attach/detach_device() directly which will use dma owner framework
to lock out other usages of the group and refcount the domain attachment.

When the last device calls detatch the domain will be disconnected.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/gpu/drm/tegra/dc.c   |  1 +
 drivers/gpu/drm/tegra/drm.c  | 47 ++++++++++++------------------------
 drivers/gpu/drm/tegra/gr2d.c |  1 +
 drivers/gpu/drm/tegra/gr3d.c |  1 +
 drivers/gpu/drm/tegra/vic.c  |  1 +
 5 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a29d64f87563..0da50b39ce00 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -3111,4 +3111,5 @@ struct platform_driver tegra_dc_driver = {
 	},
 	.probe = tegra_dc_probe,
 	.remove = tegra_dc_remove,
+	.no_kernel_api_dma = true,
 };
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 8d37d6b00562..d6c57a40b772 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -928,12 +928,15 @@ int tegra_drm_unregister_client(struct tegra_drm *tegra,
 	return 0;
 }
 
+/*
+ * Clients which use this function must set suppress_auto_claim_dma_owner in
+ * their platform_driver's device_driver struct.
+ */
 int host1x_client_iommu_attach(struct host1x_client *client)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(client->dev);
 	struct drm_device *drm = dev_get_drvdata(client->host);
 	struct tegra_drm *tegra = drm->dev_private;
-	struct iommu_group *group = NULL;
 	int err;
 
 	/*
@@ -941,48 +944,30 @@ int host1x_client_iommu_attach(struct host1x_client *client)
 	 * not the shared IOMMU domain, don't try to attach it to a different
 	 * domain. This allows using the IOMMU-backed DMA API.
 	 */
-	if (domain && domain != tegra->domain)
-		return 0;
-
-	if (tegra->domain) {
-		group = iommu_group_get(client->dev);
-		if (!group)
-			return -ENODEV;
-
-		if (domain != tegra->domain) {
-			err = iommu_attach_group(tegra->domain, group);
-			if (err < 0) {
-				iommu_group_put(group);
-				return err;
-			}
-		}
-
-		tegra->use_explicit_iommu = true;
-	}
+	client->group = NULL;
+	if (!client->dev->iommu_group || (domain && domain != tegra->domain))
+		return iommu_device_use_dma_api(client->dev);
 
-	client->group = group;
+	err = iommu_attach_device(tegra->domain, client->dev);
+	if (err)
+		return err;
 
+	tegra->use_explicit_iommu = true;
+	client->group = client->dev->iommu_group;
 	return 0;
 }
 
 void host1x_client_iommu_detach(struct host1x_client *client)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(client->dev);
 	struct drm_device *drm = dev_get_drvdata(client->host);
 	struct tegra_drm *tegra = drm->dev_private;
-	struct iommu_domain *domain;
 
 	if (client->group) {
-		/*
-		 * Devices that are part of the same group may no longer be
-		 * attached to a domain at this point because their group may
-		 * have been detached by an earlier client.
-		 */
-		domain = iommu_get_domain_for_dev(client->dev);
-		if (domain)
-			iommu_detach_group(tegra->domain, client->group);
-
-		iommu_group_put(client->group);
+		iommu_detach_device(tegra->domain, client->dev);
 		client->group = NULL;
+	} else {
+		iommu_device_unuse_dma_api(client->dev);
 	}
 }
 
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index de288cba3905..6fd502c1cdbf 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -271,4 +271,5 @@ struct platform_driver tegra_gr2d_driver = {
 	},
 	.probe = gr2d_probe,
 	.remove = gr2d_remove,
+	.no_kernel_api_dma = true,
 };
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 24442ade0da3..d47794b6394f 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -400,4 +400,5 @@ struct platform_driver tegra_gr3d_driver = {
 	},
 	.probe = gr3d_probe,
 	.remove = gr3d_remove,
+	.no_kernel_api_dma = true,
 };
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
index c02010ff2b7f..167de5509c3b 100644
--- a/drivers/gpu/drm/tegra/vic.c
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -527,6 +527,7 @@ struct platform_driver tegra_vic_driver = {
 	},
 	.probe = vic_probe,
 	.remove = vic_remove,
+	.no_kernel_api_dma = true,
 };
 
 #if IS_ENABLED(CONFIG_ARCH_TEGRA_124_SOC)
-- 
2.25.1

[PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Lu Baolu]

The individual device driver should use iommu_attach/detach_device()
for domain attachment/detachment.

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

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 58da08cc3d01..7d9d0fe89064 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -133,7 +133,7 @@ static void free_device_state(struct device_state *dev_state)
 	if (WARN_ON(!group))
 		return;
 
-	iommu_detach_group(dev_state->domain, group);
+	iommu_detach_device(dev_state->domain, &dev_state->pdev->dev);
 
 	iommu_group_put(group);
 
@@ -791,7 +791,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 		goto out_free_domain;
 	}
 
-	ret = iommu_attach_group(dev_state->domain, group);
+	ret = iommu_attach_device(dev_state->domain, &pdev->dev);
 	if (ret != 0)
 		goto out_drop_group;
 
-- 
2.25.1

[PATCH v1 6/8] gpu/host1x: Use iommu_attach/detach_device()

[Lu Baolu]

Ordinary drivers should use iommu_attach/detach_device() for domain
attaching and detaching.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/gpu/host1x/dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index fbb6447b8659..6e08cb6202cc 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host)
 			goto put_cache;
 		}
 
-		err = iommu_attach_group(host->domain, host->group);
+		err = iommu_attach_device(host->domain, host->dev);
 		if (err) {
 			if (err == -ENODEV)
 				err = 0;
@@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host)
 {
 	if (host->domain) {
 		put_iova_domain(&host->iova);
-		iommu_detach_group(host->domain, host->group);
+		iommu_detach_device(host->domain, host->dev);
 
 		iommu_domain_free(host->domain);
 		host->domain = NULL;
-- 
2.25.1

[PATCH v1 7/8] media: staging: media: tegra-vde: Use iommu_attach/detach_device()

[Lu Baolu]

Ordinary drivers should use iommu_attach/detach_device() for domain
attaching and detaching.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/staging/media/tegra-vde/iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/iommu.c b/drivers/staging/media/tegra-vde/iommu.c
index adf8dc7ee25c..a6e6eb28f1e3 100644
--- a/drivers/staging/media/tegra-vde/iommu.c
+++ b/drivers/staging/media/tegra-vde/iommu.c
@@ -91,7 +91,7 @@ int tegra_vde_iommu_init(struct tegra_vde *vde)
 	order = __ffs(vde->domain->pgsize_bitmap);
 	init_iova_domain(&vde->iova, 1UL << order, 0);
 
-	err = iommu_attach_group(vde->domain, vde->group);
+	err = iommu_attach_device(vde->domain, dev);
 	if (err)
 		goto put_iova;
 
@@ -129,7 +129,7 @@ int tegra_vde_iommu_init(struct tegra_vde *vde)
 unreserve_iova:
 	__free_iova(&vde->iova, vde->iova_resv_static_addresses);
 detach_group:
-	iommu_detach_group(vde->domain, vde->group);
+	iommu_detach_device(vde->domain, dev);
 put_iova:
 	put_iova_domain(&vde->iova);
 	iova_cache_put();
@@ -146,7 +146,7 @@ void tegra_vde_iommu_deinit(struct tegra_vde *vde)
 	if (vde->domain) {
 		__free_iova(&vde->iova, vde->iova_resv_last_page);
 		__free_iova(&vde->iova, vde->iova_resv_static_addresses);
-		iommu_detach_group(vde->domain, vde->group);
+		iommu_detach_device(vde->domain, vde->miscdev.parent);
 		put_iova_domain(&vde->iova);
 		iova_cache_put();
 		iommu_domain_free(vde->domain);
-- 
2.25.1

[PATCH v1 8/8] iommu: Remove iommu_attach/detach_group()

[Lu Baolu]

The iommu_attach/detach_group() interfaces have no reference in the tree
anymore. Remove them to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 15 ---------------
 drivers/iommu/iommu.c | 20 --------------------
 2 files changed, 35 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 66ebce3d1e11..2568ab0d0872 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -457,10 +457,6 @@ iommu_alloc_resv_region(phys_addr_t start, size_t length, int prot,
 extern int iommu_get_group_resv_regions(struct iommu_group *group,
 					struct list_head *head);
 
-extern int iommu_attach_group(struct iommu_domain *domain,
-			      struct iommu_group *group);
-extern void iommu_detach_group(struct iommu_domain *domain,
-			       struct iommu_group *group);
 extern struct iommu_group *iommu_group_alloc(void);
 extern void *iommu_group_get_iommudata(struct iommu_group *group);
 extern void iommu_group_set_iommudata(struct iommu_group *group,
@@ -818,17 +814,6 @@ static inline bool iommu_default_passthrough(void)
 	return true;
 }
 
-static inline int iommu_attach_group(struct iommu_domain *domain,
-				     struct iommu_group *group)
-{
-	return -ENODEV;
-}
-
-static inline void iommu_detach_group(struct iommu_domain *domain,
-				      struct iommu_group *group)
-{
-}
-
 static inline struct iommu_group *iommu_group_alloc(void)
 {
 	return ERR_PTR(-ENODEV);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2c9efd85e447..33f7027e677f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2308,18 +2308,6 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 	return ret;
 }
 
-int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
-{
-	int ret;
-
-	mutex_lock(&group->mutex);
-	ret = __iommu_attach_group(domain, group);
-	mutex_unlock(&group->mutex);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(iommu_attach_group);
-
 static int iommu_group_do_detach_device(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
@@ -2357,14 +2345,6 @@ static void __iommu_detach_group(struct iommu_domain *domain,
 		group->domain = group->default_domain;
 }
 
-void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
-{
-	mutex_lock(&group->mutex);
-	__iommu_detach_group(domain, group);
-	mutex_unlock(&group->mutex);
-}
-EXPORT_SYMBOL_GPL(iommu_detach_group);
-
 phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 {
 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
-- 
2.25.1

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Thu, Jan 06, 2022 at 10:20:50AM +0800, Lu Baolu wrote:
> The individual device driver should use iommu_attach/detach_device()
> for domain attachment/detachment.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>  drivers/iommu/amd/iommu_v2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
> index 58da08cc3d01..7d9d0fe89064 100644
> +++ b/drivers/iommu/amd/iommu_v2.c
> @@ -133,7 +133,7 @@ static void free_device_state(struct device_state *dev_state)
>  	if (WARN_ON(!group))
>  		return;
>  
> -	iommu_detach_group(dev_state->domain, group);
> +	iommu_detach_device(dev_state->domain, &dev_state->pdev->dev);
>  
>  	iommu_group_put(group);

This is the only user of the group in the function all the
group_get/put should be deleted too.

Joerg said in commit 55c99a4dc50f ("iommu/amd: Use
iommu_attach_group()") that the device API doesn't work here because
there are multi-device groups?

But I'm not sure how this can work with multi-device groups - this
seems to assigns a domain setup for direct map, so perhaps this only
works if all devices are setup for direct map?

> @@ -791,7 +791,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
>  		goto out_free_domain;
>  	}
>  
> -	ret = iommu_attach_group(dev_state->domain, group);
> +	ret = iommu_attach_device(dev_state->domain, &pdev->dev);
>  	if (ret != 0)
>  		goto out_drop_group;

Same comment here

Jason

Re: [PATCH v1 6/8] gpu/host1x: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote:
> Ordinary drivers should use iommu_attach/detach_device() for domain
> attaching and detaching.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>  drivers/gpu/host1x/dev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> index fbb6447b8659..6e08cb6202cc 100644
> +++ b/drivers/gpu/host1x/dev.c
> @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host)
>  			goto put_cache;
>  		}
>  
> -		err = iommu_attach_group(host->domain, host->group);
> +		err = iommu_attach_device(host->domain, host->dev);
>  		if (err) {
>  			if (err == -ENODEV)
>  				err = 0;
> @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host)
>  {
>  	if (host->domain) {
>  		put_iova_domain(&host->iova);
> -		iommu_detach_group(host->domain, host->group);
> +		iommu_detach_device(host->domain, host->dev);
>  
>  		iommu_domain_free(host->domain);
>  		host->domain = NULL;

Shouldn't this add the flag to tegra_host1x_driver ?

And do like we did in the other tegra stuff and switch to the dma api
when !host1x_wants_iommu() ?

Jason

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Jason Gunthorpe]

On Thu, Jan 06, 2022 at 10:20:46AM +0800, Lu Baolu wrote:
> Expose an interface to replace the domain of an iommu group for frameworks
> like vfio which claims the ownership of the whole iommu group.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>  include/linux/iommu.h | 10 ++++++++++
>  drivers/iommu/iommu.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 408a6d2b3034..66ebce3d1e11 100644
> +++ b/include/linux/iommu.h
> @@ -677,6 +677,9 @@ void iommu_device_unuse_dma_api(struct device *dev);
>  int iommu_group_set_dma_owner(struct iommu_group *group, void *owner);
>  void iommu_group_release_dma_owner(struct iommu_group *group);
>  bool iommu_group_dma_owner_claimed(struct iommu_group *group);
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *old,
> +			       struct iommu_domain *new);
>  
>  #else /* CONFIG_IOMMU_API */
>  
> @@ -1090,6 +1093,13 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>  {
>  	return false;
>  }
> +
> +static inline int
> +iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *old,
> +			   struct iommu_domain *new)
> +{
> +	return -ENODEV;
> +}
>  #endif /* CONFIG_IOMMU_API */
>  
>  /**
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 72a95dea688e..ab8ab95969f5 100644
> +++ b/drivers/iommu/iommu.c
> @@ -3431,3 +3431,40 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>  	return user;
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
> +
> +/**
> + * iommu_group_replace_domain() - Replace group's domain
> + * @group: The group.
> + * @old: The previous attached domain. NULL for none.
> + * @new: The new domain about to be attached.
> + *
> + * This is to support backward compatibility for vfio which manages the dma
> + * ownership in iommu_group level.

This should mention it can only be used with iommu_group_set_dma_owner()

> +	if (old)
> +		__iommu_detach_group(old, group);
> +
> +	if (new) {
> +		ret = __iommu_attach_group(new, group);
> +		if (ret && old)
> +			__iommu_attach_group(old, group);
> +	}

The sketchy error unwind here gives me some pause for sure. Maybe we
should define that on error this leaves the domain as NULL

Complicates vfio a tiny bit to cope with this failure but seems
cleaner than leaving it indeterminate.

Jason

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Jason Gunthorpe]

On Thu, Jan 06, 2022 at 10:20:48AM +0800, Lu Baolu wrote:
> The iommu_attach/detach_device() interfaces were exposed for the device
> drivers to attach/detach their own domains. The commit <426a273834eae>
> ("iommu: Limit iommu_attach/detach_device to device with their own group")
> restricted them to singleton groups to avoid different device in a group
> attaching different domain.
> 
> As we've introduced device DMA ownership into the iommu core. We can now
> extend these interfaces for muliple-device groups, and "all devices are in
> the same address space" is still guaranteed.
> 
> For multiple devices belonging to a same group, iommu_device_use_dma_api()
> and iommu_attach_device() are exclusive. Therefore, when drivers decide to
> use iommu_attach_domain(), they cannot call iommu_device_use_dma_api() at
> the same time.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>  drivers/iommu/iommu.c | 79 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ab8ab95969f5..2c9efd85e447 100644
> +++ b/drivers/iommu/iommu.c
> @@ -47,6 +47,7 @@ struct iommu_group {
>  	struct iommu_domain *domain;
>  	struct list_head entry;
>  	unsigned int owner_cnt;
> +	unsigned int attach_cnt;

Why did we suddenly need another counter? None of the prior versions
needed this. I suppose this is being used a some flag to indicate if
owner_cnt == 1 or owner_cnt == 0 should restore the default domain?
Would rather a flag 'auto_no_kernel_dma_api_compat' or something


> +/**
> + * iommu_attach_device() - attach external or UNMANAGED domain to device
> + * @domain: the domain about to attach
> + * @dev: the device about to be attached
> + *
> + * For devices belonging to the same group, iommu_device_use_dma_api() and
> + * iommu_attach_device() are exclusive. Therefore, when drivers decide to
> + * use iommu_attach_domain(), they cannot call iommu_device_use_dma_api()
> + * at the same time.
> + */
>  int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>  {
>  	struct iommu_group *group;
> +	int ret = 0;
> +
> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
> +		return -EINVAL;
>  
>  	group = iommu_group_get(dev);
>  	if (!group)
>  		return -ENODEV;
>  
> +	if (group->owner_cnt) {
> +		/*
> +		 * Group has been used for kernel-api dma or claimed explicitly
> +		 * for exclusive occupation. For backward compatibility, device
> +		 * in a singleton group is allowed to ignore setting the
> +		 * drv.no_kernel_api_dma field.

BTW why is this call 'no kernel api dma' ? That reads backwards 'no
kernel dma api' right?

Aother appeal of putting no_kernel_api_dma in the struct device_driver
is that this could could simply do 'dev->driver->no_kernel_api_dma' to
figure out how it is being called and avoid this messy implicitness.

Once we know our calling context we can always automatic switch from
DMA API mode to another domain without any trouble or special
counters:

if (!dev->driver->no_kernel_api_dma) {
    if (group->owner_cnt > 1 || group->owner)
        return -EBUSY;
    return __iommu_attach_group(domain, group);
}

if (!group->owner_cnt) {
    ret = __iommu_attach_group(domain, group);
    if (ret)
        return ret;
} else if (group->owner || group->domain != domain)
    return -EBUSY;
group->owner_cnt++;

Right?

> +	if (!group->attach_cnt) {
> +		ret = __iommu_attach_group(domain, group);

How come we don't have to detatch the default domain here? Doesn't
that mean that the iommu_replace_group could also just call attach
directly without going through detatch?

Jason

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Lu Baolu]

Hi Jason,

On 1/6/22 10:33 PM, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 10:20:50AM +0800, Lu Baolu wrote:
>> The individual device driver should use iommu_attach/detach_device()
>> for domain attachment/detachment.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>   drivers/iommu/amd/iommu_v2.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
>> index 58da08cc3d01..7d9d0fe89064 100644
>> +++ b/drivers/iommu/amd/iommu_v2.c
>> @@ -133,7 +133,7 @@ static void free_device_state(struct device_state *dev_state)
>>   	if (WARN_ON(!group))
>>   		return;
>>   
>> -	iommu_detach_group(dev_state->domain, group);
>> +	iommu_detach_device(dev_state->domain, &dev_state->pdev->dev);
>>   
>>   	iommu_group_put(group);
> 
> This is the only user of the group in the function all the
> group_get/put should be deleted too.
> 
> Joerg said in commit 55c99a4dc50f ("iommu/amd: Use
> iommu_attach_group()") that the device API doesn't work here because
> there are multi-device groups?
> 
> But I'm not sure how this can work with multi-device groups - this
> seems to assigns a domain setup for direct map, so perhaps this only
> works if all devices are setup for direct map?

It's also difficult for me to understand how this can work with multi-
device group. The iommu_attach_group() returns -EBUSY if _init_device()
is called for the second device in the group. That's the reason why I
didn't set no_kernel_dma.

> 
>> @@ -791,7 +791,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
>>   		goto out_free_domain;
>>   	}
>>   
>> -	ret = iommu_attach_group(dev_state->domain, group);
>> +	ret = iommu_attach_device(dev_state->domain, &pdev->dev);
>>   	if (ret != 0)
>>   		goto out_drop_group;
> 
> Same comment here

Yes.

> 
> Jason
> 

Best regards,
baolu

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Lu Baolu]

On 1/7/22 1:06 AM, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 10:20:46AM +0800, Lu Baolu wrote:
>> Expose an interface to replace the domain of an iommu group for frameworks
>> like vfio which claims the ownership of the whole iommu group.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>   include/linux/iommu.h | 10 ++++++++++
>>   drivers/iommu/iommu.c | 37 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 408a6d2b3034..66ebce3d1e11 100644
>> +++ b/include/linux/iommu.h
>> @@ -677,6 +677,9 @@ void iommu_device_unuse_dma_api(struct device *dev);
>>   int iommu_group_set_dma_owner(struct iommu_group *group, void *owner);
>>   void iommu_group_release_dma_owner(struct iommu_group *group);
>>   bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>> +int iommu_group_replace_domain(struct iommu_group *group,
>> +			       struct iommu_domain *old,
>> +			       struct iommu_domain *new);
>>   
>>   #else /* CONFIG_IOMMU_API */
>>   
>> @@ -1090,6 +1093,13 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>>   {
>>   	return false;
>>   }
>> +
>> +static inline int
>> +iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *old,
>> +			   struct iommu_domain *new)
>> +{
>> +	return -ENODEV;
>> +}
>>   #endif /* CONFIG_IOMMU_API */
>>   
>>   /**
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 72a95dea688e..ab8ab95969f5 100644
>> +++ b/drivers/iommu/iommu.c
>> @@ -3431,3 +3431,40 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>>   	return user;
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
>> +
>> +/**
>> + * iommu_group_replace_domain() - Replace group's domain
>> + * @group: The group.
>> + * @old: The previous attached domain. NULL for none.
>> + * @new: The new domain about to be attached.
>> + *
>> + * This is to support backward compatibility for vfio which manages the dma
>> + * ownership in iommu_group level.
> 
> This should mention it can only be used with iommu_group_set_dma_owner()

Sure.

> 
>> +	if (old)
>> +		__iommu_detach_group(old, group);
>> +
>> +	if (new) {
>> +		ret = __iommu_attach_group(new, group);
>> +		if (ret && old)
>> +			__iommu_attach_group(old, group);
>> +	}
> 
> The sketchy error unwind here gives me some pause for sure. Maybe we
> should define that on error this leaves the domain as NULL
> 
> Complicates vfio a tiny bit to cope with this failure but seems
> cleaner than leaving it indeterminate.

Fair enough.

> 
> Jason
> 

Best regards,
baolu

Re: [PATCH v1 6/8] gpu/host1x: Use iommu_attach/detach_device()

[Lu Baolu]

On 1/6/22 11:35 PM, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote:
>> Ordinary drivers should use iommu_attach/detach_device() for domain
>> attaching and detaching.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>   drivers/gpu/host1x/dev.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
>> index fbb6447b8659..6e08cb6202cc 100644
>> +++ b/drivers/gpu/host1x/dev.c
>> @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host)
>>   			goto put_cache;
>>   		}
>>   
>> -		err = iommu_attach_group(host->domain, host->group);
>> +		err = iommu_attach_device(host->domain, host->dev);
>>   		if (err) {
>>   			if (err == -ENODEV)
>>   				err = 0;
>> @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host)
>>   {
>>   	if (host->domain) {
>>   		put_iova_domain(&host->iova);
>> -		iommu_detach_group(host->domain, host->group);
>> +		iommu_detach_device(host->domain, host->dev);
>>   
>>   		iommu_domain_free(host->domain);
>>   		host->domain = NULL;
> 
> Shouldn't this add the flag to tegra_host1x_driver ?

This is called for a single driver. The call trace looks like below:

static struct platform_driver tegra_host1x_driver = {
         .driver = {
                 .name = "tegra-host1x",
                 .of_match_table = host1x_of_match,
         },
         .probe = host1x_probe,
         .remove = host1x_remove,
};

host1x_probe(dev)
->host1x_iommu_init(host)	//host is a wrapper of dev
-->host1x_iommu_attach(host)
---->iommu_group_get(host->dev)
      iommu_domain_alloc(&platform_bus_type)
      iommu_attach_group(domain, group);

It seems that the existing code only works for singleton group.

> 
> And do like we did in the other tegra stuff and switch to the dma api
> when !host1x_wants_iommu() ?
> 
> Jason
> 

Best regards,
baolu

Re: [PATCH v1 6/8] gpu/host1x: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Fri, Jan 07, 2022 at 08:35:34AM +0800, Lu Baolu wrote:
> On 1/6/22 11:35 PM, Jason Gunthorpe wrote:
> > On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote:
> > > Ordinary drivers should use iommu_attach/detach_device() for domain
> > > attaching and detaching.
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > >   drivers/gpu/host1x/dev.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> > > index fbb6447b8659..6e08cb6202cc 100644
> > > +++ b/drivers/gpu/host1x/dev.c
> > > @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host)
> > >   			goto put_cache;
> > >   		}
> > > -		err = iommu_attach_group(host->domain, host->group);
> > > +		err = iommu_attach_device(host->domain, host->dev);
> > >   		if (err) {
> > >   			if (err == -ENODEV)
> > >   				err = 0;
> > > @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host)
> > >   {
> > >   	if (host->domain) {
> > >   		put_iova_domain(&host->iova);
> > > -		iommu_detach_group(host->domain, host->group);
> > > +		iommu_detach_device(host->domain, host->dev);
> > >   		iommu_domain_free(host->domain);
> > >   		host->domain = NULL;
> > 
> > Shouldn't this add the flag to tegra_host1x_driver ?
> 
> This is called for a single driver. The call trace looks like below:
> 
> static struct platform_driver tegra_host1x_driver = {
>         .driver = {
>                 .name = "tegra-host1x",
>                 .of_match_table = host1x_of_match,
>         },
>         .probe = host1x_probe,
>         .remove = host1x_remove,
> };
> 
> host1x_probe(dev)
> ->host1x_iommu_init(host)	//host is a wrapper of dev
>      iommu_domain_alloc(&platform_bus_type)
>      iommu_attach_group(domain, group);

The main question is if the iommu group is being shared with other
drivers, not the call chain for this function.

For tegra you have to go look in each entry of the of_match_table:

        { .compatible = "nvidia,tegra114-host1x", .data = &host1x02_info, },

And find the DTS block:

        host1x@50000000 {
                compatible = "nvidia,tegra114-host1x";
                reg = <0x50000000 0x00028000>;
                interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
                             <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
                interrupt-names = "syncpt", "host1x";
                clocks = <&tegra_car TEGRA114_CLK_HOST1X>;
                clock-names = "host1x";
                resets = <&tegra_car 28>;
                reset-names = "host1x";
                iommus = <&mc TEGRA_SWGROUP_HC>;

Then check if any other devices in the DTS use the same 'iommus' which
is how the groups are setup.

I checked everything and it does look like this is a single device
group.

Jason

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Lu Baolu]

Hi Jason,

On 1/7/22 1:22 AM, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 10:20:48AM +0800, Lu Baolu wrote:
>> The iommu_attach/detach_device() interfaces were exposed for the device
>> drivers to attach/detach their own domains. The commit <426a273834eae>
>> ("iommu: Limit iommu_attach/detach_device to device with their own group")
>> restricted them to singleton groups to avoid different device in a group
>> attaching different domain.
>>
>> As we've introduced device DMA ownership into the iommu core. We can now
>> extend these interfaces for muliple-device groups, and "all devices are in
>> the same address space" is still guaranteed.
>>
>> For multiple devices belonging to a same group, iommu_device_use_dma_api()
>> and iommu_attach_device() are exclusive. Therefore, when drivers decide to
>> use iommu_attach_domain(), they cannot call iommu_device_use_dma_api() at
>> the same time.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>   drivers/iommu/iommu.c | 79 +++++++++++++++++++++++++++++++++----------
>>   1 file changed, 62 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index ab8ab95969f5..2c9efd85e447 100644
>> +++ b/drivers/iommu/iommu.c
>> @@ -47,6 +47,7 @@ struct iommu_group {
>>   	struct iommu_domain *domain;
>>   	struct list_head entry;
>>   	unsigned int owner_cnt;
>> +	unsigned int attach_cnt;
> 
> Why did we suddenly need another counter? None of the prior versions
> needed this. I suppose this is being used a some flag to indicate if
> owner_cnt == 1 or owner_cnt == 0 should restore the default domain?

Yes, exactly.

> Would rather a flag 'auto_no_kernel_dma_api_compat' or something

Adding a flag also works.

> 
> 
>> +/**
>> + * iommu_attach_device() - attach external or UNMANAGED domain to device
>> + * @domain: the domain about to attach
>> + * @dev: the device about to be attached
>> + *
>> + * For devices belonging to the same group, iommu_device_use_dma_api() and
>> + * iommu_attach_device() are exclusive. Therefore, when drivers decide to
>> + * use iommu_attach_domain(), they cannot call iommu_device_use_dma_api()
>> + * at the same time.
>> + */
>>   int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>>   {
>>   	struct iommu_group *group;
>> +	int ret = 0;
>> +
>> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
>> +		return -EINVAL;
>>   
>>   	group = iommu_group_get(dev);
>>   	if (!group)
>>   		return -ENODEV;
>>   
>> +	if (group->owner_cnt) {
>> +		/*
>> +		 * Group has been used for kernel-api dma or claimed explicitly
>> +		 * for exclusive occupation. For backward compatibility, device
>> +		 * in a singleton group is allowed to ignore setting the
>> +		 * drv.no_kernel_api_dma field.
> 
> BTW why is this call 'no kernel api dma' ? That reads backwards 'no
> kernel dma api' right?

Yes. Need to rephrase this wording.

> 
> Aother appeal of putting no_kernel_api_dma in the struct device_driver
> is that this could could simply do 'dev->driver->no_kernel_api_dma' to
> figure out how it is being called and avoid this messy implicitness.

Yes.

> 
> Once we know our calling context we can always automatic switch from
> DMA API mode to another domain without any trouble or special
> counters:
> 
> if (!dev->driver->no_kernel_api_dma) {
>      if (group->owner_cnt > 1 || group->owner)
>          return -EBUSY;
>      return __iommu_attach_group(domain, group);
> }

Is there any lock issue when referencing dev->driver here? I guess this
requires iommu_attach_device() only being called during the driver life
(a.k.a. between driver .probe and .release).

> 
> if (!group->owner_cnt) {
>      ret = __iommu_attach_group(domain, group);
>      if (ret)
>          return ret;
> } else if (group->owner || group->domain != domain)
>      return -EBUSY;
> group->owner_cnt++;
> 
> Right?

Yes. It's more straightforward if there's no issue around dev->driver
referencing.

> 
>> +	if (!group->attach_cnt) {
>> +		ret = __iommu_attach_group(domain, group);
> 
> How come we don't have to detatch the default domain here? Doesn't
> that mean that the iommu_replace_group could also just call attach
> directly without going through detatch?

__iommu_attach_group() allows replacing the default domain with a
private domain. Corresponding __iommu_detach_group() automatically
replaces private domain with the default domain.

The auto-switch logic should not apply to iommu_group_replace_domain()
which is designed for components with iommu_set_dma_owner() called.

> 
> Jason
> 

Best regards,
baolu

Re: [PATCH v1 6/8] gpu/host1x: Use iommu_attach/detach_device()

[Lu Baolu]

On 1/7/22 8:48 AM, Jason Gunthorpe wrote:
> On Fri, Jan 07, 2022 at 08:35:34AM +0800, Lu Baolu wrote:
>> On 1/6/22 11:35 PM, Jason Gunthorpe wrote:
>>> On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote:
>>>> Ordinary drivers should use iommu_attach/detach_device() for domain
>>>> attaching and detaching.
>>>>
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>>    drivers/gpu/host1x/dev.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
>>>> index fbb6447b8659..6e08cb6202cc 100644
>>>> +++ b/drivers/gpu/host1x/dev.c
>>>> @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host)
>>>>    			goto put_cache;
>>>>    		}
>>>> -		err = iommu_attach_group(host->domain, host->group);
>>>> +		err = iommu_attach_device(host->domain, host->dev);
>>>>    		if (err) {
>>>>    			if (err == -ENODEV)
>>>>    				err = 0;
>>>> @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host)
>>>>    {
>>>>    	if (host->domain) {
>>>>    		put_iova_domain(&host->iova);
>>>> -		iommu_detach_group(host->domain, host->group);
>>>> +		iommu_detach_device(host->domain, host->dev);
>>>>    		iommu_domain_free(host->domain);
>>>>    		host->domain = NULL;
>>>
>>> Shouldn't this add the flag to tegra_host1x_driver ?
>>
>> This is called for a single driver. The call trace looks like below:
>>
>> static struct platform_driver tegra_host1x_driver = {
>>          .driver = {
>>                  .name = "tegra-host1x",
>>                  .of_match_table = host1x_of_match,
>>          },
>>          .probe = host1x_probe,
>>          .remove = host1x_remove,
>> };
>>
>> host1x_probe(dev)
>> ->host1x_iommu_init(host)	//host is a wrapper of dev
>>       iommu_domain_alloc(&platform_bus_type)
>>       iommu_attach_group(domain, group);
> 
> The main question is if the iommu group is being shared with other
> drivers, not the call chain for this function.
> 
> For tegra you have to go look in each entry of the of_match_table:
> 
>          { .compatible = "nvidia,tegra114-host1x", .data = &host1x02_info, },
> 
> And find the DTS block:
> 
>          host1x@50000000 {
>                  compatible = "nvidia,tegra114-host1x";
>                  reg = <0x50000000 0x00028000>;
>                  interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
>                               <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
>                  interrupt-names = "syncpt", "host1x";
>                  clocks = <&tegra_car TEGRA114_CLK_HOST1X>;
>                  clock-names = "host1x";
>                  resets = <&tegra_car 28>;
>                  reset-names = "host1x";
>                  iommus = <&mc TEGRA_SWGROUP_HC>;
> 
> Then check if any other devices in the DTS use the same 'iommus' which
> is how the groups are setup.

Yes, exactly.

> 
> I checked everything and it does look like this is a single device
> group.

Okay, thanks!

> 
> Jason
> 

Best regards,
baolu

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Jason Gunthorpe]

On Fri, Jan 07, 2022 at 09:14:38AM +0800, Lu Baolu wrote:

> > Once we know our calling context we can always automatic switch from
> > DMA API mode to another domain without any trouble or special
> > counters:
> > 
> > if (!dev->driver->no_kernel_api_dma) {
> >      if (group->owner_cnt > 1 || group->owner)
> >          return -EBUSY;
> >      return __iommu_attach_group(domain, group);
> > }
> 
> Is there any lock issue when referencing dev->driver here? I guess this
> requires iommu_attach_device() only being called during the driver life
> (a.k.a. between driver .probe and .release).

Yes, that is correct. That would need to be documented.

It is the same reason the routine was able to get the group from the
dev. The dev's group must be stable so long as a driver is attached or
everything is broken :)

Much of the group refcounting code is useless for this reason. The
group simply cannot be concurrently destroyed in these contexts.

Jason

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Joerg Roedel]

On Thu, Jan 06, 2022 at 10:33:45AM -0400, Jason Gunthorpe wrote:
> But I'm not sure how this can work with multi-device groups - this
> seems to assigns a domain setup for direct map, so perhaps this only
> works if all devices are setup for direct map?

Right, at boot all devices in this group are already setup for using a
direct map. There are usually two devices in those groups, the GPU
itself and the sound device for HDMI sound output.

Regards,

	Joerg

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Joerg Roedel]

On Thu, Jan 06, 2022 at 10:20:48AM +0800, Lu Baolu wrote:
>  int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>  {
>  	struct iommu_group *group;
> -	int ret;
> +	int ret = 0;
> +
> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
> +		return -EINVAL;
>  
>  	group = iommu_group_get(dev);
>  	if (!group)
>  		return -ENODEV;
>  
> -	/*
> -	 * Lock the group to make sure the device-count doesn't
> -	 * change while we are attaching
> -	 */
>  	mutex_lock(&group->mutex);
> -	ret = -EINVAL;
> -	if (iommu_group_device_count(group) != 1)
> -		goto out_unlock;
> +	if (group->owner_cnt) {
> +		/*
> +		 * Group has been used for kernel-api dma or claimed explicitly
> +		 * for exclusive occupation. For backward compatibility, device
> +		 * in a singleton group is allowed to ignore setting the
> +		 * drv.no_kernel_api_dma field.
> +		 */
> +		if ((group->domain == group->default_domain &&
> +		     iommu_group_device_count(group) != 1) ||
> +		    group->owner) {
> +			ret = -EBUSY;
> +			goto unlock_out;
> +		}
> +	}
>  
> -	ret = __iommu_attach_group(domain, group);
> +	if (!group->attach_cnt) {
> +		ret = __iommu_attach_group(domain, group);
> +		if (ret)
> +			goto unlock_out;
> +	} else {
> +		if (group->domain != domain) {
> +			ret = -EPERM;
> +			goto unlock_out;
> +		}
> +	}
>  
> -out_unlock:
> +	group->owner_cnt++;
> +	group->attach_cnt++;
> +
> +unlock_out:
>  	mutex_unlock(&group->mutex);
>  	iommu_group_put(group);

This extends iommu_attach_device() to behave as iommu_attach_group(),
changing the domain for the whole group. Wouldn't it be better to scrap
the iommu_attach_device() interface instead and only rely on
iommu_attach_group()? This way it is clear that a call changes the whole
group.

IIUC this work is heading towards allowing multiple domains in one group
as long as the group is owned by one entity. That is a valid
requirement, but the way to get there is in my eyes:

	1) Introduce a concept of a sub-group (or whatever we want to
	   call it), which groups devices together which must be in the
	   same domain because they use the same request ID and thus
	   look all the same to the IOMMU.

	2) Keep todays IOMMU groups to group devices together which can
	   bypass the IOMMU when talking to each other, like
	   multi-function devices and devices behind a no-ACS bridge.

	3) Rework group->domain and group->default_domain, eventually
	   moving them to sub-groups.

This is an important distinction to make and also the reason the
iommu_attach/detach_device() interface will always be misleading. Item
1) in this list will also be beneficial to other parts of the iommu
code, namely iommu-dma code which can have finer-grained DMA-API domains
with sub-groups instead of groups.

Regards,

	Joerg

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Robin Murphy]

On 2022-01-06 02:20, Lu Baolu wrote:
> Expose an interface to replace the domain of an iommu group for frameworks
> like vfio which claims the ownership of the whole iommu group.

But if the underlying point is the new expectation that 
iommu_{attach,detach}_device() operate on the device's whole group where 
relevant, why should we invent some special mechanism for VFIO to be 
needlessly inconsistent?

I said before that it's trivial for VFIO to resolve a suitable device if 
it needs to; by now I've actually written the patch ;)

https://gitlab.arm.com/linux-arm/linux-rm/-/commit/9f37d8c17c9b606abc96e1f1001c0b97c8b93ed5

Robin.

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   include/linux/iommu.h | 10 ++++++++++
>   drivers/iommu/iommu.c | 37 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 47 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 408a6d2b3034..66ebce3d1e11 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -677,6 +677,9 @@ void iommu_device_unuse_dma_api(struct device *dev);
>   int iommu_group_set_dma_owner(struct iommu_group *group, void *owner);
>   void iommu_group_release_dma_owner(struct iommu_group *group);
>   bool iommu_group_dma_owner_claimed(struct iommu_group *group);
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *old,
> +			       struct iommu_domain *new);
>   
>   #else /* CONFIG_IOMMU_API */
>   
> @@ -1090,6 +1093,13 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>   {
>   	return false;
>   }
> +
> +static inline int
> +iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *old,
> +			   struct iommu_domain *new)
> +{
> +	return -ENODEV;
> +}
>   #endif /* CONFIG_IOMMU_API */
>   
>   /**
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 72a95dea688e..ab8ab95969f5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3431,3 +3431,40 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>   	return user;
>   }
>   EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
> +
> +/**
> + * iommu_group_replace_domain() - Replace group's domain
> + * @group: The group.
> + * @old: The previous attached domain. NULL for none.
> + * @new: The new domain about to be attached.
> + *
> + * This is to support backward compatibility for vfio which manages the dma
> + * ownership in iommu_group level.
> + */
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *old,
> +			       struct iommu_domain *new)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&group->mutex);
> +	if (!group->owner || group->domain != old) {
> +		ret = -EPERM;
> +		goto unlock_out;
> +	}
> +
> +	if (old)
> +		__iommu_detach_group(old, group);
> +
> +	if (new) {
> +		ret = __iommu_attach_group(new, group);
> +		if (ret && old)
> +			__iommu_attach_group(old, group);
> +	}
> +
> +unlock_out:
> +	mutex_unlock(&group->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_group_replace_domain);

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 12:09:36PM +0000, Robin Murphy wrote:
> On 2022-01-06 02:20, Lu Baolu wrote:
> > Expose an interface to replace the domain of an iommu group for frameworks
> > like vfio which claims the ownership of the whole iommu group.
> 
> But if the underlying point is the new expectation that
> iommu_{attach,detach}_device() operate on the device's whole group where
> relevant, why should we invent some special mechanism for VFIO to be
> needlessly inconsistent?
> 
> I said before that it's trivial for VFIO to resolve a suitable device if it
> needs to; by now I've actually written the patch ;)
> 
> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/9f37d8c17c9b606abc96e1f1001c0b97c8b93ed5

Er, how does locking work there? What keeps busdev from being
concurrently unplugged? How can iommu_group_get() be safely called on
this pointer?

All of the above only works normally inside a probe/remove context
where the driver core is blocking concurrent unplug and descruction.

I think I said this last time you brought it up that lifetime was the
challenge with this idea.

Jason

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 12:39:36PM +0100, Joerg Roedel wrote:

> This extends iommu_attach_device() to behave as iommu_attach_group(),
> changing the domain for the whole group. 

Of course, the only action to take is to change the domain of a
group..

> Wouldn't it be better to scrap the iommu_attach_device() interface
> instead and only rely on iommu_attach_group()? This way it is clear
> that a call changes the whole group.

From an API design perspective drivers should never touch groups -
they have struct devices, they should have a clean struct device based
API.

Groups should disappear into an internal implementation detail, not be
so prominent in the API.

> IIUC this work is heading towards allowing multiple domains in one group
> as long as the group is owned by one entity.

No, it isn't. This work is only about properly arbitrating which
single domain is attached to an entire group.

> 	1) Introduce a concept of a sub-group (or whatever we want to
> 	   call it), which groups devices together which must be in the
> 	   same domain because they use the same request ID and thus
> 	   look all the same to the IOMMU.
>
> 	2) Keep todays IOMMU groups to group devices together which can
> 	   bypass the IOMMU when talking to each other, like
> 	   multi-function devices and devices behind a no-ACS bridge.

We've talked about all these details before and nobody has thought
they are important enough to implement. This distinction is not the
goal of this series.

I think if someone did want to do this there is room in the API to
allow the distinction between 1 (must share) and 2 (sharing is
insecure). eg by checking owner and blocking mixing user/kernel. 

This is another reason to stick with the device centric API as if we
did someday want multi-domain groups then the device input is still
the correct input and the iommu code can figure out what sub-groups or
whatever transparently.

Jason

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 12:27:05PM +0100, Joerg Roedel wrote:
> On Thu, Jan 06, 2022 at 10:33:45AM -0400, Jason Gunthorpe wrote:
> > But I'm not sure how this can work with multi-device groups - this
> > seems to assigns a domain setup for direct map, so perhaps this only
> > works if all devices are setup for direct map?
> 
> Right, at boot all devices in this group are already setup for using a
> direct map. There are usually two devices in those groups, the GPU
> itself and the sound device for HDMI sound output.

But how does the sound device know that this has been done to it?

eg how do we know the sound device hasn't been bound to VFIO or
something at this point?

Jason

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Joerg Roedel]

On Mon, Feb 14, 2022 at 09:15:44AM -0400, Jason Gunthorpe wrote:
> But how does the sound device know that this has been done to it?
> 
> eg how do we know the sound device hasn't been bound to VFIO or
> something at this point?

The iommu_attach_group() call will fail when the group (which includes
GPU and sound device) it not in its default-domain. So if VFIO attached
the group to its own domain, there is a failure in this init function.

Note that this function is intended to be called by the driver currently
controling this device, so there should also be no race with VFIO trying
to grab the device in parallel.

Regards,

	Joerg

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 02:40:56PM +0100, Joerg Roedel wrote:
> On Mon, Feb 14, 2022 at 09:15:44AM -0400, Jason Gunthorpe wrote:
> > But how does the sound device know that this has been done to it?
> > 
> > eg how do we know the sound device hasn't been bound to VFIO or
> > something at this point?
> 
> The iommu_attach_group() call will fail when the group (which includes
> GPU and sound device) it not in its default-domain. So if VFIO attached
> the group to its own domain, there is a failure in this init function.

That works for VFIO, but it doesn't work for other in-kernel
drivers.. Is there something ensuring the group is only the GPU and
sound device? Is the GPU never an addin card?

I'd say the right way to code this after Lu's series to have both the
GPU and sound driver call iommu_attach_device() during their probe()'s
and specify the identity domain as the attaching domain.

That would be quite similar to how the Tegra drivers got arranged.

And then maybe someone could better guess what the "sound driver" is
since it would be marked with an iommu_attach_device() call..

Jason

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Robin Murphy]

On 2022-02-14 12:45, Jason Gunthorpe wrote:
> On Mon, Feb 14, 2022 at 12:09:36PM +0000, Robin Murphy wrote:
>> On 2022-01-06 02:20, Lu Baolu wrote:
>>> Expose an interface to replace the domain of an iommu group for frameworks
>>> like vfio which claims the ownership of the whole iommu group.
>>
>> But if the underlying point is the new expectation that
>> iommu_{attach,detach}_device() operate on the device's whole group where
>> relevant, why should we invent some special mechanism for VFIO to be
>> needlessly inconsistent?
>>
>> I said before that it's trivial for VFIO to resolve a suitable device if it
>> needs to; by now I've actually written the patch ;)
>>
>> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/9f37d8c17c9b606abc96e1f1001c0b97c8b93ed5
> 
> Er, how does locking work there? What keeps busdev from being
> concurrently unplugged?

Same thing that prevents the bus pointer from suddenly becoming invalid 
in the current code, I guess :)

But yes, holding a group reference alone can't prevent the group itself 
from changing, and the finer points of locking still need working out - 
there's a reason you got a link to a WIP branch in my tree rather than a 
proper patch in your inbox (TBH at the moment that one represents about 
a 5:1 ratio of time spent on the reasoning behind the commit message vs. 
the implementation itself).

> How can iommu_group_get() be safely called on
> this pointer?

VFIO hardly needs to retrieve the iommu_group from a device which it 
derived from the iommu_group it holds in the first place. What matters 
is being able to call *other* device-based IOMMU API interfaces in the 
long term. And once a robust solution for that is in place, it should 
inevitably work for a device-based attach interface too.

> All of the above only works normally inside a probe/remove context
> where the driver core is blocking concurrent unplug and descruction.
> 
> I think I said this last time you brought it up that lifetime was the
> challenge with this idea.

Indeed, but it's a challenge that needs tackling, because the bus-based 
interfaces need to go away. So either we figure it out now and let this 
attach interface rework benefit immediately, or I spend three times as 
long solving it on my own and end up deleting 
iommu_group_replace_domain() in about 6 months' time anyway.

Thanks,
Robin.

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Joerg Roedel]

On Mon, Feb 14, 2022 at 10:02:36AM -0400, Jason Gunthorpe wrote:
> That works for VFIO, but it doesn't work for other in-kernel
> drivers.. Is there something ensuring the group is only the GPU and
> sound device? Is the GPU never an addin card?

GPUs supporting this functionality are always iGPUs, AFAIK.

> I'd say the right way to code this after Lu's series to have both the
> GPU and sound driver call iommu_attach_device() during their probe()'s
> and specify the identity domain as the attaching domain.
> 
> That would be quite similar to how the Tegra drivers got arranged.
> 
> And then maybe someone could better guess what the "sound driver" is
> since it would be marked with an iommu_attach_device() call.

Device drivers calling into iommu_attach_device() is seldom a good
idea.  In this case the sound device has some generic hardware
interface so that an existing sound driver can be re-used. Making this
driver call iommu-specific functions for some devices is something hard
to justify.

With sub-groups on the other hand it would be a no-brainer, because the
sound device would be in a separate sub-group. Basically any device in
the same group as the GPU would be in a separate sub-group.

Regards,

	Joerg

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Joerg Roedel]

On Mon, Feb 14, 2022 at 09:03:13AM -0400, Jason Gunthorpe wrote:
> Groups should disappear into an internal implementation detail, not be
> so prominent in the API.

Not going to happen, IOMMU groups are ABI and todays device assignment
code, including user-space, relies on them.

Groups implement and important aspect of hardware IOMMUs that the API
can not abstract away: That there are devices which share the same
request-id. 

This is not an issue for devices concerned by iommufd, but for legacy
device assignment it is. The IOMMU-API needs to handle both in a clean
API, even if it means that drivers need to lookup the sub-group of a
device first.

And I don't see how a per-device API can handle both in a device-centric
way. For sure it is not making it 'device centric but operate on groups
under the hood'.

Regards,

	Joerg

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 02:10:19PM +0000, Robin Murphy wrote:
> On 2022-02-14 12:45, Jason Gunthorpe wrote:
> > On Mon, Feb 14, 2022 at 12:09:36PM +0000, Robin Murphy wrote:
> > > On 2022-01-06 02:20, Lu Baolu wrote:
> > > > Expose an interface to replace the domain of an iommu group for frameworks
> > > > like vfio which claims the ownership of the whole iommu group.
> > > 
> > > But if the underlying point is the new expectation that
> > > iommu_{attach,detach}_device() operate on the device's whole group where
> > > relevant, why should we invent some special mechanism for VFIO to be
> > > needlessly inconsistent?
> > > 
> > > I said before that it's trivial for VFIO to resolve a suitable device if it
> > > needs to; by now I've actually written the patch ;)
> > > 
> > > https://gitlab.arm.com/linux-arm/linux-rm/-/commit/9f37d8c17c9b606abc96e1f1001c0b97c8b93ed5
> > 
> > Er, how does locking work there? What keeps busdev from being
> > concurrently unplugged?
> 
> Same thing that prevents the bus pointer from suddenly becoming invalid in
> the current code, I guess :)

Oooh, yes, that does look broken now too. :(

> > How can iommu_group_get() be safely called on
> > this pointer?
> 
> What matters is being able to call *other* device-based IOMMU API
> interfaces in the long term.

Yes, this is what I mean, those are the ones that call
iommu_group_get().

> > All of the above only works normally inside a probe/remove context
> > where the driver core is blocking concurrent unplug and descruction.
> > 
> > I think I said this last time you brought it up that lifetime was the
> > challenge with this idea.
> 
> Indeed, but it's a challenge that needs tackling, because the bus-based
> interfaces need to go away. So either we figure it out now and let this
> attach interface rework benefit immediately, or I spend three times as long

IMHO your path is easier if you let VFIO stay with the group interface
and use something like:

   domain = iommu_group_alloc_domain(group)

Which is what VFIO is trying to accomplish. Since Lu removed the only
other user of iommu_group_for_each_dev() it means we can de-export
that interface.

This works better because the iommu code can hold the internal group
while it finds the bus/device and then invokes the driver op. We don't
have a lifetime problem anymore under that lock.

The remaining VFIO use of bus for iommu_capable() is better done
against the domain or the group object, as appropriate.

In the bigger picture, VFIO should stop doing
'iommu_group_alloc_domain' by moving the domain alloc to
VFIO_GROUP_GET_DEVICE_FD where we have a struct device to use.

We've already been experimenting with this for iommufd and the subtle
difference in the uapi doesn't seem relevant.

> solving it on my own and end up deleting
> iommu_group_replace_domain() in about 6 months' time anyway.

I expect this API to remain until we figure out a solution to the PPC
problem, and come up with an alternative way to change the attached
domain on the fly.

Jason

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 03:23:07PM +0100, Joerg Roedel wrote:

> Device drivers calling into iommu_attach_device() is seldom a good
> idea.  In this case the sound device has some generic hardware
> interface so that an existing sound driver can be re-used. Making this
> driver call iommu-specific functions for some devices is something hard
> to justify.

Er, so this is transparent to the generic sound device? I guess
something fixed up the dma_api on that device to keep working?

But, then, the requirement is that nobody is using the dma API when we
make this change?

> With sub-groups on the other hand it would be a no-brainer, because the
> sound device would be in a separate sub-group. Basically any device in
> the same group as the GPU would be in a separate sub-group.

I don't think it matters how big/small the group is, only that when we
change the domain we know everything flowing through the domain is
still happy.

Jason

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Robin Murphy]

On 2022-02-14 14:39, Joerg Roedel wrote:
> On Mon, Feb 14, 2022 at 09:03:13AM -0400, Jason Gunthorpe wrote:
>> Groups should disappear into an internal implementation detail, not be
>> so prominent in the API.
> 
> Not going to happen, IOMMU groups are ABI and todays device assignment
> code, including user-space, relies on them.
> 
> Groups implement and important aspect of hardware IOMMUs that the API
> can not abstract away: That there are devices which share the same
> request-id.
> 
> This is not an issue for devices concerned by iommufd, but for legacy
> device assignment it is. The IOMMU-API needs to handle both in a clean
> API, even if it means that drivers need to lookup the sub-group of a
> device first.
> 
> And I don't see how a per-device API can handle both in a device-centric
> way. For sure it is not making it 'device centric but operate on groups
> under the hood'.

Arguably, iommu_attach_device() could be renamed something like 
iommu_attach_group_for_dev(), since that's effectively the semantic that 
all the existing API users want anyway (even VFIO at the high level - 
the group is the means for the user to assign their GPU/NIC/whatever 
device to their process, not the end in itself). That's just a lot more 
churn.

It's not that callers should be blind to the entire concept of groups 
altogether - they remain a significant reason why iommu_attach_device() 
might fail, for one thing - however what callers really shouldn't need 
to be bothered with is the exact *implementation* of groups. I do 
actually quite like the idea of refining the group abstraction into 
isolation groups as a superset of alias groups, but if anything that's a 
further argument for not having the guts of the current abstraction 
exposed in places that don't need to care - otherwise that would be 
liable to be a microcosm of this series in itself: widespread churn vs. 
"same name, new meaning" compromises.

Robin.

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 03:18:31PM +0000, Robin Murphy wrote:

> Arguably, iommu_attach_device() could be renamed something like
> iommu_attach_group_for_dev(), since that's effectively the semantic that all
> the existing API users want anyway (even VFIO at the high level - the group
> is the means for the user to assign their GPU/NIC/whatever device to their
> process, not the end in itself). That's just a lot more churn.

Right
 
> It's not that callers should be blind to the entire concept of groups
> altogether - they remain a significant reason why iommu_attach_device()
> might fail, for one thing - however what callers really shouldn't need to be
> bothered with is the exact *implementation* of groups. I do actually quite
> like the idea of refining the group abstraction into isolation groups as a
> superset of alias groups, but if anything that's a further argument for not
> having the guts of the current abstraction exposed in places that don't need
> to care - otherwise that would be liable to be a microcosm of this series in
> itself: widespread churn vs. "same name, new meaning" compromises.

Exactly, groups should not leak out through the abstraction more than
necessary. If the caller can't do anything with the group information
then it shouldn't touch it.

VFIO needs them because its uAPI is tied, but even so we keep talking
about ways to narrow the amount of group API it consumes.

We should not set the recommended/good kAPI based on VFIOs uAPI
design.

Jason

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Robin Murphy]

On 2022-02-14 14:56, Jason Gunthorpe via iommu wrote:
> On Mon, Feb 14, 2022 at 02:10:19PM +0000, Robin Murphy wrote:
>> On 2022-02-14 12:45, Jason Gunthorpe wrote:
>>> On Mon, Feb 14, 2022 at 12:09:36PM +0000, Robin Murphy wrote:
>>>> On 2022-01-06 02:20, Lu Baolu wrote:
>>>>> Expose an interface to replace the domain of an iommu group for frameworks
>>>>> like vfio which claims the ownership of the whole iommu group.
>>>>
>>>> But if the underlying point is the new expectation that
>>>> iommu_{attach,detach}_device() operate on the device's whole group where
>>>> relevant, why should we invent some special mechanism for VFIO to be
>>>> needlessly inconsistent?
>>>>
>>>> I said before that it's trivial for VFIO to resolve a suitable device if it
>>>> needs to; by now I've actually written the patch ;)
>>>>
>>>> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/9f37d8c17c9b606abc96e1f1001c0b97c8b93ed5
>>>
>>> Er, how does locking work there? What keeps busdev from being
>>> concurrently unplugged?
>>
>> Same thing that prevents the bus pointer from suddenly becoming invalid in
>> the current code, I guess :)
> 
> Oooh, yes, that does look broken now too. :(
> 
>>> How can iommu_group_get() be safely called on
>>> this pointer?
>>
>> What matters is being able to call *other* device-based IOMMU API
>> interfaces in the long term.
> 
> Yes, this is what I mean, those are the ones that call
> iommu_group_get().
> 
>>> All of the above only works normally inside a probe/remove context
>>> where the driver core is blocking concurrent unplug and descruction.
>>>
>>> I think I said this last time you brought it up that lifetime was the
>>> challenge with this idea.
>>
>> Indeed, but it's a challenge that needs tackling, because the bus-based
>> interfaces need to go away. So either we figure it out now and let this
>> attach interface rework benefit immediately, or I spend three times as long
> 
> IMHO your path is easier if you let VFIO stay with the group interface
> and use something like:
> 
>     domain = iommu_group_alloc_domain(group)
> 
> Which is what VFIO is trying to accomplish. Since Lu removed the only
> other user of iommu_group_for_each_dev() it means we can de-export
> that interface.
> 
> This works better because the iommu code can hold the internal group
> while it finds the bus/device and then invokes the driver op. We don't
> have a lifetime problem anymore under that lock.

That's certainly one of the cleaner possibilities - per the theme of 
this thread I'm not hugely keen on proliferating special VFIO-specific 
versions of IOMMU APIs, but trying to take the dev->mutex might be a bit 
heavy-handed and risky, and getting at the vfio_group->device_lock a bit 
fiddly, so if I can't come up with anything nicer or more general it 
might be a fair compromise.

> The remaining VFIO use of bus for iommu_capable() is better done
> against the domain or the group object, as appropriate.

Indeed, although half the implementations of .capable are nonsense 
already, so I'm treating that one as a secondary priority for the moment 
(with an aim to come back afterwards and just try to kill it off as far 
as possible). RDMA and VFIO shouldn't be a serious concern for the kind 
of systems with heterogeneous IOMMUs at this point.

> In the bigger picture, VFIO should stop doing
> 'iommu_group_alloc_domain' by moving the domain alloc to
> VFIO_GROUP_GET_DEVICE_FD where we have a struct device to use.
> 
> We've already been experimenting with this for iommufd and the subtle
> difference in the uapi doesn't seem relevant.
> 
>> solving it on my own and end up deleting
>> iommu_group_replace_domain() in about 6 months' time anyway.
> 
> I expect this API to remain until we figure out a solution to the PPC
> problem, and come up with an alternative way to change the attached
> domain on the fly.

I though PPC wasn't using the IOMMU API at all... or is that the problem?

Thanks,
Robin.

Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 04:38:23PM +0000, Robin Murphy wrote:

> > This works better because the iommu code can hold the internal group
> > while it finds the bus/device and then invokes the driver op. We don't
> > have a lifetime problem anymore under that lock.
> 
> That's certainly one of the cleaner possibilities - per the theme of this
> thread I'm not hugely keen on proliferating special VFIO-specific
> versions

IMHO this is still a net better than VFIO open coding buggy versions
as it has done.

> of IOMMU APIs, but trying to take the dev->mutex might be a bit heavy-handed
> and risky,

The container->group lock is held during this code, and the
container->group_lock is taken during probe under the
dev_mutex. Acquiring the dev_mutex inside the group_lock should not be
done.

> and getting at the vfio_group->device_lock a bit fiddly, so if I
> can't come up with anything nicer or more general it might be a fair
> compromise.

Actually that doesn't look so bad. A 'vfio allocate domain from group'
function that used the above trick looks OK to me right now.

If we could move the iommu_capable() test to a domain that would make
this pretty nice - getting the bus safely is a bit more of a PITA -
I'm much less keen on holding the device_lock for the whole function.

> > The remaining VFIO use of bus for iommu_capable() is better done
> > against the domain or the group object, as appropriate.
> 
> Indeed, although half the implementations of .capable are nonsense already,
> so I'm treating that one as a secondary priority for the moment (with an aim
> to come back afterwards and just try to kill it off as far as possible).
> RDMA and VFIO shouldn't be a serious concern for the kind of systems with
> heterogeneous IOMMUs at this point.

Well, lets see:

drivers/infiniband/hw/usnic/usnic_uiom.c:       if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
drivers/vhost/vdpa.c:   if (!iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))

These are kind of hacky ways to say "userspace can actually do DMA
because we don't need privileged cache flush instructions on this
platform". I would love it if these could be moved to some struct
device API - I've aruged with Christoph a couple of times we need
something like that..

drivers/vfio/vfio_iommu_type1.c:        if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))

This is doing the above, and also the no-snoop mess that Intel has
mixed in. How to exactly properly expose their special no-snoop
behavior is definitely something that should be on the domain.

drivers/pci/controller/vmd.c:   if (iommu_capable(vmd->dev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
drivers/vfio/vfio_iommu_type1.c:                    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);

Not sure about VMD, but the VFIO one is a security statement. It could
be quite happy as a domain query, or a flag 'require secure MSI
interrupts' as input to attach_domain.

> > > solving it on my own and end up deleting
> > > iommu_group_replace_domain() in about 6 months' time anyway.
> > 
> > I expect this API to remain until we figure out a solution to the PPC
> > problem, and come up with an alternative way to change the attached
> > domain on the fly.
> 
> I though PPC wasn't using the IOMMU API at all... or is that the problem?

It needs it both ways, a way to get all the DMA security properties
from Lu's series without currently using an iommu_domain to get
them. So the design is to attach a NULL domain for PPC and leave it
like that.

It is surely eventually fixable to introduce a domain to PPC, I would
just prefer we not make anything contingent on actually doing that. :\

Jason

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Joerg Roedel]

On Mon, Feb 14, 2022 at 11:46:26AM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 14, 2022 at 03:18:31PM +0000, Robin Murphy wrote:
> 
> > Arguably, iommu_attach_device() could be renamed something like
> > iommu_attach_group_for_dev(), since that's effectively the semantic that all
> > the existing API users want anyway (even VFIO at the high level - the group
> > is the means for the user to assign their GPU/NIC/whatever device to their
> > process, not the end in itself). That's just a lot more churn.
> 
> Right

Okay, good point. I can live with an iommu_attach_group_for_dev()
interface, it is still better than making iommu_attach_device() silently
operate on whole groups.

> VFIO needs them because its uAPI is tied, but even so we keep talking
> about ways to narrow the amount of group API it consumes.
> 
> We should not set the recommended/good kAPI based on VFIOs uAPI
> design.

Agree here too. The current way groups are implemented can be turned
into a VFIO specific interface to keep its semantics and kABI. But the
IOMMU core code still needs the concept of alias groups.

Regards,

	Joerg

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Joerg Roedel]

On Mon, Feb 14, 2022 at 11:00:59AM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 14, 2022 at 03:23:07PM +0100, Joerg Roedel wrote:
> 
> > Device drivers calling into iommu_attach_device() is seldom a good
> > idea.  In this case the sound device has some generic hardware
> > interface so that an existing sound driver can be re-used. Making this
> > driver call iommu-specific functions for some devices is something hard
> > to justify.
> 
> Er, so this is transparent to the generic sound device? I guess
> something fixed up the dma_api on that device to keep working?

Right, this is completly transparent to the sound device. The IOMMU code
will not set dma_ops on the device because it uses a direct mapping and
so the standard implementation will be used.

> But, then, the requirement is that nobody is using the dma API when we
> make this change?

That is the tricky part. DMA-API keeps working after the change is made,
because the new domain is also direct mapped. The new domain just has
the ability to assign host page-tables to device PASIDs, so that DMA
requests with a PASID TLP will be remapped.

It was actually a requirement for this code that when it jumps in, the
DMA-API mappings stay live. And the reason a direct mapping is used at
all is that the page-table walker of the IOMMU is a two-dimensional
walker, which will treat the addresses found in the host page-tables as
IO-virtual an translates them through the underlying page-table. So to
use host-pagetables the underlying mapping must be direct mapped.


> I don't think it matters how big/small the group is, only that when we
> change the domain we know everything flowing through the domain is
> still happy.

Yes, that matters. The group size matters too for DMA-API performance.
If two devices compete for the same lock in the allocator and/or the
same cached magazines, things will slow down. That only matters for
high-throughput devices, but still...

Regards,

	Joerg

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Robin Murphy]

On 2022-02-15 09:11, Joerg Roedel wrote:
> On Mon, Feb 14, 2022 at 11:00:59AM -0400, Jason Gunthorpe wrote:
>> On Mon, Feb 14, 2022 at 03:23:07PM +0100, Joerg Roedel wrote:
>>
>>> Device drivers calling into iommu_attach_device() is seldom a good
>>> idea.  In this case the sound device has some generic hardware
>>> interface so that an existing sound driver can be re-used. Making this
>>> driver call iommu-specific functions for some devices is something hard
>>> to justify.
>>
>> Er, so this is transparent to the generic sound device? I guess
>> something fixed up the dma_api on that device to keep working?
> 
> Right, this is completly transparent to the sound device. The IOMMU code
> will not set dma_ops on the device because it uses a direct mapping and
> so the standard implementation will be used.
> 
>> But, then, the requirement is that nobody is using the dma API when we
>> make this change?
> 
> That is the tricky part. DMA-API keeps working after the change is made,
> because the new domain is also direct mapped. The new domain just has
> the ability to assign host page-tables to device PASIDs, so that DMA
> requests with a PASID TLP will be remapped.
> 
> It was actually a requirement for this code that when it jumps in, the
> DMA-API mappings stay live. And the reason a direct mapping is used at
> all is that the page-table walker of the IOMMU is a two-dimensional
> walker, which will treat the addresses found in the host page-tables as
> IO-virtual an translates them through the underlying page-table. So to
> use host-pagetables the underlying mapping must be direct mapped.

Given how things have evolved since that code was originally written, 
and that we seemingly now have the def_domain_type override kicking in 
as soon as we first see an IOMMUv2-capable device, do we even need to 
then subsequently switch to this special unmanaged domain with its 
pagetable sucked out, or could we just install the PASID table in the 
default domain itself?

Robin.

>> I don't think it matters how big/small the group is, only that when we
>> change the domain we know everything flowing through the domain is
>> still happy.
> 
> Yes, that matters. The group size matters too for DMA-API performance.
> If two devices compete for the same lock in the allocator and/or the
> same cached magazines, things will slow down. That only matters for
> high-throughput devices, but still...
> 
> Regards,
> 
> 	Joerg
> 

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Jason Gunthorpe]

On Tue, Feb 15, 2022 at 09:58:13AM +0100, Joerg Roedel wrote:
> On Mon, Feb 14, 2022 at 11:46:26AM -0400, Jason Gunthorpe wrote:
> > On Mon, Feb 14, 2022 at 03:18:31PM +0000, Robin Murphy wrote:
> > 
> > > Arguably, iommu_attach_device() could be renamed something like
> > > iommu_attach_group_for_dev(), since that's effectively the semantic that all
> > > the existing API users want anyway (even VFIO at the high level - the group
> > > is the means for the user to assign their GPU/NIC/whatever device to their
> > > process, not the end in itself). That's just a lot more churn.
> > 
> > Right
> 
> Okay, good point. I can live with an iommu_attach_group_for_dev()
> interface, it is still better than making iommu_attach_device() silently
> operate on whole groups.

I think this is what Lu's series currently does, it just doesn't do
the rename churn as Robin noted. Lu, why not add a note like Robin
explained to the kdoc so it is clear this api impacts the whole group?

There is no argument that the internal operation of the iommu layer
should always be using groups - we are just presenting a simplified
API toward drivers.

Jason

Re: [PATCH v1 5/8] iommu/amd: Use iommu_attach/detach_device()

[Jason Gunthorpe]

On Tue, Feb 15, 2022 at 10:11:01AM +0100, Joerg Roedel wrote:

> > But, then, the requirement is that nobody is using the dma API when we
> > make this change?
> 
> That is the tricky part. DMA-API keeps working after the change is made,
> because the new domain is also direct mapped. The new domain just has
> the ability to assign host page-tables to device PASIDs, so that DMA
> requests with a PASID TLP will be remapped.

From, what you've described this is also a good use case for the
replace_group idea..

Using Lu's series we'd:
 - Require the group to be in DMA-API mode. We know this is the case
   because of how we got here from probe()
 - Use the 'replace group's domain' API to switch the group from one
   identity translation domain to another identity translation domain
 - Rely on the DMA-API refcount to block VFIO/etc instead of checking
   for the default domain
 - Restore the default domain when all the DMA-API mode drivers unprobe

That is, if Robin's idea to just get the right domain in the first
place doesn't work.

Anyhow, given all this I think this patch is not OK as is.

Thanks,
Jason

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Lu Baolu]

On 2/15/22 9:47 PM, Jason Gunthorpe via iommu wrote:
> On Tue, Feb 15, 2022 at 09:58:13AM +0100, Joerg Roedel wrote:
>> On Mon, Feb 14, 2022 at 11:46:26AM -0400, Jason Gunthorpe wrote:
>>> On Mon, Feb 14, 2022 at 03:18:31PM +0000, Robin Murphy wrote:
>>>
>>>> Arguably, iommu_attach_device() could be renamed something like
>>>> iommu_attach_group_for_dev(), since that's effectively the semantic that all
>>>> the existing API users want anyway (even VFIO at the high level - the group
>>>> is the means for the user to assign their GPU/NIC/whatever device to their
>>>> process, not the end in itself). That's just a lot more churn.
>>>
>>> Right
>>
>> Okay, good point. I can live with an iommu_attach_group_for_dev()
>> interface, it is still better than making iommu_attach_device() silently
>> operate on whole groups.
> 
> I think this is what Lu's series currently does, it just doesn't do
> the rename churn as Robin noted. Lu, why not add a note like Robin
> explained to the kdoc so it is clear this api impacts the whole group?

I feel that the debate here is not about API name, but how should
iommu_attach/detach_device() be implemented and used.

It seems everyone agrees that for device assignment (where the I/O
address is owned by the user-space application), the iommu_group-based
APIs should always be used. Otherwise, the isolation and protection are
not guaranteed.

For kernel DMA (where the I/O address space is owned by the kernel
drivers), the device driver oriented interface should meet below
expectations:

  - the concept of iommu_group should be transparent to the device
    drivers;
  - but internally, iommu core only allows a single domain to attach to
    a group.

If the device driver uses default domain, the above expectations are
naturally met. But when the driver wants to attach its own domain, the
problem arises.

This series tries to use the DMA ownership mechanism to solve this. The
devices drivers explicitly declare that

  - I know that the device I am driving shares the iommu_group with
    others;
  - Other device drivers with the same awareness can only be bound to the
    devices in the shared group;
  - We can sync with each other so that only a shared domain could be
    attached to the devices in the group.

Another proposal (as suggested by Joerg) is to introduce the concept of
"sub-group". An iommu group could have one or multiple sub-groups with
non-aliased devices sitting in different sub-groups and use different
domains.

Above are what I get so far. If there's any misunderstanding, please
help to correct.

Best regards,
baolu

Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Jason Gunthorpe]

On Wed, Feb 16, 2022 at 02:28:09PM +0800, Lu Baolu wrote:

> It seems everyone agrees that for device assignment (where the I/O
> address is owned by the user-space application), the iommu_group-based
> APIs should always be used. Otherwise, the isolation and protection are
> not guaranteed.

This group/device split is all just driven by VFIO. There is nothing
preventing a struct device * API from being used with user-space, and
Robin has been pushing that way. With enough fixing of VFIO we can do
it.

eg the device-centric VFIO patches should be able to eventually work
entirely on an iommu device API.

> Another proposal (as suggested by Joerg) is to introduce the concept of
> "sub-group". An iommu group could have one or multiple sub-groups with
> non-aliased devices sitting in different sub-groups and use different
> domains.

I still don't see how sub groups help or really change anything here.

The API already has the concept of 'ownership' seperated from the
concept of 'attach a domain to a device'.

Ownership works on the ACS group and attach works on the 'same RID'
group.

The API can take in the struct device and select which internal group
to use based on which action is being done.

Jason