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

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

[Fabio Fantoni]

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.
Patch v3 is tested and working, except for the pointer not
visible but working.
Patch v4 is not tested now and I now use the api for test
the u.hvm.spice retro-compatibility.
Any feedback is appreciated.

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>

---

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       |  133 ++++++++++++++++++++++---------------------
 tools/libxl/libxl.h         |   13 +++++
 tools/libxl/libxl_create.c  |   28 +++++----
 tools/libxl/libxl_dm.c      |   33 ++++++-----
 tools/libxl/libxl_types.idl |    3 +-
 tools/libxl/xl_cmdimpl.c    |   39 +++++++------
 tools/libxl/xl_sxp.c        |   12 ++--
 7 files changed, 143 insertions(+), 118 deletions(-)

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
@@ -1163,71 +1163,6 @@ it.
 
 =back
 
-=head3 Spice Graphics Support
-
-The following options control the features of SPICE.
-
-=over 4
-
-=item B<spice=BOOLEAN>
-
-Allow access to the display via the SPICE protocol.  This enables the
-other SPICE-related settings.
-
-=item B<spicehost="ADDRESS">
-
-Specify the interface address to listen on if given, otherwise any
-interface.
-
-=item B<spiceport=NUMBER>
-
-Specify the port to listen on by the SPICE server if the SPICE is
-enabled.
-
-=item B<spicetls_port=NUMBER>
-
-Specify the secure port to listen on by the SPICE server if the SPICE
-is enabled. At least one of the spiceport or spicetls_port must be
-given if SPICE is enabled.  NB. the options depending on spicetls_port
-have not been supported.
-
-=item B<spicedisable_ticketing=BOOLEAN>
-
-Enable client connection without password. When disabled, spicepasswd
-must be set. The default is false (0).
-
-=item B<spicepasswd="PASSWORD">
-
-Specify the ticket password which is used by a client for connection.
-
-=item B<spiceagent_mouse=BOOLEAN>
-
-Whether SPICE agent is used for client mouse mode. The default is true (1)
-(turn on)
-
-=item B<spicevdagent=BOOLEAN>
-
-Enables spice vdagent. The Spice vdagent is an optional component for
-enhancing user experience and performing guest-oriented management
-tasks. Its features includes: client mouse mode (no need to grab mouse
-by client, no mouse lag), automatic adjustment of screen resolution,
-copy and paste (text and image) between client and domU. It also
-requires vdagent service installed on domU o.s. to work. The default is 0.
-
-=item B<spice_clipboard_sharing=BOOLEAN>
-
-Enables Spice clipboard sharing (copy/paste). It requires spicevdagent
-enabled. The default is false (0).
-
-=item B<spiceusbredirection=NUMBER>
-
-Enables spice usbredirection. Creates NUMBER usbredirection channels
-for redirection of up to 4 usb devices from spice client to domU's qemu.
-It requires an usb controller and if not defined it will automatically adds
-an usb2 controller. The default is disabled (0).
-
-=back
-
 =head3 Miscellaneous Emulated Hardware
 
 =over 4
@@ -1376,6 +1311,74 @@ option to the device-model.
 
 =back
 
