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

["Chen, Tiejun" <tiejun.chen@intel.com>]

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