[5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi All,

Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
Ignore E820 reservations for bridge windows on newer systems"):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7

breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
X1 gen 2, see:
https://bugzilla.redhat.com/show_bug.cgi?id=2029207

This regression was actually caught be Fedora already carrying this
patch for a while now and as such it has been reproduced with 5.15
with an older version of the patch which still allowed turning the
new behavior of by adding "pci=use_e820". Dmesg output with and
without the option has just been attached to the bug, I've not
analyzed this any further yet.

I guess that for now this means that we need to revert commit
7f7b4236f204. Rafael, I'll send you a revert with a commit msg
explaining why this needs to be reverted tomorrow.

More interesting IMHO is finding out another solution. Both the touchpad
problem which got me looking into this:
https://bugzilla.redhat.com/show_bug.cgi?id=1868899

As well as the thunderbolt hotplug issue Mika was looking at:
https://bugzilla.kernel.org/show_bug.cgi?id=206459

both are cases where we fail to find a memory-window for a
BAR which has not been setup yet.

So I see a couple of options here:

1. Detect that the e820 reservations fully cover (one of)
the PCI bridge main 32 bit memory windows and if that happens
ignore them. This actually was my first plan when I started
working on this. In the end I choose the other option
because Bjorn indicated that in hindsight honoring the e820
reservations might have been a mistake and maybe we should
get rid of honoring them all together.

2. Have a flag which, when we fail to alloc a 32 bit
(or 64 bit) memory PCI BAR, is set if not already set
and then retry the alloc. And make the e820 reservation
carve-out get skipped in this case.

3. When booting with pci=nocrs as a workaround for
the touchpad case a 64 but memory window ends up getting
used. There already is some special handling for some
AMD bridges where if there are no 64 bit memory Windows
in the _CRS for the bridge, one gets added. Maybe we need
to do the same for Intel bridges ?

Please let me know which of these options you think I should
try to implement next; of course alternative ideas for fixing
this are also welcome.

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi,

On 2/8/22 16:25, Hans de Goede wrote:
> Hi All,
> 
> Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
> Ignore E820 reservations for bridge windows on newer systems"):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
> 
> breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
> X1 gen 2, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
> 
> This regression was actually caught be Fedora already carrying this
> patch for a while now and as such it has been reproduced with 5.15
> with an older version of the patch which still allowed turning the
> new behavior of by adding "pci=use_e820". Dmesg output with and
> without the option has just been attached to the bug, I've not
> analyzed this any further yet.
> 
> I guess that for now this means that we need to revert commit
> 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
> explaining why this needs to be reverted tomorrow.
> 
> More interesting IMHO is finding out another solution. Both the touchpad
> problem which got me looking into this:
> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> 
> As well as the thunderbolt hotplug issue Mika was looking at:
> https://bugzilla.kernel.org/show_bug.cgi?id=206459
> 
> both are cases where we fail to find a memory-window for a
> BAR which has not been setup yet.
> 
> So I see a couple of options here:
> 
> 1. Detect that the e820 reservations fully cover (one of)
> the PCI bridge main 32 bit memory windows and if that happens
> ignore them. This actually was my first plan when I started
> working on this. In the end I choose the other option
> because Bjorn indicated that in hindsight honoring the e820
> reservations might have been a mistake and maybe we should
> get rid of honoring them all together.
> 
> 2. Have a flag which, when we fail to alloc a 32 bit
> (or 64 bit) memory PCI BAR, is set if not already set
> and then retry the alloc. And make the e820 reservation
> carve-out get skipped in this case.
> 
> 3. When booting with pci=nocrs as a workaround for
> the touchpad case a 64 but memory window ends up getting
> used. There already is some special handling for some
> AMD bridges where if there are no 64 bit memory Windows
> in the _CRS for the bridge, one gets added. Maybe we need
> to do the same for Intel bridges ?

4. It seems that all devices which have issues with allocating
a PCI bar are Ice Lake based; and the model where the ignoring
of e820 reservations has been reported to cause issues is somewhat
old. It is a Haswell, but still getting BIOS updates causing
the BIOS date check to enable the new behavior. So another
solution might be to only ignore e820 reservations on machines
with Intel Ice Lake (and presumably also Tiger Lake) CPUs.


5. It also seems that the troublesome e820 entry on all devices
ends at 0xcfffffff and starts well below 0x8000000 :

Yoga C940:
[    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved

IdeaPad 3 15IIL05:
[    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved

Lenovo IdeaPad 5 14IIL05:
[    0.000000] BIOS-e820: [mem 0x000000005bc50000-0x00000000cfffffff] reserved


Actually even the lower addresses seem to follow a pattern.

So I guess that we could do a really narrow fix here where we only
ignore the *this* e820 reservation using the properties that:

a) It is "huge"
b) Always ends at 0x00000000cfffffff

To identify it.



I was at first tempted to choose 1 as fix, which basically is also
based on this huge reservation covering the entire 32 bit mem window of
the pci-bridge(s) in the system. But that seems to boil down to the same
thing as 5. Where as 5 can be a really narrowly tailored fix, requiring
only changes to arch/x86/kernel/resource.c . So atm I'm leaning towards
trying 5.  As already said, plase let me know what you think.

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Mika Westerberg]

Hi Hans,

On Tue, Feb 08, 2022 at 04:59:13PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/8/22 16:25, Hans de Goede wrote:
> > Hi All,
> > 
> > Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
> > Ignore E820 reservations for bridge windows on newer systems"):
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
> > 
> > breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
> > X1 gen 2, see:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2029207

