Re: [edk2-devel] UefiPayloadPkg and over-eager PCI resource allocation?

["Laszlo Ersek" <lersek@redhat.com>]

On 01/07/21 23:04, Matthew Garrett wrote:
> On Thu, Jan 07, 2021 at 02:14:06PM +0100, Laszlo Ersek wrote:
> 
>> Now, if coreboot assigns resources such that even a *single bus number*
>> may have resources straddling the MMCONFIG area, then that's a
>> fundamental incompatibility between coreboot and edk2. I hope this is
>> not the case; Matt's log contains "Bus: 0 - 3", so I hope that, given
>> any single bus (= one-element bus range), the set of related MMIO
>> resources does not bracket the MMCONFIG area.
> 
> No, bus 0 includes addresses above and below MMCONFIG, and I'm not
> really sure how it could avoid doing so without moving MMCONFIG. Certain
> resources in bus 0 are required to be in the high 0xf0000000 range, and
> there's not enough space between the top of MMCONFIG and there to map
> everything else from bus 0 and keep them accessible to 32-bit
> environments. So that would presumably mean pushing MMCONFIG down from
> 0xe0000000, which is certainly possible but I'd worry that some things
> may make assumptions about where it's located.
> 
> (The resource layout under Coreboot matches the resource layout under
> vendor firmware running on the same system, so this doesn't seem like a
> Coreboot-specific thing - if EDK2 requires that there be no overlap
> between MMCONFIG and the resources allocated to single bus, it seems
> like EDK2 doesn't match the expectations of vendor UEFI implementations
> and may cause problems in future. Older versions of UEFIPayload don't
> seem to enforce this)

The last statement is intriguing, it suggests something could be bisected...

But then I run a git-log on

  UefiPayloadPkg/Library/PciHostBridgeLib

and I find *one* commit, namely the *ridiculously* huge commit
04af8bf262f1 ("UefiPayloadPkg: Enhance UEFI payload for coreboot and
Slim Bootloader", 2019-04-15).

That commit lists *at least* 10 separate high-level tasks in the commit
message, and has the beautiful diffstat

 50 files changed, 8566 insertions(+)

In other words, it's a code drop that's *maybe* tolerable for the
github.com "squash on merge" crowd, but honestly, it has *no place* in edk2.

I don't know what to recommend. And every time I obsess publicly in the
edk2 community about the importance of bisectability, I mostly draw
blank stares. Go figure.

... Apparently said commit was the initial addition of UefiPayloadPkg as
a whole to edk2. From the commit message, it was meant to replace
CorebootModulePkg and CorebootPayloadPkg. At commit 04af8bf262f1, those
separate packages still exist.

... Hm, they were removed later, in commit f684c3f5eef4 ("Coreboot*Pkg:
Retire CorebootPayloadPkg and CorebootModulePkg", 2019-05-10). So
perhaps a UEFI Payload built from CorebootModulePkg and
CorebootPayloadPkg could be bisected, up to and excluding commit
f684c3f5eef4.

However, if the symptom appeared with the "big switch" from Coreboot*Pkg
to UefiPayloadPkg (that is, with commit 04af8bf262f1), then it's a
design limitation in PciHostBridgeLib :(

Laszlo