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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 18/10/12 11:19, ZhouPeng wrote:
> v4
>
> test and fix conflict with the latest Xen
> -----
>
>
> tools:libxl: Add qxl vga interface support for upstream-qemu-xen.
>
> Usage:
>    qxl=1|0
>
> Signed-off-by: Zhou Peng <ailvpeng25@gmail.com>

Thanks, Zhou Peng.  Just a couple of comments below:

>
> diff -r c1c549c4fe9e docs/man/xl.cfg.pod.5
> --- a/docs/man/xl.cfg.pod.5	Mon Oct 15 16:51:44 2012 +0100
> +++ b/docs/man/xl.cfg.pod.5	Thu Oct 18 17:53:25 2012 +0800
> @@ -930,10 +930,13 @@ in the B<VFB_SPEC_STRING> for configurin
>
>   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. This option is only available when using the B<stdvga>
> -option (see below).
> +available. This option is only available when using the B<stdvga> and
> +B<qxl> option (see below).
>   The default amount of video ram for stdvga is 8MB which is sufficient
>   for e.g. 1600x1200 at 32bpp.
> +For B<qxl> option, the default is 128MB. If B<videoram> is set greater
> +than 128MB, it will be trimmed to 128MB; if set less than 128MB, an
> +error will be triggered.

Is this a fixed value in qemu, or is this something that can be changed 
via the command-line?

> +
> +        if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> +            && b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_QXL) {
> +            /*
> +             * QXL needs 128 Mib video ram by default.
> +             */
> +            if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) {
> +                b_info->video_memkb = 128 * 1024;
> +            }
> +            if (b_info->video_memkb < 128 * 1024) {
> +                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> +                    "128 Mib videoram is necessary for qxl default");
> +                return ERROR_INVAL;
> +            }
> +            if (b_info->video_memkb > 128 * 1024) {
> +                b_info->video_memkb = 128 * 1024;
> +                LIBXL__LOG(CTX, LIBXL__LOG_WARNING,
> +                            "trim videoram to qxl default: 128 Mib");
> +            }
> +        }

It would be better to have a single #define for the required QXL video 
mem size, rather than duplicating "128*1024" several times.

Other than that, looks pretty good to me.

Thanks,
  -George