qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH v2] pci: Teach PCI Bridges about VGA routing
Date: Fri, 1 Mar 2013 16:26:01 +0200

On Thu, Feb 28, 2013 at 02:58:13PM -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.
> 
> Signed-off-by: Alex Williamson <address@hidden>

Off-topic:

As I was reviewing this, I noted an unrelated bug: we don't make
palette snooping bit writeable.

        Bridges are not required to implement the VGA support mechanisms
        described in the following sections. However, if a bridge implements the
        support mechanisms for VGA compatible addressing, it must also implement
        the mechanisms for VGA palette snooping and vice versa.

Though I don't think we need to bother implementing palette snooping
just yet, I wonder whether we need to make it writeable.

Here's a list of things we don't implement:
- palette snooping
- subtractive decoding (optional)
- 10-bit addressing (isa aliases)

more?
I think we should have a comment in code for all this.


> ---
> 
> v2: BRIDGE_CONTROL is 2 bytes
> 
>  hw/pci/pci_bridge.c |   45 +++++++++++++++++++++++++++++++++++++++++++--
>  hw/pci/pci_bus.h    |   15 +++++++++++++++
>  2 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 995842a..ced0e95 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);
>      }
>  
> 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 {



reply via email to

[Prev in Thread] Current Thread [Next in Thread]