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

[ZhouPeng <zpengxen@xxxxxxxxx>]

On Wed, Jun 6, 2012 at 9:05 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2012-06-05 at 12:22 +0100, ZhouPeng wrote:
>> changeset:   25454:1804a873a64d
>> tag:         tip
>> user:        Zhou Peng <ailvpeng25@xxxxxxxxx>
>> 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@xxxxxxxxx>
>
> 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.
>> + */
>> +static inline uint32_t msb_mask(uint32_t val)
>> +{
>> +    uint32_t mask;
>> +
>> +    do {
>> +        mask = ~(val - 1) & val;
>> +        val &= ~mask;
>> +    } while (mask < val);
>> +
>> +    return mask;
>> +}
>> +
>> +static inline uint32_t get_qxl_ram_size(uint32_t vram_sizekb,
>> +                                    uint32_t ram_sizekb)
>> +{
>> +    uint32_t vram = msb_mask(2 * vram_sizekb * 1024 - 1);
>> +    uint32_t ram = msb_mask(2 * ram_sizekb * 1024 - 1);
>
> Why 2*?

I don't know why yet, but it works this way in qxl.
>
>> +
>> +    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 it is == LIBXL_MEMKB_DEFAULT then of course you can feel free to
> override as necessary.
>
> e.g.
>               if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
>                   b_info->video_memkb = qxl_ram;
>               if (b_info->video_memkb < qxl_ram) {
>                   LOG(...)
>                   return ERROR_INVAL;
>               }
>
>> +            }
>> +        }
>> +
>>          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);
>> diff -r 7bd08f83a2ce -r 1804a873a64d tools/libxl/libxl_dm.c
>> --- a/tools/libxl/libxl_dm.c  Tue Jun 05 17:39:37 2012 +0800
>> +++ b/tools/libxl/libxl_dm.c  Tue Jun 05 17:56:46 2012 +0800
>> @@ -181,6 +181,8 @@ static char ** libxl__build_device_model
>>              flexarray_append(dm_args, "-std-vga");
>>              break;
>>          case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>> +            break;
>> +        case LIBXL_VGA_INTERFACE_TYPE_QXL:
>>              break;
>>          }
>>
>> @@ -429,6 +431,17 @@ static char ** libxl__build_device_model
>>              break;
>>          case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>>              flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
>> +            break;
>> +        case LIBXL_VGA_INTERFACE_TYPE_QXL:
>> +            flexarray_vappend(dm_args, "-vga", "qxl", NULL);
>> +            flexarray_vappend(dm_args, "-global",
>> +                              GCSPRINTF("qxl-vga.vram_size=%lu",
>> +                                        b_info->u.hvm.vga.vramkb * 1024),
>> +                              NULL);
>> +            flexarray_vappend(dm_args, "-global",
>> +                              GCSPRINTF("qxl-vga.ram_size=%lu",
>> +                                        b_info->u.hvm.vga.ramkb * 1024),
>> +                              NULL);
>>              break;
>>          }
>>
>> diff -r 7bd08f83a2ce -r 1804a873a64d tools/libxl/libxl_types.idl
>> --- a/tools/libxl/libxl_types.idl     Tue Jun 05 17:39:37 2012 +0800
>> +++ b/tools/libxl/libxl_types.idl     Tue Jun 05 17:56:46 2012 +0800
>> @@ -128,6 +128,7 @@ libxl_vga_interface_type = Enumeration("
>>  libxl_vga_interface_type = Enumeration("vga_interface_type", [
>>      (0, "CIRRUS"),
>>      (1, "STD"),
>> +    (2, "QXL"),
>>      ], init_val = "LIBXL_VGA_INTERFACE_TYPE_DEFAULT")
>>
>>  #
>> @@ -136,6 +137,10 @@ libxl_vga_interface_type = Enumeration("
>>
>>  libxl_vga_interface_info = Struct("vga_interface_info", [
>>      ("type",    libxl_vga_interface_type),
>> +    # VRAM bar for qxl
>> +    ("vramkb",  MemKB),
>> +    # RAM bar for qxl
>> +    ("ramkb",  MemKB),
>>      ])
>>
>>  libxl_vnc_info = Struct("vnc_info", [
>> diff -r 7bd08f83a2ce -r 1804a873a64d tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c        Tue Jun 05 17:39:37 2012 +0800
>> +++ b/tools/libxl/xl_cmdimpl.c        Tue Jun 05 17:56:46 2012 +0800
>> @@ -1260,6 +1260,16 @@ skip_vfb:
>>              if (l)
>>                  b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_STD;
>>
>> +        if (!xlu_cfg_get_long(config, "qxl", &l, 0)) {
>> +            if (l) {
>> +                b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_QXL;
>> +                if (!xlu_cfg_get_long (config, "qxlvram", &l, 0))
>> +                    b_info->u.hvm.vga.vramkb = l * 1024;
>> +                if (!xlu_cfg_get_long (config, "qxlram", &l, 0))
>> +                    b_info->u.hvm.vga.ramkb = l * 1024;
>> +            }
>> +        }
>> +
>>          xlu_cfg_get_defbool(config, "vnc", &b_info->u.hvm.vnc.enable, 0);
>>          xlu_cfg_replace_string (config, "vnclisten",
>>                                  &b_info->u.hvm.vnc.listen, 0);
>>
>>
>
>



-- 
Zhou Peng

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