Re: [Xen-devel] [PATCH v4] libxl: add basic spice support for pv domUs

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-05-02 at 12:28 +0200, 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 paramters 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.
> 
> Note: this patch is only a draft and needs to be improved.

Is it? What is draft about it?

> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 146b461..8cb8c57 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5

I assume this is purely movement from one section to another and
therefore doesn't need close review?

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 3376b5c..521d387 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -215,6 +215,20 @@ 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 setted copy it in newer one */

s/setted/enabled/

I'd probably say "If HVM spice is enabled then propagate it to the top
level" or something.

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

I think this will cause hvm.spice to take precedence over the top-level
spice field, which I don't think is what you wanted, was it?

I think you need:
    libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
    libxl_defbool_setdefault(&b_info->spice.enable, false);
    if (!libxl_defbool_val(&b_info->spice.enable) && 
        libxl_defbool_setdefault(&b_info->u.hvm.spice.enable))
            b_info->spice = b_info->u.hvm.spice;

Then this bit:

> +    if (libxl_defbool_val(b_info->spice.enable)) {
> +        libxl_defbool_setdefault(&b_info->spice.disable_ticketing, false);
> +        libxl_defbool_setdefault(&b_info->spice.agent_mouse, true);

Does this work for PV guests? Having the default vary with b_info->type
would be fine IMHO.

I don't see you changing the result of calling libxl__need_xenpv_qemu
anywhere to be true if spice is enabled. I think this means qemu won't
be launched unless there happens to also be a qdisk or some other qemu
backend.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c196829..ff8fe67 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1647,6 +1647,8 @@ skip_vfb:
>  
>  #undef parse_extra_args
>  
> +    xlu_cfg_get_defbool (config, "spice", &b_info->spice.enable, 0);
> +
>      /* If we've already got vfb=[] for PV guest then ignore top level
>       * VNC config. */
>      if (c_info->type == LIBXL_DOMAIN_TYPE_PV && !d_config->num_vfbs) {
> @@ -1655,7 +1657,7 @@ skip_vfb:
>          if (!xlu_cfg_get_long (config, "vnc", &l, 0))
>              vnc_enabled = l;
>  
> -        if (vnc_enabled) {
> +        if (vnc_enabled || libxl_defbool_val(b_info->spice.enable)) {


If the config file is not saying spice explicitly on or off then
libxl_defbool_val doesn't know the default and this will fault. You need
to do something similar to vnc_enabled here.

You will also have caused parse_top_level_vnc_options() to be called
even if vnc is disabled. THat's probably harmless. Bit I think for
consistency you should move the spice stuff into
parse_top_level_spice_options().

Does spice not support multiple interfaces like the vfb option does? If
so we should plan for that in the interface I think.

> @@ -1674,6 +1676,21 @@ skip_vfb:
>          parse_top_level_sdl_options(config, &b_info->u.hvm.sdl);
>      }
>  
> +    if (!xlu_cfg_get_long (config, "spiceport", &l, 0))
> +        b_info->spice.port = l;
> +    if (!xlu_cfg_get_long (config, "spicetls_port", &l, 0))
> +        b_info->spice.tls_port = l;
> +    xlu_cfg_replace_string (config, "spicehost",
> +                            &b_info->spice.host, 0);
> +    xlu_cfg_get_defbool(config, "spicedisable_ticketing",
> +                        &b_info->spice.disable_ticketing, 0);
> +    xlu_cfg_replace_string(config, "spicepasswd",
> +                            &b_info->spice.passwd, 0);
> +    xlu_cfg_get_defbool(config, "spiceagent_mouse",
> +                        &b_info->spice.agent_mouse, 0);
> +    /* Some spice features are missed because not supported by domU pv now */

/* These SPICE features are not supported by PV domU */

> +    b_info->spice.usbredirection = 0;

It might be nice to parse the option and say "WARNING: usbredirection is
not supported for PV guests" if it is enabled. Not sure.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel