Re: [Xen-devel] [PATCH QXL 2/2] libxl: Add qxl vga interface support.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-05-30 at 11:24 +0100, ZhouPeng wrote:
> Add qxl vga interface support.
> 
> Usage:
>   qxl=1
>   qxlvram=64
>   qxlram=64

Thanks, I don't think this is 4.2 material so this would have to wait
until the 4.3 dev cycle.

The patch needs to include documentation updates to docs/man/xl*.

> Signed-off-by: Zhou Peng <ailvpeng25@xxxxxxxxx>
> 
> diff -r c6641e3fe158 tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c  Mon May 28 16:25:59 2012 +0800
> +++ b/tools/libxl/libxl_dm.c  Wed May 30 17:48:38 2012 +0800
> @@ -181,6 +181,8 @@ static char ** libxl__build_device_model
>              flexarray_append(dm_args, "-std-vga");
>              break;
>          case LIBXL_VGA_INTERFACE_TYPE_DEFAULT:
> +            break;
> +        default:

Please enumerate the options rather than using default:, so we can catch
all the places when we add new options.

>              break;
>          }
> 
> @@ -426,6 +428,17 @@ static char ** libxl__build_device_model
>          switch (b_info->u.hvm.vga.type) {
>          case LIBXL_VGA_INTERFACE_TYPE_STD:
>              flexarray_vappend(dm_args, "-vga", "std", NULL);
> +            break;
> +        case LIBXL_VGA_INTERFACE_TYPE_QXL:

Since this appears to be qemu-xen only (not qemu-xen-traditional)
libxl__domain_build_info_setdefault should be checking for sane
combinations.

> +            flexarray_vappend(dm_args, "-vga", "qxl", NULL);
> +            flexarray_vappend(dm_args, "-global",
> +                          libxl__sprintf(gc, "qxl-vga.vram_size=%lu",

Using GCSPRINTF would shorten this line a bit.

> +                              b_info->u.hvm.vga.vramkb * 1024),
> +                          NULL);
> +            flexarray_vappend(dm_args, "-global",
> +                          libxl__sprintf(gc, "qxl-vga.ram_size=%lu",
> +                              b_info->u.hvm.vga.ramkb * 1024),
> +                          NULL);
>              break;
>          case LIBXL_VGA_INTERFACE_TYPE_DEFAULT:
>              break;
[...]
> @@ -137,6 +138,7 @@ libxl_vga_interface_info = Struct("vga_i
>  libxl_vga_interface_info = Struct("vga_interface_info", [
>      ("type",    libxl_vga_interface_type),
>      ("vramkb",  MemKB),
> +    ("ramkb",  MemKB),

These could do with comments to describe the difference between vram and
ram. Also how to they both differ from video_memkb?

>      ])
> 
>  libxl_vnc_info = Struct("vnc_info", [
> diff -r c6641e3fe158 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c        Mon May 28 16:25:59 2012 +0800
> +++ b/tools/libxl/xl_cmdimpl.c        Wed May 30 17:48:38 2012 +0800
> @@ -547,6 +547,29 @@ vcpp_out:
>      libxl_cpumap_dispose(&exclude_cpumap);
> 
>      return rc;
> +}
> +
> +static inline uint32_t msb_mask(uint32_t val)
> +{
> +    uint32_t mask;
> +
> +    fprintf(stdout, "%s:val: %u\n", __func__, val);

Please remove debugging print outs.

> +    do {
> +        mask = ~(val - 1) & val;
> +        val &= ~mask;
> +    } while (mask < val);
> +
> +    fprintf(stdout, "%s:mask: %u\n", __func__, mask);
> +    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);

There is a lot of magic constants and algorithms going on here -- at the
very least they need a comment to describe them...

> +
> +    return (vram + ram + 1023) / 1024;
>  }
> 
>  static void parse_config_data(const char *config_source,
> @@ -1262,6 +1285,27 @@ skip_vfb:
>              if (libxl_defbool_val(vga))
>                  b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_STD;
> 
> +        if (!xlu_cfg_get_defbool(config, "qxl", &vga, 0)) {
> +            if (libxl_defbool_val(vga)) {
> +                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;
> +                else
> +                    b_info->u.hvm.vga.vramkb = 64 * 1024;

Defaults should be set by the setdefaults function in libxl, unless xl
has some reason to override the libxl default.

> +                if (!xlu_cfg_get_long (config, "qxlram", &l, 0))
> +                    b_info->u.hvm.vga.ramkb = l * 1024;
> +                else
> +                    b_info->u.hvm.vga.ramkb = 64 * 1024;

Same here.

> +
> +                uint32_t qxl_ram = get_qxl_ram_size(b_info->u.hvm.vga.vramkb,
> +                                                    b_info->u.hvm.vga.ramkb);
> +                if ((b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> +                    || (b_info->video_memkb < qxl_ram)) {
> +                    b_info->video_memkb = qxl_ram;

I reckon this logic belongs in setdefaults too, although as above the
relationship between video_memkb and qxl_*ram needs explaining
somewhere.

> +                }
> +            }
> +        }
> +
>          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);
> 



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