Hello Kyösti Mälkki, Patrick Rudolph, Michael Niewöhner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35516
to review the following change.
Change subject: device/pci: Ensure full 16-bit VGA port i/o decoding ......................................................................
device/pci: Ensure full 16-bit VGA port i/o decoding
So, the PCI to PCI bridge specification had a pitfall for us: Originally, when decoding i/o ports for legacy VGA cycles, bridges should only consider the 10 least significant bits of the port address. This means all VGA registers were aliased every 1024 ports! e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc.
However, it seems, we never reserved the aliased ports, resulting in random conflicts. We neither use much external VGA nor many i/o ports these days, so nobody noticed.
To avoid this mess, a bridge control bit (VGA16) was introduced in 2003 to enable decoding of 16-bit port addresses. As we don't want to clutter our i/o port space, we'll now simply fail for VGA behind bridges that don't support it. Famous last words: I assume there can't be many bridges left that don't support this bit ;)
Change-Id: Id7a07f069dd54331df79f605c6bcda37882a602d Signed-off-by: Nico Huber nico.h@gmx.de --- M src/device/device.c M src/device/pci_device.c M src/include/device/device.h M src/include/device/pci_def.h 4 files changed, 48 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35516/1
diff --git a/src/device/device.c b/src/device/device.c index 44d1f95..523bd1c 100644 --- a/src/device/device.c +++ b/src/device/device.c @@ -757,6 +757,12 @@ while ((dev = dev_find_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) { if (!dev->enabled) continue; + if (dev->bus->no_vga) { + printk(BIOS_WARNING, "Ignoring VGA at %s" + ", a bridge on the path isn't supported", + dev_path(dev)); + continue; + }
printk(BIOS_DEBUG, "found VGA at %s\n", dev_path(dev));
@@ -797,7 +803,7 @@ while (bus) { printk(BIOS_DEBUG, "Setting PCI_BRIDGE_CTL_VGA for bridge %s\n", dev_path(bus->dev)); - bus->bridge_ctrl |= PCI_BRIDGE_CTL_VGA; + bus->bridge_ctrl |= PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_VGA16; bus = (bus == bus->dev->bus) ? 0 : bus->dev->bus; } } diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 7ecb652..2fadb55 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -788,6 +788,43 @@ };
/** + * Check for compatibility to route legacy VGA cycles through a bridge. + * + * Originally, when decoding i/o ports for legacy VGA cycles, bridges + * should only consider the 10 least significant bits of the port address. + * This means all VGA registers were aliased every 1024 ports! + * e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc. + * + * To avoid this mess, a bridge control bit (VGA16) was introduced in + * 2003 to enable decoding of 16-bit port addresses. As we don't want + * to clutter our i/o port space, we simply fail for VGA behind bridges + * that don't support it (set .no_vga = 1). + */ +static void pci_bridge_vga_compat(struct bus *const bus) +{ + uint16_t bridge_ctrl; + + bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); + + /* Ensure VGA decoding is disabled during probing (it should + be by default, but we run blobs nowadays) */ + bridge_ctrl &= ~PCI_BRIDGE_CTL_VGA; + pci_write_config16(bus->dev, PCI_BRIDGE_CONTROL, bridge_ctrl); + + /* If the upstream bridge doesn't support VGA, we don't have to check */ + bus->no_vga |= bus->dev->bus->no_vga; + if (bus->no_vga) + return; + + /* Test if we can enable 16-bit decoding */ + bridge_ctrl |= PCI_BRIDGE_CTL_VGA16; + pci_write_config16(bus->dev, PCI_BRIDGE_CONTROL, bridge_ctrl); + bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); + + bus->no_vga = !(bridge_ctrl & PCI_BRIDGE_CTL_VGA16); +} + +/** * Detect the type of downstream bridge. * * This function is a heuristic to detect which type of bus is downstream @@ -1288,6 +1325,8 @@
bus = dev->link_list;
+ pci_bridge_vga_compat(bus); + pci_bridge_route(bus, PCI_ROUTE_SCAN);
do_scan_bus(bus, 0x00, 0xff); diff --git a/src/include/device/device.h b/src/include/device/device.h index b2221cc..78e234e 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -94,6 +94,7 @@ unsigned int reset_needed : 1; unsigned int disable_relaxed_ordering : 1; unsigned int ht_link_up : 1; + unsigned int no_vga : 1; /* We can't support VGA behind this bridge */ };
/* diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index bc5bc79..c8b86d5 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -138,6 +138,7 @@ #define PCI_BRIDGE_CTL_SERR 0x02 /* The same for SERR forwarding */ #define PCI_BRIDGE_CTL_NO_ISA 0x04 /* Disable bridging of ISA ports */ #define PCI_BRIDGE_CTL_VGA 0x08 /* Forward VGA addresses */ +#define PCI_BRIDGE_CTL_VGA16 0x10 /* Enable 16-bit i/o port decoding */ #define PCI_BRIDGE_CTL_MASTER_ABORT 0x20 /* Report master aborts */ #define PCI_BRIDGE_CTL_BUS_RESET 0x40 /* Secondary bus reset */ /* Fast Back2Back enabled on secondary interface */
Kyösti Mälkki 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:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG@17 PS1, Line 17: these days, so nobody noticed. Looks like this bit is only defined when upstream is PCIe and downstream is PCI/PCI-X/AGP(?), given that the definition of CTL_VGA and CTL_VGA_16 is found only in PCIe-to-PCI(-X) bridge specs, paragraph 5.1.2.13.
VGA IO decodes are not documented for PCI Express base specification rev 2.1 and 3.0 section 7.5.3.6. I believe that one would cover PCIe slots?
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); It's a bit ugly to do this with cardbus configuration header, which should use PCI_CB_BRIDGE_CONTROL instead. Since cardbus is PCI, I believe CTL_VGA and CTL_VGA16 are defined so maybe this is fine.
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@814 PS1, Line 814: /* If the upstream bridge doesn't support VGA, we don't have to check */ See comments on commit message. I am not sure if CTL_VGA and CTL_VGA16 bits are no longer defined for PCIe root ports and bridges.
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@822 PS1, Line 822: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); Write with CTL_VGA=0,CTL_VGA16=1 is strange/unexpected. Some hardware could respond with CTL_VGA16=0 here, even if CTL_VGA=1,CTL_VGA16 would be supported.
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... PS1, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08 Not sure what 0x10 is with cardbus. You can see 0x80 below does not match with PCI_BRIDGE_CONTROL register definitions.
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:
(2 comments)
So this would definitely break CardBus VGA adapters (if the bridges comply with their spec)... do we mind? I'd rather just mention it in the commit message.
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG@17 PS1, Line 17: these days, so nobody noticed.
Looks like this bit is only defined when upstream is PCIe and downstream is PCI/PCI-X/AGP(?), given […]
It's often reserved, read zero (including PC Card 8.0). It's in PCI-to-PCI bridge 1.2 and in PCIe-to-PCI/PCI-X 1.0.
Fun fact: VGA Enable (bit 3) seems to be only mandatory in the PC Card spec ;) so far, we just assume that it's present.
With slots, I guess you mean root ports? Yes, they are covered by the PCIe base spec. aaaaaaand, they only added the bits in 4.0 xD Anyway, if we'd have passed more than a decade without legacy VGA, we'd have noticed.
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@822 PS1, Line 822: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL);
Write with CTL_VGA=0,CTL_VGA16=1 is strange/unexpected. […]
Ooff, I assumed the other case, that hardware might mind if we change VGA16 while VGA is off... I don't think this can be solved without time travel and tighter specs.
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:
(1 comment)
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@822 PS1, Line 822: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL);
Ooff, I assumed the other case, that hardware might mind if we change […]
Ofc, we could also try to figure out what others did, say 10~15 years ago. Maybe there is/was a common way of probing this?
Kyösti Mälkki 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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG@17 PS1, Line 17: these days, so nobody noticed.
It's often reserved, read zero (including PC Card 8.0). It's in PCI-to-PCI […]
Oh.. the documentation state is just perfect :/
It could be that even when implemented for PCIe root ports, CTL_VGA bits for those bridges have no effect when IGD is enabled. We should also make sure only one PCIe root port will have CTL_VGA set.
src/soc/intel/common/block/pcie/pcie.c: pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~1, 1<<2);
Unconditionally setting 'ISA enable' bit is also.. interesting.
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@814 PS1, Line 814: /* If the upstream bridge doesn't support VGA, we don't have to check */
See comments on commit message. […]
Ack
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@822 PS1, Line 822: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL);
Ooff, I assumed the other case, that hardware might mind if we change VGA16 while VGA is off...
You assumed changing VGA16 while VGA is *on* could be bad? I think my approach would have been to try set them both at same time, and unconditionally reset them both before exit. Let's just keep your approach and revisit if it does not work for someone.
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: Code-Review-1
Dug a little into the chipset support for historical Intel in the tree: Older ICHs implement the bit r/w but mention to ignore it anyway, because everything goes to PCI. So that should be fine.
440BX doesn't seem to know it, though :-/ So this can't go in as is. I'm open for suggestions. What should we do if 16-bit decode isn't possible? Restrict all i/o to 10 bits? That could brick... Hmmmm, just warn about it? Instead of skipping the device?
I really don't like to reserve the VGA resources 64 times, could do that, though.
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG@17 PS1, Line 17: these days, so nobody noticed.
It could be that even when implemented for PCIe root ports, CTL_VGA bits for those bridges have no effect when IGD is enabled.
IIRC, they have an effect: the wrath of resource conflicts. This is why we have a .disable op that should be implemented for the IGD.
We should also make sure only one PCIe root port will have CTL_VGA set.
We already do that implicitly. We enable it only on the path to the winning device in set_vga_bridge_bits().
src/soc/intel/common/block/pcie/pcie.c: pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~1, 1<<2);
Unconditionally setting 'ISA enable' bit is also.. interesting.
Yeah, interesting. We try to avoid allocating in those ranges (I learned today) device.c:400 (should work as long as the resource is <= 0x100). But same as the VGA avoidance below it, it won't be noticed by static/preallocated resources. oh wait, nvm, is that an `else if`? dead branch?
Michael Niewöhner 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:
(1 comment)
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);
It's a bit ugly to do this with cardbus configuration header, which should use PCI_CB_BRIDGE_CONTROL […]
The spec for these bits match, so it is fine (see cardbus spec 4.5.2.33)
Michael Niewöhner 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:
(1 comment)
Patch Set 1: Code-Review-1
Dug a little into the chipset support for historical Intel in the tree: Older ICHs implement the bit r/w but mention to ignore it anyway, because everything goes to PCI. So that should be fine.
440BX doesn't seem to know it, though :-/ So this can't go in as is. I'm open for suggestions. What should we do if 16-bit decode isn't possible? Restrict all i/o to 10 bits? That could brick... Hmmmm, just warn about it? Instead of skipping the device?
I really don't like to reserve the VGA resources 64 times, could do that, though.
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... PS1, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08
Not sure what 0x10 is with cardbus. […]
cardbus matches, see spec
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:
(1 comment)
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);
The spec for these bits match, so it is fine (see cardbus spec 4.5.2. […]
It's not a complete match. And bit 4 was still reserved in the latest revision. So if somebody would still continue that spec, bit 4 could deviate. Luckily, it seems abandoned.
Kyösti Mälkki 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:
(3 comments)
Patch Set 1: Code-Review-1
Dug a little into the chipset support for historical Intel in the tree: Older ICHs implement the bit r/w but mention to ignore it anyway, because everything goes to PCI. So that should be fine.
440BX doesn't seem to know it, though :-/ So this can't go in as is. I'm open for suggestions. What should we do if 16-bit decode isn't possible? Restrict all i/o to 10 bits? That could brick... Hmmmm, just warn about it? Instead of skipping the device?
I really don't like to reserve the VGA resources 64 times, could do that, though.
The code in device.c seems to attempt to avoid VGA IO and "legacy" PCI IO windows when allocating (dynamic) resources. I am not convinced that code always works, and those windows would appear to be unnecessary for platforms where we can have programming of CTL_VGA=1, CTL_VGA16=1 and CTL_(NO)_ISA=0 for PCI_BRIDGE_CONTROL.
I think we could first throw errors if assigned resources overlap some of those aliased (and enabled) IO address ranges. Should be easy to (manually) adjust the conflicting static resources when they appear.
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@417 PS1, Line 417: } So.. we are shifting the start of resource upwards here.. But nothing guarantee the end of resource does not hit the range of aliased decodes?
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@794 PS1, Line 794: /* VGA is first add-on card or the only onboard VGA. */ I thought it would be the last add-on dev_find_class() call returns? Just assuming here it would be the one with highest bus number.
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);
The spec for these bits match, so it is fine (see cardbus spec 4.5.2. […]
The cardbus spec I found, dated 2001, claiming release 8.0, documents bit 4 reserved returning zero, with no references to 16bit VGA decode.
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:
(3 comments)
The code in device.c seems to attempt to avoid VGA IO and "legacy" PCI IO windows when allocating (dynamic) resources. I am not convinced that code always works, and those windows would appear to be unnecessary for platforms where we can have programming of CTL_VGA=1, CTL_VGA16=1 and CTL_(NO)_ISA=0 for PCI_BRIDGE_CONTROL.
Sounds plausible, more plausible than the comments in the existing code :)
I think we could first throw errors if assigned resources overlap some of those aliased (and enabled) IO address ranges. Should be easy to (manually) adjust the conflicting static resources when they appear.
How would you implement that? It would be nice to just add the resources when deciding to set CTL_VGA, but I assume then we'd have to exclude them from being included in the bridge's regular i/o / memory base regions?
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@403 PS1, Line 403: The legacy PCI decodes : * only 10 bits, uses 0x100 - 0x3ff. This comment suggests that there is more lurking than the ISA thing. I could only find specs back to PCI 2.3 which already says 32 bits must be decoded. But maybe an earlier spec really allowed to decode only 10 bits? I mean regular PCI devices, not bridges.
OTOH, taking the comment literally would imply that we couldn't assign any resources to these... waaaaaaaaaa
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@417 PS1, Line 417: }
So.. we are shifting the start of resource upwards here.. […]
The `else if` branch is dead. For `(base >= 0x3b0) && (base <= 0x3df)`, `(base & 0x300) != 0`!
also, TIL so much... PCI says i/o BARs have to be <= 256 ports
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@794 PS1, Line 794: /* VGA is first add-on card or the only onboard VGA. */
I thought it would be the last add-on dev_find_class() call returns? Just assuming here it would be […]
The code here is full of weird assumptions, the comment only adds to it...
* .on_mainboard means integrated? wrong if a discrete VGA chip is mentioned in the dt. * .on_mainboard appears only once? ... * integrated VGA is already disabled if not mentioned in the dt (.on_mainboard == 0)? implementation defined
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... PS1, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08
cardbus matches, see spec
well, "matches" means we can assume the bit to be reserved for now... (not all registers are the same)
Michael Niewöhner 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:
(1 comment)
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);
returning zero, with no references to 16bit VGA decode.
correct; this is what we expect, when 16bit VGA is not supported. The question is what should we do if it is not supported?
Kyösti Mälkki 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@403 PS1, Line 403: The legacy PCI decodes : * only 10 bits, uses 0x100 - 0x3ff.
This comment suggests that there is more lurking than the ISA thing. […]
I wonder if the comment was supposed to say 'legacy ISA' decodes only 10 bits? Then, if ge bo back to pre-LPC chipsets where ISA signals were actually present on "southbridge" device, aliasing problems could have appeared there? Even if PCI-to-ISA-bridge was in subtractive decode mode, PCI resources would still have precedence (on PIIX4)?
https://www.intel.com/Assets/PDF/datasheet/290562.pdf (PIIX4 data)
And you can find some vintage horrors apparently depending on this aliasing:
http://www.vcfed.org/forum/showthread.php?34938-PC-ISA-I-O-Address-Space-10-...
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@417 PS1, Line 417: }
The `else if` branch is dead. For `(base >= 0x3b0) && (base <= 0x3df)`, […]
Well.. it only took 16 years to notice?
commit bbb6d102 Date: Mon Aug 4 19:54:48 2003 +0000
Maybe we can get someone to test on asus/p2b with this 10-bit aliasing "fix" removed. But if we trust datasheet, integrated logic decodes 16-bits. So one would need some ill-behaving ISA add-on card too...
Kyösti Mälkki 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:
(1 comment)
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);
returning zero, with no references to 16bit VGA decode. […]
If upstream bridge supports CTL_VGA16, it does not matter when downstream (including cardbus) bridges do not support it? Aliased addresses are already not decoded from the primary side of that upstream bridge?
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.
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...
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:
(1 comment)
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).
Hello Kyösti Mälkki, Patrick Rudolph, Michael Niewöhner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35516
to look at the new patch set (#2).
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
device/pci: Enable full 16-bit VGA port i/o decoding
So, the PCI to PCI bridge specification had a pitfall for us: Originally, when decoding i/o ports for legacy VGA cycles, bridges should only consider the 10 least significant bits of the port address. This means all VGA registers were aliased every 1024 ports! e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc.
However, it seems, we never reserved the aliased ports, resulting in silent conflicts we preallocated resources. We neither use much external VGA nor many i/o ports these days, so nobody noticed.
To avoid this mess, a bridge control bit (VGA16) was introduced in 2003 to enable decoding of 16-bit port addresses. As older systems seem rather safe and well tested, and newer systems should support this bit, we'll use it if possible and only warn if not.
Change-Id: Id7a07f069dd54331df79f605c6bcda37882a602d Signed-off-by: Nico Huber nico.h@gmx.de --- M src/device/device.c M src/device/pci_device.c M src/include/device/device.h M src/include/device/pci_def.h 4 files changed, 46 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35516/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
I think the approach here is a good compromise. Should fix recent platforms and not break older ones. Maybe commit message could say use of VGA cards on AGP and (parallel) PCI slots may have issues due to aliasing, and reference the error messages we add here.
https://review.coreboot.org/c/coreboot/+/35516/2/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/2/src/device/device.c@799 PS2, Line 799: Can we add this:
bool vga_io_10bit_decode(void) { return (vga_pri && vga_pri->bus->no_vga16); }
I might try to fix the 'aliasing avoidance' or just throw errors after resource assignments.
Hello Kyösti Mälkki, Patrick Rudolph, Michael Niewöhner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35516
to look at the new patch set (#3).
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
device/pci: Enable full 16-bit VGA port i/o decoding
So, the PCI to PCI bridge specification had a pitfall for us: Originally, when decoding i/o ports for legacy VGA cycles, bridges should only consider the 10 least significant bits of the port address. This means all VGA registers were aliased every 1024 ports! e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc.
However, it seems, we never reserved the aliased ports, resulting in silent conflicts we preallocated resources. We neither use much external VGA nor many i/o ports these days, so nobody noticed.
To avoid this mess, a bridge control bit (VGA16) was introduced in 2003 to enable decoding of 16-bit port addresses. As older systems seem rather safe and well tested, and newer systems should support this bit, we'll use it if possible and only warn if not.
With old (AGP era) hardware one will likely encounter a warning like this:
found VGA at PCI: 06:00.0 A bridge on the path doesn't support 16-bit VGA decoding!
This is not generally fatal, but makes unnoticed resource conflicts more likely.
Change-Id: Id7a07f069dd54331df79f605c6bcda37882a602d Signed-off-by: Nico Huber nico.h@gmx.de --- M src/device/device.c M src/device/pci_device.c M src/include/device/device.h M src/include/device/pci_def.h 4 files changed, 46 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35516/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35516/2/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/2/src/device/device.c@799 PS2, Line 799:
Can we add this: […]
Um, I guess it's best if you add it with your patch (wasn't sure if you want it `static` or an API, ...).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 3: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 3:
ooops, forgot the submit this...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG@17 PS1, Line 17: these days, so nobody noticed.
It could be that even when implemented for PCIe root ports, CTL_VGA bits for those bridges have no […]
Done
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@403 PS1, Line 403: The legacy PCI decodes : * only 10 bits, uses 0x100 - 0x3ff.
I wonder if the comment was supposed to say 'legacy ISA' decodes only 10 bits? […]
Done
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@417 PS1, Line 417: }
Well.. it only took 16 years to notice? […]
Done
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@794 PS1, Line 794: /* VGA is first add-on card or the only onboard VGA. */
The code here is full of weird assumptions, the comment only adds to it... […]
Done
https://review.coreboot.org/c/coreboot/+/35516/2/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/2/src/device/device.c@799 PS2, Line 799:
Um, I guess it's best if you add it with your patch (wasn't sure if you want it […]
Done
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) brid […]
Done
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... PS1, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08
well, "matches" means we can assume the bit to be reserved for now... […]
Done
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
device/pci: Enable full 16-bit VGA port i/o decoding
So, the PCI to PCI bridge specification had a pitfall for us: Originally, when decoding i/o ports for legacy VGA cycles, bridges should only consider the 10 least significant bits of the port address. This means all VGA registers were aliased every 1024 ports! e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc.
However, it seems, we never reserved the aliased ports, resulting in silent conflicts we preallocated resources. We neither use much external VGA nor many i/o ports these days, so nobody noticed.
To avoid this mess, a bridge control bit (VGA16) was introduced in 2003 to enable decoding of 16-bit port addresses. As older systems seem rather safe and well tested, and newer systems should support this bit, we'll use it if possible and only warn if not.
With old (AGP era) hardware one will likely encounter a warning like this:
found VGA at PCI: 06:00.0 A bridge on the path doesn't support 16-bit VGA decoding!
This is not generally fatal, but makes unnoticed resource conflicts more likely.
Change-Id: Id7a07f069dd54331df79f605c6bcda37882a602d Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/35516 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Michael Niewöhner Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/device.c M src/device/pci_device.c M src/include/device/device.h M src/include/device/pci_def.h 4 files changed, 46 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Michael Niewöhner: Looks good to me, but someone else must approve
diff --git a/src/device/device.c b/src/device/device.c index 44d1f95..333f1f0 100644 --- a/src/device/device.c +++ b/src/device/device.c @@ -759,6 +759,10 @@ continue;
printk(BIOS_DEBUG, "found VGA at %s\n", dev_path(dev)); + if (dev->bus->no_vga16) { + printk(BIOS_WARNING, + "A bridge on the path doesn't support 16-bit VGA decoding!"); + }
if (dev->on_mainboard) { vga_onboard = dev; @@ -797,7 +801,7 @@ while (bus) { printk(BIOS_DEBUG, "Setting PCI_BRIDGE_CTL_VGA for bridge %s\n", dev_path(bus->dev)); - bus->bridge_ctrl |= PCI_BRIDGE_CTL_VGA; + bus->bridge_ctrl |= PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_VGA16; bus = (bus == bus->dev->bus) ? 0 : bus->dev->bus; } } diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 0a4b69b..191c846 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -793,6 +793,43 @@ };
/** + * Check for compatibility to route legacy VGA cycles through a bridge. + * + * Originally, when decoding i/o ports for legacy VGA cycles, bridges + * should only consider the 10 least significant bits of the port address. + * This means all VGA registers were aliased every 1024 ports! + * e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc. + * + * To avoid this mess, a bridge control bit (VGA16) was introduced in + * 2003 to enable decoding of 16-bit port addresses. As we don't want + * to make this any more complex for now, we use this bit if possible + * and only warn if it's not supported (in set_vga_bridge_bits()). + */ +static void pci_bridge_vga_compat(struct bus *const bus) +{ + uint16_t bridge_ctrl; + + bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); + + /* Ensure VGA decoding is disabled during probing (it should + be by default, but we run blobs nowadays) */ + bridge_ctrl &= ~PCI_BRIDGE_CTL_VGA; + pci_write_config16(bus->dev, PCI_BRIDGE_CONTROL, bridge_ctrl); + + /* If the upstream bridge doesn't support VGA16, we don't have to check */ + bus->no_vga16 |= bus->dev->bus->no_vga16; + if (bus->no_vga16) + return; + + /* Test if we can enable 16-bit decoding */ + bridge_ctrl |= PCI_BRIDGE_CTL_VGA16; + pci_write_config16(bus->dev, PCI_BRIDGE_CONTROL, bridge_ctrl); + bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); + + bus->no_vga16 = !(bridge_ctrl & PCI_BRIDGE_CTL_VGA16); +} + +/** * Detect the type of downstream bridge. * * This function is a heuristic to detect which type of bus is downstream @@ -1293,6 +1330,8 @@
bus = dev->link_list;
+ pci_bridge_vga_compat(bus); + pci_bridge_route(bus, PCI_ROUTE_SCAN);
do_scan_bus(bus, 0x00, 0xff); diff --git a/src/include/device/device.h b/src/include/device/device.h index cb37c09..991bd38 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -94,6 +94,7 @@ unsigned int reset_needed : 1; unsigned int disable_relaxed_ordering : 1; unsigned int ht_link_up : 1; + unsigned int no_vga16 : 1; /* No support for 16-bit VGA decoding */ };
/* diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index bc5bc79..c8b86d5 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -138,6 +138,7 @@ #define PCI_BRIDGE_CTL_SERR 0x02 /* The same for SERR forwarding */ #define PCI_BRIDGE_CTL_NO_ISA 0x04 /* Disable bridging of ISA ports */ #define PCI_BRIDGE_CTL_VGA 0x08 /* Forward VGA addresses */ +#define PCI_BRIDGE_CTL_VGA16 0x10 /* Enable 16-bit i/o port decoding */ #define PCI_BRIDGE_CTL_MASTER_ABORT 0x20 /* Report master aborts */ #define PCI_BRIDGE_CTL_BUS_RESET 0x40 /* Secondary bus reset */ /* Fast Back2Back enabled on secondary interface */
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Enable full 16-bit VGA port i/o decoding ......................................................................
Patch Set 4: Code-Review+2
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)