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

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

[Tiejun Chen]

v2:

* Refine patch #2's head description 
* Improve codes quality inside patch #1 based on Wei's comments
* Refill the summary inside patch #0 based on Konrad and Wei's suggestion

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".

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

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 true 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

And note actually that option "-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. 

----------------------------------------------------------------
Tiejun Chen (2):
      libxl: introduce libxl__is_igd_vga_passthru
      libxl: introduce gfx_passthru_kind

 tools/libxl/libxl_dm.c       |  15 ++++-
 tools/libxl/libxl_internal.h |   2 +
 tools/libxl/libxl_pci.c      | 124 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl  |   6 ++
 tools/libxl/xl_cmdimpl.c     |  22 ++++++-
 5 files changed, 164 insertions(+), 5 deletions(-)

Thanks
Tiejun

[Qemu-devel] [v2][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..c97c62d 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 bool 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..fc060c6 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 uint16_t sysfs_dev_get_vendor(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    char *pci_device_vendor_path =
+            GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+                      pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    uint16_t read_items;
+    uint16_t 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%hx\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 uint16_t sysfs_dev_get_device(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    char *pci_device_device_path =
+            GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/device",
+                      pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    uint16_t read_items;
+    uint16_t 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%hx\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.
+ */
+bool 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, pt_vendor, pt_device;
+
+    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+        libxl_device_pci *pcidev = &d_config->pcidevs[i];
+        pt_vendor = sysfs_dev_get_vendor(gc, pcidev);
+        pt_device = sysfs_dev_get_device(gc, pcidev);
+
+        if (pt_vendor == 0xffff || pt_device == 0xffff)
+            continue;
+
+        for (j = 0 ; j < num ; j++) {
+            vendor = fixup_ids[j].vendor;
+            device = fixup_ids[j].device;
+
+            if (pt_vendor == vendor &&  pt_device == 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] [v2][PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Ian Campbell]

On Tue, 2015-03-10 at 17:42 +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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> +    FILE *f = fopen(pci_device_vendor_path, "r");

I was about to ask if we should use the carefd construct here, but
libxl_pci.c is full of fopen, so this isn't making things any worse at
least.

Re: [v2][PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Ian Campbell]

On Tue, 2015-03-10 at 17:42 +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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> +    FILE *f = fopen(pci_device_vendor_path, "r");

I was about to ask if we should use the carefd construct here, but
libxl_pci.c is full of fopen, so this isn't making things any worse at
least.

[v2][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..c97c62d 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 bool 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..fc060c6 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 uint16_t sysfs_dev_get_vendor(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    char *pci_device_vendor_path =
+            GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+                      pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    uint16_t read_items;
+    uint16_t 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%hx\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 uint16_t sysfs_dev_get_device(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    char *pci_device_device_path =
+            GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/device",
+                      pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    uint16_t read_items;
+    uint16_t 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%hx\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.
+ */
+bool 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, pt_vendor, pt_device;
+
+    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+        libxl_device_pci *pcidev = &d_config->pcidevs[i];
+        pt_vendor = sysfs_dev_get_vendor(gc, pcidev);
+        pt_device = sysfs_dev_get_device(gc, pcidev);
+
+        if (pt_vendor == 0xffff || pt_device == 0xffff)
+            continue;
+
+        for (j = 0 ; j < num ; j++) {
+            vendor = fixup_ids[j].vendor;
+            device = fixup_ids[j].device;
+
+            if (pt_vendor == vendor &&  pt_device == 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

[v2][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 true 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.

And "-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.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxl/libxl_dm.c      | 15 ++++++++++++---
 tools/libxl/libxl_pci.c     |  4 ++--
 tools/libxl/libxl_types.idl |  6 ++++++
 tools/libxl/xl_cmdimpl.c    | 22 ++++++++++++++++++++--
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..2d06038 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,18 @@ 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_pci.c b/tools/libxl/libxl_pci.c
index fc060c6..9a534cc 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
             device = fixup_ids[j].device;
 
             if (pt_vendor == vendor &&  pt_device == device)
-                return 1;
+                return true;
         }
     }
 
-    return 0;
+    return false;
 }
 
 /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..d64ad10 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"),
+    ])
+
 # 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..d0d6ce3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,26 @@ 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

[Qemu-devel] [v2][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 true 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.

And "-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.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxl/libxl_dm.c      | 15 ++++++++++++---
 tools/libxl/libxl_pci.c     |  4 ++--
 tools/libxl/libxl_types.idl |  6 ++++++
 tools/libxl/xl_cmdimpl.c    | 22 ++++++++++++++++++++--
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..2d06038 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,18 @@ 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_pci.c b/tools/libxl/libxl_pci.c
index fc060c6..9a534cc 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
             device = fixup_ids[j].device;
 
             if (pt_vendor == vendor &&  pt_device == device)
-                return 1;
+                return true;
         }
     }
 
-    return 0;
+    return false;
 }
 
 /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..d64ad10 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"),
+    ])
+
 # 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..d0d6ce3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,26 @@ 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] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Tue, 2015-03-10 at 17:42 +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 true 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.
> 
> And "-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.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/libxl/libxl_dm.c      | 15 ++++++++++++---
>  tools/libxl/libxl_pci.c     |  4 ++--
>  tools/libxl/libxl_types.idl |  6 ++++++
>  tools/libxl/xl_cmdimpl.c    | 22 ++++++++++++++++++++--
>  4 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..2d06038 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,18 @@ 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");
> +        }

I think this whole block should be inside an:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
the semantics should be that kind is ignored unless passthru is enabled.

and then the if/else chain could become a switch perhaps?

I'm unsure what we should do if kind==DEFAULT but
libxl__is_igd_vga_passthru fails. At a minimum a warning seems
desirable. I'm not sure if it should warrant a failure or not.

> +
>          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_pci.c b/tools/libxl/libxl_pci.c
> index fc060c6..9a534cc 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
>              device = fixup_ids[j].device;
>  
>              if (pt_vendor == vendor &&  pt_device == device)
> -                return 1;
> +                return true;
>          }
>      }
>  
> -    return 0;
> +    return false;

Please fold this into the previous patch. (You may retain the ack I
gave).
>  }
>  
>  /*
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 02be466..d64ad10 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"),
> +    ])
> +
>  # 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),

Please add a #define LIBXL_HAVE_... to libxl.h to indicate that this
functionality is now available. There is a comment near the top
explaining why etc and a bunch of examples.

Note that the #define is for 3rd party code only, there is no need to
actually use it in either libxl or xl code.
                                      
>                                         ("serial",           string),
>                                         ("boot",             string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 440db78..d0d6ce3 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,26 @@ 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);
> +            }

The docs (xl.cfg man page) say, regarding booleans "A C<NUMBER>
interpreted as C<False> (C<0>) or C<True> (any other value)."

So just libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); should be
fine here.

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

Should be true, I see you wrote false in the commit message so I assume
this was my mistake from earlier as well, sorry about that.

Ian.

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

[Chen, Tiejun]

>> +
>> +        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");
>> +        }
>
> I think this whole block should be inside an:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
> the semantics should be that kind is ignored unless passthru is enabled.
>
> and then the if/else chain could become a switch perhaps?

Okay.

>
> I'm unsure what we should do if kind==DEFAULT but
> libxl__is_igd_vga_passthru fails. At a minimum a warning seems
> desirable. I'm not sure if it should warrant a failure or not.

I think a warning message plus abort() should be enough, and then user 
can determine what's next,

Please take a look at this,

+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);
+                } else {
+                    LOG(WARN, "No supported IGD to passthru,"
+                        " or please force set gfx_passthru=\"igd\".\n");
+                    abort();
+                }
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+                break;
+            }
+        }
+

>
>> +
>>           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_pci.c b/tools/libxl/libxl_pci.c
>> index fc060c6..9a534cc 100644
>> --- a/tools/libxl/libxl_pci.c
>> +++ b/tools/libxl/libxl_pci.c
>> @@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
>>               device = fixup_ids[j].device;
>>
>>               if (pt_vendor == vendor &&  pt_device == device)
>> -                return 1;
>> +                return true;
>>           }
>>       }
>>
>> -    return 0;
>> +    return false;
>
> Please fold this into the previous patch. (You may retain the ack I
> gave).

Thanks for your catch.

>>   }
>>
>>   /*
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 02be466..d64ad10 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"),
>> +    ])
>> +
>>   # 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),
>
> Please add a #define LIBXL_HAVE_... to libxl.h to indicate that this
> functionality is now available. There is a comment near the top
> explaining why etc and a bunch of examples.
>
> Note that the #define is for 3rd party code only, there is no need to
> actually use it in either libxl or xl code.

Like this,

@@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * LIBXL_HAVE_GFX_PASSTHRU_KIND
+ *
+ * If this is defined, the Graphic Device Passthrough Override is 
supported.
+ */
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
@@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
                               uint64_t *tsc_r);
  #endif

