[PATCH v2 0/3] x86/PCI: Log E820 clipping

[PATCH v2 0/3] x86/PCI: Log E820 clipping

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is still work-in-progress on the issue of PNP0A03 _CRS methods that
are buggy or not interpreted correctly by Linux.

The previous try at:
  https://lore.kernel.org/r/20220304035110.988712-1-helgaas@kernel.org
caused regressions on some Chromebooks:
  https://lore.kernel.org/r/Yjyv03JsetIsTJxN@sirena.org.uk

This v2 drops the commit that caused the Chromebook regression, so it also
doesn't fix the issue we were *trying* to fix on Lenovo Yoga and Clevo
Barebones.

The point of this v2 update is to split the logging patch into (1) a pure
logging addition and (2) the change to only clip PCI windows, which was
previously hidden inside the logging patch and not well documented.

Bjorn Helgaas (3):
  x86/PCI: Eliminate remove_e820_regions() common subexpressions
  x86: Log resource clipping for E820 regions
  x86/PCI: Clip only host bridge windows for E820 regions

 arch/x86/include/asm/e820/api.h |  5 +++++
 arch/x86/kernel/resource.c      | 23 ++++++++++++++++-------
 arch/x86/pci/acpi.c             |  5 +++++
 3 files changed, 26 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] x86/PCI: Eliminate remove_e820_regions() common subexpressions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add local variables to reduce repetition later.  No functional change
intended.

Link: https://lore.kernel.org/r/20220304035110.988712-2-helgaas@kernel.org
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/x86/kernel/resource.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..8ffe68437744 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -27,12 +27,14 @@ static void remove_e820_regions(struct resource *avail)
 {
 	int i;
 	struct e820_entry *entry;
+	u64 e820_start, e820_end;
 
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
+		e820_start = entry->addr;
+		e820_end = entry->addr + entry->size - 1;
 
-		resource_clip(avail, entry->addr,
-			      entry->addr + entry->size - 1);
+		resource_clip(avail, e820_start, e820_end);
 	}
 }
 
-- 
2.25.1

[PATCH v2 2/3] x86: Log resource clipping for E820 regions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

When remove_e820_regions() clips a resource because an E820 region overlaps
it, log a note in dmesg to add in debugging.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/x86/kernel/resource.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 8ffe68437744..30d524adb012 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/ioport.h>
+#include <linux/printk.h>
 #include <asm/e820/api.h>
 
 static void resource_clip(struct resource *res, resource_size_t start,
@@ -28,6 +29,7 @@ static void remove_e820_regions(struct resource *avail)
 	int i;
 	struct e820_entry *entry;
 	u64 e820_start, e820_end;
+	struct resource orig = *avail;
 
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
@@ -35,6 +37,11 @@ static void remove_e820_regions(struct resource *avail)
 		e820_end = entry->addr + entry->size - 1;
 
 		resource_clip(avail, e820_start, e820_end);
+		if (orig.start != avail->start || orig.end != avail->end) {
+			pr_info("clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
+				 &orig, avail, e820_start, e820_end);
+			orig = *avail;
+		}
 	}
 }
 
-- 
2.25.1

Re: [PATCH v2 2/3] x86: Log resource clipping for E820 regions

[Rafael J. Wysocki]

On Thu, Apr 14, 2022 at 8:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> When remove_e820_regions() clips a resource because an E820 region overlaps
> it, log a note in dmesg to add in debugging.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

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

> ---
>  arch/x86/kernel/resource.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> index 8ffe68437744..30d524adb012 100644
> --- a/arch/x86/kernel/resource.c
> +++ b/arch/x86/kernel/resource.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/ioport.h>
> +#include <linux/printk.h>
>  #include <asm/e820/api.h>
>
>  static void resource_clip(struct resource *res, resource_size_t start,
> @@ -28,6 +29,7 @@ static void remove_e820_regions(struct resource *avail)
>         int i;
>         struct e820_entry *entry;
>         u64 e820_start, e820_end;
> +       struct resource orig = *avail;
>
>         for (i = 0; i < e820_table->nr_entries; i++) {
>                 entry = &e820_table->entries[i];
> @@ -35,6 +37,11 @@ static void remove_e820_regions(struct resource *avail)
>                 e820_end = entry->addr + entry->size - 1;
>
>                 resource_clip(avail, e820_start, e820_end);
> +               if (orig.start != avail->start || orig.end != avail->end) {
> +                       pr_info("clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
> +                                &orig, avail, e820_start, e820_end);
> +                       orig = *avail;
> +               }
>         }
>  }
>
> --
> 2.25.1
>

