Stefan Bader wrote:
> being as bad with timely responses. Ok, so how about the following?
>
> One note: it could be the STRDUP's are not strictly needed. But
> to me it felt wrong to have two places refer to the same strings
> (as MakeVFB copies the struct containing the pointers).
Agreed. Without the STRDUP's, seems there is a potential for double
free when libxl_device_vfb and libxl_domain_config objects are disposed.
> If this
> is not needed, then all changes now in MakeVFB probably can be
> dropped (except setting the keyboard layout, maybe; which I
> might miss ;)).
>
> -Stefan
>
>
> >From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> Date: Thu, 27 Mar 2014 16:01:18 +0100
> Subject: [PATCH] libxl: Implement basic video device selection
>
> This started as an investigation into an issue where libvirt (using the
> libxl driver) and the Xen host, like an old couple, could not agree on
> who is responsible for selecting the VNC port to use.
>
> Things usually (and a bit surprisingly) did work because, just like that
> old couple, they had the same idea on what to do by default. However it
> was possible that this ended up in a big argument.
>
> The problem is that display information exists in two different places:
> in the vfbs list and in the build info. And for launching the device model,
> only the latter is used. But that never gets initialized from libvirt. So
> Xen allows the device model to select a default port while libvirt thinks
> it has told Xen that this is done by libvirt (though the vfbs config).
>
> While fixing that, I made a stab at actually evaluating the configuration
> of the video device. So that it is now possible to at least decide between
> a Cirrus or standard VGA emulation and to modify the VRAM within certain
> limits using libvirt.
>
> [v2: Check return code of VIR_STRDUP and fix indentation]
> [v3: Split out VRAM fixup and return error for unsupported video type]
> [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
>
[meta-comment]
libvirt prefers patch version history like this to be below the '---'
following your Signed-off-by, so as to not pollute the commit message.
> Signed-off-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> ---
> src/libxl/libxl_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 8eeaf82..43cabcf 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
> 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))
> + if (libxl_defbool_val(vfb.vnc.enable)) {
> memcpy(&b_info->u.hvm.vnc, &vfb.vnc, sizeof(libxl_vnc_info));
> - else if (libxl_defbool_val(vfb.sdl.enable))
> + 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;
> + } else if (libxl_defbool_val(vfb.sdl.enable)) {
> memcpy(&b_info->u.hvm.sdl, &vfb.sdl, sizeof(libxl_sdl_info));
> + 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;
> @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
> return NULL;
> }
>
> +static int
> +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> + libxl_domain_build_info *b_info = &d_config->b_info;
> +
> + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM)
> + return 0;
> +
> + /*
> + * Take the first defined video device (graphics card) to display
> + * on the first graphics device (display).
> + * Right now only type and vram info is used and anything beside
> + * type xen and vga is mapped to cirrus.
> + */
> + if (def->nvideos) {
> + switch (def->videos[0]->type) {
> + case VIR_DOMAIN_VIDEO_TYPE_VGA:
> + case VIR_DOMAIN_VIDEO_TYPE_XEN:
> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
> + break;
> + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> + break;
> + default:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s",
> + _("video type not supported by libxl"));
> + return -1;
> + }
> + b_info->video_memkb = def->videos[0]->vram ?
> + def->videos[0]->vram :
> + LIBXL_MEMKB_DEFAULT;
>
While testing this, I noticed that libvirt will set vram to 9216 if not
specified. E.g.
# cat test.xml
...
<video>
<model type='vga'/>
</video>
...
# virsh define test.xml
# virsh dumpxml test
...
<video>
<model type='vga' vram='9216' heads='1'/>
</video>
...
With type='vga', libxl will then fail to start the domain
libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
videoram must be at least 16 MB for STDVGA on QEMU_XEN
This could be handled in libxlDomainDeviceDefPostParse(), where we can
check for sane vram values for the various VIR_DOMAIN_VIDEO_TYPE_*, or
set sane defaults if vram is not specified.
BTW, sorry again for the delayed response. I somehow missed your
message and only stumbled across it today :-/. And be warned that any
followup may be delayed as I'll be on vacation July 18-27.
Regards,
Jim
> + } else {
> + libxl_defbool_set(&b_info->u.hvm.nographic, 1);
> + }
> +
> + return 0;
> +}
> +
> int
> libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> virDomainDefPtr def,
> @@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> if (libxlMakePCIList(def, d_config) < 0)
> return -1;
>
> + /*
> + * Now that any potential VFBs are defined, it is time to update the
> + * build info with the data of the primary display. Some day libxl
> + * might implicitely do so but as it does not right now, better be
> + * explicit.
> + */
> + if (libxlMakeVideo(def, d_config) < 0)
> + return -1;
> +
> d_config->on_reboot = def->onReboot;
> d_config->on_poweroff = def->onPoweroff;
> d_config->on_crash = def->onCrash;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list