From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UBQuF-0008GF-Ei for qemu-devel@nongnu.org; Fri, 01 Mar 2013 09:25:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UBQuB-0006bS-Uf for qemu-devel@nongnu.org; Fri, 01 Mar 2013 09:25:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59439) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UBQuB-0006b4-ME for qemu-devel@nongnu.org; Fri, 01 Mar 2013 09:25:47 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r21EPkmC006385 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 1 Mar 2013 09:25:46 -0500 Date: Fri, 1 Mar 2013 16:26:01 +0200 From: "Michael S. Tsirkin" Message-ID: <20130301142601.GB30955@redhat.com> References: <20130228215707.30171.86137.stgit@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130228215707.30171.86137.stgit@bling.home> Subject: Re: [Qemu-devel] [PATCH v2] pci: Teach PCI Bridges about VGA routing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org 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 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 {