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

[Fabio Fantoni <fabio.fantoni@xxxxxxx>]

2014-05-02 21:44 GMT+02:00 Don Slutz <dslutz@xxxxxxxxxxx>:

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@xxxxxxxxxxx>

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.

Â

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@xxxxxxx>

---

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

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