[PATCH v2 3/3] x86/PCI: Clip only host bridge windows for E820 regions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

ACPI firmware advertises PCI host bridge resources via PNP0A03 _CRS
methods.  Some BIOSes include non-window address space in _CRS, and if we
allocate that non-window space for PCI devices, they don't work.

4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
works around this issue by clipping out any regions mentioned in the E820
table in the allocate_resource() path, but the implementation has a couple
issues:

  - The clipping is done for *all* allocations, not just those for PCI
    address space, and

  - The clipping is done at each allocation instead of being done once when
    setting up the host bridge windows.

Rework the implementation so we only clip PCI host bridge windows, and we
do it once when setting them up.

Example output changes:

    BIOS-e820: [mem 0x00000000b0000000-0x00000000c00fffff] reserved
  + acpi PNP0A08:00: clipped [mem 0xc0000000-0xfebfffff window] to [mem 0xc0100000-0xfebfffff window] for e820 entry [mem 0xb0000000-0xc00fffff]
  - pci_bus 0000:00: root bus resource [mem 0xc0000000-0xfebfffff window]
  + pci_bus 0000:00: root bus resource [mem 0xc0100000-0xfebfffff window]

Link: https://lore.kernel.org/r/20220304035110.988712-3-helgaas@kernel.org
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/x86/include/asm/e820/api.h |  5 +++++
 arch/x86/kernel/resource.c      | 14 +++++++-------
 arch/x86/pci/acpi.c             |  5 +++++
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index e8f58ddd06d9..5a39ed59b6db 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -4,6 +4,9 @@
 
 #include <asm/e820/types.h>
 
+struct device;
+struct resource;
+
 extern struct e820_table *e820_table;
 extern struct e820_table *e820_table_kexec;
 extern struct e820_table *e820_table_firmware;
@@ -43,6 +46,8 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn);
 
 extern int  e820__get_entry_type(u64 start, u64 end);
 
+extern void remove_e820_regions(struct device *dev, struct resource *avail);
+
 /*
  * Returns true iff the specified range [start,end) is completely contained inside
  * the ISA region.
diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 30d524adb012..db2b350a37b7 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/dev_printk.h>
 #include <linux/ioport.h>
-#include <linux/printk.h>
 #include <asm/e820/api.h>
 
 static void resource_clip(struct resource *res, resource_size_t start,
@@ -24,13 +24,16 @@ static void resource_clip(struct resource *res, resource_size_t start,
 		res->start = end + 1;
 }
 
-static void remove_e820_regions(struct resource *avail)
+void remove_e820_regions(struct device *dev, struct resource *avail)
 {
 	int i;
 	struct e820_entry *entry;
 	u64 e820_start, e820_end;
 	struct resource orig = *avail;
 
+	if (!(avail->flags & IORESOURCE_MEM))
+		return;
+
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
 		e820_start = entry->addr;
@@ -38,7 +41,7 @@ static void remove_e820_regions(struct resource *avail)
 
 		resource_clip(avail, e820_start, e820_end);
 		if (orig.start != avail->start || orig.end != avail->end) {
-			pr_info("clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
+			dev_info(dev, "clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
 				 &orig, avail, e820_start, e820_end);
 			orig = *avail;
 		}
@@ -52,9 +55,6 @@ void arch_remove_reservations(struct resource *avail)
 	 * the low 1MB unconditionally, as this area is needed for some ISA
 	 * cards requiring a memory range, e.g. the i82365 PCMCIA controller.
 	 */
-	if (avail->flags & IORESOURCE_MEM) {
+	if (avail->flags & IORESOURCE_MEM)
 		resource_clip(avail, BIOS_ROM_BASE, BIOS_ROM_END);
-
-		remove_e820_regions(avail);
-	}
 }
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 052f1d78a562..562c81a51ea0 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -8,6 +8,7 @@
 #include <linux/pci-acpi.h>
 #include <asm/numa.h>
 #include <asm/pci_x86.h>
