On Thu, Feb 13, 2025 at 11:49:00PM +0000, Robin Murphy wrote:
> In hindsight, there were some crucial subtleties overlooked when moving
> {of,acpi}_dma_configure() to driver probe time to allow waiting for
> IOMMU drivers with -EPROBE_DEFER, and these have become an
> ever-increasing source of problems. The IOMMU API has some fundamental
> assumptions that iommu_probe_device() is called for every device added
> to the system, in the order in which they are added. Calling it in a
> random order or not at all dependent on driver binding leads to
> malformed groups, a potential lack of isolation for devices with no
> driver, and all manner of unexpected concurrency and race conditions.
> We've attempted to mitigate the latter with point-fix bodges like
> iommu_probe_device_lock, but it's a losing battle and the time has come
> to bite the bullet and address the true source of the problem instead.
>
> The crux of the matter is that the firmware parsing actually serves two
> distinct purposes; one is identifying the IOMMU instance associated with
> a device so we can check its availability, the second is actually
> telling that instance about the relevant firmware-provided data for the
> device. However the latter also depends on the former, and at the time
> there was no good place to defer and retry that separately from the
> availability check we also wanted for client driver probe.
>
> Nowadays, though, we have a proper notion of multiple IOMMU instances in
> the core API itself, and each one gets a chance to probe its own devices
> upon registration, so we can finally make that work as intended for
> DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to
> be able to run the iommu_fwspec machinery currently buried deep in the
> wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be
> surprisingly straightforward to bootstrap this transformation by pretty
> much just calling the same path twice. At client driver probe time,
> dev->driver is obviously set; conversely at device_add(), or a
> subsequent bus_iommu_probe(), any device waiting for an IOMMU really
> should *not* have a driver already, so we can use that as a condition to
> disambiguate the two cases, and avoid recursing back into the IOMMU core
> at the wrong times.
>
> Obviously this isn't the nicest thing, but for now it gives us a
> functional baseline to then unpick the layers in between without many
> more awkward cross-subsystem patches. There are some minor side-effects
> like dma_range_map potentially being created earlier, and some debug
> prints being repeated, but these aren't significantly detrimental. Let's
> make things work first, then deal with making them nice.
>
> With the basic flow finally in the right order again, the next step is
> probably turning the bus->dma_configure paths inside-out, since all we
> really need from bus code is its notion of which device and input ID(s)
> to parse the common firmware properties with...
>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
> drivers/acpi/arm64/dma.c | 5 ++++
> drivers/acpi/scan.c | 10 +++-----
> drivers/amba/bus.c | 2 +-
> drivers/base/platform.c | 2 +-
> drivers/bus/fsl-mc/fsl-mc-bus.c | 2 +-
> drivers/cdx/cdx.c | 2 +-
> drivers/iommu/iommu.c | 43 ++++++++++++++++++++++++---------
> drivers/iommu/of_iommu.c | 10 +++++++-
> drivers/of/device.c | 7 +++++-
> drivers/pci/pci-driver.c | 2 +-
Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> # pci-driver.c
I assume this will be merged via the IOMMU tree.
> 10 files changed, 60 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
> index 52b2abf88689..f30f138352b7 100644
> --- a/drivers/acpi/arm64/dma.c
> +++ b/drivers/acpi/arm64/dma.c
> @@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)
> else
> end = (1ULL << 32) - 1;
>
> + if (dev->dma_range_map) {
> + dev_dbg(dev, "dma_range_map already set\n");
> + return;
> + }
> +
> ret = acpi_dma_get_range(dev, &map);
> if (!ret && map) {
> end = dma_range_map_max(map);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 9f4efa8f75a6..42b8f1833c3c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1619,6 +1619,9 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
> {
> int err;
>
> + if (device_iommu_mapped(dev))
> + return 0;
> +
> /* Serialise to make dev->iommu stable under our potential fwspec */
> mutex_lock(&iommu_probe_device_lock);
> /* If we already translated the fwspec there is nothing left to do */
> @@ -1632,13 +1635,6 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
> err = viot_iommu_configure(dev);
> mutex_unlock(&iommu_probe_device_lock);
>
> - /*
> - * If we have reason to believe the IOMMU driver missed the initial
> - * iommu_probe_device() call for dev, replay it to get things in order.
> - */
> - if (!err && dev->bus)
> - err = iommu_probe_device(dev);
> -
> return err;
> }
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 8ef259b4d037..abbb37d4d228 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -364,7 +364,7 @@ static int amba_dma_configure(struct device *dev)
> ret = acpi_dma_configure(dev, attr);
> }
>
> - if (!ret && !drv->driver_managed_dma) {
> + if (dev->driver && !ret && !drv->driver_managed_dma) {
> ret = iommu_device_use_default_domain(dev);
> if (ret)
> arch_teardown_dma_ops(dev);
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 6f2a33722c52..4c7570d637f9 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1451,7 +1451,7 @@ static int platform_dma_configure(struct device *dev)
> attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
> ret = acpi_dma_configure(dev, attr);
> }
> - if (ret || drv->driver_managed_dma)
> + if (!dev->driver || ret || drv->driver_managed_dma)
> return ret;
>
> ret = iommu_device_use_default_domain(dev);
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index d1f3d327ddd1..fb58833b222a 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -153,7 +153,7 @@ static int fsl_mc_dma_configure(struct device *dev)
> else
> ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
>
> - if (!ret && !mc_drv->driver_managed_dma) {
> + if (dev->driver && !ret && !mc_drv->driver_managed_dma) {
> ret = iommu_device_use_default_domain(dev);
> if (ret)
> arch_teardown_dma_ops(dev);
> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index c573ed2ee71a..d5761b96a412 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -360,7 +360,7 @@ static int cdx_dma_configure(struct device *dev)
> return ret;
> }
>
> - if (!ret && !cdx_drv->driver_managed_dma) {
> + if (dev->driver && !ret && !cdx_drv->driver_managed_dma) {
> ret = iommu_device_use_default_domain(dev);
> if (ret)
> arch_teardown_dma_ops(dev);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 2486f6d6ef68..89f634d46aad 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -519,17 +519,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> struct group_device *gdev;
> int ret;
>
> - /*
> - * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
> - * instances with non-NULL fwnodes, and client devices should have been
> - * identified with a fwspec by this point. Otherwise, we can currently
> - * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
> - * be present, and that any of their registered instances has suitable
> - * ops for probing, and thus cheekily co-opt the same mechanism.
> - */
> - ops = iommu_fwspec_ops(dev_iommu_fwspec_get(dev));
> - if (!ops)
> - return -ENODEV;
> /*
> * Serialise to avoid races between IOMMU drivers registering in
> * parallel and/or the "replay" calls from ACPI/OF code via client
> @@ -539,9 +528,41 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> */
> lockdep_assert_held(&iommu_probe_device_lock);
>
> + /*
> + * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
> + * is buried in the bus dma_configure path. Properly unpicking that is
> + * still a fairly big job, so for now just invoke the whole thing. Our
> + * bus_iommu_probe() walk may see devices with drivers already bound,
> + * but that must mean they're already configured - either probed by
> + * another IOMMU, or there was no IOMMU for iommu_fwspec_init() to wait
> + * for - so either way we can safely skip this and avoid worrying about
> + * those recursing back here thinking they need a replay call.
> + */
> + if (!dev->driver && dev->bus->dma_configure) {
> + mutex_unlock(&iommu_probe_device_lock);
> + dev->bus->dma_configure(dev);
> + mutex_lock(&iommu_probe_device_lock);
> + }
> +
> + /*
> + * At this point, either valid devices now have a fwspec, or we can
> + * 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.
> + */
> + ops = iommu_fwspec_ops(dev_iommu_fwspec_get(dev));
> + if (!ops)
> + return -ENODEV;
> +
> /* Device is probed already if in a group */
> if (dev->iommu_group)
> return 0;
> + /*
> + * And if we do now see any replay calls, they would indicate someone
> + * misusing the dma_configure path outside bus code.
> + */
> + if (dev_iommu_fwspec_get(dev) && dev->driver)
> + dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
>
> ret = iommu_init_device(dev, ops);
> if (ret)
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 97987cd78da9..c9aaf5783b77 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -121,6 +121,9 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
> if (!master_np)
> return -ENODEV;
>
> + if (device_iommu_mapped(dev))
> + return 0;
> +
> /* Serialise to make dev->iommu stable under our potential fwspec */
> mutex_lock(&iommu_probe_device_lock);
> if (dev_iommu_fwspec_get(dev)) {
> @@ -151,7 +154,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
> iommu_fwspec_free(dev);
> mutex_unlock(&iommu_probe_device_lock);
>
> - if (!err && dev->bus)
> + /*
> + * If we have reason to believe the IOMMU driver missed the initial
> + * iommu_probe_device() call for dev, try to fix it up. This should
> + * no longer happen unless of_dma_configure() is being misused.
> + */
> + if (!err && dev->driver)
> err = iommu_probe_device(dev);
>
> if (err && err != -EPROBE_DEFER)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index edf3be197265..5053e5d532cc 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
> bool coherent, set_map = false;
> int ret;
>
> + if (dev->dma_range_map) {
> + dev_dbg(dev, "dma_range_map already set\n");
> + goto skip_map;
> + }
> +
> if (np == dev->of_node)
> bus_np = __of_get_dma_parent(np);
> else
> @@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
> end = dma_range_map_max(map);
> set_map = true;
> }
> -
> +skip_map:
> /*
> * If @dev is expected to be DMA-capable then the bus code that created
> * it should have initialised its dma_mask pointer by this point. For
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f57ea36d125d..143b2f2081ea 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1653,7 +1653,7 @@ static int pci_dma_configure(struct device *dev)
>
> pci_put_host_bridge_device(bridge);
>
> - if (!ret && !driver->driver_managed_dma) {
> + if (dev->driver && !ret && !driver->driver_managed_dma) {
> ret = iommu_device_use_default_domain(dev);
> if (ret)
> arch_teardown_dma_ops(dev);
> --
> 2.39.2.101.g768bb238c484.dirty
>