:-(

> > This regression was actually caught be Fedora already carrying this
> > patch for a while now and as such it has been reproduced with 5.15
> > with an older version of the patch which still allowed turning the
> > new behavior of by adding "pci=use_e820". Dmesg output with and
> > without the option has just been attached to the bug, I've not
> > analyzed this any further yet.
> > 
> > I guess that for now this means that we need to revert commit
> > 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
> > explaining why this needs to be reverted tomorrow.
> > 
> > More interesting IMHO is finding out another solution. Both the touchpad
> > problem which got me looking into this:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> > 
> > As well as the thunderbolt hotplug issue Mika was looking at:
> > https://bugzilla.kernel.org/show_bug.cgi?id=206459
> > 
> > both are cases where we fail to find a memory-window for a
> > BAR which has not been setup yet.
> > 
> > So I see a couple of options here:
> > 
> > 1. Detect that the e820 reservations fully cover (one of)
> > the PCI bridge main 32 bit memory windows and if that happens
> > ignore them. This actually was my first plan when I started
> > working on this. In the end I choose the other option
> > because Bjorn indicated that in hindsight honoring the e820
> > reservations might have been a mistake and maybe we should
> > get rid of honoring them all together.
> > 
> > 2. Have a flag which, when we fail to alloc a 32 bit
> > (or 64 bit) memory PCI BAR, is set if not already set
> > and then retry the alloc. And make the e820 reservation
> > carve-out get skipped in this case.
> > 
> > 3. When booting with pci=nocrs as a workaround for
> > the touchpad case a 64 but memory window ends up getting
> > used. There already is some special handling for some
> > AMD bridges where if there are no 64 bit memory Windows
> > in the _CRS for the bridge, one gets added. Maybe we need
> > to do the same for Intel bridges ?
> 
> 4. It seems that all devices which have issues with allocating
> a PCI bar are Ice Lake based; and the model where the ignoring
> of e820 reservations has been reported to cause issues is somewhat
> old. It is a Haswell, but still getting BIOS updates causing
> the BIOS date check to enable the new behavior. So another
> solution might be to only ignore e820 reservations on machines
> with Intel Ice Lake (and presumably also Tiger Lake) CPUs.
> 
> 
> 5. It also seems that the troublesome e820 entry on all devices
> ends at 0xcfffffff and starts well below 0x8000000 :
> 
> Yoga C940:
> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> 
> IdeaPad 3 15IIL05:
> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> 
> Lenovo IdeaPad 5 14IIL05:
> [    0.000000] BIOS-e820: [mem 0x000000005bc50000-0x00000000cfffffff] reserved

I don't remember the details anymore but looking at the commit log of my
"fix" attempt here:

https://bugzilla.kernel.org/attachment.cgi?id=287661

The EFI memory map actually seems to consists of several entries that somehow
are merged by something (I think this is the EFI stub but not sure). Booting
with "efi=debug" may help us to understand this further (or not).

On that Yoga system, this:

  [Reserved           |   |  |  |  |  |  |  | |   |WB|WT|WC|UC] range=[0x000000002bc50000-0x000000003fffffff] (323MB)
  [Reserved           |   |  |  |  |  |  |  | |   |WB|  |  |UC] range=[0x0000000040000000-0x0000000040ffffff] (16MB)
  [Reserved           |   |  |  |  |  |  |  | |   |  |  |  |  ] range=[0x0000000041000000-0x00000000453fffff] (68MB)
  [Memory Mapped I/O  |RUN|  |  |  |  |  |  | |   |  |  |  |UC] range=[0x0000000045400000-0x00000000cfffffff] (2220MB)

became this:

  BIOS-e820: [mem 0x000000002bc50000-0x00000000cfffffff] reserved

Since the area (0x45400000-0xcfffffff) is marked as MMIO I think maybe we can
simply skip those areas in arch_remove_reservations() or so?

I may be missing a lots of details, though. ;-)

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi Mika,

On 2/8/22 17:38, Mika Westerberg wrote:
> Hi Hans,
> 
> On Tue, Feb 08, 2022 at 04:59:13PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/8/22 16:25, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
>>> Ignore E820 reservations for bridge windows on newer systems"):
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
>>>
>>> breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
>>> X1 gen 2, see:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
> 
> :-(

Agreed.

>>> This regression was actually caught be Fedora already carrying this
>>> patch for a while now and as such it has been reproduced with 5.15
>>> with an older version of the patch which still allowed turning the
>>> new behavior of by adding "pci=use_e820". Dmesg output with and
>>> without the option has just been attached to the bug, I've not
>>> analyzed this any further yet.
>>>
>>> I guess that for now this means that we need to revert commit
>>> 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
>>> explaining why this needs to be reverted tomorrow.
>>>
>>> More interesting IMHO is finding out another solution. Both the touchpad
>>> problem which got me looking into this:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>
>>> As well as the thunderbolt hotplug issue Mika was looking at:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>>
>>> both are cases where we fail to find a memory-window for a
>>> BAR which has not been setup yet.
>>>
>>> So I see a couple of options here:
>>>
>>> 1. Detect that the e820 reservations fully cover (one of)
>>> the PCI bridge main 32 bit memory windows and if that happens
>>> ignore them. This actually was my first plan when I started
>>> working on this. In the end I choose the other option
>>> because Bjorn indicated that in hindsight honoring the e820
>>> reservations might have been a mistake and maybe we should
>>> get rid of honoring them all together.
>>>
>>> 2. Have a flag which, when we fail to alloc a 32 bit
>>> (or 64 bit) memory PCI BAR, is set if not already set
>>> and then retry the alloc. And make the e820 reservation
>>> carve-out get skipped in this case.
>>>
>>> 3. When booting with pci=nocrs as a workaround for
>>> the touchpad case a 64 but memory window ends up getting
>>> used. There already is some special handling for some
>>> AMD bridges where if there are no 64 bit memory Windows
>>> in the _CRS for the bridge, one gets added. Maybe we need
>>> to do the same for Intel bridges ?
>>
>> 4. It seems that all devices which have issues with allocating
>> a PCI bar are Ice Lake based; and the model where the ignoring
>> of e820 reservations has been reported to cause issues is somewhat
>> old. It is a Haswell, but still getting BIOS updates causing
>> the BIOS date check to enable the new behavior. So another
>> solution might be to only ignore e820 reservations on machines
>> with Intel Ice Lake (and presumably also Tiger Lake) CPUs.
>>
>>
>> 5. It also seems that the troublesome e820 entry on all devices
>> ends at 0xcfffffff and starts well below 0x8000000 :
>>
>> Yoga C940:
>> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>
>> IdeaPad 3 15IIL05:
>> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>
>> Lenovo IdeaPad 5 14IIL05:
>> [    0.000000] BIOS-e820: [mem 0x000000005bc50000-0x00000000cfffffff] reserved
> 
> I don't remember the details anymore but looking at the commit log of my
> "fix" attempt here:
> 
> https://bugzilla.kernel.org/attachment.cgi?id=287661
> 
> The EFI memory map actually seems to consists of several entries that somehow
> are merged by something (I think this is the EFI stub but not sure). Booting
> with "efi=debug" may help us to understand this further (or not).
> 
> On that Yoga system, this:
> 
>   [Reserved           |   |  |  |  |  |  |  | |   |WB|WT|WC|UC] range=[0x000000002bc50000-0x000000003fffffff] (323MB)
>   [Reserved           |   |  |  |  |  |  |  | |   |WB|  |  |UC] range=[0x0000000040000000-0x0000000040ffffff] (16MB)
>   [Reserved           |   |  |  |  |  |  |  | |   |  |  |  |  ] range=[0x0000000041000000-0x00000000453fffff] (68MB)
>   [Memory Mapped I/O  |RUN|  |  |  |  |  |  | |   |  |  |  |UC] range=[0x0000000045400000-0x00000000cfffffff] (2220MB)
> 
> became this:
> 
>   BIOS-e820: [mem 0x000000002bc50000-0x00000000cfffffff] reserved
> 
> Since the area (0x45400000-0xcfffffff) is marked as MMIO I think maybe we can
> simply skip those areas in arch_remove_reservations() or so?
> 
> I may be missing a lots of details, though. ;-)

