linux-kernel - Re: [RFC] iommu/vt-d: set default value of INTEL_IOMMU_FLOPPY

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:   Fri, 11 Nov 2022 10:31:26 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Cc:     baolu.lu@...ux.intel.com,
        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>,
        Robin Murphy <robin.murphy@....com>, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Darren Kenny <darren.kenny@...cle.com>
Subject: Re: [RFC] iommu/vt-d: set default value of INTEL_IOMMU_FLOPPY_WA to n

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(&reg->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(&reg->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.

Best regards,
baolu