+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+#endif
+
  /* misc */

  /* Each of these sets or clears the flag according to whether the

But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
we need a conversion, or other idea?

>
>>                                          ("serial",           string),
>>                                          ("boot",             string),
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 440db78..d0d6ce3 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1953,8 +1953,26 @@ 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);
>> +            }
>
> The docs (xl.cfg man page) say, regarding booleans "A C<NUMBER>
> interpreted as C<False> (C<0>) or C<True> (any other value)."
>
> So just libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); should be
> fine here.
>
>> +        } 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);
>
> Should be true, I see you wrote false in the commit message so I assume
> this was my mistake from earlier as well, sorry about that.
>

Here I update this chunk of code based on your two comments:

@@ -1953,8 +1953,22 @@ 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, true);
+            } else {
+                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+            }
+        } 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, true);
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

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

[Ian Campbell]

On Thu, 2015-03-12 at 11:18 +0800, Chen, Tiejun wrote:
> >> +
> >> +        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");
> >> +        }
> >
> > I think this whole block should be inside an:
> >          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
> > the semantics should be that kind is ignored unless passthru is enabled.
> >
> > and then the if/else chain could become a switch perhaps?
> 
> Okay.
> 
> >
> > I'm unsure what we should do if kind==DEFAULT but
> > libxl__is_igd_vga_passthru fails. At a minimum a warning seems
> > desirable. I'm not sure if it should warrant a failure or not.
> 
> I think a warning message plus abort() should be enough, and then user 
> can determine what's next,

In libxl an abort is only warranted if something _cannot_ happen and is
not under user control. I think think that applies here unless some
earlier code is guaranteed to have validated the value before it reaches
this point.

> Please take a look at this,
> 
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
> machinearg);
> +                } else {
> +                    LOG(WARN, "No supported IGD to passthru,"
> +                        " or please force set gfx_passthru=\"igd\".\n");
> +                    abort();

I don't think you can abort here, since a user can set
b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
error.

> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   #define LIBXL_HAVE_PSR_MBM 1
>   #endif
> 
> +/*
> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
> + *
> + * If this is defined, the Graphic Device Passthrough Override is 
> supported.

Almost, please also explicitly name the type field as other similar
comments do for clarity.

> + */
> +#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
> +
>   typedef char **libxl_string_list;
>   void libxl_string_list_dispose(libxl_string_list *sl);
>   int libxl_string_list_length(const libxl_string_list *sl);
> @@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
>                                uint64_t *tsc_r);
>   #endif
> 
> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND

No ifdef needed here (or in libxl_internal.h)


> +bool libxl__is_igd_vga_passthru(libxl__gc *gc,
> +                                const libxl_domain_config *d_config);

and this should be in libxl_internal.h not here...

> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
> we need a conversion, or other idea?

... which answers this question.

> Here I update this chunk of code based on your two comments:
> 
> @@ -1953,8 +1953,22 @@ 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, true);
> +            } else {
> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +            }

This is exactly the same as:
        libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);

Ian.

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

[Chen, Tiejun]

> I don't think you can abort here, since a user can set
> b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
> error.

Then, looks I should do this,

                     LOG(ERROR, "No supported IGD to passthru,"
                         " or please force set gfx_passthru=\"igd\".\n");
                     return NULL;

>
>> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
>> libxl_mac *src);
>>    #define LIBXL_HAVE_PSR_MBM 1
>>    #endif
>>
>> +/*
>> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
>> + *
>> + * If this is defined, the Graphic Device Passthrough Override is
>> supported.
>
> Almost, please also explicitly name the type field as other similar
> comments do for clarity.

Okay, maybe something is like this,

+/*
+ * LIBXL_HAVE_IGD_GFX_PASSTHRU
+ *
+ * If this is defined, the IGD Graphic Device Passthrough is supported.
+ *
+ * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the
+ * libxl_device_pci field in the hvm is present in the pci_info structure
+ * fixup_ids[] which contains all supported IGD devices. So wwe use
+ * "igd-passthru=on" specify on the qemu command-line.
+ */
+#define LIBXL_HAVE_IGD_GFX_PASSTHRU 1
+

>> + */

[snip]

> and this should be in libxl_internal.h not here...

Okay.

I mistakenly understand we always have to expose this in libxl.h...

>
>> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess

[snip]

>> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
>> +            if (l) {
>> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
>> +            } else {
>> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
>> +            }
>
> This is exactly the same as:
>          libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
>

Sure.

Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:
> > I don't think you can abort here, since a user can set
> > b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
> > error.
> 
> Then, looks I should do this,
> 
>                      LOG(ERROR, "No supported IGD to passthru,"
>                          " or please force set gfx_passthru=\"igd\".\n");
>                      return NULL;

If I remember the context correctly this is in the autodetect case, so I
think shouldn't mention IGD. Something like "Unable to detect graphics
passthru kind, please set gfx_passthru_kind. See xl.cfg(5) for more
information."

> >
> >> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
> >> libxl_mac *src);
> >>    #define LIBXL_HAVE_PSR_MBM 1
> >>    #endif
> >>
> >> +/*
> >> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
> >> + *
> >> + * If this is defined, the Graphic Device Passthrough Override is
> >> supported.
> >
> > Almost, please also explicitly name the type field as other similar
> > comments do for clarity.
> 
> Okay, maybe something is like this,
> 
> +/*
> + * LIBXL_HAVE_IGD_GFX_PASSTHRU
> + *
> + * If this is defined, the IGD Graphic Device Passthrough is supported.
> + *
> + * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the
> + * libxl_device_pci field in the hvm is present in the pci_info structure
> + * fixup_ids[] which contains all supported IGD devices. So wwe use
> + * "igd-passthru=on" specify on the qemu command-line.

This:

/*
 * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
 * the libxl_gfx_passthru_kind enumeration is defined. 
 */
#define LIBXL_HAVE_GFX_PASSTHRU_KIND

Users don't care about the internal details, just about the existence of
the support.

Ian.

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

[Ian Campbell]

On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:
> > I don't think you can abort here, since a user can set
> > b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
> > error.
> 
> Then, looks I should do this,
> 
>                      LOG(ERROR, "No supported IGD to passthru,"
>                          " or please force set gfx_passthru=\"igd\".\n");
>                      return NULL;

If I remember the context correctly this is in the autodetect case, so I
think shouldn't mention IGD. Something like "Unable to detect graphics
passthru kind, please set gfx_passthru_kind. See xl.cfg(5) for more
information."

> >
> >> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
> >> libxl_mac *src);
> >>    #define LIBXL_HAVE_PSR_MBM 1
> >>    #endif
> >>
> >> +/*
> >> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
> >> + *
> >> + * If this is defined, the Graphic Device Passthrough Override is
> >> supported.
> >
> > Almost, please also explicitly name the type field as other similar
> > comments do for clarity.
> 
> Okay, maybe something is like this,
> 
> +/*
> + * LIBXL_HAVE_IGD_GFX_PASSTHRU
> + *
> + * If this is defined, the IGD Graphic Device Passthrough is supported.
> + *
> + * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the
> + * libxl_device_pci field in the hvm is present in the pci_info structure
> + * fixup_ids[] which contains all supported IGD devices. So wwe use
> + * "igd-passthru=on" specify on the qemu command-line.

This:

/*
 * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
 * the libxl_gfx_passthru_kind enumeration is defined. 
 */
#define LIBXL_HAVE_GFX_PASSTHRU_KIND

Users don't care about the internal details, just about the existence of
the support.

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/13 18:11, Ian Campbell wrote:
> On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:
>>> I don't think you can abort here, since a user can set
>>> b_info->u.hvm.gfx_passthru_kind to default. You would need to
>>> return an error.
>>
>> Then, looks I should do this,
>>
>> LOG(ERROR, "No supported IGD to passthru," " or please force set
>> gfx_passthru=\"igd\".\n"); return NULL;
>
> If I remember the context correctly this is in the autodetect case,
> so I think shouldn't mention IGD. Something like "Unable to detect
> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> for more

s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
'gfx_passthru_kind' from 'gfx_passthru'.

