[Qemu-devel] [RFC][PATCH 1/2] hw:xen:xen_pt: register isa bridge specific to IGD passthrough

[Qemu-devel] [RFC][PATCH 1/2] hw:xen:xen_pt: register isa bridge specific to IGD passthrough

[Tiejun Chen]

We need this instance to passthrough some config fields of PCH.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/xen/xen_pt.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c1bf357..403c33f 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -60,6 +60,7 @@
 #include "xen_pt.h"
 #include "qemu/range.h"
 #include "exec/address-spaces.h"
+#include "qapi/visitor.h"
 
 #define XEN_PT_NR_IRQS (256)
 static uint8_t xen_pt_mapped_machine_irq[XEN_PT_NR_IRQS] = {0};
@@ -829,3 +830,114 @@ static void xen_pci_passthrough_register_types(void)
 }
 
 type_init(xen_pci_passthrough_register_types)
+
+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_pciisabridge_get_pci_device_id(Object *obj,
+                                                               Visitor *v,
+                                                               void *opaque,
+                                                               const char *name,
+                                                               Error **errp)
+{
+    uint32_t gpu_id = 0xffff, pch_id = 0xffff;
+    XenHostPCIDevice hdev;
+    int r = 0, num;
+
+    r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
+    if (!r) {
+        gpu_id = hdev.device_id;
+
+        num = ARRAY_SIZE(xen_igd_combo_id_infos);
+        for (r = 0; r < num; r++)
+            if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
+                pch_id = xen_igd_combo_id_infos[r].pch_device_id;
+    }
+
+    visit_type_uint32(v, &pch_id, name, errp);
+}
+
+static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
+{
+    object_property_add(obj, "device-id", "int",
+                        xen_igd_passthrough_pciisabridge_get_pci_device_id,
+                        NULL, NULL, NULL, NULL);
+}
+
+static void xen_igd_passthrough_isa_bridge_class_init(ObjectClass *klass,
+                                                      void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    dc->desc        = "XEN IGD PASSTHROUGH ISA bridge";
+    k->class_id     = PCI_CLASS_BRIDGE_ISA;
+    k->vendor_id    = PCI_VENDOR_ID_INTEL;
+};
+
+static TypeInfo xen_igd_passthrough_isa_bridge_info = {
+    .name           = "xen-igd-passthrough-isa-bridge",
+    .parent         = TYPE_PCI_DEVICE,
+    .instance_size  = sizeof(PCIDevice),
+    .instance_init  = xen_igd_passthrough_isa_bridge_initfn,
+    .class_init     = xen_igd_passthrough_isa_bridge_class_init,
+};
+
+static void xen_igd_passthrough_isa_bridge_register_types(void)
+{
+    type_register_static(&xen_igd_passthrough_isa_bridge_info);
+}
+
+type_init(xen_igd_passthrough_isa_bridge_register_types);
-- 
1.9.1

[Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Tiejun Chen]

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.chen@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
-- 
1.9.1

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Michael S. Tsirkin]

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.chen@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?
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.
Additionally the correct bridge would be initialized automatically.


> -- 
> 1.9.1

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

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.chen@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?

#2 Then if that condition is matched, we register this ISA bridge on its 
associated bus.

Thanks
Tiejun

> Additionally the correct bridge would be initialized automatically.
>
>
>> --
>> 1.9.1
>

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Michael S. Tsirkin]

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.chen@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.


> #2 Then if that condition is matched, we register this ISA bridge on its
> associated bus.
> 
> Thanks
> Tiejun

Yep.

