[PATCH v3 0/7] Iommu: Retire bus ops

[PATCH v3 0/7] Iommu: Retire bus ops

[Robin Murphy]

v2: https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/

Hi all,

I've finally been able to get back to this again, and I think it is now
hopefully ready to go. Changes from v3 are quite minor - basically a few
cosmetics and small tweaks (where I've taken the liberty of keeping
Baolu and Jason's review tags; hope that's OK!), the one functional
thing around blocking domains fixed, and plenty of reshuffling from
rebases. I'm happy to see that the IOMMUFD selftest problem has resolved
itself in the meantime, and it might even be able to use the standard
registration flow after this, however I'll leave that for someone else
more motivated, since my follow-up priority will be moving the of_xlate
business around at the bus level to sort out the probe_device ordering
mess once and for all.

Thanks,
Robin.


Robin Murphy (7):
  iommu: Factor out some helpers
  iommu: Decouple iommu_present() from bus ops
  iommu: Validate that devices match domains
  iommu: Switch __iommu_domain_alloc() to device ops
  iommu/arm-smmu: Don't register fwnode for legacy binding
  iommu: Retire bus ops
  iommu: Clean up open-coded ownership checks

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |   3 -
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  12 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |  14 +-
 drivers/iommu/iommu.c                       | 140 ++++++++++++++------
 drivers/iommu/mtk_iommu.c                   |   7 +-
 drivers/iommu/mtk_iommu_v1.c                |   3 -
 drivers/iommu/sprd-iommu.c                  |   8 +-
 drivers/iommu/virtio-iommu.c                |   3 -
 include/acpi/acpi_bus.h                     |   2 +
 include/linux/device.h                      |   1 -
 include/linux/device/bus.h                  |   5 -
 include/linux/dma-map-ops.h                 |   1 +
 include/linux/iommu.h                       |   1 +
 13 files changed, 108 insertions(+), 92 deletions(-)

-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 1/7] iommu: Factor out some helpers

[Robin Murphy]

The pattern for picking the first device out of the group list is
repeated a few times now, so it's clearly worth factoring out, which
also helps hide the iommu_group_dev detail from places that don't need
to know. Similarly, the safety check for dev_iommu_ops() at certain
public interfaces starts looking a bit repetitive, and might not be
completely obvious at first glance, so let's factor that out for clarity
as well, in preparation for more uses of both.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---

v3: Rename dev_iommu_ops_valid() to reflect what it's actually checking,
    rather than an implied consequence.
---
 drivers/iommu/iommu.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3bfc56df4f78..4566d0001cd3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -363,6 +363,15 @@ static void dev_iommu_free(struct device *dev)
 	kfree(param);
 }
 
