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.
I just got a report back from the Fedora test 5.16.12 kernel
with this series added on the X1C2 which had the suspend/resume
regression with my DMI_BIOS_DATE based approach. Everything still
works well there and it shows the new log messages from 2/3 in action:
[ 0.326504] acpi PNP0A08:00: clipped [mem 0xdfa00000-0xfebfffff window] to [mem 0xdfa10000-0xfebfffff window] for e820 entry [mem 0xdceff000-0xdfa0ffff]
[ 0.326515] acpi PNP0A08:00: clipped [mem 0xdfa10000-0xfebfffff window] to [mem 0xdfa10000-0xf7ffffff window] for e820 entry [mem 0xf8000000-0xfbffffff]
Regards,
Hans
