Re: [PATCH 0/7] iommu: Permit modular builds of ARM SMMU[v3] drivers

[Jean-Philippe Brucker]

Hi Saravana, Will,

On Wed, Oct 30, 2019 at 05:57:44PM -0700, Saravana Kannan via iommu wrote:
> > > > Obviously you need to be careful about using IOMMU drivers as modules,
> > > > since late loading of the driver for an IOMMU serving active DMA masters
> > > > is going to end badly in many cases. On Android, we're using device links
> > > > to ensure that the IOMMU probes first.
> > >
> > > Out of curiosity, which device links are those? Clearly not the RPM links
> > > created by the IOMMU drivers themselves... Is this some special Android
> > > magic, or is there actually a chance of replacing all the
> > > of_iommu_configure() machinery with something more generic?
> >
> > I'll admit that I haven't used them personally yet, but I'm referring to
> > this series from Saravana [CC'd]:
> >
> > https://lore.kernel.org/linux-acpi/20190904211126.47518-1-saravanak@xxxxxxxxxx/
> >
> > which is currently sitting in linux-next now that we're upstreaming the
> > "special Android magic" ;)

Neat, I'm trying to do the same for virtio-iommu. It needs to be modular
because it depends on the virtio transport, which distributions usually
build as a module. So far I've been managing the device links in
virtio-iommu's add_device() and remove_device() callbacks [1]. Since it
relies on the existing probe deferral, I had to make a special case for
virtio-iommu to avoid giving up after initcalls_done [2].

Currently buggy, it explodes on the second modprobe.

[1] http://jpbrucker.net/git/linux/commit/?h=virtio-iommu/module-2019-10-31&id=f72978be18cb52eaa2d46dc762711bacbfab5039
[2] http://jpbrucker.net/git/linux/commit/?h=virtio-iommu/module-2019-10-31&id=f5fe188bb7fde33422ef08b9aad956dc3c77ec39

[...]
> Wrt IOMMUs, the only missing piece in upstream is a trivial change
> that does something like this in drivers/of/property.c
> 
> +static struct device_node *parse_iommus(struct device_node *np,
> +                                        const char *prop_name, int index)
> +{
> +        return parse_prop_cells(np, prop_name, index, "iommus",
> +                                "#iommu-cells");
> +}

The 'iommus' property only applies to platform devices, do you have any
plan for PCI?  PCI devices generally don't have a DT node. Only their root
bridge has a node, with an 'iommu-map' property instead of 'iommus', so
I don't think add_links() would get called for them.

I'm also unsure about distro vendors agreeing to a mandatory kernel
parameter (of_devlink). Do you plan to eventually enable it by default?

> static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_clocks, },
>         { .parse_prop = parse_interconnects, },
>         { .parse_prop = parse_regulators, },
> +        { .parse_prop = parse_iommus, },
>         {},
> };
> 
> I plan to upstream this pretty soon, but I have other patches in
> flight that touch the same file and I'm waiting for those to get
> accepted. I also want to clean up the code a bit to reduce some
> repetition before I add support for more bindings.

I'm also wondering about ACPI support. IOMMU already has a sort of
canonical code path that links endpoints to their IOMMU
(iommu_probe_device()), after the firmware descriptions have been parsed.
So if we created the device links in the iommu core, for example
iommu_bus_notifier(), we would support all firmware interface flavors.
Otherwise we'll have to create those device links in the IORT driver as
well (plus DMAR and IVRS if they want it).

Robin pointed out that the SMMU drivers already have device links for
power management, can those be reused for module support as well?  I think
the device_link_add() flags might need to differ but I haven't looked
closely at the implementation yet.

Thanks,
Jean