1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] Don't parse/format vram attribute for cirrus video

Jonathon Jongsma posted 1 patch 4 years, 10 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190620195112.26695-1-jjongsma@redhat.com
src/conf/domain_conf.c | 2 +-
src/qemu/qemu_domain.c | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
[libvirt] [PATCH] Don't parse/format vram attribute for cirrus video
Posted by Jonathon Jongsma 4 years, 10 months ago 
Since the cirrus vga memory size isn't configurable, we can ignore any
'vram' attribute when parsing a domain definition. However, when no
value is specified, it ends up getting set to a default value of 16MB.
This 16MB value is not used anywhere (for example, it is not passed as
an argument to qemu), but is displayed in the XML definition. So by
changing this default value to 0, it will also be omitted from the XML
definition of the domain.

Fixes: rhbz#1447831

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
This is an attempt to apply the fix suggested by Gerd at
https://bugzilla.redhat.com/show_bug.cgi?id=1447831#c2. I'm not
totally confident that this is the right approach, since I'm
relatively new to the code. Another approach might be to simply close
the bug as NOTABUG since it doesn't seem that having this unused
attribute in the domain definition has any significant drawbacks.

 src/conf/domain_conf.c | 2 +-
 src/qemu/qemu_domain.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c69d382d70..d06e094b11 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15154,7 +15154,6 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
 {
     switch (type) {
     case VIR_DOMAIN_VIDEO_TYPE_VGA:
-    case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
     case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
         if (def->virtType == VIR_DOMAIN_VIRT_VBOX)
             return 8 * 1024;
@@ -15172,6 +15171,7 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
         /* QEMU use 64M as the minimal video memory for qxl device */
         return 64 * 1024;
 
+    case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
     case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
     case VIR_DOMAIN_VIDEO_TYPE_VBOX:
     case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e5c6ef3fda..7f0baf57ac 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6835,6 +6835,9 @@ qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
     if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
         !video->vgamem) {
         video->vgamem = QEMU_QXL_VGAMEM_DEFAULT;
+    } else if (video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS) {
+        /* cirrus vram is not configurable. Ignore it */
+        video->vram = 0;
     }
 
     return 0;
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't parse/format vram attribute for cirrus video
Posted by Peter Krempa 4 years, 10 months ago 
On Thu, Jun 20, 2019 at 14:51:12 -0500, Jonathon Jongsma wrote:
> Since the cirrus vga memory size isn't configurable, we can ignore any
> 'vram' attribute when parsing a domain definition. However, when no
> value is specified, it ends up getting set to a default value of 16MB.
> This 16MB value is not used anywhere (for example, it is not passed as
> an argument to qemu), but is displayed in the XML definition. So by
> changing this default value to 0, it will also be omitted from the XML
> definition of the domain.
> 
> Fixes: rhbz#1447831

Please always use the full link so that it's clickable and users don't
have to figure out what 'rhbz' is.

> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
> This is an attempt to apply the fix suggested by Gerd at
> https://bugzilla.redhat.com/show_bug.cgi?id=1447831#c2. I'm not
> totally confident that this is the right approach, since I'm
> relatively new to the code. Another approach might be to simply close
> the bug as NOTABUG since it doesn't seem that having this unused
> attribute in the domain definition has any significant drawbacks.

We certainly should not set any default if it's not used. There's not
much else we can do though as we did put a default into the
configuration. Doing any validation would mean that any VM which had the
default errorneously configured in the XML would fail to start.

Whether it's worth clearing the default or not I'm not so certain but I
don't think it should hurt.

You definitely should fix the docs though:

https://libvirt.org/formatdomain.html#elementsVideo

As it says 'For a guest of type "kvm", the default video is: type with
value "cirrus", vram with value "16384" and heads with value "1".'

(see docs/formatdomain.html.in )
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't parse/format vram attribute for cirrus video
Posted by Jonathon Jongsma 4 years, 10 months ago 
On Fri, 2019-06-21 at 08:13 +0200, Peter Krempa wrote:
> On Thu, Jun 20, 2019 at 14:51:12 -0500, Jonathon Jongsma wrote:
> > Since the cirrus vga memory size isn't configurable, we can ignore
> > any
> > 'vram' attribute when parsing a domain definition. However, when no
> > value is specified, it ends up getting set to a default value of
> > 16MB.
> > This 16MB value is not used anywhere (for example, it is not passed
> > as
> > an argument to qemu), but is displayed in the XML definition. So by
> > changing this default value to 0, it will also be omitted from the
> > XML
> > definition of the domain.
> > 
> > Fixes: rhbz#1447831
> 
> Please always use the full link so that it's clickable and users
> don't
> have to figure out what 'rhbz' is.
> 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> > This is an attempt to apply the fix suggested by Gerd at
> > https://bugzilla.redhat.com/show_bug.cgi?id=1447831#c2. I'm not
> > totally confident that this is the right approach, since I'm
> > relatively new to the code. Another approach might be to simply
> > close
> > the bug as NOTABUG since it doesn't seem that having this unused
> > attribute in the domain definition has any significant drawbacks.
> 
> We certainly should not set any default if it's not used. There's not
> much else we can do though as we did put a default into the
> configuration. 

I'm not sure I understand what you're suggesting here. This sentence
seems to imply that you think we should *not* set a default, but down
below you say that you're not certain whether we should clear the
default? It seems a bit contradictory, but perhaps I'm simply
misunderstanding.

> Doing any validation would mean that any VM which had the
> default errorneously configured in the XML would fail to start.

Yeah, I did not do any 'validation' of cirrus vram attributes because I
didn't want existing VMs to fail to start. And all existing VMs that
use the cirrus device presumably have this attribute set in their XML
definition. So I simply ignored this parameter rather than rejecting
it. But as I said, I'm not sure whether that's the correct approach for
libvirt.

> 
> Whether it's worth clearing the default or not I'm not so certain but
> I
> don't think it should hurt.
> 
> You definitely should fix the docs though:
> 
> https://libvirt.org/formatdomain.html#elementsVideo
> 
> As it says 'For a guest of type "kvm", the default video is: type
> with
> value "cirrus", vram with value "16384" and heads with value "1".'
> 
> (see docs/formatdomain.html.in )

Thanks, will do.

Jonathon

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't parse/format vram attribute for cirrus video
Posted by Peter Krempa 4 years, 10 months ago 
On Fri, Jun 21, 2019 at 09:53:59 -0500, Jonathon Jongsma wrote:
> On Fri, 2019-06-21 at 08:13 +0200, Peter Krempa wrote:
> > On Thu, Jun 20, 2019 at 14:51:12 -0500, Jonathon Jongsma wrote:
> > > Since the cirrus vga memory size isn't configurable, we can ignore
> > > any
> > > 'vram' attribute when parsing a domain definition. However, when no
> > > value is specified, it ends up getting set to a default value of
> > > 16MB.
> > > This 16MB value is not used anywhere (for example, it is not passed
> > > as
> > > an argument to qemu), but is displayed in the XML definition. So by
> > > changing this default value to 0, it will also be omitted from the
> > > XML
> > > definition of the domain.
> > > 
> > > Fixes: rhbz#1447831
> > 
> > Please always use the full link so that it's clickable and users
> > don't
> > have to figure out what 'rhbz' is.
> > 
> > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > > ---
> > > This is an attempt to apply the fix suggested by Gerd at
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1447831#c2. I'm not
> > > totally confident that this is the right approach, since I'm
> > > relatively new to the code. Another approach might be to simply
> > > close
> > > the bug as NOTABUG since it doesn't seem that having this unused
> > > attribute in the domain definition has any significant drawbacks.
> > 
> > We certainly should not set any default if it's not used. There's not
> > much else we can do though as we did put a default into the
> > configuration. 
> 
> I'm not sure I understand what you're suggesting here. This sentence
> seems to imply that you think we should *not* set a default, but down
> below you say that you're not certain whether we should clear the
> default? It seems a bit contradictory, but perhaps I'm simply
> misunderstanding.

I actually meant 'clearing the memory size', thus basically just
omitting the whole second hunk of your patch. So any previously set
value would be kept intact.

Clearing it itself is not a problem for libvirt but might be potentially
unexpected by higher level management apps. But I'm not really sure
whether't that's a valid case.

> 
> > Doing any validation would mean that any VM which had the
> > default errorneously configured in the XML would fail to start.
> 
> Yeah, I did not do any 'validation' of cirrus vram attributes because I
> didn't want existing VMs to fail to start. And all existing VMs that
> use the cirrus device presumably have this attribute set in their XML
> definition. So I simply ignored this parameter rather than rejecting
> it. But as I said, I'm not sure whether that's the correct approach for
> libvirt.

I don't think there's much to do besides ignoring it.

> 
> > 
> > Whether it's worth clearing the default or not I'm not so certain but
> > I
> > don't think it should hurt.
> > 
> > You definitely should fix the docs though:
> > 
> > https://libvirt.org/formatdomain.html#elementsVideo
> > 
> > As it says 'For a guest of type "kvm", the default video is: type
> > with
> > value "cirrus", vram with value "16384" and heads with value "1".'
> > 
> > (see docs/formatdomain.html.in )
> 
> Thanks, will do.
> 
> Jonathon
> 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't parse/format vram attribute for cirrus video
Posted by Jonathon Jongsma 4 years, 10 months ago 
On Fri, 2019-06-21 at 17:05 +0200, Peter Krempa wrote:
> On Fri, Jun 21, 2019 at 09:53:59 -0500, Jonathon Jongsma wrote:
> > On Fri, 2019-06-21 at 08:13 +0200, Peter Krempa wrote:
> > > On Thu, Jun 20, 2019 at 14:51:12 -0500, Jonathon Jongsma wrote:
> > > > Since the cirrus vga memory size isn't configurable, we can
> > > > ignore
> > > > any
> > > > 'vram' attribute when parsing a domain definition. However,
> > > > when no
> > > > value is specified, it ends up getting set to a default value
> > > > of
> > > > 16MB.
> > > > This 16MB value is not used anywhere (for example, it is not
> > > > passed
> > > > as
> > > > an argument to qemu), but is displayed in the XML definition.
> > > > So by
> > > > changing this default value to 0, it will also be omitted from
> > > > the
> > > > XML
> > > > definition of the domain.
> > > > 
> > > > Fixes: rhbz#1447831
> > > 
> > > Please always use the full link so that it's clickable and users
> > > don't
> > > have to figure out what 'rhbz' is.
> > > 
> > > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > > > ---
> > > > This is an attempt to apply the fix suggested by Gerd at
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1447831#c2. I'm not
> > > > totally confident that this is the right approach, since I'm
> > > > relatively new to the code. Another approach might be to simply
> > > > close
> > > > the bug as NOTABUG since it doesn't seem that having this
> > > > unused
> > > > attribute in the domain definition has any significant
> > > > drawbacks.
> > > 
> > > We certainly should not set any default if it's not used. There's
> > > not
> > > much else we can do though as we did put a default into the
> > > configuration. 
> > 
> > I'm not sure I understand what you're suggesting here. This
> > sentence
> > seems to imply that you think we should *not* set a default, but
> > down
> > below you say that you're not certain whether we should clear the
> > default? It seems a bit contradictory, but perhaps I'm simply
> > misunderstanding.
> 
> I actually meant 'clearing the memory size', thus basically just
> omitting the whole second hunk of your patch. So any previously set
> value would be kept intact.
> 
> Clearing it itself is not a problem for libvirt but might be
> potentially
> unexpected by higher level management apps. But I'm not really sure
> whether't that's a valid case.

I'm starting to wonder if I'm not falling into the trap of only
thinking about things from a qemu/kvm perspective. It looks like a
cirrus device can also be used in the libxl driver, and perhaps it is
configurable under that hypervisor? I don't have any experience there.
In that case, maybe it's safest to simply drop this patch and close the
bug as WONTFIX. Any thoughts?

Jonathon

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.