Re: libxl videoram for cirrus graphics

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

On Tue, 2013-09-17 at 15:52 +0100, Rob Hoes wrote:
> > > 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?
> 
> I was assuming that we need to keep the default of 8MB, as well as the
> restriction to have at least 8MB, for standard vga (when kind !=
> CIRRUS)

Oh right, this nest of twisted conditionals had me confused.I thought at
first it could be made a switch but then realised it was testing
different properties down the if/else chain...

I wonder if it might be better to duplication some code for clarities
sake:

switch(dm_versioN)
case LIBXL...QEMU_TRAD:
    switch (vga.kind):
                case CIRRUS:
                        if videomembk == default
                                videomemkb= 4
                        if videomemkbv < 4
                                error
                break
                case STDVGA:
                        some stuff
                break
        
case LIBXL..QEMU_XEN
     switch(vga.kind)
                etc duplicating some of the above with default of 8
        etc
        
or maybe it would be even clearer to refactor into
libxl__video_{cirrus,stdvga}_set defaults?

> > Can you also update the docs, it's at least wrong about the 4MB being a fixed
> > value.
> 
> Yes, I'll send a new patch.
> 
> Rob
> 
> > > @@ -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
> > > >
> > >
> > 
>