Rob Hoes
2013-Oct-21 12:41 UTC
[RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on traditional QEMU
VMs using Cirrus graphics have always had 4 MB of video RAM on XenServer, while
libxl currently does not allow values less than 8 MB.
Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
----
Resend note: No one has replied with an Acked-by line yet, but Fabio Fantoni
has replied with "I not tested it for now but seems good".
---
docs/man/xl.cfg.pod.5 | 18 +++++++-------
tools/libxl/libxl_create.c | 57 ++++++++++++++++++++++++++++++++++----------
2 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 278dba1..d2d8921 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1028,14 +1028,16 @@ in the B<VFB_SPEC_STRING> for configuring virtual
frame buffer devices
Sets the amount of RAM which the emulated video card will contain,
which in turn limits the resolutions and bit depths which will be
available.
-The default amount of video ram for stdvga is 8MB which is sufficient
-for e.g. 1600x1200 at 32bpp and videoram option is currently working
-only when using the qemu-xen-traditional device-model.
-
-When using the emulated Cirrus graphics card (B<vga="cirrus">)
-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.
+
+When using the qemu-xen-traditional device-model, the default as well as
+minimum amount of video RAM for stdvga is 8 MB, which is sufficient for e.g.
+1600x1200 at 32bpp. For the upstream qemu-xen device-model, the default and
+minimum is 16 MB.
+
+When using the emulated Cirrus graphics card (B<vga="cirrus">)
and the
+qemu-xen-traditional device-model, the amount of video RAM is fixed at 4 MB,
+which is sufficient for 1024x768 at 32 bpp. For the upstream qemu-xen
+device-model, the default and minimum is 8 MB.
=item B<stdvga=BOOLEAN>
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1b320d3..e065a1d 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -216,20 +216,55 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
b_info->shadow_memkb = 0;
- if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_STD
&&
- b_info->device_model_version =-
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+ if (!b_info->u.hvm.vga.kind)
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+
+ switch (b_info->device_model_version) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ switch (b_info->u.hvm.vga.kind) {
+ case LIBXL_VGA_INTERFACE_TYPE_STD:
+ if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
+ b_info->video_memkb = 8 * 1024;
+ if (b_info->video_memkb < (8 * 1024) ){
+ LOG(ERROR,"videoram must be at least 8 MB for STDVGA
"
+ "on QEMU_XEN_TRADITIONAL");
+ return ERROR_INVAL;
+ }
+ break;
+ case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
+ default:
+ if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
+ b_info->video_memkb = 4 * 1024;
+ if (b_info->video_memkb != (4 * 1024) ){
+ LOG(WARN,"videoram other than 4 MB for CIRRUS on
"
+ "QEMU_XEN_TRADITIONAL will be ignored"); }
+ break;
+ }
+ break;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ default:
+ switch (b_info->u.hvm.vga.kind) {
+ case LIBXL_VGA_INTERFACE_TYPE_STD:
if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
b_info->video_memkb = 16 * 1024;
- else if (b_info->video_memkb < (16 * 1024) ){
- LOG(ERROR, "videoram must be at least 16 mb with
stdvga");
+ if (b_info->video_memkb < (16 * 1024) ){
+ LOG(ERROR,"videoram must be at least 16 MB for STDVGA
"
+ "on QEMU_XEN");
+ return ERROR_INVAL;
+ }
+ break;
+ case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
+ default:
+ if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
+ b_info->video_memkb = 8 * 1024;
+ if (b_info->video_memkb < (8 * 1024) ){
+ LOG(ERROR,"videoram must be at least 8 MB for CIRRUS
"
+ "on QEMU_XEN");
return ERROR_INVAL;
}
- } else if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
- b_info->video_memkb = 8 * 1024;
- else if (b_info->video_memkb < (8 * 1024) ){
- LOG(ERROR,"videoram must be at least 8 mb");
- return ERROR_INVAL;
+ break;
+ }
+ break;
}
if (b_info->u.hvm.timer_mode == LIBXL_TIMER_MODE_DEFAULT)
@@ -254,8 +289,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->u.hvm.boot) return ERROR_NOMEM;
}
- if (!b_info->u.hvm.vga.kind)
- b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
libxl_defbool_setdefault(&b_info->u.hvm.vnc.enable, true);
if (libxl_defbool_val(b_info->u.hvm.vnc.enable)) {
libxl_defbool_setdefault(&b_info->u.hvm.vnc.findunused,
true);
--
1.7.10.4
Fabio Fantoni
2013-Oct-24 12:59 UTC
Re: [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on traditional QEMU
Il 21/10/2013 14:41, Rob Hoes ha scritto:> VMs using Cirrus graphics have always had 4 MB of video RAM on XenServer, while > libxl currently does not allow values less than 8 MB. > > Signed-off-by: Rob Hoes <rob.hoes@citrix.com>Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz> Tested with upstream qemu, I not tested with traditional but I suppose that Rob had already tested with it.> > ---- > Resend note: No one has replied with an Acked-by line yet, but Fabio Fantoni > has replied with "I not tested it for now but seems good". > --- > docs/man/xl.cfg.pod.5 | 18 +++++++------- > tools/libxl/libxl_create.c | 57 ++++++++++++++++++++++++++++++++++---------- > 2 files changed, 55 insertions(+), 20 deletions(-) > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > index 278dba1..d2d8921 100644 > --- a/docs/man/xl.cfg.pod.5 > +++ b/docs/man/xl.cfg.pod.5 > @@ -1028,14 +1028,16 @@ in the B<VFB_SPEC_STRING> for configuring virtual frame buffer devices > Sets the amount of RAM which the emulated video card will contain, > which in turn limits the resolutions and bit depths which will be > available. > -The default amount of video ram for stdvga is 8MB which is sufficient > -for e.g. 1600x1200 at 32bpp and videoram option is currently working > -only when using the qemu-xen-traditional device-model. > - > -When using the emulated Cirrus graphics card (B<vga="cirrus">) > -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. > + > +When using the qemu-xen-traditional device-model, the default as well as > +minimum amount of video RAM for stdvga is 8 MB, which is sufficient for e.g. > +1600x1200 at 32bpp. For the upstream qemu-xen device-model, the default and > +minimum is 16 MB. > + > +When using the emulated Cirrus graphics card (B<vga="cirrus">) and the > +qemu-xen-traditional device-model, the amount of video RAM is fixed at 4 MB, > +which is sufficient for 1024x768 at 32 bpp. For the upstream qemu-xen > +device-model, the default and minimum is 8 MB. > > =item B<stdvga=BOOLEAN> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 1b320d3..e065a1d 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -216,20 +216,55 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT) > b_info->shadow_memkb = 0; > > - if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_STD && > - b_info->device_model_version => - LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) { > + if (!b_info->u.hvm.vga.kind) > + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; > + > + switch (b_info->device_model_version) { > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: > + switch (b_info->u.hvm.vga.kind) { > + case LIBXL_VGA_INTERFACE_TYPE_STD: > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > + b_info->video_memkb = 8 * 1024; > + if (b_info->video_memkb < (8 * 1024) ){ > + LOG(ERROR,"videoram must be at least 8 MB for STDVGA " > + "on QEMU_XEN_TRADITIONAL"); > + return ERROR_INVAL; > + } > + break; > + case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > + default: > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > + b_info->video_memkb = 4 * 1024; > + if (b_info->video_memkb != (4 * 1024) ){ > + LOG(WARN,"videoram other than 4 MB for CIRRUS on " > + "QEMU_XEN_TRADITIONAL will be ignored"); } > + break; > + } > + break; > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: > + default: > + switch (b_info->u.hvm.vga.kind) { > + case LIBXL_VGA_INTERFACE_TYPE_STD: > if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > b_info->video_memkb = 16 * 1024; > - else if (b_info->video_memkb < (16 * 1024) ){ > - LOG(ERROR, "videoram must be at least 16 mb with stdvga"); > + if (b_info->video_memkb < (16 * 1024) ){ > + LOG(ERROR,"videoram must be at least 16 MB for STDVGA " > + "on QEMU_XEN"); > + return ERROR_INVAL; > + } > + break; > + case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > + default: > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > + b_info->video_memkb = 8 * 1024; > + if (b_info->video_memkb < (8 * 1024) ){ > + LOG(ERROR,"videoram must be at least 8 MB for CIRRUS " > + "on QEMU_XEN"); > return ERROR_INVAL; > } > - } else if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > - b_info->video_memkb = 8 * 1024; > - else if (b_info->video_memkb < (8 * 1024) ){ > - LOG(ERROR,"videoram must be at least 8 mb"); > - return ERROR_INVAL; > + break; > + } > + break; > } > > if (b_info->u.hvm.timer_mode == LIBXL_TIMER_MODE_DEFAULT) > @@ -254,8 +289,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > if (!b_info->u.hvm.boot) return ERROR_NOMEM; > } > > - if (!b_info->u.hvm.vga.kind) > - b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; > libxl_defbool_setdefault(&b_info->u.hvm.vnc.enable, true); > if (libxl_defbool_val(b_info->u.hvm.vnc.enable)) { > libxl_defbool_setdefault(&b_info->u.hvm.vnc.findunused, true);
Ian Campbell
2013-Oct-24 13:14 UTC
Re: [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on traditional QEMU
On Mon, 2013-10-21 at 13:41 +0100, Rob Hoes wrote:> VMs using Cirrus graphics have always had 4 MB of video RAM on XenServer, while > libxl currently does not allow values less than 8 MB. > > Signed-off-by: Rob Hoes <rob.hoes@citrix.com> > > ---- > Resend note: No one has replied with an Acked-by line yet,I''d like to see an ack from one of the qemu guys regarding this change. From my PoV the logic looks correct but there are some coding stylei issues which I''ll comment on below.> + switch (b_info->device_model_version) { > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: > + switch (b_info->u.hvm.vga.kind) { > + case LIBXL_VGA_INTERFACE_TYPE_STD: > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > + b_info->video_memkb = 8 * 1024; > + if (b_info->video_memkb < (8 * 1024) ){ > + LOG(ERROR,"videoram must be at least 8 MB for STDVGA "Coding style would have a space after the "," (multiple cases of this). Also the brackets around 8*1024 are not necessary, and the space at theend should be after the closing ) (before the {) i.e. + if (b_info->video_memkb < 8 * 1024) { + LOG(ERROR,"videoram must be at least 8 MB for STDVGA ..."); I prefer to avoid splitting string constants even if this violates the 80 column limit, this makes it easier to grep.> + "on QEMU_XEN_TRADITIONAL"); > + return ERROR_INVAL; > + } > + break; > + case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > + default: > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > + b_info->video_memkb = 4 * 1024; > + if (b_info->video_memkb != (4 * 1024) ){ > + LOG(WARN,"videoram other than 4 MB for CIRRUS on " > + "QEMU_XEN_TRADITIONAL will be ignored"); }Closing brace should be on a new line. Actually, since this is a single statement the braces are not needed. Rather than "will be" I might word this in a more active voice like "Ignoring videoram blah blah". Ian.
Rob Hoes
2013-Oct-24 13:24 UTC
Re: [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on traditional QEMU
> -----Original Message----- > From: Ian Campbell [mailto:ian.campbell@citrix.com] > Sent: 24 October 2013 2:14 PM > To: Rob Hoes > Cc: xen-devel@lists.xen.org; Ian Jackson; Stefano Stabellini; Anthony Perard > Subject: Re: [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on > traditional QEMU > > On Mon, 2013-10-21 at 13:41 +0100, Rob Hoes wrote: > > VMs using Cirrus graphics have always had 4 MB of video RAM on > > XenServer, while libxl currently does not allow values less than 8 MB. > > > > Signed-off-by: Rob Hoes <rob.hoes@citrix.com> > > > > ---- > > Resend note: No one has replied with an Acked-by line yet, > > I''d like to see an ack from one of the qemu guys regarding this change. > > From my PoV the logic looks correct but there are some coding stylei issues > which I''ll comment on below. > > > + switch (b_info->device_model_version) { > > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: > > + switch (b_info->u.hvm.vga.kind) { > > + case LIBXL_VGA_INTERFACE_TYPE_STD: > > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > > + b_info->video_memkb = 8 * 1024; > > + if (b_info->video_memkb < (8 * 1024) ){ > > + LOG(ERROR,"videoram must be at least 8 MB for STDVGA " > > Coding style would have a space after the "," (multiple cases of this). > Also the brackets around 8*1024 are not necessary, and the space at theend > should be after the closing ) (before the {) i.e. > + if (b_info->video_memkb < 8 * 1024) { > + LOG(ERROR,"videoram must be at least 8 MB for > + STDVGA ..."); > > I prefer to avoid splitting string constants even if this violates the > 80 column limit, this makes it easier to grep. > > > + "on QEMU_XEN_TRADITIONAL"); > > + return ERROR_INVAL; > > + } > > + break; > > + case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > > + default: > > + if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) > > + b_info->video_memkb = 4 * 1024; > > + if (b_info->video_memkb != (4 * 1024) ){ > > + LOG(WARN,"videoram other than 4 MB for CIRRUS on " > > + "QEMU_XEN_TRADITIONAL will be ignored"); } > > Closing brace should be on a new line. Actually, since this is a single > statement the braces are not needed.Thanks Ian. I''ll fix up those style issues.> Rather than "will be" I might word this in a more active voice like "Ignoring > videoram blah blah".I used "will" here because it is the QEMU later on doing the ignoring, not this particular bit of code. How about making it active this like: "Traditional QEMU will ignore videoram other than 4 MB for CIRRUS"? Cheers, Rob
Ian Campbell
2013-Oct-24 14:10 UTC
Re: [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on traditional QEMU
On Thu, 2013-10-24 at 14:24 +0100, Rob Hoes wrote:> > Closing brace should be on a new line. Actually, since this is a single > > statement the braces are not needed. > > Thanks Ian. I''ll fix up those style issues.Thanks.> > Rather than "will be" I might word this in a more active voice like "Ignoring > > videoram blah blah". > > I used "will" here because it is the QEMU later on doing the ignoring,From the point of view of the user reading the message they don''t know or care what is actually doing the ignoring, the effect is that it has been ignored. Ian.