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

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

[Tiejun Chen]

v3:

* Refine some codes based on Campbell's feedback so thanks for Campbell's
  kind guideline to patch #2
* Update the manpages in patch #2

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

 docs/man/xl.cfg.pod.5        |  11 ++--
 tools/libxl/libxl.h          |   6 ++
 tools/libxl/libxl_dm.c       |  36 +++++++++-
 tools/libxl/libxl_internal.h |   6 ++
 tools/libxl/libxl_pci.c      | 124 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl  |   6 ++
 tools/libxl/xl_cmdimpl.c     |  14 +++-
 7 files changed, 194 insertions(+), 9 deletions(-)

Thanks
Tiejun

[Qemu-devel] [v3][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>
Acked-by: Ian Campbell <ian.campbell@citrix.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..9a534cc 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 true;
+        }
+    }
+
+    return false;
+}
+
 /*
  * A brief comment about slots.  I don't know what slots are for; however,
  * I have by experimentation determined:
-- 
1.9.1

[v3][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>
Acked-by: Ian Campbell <ian.campbell@citrix.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..9a534cc 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 true;
+        }
+    }
+
+    return false;
+}
+
 /*
  * A brief comment about slots.  I don't know what slots are for; however,
  * I have by experimentation determined:
-- 
1.9.1

[Qemu-devel] [v3][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>
---
 docs/man/xl.cfg.pod.5        | 11 +++++++----
 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     | 14 ++++++++++++--
 6 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 408653f..c29bcbf 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
 devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
 above.
 
-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru="STRING">
 
 Enable graphics device PCI passthrough. This option makes an assigned
 PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
 L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
 for currently supported graphics cards for gfx_passthru.
 
-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model. Note with the
+qemu-xen-traditional device-model this option is just treated as BOOLEAN
+actually, but with upstream qemu-xen device-model this option is extended
+to pass a specific device name to force work. Currently just 'igd' is
+defined to support Intel graphics device.
 
 Note that some graphics adapters (AMD/ATI cards, for example) do not
 necessarily require gfx_passthru option, so you can use the normal Xen
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..1144c5e 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 a8b08f2..5649024 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -416,6 +416,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,
@@ -727,9 +743,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");
@@ -774,6 +787,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 04faf98..b78b4ec 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1956,8 +1956,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))
-- 
1.9.1

[v3][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>
---
 docs/man/xl.cfg.pod.5        | 11 +++++++----
 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     | 14 ++++++++++++--
 6 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 408653f..c29bcbf 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
 devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
 above.
 
-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru="STRING">
 
 Enable graphics device PCI passthrough. This option makes an assigned
 PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
 L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
 for currently supported graphics cards for gfx_passthru.
 
-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model. Note with the
+qemu-xen-traditional device-model this option is just treated as BOOLEAN
+actually, but with upstream qemu-xen device-model this option is extended
+to pass a specific device name to force work. Currently just 'igd' is
+defined to support Intel graphics device.
 
 Note that some graphics adapters (AMD/ATI cards, for example) do not
 necessarily require gfx_passthru option, so you can use the normal Xen
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..1144c5e 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 a8b08f2..5649024 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -416,6 +416,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,
@@ -727,9 +743,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");
@@ -774,6 +787,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 04faf98..b78b4ec 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1956,8 +1956,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))
-- 
1.9.1

[Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

All guys,

Sorry to bother you.

I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and 
tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like 
pyxc_methods[] and pyxl_methods[]? And how should we call these approaches?

In my specific case, I'm trying to introduce a new flag to each a device 
while assigning device. So this means I have to add a parameter, 'flag', 
into

int xc_assign_device(
     xc_interface *xch,
     uint32_t domid,
     uint32_t machine_sbdf)

Then this is extended as

int xc_assign_device(
     xc_interface *xch,
     uint32_t domid,
     uint32_t machine_sbdf,
     uint32_t flag)

After this introduction, obviously I should cover all cases using 
xc_assign_device(). And also I found this fallout goes into these two 
files. For example, here pyxc_assign_device() is involved. Currently it 
has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str' 
should represent all pci devices with SBDF format, right?

But I don't know exactly what rule should be complied to construct this 
sort of flag into 'pci_str', or any reasonable idea to achieve my goal?

Thanks
Tiejun

One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

All guys,

Sorry to bother you.

I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and 
tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like 
pyxc_methods[] and pyxl_methods[]? And how should we call these approaches?

In my specific case, I'm trying to introduce a new flag to each a device 
while assigning device. So this means I have to add a parameter, 'flag', 
into

int xc_assign_device(
     xc_interface *xch,
     uint32_t domid,
     uint32_t machine_sbdf)

Then this is extended as

int xc_assign_device(
     xc_interface *xch,
     uint32_t domid,
     uint32_t machine_sbdf,
     uint32_t flag)

After this introduction, obviously I should cover all cases using 
xc_assign_device(). And also I found this fallout goes into these two 
files. For example, here pyxc_assign_device() is involved. Currently it 
has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str' 
should represent all pci devices with SBDF format, right?

But I don't know exactly what rule should be complied to construct this 
sort of flag into 'pci_str', or any reasonable idea to achieve my goal?

Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
> All guys,
> 
> Sorry to bother you.
> 
> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and 
> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like 
> pyxc_methods[] and pyxl_methods[]?

They are registered with the Python runtime, so they are called from
Python code. The first member of the struct is the pythonic function
name, e.g. from xc.c:
    { "domain_create", 
      (PyCFunction)pyxc_domain_create, 
      METH_VARARGS | METH_KEYWORDS, "\n"
      "Create a new domain.\n"
      " dom    [int, 0]:        Domain identifier to use (allocated if zero).\n"
      "Returns: [int] new domain identifier; -1 on error.\n" },
defines a method called domain_create, in the xen.lowlevel.xc namespace.

>  And how should we call these approaches?

I'm not sure what you are asking here.

> In my specific case, I'm trying to introduce a new flag to each a device 
> while assigning device. So this means I have to add a parameter, 'flag', 
> into
> 
> int xc_assign_device(
>      xc_interface *xch,
>      uint32_t domid,
>      uint32_t machine_sbdf)
> 
> Then this is extended as
> 
> int xc_assign_device(
>      xc_interface *xch,
>      uint32_t domid,
>      uint32_t machine_sbdf,
>      uint32_t flag)
> 
> After this introduction, obviously I should cover all cases using 
> xc_assign_device(). And also I found this fallout goes into these two 
> files. For example, here pyxc_assign_device() is involved. Currently it 
> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str' 
> should represent all pci devices with SBDF format, right?

It appears so, yes.

> But I don't know exactly what rule should be complied to construct this 
> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?

If it is non-trivial to fix them IMHO it is acceptable for the new
parameter to not be plumbed up to the Python bindings until someone
comes along with a requirement to use it from Python. IOW you can just
pass whatever the nop value is for the new argument.

Ian.

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
> All guys,
> 
> Sorry to bother you.
> 
> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and 
> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like 
> pyxc_methods[] and pyxl_methods[]?

They are registered with the Python runtime, so they are called from
Python code. The first member of the struct is the pythonic function
name, e.g. from xc.c:
    { "domain_create", 
      (PyCFunction)pyxc_domain_create, 
      METH_VARARGS | METH_KEYWORDS, "\n"
      "Create a new domain.\n"
      " dom    [int, 0]:        Domain identifier to use (allocated if zero).\n"
      "Returns: [int] new domain identifier; -1 on error.\n" },
defines a method called domain_create, in the xen.lowlevel.xc namespace.

>  And how should we call these approaches?

I'm not sure what you are asking here.

> In my specific case, I'm trying to introduce a new flag to each a device 
> while assigning device. So this means I have to add a parameter, 'flag', 
> into
> 
> int xc_assign_device(
>      xc_interface *xch,
>      uint32_t domid,
>      uint32_t machine_sbdf)
> 
> Then this is extended as
> 
> int xc_assign_device(
>      xc_interface *xch,
>      uint32_t domid,
>      uint32_t machine_sbdf,
>      uint32_t flag)
> 
> After this introduction, obviously I should cover all cases using 
> xc_assign_device(). And also I found this fallout goes into these two 
> files. For example, here pyxc_assign_device() is involved. Currently it 
> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str' 
> should represent all pci devices with SBDF format, right?

It appears so, yes.

> But I don't know exactly what rule should be complied to construct this 
> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?

If it is non-trivial to fix them IMHO it is acceptable for the new
parameter to not be plumbed up to the Python bindings until someone
comes along with a requirement to use it from Python. IOW you can just
pass whatever the nop value is for the new argument.

Ian.

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 17:51, Ian Campbell wrote:
> On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
>> All guys,
>>

Thanks for your reply.

>> Sorry to bother you.
>>
>> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
>> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
>> pyxc_methods[] and pyxl_methods[]?
>
> They are registered with the Python runtime, so they are called from
> Python code. The first member of the struct is the pythonic function

Sorry I don't understanding this. So seems you mean instead of xl, this 
is called by the third party user with python?

> name, e.g. from xc.c:
>      { "domain_create",

Otherwise, often we always perform `xl create xxx' to create a VM. So I 
think this should go into this flow like this,

xl_cmdtable.c:main_create()
	|
	+ create_domain()
		|
		+ libxl_domain_create_new()
			|
			+ do_domain_create()
				|
				+ ....
Right?

>        (PyCFunction)pyxc_domain_create,

So I don't see 'pyxc_domain_create' is called. Or I'm missing something...

>        METH_VARARGS | METH_KEYWORDS, "\n"
>        "Create a new domain.\n"
>        " dom    [int, 0]:        Domain identifier to use (allocated if zero).\n"
>        "Returns: [int] new domain identifier; -1 on error.\n" },
> defines a method called domain_create, in the xen.lowlevel.xc namespace.
>
>>   And how should we call these approaches?
>
> I'm not sure what you are asking here.

If you can give a real case to call this, things couldn't be better :)

>
>> In my specific case, I'm trying to introduce a new flag to each a device
>> while assigning device. So this means I have to add a parameter, 'flag',
>> into
>>
>> int xc_assign_device(
>>       xc_interface *xch,
>>       uint32_t domid,
>>       uint32_t machine_sbdf)
>>
>> Then this is extended as
>>
>> int xc_assign_device(
>>       xc_interface *xch,
>>       uint32_t domid,
>>       uint32_t machine_sbdf,
>>       uint32_t flag)
>>
>> After this introduction, obviously I should cover all cases using
>> xc_assign_device(). And also I found this fallout goes into these two
>> files. For example, here pyxc_assign_device() is involved. Currently it
>> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
>> should represent all pci devices with SBDF format, right?
>
> It appears so, yes.
>
>> But I don't know exactly what rule should be complied to construct this
>> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?
>
> If it is non-trivial to fix them IMHO it is acceptable for the new
> parameter to not be plumbed up to the Python bindings until someone
> comes along with a requirement to use it from Python. IOW you can just
> pass whatever the nop value is for the new argument.
>

Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But 
I'm not sure if I'm breaking the existing usage since like I said, I 
don't know what scenarios are using these methods.

Thanks
Tiejun

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 17:51, Ian Campbell wrote:
> On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
>> All guys,
>>

Thanks for your reply.

>> Sorry to bother you.
>>
>> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
>> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
>> pyxc_methods[] and pyxl_methods[]?
>
> They are registered with the Python runtime, so they are called from
> Python code. The first member of the struct is the pythonic function

Sorry I don't understanding this. So seems you mean instead of xl, this 
is called by the third party user with python?

> name, e.g. from xc.c:
>      { "domain_create",

Otherwise, often we always perform `xl create xxx' to create a VM. So I 
think this should go into this flow like this,

xl_cmdtable.c:main_create()
	|
	+ create_domain()
		|
		+ libxl_domain_create_new()
			|
			+ do_domain_create()
				|
				+ ....
Right?

>        (PyCFunction)pyxc_domain_create,

So I don't see 'pyxc_domain_create' is called. Or I'm missing something...

>        METH_VARARGS | METH_KEYWORDS, "\n"
>        "Create a new domain.\n"
>        " dom    [int, 0]:        Domain identifier to use (allocated if zero).\n"
>        "Returns: [int] new domain identifier; -1 on error.\n" },
> defines a method called domain_create, in the xen.lowlevel.xc namespace.
>
>>   And how should we call these approaches?
>
> I'm not sure what you are asking here.

If you can give a real case to call this, things couldn't be better :)

>
>> In my specific case, I'm trying to introduce a new flag to each a device
>> while assigning device. So this means I have to add a parameter, 'flag',
>> into
>>
>> int xc_assign_device(
>>       xc_interface *xch,
>>       uint32_t domid,
>>       uint32_t machine_sbdf)
>>
>> Then this is extended as
>>
>> int xc_assign_device(
>>       xc_interface *xch,
>>       uint32_t domid,
>>       uint32_t machine_sbdf,
>>       uint32_t flag)
>>
>> After this introduction, obviously I should cover all cases using
>> xc_assign_device(). And also I found this fallout goes into these two
>> files. For example, here pyxc_assign_device() is involved. Currently it
>> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
>> should represent all pci devices with SBDF format, right?
>
> It appears so, yes.
>
>> But I don't know exactly what rule should be complied to construct this
>> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?
>
> If it is non-trivial to fix them IMHO it is acceptable for the new
> parameter to not be plumbed up to the Python bindings until someone
> comes along with a requirement to use it from Python. IOW you can just
> pass whatever the nop value is for the new argument.
>

Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But 
I'm not sure if I'm breaking the existing usage since like I said, I 
don't know what scenarios are using these methods.

Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Tue, 2015-03-24 at 18:15 +0800, Chen, Tiejun wrote:
> On 2015/3/24 17:51, Ian Campbell wrote:
> > On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
> >> All guys,
> >>
> 
> Thanks for your reply.
> 
> >> Sorry to bother you.
> >>
> >> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
> >> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
> >> pyxc_methods[] and pyxl_methods[]?
> >
> > They are registered with the Python runtime, so they are called from
> > Python code. The first member of the struct is the pythonic function
> 
> Sorry I don't understanding this. So seems you mean instead of xl, this 
> is called by the third party user with python?

Yes, tools/python/xen is the python bindings for various C libraries
supported by Xen.

NB, the libxl ones are broken and not even compiled right now, you can
ignore them.

> 
> > name, e.g. from xc.c:
> >      { "domain_create",
> 
> Otherwise, often we always perform `xl create xxx' to create a VM. So I 
> think this should go into this flow like this,
> 
> xl_cmdtable.c:main_create()
> 	|
> 	+ create_domain()
> 		|
> 		+ libxl_domain_create_new()
> 			|
> 			+ do_domain_create()
> 				|
> 				+ ....
> Right?

Yes, xl is written in C not python so tools/python doesn't enter the
picture.

> 
> >        (PyCFunction)pyxc_domain_create,
> 
> So I don't see 'pyxc_domain_create' is called. Or I'm missing something...

Chances are that there are no intree users of this code any more, xend
would have used it at one time with something like:
	import xen.lowlevel.xc
        xc = xen.lowlevel.xc.xc()
        xc.domain_create()
etc.

> >
> >> In my specific case, I'm trying to introduce a new flag to each a device
> >> while assigning device. So this means I have to add a parameter, 'flag',
> >> into
> >>
> >> int xc_assign_device(
> >>       xc_interface *xch,
> >>       uint32_t domid,
> >>       uint32_t machine_sbdf)
> >>
> >> Then this is extended as
> >>
> >> int xc_assign_device(
> >>       xc_interface *xch,
> >>       uint32_t domid,
> >>       uint32_t machine_sbdf,
> >>       uint32_t flag)
> >>
> >> After this introduction, obviously I should cover all cases using
> >> xc_assign_device(). And also I found this fallout goes into these two
> >> files. For example, here pyxc_assign_device() is involved. Currently it
> >> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
> >> should represent all pci devices with SBDF format, right?
> >
> > It appears so, yes.
> >
> >> But I don't know exactly what rule should be complied to construct this
> >> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?
> >
> > If it is non-trivial to fix them IMHO it is acceptable for the new
> > parameter to not be plumbed up to the Python bindings until someone
> > comes along with a requirement to use it from Python. IOW you can just
> > pass whatever the nop value is for the new argument.
> >
> 
> Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But 
> I'm not sure if I'm breaking the existing usage since like I said, I 
> don't know what scenarios are using these methods.

Like I said in the paragraph above, if it is complicated then it is fine
to ignore this new parameter from Python.

I don't know what the semantics of flag is, if it is per SBDF then I
suppose if you really wanted to expose this here then you would need to
invent some syntax for doing so.

Ian.

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Tue, 2015-03-24 at 18:15 +0800, Chen, Tiejun wrote:
> On 2015/3/24 17:51, Ian Campbell wrote:
> > On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
> >> All guys,
> >>
> 
> Thanks for your reply.
> 
> >> Sorry to bother you.
> >>
> >> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
> >> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
> >> pyxc_methods[] and pyxl_methods[]?
> >
> > They are registered with the Python runtime, so they are called from
> > Python code. The first member of the struct is the pythonic function
> 
> Sorry I don't understanding this. So seems you mean instead of xl, this 
> is called by the third party user with python?

Yes, tools/python/xen is the python bindings for various C libraries
supported by Xen.

NB, the libxl ones are broken and not even compiled right now, you can
ignore them.

> 
> > name, e.g. from xc.c:
> >      { "domain_create",
> 
> Otherwise, often we always perform `xl create xxx' to create a VM. So I 
> think this should go into this flow like this,
> 
> xl_cmdtable.c:main_create()
> 	|
> 	+ create_domain()
> 		|
> 		+ libxl_domain_create_new()
> 			|
> 			+ do_domain_create()
> 				|
> 				+ ....
> Right?

Yes, xl is written in C not python so tools/python doesn't enter the
picture.

> 
> >        (PyCFunction)pyxc_domain_create,
> 
> So I don't see 'pyxc_domain_create' is called. Or I'm missing something...

Chances are that there are no intree users of this code any more, xend
would have used it at one time with something like:
	import xen.lowlevel.xc
        xc = xen.lowlevel.xc.xc()
        xc.domain_create()
etc.

> >
> >> In my specific case, I'm trying to introduce a new flag to each a device
> >> while assigning device. So this means I have to add a parameter, 'flag',
> >> into
> >>
> >> int xc_assign_device(
> >>       xc_interface *xch,
> >>       uint32_t domid,
> >>       uint32_t machine_sbdf)
> >>
> >> Then this is extended as
> >>
> >> int xc_assign_device(
> >>       xc_interface *xch,
> >>       uint32_t domid,
> >>       uint32_t machine_sbdf,
> >>       uint32_t flag)
> >>
> >> After this introduction, obviously I should cover all cases using
> >> xc_assign_device(). And also I found this fallout goes into these two
> >> files. For example, here pyxc_assign_device() is involved. Currently it
> >> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
> >> should represent all pci devices with SBDF format, right?
> >
> > It appears so, yes.
> >
> >> But I don't know exactly what rule should be complied to construct this
> >> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?
> >
> > If it is non-trivial to fix them IMHO it is acceptable for the new
> > parameter to not be plumbed up to the Python bindings until someone
> > comes along with a requirement to use it from Python. IOW you can just
> > pass whatever the nop value is for the new argument.
> >
> 
> Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But 
> I'm not sure if I'm breaking the existing usage since like I said, I 
> don't know what scenarios are using these methods.

Like I said in the paragraph above, if it is complicated then it is fine
to ignore this new parameter from Python.

I don't know what the semantics of flag is, if it is per SBDF then I
suppose if you really wanted to expose this here then you would need to
invent some syntax for doing so.

Ian.

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 18:20, Ian Campbell wrote:
> On Tue, 2015-03-24 at 18:15 +0800, Chen, Tiejun wrote:
>> On 2015/3/24 17:51, Ian Campbell wrote:
>>> On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
>>>> All guys,
>>>>
>>
>> Thanks for your reply.
>>
>>>> Sorry to bother you.
>>>>
>>>> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
>>>> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
>>>> pyxc_methods[] and pyxl_methods[]?
>>>
>>> They are registered with the Python runtime, so they are called from
>>> Python code. The first member of the struct is the pythonic function
>>
>> Sorry I don't understanding this. So seems you mean instead of xl, this
>> is called by the third party user with python?
>
> Yes, tools/python/xen is the python bindings for various C libraries
> supported by Xen.

Thanks for your explanation.

>
> NB, the libxl ones are broken and not even compiled right now, you can
> ignore them.

Looks this is still compiled now.

>
>>
>>> name, e.g. from xc.c:
>>>       { "domain_create",
>>
>> Otherwise, often we always perform `xl create xxx' to create a VM. So I
>> think this should go into this flow like this,
>>
>> xl_cmdtable.c:main_create()
>> 	|
>> 	+ create_domain()
>> 		|
>> 		+ libxl_domain_create_new()
>> 			|
>> 			+ do_domain_create()
>> 				|
>> 				+ ....
>> Right?
>
> Yes, xl is written in C not python so tools/python doesn't enter the
> picture.

Yeah.

>
>>
>>>         (PyCFunction)pyxc_domain_create,
>>
>> So I don't see 'pyxc_domain_create' is called. Or I'm missing something...
>
> Chances are that there are no intree users of this code any more, xend
> would have used it at one time with something like:
> 	import xen.lowlevel.xc
>          xc = xen.lowlevel.xc.xc()
>          xc.domain_create()
> etc.
>
>>>
>>>> In my specific case, I'm trying to introduce a new flag to each a device
>>>> while assigning device. So this means I have to add a parameter, 'flag',
>>>> into
>>>>
>>>> int xc_assign_device(
>>>>        xc_interface *xch,
>>>>        uint32_t domid,
>>>>        uint32_t machine_sbdf)
>>>>
>>>> Then this is extended as
>>>>
>>>> int xc_assign_device(
>>>>        xc_interface *xch,
>>>>        uint32_t domid,
>>>>        uint32_t machine_sbdf,
>>>>        uint32_t flag)
>>>>
>>>> After this introduction, obviously I should cover all cases using
>>>> xc_assign_device(). And also I found this fallout goes into these two
>>>> files. For example, here pyxc_assign_device() is involved. Currently it
>>>> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
>>>> should represent all pci devices with SBDF format, right?
>>>
>>> It appears so, yes.
>>>
>>>> But I don't know exactly what rule should be complied to construct this
>>>> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?
>>>
>>> If it is non-trivial to fix them IMHO it is acceptable for the new
>>> parameter to not be plumbed up to the Python bindings until someone
>>> comes along with a requirement to use it from Python. IOW you can just
>>> pass whatever the nop value is for the new argument.
>>>
>>
>> Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But
>> I'm not sure if I'm breaking the existing usage since like I said, I
>> don't know what scenarios are using these methods.
>
> Like I said in the paragraph above, if it is complicated then it is fine
> to ignore this new parameter from Python.
>
> I don't know what the semantics of flag is, if it is per SBDF then I

Yes, this should be a flag specific to a SBDF.

You know, I'm working to fix RMRR completely. Based on some discussion 
about that design ( I assume you may read that thread previously :) ), 
now we probably need to pass a flag to introduce our policy.

> suppose if you really wanted to expose this here then you would need to
> invent some syntax for doing so.
>

Definitely.

When I finish this I will send you to review technically.

Again, really appreciate your clarification to me.

Thanks
Tiejun

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 18:20, Ian Campbell wrote:
> On Tue, 2015-03-24 at 18:15 +0800, Chen, Tiejun wrote:
>> On 2015/3/24 17:51, Ian Campbell wrote:
>>> On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:
>>>> All guys,
>>>>
>>
>> Thanks for your reply.
>>
>>>> Sorry to bother you.
>>>>
>>>> I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
>>>> tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
>>>> pyxc_methods[] and pyxl_methods[]?
>>>
>>> They are registered with the Python runtime, so they are called from
>>> Python code. The first member of the struct is the pythonic function
>>
>> Sorry I don't understanding this. So seems you mean instead of xl, this
>> is called by the third party user with python?
>
> Yes, tools/python/xen is the python bindings for various C libraries
> supported by Xen.

Thanks for your explanation.

>
> NB, the libxl ones are broken and not even compiled right now, you can
> ignore them.

Looks this is still compiled now.

>
>>
>>> name, e.g. from xc.c:
>>>       { "domain_create",
>>
>> Otherwise, often we always perform `xl create xxx' to create a VM. So I
>> think this should go into this flow like this,
>>
>> xl_cmdtable.c:main_create()
>> 	|
>> 	+ create_domain()
>> 		|
>> 		+ libxl_domain_create_new()
>> 			|
>> 			+ do_domain_create()
>> 				|
>> 				+ ....
>> Right?
>
> Yes, xl is written in C not python so tools/python doesn't enter the
> picture.

Yeah.

>
>>
>>>         (PyCFunction)pyxc_domain_create,
>>
>> So I don't see 'pyxc_domain_create' is called. Or I'm missing something...
>
> Chances are that there are no intree users of this code any more, xend
> would have used it at one time with something like:
> 	import xen.lowlevel.xc
>          xc = xen.lowlevel.xc.xc()
>          xc.domain_create()
> etc.
>
>>>
>>>> In my specific case, I'm trying to introduce a new flag to each a device
>>>> while assigning device. So this means I have to add a parameter, 'flag',
>>>> into
>>>>
>>>> int xc_assign_device(
>>>>        xc_interface *xch,
>>>>        uint32_t domid,
>>>>        uint32_t machine_sbdf)
>>>>
>>>> Then this is extended as
>>>>
>>>> int xc_assign_device(
>>>>        xc_interface *xch,
>>>>        uint32_t domid,
>>>>        uint32_t machine_sbdf,
>>>>        uint32_t flag)
>>>>
>>>> After this introduction, obviously I should cover all cases using
>>>> xc_assign_device(). And also I found this fallout goes into these two
>>>> files. For example, here pyxc_assign_device() is involved. Currently it
>>>> has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
>>>> should represent all pci devices with SBDF format, right?
>>>
>>> It appears so, yes.
>>>
>>>> But I don't know exactly what rule should be complied to construct this
>>>> sort of flag into 'pci_str', or any reasonable idea to achieve my goal?
>>>
>>> If it is non-trivial to fix them IMHO it is acceptable for the new
>>> parameter to not be plumbed up to the Python bindings until someone
>>> comes along with a requirement to use it from Python. IOW you can just
>>> pass whatever the nop value is for the new argument.
>>>
>>
>> Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But
>> I'm not sure if I'm breaking the existing usage since like I said, I
>> don't know what scenarios are using these methods.
>
> Like I said in the paragraph above, if it is complicated then it is fine
> to ignore this new parameter from Python.
>
> I don't know what the semantics of flag is, if it is per SBDF then I

Yes, this should be a flag specific to a SBDF.

You know, I'm working to fix RMRR completely. Based on some discussion 
about that design ( I assume you may read that thread previously :) ), 
now we probably need to pass a flag to introduce our policy.

> suppose if you really wanted to expose this here then you would need to
> invent some syntax for doing so.
>

Definitely.

When I finish this I will send you to review technically.

Again, really appreciate your clarification to me.

Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Tue, 2015-03-24 at 18:31 +0800, Chen, Tiejun wrote:
> > NB, the libxl ones are broken and not even compiled right now, you can
> > ignore them.
> 
> Looks this is still compiled now.

xc is, xl is not, I am sure of that.

> > I don't know what the semantics of flag is, if it is per SBDF then I
> 
> Yes, this should be a flag specific to a SBDF.
> 
> You know, I'm working to fix RMRR completely. Based on some discussion 
> about that design ( I assume you may read that thread previously :) ), 
> now we probably need to pass a flag to introduce our policy.

Unless you have a concrete requirement to expose RMRR via the Python
bindings to libxc (i.e. you know somebody is using them) then I think
you should not bother.

Making RMRR work via the (C) interface to libxl used by xl and libvirt
is sufficient for a new in tree feature.

Ian.
> > suppose if you really wanted to expose this here then you would need to
> > invent some syntax for doing so.
> >
> 
> Definitely.
> 
> When I finish this I will send you to review technically.
> 
> Again, really appreciate your clarification to me.
> 
> Thanks
> Tiejun

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Tue, 2015-03-24 at 18:31 +0800, Chen, Tiejun wrote:
> > NB, the libxl ones are broken and not even compiled right now, you can
> > ignore them.
> 
> Looks this is still compiled now.

xc is, xl is not, I am sure of that.

> > I don't know what the semantics of flag is, if it is per SBDF then I
> 
> Yes, this should be a flag specific to a SBDF.
> 
> You know, I'm working to fix RMRR completely. Based on some discussion 
> about that design ( I assume you may read that thread previously :) ), 
> now we probably need to pass a flag to introduce our policy.

Unless you have a concrete requirement to expose RMRR via the Python
bindings to libxc (i.e. you know somebody is using them) then I think
you should not bother.

Making RMRR work via the (C) interface to libxl used by xl and libvirt
is sufficient for a new in tree feature.

Ian.
> > suppose if you really wanted to expose this here then you would need to
> > invent some syntax for doing so.
> >
> 
> Definitely.
> 
> When I finish this I will send you to review technically.
> 
> Again, really appreciate your clarification to me.
> 
> Thanks
> Tiejun

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

[Ian Campbell]

On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:
> Although we already have 'gfx_passthru' in b_info, this doesn' suffice

                                                                ^t

> after we want to handle IGD specifically. Now we define a new field of
> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> this means we can benefit this to support other specific devices just
> by extending gfx_passthru_kind. And then we can cooperate with
> gfx_passthru to address IGD cases as follows:
> 
>     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>     gfx_passthru = 1    => sets build_info.u.gfx_passthru to true and
>                            build_info.u.gfx_passthru_kind to DEFAULT
>     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
                                                                   true

You had it right in the code.

>                                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>
> ---
>  docs/man/xl.cfg.pod.5        | 11 +++++++----
>  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     | 14 ++++++++++++--
>  6 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 408653f..c29bcbf 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
>  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
>  above.
>  
> -=item B<gfx_passthru=BOOLEAN>
> +=item B<gfx_passthru="STRING">

I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate.

>  Enable graphics device PCI passthrough. This option makes an assigned
>  PCI graphics card become primary graphics card in the VM. The QEMU
> @@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
>  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>  for currently supported graphics cards for gfx_passthru.
>  
> -gfx_passthru is currently only supported with the qemu-xen-traditional
> -device-model. Upstream qemu-xen device-model currently does not have
> -support for gfx_passthru.
> +gfx_passthru is currently supported both with the qemu-xen-traditional
> +device-model and upstream qemu-xen device-model. Note with the
> +qemu-xen-traditional device-model this option is just treated as BOOLEAN
> +actually, but with upstream qemu-xen device-model this option is extended
> +to pass a specific device name to force work. Currently just 'igd' is
> +defined to support Intel graphics device.

How about:

        When given as a boolean the B<gfx_passthru> option either
        disables gfx passthru or enables autodetection.
        
        When given as a string the B<gfx_passthru> option describes the
        type of passthru to enable.
        
        Valid options are:
        
        =over 4
        
        =item B<gfx_passthru=0>
        
        Disables gfx_passthru.
        
        =item B<gfx_passthru=1>, B<gfx_passthru="default">
        
        Enables gfx_passthru and autodetects the type of device which is
        being used.
        
        =item "igd"
        
        Enables gfx_passthru of the Intel Graphics Device.
        
        =back

        Note: When used with the qemu-xen-traditional device model only
        IGD passthru is supported.

(do check my pod syntax, I'm mostly making it up!)

The note at the end makes me thing that perhaps something ought to check
this constraint in the qemu-xen-traditional case. It might be easiest to
do it in libxl__build_device_model_args_old using the
libxl__detect_gfx_passthru_kind helper you have added

e.g. where libxl__build_device_model_args_old has:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
            flexarray_append(dm_args, "-gfx_passthru");
        }
would become something like:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
            if (libxl__detect_gfx_passthru_kind(gc, guest_config) !=
                LIBXL_GFX_PASSTHRU_KIND_IGD) {
                LOG("only IGD gfx_passthru is supported with qemu-xen-traditional");
                return NULL;
            }
            flexarray_append(dm_args, "-gfx_passthru");
        }

Or something like that. What do you think?

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

This should be in the previous patch. In fact I think it is and this is
a redundant second instance.

Ian.

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

[Ian Campbell]

On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:
> Although we already have 'gfx_passthru' in b_info, this doesn' suffice

                                                                ^t

> after we want to handle IGD specifically. Now we define a new field of
> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
> this means we can benefit this to support other specific devices just
> by extending gfx_passthru_kind. And then we can cooperate with
> gfx_passthru to address IGD cases as follows:
> 
>     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>     gfx_passthru = 1    => sets build_info.u.gfx_passthru to true and
>                            build_info.u.gfx_passthru_kind to DEFAULT
>     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
                                                                   true

You had it right in the code.

>                                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>
> ---
>  docs/man/xl.cfg.pod.5        | 11 +++++++----
>  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     | 14 ++++++++++++--
>  6 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 408653f..c29bcbf 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
>  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
>  above.
>  
> -=item B<gfx_passthru=BOOLEAN>
> +=item B<gfx_passthru="STRING">

I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate.

>  Enable graphics device PCI passthrough. This option makes an assigned
>  PCI graphics card become primary graphics card in the VM. The QEMU
> @@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
>  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>  for currently supported graphics cards for gfx_passthru.
>  
> -gfx_passthru is currently only supported with the qemu-xen-traditional
> -device-model. Upstream qemu-xen device-model currently does not have
> -support for gfx_passthru.
> +gfx_passthru is currently supported both with the qemu-xen-traditional
> +device-model and upstream qemu-xen device-model. Note with the
> +qemu-xen-traditional device-model this option is just treated as BOOLEAN
> +actually, but with upstream qemu-xen device-model this option is extended
> +to pass a specific device name to force work. Currently just 'igd' is
> +defined to support Intel graphics device.

How about:

        When given as a boolean the B<gfx_passthru> option either
        disables gfx passthru or enables autodetection.
        
        When given as a string the B<gfx_passthru> option describes the
        type of passthru to enable.
        
        Valid options are:
        
        =over 4
        
        =item B<gfx_passthru=0>
        
        Disables gfx_passthru.
        
        =item B<gfx_passthru=1>, B<gfx_passthru="default">
        
        Enables gfx_passthru and autodetects the type of device which is
        being used.
        
        =item "igd"
        
        Enables gfx_passthru of the Intel Graphics Device.
        
        =back

        Note: When used with the qemu-xen-traditional device model only
        IGD passthru is supported.

(do check my pod syntax, I'm mostly making it up!)

The note at the end makes me thing that perhaps something ought to check
this constraint in the qemu-xen-traditional case. It might be easiest to
do it in libxl__build_device_model_args_old using the
libxl__detect_gfx_passthru_kind helper you have added

e.g. where libxl__build_device_model_args_old has:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
            flexarray_append(dm_args, "-gfx_passthru");
        }
would become something like:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
            if (libxl__detect_gfx_passthru_kind(gc, guest_config) !=
                LIBXL_GFX_PASSTHRU_KIND_IGD) {
                LOG("only IGD gfx_passthru is supported with qemu-xen-traditional");
                return NULL;
            }
            flexarray_append(dm_args, "-gfx_passthru");
        }

Or something like that. What do you think?

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

This should be in the previous patch. In fact I think it is and this is
a redundant second instance.

Ian.

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

[Chen, Tiejun]

On 2015/3/24 22:50, Ian Campbell wrote:
> On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:
>> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
>

Fixed.

>                                                                  ^t
>
>> after we want to handle IGD specifically. Now we define a new field of
>> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
>> this means we can benefit this to support other specific devices just
>> by extending gfx_passthru_kind. And then we can cooperate with
>> gfx_passthru to address IGD cases as follows:
>>
>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to true and
>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                                                     true
>
> You had it right in the code.

Fixed.

>
>>                                 and build_info.u.gfx_passthru_kind to IGD
>>

[snip]

>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
>>   devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
>>   above.
>>
>> -=item B<gfx_passthru=BOOLEAN>
>> +=item B<gfx_passthru="STRING">
>
> I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate.

Yeah, it make more sense.

>
>>   Enable graphics device PCI passthrough. This option makes an assigned
>>   PCI graphics card become primary graphics card in the VM. The QEMU
>> @@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
>>   L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>>   for currently supported graphics cards for gfx_passthru.
>>
>> -gfx_passthru is currently only supported with the qemu-xen-traditional
>> -device-model. Upstream qemu-xen device-model currently does not have
>> -support for gfx_passthru.
>> +gfx_passthru is currently supported both with the qemu-xen-traditional
>> +device-model and upstream qemu-xen device-model. Note with the
>> +qemu-xen-traditional device-model this option is just treated as BOOLEAN
>> +actually, but with upstream qemu-xen device-model this option is extended
>> +to pass a specific device name to force work. Currently just 'igd' is
>> +defined to support Intel graphics device.
>
> How about:
>
>          When given as a boolean the B<gfx_passthru> option either
>          disables gfx passthru or enables autodetection.
>
>          When given as a string the B<gfx_passthru> option describes the
>          type of passthru to enable.
>
>          Valid options are:
>
>          =over 4
>
>          =item B<gfx_passthru=0>
>
>          Disables gfx_passthru.
>
>          =item B<gfx_passthru=1>, B<gfx_passthru="default">
>
>          Enables gfx_passthru and autodetects the type of device which is
>          being used.
>
>          =item "igd"
>
>          Enables gfx_passthru of the Intel Graphics Device.
>
>          =back
>
>          Note: When used with the qemu-xen-traditional device model only
>          IGD passthru is supported.
>
> (do check my pod syntax, I'm mostly making it up!)

Please take a look at this,

@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
  above.

-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru=BOOLEAN|"STRING">

  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B<gfx_passthru> option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B<gfx_passthru> option describes the type
+of device to enable. Not this behavior is only supported with upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B<gfx_passthru=0>
+
+Disables graphics device PCI passthrough.
+
+=item B<gfx_passthru=1>, B<gfx_passthru="default">
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but force set the type of device
+with the Intel Graphics Device.
+
+=back

  Note that some graphics adapters (AMD/ATI cards, for example) do not
  necessarily require gfx_passthru option, so you can use the normal Xen

>
> The note at the end makes me thing that perhaps something ought to check
> this constraint in the qemu-xen-traditional case. It might be easiest to

I understand what you mean but that table just includes IGDs existed on 
BDW and HSW. Because in the case of qemu upstream we're just covering 
these platforms, and with our discussion we don't have any plan to add 
those legacy platforms in the future. But qemu-xen-traditional still 
covers those platforms. So I'm afraid its not good to check this with 
that table as well.

> do it in libxl__build_device_model_args_old using the
> libxl__detect_gfx_passthru_kind helper you have added
>
> e.g. where libxl__build_device_model_args_old has:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
> would become something like:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              if (libxl__detect_gfx_passthru_kind(gc, guest_config) !=
>                  LIBXL_GFX_PASSTHRU_KIND_IGD) {
>                  LOG("only IGD gfx_passthru is supported with qemu-xen-traditional");
>                  return NULL;
>              }
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
>
> Or something like that. What do you think?
>
>> 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);
>
> This should be in the previous patch. In fact I think it is and this is
> a redundant second instance.

Yes, we can remove this completely.

Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/24 22:50, Ian Campbell wrote:
> On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:
>> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
>

Fixed.

>                                                                  ^t
>
>> after we want to handle IGD specifically. Now we define a new field of
>> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
>> this means we can benefit this to support other specific devices just
>> by extending gfx_passthru_kind. And then we can cooperate with
>> gfx_passthru to address IGD cases as follows:
>>
>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to true and
>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                                                     true
>
> You had it right in the code.

Fixed.

>
>>                                 and build_info.u.gfx_passthru_kind to IGD
>>

[snip]

>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
>>   devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
>>   above.
>>
>> -=item B<gfx_passthru=BOOLEAN>
>> +=item B<gfx_passthru="STRING">
>
> I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate.

Yeah, it make more sense.

>
>>   Enable graphics device PCI passthrough. This option makes an assigned
>>   PCI graphics card become primary graphics card in the VM. The QEMU
>> @@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
>>   L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>>   for currently supported graphics cards for gfx_passthru.
>>
>> -gfx_passthru is currently only supported with the qemu-xen-traditional
>> -device-model. Upstream qemu-xen device-model currently does not have
>> -support for gfx_passthru.
>> +gfx_passthru is currently supported both with the qemu-xen-traditional
>> +device-model and upstream qemu-xen device-model. Note with the
>> +qemu-xen-traditional device-model this option is just treated as BOOLEAN
>> +actually, but with upstream qemu-xen device-model this option is extended
>> +to pass a specific device name to force work. Currently just 'igd' is
>> +defined to support Intel graphics device.
>
> How about:
>
>          When given as a boolean the B<gfx_passthru> option either
>          disables gfx passthru or enables autodetection.
>
>          When given as a string the B<gfx_passthru> option describes the
>          type of passthru to enable.
>
>          Valid options are:
>
>          =over 4
>
>          =item B<gfx_passthru=0>
>
>          Disables gfx_passthru.
>
>          =item B<gfx_passthru=1>, B<gfx_passthru="default">
>
>          Enables gfx_passthru and autodetects the type of device which is
>          being used.
>
>          =item "igd"
>
>          Enables gfx_passthru of the Intel Graphics Device.
>
>          =back
>
>          Note: When used with the qemu-xen-traditional device model only
>          IGD passthru is supported.
>
> (do check my pod syntax, I'm mostly making it up!)

Please take a look at this,

@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
  above.

-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru=BOOLEAN|"STRING">

  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B<gfx_passthru> option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B<gfx_passthru> option describes the type
+of device to enable. Not this behavior is only supported with upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B<gfx_passthru=0>
+
+Disables graphics device PCI passthrough.
+
+=item B<gfx_passthru=1>, B<gfx_passthru="default">
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but force set the type of device
+with the Intel Graphics Device.
+
+=back

  Note that some graphics adapters (AMD/ATI cards, for example) do not
  necessarily require gfx_passthru option, so you can use the normal Xen

>
> The note at the end makes me thing that perhaps something ought to check
> this constraint in the qemu-xen-traditional case. It might be easiest to

I understand what you mean but that table just includes IGDs existed on 
BDW and HSW. Because in the case of qemu upstream we're just covering 
these platforms, and with our discussion we don't have any plan to add 
those legacy platforms in the future. But qemu-xen-traditional still 
covers those platforms. So I'm afraid its not good to check this with 
that table as well.

> do it in libxl__build_device_model_args_old using the
> libxl__detect_gfx_passthru_kind helper you have added
>
> e.g. where libxl__build_device_model_args_old has:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
> would become something like:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              if (libxl__detect_gfx_passthru_kind(gc, guest_config) !=
>                  LIBXL_GFX_PASSTHRU_KIND_IGD) {
>                  LOG("only IGD gfx_passthru is supported with qemu-xen-traditional");
>                  return NULL;
>              }
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
>
> Or something like that. What do you think?
>
>> 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);
>
> This should be in the previous patch. In fact I think it is and this is
> a redundant second instance.

Yes, we can remove this completely.

Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 18:40, Ian Campbell wrote:
> On Tue, 2015-03-24 at 18:31 +0800, Chen, Tiejun wrote:
>>> NB, the libxl ones are broken and not even compiled right now, you can
>>> ignore them.
>>
>> Looks this is still compiled now.
>
> xc is, xl is not, I am sure of that.

Indeed, you're right :)

>
>>> I don't know what the semantics of flag is, if it is per SBDF then I
>>
>> Yes, this should be a flag specific to a SBDF.
>>
>> You know, I'm working to fix RMRR completely. Based on some discussion
>> about that design ( I assume you may read that thread previously :) ),
>> now we probably need to pass a flag to introduce our policy.
>
> Unless you have a concrete requirement to expose RMRR via the Python
> bindings to libxc (i.e. you know somebody is using them) then I think
> you should not bother.

Actually my problem is that, I need to add a new parameter, 'flag', like 
this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools 
can't be compiled successfully. Or maybe you're suggesting I may isolate 
this file while building tools, right?

>
> Making RMRR work via the (C) interface to libxl used by xl and libvirt
> is sufficient for a new in tree feature.

Yeah.

Thanks
Tiejun

>
> Ian.
>>> suppose if you really wanted to expose this here then you would need to
>>> invent some syntax for doing so.
>>>
>>
>> Definitely.
>>
>> When I finish this I will send you to review technically.
>>
>> Again, really appreciate your clarification to me.
>>
>> Thanks
>> Tiejun
>
>
>

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 18:40, Ian Campbell wrote:
> On Tue, 2015-03-24 at 18:31 +0800, Chen, Tiejun wrote:
>>> NB, the libxl ones are broken and not even compiled right now, you can
>>> ignore them.
>>
>> Looks this is still compiled now.
>
> xc is, xl is not, I am sure of that.

Indeed, you're right :)

>
>>> I don't know what the semantics of flag is, if it is per SBDF then I
>>
>> Yes, this should be a flag specific to a SBDF.
>>
>> You know, I'm working to fix RMRR completely. Based on some discussion
>> about that design ( I assume you may read that thread previously :) ),
>> now we probably need to pass a flag to introduce our policy.
>
> Unless you have a concrete requirement to expose RMRR via the Python
> bindings to libxc (i.e. you know somebody is using them) then I think
> you should not bother.

Actually my problem is that, I need to add a new parameter, 'flag', like 
this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools 
can't be compiled successfully. Or maybe you're suggesting I may isolate 
this file while building tools, right?

>
> Making RMRR work via the (C) interface to libxl used by xl and libvirt
> is sufficient for a new in tree feature.

Yeah.

Thanks
Tiejun

>
> Ian.
>>> suppose if you really wanted to expose this here then you would need to
>>> invent some syntax for doing so.
>>>
>>
>> Definitely.
>>
>> When I finish this I will send you to review technically.
>>
>> Again, really appreciate your clarification to me.
>>
>> Thanks
>> Tiejun
>
>
>

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Wed, 2015-03-25 at 09:18 +0800, Chen, Tiejun wrote:
> Actually my problem is that, I need to add a new parameter, 'flag', like 
> this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools 
> can't be compiled successfully. Or maybe you're suggesting I may isolate 
> this file while building tools, right?

I answered this in my original reply:
         it is acceptable for the new
        parameter to not be plumbed up to the Python bindings until someone
        comes along with a requirement to use it from Python. IOW you can just
        pass whatever the nop value is for the new argument.

The "nop value" is whatever value should be passed to retain the current
behaviour.

Ian.

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Ian Campbell]

On Wed, 2015-03-25 at 09:18 +0800, Chen, Tiejun wrote:
> Actually my problem is that, I need to add a new parameter, 'flag', like 
> this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools 
> can't be compiled successfully. Or maybe you're suggesting I may isolate 
> this file while building tools, right?

I answered this in my original reply:
         it is acceptable for the new
        parameter to not be plumbed up to the Python bindings until someone
        comes along with a requirement to use it from Python. IOW you can just
        pass whatever the nop value is for the new argument.

The "nop value" is whatever value should be passed to retain the current
behaviour.

Ian.

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

[Ian Campbell]

On Wed, 2015-03-25 at 09:10 +0800, Chen, Tiejun wrote:

> +But when given as a string the B<gfx_passthru> option describes the type
> +of device to enable. Not this behavior is only supported with upstream

"Note" and "the upstream..."

> +=item "igd"
> +
> +Enables graphics device PCI passthrough but force set the type of device
> +with the Intel Graphics Device.

"but force set the type" doesn't scan very well. "... forcing the type
of device to Intel Graphics Device" works I think.

> I understand what you mean but that table just includes IGDs existed on 
> BDW and HSW. Because in the case of qemu upstream we're just covering 
> these platforms, and with our discussion we don't have any plan to add 
> those legacy platforms in the future. But qemu-xen-traditional still 
> covers those platforms. So I'm afraid its not good to check this with 
> that table as well.

Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
and whoever adds a new type will have to remember to add a check for
qemu-trad then.

Ian.

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

[Ian Campbell]

On Wed, 2015-03-25 at 09:10 +0800, Chen, Tiejun wrote:

> +But when given as a string the B<gfx_passthru> option describes the type
> +of device to enable. Not this behavior is only supported with upstream

"Note" and "the upstream..."

> +=item "igd"
> +
> +Enables graphics device PCI passthrough but force set the type of device
> +with the Intel Graphics Device.

"but force set the type" doesn't scan very well. "... forcing the type
of device to Intel Graphics Device" works I think.

> I understand what you mean but that table just includes IGDs existed on 
> BDW and HSW. Because in the case of qemu upstream we're just covering 
> these platforms, and with our discussion we don't have any plan to add 
> those legacy platforms in the future. But qemu-xen-traditional still 
> covers those platforms. So I'm afraid its not good to check this with 
> that table as well.

Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
and whoever adds a new type will have to remember to add a check for
qemu-trad then.

Ian.

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/25 18:26, Ian Campbell wrote:
> On Wed, 2015-03-25 at 09:18 +0800, Chen, Tiejun wrote:
>> Actually my problem is that, I need to add a new parameter, 'flag', like
>> this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools
>> can't be compiled successfully. Or maybe you're suggesting I may isolate
>> this file while building tools, right?
>
> I answered this in my original reply:
>           it is acceptable for the new
>          parameter to not be plumbed up to the Python bindings until someone
>          comes along with a requirement to use it from Python. IOW you can just
>          pass whatever the nop value is for the new argument.
>

Yes, I knew this but I'm just getting a little confusion again after 
we're starting to talk if xc.c is compiled...

And I will try to do this.

> The "nop value" is whatever value should be passed to retain the current
> behaviour.

Sure.

Thanks
Tiejun

Re: One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/25 18:26, Ian Campbell wrote:
> On Wed, 2015-03-25 at 09:18 +0800, Chen, Tiejun wrote:
>> Actually my problem is that, I need to add a new parameter, 'flag', like
>> this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools
>> can't be compiled successfully. Or maybe you're suggesting I may isolate
>> this file while building tools, right?
>
> I answered this in my original reply:
>           it is acceptable for the new
>          parameter to not be plumbed up to the Python bindings until someone
>          comes along with a requirement to use it from Python. IOW you can just
>          pass whatever the nop value is for the new argument.
>

Yes, I knew this but I'm just getting a little confusion again after 
we're starting to talk if xc.c is compiled...

And I will try to do this.

> The "nop value" is whatever value should be passed to retain the current
> behaviour.

Sure.

Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/25 18:32, Ian Campbell wrote:
> On Wed, 2015-03-25 at 09:10 +0800, Chen, Tiejun wrote:
>
>> +But when given as a string the B<gfx_passthru> option describes the type
>> +of device to enable. Not this behavior is only supported with upstream
>
> "Note" and "the upstream..."

Fixed.

>
>> +=item "igd"
>> +
>> +Enables graphics device PCI passthrough but force set the type of device
>> +with the Intel Graphics Device.
>
> "but force set the type" doesn't scan very well. "... forcing the type
> of device to Intel Graphics Device" works I think.

Fine to me as well.

>
>> I understand what you mean but that table just includes IGDs existed on
>> BDW and HSW. Because in the case of qemu upstream we're just covering
>> these platforms, and with our discussion we don't have any plan to add
>> those legacy platforms in the future. But qemu-xen-traditional still
>> covers those platforms. So I'm afraid its not good to check this with
>> that table as well.
>
> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> and whoever adds a new type will have to remember to add a check for
> qemu-trad then.
>

When we really have to introduce a new type, this means we probably need 
to change something inside qemu codes. So a new type should just go into 
that table to support qemu upstream since now we shouldn't refactor 
anything in qemu-xen-traditional, right?

Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/25 18:32, Ian Campbell wrote:
> On Wed, 2015-03-25 at 09:10 +0800, Chen, Tiejun wrote:
>
>> +But when given as a string the B<gfx_passthru> option describes the type
>> +of device to enable. Not this behavior is only supported with upstream
>
> "Note" and "the upstream..."

Fixed.

>
>> +=item "igd"
>> +
>> +Enables graphics device PCI passthrough but force set the type of device
>> +with the Intel Graphics Device.
>
> "but force set the type" doesn't scan very well. "... forcing the type
> of device to Intel Graphics Device" works I think.

Fine to me as well.

>
>> I understand what you mean but that table just includes IGDs existed on
>> BDW and HSW. Because in the case of qemu upstream we're just covering
>> these platforms, and with our discussion we don't have any plan to add
>> those legacy platforms in the future. But qemu-xen-traditional still
>> covers those platforms. So I'm afraid its not good to check this with
>> that table as well.
>
> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> and whoever adds a new type will have to remember to add a check for
> qemu-trad then.
>

When we really have to introduce a new type, this means we probably need 
to change something inside qemu codes. So a new type should just go into 
that table to support qemu upstream since now we shouldn't refactor 
anything in qemu-xen-traditional, right?

Thanks
Tiejun

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

[Ian Campbell]

On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
> > Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> > and whoever adds a new type will have to remember to add a check for
> > qemu-trad then.
> >
> 
> When we really have to introduce a new type, this means we probably need 
> to change something inside qemu codes. So a new type should just go into 
> that table to support qemu upstream since now we shouldn't refactor 
> anything in qemu-xen-traditional, right?

We'd want to error out on attempts to use qemu-xen-trad with non-IGD
passthru.

Ian.

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

[Ian Campbell]

On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
> > Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> > and whoever adds a new type will have to remember to add a check for
> > qemu-trad then.
> >
> 
> When we really have to introduce a new type, this means we probably need 
> to change something inside qemu codes. So a new type should just go into 
> that table to support qemu upstream since now we shouldn't refactor 
> anything in qemu-xen-traditional, right?

We'd want to error out on attempts to use qemu-xen-trad with non-IGD
passthru.

Ian.

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

[Chen, Tiejun]

On 2015/3/26 18:06, Ian Campbell wrote:
> On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
>>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
>>> and whoever adds a new type will have to remember to add a check for
>>> qemu-trad then.
>>>
>>
>> When we really have to introduce a new type, this means we probably need
>> to change something inside qemu codes. So a new type should just go into
>> that table to support qemu upstream since now we shouldn't refactor
>> anything in qemu-xen-traditional, right?
>
> We'd want to error out on attempts to use qemu-xen-trad with non-IGD
> passthru.
>

On qemu-xen-traditional side, we always recognize this as BOOLEAN,

         if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { 

             flexarray_append(dm_args, "-gfx_passthru"); 

         }

Additionally, this is also clarified explicitly in manpage, and 
especially we don't change this behavior now, so I'm just wondering why 
we should do this :)

Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/26 18:06, Ian Campbell wrote:
> On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
>>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
>>> and whoever adds a new type will have to remember to add a check for
>>> qemu-trad then.
>>>
>>
>> When we really have to introduce a new type, this means we probably need
>> to change something inside qemu codes. So a new type should just go into
>> that table to support qemu upstream since now we shouldn't refactor
>> anything in qemu-xen-traditional, right?
>
> We'd want to error out on attempts to use qemu-xen-trad with non-IGD
> passthru.
>

On qemu-xen-traditional side, we always recognize this as BOOLEAN,

         if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { 

             flexarray_append(dm_args, "-gfx_passthru"); 

         }

Additionally, this is also clarified explicitly in manpage, and 
especially we don't change this behavior now, so I'm just wondering why 
we should do this :)

Thanks
Tiejun

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

[Ian Campbell]

On Fri, 2015-03-27 at 09:29 +0800, Chen, Tiejun wrote:
> On 2015/3/26 18:06, Ian Campbell wrote:
> > On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
> >>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> >>> and whoever adds a new type will have to remember to add a check for
> >>> qemu-trad then.
> >>>
> >>
> >> When we really have to introduce a new type, this means we probably need
> >> to change something inside qemu codes. So a new type should just go into
> >> that table to support qemu upstream since now we shouldn't refactor
> >> anything in qemu-xen-traditional, right?
> >
> > We'd want to error out on attempts to use qemu-xen-trad with non-IGD
> > passthru.
> >
> 
> On qemu-xen-traditional side, we always recognize this as BOOLEAN,
> 
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { 
> 
>              flexarray_append(dm_args, "-gfx_passthru"); 
> 
>          }
> 
> Additionally, this is also clarified explicitly in manpage, and 
> especially we don't change this behavior now, so I'm just wondering why 
> we should do this :)

If someone does gfx_passthru = "foobar" and device_model_version =
"qemu-xen-traditional" in their xl config then it would be rather mean
of us to silently enable IGD passthru for them instead. When this occurs
libxl should notice and fail.

IGD is currently the only option, so this code would be needed when
someone adds "foobar" support.

Ian.

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

[Ian Campbell]

On Fri, 2015-03-27 at 09:29 +0800, Chen, Tiejun wrote:
> On 2015/3/26 18:06, Ian Campbell wrote:
> > On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
> >>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> >>> and whoever adds a new type will have to remember to add a check for
> >>> qemu-trad then.
> >>>
> >>
> >> When we really have to introduce a new type, this means we probably need
> >> to change something inside qemu codes. So a new type should just go into
> >> that table to support qemu upstream since now we shouldn't refactor
> >> anything in qemu-xen-traditional, right?
> >
> > We'd want to error out on attempts to use qemu-xen-trad with non-IGD
> > passthru.
> >
> 
> On qemu-xen-traditional side, we always recognize this as BOOLEAN,
> 
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { 
> 
>              flexarray_append(dm_args, "-gfx_passthru"); 
> 
>          }
> 
> Additionally, this is also clarified explicitly in manpage, and 
> especially we don't change this behavior now, so I'm just wondering why 
> we should do this :)

If someone does gfx_passthru = "foobar" and device_model_version =
"qemu-xen-traditional" in their xl config then it would be rather mean
of us to silently enable IGD passthru for them instead. When this occurs
libxl should notice and fail.

IGD is currently the only option, so this code would be needed when
someone adds "foobar" support.

Ian.

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

[Chen, Tiejun]

On 2015/3/27 17:54, Ian Campbell wrote:
> On Fri, 2015-03-27 at 09:29 +0800, Chen, Tiejun wrote:
>> On 2015/3/26 18:06, Ian Campbell wrote:
>>> On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
>>>>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
>>>>> and whoever adds a new type will have to remember to add a check for
>>>>> qemu-trad then.
>>>>>
>>>>
>>>> When we really have to introduce a new type, this means we probably need
>>>> to change something inside qemu codes. So a new type should just go into
>>>> that table to support qemu upstream since now we shouldn't refactor
>>>> anything in qemu-xen-traditional, right?
>>>
>>> We'd want to error out on attempts to use qemu-xen-trad with non-IGD
>>> passthru.
>>>
>>
>> On qemu-xen-traditional side, we always recognize this as BOOLEAN,
>>
>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>
>>               flexarray_append(dm_args, "-gfx_passthru");
>>
>>           }
>>
>> Additionally, this is also clarified explicitly in manpage, and
>> especially we don't change this behavior now, so I'm just wondering why
>> we should do this :)
>
> If someone does gfx_passthru = "foobar" and device_model_version =
> "qemu-xen-traditional" in their xl config then it would be rather mean
> of us to silently enable IGD passthru for them instead. When this occurs
> libxl should notice and fail.
>
> IGD is currently the only option, so this code would be needed when
> someone adds "foobar" support.
>

Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do 
it now,

@@ -326,6 +326,10 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,
          }
          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
              flexarray_append(dm_args, "-gfx_passthru");
+            if (b_info->u.hvm.gfx_passthru_kind >
+                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
+                LOG(ERROR, "unsupported device type for 
\"gfx_passthru\".\n");
+                return NULL;
          }
      } else {
          if (!sdl && !vnc)


Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/3/27 17:54, Ian Campbell wrote:
> On Fri, 2015-03-27 at 09:29 +0800, Chen, Tiejun wrote:
>> On 2015/3/26 18:06, Ian Campbell wrote:
>>> On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
>>>>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
>>>>> and whoever adds a new type will have to remember to add a check for
>>>>> qemu-trad then.
>>>>>
>>>>
>>>> When we really have to introduce a new type, this means we probably need
>>>> to change something inside qemu codes. So a new type should just go into
>>>> that table to support qemu upstream since now we shouldn't refactor
>>>> anything in qemu-xen-traditional, right?
>>>
>>> We'd want to error out on attempts to use qemu-xen-trad with non-IGD
>>> passthru.
>>>
>>
>> On qemu-xen-traditional side, we always recognize this as BOOLEAN,
>>
>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>
>>               flexarray_append(dm_args, "-gfx_passthru");
>>
>>           }
>>
>> Additionally, this is also clarified explicitly in manpage, and
>> especially we don't change this behavior now, so I'm just wondering why
>> we should do this :)
>
> If someone does gfx_passthru = "foobar" and device_model_version =
> "qemu-xen-traditional" in their xl config then it would be rather mean
> of us to silently enable IGD passthru for them instead. When this occurs
> libxl should notice and fail.
>
> IGD is currently the only option, so this code would be needed when
> someone adds "foobar" support.
>

Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do 
it now,

@@ -326,6 +326,10 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,
          }
          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
              flexarray_append(dm_args, "-gfx_passthru");
+            if (b_info->u.hvm.gfx_passthru_kind >
+                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
+                LOG(ERROR, "unsupported device type for 
\"gfx_passthru\".\n");
+                return NULL;
          }
      } else {
          if (!sdl && !vnc)


Thanks
Tiejun

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

[Ian Campbell]

On Mon, 2015-03-30 at 09:28 +0800, Chen, Tiejun wrote:
> Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do 
> it now,
> 
> @@ -326,6 +326,10 @@ static char ** 
> libxl__build_device_model_args_old(libxl__gc *gc,
>           }
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>               flexarray_append(dm_args, "-gfx_passthru");
> +            if (b_info->u.hvm.gfx_passthru_kind >
> +                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
> +                LOG(ERROR, "unsupported device type for 
> \"gfx_passthru\".\n");
> +                return NULL;

I'd rather not encode any ordering constraints if we don't have to. I
think this is preferable:

          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
               switch (b_info->u.hvm.gfx_passthru_kind) {
               case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
               case LIBXL_GFX_PASSTHRU_KIND_IGD:
                   flexarray_append(dm_args, "-gfx_passthru");
                   break;
               default:
                   LOG(ERROR, "unsupported gfx_passthru_kind.\n");
                   return NULL;
               }
         }

(notice that the error message above doesn't refer to the xl specific
option naming).

Ian.

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

[Ian Campbell]

On Mon, 2015-03-30 at 09:28 +0800, Chen, Tiejun wrote:
> Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do 
> it now,
> 
> @@ -326,6 +326,10 @@ static char ** 
> libxl__build_device_model_args_old(libxl__gc *gc,
>           }
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>               flexarray_append(dm_args, "-gfx_passthru");
> +            if (b_info->u.hvm.gfx_passthru_kind >
> +                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
> +                LOG(ERROR, "unsupported device type for 
> \"gfx_passthru\".\n");
> +                return NULL;

I'd rather not encode any ordering constraints if we don't have to. I
think this is preferable:

          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
               switch (b_info->u.hvm.gfx_passthru_kind) {
               case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
               case LIBXL_GFX_PASSTHRU_KIND_IGD:
                   flexarray_append(dm_args, "-gfx_passthru");
                   break;
               default:
                   LOG(ERROR, "unsupported gfx_passthru_kind.\n");
                   return NULL;
               }
         }

(notice that the error message above doesn't refer to the xl specific
option naming).

Ian.

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

[Chen, Tiejun]

On 2015/3/30 17:19, Ian Campbell wrote:
> On Mon, 2015-03-30 at 09:28 +0800, Chen, Tiejun wrote:
>> Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do
>> it now,
>>
>> @@ -326,6 +326,10 @@ static char **
>> libxl__build_device_model_args_old(libxl__gc *gc,
>>            }
>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>                flexarray_append(dm_args, "-gfx_passthru");
>> +            if (b_info->u.hvm.gfx_passthru_kind >
>> +                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
>> +                LOG(ERROR, "unsupported device type for
>> \"gfx_passthru\".\n");
>> +                return NULL;
>
> I'd rather not encode any ordering constraints if we don't have to. I
> think this is preferable:
>
>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>                 switch (b_info->u.hvm.gfx_passthru_kind) {
>                 case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                 case LIBXL_GFX_PASSTHRU_KIND_IGD:
>                     flexarray_append(dm_args, "-gfx_passthru");
>                     break;
>                 default:
>                     LOG(ERROR, "unsupported gfx_passthru_kind.\n");
>                     return NULL;
>                 }
>           }
>
> (notice that the error message above doesn't refer to the xl specific
> option naming).
>

Sorry for this delay response.

This looks reasonable and I regenerate this patch based on this comment:

  libxl: introduce gfx_passthru_kind

Although we already have 'gfx_passthru' in b_info, this doesn't 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 true
                                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 now "gfx_passthru" is supported both with the qemu-xen-traditional
device-model and upstream qemu-xen device-model. But when given as a
string this option describes the type of device to enable. Note this
behavior is only supported with the upstream qemu-xen device-model.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
  docs/man/xl.cfg.pod.5       | 34 +++++++++++++++++++++++++++++----
  tools/libxl/libxl.h         |  6 ++++++
  tools/libxl/libxl_dm.c      | 46 
+++++++++++++++++++++++++++++++++++++++++----
  tools/libxl/libxl_types.idl |  6 ++++++
  tools/libxl/xl_cmdimpl.c    | 14 ++++++++++++--
  5 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 408653f..dfde92d 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
  above.

-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru=BOOLEAN|"STRING">

  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B<gfx_passthru> option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B<gfx_passthru> option describes the type
+of device to enable. Note this behavior is only supported with the upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B<gfx_passthru=0>
+
+Disables graphics device PCI passthrough.
+
+=item B<gfx_passthru=1>, B<gfx_passthru="default">
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but forcing the type
+of device to Intel Graphics Device.
+
+=back

  Note that some graphics adapters (AMD/ATI cards, for example) do not
  necessarily require gfx_passthru option, so you can use the normal Xen
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..1144c5e 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 a8b08f2..4fd6310 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -325,7 +325,15 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,
              flexarray_vappend(dm_args, "-net", "none", NULL);
          }
          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-            flexarray_append(dm_args, "-gfx_passthru");
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                flexarray_append(dm_args, "-gfx_passthru");
+                break;
+            default:
+                LOG(ERROR, "unsupported gfx_passthru_kind.\n");
+                return NULL;
+            }
          }
      } else {
          if (!sdl && !vnc)
@@ -416,6 +424,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,
@@ -727,9 +751,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");
@@ -774,6 +795,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_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 04faf98..b78b4ec 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1956,8 +1956,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: [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/30 17:19, Ian Campbell wrote:
> On Mon, 2015-03-30 at 09:28 +0800, Chen, Tiejun wrote:
>> Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do
>> it now,
>>
>> @@ -326,6 +326,10 @@ static char **
>> libxl__build_device_model_args_old(libxl__gc *gc,
>>            }
>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>                flexarray_append(dm_args, "-gfx_passthru");
>> +            if (b_info->u.hvm.gfx_passthru_kind >
>> +                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
>> +                LOG(ERROR, "unsupported device type for
>> \"gfx_passthru\".\n");
>> +                return NULL;
>
> I'd rather not encode any ordering constraints if we don't have to. I
> think this is preferable:
>
>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>                 switch (b_info->u.hvm.gfx_passthru_kind) {
>                 case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                 case LIBXL_GFX_PASSTHRU_KIND_IGD:
>                     flexarray_append(dm_args, "-gfx_passthru");
>                     break;
>                 default:
>                     LOG(ERROR, "unsupported gfx_passthru_kind.\n");
>                     return NULL;
>                 }
>           }
>
> (notice that the error message above doesn't refer to the xl specific
> option naming).
>

Sorry for this delay response.

This looks reasonable and I regenerate this patch based on this comment:

  libxl: introduce gfx_passthru_kind

Although we already have 'gfx_passthru' in b_info, this doesn't 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 true
                                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 now "gfx_passthru" is supported both with the qemu-xen-traditional
device-model and upstream qemu-xen device-model. But when given as a
string this option describes the type of device to enable. Note this
behavior is only supported with the upstream qemu-xen device-model.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
  docs/man/xl.cfg.pod.5       | 34 +++++++++++++++++++++++++++++----
  tools/libxl/libxl.h         |  6 ++++++
  tools/libxl/libxl_dm.c      | 46 
+++++++++++++++++++++++++++++++++++++++++----
  tools/libxl/libxl_types.idl |  6 ++++++
  tools/libxl/xl_cmdimpl.c    | 14 ++++++++++++--
  5 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 408653f..dfde92d 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
  above.

-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru=BOOLEAN|"STRING">

  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B<gfx_passthru> option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B<gfx_passthru> option describes the type
+of device to enable. Note this behavior is only supported with the upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B<gfx_passthru=0>
+
+Disables graphics device PCI passthrough.
+
+=item B<gfx_passthru=1>, B<gfx_passthru="default">
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but forcing the type
+of device to Intel Graphics Device.
+
+=back

  Note that some graphics adapters (AMD/ATI cards, for example) do not
  necessarily require gfx_passthru option, so you can use the normal Xen
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..1144c5e 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 a8b08f2..4fd6310 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -325,7 +325,15 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,
              flexarray_vappend(dm_args, "-net", "none", NULL);
          }
          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-            flexarray_append(dm_args, "-gfx_passthru");
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                flexarray_append(dm_args, "-gfx_passthru");
+                break;
+            default:
+                LOG(ERROR, "unsupported gfx_passthru_kind.\n");
+                return NULL;
+            }
          }
      } else {
          if (!sdl && !vnc)
@@ -416,6 +424,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,
@@ -727,9 +751,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");
@@ -774,6 +795,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_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 04faf98..b78b4ec 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1956,8 +1956,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] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Wed, 2015-04-01 at 09:05 +0800, Chen, Tiejun wrote:
> @@ -699,9 +699,35 @@ working graphics passthrough. See the 
> XenVGAPassthroughTestedAdapters
>   L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>   for currently supported graphics cards for gfx_passthru.
> 
> -gfx_passthru is currently only supported with the qemu-xen-traditional
> -device-model. Upstream qemu-xen device-model currently does not have
> -support for gfx_passthru.
> +gfx_passthru is currently supported both with the qemu-xen-traditional
> +device-model and upstream qemu-xen device-model.
> +
> +When given as a boolean the B<gfx_passthru> option either disables gfx
> +passthru or enables autodetection.
> +
> +But when given as a string the B<gfx_passthru> option describes the type
> +of device to enable. Note this behavior is only supported with the upstream
> +qemu-xen device-model.

Perhaps add "With qemu-xen-traditional IGD is always assumed and other
options than autodetect or explicit IGD will result in an error"?

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a8b08f2..4fd6310 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -325,7 +325,15 @@ static char ** 
> libxl__build_device_model_args_old(libxl__gc *gc,
>               flexarray_vappend(dm_args, "-net", "none", NULL);
>           }
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> -            flexarray_append(dm_args, "-gfx_passthru");
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                flexarray_append(dm_args, "-gfx_passthru");
> +                break;
> +            default:
> +                LOG(ERROR, "unsupported gfx_passthru_kind.\n");

Sorry, LOG should not get a \n like my example had, my fault.

With that if you resend the series with git send-email (so it doesn't
get whitespace mangled) I think we are good to go!

Ian.

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

[Ian Campbell]

On Wed, 2015-04-01 at 09:05 +0800, Chen, Tiejun wrote:
> @@ -699,9 +699,35 @@ working graphics passthrough. See the 
> XenVGAPassthroughTestedAdapters
>   L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>   for currently supported graphics cards for gfx_passthru.
> 
> -gfx_passthru is currently only supported with the qemu-xen-traditional
> -device-model. Upstream qemu-xen device-model currently does not have
> -support for gfx_passthru.
> +gfx_passthru is currently supported both with the qemu-xen-traditional
> +device-model and upstream qemu-xen device-model.
> +
> +When given as a boolean the B<gfx_passthru> option either disables gfx
> +passthru or enables autodetection.
> +
> +But when given as a string the B<gfx_passthru> option describes the type
> +of device to enable. Note this behavior is only supported with the upstream
> +qemu-xen device-model.

Perhaps add "With qemu-xen-traditional IGD is always assumed and other
options than autodetect or explicit IGD will result in an error"?

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a8b08f2..4fd6310 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -325,7 +325,15 @@ static char ** 
> libxl__build_device_model_args_old(libxl__gc *gc,
>               flexarray_vappend(dm_args, "-net", "none", NULL);
>           }
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> -            flexarray_append(dm_args, "-gfx_passthru");
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                flexarray_append(dm_args, "-gfx_passthru");
> +                break;
> +            default:
> +                LOG(ERROR, "unsupported gfx_passthru_kind.\n");

Sorry, LOG should not get a \n like my example had, my fault.

With that if you resend the series with git send-email (so it doesn't
get whitespace mangled) I think we are good to go!

Ian.

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

[Chen, Tiejun]

> Perhaps add "With qemu-xen-traditional IGD is always assumed and other
> options than autodetect or explicit IGD will result in an error"?

Will do.

>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index a8b08f2..4fd6310 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -325,7 +325,15 @@ static char **
>> libxl__build_device_model_args_old(libxl__gc *gc,
>>                flexarray_vappend(dm_args, "-net", "none", NULL);
>>            }
>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> -            flexarray_append(dm_args, "-gfx_passthru");
>> +            switch (b_info->u.hvm.gfx_passthru_kind) {
>> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
>> +                flexarray_append(dm_args, "-gfx_passthru");
>> +                break;
>> +            default:
>> +                LOG(ERROR, "unsupported gfx_passthru_kind.\n");
>
> Sorry, LOG should not get a \n like my example had, my fault.

Actually myself really should double check this.

>
> With that if you resend the series with git send-email (so it doesn't
> get whitespace mangled) I think we are good to go!
>

Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
complete to review all associated qemu patch set. Although that don't 
bring any change to our two patches on Xen side, I think we'd better 
merge these patches until qemu patches are really applied into qemu 
tree. So I will send this series again until we can really consume this 
with qemu upstream, right?

BTW, I really appreciate your all comments in this thread.

Thanks
Tiejun

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

[Chen, Tiejun]

> Perhaps add "With qemu-xen-traditional IGD is always assumed and other
> options than autodetect or explicit IGD will result in an error"?

Will do.

>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index a8b08f2..4fd6310 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -325,7 +325,15 @@ static char **
>> libxl__build_device_model_args_old(libxl__gc *gc,
>>                flexarray_vappend(dm_args, "-net", "none", NULL);
>>            }
>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> -            flexarray_append(dm_args, "-gfx_passthru");
>> +            switch (b_info->u.hvm.gfx_passthru_kind) {
>> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
>> +                flexarray_append(dm_args, "-gfx_passthru");
>> +                break;
>> +            default:
>> +                LOG(ERROR, "unsupported gfx_passthru_kind.\n");
>
> Sorry, LOG should not get a \n like my example had, my fault.

Actually myself really should double check this.

>
> With that if you resend the series with git send-email (so it doesn't
> get whitespace mangled) I think we are good to go!
>

Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
complete to review all associated qemu patch set. Although that don't 
bring any change to our two patches on Xen side, I think we'd better 
merge these patches until qemu patches are really applied into qemu 
tree. So I will send this series again until we can really consume this 
with qemu upstream, right?

BTW, I really appreciate your all comments in this thread.

Thanks
Tiejun

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

[Ian Campbell]

On Wed, 2015-04-01 at 17:18 +0800, Chen, Tiejun wrote:
> Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
> complete to review all associated qemu patch set. Although that don't 
> bring any change to our two patches on Xen side, I think we'd better 
> merge these patches until qemu patches are really applied into qemu 
> tree. So I will send this series again until we can really consume this 
> with qemu upstream, right?

IOW I should put this to one side until you tell me the qemu side is in
place (by resending the series)? Fine by me, thanks.

> BTW, I really appreciate your all comments in this thread.

No problem, thanks for sticking with my nit picking.

Ian.

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

[Ian Campbell]

On Wed, 2015-04-01 at 17:18 +0800, Chen, Tiejun wrote:
> Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
> complete to review all associated qemu patch set. Although that don't 
> bring any change to our two patches on Xen side, I think we'd better 
> merge these patches until qemu patches are really applied into qemu 
> tree. So I will send this series again until we can really consume this 
> with qemu upstream, right?

IOW I should put this to one side until you tell me the qemu side is in
place (by resending the series)? Fine by me, thanks.

> BTW, I really appreciate your all comments in this thread.

No problem, thanks for sticking with my nit picking.

Ian.