[Qemu-devel] [PATCH v4 0/2] PCI learns about VGA

[Qemu-devel] [PATCH v4 0/2] PCI learns about VGA

[Alex Williamson]

Bridges aren't the only thing that needs to know about VGA, any class
VGA device can register for VGA.  This adds infrastructure to PCI to
allow devices to register VGA regions and incorporates the previous
PCI bridge only patch to also make use of this.  vfio-pci will also
use the pci_register_vga() interface when attaching VGA devices.

v4: Add common pci_vga_[un]register helpers and convert pci_bridge
v3: pci_bridge only: add comments and enable bits for snooping & alias
v2: pci_bridge only: BRIDGE_CONTROL is 2 bytes

---

Alex Williamson (2):
      pci: Add PCI VGA helpers
      pci: Teach PCI Bridges about VGA routing


 hw/pci/pci.c        |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pci.h        |   21 ++++++++++++++++
 hw/pci/pci_bridge.c |   45 ++++++++++++++++++++++++++++++++++-
 hw/pci/pci_bus.h    |    7 +++++
 hw/pci/pcie_port.c  |    2 ++
 5 files changed, 138 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH v4 1/2] pci: Add PCI VGA helpers

[Alex Williamson]

Allow devices to register VGA memory regions for handling PCI spec
defined VGA I/O port and MMIO areas.  PCI will attach these to the
bus address spaces and enable them according to the device command
register value.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci/pci.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pci.h |   21 ++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f45c8f..ed43111 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -875,6 +875,8 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
             continue;
         memory_region_del_subregion(r->address_space, r->memory);
     }
+
+    pci_unregister_vga(pci_dev);
 }
 
 static int pci_unregister_device(DeviceState *dev)
@@ -937,6 +939,63 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
         : pci_dev->bus->address_space_mem;
 }
 
+static void pci_update_vga(PCIDevice *pci_dev)
+{
+    uint16_t cmd;
+
+    if (!pci_dev->has_vga) {
+        return;
+    }
+
+    cmd = pci_get_word(pci_dev->config + PCI_COMMAND);
+
+    memory_region_set_enabled(pci_dev->vga_regions[QEMU_PCI_VGA_MEM],
+                              cmd & PCI_COMMAND_MEMORY);
+    memory_region_set_enabled(pci_dev->vga_regions[QEMU_PCI_VGA_IO_LO],
+                              cmd & PCI_COMMAND_IO);
+    memory_region_set_enabled(pci_dev->vga_regions[QEMU_PCI_VGA_IO_HI],
+                              cmd & PCI_COMMAND_IO);
+}
+
+void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
+                      MemoryRegion *io_lo, MemoryRegion *io_hi)
+{
+    assert(!pci_dev->has_vga);
+
+    assert(memory_region_size(mem) == QEMU_PCI_VGA_MEM_SIZE);
+    pci_dev->vga_regions[QEMU_PCI_VGA_MEM] = mem;
+    memory_region_add_subregion_overlap(pci_dev->bus->address_space_mem,
+                                        QEMU_PCI_VGA_MEM_BASE, mem, 1);
+
+    assert(memory_region_size(io_lo) == QEMU_PCI_VGA_IO_LO_SIZE);
+    pci_dev->vga_regions[QEMU_PCI_VGA_IO_LO] = io_lo;
+    memory_region_add_subregion_overlap(pci_dev->bus->address_space_io,
+                                        QEMU_PCI_VGA_IO_LO_BASE, io_lo, 1);
+
+    assert(memory_region_size(io_hi) == QEMU_PCI_VGA_IO_HI_SIZE);
+    pci_dev->vga_regions[QEMU_PCI_VGA_IO_HI] = io_hi;
+    memory_region_add_subregion_overlap(pci_dev->bus->address_space_io,
+                                        QEMU_PCI_VGA_IO_HI_BASE, io_hi, 1);
+    pci_dev->has_vga = true;
+
+    pci_update_vga(pci_dev);
+}
+
+void pci_unregister_vga(PCIDevice *pci_dev)
+{
+    if (!pci_dev->has_vga) {
+        return;
+    }
+
+    memory_region_del_subregion(pci_dev->bus->address_space_mem,
+                                pci_dev->vga_regions[QEMU_PCI_VGA_MEM]);
+    memory_region_del_subregion(pci_dev->bus->address_space_io,
+                                pci_dev->vga_regions[QEMU_PCI_VGA_IO_LO]);
+    memory_region_del_subregion(pci_dev->bus->address_space_io,
+                                pci_dev->vga_regions[QEMU_PCI_VGA_IO_HI]);
+    pci_dev->has_vga = false;
+}
+
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num)
 {
     return pci_dev->io_regions[region_num].addr;
@@ -1036,6 +1095,8 @@ static void pci_update_mappings(PCIDevice *d)
                                                 r->addr, r->memory, 1);
         }
     }