+/*
+ * Internal equivalent of device_iommu_mapped() for when we care that a device
+ * actually has API ops, and don't want false positives from VFIO-only groups.
+ */
+static bool dev_has_iommu(struct device *dev)
+{
+	return dev->iommu && dev->iommu->iommu_dev;
+}
+
 static u32 dev_iommu_get_max_pasids(struct device *dev)
 {
 	u32 max_pasids = 0, bits = 0;
@@ -614,7 +623,7 @@ static void __iommu_group_remove_device(struct device *dev)
 
 		list_del(&device->list);
 		__iommu_group_free_device(group, device);
-		if (dev->iommu && dev->iommu->iommu_dev)
+		if (dev_has_iommu(dev))
 			iommu_deinit_device(dev);
 		else
 			dev->iommu_group = NULL;
@@ -1218,6 +1227,12 @@ void iommu_group_remove_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
+static struct device *iommu_group_first_dev(struct iommu_group *group)
+{
+	lockdep_assert_held(&group->mutex);
+	return list_first_entry(&group->devices, struct group_device, list)->dev;
+}
+
 /**
  * iommu_group_for_each_dev - iterate over each device in the group
  * @group: the group
@@ -1745,9 +1760,7 @@ __iommu_group_alloc_default_domain(const struct bus_type *bus,
 static struct iommu_domain *
 iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
-	const struct bus_type *bus =
-		list_first_entry(&group->devices, struct group_device, list)
-			->dev->bus;
+	const struct bus_type *bus = iommu_group_first_dev(group)->bus;
 	struct iommu_domain *dom;
 
 	lockdep_assert_held(&group->mutex);
@@ -1912,7 +1925,7 @@ bool device_iommu_capable(struct device *dev, enum iommu_cap cap)
 {
 	const struct iommu_ops *ops;
 
-	if (!dev->iommu || !dev->iommu->iommu_dev)
+	if (!dev_has_iommu(dev))
 		return false;
 
 	ops = dev_iommu_ops(dev);
@@ -2903,8 +2916,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
  */
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-	if (dev->iommu && dev->iommu->iommu_dev) {
-		const struct iommu_ops *ops = dev->iommu->iommu_dev->ops;
+	if (dev_has_iommu(dev)) {
+		const struct iommu_ops *ops = dev_iommu_ops(dev);
 
 		if (ops->dev_enable_feat)
 			return ops->dev_enable_feat(dev, feat);
@@ -2919,8 +2932,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
  */
 int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-	if (dev->iommu && dev->iommu->iommu_dev) {
-		const struct iommu_ops *ops = dev->iommu->iommu_dev->ops;
+	if (dev_has_iommu(dev)) {
+		const struct iommu_ops *ops = dev_iommu_ops(dev);
 
 		if (ops->dev_disable_feat)
 			return ops->dev_disable_feat(dev, feat);
@@ -3190,21 +3203,18 @@ void iommu_device_unuse_default_domain(struct device *dev)
 
 static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 {
-	struct group_device *dev =
-		list_first_entry(&group->devices, struct group_device, list);
+	struct device *dev = iommu_group_first_dev(group);
 
 	if (group->blocking_domain)
 		return 0;
 
-	group->blocking_domain =
-		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
+	group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
 	if (!group->blocking_domain) {
 		/*
 		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
 		 * create an empty domain instead.
 		 */
-		group->blocking_domain = __iommu_domain_alloc(
-			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
+		group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
 		if (!group->blocking_domain)
 			return -EINVAL;
 	}
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 2/7] iommu: Decouple iommu_present() from bus ops

[Robin Murphy]

Much as I'd like to remove iommu_present(), the final remaining users
are proving stubbornly difficult to clean up, so kick that can down
the road and just rework it to preserve the current behaviour without
depending on bus ops.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---

v3: Tweak to use the ops-based check rather than group-based, to
    properly match the existing behaviour
---
 drivers/iommu/iommu.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4566d0001cd3..2f29ee9dea64 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1907,9 +1907,24 @@ int bus_iommu_probe(const struct bus_type *bus)
 	return 0;
 }
 
+static int __iommu_present(struct device *dev, void *unused)
+{
+	return dev_has_iommu(dev);
+}
+
+/**
+ * iommu_present() - make platform-specific assumptions about an IOMMU
+ * @bus: bus to check
+ *
+ * Do not use this function. You want device_iommu_mapped() instead.
+ *
+ * Return: true if some IOMMU is present for some device on the given bus. In
+ * general it may not be the only IOMMU, and it may not be for the device you
+ * are ultimately interested in.
+ */
 bool iommu_present(const struct bus_type *bus)
 {
-	return bus->iommu_ops != NULL;
+	return bus_for_each_dev(bus, NULL, NULL, __iommu_present) > 0;
 }
 EXPORT_SYMBOL_GPL(iommu_present);
 
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 3/7] iommu: Validate that devices match domains

[Robin Murphy]

Before we can allow drivers to coexist, we need to make sure that one
driver's domain ops can't misinterpret another driver's dev_iommu_priv
data. To that end, add a token to the domain so we can remember how it
was allocated - for now this may as well be the device ops, since they
still correlate 1:1 with drivers. We can trust ourselves for internal
default domain attachment, so add the check where it covers both the
external attach interfaces.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 13 +++++++++----
 include/linux/iommu.h |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2f29ee9dea64..f4cc91227b22 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2000,26 +2000,28 @@ EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
 						 unsigned type)
 {
+	const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
 	struct iommu_domain *domain;
 	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
 
-	if (bus == NULL || bus->iommu_ops == NULL)
+	if (!ops)
 		return NULL;
 
-	domain = bus->iommu_ops->domain_alloc(alloc_type);
+	domain = ops->domain_alloc(alloc_type);
 	if (!domain)
 		return NULL;
 
 	domain->type = type;
+	domain->owner = ops;
 	/*
 	 * If not already set, assume all sizes by default; the driver
 	 * may override this later
 	 */
 	if (!domain->pgsize_bitmap)
-		domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
+		domain->pgsize_bitmap = ops->pgsize_bitmap;
 
 	if (!domain->ops)
-		domain->ops = bus->iommu_ops->default_domain_ops;
+		domain->ops = ops->default_domain_ops;
 
 	if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
 		iommu_domain_free(domain);
@@ -2176,6 +2178,9 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 	    group->domain != group->blocking_domain)
 		return -EBUSY;
 
+	if (dev_iommu_ops(iommu_group_first_dev(group)) != domain->owner)
+		return -EINVAL;
+
 	return __iommu_group_set_domain(group, domain);
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a249e10c8e9f..75ffcac199e3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -95,6 +95,7 @@ struct iommu_domain_geometry {
 struct iommu_domain {
 	unsigned type;
 	const struct iommu_domain_ops *ops;
+	const struct iommu_ops *owner; /* Whose domain_alloc we came from */
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	struct iommu_domain_geometry geometry;
 	struct iommu_dma_cookie *iova_cookie;
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 4/7] iommu: Switch __iommu_domain_alloc() to device ops

[Robin Murphy]

In all the places we allocate default domains, we have (or can easily
get hold of) a device from which to resolve the right IOMMU ops; only
the public iommu_domain_alloc() interface actually depends on bus ops.
Reworking the public API is a big enough mission in its own right, but
in the meantime we can still decouple it from bus ops internally to move
forward.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---

v3: Make sure blocking domains are covered as well
---
 drivers/iommu/iommu.c | 50 +++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f4cc91227b22..29ebb4b57df4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -96,7 +96,7 @@ static const char * const iommu_group_resv_type_string[] = {
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
 static void iommu_release_device(struct device *dev);
-static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
+static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
@@ -1745,12 +1745,11 @@ static int iommu_get_def_domain_type(struct device *dev)
 }
 
 static struct iommu_domain *
-__iommu_group_alloc_default_domain(const struct bus_type *bus,
-				   struct iommu_group *group, int req_type)
+__iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
 	if (group->default_domain && group->default_domain->type == req_type)
 		return group->default_domain;
-	return __iommu_domain_alloc(bus, req_type);
+	return __iommu_domain_alloc(iommu_group_first_dev(group), req_type);
 }
 
 /*
@@ -1760,23 +1759,22 @@ __iommu_group_alloc_default_domain(const struct bus_type *bus,
 static struct iommu_domain *
 iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
-	const struct bus_type *bus = iommu_group_first_dev(group)->bus;
 	struct iommu_domain *dom;
 
 	lockdep_assert_held(&group->mutex);
 
 	if (req_type)
-		return __iommu_group_alloc_default_domain(bus, group, req_type);
+		return __iommu_group_alloc_default_domain(group, req_type);
 
 	/* The driver gave no guidance on what type to use, try the default */
-	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
+	dom = __iommu_group_alloc_default_domain(group, iommu_def_domain_type);
 	if (dom)
 		return dom;
 
 	/* Otherwise IDENTITY and DMA_FQ defaults will try DMA */
 	if (iommu_def_domain_type == IOMMU_DOMAIN_DMA)
 		return NULL;
-	dom = __iommu_group_alloc_default_domain(bus, group, IOMMU_DOMAIN_DMA);
+	dom = __iommu_group_alloc_default_domain(group, IOMMU_DOMAIN_DMA);
 	if (!dom)
 		return NULL;
 
@@ -1997,16 +1995,13 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
-static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
+static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
 						 unsigned type)
 {
-	const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_domain *domain;
 	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
 
-	if (!ops)
-		return NULL;
-
 	domain = ops->domain_alloc(alloc_type);
 	if (!domain)
 		return NULL;
@@ -2030,9 +2025,28 @@ static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
 	return domain;
 }
 
+static int __iommu_domain_alloc_dev(struct device *dev, void *data)
+{
+	struct device **alloc_dev = data;
+
+	if (!dev_has_iommu(dev))
+		return 0;
+
+	WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != dev_iommu_ops(*alloc_dev),
+		  "Multiple IOMMU drivers present, which the public IOMMU API can't fully support yet. You may still need to disable one or more to get the expected result here, sorry!\n");
+
+	*alloc_dev = dev;
+	return 0;
+}
+
 struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
 {
-	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+	struct device *dev = NULL;
+
+	if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
+		return NULL;
+
+	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
 }
 EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
@@ -3228,13 +3242,17 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 	if (group->blocking_domain)
 		return 0;
 
-	group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
+	/* noiommu groups should never be here */
+	if (WARN_ON(!dev_has_iommu(dev)))
+		return -ENODEV;
+
+	group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_BLOCKED);
 	if (!group->blocking_domain) {
 		/*
 		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
 		 * create an empty domain instead.
 		 */
-		group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
+		group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
 		if (!group->blocking_domain)
 			return -EINVAL;
 	}
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 5/7] iommu/arm-smmu: Don't register fwnode for legacy binding

