Messages in this thread | | | Date | Wed, 9 Nov 2022 19:25:02 +0530 | Subject | Re: [RFC] iommu/vt-d: set default value of INTEL_IOMMU_FLOPPY_WA to n | From | Harshit Mogalapalli <> |
| |
On 09/11/22 6:56 pm, Robin Murphy wrote: > On 09/11/2022 12:16 pm, 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(®->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? > Hi Baolu and Robin,
> No, machines with floppy drives may be uncommon, but almost certainly > some are still in use and being updated with new kernels, so we should > continue to support them. > Thanks a lot for explaining.
I misunderstood the code, I thought when we are removing ifdef-endif statement, the code inside(within ifdef-endif) becomes redundant, thanks for correcting me.
> Do you have any practical issues with the ISA reserved region being > present, or is the only motivation here to save a few bytes of code when > it won't be needed? In the latter case, making it "def_bool BLK_DEV_FD" > might be sensible. >
No practical issues seen.
> (For the original patch, note that "def_bool n" is always a bit > redundant since plain "bool" implicitly defaults to "n" anyway) > Okay. How about Baolu's version of the patch, where we are removing the CONFIG item from Kconfig and ifdef statement from the code in iommu.c?
Thanks, Harshit
> Thanks, > Robin. > >> 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") >> >> If others don't have an objection I can send a patch with the above diff. >> >> Note: I am unsure about the code internals. >> >> Thanks, >> Harshit >> >> >>> Best regards, >>> baolu
| |