* [PATCH] libxl: add basic spice support for pv domUs
@ 2015-12-01 16:04 Fabio Fantoni
2016-01-11 15:00 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Fabio Fantoni @ 2015-12-01 16:04 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, Ian.Campbell, Stefano.Stabellini, George.Dunlap,
Ian.Jackson, jfehlig, Fabio Fantoni, anthony.perard
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>
-
-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).
-
-=item B<spice_image_compression=[auto_glz|auto_lz|quic|glz|lz|off]>
-
-Specifies what image compression is to be used by spice (if given), otherwise
-the qemu default will be used. Please see documentations of your current qemu
-version for details.
-
-=item B<spice_streaming_video=[filter|all|off]>
-
-Specifies what streaming video setting is to be used by spice (if given),
-otherwise the qemu default will be used.
-
-=back
-
=head3 Miscellaneous Emulated Hardware
=over 4
@@ -1838,6 +1762,85 @@ xen-qemudepriv-domid$domid or xen-qemudepriv-shared or root.
=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.
+
+=item B<spice_image_compression=[auto_glz|auto_lz|quic|glz|lz|off]>
+
+Specifies what image compression is to be used by spice (if given), otherwise
+the qemu default will be used. Please see documentations of your current qemu
+version for details.
+
+=item B<spice_streaming_video=[filter|all|off]>
+
+Specifies what streaming video setting is to be used by spice (if given),
+otherwise the qemu default will be used.
+
+=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 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.
+ *
+ * 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 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);
+ 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;
+ }
+
+ 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");
+ }
+
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);
+ }
+ }
+
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),
("event_channels", uint32),
("kernel", string),
("cmdline", string),
@@ -466,6 +466,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
# Note that the partial device tree should avoid to use the phandle
# 65000 which is reserved by the toolstack.
("device_tree", string),
+ ("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 2b6371d..1f65472 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2223,6 +2223,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) {
@@ -2231,7 +2233,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;
@@ -2250,6 +2252,30 @@ 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);
+ /* These SPICE features are not supported by PV domU */
+ xlu_cfg_get_defbool(config, "spicevdagent",
+ &b_info->spice.vdagent, 0);
+ xlu_cfg_get_defbool(config, "spice_clipboard_sharing",
+ &b_info->spice.clipboard_sharing, 0);
+ if (!xlu_cfg_get_long (config, "spiceusbredirection", &l, 0))
+ b_info->spice.usbredirection = l;
+ xlu_cfg_replace_string (config, "spice_image_compression",
+ &b_info->spice.image_compression, 0);
+ xlu_cfg_replace_string (config, "spice_streaming_video",
+ &b_info->spice.streaming_video, 0);
+
if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
if (!strcmp(buf, "stdvga")) {
@@ -2276,29 +2302,6 @@ skip_vfb:
}
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);
- xlu_cfg_get_defbool(config, "spice_clipboard_sharing",
- &b_info->u.hvm.spice.clipboard_sharing, 0);
- if (!xlu_cfg_get_long (config, "spiceusbredirection", &l, 0))
- b_info->u.hvm.spice.usbredirection = l;
- xlu_cfg_replace_string (config, "spice_image_compression",
- &b_info->u.hvm.spice.image_compression, 0);
- 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);
if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
index a8c127b..3c4f8dd 100644
--- a/tools/libxl/xl_sxp.c
+++ b/tools/libxl/xl_sxp.c
@@ -124,14 +124,14 @@ void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh)
fprintf(fh, "\t\t\t(nographic %s)\n",
libxl_defbool_to_string(b_info->u.hvm.nographic));
fprintf(fh, "\t\t\t(spice %s)\n",
- libxl_defbool_to_string(b_info->u.hvm.spice.enable));
- fprintf(fh, "\t\t\t(spiceport %d)\n", b_info->u.hvm.spice.port);
- fprintf(fh, "\t\t\t(spicetls_port %d)\n", b_info->u.hvm.spice.tls_port);
- fprintf(fh, "\t\t\t(spicehost %s)\n", b_info->u.hvm.spice.host);
+ libxl_defbool_to_string(b_info->spice.enable));
+ fprintf(fh, "\t\t\t(spiceport %d)\n", b_info->spice.port);
+ fprintf(fh, "\t\t\t(spicetls_port %d)\n", b_info->spice.tls_port);
+ fprintf(fh, "\t\t\t(spicehost %s)\n", b_info->spice.host);
fprintf(fh, "\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));
fprintf(fh, "\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));
fprintf(fh, "\t\t\t(device_model %s)\n", b_info->device_model ? : "default");
fprintf(fh, "\t\t\t(gfx_passthru %s)\n",
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] libxl: add basic spice support for pv domUs
2015-12-01 16:04 [PATCH] libxl: add basic spice support for pv domUs Fabio Fantoni
@ 2016-01-11 15:00 ` Wei Liu
2016-01-13 10:47 ` Fabio Fantoni
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2016-01-11 15:00 UTC (permalink / raw)
To: Fabio Fantoni
Cc: xen-devel, wei.liu2, Ian.Campbell, Stefano.Stabellini,
George.Dunlap, Ian.Jackson, jfehlig, anthony.perard
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] libxl: add basic spice support for pv domUs
2016-01-11 15:00 ` Wei Liu
@ 2016-01-13 10:47 ` Fabio Fantoni
2016-01-19 16:47 ` Ian Campbell
2016-02-16 17:44 ` Wei Liu
0 siblings, 2 replies; 6+ messages in thread
From: Fabio Fantoni @ 2016-01-13 10:47 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel, Ian.Campbell, Stefano.Stabellini, George.Dunlap,
Ian.Jackson, jfehlig, anthony.perard
Il 11/01/2016 16:00, Wei Liu ha scritto:
> 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?
Now is inside hvm specific section but if basic support for the pv domUs
will be added I think should be moved out of hvm section. Or I'm wrong?
>
>> =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.
Yes I keeped hvm.spice (as "duplicate") for compatibility as suggested
by someone long time ago.
>
>> + * 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).
This is needed to know if the new one is present and use it, also this
added after advice of someone, I think should be useful for people that
use libxl "without xl", like libvirt.
More exactly libxl_spice_info was already present, is the "spice" new
instead "hvm.spice"
> tools/libxl/libxl_types.idl
> + ("spice", libxl_spice_info),
I should rewrite the description to be more comprensible? If yes what
exactly I must write?
> + * 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 above (already present) seems comprensible to me but probably my
english is too bad.
>
>> + * 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.
If I remember good without setting always setdefault I saw some cases of
xl create segfault.
I didn't remember exactly cases, was long time ago.
>
>> + 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.
Thank for spotted it.
Can you explain exactly how to do it? I did a fast search but I'm
probably not understanding correctly.
> libxl_spice_info_copy(ctx, &b_info->spice, b_info->u.hvm.spice);
Should the code be as the line above?
I also not understand if I must also an _init before the _copy like this:
> libxl_spice_info_init(&b_info->spice);
>
>> + }
>> +
>> + 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?
No, FWIK. PV domUs don't have/support emulated pci, for example emulated
qemu usb controller used by spice usbredirection. Probably a pvusb
controller can be used for add it but I don't know how to do the needed
changes.
Same for vdagent (and other features provided by it) that require
virtio-serial, someone wrote there will be probably possible in future
with mmio on pvh (when will be implemented) but I don't remember the
details.
>
>> 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?
Was already present is for add virtio-serial port used by vdagent.
FWIK there isn't a better and simple way to do it.
http://www.spice-space.org/docs/manual/#_configuration_2
>
>> + }
>> + }
>> +
>> 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.
Is only a small indentation fix, I found it long time ago and still not
present.
I must do separate patch only with it?
>
>
> Wei.
Thanks for any reply and sorry for my bad english.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] libxl: add basic spice support for pv domUs
2016-01-13 10:47 ` Fabio Fantoni
@ 2016-01-19 16:47 ` Ian Campbell
2016-02-16 17:44 ` Wei Liu
1 sibling, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2016-01-19 16:47 UTC (permalink / raw)
To: Fabio Fantoni, Wei Liu
Cc: xen-devel, Stefano.Stabellini, George.Dunlap, Ian.Jackson,
jfehlig, anthony.perard
On Wed, 2016-01-13 at 11:47 +0100, Fabio Fantoni wrote:
> > > =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.
>
> Yes I keeped hvm.spice (as "duplicate") for compatibility as suggested
> by someone long time ago.
Saying "is used in preference to", rather than "instead of" in the comment
would express this better since "instead" implies replacing, not just
adding a newer interface).
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl: add basic spice support for pv domUs
2016-01-13 10:47 ` Fabio Fantoni
2016-01-19 16:47 ` Ian Campbell
@ 2016-02-16 17:44 ` Wei Liu
2016-02-19 15:26 ` Fabio Fantoni
1 sibling, 1 reply; 6+ messages in thread
From: Wei Liu @ 2016-02-16 17:44 UTC (permalink / raw)
To: Fabio Fantoni
Cc: xen-devel, Wei Liu, Ian.Campbell, Stefano.Stabellini,
George.Dunlap, Ian.Jackson, jfehlig, anthony.perard
On Wed, Jan 13, 2016 at 11:47:55AM +0100, Fabio Fantoni wrote:
[...]
> >>
> >>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?
>
> Now is inside hvm specific section but if basic support for the pv domUs
> will be added I think should be moved out of hvm section. Or I'm wrong?
>
Yes, you're right. Could you say so in the commit message?
> >
> >> =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.
>
> Yes I keeped hvm.spice (as "duplicate") for compatibility as suggested by
> someone long time ago.
>
> >
[...]
> >> * 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.
>
> If I remember good without setting always setdefault I saw some cases of xl
> create segfault.
> I didn't remember exactly cases, was long time ago.
>
I was not asking you to not set this. Presumably you still need to set
this, just that you can't set it unconditionally. You need to check if
this guest is really a 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.
>
> Thank for spotted it.
> Can you explain exactly how to do it? I did a fast search but I'm probably
> not understanding correctly.
> >libxl_spice_info_copy(ctx, &b_info->spice, b_info->u.hvm.spice);
> Should the code be as the line above?
> I also not understand if I must also an _init before the _copy like this:
> >libxl_spice_info_init(&b_info->spice);
>
You don't need to call _init because it should already be initialised
at this point.
And yes, you should just use libxl_spice_info_copy to replace the
direct assignment.
>
> >
> >>+ }
> >>+
> >>+ 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?
>
> No, FWIK. PV domUs don't have/support emulated pci, for example emulated
> qemu usb controller used by spice usbredirection. Probably a pvusb
> controller can be used for add it but I don't know how to do the needed
> changes.
> Same for vdagent (and other features provided by it) that require
> virtio-serial, someone wrote there will be probably possible in future with
> mmio on pvh (when will be implemented) but I don't remember the details.
>
If QEMU doesn't have one, can you provide one for Xen to reflect the
reality? There are several components in SPICE, can you point out
what each of them needs?
> >
> >> 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?
>
> Was already present is for add virtio-serial port used by vdagent.
> FWIK there isn't a better and simple way to do it.
> http://www.spice-space.org/docs/manual/#_configuration_2
>
Fair enough.
> >
> >>+ }
> >>+ }
> >>+
> >> 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.
>
> Is only a small indentation fix, I found it long time ago and still not
> present.
> I must do separate patch only with it?
Yes, a separate patch is more appropriate.
Wei.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] libxl: add basic spice support for pv domUs
2016-02-16 17:44 ` Wei Liu
@ 2016-02-19 15:26 ` Fabio Fantoni
0 siblings, 0 replies; 6+ messages in thread
From: Fabio Fantoni @ 2016-02-19 15:26 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel, Ian.Campbell, Stefano.Stabellini, George.Dunlap,
Ian.Jackson, jfehlig, anthony.perard
Il 16/02/2016 18:44, Wei Liu ha scritto:
> On Wed, Jan 13, 2016 at 11:47:55AM +0100, Fabio Fantoni wrote:
> [...]
>>>> 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?
>> Now is inside hvm specific section but if basic support for the pv domUs
>> will be added I think should be moved out of hvm section. Or I'm wrong?
>>
> Yes, you're right. Could you say so in the commit message?
yes, I'll do it
>
>>>> =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.
>> Yes I keeped hvm.spice (as "duplicate") for compatibility as suggested by
>> someone long time ago.
>>
> [...]
>>>> * 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.
>> If I remember good without setting always setdefault I saw some cases of xl
>> create segfault.
>> I didn't remember exactly cases, was long time ago.
>>
> I was not asking you to not set this. Presumably you still need to set
> this, just that you can't set it unconditionally. You need to check if
> this guest is really a HVM guest.
If I remember good some version ago I did it hvm specific but xl create
of pv domUs fails for this default not setted
>
>>>> + 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.
>> Thank for spotted it.
>> Can you explain exactly how to do it? I did a fast search but I'm probably
>> not understanding correctly.
>>> libxl_spice_info_copy(ctx, &b_info->spice, b_info->u.hvm.spice);
>> Should the code be as the line above?
>> I also not understand if I must also an _init before the _copy like this:
>>> libxl_spice_info_init(&b_info->spice);
> You don't need to call _init because it should already be initialised
> at this point.
>
> And yes, you should just use libxl_spice_info_copy to replace the
> direct assignment.
So replace "b_info->spice = b_info->u.hvm.spice;" with
"libxl_spice_info_copy(ctx, &b_info->spice, b_info->u.hvm.spice);" is
correct?
>
>>>> + }
>>>> +
>>>> + 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?
>> No, FWIK. PV domUs don't have/support emulated pci, for example emulated
>> qemu usb controller used by spice usbredirection. Probably a pvusb
>> controller can be used for add it but I don't know how to do the needed
>> changes.
>> Same for vdagent (and other features provided by it) that require
>> virtio-serial, someone wrote there will be probably possible in future with
>> mmio on pvh (when will be implemented) but I don't remember the details.
>>
> If QEMU doesn't have one, can you provide one for Xen to reflect the
> reality? There are several components in SPICE, can you point out
> what each of them needs?
usb redirection is based/needs an emulated usb controller (that require
emulated pci controller missed in pv), I saw that there is a xen pvusb
project without needing qemu emulated but I don't know if will be
possible use it (I suppose yes but doing some changes)
qxl is an emulated vga and require emulated pci controller missed in pv
vdagent require virtio-serial (that require emulated pci controller
missed in pv)
someone told that probably will be possible support them in pvh when
mmio support will be added but I don't know how to do it
>
>
>>>> 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?
>> Was already present is for add virtio-serial port used by vdagent.
>> FWIK there isn't a better and simple way to do it.
>> http://www.spice-space.org/docs/manual/#_configuration_2
>>
> Fair enough.
>
>>>> + }
>>>> + }
>>>> +
>>>> 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.
>> Is only a small indentation fix, I found it long time ago and still not
>> present.
>> I must do separate patch only with it?
> Yes, a separate patch is more appropriate.
Done and sent
>
> Wei.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-19 15:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-01 16:04 [PATCH] libxl: add basic spice support for pv domUs Fabio Fantoni
2016-01-11 15:00 ` Wei Liu
2016-01-13 10:47 ` Fabio Fantoni
2016-01-19 16:47 ` Ian Campbell
2016-02-16 17:44 ` Wei Liu
2016-02-19 15:26 ` Fabio Fantoni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).