Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Ensure full 16-bit VGA port i/o decoding ......................................................................
Patch Set 1:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@807 PS1, Line 807: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL);
If upstream bridge supports CTL_VGA16, it does not matter when downstream (including cardbus) bridges do not support it?
I doesn't matter upstream nor downstream but it does matter for siblings of the bridge that positively decodes them. So with our "simple" allocator, we would have to reserve all the aliased regions. Not an option, I guess, see below.
I think the cleanest approach is to add VGA IO resource once when we encounter a VGA class device. If platform and configuration is such that CTL_VGA would be set without CTL_VGA16 (on the most upstream PCI bridge), then add those 63 aliased resources.
But what does adding the resources mean? As fixed resources it wouldn't work: The way we handle fixed resources (see constrain_resources()) would stab us in the back. I assume it would restrict all dynamic allocations to either x..0x3b0 or 0xffe0..0xffff for the whole domain. It doesn't seem to take topology into account.
As regular resource, they would be considered for forwarding through the bridge's regular i/o window.
I can test something with aopen/dxplplusu, nb/intel/e7505 lacks CTL_VGA16. No AGP graphics cards around anymore, so I have to hack something for testing though. And we may have to sort out the mess on AMD side too...
I'm all for ignoring the problem, now. We seem rather safe for dynamic allocations and I doubt these old boards have any bad fixed resources. For newer boards we can assume CTL_VGA16. Setting this bit is already a huge plus :)
I currently don't have the time to redesign things. But ideas are already popping up: Don't descent in constrain_resources(), only consider the current device and its buses. That would require, how- ever, that we correctly add fixed resources that are forwarded by a bridge (which I don't see currently).