| 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
| ||
|
Date: Fri, 11 Nov 2022 15:09:33 +0000
From: Darren Kenny <darren.kenny@...cle.com>
To: Robin Murphy <robin.murphy@....com>,
Baolu Lu <baolu.lu@...ux.intel.com>,
Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Cc: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>,
harshit.m.mogalapalli@...il.com,
David Woodhouse <dwmw2@...radead.org>,
Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC] iommu/vt-d: set default value of INTEL_IOMMU_FLOPPY_WA to n
On Friday, 2022-11-11 at 12:36:48 UTC, Robin Murphy wrote:
> On 2022-11-11 02:31, Baolu Lu wrote:
>> On 2022/11/11 5:00, Konrad Rzeszutek Wilk wrote:
>>> On Thu, Nov 10, 2022 at 02:39:53PM +0800, Baolu Lu wrote:
>>>> On 2022/11/10 4:17, Konrad Rzeszutek Wilk wrote:
>>>>> On Wed, Nov 09, 2022 at 09:16:53PM +0800, Baolu Lu wrote:
>>>>>> On 2022/11/9 20:16, Harshit Mogalapalli wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 09/11/22 12:35 pm, Baolu Lu wrote:
>>>>>>>> On 2022/11/8 20:58, Harshit Mogalapalli wrote:
>>>>>>>>> It is likely that modern intel motherboard will not ship with a
>>>>>>>>> floppy connection anymore, so let us disable it by default, as it
>>>>>>>>> gets turned on when we do a make defconfig.
>>>>>>>>>
>>>>>>>>> Suggested-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
>>>>>>>>> Signed-off-by: Harshit Mogalapalli
>>>>>>>>> <harshit.m.mogalapalli@...cle.com>
>>>>>>>>> ---
>>>>>>>>> drivers/iommu/intel/Kconfig | 2 +-
>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/iommu/intel/Kconfig
>>>>>>>>> b/drivers/iommu/intel/Kconfig
>>>>>>>>> index b7dff5092fd2..c783ae85ca9b 100644
>>>>>>>>> --- a/drivers/iommu/intel/Kconfig
>>>>>>>>> +++ b/drivers/iommu/intel/Kconfig
>>>>>>>>> @@ -76,7 +76,7 @@ config INTEL_IOMMU_BROKEN_GFX_WA
>>>>>>>>> option is removed in the 2.6.32 kernel.
>>>>>>>>> config INTEL_IOMMU_FLOPPY_WA
>>>>>>>>> - def_bool y
>>>>>>>>> + def_bool n
>>>>>>>>> depends on X86
>>>>>>>>> help
>>>>>>>>> Floppy disk drivers are known to bypass DMA API calls
>>>>>>>>
>>>>>>>> Nobody selects or depends on this. How about removing this bool?
>>>>>>>> Only
>>>>>>>> less than 10 lines of code are impacted and are not in any
>>>>>>>> performance
>>>>>>>> path.
>>>>>>>>
>>>>>>>> diff --git a/drivers/iommu/intel/Kconfig
>>>>>>>> b/drivers/iommu/intel/Kconfig
>>>>>>>> index b7dff5092fd2..5e077d1c5f5d 100644
>>>>>>>> --- a/drivers/iommu/intel/Kconfig
>>>>>>>> +++ b/drivers/iommu/intel/Kconfig
>>>>>>>> @@ -75,15 +75,6 @@ config INTEL_IOMMU_BROKEN_GFX_WA
>>>>>>>> to use physical addresses for DMA, at least until this
>>>>>>>> option is removed in the 2.6.32 kernel.
>>>>>>>>
>>>>>>>> -config INTEL_IOMMU_FLOPPY_WA
>>>>>>>> - def_bool y
>>>>>>>> - depends on X86
>>>>>>>> - help
>>>>>>>> - Floppy disk drivers are known to bypass DMA API calls
>>>>>>>> - thereby failing to work when IOMMU is enabled. This
>>>>>>>> - workaround will setup a 1:1 mapping for the first
>>>>>>>> - 16MiB to make floppy (an ISA device) work.
>>>>>>>> -
>>>>>>>> config INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON
>>>>>>>> bool "Enable Intel IOMMU scalable mode by default"
>>>>>>>> default y
>>>>>>>> diff --git a/drivers/iommu/intel/iommu.c
>>>>>>>> b/drivers/iommu/intel/iommu.c
>>>>>>>> index 48cdcd0a5cf3..22801850f339 100644
>>>>>>>> --- a/drivers/iommu/intel/iommu.c
>>>>>>>> +++ b/drivers/iommu/intel/iommu.c
>>>>>>>> @@ -4567,7 +4567,6 @@ static void
>>>>>>>> intel_iommu_get_resv_regions(struct device *device,
>>>>>>>> }
>>>>>>>> rcu_read_unlock();
>>>>>>>>
>>>>>>>> -#ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
>>>>>>>> if (dev_is_pci(device)) {
>>>>>>>> struct pci_dev *pdev = to_pci_dev(device);
>>>>>>>>
>>>>>>>> @@ -4579,7 +4578,6 @@ static void
>>>>>>>> intel_iommu_get_resv_regions(struct device *device,
>>>>>>>> list_add_tail(®->list, head);
>>>>>>>> }
>>>>>>>> }
>>>>>>>> -#endif /* CONFIG_INTEL_IOMMU_FLOPPY_WA */
>>>>>>>>
>>>>>>>> reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>>>>>>>> IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
>>>>>>>>
>>>>>>>
>>>>>>> Hi Baolu,
>>>>>>>
>>>>>>> I have a question:
>>>>>>> Shouldn't we remove the code between ifdef-endif statements?
>>>>>>>
>>>>>>> I mean something like this:
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/intel/Kconfig
>>>>>>> b/drivers/iommu/intel/Kconfig
>>>>>>> index b7dff5092fd2..5e077d1c5f5d 100644
>>>>>>> --- a/drivers/iommu/intel/Kconfig
>>>>>>> +++ b/drivers/iommu/intel/Kconfig
>>>>>>> @@ -75,15 +75,6 @@ config INTEL_IOMMU_BROKEN_GFX_WA
>>>>>>> to use physical addresses for DMA, at least until this
>>>>>>> option is removed in the 2.6.32 kernel.
>>>>>>>
>>>>>>> -config INTEL_IOMMU_FLOPPY_WA
>>>>>>> - def_bool y
>>>>>>> - depends on X86
>>>>>>> - help
>>>>>>> - Floppy disk drivers are known to bypass DMA API calls
>>>>>>> - thereby failing to work when IOMMU is enabled. This
>>>>>>> - workaround will setup a 1:1 mapping for the first
>>>>>>> - 16MiB to make floppy (an ISA device) work.
>>>>>>> -
>>>>>>> config INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON
>>>>>>> bool "Enable Intel IOMMU scalable mode by default"
>>>>>>> default y
>>>>>>> diff --git a/drivers/iommu/intel/iommu.c
>>>>>>> b/drivers/iommu/intel/iommu.c
>>>>>>> index 48cdcd0a5cf3..2c416ad3204e 100644
>>>>>>> --- a/drivers/iommu/intel/iommu.c
>>>>>>> +++ b/drivers/iommu/intel/iommu.c
>>>>>>> @@ -4567,20 +4567,6 @@ static void
>>>>>>> intel_iommu_get_resv_regions(struct
>>>>>>> device *device,
>>>>>>> }
>>>>>>> rcu_read_unlock();
>>>>>>>
>>>>>>> -#ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
>>>>>>> - if (dev_is_pci(device)) {
>>>>>>> - struct pci_dev *pdev = to_pci_dev(device);
>>>>>>> -
>>>>>>> - if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
>>>>>>> - reg = iommu_alloc_resv_region(0, 1UL <<
>>>>>>> 24, prot,
>>>>>>> - IOMMU_RESV_DIRECT_RELAXABLE,
>>>>>>> - GFP_KERNEL);
>>>>>>> - if (reg)
>>>>>>> - list_add_tail(®->list, head);
>>>>>>> - }
>>>>>>> - }
>>>>>>> -#endif /* CONFIG_INTEL_IOMMU_FLOPPY_WA */
>>>>>>> -
>>>>>>> reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>>>>>>> IOAPIC_RANGE_END -
>>>>>>> IOAPIC_RANGE_START + 1,
>>>>>>> 0, IOMMU_RESV_MSI,
>>>>>>> GFP_KERNEL);
>>>>>>>
>>>>>>> This code is introduced in Commit d850c2ee5fe2 ("iommu/vt-d:
>>>>>>> Expose ISA
>>>>>>> direct mapping region via iommu_get_resv_regions")
>>>>>>
>>>>>> As long as floppy driver exists in the tree, we have to include above
>>>>>> code. Otherwise, floppy drivers don't work. At least we can easily
>>>>>> find
>>>>>> drivers/block/floppy.c which is still maintained (check
>>>>>> MAINTAINERS).:-)
>>>>>
>>>>> But this requires a machine with Intel IOMMU and ISA:
>>>>>
>>>>>>>> - 16MiB to make floppy (an ISA device) work.
>>>>>
>>>>> ISA device? I don't believe there are any Intel machines with an IOMMU
>>>>> and an ISA device?
>>>>
>>>> This workaround was introduced by commit 49a0429e53f2 ("Intel IOMMU:
>>>> Iommu floppy workaround") in 2007. I can't remember what happened 15
>>>> years ago, but I believe there must have been corresponding hardware
>>>> configurations at that time, and the Linux kernel has been maintained it
>>>> to now.
>>>
>>> At what point can this be removed then?
>>
>> No floppy block drivers in the tree or all floppy drivers' DMA going
>> through the kernel DMA APIs.
>
> Presumably the point of the config is that ISA bridges aren't expected
> on IA-64, so a tiny code saving can be made there, but at this point is
> anyone really that bothered about any more? There's already tons more
> code all through the driver to support newer features that aren't
> meaningful to old IA-64 hardware, so in my opinion, meh. Given that this
> already won't affect systems that truly can't have any ISA devices, I
> don't see any issue with simply making it unconditional.
>
> Note that plenty of chipsets still have an "ISA" bridge to an LPC
> interface, so ISA DMA is not necessarily as dead as one might like to think.
>
Based on what you're saying then, the fix that you agree with is what Baolu
first suggested, which is to remove it from Kconfig and just remove the
#ifdef/#endif, but not the code that it contains.
Is that correct?
Thanks,
Darren.
Powered by blists - more mailing lists