+=head2 Spice Graphics Support
+
+The following options control the features of SPICE.
+
+=over 4
+
+=item B<spice=BOOLEAN>
+
+Allow access to the display via the SPICE protocol.  This enables the
+other SPICE-related settings.
+
+=item B<spicehost="ADDRESS">
+
+Specify the interface address to listen on if given, otherwise any
+interface.
+
+=item B<spiceport=NUMBER>
+
+Specify the port to listen on by the SPICE server if the SPICE is
+enabled.
+
+=item B<spicetls_port=NUMBER>
+
+Specify the secure port to listen on by the SPICE server if the SPICE
+is enabled. At least one of the spiceport or spicetls_port must be
+given if SPICE is enabled.  NB. the options depending on spicetls_port
+have not been supported.
+
+=item B<spicedisable_ticketing=BOOLEAN>
+
+Enable client connection without password. When disabled, spicepasswd
+must be set. The default is false (0).
+
+=item B<spicepasswd="PASSWORD">
+
+Specify the ticket password which is used by a client for connection.
+
+=item B<spiceagent_mouse=BOOLEAN>
+
+Whether SPICE agent is used for client mouse mode. The default is true (1)
+(turn on)
+
+=item B<spicevdagent=BOOLEAN>
+
+Enables spice vdagent. The Spice vdagent is an optional component for
+enhancing user experience and performing guest-oriented management
+tasks. Its features includes: client mouse mode (no need to grab mouse
+by client, no mouse lag), automatic adjustment of screen resolution,
+copy and paste (text and image) between client and domU. It also
+requires vdagent service installed on domU o.s. to work. The default is 0.
+This feature is actually supported only by hvm domUs.
+
+=item B<spice_clipboard_sharing=BOOLEAN>
+
+Enables Spice clipboard sharing (copy/paste). It requires spicevdagent
+enabled. The default is false (0).
+This feature is actually supported only by hvm domUs.
+
+=item B<spiceusbredirection=NUMBER>
+
+Enables spice usbredirection. Creates NUMBER usbredirection channels
+for redirection of up to 4 usb devices from spice client to domU's qemu.
+It requires an usb controller and if not defined it will automatically adds
+an usb2 controller. The default is disabled (0).
+This feature is actually supported only by hvm domUs.
+
+=back
+
 =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 b2c3015..15f4df0 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -325,6 +325,19 @@
 #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.
+ *
+ * If it is set, callers may use spice to specify the spice values.
+ *
+ * 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 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 */
+    libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
+    if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
+        b_info->spice = b_info->u.hvm.spice;
+    }
+
+    libxl_defbool_setdefault(&b_info->spice.enable, false);
+    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);
+        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)
@@ -306,10 +320,10 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
 
         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) ){
@@ -337,16 +351,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
             libxl_defbool_setdefault(&b_info->u.hvm.sdl.opengl, false);
         }
 
-        libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, 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);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index d2530ab..071ebd9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -478,6 +478,21 @@ static char ** 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 NULL;
+
+        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);
+        }
+    }
+
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         int ioemu_nics = 0;
 
@@ -489,22 +504,6 @@ static char ** 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 NULL;
-
-            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",
@@ -640,7 +639,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-gfx_passthru");
         }
     } 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 e3404a0..3715513 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -333,8 +333,9 @@ 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),
     ("event_channels",   uint32),
+    ("spice",            libxl_spice_info),
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
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)) {
             libxl_device_vfb *vfb;
             libxl_device_vkb *vkb;
 
@@ -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 */
+    b_info->spice.usbredirection = 0;
+
     if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
             if (!strcmp(buf, "stdvga")) {
@@ -1693,25 +1710,13 @@ skip_vfb:
                                          LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
 
         xlu_cfg_replace_string (config, "keymap", &b_info->u.hvm.keymap, 0);
-        xlu_cfg_get_defbool (config, "spice", &b_info->u.hvm.spice.enable, 0);
-        if (!xlu_cfg_get_long (config, "spiceport", &l, 0))
-            b_info->u.hvm.spice.port = l;
-        if (!xlu_cfg_get_long (config, "spicetls_port", &l, 0))
-            b_info->u.hvm.spice.tls_port = l;
-        xlu_cfg_replace_string (config, "spicehost",
-                                &b_info->u.hvm.spice.host, 0);
-        xlu_cfg_get_defbool(config, "spicedisable_ticketing",
-                            &b_info->u.hvm.spice.disable_ticketing, 0);
-        xlu_cfg_replace_string (config, "spicepasswd",
-                                &b_info->u.hvm.spice.passwd, 0);
-        xlu_cfg_get_defbool(config, "spiceagent_mouse",
-                            &b_info->u.hvm.spice.agent_mouse, 0);
+
         xlu_cfg_get_defbool(config, "spicevdagent",
-                            &b_info->u.hvm.spice.vdagent, 0);
+                            &b_info->spice.vdagent, 0);
         xlu_cfg_get_defbool(config, "spice_clipboard_sharing",
-                            &b_info->u.hvm.spice.clipboard_sharing, 0);
+                            &b_info->spice.clipboard_sharing, 0);
         if (!xlu_cfg_get_long (config, "spiceusbredirection", &l, 0))
