[Qemu-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Qemu-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Tiejun Chen]

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,igd-passthru=on". This need
to bring a change on tool side.

After a discussion with Campbell, we'd like to construct a table to record
all IGD devices we can support. If we hit that table, we should pass that
option. And so we also introduce a new field of type, 'gfx_passthru_kind',
to cooperate with 'gfx_passthru' to cover all scenarios like this,

    gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
    gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
                           build_info.u.gfx_passthru_kind to DEFAULT
    gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
                               and build_info.u.gfx_passthru_kind to IGD

----------------------------------------------------------------
Tiejun Chen (2):
      libxl: introduce libxl__is_igd_vga_passthru
      libxl: introduce gfx_passthru_kind
 
 tools/libxl/libxl_dm.c       |  13 +++++++++++++
 tools/libxl/libxl_internal.h |   2 ++
 tools/libxl/libxl_pci.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl  |   6 ++++++
 tools/libxl/xl_cmdimpl.c     |  19 +++++++++++++++++--
 5 files changed, 162 insertions(+), 2 deletions(-)

Thanks
Tiejun

[Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Tiejun Chen]

While working with qemu, IGD is a specific device in the case of pass through
so we need to identify that to handle more later. Here we define a table to
record all IGD types currently we can support. Also we need to introduce two
helper functions to get vendor and device ids to lookup that table.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxl/libxl_internal.h |   2 +
 tools/libxl/libxl_pci.c      | 124 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..8b952b8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1176,6 +1176,8 @@ _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pc
 _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 -----*/
 
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..dc5a89e 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;
+}
+
+typedef struct {
+    uint16_t vendor;
+    uint16_t device;
+} pci_info;
+
+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;
+}
+
 /*
  * A brief comment about slots.  I don't know what slots are for; however,
  * I have by experimentation determined:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Wei Liu]

On Fri, Mar 06, 2015 at 05:08:22PM +0800, Tiejun Chen wrote:
> While working with qemu, IGD is a specific device in the case of pass through
> so we need to identify that to handle more later. Here we define a table to
> record all IGD types currently we can support. Also we need to introduce two
> helper functions to get vendor and device ids to lookup that table.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/libxl/libxl_internal.h |   2 +
>  tools/libxl/libxl_pci.c      | 124 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 934465a..8b952b8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1176,6 +1176,8 @@ _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pc
>  _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 -----*/
>  
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index f3ae132..dc5a89e 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)

uint16_t?

> +{
> +    char *pci_device_vendor_path =
> +            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
> +                           pcidev->domain, pcidev->bus, pcidev->dev,
> +                           pcidev->func);

Please use GCSPRINTF macro.

> +    int read_items;
> +    unsigned long pci_device_vendor;

uint16_t?

Same comments apply to _get_device function.

> +
> +    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;
> +}
> +

[...]

> +/*
> + * 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,

bool.

> +                               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;

Get vendor and device in outer loop to avoid wasting cpu cycles. :-)

Wei.

> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * A brief comment about slots.  I don't know what slots are for; however,
>   * I have by experimentation determined:
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Chen, Tiejun]

On 2015/3/6 20:40, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:08:22PM +0800, Tiejun Chen wrote:
>> While working with qemu, IGD is a specific device in the case of pass through
>> so we need to identify that to handle more later. Here we define a table to
>> record all IGD types currently we can support. Also we need to introduce two
>> helper functions to get vendor and device ids to lookup that table.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/libxl/libxl_internal.h |   2 +
>>   tools/libxl/libxl_pci.c      | 124 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 126 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 934465a..8b952b8 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1176,6 +1176,8 @@ _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pc
>>   _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 -----*/
>>
>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
>> index f3ae132..dc5a89e 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)
>
> uint16_t?
>
>> +{
>> +    char *pci_device_vendor_path =
>> +            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
>> +                           pcidev->domain, pcidev->bus, pcidev->dev,
>> +                           pcidev->func);
>
> Please use GCSPRINTF macro.

Okay.