Oh, I just did some initial digging through the source code and indeed on EFI
there is no actual e820 memory map at all instead it gets faked to be able
to re-use the BIOS boot based e820 code in the kernel by do_add_efi_memmap()
from arch/x86/platform/efi/efi.c .

And that does:

		switch (md->type) {
		...
                default:
                        /*
                         * EFI_RESERVED_TYPE EFI_RUNTIME_SERVICES_CODE
                         * EFI_RUNTIME_SERVICES_DATA EFI_MEMORY_MAPPED_IO
                         * EFI_MEMORY_MAPPED_IO_PORT_SPACE EFI_PAL_CODE
                         */
                        e820_type = E820_TYPE_RESERVED;
                        break;

Which seems to be the root cause of the problems, at least on the Yoga C940,
but I expect on the others too (will try to get that confirmed).

So yes this seems like a very promising solution direction actually.
I will try to see if I can find a test-machine here in my home office
with an EFI memmap entry with a MMIO type and then see if I can come
up with a patch to make arch_remove_reservations() not exclude those
areas.

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi,

On 2/8/22 17:38, Mika Westerberg wrote:
> Hi Hans,
> 
> On Tue, Feb 08, 2022 at 04:59:13PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/8/22 16:25, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
>>> Ignore E820 reservations for bridge windows on newer systems"):
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
>>>
>>> breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
>>> X1 gen 2, see:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
> 
> :-(
> 
>>> This regression was actually caught be Fedora already carrying this
>>> patch for a while now and as such it has been reproduced with 5.15
>>> with an older version of the patch which still allowed turning the
>>> new behavior of by adding "pci=use_e820". Dmesg output with and
>>> without the option has just been attached to the bug, I've not
>>> analyzed this any further yet.
>>>
>>> I guess that for now this means that we need to revert commit
>>> 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
>>> explaining why this needs to be reverted tomorrow.
>>>
>>> More interesting IMHO is finding out another solution. Both the touchpad
>>> problem which got me looking into this:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>
>>> As well as the thunderbolt hotplug issue Mika was looking at:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>>
>>> both are cases where we fail to find a memory-window for a
>>> BAR which has not been setup yet.
>>>
>>> So I see a couple of options here:
>>>
>>> 1. Detect that the e820 reservations fully cover (one of)
>>> the PCI bridge main 32 bit memory windows and if that happens
>>> ignore them. This actually was my first plan when I started
>>> working on this. In the end I choose the other option
>>> because Bjorn indicated that in hindsight honoring the e820
>>> reservations might have been a mistake and maybe we should
>>> get rid of honoring them all together.
>>>
>>> 2. Have a flag which, when we fail to alloc a 32 bit
>>> (or 64 bit) memory PCI BAR, is set if not already set
>>> and then retry the alloc. And make the e820 reservation
>>> carve-out get skipped in this case.
>>>
>>> 3. When booting with pci=nocrs as a workaround for
>>> the touchpad case a 64 but memory window ends up getting
>>> used. There already is some special handling for some
>>> AMD bridges where if there are no 64 bit memory Windows
>>> in the _CRS for the bridge, one gets added. Maybe we need
>>> to do the same for Intel bridges ?
>>
>> 4. It seems that all devices which have issues with allocating
>> a PCI bar are Ice Lake based; and the model where the ignoring
>> of e820 reservations has been reported to cause issues is somewhat
>> old. It is a Haswell, but still getting BIOS updates causing
>> the BIOS date check to enable the new behavior. So another
>> solution might be to only ignore e820 reservations on machines
>> with Intel Ice Lake (and presumably also Tiger Lake) CPUs.
>>
>>
>> 5. It also seems that the troublesome e820 entry on all devices
>> ends at 0xcfffffff and starts well below 0x8000000 :
>>
>> Yoga C940:
>> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>
>> IdeaPad 3 15IIL05:
>> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>
>> Lenovo IdeaPad 5 14IIL05:
>> [    0.000000] BIOS-e820: [mem 0x000000005bc50000-0x00000000cfffffff] reserved
> 
> I don't remember the details anymore but looking at the commit log of my
> "fix" attempt here:
> 
> https://bugzilla.kernel.org/attachment.cgi?id=287661
> 
> The EFI memory map actually seems to consists of several entries that somehow
> are merged by something (I think this is the EFI stub but not sure). Booting
> with "efi=debug" may help us to understand this further (or not).
> 
> On that Yoga system, this:
> 
>   [Reserved           |   |  |  |  |  |  |  | |   |WB|WT|WC|UC] range=[0x000000002bc50000-0x000000003fffffff] (323MB)
>   [Reserved           |   |  |  |  |  |  |  | |   |WB|  |  |UC] range=[0x0000000040000000-0x0000000040ffffff] (16MB)
>   [Reserved           |   |  |  |  |  |  |  | |   |  |  |  |  ] range=[0x0000000041000000-0x00000000453fffff] (68MB)
>   [Memory Mapped I/O  |RUN|  |  |  |  |  |  | |   |  |  |  |UC] range=[0x0000000045400000-0x00000000cfffffff] (2220MB)
> 
> became this:
> 
>   BIOS-e820: [mem 0x000000002bc50000-0x00000000cfffffff] reserved
> 
> Since the area (0x45400000-0xcfffffff) is marked as MMIO I think maybe we can
> simply skip those areas in arch_remove_reservations() or so?
> 
> I may be missing a lots of details, though. ;-)

So I've been looking at this for a couple of hours now and I've
basically re-invented your original fix from:

https://lkml.org/lkml/2020/6/17/751

So here we are 2 years later and that still looks like the best
fix, so IMHO we should just go with that fix.

An alternative, much more elaborate fix would be to:

1. Add E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW types to
   enum e820_type and modify the 2 places which check for type == reserved
   to treat these both as reserved too, so as to not have any functional
   changes there

2. Modify the code building e820 tables from the EFI memmap to use
   E820_TYPE_MMIO for MMIO EFI memmap entries and make e820_nomerge()
   treat E820_TYPE_MMIO as non mergable to retain these as is

3. Modify pci_acpi_root_prepare_resources() to compare ACPI provided
   bridge mmio windows against E820_TYPE_MMIO e820_Table entries and
   if there is an exact match (as is the case on the Yoga C940)
   then change the e820 type to E820_TYPE_MMIO_PCI_BRIDGE_WINDOW

   This means that we are now very narrowly treating EFI MMIO marked
   regions special, in the special case where they are a 1:1 map
   to an ACPI PCI bridge window resource.

   Note since we treat both E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
   identical to the old RESERVED everywhere there is no functional change
   here.

4. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
   e820 table entries with a type of E820_TYPE_MMIO_PCI_BRIDGE_WINDOW,
   this would actually be a functional change and should fix the
   issues we are trying to fix.

Note an alternative would be to skip having the extra E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
type and to skip step 3. above. That would boil down to doing the same
as your original patch in a somewhat cleaner manner.

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Mika Westerberg]

