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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Nov 2019 13:29:31 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     John Garry <john.garry@...wei.com>,
        Saravana Kannan <saravanak@...gle.com>
Cc:     Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        Will Deacon <will@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/7] iommu: Permit modular builds of ARM SMMU[v3] drivers

On 04/11/2019 12:16, John Garry wrote:
> On 01/11/2019 21:13, Saravana Kannan wrote:
>> On Fri, Nov 1, 2019 at 3:28 AM John Garry <john.garry@...wei.com> wrote:
>>>
>>> On 31/10/2019 23:34, Saravana Kannan via iommu wrote:
>>>> 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.
>>>
>>> If possible to add, that may be useful for remedying this:
>>>
>>> https://lore.kernel.org/linux-iommu/9625faf4-48ef-2dd3-d82f-931d9cf26976@huawei.com/ 
>>>
>>
>> I'm happy that this change might fix that problem, but isn't the
>> problem reported in that thread more to do with child devices getting
>> added before the parent probes successfully? That doesn't make sense
>> to me. 
> 
> So the pcieport device and then the child device are added in the PCI 
> scan, but only some time later do the device drivers probe for these 
> devices; so it's not that the that pcieport driver creates the child 
> device.
> 
> The problem then occurs in that the ordering the of device driver probe 
> is such that we have this: pcieport probe + defer (as no IOMMU group 
> registered), SMMU probe (registers the IOMMU group), child device probe, 
> pcieport really probe.
> 
> Can't the piceport driver not add its child devices before it
>> probes successfully? Or more specifically, who adds the child devices
>> of the pcieport before the pcieport itself probes?
> 
> The devices are actually added in order pcieport, child device, but not 
> really probed in that same order, as above.

Right, in short the fundamental problem is that of_iommu_configure() now 
does the wrong thing. Deferring probe of the entire host bridge/root 
complex based on "iommu-map" would indeed happen to solve the problem by 
brute force, I think, but could lead to a dependency cycle for PCI-based 
IOMMUs as Jean points out. I hope to have time this week to work a bit 
more on pulling of_iommu_configure() apart to fix it properly, after 
which of_devlink *should* only have to worry about the child devices 
themselves...

Robin.

> I'll add you to that thread if you want to discuss further.
> 
> Thanks,
> John
>