Re: libxl videoram for cirrus graphics

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2013-09-17 at 14:29 +0100, Rob Hoes wrote:
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: 17 September 2013 2:18 PM
> > To: Rob Hoes
> > Cc: xen-devel@lists.xen.org; Stefano Stabellini; Ian Jackson
> > Subject: Re: [Xen-devel] libxl videoram for cirrus graphics
> > 
> > On Tue, 2013-09-17 at 12:54 +0000, Rob Hoes wrote:
> > > Hi,
> > >
> > > The videoram setting in xl config files is documented as follows:
> > >
> > > "
> > > videoram=MBYTES
> > >
> > >     Sets the amount of RAM which the emulated video card will contain,
> > > which in turn limits the resolutions and bit depths which will be
> > > available. The default amount of video ram for stdvga is 8MB which is
> > > sufficient for e.g. 1600x1200 at 32bpp and videoram option is
> > > currently working only when using the qemu-xen-traditional
> > > device-model.
> > >
> > >     When using the emulated Cirrus graphics card (vga="cirrus") the
> > > amount of video ram is fixed at 4MB which is sufficient for 1024x768
> > > at 32 bpp and videoram option is currently working only when using the
> > > upstream qemu-xen device-model.
> > > "
> > >
> > > XenServer also uses a default of 4MB video ram for cirrus graphics.
> > >
> > > However, when I use xl to create a VM with cirrus graphics, I get 8MB
> > > of video RAM by default. Moreover, setting it to videoram=4 in the
> > > config file leads to complaints from libxl:
> > >
> > >     libxl_create.c:228:libxl__domain_build_info_setdefault: videoram
> > > must be at least 8 mb
> > >
> > > Is there a bug in the logic in libxl_create.c?
> > 
> > Either the docs of the code or both would seem to be wrong. Since you say
> > you happily use 4MB in XenServer I guess it is both?
> > 
> > I noticed that the docs also say the amount is fixed, but that's not what the
> > code implements either..
> 
> Yes, as far as I can see, XenServer has always used 4MB by default, and allows one to override it as well (so the amount is not fixed). Not allowing 4MB anymore, as well as not allowing the override, may therefore lead to upgrade problems for XenServer users when we switch to using libxl, so I am hoping that we can make the checks in libxl a little less strict.
> 
> I think that something like this would work:
> 
> diff -r ba248e555458 tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -213,8 +213,13 @@
>          if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
>              b_info->shadow_memkb = 0;
>  
> -        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_STD &&
> -            b_info->device_model_version ==
> +        if (!b_info->u.hvm.vga.kind)
> +            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +
> +        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_CIRRUS) {
> +            if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> +                b_info->video_memkb = 4 * 1024;
> +        } else if (b_info->device_model_version ==
>              LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
>                  if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
>                      b_info->video_memkb = 16 * 1024;

Do the following checks for b_info->video_memkb == LIBXL_MEMKB_DEFAULT
and b_info->video_memkb < (8 * 1024) not need modifying then?

Is the behaviour here any different to just changing the 8s into 4s?

Can you also update the docs, it's at least wrong about the 4MB being a
fixed value.

> @@ -251,8 +256,6 @@
>              if (!b_info->u.hvm.boot) return ERROR_NOMEM;
>          }
>  
> -        if (!b_info->u.hvm.vga.kind)
> -            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>          libxl_defbool_setdefault(&b_info->u.hvm.vnc.enable, true);
>          if (libxl_defbool_val(b_info->u.hvm.vnc.enable)) {
>              libxl_defbool_setdefault(&b_info->u.hvm.vnc.findunused, true);
> 
> > I'm not really sure who, if anyone, might know definitively what is going on
> > here. Stefano has some involvement in this video mem stuff once upon a
> > time and Ian is the qemu-trad maintainer, so I've CCdthem both ;-)
> > 
> > >
> > > Cheers,
> > > Rob
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> > 
>