Re: [Xen-devel] [PATCH] libxl: Implement basic video device selection

[Jim Fehlig <jfehlig@xxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel