[PATCH v2 0/8] iommu: The early demise of bus ops

[PATCH v2 0/8] iommu: The early demise of bus ops

[Robin Murphy]

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

Hi all,

Here's v2, now rebased on today's iommu/core and with just a few minor
changes. I've dropped "iommu: Pass device through ops->domain_alloc"
for now since I have nothing ready to take advantage of it yet, and I've
also realised that cleaning up set_pgtable_quirks will likely need yet
another change there anyway.

Cheers,
Robin.


Robin Murphy (8):
  iommu: Decouple iommu_present() from bus ops
  iommu: Validate that devices match domains
  iommu: Add lockdep annotations for group list iterators
  iommu: Factor out some helpers
  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                       | 151 +++++++++++++-------
 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, 113 insertions(+), 98 deletions(-)

-- 
2.36.1.dirty

[PATCH v2 1/8] 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: No change

 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 42591766266d..b27f5d3453bb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1871,9 +1871,24 @@ int bus_iommu_probe(struct bus_type *bus)
 	return ret;
 }
 
+static int __iommu_present(struct device *dev, void *unused)
+{
+	return device_iommu_mapped(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(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.36.1.dirty

[PATCH v2 2/8] 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: Tweaked commit message

 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 b27f5d3453bb..d48e5499e0fa 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1941,20 +1941,22 @@ EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 						 unsigned type)
 {
+	const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
 	struct iommu_domain *domain;
 
-	if (bus == NULL || bus->iommu_ops == NULL)
+	if (!ops)
 		return NULL;
 
-	domain = bus->iommu_ops->domain_alloc(type);
+	domain = ops->domain_alloc(type);
 	if (!domain)
 		return NULL;
 
 	domain->type = type;
+	domain->owner = ops;
 	/* Assume all sizes by default; the driver may override this later */
-	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);
@@ -2120,6 +2122,9 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
 
+	if (dev_iommu_ops(dev) != domain->owner)
+		return -EINVAL;
+
 	return __iommu_attach_device(domain, dev);
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3589d1b8f922..86fa52025e75 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.36.1.dirty

[PATCH v2 3/8] iommu: Add lockdep annotations for group list iterators

[Robin Murphy]

Before we add any more common helpers for iterating or otherwise
accessing the group device list, let's start the good habit of
annotating their locking expectations for robustness.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: New. Note that I've left the group_pasid helpers, since they're
    pretty much right next to where the relevant locking happens, and
    highly unlikely to be reused from anywhere else anyway.

 drivers/iommu/iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d48e5499e0fa..77f076030995 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -20,6 +20,7 @@
 #include <linux/iommu.h>
 #include <linux/idr.h>
 #include <linux/err.h>
+#include <linux/lockdep.h>
 #include <linux/pci.h>
 #include <linux/pci-ats.h>
 #include <linux/bitops.h>
@@ -1100,6 +1101,7 @@ static int iommu_group_device_count(struct iommu_group *group)
 	struct group_device *entry;
 	int ret = 0;
 
+	lockdep_assert_held(&group->mutex);
 	list_for_each_entry(entry, &group->devices, list)
 		ret++;
 
@@ -1112,6 +1114,7 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
 	struct group_device *device;
 	int ret = 0;
 
+	lockdep_assert_held(&group->mutex);
 	list_for_each_entry(device, &group->devices, list) {
 		ret = fn(device->dev, data);
 		if (ret)
-- 
2.36.1.dirty

[PATCH v2 4/8] 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 to hide
the group_device detail from places that don't need to know. Similarly,
the safety check for dev_iommu_ops() at 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: - Add dev_iommu_ops_valid() helper
    - Add lockdep assertion [Jason]

 drivers/iommu/iommu.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 77f076030995..440bb3b7bded 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -284,6 +284,12 @@ static void dev_iommu_free(struct device *dev)
 	kfree(param);
 }
 
+/* Only needed in public APIs which allow unchecked devices for convenience */
+static bool dev_iommu_ops_valid(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;
@@ -1096,6 +1102,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;
+}
+
 static int iommu_group_device_count(struct iommu_group *group)
 {
 	struct group_device *entry;
@@ -1907,7 +1919,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_iommu_ops_valid(dev))
 		return false;
 
 	ops = dev_iommu_ops(dev);
@@ -2770,8 +2782,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_iommu_ops_valid(dev)) {
+		const struct iommu_ops *ops = dev_iommu_ops(dev);
 
 		if (ops->dev_enable_feat)
 			return ops->dev_enable_feat(dev, feat);
@@ -2786,8 +2798,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_iommu_ops_valid(dev)) {
+		const struct iommu_ops *ops = dev_iommu_ops(dev);
 
 		if (ops->dev_disable_feat)
 			return ops->dev_disable_feat(dev, feat);
@@ -2816,7 +2828,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 				       struct device *prev_dev, int type)
 {
 	struct iommu_domain *prev_dom;
-	struct group_device *grp_dev;
 	int ret, dev_def_dom;
 	struct device *dev;
 
@@ -2848,8 +2859,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	}
 
 	/* Since group has only one device */
-	grp_dev = list_first_entry(&group->devices, struct group_device, list);
-	dev = grp_dev->dev;
+	dev = iommu_group_first_dev(group);
 
 	if (prev_dev != dev) {
 		dev_err_ratelimited(prev_dev, "Cannot change default domain: Device has been changed\n");
@@ -2946,7 +2956,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count)
 {
-	struct group_device *grp_dev;
 	struct device *dev;
 	int ret, req_type;
 
@@ -2981,8 +2990,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	}
 
 	/* Since group has only one device */
-	grp_dev = list_first_entry(&group->devices, struct group_device, list);
-	dev = grp_dev->dev;
+	dev = iommu_group_first_dev(group);
 	get_device(dev);
 
 	/*
@@ -3107,21 +3115,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.36.1.dirty

[PATCH v2 5/8] 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: - Explain the mitigation better in the warning message
    - Fix theoretical bug if alloc_dev is never assigned because the
      bus has no devices
    - Use new dev_iommu_ops_valid() since in theory VFIO noiommu makes
      device_iommu_mapped() -> dev_iommu_ops() an unsafe assumption
      [Baolu]

 drivers/iommu/iommu.c | 59 ++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 440bb3b7bded..bdc5fdf39d2b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -89,7 +89,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
 static int iommu_alloc_default_domain(struct iommu_group *group,
 				      struct device *dev);
-static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
@@ -1641,15 +1641,15 @@ static int iommu_get_def_domain_type(struct device *dev)
 	return 0;
 }
 
-static int iommu_group_alloc_default_domain(struct bus_type *bus,
-					    struct iommu_group *group,
+static int iommu_group_alloc_default_domain(struct iommu_group *group,
+					    struct device *dev,
 					    unsigned int type)
 {
 	struct iommu_domain *dom;
 
-	dom = __iommu_domain_alloc(bus, type);
+	dom = __iommu_domain_alloc(dev, type);
 	if (!dom && type != IOMMU_DOMAIN_DMA) {
-		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
+		dom = __iommu_domain_alloc(dev, IOMMU_DOMAIN_DMA);
 		if (dom)
 			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
 				type, group->name);
@@ -1674,7 +1674,7 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
 
 	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
 
-	return iommu_group_alloc_default_domain(dev->bus, group, type);
+	return iommu_group_alloc_default_domain(group, dev, type);
 }
 
 /**
@@ -1787,8 +1787,7 @@ static int probe_get_default_domain_type(struct device *dev, void *data)
 	return 0;
 }
 
-static void probe_alloc_default_domain(struct bus_type *bus,
-				       struct iommu_group *group)
+static void probe_alloc_default_domain(struct iommu_group *group)
 {
 	struct __group_domain_type gtype;
 
@@ -1798,10 +1797,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 	__iommu_group_for_each_dev(group, &gtype,
 				   probe_get_default_domain_type);
 
-	if (!gtype.type)
+	if (!gtype.type) {
 		gtype.type = iommu_def_domain_type;
+		gtype.dev = iommu_group_first_dev(group);
+	}
 
-	iommu_group_alloc_default_domain(bus, group, gtype.type);
+	iommu_group_alloc_default_domain(group, gtype.dev, gtype.type);
 
 }
 
@@ -1864,7 +1865,7 @@ int bus_iommu_probe(struct bus_type *bus)
 		list_del_init(&group->entry);
 
 		/* Try to allocate default domain */
-		probe_alloc_default_domain(bus, group);
+		probe_alloc_default_domain(group);
 
 		if (!group->default_domain) {
 			mutex_unlock(&group->mutex);
@@ -1953,15 +1954,12 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
-static struct iommu_domain *__iommu_domain_alloc(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;
 
-	if (!ops)
-		return NULL;
-
 	domain = ops->domain_alloc(type);
 	if (!domain)
 		return NULL;
@@ -1980,9 +1978,30 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 	return domain;
 }
 
+static int __iommu_domain_alloc_dev(struct device *dev, void *data)
+{
+	struct device **alloc_dev = data;
+
+	if (!dev_iommu_ops_valid(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(struct bus_type *bus)
 {
-	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+	struct device *dev = NULL;
+
+	/* We always check the whole bus, so the return value isn't useful */
+	bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
+	if (!dev)
+		return NULL;
+
+	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
 }
 EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
@@ -2906,7 +2925,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	}
 
 	/* Sets group->default_domain to the newly allocated domain */
-	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
+	ret = iommu_group_alloc_default_domain(group, dev, type);
 	if (ret)
 		goto out;
 
@@ -3120,13 +3139,13 @@ 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);
+	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.36.1.dirty

[PATCH v2 6/8] 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: No change

 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 719fbca1fe52..607f06af01b6 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2156,7 +2156,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.36.1.dirty

[PATCH v2 7/8] 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>
CC: Christoph Hellwig <hch@lst.de>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: Clarify the iommu_ops_from_fwnode(NULL) assumption [Baolu]

 drivers/iommu/iommu.c       | 28 +++++++++++++++++-----------
 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, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bdc5fdf39d2b..7fb7c84e3dc6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -219,13 +219,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)
@@ -235,10 +228,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;
@@ -310,12 +301,27 @@ static u32 dev_iommu_get_max_pasids(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_device *iommu_dev;
+	struct iommu_fwspec *fwspec;
 	struct iommu_group *group;
 	static DEFINE_MUTEX(iommu_probe_device_lock);
 	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 cd3b75e08ec3..067dde9291c9 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -614,6 +614,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 44e3acae7b36..f7a7ecafedd3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -41,7 +41,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 d8b29ccd07e5..4ece3470112f 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -63,9 +63,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
  * @p:		The private data of the driver core, only the driver core can
  *		touch this.
  * @lock_key:	Lock class key for use by the lock validator
@@ -109,8 +106,6 @@ struct bus_type {
 
 	const struct dev_pm_ops *pm;
 
-	const struct iommu_ops *iommu_ops;
-
 	struct subsys_private *p;
 	struct lock_class_key lock_key;
 
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index d678afeb8a13..e8ebf0bf611b 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -10,6 +10,7 @@
 #include <linux/pgtable.h>
 
 struct cma;
+struct iommu_ops;
 
 /*
  * Values for struct dma_map_ops.flags:
-- 
2.36.1.dirty

[PATCH v2 8/8] 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: No change, but I'll note here that it's really about the fwspec->ops
    checks; the !fwspec clauses are just going along for the ride where
    that's clearly impossible as well. I plan to sweep for other
    redundant checks in future when looking at the relevant flows.

 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 ab160198edd6..cb05d9771192 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2642,9 +2642,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 607f06af01b6..235550db0d59 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1118,11 +1118,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
@@ -1352,10 +1347,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 d7be3adee426..3fe02720ab29 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;
@@ -361,7 +351,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;
 
@@ -486,7 +476,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 d5a4955910ff..ce19a2a21e2a 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -776,16 +776,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 78d0a84c704f..328833823181 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 ae94d74b73f4..85378f7cfd1f 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -358,13 +358,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 5b8fe9bfa9a5..59f1abd6ee53 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -945,9 +945,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.36.1.dirty

Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops

[Jacob Pan]

Hi Robin,

On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com>
wrote:

>  
> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> +{
> +	struct device **alloc_dev = data;
> +
> +	if (!dev_iommu_ops_valid(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(struct bus_type *bus)
>  {
> -	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> +	struct device *dev = NULL;
> +
> +	/* We always check the whole bus, so the return value isn't
> useful */
> +	bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
> +	if (!dev)
> +		return NULL;
Since __iommu_domain_alloc_dev() will always return 0, bus_for_each_dev()
will never breakout until the whole dev list is iterated over. If so, would
dev only record the last one? i.e. prior results get overwritten.  Maybe a
misunderstood the logic.

> +	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
>  }
>  EXPORT_SYMBOL_GPL(iommu_domain_alloc);

Thanks,

Jacob

Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops

[Robin Murphy]

On 2023-01-26 23:22, Jacob Pan wrote:
> Hi Robin,
> 
> On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com>
> wrote:
> 
>>   
>> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
>> +{
>> +	struct device **alloc_dev = data;
>> +
>> +	if (!dev_iommu_ops_valid(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(struct bus_type *bus)
>>   {
>> -	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
>> +	struct device *dev = NULL;
>> +
>> +	/* We always check the whole bus, so the return value isn't
>> useful */
>> +	bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
>> +	if (!dev)
>> +		return NULL;
> Since __iommu_domain_alloc_dev() will always return 0, bus_for_each_dev()
> will never breakout until the whole dev list is iterated over. If so, would
> dev only record the last one? i.e. prior results get overwritten.  Maybe a
> misunderstood the logic.

Yes, as the comment points out, the intent is to walk the whole bus to 
check it for consistency. Beyond that, we just need *a* device with 
IOMMU ops; it doesn't matter at all which one it is. It happens to be 
the last one off the list because that's what fell out of writing the 
fewest lines of code.

(You could argue that there's no need to repeat the full walk if the 
WARN_ONCE has already fired, but I'd rather keep the behaviour simple 
and consistent - this is only meant to be a short-term solution, and 
it's not a performance-critical path)

Thanks,
Robin.

> 
>> +	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
> 
> Thanks,
> 
> Jacob

Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops

[Jacob Pan]

Hi Robin,

On Fri, 27 Jan 2023 11:42:27 +0000, Robin Murphy <robin.murphy@arm.com>
wrote:

> On 2023-01-26 23:22, Jacob Pan wrote:
> > Hi Robin,
> > 
> > On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com>
> > wrote:
> >   
> >>   
> >> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> >> +{
> >> +	struct device **alloc_dev = data;
> >> +
> >> +	if (!dev_iommu_ops_valid(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(struct bus_type *bus)
> >>   {
> >> -	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> >> +	struct device *dev = NULL;
> >> +
> >> +	/* We always check the whole bus, so the return value isn't
> >> useful */
> >> +	bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
> >> +	if (!dev)
> >> +		return NULL;  
> > Since __iommu_domain_alloc_dev() will always return 0,
> > bus_for_each_dev() will never breakout until the whole dev list is
> > iterated over. If so, would dev only record the last one? i.e. prior
> > results get overwritten.  Maybe a misunderstood the logic.  
> 
> Yes, as the comment points out, the intent is to walk the whole bus to 
> check it for consistency. Beyond that, we just need *a* device with 
> IOMMU ops; it doesn't matter at all which one it is. It happens to be 
> the last one off the list because that's what fell out of writing the 
> fewest lines of code.
> 
> (You could argue that there's no need to repeat the full walk if the 
> WARN_ONCE has already fired, but I'd rather keep the behaviour simple 
> and consistent - this is only meant to be a short-term solution, and 
> it's not a performance-critical path)
That make sense now, thank you for the explanation.


Jacob

Re: [PATCH v2 1/8] iommu: Decouple iommu_present() from bus ops

[Baolu Lu]

On 2023/1/27 2:26, 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.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 2/8] iommu: Validate that devices match domains

[Baolu Lu]

On 2023/1/27 2:26, 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.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 3/8] iommu: Add lockdep annotations for group list iterators

[Baolu Lu]

On 2023/1/27 2:26, Robin Murphy wrote:
> Before we add any more common helpers for iterating or otherwise
> accessing the group device list, let's start the good habit of
> annotating their locking expectations for robustness.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Baolu Lu]

On 2023/1/27 2:26, 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 to hide
> the group_device detail from places that don't need to know. Similarly,
> the safety check for dev_iommu_ops() at 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.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops

[Baolu Lu]

On 2023/1/27 2:26, Robin Murphy wrote:
> 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.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 7/8] iommu: Retire bus ops

[Baolu Lu]

On 2023/1/27 2:26, Robin Murphy wrote:
> 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>
> CC: Christoph Hellwig<hch@lst.de>
> Acked-by: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 3/8] iommu: Add lockdep annotations for group list iterators

[Baolu Lu]

On 2023/1/27 2:26, Robin Murphy wrote:
> Before we add any more common helpers for iterating or otherwise
> accessing the group device list, let's start the good habit of
> annotating their locking expectations for robustness.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2: New. Note that I've left the group_pasid helpers, since they're
>      pretty much right next to where the relevant locking happens, and
>      highly unlikely to be reused from anywhere else anyway.
> 
>   drivers/iommu/iommu.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d48e5499e0fa..77f076030995 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -20,6 +20,7 @@
>   #include <linux/iommu.h>
>   #include <linux/idr.h>
>   #include <linux/err.h>
> +#include <linux/lockdep.h>
>   #include <linux/pci.h>
>   #include <linux/pci-ats.h>
>   #include <linux/bitops.h>
> @@ -1100,6 +1101,7 @@ static int iommu_group_device_count(struct iommu_group *group)
>   	struct group_device *entry;
>   	int ret = 0;
>   
> +	lockdep_assert_held(&group->mutex);
>   	list_for_each_entry(entry, &group->devices, list)
>   		ret++;
>   
> @@ -1112,6 +1114,7 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
>   	struct group_device *device;
>   	int ret = 0;
>   
> +	lockdep_assert_held(&group->mutex);

When I tested this patch on an Intel platform, I found that the
bus_iommu_probe() path iterates the group device list but does not hold
the lock.

1831 static void __iommu_group_dma_finalize(struct iommu_group *group)
1832 {
1833         __iommu_group_for_each_dev(group, group->default_domain,
1834                                    iommu_group_do_probe_finalize);
1835 }

bus_iommu_probe:

1873                 /* Try to allocate default domain */
1874                 probe_alloc_default_domain(group);
1875
1876                 if (!group->default_domain) {
1877                         mutex_unlock(&group->mutex);
1878                         continue;
1879                 }
1880
1881                 iommu_group_create_direct_mappings(group);
1882
1883                 ret = __iommu_group_dma_first_attach(group);
1884
1885                 mutex_unlock(&group->mutex);
1886
1887                 if (ret)
1888                         break;
1889
1890                 __iommu_group_dma_finalize(group);

The lockdep_assert_held() caused below kernel trace:

[   70.022590] ------------[ cut here ]------------
[   70.027967] WARNING: CPU: 1 PID: 1 at drivers/iommu/iommu.c:1135 
__iommu_group_for_each_dev+0x72/0x80
[   70.038672] Modules linked in:
[   70.042264] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W 
  6.2.0-rc5+ #421
[   70.063860] RIP: 0010:__iommu_group_for_each_dev+0x72/0x80
[   70.070267] Code: 5c 41 5d c3 cc cc cc cc 5b 31 c0 5d 41 5c 41 5d c3 
cc cc cc cc 48 8d bf 10 01 00 00 be ff ff ff ff e8 92 7b 8f 00 85 c0 75 
ac <0f> 0b eb a8 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90
[   70.091981] RSP: 0000:ffffad104006fd30 EFLAGS: 00010246
[   70.098058] RAX: 0000000000000000 RBX: ffff9f490bb95600 RCX: 
0000000000000001
[   70.106378] RDX: 0000000000000001 RSI: ffff9f490bb95710 RDI: 
ffff9f4900b10d88
[   70.114674] RBP: ffff9f490bb95600 R08: ffffcd26844fde80 R09: 
ffffcd26844fde40
[   70.122970] R10: 0000000000000000 R11: 0000000000000000 R12: 
ffff9f490bb95ed0
[   70.131265] R13: ffffffff8aadd520 R14: ffffffff8aade3a0 R15: 
0000000000000000
[   70.139561] FS:  0000000000000000(0000) GS:ffff9f583ff00000(0000) 
knlGS:0000000000000000
[   70.148969] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   70.155667] CR2: 0000000000000000 CR3: 0000000cc2012001 CR4: 
0000000000370ee0
[   70.163963] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[   70.172282] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 
0000000000000400
[   70.180559] Call Trace:
[   70.183469]  <TASK>
[   70.185972]  bus_iommu_probe+0xee/0x1e0
[   70.190496]  iommu_device_register+0xaf/0x100
[   70.195580]  intel_iommu_init+0x3e9/0x6f2
[   70.200271]  ? __pfx_pci_iommu_init+0x10/0x10
[   70.205381]  ? __pfx_ignore_unknown_bootoption+0x10/0x10
[   70.211564]  pci_iommu_init+0x12/0x3a
[   70.215870]  do_one_initcall+0x65/0x330
[   70.220376]  ? __pfx_ignore_unknown_bootoption+0x10/0x10
[   70.226577]  ? rcu_read_lock_sched_held+0x5a/0x80
[   70.232076]  kernel_init_freeable+0x287/0x2f0
[   70.237914]  ? __pfx_kernel_init+0x10/0x10
[   70.241979]  kernel_init+0x1a/0x130
[   70.246075]  ret_from_fork+0x29/0x50
[   70.251014]  </TASK>
[   70.252867] irq event stamp: 16350051
[   70.257167] hardirqs last  enabled at (16350061): 
[<ffffffff8a1c3ed2>] __up_console_sem+0x52/0x60
[   70.267468] hardirqs last disabled at (16350070): 
[<ffffffff8a1c3eb7>] __up_console_sem+0x37/0x60
[   70.277768] softirqs last  enabled at (16350006): 
[<ffffffff8b3edd03>] __do_softirq+0x283/0x451
[   70.287859] softirqs last disabled at (16350001): 
[<ffffffff8a132c6a>] irq_exit_rcu+0xaa/0xc0
[   70.297777] ---[ end trace 0000000000000000 ]---

>   	list_for_each_entry(device, &group->devices, list) {
>   		ret = fn(device->dev, data);
>   		if (ret)

Best regards,
baolu

Re: [PATCH v2 7/8] iommu: Retire bus ops

[kernel test robot]

Hi Robin,

I love your patch! Yet something to improve:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master v6.2-rc5]
[cannot apply to joro-iommu/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/iommu-Decouple-iommu_present-from-bus-ops/20230128-141510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/198e82a6b1a28605409c395da4ec1a67b0e1587b.1674753627.git.robin.murphy%40arm.com
patch subject: [PATCH v2 7/8] iommu: Retire bus ops
config: s390-randconfig-r012-20230123 (https://download.01.org/0day-ci/archive/20230128/202301282015.hjj2YFYy-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/399f4b37d8065bffafeec12de1344a7ff6098e64
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Robin-Murphy/iommu-Decouple-iommu_present-from-bus-ops/20230128-141510
        git checkout 399f4b37d8065bffafeec12de1344a7ff6098e64
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/iommu/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/iommu/iommufd/selftest.c:7:
   In file included from include/linux/iommu.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from drivers/iommu/iommufd/selftest.c:7:
   In file included from include/linux/iommu.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from drivers/iommu/iommufd/selftest.c:7:
   In file included from include/linux/iommu.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/iommu/iommufd/selftest.c:276:39: error: field designator 'iommu_ops' does not refer to any field in type 'struct bus_type'
           static struct bus_type mock_bus = { .iommu_ops = &mock_ops };
                                                ^
   12 warnings and 1 error generated.


vim +276 drivers/iommu/iommufd/selftest.c

f4b20bb34c83dc Jason Gunthorpe 2022-11-29  271  
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  272  /* Create an hw_pagetable with the mock domain so we can test the domain ops */
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  273  static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  274  				    struct iommu_test_cmd *cmd)
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  275  {
f4b20bb34c83dc Jason Gunthorpe 2022-11-29 @276  	static struct bus_type mock_bus = { .iommu_ops = &mock_ops };
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  277  	struct iommufd_hw_pagetable *hwpt;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  278  	struct selftest_obj *sobj;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  279  	struct iommufd_ioas *ioas;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  280  	int rc;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  281  
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  282  	ioas = iommufd_get_ioas(ucmd, cmd->id);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  283  	if (IS_ERR(ioas))
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  284  		return PTR_ERR(ioas);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  285  
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  286  	sobj = iommufd_object_alloc(ucmd->ictx, sobj, IOMMUFD_OBJ_SELFTEST);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  287  	if (IS_ERR(sobj)) {
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  288  		rc = PTR_ERR(sobj);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  289  		goto out_ioas;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  290  	}
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  291  	sobj->idev.ictx = ucmd->ictx;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  292  	sobj->type = TYPE_IDEV;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  293  	sobj->idev.mock_dev.bus = &mock_bus;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  294  
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  295  	hwpt = iommufd_device_selftest_attach(ucmd->ictx, ioas,
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  296  					      &sobj->idev.mock_dev);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  297  	if (IS_ERR(hwpt)) {
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  298  		rc = PTR_ERR(hwpt);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  299  		goto out_sobj;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  300  	}
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  301  	sobj->idev.hwpt = hwpt;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  302  
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  303  	/* Userspace must destroy both of these IDs to destroy the object */
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  304  	cmd->mock_domain.out_hwpt_id = hwpt->obj.id;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  305  	cmd->mock_domain.out_device_id = sobj->obj.id;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  306  	iommufd_object_finalize(ucmd->ictx, &sobj->obj);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  307  	iommufd_put_object(&ioas->obj);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  308  	return iommufd_ucmd_respond(ucmd, sizeof(*cmd));
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  309  
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  310  out_sobj:
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  311  	iommufd_object_abort(ucmd->ictx, &sobj->obj);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  312  out_ioas:
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  313  	iommufd_put_object(&ioas->obj);
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  314  	return rc;
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  315  }
f4b20bb34c83dc Jason Gunthorpe 2022-11-29  316  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 0/8] iommu: The early demise of bus ops

[Christoph Hellwig]

The whole series looks good to me:

Acked-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 7/8] iommu: Retire bus ops

[Jason Gunthorpe]

On Sat, Jan 28, 2023 at 08:55:09PM +0800, kernel test robot wrote:

> >> drivers/iommu/iommufd/selftest.c:276:39: error: field designator 'iommu_ops' does not refer to any field in type 'struct bus_type'
>            static struct bus_type mock_bus = { .iommu_ops = &mock_ops };

So this shortcut isn't going to work..

Probably something like this is the simplest thing:

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index cfb5fe9a5e0ee8..27bab42b1ff841 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -274,6 +274,8 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
                                    struct iommu_test_cmd *cmd)
 {
        static struct bus_type mock_bus = { .iommu_ops = &mock_ops };
+       static struct iommu_device mock_iommu = { .ops = &mock_ops };
+       static struct dev_iommu mock_dev_iommu = { .iommu_dev = &mock_iommu };
        struct iommufd_hw_pagetable *hwpt;
        struct selftest_obj *sobj;
        struct iommufd_ioas *ioas;
@@ -291,6 +293,7 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
        sobj->idev.ictx = ucmd->ictx;
        sobj->type = TYPE_IDEV;
        sobj->idev.mock_dev.bus = &mock_bus;
+       sobj->idev.mock_dev.iommu = &mock_dev_iommu;
 
        hwpt = iommufd_device_selftest_attach(ucmd->ictx, ioas,
                                              &sobj->idev.mock_dev);

And then delete mock_bus at this patch

Jason

Re: [PATCH v2 3/8] iommu: Add lockdep annotations for group list iterators

[Robin Murphy]

On 2023-01-28 12:20, Baolu Lu wrote:
> On 2023/1/27 2:26, Robin Murphy wrote:
>> Before we add any more common helpers for iterating or otherwise
>> accessing the group device list, let's start the good habit of
>> annotating their locking expectations for robustness.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v2: New. Note that I've left the group_pasid helpers, since they're
>>      pretty much right next to where the relevant locking happens, and
>>      highly unlikely to be reused from anywhere else anyway.
>>
>>   drivers/iommu/iommu.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index d48e5499e0fa..77f076030995 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/iommu.h>
>>   #include <linux/idr.h>
>>   #include <linux/err.h>
>> +#include <linux/lockdep.h>
>>   #include <linux/pci.h>
>>   #include <linux/pci-ats.h>
>>   #include <linux/bitops.h>
>> @@ -1100,6 +1101,7 @@ static int iommu_group_device_count(struct 
>> iommu_group *group)
>>       struct group_device *entry;
>>       int ret = 0;
>> +    lockdep_assert_held(&group->mutex);
>>       list_for_each_entry(entry, &group->devices, list)
>>           ret++;
>> @@ -1112,6 +1114,7 @@ static int __iommu_group_for_each_dev(struct 
>> iommu_group *group, void *data,
>>       struct group_device *device;
>>       int ret = 0;
>> +    lockdep_assert_held(&group->mutex);
> 
> When I tested this patch on an Intel platform, I found that the
> bus_iommu_probe() path iterates the group device list but does not hold
> the lock.

Urgh, that's what I get for being in a hurry and not boot-testing it 
myself :(

> 1831 static void __iommu_group_dma_finalize(struct iommu_group *group)
> 1832 {
> 1833         __iommu_group_for_each_dev(group, group->default_domain,
> 1834                                    iommu_group_do_probe_finalize);
> 1835 }
> 
> bus_iommu_probe:
> 
> 1873                 /* Try to allocate default domain */
> 1874                 probe_alloc_default_domain(group);
> 1875
> 1876                 if (!group->default_domain) {
> 1877                         mutex_unlock(&group->mutex);
> 1878                         continue;
> 1879                 }
> 1880
> 1881                 iommu_group_create_direct_mappings(group);
> 1882
> 1883                 ret = __iommu_group_dma_first_attach(group);
> 1884
> 1885                 mutex_unlock(&group->mutex);
> 1886
> 1887                 if (ret)
> 1888                         break;
> 1889
> 1890                 __iommu_group_dma_finalize(group);

...and of course it's the one case where the reason for not holding the 
lock is a bit dodgy, but most definitely intentional. I can see ways to 
hack around it further, but frankly I think the most practical solution 
would be to just drop this patch for now until I've sorted out the ARM 
stuff.

Thanks,
Robin.

> The lockdep_assert_held() caused below kernel trace:
> 
> [   70.022590] ------------[ cut here ]------------
> [   70.027967] WARNING: CPU: 1 PID: 1 at drivers/iommu/iommu.c:1135 
> __iommu_group_for_each_dev+0x72/0x80
> [   70.038672] Modules linked in:
> [   70.042264] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W 
>   6.2.0-rc5+ #421
> [   70.063860] RIP: 0010:__iommu_group_for_each_dev+0x72/0x80
> [   70.070267] Code: 5c 41 5d c3 cc cc cc cc 5b 31 c0 5d 41 5c 41 5d c3 
> cc cc cc cc 48 8d bf 10 01 00 00 be ff ff ff ff e8 92 7b 8f 00 85 c0 75 
> ac <0f> 0b eb a8 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90
> [   70.091981] RSP: 0000:ffffad104006fd30 EFLAGS: 00010246
> [   70.098058] RAX: 0000000000000000 RBX: ffff9f490bb95600 RCX: 
> 0000000000000001
> [   70.106378] RDX: 0000000000000001 RSI: ffff9f490bb95710 RDI: 
> ffff9f4900b10d88
> [   70.114674] RBP: ffff9f490bb95600 R08: ffffcd26844fde80 R09: 
> ffffcd26844fde40
> [   70.122970] R10: 0000000000000000 R11: 0000000000000000 R12: 
> ffff9f490bb95ed0
> [   70.131265] R13: ffffffff8aadd520 R14: ffffffff8aade3a0 R15: 
> 0000000000000000
> [   70.139561] FS:  0000000000000000(0000) GS:ffff9f583ff00000(0000) 
> knlGS:0000000000000000
> [   70.148969] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   70.155667] CR2: 0000000000000000 CR3: 0000000cc2012001 CR4: 
> 0000000000370ee0
> [   70.163963] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
> 0000000000000000
> [   70.172282] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 
> 0000000000000400
> [   70.180559] Call Trace:
> [   70.183469]  <TASK>
> [   70.185972]  bus_iommu_probe+0xee/0x1e0
> [   70.190496]  iommu_device_register+0xaf/0x100
> [   70.195580]  intel_iommu_init+0x3e9/0x6f2
> [   70.200271]  ? __pfx_pci_iommu_init+0x10/0x10
> [   70.205381]  ? __pfx_ignore_unknown_bootoption+0x10/0x10
> [   70.211564]  pci_iommu_init+0x12/0x3a
> [   70.215870]  do_one_initcall+0x65/0x330
> [   70.220376]  ? __pfx_ignore_unknown_bootoption+0x10/0x10
> [   70.226577]  ? rcu_read_lock_sched_held+0x5a/0x80
> [   70.232076]  kernel_init_freeable+0x287/0x2f0
> [   70.237914]  ? __pfx_kernel_init+0x10/0x10
> [   70.241979]  kernel_init+0x1a/0x130
> [   70.246075]  ret_from_fork+0x29/0x50
> [   70.251014]  </TASK>
> [   70.252867] irq event stamp: 16350051
> [   70.257167] hardirqs last  enabled at (16350061): 
> [<ffffffff8a1c3ed2>] __up_console_sem+0x52/0x60
> [   70.267468] hardirqs last disabled at (16350070): 
> [<ffffffff8a1c3eb7>] __up_console_sem+0x37/0x60
> [   70.277768] softirqs last  enabled at (16350006): 
> [<ffffffff8b3edd03>] __do_softirq+0x283/0x451
> [   70.287859] softirqs last disabled at (16350001): 
> [<ffffffff8a132c6a>] irq_exit_rcu+0xaa/0xc0
> [   70.297777] ---[ end trace 0000000000000000 ]---
> 
>>       list_for_each_entry(device, &group->devices, list) {
>>           ret = fn(device->dev, data);
>>           if (ret)
> 
> Best regards,
> baolu

Re: [PATCH v2 2/8] iommu: Validate that devices match domains

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:17PM +0000, Robin Murphy wrote:

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 3589d1b8f922..86fa52025e75 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 */

For some reason I thought we were trying to save bytes in this struct?

Putting a pointer back to iommu_ops in iommu_domain_ops would allow
that, then it is just domain->ops->owner

Regardless,

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

Jason

Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:19PM +0000, 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 to hide
> the group_device detail from places that don't need to know. Similarly,
> the safety check for dev_iommu_ops() at 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2: - Add dev_iommu_ops_valid() helper
>     - Add lockdep assertion [Jason]
> 
>  drivers/iommu/iommu.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 77f076030995..440bb3b7bded 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -284,6 +284,12 @@ static void dev_iommu_free(struct device *dev)
>  	kfree(param);
>  }
>  
> +/* Only needed in public APIs which allow unchecked devices for convenience */
> +static bool dev_iommu_ops_valid(struct device *dev)
> +{
> +	return dev->iommu && dev->iommu->iommu_dev;
> +}

I did an audit and more than half the callers need this test, and a
few places are missing it already.

We've kind of made a systematic error that assumes being in a group is
sufficient to prove there are non-NULL ops.

So I suggest that we make dev_iommu_ops() return NULL in all cases
where there is no driver and have a new API to get the ops in cases
where the call chain knows that it is safe, and there are only 5 of
those cases.

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705bd3..4b04ad50de8d6b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -98,7 +98,8 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 				    struct iommu_domain *new_domain);
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
-static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
+static struct iommu_group *iommu_group_get_for_dev(struct device *dev,
+						   const struct iommu_ops *ops);
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count);
 
@@ -130,6 +131,19 @@ static struct bus_type * const iommu_buses[] = {
 #endif
 };
 
+/*
+ * This may only be called if it is already clear from the calling context that
+ * the device has an ops. Seeing the device is part of an iommu_group is not
+ * enough as VFIO and POWER put devices in iommu_groups and do not attach
+ * drivers. Thus the only places that are safe have either already called
+ * dev_iommu_ops() on their call chain, or were responsible for assigning ops in
+ * the first place.
+ */
+static inline const struct iommu_ops *dev_iommu_ops_safe(struct device *dev)
+{
+	return dev->iommu->iommu_dev->ops;
+}
+
 /*
  * Use a function instead of an array here because the domain-type is a
  * bit-field, so an array would waste memory.
@@ -338,7 +352,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	dev->iommu->iommu_dev = iommu_dev;
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
 
-	group = iommu_group_get_for_dev(dev);
+	group = iommu_group_get_for_dev(dev, ops);
 	if (IS_ERR(group)) {
 		ret = PTR_ERR(group);
 		goto out_release;
@@ -414,7 +428,8 @@ int iommu_probe_device(struct device *dev)
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
 
-	ops = dev_iommu_ops(dev);
+	/* __iommu_probe_device() set the ops */
+	ops = dev_iommu_ops_safe(dev);
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
 
@@ -430,14 +445,13 @@ int iommu_probe_device(struct device *dev)
 
 void iommu_release_device(struct device *dev)
 {
-	const struct iommu_ops *ops;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
-	if (!dev->iommu)
+	if (!ops)
 		return;
 
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
-	ops = dev_iommu_ops(dev);
 	if (ops->release_device)
 		ops->release_device(dev);
 
@@ -614,13 +628,6 @@ int iommu_get_group_resv_regions(struct iommu_group *group,
 	list_for_each_entry(device, &group->devices, list) {
 		struct list_head dev_resv_regions;
 
-		/*
-		 * Non-API groups still expose reserved_regions in sysfs,
-		 * so filter out calls that get here that way.
-		 */
-		if (!device->dev->iommu)
-			break;
-
 		INIT_LIST_HEAD(&dev_resv_regions);
 		iommu_get_resv_regions(device->dev, &dev_resv_regions);
 		ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
@@ -1332,7 +1339,8 @@ int iommu_page_response(struct device *dev,
 	struct iommu_fault_event *evt;
 	struct iommu_fault_page_request *prm;
 	struct dev_iommu *param = dev->iommu;
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	/* This API should only be called from an IOMMU driver */
+	const struct iommu_ops *ops = dev_iommu_ops_safe(dev);
 	bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
 
 	if (!ops->page_response)
@@ -1601,7 +1609,8 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 
 static int iommu_get_def_domain_type(struct device *dev)
 {
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	/* It is not locked but all callers know there is an ops */
+	const struct iommu_ops *ops = dev_iommu_ops_safe(dev);
 
 	if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
 		return IOMMU_DOMAIN_DMA;
@@ -1658,9 +1667,9 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
  * to the returned IOMMU group, which will already include the provided
  * device.  The reference should be released with iommu_group_put().
  */
-static struct iommu_group *iommu_group_get_for_dev(struct device *dev)
+static struct iommu_group *iommu_group_get_for_dev(struct device *dev,
+						   const struct iommu_ops *ops)
 {
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_group *group;
 	int ret;
 
@@ -1795,7 +1804,8 @@ static int __iommu_group_dma_attach(struct iommu_group *group)
 
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
 {
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	/* It is unlocked but all callers know there is an ops */
+	const struct iommu_ops *ops = dev_iommu_ops_safe(dev);
 
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
@@ -1884,13 +1894,9 @@ EXPORT_SYMBOL_GPL(iommu_present);
  */
 bool device_iommu_capable(struct device *dev, enum iommu_cap cap)
 {
-	const struct iommu_ops *ops;
-
-	if (!dev->iommu || !dev->iommu->iommu_dev)
-		return false;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
-	ops = dev_iommu_ops(dev);
-	if (!ops->capable)
+	if (!ops || !ops->capable)
 		return false;
 
 	return ops->capable(dev, cap);
@@ -2620,7 +2626,11 @@ void iommu_get_resv_regions(struct device *dev, struct list_head *list)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
-	if (ops->get_resv_regions)
+	/*
+	 * Non-API groups still expose reserved_regions in sysfs, so filter out
+	 * calls that get here that way.
+	 */
+	if (ops && ops->get_resv_regions)
 		ops->get_resv_regions(dev, list);
 }
 
@@ -2979,6 +2989,11 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	/* Since group has only one device */
 	grp_dev = list_first_entry(&group->devices, struct group_device, list);
 	dev = grp_dev->dev;
+	if (!dev_iommu_ops(dev)) {
+		/* The group doesn't have an iommu driver attached */
+		mutex_unlock(&group->mutex);
+		return -EINVAL;
+	}
 	get_device(dev);
 
 	/*
@@ -3301,7 +3316,7 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 	const struct iommu_ops *ops;
 
 	list_for_each_entry(device, &group->devices, list) {
-		ops = dev_iommu_ops(device->dev);
+		ops = dev_iommu_ops_safe(device->dev);
 		ops->remove_dev_pasid(device->dev, pasid);
 	}
 }
@@ -3413,6 +3428,9 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_domain *domain;
 
+	if (!ops)
+		return NULL;
+
 	domain = ops->domain_alloc(IOMMU_DOMAIN_SVA);
 	if (!domain)
 		return NULL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa2286..60e84f8c7c46e9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -441,14 +441,11 @@ static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather)
 	};
 }
 
+/* May return NULL if the device has no iommu driver */
 static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 {
-	/*
-	 * Assume that valid ops must be installed if iommu_probe_device()
-	 * has succeeded. The device ops are essentially for internal use
-	 * within the IOMMU subsystem itself, so we should be able to trust
-	 * ourselves not to misuse the helper.
-	 */
+	if (!dev->iommu)
+		return NULL;
 	return dev->iommu->iommu_dev->ops;
 }
 

Re: [PATCH v2 7/8] iommu: Retire bus ops

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:22PM +0000, Robin Murphy wrote:
> 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>
> CC: Christoph Hellwig <hch@lst.de>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---

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

Jason

Re: [PATCH v2 8/8] iommu: Clean up open-coded ownership checks

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:23PM +0000, Robin Murphy wrote:
> 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2: No change, but I'll note here that it's really about the fwspec->ops
>     checks; the !fwspec clauses are just going along for the ride where
>     that's clearly impossible as well. I plan to sweep for other
>     redundant checks in future when looking at the relevant flows.

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

Jason

Re: [PATCH v2 6/8] iommu/arm-smmu: Don't register fwnode for legacy binding

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:21PM +0000, Robin Murphy wrote:
> 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2: No change

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

Jason

Re: [PATCH v2 1/8] iommu: Decouple iommu_present() from bus ops

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:16PM +0000, 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---

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

This below might help. You might need to ask Thierry for help on the
problematic stuff in tegra. Really we should finish the series here
and organize things so that the domain allocation is defered until a
client is present.

https://lore.kernel.org/linux-iommu/20220106022053.2406748-1-baolu.lu@linux.intel.com/

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index cd5b18ef79512c..e5c257e41f4933 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -353,9 +353,6 @@ static int mtk_drm_kms_init(struct drm_device *drm)
 	if (drm_firmware_drivers_only())
 		return -ENODEV;
 
-	if (!iommu_present(&platform_bus_type))
-		return -EPROBE_DEFER;
-
 	pdev = of_find_device_by_node(private->mutex_node);
 	if (!pdev) {
 		dev_err(drm->dev, "Waiting for disp-mutex device %pOF\n",
@@ -709,6 +706,9 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
+	if (!device_iommu_mapped(dev))
+		return -EPROBE_DEFER;
+
 	private = devm_kzalloc(dev, sizeof(*private), GFP_KERNEL);
 	if (!private)
 		return -ENOMEM;

Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops

[Jason Gunthorpe]

On Thu, Jan 26, 2023 at 06:26:20PM +0000, Robin Murphy wrote:

> @@ -1980,9 +1978,30 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>  	return domain;
>  }
>  
> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> +{
> +	struct device **alloc_dev = data;
> +
> +	if (!dev_iommu_ops_valid(dev))
> +		return 0;

This valid check is necessary to be done in __iommu_domain_alloc()
because the callback coming from iommu_device_claim_dma_owner() never
validated that the device/group has an iommu driver:

> @@ -3120,13 +3139,13 @@ 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);
> +	group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_BLOCKED);

Jason

Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Robin Murphy]

On 2023-01-30 16:38, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2023 at 06:26:19PM +0000, 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 to hide
>> the group_device detail from places that don't need to know. Similarly,
>> the safety check for dev_iommu_ops() at 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.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v2: - Add dev_iommu_ops_valid() helper
>>      - Add lockdep assertion [Jason]
>>
>>   drivers/iommu/iommu.c | 39 ++++++++++++++++++++++-----------------
>>   1 file changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 77f076030995..440bb3b7bded 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -284,6 +284,12 @@ static void dev_iommu_free(struct device *dev)
>>   	kfree(param);
>>   }
>>   
>> +/* Only needed in public APIs which allow unchecked devices for convenience */
>> +static bool dev_iommu_ops_valid(struct device *dev)
>> +{
>> +	return dev->iommu && dev->iommu->iommu_dev;
>> +}
> 
> I did an audit and more than half the callers need this test, and a
> few places are missing it already.
> 
> We've kind of made a systematic error that assumes being in a group is
> sufficient to prove there are non-NULL ops.
> 
> So I suggest that we make dev_iommu_ops() return NULL in all cases
> where there is no driver and have a new API to get the ops in cases
> where the call chain knows that it is safe, and there are only 5 of
> those cases.
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index de91dd88705bd3..4b04ad50de8d6b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -98,7 +98,8 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>   				    struct iommu_domain *new_domain);
>   static int iommu_create_device_direct_mappings(struct iommu_group *group,
>   					       struct device *dev);
> -static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> +static struct iommu_group *iommu_group_get_for_dev(struct device *dev,
> +						   const struct iommu_ops *ops);
>   static ssize_t iommu_group_store_type(struct iommu_group *group,
>   				      const char *buf, size_t count);
>   
> @@ -130,6 +131,19 @@ static struct bus_type * const iommu_buses[] = {
>   #endif
>   };
>   
> +/*
> + * This may only be called if it is already clear from the calling context that
> + * the device has an ops. Seeing the device is part of an iommu_group is not
> + * enough as VFIO and POWER put devices in iommu_groups and do not attach
> + * drivers. Thus the only places that are safe have either already called
> + * dev_iommu_ops() on their call chain, or were responsible for assigning ops in
> + * the first place.
> + */
> +static inline const struct iommu_ops *dev_iommu_ops_safe(struct device *dev)
> +{
> +	return dev->iommu->iommu_dev->ops;
> +}
> +
>   /*
>    * Use a function instead of an array here because the domain-type is a
>    * bit-field, so an array would waste memory.
> @@ -338,7 +352,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>   	dev->iommu->iommu_dev = iommu_dev;
>   	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
>   
> -	group = iommu_group_get_for_dev(dev);
> +	group = iommu_group_get_for_dev(dev, ops);

Why? We've literally just assigned dev->iommu->iommu_dev 2 lines above; 
it is not allowed to have invalid ops. Plus in future they may 
potentially be a different set of device ops from the ones we initially 
found to provide ->probe_device - in that case we definitely want 
group_get_for_dev to use the proper instance ops. This is the only place 
it is (and should be) called, so I don't see any problem.

>   	if (IS_ERR(group)) {
>   		ret = PTR_ERR(group);
>   		goto out_release;
> @@ -414,7 +428,8 @@ int iommu_probe_device(struct device *dev)
>   	mutex_unlock(&group->mutex);
>   	iommu_group_put(group);
>   
> -	ops = dev_iommu_ops(dev);
> +	/* __iommu_probe_device() set the ops */
> +	ops = dev_iommu_ops_safe(dev);
>   	if (ops->probe_finalize)
>   		ops->probe_finalize(dev);
>   
> @@ -430,14 +445,13 @@ int iommu_probe_device(struct device *dev)
>   
>   void iommu_release_device(struct device *dev)
>   {
> -	const struct iommu_ops *ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);

This is just moving an existing check around.

> -	if (!dev->iommu)
> +	if (!ops)
>   		return;
>   
>   	iommu_device_unlink(dev->iommu->iommu_dev, dev);
>   
> -	ops = dev_iommu_ops(dev);
>   	if (ops->release_device)
>   		ops->release_device(dev);
>   
> @@ -614,13 +628,6 @@ int iommu_get_group_resv_regions(struct iommu_group *group,
>   	list_for_each_entry(device, &group->devices, list) {
>   		struct list_head dev_resv_regions;
>   
> -		/*
> -		 * Non-API groups still expose reserved_regions in sysfs,
> -		 * so filter out calls that get here that way.
> -		 */
> -		if (!device->dev->iommu)
> -			break;
> -
>   		INIT_LIST_HEAD(&dev_resv_regions);
>   		iommu_get_resv_regions(device->dev, &dev_resv_regions);
>   		ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
> @@ -1332,7 +1339,8 @@ int iommu_page_response(struct device *dev,
>   	struct iommu_fault_event *evt;
>   	struct iommu_fault_page_request *prm;
>   	struct dev_iommu *param = dev->iommu;
> -	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	/* This API should only be called from an IOMMU driver */
> +	const struct iommu_ops *ops = dev_iommu_ops_safe(dev);
>   	bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
>   
>   	if (!ops->page_response)
> @@ -1601,7 +1609,8 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   
>   static int iommu_get_def_domain_type(struct device *dev)
>   {
> -	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	/* It is not locked but all callers know there is an ops */
> +	const struct iommu_ops *ops = dev_iommu_ops_safe(dev);
>   
>   	if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
>   		return IOMMU_DOMAIN_DMA;
> @@ -1658,9 +1667,9 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
>    * to the returned IOMMU group, which will already include the provided
>    * device.  The reference should be released with iommu_group_put().
>    */
> -static struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> +static struct iommu_group *iommu_group_get_for_dev(struct device *dev,
> +						   const struct iommu_ops *ops)
>   {
> -	const struct iommu_ops *ops = dev_iommu_ops(dev);
>   	struct iommu_group *group;
>   	int ret;
>   
> @@ -1795,7 +1804,8 @@ static int __iommu_group_dma_attach(struct iommu_group *group)
>   
>   static int iommu_group_do_probe_finalize(struct device *dev, void *data)
>   {
> -	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	/* It is unlocked but all callers know there is an ops */
> +	const struct iommu_ops *ops = dev_iommu_ops_safe(dev);
>   
>   	if (ops->probe_finalize)
>   		ops->probe_finalize(dev);
> @@ -1884,13 +1894,9 @@ EXPORT_SYMBOL_GPL(iommu_present);
>    */
>   bool device_iommu_capable(struct device *dev, enum iommu_cap cap)
>   {
> -	const struct iommu_ops *ops;
> -
> -	if (!dev->iommu || !dev->iommu->iommu_dev)
> -		return false;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>   
> -	ops = dev_iommu_ops(dev);
> -	if (!ops->capable)
> +	if (!ops || !ops->capable)
>   		return false;
>   
>   	return ops->capable(dev, cap);
> @@ -2620,7 +2626,11 @@ void iommu_get_resv_regions(struct device *dev, struct list_head *list)
>   {
>   	const struct iommu_ops *ops = dev_iommu_ops(dev);
>   
> -	if (ops->get_resv_regions)
> +	/*
> +	 * Non-API groups still expose reserved_regions in sysfs, so filter out
> +	 * calls that get here that way.
> +	 */
> +	if (ops && ops->get_resv_regions)

This is just moving the existing check from the public interface to 
pointlessly impose it on the internal call path as well.

>   		ops->get_resv_regions(dev, list);
>   }
>   
> @@ -2979,6 +2989,11 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>   	/* Since group has only one device */
>   	grp_dev = list_first_entry(&group->devices, struct group_device, list);
>   	dev = grp_dev->dev;
> +	if (!dev_iommu_ops(dev)) {
> +		/* The group doesn't have an iommu driver attached */
> +		mutex_unlock(&group->mutex);
> +		return -EINVAL;
> +	}

Withot any ops, group->default_domain will be NULL so we could never 
even get here.

>   	get_device(dev);
>   
>   	/*
> @@ -3301,7 +3316,7 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>   	const struct iommu_ops *ops;
>   
>   	list_for_each_entry(device, &group->devices, list) {
> -		ops = dev_iommu_ops(device->dev);
> +		ops = dev_iommu_ops_safe(device->dev);
>   		ops->remove_dev_pasid(device->dev, pasid);
>   	}
>   }
> @@ -3413,6 +3428,9 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
>   	const struct iommu_ops *ops = dev_iommu_ops(dev);
>   	struct iommu_domain *domain;
>   
> +	if (!ops)
> +		return NULL;

Anyone who incorrectly calls sva_domain_alloc() directly without having 
successfully called iommu_dev_enable_feature() first deserves to crash.

(Incidentally, you've missed enable/disable_feature here.)

> +
>   	domain = ops->domain_alloc(IOMMU_DOMAIN_SVA);
>   	if (!domain)
>   		return NULL;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 46e1347bfa2286..60e84f8c7c46e9 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -441,14 +441,11 @@ static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather)
>   	};
>   }
>   
> +/* May return NULL if the device has no iommu driver */
>   static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
>   {
> -	/*
> -	 * Assume that valid ops must be installed if iommu_probe_device()
> -	 * has succeeded. The device ops are essentially for internal use
> -	 * within the IOMMU subsystem itself, so we should be able to trust
> -	 * ourselves not to misuse the helper.
> -	 */
> +	if (!dev->iommu)
> +		return NULL;
>   	return dev->iommu->iommu_dev->ops;

This is actively broken, since dev->iommu may be non-NULL before 
dev->iommu->iommu_dev is set (a fwspec will always be set before the 
instyance is registered, and a device may potentially hang around in 
that state for evertt if the relevant IOMMU instance never appears).

Sorry, I don't think any of this makes sense :/

Thanks,
Robin.

Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Jason Gunthorpe]

On Mon, Jan 30, 2023 at 06:05:12PM +0000, Robin Murphy wrote:
> >    * Use a function instead of an array here because the domain-type is a
> >    * bit-field, so an array would waste memory.
> > @@ -338,7 +352,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> >   	dev->iommu->iommu_dev = iommu_dev;
> >   	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
> > -	group = iommu_group_get_for_dev(dev);
> > +	group = iommu_group_get_for_dev(dev, ops);
> 
> Why? We've literally just assigned dev->iommu->iommu_dev 2 lines above; it
> is not allowed to have invalid ops. Plus in future they may potentially be a
> different set of device ops from the ones we initially found to provide
> ->probe_device - in that case we definitely want group_get_for_dev to use
> the proper instance ops. This is the only place it is (and should be)
> called, so I don't see any problem.

Sure, but IMHO it was clearer to pass the ops down than to obtain it
again. But maybe this could be tidied in a different way.

> >   	if (IS_ERR(group)) {
> >   		ret = PTR_ERR(group);
> >   		goto out_release;
> > @@ -414,7 +428,8 @@ int iommu_probe_device(struct device *dev)
> >   	mutex_unlock(&group->mutex);
> >   	iommu_group_put(group);
> > -	ops = dev_iommu_ops(dev);
> > +	/* __iommu_probe_device() set the ops */
> > +	ops = dev_iommu_ops_safe(dev);
> >   	if (ops->probe_finalize)
> >   		ops->probe_finalize(dev);
> > @@ -430,14 +445,13 @@ int iommu_probe_device(struct device *dev)
> >   void iommu_release_device(struct device *dev)
> >   {
> > -	const struct iommu_ops *ops;
> > +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> 
> This is just moving an existing check around.

The point is to have one check. If you need to get the ops and you
don't know the state of dev then you calll dev_iommu_ops() and check
for NULL. Simple and consistent.

> > -     if (!dev->iommu)
> > +     if (!ops)
> >               return;

As you pointed out below this isn't even coded right since iommu can
be non-NULL.

> > @@ -2620,7 +2626,11 @@ void iommu_get_resv_regions(struct device *dev, struct list_head *list)
> >   {
> >   	const struct iommu_ops *ops = dev_iommu_ops(dev);
> > -	if (ops->get_resv_regions)
> > +	/*
> > +	 * Non-API groups still expose reserved_regions in sysfs, so filter out
> > +	 * calls that get here that way.
> > +	 */
> > +	if (ops && ops->get_resv_regions)
> 
> This is just moving the existing check from the public interface to
> pointlessly impose it on the internal call path as well.

Who cares? We don't need to micro-optimize this stuff. The fact there
are a few bugs already is proof enough of that.

> >   		ops->get_resv_regions(dev, list);
> >   }
> > @@ -2979,6 +2989,11 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> >   	/* Since group has only one device */
> >   	grp_dev = list_first_entry(&group->devices, struct group_device, list);
> >   	dev = grp_dev->dev;
> > +	if (!dev_iommu_ops(dev)) {
> > +		/* The group doesn't have an iommu driver attached */
> > +		mutex_unlock(&group->mutex);
> > +		return -EINVAL;
> > +	}
> 
> Withot any ops, group->default_domain will be NULL so we could never even
> get here.

Fair enough, deserves a comment

> >   	get_device(dev);
> >   	/*
> > @@ -3301,7 +3316,7 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
> >   	const struct iommu_ops *ops;
> >   	list_for_each_entry(device, &group->devices, list) {
> > -		ops = dev_iommu_ops(device->dev);
> > +		ops = dev_iommu_ops_safe(device->dev);
> >   		ops->remove_dev_pasid(device->dev, pasid);
> >   	}
> >   }
> > @@ -3413,6 +3428,9 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
> >   	const struct iommu_ops *ops = dev_iommu_ops(dev);
> >   	struct iommu_domain *domain;
> > +	if (!ops)
> > +		return NULL;
> 
> Anyone who incorrectly calls sva_domain_alloc() directly without having
> successfully called iommu_dev_enable_feature() first deserves to crash.

Lets not build assumptions like this into the code please. That
iommu_dev_enable_feature() thing is on my hitlist too :(

> (Incidentally, you've missed enable/disable_feature here.)

Yes, because they don't call dev_iommu_ops for some reason. It should
be fixed too.

> > +/* May return NULL if the device has no iommu driver */
> >   static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
> >   {
> > -	/*
> > -	 * Assume that valid ops must be installed if iommu_probe_device()
> > -	 * has succeeded. The device ops are essentially for internal use
> > -	 * within the IOMMU subsystem itself, so we should be able to trust
> > -	 * ourselves not to misuse the helper.
> > -	 */
> > +	if (!dev->iommu)
> > +		return NULL;
> >   	return dev->iommu->iommu_dev->ops;
> 
> This is actively broken, since dev->iommu may be non-NULL before
> dev->iommu->iommu_dev is set (a fwspec will always be set before the
> instyance is registered, and a device may potentially hang around in that
> state for evertt if the relevant IOMMU instance never appears).

Sure, I missed a NULL

> Sorry, I don't think any of this makes sense :/

The point is to be more consistent and not just blindly add more
helpers. If you need ops then ask for the ops. If you aren't sure the
dev has ops then check ops for NULL. It is pretty simple.

Jason

Re: [PATCH v2 1/8] iommu: Decouple iommu_present() from bus ops

[Robin Murphy]

On 2023-01-30 17:31, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2023 at 06:26:16PM +0000, 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.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> This below might help. You might need to ask Thierry for help on the
> problematic stuff in tegra. Really we should finish the series here
> and organize things so that the domain allocation is defered until a
> client is present.

mrk_drm doesn't even allocate, it relies on the DMA API domain; it also 
only runs on DT systems where IOMMU dependency ordering has been 
enforced by core code for a very long time now. I sent a patch simply 
removing that check back at the time of the rest of my iommu_present() 
cleanup, and it just got resoundingly ignored :(

Thanks,
Robin.

> https://lore.kernel.org/linux-iommu/20220106022053.2406748-1-baolu.lu@linux.intel.com/
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index cd5b18ef79512c..e5c257e41f4933 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -353,9 +353,6 @@ static int mtk_drm_kms_init(struct drm_device *drm)
>   	if (drm_firmware_drivers_only())
>   		return -ENODEV;
>   
> -	if (!iommu_present(&platform_bus_type))
> -		return -EPROBE_DEFER;
> -
>   	pdev = of_find_device_by_node(private->mutex_node);
>   	if (!pdev) {
>   		dev_err(drm->dev, "Waiting for disp-mutex device %pOF\n",
> @@ -709,6 +706,9 @@ static int mtk_drm_probe(struct platform_device *pdev)
>   	int ret;
>   	int i;
>   
> +	if (!device_iommu_mapped(dev))
> +		return -EPROBE_DEFER;
> +
>   	private = devm_kzalloc(dev, sizeof(*private), GFP_KERNEL);
>   	if (!private)
>   		return -ENOMEM;

Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Robin Murphy]

On 2023-01-30 18:20, Jason Gunthorpe wrote:
> On Mon, Jan 30, 2023 at 06:05:12PM +0000, Robin Murphy wrote:
>>>     * Use a function instead of an array here because the domain-type is a
>>>     * bit-field, so an array would waste memory.
>>> @@ -338,7 +352,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>>>    	dev->iommu->iommu_dev = iommu_dev;
>>>    	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
>>> -	group = iommu_group_get_for_dev(dev);
>>> +	group = iommu_group_get_for_dev(dev, ops);
>>
>> Why? We've literally just assigned dev->iommu->iommu_dev 2 lines above; it
>> is not allowed to have invalid ops. Plus in future they may potentially be a
>> different set of device ops from the ones we initially found to provide
>> ->probe_device - in that case we definitely want group_get_for_dev to use
>> the proper instance ops. This is the only place it is (and should be)
>> called, so I don't see any problem.
> 
> Sure, but IMHO it was clearer to pass the ops down than to obtain it
> again. But maybe this could be tidied in a different way.

I disagree. In what we have now, every operation which calls into a 
driver consistently uses the instance ops (or domain ops), except for 
->probe_device for obvious reasons. Making ->device_group look special 
for no technical reason is less consistent, and as such less clear. It 
would be the only place in the kernel where ops are called from a 
function argument, and to anyone unfamiliar looking at the code and 
wondering why that is, the answer "because Jason thinks it looks better" 
is not going to be obvious at all.

>>>    	if (IS_ERR(group)) {
>>>    		ret = PTR_ERR(group);
>>>    		goto out_release;
>>> @@ -414,7 +428,8 @@ int iommu_probe_device(struct device *dev)
>>>    	mutex_unlock(&group->mutex);
>>>    	iommu_group_put(group);
>>> -	ops = dev_iommu_ops(dev);
>>> +	/* __iommu_probe_device() set the ops */
>>> +	ops = dev_iommu_ops_safe(dev);
>>>    	if (ops->probe_finalize)
>>>    		ops->probe_finalize(dev);
>>> @@ -430,14 +445,13 @@ int iommu_probe_device(struct device *dev)
>>>    void iommu_release_device(struct device *dev)
>>>    {
>>> -	const struct iommu_ops *ops;
>>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>>
>> This is just moving an existing check around.
> 
> The point is to have one check. If you need to get the ops and you
> don't know the state of dev then you calll dev_iommu_ops() and check
> for NULL. Simple and consistent.
> 
>>> -     if (!dev->iommu)
>>> +     if (!ops)
>>>                return;
> 
> As you pointed out below this isn't even coded right since iommu can
> be non-NULL.

Oh, indeed that is technically a bug, although it's pretty much 
impossible to hit in practice because there's no real device hotplug on 
DT-based systems using fwspec - "dynamic" buses like PCI SR-IOV or 
fsl-mc aren't going to be discovered at all until the relevant IOMMU 
associated with the main controller device is ready, thus no removable 
child device would ever be in the "half-probed" state. I missed this one 
since I was looking for the dev->iommu->iommu_dev pattern - since it 
looks like this series is headed for a v3 next cycle, I've added this 
site to $SUBJECT locally.

>>> @@ -2620,7 +2626,11 @@ void iommu_get_resv_regions(struct device *dev, struct list_head *list)
>>>    {
>>>    	const struct iommu_ops *ops = dev_iommu_ops(dev);
>>> -	if (ops->get_resv_regions)
>>> +	/*
>>> +	 * Non-API groups still expose reserved_regions in sysfs, so filter out
>>> +	 * calls that get here that way.
>>> +	 */
>>> +	if (ops && ops->get_resv_regions)
>>
>> This is just moving the existing check from the public interface to
>> pointlessly impose it on the internal call path as well.
> 
> Who cares? We don't need to micro-optimize this stuff. The fact there
> are a few bugs already is proof enough of that.

"a few"? So far there's only one, and it's not even the class of bug 
you're trying to address, because there _is_ an explicit validity check 
already, it just has an oversight in it.

This isn't micro-optimisation, it's consistency: we have validity checks 
close to the entrypoints of public interfaces where they are required. 
On internal paths where they are not required, we do not sometimes have 
checks and sometimes not, leaving people to wonder what the requirements 
actually are - in fact someone went so far as to call such patterns 
"confusing" and "overzealous" back when dev_iommu_ops() was reviewed. It 
was agreed that the lack of a check where ops are retrieved clearly 
documents where they are expected to be valid.

>>>    		ops->get_resv_regions(dev, list);
>>>    }
>>> @@ -2979,6 +2989,11 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>>>    	/* Since group has only one device */
>>>    	grp_dev = list_first_entry(&group->devices, struct group_device, list);
>>>    	dev = grp_dev->dev;
>>> +	if (!dev_iommu_ops(dev)) {
>>> +		/* The group doesn't have an iommu driver attached */
>>> +		mutex_unlock(&group->mutex);
>>> +		return -EINVAL;
>>> +	}
>>
>> Withot any ops, group->default_domain will be NULL so we could never even
>> get here.
> 
> Fair enough, deserves a comment

Great big function specifically for changing the default domain of a 
group... right at the top, literally the second thing it does on entry 
is check that the group and default domain are valid, and return if not. 
If that isn't sufficiently clear, I'm not sure what to say :/

>>>    	get_device(dev);
>>>    	/*
>>> @@ -3301,7 +3316,7 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>>>    	const struct iommu_ops *ops;
>>>    	list_for_each_entry(device, &group->devices, list) {
>>> -		ops = dev_iommu_ops(device->dev);
>>> +		ops = dev_iommu_ops_safe(device->dev);
>>>    		ops->remove_dev_pasid(device->dev, pasid);
>>>    	}
>>>    }
>>> @@ -3413,6 +3428,9 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
>>>    	const struct iommu_ops *ops = dev_iommu_ops(dev);
>>>    	struct iommu_domain *domain;
>>> +	if (!ops)
>>> +		return NULL;
>>
>> Anyone who incorrectly calls sva_domain_alloc() directly without having
>> successfully called iommu_dev_enable_feature() first deserves to crash.
> 
> Lets not build assumptions like this into the code please. That
> iommu_dev_enable_feature() thing is on my hitlist too :(

Huh? The whole public API is full of assumptions that callers aren't 
doing nonsensical things. Pass a bogus group or domain pointer to 
iommu_attach_group()? Boom! Pass a bogus domain pointer to iommu_map()? 
Boom! AFAICT the only thing that should be calling 
iommu_sva_domain_alloc() at all at the moment is 
iommu_sva_bind_device(), so it's effectively an internal interface where 
bogus device pointers really shouldn't be expected at all.

Sure, if you change the interface so that random drivers are free to 
allocate SVA domains directly and feed them to iommu_attach_group(), and 
checks are warranted in different places, then add them then.

>> (Incidentally, you've missed enable/disable_feature here.)
> 
> Yes, because they don't call dev_iommu_ops for some reason. It should
> be fixed too.

It is, in the patch you're replying to.

>>> +/* May return NULL if the device has no iommu driver */
>>>    static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
>>>    {
>>> -	/*
>>> -	 * Assume that valid ops must be installed if iommu_probe_device()
>>> -	 * has succeeded. The device ops are essentially for internal use
>>> -	 * within the IOMMU subsystem itself, so we should be able to trust
>>> -	 * ourselves not to misuse the helper.
>>> -	 */
>>> +	if (!dev->iommu)
>>> +		return NULL;
>>>    	return dev->iommu->iommu_dev->ops;
>>
>> This is actively broken, since dev->iommu may be non-NULL before
>> dev->iommu->iommu_dev is set (a fwspec will always be set before the
>> instyance is registered, and a device may potentially hang around in that
>> state for evertt if the relevant IOMMU instance never appears).
> 
> Sure, I missed a NULL
> 
>> Sorry, I don't think any of this makes sense :/
> 
> The point is to be more consistent and not just blindly add more
> helpers. If you need ops then ask for the ops. If you aren't sure the
> dev has ops then check ops for NULL. It is pretty simple.

I'm not "blindly" adding more helpers, I'm ratifying clear common 
elements of the code and logic that we already have, to make them that 
much easier to replicate further. And I'm still no less puzzled by this 
thread... adding a new helper to consolidate having one thing in some 
places and another in others so offends your sensibilities that what 
you'd much rather do instead is add a new helper to have one thing in 
some places and another thing in others? There is a logical consistency 
to the current code already, which I assume is sufficiently clear to the 
majority of us because it's what made it through community review and 
what we've all been working on top of for the last year. "If you need 
ops then ask for the ops. If you aren't sure the dev has ops then check 
for valid ops first." Just as simple.

FWIW, personally I find up-front availability checks perfectly intuitive 
and natural, because they can easily be imagined as a real-life interaction:

"Do you have any coffee?"
"No, sorry."

vs.

"Please give me a cup of coffee."
<hands over empty cup>
"Is this coffee?"
"No."

One could possibly even argue that a separate up-front check also 
visibly implies that there are no concurrency or TOCTOU considerations 
expected, which for dev_iommu_ops() there should certainly not be.

Please understand that I'm not going to this length just to be 
objectionable; this is me genuinely being unable to rationalise your 
argument and spending my entire evening on a deep dive into my own 
thought process to lay it out and check for any obvious errors.

Thanks,
Robin.

Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Jason Gunthorpe]

On Mon, Jan 30, 2023 at 11:33:54PM +0000, Robin Murphy wrote:

> Please understand that I'm not going to this length just to be
> objectionable; this is me genuinely being unable to rationalise your
> argument and spending my entire evening on a deep dive into my own thought
> process to lay it out and check for any obvious errors.

Sorry, I don't want to use up your time like this. It is a minor style
remark, if you don't like it then you should go ahead with your
original.

It is hard to debate style, everyone has their own viewpoint of what
is better style or not.

But to elaborate my feeling, I find this:

	const struct iommu_ops *ops = dev_iommu_ops(dev);

	if (!ops)
	   return -ENODEV

To be nicer code and more kernely then this:

	const struct iommu_ops *ops;

	if (!dev_iomm_ops_valid(dev))
	   return -ENODEV
        ops = dev_iommu_ops(dev);

In part because the former is a harder to type wrong and when used
consistently it is alot more obvious that the NULL test is
missing. static checkers like smatch/coverity will even warn on the
obvious possible NULL deref if someone misses the NULL test.

In general in kernel we have the API flow of call a function and check
the result for error, asking permission to call an API is less
typical.

This case is complicated because of the effort to try and document
cases that can't fail. I'm not sure if this is worthwhile, but..

Documentation of special cases is better by labeling them as a special
case, eg with a special function name. Think
rcu_dereference_protected(). Making them special by observing a
missing related function call is trickier to learn and remember.

Also if you rely on ops testing you are sort of forced into a second
function because the static tools will complain about all the places
that don't test for null if only one API is provided.

Basically, if you have dev_iommu_ops/dev_iommu_ops_safe then the
choice to use safe is obviously documented in the code and if you
mis-use dev_iommu_ops then a static tool will complain. Reviewers who
see 'safe' in a diff are reminded to look at it for safety rules.

If you have dev_iommu_ops/dev_iommu_ops_valid then static tools will
never complain and the choice to use 'safe' is implicitly documented
by not calling dev_iommu_ops_valid. Reviewers have to remember to
check every call to dev_iommu_ops() to see if it should have the valid
check.

Regards,
Jason