[v4,2/2] pci: Teach PCI Bridges about VGA routing

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(-)

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

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

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