+#include <asm/e820/api.h>
 
 struct pci_root_info {
 	struct acpi_pci_root_info common;
@@ -299,6 +300,10 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 	int status;
 
 	status = acpi_pci_probe_root_resources(ci);
+
+	resource_list_for_each_entry(entry, &ci->resources)
+		remove_e820_regions(&device->dev, entry->res);
+
 	if (pci_use_crs) {
 		resource_list_for_each_entry_safe(entry, tmp, &ci->resources)
 			if (resource_is_pcicfg_ioport(entry->res))
-- 
2.25.1

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Hans de Goede]

Hi All,

On 1/1/70 01:00, Bjorn Helgaas wrote:
> This is still work-in-progress on the issue of PNP0A03 _CRS methods that
> are buggy or not interpreted correctly by Linux.
> 
> The previous try at:
>   https://lore.kernel.org/r/20220304035110.988712-1-helgaas@kernel.org
> caused regressions on some Chromebooks:
>   https://lore.kernel.org/r/Yjyv03JsetIsTJxN@sirena.org.uk
> 
> This v2 drops the commit that caused the Chromebook regression, so it also
> doesn't fix the issue we were *trying* to fix on Lenovo Yoga and Clevo
> Barebones.
> 
> The point of this v2 update is to split the logging patch into (1) a pure
> logging addition and (2) the change to only clip PCI windows, which was
> previously hidden inside the logging patch and not well documented.
> 
> Bjorn Helgaas (3):
>   x86/PCI: Eliminate remove_e820_regions() common subexpressions
>   x86: Log resource clipping for E820 regions
>   x86/PCI: Clip only host bridge windows for E820 regions

Thanks, the entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

For the series.

So what is the plan to actually fix the issue seen on some Lenovo models
and Clevo Barebones ?   As I mentioned previously I think that since all
our efforts have failed so far that we should maybe reconsider just
using DMI quirks to ignore the E820 reservation windows for host bridges
on affected models ?

That or implement 3 of:

> So I think the progression is:
>
>  1) Remove anything mentioned in E820 from _CRS (4dc2287c1805 [7]).
>     This worked around some issues on Dell systems.
>
>  2) Remove things mentioned in E820 unless they cover the entire
>     window (5949965ec934 [8])
>
>  3) Coalesce adjacent _CRS windows, *then* remove things mentioned in
>     E820 unless they cover the entire (coalesced) window (current
>     proposal)

Regards,

Hans

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Bjorn Helgaas]

On Tue, Apr 19, 2022 at 11:59:17AM +0200, Hans de Goede wrote:
> On 1/1/70 01:00, Bjorn Helgaas wrote:
> > This is still work-in-progress on the issue of PNP0A03 _CRS methods that
> > are buggy or not interpreted correctly by Linux.
> > 
> > The previous try at:
> >   https://lore.kernel.org/r/20220304035110.988712-1-helgaas@kernel.org
> > caused regressions on some Chromebooks:
> >   https://lore.kernel.org/r/Yjyv03JsetIsTJxN@sirena.org.uk
> > 
> > This v2 drops the commit that caused the Chromebook regression, so it also
> > doesn't fix the issue we were *trying* to fix on Lenovo Yoga and Clevo
> > Barebones.
> > 
> > The point of this v2 update is to split the logging patch into (1) a pure
> > logging addition and (2) the change to only clip PCI windows, which was
> > previously hidden inside the logging patch and not well documented.
> > 
> > Bjorn Helgaas (3):
> >   x86/PCI: Eliminate remove_e820_regions() common subexpressions
> >   x86: Log resource clipping for E820 regions
> >   x86/PCI: Clip only host bridge windows for E820 regions
> 
> Thanks, the entire series looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thank you!

> So what is the plan to actually fix the issue seen on some Lenovo models
> and Clevo Barebones ?   As I mentioned previously I think that since all
> our efforts have failed so far that we should maybe reconsider just
> using DMI quirks to ignore the E820 reservation windows for host bridges
> on affected models ?

I have been resisting DMI quirks but I'm afraid there's no other way.
I think the web we've gotten into, where vendors have used E820 to
interact with _CRS in incompatible and undocumented ways, is not
sustainable.

I'm not aware of any spec that says the OS should use E820 to clip
things out of _CRS, so I think the long term plan should be to
decouple them by default.

Straw man:

  - Disable E820 clipping by default.

  - Add a quirk to enable E820 clipping for machines older than X,
    e.g., 2023, to avoid breaking machines that currently work.

  - Add quirks to disable E820 clipping for individual machines like
    the Lenovo and Clevos that predate X, but E820 clipping breaks
    them.

  - Add quirks to enable E820 clipping for individual machines like
    the Chromebooks (and probably machines we don't know about yet)
    that have devices that consume part of _CRS but are not
    enumerable.

  - Communicate this to OEMs to try to prevent future machines that
    need quirks.

Bjorn

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Hans de Goede]

