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

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

Ian,

Just ping this, or do you think I should send this as a patch?

Thanks
Tiejun

On 2015/2/11 10:45, Chen, Tiejun wrote:
> On 2015/2/9 19:05, Ian Campbell wrote:
>> On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote:
>>
>>> What about this?
>>
>> I've not read the code in detail,since I'm travelling but from a quick
>> glance it looks to be implementing the sort of thing I meant, thanks.
>
> Thanks for your time.
>
>>
>> A couple of higher level comments:
>>
>> I'd suggest to put the code for reading the vid/did into a helper
>> function so it can be reused.
>
> Looks good.
>
>>
>> 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?). Perhaps that
>> isn't needed for a first cut though and it would be a libxl API so
>> thought required.
>
> What about 'gfx_passthru_force'? Because what we're doing is, we want to
> make sure if we have a such a IGD that needs to workaround by posting a
> parameter to qemu. So in case of non-listed devices we just need to
> provide a bool to force this regardless of that real device.
>
>>
>> I think it should probably log something at a lowish level when it has
>> autodetected IGD.
>>
>
> So I tried to rebase that according to your all comments,
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 98687bd..398d9da 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -361,6 +361,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>           libxl_defbool_setdefault(&b_info->u.hvm.nographic, false);
>
>           libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false);
> +        libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru_force,
> false);
>
>           break;
>       case LIBXL_DOMAIN_TYPE_PV:
> 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..07b9e22 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc,
> libxl_device_pci *pcidev,
>       return 0;
>   }
>
> +static unsigned long sysfs_dev_get_vendor(libxl__gc *gc,
> +                                          libxl_device_pci *pcidev)
> +{
> +    char *pci_device_vendor_path =
> +            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
> +                           pcidev->domain, pcidev->bus, pcidev->dev,
> +                           pcidev->func);
> +    int read_items;
> +    unsigned long pci_device_vendor;
> +
> +    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);
> +        return 0xffff;
> +    }
> +    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);
> +        return 0xffff;
> +    }
> +
> +    return pci_device_vendor;
> +}
> +
> +static unsigned long sysfs_dev_get_device(libxl__gc *gc,
> +                                          libxl_device_pci *pcidev)
> +{
> +    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_device;
> +
> +    FILE *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);
> +        return 0xffff;
> +    }
> +    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);
> +        return 0xffff;
> +    }
> +
> +    return pci_device_device;
> +}
> +
> +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)
> +{
> +    unsigned int i, j, num = ARRAY_SIZE(igd_ids);
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> +    if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru))
> +        return 0;
> +
> +    if (libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru_force)) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "Force to workaround IGD.");
> +        return 1;
> +    }
> +
> +    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
> +        libxl_device_pci *pcidev = &d_config->pcidevs[i];
> +
> +        if (sysfs_dev_get_vendor(gc, pcidev) != 0x8086) /* Intel class */
> +            continue;
> +
> +        for (j = 0 ; j < num ; j++) {
> +            if (sysfs_dev_get_device(gc, pcidev) == igd_ids[j]) { /*
> IGD */
> +                LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Detected supported
> IGD.");
> +                return 1;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   /*
>    * A brief comment about slots.  I don't know what slots are for;
> however,
>    * I have by experimentation determined:
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 02be466..d3015bc 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -430,6 +430,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                          ("spice", libxl_spice_info),
>
>                                          ("gfx_passthru", libxl_defbool),
> +                                       ("gfx_passthru_force",
> libxl_defbool),
>
>                                          ("serial",           string),
>                                          ("boot",             string),
>
> Thanks
> Tiejun
>
>
>