Re: [PATCH] libxl: fix cirrus vga video memory setting with upstream qemu

[Don Slutz <dslutz@verizon.com>]

On 05/02/14 07:41, Ian Campbell wrote:
> On Sat, 2014-04-19 at 14:16 +0200, Fabio Fantoni wrote:
>> Reading one qemu-devel post seems that setting video memory of
>> cirrus vga with upstream qemu is wrong even if not show errors.

Fabio,
   You can add my:

Reviewed-by: Don Slutz <dslutz@verizon.com>

I have a similar code change locally (part of my pending
list of to dos) (I just changed the global arg...).

> You later provided links but I think the conversation should be
> referenced here.

I was part of the conversation.  When I was looking into upstreaming
a change I have (pci_min_hole) to xen & qemu, I was asked by QEMU
to report if it was not used (i.e. non x86 cpu's).  While I was testing my
change to QEMU under xen I noticed:

Warning: "-global vga.vram_size_mb=16" not used


In /var/log/xen/qemu-dm-<guest>.log

Here is the cross post to xen-devel:


  [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global


http://lists.xen.org/archives/html/xen-devel/2014-03/msg03128.html



> Is this change correct for all versions of mainline qemu which people
> might be using with Xen?

What I know is that "-global cirrus-vga.vgamem_mb=32" does work
with upstream QEMU 1.5.0, 1.6.0, 1.7.0 and 2.0.0.  I had added a debug
output into the QEMU version above that show the amount of video ram
that gets allocated and so the testing was quick and easy.  My
understanding of QEMU is that when specified this way:

  "-device cirrus-vga,vgamem_mb=32"

the error checking in QEMU will report when it does not like it:

qemu-system-x86_64: Property '.vram_size_mb' not found


(unlike -global).  So while I have not tested it, I would guess that
any QEMU that accepts "-device cirrus-vga" will also either
accept the change or report and error and not start. (Note: the
change from "-vga cirrus" to "-device cirrus-vga" was done since
4.3.0 and has yet to generate a bug).

> Please can you also explain what "wrong" means? Does it crash? Does it
> silently ignore the setting? Does it do something else "wrong"? How bad
> is it?

For me it just silently ignores the setting.  Which means that the
cirrus vga has the default memory and so will not support any
display setting that needs more memory (the most likely reason
videoram=32 is specified in the config file).

I feel it is bad enough that a backport to 4.4 makes sense to
me.

Hope this helps.
     -Don Slutz

>> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
>>
>> ---
>>
>> Note:
>> - when this patch will be accepted upstream should be
>>    backported also on xen 4.4
> I think this very much depends on what you mean by "wrong" above.
>
>> - with this qemu parameters seems correct but for further
>>    confirmation I posted a question about it:
>> http://lists.xen.org/archives/html/xen-devel/2014-04/msg02606.html
> Any reply to this question?
>
> Anthony, Stefano: Opinions on this change?
>
>> ---
>>   tools/libxl/libxl_dm.c |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 8abed7b..90f19b7 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -508,9 +508,8 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>               flexarray_append_pair(dm_args, "-device", "VGA");
>>               break;
>>           case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>> -            flexarray_append_pair(dm_args, "-device", "cirrus-vga");
>> -            flexarray_append_pair(dm_args, "-global",
>> -                GCSPRINTF("vga.vram_size_mb=%d",
>> +            flexarray_append_pair(dm_args, "-device",
>> +                GCSPRINTF("cirrus-vga,vgamem_mb=%d",
>>                   libxl__sizekb_to_mb(b_info->video_memkb)));
>>               break;
>>           case LIBXL_VGA_INTERFACE_TYPE_NONE:
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel