From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@eu.citrix.com>
Subject: Re: [PATCH 2/3] tools:libxl: Add qxl vga interface
 support v2
Date: Mon, 22 Oct 2012 15:36:24 +0100
Message-ID: <508559E8.10102@eu.citrix.com>
References: <CAAh7U5MHRnwwK-X==Hh7F2uy94i4yYU3oFFQEesowUtPQGf6Eg@mail.gmail.com>
	<1338987918.32319.84.camel@zakaz.uk.xensource.com>
	<CAAh7U5PYuJJcrAVCzVPLR0vZ1PLpS5mNqsrBmToTsm2KW46_wQ@mail.gmail.com>
	<1339049291.6557.11.camel@dagon.hellion.org.uk>
	<CAAh7U5O2FCeT7Mzp5KNkxqcX5B037YUmqef=TJTrQ9b=JgsHZA@mail.gmail.com>
	<1341325462.21547.20.camel@zakaz.uk.xensource.com>
	<CAFLBxZZfh0eXszeVvZ6WYDuGV6cDcvcgpOByf8FWz9EOvOUorQ@mail.gmail.com>
	<CAAh7U5PBASWB6Y6P-Sz3yg2Z1xv-JYAMtFSYGWe=3oov3h32dQ@mail.gmail.com>
	<CAAh7U5MMmAz047KzHVMRkUF=hGZKGfDfM1KM41jF8g6bZAXq7g@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAAh7U5MMmAz047KzHVMRkUF=hGZKGfDfM1KM41jF8g6bZAXq7g@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: ZhouPeng <zpengxen@gmail.com>
Cc: "Xen-Devel (E-mail)" <xen-devel@lists.xensource.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

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