Re: [PATCH] libxl: add basic spice support for pv domUs

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Dec 01, 2015 at 05:04:35PM +0100, Fabio Fantoni wrote:
> This patch adds basic spice support for pv domUs.
> The qemu parameters are the same as the hvm ones and they works.
> Therefore xl cfg parameters are the same as the hvm ones except that
> features not supported yet by pv domUs (vdagent and usbredirection)
> are kept disabled by default.
> It also enables vfb and vkb required to have basic spice working.
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> 
> ---
> 
> Notes:
> - The vfb part is only a draft and needs to be improved.
> - Patch is tested and working, except for the pointer not
> visible in some cases with pv domUs but always working.
> - I not use the api, test the u.hvm.spice retro-compatibility with
> api is needed.
> 
> Any feedback is appreciated.
> 
> Changes in v8:
> - refresh all for xen 4.7
> 
> Changes in v7:
> - refresh xl_sxp.c
> 
> Changes in v6:
> - refresh libxl_create.c
> 
> Changes in v5:
> - libxl_create.c: * don't copy u.hvm.spice in the newer if
> the newer is already used
> * set default for all spice bool options in any case
> * spice features not supported in pv will be disabled and
> will show a warning about them if was setted enabled
> - xl_cmdimpl.c: parse all spice options out of hvm part
> - libxl_dm.c: changed some forgotten u.hvm.spice to spice
> 
> Changes in v4:
> - added libxl.h changes
> - libxl_create.c: added older u.hvm.spice compatibility
> copying it in newer one
> 
> Changes in v3:
> - xl.cfg.pod.5: moved spice out of hvm section and specified
> the features for now hvm only.
> - libxl_types.idl: added spice struct out of keyedunion hvm only.
> - use new generic spice struct instead of hvm only ones.
> 
> Changes in v2:
> - xl_cmdimpl.c: always set vnc and sdl toplevel parameters in &vfb
> with vnc or spice enabled on pv domUs otherwise in some cases it
> would fail with error for one bool default value missing.
> - libxl_dm.c: do not add -nographic if spice is enabled, even though
> -nographic seems buggy in upstream qemu.
> ---
>  docs/man/xl.cfg.pod.5       | 155 ++++++++++++++++++++++----------------------
>  tools/libxl/libxl.h         |  13 ++++
>  tools/libxl/libxl_create.c  |  49 +++++++++-----
>  tools/libxl/libxl_dm.c      |  39 ++++++-----
>  tools/libxl/libxl_types.idl |   3 +-
>  tools/libxl/xl_cmdimpl.c    |  51 ++++++++-------
>  tools/libxl/xl_sxp.c        |  12 ++--
>  7 files changed, 179 insertions(+), 143 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 3b695bd..04a96ba 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1607,82 +1607,6 @@ it.
>  
>  =back
>  
> -=head3 Spice Graphics Support
> -
> -The following options control the features of SPICE.
> -
> -=over 4
> -
> -=item B<spice=BOOLEAN>
> -

To be honest I'm a bit confused by this large hunk. Did you notice some
sort of misplacement of this section? Or is it your editor is using
different wrapping setting?

>  =head2 Keymaps
>  
>  The keymaps available are defined by the device-model which you are
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6b73848..a5cbcfc 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -510,6 +510,19 @@ typedef struct libxl__ctx libxl_ctx;
>  #define LIBXL_HAVE_BUILDINFO_USBVERSION 1
>  
>  /*
> + * LIBXL_HAVE_BUILDINFO_SPICE
> + *
> + * If this is defined, then the libxl_domain_build_info structure will
> + * contain spice, a libxl_spice_info struct instead of older hvm.spice one
> + * which is now deprecated.
> + *

This reads like hvm.spice is removed but I think hvm.spice still exists.

> + * If it is set, callers may use spice to specify the spice values.
> + *

May use which one? hvm.spice or the new spice (libxl_spice_info).

> + * If this is not defined, the spice struct does not exist.
> + */
> +#define LIBXL_HAVE_BUILDINFO_SPICE 1
> +
> +/*
>   * LIBXL_HAVE_DEVICE_BACKEND_DOMNAME
>   *
>   * If this is defined, libxl_device_* structures containing a backend_domid
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 8770486..a1853dc 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -195,6 +195,19 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      if (!b_info->event_channels)
>          b_info->event_channels = 1023;
>  
> +    /* If older u.hvm.spice is enabled then propagate it to the top level */
> +    libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);

