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

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

Campbell,

Are you free to look at my reply?

Thanks
Tiejun

On 2015/3/2 9:20, Chen, Tiejun wrote:
>>> 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
>>
>>
>>
>
>
>