Re: [PATCH] x86/pci: Stop requiring MMCONFIG to be declared in E820, ACPI or EFI for newer systems

[Mario Limonciello <mario.limonciello@amd.com>]

On 12/14/2023 17:30, Bjorn Helgaas wrote:

>> I'm fairly certain we're just getting lucky in Linux on a lot of
>> devices that the region is often overlapping with a region for EFI
>> runtime services.
> 
> Ugh.  Yes, I'm sure it's not an isolated problem.
> 
>> Given the severity of what I've seen it can do to a system I'm
>> proposing FWTS to move it to HIGH:
>>
>> https://lists.ubuntu.com/archives/fwts-devel/2023-December/013772.html
> 
> Thanks.  I don't know anything about FWTS, but I'm a little skeptical
> that it actually catches this issue.  It *looks* like FWTS builds its
> idea of the memory map from a dmesg log or /sys/firmware/memmap, which
> I think both come from the E820 map, which is x86-specific, of course.
> 
> I don't see anything that builds a memory map based on _CRS methods,
> which I think is what we really want since the spec says:
> 
>    The resources can optionally be returned in Int15 E820h or
>    EFIGetMemoryMap as reserved memory but must always be reported
>    through ACPI as a motherboard resource.
> 
> (PCI Firmware spec r3.3, sec 4.1.2)

You're right; it doesn't catch the "root" of this issue, it only catches 
specifically when the region doesn't overlap with an existing 
reservation (like EFI runtime services).

A more thorough check would need to build a memory map.

> 
>> What is the actual *harm* in just using this MCFG table to make a
>> reservation when there isn't a PNP0C02 _CRS region declared?
>>
>> At worst (a buggy BIOS) you would end up with hole in the memory map
>> that isn't usable for devices.  At best you end up with more working
>> devices without changing the firmware.
> 
> We definitely need to work around this in Linux, and your patch might
> well be the right thing.
> 
> I'm a *little* hesitant because all the code in mmconfig-shared.c that
> attempts to validate MCFG entries suggests that relying on them
> uncritically was a problem in some cases, so I want to try to convince
> myself that we really won't break something.
> 
> Bjorn

As I mentioned in commit message this type of check was first introduced 
in 7752d5cfe3d1.

$ git describe --contains 7752d5cfe3d1
v2.6.26-rc1~369^2~18

That's roughly ~2008.  This is a long time back; IIRC it's before MMIO 
over 4GB was really added to BIOS in many PC platforms.

How about we build an escape hatch for users to put on the kernel 
command line in case of problems to restore the behavior that enforces 
reservations?
Maybe "enforce_ecam_resv"?

We could keep that around for a a year or two and if nothing pops up 
tear it out later.