Hi Hans,

Thanks for looking into this!

On Wed, Feb 09, 2022 at 04:12:32PM +0100, Hans de Goede wrote:
> An alternative, much more elaborate fix would be to:
> 
> 1. Add E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW types to
>    enum e820_type and modify the 2 places which check for type == reserved
>    to treat these both as reserved too, so as to not have any functional
>    changes there
> 
> 2. Modify the code building e820 tables from the EFI memmap to use
>    E820_TYPE_MMIO for MMIO EFI memmap entries and make e820_nomerge()
>    treat E820_TYPE_MMIO as non mergable to retain these as is
> 
> 3. Modify pci_acpi_root_prepare_resources() to compare ACPI provided
>    bridge mmio windows against E820_TYPE_MMIO e820_Table entries and
>    if there is an exact match (as is the case on the Yoga C940)
>    then change the e820 type to E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
> 
>    This means that we are now very narrowly treating EFI MMIO marked
>    regions special, in the special case where they are a 1:1 map
>    to an ACPI PCI bridge window resource.
> 
>    Note since we treat both E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
>    identical to the old RESERVED everywhere there is no functional change
>    here.
> 
> 4. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
>    e820 table entries with a type of E820_TYPE_MMIO_PCI_BRIDGE_WINDOW,
>    this would actually be a functional change and should fix the
>    issues we are trying to fix.
> 
> Note an alternative would be to skip having the extra E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
> type and to skip step 3. above. That would boil down to doing the same
> as your original patch in a somewhat cleaner manner.

I agree and my vote goes for this last alternative (e.g skipping step 3).
That would also make it slightly easier to follow the logic if someone
in the future needs to investigate possible issues around this, I think.

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi,

On 2/9/22 17:01, Mika Westerberg wrote:
> Hi Hans,
> 
> Thanks for looking into this!
> 
> On Wed, Feb 09, 2022 at 04:12:32PM +0100, Hans de Goede wrote:
>> An alternative, much more elaborate fix would be to:
>>
>> 1. Add E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW types to
>>    enum e820_type and modify the 2 places which check for type == reserved
>>    to treat these both as reserved too, so as to not have any functional
>>    changes there
>>
>> 2. Modify the code building e820 tables from the EFI memmap to use
>>    E820_TYPE_MMIO for MMIO EFI memmap entries and make e820_nomerge()
>>    treat E820_TYPE_MMIO as non mergable to retain these as is
>>
>> 3. Modify pci_acpi_root_prepare_resources() to compare ACPI provided
>>    bridge mmio windows against E820_TYPE_MMIO e820_Table entries and
>>    if there is an exact match (as is the case on the Yoga C940)
>>    then change the e820 type to E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
>>
>>    This means that we are now very narrowly treating EFI MMIO marked
>>    regions special, in the special case where they are a 1:1 map
>>    to an ACPI PCI bridge window resource.
>>
>>    Note since we treat both E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
>>    identical to the old RESERVED everywhere there is no functional change
>>    here.
>>
>> 4. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
>>    e820 table entries with a type of E820_TYPE_MMIO_PCI_BRIDGE_WINDOW,
>>    this would actually be a functional change and should fix the
>>    issues we are trying to fix.
>>
>> Note an alternative would be to skip having the extra E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
>> type and to skip step 3. above. That would boil down to doing the same
>> as your original patch in a somewhat cleaner manner.
> 
> I agree and my vote goes for this last alternative (e.g skipping step 3).
> That would also make it slightly easier to follow the logic if someone
> in the future needs to investigate possible issues around this, I think.

As mentioned in my email from 10 seconds ago I think a better simpler
fix would be to just do:

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..18656f823764 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -28,6 +28,10 @@ static void remove_e820_regions(struct resource *avail)
 	int i;
 	struct e820_entry *entry;
 
+	/* Only remove E820 reservations on classic BIOS boot */
+	if (efi_enabled(EFI_MEMMAP))
+		return;
+
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
 

I'm curious what you think of that?

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Mika Westerberg]

Hi Hans,