> information."
>
>>>
>>>> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx,
>>>> libxl_mac *dst, libxl_mac *src);

[snip]

> /* * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field
> and * the libxl_gfx_passthru_kind enumeration is defined. */ #define
> LIBXL_HAVE_GFX_PASSTHRU_KIND
>
> Users don't care about the internal details, just about the existence
> of the support.
>

Updated.

Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/13 18:11, Ian Campbell wrote:
> On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:
>>> I don't think you can abort here, since a user can set
>>> b_info->u.hvm.gfx_passthru_kind to default. You would need to
>>> return an error.
>>
>> Then, looks I should do this,
>>
>> LOG(ERROR, "No supported IGD to passthru," " or please force set
>> gfx_passthru=\"igd\".\n"); return NULL;
>
> If I remember the context correctly this is in the autodetect case,
> so I think shouldn't mention IGD. Something like "Unable to detect
> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> for more

s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
'gfx_passthru_kind' from 'gfx_passthru'.

> information."
>
>>>
>>>> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx,
>>>> libxl_mac *dst, libxl_mac *src);

[snip]

> /* * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field
> and * the libxl_gfx_passthru_kind enumeration is defined. */ #define
> LIBXL_HAVE_GFX_PASSTHRU_KIND
>
> Users don't care about the internal details, just about the existence
> of the support.
>

Updated.

Thanks
Tiejun

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

[Ian Campbell]

On Mon, 2015-03-16 at 09:07 +0800, Chen, Tiejun wrote:
> On 2015/3/13 18:11, Ian Campbell wrote:
> > On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:
> >>> I don't think you can abort here, since a user can set
> >>> b_info->u.hvm.gfx_passthru_kind to default. You would need to
> >>> return an error.
> >>
> >> Then, looks I should do this,
> >>
> >> LOG(ERROR, "No supported IGD to passthru," " or please force set
> >> gfx_passthru=\"igd\".\n"); return NULL;
> >
> > If I remember the context correctly this is in the autodetect case,
> > so I think shouldn't mention IGD. Something like "Unable to detect
> > graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> > for more
> 
> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
> 'gfx_passthru_kind' from 'gfx_passthru'.

I think you have it backwards.

In the case here gfx_passthru=1 has been set by the user, but
gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
failed.

So if the user wants to make progress they should set gfx_passthru_kind
to whatever type of passthrough they were trying to do.

Alternatively I suppose you could recommend removing gfx_passthru=1 (or
changing to=0), but given they've set =1 that doesn't seem to be the
most productive suggestion.

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

>>> If I remember the context correctly this is in the autodetect case,
>>> so I think shouldn't mention IGD. Something like "Unable to detect
>>> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
>>> for more
>>
>> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
>> 'gfx_passthru_kind' from 'gfx_passthru'.
>
> I think you have it backwards.
>
> In the case here gfx_passthru=1 has been set by the user, but
> gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
> failed.
>
> So if the user wants to make progress they should set gfx_passthru_kind
> to whatever type of passthrough they were trying to do.

Looks you're saying 'gfx_passthru_kind' shouldn't reply on 
'gfx_passthru', so 'gfx_passthru_kind' can override that previous value 
parsed from 'gfx_passthru', right?

>
> Alternatively I suppose you could recommend removing gfx_passthru=1 (or

I'm still keep that currently.

> changing to=0), but given they've set =1 that doesn't seem to be the
> most productive suggestion.
>

So looks the whole policy should be something like this,

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..5518759 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,27 @@ 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)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } 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, true);
+        }
+        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
+                        buf);
+                exit (1);
+            }
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

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

[Chen, Tiejun]

>>> If I remember the context correctly this is in the autodetect case,
>>> so I think shouldn't mention IGD. Something like "Unable to detect
>>> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
>>> for more
>>
>> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
>> 'gfx_passthru_kind' from 'gfx_passthru'.
>
> I think you have it backwards.
>
> In the case here gfx_passthru=1 has been set by the user, but
> gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
> failed.
>
> So if the user wants to make progress they should set gfx_passthru_kind
> to whatever type of passthrough they were trying to do.

Looks you're saying 'gfx_passthru_kind' shouldn't reply on 
'gfx_passthru', so 'gfx_passthru_kind' can override that previous value 
parsed from 'gfx_passthru', right?

>
> Alternatively I suppose you could recommend removing gfx_passthru=1 (or

I'm still keep that currently.

> changing to=0), but given they've set =1 that doesn't seem to be the
> most productive suggestion.
>

So looks the whole policy should be something like this,

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..5518759 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,27 @@ 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)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } 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, true);
+        }
+        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
+                        buf);
+                exit (1);
+            }
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

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

[Ian Campbell]

On Tue, 2015-03-17 at 15:46 +0800, Chen, Tiejun wrote:
> >>> If I remember the context correctly this is in the autodetect case,
> >>> so I think shouldn't mention IGD. Something like "Unable to detect
> >>> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> >>> for more
> >>
> >> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
> >> 'gfx_passthru_kind' from 'gfx_passthru'.
> >
> > I think you have it backwards.
> >
> > In the case here gfx_passthru=1 has been set by the user, but
> > gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
> > failed.
> >
> > So if the user wants to make progress they should set gfx_passthru_kind
> > to whatever type of passthrough they were trying to do.
> 
> Looks you're saying 'gfx_passthru_kind' shouldn't reply on 
> 'gfx_passthru', so 'gfx_passthru_kind' can override that previous value 
> parsed from 'gfx_passthru', right?

I think perhaps the confusion arises because libxl and xl differ here. I
was commenting on some libxl code, which has two fields, whereas you are
talking about the xl cfg level, which only has one, which can be parsed
in two ways. So I think I've probably been adding lots of confusion
here, sorry.

I think the _libxl_ message needs to be just "Unable to detect graphics
passthru kind". i.e. it can't/shouldn't reference anything to do with xl
config options etc (which would make no sense if libvirt was being
used).

That's not very user friendly though, so you may want to consider adding
a new specific error code for this case and returning it here, such that
xl or libvirt can then give a more comprehensible error message.

> So looks the whole policy should be something like this,
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5c40e84..5518759 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,27 @@ 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)) {
> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
> +        } 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, true);
> +        }

Up to here is fine.

> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +        }

I don't think you need this bit.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

> I think the _libxl_ message needs to be just "Unable to detect graphics
> passthru kind". i.e. it can't/shouldn't reference anything to do with xl
> config options etc (which would make no sense if libvirt was being
> used).
>
> That's not very user friendly though, so you may want to consider adding
> a new specific error code for this case and returning it here, such that
> xl or libvirt can then give a more comprehensible error message.

But,
     args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
                                           guest_config, d_state, NULL);
	|
	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
guest_config, state, dm_state_fd);
		|
		+  Currently we always return "NULL" inside.

     if (!args) {
         ret = ERROR_FAIL;
         goto out;
     }

So I'm not very sure how to pass this new specific error code to xl/libvirt.

>
>> So looks the whole policy should be something like this,
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 5c40e84..5518759 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1953,8 +1953,27 @@ 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)) {
>> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
>> +        } 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, true);
>> +        }
>
> Up to here is fine.

Thanks but I guess I'd better to paste this whole patch to avoid I'm 
still missing something :)

---
  tools/libxl/libxl.h          |  6 ++++++
  tools/libxl/libxl_dm.c       | 25 ++++++++++++++++++++++---
  tools/libxl/libxl_internal.h |  5 +++++
  tools/libxl/libxl_types.idl  |  6 ++++++
  tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
  5 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..045d48a 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,28 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);
+                } else {
+                    LOG(ERROR, "Unable to detect graphics passthru kind,"
+                        " please set gfx_passthru_kind. See xl.cfg(5) 
for more"
+                        " information.\n");
+                    return NULL;
+                }
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+                break;
+            }
+        }
+
          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 c97c62d..8912421 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,11 @@ static inline void 
libxl__update_config_vtpm(libxl__gc *gc,
   */
  void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                      const libxl_bitmap *sptr);
+
+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+#endif
  #endif

  /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..76642a8 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"),
+    ])
+
  # 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 5c40e84..3ea3833 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,18 @@ 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)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } 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, true);
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))


Thanks
Tiejun

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

[Chen, Tiejun]

> I think the _libxl_ message needs to be just "Unable to detect graphics
> passthru kind". i.e. it can't/shouldn't reference anything to do with xl
> config options etc (which would make no sense if libvirt was being
> used).
>
> That's not very user friendly though, so you may want to consider adding
> a new specific error code for this case and returning it here, such that
> xl or libvirt can then give a more comprehensible error message.

