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