[PATCH v2 00/14] iommu: Retire bus_set_iommu()

[PATCH v2 00/14] iommu: Retire bus_set_iommu()

[Robin Murphy]

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

Hi all,

Just some minor updates for v2, adding a workaround to avoid changing
VT-d behaviour for now, cleaning up the extra include I missed in
virtio-iommu, and collecting all the acks so far. As before, this is
based on the AMD IOMMU patch now applied, plus v2 of my
device_iommu_capable() series here:

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

Cheers,
Robin.



Robin Murphy (14):
  iommu/vt-d: Temporarily reject probing non-PCI devices
  iommu: Always register bus notifiers
  iommu: Move bus setup to IOMMU device registration
  iommu/amd: Clean up bus_set_iommu()
  iommu/arm-smmu: Clean up bus_set_iommu()
  iommu/arm-smmu-v3: Clean up bus_set_iommu()
  iommu/dart: Clean up bus_set_iommu()
  iommu/exynos: Clean up bus_set_iommu()
  iommu/ipmmu-vmsa: Clean up bus_set_iommu()
  iommu/mtk: Clean up bus_set_iommu()
  iommu/omap: Clean up bus_set_iommu()
  iommu/tegra-smmu: Clean up bus_set_iommu()
  iommu/virtio: Clean up bus_set_iommu()
  iommu: Clean up bus_set_iommu()

 drivers/iommu/amd/amd_iommu.h               |   1 -
 drivers/iommu/amd/init.c                    |   9 +-
 drivers/iommu/amd/iommu.c                   |  21 ----
 drivers/iommu/apple-dart.c                  |  30 +-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  53 +---------
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  84 +--------------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |   4 -
 drivers/iommu/exynos-iommu.c                |   9 --
 drivers/iommu/fsl_pamu_domain.c             |   4 -
 drivers/iommu/intel/iommu.c                 |   5 +-
 drivers/iommu/iommu.c                       | 110 +++++++++-----------
 drivers/iommu/ipmmu-vmsa.c                  |  35 +------
 drivers/iommu/msm_iommu.c                   |   2 -
 drivers/iommu/mtk_iommu.c                   |  13 +--
 drivers/iommu/mtk_iommu_v1.c                |  13 +--
 drivers/iommu/omap-iommu.c                  |   6 --
 drivers/iommu/rockchip-iommu.c              |   2 -
 drivers/iommu/s390-iommu.c                  |   6 --
 drivers/iommu/sprd-iommu.c                  |   5 -
 drivers/iommu/sun50i-iommu.c                |   2 -
 drivers/iommu/tegra-smmu.c                  |  29 ++----
 drivers/iommu/virtio-iommu.c                |  25 -----
 include/linux/iommu.h                       |   1 -
 23 files changed, 67 insertions(+), 402 deletions(-)

-- 
2.35.3.dirty

[PATCH v2 01/14] iommu/vt-d: Temporarily reject probing non-PCI devices

[Robin Murphy]

Although the driver has some support implemented for non-PCI devices via
ANDD, it only registers itself for pci_bus_type, so has never actually
seen probe_device for a non-PCI device. Once the bus details move into
the IOMMU core, it appears there may be some issues with correctly
rejecting non-ANDD platform devices, so let's temporarily enforce the
current behaviour of only considering PCI devices until that can be
investigated properly.

Reported-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/intel/iommu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 0edf6084dc14..9507b64fdf6b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4565,6 +4565,10 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
 	unsigned long flags;
 	u8 bus, devfn;
 
+	/* ANDD platform device support needs fixing */
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
 	iommu = device_to_iommu(dev, &bus, &devfn);
 	if (!iommu)
 		return ERR_PTR(-ENODEV);
-- 
2.35.3.dirty

[PATCH v2 02/14] iommu: Always register bus notifiers

[Robin Murphy]

The number of bus types that the IOMMU subsystem deals with is small and
manageable, so pull that list into core code as a first step towards
cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device() before bus->iommu_ops is set will simply return
-ENODEV and not break the notifier call chain, so there should be no
harm in proactively registering all our bus notifiers at init time.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-By: Krishna Reddy <vdumpa@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 49 ++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 00218b56a441..6c4621afc8cf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -6,6 +6,7 @@
 
 #define pr_fmt(fmt)    "iommu: " fmt
 
+#include <linux/amba/bus.h>
 #include <linux/device.h>
 #include <linux/dma-iommu.h>
 #include <linux/kernel.h>
@@ -22,6 +23,7 @@
 #include <linux/err.h>
 #include <linux/pci.h>
 #include <linux/bitops.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/fsl/mc.h>
 #include <linux/module.h>
@@ -74,6 +76,7 @@ static const char * const iommu_group_resv_type_string[] = {
 #define IOMMU_CMD_LINE_DMA_API		BIT(0)
 #define IOMMU_CMD_LINE_STRICT		BIT(1)
 
+static int iommu_bus_init(struct bus_type *bus);
 static int iommu_alloc_default_domain(struct iommu_group *group,
 				      struct device *dev);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
@@ -102,6 +105,19 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
 static LIST_HEAD(iommu_device_list);
 static DEFINE_SPINLOCK(iommu_device_lock);
 
+static struct bus_type * const iommu_buses[] = {
+	&platform_bus_type,
+#ifdef CONFIG_PCI
+	&pci_bus_type,
+#endif
+#ifdef CONFIG_ARM_AMBA
+	&amba_bustype,
+#endif
+#ifdef CONFIG_FSL_MC_BUS
+	&fsl_mc_bus_type,
+#endif
+};
+
 /*
  * Use a function instead of an array here because the domain-type is a
  * bit-field, so an array would waste memory.
@@ -151,6 +167,10 @@ static int __init iommu_subsys_init(void)
 			(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
 				"(set via kernel command line)" : "");
 
+	/* If the system is so broken that this fails, it will WARN anyway */
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
+		iommu_bus_init(iommu_buses[i]);
+
 	return 0;
 }
 subsys_initcall(iommu_subsys_init);
@@ -1832,35 +1852,19 @@ int bus_iommu_probe(struct bus_type *bus)
 	return ret;
 }
 
-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
+static int iommu_bus_init(struct bus_type *bus)
 {
 	struct notifier_block *nb;
 	int err;
 
-	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
+	nb = kzalloc(sizeof(*nb), GFP_KERNEL);
 	if (!nb)
 		return -ENOMEM;
 
 	nb->notifier_call = iommu_bus_notifier;
-
 	err = bus_register_notifier(bus, nb);
 	if (err)
-		goto out_free;
-
-	err = bus_iommu_probe(bus);
-	if (err)
-		goto out_err;
-
-
-	return 0;
-
-out_err:
-	/* Clean up */
-	bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
-	bus_unregister_notifier(bus, nb);
-
-out_free:
-	kfree(nb);
+		kfree(nb);
 
 	return err;
 }
@@ -1893,9 +1897,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
 	bus->iommu_ops = ops;
 
 	/* Do IOMMU specific setup for this bus-type */
-	err = iommu_bus_init(bus, ops);
-	if (err)
+	err = bus_iommu_probe(bus);
+	if (err) {
+		/* Clean up */
+		bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
 		bus->iommu_ops = NULL;
+	}
 
 	return err;
 }
-- 
2.35.3.dirty

[PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

Move the bus setup to iommu_device_register(). This should allow
bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.

At this point we can also handle cleanup better than just rolling back
the most-recently-touched bus upon failure - which may release devices
owned by other already-registered instances, and still leave devices on
other buses with dangling pointers to the failed instance. Now it's easy
to clean up the exact footprint of a given instance, no more, no less.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 51 +++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6c4621afc8cf..c89af4dc54c2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -175,6 +175,14 @@ static int __init iommu_subsys_init(void)
 }
 subsys_initcall(iommu_subsys_init);
 
+static int remove_iommu_group(struct device *dev, void *data)
+{
+	if (dev->iommu && dev->iommu->iommu_dev == data)
+		iommu_release_device(dev);
+
+	return 0;
+}
+
 /**
  * iommu_device_register() - Register an IOMMU hardware instance
  * @iommu: IOMMU handle for the instance
@@ -197,12 +205,29 @@ int iommu_device_register(struct iommu_device *iommu,
 	spin_lock(&iommu_device_lock);
 	list_add_tail(&iommu->list, &iommu_device_list);
 	spin_unlock(&iommu_device_lock);
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+		struct bus_type *bus = iommu_buses[i];
+		int err;
+
+		WARN_ON(bus->iommu_ops && bus->iommu_ops != ops);
+		bus->iommu_ops = ops;
+		err = bus_iommu_probe(bus);
+		if (err) {
+			iommu_device_unregister(iommu);
+			return err;
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_device_register);
 
 void iommu_device_unregister(struct iommu_device *iommu)
 {
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
+		bus_for_each_dev(iommu_buses[i], NULL, iommu, remove_iommu_group);
+
 	spin_lock(&iommu_device_lock);
 	list_del(&iommu->list);
 	spin_unlock(&iommu_device_lock);
@@ -1655,13 +1680,6 @@ static int probe_iommu_group(struct device *dev, void *data)
 	return ret;
 }
 
-static int remove_iommu_group(struct device *dev, void *data)
-{
-	iommu_release_device(dev);
-
-	return 0;
-}
-
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data)
 {
@@ -1884,27 +1902,12 @@ static int iommu_bus_init(struct bus_type *bus)
  */
 int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
 {
-	int err;
-
-	if (ops == NULL) {
-		bus->iommu_ops = NULL;
-		return 0;
-	}
-
-	if (bus->iommu_ops != NULL)
+	if (bus->iommu_ops && ops && bus->iommu_ops != ops)
 		return -EBUSY;
 
 	bus->iommu_ops = ops;
 
-	/* Do IOMMU specific setup for this bus-type */
-	err = bus_iommu_probe(bus);
-	if (err) {
-		/* Clean up */
-		bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
-		bus->iommu_ops = NULL;
-	}
-
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(bus_set_iommu);
 
-- 
2.35.3.dirty

[PATCH v2 04/14] iommu/amd: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and
garbage-collect the last remnants of amd_iommu_init_api().

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/amd/amd_iommu.h |  1 -
 drivers/iommu/amd/init.c      |  9 +--------
 drivers/iommu/amd/iommu.c     | 21 ---------------------
 3 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 1ab31074f5b3..384393ce57fb 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -18,7 +18,6 @@ extern void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
 extern int amd_iommu_init_devices(void);
 extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
-extern int amd_iommu_init_api(void);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 0467918bf7fd..1cb10d8b0df4 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1970,20 +1970,13 @@ static int __init amd_iommu_init_pci(void)
 	/*
 	 * Order is important here to make sure any unity map requirements are
 	 * fulfilled. The unity mappings are created and written to the device
-	 * table during the amd_iommu_init_api() call.
+	 * table during the iommu_init_pci() call.
 	 *
 	 * After that we call init_device_table_dma() to make sure any
 	 * uninitialized DTE will block DMA, and in the end we flush the caches
 	 * of all IOMMUs to make sure the changes to the device table are
 	 * active.
 	 */
-	ret = amd_iommu_init_api();
-	if (ret) {
-		pr_err("IOMMU: Failed to initialize IOMMU-API interface (error=%d)!\n",
-		       ret);
-		goto out;
-	}
-
 	init_device_table_dma();
 
 	for_each_iommu(iommu)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 038e104b922c..d907c96ff84e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -11,8 +11,6 @@
 #include <linux/ratelimit.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
-#include <linux/amba/bus.h>
-#include <linux/platform_device.h>
 #include <linux/pci-ats.h>
 #include <linux/bitmap.h>
 #include <linux/slab.h>
@@ -1838,25 +1836,6 @@ void amd_iommu_domain_update(struct protection_domain *domain)
 	amd_iommu_domain_flush_complete(domain);
 }
 
-int __init amd_iommu_init_api(void)
-{
-	int err;
-
-	err = bus_set_iommu(&pci_bus_type, &amd_iommu_ops);
-	if (err)
-		return err;
-#ifdef CONFIG_ARM_AMBA
-	err = bus_set_iommu(&amba_bustype, &amd_iommu_ops);
-	if (err)
-		return err;
-#endif
-	err = bus_set_iommu(&platform_bus_type, &amd_iommu_ops);
-	if (err)
-		return err;
-
-	return 0;
-}
-
 /*****************************************************************************
  *
  * The following functions belong to the exported interface of AMD IOMMU
-- 
2.35.3.dirty

[PATCH v2 05/14] iommu/arm-smmu: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary. With device
probes now replayed for every IOMMU instance registration, the whole
sorry ordering workaround for legacy DT bindings goes too, hooray!

Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +--------------------------
 1 file changed, 2 insertions(+), 82 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 568cce590ccc..f0bec4a35df5 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -37,7 +37,6 @@
 #include <linux/ratelimit.h>
 #include <linux/slab.h>
 
-#include <linux/amba/bus.h>
 #include <linux/fsl/mc.h>
 
 #include "arm-smmu.h"
@@ -93,8 +92,6 @@ static struct platform_driver arm_smmu_driver;
 static struct iommu_ops arm_smmu_ops;
 
 #ifdef CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS
-static int arm_smmu_bus_init(struct iommu_ops *ops);
-
 static struct device_node *dev_get_dev_node(struct device *dev)
 {
 	if (dev_is_pci(dev)) {
@@ -180,20 +177,6 @@ static int arm_smmu_register_legacy_master(struct device *dev,
 	kfree(sids);
 	return err;
 }
-
-/*
- * With the legacy DT binding in play, we have no guarantees about
- * probe order, but then we're also not doing default domains, so we can
- * delay setting bus ops until we're sure every possible SMMU is ready,
- * and that way ensure that no probe_device() calls get missed.
- */
-static int arm_smmu_legacy_bus_init(void)
-{
-	if (using_legacy_binding)
-		return arm_smmu_bus_init(&arm_smmu_ops);
-	return 0;
-}
-device_initcall_sync(arm_smmu_legacy_bus_init);
 #else
 static int arm_smmu_register_legacy_master(struct device *dev,
 					   struct arm_smmu_device **smmu)
@@ -2022,52 +2005,6 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,
 	return 0;
 }
 
-static int arm_smmu_bus_init(struct iommu_ops *ops)
-{
-	int err;
-
-	/* Oh, for a proper bus abstraction */
-	if (!iommu_present(&platform_bus_type)) {
-		err = bus_set_iommu(&platform_bus_type, ops);
-		if (err)
-			return err;
-	}
-#ifdef CONFIG_ARM_AMBA
-	if (!iommu_present(&amba_bustype)) {
-		err = bus_set_iommu(&amba_bustype, ops);
-		if (err)
-			goto err_reset_platform_ops;
-	}
-#endif
-#ifdef CONFIG_PCI
-	if (!iommu_present(&pci_bus_type)) {
-		err = bus_set_iommu(&pci_bus_type, ops);
-		if (err)
-			goto err_reset_amba_ops;
-	}
-#endif
-#ifdef CONFIG_FSL_MC_BUS
-	if (!iommu_present(&fsl_mc_bus_type)) {
-		err = bus_set_iommu(&fsl_mc_bus_type, ops);
-		if (err)
-			goto err_reset_pci_ops;
-	}
-#endif
-	return 0;
-
-err_reset_pci_ops: __maybe_unused;
-#ifdef CONFIG_PCI
-	bus_set_iommu(&pci_bus_type, NULL);
-#endif
-err_reset_amba_ops: __maybe_unused;
-#ifdef CONFIG_ARM_AMBA
-	bus_set_iommu(&amba_bustype, NULL);
-#endif
-err_reset_platform_ops: __maybe_unused;
-	bus_set_iommu(&platform_bus_type, NULL);
-	return err;
-}
-
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -2185,7 +2122,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
-		goto err_sysfs_remove;
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return err;
 	}
 
 	platform_set_drvdata(pdev, smmu);
@@ -2203,24 +2141,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		pm_runtime_enable(dev);
 	}
 
-	/*
-	 * For ACPI and generic DT bindings, an SMMU will be probed before
-	 * any device which might need it, so we want the bus ops in place
-	 * ready to handle default domain setup as soon as any SMMU exists.
-	 */
-	if (!using_legacy_binding) {
-		err = arm_smmu_bus_init(&arm_smmu_ops);
-		if (err)
-			goto err_unregister_device;
-	}
-
 	return 0;
-
-err_unregister_device:
-	iommu_device_unregister(&smmu->iommu);
-err_sysfs_remove:
-	iommu_device_sysfs_remove(&smmu->iommu);
-	return err;
 }
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
@@ -2233,7 +2154,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
 		dev_notice(&pdev->dev, "disabling translation\n");
 
-	arm_smmu_bus_init(NULL);
 	iommu_device_unregister(&smmu->iommu);
 	iommu_device_sysfs_remove(&smmu->iommu);
 