But,
     args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
                                           guest_config, d_state, NULL);
	|
	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
guest_config, state, dm_state_fd);
		|
		+  Currently we always return "NULL" inside.

     if (!args) {
         ret = ERROR_FAIL;
         goto out;
     }

So I'm not very sure how to pass this new specific error code to xl/libvirt.

>
>> So looks the whole policy should be something like this,
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 5c40e84..5518759 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1953,8 +1953,27 @@ 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)) {
>> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
>> +        } 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, true);
>> +        }
>
> Up to here is fine.

Thanks but I guess I'd better to paste this whole patch to avoid I'm 
still missing something :)

---
  tools/libxl/libxl.h          |  6 ++++++
  tools/libxl/libxl_dm.c       | 25 ++++++++++++++++++++++---
  tools/libxl/libxl_internal.h |  5 +++++
  tools/libxl/libxl_types.idl  |  6 ++++++
  tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
  5 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..045d48a 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,28 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);
+                } else {
+                    LOG(ERROR, "Unable to detect graphics passthru kind,"
+                        " please set gfx_passthru_kind. See xl.cfg(5) 
for more"
+                        " information.\n");
+                    return NULL;
+                }
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+                break;
+            }
+        }
+
          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 c97c62d..8912421 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,11 @@ static inline void 
libxl__update_config_vtpm(libxl__gc *gc,
   */
  void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                      const libxl_bitmap *sptr);
+
+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+#endif
  #endif

  /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..76642a8 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"),
+    ])
+
  # 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 5c40e84..3ea3833 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,18 @@ 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)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } 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, true);
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))


Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Wed, 2015-03-18 at 15:32 +0800, Chen, Tiejun wrote:
> > I think the _libxl_ message needs to be just "Unable to detect graphics
> > passthru kind". i.e. it can't/shouldn't reference anything to do with xl
> > config options etc (which would make no sense if libvirt was being
> > used).
> >
> > That's not very user friendly though, so you may want to consider adding
> > a new specific error code for this case and returning it here, such that
> > xl or libvirt can then give a more comprehensible error message.
> 
> But,
>      args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
>                                            guest_config, d_state, NULL);
> 	|
> 	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
> guest_config, state, dm_state_fd);
> 		|
> 		+  Currently we always return "NULL" inside.
> 
>      if (!args) {
>          ret = ERROR_FAIL;
>          goto out;
>      }
> 
> So I'm not very sure how to pass this new specific error code to xl/libvirt.

Bah, yes. I don't think it is reasonable to ask you to rework the error
handling here completely for this. Lets leave this as a potential future
enhancement.

> Thanks but I guess I'd better to paste this whole patch to avoid I'm 
> still missing something :)
> 
> ---
>   tools/libxl/libxl.h          |  6 ++++++
>   tools/libxl/libxl_dm.c       | 25 ++++++++++++++++++++++---
>   tools/libxl/libxl_internal.h |  5 +++++
>   tools/libxl/libxl_types.idl  |  6 ++++++
>   tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
>   5 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bbc52d..62b3ae5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   #define LIBXL_HAVE_PSR_MBM 1
>   #endif
> 
> +/*
> + * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
> + * the libxl_gfx_passthru_kind enumeration is defined.
> +*/
> +#define LIBXL_HAVE_GFX_PASSTHRU_KIND
> +
>   typedef char **libxl_string_list;
>   void libxl_string_list_dispose(libxl_string_list *sl);
>   int libxl_string_list_length(const libxl_string_list *sl);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..045d48a 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,28 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case _info->u.hvm.gfx_passthru_kind:
> +                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
> machinearg);
> +                } else {
> +                    LOG(ERROR, "Unable to detect graphics passthru kind,"
> +                        " please set gfx_passthru_kind. See xl.cfg(5) 
> for more"
> +                        " information.\n");

Please change this to "Unable to detect graphics passthru kind" to avoid
talking xl-isms in libxl.

> +                    return NULL;
> +                }
> +                break;
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;

This duplicates the code from above. I think this would be best done as:

static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
{
    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
        return 0;

    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
        b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
        return 0;
    }

    LOG(ERROR, "Unable to detect graphics passthru kind");
    return 1;
}

Then for the code in libxl__build_device_model_args_new:

         if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
             if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
                  return NULL
             switch (b_info->u.hvm.gfx_passthru_kind) {
             case LIBXL_GFX_PASSTHRU_KIND_IGD:
                 machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
                 break;
             default:
                 LOG(ERROR, "unknown gfx_passthru_kind\n");
                return NULL;
             }
        }

That is, a helper to try and autodetect kind if it is default and then a
single switch entry for each kind.

> +            default:
> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");

Please return an error here, as I've shown above.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c97c62d..8912421 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3632,6 +3632,11 @@ static inline void 
> libxl__update_config_vtpm(libxl__gc *gc,
>    */
>   void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>                                       const libxl_bitmap *sptr);
> +
> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND

No need for this ifdef.

Ian.

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

[Ian Campbell]

On Wed, 2015-03-18 at 15:32 +0800, Chen, Tiejun wrote:
> > I think the _libxl_ message needs to be just "Unable to detect graphics
> > passthru kind". i.e. it can't/shouldn't reference anything to do with xl
> > config options etc (which would make no sense if libvirt was being
> > used).
> >
> > That's not very user friendly though, so you may want to consider adding
> > a new specific error code for this case and returning it here, such that
> > xl or libvirt can then give a more comprehensible error message.
> 
> But,
>      args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
>                                            guest_config, d_state, NULL);
> 	|
> 	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
> guest_config, state, dm_state_fd);
> 		|
> 		+  Currently we always return "NULL" inside.
> 
>      if (!args) {
>          ret = ERROR_FAIL;
>          goto out;
>      }
> 
> So I'm not very sure how to pass this new specific error code to xl/libvirt.

Bah, yes. I don't think it is reasonable to ask you to rework the error
handling here completely for this. Lets leave this as a potential future
enhancement.

> Thanks but I guess I'd better to paste this whole patch to avoid I'm 
> still missing something :)
> 
> ---
>   tools/libxl/libxl.h          |  6 ++++++
>   tools/libxl/libxl_dm.c       | 25 ++++++++++++++++++++++---
>   tools/libxl/libxl_internal.h |  5 +++++
>   tools/libxl/libxl_types.idl  |  6 ++++++
>   tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
>   5 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bbc52d..62b3ae5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   #define LIBXL_HAVE_PSR_MBM 1
>   #endif
> 
> +/*
> + * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
> + * the libxl_gfx_passthru_kind enumeration is defined.
> +*/
> +#define LIBXL_HAVE_GFX_PASSTHRU_KIND
> +
>   typedef char **libxl_string_list;
>   void libxl_string_list_dispose(libxl_string_list *sl);
>   int libxl_string_list_length(const libxl_string_list *sl);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..045d48a 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,28 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case _info->u.hvm.gfx_passthru_kind:
> +                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
> machinearg);
> +                } else {
> +                    LOG(ERROR, "Unable to detect graphics passthru kind,"
> +                        " please set gfx_passthru_kind. See xl.cfg(5) 
> for more"
> +                        " information.\n");

Please change this to "Unable to detect graphics passthru kind" to avoid
talking xl-isms in libxl.

> +                    return NULL;
> +                }
> +                break;
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;

This duplicates the code from above. I think this would be best done as:

static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
{
    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
        return 0;

    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
        b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
        return 0;
    }

    LOG(ERROR, "Unable to detect graphics passthru kind");
    return 1;
}

Then for the code in libxl__build_device_model_args_new:

         if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
             if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
                  return NULL
             switch (b_info->u.hvm.gfx_passthru_kind) {
             case LIBXL_GFX_PASSTHRU_KIND_IGD:
                 machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
                 break;
             default:
                 LOG(ERROR, "unknown gfx_passthru_kind\n");
                return NULL;
             }
        }

That is, a helper to try and autodetect kind if it is default and then a
single switch entry for each kind.

> +            default:
> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");

Please return an error here, as I've shown above.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c97c62d..8912421 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3632,6 +3632,11 @@ static inline void 
> libxl__update_config_vtpm(libxl__gc *gc,
>    */
>   void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>                                       const libxl_bitmap *sptr);
> +
> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND

No need for this ifdef.

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

