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

[Saravana Kannan]

On Thu, Oct 31, 2019 at 12:38 PM Jean-Philippe Brucker
<jean-philippe@xxxxxxxxxx> wrote:
>
> 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,

An early version of this patch series was limited to platform device,
but that's not true with the version that Will pointed to and was
merged into driver-core-next. The iommu parsing and creating device
links applies to all devices that use DT. That's why this code is in
of/property.c opposed to of/platform.c.

> 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 looked into the iommu-map property and it shouldn't be too hard to
add support for it. Looks like we can simply hold off on probing the
root bridge device till all the iommus in its iommu-map are probed and
we should be fine.

> 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.

I'd love to add ACPI support too, but I have zero knowledge of ACPI.
I'd be happy to help anyone who wants to add ACPI support that allows
ACPI to add device links.

> 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).

IOMMU driver/framework or whoever else can create device links as
necessary. That's not mutually exclusive to the firmware adding device
links (the device links APIs handle this nicely). While device probe
ordering is one benefit of my patch series, that's not all of it
though. It also deals with making sure suppliers known when they can
clean up the boot state of their device even when the drivers for all
their consumers are loaded as modules (so late initcall won't work). I
can go into more details on this if needed, but that latter part is
not very relevant in this context and you can find most of the details
in my patch series/documentation I added.

Thanks,
Saravana