|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libxl: add basic spice support for pv domUs
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@xxxxxxx>
>
> ---
>
> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |