[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v4] libxl: add basic spice support for pv domUs
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