> >Additionally the correct bridge would be initialized automatically.
> >
> >
> >>--
> >>1.9.1
> >

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

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.chen@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, pch_id = 0xffff;
+    PCIDevice *d = &s->dev;
+
+    if (is_vga_passthrough(dev)) {
+        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;
+
+        /* 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.
+         */
+        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");
+    }
+}
+
  /* 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

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Michael S. Tsirkin]

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.chen@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

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> 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.chen@intel.com>
>>>>>> ---

[snip]

> Cleaner:
> 	 if (!pci_dev) {
> 		fprintf
> 		return;
> 	}
>           pci_config_set_device_id(pci_dev->config, pch_id);

I will address all comments and thanks.

>
>> +    }
>> +}
>> +
>>   /* 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?
>

If this is just used for IGD, its always fine without checking vendor_id.

So after remove that check, I guess I need to rename that as 
is_igd_vga_passthrough() to make sense.

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> >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.chen@intel.com>
> >>>>>>---
> 
> [snip]
> 
> >Cleaner:
> >	 if (!pci_dev) {
> >		fprintf
> >		return;
> >	}
> >          pci_config_set_device_id(pci_dev->config, pch_id);
> 
> I will address all comments and thanks.
> 
> >
> >>+    }
> >>+}
> >>+
> >>  /* 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?
> >
> 
> If this is just used for IGD, its always fine without checking vendor_id.

You need to match device ID to *know* it's IGD.

> So after remove that check, I guess I need to rename that as
> is_igd_vga_passthrough() to make sense.
> 
> Thanks
> Tiejun

There is no need to check class code or xen_has_gfx_passthru flag.
Device ID + vendor ID identifies each device uniquely.

-- 
MST

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/17 18:13, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
>> On 2014/11/17 17:25, Michael S. Tsirkin wrote:
>>> 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.chen@intel.com>
>>>>>>>> ---
>>
>> [snip]
>>
>>> Cleaner:
>>> 	 if (!pci_dev) {
>>> 		fprintf
>>> 		return;
>>> 	}
>>>           pci_config_set_device_id(pci_dev->config, pch_id);
>>
>> I will address all comments and thanks.
>>
>>>
>>>> +    }
>>>> +}
>>>> +
>>>>   /* 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?
>>>
>>
>> If this is just used for IGD, its always fine without checking vendor_id.
>
> You need to match device ID to *know* it's IGD.
>
>> So after remove that check, I guess I need to rename that as
>> is_igd_vga_passthrough() to make sense.
>>
>> Thanks
>> Tiejun
>
> There is no need to check class code or xen_has_gfx_passthru flag.
> Device ID + vendor ID identifies each device uniquely.
>

Yeah.

Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means 
I also need to check that table to do something like,

is_igd_vga_passthugh(dev)
{	
	int i;
	int num = ARRAY_SIZE(xen_igd_combo_id_infos);
	for (i = 0; i < num; i++) {
		if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
			return 1;
	return 0;
}

Then we can simplify the subsequent codes based on this, right?

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 07:18:17PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 18:13, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
> >>On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> >>>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.chen@intel.com>
> >>>>>>>>---
> >>
> >>[snip]
> >>
> >>>Cleaner:
> >>>	 if (!pci_dev) {
> >>>		fprintf
> >>>		return;
> >>>	}
> >>>          pci_config_set_device_id(pci_dev->config, pch_id);
> >>
> >>I will address all comments and thanks.
> >>
> >>>
> >>>>+    }
> >>>>+}
> >>>>+
> >>>>  /* 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?
> >>>
> >>
> >>If this is just used for IGD, its always fine without checking vendor_id.
> >
> >You need to match device ID to *know* it's IGD.
> >
> >>So after remove that check, I guess I need to rename that as
> >>is_igd_vga_passthrough() to make sense.
> >>
> >>Thanks
> >>Tiejun
> >
> >There is no need to check class code or xen_has_gfx_passthru flag.
> >Device ID + vendor ID identifies each device uniquely.
> >
> 
> Yeah.
> 
> Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means I
> also need to check that table to do something like,
> 
> is_igd_vga_passthugh(dev)
> {	
> 	int i;
> 	int num = ARRAY_SIZE(xen_igd_combo_id_infos);
> 	for (i = 0; i < num; i++) {
> 		if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
> 			return 1;
> 	return 0;
> }
> 
> Then we can simplify the subsequent codes based on this, right?
> 
> Thanks
> Tiejun

Yea.
Basically let's try to treat this simply as a device quirk,
and see where this gets us.

-- 
MST