> This duplicates the code from above. I think this would be best done as:
>
> static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> {
>      if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>          return 0;
>
>      if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>          b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>          return 0;
>      }
>
>      LOG(ERROR, "Unable to detect graphics passthru kind");
>      return 1;
> }
>
> Then for the code in libxl__build_device_model_args_new:
>
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>               if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
>                    return NULL
>               switch (b_info->u.hvm.gfx_passthru_kind) {
>               case LIBXL_GFX_PASSTHRU_KIND_IGD:
>                   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>                   break;
>               default:
>                   LOG(ERROR, "unknown gfx_passthru_kind\n");
>                  return NULL;
>               }
>          }
>
> That is, a helper to try and autodetect kind if it is default and then a
> single switch entry for each kind.
>
>> +            default:
>> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
>
> Please return an error here, as I've shown above.

Looks good and thanks, but here 'guest_config' is a const so we 
shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,

b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;

So I tried to refactor a little bit to follow up yours,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..605b17c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static int
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    LOG(ERROR, "Unable to detect graphics passthru kind");
+    return -1;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -427,7 +444,7 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
      const char *keymap = dm_keymap(guest_config);
      char *machinearg;
      flexarray_t *dm_args;
-    int i, connection, devid;
+    int i, connection, devid, gfx_passthru_kind;
      uint64_t ram_size;
      const char *path, *chardev;

@@ -710,9 +727,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 +771,20 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
+ 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(ERROR, "unknown gfx_passthru_kind\n");
+                return NULL;
+            }
+        }
+
          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 c97c62d..8912421 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -3632,6 +3632,11 @@ static inline void
>> libxl__update_config_vtpm(libxl__gc *gc,
>>     */
>>    void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>>                                        const libxl_bitmap *sptr);
>> +
>> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
>
> No need for this ifdef.

Removed.

Thanks
Tiejun

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

[Chen, Tiejun]

> This duplicates the code from above. I think this would be best done as:
>
> static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> {
>      if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>          return 0;
>
>      if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>          b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>          return 0;
>      }
>
>      LOG(ERROR, "Unable to detect graphics passthru kind");
>      return 1;
> }
>
> Then for the code in libxl__build_device_model_args_new:
>
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>               if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
>                    return NULL
>               switch (b_info->u.hvm.gfx_passthru_kind) {
>               case LIBXL_GFX_PASSTHRU_KIND_IGD:
>                   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>                   break;
>               default:
>                   LOG(ERROR, "unknown gfx_passthru_kind\n");
>                  return NULL;
>               }
>          }
>
> That is, a helper to try and autodetect kind if it is default and then a
> single switch entry for each kind.
>
>> +            default:
>> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
>
> Please return an error here, as I've shown above.

Looks good and thanks, but here 'guest_config' is a const so we 
shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,

b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;

So I tried to refactor a little bit to follow up yours,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..605b17c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static int
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    LOG(ERROR, "Unable to detect graphics passthru kind");
+    return -1;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -427,7 +444,7 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
      const char *keymap = dm_keymap(guest_config);
      char *machinearg;
      flexarray_t *dm_args;
-    int i, connection, devid;
+    int i, connection, devid, gfx_passthru_kind;
      uint64_t ram_size;
      const char *path, *chardev;

@@ -710,9 +727,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 +771,20 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
+ 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(ERROR, "unknown gfx_passthru_kind\n");
+                return NULL;
+            }
+        }
+
          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 c97c62d..8912421 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -3632,6 +3632,11 @@ static inline void
>> libxl__update_config_vtpm(libxl__gc *gc,
>>     */
>>    void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>>                                        const libxl_bitmap *sptr);
>> +
>> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
>
> No need for this ifdef.

Removed.

Thanks
Tiejun

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

[Ian Campbell]

On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:
> > This duplicates the code from above. I think this would be best done as:
> >
> > static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> > {
> >      if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> >          return 0;
> >
> >      if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> >          b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> >          return 0;
> >      }
> >
> >      LOG(ERROR, "Unable to detect graphics passthru kind");
> >      return 1;
> > }
> >
> > Then for the code in libxl__build_device_model_args_new:
> >
> >           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >               if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
> >                    return NULL
> >               switch (b_info->u.hvm.gfx_passthru_kind) {
> >               case LIBXL_GFX_PASSTHRU_KIND_IGD:
> >                   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> >                   break;
> >               default:
> >                   LOG(ERROR, "unknown gfx_passthru_kind\n");
> >                  return NULL;
> >               }
> >          }
> >
> > That is, a helper to try and autodetect kind if it is default and then a
> > single switch entry for each kind.
> >
> >> +            default:
> >> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> >
> > Please return an error here, as I've shown above.
> 
> Looks good and thanks, but here 'guest_config' is a const so we 
> shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,
> 
> b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> 
> So I tried to refactor a little bit to follow up yours,
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..605b17c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>       return opt;
>   }
> 
> +static int
> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
> +                                const libxl_domain_config *guest_config)
> +{
> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
> +
> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> +        return b_info->u.hvm.gfx_passthru_kind;
> +
> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
> +    }
> +
> +    LOG(ERROR, "Unable to detect graphics passthru kind");
> +    return -1;

I think you can make this function return enum
libxl_gfx_passthrough_kind and then return
LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.

> +}
> +
>   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                           const char *dm, int guest_domid,
>                                           const libxl_domain_config 
> *guest_config,
> @@ -427,7 +444,7 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>       const char *keymap = dm_keymap(guest_config);
>       char *machinearg;
>       flexarray_t *dm_args;
> -    int i, connection, devid;
> +    int i, connection, devid, gfx_passthru_kind;

Please declare this in the smallest necessary scope...
[...]
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,

i.e. here.