Hi,

On 4/19/22 17:03, Bjorn Helgaas wrote:
> On Tue, Apr 19, 2022 at 11:59:17AM +0200, Hans de Goede wrote:
>> On 1/1/70 01:00, Bjorn Helgaas wrote:
>>> This is still work-in-progress on the issue of PNP0A03 _CRS methods that
>>> are buggy or not interpreted correctly by Linux.
>>>
>>> The previous try at:
>>>   https://lore.kernel.org/r/20220304035110.988712-1-helgaas@kernel.org
>>> caused regressions on some Chromebooks:
>>>   https://lore.kernel.org/r/Yjyv03JsetIsTJxN@sirena.org.uk
>>>
>>> This v2 drops the commit that caused the Chromebook regression, so it also
>>> doesn't fix the issue we were *trying* to fix on Lenovo Yoga and Clevo
>>> Barebones.
>>>
>>> The point of this v2 update is to split the logging patch into (1) a pure
>>> logging addition and (2) the change to only clip PCI windows, which was
>>> previously hidden inside the logging patch and not well documented.
>>>
>>> Bjorn Helgaas (3):
>>>   x86/PCI: Eliminate remove_e820_regions() common subexpressions
>>>   x86: Log resource clipping for E820 regions
>>>   x86/PCI: Clip only host bridge windows for E820 regions
>>
>> Thanks, the entire series looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thank you!
> 
>> So what is the plan to actually fix the issue seen on some Lenovo models
>> and Clevo Barebones ?   As I mentioned previously I think that since all
>> our efforts have failed so far that we should maybe reconsider just
>> using DMI quirks to ignore the E820 reservation windows for host bridges
>> on affected models ?
> 
> I have been resisting DMI quirks but I'm afraid there's no other way.

Well there is the first match adjacent windows returned by _CRS and
only then do the "covers whole region" exception check. I still
think that would work at least for the chromebook regression...

So do you want me to give that a try; or shall I write a patch
using DMI quirks. And if we go the DMI quirks, what about
matching cmdline arguments?  If we add matching cmdline arguments,
which seems to be the sensible thing to do then to allow users
to test if they need the quirk, then we basically end up with my
first attempt at fixing this from 6 months ago:

https://lore.kernel.org/linux-pci/20211005150956.303707-1-hdegoede@redhat.com/

> I think the web we've gotten into, where vendors have used E820 to
> interact with _CRS in incompatible and undocumented ways, is not
> sustainable.
> 
> I'm not aware of any spec that says the OS should use E820 to clip
> things out of _CRS, so I think the long term plan should be to
> decouple them by default.

Right and AFAICT the reason Windows is getting away with this is
the same as with the original Dell _CRS has overlap with
physical RAM issue (1), Linux assigns address to unassigneds BAR-s
starting with the lowest available address in the bridge window,
where as Windows assigns addresses from the highest available
address in the window. So the real fix here might very well be
to rework the BAR assignment code to switch to fill the window
from the top rather then from the bottom. AFAICT all issues where
excluding _E820 reservations have helped are with _E820 - bridge
window overlaps at the bottom of the window.

IOW these are really all bugs in the _CRS method for the bridge,
which Windows does not hit because it never actually uses
the lowest address(es) of the _CRS returned window.

Regards,

Hans



1) At least I read in either a bugzilla, or email thread about
this that Windows allocating bridge window space from the top
was assumed to be why Windows was not impacted.





> Straw man:
> 
>   - Disable E820 clipping by default.
> 
>   - Add a quirk to enable E820 clipping for machines older than X,
>     e.g., 2023, to avoid breaking machines that currently work.
> 
>   - Add quirks to disable E820 clipping for individual machines like
>     the Lenovo and Clevos that predate X, but E820 clipping breaks
>     them.
> 
>   - Add quirks to enable E820 clipping for individual machines like
>     the Chromebooks (and probably machines we don't know about yet)
>     that have devices that consume part of _CRS but are not
>     enumerable.
> 
>   - Communicate this to OEMs to try to prevent future machines that
>     need quirks.
> 
> Bjorn
> 

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Bjorn Helgaas]