+
+    pci_update_vga(d);
 }
 
 static inline int pci_irq_disabled(PCIDevice *d)
diff --git a/hw/pci/pci.h b/hw/pci/pci.h
index f340fe5..d837a65 100644
--- a/hw/pci/pci.h
+++ b/hw/pci/pci.h
@@ -108,6 +108,20 @@ typedef struct PCIIORegion {
 #define PCI_ROM_SLOT 6
 #define PCI_NUM_REGIONS 7
 
+enum {
+    QEMU_PCI_VGA_MEM,
+    QEMU_PCI_VGA_IO_LO,
+    QEMU_PCI_VGA_IO_HI,
+    QEMU_PCI_VGA_NUM_REGIONS,
+};
+
+#define QEMU_PCI_VGA_MEM_BASE 0xa0000
+#define QEMU_PCI_VGA_MEM_SIZE 0x20000
+#define QEMU_PCI_VGA_IO_LO_BASE 0x3b0
+#define QEMU_PCI_VGA_IO_LO_SIZE 0xc
+#define QEMU_PCI_VGA_IO_HI_BASE 0x3c0
+#define QEMU_PCI_VGA_IO_HI_SIZE 0x20
+
 #include "hw/pci/pci_regs.h"
 
 /* PCI HEADER_TYPE */
@@ -234,6 +248,10 @@ struct PCIDevice {
     /* IRQ objects for the INTA-INTD pins.  */
     qemu_irq *irq;
 
+    /* Legacy PCI VGA regions */
+    MemoryRegion *vga_regions[QEMU_PCI_VGA_NUM_REGIONS];
+    bool has_vga;
+
     /* Current IRQ levels.  Used internally by the generic PCI code.  */
     uint8_t irq_state;
 
@@ -287,6 +305,9 @@ struct PCIDevice {
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
                       uint8_t attr, MemoryRegion *memory);
+void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
+                      MemoryRegion *io_lo, MemoryRegion *io_hi);
+void pci_unregister_vga(PCIDevice *pci_dev);
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,

[Qemu-devel] [PATCH v4 2/2] 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 <alex.williamson@redhat.com>
---
 hw/pci/pci.c        |    4 ++++
 hw/pci/pci_bridge.c |   45 +++++++++++++++++++++++++++++++++++++++++++--
 hw/pci/pci_bus.h    |    7 +++++++
 hw/pci/pcie_port.c  |    2 ++
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ed43111..a881602 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..84e7c19 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -151,6 +151,28 @@ 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,
+                                        MemoryRegion *alias_vga)
+{
+    uint16_t brctl = pci_get_word(br->dev.config + PCI_BRIDGE_CONTROL);
+
+    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO],
+                             "pci_bridge_vga_io_lo", &br->address_space_io,
+                             QEMU_PCI_VGA_IO_LO_BASE, QEMU_PCI_VGA_IO_LO_SIZE);
+    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_HI],
+                             "pci_bridge_vga_io_hi", &br->address_space_io,
+                             QEMU_PCI_VGA_IO_HI_BASE, QEMU_PCI_VGA_IO_HI_SIZE);
+    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_MEM],
+                             "pci_bridge_vga_mem", &br->address_space_mem,
+                             QEMU_PCI_VGA_MEM_BASE, QEMU_PCI_VGA_MEM_SIZE);
+
+    if (brctl & PCI_BRIDGE_CTL_VGA) {
+        pci_register_vga(&br->dev, &alias_vga[QEMU_PCI_VGA_MEM],
+                         &alias_vga[QEMU_PCI_VGA_IO_LO],
+                         &alias_vga[QEMU_PCI_VGA_IO_HI]);
+    }
+}
+
 static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br)
 {
     PCIBus *parent = br->dev.bus;
@@ -175,7 +197,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->alias_vga);
 
     return w;
 }