> + 
> guest_config);
> +            switch (gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;
> +            default:

With the suggestion to return KIND_DEFAULT if detection fails then I
think an extra case should be added:
        case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
                LOG(ERROR, "unable to detect required
                gfx_passthru_kind");
                return NULL;

> +                LOG(ERROR, "unknown gfx_passthru_kind\n");

I think LOG is supposed to not include the final \n.

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/19 18:44, Ian Campbell wrote:
> On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:
>>> This duplicates the code from above. I think this would be best done as:
>>>
>>> static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
>>> {
>>>       if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>>>           return 0;
>>>
>>>       if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>>>           b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>>>           return 0;
>>>       }
>>>
>>>       LOG(ERROR, "Unable to detect graphics passthru kind");
>>>       return 1;
>>> }
>>>
>>> Then for the code in libxl__build_device_model_args_new:
>>>
>>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>>                if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
>>>                     return NULL
>>>                switch (b_info->u.hvm.gfx_passthru_kind) {
>>>                case LIBXL_GFX_PASSTHRU_KIND_IGD:
>>>                    machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>>>                    break;
>>>                default:
>>>                    LOG(ERROR, "unknown gfx_passthru_kind\n");
>>>                   return NULL;
>>>                }
>>>           }
>>>
>>> That is, a helper to try and autodetect kind if it is default and then a
>>> single switch entry for each kind.
>>>
>>>> +            default:
>>>> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
>>>
>>> Please return an error here, as I've shown above.
>>
>> Looks good and thanks, but here 'guest_config' is a const so we
>> shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,
>>
>> b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>>
>> So I tried to refactor a little bit to follow up yours,
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 8599a6a..605b17c 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>>        return opt;
>>    }
>>
>> +static int
>> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
>> +                                const libxl_domain_config *guest_config)
>> +{
>> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
>> +
>> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>> +        return b_info->u.hvm.gfx_passthru_kind;
>> +
>> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
>> +    }
>> +
>> +    LOG(ERROR, "Unable to detect graphics passthru kind");
>> +    return -1;
>
> I think you can make this function return enum
> libxl_gfx_passthrough_kind and then return
> LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.
>
>> +}
>> +
>>    static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                            const char *dm, int guest_domid,
>>                                            const libxl_domain_config
>> *guest_config,
>> @@ -427,7 +444,7 @@ static char **
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>        const char *keymap = dm_keymap(guest_config);
>>        char *machinearg;
>>        flexarray_t *dm_args;
>> -    int i, connection, devid;
>> +    int i, connection, devid, gfx_passthru_kind;
>
> Please declare this in the smallest necessary scope...
> [...]
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
>
> i.e. here.
>
>> +
>> guest_config);
>> +            switch (gfx_passthru_kind) {
>> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
>> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>> +                break;
>> +            default:
>
> With the suggestion to return KIND_DEFAULT if detection fails then I
> think an extra case should be added:
>          case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                  LOG(ERROR, "unable to detect required
>                  gfx_passthru_kind");
>                  return NULL;
>
>> +                LOG(ERROR, "unknown gfx_passthru_kind\n");
>
> I think LOG is supposed to not include the final \n.
>

Refactor again,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..05c8916 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static enum libxl_gfx_passthru_kind
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    LOG(ERROR, "Unable to detect graphics passthru kind");
+    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -757,6 +771,21 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            enum libxl_gfx_passthru_kind gfx_passthru_kind =
+                            libxl__detect_gfx_passthru_kind(gc, 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                LOG(ERROR, "unable to detect required gfx_passthru_kind");
+            default:
+                return NULL;
+            }
+        }
+
          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]);


Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/19 18:44, Ian Campbell wrote:
> On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:
>>> This duplicates the code from above. I think this would be best done as:
>>>
>>> static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
>>> {
>>>       if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>>>           return 0;
>>>
>>>       if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>>>           b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>>>           return 0;
>>>       }
>>>
>>>       LOG(ERROR, "Unable to detect graphics passthru kind");
>>>       return 1;
>>> }
>>>
>>> Then for the code in libxl__build_device_model_args_new:
>>>
>>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>>                if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
>>>                     return NULL
>>>                switch (b_info->u.hvm.gfx_passthru_kind) {
>>>                case LIBXL_GFX_PASSTHRU_KIND_IGD:
>>>                    machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>>>                    break;
>>>                default:
>>>                    LOG(ERROR, "unknown gfx_passthru_kind\n");
>>>                   return NULL;
>>>                }
>>>           }
>>>
>>> That is, a helper to try and autodetect kind if it is default and then a
>>> single switch entry for each kind.
>>>
>>>> +            default:
>>>> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
>>>
>>> Please return an error here, as I've shown above.
>>
>> Looks good and thanks, but here 'guest_config' is a const so we
>> shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,
>>
>> b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>>
>> So I tried to refactor a little bit to follow up yours,
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 8599a6a..605b17c 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>>        return opt;
>>    }
>>
>> +static int
>> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
>> +                                const libxl_domain_config *guest_config)
>> +{
>> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
>> +
>> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
>> +        return b_info->u.hvm.gfx_passthru_kind;
>> +
>> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
>> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
>> +    }
>> +
>> +    LOG(ERROR, "Unable to detect graphics passthru kind");
>> +    return -1;
>
> I think you can make this function return enum
> libxl_gfx_passthrough_kind and then return
> LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.
>
>> +}
>> +
>>    static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                            const char *dm, int guest_domid,
>>                                            const libxl_domain_config
>> *guest_config,
>> @@ -427,7 +444,7 @@ static char **
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>        const char *keymap = dm_keymap(guest_config);
>>        char *machinearg;
>>        flexarray_t *dm_args;
>> -    int i, connection, devid;
>> +    int i, connection, devid, gfx_passthru_kind;
>
> Please declare this in the smallest necessary scope...
> [...]
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
>
> i.e. here.
>
>> +
>> guest_config);
>> +            switch (gfx_passthru_kind) {
>> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
>> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
>> +                break;
>> +            default:
>
> With the suggestion to return KIND_DEFAULT if detection fails then I
> think an extra case should be added:
>          case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                  LOG(ERROR, "unable to detect required
>                  gfx_passthru_kind");
>                  return NULL;
>
>> +                LOG(ERROR, "unknown gfx_passthru_kind\n");
>
> I think LOG is supposed to not include the final \n.
>

Refactor again,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..05c8916 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static enum libxl_gfx_passthru_kind
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    LOG(ERROR, "Unable to detect graphics passthru kind");
+    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -757,6 +771,21 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            enum libxl_gfx_passthru_kind gfx_passthru_kind =
+                            libxl__detect_gfx_passthru_kind(gc, 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                LOG(ERROR, "unable to detect required gfx_passthru_kind");
+            default:
+                return NULL;
+            }
+        }
+
          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]);


Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Fri, 2015-03-20 at 09:04 +0800, Chen, Tiejun wrote:
> Refactor again,
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..05c8916 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>       return opt;
>   }
> 
> +static enum libxl_gfx_passthru_kind
> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
> +                                const libxl_domain_config *guest_config)
> +{
> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
> +
> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> +        return b_info->u.hvm.gfx_passthru_kind;
> +
> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
> +    }
> +
> +    LOG(ERROR, "Unable to detect graphics passthru kind");
> +    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
> +}
> +
>   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                           const char *dm, int guest_domid,
>                                           const libxl_domain_config 
> *guest_config,
> @@ -757,6 +771,21 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            enum libxl_gfx_passthru_kind gfx_passthru_kind =
> +                            libxl__detect_gfx_passthru_kind(gc, 
> guest_config);
> +            switch (gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +                LOG(ERROR, "unable to detect required gfx_passthru_kind");

In this case you will now have logged twice. I'd suggest logging only
here and not in the helper.

> +            default:

And this case is subtly different to LIBXL_GFX_PASSTHRU_KIND_DEFAULT
since it would indicate some sort of corruption. So, I would drop the
logging on failure in libxl__detect_gfx_passthru_kind and here do:
           case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
               LOG(ERROR, "unable to detect required gfx_passthru_kind");
               return NULL;
           default:
               LOG(ERROR, "invalid value for gfx_passthru_kind");
               return NULL;

Ian

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

[Ian Campbell]

On Fri, 2015-03-20 at 09:04 +0800, Chen, Tiejun wrote:
> Refactor again,
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..05c8916 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>       return opt;
>   }
> 
> +static enum libxl_gfx_passthru_kind
> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
> +                                const libxl_domain_config *guest_config)
> +{
> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
> +
> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> +        return b_info->u.hvm.gfx_passthru_kind;
> +
> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
> +    }
> +
> +    LOG(ERROR, "Unable to detect graphics passthru kind");
> +    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
> +}
> +
>   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                           const char *dm, int guest_domid,
>                                           const libxl_domain_config 
> *guest_config,
> @@ -757,6 +771,21 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            enum libxl_gfx_passthru_kind gfx_passthru_kind =
> +                            libxl__detect_gfx_passthru_kind(gc, 
> guest_config);
> +            switch (gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +                LOG(ERROR, "unable to detect required gfx_passthru_kind");

In this case you will now have logged twice. I'd suggest logging only
here and not in the helper.

> +            default:

And this case is subtly different to LIBXL_GFX_PASSTHRU_KIND_DEFAULT
since it would indicate some sort of corruption. So, I would drop the
logging on failure in libxl__detect_gfx_passthru_kind and here do:
           case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
               LOG(ERROR, "unable to detect required gfx_passthru_kind");
               return NULL;
           default:
               LOG(ERROR, "invalid value for gfx_passthru_kind");
               return NULL;

Ian

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

[Chen, Tiejun]

>> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>> +                LOG(ERROR, "unable to detect required gfx_passthru_kind");
>
> In this case you will now have logged twice. I'd suggest logging only
> here and not in the helper.
>
>> +            default:
>
> And this case is subtly different to LIBXL_GFX_PASSTHRU_KIND_DEFAULT
> since it would indicate some sort of corruption. So, I would drop the
> logging on failure in libxl__detect_gfx_passthru_kind and here do:
>             case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                 LOG(ERROR, "unable to detect required gfx_passthru_kind");
>                 return NULL;
>             default:
>                 LOG(ERROR, "invalid value for gfx_passthru_kind");
>                 return NULL;
>


Okay, I update this patch. If this is fine to you I'd like to send this 
whole series.

---
  tools/libxl/libxl.h          |  6 ++++++
  tools/libxl/libxl_dm.c       | 36 +++++++++++++++++++++++++++++++++---
  tools/libxl/libxl_internal.h |  4 ++++
  tools/libxl/libxl_types.idl  |  6 ++++++
  tools/libxl/xl_cmdimpl.c     | 23 +++++++++++++++++++++--
  5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..bf72103 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,22 @@ static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static enum libxl_gfx_passthru_kind
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -710,9 +726,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 +770,23 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            enum libxl_gfx_passthru_kind gfx_passthru_kind =
+                            libxl__detect_gfx_passthru_kind(gc, 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                LOG(ERROR, "unable to detect required gfx_passthru_kind");
+                return NULL;
+            default:
+                LOG(ERROR, "invalid value for gfx_passthru_kind");
+                return NULL;
+            }
+        }
+
          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 c97c62d..26a01cb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,10 @@ static inline void 
libxl__update_config_vtpm(libxl__gc *gc,
   */
  void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                      const libxl_bitmap *sptr);
+
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+
  #endif

  /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..76642a8 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"),
+    ])
+
  # 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 5c40e84..5518759 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,27 @@ 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)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } 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, true);