On Tue, Apr 19, 2022 at 05:16:44PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 4/19/22 17:03, Bjorn Helgaas wrote:
> > On Tue, Apr 19, 2022 at 11:59:17AM +0200, Hans de Goede wrote:
> >> On 1/1/70 01:00, Bjorn Helgaas wrote:
> >>> This is still work-in-progress on the issue of PNP0A03 _CRS methods that
> >>> are buggy or not interpreted correctly by Linux.
> >>>
> >>> The previous try at:
> >>>   https://lore.kernel.org/r/20220304035110.988712-1-helgaas@kernel.org
> >>> caused regressions on some Chromebooks:
> >>>   https://lore.kernel.org/r/Yjyv03JsetIsTJxN@sirena.org.uk
> >>>
> >>> This v2 drops the commit that caused the Chromebook regression, so it also
> >>> doesn't fix the issue we were *trying* to fix on Lenovo Yoga and Clevo
> >>> Barebones.
> >>>
> >>> The point of this v2 update is to split the logging patch into (1) a pure
> >>> logging addition and (2) the change to only clip PCI windows, which was
> >>> previously hidden inside the logging patch and not well documented.
> >>>
> >>> Bjorn Helgaas (3):
> >>>   x86/PCI: Eliminate remove_e820_regions() common subexpressions
> >>>   x86: Log resource clipping for E820 regions
> >>>   x86/PCI: Clip only host bridge windows for E820 regions
> >>
> >> Thanks, the entire series looks good to me:
> >>
> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Thank you!
> > 
> >> So what is the plan to actually fix the issue seen on some Lenovo models
> >> and Clevo Barebones ?   As I mentioned previously I think that since all
> >> our efforts have failed so far that we should maybe reconsider just
> >> using DMI quirks to ignore the E820 reservation windows for host bridges
> >> on affected models ?
> > 
> > I have been resisting DMI quirks but I'm afraid there's no other way.
> 
> Well there is the first match adjacent windows returned by _CRS and
> only then do the "covers whole region" exception check. I still
> think that would work at least for the chromebook regression...

Without a crystal clear strategy, I think we're going to be tweaking
the algorithm forever as the _CRS/E820 mix changes.  That's why I
think that in the long term, a "use _CRS only, with quirks for
exceptions" strategy will be simplest.

> So do you want me to give that a try; or shall I write a patch
> using DMI quirks. And if we go the DMI quirks, what about
> matching cmdline arguments?  If we add matching cmdline arguments,
> which seems to be the sensible thing to do then to allow users
> to test if they need the quirk, then we basically end up with my
> first attempt at fixing this from 6 months ago:
> 
> https://lore.kernel.org/linux-pci/20211005150956.303707-1-hdegoede@redhat.com/

So I think we should go ahead with DMI quirks instead of trying to
make the algorithm smarter, and yes, I think we will need commandline
arguments, probably one to force E820 clipping for future machines,
and one to disable it for old machines.

> > I think the web we've gotten into, where vendors have used E820 to
> > interact with _CRS in incompatible and undocumented ways, is not
> > sustainable.
> > 
> > I'm not aware of any spec that says the OS should use E820 to clip
> > things out of _CRS, so I think the long term plan should be to
> > decouple them by default.
> 
> Right and AFAICT the reason Windows is getting away with this is
> the same as with the original Dell _CRS has overlap with
> physical RAM issue (1), Linux assigns address to unassigneds BAR-s
> starting with the lowest available address in the bridge window,
> where as Windows assigns addresses from the highest available
> address in the window.

Right, I agree.  I'm guessing Chromebooks don't get tested with
Windows at all, so we don't even have that level of testing to help.

> So the real fix here might very well be
> to rework the BAR assignment code to switch to fill the window
> from the top rather then from the bottom. AFAICT all issues where
> excluding _E820 reservations have helped are with _E820 - bridge
> window overlaps at the bottom of the window.
> 
> IOW these are really all bugs in the _CRS method for the bridge,
> which Windows does not hit because it never actually uses
> the lowest address(es) of the _CRS returned window.