@@ -187,6 +210,7 @@ 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);
+    pci_unregister_vga(&br->dev);
 }
 
 static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
@@ -194,6 +218,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->alias_vga[QEMU_PCI_VGA_IO_LO]);
+    memory_region_destroy(&w->alias_vga[QEMU_PCI_VGA_IO_HI]);
+    memory_region_destroy(&w->alias_vga[QEMU_PCI_VGA_MEM]);
     g_free(w);
 }
 
@@ -227,7 +254,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 +336,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..aef559a 100644
--- a/hw/pci/pci_bus.h
+++ b/hw/pci/pci_bus.h
@@ -47,6 +47,13 @@ struct PCIBridgeWindows {
     MemoryRegion alias_pref_mem;
     MemoryRegion alias_mem;
     MemoryRegion alias_io;
+    /*
+     * When bridge control VGA forwarding is enabled, bridges will
+     * provide positive decode on the PCI VGA defined I/O port and
+     * MMIO ranges.  When enabled forwarding is only qualified on the
+     * I/O and memory enable bits in the bridge command register.
+     */
+    MemoryRegion alias_vga[QEMU_PCI_VGA_NUM_REGIONS];
 };
 
 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);
 }

Re: [Qemu-devel] [PATCH v4 2/2] pci: Teach PCI Bridges about VGA routing

[Peter Maydell]

On 4 March 2013 01:21, Alex Williamson <alex.williamson@redhat.com> wrote:
> --- 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 */

Shouldn't this #define be in pci_regs.h with the other PCI_BRIDGE_CTL_*
constants?

>      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);
>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] pci: Teach PCI Bridges about VGA routing

[Alex Williamson]

On Mon, 2013-03-04 at 09:39 +0800, Peter Maydell wrote:
> On 4 March 2013 01:21, Alex Williamson <alex.williamson@redhat.com> wrote:
> > --- 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 */
> 
> Shouldn't this #define be in pci_regs.h with the other PCI_BRIDGE_CTL_*
> constants?

See the existing define in pci.c.  pci_regs.h is derived from the Linux
kernel header, which is not 100% complete.  Ideally it would contain
this, but it doesn't currently so I'm following the existing example.
Thanks,

Alex

> >      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);
> >  }
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v4 2/2] pci: Teach PCI Bridges about VGA routing

[Michael S. Tsirkin]

