Messages in this thread | | | From | Saravana Kannan <> | Date | Thu, 31 Oct 2019 16:34:14 -0700 | Subject | Re: [PATCH 0/7] iommu: Permit modular builds of ARM SMMU[v3] drivers |
| |
On Thu, Oct 31, 2019 at 12:38 PM Jean-Philippe Brucker <jean-philippe@linaro.org> 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@google.com/ > > > > > > 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
| |