>
>> +    int read_items;
>> +    unsigned long pci_device_vendor;
>
> uint16_t?

Yes, I can but I don't see other similar helpers are doing this in this 
file :)

>
> Same comments apply to _get_device function.

And especially, if we really set that as uint16_t,

>
>> +
>> +    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);

we have to refactor this as well,

read_items = fscanf(f, "0x%hx\n", &pci_device_vendor);

Right?

>> +    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;
>> +}
>> +
>
> [...]
>
>> +/*
>> + * 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,
>
> bool.

Okay.

>
>> +                               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;
>
> Get vendor and device in outer loop to avoid wasting cpu cycles. :-)
>

Yeah.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Wei Liu]

On Mon, Mar 09, 2015 at 02:27:46PM +0800, Chen, Tiejun wrote:
[...]
> >>+
> >>+    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);
> 
> we have to refactor this as well,
> 
> read_items = fscanf(f, "0x%hx\n", &pci_device_vendor);
> 
> Right?
> 

Sure. Just eliminate any warnings / errors.

Wei.

[Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Tiejun Chen]

Although we already have 'gfx_passthru' in b_info, this doesn' suffice
after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

    gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
    gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
                           build_info.u.gfx_passthru_kind to DEFAULT
    gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
                               and build_info.u.gfx_passthru_kind to IGD

Here if gfx_passthru_kind = DEFAULT, we will call
libxl__is_igd_vga_passthru() to check if we're hitting that table to need
to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
force to pass that.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxl/libxl_dm.c      | 13 +++++++++++++
 tools/libxl/libxl_types.idl |  6 ++++++
 tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..780dd2a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                             machinearg, max_ram_below_4g);
             }
         }
+
+        if (b_info->u.hvm.gfx_passthru_kind ==
+                LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
+            if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+            }
+        } else if (b_info->u.hvm.gfx_passthru_kind ==
+                        LIBXL_GFX_PASSTHRU_KIND_IGD) {
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+        } else {
+            LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+        }
+
         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_types.idl b/tools/libxl/libxl_types.idl
index 02be466..e209460 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
     (3, "native_paravirt"),
     ])
 
+libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
+    (0, "default"),
+    (1, "igd"),
+    ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT")
+
 # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
 libxl_timer_mode = Enumeration("timer_mode", [
     (-1, "unknown"),
@@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("spice",            libxl_spice_info),
                                        
                                        ("gfx_passthru",     libxl_defbool),
+                                       ("gfx_passthru_kind",     libxl_gfx_passthru_kind),
                                        
                                        ("serial",           string),
                                        ("boot",             string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 440db78..3f7fede 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,23 @@ skip_vfb:
         xlu_cfg_replace_string (config, "spice_streaming_video",
                                 &b_info->u.hvm.spice.streaming_video, 0);
         xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0);
-        xlu_cfg_get_defbool(config, "gfx_passthru",
-                            &b_info->u.hvm.gfx_passthru, 0);
+        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+            if (!l) {
+                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+            } else if (i == 1) {
+                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+            } else {
+                fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l);
+                exit (1);
+            }
+        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+            if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) {
+                fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n",
+                        buf);
+                exit (1);
+            }
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+        }
         switch (xlu_cfg_get_list_as_string_list(config, "serial",
                                                 &b_info->u.hvm.serial_list,
                                                 1))
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/6 17:08, Tiejun Chen wrote:
> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
> after we want to handle IGD specifically. Now we define a new field of
> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> this means we can benefit this to support other specific devices just
> by extending gfx_passthru_kind. And then we can cooperate with
> gfx_passthru to address IGD cases as follows:
>
>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>                             build_info.u.gfx_passthru_kind to DEFAULT
>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                 and build_info.u.gfx_passthru_kind to IGD
>
> Here if gfx_passthru_kind = DEFAULT, we will call
> libxl__is_igd_vga_passthru() to check if we're hitting that table to need
> to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
> force to pass that.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>   tools/libxl/libxl_dm.c      | 13 +++++++++++++
>   tools/libxl/libxl_types.idl |  6 ++++++
>   tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
>   3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..780dd2a 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c

Sorry, looks I'm missing to remove '-gfx_passthru' since this should be 
gone in the case of qemu upstream,

@@ -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");

I will fold this into next revision after this review.

Thanks
Tiejun

> @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (b_info->u.hvm.gfx_passthru_kind ==
> +                LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
> +            if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +            }
> +        } else if (b_info->u.hvm.gfx_passthru_kind ==
> +                        LIBXL_GFX_PASSTHRU_KIND_IGD) {
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +        } else {
> +            LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> +        }
> +
>           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_types.idl b/tools/libxl/libxl_types.idl
> index 02be466..e209460 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
>       (3, "native_paravirt"),
>       ])
>
> +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
> +    (0, "default"),
> +    (1, "igd"),
> +    ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT")
> +
>   # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
>   libxl_timer_mode = Enumeration("timer_mode", [
>       (-1, "unknown"),
> @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                          ("spice",            libxl_spice_info),
>
>                                          ("gfx_passthru",     libxl_defbool),
> +                                       ("gfx_passthru_kind",     libxl_gfx_passthru_kind),
>
>                                          ("serial",           string),
>                                          ("boot",             string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 440db78..3f7fede 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,23 @@ skip_vfb:
>           xlu_cfg_replace_string (config, "spice_streaming_video",
>                                   &b_info->u.hvm.spice.streaming_video, 0);
>           xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0);
> -        xlu_cfg_get_defbool(config, "gfx_passthru",
> -                            &b_info->u.hvm.gfx_passthru, 0);
> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
> +            if (!l) {
> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +            } else if (i == 1) {
> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
> +            } else {
> +                fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l);
> +                exit (1);
> +            }
> +        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
> +            if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) {
> +                fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +        }
>           switch (xlu_cfg_get_list_as_string_list(config, "serial",
>                                                   &b_info->u.hvm.serial_list,
>                                                   1))
>

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Wei Liu]

On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote:
> On 2015/3/6 17:08, Tiejun Chen wrote:
> >Although we already have 'gfx_passthru' in b_info, this doesn' suffice
> >after we want to handle IGD specifically. Now we define a new field of
> >type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> >this means we can benefit this to support other specific devices just
> >by extending gfx_passthru_kind. And then we can cooperate with
> >gfx_passthru to address IGD cases as follows:
> >
> >     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
> >     gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
> >                            build_info.u.gfx_passthru_kind to DEFAULT
> >     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
> >                                and build_info.u.gfx_passthru_kind to IGD
> >
> >Here if gfx_passthru_kind = DEFAULT, we will call
> >libxl__is_igd_vga_passthru() to check if we're hitting that table to need
> >to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
> >force to pass that.
> >
> >Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >---
> >  tools/libxl/libxl_dm.c      | 13 +++++++++++++
> >  tools/libxl/libxl_types.idl |  6 ++++++
> >  tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
> >  3 files changed, 36 insertions(+), 2 deletions(-)
> >
> >diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >index 8599a6a..780dd2a 100644
> >--- a/tools/libxl/libxl_dm.c
> >+++ b/tools/libxl/libxl_dm.c
> 
> Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone
> in the case of qemu upstream,
> 
> @@ -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");
> 
> I will fold this into next revision after this review.
> 

I think this change alone warrants a separate changeset. It should be
patch 0 of your series as a preparatory patch. Please remember to
elaborate in commit message why that hunk is not needed in upstream
QEMU.

Wei.

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/6 20:59, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote:
>> On 2015/3/6 17:08, Tiejun Chen wrote:
>>> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
>>> after we want to handle IGD specifically. Now we define a new field of
>>> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
>>> this means we can benefit this to support other specific devices just
>>> by extending gfx_passthru_kind. And then we can cooperate with
>>> gfx_passthru to address IGD cases as follows:
>>>
>>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>>>                                 and build_info.u.gfx_passthru_kind to IGD
>>>
>>> Here if gfx_passthru_kind = DEFAULT, we will call
>>> libxl__is_igd_vga_passthru() to check if we're hitting that table to need
>>> to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
>>> force to pass that.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   tools/libxl/libxl_dm.c      | 13 +++++++++++++
>>>   tools/libxl/libxl_types.idl |  6 ++++++
>>>   tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
>>>   3 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>>> index 8599a6a..780dd2a 100644
>>> --- a/tools/libxl/libxl_dm.c
>>> +++ b/tools/libxl/libxl_dm.c
>>
>> Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone
>> in the case of qemu upstream,
>>
>> @@ -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");
>>
>> I will fold this into next revision after this review.
>>
>
> I think this change alone warrants a separate changeset. It should be
> patch 0 of your series as a preparatory patch. Please remember to
> elaborate in commit message why that hunk is not needed in upstream
> QEMU.
>

What about this?

"-gfx_passthru" is just introduced to work for qemu-xen-traditional so 
we should get this away from libxl__build_device_model_args_new() in the 
case of qemu upstream.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Wei Liu]

On Mon, Mar 09, 2015 at 02:50:42PM +0800, Chen, Tiejun wrote:
> On 2015/3/6 20:59, Wei Liu wrote:
> >On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote:
> >>On 2015/3/6 17:08, Tiejun Chen wrote:
> >>>Although we already have 'gfx_passthru' in b_info, this doesn' suffice
> >>>after we want to handle IGD specifically. Now we define a new field of
> >>>type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> >>>this means we can benefit this to support other specific devices just
> >>>by extending gfx_passthru_kind. And then we can cooperate with
> >>>gfx_passthru to address IGD cases as follows:
> >>>
> >>>     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
> >>>     gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
> >>>                            build_info.u.gfx_passthru_kind to DEFAULT
> >>>     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
> >>>                                and build_info.u.gfx_passthru_kind to IGD
> >>>
> >>>Here if gfx_passthru_kind = DEFAULT, we will call
> >>>libxl__is_igd_vga_passthru() to check if we're hitting that table to need
> >>>to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
> >>>force to pass that.
> >>>
> >>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>---
> >>>  tools/libxl/libxl_dm.c      | 13 +++++++++++++
> >>>  tools/libxl/libxl_types.idl |  6 ++++++
> >>>  tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
> >>>  3 files changed, 36 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >>>index 8599a6a..780dd2a 100644
> >>>--- a/tools/libxl/libxl_dm.c
> >>>+++ b/tools/libxl/libxl_dm.c
> >>
> >>Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone
> >>in the case of qemu upstream,
> >>
> >>@@ -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");
> >>
> >>I will fold this into next revision after this review.
> >>
> >
> >I think this change alone warrants a separate changeset. It should be
> >patch 0 of your series as a preparatory patch. Please remember to
> >elaborate in commit message why that hunk is not needed in upstream
> >QEMU.
> >
> 
> What about this?
> 
> "-gfx_passthru" is just introduced to work for qemu-xen-traditional so we
> should get this away from libxl__build_device_model_args_new() in the case
> of qemu upstream.
> 

This looks good.

Wei.

> Thanks
> Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Wei Liu]

On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote:
> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
> after we want to handle IGD specifically. Now we define a new field of
> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> this means we can benefit this to support other specific devices just
> by extending gfx_passthru_kind. And then we can cooperate with
> gfx_passthru to address IGD cases as follows:
> 
>     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>     gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
                                                               ^^^^^
                                                               true?

>                            build_info.u.gfx_passthru_kind to DEFAULT
>     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                and build_info.u.gfx_passthru_kind to IGD
> 
> Here if gfx_passthru_kind = DEFAULT, we will call
> libxl__is_igd_vga_passthru() to check if we're hitting that table to need
> to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
> force to pass that.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/libxl/libxl_dm.c      | 13 +++++++++++++
>  tools/libxl/libxl_types.idl |  6 ++++++
>  tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..780dd2a 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (b_info->u.hvm.gfx_passthru_kind ==
> +                LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
> +            if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +            }

You can remove that {} around machinearg -- it's only one line after
`if'.

> +        } else if (b_info->u.hvm.gfx_passthru_kind ==
> +                        LIBXL_GFX_PASSTHRU_KIND_IGD) {
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +        } else {
> +            LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> +        }
> +
>          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_types.idl b/tools/libxl/libxl_types.idl
> index 02be466..e209460 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
>      (3, "native_paravirt"),
>      ])
>  
> +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
> +    (0, "default"),
> +    (1, "igd"),
> +    ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT")
> +

You don't need to set init_val if the default value is 0.

>  # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
>  libxl_timer_mode = Enumeration("timer_mode", [
>      (-1, "unknown"),
> @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("spice",            libxl_spice_info),
>                                         
>                                         ("gfx_passthru",     libxl_defbool),
> +                                       ("gfx_passthru_kind",     libxl_gfx_passthru_kind),
                                                                ^^^^
One space is enough, I think.

>                                         
>                                         ("serial",           string),
>                                         ("boot",             string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 440db78..3f7fede 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,23 @@ skip_vfb:
>          xlu_cfg_replace_string (config, "spice_streaming_video",
>                                  &b_info->u.hvm.spice.streaming_video, 0);
>          xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0);
> -        xlu_cfg_get_defbool(config, "gfx_passthru",
> -                            &b_info->u.hvm.gfx_passthru, 0);
> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
> +            if (!l) {
> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +            } else if (i == 1) {
> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
> +            } else {
> +                fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l);

Please wrap this line to be < 80 column.

> +                exit (1);
> +            }
> +        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
> +            if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) {

Ditto.

> +                fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n",

Ditto.

Wei.

> +                        buf);
> +                exit (1);
> +            }
> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +        }
>          switch (xlu_cfg_get_list_as_string_list(config, "serial",
>                                                  &b_info->u.hvm.serial_list,
>                                                  1))
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/6 20:55, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote:
>> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
>> after we want to handle IGD specifically. Now we define a new field of
>> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
>> this means we can benefit this to support other specific devices just
>> by extending gfx_passthru_kind. And then we can cooperate with
>> gfx_passthru to address IGD cases as follows:
>>
>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>                                                                 ^^^^^
>                                                                 true?

I just simply pick up this from Campbell's comment but I agree you.

>
>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>>                                 and build_info.u.gfx_passthru_kind to IGD
>>
>> Here if gfx_passthru_kind = DEFAULT, we will call
>> libxl__is_igd_vga_passthru() to check if we're hitting that table to need
>> to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
>> force to pass that.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/libxl/libxl_dm.c      | 13 +++++++++++++
>>   tools/libxl/libxl_types.idl |  6 ++++++
>>   tools/libxl/xl_cmdimpl.c    | 19 +++++++++++++++++--
>>   3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 8599a6a..780dd2a 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -757,6 +757,19 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (b_info->u.hvm.gfx_passthru_kind ==
>> +                LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
>> +            if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>> +            }
>
> You can remove that {} around machinearg -- it's only one line after
> `if'.

Okay.

>
>> +        } else if (b_info->u.hvm.gfx_passthru_kind ==
>> +                        LIBXL_GFX_PASSTHRU_KIND_IGD) {
>> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>> +        } else {
>> +            LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
>> +        }
>> +
>>           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_types.idl b/tools/libxl/libxl_types.idl
>> index 02be466..e209460 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
>>       (3, "native_paravirt"),
>>       ])
>>
>> +libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
>> +    (0, "default"),
>> +    (1, "igd"),
>> +    ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT")
>> +
>
> You don't need to set init_val if the default value is 0.

Got it.

>
>>   # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
>>   libxl_timer_mode = Enumeration("timer_mode", [
>>       (-1, "unknown"),
>> @@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>                                          ("spice",            libxl_spice_info),
>>
>>                                          ("gfx_passthru",     libxl_defbool),
>> +                                       ("gfx_passthru_kind",     libxl_gfx_passthru_kind),
>                                                                  ^^^^
> One space is enough, I think.

Okay.

>
>>
>>                                          ("serial",           string),
>>                                          ("boot",             string),
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 440db78..3f7fede 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1953,8 +1953,23 @@ skip_vfb:
>>           xlu_cfg_replace_string (config, "spice_streaming_video",
>>                                   &b_info->u.hvm.spice.streaming_video, 0);
>>           xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0);
>> -        xlu_cfg_get_defbool(config, "gfx_passthru",
>> -                            &b_info->u.hvm.gfx_passthru, 0);
>> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
>> +            if (!l) {
>> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
>> +            } else if (i == 1) {
>> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
>> +            } else {
>> +                fprintf(stderr, "ERROR: invalid value %ld for \"gfx_passthru\"\n", l);
>
> Please wrap this line to be < 80 column.
>
>> +                exit (1);
>> +            }
>> +        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
>> +            if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) {
>
> Ditto.
>
>> +                fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n",
>
> Ditto.

So,

@@ -1959,13 +1959,15 @@ skip_vfb:
              } else if (i == 1) {
                  libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
              } else {
-                fprintf(stderr, "ERROR: invalid value %ld for 
\"gfx_passthru\"\n", l);
+                fprintf(stderr, "ERROR: invalid value %ld for"
+                                " \"gfx_passthru\"\n", l);
                  exit (1);
              }
          } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
-            if (libxl_gfx_passthru_kind_from_string(buf, 
&b_info->u.hvm.gfx_passthru_kind)) {
-                fprintf(stderr, "ERROR: invalid value \"%s\" for 
\"gfx_passthru\"\n",
-                        buf);
+            if (libxl_gfx_passthru_kind_from_string(buf,
+ 
&b_info->u.hvm.gfx_passthru_kind)) {
+                fprintf(stderr, "ERROR: invalid value \"%s\" for"
+                                " \"gfx_passthru\"\n", buf);
                  exit (1);
              }
              libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);

Thanks
Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Wei Liu]

On Mon, Mar 09, 2015 at 02:45:36PM +0800, Chen, Tiejun wrote:
[...]
> >
> >>+                exit (1);
> >>+            }
> >>+        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
> >>+            if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) {
> >
> >Ditto.
> >
> >>+                fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n",
> >
> >Ditto.
> 
> So,
> 
> @@ -1959,13 +1959,15 @@ skip_vfb:
>              } else if (i == 1) {
>                  libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
>              } else {
> -                fprintf(stderr, "ERROR: invalid value %ld for
> \"gfx_passthru\"\n", l);
> +                fprintf(stderr, "ERROR: invalid value %ld for"
> +                                " \"gfx_passthru\"\n", l);
>                  exit (1);
>              }
>          } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
> -            if (libxl_gfx_passthru_kind_from_string(buf,
> &b_info->u.hvm.gfx_passthru_kind)) {
> -                fprintf(stderr, "ERROR: invalid value \"%s\" for
> \"gfx_passthru\"\n",
> -                        buf);
> +            if (libxl_gfx_passthru_kind_from_string(buf,
> + &b_info->u.hvm.gfx_passthru_kind)) {
> +                fprintf(stderr, "ERROR: invalid value \"%s\" for"
> +                                " \"gfx_passthru\"\n", buf);
>                  exit (1);
>              }
>              libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> 

Your changes are mangled by your email client. But we don't normally
split the format string so that it's easier to grep.

What we normally do is 

     fprintf(stderr,
             "FORMAT STRING",
	     a, b, c);

If format string still treads over 80 column it's fine. We can live
with that.

Wei.

> Thanks
> Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/9 18:17, Wei Liu wrote:
> On Mon, Mar 09, 2015 at 02:45:36PM +0800, Chen, Tiejun wrote:
> [...]
>>>
>>>> +                exit (1);
>>>> +            }
>>>> +        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
>>>> +            if (libxl_gfx_passthru_kind_from_string(buf, &b_info->u.hvm.gfx_passthru_kind)) {
>>>
>>> Ditto.
>>>
>>>> +                fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n",
>>>
>>> Ditto.
>>
>> So,
>>
>> @@ -1959,13 +1959,15 @@ skip_vfb:
>>               } else if (i == 1) {
>>                   libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
>>               } else {
>> -                fprintf(stderr, "ERROR: invalid value %ld for
>> \"gfx_passthru\"\n", l);
>> +                fprintf(stderr, "ERROR: invalid value %ld for"
>> +                                " \"gfx_passthru\"\n", l);
>>                   exit (1);
>>               }
>>           } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
>> -            if (libxl_gfx_passthru_kind_from_string(buf,
>> &b_info->u.hvm.gfx_passthru_kind)) {
>> -                fprintf(stderr, "ERROR: invalid value \"%s\" for
>> \"gfx_passthru\"\n",
>> -                        buf);
>> +            if (libxl_gfx_passthru_kind_from_string(buf,
>> + &b_info->u.hvm.gfx_passthru_kind)) {
>> +                fprintf(stderr, "ERROR: invalid value \"%s\" for"
>> +                                " \"gfx_passthru\"\n", buf);
>>                   exit (1);
>>               }
>>               libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
>>
>
> Your changes are mangled by your email client. But we don't normally
> split the format string so that it's easier to grep.

Yeah.

>
> What we normally do is
>
>       fprintf(stderr,
>               "FORMAT STRING",
> 	     a, b, c);
>
> If format string still treads over 80 column it's fine. We can live
> with that.
>

Understood.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Mon, 2015-03-09 at 14:45 +0800, Chen, Tiejun wrote:
> On 2015/3/6 20:55, Wei Liu wrote:
> > On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote:
> >> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
                                                                   ^t

> >> after we want to handle IGD specifically. Now we define a new field of
> >> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> >> this means we can benefit this to support other specific devices just
> >> by extending gfx_passthru_kind. And then we can cooperate with
> >> gfx_passthru to address IGD cases as follows:
> >>
> >>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
> >>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
> >                                                                 ^^^^^
> >                                                                 true?
> 
> I just simply pick up this from Campbell's comment but I agree you.

Sorry! Wei is right.

Re: [Qemu-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Wei Liu]

On Fri, Mar 06, 2015 at 05:08:21PM +0800, Tiejun Chen wrote:
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that
> a machine option, "-machine xxx,igd-passthru=on". This need
> to bring a change on tool side.
> 
> After a discussion with Campbell, we'd like to construct a table to record
> all IGD devices we can support. If we hit that table, we should pass that
> option. And so we also introduce a new field of type, 'gfx_passthru_kind',
> to cooperate with 'gfx_passthru' to cover all scenarios like this,
> 
>     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>     gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>                            build_info.u.gfx_passthru_kind to DEFAULT
>     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                and build_info.u.gfx_passthru_kind to IGD
> 
> ----------------------------------------------------------------
> Tiejun Chen (2):
>       libxl: introduce libxl__is_igd_vga_passthru
>       libxl: introduce gfx_passthru_kind
>  
>  tools/libxl/libxl_dm.c       |  13 +++++++++++++
>  tools/libxl/libxl_internal.h |   2 ++
>  tools/libxl/libxl_pci.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

It might be helpful to use --stat-width=72 or some other options to
limit the width of your diffstat. :-)

Wei.

>  tools/libxl/libxl_types.idl  |   6 ++++++
>  tools/libxl/xl_cmdimpl.c     |  19 +++++++++++++++++--
>  5 files changed, 162 insertions(+), 2 deletions(-)
> 
> Thanks
> Tiejun

Re: [Qemu-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Chen, Tiejun]

On 2015/3/6 20:28, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:08:21PM +0800, Tiejun Chen wrote:
>> When we're working to support IGD GFX passthrough with qemu
>> upstream, instead of "-gfx_passthru" we'd like to make that
>> a machine option, "-machine xxx,igd-passthru=on". This need
>> to bring a change on tool side.
>>
>> After a discussion with Campbell, we'd like to construct a table to record
>> all IGD devices we can support. If we hit that table, we should pass that
>> option. And so we also introduce a new field of type, 'gfx_passthru_kind',
>> to cooperate with 'gfx_passthru' to cover all scenarios like this,
>>
>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>>                                 and build_info.u.gfx_passthru_kind to IGD
>>
>> ----------------------------------------------------------------
>> Tiejun Chen (2):
>>        libxl: introduce libxl__is_igd_vga_passthru
>>        libxl: introduce gfx_passthru_kind
>>
>>   tools/libxl/libxl_dm.c       |  13 +++++++++++++
>>   tools/libxl/libxl_internal.h |   2 ++
>>   tools/libxl/libxl_pci.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> It might be helpful to use --stat-width=72 or some other options to
> limit the width of your diffstat. :-)
>

Will do.

Thanks
Tiejun

Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Konrad Rzeszutek Wilk]

On Fri, Mar 06, 2015 at 05:08:21PM +0800, Tiejun Chen wrote:
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that

Could you also include in the cover letter an URL link to the latest
discussion on this?

Thank you.
> a machine option, "-machine xxx,igd-passthru=on". This need
> to bring a change on tool side.
> 
> After a discussion with Campbell, we'd like to construct a table to record
> all IGD devices we can support. If we hit that table, we should pass that
> option. And so we also introduce a new field of type, 'gfx_passthru_kind',
> to cooperate with 'gfx_passthru' to cover all scenarios like this,
> 
>     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>     gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>                            build_info.u.gfx_passthru_kind to DEFAULT
>     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                and build_info.u.gfx_passthru_kind to IGD
> 
> ----------------------------------------------------------------
> Tiejun Chen (2):
>       libxl: introduce libxl__is_igd_vga_passthru
>       libxl: introduce gfx_passthru_kind
>  
>  tools/libxl/libxl_dm.c       |  13 +++++++++++++
>  tools/libxl/libxl_internal.h |   2 ++
>  tools/libxl/libxl_pci.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl  |   6 ++++++
>  tools/libxl/xl_cmdimpl.c     |  19 +++++++++++++++++--
>  5 files changed, 162 insertions(+), 2 deletions(-)
> 
> Thanks
> Tiejun
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Chen, Tiejun]

On 2015/3/6 23:53, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 06, 2015 at 05:08:21PM +0800, Tiejun Chen wrote:
>> When we're working to support IGD GFX passthrough with qemu
>> upstream, instead of "-gfx_passthru" we'd like to make that
>
> Could you also include in the cover letter an URL link to the latest
> discussion on this?

Thanks for reminding. I really should do this.

https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02050.html

Thanks
Tiejun

>
> Thank you.
>> a machine option, "-machine xxx,igd-passthru=on". This need
>> to bring a change on tool side.
>>
>> After a discussion with Campbell, we'd like to construct a table to record
>> all IGD devices we can support. If we hit that table, we should pass that
>> option. And so we also introduce a new field of type, 'gfx_passthru_kind',
>> to cooperate with 'gfx_passthru' to cover all scenarios like this,
>>
>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to false and
>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>>                                 and build_info.u.gfx_passthru_kind to IGD
>>
>> ----------------------------------------------------------------
>> Tiejun Chen (2):
>>        libxl: introduce libxl__is_igd_vga_passthru
>>        libxl: introduce gfx_passthru_kind
>>
>>   tools/libxl/libxl_dm.c       |  13 +++++++++++++
>>   tools/libxl/libxl_internal.h |   2 ++
>>   tools/libxl/libxl_pci.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/libxl/libxl_types.idl  |   6 ++++++
>>   tools/libxl/xl_cmdimpl.c     |  19 +++++++++++++++++--
>>   5 files changed, 162 insertions(+), 2 deletions(-)
>>
>> Thanks
>> Tiejun
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>