Nico Huber would like Kyösti Mälkki, Patrick Rudolph and Michael Niewöhner to review this change.
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 */
To view, visit change 35516. To unsubscribe, or for help writing mail filters, visit settings.