From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44750) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WziIw-0003pe-O0 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 04:11:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WziIo-0003KZ-6i for qemu-devel@nongnu.org; Wed, 25 Jun 2014 04:11:42 -0400 Received: from mga02.intel.com ([134.134.136.20]:8381) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WziIo-0003KH-10 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 04:11:34 -0400 Message-ID: <53AA8404.8040708@intel.com> Date: Wed, 25 Jun 2014 16:10:44 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1403662641-28526-1-git-send-email-tiejun.chen@intel.com> <1403662641-28526-3-git-send-email-tiejun.chen@intel.com> <20140625064545.GB25563@redhat.com> In-Reply-To: <20140625064545.GB25563@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" 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 2014/6/25 14:45, Michael S. Tsirkin wrote: > 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. I guess these stubs are left to extend something in the future. But maybe we can remove them now. > >> + >> +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. */ > Good comments so thanks so much. > >> + 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? This is a real ISA bridge. > So why does it make sense to get it's vendor/device/revision and > stick in the ISA bridge? The Intel generation of integrated graphics needs to probe this ISA bridge to initialize the i915 driver properly. Thanks Tiejun > > 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 > >