Re: [PATCH] libxl: new parameters for upstream qemu's vga interfaces

[Fabio Fantoni <fabio.fantoni@m2r.biz>]

Il 10/10/2013 12:43, Stefano Stabellini ha scritto:
> On Thu, 10 Oct 2013, Fabio Fantoni wrote:
>> Il 10/10/2013 11:32, Ian Campbell ha scritto:
>>> On Thu, 2013-10-10 at 11:26 +0200, Fabio Fantoni wrote:
>>>> Il 30/09/2013 12:12, Fabio Fantoni ha scritto:
>>>>> Change the qemu parameters for upstream qemu vgas to the
>>>>> new ones (-device), introduced some years ago.
>>>> Ping
>>> Please CC the qemu maintainers (Stefano & Anthony), although these
>>> patches touch the toolstack they are logically qemu patches and need
>>> their ack.
>> Thanks for your reply, I added Anthony on CC and Stefano was already on it.
>>
>>>>> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
>>>>> ---
>>>>>     tools/libxl/libxl_dm.c |   12 +++++-------
>>>>>     1 file changed, 5 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>>>>> index 43c3bec..2c6f5d9 100644
>>>>> --- a/tools/libxl/libxl_dm.c
>>>>> +++ b/tools/libxl/libxl_dm.c
>>>>> @@ -486,15 +486,13 @@ static char **
>>>>> libxl__build_device_model_args_new(libxl__gc *gc,
>>>>>                switch (b_info->u.hvm.vga.kind) {
>>>>>             case LIBXL_VGA_INTERFACE_TYPE_STD:
>>>>> -            flexarray_vappend(dm_args, "-vga", "std", NULL);
>>>>> +            flexarray_append_pair(dm_args, "-device", "VGA");
>>>>>                 break;
>>>>>             case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>>>>> -            flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
>>>>> -            if (b_info->video_memkb) {
>>>>> -                flexarray_vappend(dm_args, "-global",
>>>>> -                    GCSPRINTF("vga.vram_size_mb=%d",
>>>>> -                    libxl__sizekb_to_mb(b_info->video_memkb)), NULL);
>>>>> -            }
>>>>> +            flexarray_append_pair(dm_args, "-device", "cirrus-vga");
>>>>> +            flexarray_append_pair(dm_args, "-global",
>>>>> +                GCSPRINTF("vga.vram_size_mb=%d",
>>>>> +                libxl__sizekb_to_mb(b_info->video_memkb)));
>>>>>                 break;
>>>>>             }
> Aside from switching to the new parameter format, this patch also
> removes the
>
> if (b_info->video_memkb) {
>
> check before passing the -global paramter. Is it deliberate?
> If so, it would be best to write down the motivation in the commit
> message.
> That said, the parameter change looks OK.
>

I removed the "if" after seeing that it isn't necessary because 
b_info->video_memkb is always set (also without videoram parameter on 
xl.cfg file).
I already tested it in both cases and I didn't find any problem.