Attention is currently required from: Shelley Chen, Ravi Kumar Bokka, Hung-Te Lin, Tim Wawrzynczak, Paul Menzel, Rex-BC Chen, Julius Werner, Arthur Heymans, Yu-Ping Wu, Jianjun Wang. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63251 )
Change subject: coreboot_tables: Add PCIe info to coreboot table ......................................................................
Patch Set 20:
(1 comment)
Patchset:
PS9:
I really don't understand where the 1MB number is coming from here.
Wild guess, maybe it was meant as a feature but turned out to be too subtle to be useful. Usually 1MiB covers the config space for all devices on a single bus, as the calculation above suggests. However, there are several clues why this might not work as expected:
1. On the PCI side of the ATU, we don't have 4KiB per function as PCIe needs but 64KiB instead (BDF on the PCI-side address is covers bits 31..16). Assuming the ATU translates this 1:1, there's only space for 16 functions instead of the expected 256.
2. The ATU sharing with the I/O-port mapping. It kind of implies that one can't leave the PCI config space mapped for future access. And without proper locking, it's even dangerous (a concurrent PCI driver trying to write I/O ports might write to random config space instead). This wouldn't matter for coreboot, but if the code was originally tested for Linux, who knows...
It's all probably hard to test. One would need a legacy PCI bridge or a PCIe switch device. A single device with multiple functions could serve to test at least multiple functions.
If one has address space to waste and the secondary bus at the root port can be configured, one could also test a bigger mapping some- where else (after all the ATU is configured with the CPU-side address and size). Then configure the secondary bus >1, map 00:00.0 and see if the downstream device shows up anywhere in the mapped space.
If you want to access PCIe config space registers for the controller itself, you have to go over the DBI (data bus interface), where those 4096 bytes are mapped into the lower 4096 bytes of the DBI space.
This is also what I read in the kernel but couldn't find in coreboot code yet.