On Wed, Feb 09, 2022 at 05:08:13PM +0100, Hans de Goede wrote:
> As mentioned in my email from 10 seconds ago I think a better simpler
> fix would be to just do:
> 
> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> index 9b9fb7882c20..18656f823764 100644
> --- a/arch/x86/kernel/resource.c
> +++ b/arch/x86/kernel/resource.c
> @@ -28,6 +28,10 @@ static void remove_e820_regions(struct resource *avail)
>  	int i;
>  	struct e820_entry *entry;
>  
> +	/* Only remove E820 reservations on classic BIOS boot */
> +	if (efi_enabled(EFI_MEMMAP))
> +		return;
> +
>  	for (i = 0; i < e820_table->nr_entries; i++) {
>  		entry = &e820_table->entries[i];
>  
> 
> I'm curious what you think of that?

I'm not an expert in this e820 stuff but this one looks really simple
and makes sense to me. So definitely should go with it assuming there
are no objections from the x86 maintainers.

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi,

On 2/10/22 07:39, Mika Westerberg wrote:
> Hi Hans,
> 
> On Wed, Feb 09, 2022 at 05:08:13PM +0100, Hans de Goede wrote:
>> As mentioned in my email from 10 seconds ago I think a better simpler
>> fix would be to just do:
>>
>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>> index 9b9fb7882c20..18656f823764 100644
>> --- a/arch/x86/kernel/resource.c
>> +++ b/arch/x86/kernel/resource.c
>> @@ -28,6 +28,10 @@ static void remove_e820_regions(struct resource *avail)
>>  	int i;
>>  	struct e820_entry *entry;
>>  
>> +	/* Only remove E820 reservations on classic BIOS boot */
>> +	if (efi_enabled(EFI_MEMMAP))
>> +		return;
>> +
>>  	for (i = 0; i < e820_table->nr_entries; i++) {
>>  		entry = &e820_table->entries[i];
>>  
>>
>> I'm curious what you think of that?
> 
> I'm not an expert in this e820 stuff but this one looks really simple
> and makes sense to me. So definitely should go with it assuming there
> are no objections from the x86 maintainers.

Unfortunately with this suspend/resume is still broken on the ThinkPad X1 carbon gen 2 of the reporter reporting the regression. The reporter has been kind enough to also test in EFI mode (at my request) and then the problem is back again with this patch. So just differentiating between EFI / non EFI mode is not an option.

FYI, here is what I believe is the root-cause of the issue on the ThinkPad X1 carbon gen 2:

The E820 reservations table has the following in both BIOS and EFI boot modes:

[    0.000000] BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved

Which has a small overlap with:

[    0.884684] pci_bus 0000:00: root bus resource [mem 0xdfa00000-0xfebfffff window]

This leads to the following difference in assignments of PCI resources when honoring E820 reservations

[    0.966573] pci 0000:00:1c.0: BAR 14: assigned [mem 0xdfb00000-0xdfcfffff]
[    0.966698] pci_bus 0000:02: resource 1 [mem 0xdfb00000-0xdfcfffff]

vs the following when ignoring E820 reservations:

[    0.966850] pci 0000:00:1c.0: BAR 14: assigned [mem 0xdfa00000-0xdfbfffff]
[    0.966973] pci_bus 0000:02: resource 1 [mem 0xdfa00000-0xdfbfffff]

And the overlap of 0xdfa00000-0xdfa0ffff from the e820 reservations seems to be what is causing the suspend/resume issue.

###

As already somewhat discussed, I'll go and prepare this solution instead:

1. Add E820_TYPE_MMIO to enum e820_type and modify the 2 places which check for
   type == reserved to treat this as reserved too, so as to not have any
   functional changes there

2. Modify the code building e820 tables from the EFI memmap to use
   E820_TYPE_MMIO for MMIO EFI memmap entries.

3. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
   e820 table entries with a type of E820_TYPE_MMIO,
   this would actually be a functional change and should fix the
   issues we are trying to fix.

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Mika Westerberg]

Hi Hans,

On Mon, Feb 14, 2022 at 01:42:29PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/10/22 07:39, Mika Westerberg wrote:
> > Hi Hans,
> > 
> > On Wed, Feb 09, 2022 at 05:08:13PM +0100, Hans de Goede wrote:
> >> As mentioned in my email from 10 seconds ago I think a better simpler
> >> fix would be to just do:
> >>
> >> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> >> index 9b9fb7882c20..18656f823764 100644
> >> --- a/arch/x86/kernel/resource.c
> >> +++ b/arch/x86/kernel/resource.c
> >> @@ -28,6 +28,10 @@ static void remove_e820_regions(struct resource *avail)
> >>  	int i;
> >>  	struct e820_entry *entry;
> >>  
> >> +	/* Only remove E820 reservations on classic BIOS boot */
> >> +	if (efi_enabled(EFI_MEMMAP))
> >> +		return;
> >> +
> >>  	for (i = 0; i < e820_table->nr_entries; i++) {
> >>  		entry = &e820_table->entries[i];
> >>  
> >>
> >> I'm curious what you think of that?
> > 
> > I'm not an expert in this e820 stuff but this one looks really simple
> > and makes sense to me. So definitely should go with it assuming there
> > are no objections from the x86 maintainers.
> 
> Unfortunately with this suspend/resume is still broken on the ThinkPad
> X1 carbon gen 2 of the reporter reporting the regression. The reporter
> has been kind enough to also test in EFI mode (at my request) and then
> the problem is back again with this patch. So just differentiating
> between EFI / non EFI mode is not an option.

Thanks for the update! Too bad that it did not solve the regression, though :(

> FYI, here is what I believe is the root-cause of the issue on the ThinkPad X1 carbon gen 2:
> 
> The E820 reservations table has the following in both BIOS and EFI boot modes:
> 
> [    0.000000] BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved
> 
> Which has a small overlap with:
> 
> [    0.884684] pci_bus 0000:00: root bus resource [mem 0xdfa00000-0xfebfffff window]
> 
> This leads to the following difference in assignments of PCI resources when honoring E820 reservations
> 
> [    0.966573] pci 0000:00:1c.0: BAR 14: assigned [mem 0xdfb00000-0xdfcfffff]
> [    0.966698] pci_bus 0000:02: resource 1 [mem 0xdfb00000-0xdfcfffff]
> 
> vs the following when ignoring E820 reservations:
> 
> [    0.966850] pci 0000:00:1c.0: BAR 14: assigned [mem 0xdfa00000-0xdfbfffff]
> [    0.966973] pci_bus 0000:02: resource 1 [mem 0xdfa00000-0xdfbfffff]
> 
> And the overlap of 0xdfa00000-0xdfa0ffff from the e820 reservations seems to be what is causing the suspend/resume issue.

Any idea what is using that range?

> ###
> 
> As already somewhat discussed, I'll go and prepare this solution instead:
> 
> 1. Add E820_TYPE_MMIO to enum e820_type and modify the 2 places which check for
>    type == reserved to treat this as reserved too, so as to not have any
>    functional changes there
> 
> 2. Modify the code building e820 tables from the EFI memmap to use
>    E820_TYPE_MMIO for MMIO EFI memmap entries.
> 
> 3. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
>    e820 table entries with a type of E820_TYPE_MMIO,
>    this would actually be a functional change and should fix the
>    issues we are trying to fix.

Given the above regression, I can't think of a better way to solve this.

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi,

On 2/14/22 14:42, Mika Westerberg wrote:
> Hi Hans,
> 
> On Mon, Feb 14, 2022 at 01:42:29PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/10/22 07:39, Mika Westerberg wrote:
>>> Hi Hans,
>>>
>>> On Wed, Feb 09, 2022 at 05:08:13PM +0100, Hans de Goede wrote:
>>>> As mentioned in my email from 10 seconds ago I think a better simpler
>>>> fix would be to just do:
>>>>
>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>>> index 9b9fb7882c20..18656f823764 100644
>>>> --- a/arch/x86/kernel/resource.c
>>>> +++ b/arch/x86/kernel/resource.c
>>>> @@ -28,6 +28,10 @@ static void remove_e820_regions(struct resource *avail)
>>>>  	int i;
>>>>  	struct e820_entry *entry;
>>>>  
>>>> +	/* Only remove E820 reservations on classic BIOS boot */
>>>> +	if (efi_enabled(EFI_MEMMAP))
>>>> +		return;
>>>> +
>>>>  	for (i = 0; i < e820_table->nr_entries; i++) {
>>>>  		entry = &e820_table->entries[i];
>>>>  
>>>>
>>>> I'm curious what you think of that?
>>>
>>> I'm not an expert in this e820 stuff but this one looks really simple
>>> and makes sense to me. So definitely should go with it assuming there
>>> are no objections from the x86 maintainers.
>>
>> Unfortunately with this suspend/resume is still broken on the ThinkPad
>> X1 carbon gen 2 of the reporter reporting the regression. The reporter
>> has been kind enough to also test in EFI mode (at my request) and then
>> the problem is back again with this patch. So just differentiating
>> between EFI / non EFI mode is not an option.
> 
> Thanks for the update! Too bad that it did not solve the regression, though :(
> 
>> FYI, here is what I believe is the root-cause of the issue on the ThinkPad X1 carbon gen 2:
>>
>> The E820 reservations table has the following in both BIOS and EFI boot modes:
>>
>> [    0.000000] BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved
>>
>> Which has a small overlap with:
>>
>> [    0.884684] pci_bus 0000:00: root bus resource [mem 0xdfa00000-0xfebfffff window]
>>
>> This leads to the following difference in assignments of PCI resources when honoring E820 reservations
>>
>> [    0.966573] pci 0000:00:1c.0: BAR 14: assigned [mem 0xdfb00000-0xdfcfffff]
>> [    0.966698] pci_bus 0000:02: resource 1 [mem 0xdfb00000-0xdfcfffff]
>>
>> vs the following when ignoring E820 reservations:
>>
>> [    0.966850] pci 0000:00:1c.0: BAR 14: assigned [mem 0xdfa00000-0xdfbfffff]
>> [    0.966973] pci_bus 0000:02: resource 1 [mem 0xdfa00000-0xdfbfffff]
>>
>> And the overlap of 0xdfa00000-0xdfa0ffff from the e820 reservations seems to be what is causing the suspend/resume issue.
> 
> Any idea what is using that range?

No, no clue I'm afraid.

>> ###
>>
>> As already somewhat discussed, I'll go and prepare this solution instead:
>>
>> 1. Add E820_TYPE_MMIO to enum e820_type and modify the 2 places which check for
>>    type == reserved to treat this as reserved too, so as to not have any
>>    functional changes there
>>
>> 2. Modify the code building e820 tables from the EFI memmap to use
>>    E820_TYPE_MMIO for MMIO EFI memmap entries.
>>
>> 3. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
>>    e820 table entries with a type of E820_TYPE_MMIO,
>>    this would actually be a functional change and should fix the
>>    issues we are trying to fix.
> 
> Given the above regression, I can't think of a better way to solve this.

Ack, note I'm still waiting for efi=debug output from the X1 carbon gen 2,
so I hope that what seems to be the conflicting range is not also marked
as EFI_MEMORY_MAPPED_IO. Otherwise things will get a bit more complicated (*)

Regards,

Hans

*) On the systems where the EFI_MEMORY_MAPPED_IO memmap entries are causing
issues they fully overlap the PCI bridge window, so we can use that as an
extra check if necessary.




> 

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Hans de Goede]

Hi,

On 2/9/22 16:12, Hans de Goede wrote:
> Hi,
> 
> On 2/8/22 17:38, Mika Westerberg wrote:
>> Hi Hans,
>>
>> On Tue, Feb 08, 2022 at 04:59:13PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 2/8/22 16:25, Hans de Goede wrote:
>>>> Hi All,
>>>>
>>>> Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
>>>> Ignore E820 reservations for bridge windows on newer systems"):
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
>>>>
>>>> breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
>>>> X1 gen 2, see:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
>>
>> :-(
>>
>>>> This regression was actually caught be Fedora already carrying this
>>>> patch for a while now and as such it has been reproduced with 5.15
>>>> with an older version of the patch which still allowed turning the
>>>> new behavior of by adding "pci=use_e820". Dmesg output with and
>>>> without the option has just been attached to the bug, I've not
>>>> analyzed this any further yet.
>>>>
>>>> I guess that for now this means that we need to revert commit
>>>> 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
>>>> explaining why this needs to be reverted tomorrow.
>>>>
>>>> More interesting IMHO is finding out another solution. Both the touchpad
>>>> problem which got me looking into this:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>>
>>>> As well as the thunderbolt hotplug issue Mika was looking at:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>>>
>>>> both are cases where we fail to find a memory-window for a
>>>> BAR which has not been setup yet.
>>>>
>>>> So I see a couple of options here:
>>>>
>>>> 1. Detect that the e820 reservations fully cover (one of)
>>>> the PCI bridge main 32 bit memory windows and if that happens
>>>> ignore them. This actually was my first plan when I started
>>>> working on this. In the end I choose the other option
>>>> because Bjorn indicated that in hindsight honoring the e820
>>>> reservations might have been a mistake and maybe we should
>>>> get rid of honoring them all together.
>>>>
>>>> 2. Have a flag which, when we fail to alloc a 32 bit
>>>> (or 64 bit) memory PCI BAR, is set if not already set
>>>> and then retry the alloc. And make the e820 reservation
>>>> carve-out get skipped in this case.
>>>>
>>>> 3. When booting with pci=nocrs as a workaround for
>>>> the touchpad case a 64 but memory window ends up getting
>>>> used. There already is some special handling for some
>>>> AMD bridges where if there are no 64 bit memory Windows
>>>> in the _CRS for the bridge, one gets added. Maybe we need
>>>> to do the same for Intel bridges ?
>>>
>>> 4. It seems that all devices which have issues with allocating
>>> a PCI bar are Ice Lake based; and the model where the ignoring
>>> of e820 reservations has been reported to cause issues is somewhat
>>> old. It is a Haswell, but still getting BIOS updates causing
>>> the BIOS date check to enable the new behavior. So another
>>> solution might be to only ignore e820 reservations on machines
>>> with Intel Ice Lake (and presumably also Tiger Lake) CPUs.
>>>
>>>
>>> 5. It also seems that the troublesome e820 entry on all devices
>>> ends at 0xcfffffff and starts well below 0x8000000 :
>>>
>>> Yoga C940:
>>> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>>
>>> IdeaPad 3 15IIL05:
>>> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>>
>>> Lenovo IdeaPad 5 14IIL05:
>>> [    0.000000] BIOS-e820: [mem 0x000000005bc50000-0x00000000cfffffff] reserved
>>
>> I don't remember the details anymore but looking at the commit log of my
>> "fix" attempt here:
>>
>> https://bugzilla.kernel.org/attachment.cgi?id=287661
>>
>> The EFI memory map actually seems to consists of several entries that somehow
>> are merged by something (I think this is the EFI stub but not sure). Booting
>> with "efi=debug" may help us to understand this further (or not).
>>
>> On that Yoga system, this:
>>
>>   [Reserved           |   |  |  |  |  |  |  | |   |WB|WT|WC|UC] range=[0x000000002bc50000-0x000000003fffffff] (323MB)
>>   [Reserved           |   |  |  |  |  |  |  | |   |WB|  |  |UC] range=[0x0000000040000000-0x0000000040ffffff] (16MB)
>>   [Reserved           |   |  |  |  |  |  |  | |   |  |  |  |  ] range=[0x0000000041000000-0x00000000453fffff] (68MB)
>>   [Memory Mapped I/O  |RUN|  |  |  |  |  |  | |   |  |  |  |UC] range=[0x0000000045400000-0x00000000cfffffff] (2220MB)
>>
>> became this:
>>
>>   BIOS-e820: [mem 0x000000002bc50000-0x00000000cfffffff] reserved
>>
>> Since the area (0x45400000-0xcfffffff) is marked as MMIO I think maybe we can
>> simply skip those areas in arch_remove_reservations() or so?
>>
>> I may be missing a lots of details, though. ;-)
> 
> So I've been looking at this for a couple of hours now and I've
> basically re-invented your original fix from:
> 
> https://lkml.org/lkml/2020/6/17/751
> 
> So here we are 2 years later and that still looks like the best
> fix, so IMHO we should just go with that fix.
> 
> An alternative, much more elaborate fix would be to:
> 
> 1. Add E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW types to
>    enum e820_type and modify the 2 places which check for type == reserved
>    to treat these both as reserved too, so as to not have any functional
>    changes there
> 
> 2. Modify the code building e820 tables from the EFI memmap to use
>    E820_TYPE_MMIO for MMIO EFI memmap entries and make e820_nomerge()
>    treat E820_TYPE_MMIO as non mergable to retain these as is
> 
> 3. Modify pci_acpi_root_prepare_resources() to compare ACPI provided
>    bridge mmio windows against E820_TYPE_MMIO e820_Table entries and
>    if there is an exact match (as is the case on the Yoga C940)
>    then change the e820 type to E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
> 
>    This means that we are now very narrowly treating EFI MMIO marked
>    regions special, in the special case where they are a 1:1 map
>    to an ACPI PCI bridge window resource.
> 
>    Note since we treat both E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
>    identical to the old RESERVED everywhere there is no functional change
>    here.
> 
> 4. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
>    e820 table entries with a type of E820_TYPE_MMIO_PCI_BRIDGE_WINDOW,
>    this would actually be a functional change and should fix the
>    issues we are trying to fix.
> 
> Note an alternative would be to skip having the extra E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
> type and to skip step 3. above. That would boil down to doing the same
> as your original patch in a somewhat cleaner manner.

So I just went outside for a stroll and realized that:

1. The touchpad and thunderbolt hotplug issues are all happening on
devices booting in EFI mode.

2. The X1 carbon gen 2 from the regression is booting in classic BIOS mode

3. The old devices for which the remove_e820_regions() was originally added
to resolve an issue where there was overlap between the PCI bridge mmio
window and RAM are so old that they only supports classic BIOS boot

4. The problem which we are seeing is coming from the EFI memmap to
e820 reservation table which is only done when booting in EFI mode.

So I believe that there actually is a pretty simple and straight forward
fix for this:

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..18656f823764 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -28,6 +28,10 @@ static void remove_e820_regions(struct resource *avail)
 	int i;
 	struct e820_entry *entry;
 
+	/* Only remove E820 reservations on classic BIOS boot */
+	if (efi_enabled(EFI_MEMMAP))
+		return;
+
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
 
I'll go and ask the reporters of both the regression as well as of the
original touchpad issue to test this.

Regards,

Hans

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

[Thorsten Leemhuis]

[TLDR: I'm adding the regression report below to regzbot, the Linux
kernel regression tracking bot; all text you find below is compiled from
a few templates paragraphs you might have encountered already already
from similar mails.]

Hi, this is your Linux kernel regression tracker speaking.

CCing the regression mailing list, as it should be in the loop for all
regressions, as explained here:
https://www.kernel.org/doc/html/latest/admin-guide/reporting-issues.html

To be sure this issue doesn't fall through the cracks unnoticed, I'm
adding it to regzbot, my Linux kernel regression tracking bot:

#regzbot ^introduced 7f7b4236f204
#regzbot title x86/PCI: "x86/PCI: Ignore E820 reservations for bridge
windows on newer systems" breaks suspend/resume
#regzbot ignore-activity

Reminder for developers: when fixing the issue, please add a 'Link:'
tags pointing to the report (the mail quoted above) using
lore.kernel.org/r/, as explained in
'Documentation/process/submitting-patches.rst' and
'Documentation/process/5.Posting.rst'. This allows the bot to connect
the report with any patches posted or committed to fix the issue; this
again allows the bot to show the current status of regressions and
automatically resolve the issue when the fix hits the right tree.

I'm sending this to everyone that got the initial report, to make them
aware of the tracking. I also hope that messages like this motivate
people to directly get at least the regression mailing list and ideally
even regzbot involved when dealing with regressions, as messages like
this wouldn't be needed then.

Don't worry, I'll send further messages wrt to this regression just to
the lists (with a tag in the subject so people can filter them away), if
they are relevant just for regzbot. With a bit of luck no such messages
will be needed anyway.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I'm getting a lot of
reports on my table. I can only look briefly into most of them and lack
knowledge about most of the areas they concern. I thus unfortunately
will sometimes get things wrong or miss something important. I hope
that's not the case here; if you think it is, don't hesitate to tell me
in a public reply, it's in everyone's interest to set the public record
straight.

On 08.02.22 16:25, Hans de Goede wrote:
> Hi All,
> 
> Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
> Ignore E820 reservations for bridge windows on newer systems"):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
> 
> breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
> X1 gen 2, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
> 
> This regression was actually caught be Fedora already carrying this
> patch for a while now and as such it has been reproduced with 5.15
> with an older version of the patch which still allowed turning the
> new behavior of by adding "pci=use_e820". Dmesg output with and
> without the option has just been attached to the bug, I've not
> analyzed this any further yet.
> 
> I guess that for now this means that we need to revert commit
> 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
> explaining why this needs to be reverted tomorrow.
> 
> More interesting IMHO is finding out another solution. Both the touchpad
> problem which got me looking into this:
> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> 
> As well as the thunderbolt hotplug issue Mika was looking at:
> https://bugzilla.kernel.org/show_bug.cgi?id=206459
> 
> both are cases where we fail to find a memory-window for a
> BAR which has not been setup yet.
> 
> So I see a couple of options here:
> 
> 1. Detect that the e820 reservations fully cover (one of)
> the PCI bridge main 32 bit memory windows and if that happens
> ignore them. This actually was my first plan when I started
> working on this. In the end I choose the other option
> because Bjorn indicated that in hindsight honoring the e820
> reservations might have been a mistake and maybe we should
> get rid of honoring them all together.
> 
> 2. Have a flag which, when we fail to alloc a 32 bit
> (or 64 bit) memory PCI BAR, is set if not already set
> and then retry the alloc. And make the e820 reservation
> carve-out get skipped in this case.
> 
> 3. When booting with pci=nocrs as a workaround for
> the touchpad case a 64 but memory window ends up getting
> used. There already is some special handling for some
> AMD bridges where if there are no 64 bit memory Windows
> in the _CRS for the bridge, one gets added. Maybe we need
> to do the same for Intel bridges ?
> 
> Please let me know which of these options you think I should
> try to implement next; of course alternative ideas for fixing
> this are also welcome.
> 
> Regards,
> 
> Hans
> 

-- 
Additional information about regzbot:

If you want to know more about regzbot, check out its web-interface, the
getting start guide, and the references documentation:

https://linux-regtracking.leemhuis.info/regzbot/
https://gitlab.com/knurd42/regzbot/-/blob/main/docs/getting_started.md
https://gitlab.com/knurd42/regzbot/-/blob/main/docs/reference.md

The last two documents will explain how you can interact with regzbot
yourself if your want to.

Hint for reporters: when reporting a regression it's in your interest to
CC the regression list and tell regzbot about the issue, as that ensures
the regression makes it onto the radar of the Linux kernel's regression
tracker -- that's in your interest, as it ensures your report won't fall
through the cracks unnoticed.

Hint for developers: you normally don't need to care about regzbot once
it's involved. Fix the issue as you normally would, just remember to
include 'Link:' tag in the patch descriptions pointing to all reports
about the issue. This has been expected from developers even before
regzbot showed up for reasons explained in
'Documentation/process/submitting-patches.rst' and
'Documentation/process/5.Posting.rst'.

Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume #forregzbot

[Thorsten Leemhuis]

Hi, this is your Linux kernel regression tracker speaking. Top-posting
for once, to make this easy accessible to everyone.

Thx for fixing this. Sadly the fix for the regression discussed didn't
in this thread link to the report (e.g. the mail that started this
thread), as 'Documentation/process/submitting-patches.rst' and
'Documentation/process/5.Posting.rst' demand. So I have to tell regzbot
about the fix manually now:

#regzbot fixed-by: 3eb616b26408ac81
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I'm getting a lot of
reports on my table. I can only look briefly into most of them and lack
knowledge about most of the areas they concern. I thus unfortunately
will sometimes get things wrong or miss something important. I hope
that's not the case here; if you think it is, don't hesitate to tell me
in a public reply, it's in everyone's interest to set the public record
straight.



On 09.02.22 10:13, Thorsten Leemhuis wrote:
> [TLDR: I'm adding the regression report below to regzbot, the Linux
> kernel regression tracking bot; all text you find below is compiled from
> a few templates paragraphs you might have encountered already already
> from similar mails.]
> 
> Hi, this is your Linux kernel regression tracker speaking.
> 
> CCing the regression mailing list, as it should be in the loop for all
> regressions, as explained here:
> https://www.kernel.org/doc/html/latest/admin-guide/reporting-issues.html
> 
> To be sure this issue doesn't fall through the cracks unnoticed, I'm
> adding it to regzbot, my Linux kernel regression tracking bot:
> 
> #regzbot ^introduced 7f7b4236f204
> #regzbot title x86/PCI: "x86/PCI: Ignore E820 reservations for bridge
> windows on newer systems" breaks suspend/resume
> #regzbot ignore-activity
> 
> Reminder for developers: when fixing the issue, please add a 'Link:'
> tags pointing to the report (the mail quoted above) using
> lore.kernel.org/r/, as explained in
> 'Documentation/process/submitting-patches.rst' and
> 'Documentation/process/5.Posting.rst'. This allows the bot to connect
> the report with any patches posted or committed to fix the issue; this
> again allows the bot to show the current status of regressions and
> automatically resolve the issue when the fix hits the right tree.
> 
> I'm sending this to everyone that got the initial report, to make them
> aware of the tracking. I also hope that messages like this motivate
> people to directly get at least the regression mailing list and ideally
> even regzbot involved when dealing with regressions, as messages like
> this wouldn't be needed then.
> 
> Don't worry, I'll send further messages wrt to this regression just to
> the lists (with a tag in the subject so people can filter them away), if
> they are relevant just for regzbot. With a bit of luck no such messages
> will be needed anyway.
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> 
> P.S.: As the Linux kernel's regression tracker I'm getting a lot of
> reports on my table. I can only look briefly into most of them and lack
> knowledge about most of the areas they concern. I thus unfortunately
> will sometimes get things wrong or miss something important. I hope
> that's not the case here; if you think it is, don't hesitate to tell me
> in a public reply, it's in everyone's interest to set the public record
> straight.
> 
> On 08.02.22 16:25, Hans de Goede wrote:
>> Hi All,
>>
>> Unfortunately I've just learned that commit 7f7b4236f204 ("x86/PCI:
>> Ignore E820 reservations for bridge windows on newer systems"):
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f7b4236f2040d19df1ddaf30047128b41e78de7
>>
>> breaks suspend/resume on at least one laptop model, the Lenovo ThinkPad
>> X1 gen 2, see:
>> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
>>
>> This regression was actually caught be Fedora already carrying this
>> patch for a while now and as such it has been reproduced with 5.15
>> with an older version of the patch which still allowed turning the
>> new behavior of by adding "pci=use_e820". Dmesg output with and
>> without the option has just been attached to the bug, I've not
>> analyzed this any further yet.
>>
>> I guess that for now this means that we need to revert commit
>> 7f7b4236f204. Rafael, I'll send you a revert with a commit msg
>> explaining why this needs to be reverted tomorrow.
>>
>> More interesting IMHO is finding out another solution. Both the touchpad
>> problem which got me looking into this:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>
>> As well as the thunderbolt hotplug issue Mika was looking at:
>> https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>
>> both are cases where we fail to find a memory-window for a
>> BAR which has not been setup yet.
>>
>> So I see a couple of options here:
>>
>> 1. Detect that the e820 reservations fully cover (one of)
>> the PCI bridge main 32 bit memory windows and if that happens
>> ignore them. This actually was my first plan when I started
>> working on this. In the end I choose the other option
>> because Bjorn indicated that in hindsight honoring the e820
>> reservations might have been a mistake and maybe we should
>> get rid of honoring them all together.
>>
>> 2. Have a flag which, when we fail to alloc a 32 bit
>> (or 64 bit) memory PCI BAR, is set if not already set
>> and then retry the alloc. And make the e820 reservation
>> carve-out get skipped in this case.
>>
>> 3. When booting with pci=nocrs as a workaround for
>> the touchpad case a 64 but memory window ends up getting
>> used. There already is some special handling for some
>> AMD bridges where if there are no 64 bit memory Windows
>> in the _CRS for the bridge, one gets added. Maybe we need
>> to do the same for Intel bridges ?
>>
>> Please let me know which of these options you think I should
>> try to implement next; of course alternative ideas for fixing
>> this are also welcome.
>>
>> Regards,
>>
>> Hans
>>
>