[PATCH v10] tools/libxl: Add qxl vga interface support for upstream qemu

[PATCH v10] tools/libxl: Add qxl vga interface support for upstream qemu

[fantonifabio]

From: Fabio Fantoni <fabio.fantoni@heliman.it>

Usage:
  vga="qxl"

Changes from v8:
- vga=qxl instead of qxl=1 to use it.
- Show an error and exit if vga="qxl" without qemu upstream.
- Other small improvements.

Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it>
Signed-off-by: Zhou Peng <zpengxen@gmail.com>
---
 docs/man/xl.cfg.pod.5       |   10 +++++++++-
 tools/libxl/libxl_create.c  |   16 ++++++++++++++++
 tools/libxl/libxl_dm.c      |   13 +++++++++++++
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |    3 +++
 5 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 9c5def4..25523c9 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1003,6 +1003,9 @@ the amount of video ram is fixed at 4MB which is sufficient
 for 1024x768 at 32 bpp and videoram option is currently working
 only when using the upstream qemu-xen device-model.
 
+For B<qxl> vga, the default is both default and minimal 128MB.
+If B<videoram> is set less than 128MB, an error will be triggered.
+
 =item B<stdvga=BOOLEAN>
 
 Select a standard VGA card with VBE (VESA BIOS Extensions) as the
@@ -1014,9 +1017,14 @@ This option is deprecated, use vga="stdvga" instead.
 
 =item B<vga="STRING">
 
-Selects the emulated video card (stdvga|cirrus).
+Selects the emulated video card (stdvga|cirrus|qxl).
 The default is cirrus.
 
+In general, QXL should work with the Spice remote display protocol
+for acceleration, and QXL driver is necessary in guest in this case.
+QXL can also work with the VNC protocol, but it will be like a standard
+VGA without acceleration.
+
 =item B<vnc=BOOLEAN>
 
 Allow access to the display via the VNC protocol.  This enables the
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fa81f88..eea885c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -198,6 +198,22 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->shadow_memkb = 0;
 
+        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_QXL) {
+            if (b_info->device_model_version ==
+               LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+                if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) {
+                    b_info->video_memkb = (128 * 1024);
+                }else if (b_info->video_memkb < (128 * 1024)) {
+                    LOG(ERROR,
+                    "128 Mib videoram is the minimum for qxl default");
+                    return ERROR_INVAL;
+                }
+            } else {
+                LOG(ERROR,"qemu upstream required for qxl vga");
+                return ERROR_INVAL;
+            }
+        }
+
         if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_STD &&
             b_info->device_model_version ==
             LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a2c99bd..59fc86a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -181,6 +181,8 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
             break;
         case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
             break;
+        case LIBXL_VGA_INTERFACE_TYPE_QXL:
+            break;
         }
 
         if (b_info->u.hvm.boot) {
@@ -440,6 +442,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                 NULL);
             }
             break;
+        case LIBXL_VGA_INTERFACE_TYPE_QXL:
+            /*QXL have 2 ram regions, ram and vram*/
+            flexarray_vappend(dm_args, "-vga", "qxl", NULL);
+            if (b_info->video_memkb) {
+                flexarray_vappend(dm_args, "-global",
+                libxl__sprintf(gc, "qxl-vga.vram_size_mb=%lu",
+                (b_info->video_memkb/2/1024)), "-global",
+                libxl__sprintf(gc, "qxl-vga.ram_size_mb=%lu",
+                (b_info->video_memkb/2/1024)), NULL);
+            }
+            break;
         }
 
         if (b_info->u.hvm.boot) {
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 89a8030..5b080ed 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -130,6 +130,7 @@ libxl_shutdown_reason = Enumeration("shutdown_reason", [
 libxl_vga_interface_type = Enumeration("vga_interface_type", [
     (1, "CIRRUS"),
     (2, "STD"),
+    (3, "QXL"),
     ], init_val = 0)
 
 #
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 003b129..f36a293 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1481,6 +1481,9 @@ skip_vfb:
             } else if (!strcmp(buf, "cirrus")) {
                 b_info->u.hvm.vga.kind
                 = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+            }else if (!strcmp(buf, "qxl")) {
+                b_info->u.hvm.vga.kind
+                = LIBXL_VGA_INTERFACE_TYPE_QXL;
             } else {
                 fprintf(stderr,
                 "Unknown vga \"%s\" specified\n", buf);
-- 
1.7.9.5

Re: [PATCH v10] tools/libxl: Add qxl vga interface support for upstream qemu

[Ian Jackson]

fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH v10] tools/libxl: Add qxl vga interface support for upstream qemu"):
> Changes from v8:
> - vga=qxl instead of qxl=1 to use it.
> - Show an error and exit if vga="qxl" without qemu upstream.
> - Other small improvements.

This looks plausible to me.  I'd like an ack from Stefano.

Also this patch has some more of the formatting problems I mentioned
before and which Ian C has expanded on:

> +        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_QXL) {
> +            if (b_info->device_model_version ==
> +               LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +                if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) {
> +                    b_info->video_memkb = (128 * 1024);
> +                }else if (b_info->video_memkb < (128 * 1024)) {

                    ^ extra space neeeded

Also it would be better for the 128MB figure to occur only once in the
source code, perhaps with a const or a #define.

> +                    LOG(ERROR,
> +                    "128 Mib videoram is the minimum for qxl default");

                           ^ should be indented to here; you can
                             use "a" "b" style string concatenation if
                             that's convenient to split the string
                             across multiple lines

> +        case LIBXL_VGA_INTERFACE_TYPE_QXL:
> +            /*QXL have 2 ram regions, ram and vram*/
                 ^ spaces here and                   ^ here
> +            flexarray_vappend(dm_args, "-vga", "qxl", NULL);
> +            if (b_info->video_memkb) {
> +                flexarray_vappend(dm_args, "-global",
> +                libxl__sprintf(gc, "qxl-vga.vram_size_mb=%lu",
> +                (b_info->video_memkb/2/1024)), "-global",
> +                libxl__sprintf(gc, "qxl-vga.ram_size_mb=%lu",
> +                (b_info->video_memkb/2/1024)), NULL);

This needs indentation to the right level, in each case to where the
relevant parens open.  Also you will probably find GCSPRINTF a helpful
macro which will make this a lot shorter.

> +            }else if (!strcmp(buf, "qxl")) {

                ^ space

> +                b_info->u.hvm.vga.kind
> +                = LIBXL_VGA_INTERFACE_TYPE_QXL;

                       ^ indentation needs to be to here

I appreciate that these will seem like nits, and that there are
existing places where the indentation and formatting is wrong, but
they are things that the reader's eye trips over and we are trying to
fix them, not introduce more of them.

Thanks,
Ian.