-            b_info->u.hvm.spice.usbredirection = l;
+            b_info->spice.usbredirection = l;
         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);
diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
index a16a025..e9eed99 100644
--- a/tools/libxl/xl_sxp.c
+++ b/tools/libxl/xl_sxp.c
@@ -127,14 +127,14 @@ void printf_info_sexp(int domid, libxl_domain_config *d_config)
         printf("\t\t\t(nographic %s)\n",
                libxl_defbool_to_string(b_info->u.hvm.nographic));
         printf("\t\t\t(spice %s)\n",
-               libxl_defbool_to_string(b_info->u.hvm.spice.enable));
-        printf("\t\t\t(spiceport %d)\n", b_info->u.hvm.spice.port);
-        printf("\t\t\t(spicetls_port %d)\n", b_info->u.hvm.spice.tls_port);
-        printf("\t\t\t(spicehost %s)\n", b_info->u.hvm.spice.host);
+               libxl_defbool_to_string(b_info->spice.enable));
+        printf("\t\t\t(spiceport %d)\n", b_info->spice.port);
+        printf("\t\t\t(spicetls_port %d)\n", b_info->spice.tls_port);
+        printf("\t\t\t(spicehost %s)\n", b_info->spice.host);
         printf("\t\t\t(spicedisable_ticketing %s)\n",
-               libxl_defbool_to_string(b_info->u.hvm.spice.disable_ticketing));
+               libxl_defbool_to_string(b_info->spice.disable_ticketing));
         printf("\t\t\t(spiceagent_mouse %s)\n",
-               libxl_defbool_to_string(b_info->u.hvm.spice.agent_mouse));
+               libxl_defbool_to_string(b_info->spice.agent_mouse));
 
         printf("\t\t\t(device_model %s)\n", b_info->device_model ? : "default");
         printf("\t\t\t(gfx_passthru %s)\n",
-- 
1.7.9.5

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

[Ian Campbell]

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.

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

[Fabio Fantoni]

Il 08/05/2014 11:33, Ian Campbell ha scritto:
> 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?

Probably is no more a draft except for api compatibility part.

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

Yes (except for vdagent and usbredir notes)

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

ok

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

Spice requires qemu.
What would I do to always start qemu with spice enabled? (even if unused 
qdisk and vnc and/or using api instead of xl)

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

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

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

I not know for sure what spice multiple interfaces support is.
I only know that support multihead with qxl vga.

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

I'll do it.

>
> Ian.
>

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

[Ian Campbell]

On Fri, 2014-05-09 at 15:48 +0200, Fabio Fantoni wrote:

> >
> >> +    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.
> 
> Spice requires qemu.

Understood.

> What would I do to always start qemu with spice enabled? (even if unused 
> qdisk and vnc and/or using api instead of xl)

You need to update libxl__need_xenpv_qemu to return true if spice is
enabled, along with all the other conditions.

But: Does spice already require a VFB? In which case the existing
handing of that will suffice.

I think I'm probably confused about something: Does this patch
enable/expose SPICE to the guest? Or does it simply enable the existing
PVFB's output to be exposed from the qemu process to a spice client? I
had been thinking this was a parallel feature to PVFB, but I'm starting
to suspect this might actually be an additional feature *of* PVFB, which
is correct? (I'm afraid that depending on which it is I might have to
check back over my review to make sure I haven't suggested anything
stupid)

Does QXL change that answer?

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

I think the importance of this depends on my question about VFB.

Ian.

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

[Fabio Fantoni]

Il 09/05/2014 16:00, Ian Campbell ha scritto:
> On Fri, 2014-05-09 at 15:48 +0200, Fabio Fantoni wrote:
>
>>>> +    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.
>> Spice requires qemu.
> Understood.
>
>> What would I do to always start qemu with spice enabled? (even if unused
>> qdisk and vnc and/or using api instead of xl)
> You need to update libxl__need_xenpv_qemu to return true if spice is
> enabled, along with all the other conditions.

Now I understand, thanks.

> But: Does spice already require a VFB? In which case the existing
> handing of that will suffice.
>
> I think I'm probably confused about something: Does this patch
> enable/expose SPICE to the guest? Or does it simply enable the existing
> PVFB's output to be exposed from the qemu process to a spice client? I
> had been thinking this was a parallel feature to PVFB, but I'm starting
> to suspect this might actually be an additional feature *of* PVFB, which
> is correct? (I'm afraid that depending on which it is I might have to
> check back over my review to make sure I haven't suggested anything
> stupid)
>
> Does QXL change that answer?

QXL and other emulated vga is now not supported on pv domUs because 
require an emulated pci bus and MMIO not supported on pv domUs FWIK.

During my first test, I was enabled spice in pv domus but the guest wont 
show nothing while connecting to it.
On my second shot, I've added -vga xenfb but it is deprecated. This way 
I've got at least video out but no mouse nor keyboard.
Following the suggestions from Stefano Stabellini I've then activated 
vfb the same way as for vnc and i got it all working but mouse, which is 
only visible in some circumstances (such grafical debian installer) but 
always working (even if not visible).
The last one is the actual patch.
Probably the vfb part is not complete or at least improvable.
Perhaps it is even not necessary if pvfb will became modified to works 
with spice, so domUs use this through its modules.

I feel I'm not perfectly undrestanding pvfb. Please would you like 
better describe me this component?
Another thing: i noticed that vnc has all its parameters duplicated in 
vfb too. For the moment I omissed them since i feel they are not 
necessary - aren't they?

Thanks so much for any replies.

>>> Does spice not support multiple interfaces like the vfb option does? If
>>> so we should plan for that in the interface I think.
>> I not know for sure what spice multiple interfaces support is.
> I think the importance of this depends on my question about VFB.
>
> Ian.
>

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

[Ian Campbell]

On Tue, 2014-05-13 at 10:00 +0200, Fabio Fantoni wrote:
> > But: Does spice already require a VFB? In which case the existing
> > handing of that will suffice.
> >
> > I think I'm probably confused about something: Does this patch
> > enable/expose SPICE to the guest? Or does it simply enable the existing
> > PVFB's output to be exposed from the qemu process to a spice client? I
> > had been thinking this was a parallel feature to PVFB, but I'm starting
> > to suspect this might actually be an additional feature *of* PVFB, which
> > is correct? (I'm afraid that depending on which it is I might have to
> > check back over my review to make sure I haven't suggested anything
> > stupid)
> >
> > Does QXL change that answer?
> 
> QXL and other emulated vga is now not supported on pv domUs because 
> require an emulated pci bus and MMIO not supported on pv domUs FWIK.

Right, but what about the previous paragraph? That was the important bit
for me to understand, since it impacts the entire configuration model
and libxl API.

> During my first test, I was enabled spice in pv domus but the guest wont 
> show nothing while connecting to it.
> On my second shot, I've added -vga xenfb but it is deprecated. This way 
> I've got at least video out but no mouse nor keyboard.
> Following the suggestions from Stefano Stabellini I've then activated 
> vfb the same way as for vnc and i got it all working but mouse, which is 
> only visible in some circumstances (such grafical debian installer) but 
> always working (even if not visible).
> The last one is the actual patch.
> Probably the vfb part is not complete or at least improvable.
> Perhaps it is even not necessary if pvfb will became modified to works 
> with spice, so domUs use this through its modules.

Is SPICE a mechanism for exposing the PVFB or is it something entirely
separate? That is the crux of my question above.

> I feel I'm not perfectly undrestanding pvfb. Please would you like 
> better describe me this component?

I need to understand what it is this patch is actually doing. I'm
starting to worry that perhaps you don't understand either.

> Another thing: i noticed that vnc has all its parameters duplicated in 
> vfb too. For the moment I omissed them since i feel they are not 
> necessary - aren't they

It really depends on the answer to my questions above.

If SPICE is nothing to do with PVFB then it obviously makes no sense for
it to be separate.

If SPICE is just a backend for PVFB then it actually makes *more* sense
to expose spice as a PVFB configuration parameter than as a set of top
level options. The fact that VNC is exposed as a top level thing too is
somewhat anomalous but was done for xend compatibility (nb: it is the
top level duplicating the vfb VNC settings, not vice versa IMHO).

Ian.

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

[Fabio Fantoni]

Il 13/05/2014 11:50, Ian Campbell ha scritto:
> On Tue, 2014-05-13 at 10:00 +0200, Fabio Fantoni wrote:
>>> But: Does spice already require a VFB? In which case the existing
>>> handing of that will suffice.
>>>
>>> I think I'm probably confused about something: Does this patch
>>> enable/expose SPICE to the guest? Or does it simply enable the existing
>>> PVFB's output to be exposed from the qemu process to a spice client? I
>>> had been thinking this was a parallel feature to PVFB, but I'm starting
>>> to suspect this might actually be an additional feature *of* PVFB, which
>>> is correct? (I'm afraid that depending on which it is I might have to
>>> check back over my review to make sure I haven't suggested anything
>>> stupid)
>>>
>>> Does QXL change that answer?
>> QXL and other emulated vga is now not supported on pv domUs because
>> require an emulated pci bus and MMIO not supported on pv domUs FWIK.
> Right, but what about the previous paragraph? That was the important bit
> for me to understand, since it impacts the entire configuration model
> and libxl API.
>
>> During my first test, I was enabled spice in pv domus but the guest wont
>> show nothing while connecting to it.
>> On my second shot, I've added -vga xenfb but it is deprecated. This way
>> I've got at least video out but no mouse nor keyboard.
>> Following the suggestions from Stefano Stabellini I've then activated
>> vfb the same way as for vnc and i got it all working but mouse, which is
>> only visible in some circumstances (such grafical debian installer) but
>> always working (even if not visible).
>> The last one is the actual patch.
>> Probably the vfb part is not complete or at least improvable.
>> Perhaps it is even not necessary if pvfb will became modified to works
>> with spice, so domUs use this through its modules.
> Is SPICE a mechanism for exposing the PVFB or is it something entirely
> separate? That is the crux of my question above.

I added also qemu-devel and spice-devel on cc:
Any one on this please?

>
>> I feel I'm not perfectly undrestanding pvfb. Please would you like
>> better describe me this component?
> I need to understand what it is this patch is actually doing. I'm
> starting to worry that perhaps you don't understand either.

Make possible using spice with basic features also on pv domUs.
Since emulated vgas is not supported on pv I use vfb instead, even if 
this part is incomplete and rudimental.
I'm waiting from some aswers from spice and qemu teams to share some 
light on the question above.

Thanks for any reply and sorry for my bad english.

>
>> Another thing: i noticed that vnc has all its parameters duplicated in
>> vfb too. For the moment I omissed them since i feel they are not
>> necessary - aren't they
> It really depends on the answer to my questions above.
>
> If SPICE is nothing to do with PVFB then it obviously makes no sense for
> it to be separate.
>
> If SPICE is just a backend for PVFB then it actually makes *more* sense
> to expose spice as a PVFB configuration parameter than as a set of top
> level options. The fact that VNC is exposed as a top level thing too is
> somewhat anomalous but was done for xend compatibility (nb: it is the
> top level duplicating the vfb VNC settings, not vice versa IMHO).
>
> Ian.
>

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

[Ian Campbell]

On Tue, 2014-05-13 at 12:51 +0200, Fabio Fantoni wrote:
> Il 13/05/2014 11:50, Ian Campbell ha scritto:
> > On Tue, 2014-05-13 at 10:00 +0200, Fabio Fantoni wrote:
> >>> But: Does spice already require a VFB? In which case the existing
> >>> handing of that will suffice.
> >>>
> >>> I think I'm probably confused about something: Does this patch
> >>> enable/expose SPICE to the guest? Or does it simply enable the existing
> >>> PVFB's output to be exposed from the qemu process to a spice client? I
> >>> had been thinking this was a parallel feature to PVFB, but I'm starting
> >>> to suspect this might actually be an additional feature *of* PVFB, which
> >>> is correct? (I'm afraid that depending on which it is I might have to
> >>> check back over my review to make sure I haven't suggested anything
> >>> stupid)
> >>>
> >>> Does QXL change that answer?
> >> QXL and other emulated vga is now not supported on pv domUs because
> >> require an emulated pci bus and MMIO not supported on pv domUs FWIK.
> > Right, but what about the previous paragraph? That was the important bit
> > for me to understand, since it impacts the entire configuration model
> > and libxl API.
> >
> >> During my first test, I was enabled spice in pv domus but the guest wont
> >> show nothing while connecting to it.
> >> On my second shot, I've added -vga xenfb but it is deprecated. This way
> >> I've got at least video out but no mouse nor keyboard.
> >> Following the suggestions from Stefano Stabellini I've then activated
> >> vfb the same way as for vnc and i got it all working but mouse, which is
> >> only visible in some circumstances (such grafical debian installer) but
> >> always working (even if not visible).
> >> The last one is the actual patch.
> >> Probably the vfb part is not complete or at least improvable.
> >> Perhaps it is even not necessary if pvfb will became modified to works
> >> with spice, so domUs use this through its modules.
> > Is SPICE a mechanism for exposing the PVFB or is it something entirely
> > separate? That is the crux of my question above.
> 
> I added also qemu-devel and spice-devel on cc:
> Any one on this please?
> 
> >
> >> I feel I'm not perfectly undrestanding pvfb. Please would you like
> >> better describe me this component?
> > I need to understand what it is this patch is actually doing. I'm
> > starting to worry that perhaps you don't understand either.
> 
> Make possible using spice with basic features also on pv domUs.

*How* is the important thing. How is SPICE exposed to the guest? How is
SPICE exposed to the user/client?

I'm a bit surprised you've been pushing a patch to enable something
without knowing the answers to these sorts questions. If you don't
understand what it does how do you know it is doing the correct thing,
and how can you therefore explain it to the person reviewing the patch?

> Since emulated vgas is not supported on pv I use vfb instead, even if 
> this part is incomplete and rudimental.

What is incomplete and rudimentary?

> I'm waiting from some aswers from spice and qemu teams to share some 
> light on the question above.
> 
> Thanks for any reply and sorry for my bad english.
> 
> >
> >> Another thing: i noticed that vnc has all its parameters duplicated in
> >> vfb too. For the moment I omissed them since i feel they are not
> >> necessary - aren't they
> > It really depends on the answer to my questions above.
> >
> > If SPICE is nothing to do with PVFB then it obviously makes no sense for
> > it to be separate.
> >
> > If SPICE is just a backend for PVFB then it actually makes *more* sense
> > to expose spice as a PVFB configuration parameter than as a set of top
> > level options. The fact that VNC is exposed as a top level thing too is
> > somewhat anomalous but was done for xend compatibility (nb: it is the
> > top level duplicating the vfb VNC settings, not vice versa IMHO).
> >
> > Ian.
> >

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

[Fabio Fantoni]

Il 13/05/2014 13:00, Ian Campbell ha scritto:
> On Tue, 2014-05-13 at 12:51 +0200, Fabio Fantoni wrote:
>> Il 13/05/2014 11:50, Ian Campbell ha scritto:
>>> On Tue, 2014-05-13 at 10:00 +0200, Fabio Fantoni wrote:
>>>>> But: Does spice already require a VFB? In which case the existing
>>>>> handing of that will suffice.
>>>>>
>>>>> I think I'm probably confused about something: Does this patch
>>>>> enable/expose SPICE to the guest? Or does it simply enable the existing
>>>>> PVFB's output to be exposed from the qemu process to a spice client? I
>>>>> had been thinking this was a parallel feature to PVFB, but I'm starting
>>>>> to suspect this might actually be an additional feature *of* PVFB, which
>>>>> is correct? (I'm afraid that depending on which it is I might have to
>>>>> check back over my review to make sure I haven't suggested anything
>>>>> stupid)
>>>>>
>>>>> Does QXL change that answer?
>>>> QXL and other emulated vga is now not supported on pv domUs because
>>>> require an emulated pci bus and MMIO not supported on pv domUs FWIK.
>>> Right, but what about the previous paragraph? That was the important bit
>>> for me to understand, since it impacts the entire configuration model
>>> and libxl API.
>>>
>>>> During my first test, I was enabled spice in pv domus but the guest wont
>>>> show nothing while connecting to it.
>>>> On my second shot, I've added -vga xenfb but it is deprecated. This way
>>>> I've got at least video out but no mouse nor keyboard.
>>>> Following the suggestions from Stefano Stabellini I've then activated
>>>> vfb the same way as for vnc and i got it all working but mouse, which is
>>>> only visible in some circumstances (such grafical debian installer) but
>>>> always working (even if not visible).
>>>> The last one is the actual patch.
>>>> Probably the vfb part is not complete or at least improvable.
>>>> Perhaps it is even not necessary if pvfb will became modified to works
>>>> with spice, so domUs use this through its modules.
>>> Is SPICE a mechanism for exposing the PVFB or is it something entirely
>>> separate? That is the crux of my question above.
>> I added also qemu-devel and spice-devel on cc:
>> Any one on this please?
>>
>>>> I feel I'm not perfectly undrestanding pvfb. Please would you like
>>>> better describe me this component?
>>> I need to understand what it is this patch is actually doing. I'm
>>> starting to worry that perhaps you don't understand either.
>> Make possible using spice with basic features also on pv domUs.
> *How* is the important thing. How is SPICE exposed to the guest?

FWIK spice-server is inside qemu, used but not "visible" inside the 
guest (seems similar to qemu's vnc in hvm domUs).
Inside the guest there are some advanced and optional features visible 
(installing software and additional drivers like spice-guest-tools).
The advanced features are actually not supported in the pv domUs for 
some missed requirements (for example virtio-serial and emulated pci).

> How is
> SPICE exposed to the user/client?

Users can use these clients to connect to guest using spice: 
remote-viewer (mainly used), spicy or spicec (deprecated).
These clients basically connect to domU's qemu using spice and also to 
some additional software/drivers installed in domU's S.O. for some 
advanced features (if enabled).

>
> I'm a bit surprised you've been pushing a patch to enable something
> without knowing the answers to these sorts questions. If you don't
> understand what it does how do you know it is doing the correct thing,
> and how can you therefore explain it to the person reviewing the patch?

Unfortunately at the moment seems there is no other person available to 
implementing full spice support into xen, so I'm trying to do it myself 
even if with a very short knowledge about it and few time.
Spice is a lot better then what I have used since now (xen's vnc + guest 
rdp and proprietary usb redirection).
My main goal is to use spice full featured on a thin client system to 
gain pc-like experience accessing hvm and pv domUs.
Also use different architectures (for example starting with arm on thin 
client) for now impossibile with proprietary usbredirection (only for 
x86) and with some problem that usbredirect used by spice hasn't ecc...

Spice requires the same qemu parameters on both pv and hvm domUs.
The main difference is that with pv which actually lack of video out and 
keyboard/pointer input, I have used the only thing I found working: vfb 
(+vkb).

>
>> Since emulated vgas is not supported on pv I use vfb instead, even if
>> this part is incomplete and rudimental.
> What is incomplete and rudimentary?

The patch's part about setting/use of vfb(+vkb) with spice on pv domUs.

>
>> I'm waiting from some aswers from spice and qemu teams to share some
>> light on the question above.
>>
>> Thanks for any reply and sorry for my bad english.
>>
>>>> Another thing: i noticed that vnc has all its parameters duplicated in
>>>> vfb too. For the moment I omissed them since i feel they are not
>>>> necessary - aren't they
>>> It really depends on the answer to my questions above.
>>>
>>> If SPICE is nothing to do with PVFB then it obviously makes no sense for
>>> it to be separate.
>>>
>>> If SPICE is just a backend for PVFB then it actually makes *more* sense
>>> to expose spice as a PVFB configuration parameter than as a set of top
>>> level options. The fact that VNC is exposed as a top level thing too is
>>> somewhat anomalous but was done for xend compatibility (nb: it is the
>>> top level duplicating the vfb VNC settings, not vice versa IMHO).
>>>
>>> Ian.
>>>
>