From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WziZV-0005Pt-VJ for qemu-devel@nongnu.org; Wed, 25 Jun 2014 04:28:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WziZP-0000f6-Vh for qemu-devel@nongnu.org; Wed, 25 Jun 2014 04:28:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14519) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WziZP-0000ex-OR for qemu-devel@nongnu.org; Wed, 25 Jun 2014 04:28:43 -0400 Date: Wed, 25 Jun 2014 11:28:50 +0300 From: "Michael S. Tsirkin" Message-ID: <20140625082850.GB32652@redhat.com> 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> <53AA8404.8040708@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53AA8404.8040708@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: "Chen, Tiejun" 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 04:10:44PM +0800, Chen, Tiejun wrote: > 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 So vendor/device/revision IDs must match real device then? Stick this in the comment then. In fact it's exactly what passthrough does. I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs? > > > >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 > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [v5][PATCH 2/5] xen, gfx passthrough: create pseudo intel isa bridge Date: Wed, 25 Jun 2014 11:28:50 +0300 Message-ID: <20140625082850.GB32652@redhat.com> 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> <53AA8404.8040708@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <53AA8404.8040708@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Chen, Tiejun" 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 List-Id: xen-devel@lists.xenproject.org On Wed, Jun 25, 2014 at 04:10:44PM +0800, Chen, Tiejun wrote: > 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 So vendor/device/revision IDs must match real device then? Stick this in the comment then. In fact it's exactly what passthrough does. I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs? > > > >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 > > > >