Re: [PATCH v8] tools/libxl: Add qxl vga interface support for upstream-qemu-xen.

[Fabio Fantoni <fantonifabio@tiscali.it>]

[-- Attachment #1.1: Type: text/plain, Size: 6281 bytes --]

Il 05/02/2013 11:33, Ian Campbell ha scritto:
> On Mon, 2013-01-28 at 16:03 +0000, Fabio Fantoni wrote:
>> tools/libxl: Add qxl vga interface support for
>>    upstream-qemu-xen.
>>
>> Usage:
>>     qxl=1|0
>>
>> Changes from v7:
>> - Fix videoram settings parameters for qemu.
>>
>> Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it>
>> Signed-off-by: Zhou Peng <zpengxen@gmail.com>
> Unfortunately this patch is whitespace damaged in various places.

I'll try from linux instead windows, I can also add patches as 
attachment on next messages?

>> ---
>>    docs/man/xl.cfg.pod.5       |   11 +++++++++++
>>    tools/libxl/libxl_create.c  |   12 ++++++++++++
>>    tools/libxl/libxl_dm.c      |   15 +++++++++++++++
>>    tools/libxl/libxl_types.idl |    1 +
>>    tools/libxl/xl_cmdimpl.c    |    7 ++++++-
>>    5 files changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 9c5cdcd..a0f0dc3 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -984,6 +984,9 @@ the amount of video ram is fixed at 4MB which is
>> sufficient
>>    for 1024x768 at 32 bpp and videoram option is currently working
>>    only when using the upstream qemu-xen device-model.
>>
>> +For B<qxl> vga, the default is both default and minimal 128MB.
>> +If B<videoram> is set less than 128MB, an error will be triggered.
>> +
>>    =item B<stdvga=BOOLEAN>
>>
>>    Select a standard VGA card with VBE (VESA BIOS Extensions) as the
>> @@ -992,6 +995,14 @@ a Cirrus Logic GD5446 VGA card. If your guest
>> supports VBE 2.0 or
>>    later (e.g. Windows XP onwards) then you should enable this.
>>    stdvga supports more video ram and bigger resolutions than Cirrus.
>>
>> +=item B<qxl=BOOLEAN>
> What happens if I give qxl=1 and stdvga=1?
>
> Perhaps we should deprecate stdvga and add a new option:
> 	vga = "stdvga|cirrus|qxl"
> ?

Yes that should be nice.
I'll do a patch that remove stdvga option and add vga option.

>> +
>> +Select a QXL VGA card as the emulated graphics device.
>> +In general, QXL should work with the Spice remote display protocol
>> +for acceleration, and QXL driver is necessary in guest in this case.
> Do we have any docs on where to obtain this driver and how to install
> it? On the wiki perhaps, a link would be useful.
>
>> +QXL can also work with the VNC protocol, but it will be like a standard
>> +VGA without acceleration.
>> +
>>    =item B<vnc=BOOLEAN>
>>
>>    Allow access to the display via the VNC protocol.  This enables the
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 55014e5..4761b5a 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -197,6 +197,18 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>        case LIBXL_DOMAIN_TYPE_HVM:
>>            if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
>>                b_info->shadow_memkb = 0;
>> +
>> +        if (b_info->device_model_version ==
>> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
>> +            && b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_QXL) {
>> +            if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) {
>> +                b_info->video_memkb = (128 * 1024);
>> +            }else if (b_info->video_memkb < (128 * 1024)) {
>> +                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
>> +                    "128 Mib videoram is the minimum for qxl default");
> You can use the LOG() macros to shorten this line (and in other places
> including you videoram patch too).
>
> Should this error out on qemu == traditional and vga == QXL?

Must I only replace LIBXL__LOG with LOG?

I'll add error and exit if vga=qxl and qemu is traditional.

>> +                return ERROR_INVAL;
>> +            }
>> +        }
>> +
>>            if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
>>                b_info->video_memkb = 8 * 1024;
>>            else if (b_info->video_memkb < 8192){
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 465b1fd..0813258 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -437,6 +439,19 @@ static char **
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>                    NULL);
>>                }
>>                break;
>> +        case LIBXL_VGA_INTERFACE_TYPE_QXL:
>> +            flexarray_vappend(dm_args, "-vga", "qxl", NULL);
> flexarray_append_pair() ?
>> +            if (b_info->video_memkb) {
>> +                flexarray_vappend(dm_args, "-global",
>> +                libxl__sprintf(gc, "qxl-vga.vram_size_mb=%lu",
>> +                (b_info->video_memkb/2/1024)),
>> +                NULL);
>> +                flexarray_vappend(dm_args, "-global",
>> +                libxl__sprintf(gc, "qxl-vga.ram_size_mb=%lu",
>> +                (b_info->video_memkb/2/1024)),
>> +                NULL);
> You could combine the second two vappends into a single one, or use
> flexarray_append_pair().
>
> The /2 is because the videomem is split between vram and ram? A comment
> to that affect would be useful.

I'll try to use flexarray_append_pair() instead.
I'll also add small comment about 2 qxl ram regions.

>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 080bbd8..b4f7a0e 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1469,7 +1469,12 @@ skip_vfb:
>>    #undef parse_extra_args
>>
>>        if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>> -        if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
>> +        if (!xlu_cfg_get_long(config, "qxl", &l, 0))
>> +            b_info->u.hvm.vga.kind = l ? LIBXL_VGA_INTERFACE_TYPE_QXL :
>> + LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>> +
>> +        if (!xlu_cfg_get_long(config, "stdvga", &l, 0) &&
>> +            b_info->u.hvm.vga.kind != LIBXL_VGA_INTERFACE_TYPE_QXL)
>>                b_info->u.hvm.vga.kind = l ? LIBXL_VGA_INTERFACE_TYPE_STD :
>> LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>>
>
>
>
> -----
> Nessun virus nel messaggio.
> Controllato da AVG - www.avg.com
> Versione: 2013.0.2897 / Database dei virus: 2639/6080 -  Data di rilascio: 04/02/2013
>
>



[-- Attachment #1.2: Firma crittografica S/MIME --]
[-- Type: application/pkcs7-signature, Size: 4510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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