[libvirt] [PATCH 0/4] libxl: support SPICE graphics
This series provides support for SPICE graphics in the libxl driver. The first patch is mostly code movement. The second patch is a trivial name change of a structure member. Patch3 populates the libxl_domain_build_info struct with SPICE info. SPICE isn't as nice without QXL, so patch4 provides support for QXL video device. Jim Fehlig (4): libxl: populate build_info vfb in separate function libxl: change reservedVNCPorts to reservedGraphicsPorts libxl: support SPICE graphics for HVM domains libxl: support QXL video device src/libxl/libxl_conf.c | 150 +++++++++++++++++++++++++++++++++++++---------- src/libxl/libxl_conf.h | 2 +- src/libxl/libxl_domain.c | 8 ++- src/libxl/libxl_driver.c | 4 +- 4 files changed, 128 insertions(+), 36 deletions(-) -- 1.8.4.5
For HVM domains, vfb info must be populated in the libxl_domain_build_info stuct. Currently this is done in the libxlMakeVfbList function, but IMO it would be cleaner to populate the build_info vfb in a separate libxlMakeBuildInfoVfb function. libxlMakeVfbList would then handle only vfb devices, simiar to the other libxlMake<device>List functions. A future patch will extend libxlMakeBuildInfoVfb to support SPICE. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 31 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index fccada5..8552c77 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, d_config->vkbs = x_vkbs; d_config->num_vfbs = d_config->num_vkbs = nvfbs; - /* - * VNC or SDL info must also be set in libxl_domain_build_info - * for HVM domains. Use the first vfb device. - */ - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - libxl_domain_build_info *b_info = &d_config->b_info; - libxl_device_vfb vfb = d_config->vfbs[0]; - - if (libxl_defbool_val(vfb.vnc.enable)) { - libxl_defbool_set(&b_info->u.hvm.vnc.enable, true); - if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0) - goto error; - if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0) - goto error; - b_info->u.hvm.vnc.display = vfb.vnc.display; - libxl_defbool_set(&b_info->u.hvm.vnc.findunused, - libxl_defbool_val(vfb.vnc.findunused)); - } else if (libxl_defbool_val(vfb.sdl.enable)) { - libxl_defbool_set(&b_info->u.hvm.sdl.enable, true); - libxl_defbool_set(&b_info->u.hvm.sdl.opengl, - libxl_defbool_val(vfb.sdl.opengl)); - if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0) - goto error; - if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0) - goto error; - } - - if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0) - goto error; - } - return 0; error: @@ -1367,6 +1336,51 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, } /* + * Populate vfb info in libxl_domain_build_info struct for HVM domains. + * Use first libxl_device_vfb device in libxl_domain_config->vfbs. + * Prior to calling this function, libxlMakeVfbList must be called to + * populate libxl_domain_config->vfbs. + */ +static int +libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +{ + libxl_domain_build_info *b_info = &d_config->b_info; + libxl_device_vfb x_vfb; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0; + + if (d_config->num_vfbs == 0) + return 0; + + x_vfb = d_config->vfbs[0]; + + if (libxl_defbool_val(x_vfb.vnc.enable)) { + libxl_defbool_set(&b_info->u.hvm.vnc.enable, true); + if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0) + return -1; + if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0) + return -1; + b_info->u.hvm.vnc.display = x_vfb.vnc.display; + libxl_defbool_set(&b_info->u.hvm.vnc.findunused, + libxl_defbool_val(x_vfb.vnc.findunused)); + } else if (libxl_defbool_val(x_vfb.sdl.enable)) { + libxl_defbool_set(&b_info->u.hvm.sdl.enable, true); + libxl_defbool_set(&b_info->u.hvm.sdl.opengl, + libxl_defbool_val(x_vfb.sdl.opengl)); + if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0) + return -1; + if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0) + return -1; + } + + if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0) + return -1; + + return 0; +} + +/* * Get domain0 autoballoon configuration. Honor user-specified * setting in libxl.conf first. If not specified, autoballooning * is disabled when domain0's memory is set with 'dom0_mem'. @@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, if (libxlMakeVfbList(graphicsports, def, d_config) < 0) return -1; + if (libxlMakeBuildInfoVfb(def, d_config) < 0) + return -1; + if (libxlMakePCIList(def, d_config) < 0) return -1; -- 1.8.4.5
On Fri, May 08, 2015 at 04:31:02PM -0600, Jim Fehlig wrote:
For HVM domains, vfb info must be populated in the libxl_domain_build_info stuct. Currently this is done in the libxlMakeVfbList function, but IMO
struct
it would be cleaner to populate the build_info vfb in a separate libxlMakeBuildInfoVfb function. libxlMakeVfbList would then handle only vfb devices, simiar to the other libxlMake<device>List functions.
A future patch will extend libxlMakeBuildInfoVfb to support SPICE.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index fccada5..8552c77 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, d_config->vkbs = x_vkbs; d_config->num_vfbs = d_config->num_vkbs = nvfbs;
- /* - * VNC or SDL info must also be set in libxl_domain_build_info - * for HVM domains. Use the first vfb device. - */ - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - libxl_domain_build_info *b_info = &d_config->b_info; - libxl_device_vfb vfb = d_config->vfbs[0]; - - if (libxl_defbool_val(vfb.vnc.enable)) { - libxl_defbool_set(&b_info->u.hvm.vnc.enable, true); - if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0) - goto error; - if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0) - goto error; - b_info->u.hvm.vnc.display = vfb.vnc.display; - libxl_defbool_set(&b_info->u.hvm.vnc.findunused, - libxl_defbool_val(vfb.vnc.findunused)); - } else if (libxl_defbool_val(vfb.sdl.enable)) { - libxl_defbool_set(&b_info->u.hvm.sdl.enable, true); - libxl_defbool_set(&b_info->u.hvm.sdl.opengl, - libxl_defbool_val(vfb.sdl.opengl)); - if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0) - goto error; - if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0) - goto error; - } - - if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0) - goto error; - } - return 0;
error: @@ -1367,6 +1336,51 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, }
/* + * Populate vfb info in libxl_domain_build_info struct for HVM domains. + * Use first libxl_device_vfb device in libxl_domain_config->vfbs. + * Prior to calling this function, libxlMakeVfbList must be called to + * populate libxl_domain_config->vfbs. + */ +static int +libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +{ + libxl_domain_build_info *b_info = &d_config->b_info; + libxl_device_vfb x_vfb; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0; + + if (d_config->num_vfbs == 0) + return 0; + + x_vfb = d_config->vfbs[0]; + + if (libxl_defbool_val(x_vfb.vnc.enable)) { + libxl_defbool_set(&b_info->u.hvm.vnc.enable, true); + if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0) + return -1; + if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0) + return -1; + b_info->u.hvm.vnc.display = x_vfb.vnc.display; + libxl_defbool_set(&b_info->u.hvm.vnc.findunused, + libxl_defbool_val(x_vfb.vnc.findunused)); + } else if (libxl_defbool_val(x_vfb.sdl.enable)) { + libxl_defbool_set(&b_info->u.hvm.sdl.enable, true); + libxl_defbool_set(&b_info->u.hvm.sdl.opengl, + libxl_defbool_val(x_vfb.sdl.opengl)); + if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0) + return -1; + if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0) + return -1; + } + + if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0) + return -1; + + return 0; +} + +/* * Get domain0 autoballoon configuration. Honor user-specified * setting in libxl.conf first. If not specified, autoballooning * is disabled when domain0's memory is set with 'dom0_mem'. @@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, if (libxlMakeVfbList(graphicsports, def, d_config) < 0) return -1;
+ if (libxlMakeBuildInfoVfb(def, d_config) < 0) + return -1; + if (libxlMakePCIList(def, d_config) < 0) return -1;
-- 1.8.4.5
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk wrote:
On Fri, May 08, 2015 at 04:31:02PM -0600, Jim Fehlig wrote:
For HVM domains, vfb info must be populated in the libxl_domain_build_info stuct. Currently this is done in the libxlMakeVfbList function, but IMO
struct
Thanks. I've fixed the typo in my local branch but will wait for additional comments before posting a V2 (if necessary). Regards, Jim
it would be cleaner to populate the build_info vfb in a separate libxlMakeBuildInfoVfb function. libxlMakeVfbList would then handle only vfb devices, simiar to the other libxlMake<device>List functions.
A future patch will extend libxlMakeBuildInfoVfb to support SPICE.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index fccada5..8552c77 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1323,37 +1323,6 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, d_config->vkbs = x_vkbs; d_config->num_vfbs = d_config->num_vkbs = nvfbs;
- /* - * VNC or SDL info must also be set in libxl_domain_build_info - * for HVM domains. Use the first vfb device. - */ - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - libxl_domain_build_info *b_info = &d_config->b_info; - libxl_device_vfb vfb = d_config->vfbs[0]; - - if (libxl_defbool_val(vfb.vnc.enable)) { - libxl_defbool_set(&b_info->u.hvm.vnc.enable, true); - if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0) - goto error; - if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0) - goto error; - b_info->u.hvm.vnc.display = vfb.vnc.display; - libxl_defbool_set(&b_info->u.hvm.vnc.findunused, - libxl_defbool_val(vfb.vnc.findunused)); - } else if (libxl_defbool_val(vfb.sdl.enable)) { - libxl_defbool_set(&b_info->u.hvm.sdl.enable, true); - libxl_defbool_set(&b_info->u.hvm.sdl.opengl, - libxl_defbool_val(vfb.sdl.opengl)); - if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0) - goto error; - if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0) - goto error; - } - - if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0) - goto error; - } - return 0;
error: @@ -1367,6 +1336,51 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, }
/* + * Populate vfb info in libxl_domain_build_info struct for HVM domains. + * Use first libxl_device_vfb device in libxl_domain_config->vfbs. + * Prior to calling this function, libxlMakeVfbList must be called to + * populate libxl_domain_config->vfbs. + */ +static int +libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +{ + libxl_domain_build_info *b_info = &d_config->b_info; + libxl_device_vfb x_vfb; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0; + + if (d_config->num_vfbs == 0) + return 0; + + x_vfb = d_config->vfbs[0]; + + if (libxl_defbool_val(x_vfb.vnc.enable)) { + libxl_defbool_set(&b_info->u.hvm.vnc.enable, true); + if (VIR_STRDUP(b_info->u.hvm.vnc.listen, x_vfb.vnc.listen) < 0) + return -1; + if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, x_vfb.vnc.passwd) < 0) + return -1; + b_info->u.hvm.vnc.display = x_vfb.vnc.display; + libxl_defbool_set(&b_info->u.hvm.vnc.findunused, + libxl_defbool_val(x_vfb.vnc.findunused)); + } else if (libxl_defbool_val(x_vfb.sdl.enable)) { + libxl_defbool_set(&b_info->u.hvm.sdl.enable, true); + libxl_defbool_set(&b_info->u.hvm.sdl.opengl, + libxl_defbool_val(x_vfb.sdl.opengl)); + if (VIR_STRDUP(b_info->u.hvm.sdl.display, x_vfb.sdl.display) < 0) + return -1; + if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, x_vfb.sdl.xauthority) < 0) + return -1; + } + + if (VIR_STRDUP(b_info->u.hvm.keymap, x_vfb.keymap) < 0) + return -1; + + return 0; +} + +/* * Get domain0 autoballoon configuration. Honor user-specified * setting in libxl.conf first. If not specified, autoballooning * is disabled when domain0's memory is set with 'dom0_mem'. @@ -1764,6 +1778,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, if (libxlMakeVfbList(graphicsports, def, d_config) < 0) return -1;
+ if (libxlMakeBuildInfoVfb(def, d_config) < 0) + return -1; + if (libxlMakePCIList(def, d_config) < 0) return -1;
-- 1.8.4.5
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
A later change will use the PortAllocator for SPICE too. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.h | 2 +- src/libxl/libxl_domain.c | 4 ++-- src/libxl/libxl_driver.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 0a1c0db..9c29b1e 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -140,7 +140,7 @@ struct _libxlDriverPrivate { virObjectEventStatePtr domainEventState; /* Immutable pointer, self-locking APIs */ - virPortAllocatorPtr reservedVNCPorts; + virPortAllocatorPtr reservedGraphicsPorts; /* Immutable pointer, self-locking APIs */ virPortAllocatorPtr migrationPorts; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 5f5f8e5..68dd28e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -715,7 +715,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, vm->def->graphics[0]->data.vnc.autoport) { vnc_port = vm->def->graphics[0]->data.vnc.port; if (vnc_port >= LIBXL_VNC_PORT_MIN) { - if (virPortAllocatorRelease(driver->reservedVNCPorts, + if (virPortAllocatorRelease(driver->reservedGraphicsPorts, vnc_port) < 0) VIR_DEBUG("Could not mark port %d as unused", vnc_port); } @@ -979,7 +979,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, VIR_FREE(managed_save_path); } - if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def, + if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, cfg->ctx, &d_config) < 0) goto cleanup; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 60c139e..730ca77 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -410,7 +410,7 @@ libxlStateCleanup(void) virObjectUnref(libxl_driver->config); virObjectUnref(libxl_driver->xmlopt); virObjectUnref(libxl_driver->domains); - virObjectUnref(libxl_driver->reservedVNCPorts); + virObjectUnref(libxl_driver->reservedGraphicsPorts); virObjectUnref(libxl_driver->migrationPorts); virLockManagerPluginUnref(libxl_driver->lockManager); @@ -523,7 +523,7 @@ libxlStateInitialize(bool privileged, } /* Allocate bitmap for vnc port reservation */ - if (!(libxl_driver->reservedVNCPorts = + if (!(libxl_driver->reservedGraphicsPorts = virPortAllocatorNew(_("VNC"), LIBXL_VNC_PORT_MIN, LIBXL_VNC_PORT_MAX, -- 1.8.4.5
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8552c77..5bb0425 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, /* * Populate vfb info in libxl_domain_build_info struct for HVM domains. - * Use first libxl_device_vfb device in libxl_domain_config->vfbs. - * Prior to calling this function, libxlMakeVfbList must be called to - * populate libxl_domain_config->vfbs. + * Prefer SPICE, otherwise use first libxl_device_vfb device in + * libxl_domain_config->vfbs. Prior to calling this function, + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs. */ static int -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports, + virDomainDefPtr def, + libxl_domain_config *d_config) { libxl_domain_build_info *b_info = &d_config->b_info; libxl_device_vfb x_vfb; + size_t i; if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) return 0; - if (d_config->num_vfbs == 0) + if (def->ngraphics == 0) return 0; + for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr l_vfb = def->graphics[0]; + unsigned short port; + const char *listenAddr; + + if (l_vfb->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) + continue; + + libxl_defbool_set(&b_info->u.hvm.spice.enable, true); + + if (l_vfb->data.spice.autoport) { + if (virPortAllocatorAcquire(graphicsports, &port) < 0) + return -1; + l_vfb->data.spice.port = port; + } + b_info->u.hvm.spice.port = l_vfb->data.spice.port; + + listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0); + if (VIR_STRDUP(b_info->u.hvm.spice.host, listenAddr) < 0) + return -1; + + if (VIR_STRDUP(b_info->u.hvm.keymap, l_vfb->data.spice.keymap) < 0) + return -1; + + if (l_vfb->data.spice.auth.passwd) { + if (VIR_STRDUP(b_info->u.hvm.spice.passwd, + l_vfb->data.spice.auth.passwd) < 0) + return -1; + libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, false); + } else { + libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, true); + } + + switch (l_vfb->data.spice.mousemode) { + /* client mouse mode is default in xl.cfg */ + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT: + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: + libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, true); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER: + libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, false); + break; + } + +#ifdef LIBXL_HAVE_SPICE_VDAGENT + if (l_vfb->data.spice.copypaste == VIR_TRISTATE_BOOL_YES) { + libxl_defbool_set(&b_info->u.hvm.spice.vdagent, true); + libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, true); + } else { + libxl_defbool_set(&b_info->u.hvm.spice.vdagent, false); + libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, false); + } +#endif + + return 0; + } + x_vfb = d_config->vfbs[0]; if (libxl_defbool_val(x_vfb.vnc.enable)) { @@ -1778,7 +1838,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, if (libxlMakeVfbList(graphicsports, def, d_config) < 0) return -1; - if (libxlMakeBuildInfoVfb(def, d_config) < 0) + if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0) return -1; if (libxlMakePCIList(def, d_config) < 0) -- 1.8.4.5
On 09.05.2015 00:31, Jim Fehlig wrote:
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8552c77..5bb0425 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
/* * Populate vfb info in libxl_domain_build_info struct for HVM domains. - * Use first libxl_device_vfb device in libxl_domain_config->vfbs. - * Prior to calling this function, libxlMakeVfbList must be called to - * populate libxl_domain_config->vfbs. + * Prefer SPICE, otherwise use first libxl_device_vfb device in + * libxl_domain_config->vfbs. Prior to calling this function, + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs. */ static int -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports, + virDomainDefPtr def, + libxl_domain_config *d_config) { libxl_domain_build_info *b_info = &d_config->b_info; libxl_device_vfb x_vfb; + size_t i;
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) return 0;
- if (d_config->num_vfbs == 0) + if (def->ngraphics == 0) return 0;
+ for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr l_vfb = def->graphics[0];
This seems really awkward to me. So you're using for() loop just to check if the first graphics card (assuming there can't be more than one anyway) is SPICE. If not, you could use 'continue' to continue with VNC. I think this obfucates the code. Just move this into a separate function and call it from here.
+ unsigned short port; + const char *listenAddr; + + if (l_vfb->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) + continue; + + libxl_defbool_set(&b_info->u.hvm.spice.enable, true); + + if (l_vfb->data.spice.autoport) { + if (virPortAllocatorAcquire(graphicsports, &port) < 0) + return -1; + l_vfb->data.spice.port = port; + } + b_info->u.hvm.spice.port = l_vfb->data.spice.port; + + listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0); + if (VIR_STRDUP(b_info->u.hvm.spice.host, listenAddr) < 0) + return -1; + + if (VIR_STRDUP(b_info->u.hvm.keymap, l_vfb->data.spice.keymap) < 0) + return -1; + + if (l_vfb->data.spice.auth.passwd) { + if (VIR_STRDUP(b_info->u.hvm.spice.passwd, + l_vfb->data.spice.auth.passwd) < 0) + return -1; + libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, false); + } else { + libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, true); + } + + switch (l_vfb->data.spice.mousemode) { + /* client mouse mode is default in xl.cfg */ + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT: + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: + libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, true); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER: + libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, false); + break; + } + +#ifdef LIBXL_HAVE_SPICE_VDAGENT + if (l_vfb->data.spice.copypaste == VIR_TRISTATE_BOOL_YES) { + libxl_defbool_set(&b_info->u.hvm.spice.vdagent, true); + libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, true); + } else { + libxl_defbool_set(&b_info->u.hvm.spice.vdagent, false); + libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, false); + } +#endif + + return 0; + } + x_vfb = d_config->vfbs[0];
if (libxl_defbool_val(x_vfb.vnc.enable)) { @@ -1778,7 +1838,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, if (libxlMakeVfbList(graphicsports, def, d_config) < 0) return -1;
- if (libxlMakeBuildInfoVfb(def, d_config) < 0) + if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0) return -1;
if (libxlMakePCIList(def, d_config) < 0)
Otherwise looking good. Michal
On May 28, 2015, at 4:07 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09.05.2015 00:31, Jim Fehlig wrote: Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8552c77..5bb0425 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
/* * Populate vfb info in libxl_domain_build_info struct for HVM domains. - * Use first libxl_device_vfb device in libxl_domain_config->vfbs. - * Prior to calling this function, libxlMakeVfbList must be called to - * populate libxl_domain_config->vfbs. + * Prefer SPICE, otherwise use first libxl_device_vfb device in + * libxl_domain_config->vfbs. Prior to calling this function, + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs. */ static int -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports, + virDomainDefPtr def, + libxl_domain_config *d_config) { libxl_domain_build_info *b_info = &d_config->b_info; libxl_device_vfb x_vfb; + size_t i;
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) return 0;
- if (d_config->num_vfbs == 0) + if (def->ngraphics == 0) return 0;
+ for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr l_vfb = def->graphics[0];
This seems really awkward to me. So you're using for() loop just to check if the first graphics card (assuming there can't be more than one anyway) is SPICE. If not, you could use 'continue' to continue with VNC. I think this obfucates the code. Just move this into a separate function and call it from here.
That's actually a bug, it should be def->graphics[i]. The idea is to prefer SPICE, but fall back to the first graphics device if no SPICE device is found. I mentioned this in the function comment. Perhaps that part of the comment should be moved to the for loop? Regards, Jim
+ unsigned short port; + const char *listenAddr; + + if (l_vfb->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) + continue; + + libxl_defbool_set(&b_info->u.hvm.spice.enable, true); + + if (l_vfb->data.spice.autoport) { + if (virPortAllocatorAcquire(graphicsports, &port) < 0) + return -1; + l_vfb->data.spice.port = port; + } + b_info->u.hvm.spice.port = l_vfb->data.spice.port; + + listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0); + if (VIR_STRDUP(b_info->u.hvm.spice.host, listenAddr) < 0) + return -1; + + if (VIR_STRDUP(b_info->u.hvm.keymap, l_vfb->data.spice.keymap) < 0) + return -1; + + if (l_vfb->data.spice.auth.passwd) { + if (VIR_STRDUP(b_info->u.hvm.spice.passwd, + l_vfb->data.spice.auth.passwd) < 0) + return -1; + libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, false); + } else { + libxl_defbool_set(&b_info->u.hvm.spice.disable_ticketing, true); + } + + switch (l_vfb->data.spice.mousemode) { + /* client mouse mode is default in xl.cfg */ + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT: + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: + libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, true); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER: + libxl_defbool_set(&b_info->u.hvm.spice.agent_mouse, false); + break; + } + +#ifdef LIBXL_HAVE_SPICE_VDAGENT + if (l_vfb->data.spice.copypaste == VIR_TRISTATE_BOOL_YES) { + libxl_defbool_set(&b_info->u.hvm.spice.vdagent, true); + libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, true); + } else { + libxl_defbool_set(&b_info->u.hvm.spice.vdagent, false); + libxl_defbool_set(&b_info->u.hvm.spice.clipboard_sharing, false); + } +#endif + + return 0; + } + x_vfb = d_config->vfbs[0];
if (libxl_defbool_val(x_vfb.vnc.enable)) { @@ -1778,7 +1838,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, if (libxlMakeVfbList(graphicsports, def, d_config) < 0) return -1;
- if (libxlMakeBuildInfoVfb(def, d_config) < 0) + if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0) return -1;
if (libxlMakePCIList(def, d_config) < 0)
Otherwise looking good.
Michal
On 28.05.2015 17:45, Jim Fehlig wrote:
On May 28, 2015, at 4:07 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09.05.2015 00:31, Jim Fehlig wrote: Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8552c77..5bb0425 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1337,22 +1337,82 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
/* * Populate vfb info in libxl_domain_build_info struct for HVM domains. - * Use first libxl_device_vfb device in libxl_domain_config->vfbs. - * Prior to calling this function, libxlMakeVfbList must be called to - * populate libxl_domain_config->vfbs. + * Prefer SPICE, otherwise use first libxl_device_vfb device in + * libxl_domain_config->vfbs. Prior to calling this function, + * libxlMakeVfbList must be called to populate libxl_domain_config->vfbs. */ static int -libxlMakeBuildInfoVfb(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports, + virDomainDefPtr def, + libxl_domain_config *d_config) { libxl_domain_build_info *b_info = &d_config->b_info; libxl_device_vfb x_vfb; + size_t i;
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) return 0;
- if (d_config->num_vfbs == 0) + if (def->ngraphics == 0) return 0;
+ for (i = 0; i < def->ngraphics; i++) { + virDomainGraphicsDefPtr l_vfb = def->graphics[0];
This seems really awkward to me. So you're using for() loop just to check if the first graphics card (assuming there can't be more than one anyway) is SPICE. If not, you could use 'continue' to continue with VNC. I think this obfucates the code. Just move this into a separate function and call it from here.
That's actually a bug, it should be def->graphics[i]. The idea is to prefer SPICE, but fall back to the first graphics device if no SPICE device is found. I mentioned this in the function comment. Perhaps that part of the comment should be moved to the for loop?
Yes, that sounds reasonable to me. Michal
libxl recently gained support for QXL video device. Support it in the libxl driver too. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- Note: Commit 161212ef in xen.git staging branch provides QXL support in libxl src/libxl/libxl_conf.c | 11 +++++++++++ src/libxl/libxl_domain.c | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 5bb0425..f2cffc7 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1736,6 +1736,17 @@ libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) } break; +#ifdef LIBXL_HAVE_QXL + case VIR_DOMAIN_VIDEO_TYPE_QXL: + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_QXL; + if (def->videos[0]->vram < 128 * 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("videoram must be at least 128MB for QXL")); + return -1; + } + break; +#endif + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("video type %s is not supported by libxl"), diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 68dd28e..c7f0ed9 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -324,6 +324,10 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, dev->data.video->vram = 4 * 1024; } break; + case VIR_DOMAIN_VIDEO_TYPE_QXL: + if (dev->data.video->vram == 0) + dev->data.video->vram = 128 * 1024; + break; } } -- 1.8.4.5
On 09.05.2015 00:31, Jim Fehlig wrote:
This series provides support for SPICE graphics in the libxl driver. The first patch is mostly code movement. The second patch is a trivial name change of a structure member. Patch3 populates the libxl_domain_build_info struct with SPICE info. SPICE isn't as nice without QXL, so patch4 provides support for QXL video device.
Jim Fehlig (4): libxl: populate build_info vfb in separate function libxl: change reservedVNCPorts to reservedGraphicsPorts libxl: support SPICE graphics for HVM domains libxl: support QXL video device
src/libxl/libxl_conf.c | 150 +++++++++++++++++++++++++++++++++++++---------- src/libxl/libxl_conf.h | 2 +- src/libxl/libxl_domain.c | 8 ++- src/libxl/libxl_driver.c | 4 +- 4 files changed, 128 insertions(+), 36 deletions(-)
ACK to all, but please see my comment to 3/4 before pushing. Even though we are in freeze, this has been lying around for a while therefore I think it's safe to push these. Michal
On 05/28/2015 05:10 AM, Michal Privoznik wrote:
On 09.05.2015 00:31, Jim Fehlig wrote:
This series provides support for SPICE graphics in the libxl driver. The first patch is mostly code movement. The second patch is a trivial name change of a structure member. Patch3 populates the libxl_domain_build_info struct with SPICE info. SPICE isn't as nice without QXL, so patch4 provides support for QXL video device.
Jim Fehlig (4): libxl: populate build_info vfb in separate function libxl: change reservedVNCPorts to reservedGraphicsPorts libxl: support SPICE graphics for HVM domains libxl: support QXL video device
src/libxl/libxl_conf.c | 150 +++++++++++++++++++++++++++++++++++++---------- src/libxl/libxl_conf.h | 2 +- src/libxl/libxl_domain.c | 8 ++- src/libxl/libxl_driver.c | 4 +- 4 files changed, 128 insertions(+), 36 deletions(-)
ACK to all, but please see my comment to 3/4 before pushing.
Even though we are in freeze, this has been lying around for a while therefore I think it's safe to push these.
Thanks for the review and finding the bug in 3/4! I changed it as we discussed and pushed the series. Regards, Jim
participants (3)
-
Jim Fehlig -
Konrad Rzeszutek Wilk -
Michal Privoznik