+        }
+        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
+                        buf);
+                exit (1);
+            }
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

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

[Ian Campbell]

On Fri, 2015-03-20 at 18:08 +0800, Chen, Tiejun wrote:
> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +        }

This unnecessary bit seems to have crept back in.

Don't forget to update the manpages if you haven't already.

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

[Chen, Tiejun]

On 2015/3/20 18:11, Ian Campbell wrote:
> On Fri, 2015-03-20 at 18:08 +0800, Chen, Tiejun wrote:
>> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
>> +                        buf);
>> +                exit (1);
>> +            }
>> +        }
>
> This unnecessary bit seems to have crept back in.

Sorry to hit this again when I address other comments.

>
> Don't forget to update the manpages if you haven't already.
>

Looks I need to update docs/man/xl.cfg.pod.5.

Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/20 18:11, Ian Campbell wrote:
> On Fri, 2015-03-20 at 18:08 +0800, Chen, Tiejun wrote:
>> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
>> +                        buf);
>> +                exit (1);
>> +            }
>> +        }
>
> This unnecessary bit seems to have crept back in.

Sorry to hit this again when I address other comments.

>
> Don't forget to update the manpages if you haven't already.
>

Looks I need to update docs/man/xl.cfg.pod.5.

Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Fri, 2015-03-20 at 18:08 +0800, Chen, Tiejun wrote:
> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +        }

This unnecessary bit seems to have crept back in.

Don't forget to update the manpages if you haven't already.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

>> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>> +                LOG(ERROR, "unable to detect required gfx_passthru_kind");
>
> In this case you will now have logged twice. I'd suggest logging only
> here and not in the helper.
>
>> +            default:
>
> And this case is subtly different to LIBXL_GFX_PASSTHRU_KIND_DEFAULT
> since it would indicate some sort of corruption. So, I would drop the
> logging on failure in libxl__detect_gfx_passthru_kind and here do:
>             case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                 LOG(ERROR, "unable to detect required gfx_passthru_kind");
>                 return NULL;
>             default:
>                 LOG(ERROR, "invalid value for gfx_passthru_kind");
>                 return NULL;
>


Okay, I update this patch. If this is fine to you I'd like to send this 
whole series.

---
  tools/libxl/libxl.h          |  6 ++++++
  tools/libxl/libxl_dm.c       | 36 +++++++++++++++++++++++++++++++++---
  tools/libxl/libxl_internal.h |  4 ++++
  tools/libxl/libxl_types.idl  |  6 ++++++
  tools/libxl/xl_cmdimpl.c     | 23 +++++++++++++++++++++--
  5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..bf72103 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,22 @@ static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static enum libxl_gfx_passthru_kind
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -710,9 +726,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 +770,23 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            enum libxl_gfx_passthru_kind gfx_passthru_kind =
+                            libxl__detect_gfx_passthru_kind(gc, 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                LOG(ERROR, "unable to detect required gfx_passthru_kind");
+                return NULL;
+            default:
+                LOG(ERROR, "invalid value for gfx_passthru_kind");
+                return NULL;
+            }
+        }
+
          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 c97c62d..26a01cb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,10 @@ static inline void 
libxl__update_config_vtpm(libxl__gc *gc,
   */
  void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                      const libxl_bitmap *sptr);
+
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+
  #endif

  /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..76642a8 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"),
+    ])
+
  # 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 5c40e84..5518759 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,27 @@ 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)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } 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, true);
+        }
+        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
+                        buf);
+                exit (1);
+            }
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:
> > This duplicates the code from above. I think this would be best done as:
> >
> > static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> > {
> >      if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> >          return 0;
> >
> >      if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> >          b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> >          return 0;
> >      }
> >
> >      LOG(ERROR, "Unable to detect graphics passthru kind");
> >      return 1;
> > }
> >
> > Then for the code in libxl__build_device_model_args_new:
> >
> >           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >               if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
> >                    return NULL
> >               switch (b_info->u.hvm.gfx_passthru_kind) {
> >               case LIBXL_GFX_PASSTHRU_KIND_IGD:
> >                   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> >                   break;
> >               default:
> >                   LOG(ERROR, "unknown gfx_passthru_kind\n");
> >                  return NULL;
> >               }
> >          }
> >
> > That is, a helper to try and autodetect kind if it is default and then a
> > single switch entry for each kind.
> >
> >> +            default:
> >> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> >
> > Please return an error here, as I've shown above.
> 
> Looks good and thanks, but here 'guest_config' is a const so we 
> shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,
> 
> b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> 
> So I tried to refactor a little bit to follow up yours,
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..605b17c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
>       return opt;
>   }
> 
> +static int
> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
> +                                const libxl_domain_config *guest_config)
> +{
> +    const libxl_domain_build_info *b_info = &guest_config->b_info;
> +
> +    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> +        return b_info->u.hvm.gfx_passthru_kind;
> +
> +    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +        return LIBXL_GFX_PASSTHRU_KIND_IGD;
> +    }
> +
> +    LOG(ERROR, "Unable to detect graphics passthru kind");
> +    return -1;

I think you can make this function return enum
libxl_gfx_passthrough_kind and then return
LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.

> +}
> +
>   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                           const char *dm, int guest_domid,
>                                           const libxl_domain_config 
> *guest_config,
> @@ -427,7 +444,7 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>       const char *keymap = dm_keymap(guest_config);
>       char *machinearg;
>       flexarray_t *dm_args;
> -    int i, connection, devid;
> +    int i, connection, devid, gfx_passthru_kind;

Please declare this in the smallest necessary scope...
[...]
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,

i.e. here.

> + 
> guest_config);
> +            switch (gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;
> +            default:

With the suggestion to return KIND_DEFAULT if detection fails then I
think an extra case should be added:
        case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
                LOG(ERROR, "unable to detect required
                gfx_passthru_kind");
                return NULL;

> +                LOG(ERROR, "unknown gfx_passthru_kind\n");

I think LOG is supposed to not include the final \n.

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Tue, 2015-03-17 at 15:46 +0800, Chen, Tiejun wrote:
> >>> If I remember the context correctly this is in the autodetect case,
> >>> so I think shouldn't mention IGD. Something like "Unable to detect
> >>> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> >>> for more
> >>
> >> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
> >> 'gfx_passthru_kind' from 'gfx_passthru'.
> >
> > I think you have it backwards.
> >
> > In the case here gfx_passthru=1 has been set by the user, but
> > gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
> > failed.
> >
> > So if the user wants to make progress they should set gfx_passthru_kind
> > to whatever type of passthrough they were trying to do.
> 
> Looks you're saying 'gfx_passthru_kind' shouldn't reply on 
> 'gfx_passthru', so 'gfx_passthru_kind' can override that previous value 
> parsed from 'gfx_passthru', right?

I think perhaps the confusion arises because libxl and xl differ here. I
was commenting on some libxl code, which has two fields, whereas you are
talking about the xl cfg level, which only has one, which can be parsed
in two ways. So I think I've probably been adding lots of confusion
here, sorry.

I think the _libxl_ message needs to be just "Unable to detect graphics
passthru kind". i.e. it can't/shouldn't reference anything to do with xl
config options etc (which would make no sense if libvirt was being
used).

That's not very user friendly though, so you may want to consider adding
a new specific error code for this case and returning it here, such that
xl or libvirt can then give a more comprehensible error message.

> So looks the whole policy should be something like this,
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5c40e84..5518759 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,27 @@ 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)) {
> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
> +        } 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, true);
> +        }

Up to here is fine.

> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &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_kind\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +        }

I don't think you need this bit.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Mon, 2015-03-16 at 09:07 +0800, Chen, Tiejun wrote:
> On 2015/3/13 18:11, Ian Campbell wrote:
> > On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:
> >>> I don't think you can abort here, since a user can set
> >>> b_info->u.hvm.gfx_passthru_kind to default. You would need to
> >>> return an error.
> >>
> >> Then, looks I should do this,
> >>
> >> LOG(ERROR, "No supported IGD to passthru," " or please force set
> >> gfx_passthru=\"igd\".\n"); return NULL;
> >
> > If I remember the context correctly this is in the autodetect case,
> > so I think shouldn't mention IGD. Something like "Unable to detect
> > graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> > for more
> 
> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
> 'gfx_passthru_kind' from 'gfx_passthru'.

I think you have it backwards.

In the case here gfx_passthru=1 has been set by the user, but
gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
failed.

So if the user wants to make progress they should set gfx_passthru_kind
to whatever type of passthrough they were trying to do.

