Re: [RFC] iommu/vt-d: set default value of INTEL_IOMMU_FLOPPY_WA to n

[Baolu Lu <baolu.lu@linux.intel.com>]

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@oracle.com>
>>>>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.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.

Best regards,
baolu