Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Rafael J. Wysocki]

On Mon, Mar 7, 2022 at 11:33 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Bjorn, Rafael,
>
> On 3/5/22 11:37, Hans de Goede wrote:
> > Hi,
> >
> > On 3/4/22 16:46, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 3/4/22 16:32, Bjorn Helgaas wrote:
> >>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
> >>>> Hi Bjorn,
> >>>>
> >>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
> >>>>> From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>>>>
> >>>>> Many folks have reported PCI devices not working.  It could affect any
> >>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
> >>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
> >>>>>
> >>>>> In every report, a region in the E820 table entirely encloses a PCI host
> >>>>> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
> >>>>> regions when allocating address space"), we ignore the entire window,
> >>>>> preventing us from assigning space to PCI devices.
> >>>>>
> >>>>> For example, the dmesg log [2] from bug report [1] shows:
> >>>>>
> >>>>>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> >>>>>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
> >>>>>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
> >>>>>
> >>>>> The efi=debug dmesg log [3] from the same report shows the EFI memory map
> >>>>> entries that created the E820 map:
> >>>>>
> >>>>>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
> >>>>>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
> >>>>>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
> >>>>>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
> >>>>>
> >>>>> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> >>>>> works around issues where _CRS contains non-window address space that can't
> >>>>> be used for PCI devices.  It does this by removing E820 regions from host
> >>>>> bridge windows.  But in these reports, the E820 region covers the entire
> >>>>> window, so 4dc2287c1805 makes it completely unusable.
> >>>>>
> >>>>> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
> >>>>>
> >>>>>   Used by system firmware to request that a memory-mapped IO region be
> >>>>>   mapped by the OS to a virtual address so it can be accessed by EFI
> >>>>>   runtime services.
> >>>>>
> >>>>> A host bridge window is definitely a memory-mapped IO region, and EFI
> >>>>> runtime services may need to access it, so I don't think we can argue that
> >>>>> this is a firmware defect.
> >>>>>
> >>>>> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
> >>>>> when they overlap *part* of a host bridge window on the assumption that a
> >>>>> partial overlap is really register space, not part of the window proper.
> >>>>>
> >>>>> If an E820 region covers the entire window from _CRS, assume the _CRS
> >>>>> window is correct and do nothing.
> >>>>>
> >>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> >>>>> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
> >>>>> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
> >>>>>
> >>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
> >>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
> >>>>> 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
> >>>>> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> >>>>> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@xxxxxxxxxx
> >>>>> Based-on-patch-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>>> Reported-by: Benoit Grégoire <benoitg@xxxxxxxx>   # BZ 206459
> >>>>> Reported-by: wse@xxxxxxxxxxxxxxxxxxx              # BZ 214259
> >>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>>>> ---
> >>>>>  arch/x86/kernel/resource.c | 11 +++++++++++
> >>>>>  1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> >>>>> index 7378ea146976..405f0af53e3d 100644
> >>>>> --- a/arch/x86/kernel/resource.c
> >>>>> +++ b/arch/x86/kernel/resource.c
> >>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
> >>>>>           e820_start = entry->addr;
> >>>>>           e820_end = entry->addr + entry->size - 1;
> >>>>>
> >>>>> +         /*
> >>>>> +          * If an E820 entry covers just part of the resource, we
> >>>>> +          * assume E820 is telling us about something like host
> >>>>> +          * bridge register space that is unavailable for PCI
> >>>>> +          * devices.  But if it covers the *entire* resource, it's
> >>>>> +          * more likely just telling us that this is MMIO space, and
> >>>>> +          * that doesn't need to be removed.
> >>>>> +          */
> >>>>> +         if (e820_start <= avail->start && avail->end <= e820_end)
> >>>>> +                 continue;
> >>>>> +
> >>>>
> >>>> IMHO it would be good to add some logging here, since hitting this is
> >>>> somewhat of a special case. For the Fedora test kernels I did I changed
> >>>> this to:
> >>>>
> >>>>            if (e820_start <= avail->start && avail->end <= e820_end) {
> >>>>                    dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
> >>>>                             avail, e820_start, e820_end);
> >>>>                    continue;
> >>>>            }
> >>>>
> >>>> And I expect/hope to see this new info message on the ideapad with the
> >>>> touchpad issue.
> >>>
> >>> Right, I would expect the same.
> >>>
> >>> We could add something like this.  But both the e820 entry and the
> >>> host bridge window are already in the dmesg log, so it doesn't really
> >>> add new information
> >>
> >> Well it adds the information that the workaround (to the workaround)
> >> which we added for this case is working as expected and it allows
> >> seeing that is the case in a single glance.
> >
> > So I just got the first report back from the Fedora test 5.16.12 kernel
> > with this series added. Good news on the ideapad this wotks fine to
> > fix the touchpad issue (as expected).
> >
> > What is interesting is that the above dev_info message which I added
> > triggers *twice*:
> >
> > [    0.327837] acpi PNP0A08:00: resource [mem 0x000a0000-0x000bffff window] fully covered by e820 entry [mem 0x0009f000-0x000fffff]
> > [    0.327843] acpi PNP0A08:00: resource [mem 0x65400000-0xbfffffff window] fully covered by e820 entry [mem 0x4bc50000-0xcfffffff]
> >
> > Notice that it also stops from the mem-window for ISA io getting fully
> > clipped, which I did not realize also was a potential issue.
> >
> > I hope this also shows that having the dev_info here is good,
> > at least IMHO this confirms that having the dev_info for this
> > is a good thing.
> >
> > I'm still waiting for testing results on the X1C2 which had the
> > suspend/resume regressions with my bios-date based approach.
>
> I have heard back from the X1C2 user, he does not have access to
> the machine atm he will get back to me in a couple of days.
>
> I don't really expect any surprises there though, so given where
> we are in the kernel-cycle and that we already have confirmation
> that it fixes the ideapad touchpad issues I think we should move
> forward with this patch-set now.
>
> Rafael, can you drop my variant of this patch?  (this series is
> a cleaner implementation of basically the same method to fix
> things)

Done.

> Bjorn, I assume you will merge this series through your tree?

Same here, and please feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

to all of the patches in this series.

Thanks!