Alternatively I suppose you could recommend removing gfx_passthru=1 (or
changing to=0), but given they've set =1 that doesn't seem to be the
most productive suggestion.

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

> I don't think you can abort here, since a user can set
> b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
> error.

Then, looks I should do this,

                     LOG(ERROR, "No supported IGD to passthru,"
                         " or please force set gfx_passthru=\"igd\".\n");
                     return NULL;

>
>> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
>> libxl_mac *src);
>>    #define LIBXL_HAVE_PSR_MBM 1
>>    #endif
>>
>> +/*
>> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
>> + *
>> + * If this is defined, the Graphic Device Passthrough Override is
>> supported.
>
> Almost, please also explicitly name the type field as other similar
> comments do for clarity.

Okay, maybe something is like this,

+/*
+ * LIBXL_HAVE_IGD_GFX_PASSTHRU
+ *
+ * If this is defined, the IGD Graphic Device Passthrough is supported.
+ *
+ * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the
+ * libxl_device_pci field in the hvm is present in the pci_info structure
+ * fixup_ids[] which contains all supported IGD devices. So wwe use
+ * "igd-passthru=on" specify on the qemu command-line.
+ */
+#define LIBXL_HAVE_IGD_GFX_PASSTHRU 1
+

>> + */

[snip]

> and this should be in libxl_internal.h not here...

Okay.

I mistakenly understand we always have to expose this in libxl.h...

>
>> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess

[snip]

>> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
>> +            if (l) {
>> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
>> +            } else {
>> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
>> +            }
>
> This is exactly the same as:
>          libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
>

Sure.

Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Thu, 2015-03-12 at 11:18 +0800, Chen, Tiejun wrote:
> >> +
> >> +        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");
> >> +        }
> >
> > I think this whole block should be inside an:
> >          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
> > the semantics should be that kind is ignored unless passthru is enabled.
> >
> > and then the if/else chain could become a switch perhaps?
> 
> Okay.
> 
> >
> > I'm unsure what we should do if kind==DEFAULT but
> > libxl__is_igd_vga_passthru fails. At a minimum a warning seems
> > desirable. I'm not sure if it should warrant a failure or not.
> 
> I think a warning message plus abort() should be enough, and then user 
> can determine what's next,

In libxl an abort is only warranted if something _cannot_ happen and is
not under user control. I think think that applies here unless some
earlier code is guaranteed to have validated the value before it reaches
this point.

> Please take a look at this,
> 
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
> machinearg);
> +                } else {
> +                    LOG(WARN, "No supported IGD to passthru,"
> +                        " or please force set gfx_passthru=\"igd\".\n");
> +                    abort();

I don't think you can abort here, since a user can set
b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
error.

> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   #define LIBXL_HAVE_PSR_MBM 1
>   #endif
> 
> +/*
> + * LIBXL_HAVE_GFX_PASSTHRU_KIND
> + *
> + * If this is defined, the Graphic Device Passthrough Override is 
> supported.

Almost, please also explicitly name the type field as other similar
comments do for clarity.

> + */
> +#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
> +
>   typedef char **libxl_string_list;
>   void libxl_string_list_dispose(libxl_string_list *sl);
>   int libxl_string_list_length(const libxl_string_list *sl);
> @@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
>                                uint64_t *tsc_r);
>   #endif
> 
> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND

No ifdef needed here (or in libxl_internal.h)


> +bool libxl__is_igd_vga_passthru(libxl__gc *gc,
> +                                const libxl_domain_config *d_config);

and this should be in libxl_internal.h not here...

> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
> we need a conversion, or other idea?

... which answers this question.

> Here I update this chunk of code based on your two comments:
> 
> @@ -1953,8 +1953,22 @@ 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, true);
> +            } else {
> +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +            }

This is exactly the same as:
        libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);

Ian.

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

>> +
>> +        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");
>> +        }
>
> I think this whole block should be inside an:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
> the semantics should be that kind is ignored unless passthru is enabled.
>
> and then the if/else chain could become a switch perhaps?

Okay.

>
> I'm unsure what we should do if kind==DEFAULT but
> libxl__is_igd_vga_passthru fails. At a minimum a warning seems
> desirable. I'm not sure if it should warrant a failure or not.

I think a warning message plus abort() should be enough, and then user 
can determine what's next,

Please take a look at this,

+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);
+                } else {
+                    LOG(WARN, "No supported IGD to passthru,"
+                        " or please force set gfx_passthru=\"igd\".\n");
+                    abort();
+                }
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+                break;
+            }
+        }
+

>
>> +
>>           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_pci.c b/tools/libxl/libxl_pci.c
>> index fc060c6..9a534cc 100644
>> --- a/tools/libxl/libxl_pci.c
>> +++ b/tools/libxl/libxl_pci.c
>> @@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
>>               device = fixup_ids[j].device;
>>
>>               if (pt_vendor == vendor &&  pt_device == device)
>> -                return 1;
>> +                return true;
>>           }
>>       }
>>
>> -    return 0;
>> +    return false;
>
> Please fold this into the previous patch. (You may retain the ack I
> gave).

Thanks for your catch.

>>   }
>>
>>   /*
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 02be466..d64ad10 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"),
>> +    ])
>> +
>>   # 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),
>
> Please add a #define LIBXL_HAVE_... to libxl.h to indicate that this
> functionality is now available. There is a comment near the top
> explaining why etc and a bunch of examples.
>
> Note that the #define is for 3rd party code only, there is no need to
> actually use it in either libxl or xl code.

Like this,

@@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * LIBXL_HAVE_GFX_PASSTHRU_KIND
+ *
+ * If this is defined, the Graphic Device Passthrough Override is 
supported.
+ */
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
@@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
                               uint64_t *tsc_r);
  #endif

+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+#endif
+
  /* misc */

  /* Each of these sets or clears the flag according to whether the

But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
we need a conversion, or other idea?

>
>>                                          ("serial",           string),
>>                                          ("boot",             string),
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 440db78..d0d6ce3 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1953,8 +1953,26 @@ 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);
>> +            }
>
> The docs (xl.cfg man page) say, regarding booleans "A C<NUMBER>
> interpreted as C<False> (C<0>) or C<True> (any other value)."
>
> So just libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); should be
> fine here.
>
>> +        } 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);
>
> Should be true, I see you wrote false in the commit message so I assume
> this was my mistake from earlier as well, sorry about that.
>

Here I update this chunk of code based on your two comments:

@@ -1953,8 +1953,22 @@ 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, true);
+            } else {
+                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+            }
+        } 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, true);
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Tue, 2015-03-10 at 17:42 +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 true 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.
> 
> And "-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.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/libxl/libxl_dm.c      | 15 ++++++++++++---
>  tools/libxl/libxl_pci.c     |  4 ++--
>  tools/libxl/libxl_types.idl |  6 ++++++
>  tools/libxl/xl_cmdimpl.c    | 22 ++++++++++++++++++++--
>  4 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..2d06038 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,18 @@ 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");
> +        }

I think this whole block should be inside an:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
the semantics should be that kind is ignored unless passthru is enabled.

and then the if/else chain could become a switch perhaps?

I'm unsure what we should do if kind==DEFAULT but
libxl__is_igd_vga_passthru fails. At a minimum a warning seems
desirable. I'm not sure if it should warrant a failure or not.

> +
>          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_pci.c b/tools/libxl/libxl_pci.c
> index fc060c6..9a534cc 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
>              device = fixup_ids[j].device;
>  
>              if (pt_vendor == vendor &&  pt_device == device)
> -                return 1;
> +                return true;
>          }
>      }
>  
> -    return 0;
> +    return false;

Please fold this into the previous patch. (You may retain the ack I
gave).
>  }
>  
>  /*
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 02be466..d64ad10 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"),
> +    ])
> +
>  # 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),

Please add a #define LIBXL_HAVE_... to libxl.h to indicate that this
functionality is now available. There is a comment near the top
explaining why etc and a bunch of examples.

Note that the #define is for 3rd party code only, there is no need to
actually use it in either libxl or xl code.
                                      
>                                         ("serial",           string),
>                                         ("boot",             string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 440db78..d0d6ce3 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,26 @@ 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);
> +            }

The docs (xl.cfg man page) say, regarding booleans "A C<NUMBER>
interpreted as C<False> (C<0>) or C<True> (any other value)."

So just libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); should be
fine here.

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

Should be true, I see you wrote false in the commit message so I assume
this was my mistake from earlier as well, sorry about that.

Ian.