[PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Fabio Fantoni]

Setting video memory of stdvga with upstream qemu, already
present on qemu traditional but missed in upstream qemu.
This patch add missed qemu parameters only.
Seems no docs modifications needed for commit
13d13a45d0591fc195666ea20ddf8781a0367e88

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>

---

Changes in v2:
Improved commit message

Note:
I think that this patch can be also backported to xen 4.4.
---
 tools/libxl/libxl_dm.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 90f19b7..51ab2bf 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -505,7 +505,9 @@ 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_append_pair(dm_args, "-device", "VGA");
+            flexarray_append_pair(dm_args, "-device",
+                GCSPRINTF("VGA,vgamem_mb=%d",
+                libxl__sizekb_to_mb(b_info->video_memkb)));
             break;
         case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
             flexarray_append_pair(dm_args, "-device",
-- 
1.7.9.5

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Ian Campbell]

On Fri, 2014-05-09 at 15:04 +0200, Fabio Fantoni wrote:
> Setting video memory of stdvga with upstream qemu, already
> present on qemu traditional but missed in upstream qemu.
> This patch add missed qemu parameters only.
> Seems no docs modifications needed for commit
> 13d13a45d0591fc195666ea20ddf8781a0367e88
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>

Acked + applied. I reworded the commit message a bit to:
    libxl: add stdvga video memory setting with upstream qemu
    
    Currently we set the stdvga video memory with qemu-traditional only, add the
    necessary settings for qemu upstream too.
    
    Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Ian Campbell]

On Fri, 2014-05-09 at 15:04 +0200, Fabio Fantoni wrote:
> Setting video memory of stdvga with upstream qemu, already
> present on qemu traditional but missed in upstream qemu.
> This patch add missed qemu parameters only.
> Seems no docs modifications needed for commit
> 13d13a45d0591fc195666ea20ddf8781a0367e88
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>

Acked + applied. I reworded the commit message a bit to:
    libxl: add stdvga video memory setting with upstream qemu
    
    Currently we set the stdvga video memory with qemu-traditional only, add the
    necessary settings for qemu upstream too.
    
    Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Ian Campbell]

On Fri, 2014-05-09 at 15:04 +0200, Fabio Fantoni wrote:
> Setting video memory of stdvga with upstream qemu, already
> present on qemu traditional but missed in upstream qemu.
> This patch add missed qemu parameters only.
> Seems no docs modifications needed for commit
> 13d13a45d0591fc195666ea20ddf8781a0367e88
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>

Acked + applied. I reworded the commit message a bit to:
    libxl: add stdvga video memory setting with upstream qemu
    
    Currently we set the stdvga video memory with qemu-traditional only, add the
    necessary settings for qemu upstream too.
    
    Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Fabio Fantoni]

Il 15/05/2014 18:00, Ian Campbell ha scritto:
> On Fri, 2014-05-09 at 15:04 +0200, Fabio Fantoni wrote:
>> Setting video memory of stdvga with upstream qemu, already
>> present on qemu traditional but missed in upstream qemu.
>> This patch add missed qemu parameters only.
>> Seems no docs modifications needed for commit
>> 13d13a45d0591fc195666ea20ddf8781a0367e88
>>
>> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> Acked + applied. I reworded the commit message a bit to:
>      libxl: add stdvga video memory setting with upstream qemu
>      
>      Currently we set the stdvga video memory with qemu-traditional only, add the
>      necessary settings for qemu upstream too.
>      
>      Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
>      Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>
>
Thanks.
I think that this patch can be also backported to xen 4.4.

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Ian Jackson]

Fabio Fantoni writes ("Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu"):
> Il 15/05/2014 18:00, Ian Campbell ha scritto:
> > Acked + applied. I reworded the commit message a bit to:
> >      libxl: add stdvga video memory setting with upstream qemu
> >      
> >      Currently we set the stdvga video memory with qemu-traditional only, add the
> >      necessary settings for qemu upstream too.
> >      
> >      Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> >      Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> >
> >
> Thanks.
> I think that this patch can be also backported to xen 4.4.

I think this falls on the bugfix side of the line, but:

I worry that it might cause regressions in existing setups, because it
changes the interpretation of the domain config file (from a incorrect
to a correct interpretation, but that is still a problem for a
backport).

So I would be inclined not to backport this one.  I've added Jan, the
hypervisor-side stable tree maintainer, in case he has a different
view.

Thanks,
Ian.

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

[Fabio Fantoni]

Upstream qemu cirrus vga videoram setting parameter is wrong.
Under this condition qemu silently ignores this setting and
doesn't show any errors.

This patch use vgamem_mb property that was added in qemu 1.3,
in which was introduced cirrus vga videoram modification
(previous missed)

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
Reviewed-by: Don Slutz <dslutz@verizon.com>

---

Changes in v2:
Improved commit message

Note:
- when this patch will be accepted upstream should be
  backported also on xen 4.4
---
 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:
-- 
1.7.9.5

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

[Ian Campbell]