-- 
2.35.3.dirty

[PATCH v2 06/14] iommu/arm-smmu-v3: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +--------------------
 1 file changed, 2 insertions(+), 51 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 627a3ed5ee8f..73b7b1b17b77 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -28,8 +28,6 @@
 #include <linux/pci-ats.h>
 #include <linux/platform_device.h>
 
-#include <linux/amba/bus.h>
-
 #include "arm-smmu-v3.h"
 #include "../../iommu-sva-lib.h"
 
@@ -3698,43 +3696,6 @@ static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)
 		return SZ_128K;
 }
 
-static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
-{
-	int err;
-
-#ifdef CONFIG_PCI
-	if (pci_bus_type.iommu_ops != ops) {
-		err = bus_set_iommu(&pci_bus_type, ops);
-		if (err)
-			return err;
-	}
-#endif
-#ifdef CONFIG_ARM_AMBA
-	if (amba_bustype.iommu_ops != ops) {
-		err = bus_set_iommu(&amba_bustype, ops);
-		if (err)
-			goto err_reset_pci_ops;
-	}
-#endif
-	if (platform_bus_type.iommu_ops != ops) {
-		err = bus_set_iommu(&platform_bus_type, ops);
-		if (err)
-			goto err_reset_amba_ops;
-	}
-
-	return 0;
-
-err_reset_amba_ops:
-#ifdef CONFIG_ARM_AMBA
-	bus_set_iommu(&amba_bustype, NULL);
-#endif
-err_reset_pci_ops: __maybe_unused;
-#ifdef CONFIG_PCI
-	bus_set_iommu(&pci_bus_type, NULL);
-#endif
-	return err;
-}
-
 static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
 				      resource_size_t size)
 {
@@ -3838,27 +3799,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	ret = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (ret) {
 		dev_err(dev, "Failed to register iommu\n");
-		goto err_sysfs_remove;
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return ret;
 	}
 
-	ret = arm_smmu_set_bus_ops(&arm_smmu_ops);
-	if (ret)
-		goto err_unregister_device;
-
 	return 0;
-
-err_unregister_device:
-	iommu_device_unregister(&smmu->iommu);
-err_sysfs_remove:
-	iommu_device_sysfs_remove(&smmu->iommu);
-	return ret;
 }
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 
-	arm_smmu_set_bus_ops(NULL);
 	iommu_device_unregister(&smmu->iommu);
 	iommu_device_sysfs_remove(&smmu->iommu);
 	arm_smmu_device_disable(smmu);
-- 
2.35.3.dirty

[PATCH v2 07/14] iommu/dart: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Tested-by: Sven Peter <sven@svenpeter.dev>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/apple-dart.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index decafb07ad08..a679e4c02291 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -823,27 +823,6 @@ static irqreturn_t apple_dart_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static int apple_dart_set_bus_ops(const struct iommu_ops *ops)
-{
-	int ret;
-
-	if (!iommu_present(&platform_bus_type)) {
-		ret = bus_set_iommu(&platform_bus_type, ops);
-		if (ret)
-			return ret;
-	}
-#ifdef CONFIG_PCI
-	if (!iommu_present(&pci_bus_type)) {
-		ret = bus_set_iommu(&pci_bus_type, ops);
-		if (ret) {
-			bus_set_iommu(&platform_bus_type, NULL);
-			return ret;
-		}
-	}
-#endif
-	return 0;
-}
-
 static int apple_dart_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -899,14 +878,10 @@ static int apple_dart_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dart);
 
-	ret = apple_dart_set_bus_ops(&apple_dart_iommu_ops);
-	if (ret)
-		goto err_free_irq;
-
 	ret = iommu_device_sysfs_add(&dart->iommu, dev, NULL, "apple-dart.%s",
 				     dev_name(&pdev->dev));
 	if (ret)
-		goto err_remove_bus_ops;
+		goto err_free_irq;
 
 	ret = iommu_device_register(&dart->iommu, &apple_dart_iommu_ops, dev);
 	if (ret)
@@ -920,8 +895,6 @@ static int apple_dart_probe(struct platform_device *pdev)
 
 err_sysfs_remove:
 	iommu_device_sysfs_remove(&dart->iommu);
-err_remove_bus_ops:
-	apple_dart_set_bus_ops(NULL);
 err_free_irq:
 	free_irq(dart->irq, dart);
 err_clk_disable:
@@ -936,7 +909,6 @@ static int apple_dart_remove(struct platform_device *pdev)
 
 	apple_dart_hw_reset(dart);
 	free_irq(dart->irq, dart);
-	apple_dart_set_bus_ops(NULL);
 
 	iommu_device_unregister(&dart->iommu);
 	iommu_device_sysfs_remove(&dart->iommu);
-- 
2.35.3.dirty

[PATCH v2 08/14] iommu/exynos: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the init failure path accordingly.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/exynos-iommu.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 71f2018e23fe..359b255b3924 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1356,16 +1356,7 @@ static int __init exynos_iommu_init(void)
 		goto err_zero_lv2;
 	}
 
-	ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
-	if (ret) {
-		pr_err("%s: Failed to register exynos-iommu driver.\n",
-								__func__);
-		goto err_set_iommu;
-	}
-
 	return 0;
-err_set_iommu:
-	kmem_cache_free(lv2table_kmem_cache, zero_lv2_table);
 err_zero_lv2:
 	platform_driver_unregister(&exynos_sysmmu_driver);
 err_reg_driver:
-- 
2.35.3.dirty

[PATCH v2 09/14] iommu/ipmmu-vmsa: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary. This also
leaves the custom initcall effectively doing nothing but register
the driver, which no longer needs to happen early either, so convert
it to builtin_platform_driver().

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/ipmmu-vmsa.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 8fdb84b3642b..2549d32f0ddd 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device *pdev)
 		ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
 		if (ret)
 			return ret;