[Robin Murphy]

When using the legacy binding we bypass the of_xlate mechanism, so avoid
registering the instance fwnodes which act as keys for that. This will
help __iommu_probe_device() to retrieve the registered ops the same way
as for x86 etc. when no fwspec has previously been set up by of_xlate.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc0..4b83a3adacd6 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2161,7 +2161,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
+	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
+				    using_legacy_binding ? NULL : dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
 		iommu_device_sysfs_remove(&smmu->iommu);
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 6/7] iommu: Retire bus ops

[Robin Murphy]

With the rest of the API internals converted, it's time to finally
tackle probe_device and how we bootstrap the per-device ops association
to begin with. This ends up being disappointingly straightforward, since
fwspec users are already doing it in order to find their of_xlate
callback, and it works out that we can easily do the equivalent for
other drivers too. Then shuffle the remaining awareness of iommu_ops
into the couple of core headers that still need it, and breathe a sigh
of relief.

Ding dong the bus ops are gone!

CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Christoph Hellwig <hch@lst.de>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c       | 30 ++++++++++++++++++------------
 include/acpi/acpi_bus.h     |  2 ++
 include/linux/device.h      |  1 -
 include/linux/device/bus.h  |  5 -----
 include/linux/dma-map-ops.h |  1 +
 5 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 29ebb4b57df4..d38c3023887a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -149,7 +149,7 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
 static LIST_HEAD(iommu_device_list);
 static DEFINE_SPINLOCK(iommu_device_lock);
 
