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

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

Il 02/05/2014 22:04, Fabio Fantoni ha scritto:
> 2014-05-02 21:44 GMT+02:00 Don Slutz <dslutz@verizon.com 
> <mailto: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
>     <mailto: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).
>
>
> Thanks for your reply, probably with my bad english I not understand 
> good this part of your reply.
> I changed to -device following this official qemu doc:
> http://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/qdev-device-use.txt;hb=master
> I did several tests of additional vga's parameters without using 
> the-global in recent days with cirrus, stdvga and qxl, I've never seen 
> errors and the amount of videoram by domUs seem ok.

Ping

>
>
>         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
>             <mailto: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 <mailto:Xen-devel@lists.xen.org>
>         http://lists.xen.org/xen-devel
>
>
>