From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzgxj-0001nK-6L for qemu-devel@nongnu.org; Wed, 25 Jun 2014 02:45:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wzgxe-00048e-1K for qemu-devel@nongnu.org; Wed, 25 Jun 2014 02:45:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzgxd-00048V-GD for qemu-devel@nongnu.org; Wed, 25 Jun 2014 02:45:37 -0400 Date: Wed, 25 Jun 2014 09:45:45 +0300 From: "Michael S. Tsirkin" Message-ID: <20140625064545.GB25563@redhat.com> References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-3-git-send-email-tiejun.chen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1403662641-28526-3-git-send-email-tiejun.chen@intel.com> Subject: Re: [Qemu-devel] [v5][PATCH 2/5] xen, gfx passthrough: create pseudo intel isa bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tiejun Chen Cc: peter.maydell@linaro.org, xen-devel@lists.xensource.com, stefano.stabellini@eu.citrix.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, Kelly.Zytaruk@amd.com, yang.z.zhang@intel.com, anthony@codemonkey.ws, anthony.perard@citrix.com, pbonzini@redhat.com On Wed, Jun 25, 2014 at 10:17:18AM +0800, Tiejun Chen wrote: > ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 > to make graphics device passthrough work well for VMM, that only need > to expose this pseudo ISA bridge to let driver know the real hardware > underneath. > > The original patch is from Allen Kay > > Signed-off-by: Yang Zhang > Signed-off-by: Tiejun Chen > Cc: Allen Kay > --- > v5: > > * Don't set this ISA class property, instead, just fake this ISA bridge > with 00:1f.0. > > v4: > > * Remove some unnecessary "return" in void foo(). > > v3: > > * Fix some typos. > * Improve some return paths. > > v2: > > * Nothing is changed. > > hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c > index 461e526..974b7e9 100644 > --- a/hw/xen/xen_pt_graphics.c > +++ b/hw/xen/xen_pt_graphics.c > @@ -230,3 +230,64 @@ out: > g_free(bios); > return rc; > } > + > +static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len) > +{ > + return pci_default_read_config(d, addr, len); > +} > + > +static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v, > + int len) > +{ > + pci_default_write_config(d, addr, v, len); > +} > + > +static void isa_bridge_class_init(ObjectClass *klass, void *data) > +{ > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + > + k->config_read = isa_bridge_read_config; > + k->config_write = isa_bridge_write_config; > +}; You don't need these stubs, just don't fill anything, pci core will use defaults then. > + > +typedef struct { > + PCIDevice dev; > +} ISABridgeState; > + Nor do you need an empty structure if it has no state. > +static TypeInfo isa_bridge_info = { > + .name = "pseudo-intel-pch-isa-bridge", > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(ISABridgeState), > + .class_init = isa_bridge_class_init, > +}; > + > +static void xen_pt_graphics_register_types(void) > +{ > + type_register_static(&isa_bridge_info); > +} > + > +type_init(xen_pt_graphics_register_types) > + > +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) > +{ > + struct PCIDevice *dev; > + > + char rid; > + > + /* We havt to use a simple PCI device to fake this ISA bridge > + * to avoid making some confusion to BIOS and ACPI. > + */ A typo and confusing wording above, I'm not really sure what this comment means. Maybe: /* Create a fake ISA bridge device at the location expected by guests. */ > + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); > + > + qdev_init_nofail(&dev->qdev); > + > + pci_config_set_vendor_id(dev->config, hdev->vendor_id); > + pci_config_set_device_id(dev->config, hdev->device_id); > + > + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); Host PCI device is the VGA card? So why does it make sense to get it's vendor/device/revision and stick in the ISA bridge? Also change rid to uint8_t, you won't need to cast then. > + > + pci_config_set_revision(dev->config, rid); > + > + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); > + return 0; > +} > -- > 1.9.1