Re: [PATCH 2/3] tools:libxl: Add qxl vga interface support v2

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

On Thu, 2012-06-07 at 04:19 +0100, ZhouPeng wrote:
> On Wed, Jun 6, 2012 at 9:05 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2012-06-05 at 12:22 +0100, ZhouPeng wrote:
> >> changeset:   25454:1804a873a64d
> >> tag:         tip
> >> user:        Zhou Peng <ailvpeng25@gmail.com>
> >> date:        Tue Jun 05 17:56:46 2012 +0800
> >> files:       tools/libxl/libxl_create.c tools/libxl/libxl_dm.c
> >> tools/libxl/libxl_types.idl tools/libxl/xl_cmdimpl.c
> >> description:
> >> tools:libxl: Add qxl vga interface support.
> >>
> >> Usage:
> >>  qxl=1
> >>  qxlvram=64
> >>  qxlram=64
> >>
> >> Signed-off-by: Zhou Peng <ailvpeng25@gmail.com>
> >
> > Thanks.
> >
> > As previously mentioned this is  4.3 material. Please can you resubmit
> > once the 4.3 dev cycle opens.
> ok, thanks.
> >
> >> diff -r 7bd08f83a2ce -r 1804a873a64d tools/libxl/libxl_create.c
> >> --- a/tools/libxl/libxl_create.c      Tue Jun 05 17:39:37 2012 +0800
> >> +++ b/tools/libxl/libxl_create.c      Tue Jun 05 17:56:46 2012 +0800
> >> @@ -23,6 +23,32 @@
> >>  #include <xc_dom.h>
> >>  #include <xenguest.h>
> >>
> >> +/*
> >> + * For qxl vga interface, the total video mem is determined by
> >> + * RAM bar and VRAM bar. But it is not simply linearly determined,
> >> + * get_qxl_ram_size below gives the details.
> >
> > From this statement I expected get_qxl_ram_size to have a nice comment
> > explaining what is going on, but it doesn't have this.
> >
> > Can you please explain somewhere how this value is determined (I can see
> > it is not simple ;-)). Perhaps a link to some QXL/qemu document
> > discussing these parameters would be helpful too?
> 
> Sorry, there is not a piece of docs on ram bar and vram bar, and how
> the total size
> of qxl video memory is calculated from ram bar and vram bar parameters.
> 
> But from my digging into qxl's source code and debugging, it works this way.
> 
> I asked similar question in spice-list and get response from:
> http://comments.gmane.org/gmane.comp.emulators.spice.devel/9501
> 
> Any way, I will rich the document if get more info.

OK, thanks.

> >> +
> >> +    return (vram + ram + 1023) / 1024;
> >> +}
> >> +
> >>  void libxl_domain_config_init(libxl_domain_config *d_config)
> >>  {
> >>      memset(d_config, 0, sizeof(*d_config));
> >> @@ -195,6 +221,25 @@ int libxl__domain_build_info_setdefault(
> >>
> >>          if (b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_DEFAULT)
> >>              b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> >> +        if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> >> +            && b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_QXL) {
> >> +            if (b_info->u.hvm.vga.vramkb == LIBXL_MEMKB_DEFAULT)
> >> +                b_info->u.hvm.vga.vramkb = 64 * 1024;
> >> +            if (b_info->u.hvm.vga.ramkb == LIBXL_MEMKB_DEFAULT)
> >> +                b_info->u.hvm.vga.ramkb = 64 * 1024;
> >> +            uint32_t qxl_ram = get_qxl_ram_size(b_info->u.hvm.vga.vramkb,
> >> +                                                b_info->u.hvm.vga.ramkb);
> >> +            /*
> >> +             * video_memkb is the real size of video memory to assign.
> >> +             * If video_memkb can't meet the need of qxl, adjust it
> >> +             * accordingly.
> >> +             */
> >> +            if ((b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> >> +                || (b_info->video_memkb < qxl_ram)) {
> >> +                b_info->video_memkb = qxl_ram;
> >
> > If video_memkb is != LIBXL_MEMKB_DEFAULT and it is not sufficiently
> > large then I think the correct thing to do is to error out and return
> > ERROR_INVAL.
> 
> Not agreed.
> This will let user must to set a proper value to meet qxl, but from
> discussing above, it's difficult for user to give this decision.
> qxlram and qxlvram parameters are enough  for user to set qxl's video
> ram (libvirt also use these two parameters).

If the user has asked for a specific amount of video RAM which is not
sufficient then the correct answer is to log an error (including the
required minimum) and exit.

You are correct that it is hard to figure out what "enough" RAM is. I
expect that for that reason almost all users won't pass any of these
arguments and will just accept the default, which will work just fine.

Ian.