-static struct bus_type * const iommu_buses[] = {
+static const struct bus_type * const iommu_buses[] = {
 	&platform_bus_type,
 #ifdef CONFIG_PCI
 	&pci_bus_type,
@@ -256,13 +256,6 @@ int iommu_device_register(struct iommu_device *iommu,
 	/* We need to be able to take module references appropriately */
 	if (WARN_ON(is_module_address((unsigned long)ops) && !ops->owner))
 		return -EINVAL;
-	/*
-	 * Temporarily enforce global restriction to a single driver. This was
-	 * already the de-facto behaviour, since any possible combination of
-	 * existing drivers would compete for at least the PCI or platform bus.
-	 */
-	if (iommu_buses[0]->iommu_ops && iommu_buses[0]->iommu_ops != ops)
-		return -EBUSY;
 
 	iommu->ops = ops;
 	if (hwdev)
@@ -272,10 +265,8 @@ int iommu_device_register(struct iommu_device *iommu,
 	list_add_tail(&iommu->list, &iommu_device_list);
 	spin_unlock(&iommu_device_lock);
 
-	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
-		iommu_buses[i]->iommu_ops = ops;
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++)
 		err = bus_iommu_probe(iommu_buses[i]);
-	}
 	if (err)
 		iommu_device_unregister(iommu);
 	return err;
@@ -490,12 +481,27 @@ static void iommu_deinit_device(struct device *dev)
 
 static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
+	const struct iommu_ops *ops;
+	struct iommu_fwspec *fwspec;
 	struct iommu_group *group;
 	static DEFINE_MUTEX(iommu_probe_device_lock);
 	struct group_device *gdev;
 	int ret;
 
+	/*
+	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
+	 * instances with non-NULL fwnodes, and client devices should have been
+	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
+	 * be present, and that any of their registered instances has suitable
+	 * ops for probing, and thus cheekily co-opt the same mechanism.
+	 */
+	fwspec = dev_iommu_fwspec_get(dev);
+	if (fwspec && fwspec->ops)
+		ops = fwspec->ops;
+	else
+		ops = iommu_ops_from_fwnode(NULL);
+
 	if (!ops)
 		return -ENODEV;
 	/*
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 254685085c82..13d959b3ba29 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -623,6 +623,8 @@ struct acpi_pci_root {
 
 /* helper */
 
+struct iommu_ops;
+
 bool acpi_dma_supported(const struct acpi_device *adev);
 enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
 int acpi_iommu_fwspec_init(struct device *dev, u32 id,
diff --git a/include/linux/device.h b/include/linux/device.h
index 56d93a1ffb7b..b78e66f3b34a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -42,7 +42,6 @@ struct class;
 struct subsys_private;
 struct device_node;
 struct fwnode_handle;
-struct iommu_ops;
 struct iommu_group;
 struct dev_pin_info;
 struct dev_iommu;
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index ae10c4322754..e25aab08f873 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -62,9 +62,6 @@ struct fwnode_handle;
  *			this bus.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
- * @iommu_ops:  IOMMU specific operations for this bus, used to attach IOMMU
- *              driver implementations to a bus and allow the driver to do
- *              bus-specific setup
  * @need_parent_lock:	When probing or removing a device on this bus, the
  *			device core should lock the device's parent.
  *
@@ -104,8 +101,6 @@ struct bus_type {
 
 	const struct dev_pm_ops *pm;
 
-	const struct iommu_ops *iommu_ops;
-
 	bool need_parent_lock;
 };
 
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index f2fc203fb8a1..a52e508d1869 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 
 struct cma;
+struct iommu_ops;
 
 /*
  * Values for struct dma_map_ops.flags:
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 7/7] iommu: Clean up open-coded ownership checks

[Robin Murphy]

Some drivers already implement their own defence against the possibility
of being given someone else's device. Since this is now taken care of by
the core code (and via a slightly different path from the original
fwspec-based idea), let's clean them up.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  3 ---
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  9 +--------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     | 14 ++------------
 drivers/iommu/mtk_iommu.c                   |  7 +------
 drivers/iommu/mtk_iommu_v1.c                |  3 ---
 drivers/iommu/sprd-iommu.c                  |  8 +-------
 drivers/iommu/virtio-iommu.c                |  3 ---
 7 files changed, 5 insertions(+), 42 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e82bf1c449a3..01ea307c7791 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2653,9 +2653,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 	struct arm_smmu_master *master;
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 
-	if (!fwspec || fwspec->ops != &arm_smmu_ops)
-		return ERR_PTR(-ENODEV);
-
 	if (WARN_ON_ONCE(dev_iommu_priv_get(dev)))
 		return ERR_PTR(-EBUSY);
 
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4b83a3adacd6..4d09c0047892 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1116,11 +1116,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	struct arm_smmu_device *smmu;
 	int ret;
 
-	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
-		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
-		return -ENXIO;
-	}
-
 	/*
 	 * FIXME: The arch/arm DMA API code tries to attach devices to its own
 	 * domains between of_xlate() and probe_device() - we have no way to cope
@@ -1357,10 +1352,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 		fwspec = dev_iommu_fwspec_get(dev);
 		if (ret)
 			goto out_free;
-	} else if (fwspec && fwspec->ops == &arm_smmu_ops) {
-		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
 	} else {
-		return ERR_PTR(-ENODEV);
+		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
 	}
 
 	ret = -EINVAL;
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 775a3cbaff4e..b86fcd761ff7 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -79,16 +79,6 @@ static struct qcom_iommu_domain *to_qcom_iommu_domain(struct iommu_domain *dom)
 
 static const struct iommu_ops qcom_iommu_ops;
 
-static struct qcom_iommu_dev * to_iommu(struct device *dev)
-{
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
-	if (!fwspec || fwspec->ops != &qcom_iommu_ops)
-		return NULL;
-
-	return dev_iommu_priv_get(dev);
-}
-
 static struct qcom_iommu_ctx * to_ctx(struct qcom_iommu_domain *d, unsigned asid)
 {
 	struct qcom_iommu_dev *qcom_iommu = d->iommu;
@@ -374,7 +364,7 @@ static void qcom_iommu_domain_free(struct iommu_domain *domain)
 
 static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
-	struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
+	struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
 	struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
 	int ret;
 
@@ -499,7 +489,7 @@ static bool qcom_iommu_capable(struct device *dev, enum iommu_cap cap)
 
 static struct iommu_device *qcom_iommu_probe_device(struct device *dev)
 {
-	struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
+	struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
 	struct device_link *link;
 
 	if (!qcom_iommu)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 640275873a27..dc292aeb6dbf 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -844,16 +844,11 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
 static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct mtk_iommu_data *data;
+	struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
 	struct device_link *link;
 	struct device *larbdev;
 	unsigned int larbid, larbidx, i;
 
-	if (!fwspec || fwspec->ops != &mtk_iommu_ops)
-		return ERR_PTR(-ENODEV); /* Not a iommu client device */
-
-	data = dev_iommu_priv_get(dev);
-
 	if (!MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM))
 		return &data->iommu;
 
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 8a0a5e5d049f..2a764b73eb32 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -478,9 +478,6 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev)
 		idx++;
 	}
 