On Sun, Mar 03, 2013 at 10:21:32AM -0700, Alex Williamson wrote:
> 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 <alex.williamson@redhat.com>
> ---
>  hw/pci/pci.c        |    4 ++++
>  hw/pci/pci_bridge.c |   45 +++++++++++++++++++++++++++++++++++++++++++--
>  hw/pci/pci_bus.h    |    7 +++++++
>  hw/pci/pcie_port.c  |    2 ++
>  4 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index ed43111..a881602 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..84e7c19 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -151,6 +151,28 @@ 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,
> +                                        MemoryRegion *alias_vga)
> +{
> +    uint16_t brctl = pci_get_word(br->dev.config + PCI_BRIDGE_CONTROL);
> +
> +    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO],
> +                             "pci_bridge_vga_io_lo", &br->address_space_io,
> +                             QEMU_PCI_VGA_IO_LO_BASE, QEMU_PCI_VGA_IO_LO_SIZE);
> +    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_HI],
> +                             "pci_bridge_vga_io_hi", &br->address_space_io,
> +                             QEMU_PCI_VGA_IO_HI_BASE, QEMU_PCI_VGA_IO_HI_SIZE);
> +    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_MEM],
> +                             "pci_bridge_vga_mem", &br->address_space_mem,
> +                             QEMU_PCI_VGA_MEM_BASE, QEMU_PCI_VGA_MEM_SIZE);
> +
> +    if (brctl & PCI_BRIDGE_CTL_VGA) {
> +        pci_register_vga(&br->dev, &alias_vga[QEMU_PCI_VGA_MEM],
> +                         &alias_vga[QEMU_PCI_VGA_IO_LO],
> +                         &alias_vga[QEMU_PCI_VGA_IO_HI]);
> +    }
> +}
> +
>  static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br)
>  {
>      PCIBus *parent = br->dev.bus;
> @@ -175,7 +197,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->alias_vga);
>  
>      return w;
>  }
> @@ -187,6 +210,7 @@ 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);
> +    pci_unregister_vga(&br->dev);
>  }
>  
>  static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
> @@ -194,6 +218,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->alias_vga[QEMU_PCI_VGA_IO_LO]);
> +    memory_region_destroy(&w->alias_vga[QEMU_PCI_VGA_IO_HI]);
> +    memory_region_destroy(&w->alias_vga[QEMU_PCI_VGA_MEM]);
>      g_free(w);
>  }
>  
> @@ -227,7 +254,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 +336,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);
> +

I've replaced this one with a comment. Both what this patch does and the
current behaviour are out of spec, but less change seems like a prudent
thing to do.

>      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..aef559a 100644
> --- a/hw/pci/pci_bus.h
> +++ b/hw/pci/pci_bus.h
> @@ -47,6 +47,13 @@ struct PCIBridgeWindows {
>      MemoryRegion alias_pref_mem;
>      MemoryRegion alias_mem;
>      MemoryRegion alias_io;
> +    /*
> +     * When bridge control VGA forwarding is enabled, bridges will
> +     * provide positive decode on the PCI VGA defined I/O port and
> +     * MMIO ranges.  When enabled forwarding is only qualified on the
> +     * I/O and memory enable bits in the bridge command register.
> +     */
> +    MemoryRegion alias_vga[QEMU_PCI_VGA_NUM_REGIONS];
>  };
>  
>  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);
>  }

Re: [Qemu-devel] [PATCH v4 0/2] PCI learns about VGA

[Michael S. Tsirkin]

On Sun, Mar 03, 2013 at 10:21:20AM -0700, Alex Williamson wrote:
> Bridges aren't the only thing that needs to know about VGA, any class
> VGA device can register for VGA.  This adds infrastructure to PCI to
> allow devices to register VGA regions and incorporates the previous
> PCI bridge only patch to also make use of this.  vfio-pci will also
> use the pci_register_vga() interface when attaching VGA devices.
> 
> v4: Add common pci_vga_[un]register helpers and convert pci_bridge
> v3: pci_bridge only: add comments and enable bits for snooping & alias
> v2: pci_bridge only: BRIDGE_CONTROL is 2 bytes

Thanks, applied.

> ---
> 
> Alex Williamson (2):
>       pci: Add PCI VGA helpers
>       pci: Teach PCI Bridges about VGA routing
> 
> 
>  hw/pci/pci.c        |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci/pci.h        |   21 ++++++++++++++++
>  hw/pci/pci_bridge.c |   45 ++++++++++++++++++++++++++++++++++-
>  hw/pci/pci_bus.h    |    7 +++++
>  hw/pci/pcie_port.c  |    2 ++
>  5 files changed, 138 insertions(+), 2 deletions(-)