On Fri, 2014-05-09 at 14:55 +0200, Fabio Fantoni wrote:
> Upstream qemu cirrus vga videoram setting parameter is wrong.
> Under this condition qemu silently ignores this setting and
> doesn't show any errors.
> 
> This patch use vgamem_mb property that was added in qemu 1.3,
> in which was introduced cirrus vga videoram modification
> (previous missed)
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> Reviewed-by: Don Slutz <dslutz@verizon.com>

Acked + applied.
> 
> ---
> 
> Changes in v2:
> Improved commit message

I improved it further to:
libxl: fix cirrus vga video memory setting with upstream qemu

The Cirrus VGA videoram setting used with upstream qemu is wrong. Qemu
silently ignores the incorrect setting.
 
Switch to the correct vgamem_mb property which was added in qemu 1.3.

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
Reviewed-by: Don Slutz <dslutz@verizon.com>

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

[Fabio Fantoni]

Il 15/05/2014 18:00, Ian Campbell ha scritto:
> On Fri, 2014-05-09 at 14:55 +0200, Fabio Fantoni wrote:
>> Upstream qemu cirrus vga videoram setting parameter is wrong.
>> Under this condition qemu silently ignores this setting and
>> doesn't show any errors.
>>
>> This patch use vgamem_mb property that was added in qemu 1.3,
>> in which was introduced cirrus vga videoram modification
>> (previous missed)
>>
>> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
>> Reviewed-by: Don Slutz <dslutz@verizon.com>
> Acked + applied.

Thanks.
I think that this patch should be backported also on xen 4.4.

>> ---
>>
>> Changes in v2:
>> Improved commit message
> I improved it further to:
> libxl: fix cirrus vga video memory setting with upstream qemu
>
> The Cirrus VGA videoram setting used with upstream qemu is wrong. Qemu
> silently ignores the incorrect setting.
>   
> Switch to the correct vgamem_mb property which was added in qemu 1.3.
>
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> Reviewed-by: Don Slutz <dslutz@verizon.com>
>
>
>

Re: [PATCH v2] libxl: fix cirrus vga video memory setting with upstream qemu [and 2 more messages]

[Ian Jackson]

For the avoidance of doubt, my comments apply to both of these video
memory setting patches.

Ian.

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Jan Beulich]

>>> On 19.05.14 at 13:41, <Ian.Jackson@eu.citrix.com> wrote:
> Fabio Fantoni writes ("Re: [PATCH v2] libxl: add stdvga video memory setting 
> with upstream qemu"):
>> Il 15/05/2014 18:00, Ian Campbell ha scritto:
>> > Acked + applied. I reworded the commit message a bit to:
>> >      libxl: add stdvga video memory setting with upstream qemu
>> >      
>> >      Currently we set the stdvga video memory with qemu-traditional only, 
> add the
>> >      necessary settings for qemu upstream too.
>> >      
>> >      Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
>> >      Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> >
>> >
>> >
>> Thanks.
>> I think that this patch can be also backported to xen 4.4.
> 
> I think this falls on the bugfix side of the line, but:
> 
> I worry that it might cause regressions in existing setups, because it
> changes the interpretation of the domain config file (from a incorrect
> to a correct interpretation, but that is still a problem for a
> backport).
> 
> So I would be inclined not to backport this one.  I've added Jan, the
> hypervisor-side stable tree maintainer, in case he has a different
> view.

Help me understand the implications on the guest that this
changed (corrected) interpretation of the config file would have:
Would the guest see differing resources between reboots? Would
such differences also surface post-migration (which would pretty
clearly make this a no-go for a backport)?

Jan

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu"):
> On 19.05.14 at 13:41, <Ian.Jackson@eu.citrix.com> wrote:
> > So I would be inclined not to backport this one.  I've added Jan, the
> > hypervisor-side stable tree maintainer, in case he has a different
> > view.
> 
> Help me understand the implications on the guest that this
> changed (corrected) interpretation of the config file would have:
> Would the guest see differing resources between reboots?

Yes.  Also it might be that the configuration file has a wrong value
which was previously ignored but now causes something to fail.

> Would such differences also surface post-migration (which would
> pretty clearly make this a no-go for a backport)?

Yes, with the current state of affairs, it would indeed see
differences across migration.

Ian.

Re: [PATCH v2] libxl: add stdvga video memory setting with upstream qemu

[Jan Beulich]

>>> On 19.05.14 at 13:52, <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v2] libxl: add stdvga video memory setting 
> with upstream qemu"):
>> On 19.05.14 at 13:41, <Ian.Jackson@eu.citrix.com> wrote:
>> > So I would be inclined not to backport this one.  I've added Jan, the
>> > hypervisor-side stable tree maintainer, in case he has a different
>> > view.
>> 
>> Help me understand the implications on the guest that this
>> changed (corrected) interpretation of the config file would have:
>> Would the guest see differing resources between reboots?
> 
> Yes.  Also it might be that the configuration file has a wrong value
> which was previously ignored but now causes something to fail.
> 
>> Would such differences also surface post-migration (which would
>> pretty clearly make this a no-go for a backport)?
> 
> Yes, with the current state of affairs, it would indeed see
> differences across migration.

In which case I agree that this would better not be backported.

Jan