hw:xen:xen_pt: register isa bridge specific to IGD passthrough

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 <[email protected]>

---
 hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

hw/i386/pc_piix.c +26 -1

index b559181..b19c7a9 100644
@@ -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;

Signature · 1 line

1.9.1

Quoted 55 lines · 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 <[email protected]>

---
  hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
  1 file changed, 27 insertions(+), 1 deletion(-)

hw/i386/pc_piix.c +26 -1

index b559181..b19c7a9 100644
@@ -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();

hw/i386/pc_piix.c static void pc_xen_hvm_init(MachineState *machine) · line 492

Quoted 6 lines

hw/i386/pc_piix.c +1 -0

      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.

Quoted 2 lines

--
1.9.1

Quoted 56 lines · Michael S. Tsirkin

Quoted 55 lines · 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 <[email protected]>

---
 hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

hw/i386/pc_piix.c +26 -1

index b559181..b19c7a9 100644
@@ -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();

Quoted 8 lines

hw/i386/pc_piix.c static void pc_xen_hvm_init(MachineState *machine) · line 492

Quoted 6 lines

hw/i386/pc_piix.c +1 -0

     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.

Quoted 3 lines

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

Quoted 6 lines

Additionally the correct bridge would be initialized automatically.

Quoted 2 lines

--
1.9.1

Quoted 72 lines · Tiejun

Quoted 56 lines · Michael S. Tsirkin

Quoted 55 lines · 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 <[email protected]>

---
  hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
  1 file changed, 27 insertions(+), 1 deletion(-)

hw/i386/pc_piix.c +26 -1

index b559181..b19c7a9 100644
@@ -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();

Quoted 8 lines

hw/i386/pc_piix.c static void pc_xen_hvm_init(MachineState *machine) · line 492

Quoted 6 lines

hw/i386/pc_piix.c +1 -0

      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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 73 lines · Michael S. Tsirkin

Quoted 72 lines · Tiejun

Quoted 56 lines · Michael S. Tsirkin

Quoted 55 lines · 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 <[email protected]>

---
 hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

hw/i386/pc_piix.c +26 -1

index b559181..b19c7a9 100644
@@ -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();

Quoted 8 lines

hw/i386/pc_piix.c static void pc_xen_hvm_init(MachineState *machine) · line 492

Quoted 6 lines

hw/i386/pc_piix.c +1 -0

     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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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:

hw/xen/xen_pt.c +52 -0

index c6466dc..f3ea313 100644
@@ -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 */

hw/xen/xen_pt.c +3 -0

+    }
+}
+
 /* init */

static int xen_pt_initfn(PCIDevice *d)

hw/xen/xen_pt.c +3 -0

@@ -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,

hw/xen/xen_pt.c +5 -0

+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

Quoted 9 lines · Tiejun

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Tiejun

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · 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 <[email protected]>

---

Cleaner:

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

Quoted 26 lines

hw/xen/xen_pt.c +6 -0

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

(quoted diff) +5 -0

+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));
+}

Quoted 2 lines

Thanks
Tiejun

Why bother with all these conditions?
Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Tiejun

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Tiejun

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · 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 <[email protected]>

---

[snip]

Quoted 6 lines

Cleaner:

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

I will address all comments and thanks.

Quoted 26 lines

Quoted 26 lines

hw/xen/xen_pt.c +6 -0

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

(quoted diff) +5 -0

+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));
+}

Quoted 6 lines

Quoted 2 lines

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

Quoted 49 lines · Tiejun

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Tiejun

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Tiejun

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · 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 <[email protected]>

---

[snip]

Quoted 6 lines

Cleaner:

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

I will address all comments and thanks.

Quoted 26 lines

Quoted 26 lines

hw/xen/xen_pt.c +6 -0

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

(quoted diff) +5 -0

+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));
+}

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 61 lines · Michael S. Tsirkin

Quoted 49 lines · Tiejun

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Tiejun

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Tiejun

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · 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 <[email protected]>

---

[snip]

Quoted 6 lines

Cleaner:

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

I will address all comments and thanks.

Quoted 26 lines

Quoted 26 lines

hw/xen/xen_pt.c +6 -0

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

(quoted diff) +5 -0

+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));
+}

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 5 lines

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