Re: [Xen-devel] [libvirt] [PATCH 1/2] libxl: Implement basic video device selection

[Jim Fehlig <jfehlig@xxxxxxxx>]

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

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