-
-#if defined(CONFIG_IOMMU_DMA)
-		if (!iommu_present(&platform_bus_type))
-			bus_set_iommu(&platform_bus_type, &ipmmu_ops);
-#endif
 	}
 
 	/*
@@ -1168,32 +1163,4 @@ static struct platform_driver ipmmu_driver = {
 	.probe = ipmmu_probe,
 	.remove	= ipmmu_remove,
 };
-
-static int __init ipmmu_init(void)
-{
-	struct device_node *np;
-	static bool setup_done;
-	int ret;
-
-	if (setup_done)
-		return 0;
-
-	np = of_find_matching_node(NULL, ipmmu_of_ids);
-	if (!np)
-		return 0;
-
-	of_node_put(np);
-
-	ret = platform_driver_register(&ipmmu_driver);
-	if (ret < 0)
-		return ret;
-
-#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
-	if (!iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
-#endif
-
-	setup_done = true;
-	return 0;
-}
-subsys_initcall(ipmmu_init);
+builtin_platform_driver(ipmmu_driver);
-- 
2.35.3.dirty

[PATCH v2 10/14] iommu/mtk: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure paths accordingly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/mtk_iommu.c    | 13 +------------
 drivers/iommu/mtk_iommu_v1.c | 13 +------------
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6fd75a60abd6..4278d9e032ad 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -920,19 +920,11 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	spin_lock_init(&data->tlb_lock);
 	list_add_tail(&data->list, &m4ulist);
 
-	if (!iommu_present(&platform_bus_type)) {
-		ret = bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);
-		if (ret)
-			goto out_list_del;
-	}
-
 	ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match);
 	if (ret)
-		goto out_bus_set_null;
+		goto out_list_del;
 	return ret;
 
-out_bus_set_null:
-	bus_set_iommu(&platform_bus_type, NULL);
 out_list_del:
 	list_del(&data->list);
 	iommu_device_unregister(&data->iommu);
@@ -952,9 +944,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)
 	iommu_device_sysfs_remove(&data->iommu);
 	iommu_device_unregister(&data->iommu);
 
-	if (iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, NULL);
-
 	clk_disable_unprepare(data->bclk);
 	device_link_remove(data->smicomm_dev, &pdev->dev);
 	pm_runtime_disable(&pdev->dev);
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index ecff800656e6..7d17d6a21803 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -660,19 +660,11 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_sysfs_remove;
 
-	if (!iommu_present(&platform_bus_type)) {
-		ret = bus_set_iommu(&platform_bus_type,  &mtk_iommu_ops);
-		if (ret)
-			goto out_dev_unreg;
-	}
-
 	ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match);
 	if (ret)
-		goto out_bus_set_null;
+		goto out_dev_unreg;
 	return ret;
 
-out_bus_set_null:
-	bus_set_iommu(&platform_bus_type, NULL);
 out_dev_unreg:
 	iommu_device_unregister(&data->iommu);
 out_sysfs_remove:
@@ -687,9 +679,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)
 	iommu_device_sysfs_remove(&data->iommu);
 	iommu_device_unregister(&data->iommu);
 
-	if (iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, NULL);
-
 	clk_disable_unprepare(data->bclk);
 	devm_free_irq(&pdev->dev, data->irq, data);
 	component_master_del(&pdev->dev, &mtk_iommu_com_ops);
-- 
2.35.3.dirty

[PATCH v2 11/14] iommu/omap: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the init failure path accordingly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/omap-iommu.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..07ee2600113c 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1776,14 +1776,8 @@ static int __init omap_iommu_init(void)
 		goto fail_driver;
 	}
 
-	ret = bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
-	if (ret)
-		goto fail_bus;
-
 	return 0;
 
-fail_bus:
-	platform_driver_unregister(&omap_iommu_driver);
 fail_driver:
 	kmem_cache_destroy(iopte_cachep);
 	return ret;
-- 
2.35.3.dirty

[PATCH v2 12/14] iommu/tegra-smmu: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/tegra-smmu.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1fea68e551f1..2e4d2e4c65bb 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1086,8 +1086,8 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 
 	/*
 	 * This is a bit of a hack. Ideally we'd want to simply return this
-	 * value. However the IOMMU registration process will attempt to add
-	 * all devices to the IOMMU when bus_set_iommu() is called. In order
+	 * value. However iommu_device_register() will attempt to add
+	 * all devices to the IOMMU before we get that far. In order
 	 * not to rely on global variables to track the IOMMU instance, we
 	 * set it here so that it can be looked up from the .probe_device()
 	 * callback via the IOMMU device's .drvdata field.
@@ -1141,32 +1141,15 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 		return ERR_PTR(err);
 
 	err = iommu_device_register(&smmu->iommu, &tegra_smmu_ops, dev);
-	if (err)
-		goto remove_sysfs;
-
-	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0)
-		goto unregister;
-
-#ifdef CONFIG_PCI
-	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
-	if (err < 0)
-		goto unset_platform_bus;
-#endif
+	if (err) {
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return ERR_PTR(err);
+	}
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 
 	return smmu;
-
-unset_platform_bus: __maybe_unused;
-	bus_set_iommu(&platform_bus_type, NULL);
-unregister:
-	iommu_device_unregister(&smmu->iommu);
-remove_sysfs:
-	iommu_device_sysfs_remove(&smmu->iommu);
-
-	return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.35.3.dirty

[PATCH v2 13/14] iommu/virtio: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/virtio-iommu.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 25be4b822aa0..bcbd10ec4ccb 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -7,7 +7,6 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/amba/bus.h>
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
 #include <linux/dma-map-ops.h>
@@ -17,7 +16,6 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
-#include <linux/platform_device.h>
 #include <linux/virtio.h>
 #include <linux/virtio_config.h>
 #include <linux/virtio_ids.h>
@@ -1146,26 +1144,6 @@ static int viommu_probe(struct virtio_device *vdev)
 
 	iommu_device_register(&viommu->iommu, &viommu_ops, parent_dev);
 
-#ifdef CONFIG_PCI
-	if (pci_bus_type.iommu_ops != &viommu_ops) {
-		ret = bus_set_iommu(&pci_bus_type, &viommu_ops);
-		if (ret)
-			goto err_unregister;
-	}
-#endif
-#ifdef CONFIG_ARM_AMBA
-	if (amba_bustype.iommu_ops != &viommu_ops) {
-		ret = bus_set_iommu(&amba_bustype, &viommu_ops);
-		if (ret)
-			goto err_unregister;
-	}
-#endif
-	if (platform_bus_type.iommu_ops != &viommu_ops) {
-		ret = bus_set_iommu(&platform_bus_type, &viommu_ops);
-		if (ret)
-			goto err_unregister;
-	}
-
 	vdev->priv = viommu;
 
 	dev_info(dev, "input address: %u bits\n",
@@ -1174,9 +1152,6 @@ static int viommu_probe(struct virtio_device *vdev)
 
 	return 0;
 
-err_unregister:
-	iommu_device_sysfs_remove(&viommu->iommu);
-	iommu_device_unregister(&viommu->iommu);
 err_free_vqs:
 	vdev->config->del_vqs(vdev);
 
-- 
2.35.3.dirty

[PATCH v2 14/14] iommu: Clean up bus_set_iommu()

[Robin Murphy]

Clean up the remaining trivial bus_set_iommu() callsites along
with the implementation. Now drivers only have to know and care
about iommu_device instances, phew!

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c |  4 ----
 drivers/iommu/fsl_pamu_domain.c         |  4 ----
 drivers/iommu/intel/iommu.c             |  1 -
 drivers/iommu/iommu.c                   | 24 ------------------------
 drivers/iommu/msm_iommu.c               |  2 --
 drivers/iommu/rockchip-iommu.c          |  2 --
 drivers/iommu/s390-iommu.c              |  6 ------
 drivers/iommu/sprd-iommu.c              |  5 -----
 drivers/iommu/sun50i-iommu.c            |  2 --
 include/linux/iommu.h                   |  1 -
 10 files changed, 51 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4c077c38fbd6..80af00f468b4 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -845,8 +845,6 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 		goto err_pm_disable;
 	}
 
-	bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
-
 	if (qcom_iommu->local_base) {
 		pm_runtime_get_sync(dev);
 		writel_relaxed(0xffffffff, qcom_iommu->local_base + SMMU_INTR_SEL_NS);
@@ -864,8 +862,6 @@ static int qcom_iommu_device_remove(struct platform_device *pdev)
 {
 	struct qcom_iommu_dev *qcom_iommu = platform_get_drvdata(pdev);
 
-	bus_set_iommu(&platform_bus_type, NULL);
-
 	pm_runtime_force_suspend(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
 	iommu_device_sysfs_remove(&qcom_iommu->iommu);
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 69a4a62dc3b9..7274f86b2bc4 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -480,11 +480,7 @@ int __init pamu_domain_init(void)
 	if (ret) {
 		iommu_device_sysfs_remove(&pamu_iommu);
 		pr_err("Can't register iommu device\n");
-		return ret;
 	}
 
-	bus_set_iommu(&platform_bus_type, &fsl_pamu_ops);
-	bus_set_iommu(&pci_bus_type, &fsl_pamu_ops);
-
 	return ret;
 }
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9507b64fdf6b..e0a31fa6a70c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4178,7 +4178,6 @@ int __init intel_iommu_init(void)
 	}
 	up_read(&dmar_global_lock);
 
-	bus_set_iommu(&pci_bus_type, &intel_iommu_ops);
 	if (si_domain && !hw_pass_through)
 		register_memory_notifier(&intel_iommu_memory_nb);
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c89af4dc54c2..5f10e7ad04b0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1887,30 +1887,6 @@ static int iommu_bus_init(struct bus_type *bus)
 	return err;
 }
 
-/**
- * bus_set_iommu - set iommu-callbacks for the bus
- * @bus: bus.
- * @ops: the callbacks provided by the iommu-driver
- *
- * This function is called by an iommu driver to set the iommu methods
- * used for a particular bus. Drivers for devices on that bus can use
- * the iommu-api after these ops are registered.
- * This special function is needed because IOMMUs are usually devices on
- * the bus itself, so the iommu drivers are not initialized when the bus
- * is set up. With this function the iommu-driver can set the iommu-ops
- * afterwards.
- */
-int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
-{
-	if (bus->iommu_ops && ops && bus->iommu_ops != ops)
-		return -EBUSY;
-
-	bus->iommu_ops = ops;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(bus_set_iommu);
-
 bool iommu_present(struct bus_type *bus)
 {
 	return bus->iommu_ops != NULL;
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 50f57624610f..5b89fb16feb8 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -789,8 +789,6 @@ static int msm_iommu_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	bus_set_iommu(&platform_bus_type, &msm_iommu_ops);
-
 	pr_info("device mapped at %p, irq %d with %d ctx banks\n",
 		iommu->base, iommu->irq, iommu->ncb);
 
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index ab57c4b8fade..a3fc59b814ab 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1300,8 +1300,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (!dma_dev)
 		dma_dev = &pdev->dev;
 
-	bus_set_iommu(&platform_bus_type, &rk_iommu_ops);
-
 	pm_runtime_enable(dev);
 
 	for (i = 0; i < iommu->num_irq; i++) {
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 3833e86c6e7b..5f5f4bd91e6f 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -376,9 +376,3 @@ static const struct iommu_ops s390_iommu_ops = {
 		.free		= s390_domain_free,
 	}
 };
-
-static int __init s390_iommu_init(void)
-{
-	return bus_set_iommu(&pci_bus_type, &s390_iommu_ops);
-}
-subsys_initcall(s390_iommu_init);
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index bd409bab6286..6770e6a72283 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -507,9 +507,6 @@ static int sprd_iommu_probe(struct platform_device *pdev)
 	if (ret)
 		goto remove_sysfs;
 
-	if (!iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, &sprd_iommu_ops);
-
 	ret = sprd_iommu_clk_enable(sdev);
 	if (ret)
 		goto unregister_iommu;
@@ -545,8 +542,6 @@ static int sprd_iommu_remove(struct platform_device *pdev)
 	iommu_group_put(sdev->group);
 	sdev->group = NULL;
 
-	bus_set_iommu(&platform_bus_type, NULL);
-
 	platform_set_drvdata(pdev, NULL);
 	iommu_device_sysfs_remove(&sdev->iommu);
 	iommu_device_unregister(&sdev->iommu);
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index c54ab477b8fd..e104543b78d9 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -968,8 +968,6 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_unregister;
 
-	bus_set_iommu(&platform_bus_type, &sun50i_iommu_ops);
-
 	return 0;
 
 err_unregister:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4123693ae319..17be860a3194 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -416,7 +416,6 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 #define IOMMU_GROUP_NOTIFY_UNBIND_DRIVER	5 /* Pre Driver unbind */
 #define IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER	6 /* Post Driver unbind */
 
-extern int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops);
 extern int bus_iommu_probe(struct bus_type *bus);
 extern bool iommu_present(struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
-- 
2.35.3.dirty

Re: [PATCH v2 00/14] iommu: Retire bus_set_iommu()

[Robin Murphy]

On 2022-04-28 14:18, Robin Murphy wrote:
> v1: https://lore.kernel.org/linux-iommu/cover.1649935679.git.robin.murphy@arm.com/
> 
> Hi all,
> 
> Just some minor updates for v2, adding a workaround to avoid changing
> VT-d behaviour for now, cleaning up the extra include I missed in
> virtio-iommu, and collecting all the acks so far.

Oh, and I added the proper error handling for bus_iommu_probe() since it 
turned out to be trivial, and applied Krishna's R-b to the wrong patch 
because of course what was patch #2 is now actually patch #3 :(

Apologies, juggling one too many things at the moment...

Robin.

> As before, this is
> based on the AMD IOMMU patch now applied, plus v2 of my
> device_iommu_capable() series here:
> 
> https://lore.kernel.org/linux-iommu/cover.1650878781.git.robin.murphy@arm.com/
> 
> Cheers,
> Robin.
> 
> 
> 
> Robin Murphy (14):
>    iommu/vt-d: Temporarily reject probing non-PCI devices
>    iommu: Always register bus notifiers
>    iommu: Move bus setup to IOMMU device registration
>    iommu/amd: Clean up bus_set_iommu()
>    iommu/arm-smmu: Clean up bus_set_iommu()
>    iommu/arm-smmu-v3: Clean up bus_set_iommu()
>    iommu/dart: Clean up bus_set_iommu()
>    iommu/exynos: Clean up bus_set_iommu()
>    iommu/ipmmu-vmsa: Clean up bus_set_iommu()
>    iommu/mtk: Clean up bus_set_iommu()
>    iommu/omap: Clean up bus_set_iommu()
>    iommu/tegra-smmu: Clean up bus_set_iommu()
>    iommu/virtio: Clean up bus_set_iommu()
>    iommu: Clean up bus_set_iommu()
> 
>   drivers/iommu/amd/amd_iommu.h               |   1 -
>   drivers/iommu/amd/init.c                    |   9 +-
>   drivers/iommu/amd/iommu.c                   |  21 ----
>   drivers/iommu/apple-dart.c                  |  30 +-----
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  53 +---------
>   drivers/iommu/arm/arm-smmu/arm-smmu.c       |  84 +--------------
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c     |   4 -
>   drivers/iommu/exynos-iommu.c                |   9 --
>   drivers/iommu/fsl_pamu_domain.c             |   4 -
>   drivers/iommu/intel/iommu.c                 |   5 +-
>   drivers/iommu/iommu.c                       | 110 +++++++++-----------
>   drivers/iommu/ipmmu-vmsa.c                  |  35 +------
>   drivers/iommu/msm_iommu.c                   |   2 -
>   drivers/iommu/mtk_iommu.c                   |  13 +--
>   drivers/iommu/mtk_iommu_v1.c                |  13 +--
>   drivers/iommu/omap-iommu.c                  |   6 --
>   drivers/iommu/rockchip-iommu.c              |   2 -
>   drivers/iommu/s390-iommu.c                  |   6 --
>   drivers/iommu/sprd-iommu.c                  |   5 -
>   drivers/iommu/sun50i-iommu.c                |   2 -
>   drivers/iommu/tegra-smmu.c                  |  29 ++----
>   drivers/iommu/virtio-iommu.c                |  25 -----
>   include/linux/iommu.h                       |   1 -
>   23 files changed, 67 insertions(+), 402 deletions(-)
> 

Re: [PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

Hi Robin,

On 2022/4/28 21:18, Robin Murphy wrote:
> Move the bus setup to iommu_device_register(). This should allow
> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.

I re-fetched the latest patches on

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

and rolled back the head to "iommu: Cleanup bus_set_iommu".

The test machine still hangs during boot.

I went through the code. It seems that the .probe_device for Intel IOMMU
driver can't handle the probe replay well. It always assumes that the
device has never been probed.

Best regards,
baolu

> 
> At this point we can also handle cleanup better than just rolling back
> the most-recently-touched bus upon failure - which may release devices
> owned by other already-registered instances, and still leave devices on
> other buses with dangling pointers to the failed instance. Now it's easy
> to clean up the exact footprint of a given instance, no more, no less.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/iommu.c | 51 +++++++++++++++++++++++--------------------
>   1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6c4621afc8cf..c89af4dc54c2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -175,6 +175,14 @@ static int __init iommu_subsys_init(void)
>   }
>   subsys_initcall(iommu_subsys_init);
>   
> +static int remove_iommu_group(struct device *dev, void *data)
> +{
> +	if (dev->iommu && dev->iommu->iommu_dev == data)
> +		iommu_release_device(dev);
> +
> +	return 0;
> +}
> +
>   /**
>    * iommu_device_register() - Register an IOMMU hardware instance
>    * @iommu: IOMMU handle for the instance
> @@ -197,12 +205,29 @@ int iommu_device_register(struct iommu_device *iommu,
>   	spin_lock(&iommu_device_lock);
>   	list_add_tail(&iommu->list, &iommu_device_list);
>   	spin_unlock(&iommu_device_lock);
> +
> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> +		struct bus_type *bus = iommu_buses[i];
> +		int err;
> +
> +		WARN_ON(bus->iommu_ops && bus->iommu_ops != ops);
> +		bus->iommu_ops = ops;
> +		err = bus_iommu_probe(bus);
> +		if (err) {
> +			iommu_device_unregister(iommu);
> +			return err;
> +		}
> +	}
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(iommu_device_register);
>   
>   void iommu_device_unregister(struct iommu_device *iommu)
>   {
> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
> +		bus_for_each_dev(iommu_buses[i], NULL, iommu, remove_iommu_group);
> +
>   	spin_lock(&iommu_device_lock);
>   	list_del(&iommu->list);
>   	spin_unlock(&iommu_device_lock);
> @@ -1655,13 +1680,6 @@ static int probe_iommu_group(struct device *dev, void *data)
>   	return ret;
>   }
>   
> -static int remove_iommu_group(struct device *dev, void *data)
> -{
> -	iommu_release_device(dev);
> -
> -	return 0;
> -}
> -
>   static int iommu_bus_notifier(struct notifier_block *nb,
>   			      unsigned long action, void *data)
>   {
> @@ -1884,27 +1902,12 @@ static int iommu_bus_init(struct bus_type *bus)
>    */
>   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
>   {
> -	int err;
> -
> -	if (ops == NULL) {
> -		bus->iommu_ops = NULL;
> -		return 0;
> -	}
> -
> -	if (bus->iommu_ops != NULL)
> +	if (bus->iommu_ops && ops && bus->iommu_ops != ops)
>   		return -EBUSY;
>   
>   	bus->iommu_ops = ops;
>   
> -	/* Do IOMMU specific setup for this bus-type */
> -	err = bus_iommu_probe(bus);
> -	if (err) {
> -		/* Clean up */
> -		bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
> -		bus->iommu_ops = NULL;
> -	}
> -
> -	return err;
> +	return 0;
>   }
>   EXPORT_SYMBOL_GPL(bus_set_iommu);
>   

Re: [PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-04-29 07:57, Baolu Lu wrote:
> Hi Robin,
> 
> On 2022/4/28 21:18, Robin Murphy wrote:
>> Move the bus setup to iommu_device_register(). This should allow
>> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
> 
> I re-fetched the latest patches on
> 
> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus
> 
> and rolled back the head to "iommu: Cleanup bus_set_iommu".
> 
> The test machine still hangs during boot.
> 
> I went through the code. It seems that the .probe_device for Intel IOMMU
> driver can't handle the probe replay well. It always assumes that the
> device has never been probed.

Hmm, but probe_iommu_group() is supposed to prevent the 
__iommu_probe_device() call even happening if the device *has* already 
been probed before :/

I've still got an old Intel box spare in the office so I'll rig that up 
and see if I can see what might be going on here...

Thanks,
Robin.

> 
> Best regards,
> baolu
> 
>>
>> At this point we can also handle cleanup better than just rolling back
>> the most-recently-touched bus upon failure - which may release devices
>> owned by other already-registered instances, and still leave devices on
>> other buses with dangling pointers to the failed instance. Now it's easy
>> to clean up the exact footprint of a given instance, no more, no less.
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/iommu.c | 51 +++++++++++++++++++++++--------------------
>>   1 file changed, 27 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 6c4621afc8cf..c89af4dc54c2 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -175,6 +175,14 @@ static int __init iommu_subsys_init(void)
>>   }
>>   subsys_initcall(iommu_subsys_init);
>> +static int remove_iommu_group(struct device *dev, void *data)
>> +{
>> +    if (dev->iommu && dev->iommu->iommu_dev == data)
>> +        iommu_release_device(dev);
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    * iommu_device_register() - Register an IOMMU hardware instance
>>    * @iommu: IOMMU handle for the instance
>> @@ -197,12 +205,29 @@ int iommu_device_register(struct iommu_device 
>> *iommu,
>>       spin_lock(&iommu_device_lock);
>>       list_add_tail(&iommu->list, &iommu_device_list);
>>       spin_unlock(&iommu_device_lock);
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +        struct bus_type *bus = iommu_buses[i];
>> +        int err;
>> +
>> +        WARN_ON(bus->iommu_ops && bus->iommu_ops != ops);
>> +        bus->iommu_ops = ops;
>> +        err = bus_iommu_probe(bus);
>> +        if (err) {
>> +            iommu_device_unregister(iommu);
>> +            return err;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_device_register);
>>   void iommu_device_unregister(struct iommu_device *iommu)
>>   {
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
>> +        bus_for_each_dev(iommu_buses[i], NULL, iommu, 
>> remove_iommu_group);
>> +
>>       spin_lock(&iommu_device_lock);
>>       list_del(&iommu->list);
>>       spin_unlock(&iommu_device_lock);
>> @@ -1655,13 +1680,6 @@ static int probe_iommu_group(struct device 
>> *dev, void *data)
>>       return ret;
>>   }
>> -static int remove_iommu_group(struct device *dev, void *data)
>> -{
>> -    iommu_release_device(dev);
>> -
>> -    return 0;
>> -}
>> -
>>   static int iommu_bus_notifier(struct notifier_block *nb,
>>                     unsigned long action, void *data)
>>   {
>> @@ -1884,27 +1902,12 @@ static int iommu_bus_init(struct bus_type *bus)
>>    */
>>   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
>>   {
>> -    int err;
>> -
>> -    if (ops == NULL) {
>> -        bus->iommu_ops = NULL;
>> -        return 0;
>> -    }
>> -
>> -    if (bus->iommu_ops != NULL)
>> +    if (bus->iommu_ops && ops && bus->iommu_ops != ops)
>>           return -EBUSY;
>>       bus->iommu_ops = ops;
>> -    /* Do IOMMU specific setup for this bus-type */
>> -    err = bus_iommu_probe(bus);
>> -    if (err) {
>> -        /* Clean up */
>> -        bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
>> -        bus->iommu_ops = NULL;
>> -    }
>> -
>> -    return err;
>> +    return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(bus_set_iommu);
> 

Re: [PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 29/04/2022 9:50 am, Robin Murphy wrote:
> On 2022-04-29 07:57, Baolu Lu wrote:
>> Hi Robin,
>>
>> On 2022/4/28 21:18, Robin Murphy wrote:
>>> Move the bus setup to iommu_device_register(). This should allow
>>> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
>>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>
>> I re-fetched the latest patches on
>>
>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus
>>
>> and rolled back the head to "iommu: Cleanup bus_set_iommu".
>>
>> The test machine still hangs during boot.
>>
>> I went through the code. It seems that the .probe_device for Intel IOMMU
>> driver can't handle the probe replay well. It always assumes that the
>> device has never been probed.
> 
> Hmm, but probe_iommu_group() is supposed to prevent the 
> __iommu_probe_device() call even happening if the device *has* already 
> been probed before :/
> 
> I've still got an old Intel box spare in the office so I'll rig that up 
> and see if I can see what might be going on here...

OK, on a Xeon with two DMAR units, this seems to boot OK with or without 
patch #1, so it doesn't seem to be a general problem with replaying in 
iommu_device_register(), or with platform devices. Not sure where to go 
from here... :/

Cheers,
Robin.

Re: [PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

Hi Robin,

On 2022/4/30 02:06, Robin Murphy wrote:
> On 29/04/2022 9:50 am, Robin Murphy wrote:
>> On 2022-04-29 07:57, Baolu Lu wrote:
>>> Hi Robin,
>>>
>>> On 2022/4/28 21:18, Robin Murphy wrote:
>>>> Move the bus setup to iommu_device_register(). This should allow
>>>> bus_iommu_probe() to be correctly replayed for multiple IOMMU 
>>>> instances,
>>>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>>
>>> I re-fetched the latest patches on
>>>
>>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus
>>>
>>> and rolled back the head to "iommu: Cleanup bus_set_iommu".
>>>
>>> The test machine still hangs during boot.
>>>
>>> I went through the code. It seems that the .probe_device for Intel IOMMU
>>> driver can't handle the probe replay well. It always assumes that the
>>> device has never been probed.
>>
>> Hmm, but probe_iommu_group() is supposed to prevent the 
>> __iommu_probe_device() call even happening if the device *has* already 
>> been probed before :/
>>
>> I've still got an old Intel box spare in the office so I'll rig that 
>> up and see if I can see what might be going on here...
> 
> OK, on a Xeon with two DMAR units, this seems to boot OK with or without 
> patch #1, so it doesn't seem to be a general problem with replaying in 
> iommu_device_register(), or with platform devices. Not sure where to go 
> from here... :/

The kernel boot panic message:

[    6.639432] BUG: kernel NULL pointer dereference, address: 
0000000000000028
[    6.743829] #PF: supervisor read access in kernel mode
[    6.743829] #PF: error_code(0x0000) - not-present page
[    6.743829] PGD 0
[    6.743829] Oops: 0000 [#1] PREEMPT SMP NOPTI
[    6.743829] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G          I 
  5.19.0-rc3+ #182
[    6.743829] RIP: 0010:__iommu_probe_device+0x115/0x200
[    6.743829] Code: 89 ff e8 3e e1 ff ff 48 85 c0 0f 85 47 ff ff ff 41 
bd f4 ff ff ff eb 83 48 8b 83 d8 02 00 00 48 89 df 48 8b 40 38 48 8b 40 
10 <48> 8b 40 28 ff d0 0f 1f 00 48 89 c1 48 85 c0 0f 84 b7 00 00 00 48
[    6.743829] RSP: 0000:ff30605c00063d40 EFLAGS: 00010246
[    6.743829] RAX: 0000000000000000 RBX: ff128b9c5fdc90d0 RCX: 
0000000000000000
[    6.743829] RDX: 0000000080000001 RSI: 0000000000000246 RDI: 
ff128b9c5fdc90d0
[    6.743829] RBP: ffffffffb60c34e0 R08: ffffffffb68664d0 R09: 
ff128b9501d4ce40
[    6.743829] R10: ffffffffb6267096 R11: ff128b950014c267 R12: 
ff30605c00063de0
[    6.743829] R13: 00000000001b9d28 R14: ff128b95001b9d28 R15: 
ff128b9c5fdc93a8
[    6.743829] FS:  0000000000000000(0000) GS:ff128b9c5fc00000(0000) 
knlGS:0000000000000000
[    6.743829] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    6.743829] CR2: 0000000000000028 CR3: 0000000149210001 CR4: 
0000000000771ef0
[    6.743829] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[    6.743829] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 
0000000000000400
[    6.743829] PKRU: 55555554
[    6.743829] Call Trace:
[    6.743829]  <TASK>
[    6.743829]  ? _raw_spin_lock_irqsave+0x17/0x40
[    6.743829]  ? __iommu_probe_device+0x200/0x200
[    6.743829]  probe_iommu_group+0x2d/0x40
[    6.743829]  bus_for_each_dev+0x74/0xc0
[    6.743829]  bus_iommu_probe+0x48/0x2d0
[    6.743829]  iommu_device_register+0xde/0x120
[    6.743829]  intel_iommu_init+0x35f/0x5f2
[    6.743829]  ? iommu_setup+0x27d/0x27d
[    6.743829]  ? rdinit_setup+0x2c/0x2c
[    6.743829]  pci_iommu_init+0xe/0x32
[    6.743829]  do_one_initcall+0x41/0x200
[    6.743829]  kernel_init_freeable+0x1de/0x228
[    6.743829]  ? rest_init+0xc0/0xc0
[    6.743829]  kernel_init+0x16/0x120
[    6.743829]  ret_from_fork+0x1f/0x30
[    6.743829]  </TASK>
[    6.743829] Modules linked in:
[    6.743829] CR2: 0000000000000028
[    6.743829] ---[ end trace 0000000000000000 ]---
[    6.743829] RIP: 0010:__iommu_probe_device+0x115/0x200
[    6.743829] Code: 89 ff e8 3e e1 ff ff 48 85 c0 0f 85 47 ff ff ff 41 
bd f4 ff ff ff eb 83 48 8b 83 d8 02 00 00 48 89 df 48 8b 40 38 48 8b 40 
10 <48> 8b 40 28 ff d0 0f 1f 00 48 89 c1 48 85 c0 0f 84 b7 00 00 00 48
[    6.743829] RSP: 0000:ff30605c00063d40 EFLAGS: 00010246
[    6.743829] RAX: 0000000000000000 RBX: ff128b9c5fdc90d0 RCX: 
0000000000000000
[    6.743829] RDX: 0000000080000001 RSI: 0000000000000246 RDI: 
ff128b9c5fdc90d0
[    6.743829] RBP: ffffffffb60c34e0 R08: ffffffffb68664d0 R09: 
ff128b9501d4ce40
[    6.743829] R10: ffffffffb6267096 R11: ff128b950014c267 R12: 
ff30605c00063de0
[    6.743829] R13: 00000000001b9d28 R14: ff128b95001b9d28 R15: 
ff128b9c5fdc93a8
[    6.743829] FS:  0000000000000000(0000) GS:ff128b9c5fc00000(0000) 
knlGS:0000000000000000
[    6.743829] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    6.743829] CR2: 0000000000000028 CR3: 0000000149210001 CR4: 
0000000000771ef0
[    6.743829] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[    6.743829] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 
0000000000000400
[    6.743829] PKRU: 55555554
[    6.743829] Kernel panic - not syncing: Fatal exception
[    6.743829] ---[ end Kernel panic - not syncing: Fatal exception ]---

The "NULL pointer dereference" happens at line 1620 of below code.

1610 static struct iommu_group *iommu_group_get_for_dev(struct device *dev)
1611 {
1612         const struct iommu_ops *ops = dev_iommu_ops(dev);
1613         struct iommu_group *group;
1614         int ret;
1615
1616         group = iommu_group_get(dev);
1617         if (group)
1618                 return group;
1619
1620         group = ops->device_group(dev);
1621         if (WARN_ON_ONCE(group == NULL))
1622                 return ERR_PTR(-EINVAL);
1623
1624         if (IS_ERR(group))
1625                 return group;

This platform has multiple IOMMU units, each serving different PCI
devices. The ops field of each iommu_device is initialized in
iommu_device_register(), hence when the first IOMMU device gets
registered, ops fields of other iommu_devices are NULL.

Unfortunately bus_iommu_probe() called in iommu_device_register() probes
*all* PCI devices. This probably leads to above NULL pointer dereference
issue.

Please correct me if I overlooked anything.

Best regards,
baolu

Re: [PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-07-05 05:51, Baolu Lu wrote:
> Hi Robin,
> 
> On 2022/4/30 02:06, Robin Murphy wrote:
>> On 29/04/2022 9:50 am, Robin Murphy wrote:
>>> On 2022-04-29 07:57, Baolu Lu wrote:
>>>> Hi Robin,
>>>>
>>>> On 2022/4/28 21:18, Robin Murphy wrote:
>>>>> Move the bus setup to iommu_device_register(). This should allow
>>>>> bus_iommu_probe() to be correctly replayed for multiple IOMMU 
>>>>> instances,
>>>>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>>>
>>>> I re-fetched the latest patches on
>>>>
>>>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus
>>>>
>>>> and rolled back the head to "iommu: Cleanup bus_set_iommu".
>>>>
>>>> The test machine still hangs during boot.
>>>>
>>>> I went through the code. It seems that the .probe_device for Intel 
>>>> IOMMU
>>>> driver can't handle the probe replay well. It always assumes that the
>>>> device has never been probed.
>>>
>>> Hmm, but probe_iommu_group() is supposed to prevent the 
>>> __iommu_probe_device() call even happening if the device *has* 
>>> already been probed before :/
>>>
>>> I've still got an old Intel box spare in the office so I'll rig that 
>>> up and see if I can see what might be going on here...
>>
>> OK, on a Xeon with two DMAR units, this seems to boot OK with or 
>> without patch #1, so it doesn't seem to be a general problem with 
>> replaying in iommu_device_register(), or with platform devices. Not 
>> sure where to go from here... :/
> 
> The kernel boot panic message:
> 
> [    6.639432] BUG: kernel NULL pointer dereference, address: 
> 0000000000000028
> [    6.743829] #PF: supervisor read access in kernel mode
> [    6.743829] #PF: error_code(0x0000) - not-present page
> [    6.743829] PGD 0
> [    6.743829] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [    6.743829] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G          I 
>   5.19.0-rc3+ #182
> [    6.743829] RIP: 0010:__iommu_probe_device+0x115/0x200
> [    6.743829] Code: 89 ff e8 3e e1 ff ff 48 85 c0 0f 85 47 ff ff ff 41 
> bd f4 ff ff ff eb 83 48 8b 83 d8 02 00 00 48 89 df 48 8b 40 38 48 8b 40 
> 10 <48> 8b 40 28 ff d0 0f 1f 00 48 89 c1 48 85 c0 0f 84 b7 00 00 00 48
> [    6.743829] RSP: 0000:ff30605c00063d40 EFLAGS: 00010246
> [    6.743829] RAX: 0000000000000000 RBX: ff128b9c5fdc90d0 RCX: 
> 0000000000000000
> [    6.743829] RDX: 0000000080000001 RSI: 0000000000000246 RDI: 
> ff128b9c5fdc90d0
> [    6.743829] RBP: ffffffffb60c34e0 R08: ffffffffb68664d0 R09: 
> ff128b9501d4ce40
> [    6.743829] R10: ffffffffb6267096 R11: ff128b950014c267 R12: 
> ff30605c00063de0
> [    6.743829] R13: 00000000001b9d28 R14: ff128b95001b9d28 R15: 
> ff128b9c5fdc93a8
> [    6.743829] FS:  0000000000000000(0000) GS:ff128b9c5fc00000(0000) 
> knlGS:0000000000000000
> [    6.743829] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    6.743829] CR2: 0000000000000028 CR3: 0000000149210001 CR4: 
> 0000000000771ef0
> [    6.743829] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
> 0000000000000000
> [    6.743829] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 
> 0000000000000400
> [    6.743829] PKRU: 55555554
> [    6.743829] Call Trace:
> [    6.743829]  <TASK>
> [    6.743829]  ? _raw_spin_lock_irqsave+0x17/0x40
> [    6.743829]  ? __iommu_probe_device+0x200/0x200
> [    6.743829]  probe_iommu_group+0x2d/0x40
> [    6.743829]  bus_for_each_dev+0x74/0xc0
> [    6.743829]  bus_iommu_probe+0x48/0x2d0
> [    6.743829]  iommu_device_register+0xde/0x120
> [    6.743829]  intel_iommu_init+0x35f/0x5f2
> [    6.743829]  ? iommu_setup+0x27d/0x27d
> [    6.743829]  ? rdinit_setup+0x2c/0x2c
> [    6.743829]  pci_iommu_init+0xe/0x32
> [    6.743829]  do_one_initcall+0x41/0x200
> [    6.743829]  kernel_init_freeable+0x1de/0x228
> [    6.743829]  ? rest_init+0xc0/0xc0
> [    6.743829]  kernel_init+0x16/0x120
> [    6.743829]  ret_from_fork+0x1f/0x30
> [    6.743829]  </TASK>
> [    6.743829] Modules linked in:
> [    6.743829] CR2: 0000000000000028
> [    6.743829] ---[ end trace 0000000000000000 ]---
> [    6.743829] RIP: 0010:__iommu_probe_device+0x115/0x200
> [    6.743829] Code: 89 ff e8 3e e1 ff ff 48 85 c0 0f 85 47 ff ff ff 41 
> bd f4 ff ff ff eb 83 48 8b 83 d8 02 00 00 48 89 df 48 8b 40 38 48 8b 40 
> 10 <48> 8b 40 28 ff d0 0f 1f 00 48 89 c1 48 85 c0 0f 84 b7 00 00 00 48
> [    6.743829] RSP: 0000:ff30605c00063d40 EFLAGS: 00010246
> [    6.743829] RAX: 0000000000000000 RBX: ff128b9c5fdc90d0 RCX: 
> 0000000000000000
> [    6.743829] RDX: 0000000080000001 RSI: 0000000000000246 RDI: 
> ff128b9c5fdc90d0
> [    6.743829] RBP: ffffffffb60c34e0 R08: ffffffffb68664d0 R09: 
> ff128b9501d4ce40
> [    6.743829] R10: ffffffffb6267096 R11: ff128b950014c267 R12: 
> ff30605c00063de0
> [    6.743829] R13: 00000000001b9d28 R14: ff128b95001b9d28 R15: 
> ff128b9c5fdc93a8
> [    6.743829] FS:  0000000000000000(0000) GS:ff128b9c5fc00000(0000) 
> knlGS:0000000000000000
> [    6.743829] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    6.743829] CR2: 0000000000000028 CR3: 0000000149210001 CR4: 
> 0000000000771ef0
> [    6.743829] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
> 0000000000000000
> [    6.743829] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 
> 0000000000000400
> [    6.743829] PKRU: 55555554
> [    6.743829] Kernel panic - not syncing: Fatal exception
> [    6.743829] ---[ end Kernel panic - not syncing: Fatal exception ]---
> 
> The "NULL pointer dereference" happens at line 1620 of below code.
> 
> 1610 static struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> 1611 {
> 1612         const struct iommu_ops *ops = dev_iommu_ops(dev);
> 1613         struct iommu_group *group;
> 1614         int ret;
> 1615
> 1616         group = iommu_group_get(dev);
> 1617         if (group)
> 1618                 return group;
> 1619
> 1620         group = ops->device_group(dev);
> 1621         if (WARN_ON_ONCE(group == NULL))
> 1622                 return ERR_PTR(-EINVAL);
> 1623
> 1624         if (IS_ERR(group))
> 1625                 return group;
> 
> This platform has multiple IOMMU units, each serving different PCI
> devices. The ops field of each iommu_device is initialized in
> iommu_device_register(), hence when the first IOMMU device gets
> registered, ops fields of other iommu_devices are NULL.
> 
> Unfortunately bus_iommu_probe() called in iommu_device_register() probes
> *all* PCI devices. This probably leads to above NULL pointer dereference
> issue.
> 
> Please correct me if I overlooked anything.

Ha, as it happens I also just figured this out yesterday (after 
remembering the important detail of "intel_iommu=on", trying the lockdep 
test), so it's good to have confirmation, thanks! I'll test a fix today 
and send v3 soon.

Cheers,
Robin.

Re: [PATCH v2 09/14] iommu/ipmmu-vmsa: Clean up bus_set_iommu()

[Alexey Kardashevskiy]

On 28/04/2022 23:18, Robin Murphy wrote:
> Stop calling bus_set_iommu() since it's now unnecessary. This also
> leaves the custom initcall effectively doing nothing but register
> the driver, which no longer needs to happen early either, so convert
> it to builtin_platform_driver().
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/ipmmu-vmsa.c | 35 +----------------------------------
>   1 file changed, 1 insertion(+), 34 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 8fdb84b3642b..2549d32f0ddd 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device *pdev)
>   		ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
>   		if (ret)
>   			return ret;
> -
> -#if defined(CONFIG_IOMMU_DMA)
> -		if (!iommu_present(&platform_bus_type))
> -			bus_set_iommu(&platform_bus_type, &ipmmu_ops);
> -#endif
>   	}
>   
>   	/*

The comment which starts here did not make it to the patch but it should 
have as it mentions bus_set_iommu() which is gone by the end of the series.


More general question/request - could you please include the exact sha1 
the patchset is based on? It did not apply to any current trees and 
while it was trivial, it was slightly annoying to resolve the conflicts 
:)  Thanks,


> @@ -1168,32 +1163,4 @@ static struct platform_driver ipmmu_driver = {
>   	.probe = ipmmu_probe,
>   	.remove	= ipmmu_remove,
>   };
> -
> -static int __init ipmmu_init(void)
> -{
> -	struct device_node *np;
> -	static bool setup_done;
> -	int ret;
> -
> -	if (setup_done)
> -		return 0;
> -
> -	np = of_find_matching_node(NULL, ipmmu_of_ids);
> -	if (!np)
> -		return 0;
> -
> -	of_node_put(np);
> -
> -	ret = platform_driver_register(&ipmmu_driver);
> -	if (ret < 0)
> -		return ret;
> -
> -#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> -	if (!iommu_present(&platform_bus_type))
> -		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
> -#endif
> -
> -	setup_done = true;
> -	return 0;
> -}
> -subsys_initcall(ipmmu_init);
> +builtin_platform_driver(ipmmu_driver);

-- 
Alexey

Re: [PATCH v2 09/14] iommu/ipmmu-vmsa: Clean up bus_set_iommu()

[Robin Murphy]

On 2022-07-06 09:38, Alexey Kardashevskiy wrote:
> 
> 
> On 28/04/2022 23:18, Robin Murphy wrote:
>> Stop calling bus_set_iommu() since it's now unnecessary. This also
>> leaves the custom initcall effectively doing nothing but register
>> the driver, which no longer needs to happen early either, so convert
>> it to builtin_platform_driver().
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/ipmmu-vmsa.c | 35 +----------------------------------
>>   1 file changed, 1 insertion(+), 34 deletions(-)
>>
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index 8fdb84b3642b..2549d32f0ddd 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device 
>> *pdev)
>>           ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, 
>> &pdev->dev);
>>           if (ret)
>>               return ret;
>> -
>> -#if defined(CONFIG_IOMMU_DMA)
>> -        if (!iommu_present(&platform_bus_type))
>> -            bus_set_iommu(&platform_bus_type, &ipmmu_ops);
>> -#endif
>>       }
>>       /*
> 
> The comment which starts here did not make it to the patch but it should 
> have as it mentions bus_set_iommu() which is gone by the end of the series.

Heh, busted! In fact I think the whole point of that comment stops being 
true, but I couldn't be bothered to reason about it since one of the 
next steps after this is to start ripping all the arm_iommu_* stuff out 
anyway.

> More general question/request - could you please include the exact sha1 
> the patchset is based on? It did not apply to any current trees and 
> while it was trivial, it was slightly annoying to resolve the conflicts 
> :)  Thanks,

v3 is based directly on 5.19-rc3:

https://lore.kernel.org/lkml/cover.1657034827.git.robin.murphy@arm.com/

And if it helps I have it on a branch here as well:

https://gitlab.arm.com/linux-arm/linux-rm/-/tree/bus-set-iommu-v3

Robin.