[Qemu-devel] [PATCH v3] pci: Teach PCI Bridges about VGA routing

[Alex Williamson]

Each PCI Bridge has a set of implied VGA regions that are enabled when
the VGA bit is set in the bridge control register.  This allows VGA
devices behind bridges.  Unfortunately with VGA Enable, which we
formerly allowed but didn't back, comes along some required VGA
baggage.  VGA Palette Snooping is required, along with VGA 16-bit
decoding.  We don't yet have support for palette snooping, but we do
make the bit writable on bridges.  We also don't have support for
10-bit VGA aliases, the default mode, but we enable the register, even
on root ports, to avoid confusing guests.  Fortunately there's likely
nothing from this century that requires these features, so the missing
bits are noted with TODOs.

Signed-off-by: Alex Williamson <address@hidden>
---
v2: BRIDGE_CONTROL is 2 bytes
v3: Add missing comments and bits around VGA Palette Snooping and aliases

 hw/pci/pci.c        |    4 ++++
 hw/pci/pci_bridge.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/pci/pci_bus.h    |   15 ++++++++++++++
 hw/pci/pcie_port.c  |    2 ++
 4 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f45c8f..2ea831d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -674,6 +674,10 @@ static void pci_init_mask_bridge(PCIDevice *d)
 #define  PCI_BRIDGE_CTL_SEC_DISCARD    0x200   /* Secondary discard timer */
 #define  PCI_BRIDGE_CTL_DISCARD_STATUS 0x400   /* Discard timer status */
 #define  PCI_BRIDGE_CTL_DISCARD_SERR   0x800   /* Discard timer SERR# enable */
+/*
+ * TODO: Bridges default to 10-bit VGA decoding but we currently only
+ * implement 16-bit decoding (no alias support).
+ */
     pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
                  PCI_BRIDGE_CTL_PARITY |
                  PCI_BRIDGE_CTL_SERR |
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 995842a..5f9d686 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -151,6 +151,37 @@ static void pci_bridge_init_alias(PCIBridge *bridge, 
MemoryRegion *alias,
     memory_region_add_subregion_overlap(parent_space, base, alias, 1);
 }
 
+static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent,
+                                        PCIBridgeVgaWindows *vga)
+{
+    uint16_t cmd = pci_get_word(br->dev.config + PCI_COMMAND);
+    uint16_t brctl = pci_get_word(br->dev.config + PCI_BRIDGE_CONTROL);
+
+    memory_region_init_alias(&vga->alias_io_lo, "pci_bridge_vga_io_lo",
+                             &br->address_space_io, 0x3b0, 0xc);
+    memory_region_add_subregion_overlap(parent->address_space_io, 0x3b0,
+                                        &vga->alias_io_lo, 1);
+
+    memory_region_init_alias(&vga->alias_io_hi, "pci_bridge_vga_io_hi",
+                             &br->address_space_io, 0x3c0, 0x20);
+    memory_region_add_subregion_overlap(parent->address_space_io, 0x3c0,
+                                        &vga->alias_io_hi, 1);
+
+    if (!(cmd & PCI_COMMAND_IO) || !(brctl & PCI_BRIDGE_CTL_VGA)) {
+        memory_region_set_enabled(&vga->alias_io_lo, false);
+        memory_region_set_enabled(&vga->alias_io_hi, false);
+    }
+
+    memory_region_init_alias(&vga->alias_mem, "pci_bridge_vga_mem",
+                             &br->address_space_mem, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(parent->address_space_mem, 0xa0000,
+                                        &vga->alias_mem, 1);
+
+    if (!(cmd & PCI_COMMAND_MEMORY) || !(brctl & PCI_BRIDGE_CTL_VGA)) {
+        memory_region_set_enabled(&vga->alias_mem, false);
+    }
+}
+
 static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br)
 {
     PCIBus *parent = br->dev.bus;
@@ -175,7 +206,8 @@ static PCIBridgeWindows *pci_bridge_region_init(PCIBridge 
*br)
                           &br->address_space_io,
                           parent->address_space_io,
                           cmd & PCI_COMMAND_IO);
-   /* TODO: optinal VGA and VGA palette snooping support. */
+
+    pci_bridge_init_vga_aliases(br, parent, &w->vga);
 
     return w;
 }
@@ -187,6 +219,9 @@ static void pci_bridge_region_del(PCIBridge *br, 
PCIBridgeWindows *w)
     memory_region_del_subregion(parent->address_space_io, &w->alias_io);
     memory_region_del_subregion(parent->address_space_mem, &w->alias_mem);
     memory_region_del_subregion(parent->address_space_mem, &w->alias_pref_mem);
