linux-kernel - Re: [PATCH 02/13] iommu: Move bus setup to IOMMU device registration

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]

Message-ID: <8cd04084-54d0-9e10-f16f-99541e42d7eb@arm.com>
Date:   Sat, 23 Apr 2022 09:37:21 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>, joro@...tes.org,
        will@...nel.org
Cc:     iommu@...ts.linux-foundation.org, sven@...npeter.dev,
        robdclark@...il.com, m.szyprowski@...sung.com,
        yong.wu@...iatek.com, mjrosato@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, zhang.lyra@...il.com,
        thierry.reding@...il.com, vdumpa@...dia.com,
        jean-philippe@...aro.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/13] iommu: Move bus setup to IOMMU device registration

On 2022-04-23 09:01, Lu Baolu wrote:
> Hi Robin,
> 
> On 2022/4/19 15:20, Robin Murphy wrote:
>> On 2022-04-19 00:37, Lu Baolu wrote:
>>> On 2022/4/19 6:09, Robin Murphy wrote:
>>>> On 2022-04-16 01:04, Lu Baolu wrote:
>>>>> On 2022/4/14 20:42, Robin Murphy wrote:
>>>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type 
>>>>>> *bus)
>>>>>>    */
>>>>>>   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops 
>>>>>> *ops)
>>>>>>   {
>>>>>> -    int err;
>>>>>> -
>>>>>> -    if (ops == NULL) {
>>>>>> -        bus->iommu_ops = NULL;
>>>>>> -        return 0;
>>>>>> -    }
>>>>>> -
>>>>>> -    if (bus->iommu_ops != NULL)
>>>>>> +    if (bus->iommu_ops && ops && bus->iommu_ops != ops)
>>>>>>           return -EBUSY;
>>>>>>       bus->iommu_ops = ops;
>>>>>
>>>>> Do we still need to keep above lines in bus_set_iommu()?
>>>>
>>>> It preserves the existing behaviour until each callsite and its 
>>>> associated error handling are removed later on, which seems like as 
>>>> good a thing to do as any. Since I'm already relaxing 
>>>> iommu_device_register() to a warn-but-continue behaviour while it 
>>>> keeps the bus ops on life-support internally, I figured not changing 
>>>> too much at once would make it easier to bisect any potential issues 
>>>> arising from this first step.
>>>
>>> Fair enough. Thank you for the explanation.
>>>
>>> Do you have a public tree that I could pull these patches and try them
>>> on an Intel hardware? Or perhaps you have done this? I like the whole
>>> idea of this series, but it's better to try it with a real hardware.
>>
>> I haven't bothered with separate branches since there's so many 
>> different pieces in-flight, but my complete (unstable) development 
>> branch can be found here:
>>
>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus
>>
>> For now I'd recommend winding the head back to "iommu: Clean up 
>> bus_set_iommu()" for testing - some of the patches above that have 
>> already been posted and picked up by their respective subsystems, but 
>> others are incomplete and barely compile-tested. I'll probably 
>> rearrange it later this week to better reflect what's happened so far.
> 
> I wound the head back to "iommu: Clean up bus_set_iommu" and tested it
> on an Intel machine. It got stuck during boot. This test was on a remote
> machine and I have no means to access it physically. So I can't get any
> kernel debugging messages. (I have to work from home these days. :-()
> 
> I guess it's due to the fact that intel_iommu_probe_device() callback
> only works for the pci devices. The issue occurs when probing a device
> other than a PCI one.

Yeah, I was wondering if that would be significant, since it's the only 
driver that never registered itself for platform_bus_type so won't have 
actually seen those calls before. I'm inclined to bodge that as below 
for now, as long as it then works OK in terms of the rest of the changes.

Thanks,
Robin.

----->8-----
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9fa1b98186a3..6e359f92ec00 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4565,6 +4565,10 @@ static struct iommu_device 
*intel_iommu_probe_device(struct device *dev)
  	unsigned long flags;
  	u8 bus, devfn;

+	/* ANDD platform device support needs fixing */
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
  	iommu = device_to_iommu(dev, &bus, &devfn);
  	if (!iommu)
  		return ERR_PTR(-ENODEV);