Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

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

On 2015/2/6 9:01, Chen, Tiejun wrote:
> On 2015/2/5 17:52, Ian Campbell wrote:
>> On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote:
>>> Indeed this is not something workaround, and I think in any type of VGA
>>> devices, we'd like to diminish this sort of thing gradually, right?
>>>
>>> This mightn't come true in real world :)
>>
>> It's not really something we can control, the h/w guys will do what they
>> do, including integrating on-board graphics tightly with the N/S-bridges
>> etc.
>
> Yeah.
>
>>
>>>>
>>>> I think there are three ways to achieve that:
>>>>
>>>>         * Make the libxl/xl option something which is not generic e.g.
>>>>           igd_passthru=1
>>>>         * Add a second option to allow the user to configure the
>>>> kind of
>>>>           graphics device being passed thru (e.g. gfx_passthru=1,
>>>>           passthru_device="igd"), or combine the two by making the
>>>>           gfx_passthru option a string instead of a boolean.
>>>
>>> It makes more sense but this mean we're going to change that existing
>>> rule in qemu-traditional. But here I guess we shouldn't consider that
>>> case.
>>
>> qemu-trad is frozen so we wouldn't be adding new features such as
>> support for new graphics passthru devices, so we can ignore it's
>> deficiencies in this area and just improve things in the qemu-xen case.
>> (we may want to add a compat handling for any new option we add to libxl
>> to translate to -gfx_passthru, but that's about it)
>
> Understood.
>
>>
>>>>         * Make libxl detect the type of graphics device somehow and
>>>>           therefore automatically determine when gfx_passthru=1 =>
>>>>           igd-passthru
>>>
>>> This way confounds me all. Can libxl detect the graphics device *before*
>>> we intend to pass a parameter to active qemu?
>>
>> We know the BDF of all devices which we are going to pass to the guest
>> and we can observe various properties of that device
>> via /sys/bus/pci/devices/0000:B:D:F.
>
> So sounds what you're saying is Xen already have this sort of example in
> libxl.
>
>>
>>>
>>>>
>>>>>    Currently, we have to set
>>>>> something as follows,
>>>>>
>>>>> gfx_passthru=1
>>>>> pci=["00:02.0"]
>>>>>
>>>>> This always works for qemu-xen-traditional.
>>>>>
>>>>> But you should know '00:02.0' doesn't mean we are passing IGD through.
>>>>
>>>> But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID
>>>> and other properties) we can unambiguously determine if it is an IGD
>>>> device or not, can't we?
>>>
>>> Again, like what I said above, I'm not sure if its possible in my case.
>>> If I'm wrong please correct me.
>>
>> Is my reply above sufficient?
>
> Yes, I can understand what you mean but I need to take close look at
> exactly what should be done in libxl :)
>
> In high level, this way may come out as follows,
>
> #1 libxl parse 'pci=[]' to get SBDF
> #2 Scan SBDF by accessing sysfs to get vendor/device IDs.
> #3 If this pair of IDs is identified to our target device, IGD,
> "igd-passthru" would be delivered to qemu.
>
> Right?

What about this?

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..507034f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -710,9 +710,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
              flexarray_append(dm_args, "-net");
              flexarray_append(dm_args, "none");
          }
-        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-            flexarray_append(dm_args, "-gfx_passthru");
-        }
      } else {
          if (!sdl && !vnc) {
              flexarray_append(dm_args, "-nographic");
@@ -757,6 +754,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+            machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+        }
+
          flexarray_append(dm_args, machinearg);
          for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)
              flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..35ec5fc 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc 
*gc, uint32_t domid,
                                        libxl_device_pci *pcidev, int num);
  _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);

+_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                       const libxl_domain_config 
*d_config);
+
  /*----- xswait: wait for a xenstore node to be suitable -----*/

  typedef struct libxl__xswait_state libxl__xswait_state;
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..9cccbfe 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -491,6 +491,108 @@ static int sysfs_dev_unbind(libxl__gc *gc, 
libxl_device_pci *pcidev,
      return 0;
  }

