From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38752) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UCROc-0003y1-TD for qemu-devel@nongnu.org; Mon, 04 Mar 2013 04:09:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UCROZ-0008Nr-Fe for qemu-devel@nongnu.org; Mon, 04 Mar 2013 04:09:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UCROZ-0008Nk-7r for qemu-devel@nongnu.org; Mon, 04 Mar 2013 04:09:19 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2499IOP030536 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 4 Mar 2013 04:09:18 -0500 Date: Mon, 4 Mar 2013 11:09:37 +0200 From: "Michael S. Tsirkin" Message-ID: <20130304090937.GA29924@redhat.com> References: <20130303171901.15425.61733.stgit@bling.home> <20130303172132.15425.15034.stgit@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130303172132.15425.15034.stgit@bling.home> Subject: Re: [Qemu-devel] [PATCH v4 2/2] 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 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 > --- > 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); > }