Yes.  We actually did try this
(https://git.kernel.org/linus/1af3c2e45e7a), but unfortunately we had
to revert it.  Even more unfortunately, the revert
(https://git.kernel.org/linus/5e52f1c5e85f) doesn't have any details
about what went wrong.

> 1) At least I read in either a bugzilla, or email thread about
> this that Windows allocating bridge window space from the top
> was assumed to be why Windows was not impacted.
> 
> > Straw man:
> > 
> >   - Disable E820 clipping by default.
> > 
> >   - Add a quirk to enable E820 clipping for machines older than X,
> >     e.g., 2023, to avoid breaking machines that currently work.
> > 
> >   - Add quirks to disable E820 clipping for individual machines like
> >     the Lenovo and Clevos that predate X, but E820 clipping breaks
> >     them.
> > 
> >   - Add quirks to enable E820 clipping for individual machines like
> >     the Chromebooks (and probably machines we don't know about yet)
> >     that have devices that consume part of _CRS but are not
> >     enumerable.
> > 
> >   - Communicate this to OEMs to try to prevent future machines that
> >     need quirks.
> > 
> > Bjorn
> > 
> 

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Hans de Goede]

Hi,

Sorry for the late reply.

On 4/19/22 18:45, Bjorn Helgaas wrote:
> On Tue, Apr 19, 2022 at 05:16:44PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 4/19/22 17:03, Bjorn Helgaas wrote:
>>> On Tue, Apr 19, 2022 at 11:59:17AM +0200, Hans de Goede wrote:
>>>> On 1/1/70 01:00, Bjorn Helgaas wrote:
>>>>> This is still work-in-progress on the issue of PNP0A03 _CRS methods that
>>>>> are buggy or not interpreted correctly by Linux.
>>>>>
>>>>> The previous try at:
>>>>>   https://lore.kernel.org/r/20220304035110.988712-1-helgaas@kernel.org
>>>>> caused regressions on some Chromebooks:
>>>>>   https://lore.kernel.org/r/Yjyv03JsetIsTJxN@sirena.org.uk
>>>>>
>>>>> This v2 drops the commit that caused the Chromebook regression, so it also
>>>>> doesn't fix the issue we were *trying* to fix on Lenovo Yoga and Clevo
>>>>> Barebones.
>>>>>
>>>>> The point of this v2 update is to split the logging patch into (1) a pure
>>>>> logging addition and (2) the change to only clip PCI windows, which was
>>>>> previously hidden inside the logging patch and not well documented.
>>>>>
>>>>> Bjorn Helgaas (3):
>>>>>   x86/PCI: Eliminate remove_e820_regions() common subexpressions
>>>>>   x86: Log resource clipping for E820 regions
>>>>>   x86/PCI: Clip only host bridge windows for E820 regions
>>>>
>>>> Thanks, the entire series looks good to me:
>>>>
>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Thank you!
>>>
>>>> So what is the plan to actually fix the issue seen on some Lenovo models
>>>> and Clevo Barebones ?   As I mentioned previously I think that since all
>>>> our efforts have failed so far that we should maybe reconsider just
>>>> using DMI quirks to ignore the E820 reservation windows for host bridges
>>>> on affected models ?
>>>
>>> I have been resisting DMI quirks but I'm afraid there's no other way.
>>
>> Well there is the first match adjacent windows returned by _CRS and
>> only then do the "covers whole region" exception check. I still
>> think that would work at least for the chromebook regression...
> 
> Without a crystal clear strategy, I think we're going to be tweaking
> the algorithm forever as the _CRS/E820 mix changes.  That's why I
> think that in the long term, a "use _CRS only, with quirks for
> exceptions" strategy will be simplest.

Looking at the amount of exception we already now about I'm
not sure if that will work well.


> 
>> So do you want me to give that a try; or shall I write a patch
>> using DMI quirks. And if we go the DMI quirks, what about
>> matching cmdline arguments?  If we add matching cmdline arguments,
>> which seems to be the sensible thing to do then to allow users
>> to test if they need the quirk, then we basically end up with my
>> first attempt at fixing this from 6 months ago:
>>
>> https://lore.kernel.org/linux-pci/20211005150956.303707-1-hdegoede@redhat.com/
> 
> So I think we should go ahead with DMI quirks instead of trying to
> make the algorithm smarter, and yes, I think we will need commandline
> arguments, probably one to force E820 clipping for future machines,
> and one to disable it for old machines.

So what you are suggesting is to go back to a bios-date based approach
(to determine old vs new machines) combined with DMI quirks to force
E820 clipping on new machines which turn out to need it despite them
being new ?

> 
>>> I think the web we've gotten into, where vendors have used E820 to
>>> interact with _CRS in incompatible and undocumented ways, is not
>>> sustainable.
>>>
>>> I'm not aware of any spec that says the OS should use E820 to clip
>>> things out of _CRS, so I think the long term plan should be to
>>> decouple them by default.
>>
>> Right and AFAICT the reason Windows is getting away with this is
>> the same as with the original Dell _CRS has overlap with
>> physical RAM issue (1), Linux assigns address to unassigneds BAR-s
>> starting with the lowest available address in the bridge window,
>> where as Windows assigns addresses from the highest available
>> address in the window.
> 
> Right, I agree.  I'm guessing Chromebooks don't get tested with
> Windows at all, so we don't even have that level of testing to help.
> 
>> So the real fix here might very well be
>> to rework the BAR assignment code to switch to fill the window
>> from the top rather then from the bottom. AFAICT all issues where
>> excluding _E820 reservations have helped are with _E820 - bridge
>> window overlaps at the bottom of the window.
>>
>> IOW these are really all bugs in the _CRS method for the bridge,
>> which Windows does not hit because it never actually uses
>> the lowest address(es) of the _CRS returned window.
> 
> Yes.  We actually did try this
> (https://git.kernel.org/linus/1af3c2e45e7a), but unfortunately we had
> to revert it.  Even more unfortunately, the revert
> (https://git.kernel.org/linus/5e52f1c5e85f) doesn't have any details
> about what went wrong.

When I first started working on this I did read the entire old
email thread and IIRC this approach was reverted because the
e820 based approach was deemed to be a cleaner fix. Also the
single resource_alloc_from_bottom flag influenced all types
of resource allocations, not just PCI host bridge window
allocations.

Note that the current kernel no longer has the resource_alloc_from_bottom
flag. Still I think it might be worthwhile to give switching to
top-down allocating for host bridge window allocs a try. Maybe we
can make the desired allocation strategy a flag in the resource ?

I have the feeling that if we switch to top-down allocating
that we can then switch to just using _CRS and that everything
will then just work, because we then match what Windows is doing...

Regards,

Hans







> 
>> 1) At least I read in either a bugzilla, or email thread about
>> this that Windows allocating bridge window space from the top
>> was assumed to be why Windows was not impacted.
>>
>>> Straw man:
>>>
>>>   - Disable E820 clipping by default.
>>>
>>>   - Add a quirk to enable E820 clipping for machines older than X,
>>>     e.g., 2023, to avoid breaking machines that currently work.
>>>
>>>   - Add quirks to disable E820 clipping for individual machines like
>>>     the Lenovo and Clevos that predate X, but E820 clipping breaks
>>>     them.
>>>
>>>   - Add quirks to enable E820 clipping for individual machines like
>>>     the Chromebooks (and probably machines we don't know about yet)
>>>     that have devices that consume part of _CRS but are not
>>>     enumerable.
>>>
>>>   - Communicate this to OEMs to try to prevent future machines that
>>>     need quirks.
>>>
>>> Bjorn
>>>
>>
> 

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Bjorn Helgaas]

On Mon, May 02, 2022 at 02:24:26PM +0200, Hans de Goede wrote:
> On 4/19/22 18:45, Bjorn Helgaas wrote:
> > On Tue, Apr 19, 2022 at 05:16:44PM +0200, Hans de Goede wrote:
> >> On 4/19/22 17:03, Bjorn Helgaas wrote:
> >>> On Tue, Apr 19, 2022 at 11:59:17AM +0200, Hans de Goede wrote:

> >>>> So what is the plan to actually fix the issue seen on some
> >>>> Lenovo models and Clevo Barebones ?   As I mentioned previously
> >>>> I think that since all our efforts have failed so far that we
> >>>> should maybe reconsider just using DMI quirks to ignore the
> >>>> E820 reservation windows for host bridges on affected models ?
> >>>
> >>> I have been resisting DMI quirks but I'm afraid there's no other
> >>> way.
> >>
> >> Well there is the first match adjacent windows returned by _CRS
> >> and only then do the "covers whole region" exception check. I
> >> still think that would work at least for the chromebook
> >> regression...
> > 
> > Without a crystal clear strategy, I think we're going to be
> > tweaking the algorithm forever as the _CRS/E820 mix changes.
> > That's why I think that in the long term, a "use _CRS only, with
> > quirks for exceptions" strategy will be simplest.
> 
> Looking at the amount of exception we already now about I'm not sure
> if that will work well.

It's possible that many quirks will be required.  But I think in the
long run the value of the simplest, most obvious strategy is huge.
It's laid out in the spec already and it's the clearest way to
agreement between firmware and OS.  When we trip over something, it's
very easy to determine whether _CRS is wrong or Linux is using it
wrong.  If we have to bring in question of looking at E820 entries,
possibly merging them, using them or not based on overlaps ... that's
a much more difficult conversation without a clear resolution.

> > So I think we should go ahead with DMI quirks instead of trying to
> > make the algorithm smarter, and yes, I think we will need commandline
> > arguments, probably one to force E820 clipping for future machines,
> > and one to disable it for old machines.
> 
> So what you are suggesting is to go back to a bios-date based approach
> (to determine old vs new machines) combined with DMI quirks to force
> E820 clipping on new machines which turn out to need it despite them
> being new ?

Yes.  It's ugly but I think the 10-year outlook is better.

> I have the feeling that if we switch to top-down allocating
> that we can then switch to just using _CRS and that everything
> will then just work, because we then match what Windows is doing...

Yes, it might.  But I'm not 100% comfortable because it basically
sweeps _CRS bugs under the rug, and we may trip over them as we do
more hotplug and (eventually) resource rebalancing.  I think we need
to work toward getting _CRS more reliable.

Bjorn

Re: [PATCH v2 0/3] x86/PCI: Log E820 clipping

[Hans de Goede]

Hi,

On 5/2/22 22:32, Bjorn Helgaas wrote:
> On Mon, May 02, 2022 at 02:24:26PM +0200, Hans de Goede wrote:
>> On 4/19/22 18:45, Bjorn Helgaas wrote:
>>> On Tue, Apr 19, 2022 at 05:16:44PM +0200, Hans de Goede wrote:
>>>> On 4/19/22 17:03, Bjorn Helgaas wrote:
>>>>> On Tue, Apr 19, 2022 at 11:59:17AM +0200, Hans de Goede wrote:
> 
>>>>>> So what is the plan to actually fix the issue seen on some
>>>>>> Lenovo models and Clevo Barebones ?   As I mentioned previously
>>>>>> I think that since all our efforts have failed so far that we
>>>>>> should maybe reconsider just using DMI quirks to ignore the
>>>>>> E820 reservation windows for host bridges on affected models ?
>>>>>
>>>>> I have been resisting DMI quirks but I'm afraid there's no other
>>>>> way.
>>>>
>>>> Well there is the first match adjacent windows returned by _CRS
>>>> and only then do the "covers whole region" exception check. I
>>>> still think that would work at least for the chromebook
>>>> regression...
>>>
>>> Without a crystal clear strategy, I think we're going to be
>>> tweaking the algorithm forever as the _CRS/E820 mix changes.
>>> That's why I think that in the long term, a "use _CRS only, with
>>> quirks for exceptions" strategy will be simplest.
>>
>> Looking at the amount of exception we already now about I'm not sure
>> if that will work well.
> 
> It's possible that many quirks will be required.  But I think in the
> long run the value of the simplest, most obvious strategy is huge.
> It's laid out in the spec already and it's the clearest way to
> agreement between firmware and OS.  When we trip over something, it's
> very easy to determine whether _CRS is wrong or Linux is using it
> wrong.  If we have to bring in question of looking at E820 entries,
> possibly merging them, using them or not based on overlaps ... that's
> a much more difficult conversation without a clear resolution.
> 
>>> So I think we should go ahead with DMI quirks instead of trying to
>>> make the algorithm smarter, and yes, I think we will need commandline
>>> arguments, probably one to force E820 clipping for future machines,
>>> and one to disable it for old machines.
>>
>> So what you are suggesting is to go back to a bios-date based approach
>> (to determine old vs new machines) combined with DMI quirks to force
>> E820 clipping on new machines which turn out to need it despite them
>> being new ?
> 
> Yes.  It's ugly but I think the 10-year outlook is better.

Ok, I've brushed off one of my earlier patches doing this and
added DMI quirks for the "Lenovo X1 Carbon 2nd gen" suspend
issue + the Asus C523NA / Google Coral Chromebook not booting
issues which we already know will get triggered by this based
on earlier testing.

I'll send this out right after this email.

Note this new patch is based on top of your:

https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/resource

>> I have the feeling that if we switch to top-down allocating
>> that we can then switch to just using _CRS and that everything
>> will then just work, because we then match what Windows is doing...
> 
> Yes, it might.  But I'm not 100% comfortable because it basically
> sweeps _CRS bugs under the rug, and we may trip over them as we do
> more hotplug and (eventually) resource rebalancing.  I think we need
> to work toward getting _CRS more reliable.

Ok.

Regards,

Hans