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 <address@hidden>
> ---
>  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);
>  }