Hi, On 3/30/22 13:35, Bjorn Helgaas wrote: > On Mon, Mar 28, 2022 at 02:54:42PM +0200, Hans de Goede wrote: >> Hi, >> >> On 3/24/22 23:19, Mark Brown wrote: >>> On Thu, Mar 24, 2022 at 09:34:30PM +0100, Hans de Goede wrote: >>> >>>> Mark, if one of use writes a test patch, can you get that Asus machine to boot a >>>> kernel build from next + the test patch ? >>> >>> I can't directly unfortunately as the board is in Collabora's lab but >>> Guillaume (who's already CCed) ought to be able to, and I can generally >>> prod and try to get that done. >> >> Ok, Guillaume, can you try a kernel with commit 5949965ec9340cfc0e65f7d8a576b660b26e2535 >> ("x86/PCI: Preserve host bridge windows completely covered by E820") + the >> attached patch added on top a try on the asus-C523NA-A20057-coral machine please >> and see if that makes it boot again ? >> >> Regards, >> >> Hans > >> From b8080a6d2d889847900e1408f71d0c01c73f5c94 Mon Sep 17 00:00:00 2001 >> From: Hans de Goede <hdegoede@xxxxxxxxxx> >> Date: Mon, 28 Mar 2022 14:47:41 +0200 >> Subject: [PATCH] x86/PCI: Limit "e820 entry fully covers window" check to non >> ISA MMIO addresses >> >> Commit FIXME ("x86/PCI: Preserve host bridge windows completely >> covered by E820") added a check to skip e820 table entries which >> fully cover a PCI bride's memory window when clipping PCI bridge >> memory windows. >> >> This check also caused ISA MMIO windows to not get clipped when >> fully covered, which is causing some coreboot based Chromebooks >> to not boot. >> >> Modify the fully covered check to not apply to ISA MMIO windows. > > I'd like to include URLs to the kernelci results unless they are > ephemeral. There's a lot of valuable information in these: > > Asus C523NA-A20057-coral with the last good commit: > https://lava.collabora.co.uk/scheduler/job/5937945 > > https://storage.kernelci.org/next/master/next-20220310/x86_64/x86_64_defconfig+x86-chromebook/gcc-10/lab-collabora/baseline-asus-C523NA-A20057-coral.html > https://storage.kernelci.org/next/master/next-20220310/x86_64/x86_64_defconfig+x86-chromebook/gcc-10/lab-collabora/baseline-hp-x360-12b-n4000-octopus.html Ok, I'll include this in any future patches for this. > >> Fixes: FIXME ("x86/PCI: Preserve host bridge windows completely covered by E820") >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> arch/x86/kernel/resource.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c >> index 6be82e16e5f4..d9ec913619c3 100644 >> --- a/arch/x86/kernel/resource.c >> +++ b/arch/x86/kernel/resource.c >> @@ -46,8 +46,12 @@ void remove_e820_regions(struct device *dev, struct resource *avail) >> * devices. But if it covers the *entire* resource, it's >> * more likely just telling us that this is MMIO space, and >> * that doesn't need to be removed. >> + * Note this *entire* resource covering check is only >> + * intended for 32 bit memory resources for the 16 bit >> + * isa window we always apply the e820 entries. >> */ >> - if (e820_start <= avail->start && avail->end <= e820_end) { >> + if (avail->start >= ISA_END_ADDRESS && > > What is the justification for needing to check ISA_END_ADDRESS here? > The commit log basically says "this makes it work", which isn't very > satisfying. I did not have a log with the: > acpi PNP0A08:00: clipped [mem 0x000a0000-0x000bffff window] to [mem 0x00100000-0x000bffff window] for e820 entry [mem 0x000a0000-0x000fffff] > acpi PNP0A08:00: clipped [mem 0x7b800000-0x7fffffff window] to [mem 0x80000000-0x7fffffff window] for e820 entry [mem 0x7b000000-0x7fffffff] > acpi PNP0A08:00: clipped [mem 0x80000000-0xe0000000 window] to [mem 0x80000000-0xcfffffff window] for e820 entry [mem 0xd0000000-0xd0ffffff] messages. Instead I was looking at this log: https://storage.kernelci.org/next/master/next-20220310/x86_64/x86_64_defconfig+x86-chromebook/gcc-10/lab-collabora/baseline-asus-C523NA-A20057-coral.html With the following messages (as I quoted higher up in the email-thread): """ 1839 17:54:41.406548 <6>[ 0.000000] BIOS-provided physical RAM map: 1840 17:54:41.413121 <6>[ 0.000000] BIOS-e820: [mem 0x0000000000000000-0x0000000000000fff] type 16 1841 17:54:41.419712 <6>[ 0.000000] BIOS-e820: [mem 0x0000000000001000-0x000000000009ffff] usable 1842 17:54:41.430192 <6>[ 0.000000] BIOS-e820: [mem 0x00000000000a0000-0x00000000000fffff] reserved 1843 17:54:41.436207 <6>[ 0.000000] BIOS-e820: [mem 0x0000000000100000-0x000000000fffffff] usable 1844 17:54:41.446353 <6>[ 0.000000] BIOS-e820: [mem 0x0000000010000000-0x0000000012150fff] reserved 1845 17:54:41.453290 <6>[ 0.000000] BIOS-e820: [mem 0x0000000012151000-0x000000007a9fcfff] usable 1846 17:54:41.459966 <6>[ 0.000000] BIOS-e820: [mem 0x000000007a9fd000-0x000000007affffff] type 16 1847 17:54:41.469549 <6>[ 0.000000] BIOS-e820: [mem 0x000000007b000000-0x000000007fffffff] reserved 1848 17:54:41.476685 <6>[ 0.000000] BIOS-e820: [mem 0x00000000d0000000-0x00000000d0ffffff] reserved 1849 17:54:41.486439 <6>[ 0.000000] BIOS-e820: [mem 0x00000000e0000000-0x00000000efffffff] reserved 1850 17:54:41.492994 <6>[ 0.000000] BIOS-e820: [mem 0x00000000fed10000-0x00000000fed17fff] reserved 1851 17:54:41.503008 <6>[ 0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000017fffffff] usable ... 2030 17:54:42.809183 <6>[ 0.313771] pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000bffff window] 2031 17:54:42.819092 <6>[ 0.314424] pci_bus 0000:00: root bus resource [mem 0x7b800000-0xe0000000 window] """ ### What I find weird here is that this boot with a somewhat earlier kernel has: 2030 17:54:42.809183 <6>[ 0.313771] pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000bffff window] 2031 17:54:42.819092 <6>[ 0.314424] pci_bus 0000:00: root bus resource [mem 0x7b800000-0xe0000000 window] Where as the boot with the clipped messages has: <6>[ 0.313705] acpi PNP0A08:00: ignoring host bridge window [mem 0x00100000-0x000bffff window] (conflicts with PCI mem [mem 0x00000000-0x7fffffffff]) <6>[ 0.314702] acpi PNP0A08:00: ignoring host bridge window [mem 0x80000000-0x7fffffff window] (conflicts with PCI mem [mem 0x00000000-0x7fffffffff]) <6>[ 0.315747] PCI host bridge to bus 0000:00 <6>[ 0.316118] pci_bus 0000:00: root bus resource [io 0x0000-0x0cf7 window] <6>[ 0.316703] pci_bus 0000:00: root bus resource [io 0x1000-0xffff window] <6>[ 0.317298] pci_bus 0000:00: root bus resource [mem 0x80000000-0xcfffffff window] <6>[ 0.317703] pci_bus 0000:00: root bus resource [bus 00-ff] So in the boot with the clipped messages we are getting 3 windows from _CRS where as before we were getting only 2? I know that we are now applying the clipping directly when we are parsing the resources. So I guess that before we somehow also merged the 2 resources which are back to back together before the "root bus resource" messages get printed. This caused me to just see the "root bus resource [mem 0x7b800000-0xe0000000 window]" which is not fully covered which is why I focused on the ISA MMIO window. > The Asus log of the last good commit shows: > > PCI: 00:0d.0 [8086/5a92] enabled > constrain_resources: PCI: 00:0d.0 10 base d0000000 limit d0ffffff mem (fixed) > ... > BIOS-e820: [mem 0x000000007b000000-0x000000007fffffff] reserved > BIOS-e820: [mem 0x00000000d0000000-0x00000000d0ffffff] reserved > BIOS-e820: [mem 0x00000000e0000000-0x00000000efffffff] reserved > ... > acpi PNP0A08:00: clipped [mem 0x000a0000-0x000bffff window] to [mem 0x00100000-0x000bffff window] for e820 entry [mem 0x000a0000-0x000fffff] > acpi PNP0A08:00: clipped [mem 0x7b800000-0x7fffffff window] to [mem 0x80000000-0x7fffffff window] for e820 entry [mem 0x7b000000-0x7fffffff] > acpi PNP0A08:00: clipped [mem 0x80000000-0xe0000000 window] to [mem 0x80000000-0xcfffffff window] for e820 entry [mem 0xd0000000-0xd0ffffff] > acpi PNP0A08:00: ignoring host bridge window [mem 0x00100000-0x000bffff window] (conflicts with PCI mem [mem 0x00000000-0x7fffffffff]) > acpi PNP0A08:00: ignoring host bridge window [mem 0x80000000-0x7fffffff window] (conflicts with PCI mem [mem 0x00000000-0x7fffffffff]) > > It looks like _CRS gave us [mem 0x80000000-0xe0000000 window], which > is one byte too big (it should end at 0xdfffffff). Yeah but that gets clipped off anyways, so that should not matter. s> > From the firmware part of the log, it looks like 00:0d.0 is a hidden > device that consumes [mem d0000000-0xd0ffffff]. Linux doesn't > enumerate 00:0d.0, so firmware should have carved that out of the [mem > 0x80000000-0xe0000000 window] in _CRS. > > We don't have a log with 5949965ec934 ("x86/PCI: Preserve host bridge > windows completely covered by E820") applied, but I think it would > show this: > > acpi PNP0A08:00: resource [mem 0x000a0000-0x000bffff window] fully covered by e820 entry [mem 0x000a0000-0x000fffff] > acpi PNP0A08:00: resource [mem 0x7b800000-0x7fffffff window] fully covered by e820 entry [mem 0x7b000000-0x7fffffff] > > instead of clipping those windows. But none of the devices we > enumerate appears to be using either of those windows. Not with a working kernel no, because they are clipped of, but with the don't clip fully-covered _CRS windows change, the [mem 0x7b000000-0x7fffffff] all of a sudden becomes fair game to assign BARs to. I agree that we will get a fully-covered msg for that one with the patch, which would change: [ 0.317298] pci_bus 0000:00: root bus resource [mem 0x80000000-0xcfffffff window] to: [ 0.317298] pci_bus 0000:00: root bus resource [mem 0x7b800000-0xcfffffff window] and I believe that likely is our culprit. So to fix this I guess that we first need to merge back-to-back windows coming from _CRS into a single window, before calling remove_e820_regions() That would pass [mem 0x7b800000-0xe0000000 window] to remove_e820_regions() in a single call (as I expected from the logs), which should result in both the top and the bottom still getting clipped as before. I've been looking around in the code a but I could not quickly find a helper to do the back-to-back resource merging before calling remove_e820_regions(), any suggestions for this? Regards, Hans > > We do have this: > > pci 0000:00:18.2: reg 0x10: [mem 0xde000000-0xde000fff 64bit] > pci 0000:00:18.2: reg 0x18: [mem 0xc2b31000-0xc2b31fff 64bit] > pci 0000:00:18.2: can't claim BAR 0 [mem 0xde000000-0xde000fff 64bit]: no compatible bridge window > pci 0000:00:18.2: BAR 0: assigned [mem 0x80000000-0x80000fff 64bit] > > Where the original [mem 0xde000000-0xde000fff 64bit] assignment was > perfectly legal, but we clipped [mem 0x80000000-0xe0000000 window] to > [mem 0x80000000-0xcfffffff window] instead of just punching a hole for > the 00:0d.0 carve-out. > > Maybe 5949965ec934 puts 00:18.2 BAR 0 somewhere that doesn't work, > or maybe the clipping to [mem 0x00100000-0x000bffff window] or > [mem 0x80000000-0x7fffffff window] doesn't work as expected? > They are supposed to be interpreted as "empty", but certainly > resource_size([0x00100000-0x000bffff]) is != 0. > >> + e820_start <= avail->start && avail->end <= e820_end) { >> dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n", >> avail, e820_start, e820_end); >> continue; >> -- >> 2.35.1 >> >