+/* Currently we only support HSW and BDW in qemu upstream.*/
+static const uint16_t igd_ids[] = {
+    /* HSW Classic */
+    0x0402, /* HSWGT1D, HSWD_w7 */
+    0x0406,  /* HSWGT1M, HSWM_w7 */
+    0x0412,  /* HSWGT2D, HSWD_w7 */
+    0x0416,  /* HSWGT2M, HSWM_w7 */
+    0x041E,  /* HSWGT15D, HSWD_w7 */
+    /* HSW ULT */
+    0x0A06,  /* HSWGT1UT, HSWM_w7 */
+    0x0A16,  /* HSWGT2UT, HSWM_w7 */
+    0x0A26,  /* HSWGT3UT, HSWM_w7 */
+    0x0A2E,  /* HSWGT3UT28W, HSWM_w7 */
+    0x0A1E,  /* HSWGT2UX, HSWM_w7 */
+    0x0A0E,  /* HSWGT1ULX, HSWM_w7 */
+    /* HSW CRW */
+    0x0D26,  /* HSWGT3CW, HSWM_w7 */
+    0x0D22,  /* HSWGT3CWDT, HSWD_w7 */
+    /* HSW Sever */
+    0x041A,  /* HSWSVGT2, HSWD_w7 */
+    /* HSW SRVR */
+    0x040A,  /* HSWSVGT1, HSWD_w7 */
+    /* BSW */
+    0x1606,  /* BDWULTGT1, BDWM_w7 */
+    0x1616,  /* BDWULTGT2, BDWM_w7 */
+    0x1626,  /* BDWULTGT3, BDWM_w7 */
+    0x160E,  /* BDWULXGT1, BDWM_w7 */
+    0x161E,  /* BDWULXGT2, BDWM_w7 */
+    0x1602,  /* BDWHALOGT1, BDWM_w7 */
+    0x1612,  /* BDWHALOGT2, BDWM_w7 */
+    0x1622,  /* BDWHALOGT3, BDWM_w7 */
+    0x162B,  /* BDWHALO28W, BDWM_w7 */
+    0x162A,  /* BDWGT3WRKS, BDWM_w7 */
+    0x162D,  /* BDWGT3SRVR, BDWM_w7 */
+};
+
+int libxl__is_igd_vga_passthru(libxl__gc *gc,
+                               const libxl_domain_config *d_config)
+{
+    int i, j, ret = 0;
+
+    if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru))
+        return ret;
+
+    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+        libxl_device_pci *pcidev = &d_config->pcidevs[i];
+        char *pci_device_vendor_path =
+            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+                           pcidev->domain, pcidev->bus, pcidev->dev,
+                           pcidev->func);
+        char *pci_device_device_path =
+            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/device",
+                           pcidev->domain, pcidev->bus, pcidev->dev,
+                           pcidev->func);
+        int read_items;
+        unsigned long pci_device_vendor, pci_device_device;
+        unsigned int num = ARRAY_SIZE(igd_ids);
+
+        FILE *f = fopen(pci_device_vendor_path, "r");
+        if (!f) {
+            LOGE(ERROR,
+                 "pci device "PCI_BDF" does not have vendor attribute",
+                 pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+            continue;
+        }
+        read_items = fscanf(f, "0x%lx\n", &pci_device_vendor);
+        fclose(f);
+        if (read_items != 1) {
+            LOGE(ERROR,
+                 "cannot read vendor of pci device "PCI_BDF,
+                 pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+            continue;
+        }
+        if (pci_device_vendor != 0x8086) /* Intel class */
+            continue;
+
+        f = fopen(pci_device_device_path, "r");
+        if (!f) {
+            LOGE(ERROR,
+                 "pci device "PCI_BDF" does not have device attribute",
+                 pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+            continue;
+        }
+        read_items = fscanf(f, "0x%lx\n", &pci_device_device);
+        fclose(f);
+        if (read_items != 1) {
+            LOGE(ERROR,
+                 "cannot read device of pci device "PCI_BDF,
+                 pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+            continue;
+        }
+        for (j = 0 ; j < num ; j++) {
+            if (pci_device_device == igd_ids[j]) { /* IGD class */
+                ret = 1;
+                break;
+            }
+        }
+    }
+
+    return ret;
+}
+
  /*
   * A brief comment about slots.  I don't know what slots are for; however,
   * I have by experimentation determined:


Thanks
Tiejun