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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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@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.

At what point can this be removed then?