-	if (!fwspec || fwspec->ops != &mtk_iommu_v1_ops)
-		return ERR_PTR(-ENODEV); /* Not a iommu client device */
-
 	data = dev_iommu_priv_get(dev);
 
 	/* Link the consumer device with the smi-larb device(supplier) */
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 2fa9afebd4f5..a69680fbb519 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -388,13 +388,7 @@ static phys_addr_t sprd_iommu_iova_to_phys(struct iommu_domain *domain,
 
 static struct iommu_device *sprd_iommu_probe_device(struct device *dev)
 {
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct sprd_iommu_device *sdev;
-
-	if (!fwspec || fwspec->ops != &sprd_iommu_ops)
-		return ERR_PTR(-ENODEV);
-
-	sdev = dev_iommu_priv_get(dev);
+	struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
 
 	return &sdev->iommu;
 }
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 17dcd826f5c2..bb2e795a80d0 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -969,9 +969,6 @@ static struct iommu_device *viommu_probe_device(struct device *dev)
 	struct viommu_dev *viommu = NULL;
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 
-	if (!fwspec || fwspec->ops != &viommu_ops)
-		return ERR_PTR(-ENODEV);
-
 	viommu = viommu_get_by_fwnode(fwspec->iommu_fwnode);
 	if (!viommu)
 		return ERR_PTR(-ENODEV);
-- 
2.39.2.101.g768bb238c484.dirty

Re: [PATCH v3 3/7] iommu: Validate that devices match domains

[Baolu Lu]

On 9/16/23 12:58 AM, Robin Murphy wrote:
> Before we can allow drivers to coexist, we need to make sure that one
> driver's domain ops can't misinterpret another driver's dev_iommu_priv
> data. To that end, add a token to the domain so we can remember how it
> was allocated - for now this may as well be the device ops, since they
> still correlate 1:1 with drivers. We can trust ourselves for internal
> default domain attachment, so add the check where it covers both the
> external attach interfaces.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/iommu.c | 13 +++++++++----
>   include/linux/iommu.h |  1 +
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 2f29ee9dea64..f4cc91227b22 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2000,26 +2000,28 @@ EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>   static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
>   						 unsigned type)
>   {
> +	const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
>   	struct iommu_domain *domain;
>   	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>   
> -	if (bus == NULL || bus->iommu_ops == NULL)
> +	if (!ops)
>   		return NULL;
>   
> -	domain = bus->iommu_ops->domain_alloc(alloc_type);
> +	domain = ops->domain_alloc(alloc_type);
>   	if (!domain)
>   		return NULL;
>   
>   	domain->type = type;
> +	domain->owner = ops;
>   	/*
>   	 * If not already set, assume all sizes by default; the driver
>   	 * may override this later
>   	 */
>   	if (!domain->pgsize_bitmap)
> -		domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
> +		domain->pgsize_bitmap = ops->pgsize_bitmap;
>   
>   	if (!domain->ops)
> -		domain->ops = bus->iommu_ops->default_domain_ops;
> +		domain->ops = ops->default_domain_ops;
>   
>   	if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
>   		iommu_domain_free(domain);
> @@ -2176,6 +2178,9 @@ static int __iommu_attach_group(struct iommu_domain *domain,
>   	    group->domain != group->blocking_domain)
>   		return -EBUSY;
>   
> +	if (dev_iommu_ops(iommu_group_first_dev(group)) != domain->owner)
> +		return -EINVAL;

Should we apply this check in iommu_attach_device_pasid()?

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3bfc56df4f78..43acf1b8ed56 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3414,6 +3414,9 @@ int iommu_attach_device_pasid(struct iommu_domain 
*domain,
         if (!group)
                 return -ENODEV;

+       if (dev_iommu_ops(dev) != domain->owner)
+               return -EINVAL;
+
         mutex_lock(&group->mutex);
         curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, 
GFP_KERNEL);
         if (curr) {

> +
>   	return __iommu_group_set_domain(group, domain);
>   }
>   
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a249e10c8e9f..75ffcac199e3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -95,6 +95,7 @@ struct iommu_domain_geometry {
>   struct iommu_domain {
>   	unsigned type;
>   	const struct iommu_domain_ops *ops;
> +	const struct iommu_ops *owner; /* Whose domain_alloc we came from */
>   	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
>   	struct iommu_domain_geometry geometry;
>   	struct iommu_dma_cookie *iova_cookie;

Best regards,
baolu

Re: [PATCH v3 4/7] iommu: Switch __iommu_domain_alloc() to device ops

[Baolu Lu]

On 9/16/23 12:58 AM, Robin Murphy wrote:
> @@ -1997,16 +1995,13 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
>   }
>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>   
> -static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
> +static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
>   						 unsigned type)
>   {
> -	const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>   	struct iommu_domain *domain;
>   	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>   
> -	if (!ops)
> -		return NULL;
> -
>   	domain = ops->domain_alloc(alloc_type);
>   	if (!domain)
>   		return NULL;
> @@ -2030,9 +2025,28 @@ static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
>   	return domain;
>   }
>   
> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> +{
> +	struct device **alloc_dev = data;
> +
> +	if (!dev_has_iommu(dev))
> +		return 0;
> +
> +	WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != dev_iommu_ops(*alloc_dev),
> +		  "Multiple IOMMU drivers present, which the public IOMMU API can't fully support yet. You may still need to disable one or more to get the expected result here, sorry!\n");
> +
> +	*alloc_dev = dev;
> +	return 0;
> +}
> +
>   struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
>   {
> -	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> +	struct device *dev = NULL;
> +
> +	if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
> +		return NULL;

__iommu_domain_alloc_dev() always returns 0. Hence above if condition
will never be true. Perhaps, in __iommu_domain_alloc_dev(),

	if (WARN_ON(*alloc_dev && dev_iommu_ops(dev) !=
             dev_iommu_ops(*alloc_dev))
		return -EPERM;

?

> +
> +	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);

Is it possible that all devices on this bus have dev_has_iommu() to be
false? If so, we probably need something like below:

	if (!dev_has_iommu(dev))
		return -ENODEV;

?

>   }
>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>   
> @@ -3228,13 +3242,17 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
>   	if (group->blocking_domain)
>   		return 0;
>   
> -	group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
> +	/* noiommu groups should never be here */
> +	if (WARN_ON(!dev_has_iommu(dev)))
> +		return -ENODEV;
> +
> +	group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_BLOCKED);
>   	if (!group->blocking_domain) {
>   		/*
>   		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
>   		 * create an empty domain instead.
>   		 */
> -		group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +		group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
>   		if (!group->blocking_domain)
>   			return -EINVAL;
>   	}

Best regards,
baolu

Re: [PATCH v3 3/7] iommu: Validate that devices match domains

[Robin Murphy]

