Hi Stefan,
Thanks for the patches, sorry for the delay...
Stefan Bader wrote:
> 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]
>
> Signed-off-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> ---
> src/libxl/libxl_conf.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b7fed7f..2b5c469 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1304,6 +1304,64 @@ libxlMakeCapabilities(libxl_ctx *ctx)
> return NULL;
> }
>
> +static int
> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> + libxl_domain_build_info *b_info = &d_config->b_info;
> +
> + /*
> + * 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;
> + } else {
> + libxl_defbool_set(&b_info->u.hvm.nographic, 1);
> + }
>
This part configures a video device.
> +
> + /*
> + * When making the list of displays, only VNC and SDL types were
> + * taken into account. So it seems sensible to connect the default
> + * video device to the first in the vfb list.
> + */
> + if (d_config->num_vfbs) {
> + libxl_device_vfb *vfb0 = &d_config->vfbs[0];
> +
> + b_info->u.hvm.vnc = vfb0->vnc;
> + if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen) < 0)
> + return -1;
> + if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd) < 0)
> + return -1;
> + b_info->u.hvm.sdl = vfb0->sdl;
> + if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display) < 0)
> + return -1;
> + if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority) < 0)
> + return -1;
> + if (VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
>
And this part configures a frame buffer. I think it would be better for
this part to be in libxlMakeVfbList(), similar to commit b55cc5f4. This
function then becomes libxlMakeVideo(), continuing the libxlMake<Device>
pattern.
Regards,
Jim
> +
> int
> libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> virDomainObjPtr vm, libxl_domain_config *d_config)
> @@ -1331,6 +1389,16 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> 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 (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
> + if (libxlSetBuildGraphics(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