Message-ID: <20191101114148.GA2694906@lophozonia>
Date:   Fri, 1 Nov 2019 12:41:48 +0100
From:   Jean-Philippe Brucker <jean-philippe@...aro.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Will Deacon <will@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        iommu@...ts.linux-foundation.org,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 0/7] iommu: Permit modular builds of ARM SMMU[v3] drivers

On Thu, Oct 31, 2019 at 04:34:14PM -0700, Saravana Kannan wrote:
> > 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.

Virtio-iommu can use the PCI transport [3], in which case you need a
finer-grained approach. In some implementations you'll have the IOMMU
interface and endpoints managed by that IOMMU under the same RC. I'm not
proud of it.

[3] 6c9e92ef8bdd ("dt-bindings: virtio: Add virtio-pci-iommu node")

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

It's not as generic as device-tree, each vendor has their own table to
describe the IOMMU topology. I don't see a nice way to transpose the
add_links() callback there. Links need to be created either in a common
path (iommu_probe_device()) or in the APCI IORT driver.

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

Oh right, device_link_add() would reuse an existing link. It even seems to
work for IOMMU drivers that want to manage PM links themselves
(DL_FLAG_STATELESS).

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

Yes I admit I only glanced over it, but it seems like a nice solution. At
the moment of_iommu_xlate() handles the dependency itself. It currently
give up after initcalls but than can be tweaked to wait indefinitely (my
patch [2]). If of_devlink was enabled by default and handled iommu-map it
could probably replace the of_iommu probe deferral. For now it might be
best to manage device links somewhere in the IOMMU core, so ACPI
implementations (which do probe deferral their own way) can benefit from
it as well.

Thanks,
Jean

Powered by blists - more mailing lists