On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote: > On 2014/11/17 14:10, Michael S. Tsirkin wrote: > >On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote: > >>On 2014/11/5 22:09, Michael S. Tsirkin wrote: > >>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote: > >>>>Currently IGD drivers always need to access PCH by 1f.0, and > >>>>PCH vendor/device id is used to identify the card. > >>>> > >>>>Signed-off-by: Tiejun Chen <tiejun.c...@intel.com> > >>>>--- > >>>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++- > >>>> 1 file changed, 27 insertions(+), 1 deletion(-) > >>>> > >>>>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >>>>index b559181..b19c7a9 100644 > >>>>--- a/hw/i386/pc_piix.c > >>>>+++ b/hw/i386/pc_piix.c > >>>>@@ -50,7 +50,7 @@ > >>>> #include "cpu.h" > >>>> #include "qemu/error-report.h" > >>>> #ifdef CONFIG_XEN > >>>>-# include <xen/hvm/hvm_info_table.h> > >>>>+#include <xen/hvm/hvm_info_table.h> > >>>> #endif > >>>> > >>>> #define MAX_IDE_BUS 2 > >>>>@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine) > >>>> } > >>>> > >>>> #ifdef CONFIG_XEN > >>>>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus) > >>>>+{ > >>>>+ struct PCIDevice *dev; > >>>>+ Error *local_err = NULL; > >>>>+ uint16_t device_id = 0xffff; > >>>>+ > >>>>+ /* Currently IGD drivers always need to access PCH by 1f.0. */ > >>>>+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0), > >>>>+ "xen-igd-passthrough-isa-bridge"); > >>>>+ > >>>>+ /* Identify PCH card with its own real vendor/device ids. > >>>>+ * Here that vendor id is always PCI_VENDOR_ID_INTEL. > >>>>+ */ > >>>>+ if (dev) { > >>>>+ device_id = object_property_get_int(OBJECT(dev), "device-id", > >>>>+ &local_err); > >>>>+ if (!local_err && device_id != 0xffff) { > >>>>+ pci_config_set_device_id(dev->config, device_id); > >>>>+ return; > >>>>+ } > >>>>+ } > >>>>+ > >>>>+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n"); > >>>>+} > >>>>+ > >>>> static void pc_xen_hvm_init(MachineState *machine) > >>>> { > >>>> PCIBus *bus; > >>>>@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine) > >>>> bus = pci_find_primary_bus(); > >>>> if (bus != NULL) { > >>>> pci_create_simple(bus, -1, "xen-platform"); > >>>>+ xen_igd_passthrough_isa_bridge_create(bus); > >>>> } > >>>> } > >>>> #endif > >>> > >>>Can't we defer this step until the GPU is added? > >> > >>Sounds great but I can't figure out where we can to do this exactly. > >> > >>>This way there won't be need to poke at host device > >>>directly, you could get all info from dev->config > >>>of the host device. > >> > >>As I understand We have two steps here: > >> > >>#1 At first I have to write something to check if we're registering 00:02.0 > >>& IGD, right? But where? While registering each pci device? > > > >In xen_pt_initfn. > >Just check the device and vendor ID against the table you have. > > > > Okay. Please see the follows which is just compiled: > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index c6466dc..f3ea313 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = { > .priority = 10, > }; > > +typedef struct { > + uint16_t gpu_device_id; > + uint16_t pch_device_id; > +} XenIGDDeviceIDInfo; > + > +/* In real world different GPU should have different PCH. But actually > + * the different PCH DIDs likely map to different PCH SKUs. We do the > + * same thing for the GPU. For PCH, the different SKUs are going to be > + * all the same silicon design and implementation, just different > + * features turn on and off with fuses. The SW interfaces should be > + * consistent across all SKUs in a given family (eg LPT). But just same > + * features may not be supported. > + * > + * Most of these different PCH features probably don't matter to the > + * Gfx driver, but obviously any difference in display port connections > + * will so it should be fine with any PCH in case of passthrough. > + * > + * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell) > + * scenarios, 0x9cc3 for BDW(Broadwell). > + */ > +static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = { > + /* HSW Classic */ > + {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */ > + {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */ > + {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */ > + {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */ > + {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */ > + /* HSW ULT */ > + {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */ > + {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */ > + {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */ > + {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */ > + {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */ > + {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */ > + /* HSW CRW */ > + {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */ > + {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */ > + /* HSW Server */ > + {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */ > + /* HSW SRVR */ > + {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */ > + /* BSW */ > + {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */ > + {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */ > + {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */ > + {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */ > + {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */ > + {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */ > + {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */ > + {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */ > + {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */ > + {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */ > + {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */ > +}; > + > +static void > +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s, > + XenHostPCIDevice *dev) > +{ > + struct PCIDevice *pci_dev; > + int i, num; > + uint16_t gpu_id = 0xffff,
no need to init gpu_id. > pch_id = 0xffff; > + PCIDevice *d = &s->dev; > + > + if (is_vga_passthrough(dev)) { I would do if (!is_vga_passthrough) { return; } here. Makes following code shorter. > + gpu_id = dev->device_id; > + num = ARRAY_SIZE(xen_igd_combo_id_infos); > + for (i = 0; i < num; i++) > + if (gpu_id == xen_igd_combo_id_infos[i].gpu_device_id) > + pch_id = xen_igd_combo_id_infos[i].pch_device_id; Add {} to match our coding style please. At this point I would do if (pch_id == 0xffff) { return; } maybe replace 0xffff with 0x0, then you can do if (!pch_id) { return; }. > + > + /* Currently IGD drivers always need to access PCH by 1f.0. */ > + pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0), > + "xen-igd-passthrough-isa-bridge"); > + > + /* > + * Identify PCH card with its own real vendor/device ids. > + * Here that vendor id is always PCI_VENDOR_ID_INTEL. s/Here/Note/ > + */ > + if (pci_dev) { > + pci_config_set_device_id(pci_dev->config, pch_id); > + return; > + } > + > + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge > failed!\n"); Cleaner: if (!pci_dev) { fprintf return; } pci_config_set_device_id(pci_dev->config, pch_id); > + } > +} > + > /* init */ > > static int xen_pt_initfn(PCIDevice *d) > @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d) > return -1; > } > > + /* Register ISA bridge for passthrough GFX. */ > + xen_igd_passthrough_isa_bridge_create(s, &s->real_device); > + > /* reinitialize each config register to be emulated */ > if (xen_pt_config_init(s)) { > XEN_PT_ERR(d, "PCI Config space initialisation failed.\n"); > > Note I will introduce a inline function in another patch, > > +static inline int is_vga_passthrough(XenHostPCIDevice *dev) > +{ > + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL) > + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA)); > +} > > Thanks > Tiejun Why bother with all these conditions? Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL? -- MST