On 2023-09-18 06:49, Baolu Lu wrote:
> On 9/16/23 12:58 AM, Robin Murphy wrote:
>> Before we can allow drivers to coexist, we need to make sure that one
>> driver's domain ops can't misinterpret another driver's dev_iommu_priv
>> data. To that end, add a token to the domain so we can remember how it
>> was allocated - for now this may as well be the device ops, since they
>> still correlate 1:1 with drivers. We can trust ourselves for internal
>> default domain attachment, so add the check where it covers both the
>> external attach interfaces.
>>
>> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/iommu.c | 13 +++++++++----
>>   include/linux/iommu.h |  1 +
>>   2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 2f29ee9dea64..f4cc91227b22 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2000,26 +2000,28 @@ EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>>   static struct iommu_domain *__iommu_domain_alloc(const struct 
>> bus_type *bus,
>>                            unsigned type)
>>   {
>> +    const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
>>       struct iommu_domain *domain;
>>       unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>> -    if (bus == NULL || bus->iommu_ops == NULL)
>> +    if (!ops)
>>           return NULL;
>> -    domain = bus->iommu_ops->domain_alloc(alloc_type);
>> +    domain = ops->domain_alloc(alloc_type);
>>       if (!domain)
>>           return NULL;
>>       domain->type = type;
>> +    domain->owner = ops;
>>       /*
>>        * If not already set, assume all sizes by default; the driver
>>        * may override this later
>>        */
>>       if (!domain->pgsize_bitmap)
>> -        domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>> +        domain->pgsize_bitmap = ops->pgsize_bitmap;
>>       if (!domain->ops)
>> -        domain->ops = bus->iommu_ops->default_domain_ops;
>> +        domain->ops = ops->default_domain_ops;
>>       if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
>>           iommu_domain_free(domain);
>> @@ -2176,6 +2178,9 @@ static int __iommu_attach_group(struct 
>> iommu_domain *domain,
>>           group->domain != group->blocking_domain)
>>           return -EBUSY;
>> +    if (dev_iommu_ops(iommu_group_first_dev(group)) != domain->owner)
>> +        return -EINVAL;
> 
> Should we apply this check in iommu_attach_device_pasid()?

Oh, probably - I don't think I've really noticed that we've grown yet 
another attach interface since I started this. Looks like current users 
are passing appropriate domains by construction, but I concur that that 
path wants covering as well for completeness.

Thanks,
Robin.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3bfc56df4f78..43acf1b8ed56 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3414,6 +3414,9 @@ int iommu_attach_device_pasid(struct iommu_domain 
> *domain,
>          if (!group)
>                  return -ENODEV;
> 
> +       if (dev_iommu_ops(dev) != domain->owner)
> +               return -EINVAL;
> +
>          mutex_lock(&group->mutex);
>          curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, 
> GFP_KERNEL);
>          if (curr) {
> 
>> +
>>       return __iommu_group_set_domain(group, domain);
>>   }
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index a249e10c8e9f..75ffcac199e3 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -95,6 +95,7 @@ struct iommu_domain_geometry {
>>   struct iommu_domain {
>>       unsigned type;
>>       const struct iommu_domain_ops *ops;
>> +    const struct iommu_ops *owner; /* Whose domain_alloc we came from */
>>       unsigned long pgsize_bitmap;    /* Bitmap of page sizes in use */
>>       struct iommu_domain_geometry geometry;
>>       struct iommu_dma_cookie *iova_cookie;
> 
> Best regards,
> baolu

Re: [PATCH v3 4/7] iommu: Switch __iommu_domain_alloc() to device ops

[Robin Murphy]

On 2023-09-18 07:10, Baolu Lu wrote:
> On 9/16/23 12:58 AM, Robin Murphy wrote:
>> @@ -1997,16 +1995,13 @@ void iommu_set_fault_handler(struct 
>> iommu_domain *domain,
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>> -static struct iommu_domain *__iommu_domain_alloc(const struct 
>> bus_type *bus,
>> +static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
>>                            unsigned type)
>>   {
>> -    const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
>> +    const struct iommu_ops *ops = dev_iommu_ops(dev);
>>       struct iommu_domain *domain;
>>       unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>> -    if (!ops)
>> -        return NULL;
>> -
>>       domain = ops->domain_alloc(alloc_type);
>>       if (!domain)
>>           return NULL;
>> @@ -2030,9 +2025,28 @@ static struct iommu_domain 
>> *__iommu_domain_alloc(const struct bus_type *bus,
>>       return domain;
>>   }
>> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
>> +{
>> +    struct device **alloc_dev = data;
>> +
>> +    if (!dev_has_iommu(dev))
>> +        return 0;
>> +
>> +    WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != 
>> dev_iommu_ops(*alloc_dev),
>> +          "Multiple IOMMU drivers present, which the public IOMMU API 
>> can't fully support yet. You may still need to disable one or more to 
>> get the expected result here, sorry!\n");
>> +
>> +    *alloc_dev = dev;
>> +    return 0;
>> +}
>> +
>>   struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
>>   {
>> -    return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
>> +    struct device *dev = NULL;
>> +
>> +    if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
>> +        return NULL;
> 
> __iommu_domain_alloc_dev() always returns 0. Hence above if condition
> will never be true. Perhaps, in __iommu_domain_alloc_dev(),

Oh bugger, seems I screwed up the unnecessarily overcomplicated rebase 
that I made for myself, and managed to put this back to the v1 code, so 
it's just wrong (bus_for_each_dev() can return an error itself if the 
bus isn't properly initialised, but it also returns success if the bus 
has no devices, which was handled properly in v2 that you actually R-b'd).

> 
>      if (WARN_ON(*alloc_dev && dev_iommu_ops(dev) !=
>              dev_iommu_ops(*alloc_dev))
>          return -EPERM;

I went back and forth on this initially, but in the end I figured since 
the other patches are meant to be making the rest of the public API 
sufficiently robust, then if someone does try it with multiple drivers 
before full support can be finished, they can at least have some chance 
of getting the desired result, rather than a guarantee of not. I am 
still open to being convinced otherwise, though.

Thanks,
Robin.

> 
> ?
> 
>> +
>> +    return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
> 
> Is it possible that all devices on this bus have dev_has_iommu() to be
> false? If so, we probably need something like below:
> 
>      if (!dev_has_iommu(dev))
>          return -ENODEV;
> 
> ?
> 
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>> @@ -3228,13 +3242,17 @@ static int 
>> __iommu_group_alloc_blocking_domain(struct iommu_group *group)
>>       if (group->blocking_domain)
>>           return 0;
>> -    group->blocking_domain = __iommu_domain_alloc(dev->bus, 
>> IOMMU_DOMAIN_BLOCKED);
>> +    /* noiommu groups should never be here */
>> +    if (WARN_ON(!dev_has_iommu(dev)))
>> +        return -ENODEV;
>> +
>> +    group->blocking_domain = __iommu_domain_alloc(dev, 
>> IOMMU_DOMAIN_BLOCKED);
>>       if (!group->blocking_domain) {
>>           /*
>>            * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
>>            * create an empty domain instead.
>>            */
>> -        group->blocking_domain = __iommu_domain_alloc(dev->bus, 
>> IOMMU_DOMAIN_UNMANAGED);
>> +        group->blocking_domain = __iommu_domain_alloc(dev, 
>> IOMMU_DOMAIN_UNMANAGED);
>>           if (!group->blocking_domain)
>>               return -EINVAL;
>>       }
> 
> Best regards,
> baolu

Re: [PATCH v3 0/7] Iommu: Retire bus ops

[Jason Gunthorpe]

On Fri, Sep 15, 2023 at 05:58:04PM +0100, Robin Murphy wrote:
> v2: https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/
> 
> Hi all,
> 
> I've finally been able to get back to this again, and I think it is now
> hopefully ready to go. Changes from v3 are quite minor - basically a few
> cosmetics and small tweaks (where I've taken the liberty of keeping
> Baolu and Jason's review tags; hope that's OK!), the one functional

Looks OK

> thing around blocking domains fixed, and plenty of reshuffling from
> rebases. I'm happy to see that the IOMMUFD selftest problem has resolved
> itself in the meantime, and it might even be able to use the standard
> registration flow after this, however I'll leave that for someone else
> more motivated,

The main issue is the dedicated bus the test needs.

We can change the two wonky interfaces to two new APIs to add/remove a
new bus from iommu monitoring. Then the normal register APIs are
probably OK after this series.

Jason

Re: [PATCH v3 1/7] iommu: Factor out some helpers

[Jason Gunthorpe]

On Fri, Sep 15, 2023 at 05:58:05PM +0100, Robin Murphy wrote:
> The pattern for picking the first device out of the group list is
> repeated a few times now, so it's clearly worth factoring out, which
> also helps hide the iommu_group_dev detail from places that don't need
> to know. Similarly, the safety check for dev_iommu_ops() at certain
> public interfaces starts looking a bit repetitive, and might not be
> completely obvious at first glance, so let's factor that out for clarity
> as well, in preparation for more uses of both.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> ---
> 
> v3: Rename dev_iommu_ops_valid() to reflect what it's actually checking,
>     rather than an implied consequence.
> ---
>  drivers/iommu/iommu.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3bfc56df4f78..4566d0001cd3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -363,6 +363,15 @@ static void dev_iommu_free(struct device *dev)
>  	kfree(param);
>  }
>  
> +/*
> + * Internal equivalent of device_iommu_mapped() for when we care that a device
> + * actually has API ops, and don't want false positives from VFIO-only groups.
> + */
> +static bool dev_has_iommu(struct device *dev)
> +{
> +	return dev->iommu && dev->iommu->iommu_dev;
> +}

After having gone through all the locking here, I'd prefer to err on
the side of clearer documentation when it is actually safe to invoke
this.

I suggest

/* Use in driver facing APIs, API must only be called by a probed driver */
static inline const struct iommu_ops *dev_maybe_iommu_ops(struct device *dev)
{
	if (!dev->iommu || !dev->iommu_iommu_dev))
		return NULL;
	return dev_iommu_ops(dev);
}

Since only this:

>  static u32 dev_iommu_get_max_pasids(struct device *dev)
>  {
>  	u32 max_pasids = 0, bits = 0;
> @@ -614,7 +623,7 @@ static void __iommu_group_remove_device(struct device *dev)
>  
>  		list_del(&device->list);
>  		__iommu_group_free_device(group, device);
> -		if (dev->iommu && dev->iommu->iommu_dev)
> +		if (dev_has_iommu(dev))
>  			iommu_deinit_device(dev);
>  		else
>  			dev->iommu_group = NULL;

Uses a different rule, and it is safe for some pretty unique reasons.

The next patch doesn't follow these rules, I will add a note there..

> @@ -3190,21 +3203,18 @@ void iommu_device_unuse_default_domain(struct device *dev)
>  
>  static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
>  {
> -	struct group_device *dev =
> -		list_first_entry(&group->devices, struct group_device, list);
> +	struct device *dev = iommu_group_first_dev(group);
>  
>  	if (group->blocking_domain)
>  		return 0;
>  
> -	group->blocking_domain =
> -		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
> +	group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
>  	if (!group->blocking_domain) {
>  		/*
>  		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
>  		 * create an empty domain instead.
>  		 */
> -		group->blocking_domain = __iommu_domain_alloc(
> -			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +		group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
>  		if (!group->blocking_domain)
>  			return -EINVAL;
>  	}

My identity domain series fixed this up by adding __iommu_group_domain_alloc()

Jason

Re: [PATCH v3 2/7] iommu: Decouple iommu_present() from bus ops

[Jason Gunthorpe]

On Fri, Sep 15, 2023 at 05:58:06PM +0100, Robin Murphy wrote:
> Much as I'd like to remove iommu_present(), the final remaining users
> are proving stubbornly difficult to clean up, so kick that can down
> the road and just rework it to preserve the current behaviour without
> depending on bus ops.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> ---
> 
> v3: Tweak to use the ops-based check rather than group-based, to
>     properly match the existing behaviour
> ---
>  drivers/iommu/iommu.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4566d0001cd3..2f29ee9dea64 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1907,9 +1907,24 @@ int bus_iommu_probe(const struct bus_type *bus)
>  	return 0;
>  }
>  
> +static int __iommu_present(struct device *dev, void *unused)
> +{
> +	return dev_has_iommu(dev);
> +}

This is not locked right..

Rather than perpetuate that, can we fix the two callers instead?

Maybe this for mtk:

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 93552d76b6e778..e7fe0e6f27de85 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -500,6 +500,8 @@ static int mtk_drm_kms_init(struct drm_device *drm)
                dev_err(drm->dev, "Need at least one OVL device\n");
                goto err_component_unbind;
        }
+       if (!device_iommu_mapped(dma_dev))
+               return -EPROBE_DEFER;
 
        for (i = 0; i < private->data->mmsys_dev_num; i++)
                private->all_drm_private[i]->dma_dev = dma_dev;
@@ -583,9 +585,6 @@ static int mtk_drm_bind(struct device *dev)
        struct drm_device *drm;
        int ret, i;
 
-       if (!iommu_present(&platform_bus_type))
-               return -EPROBE_DEFER;
-
        pdev = of_find_device_by_node(private->mutex_node);
        if (!pdev) {
                dev_err(dev, "Waiting for disp-mutex device %pOF\n",


? It doesn't seem to use the iommu API so I guess all it is doing is
trying to fix some kind of probe ordering issue? Maybe the probe
ordering issue is already gone and we can just delete the check?

And tegra:

	if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) {
		tegra->domain = iommu_domain_alloc(&platform_bus_type);
		if (!tegra->domain) {

Lets do the same:

	if (host1x_drm_wants_iommu(dev) && device_iommu_mapped(dev->dev.parent)) {

?

Alternatively how about:

bool iommu_present(void)
{
	bool ret;

	spin_lock(&iommu_device_lock);
	ret = !list_empty(&iommu_device_list);
	spin_unlock(&iommu_device_lock);
	return ret;
}
EXPORT_SYMBOL_GPL(iommu_present);

Since neither of the two users is really needing anything more than that?

Jason

Re: [PATCH v3 2/7] iommu: Decouple iommu_present() from bus ops

[Robin Murphy]

On 2023-09-18 18:12, Jason Gunthorpe wrote:
> On Fri, Sep 15, 2023 at 05:58:06PM +0100, Robin Murphy wrote:
>> Much as I'd like to remove iommu_present(), the final remaining users
>> are proving stubbornly difficult to clean up, so kick that can down
>> the road and just rework it to preserve the current behaviour without
>> depending on bus ops.
>>
>> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>
>> ---
>>
>> v3: Tweak to use the ops-based check rather than group-based, to
>>      properly match the existing behaviour
>> ---
>>   drivers/iommu/iommu.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 4566d0001cd3..2f29ee9dea64 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1907,9 +1907,24 @@ int bus_iommu_probe(const struct bus_type *bus)
>>   	return 0;
>>   }
>>   
>> +static int __iommu_present(struct device *dev, void *unused)
>> +{
>> +	return dev_has_iommu(dev);
>> +}
> 
> This is not locked right..

Urgh, yes, I suppose technically this walk could run in parallel with 
the bus_iommu_probe() of another IOMMU instance that our caller here 
doesn't depend on. I agree that's suboptimal, even if it shouldn't 
happen in practice for the remaining in-tree callers.

> Rather than perpetuate that, can we fix the two callers instead?
> 
> Maybe this for mtk:
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 93552d76b6e778..e7fe0e6f27de85 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -500,6 +500,8 @@ static int mtk_drm_kms_init(struct drm_device *drm)
>                  dev_err(drm->dev, "Need at least one OVL device\n");
>                  goto err_component_unbind;
>          }
> +       if (!device_iommu_mapped(dma_dev))
> +               return -EPROBE_DEFER;
>   
>          for (i = 0; i < private->data->mmsys_dev_num; i++)
>                  private->all_drm_private[i]->dma_dev = dma_dev;
> @@ -583,9 +585,6 @@ static int mtk_drm_bind(struct device *dev)
>          struct drm_device *drm;
>          int ret, i;
>   
> -       if (!iommu_present(&platform_bus_type))
> -               return -EPROBE_DEFER;
> -
>          pdev = of_find_device_by_node(private->mutex_node);
>          if (!pdev) {
>                  dev_err(dev, "Waiting for disp-mutex device %pOF\n",
> 
> 
> ? It doesn't seem to use the iommu API so I guess all it is doing is
> trying to fix some kind of probe ordering issue? Maybe the probe
> ordering issue is already gone and we can just delete the check?

As I've said before, the correct fix for this one is [1]. I've sent it 
twice now, it just gets ignored :(

> And tegra:
> 
> 	if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) {
> 		tegra->domain = iommu_domain_alloc(&platform_bus_type);
> 		if (!tegra->domain) {
> 
> Lets do the same:
> 
> 	if (host1x_drm_wants_iommu(dev) && device_iommu_mapped(dev->dev.parent)) {
> 
> ?

IIRC the problem here is that the Host1x (or GPU?) wants to allocate a 
domain for the GPU (or Host1x) to use, even if the former isn't itself 
associated with the IOMMU, and at this point it doesn't actually have a 
suitable handle to the latter device.

> Alternatively how about:
> 
> bool iommu_present(void)
> {
> 	bool ret;
> 
> 	spin_lock(&iommu_device_lock);
> 	ret = !list_empty(&iommu_device_list);
> 	spin_unlock(&iommu_device_lock);
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_present);
> 
> Since neither of the two users is really needing anything more than that?

Hmm, I guess maybe I did get a bit hung up on the bus notion... Indeed I 
think this wouldn't really be any more inaccurate than the current 
behaviour, and might be arguably truer to the intent of the function 
(whatever that is) since in the new design any instance is effectively 
present for all relevant buses anyway. I've respun along these lines 
(but retaining the argument with some token validation) and I don't hate 
it, so I'll send that as v4.

Thanks,
Robin.

[1] 
https://lore.kernel.org/dri-devel/49bafdabd2263cfc543bb22fb7f1bf32ea6bfd22.1683735862.git.robin.murphy@arm.com/

Re: [PATCH v3 2/7] iommu: Decouple iommu_present() from bus ops

[Jason Gunthorpe]

On Mon, Sep 18, 2023 at 08:21:45PM +0100, Robin Murphy wrote:

> > ? It doesn't seem to use the iommu API so I guess all it is doing is
> > trying to fix some kind of probe ordering issue? Maybe the probe
> > ordering issue is already gone and we can just delete the check?
> 
> As I've said before, the correct fix for this one is [1]. I've sent it twice
> now, it just gets ignored :(

IMHO at this point just put it in this series and have Joerg take it
:(

> Hmm, I guess maybe I did get a bit hung up on the bus notion... Indeed I
> think this wouldn't really be any more inaccurate than the current
> behaviour, and might be arguably truer to the intent of the function
> (whatever that is) since in the new design any instance is effectively
> present for all relevant buses anyway. I've respun along these lines (but
> retaining the argument with some token validation) and I don't hate it, so
> I'll send that as v4.

Eventually tegra is going to need to pass in a real struct device to
get the domain, so at that moment we can switch it to use the device
API on that real struct device. So this definately seems good enough
for now.

Jason