Re: [PATCH v3] x86/PCI: Ignore E820 reservations for bridge windows on newer systems

[Rafael J. Wysocki]

On Thu, Oct 14, 2021 at 1:24 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 14-10-2021 13:14, Rafael J. Wysocki wrote:
> > On Thu, Oct 14, 2021 at 1:04 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Some BIOS-es contain a bug where they add addresses which map to system RAM
> >> in the PCI bridge memory window returned by the ACPI _CRS method, see
> >> commit 4dc2287c1805 ("x86: avoid E820 regions when allocating address
> >> space").
> >>
> >> To avoid this Linux by default excludes E820 reservations when allocating
> >> addresses since 2010. Windows however ignores E820 reserved regions for PCI
> >> mem allocations, so in hindsight Linux honoring them is a problem.
> >>
> >> Recently (2020) some systems have shown-up with E820 reservations which
> >> cover the entire _CRS returned PCI bridge memory window, causing all
> >> attempts to assign memory to PCI BARs which have not been setup by the
> >> BIOS to fail. For example here are the relevant dmesg bits from a
> >> Lenovo IdeaPad 3 15IIL 81WE:
> >>
> >>  [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> >>  pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
> >>
> >> Ideally Linux would fully stop honoring E820 reservations for PCI mem
> >> allocations, but then the old systems this was added for will regress.
> >> Instead keep the old behavior for old systems, while ignoring the E820
> >> reservations like Windows does for any systems from now on.
> >>
> >> Old systems are defined here as BIOS year < 2018, this was chosen to
> >> make sure that pci_use_e820 will not be set on the currently affected
> >> systems, while at the same time also taking into account that the
> >> systems for which the E820 checking was originally added may have
> >> received BIOS updates for quite a while (esp. CVE related ones),
> >> giving them a more recent BIOS year then 2010.
> >>
> >> Also add pci=no_e820 and pci=use_e820 options to allow overriding
> >> the BIOS year heuristic.
> >>
> >> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
> >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
> >> BugLink: https://bugs.launchpad.net/bugs/1878279
> >> BugLink: https://bugs.launchpad.net/bugs/1931715
> >> BugLink: https://bugs.launchpad.net/bugs/1932069
> >> BugLink: https://bugs.launchpad.net/bugs/1921649
> >> Cc: Benoit Grégoire <benoitg@xxxxxxxx>
> >> Cc: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> >> Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Thank you.
>
> > with one tiny nit below.
> >
> > Or please let me know if you want me to pick this up.
>
> Since all of the changes are under arch/x86/ I expect the x86/tip
> folks to pick this up ?

OK

> >
> >> ---
> >> Changes in v3:
> >> - Commit msg tweaks (drop dmesg timestamps, typo fix)
> >> - Use "defined(CONFIG_...)" instead of "defined CONFIG_..."
> >> - Add Mika's Reviewed-by
> >>
> >> Changes in v2:
> >> - Replace the per model DMI quirk approach with disabling E820 reservations
> >>   checking for all systems with a BIOS year >= 2018
> >> - Add documentation for the new kernel-parameters to
> >>   Documentation/admin-guide/kernel-parameters.txt
> >> ---
> >> Other patches trying to address the same issue:
> >> https://lore.kernel.org/r/20210624095324.34906-1-hui.wang@xxxxxxxxxxxxx
> >> https://lore.kernel.org/r/20200617164734.84845-1-mika.westerberg@xxxxxxxxxxxxxxx
> >> V1 patch:
> >> https://lore.kernel.org/r/20211005150956.303707-1-hdegoede@xxxxxxxxxx
> >> ---
> >>  .../admin-guide/kernel-parameters.txt         |  6 ++++
> >>  arch/x86/include/asm/pci_x86.h                | 10 +++++++
> >>  arch/x86/kernel/resource.c                    |  4 +++
> >>  arch/x86/pci/acpi.c                           | 29 +++++++++++++++++++
> >>  arch/x86/pci/common.c                         |  6 ++++
> >>  5 files changed, 55 insertions(+)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index 43dc35fe5bc0..969cde5d74c8 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -3949,6 +3949,12 @@
> >>                                 please report a bug.
> >>                 nocrs           [X86] Ignore PCI host bridge windows from ACPI.
> >>                                 If you need to use this, please report a bug.
> >> +               use_e820        [X86] Honor E820 reservations when allocating
> >> +                               PCI host bridge memory. If you need to use this,
> >> +                               please report a bug.
> >> +               no_e820         [X86] ignore E820 reservations when allocating
> >> +                               PCI host bridge memory. If you need to use this,
> >> +                               please report a bug.
> >>                 routeirq        Do IRQ routing for all PCI devices.
> >>                                 This is normally done in pci_enable_device(),
> >>                                 so this option is a temporary workaround
> >> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
> >> index 490411dba438..0bb4e7dd0ffc 100644
> >> --- a/arch/x86/include/asm/pci_x86.h
> >> +++ b/arch/x86/include/asm/pci_x86.h
> >> @@ -39,6 +39,8 @@ do {                                          \
> >>  #define PCI_ROOT_NO_CRS                0x100000
> >>  #define PCI_NOASSIGN_BARS      0x200000
> >>  #define PCI_BIG_ROOT_WINDOW    0x400000
> >> +#define PCI_USE_E820           0x800000
> >> +#define PCI_NO_E820            0x1000000
> >>
> >>  extern unsigned int pci_probe;
> >>  extern unsigned long pirq_table_addr;
> >> @@ -64,6 +66,8 @@ void pcibios_scan_specific_bus(int busn);
> >>
> >>  /* pci-irq.c */
> >>
> >> +struct pci_dev;
> >> +
> >>  struct irq_info {
> >>         u8 bus, devfn;                  /* Bus, device and function */
> >>         struct {
> >> @@ -232,3 +236,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
> >>  # define x86_default_pci_init_irq      NULL
> >>  # define x86_default_pci_fixup_irqs    NULL
> >>  #endif
> >> +
> >> +#if defined(CONFIG_PCI) && defined(CONFIG_ACPI)
> >> +extern bool pci_use_e820;
> >> +#else
> >> +#define pci_use_e820 false
> >> +#endif
> >> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> >> index 9b9fb7882c20..e8dc9bc327bd 100644
> >> --- a/arch/x86/kernel/resource.c
> >> +++ b/arch/x86/kernel/resource.c
> >> @@ -1,6 +1,7 @@
> >>  // SPDX-License-Identifier: GPL-2.0
> >>  #include <linux/ioport.h>
> >>  #include <asm/e820/api.h>
> >> +#include <asm/pci_x86.h>
> >>
> >>  static void resource_clip(struct resource *res, resource_size_t start,
> >>                           resource_size_t end)
> >> @@ -28,6 +29,9 @@ static void remove_e820_regions(struct resource *avail)
> >>         int i;
> >>         struct e820_entry *entry;
> >>
> >> +       if (!pci_use_e820)
> >> +               return;
> >> +
> >>         for (i = 0; i < e820_table->nr_entries; i++) {
> >>                 entry = &e820_table->entries[i];
> >>
> >> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> >> index 948656069cdd..6c2febe84b6f 100644
> >> --- a/arch/x86/pci/acpi.c
> >> +++ b/arch/x86/pci/acpi.c
> >> @@ -21,6 +21,8 @@ struct pci_root_info {
> >>
> >>  static bool pci_use_crs = true;
> >>  static bool pci_ignore_seg = false;
> >> +/* Consumed in arch/x86/kernel/resource.c */
> >> +bool pci_use_e820 = false;
> >>
> >>  static int __init set_use_crs(const struct dmi_system_id *id)
> >>  {
> >> @@ -160,6 +162,33 @@ void __init pci_acpi_crs_quirks(void)
> >>                "if necessary, use \"pci=%s\" and report a bug\n",
> >>                pci_use_crs ? "Using" : "Ignoring",
> >>                pci_use_crs ? "nocrs" : "use_crs");
> >> +
> >> +       /*
> >> +        * Some BIOS-es contain a bug where they add addresses which map to system
> >> +        * RAM in the PCI bridge memory window returned by the ACPI _CRS method, see
> >> +        * commit 4dc2287c1805 ("x86: avoid E820 regions when allocating address space").
> >> +        * To avoid this Linux by default excludes E820 reservations when allocating
> >> +        * addresses since 2010. Windows however ignores E820 reserved regions for
> >> +        * PCI mem allocations, so in hindsight Linux honoring them is a problem.
> >> +        * In 2020 some systems have shown-up with E820 reservations which cover the
> >> +        * entire _CRS returned PCI bridge memory window, causing all attempts to
> >> +        * assign memory to PCI BARs to fail if Linux honors the E820 reservations.
> >> +        *
> >> +        * Ideally Linux would fully stop honoring E820 reservations for PCI mem
> >> +        * allocations, but then the old systems this was added for will regress.
> >> +        * Instead keep the old behavior for old systems, while ignoring the E820
> >> +        * reservations like Windows does for any systems from now on.
> >> +        */
> >> +       if (year >= 0 && year < 2018)
> >> +               pci_use_e820 = true;
> >> +
> >> +       if (pci_probe & PCI_NO_E820)
> >> +               pci_use_e820 = false;
> >> +       else if (pci_probe & PCI_USE_E820)
> >> +               pci_use_e820 = true;
> >> +
> >> +       printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
> >> +              pci_use_e820 ? "Honoring" : "Ignoring");
> >
> > Why not pr_info()?
>
> This file is using printk(KERN_... consistently everywhere. I'm just following
> the existing style here. I very much dislike mixing styles in a single file.

In this particular case, it isn't just a matter of style.

Also, if what is regarded as a good practice has changed since the
file was created, should new code added to it be prevented from
following the new good practice, because the old code didn't follow
it?

> If we want to change this for this file then IMHO the right thing to do would
> be a follow up patch changing all the printk-s at once.

I would do the pr_info() here in this patch and change the rest of the
file to follow in a subsequent patch.