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

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

>> Here I just mean when Qemu realizes IGD is passed through but without
>> that appropriate option set, Qemu can post something to explicitly
>> notify user that this option is needed in his case. But it may be a lazy
>> idea.
>
> In any case I think the additions of such warnings in qemu are a
> separate to the discussion in this thread, so I propose to leave it
> alone for now.

Okay.

>
>> So now I think I'd better go back handling this on Xen side with your
>> comments. As you said the Boolean doesn't suffice to indicate that IGD
>> workarounds are needed. So I think we can reuse that existing bool
>> 'gfx_passthru'.
>>
>> Firstly we can redefine this as string,
>
> Unfortunately not since libxl's API guarantee requires older clients to
> keep working, i.e. those who use libxl_defbool_set on this field.
>
> Probably the best which can be done is to deprecate this field in favour
> of a new one (the old field would need to be obeyed only if the new one
> was set to its default value).
>
> Probably an Enumeration would be better than a raw string here as well.
>
> This approach doesn't allow for the possibility of multiple such
> workarounds though. It's unclear to me if this matters or not.
>
> The other option which I've mentioned is to leave gfx_passthru and have
> libxl figure out which workarounds to enable based on the set of PCI
> devices passed through. I guess you don't like that approach? (due to
> the need to maintain the pci vid:did list?)

No, I like that approach currently :) Please see the below,

>
>>
>> -                                       ("gfx_passthru",     libxl_defbool),
>> +                                       ("gfx_passthru",     string),
>>
>> Then
>>
>> +
>> +        if (libxl__is_igd_vga_passthru(gc, guest_config) ||

This is just working out that way that I already posted previously, and 
here I paste this code fragment again,

+static const pci_info fixup_ids[] = {
+    /* Intel HSW Classic */
+    {0x8086, 0x0402}, /* HSWGT1D, HSWD_w7 */
+    {0x8086, 0x0406}, /* HSWGT1M, HSWM_w7 */
+    {0x8086, 0x0412}, /* HSWGT2D, HSWD_w7 */
+    {0x8086, 0x0416}, /* HSWGT2M, HSWM_w7 */
+    {0x8086, 0x041E}, /* HSWGT15D, HSWD_w7 */
+    /* Intel HSW ULT */
+    {0x8086, 0x0A06}, /* HSWGT1UT, HSWM_w7 */
+    {0x8086, 0x0A16}, /* HSWGT2UT, HSWM_w7 */
+    {0x8086, 0x0A26}, /* HSWGT3UT, HSWM_w7 */
+    {0x8086, 0x0A2E}, /* HSWGT3UT28W, HSWM_w7 */
+    {0x8086, 0x0A1E}, /* HSWGT2UX, HSWM_w7 */
+    {0x8086, 0x0A0E}, /* HSWGT1ULX, HSWM_w7 */
+    /* Intel HSW CRW */
+    {0x8086, 0x0D26}, /* HSWGT3CW, HSWM_w7 */
+    {0x8086, 0x0D22}, /* HSWGT3CWDT, HSWD_w7 */
+    /* Intel HSW Server */
+    {0x8086, 0x041A}, /* HSWSVGT2, HSWD_w7 */
+    /* Intel HSW SRVR */
+    {0x8086, 0x040A}, /* HSWSVGT1, HSWD_w7 */
+    /* Intel BSW */
+    {0x8086, 0x1606}, /* BDWULTGT1, BDWM_w7 */
+    {0x8086, 0x1616}, /* BDWULTGT2, BDWM_w7 */
+    {0x8086, 0x1626}, /* BDWULTGT3, BDWM_w7 */
+    {0x8086, 0x160E}, /* BDWULXGT1, BDWM_w7 */
+    {0x8086, 0x161E}, /* BDWULXGT2, BDWM_w7 */
+    {0x8086, 0x1602}, /* BDWHALOGT1, BDWM_w7 */
+    {0x8086, 0x1612}, /* BDWHALOGT2, BDWM_w7 */
+    {0x8086, 0x1622}, /* BDWHALOGT3, BDWM_w7 */
+    {0x8086, 0x162B}, /* BDWHALO28W, BDWM_w7 */
+    {0x8086, 0x162A}, /* BDWGT3WRKS, BDWM_w7 */
+    {0x8086, 0x162D}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+/*
+ * Some devices may need some ways to work well. Here like IGD,
+ * we have to pass a specific option to qemu.
+ */
+int libxl__is_igd_vga_passthru(libxl__gc *gc,
+                               const libxl_domain_config *d_config)
+{
+    unsigned int i, j, num = ARRAY_SIZE(fixup_ids);
+    uint16_t vendor, device;
+
+    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+        libxl_device_pci *pcidev = &d_config->pcidevs[i];
+
+        for (j = 0 ; j < num ; j++) {
+            vendor = fixup_ids[j].vendor;
+            device = fixup_ids[j].device;
+
+            if (sysfs_dev_get_vendor(gc, pcidev) == vendor &&
+                sysfs_dev_get_device(gc, pcidev) == device)
+                return 1;
+        }
+    }
+
+    return 0;
+}
+

Is this expected?

>> +            (b_info->u.hvm.gfx_passthru &&
>> +             strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) {

But as you mentioned previously,

"
You might like to optionally consider add a forcing option somehow so
that people with new devices not in the list can control things without
the need to recompile (e.g. gfx_passthru_kind_override?).
"

Here I was trying to convert "gfx_passthru" to address this thing. 
According to your comment right now, you prefer we should introduce a 
new field instead of the original 'gfx_passthru' to enumerate such a 
type. So what about this?

libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [
     (0, "unknown"),
     (1, "igd"),
     ])

Then if we want to override this, just submit the following line into .cfg:

gfx_passthru_kind_override = "igd"

Thanks
Tiejun

>> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>> +        }
>> +
>>
>> Of course we need modify something else to align this change.
>>
>> Thanks
>> Tiejun
>
>
>