+    memory_region_del_subregion(parent->address_space_io, &w->vga.alias_io_lo);
+    memory_region_del_subregion(parent->address_space_io, &w->vga.alias_io_hi);
+    memory_region_del_subregion(parent->address_space_mem, &w->vga.alias_mem);
 }
 
 static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
@@ -194,6 +229,9 @@ static void pci_bridge_region_cleanup(PCIBridge *br, 
PCIBridgeWindows *w)
     memory_region_destroy(&w->alias_io);
     memory_region_destroy(&w->alias_mem);
     memory_region_destroy(&w->alias_pref_mem);
+    memory_region_destroy(&w->vga.alias_io_lo);
+    memory_region_destroy(&w->vga.alias_io_hi);
+    memory_region_destroy(&w->vga.alias_mem);
     g_free(w);
 }
 
@@ -227,7 +265,10 @@ void pci_bridge_write_config(PCIDevice *d,
 
         /* memory base/limit, prefetchable base/limit and
            io base/limit upper 16 */
-        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
+        ranges_overlap(address, len, PCI_MEMORY_BASE, 20) ||
+
+        /* vga enable */
+        ranges_overlap(address, len, PCI_BRIDGE_CONTROL, 2)) {
         pci_bridge_update_mappings(s);
     }
 
@@ -306,6 +347,17 @@ int pci_bridge_initfn(PCIDevice *dev)
 
     pci_word_test_and_set_mask(dev->config + PCI_STATUS,
                                PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
+
+    /*
+     * TODO: We implement VGA Enable in the Bridge Control Register
+     * therefore per the PCI spec we must also implement VGA Palette
+     * Snooping.  We set this bit writable, but there's not yet
+     * backing for doing positive decode on the subset of VGA
+     * registers required for this.
+     */
+    pci_word_test_and_set_mask(dev->wmask + PCI_COMMAND,
+                               PCI_COMMAND_VGA_PALETTE);
+
     pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
     dev->config[PCI_HEADER_TYPE] =
         (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
diff --git a/hw/pci/pci_bus.h b/hw/pci/pci_bus.h
index f905b9e..60d5378 100644
--- a/hw/pci/pci_bus.h
+++ b/hw/pci/pci_bus.h
@@ -36,6 +36,20 @@ struct PCIBus {
     int *irq_count;
 };
 
+typedef struct PCIBridgeVgaWindows PCIBridgeVgaWindows;
+
+/*
+ * When bridge control VGA forwarding is enabled, bridges will provide
+ * positive decode on the PCI VGA defined I/O port and MMIO ranges noted
+ * below.  When set, forwarding is only qualified on the I/O and memory
+ * enable bits in the bridge command register.
+ */
+struct PCIBridgeVgaWindows {
+    MemoryRegion alias_io_lo; /* I/O ports 0x3b0 - 0x3bb */
+    MemoryRegion alias_io_hi; /* I/O ports 0x3c0 - 0x3df */
+    MemoryRegion alias_mem; /* MMIO 0xa0000 - 0xbffff */
+};
+
 typedef struct PCIBridgeWindows PCIBridgeWindows;
 
 /*
@@ -47,6 +61,7 @@ struct PCIBridgeWindows {
     MemoryRegion alias_pref_mem;
     MemoryRegion alias_mem;
     MemoryRegion alias_io;
+    PCIBridgeVgaWindows vga;
 };
 
 struct PCIBridge {
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 33a6b0a..1be107b 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -28,10 +28,12 @@ void pcie_port_init_reg(PCIDevice *d)
     pci_set_word(d->config + PCI_SEC_STATUS, 0);
 
     /* Unlike conventional pci bridge, some bits are hardwired to 0. */
+#define  PCI_BRIDGE_CTL_VGA_16BIT       0x10    /* VGA 16-bit decode */
     pci_set_word(d->wmask + PCI_BRIDGE_CONTROL,
                  PCI_BRIDGE_CTL_PARITY |
                  PCI_BRIDGE_CTL_ISA |
                  PCI_BRIDGE_CTL_VGA |
+                 PCI_BRIDGE_CTL_VGA_16BIT | /* Req, but no alias support yet */
                  PCI_BRIDGE_CTL_SERR |
                  PCI_BRIDGE_CTL_BUS_RESET);
 }