This is wrong -- don't set u.hvm without checking the guest is actually
hvm guest.

> +    libxl_defbool_setdefault(&b_info->spice.enable, false);
> +    if (!libxl_defbool_val(b_info->spice.enable) &&
> +        libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> +            b_info->spice = b_info->u.hvm.spice;

Please use libxl_spice_info_copy (an autogenerated function) to do the
copying.  Doing a shallow copy like this is prone to error -- consider
in the future your structure contains pointers to allocated memory,
doing shallow copy will cause double free.

> +    }
> +
> +    libxl_defbool_setdefault(&b_info->spice.disable_ticketing, false);
> +    libxl_defbool_setdefault(&b_info->spice.agent_mouse, true);
> +    libxl_defbool_setdefault(&b_info->spice.vdagent, false);
> +    libxl_defbool_setdefault(&b_info->spice.clipboard_sharing, false);
> +
>      switch (b_info->type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>          if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -291,18 +304,17 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
>  
> -        libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
> -        if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
> -            (b_info->u.hvm.spice.usbredirection > 0) ){
> -            b_info->u.hvm.spice.usbredirection = 0;
> -            LOG(WARN, "spice disabled, disabling usbredirection");
> +        if (!libxl_defbool_val(b_info->spice.enable) &&
> +            (b_info->spice.usbredirection > 0) ){
> +            b_info->spice.usbredirection = 0;
> +            LOG(WARN,"spice disabled, disabling usbredirection");
>          }
>  
>          if (!b_info->u.hvm.usbversion &&
> -            (b_info->u.hvm.spice.usbredirection > 0) )
> +            (b_info->spice.usbredirection > 0) )
>              b_info->u.hvm.usbversion = 2;
>  
> -        if ((b_info->u.hvm.usbversion || b_info->u.hvm.spice.usbredirection) &&
> +        if ((b_info->u.hvm.usbversion || b_info->spice.usbredirection) &&
>              ( libxl_defbool_val(b_info->u.hvm.usb)
>              || b_info->u.hvm.usbdevice_list
>              || b_info->u.hvm.usbdevice) ){
> @@ -330,15 +342,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>              libxl_defbool_setdefault(&b_info->u.hvm.sdl.opengl, false);
>          }
>  
> -        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> -            libxl_defbool_setdefault(&b_info->u.hvm.spice.disable_ticketing,
> -                                     false);
> -            libxl_defbool_setdefault(&b_info->u.hvm.spice.agent_mouse, true);
> -            libxl_defbool_setdefault(&b_info->u.hvm.spice.vdagent, false);
> -            libxl_defbool_setdefault(&b_info->u.hvm.spice.clipboard_sharing,
> -                                     false);
> -        }
> -
>          libxl_defbool_setdefault(&b_info->u.hvm.nographic, false);
>  
>          libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false);
> @@ -372,6 +375,20 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>              b_info->cmdline = b_info->u.pv.cmdline;
>              b_info->u.pv.cmdline = NULL;
>          }
> +
> +        if (libxl_defbool_val(b_info->spice.vdagent)) {
> +            libxl_defbool_set(&b_info->spice.vdagent, false);
> +            LOG(WARN, "vdagent is not supported for PV guests");
> +        }
> +        if (libxl_defbool_val(b_info->spice.clipboard_sharing)) {
> +            libxl_defbool_set(&b_info->spice.clipboard_sharing, false);
> +            LOG(WARN, "clipboard sharing is not supported for PV guests");
> +        }
> +        if (b_info->spice.usbredirection > 0) {
> +            b_info->spice.usbredirection = 0;
> +            LOG(WARN, "usbredirection is not supported for PV guests");
> +        }
> +

Is there any support matrix in QEMU that can be used as reference?

>          break;
>      default:
>          LOG(ERROR, "invalid domain type %s in create info",
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a4934df..bf7cf1c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -897,6 +897,21 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>          flexarray_vappend(dm_args, "-k", keymap, NULL);
>      }
>  
> +    if (libxl_defbool_val(b_info->spice.enable)) {
> +        const libxl_spice_info *spice = &b_info->spice;
> +        char *spiceoptions = dm_spice_options(gc, spice);
> +        if (!spiceoptions)
> +            return ERROR_INVAL;
> +
> +        flexarray_append(dm_args, "-spice");
> +        flexarray_append(dm_args, spiceoptions);
> +        if (libxl_defbool_val(b_info->spice.vdagent)) {
> +            flexarray_vappend(dm_args, "-device", "virtio-serial",
> +                "-chardev", "spicevmc,id=vdagent,name=vdagent", "-device",
> +                "virtserialport,chardev=vdagent,name=com.redhat.spice.0", NULL);

There is hardcoded string here. Any reference why it is used?

> +        }
> +    }
> +
>      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          int ioemu_nics = 0;
>  
> @@ -934,22 +949,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-nographic");
>          }
>  
> -        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> -            const libxl_spice_info *spice = &b_info->u.hvm.spice;
> -            char *spiceoptions = dm_spice_options(gc, spice);
> -            if (!spiceoptions)
> -                return ERROR_INVAL;
> -
> -            flexarray_append(dm_args, "-spice");
> -            flexarray_append(dm_args, spiceoptions);
> -            if (libxl_defbool_val(b_info->u.hvm.spice.vdagent)) {
> -                flexarray_vappend(dm_args, "-device", "virtio-serial",
> -                    "-chardev", "spicevmc,id=vdagent,name=vdagent", "-device",
> -                    "virtserialport,chardev=vdagent,name=com.redhat.spice.0",
> -                    NULL);
> -            }
> -        }
> -
>          switch (b_info->u.hvm.vga.kind) {
>          case LIBXL_VGA_INTERFACE_TYPE_STD:
>              flexarray_append_pair(dm_args, "-device",
> @@ -1021,9 +1020,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                      "must be between 1 and 3");
>                  return ERROR_INVAL;
>              }
> -            if (b_info->u.hvm.spice.usbredirection >= 0 &&
> -                b_info->u.hvm.spice.usbredirection < 5) {
> -                for (i = 1; i <= b_info->u.hvm.spice.usbredirection; i++)
> +            if (b_info->spice.usbredirection >= 0 &&
> +                b_info->spice.usbredirection < 5) {
> +                for (i = 1; i <= b_info->spice.usbredirection; i++)
>                      flexarray_vappend(dm_args, "-chardev",
>                          GCSPRINTF("spicevmc,name=usbredir,id=usbrc%d", i),
>                          "-device",
> @@ -1081,7 +1080,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "none");
>          }
>      } else {
> -        if (!sdl && !vnc) {
> +        if (!sdl && !vnc && !libxl_defbool_val(b_info->spice.enable)) {
>              flexarray_append(dm_args, "-nographic");
>          }
>      }
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index cf3730f..354af0a 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -455,7 +455,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("ioports",          Array(libxl_ioport_range, "num_ioports")),
>      ("irqs",             Array(uint32, "num_irqs")),
>      ("iomem",            Array(libxl_iomem_range, "num_iomem")),
> -    ("claim_mode",	     libxl_defbool),
> +    ("claim_mode",       libxl_defbool),

Unrelated change.


Wei.