8:50 a.m.
Sorry, this fell complete off my todos for a while. So I split off
the fixup of VRAM into a separate patch which may or may not be used
and only accept vga, xen and cirrus as supported types in the main
patch.
I believe I saw some discussions about how to fix some of the VRAM
values as they are passed into qemu. At least for the Cirrus type
I saw that the command line looked ok but the guest was getting a
much larger VRAM size than it was told.
-Stefan
8:50 a.m.
New subject: [libvirt] [PATCH 1/2] libxl: Implement basic video device selection
This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.
Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.
The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).
While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.
[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
---
src/libxl/libxl_conf.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b7fed7f..2b5c469 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1304,6 +1304,64 @@ libxlMakeCapabilities(libxl_ctx *ctx)
return NULL;
}
+static int
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ libxl_domain_build_info *b_info = &d_config->b_info;
+
+ /*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+ if (def->nvideos) {
+ switch (def->videos[0]->type) {
+ case VIR_DOMAIN_VIDEO_TYPE_VGA:
+ case VIR_DOMAIN_VIDEO_TYPE_XEN:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+ break;
+ case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+ break;
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s",
+ _("video type not supported by libxl"));
+ return -1;
+ }
+ b_info->video_memkb = def->videos[0]->vram ?
+ def->videos[0]->vram :
+ LIBXL_MEMKB_DEFAULT;
+ } else {
+ libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+ }
+
+ /*
+ * When making the list of displays, only VNC and SDL types were
+ * taken into account. So it seems sensible to connect the default
+ * video device to the first in the vfb list.
+ */
+ if (d_config->num_vfbs) {
+ libxl_device_vfb *vfb0 = &d_config->vfbs[0];
+
+ b_info->u.hvm.vnc = vfb0->vnc;
+ if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen) < 0)
+ return -1;
+ if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd) < 0)
+ return -1;
+ b_info->u.hvm.sdl = vfb0->sdl;
+ if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display) < 0)
+ return -1;
+ if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority) < 0)
+ return -1;
+ if (VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
int
libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
virDomainObjPtr vm, libxl_domain_config *d_config)
@@ -1331,6 +1389,16 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
if (libxlMakePCIList(def, d_config) < 0)
return -1;
+ /*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+ if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
+ if (libxlSetBuildGraphics(def, d_config) < 0)
+ return -1;
+
d_config->on_reboot = def->onReboot;
d_config->on_poweroff = def->onPoweroff;
d_config->on_crash = def->onCrash;
--
1.7.9.5
5:35 p.m.
New subject: [libvirt] [PATCH 1/2] libxl: Implement basic video device selection
Hi Stefan,
Thanks for the patches, sorry for the delay...
Stefan Bader wrote:
This started as an investigation into an issue where libvirt (using
the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.
Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.
The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).
While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.
[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
---
src/libxl/libxl_conf.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b7fed7f..2b5c469 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1304,6 +1304,64 @@ libxlMakeCapabilities(libxl_ctx *ctx)
return NULL;
}
+static int
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ libxl_domain_build_info *b_info = &d_config->b_info;
+
+ /*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+ if (def->nvideos) {
+ switch (def->videos[0]->type) {
+ case VIR_DOMAIN_VIDEO_TYPE_VGA:
+ case VIR_DOMAIN_VIDEO_TYPE_XEN:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+ break;
+ case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+ break;
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s",
+ _("video type not supported by libxl"));
+ return -1;
+ }
+ b_info->video_memkb = def->videos[0]->vram ?
+ def->videos[0]->vram :
+ LIBXL_MEMKB_DEFAULT;
+ } else {
+ libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+ }
This part configures a video device.
+
+ /*
+ * When making the list of displays, only VNC and SDL types were
+ * taken into account. So it seems sensible to connect the default
+ * video device to the first in the vfb list.
+ */
+ if (d_config->num_vfbs) {
+ libxl_device_vfb *vfb0 = &d_config->vfbs[0];
+
+ b_info->u.hvm.vnc = vfb0->vnc;
+ if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen) < 0)
+ return -1;
+ if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd) < 0)
+ return -1;
+ b_info->u.hvm.sdl = vfb0->sdl;
+ if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display) < 0)
+ return -1;
+ if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority) <
0)
+ return -1;
+ if (VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap) < 0)
+ return -1;
+ }
+
+ return 0;
+}
And this part configures a frame buffer. I think it would be better for
this part to be in libxlMakeVfbList(), similar to commit b55cc5f4. This
function then becomes libxlMakeVideo(), continuing the libxlMake<Device>
pattern.
Regards,
Jim
+
int
libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
virDomainObjPtr vm, libxl_domain_config *d_config)
@@ -1331,6 +1389,16 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
if (libxlMakePCIList(def, d_config) < 0)
return -1;
+ /*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+ if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
+ if (libxlSetBuildGraphics(def, d_config) < 0)
+ return -1;
+
d_config->on_reboot = def->onReboot;
d_config->on_poweroff = def->onPoweroff;
d_config->on_crash = def->onCrash;
2:58 a.m.
New subject: [libvirt] [PATCH] libxl: Implement basic video device selection
being as bad with timely responses. Ok, so how about the following?
One note: it could be the STRDUP's are not strictly needed. But
to me it felt wrong to have two places refer to the same strings
(as MakeVFB copies the struct containing the pointers). If this
is not needed, then all changes now in MakeVFB probably can be
dropped (except setting the keyboard layout, maybe; which I
might miss ;)).
-Stefan
From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader(a)canonical.com>
Date: Thu, 27 Mar 2014 16:01:18 +0100
Subject: [PATCH] libxl: Implement basic video device selection
This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.
Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.
The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).
While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.
[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
[v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
---
src/libxl/libxl_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 61 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8eeaf82..43cabcf 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
libxl_domain_build_info *b_info = &d_config->b_info;
libxl_device_vfb vfb = d_config->vfbs[0];
- if (libxl_defbool_val(vfb.vnc.enable))
+ if (libxl_defbool_val(vfb.vnc.enable)) {
memcpy(&b_info->u.hvm.vnc, &vfb.vnc, sizeof(libxl_vnc_info));
- else if (libxl_defbool_val(vfb.sdl.enable))
+ if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
+ goto error;
+ if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
+ goto error;
+ } else if (libxl_defbool_val(vfb.sdl.enable)) {
memcpy(&b_info->u.hvm.sdl, &vfb.sdl, sizeof(libxl_sdl_info));
+ if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
+ goto error;
+ if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
+ goto error;
+ }
+ if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
+ goto error;
}
return 0;
@@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
return NULL;
}
+static int
+libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ libxl_domain_build_info *b_info = &d_config->b_info;
+
+ if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM)
+ return 0;
+
+ /*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+ if (def->nvideos) {
+ switch (def->videos[0]->type) {
+ case VIR_DOMAIN_VIDEO_TYPE_VGA:
+ case VIR_DOMAIN_VIDEO_TYPE_XEN:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+ break;
+ case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+ break;
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s",
+ _("video type not supported by libxl"));
+ return -1;
+ }
+ b_info->video_memkb = def->videos[0]->vram ?
+ def->videos[0]->vram :
+ LIBXL_MEMKB_DEFAULT;
+ } else {
+ libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+ }
+
+ return 0;
+}
+
int
libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
virDomainDefPtr def,
@@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
if (libxlMakePCIList(def, d_config) < 0)
return -1;
+ /*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+ if (libxlMakeVideo(def, d_config) < 0)
+ return -1;
+
d_config->on_reboot = def->onReboot;
d_config->on_poweroff = def->onPoweroff;
d_config->on_crash = def->onCrash;
--
1.7.9.5
4:05 p.m.
New subject: [libvirt] [PATCH] libxl: Implement basic video device selection
Stefan Bader wrote:
being as bad with timely responses. Ok, so how about the following?
One note: it could be the STRDUP's are not strictly needed. But
to me it felt wrong to have two places refer to the same strings
(as MakeVFB copies the struct containing the pointers).
Agreed. Without the STRDUP's, seems there is a potential for double
free when libxl_device_vfb and libxl_domain_config objects are disposed.
If this
is not needed, then all changes now in MakeVFB probably can be
dropped (except setting the keyboard layout, maybe; which I
might miss ;)).
-Stefan
>From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader(a)canonical.com>
Date: Thu, 27 Mar 2014 16:01:18 +0100
Subject: [PATCH] libxl: Implement basic video device selection
This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.
Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.
The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).
While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.
[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
[v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
[meta-comment]
libvirt prefers patch version history like this to be below the '---'
following your Signed-off-by, so as to not pollute the commit message.
Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
---
src/libxl/libxl_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 61 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8eeaf82..43cabcf 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
libxl_domain_build_info *b_info = &d_config->b_info;
libxl_device_vfb vfb = d_config->vfbs[0];
- if (libxl_defbool_val(vfb.vnc.enable))
+ if (libxl_defbool_val(vfb.vnc.enable)) {
memcpy(&b_info->u.hvm.vnc, &vfb.vnc, sizeof(libxl_vnc_info));
- else if (libxl_defbool_val(vfb.sdl.enable))
+ if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
+ goto error;
+ if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
+ goto error;
+ } else if (libxl_defbool_val(vfb.sdl.enable)) {
memcpy(&b_info->u.hvm.sdl, &vfb.sdl, sizeof(libxl_sdl_info));
+ if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
+ goto error;
+ if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
+ goto error;
+ }
+ if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
+ goto error;
}
return 0;
@@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
return NULL;
}
+static int
+libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ libxl_domain_build_info *b_info = &d_config->b_info;
+
+ if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM)
+ return 0;
+
+ /*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+ if (def->nvideos) {
+ switch (def->videos[0]->type) {
+ case VIR_DOMAIN_VIDEO_TYPE_VGA:
+ case VIR_DOMAIN_VIDEO_TYPE_XEN:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+ break;
+ case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+ b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+ break;
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s",
+ _("video type not supported by libxl"));
+ return -1;
+ }
+ b_info->video_memkb = def->videos[0]->vram ?
+ def->videos[0]->vram :
+ LIBXL_MEMKB_DEFAULT;
While testing this, I noticed that libvirt will set vram to 9216 if not
specified. E.g.
# cat test.xml
...
<video>
<model type='vga'/>
</video>
...
# virsh define test.xml
# virsh dumpxml test
...
<video>
<model type='vga' vram='9216' heads='1'/>
</video>
...
With type='vga', libxl will then fail to start the domain
libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
videoram must be at least 16 MB for STDVGA on QEMU_XEN
This could be handled in libxlDomainDeviceDefPostParse(), where we can
check for sane vram values for the various VIR_DOMAIN_VIDEO_TYPE_*, or
set sane defaults if vram is not specified.
BTW, sorry again for the delayed response. I somehow missed your
message and only stumbled across it today :-/. And be warned that any
followup may be delayed as I'll be on vacation July 18-27.
Regards,
Jim
+ } else {
+ libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+ }
+
+ return 0;
+}
+
int
libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
virDomainDefPtr def,
@@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
if (libxlMakePCIList(def, d_config) < 0)
return -1;
+ /*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+ if (libxlMakeVideo(def, d_config) < 0)
+ return -1;
+
d_config->on_reboot = def->onReboot;
d_config->on_poweroff = def->onPoweroff;
d_config->on_crash = def->onCrash;
4:16 p.m.
New subject: [libvirt] [PATCH] libxl: Implement basic video device selection
On 07/16/2014 03:05 PM, Jim Fehlig wrote:
> + b_info->video_memkb = def->videos[0]->vram ?
> + def->videos[0]->vram :
> + LIBXL_MEMKB_DEFAULT;
>
While testing this, I noticed that libvirt will set vram to 9216 if not
specified. E.g.
The 9216 default for qemu is absolutely stupid. No real hardware has a
limit of 9M (8M or 16M are more likely). Please feel free to not
perpetuate that stupidity into libxl.
With type='vga', libxl will then fail to start the domain
libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
videoram must be at least 16 MB for STDVGA on QEMU_XEN
This could be handled in libxlDomainDeviceDefPostParse(), where we can
check for sane vram values for the various VIR_DOMAIN_VIDEO_TYPE_*, or
set sane defaults if vram is not specified.
Sounds like for libxl, a sane default is 16M.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:30 a.m.
New subject: [libvirt] [PATCH] libxl: Implement basic video device selection
On 16.07.2014 23:05, Jim Fehlig wrote:
Stefan Bader wrote:
> being as bad with timely responses. Ok, so how about the following?
>
> One note: it could be the STRDUP's are not strictly needed. But
> to me it felt wrong to have two places refer to the same strings
> (as MakeVFB copies the struct containing the pointers).
Agreed. Without the STRDUP's, seems there is a potential for double
free when libxl_device_vfb and libxl_domain_config objects are disposed.
> If this
> is not needed, then all changes now in MakeVFB probably can be
> dropped (except setting the keyboard layout, maybe; which I
> might miss ;)).
>
> -Stefan
>
>
> >From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader(a)canonical.com>
> Date: Thu, 27 Mar 2014 16:01:18 +0100
> Subject: [PATCH] libxl: Implement basic video device selection
>
> This started as an investigation into an issue where libvirt (using the
> libxl driver) and the Xen host, like an old couple, could not agree on
> who is responsible for selecting the VNC port to use.
>
> Things usually (and a bit surprisingly) did work because, just like that
> old couple, they had the same idea on what to do by default. However it
> was possible that this ended up in a big argument.
>
> The problem is that display information exists in two different places:
> in the vfbs list and in the build info. And for launching the device model,
> only the latter is used. But that never gets initialized from libvirt. So
> Xen allows the device model to select a default port while libvirt thinks
> it has told Xen that this is done by libvirt (though the vfbs config).
>
> While fixing that, I made a stab at actually evaluating the configuration
> of the video device. So that it is now possible to at least decide between
> a Cirrus or standard VGA emulation and to modify the VRAM within certain
> limits using libvirt.
>
> [v2: Check return code of VIR_STRDUP and fix indentation]
> [v3: Split out VRAM fixup and return error for unsupported video type]
> [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
>
[meta-comment]
libvirt prefers patch version history like this to be below the '---'
following your Signed-off-by, so as to not pollute the commit message.
Ah yeah, makes sense. I try to keep it in mind.
> Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
> ---
> src/libxl/libxl_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 8eeaf82..43cabcf 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
> libxl_domain_build_info *b_info = &d_config->b_info;
> libxl_device_vfb vfb = d_config->vfbs[0];
>
> - if (libxl_defbool_val(vfb.vnc.enable))
> + if (libxl_defbool_val(vfb.vnc.enable)) {
> memcpy(&b_info->u.hvm.vnc, &vfb.vnc,
sizeof(libxl_vnc_info));
> - else if (libxl_defbool_val(vfb.sdl.enable))
> + if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
> + goto error;
> + if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
> + goto error;
> + } else if (libxl_defbool_val(vfb.sdl.enable)) {
> memcpy(&b_info->u.hvm.sdl, &vfb.sdl,
sizeof(libxl_sdl_info));
> + if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
> + goto error;
> + if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) <
0)
> + goto error;
> + }
> + if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
> + goto error;
> }
>
> return 0;
> @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
> return NULL;
> }
>
> +static int
> +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> + libxl_domain_build_info *b_info = &d_config->b_info;
> +
> + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM)
> + return 0;
> +
> + /*
> + * Take the first defined video device (graphics card) to display
> + * on the first graphics device (display).
> + * Right now only type and vram info is used and anything beside
> + * type xen and vga is mapped to cirrus.
> + */
> + if (def->nvideos) {
> + switch (def->videos[0]->type) {
> + case VIR_DOMAIN_VIDEO_TYPE_VGA:
> + case VIR_DOMAIN_VIDEO_TYPE_XEN:
> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
> + break;
> + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> + break;
> + default:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s",
> + _("video type not supported by libxl"));
> + return -1;
> + }
> + b_info->video_memkb = def->videos[0]->vram ?
> + def->videos[0]->vram :
> + LIBXL_MEMKB_DEFAULT;
>
While testing this, I noticed that libvirt will set vram to 9216 if not
specified. E.g.
# cat test.xml
...
<video>
<model type='vga'/>
</video>
...
# virsh define test.xml
# virsh dumpxml test
...
<video>
<model type='vga' vram='9216' heads='1'/>
</video>
...
With type='vga', libxl will then fail to start the domain
libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
videoram must be at least 16 MB for STDVGA on QEMU_XEN
Heh, thats "funny". At least this was the same thing I observed when creating
new guests with virt-manager. Maybe I blamed the wrong part of the stack. Or
they both do it. This lead the the second part of my changes which was not so
clear about whether it should or should not go upstream.
So from my side a fixup of some kind would be good but we can work on this in a
follow-up.
This could be handled in libxlDomainDeviceDefPostParse(), where we can
check for sane vram values for the various VIR_DOMAIN_VIDEO_TYPE_*, or
set sane defaults if vram is not specified.
BTW, sorry again for the delayed response. I somehow missed your
message and only stumbled across it today :-/. And be warned that any
followup may be delayed as I'll be on vacation July 18-27.
Oh I am equally bad at having it sit in my reader and then getting back at
random times. :)
-Stefan
Regards,
Jim
> + } else {
> + libxl_defbool_set(&b_info->u.hvm.nographic, 1);
> + }
> +
> + return 0;
> +}
> +
> int
> libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> virDomainDefPtr def,
> @@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> if (libxlMakePCIList(def, d_config) < 0)
> return -1;
>
> + /*
> + * Now that any potential VFBs are defined, it is time to update the
> + * build info with the data of the primary display. Some day libxl
> + * might implicitely do so but as it does not right now, better be
> + * explicit.
> + */
> + if (libxlMakeVideo(def, d_config) < 0)
> + return -1;
> +
> d_config->on_reboot = def->onReboot;
> d_config->on_poweroff = def->onPoweroff;
> d_config->on_crash = def->onCrash;
>
4:31 p.m.
New subject: [libvirt] [PATCH] libxl: Implement basic video device selection
Stefan Bader wrote:
On 16.07.2014 23:05, Jim Fehlig wrote:
> While testing this, I noticed that libvirt will set vram to 9216 if not
> specified. E.g.
>
> # cat test.xml
> ...
> <video>
> <model type='vga'/>
> </video>
> ...
> # virsh define test.xml
> # virsh dumpxml test
> ...
> <video>
> <model type='vga' vram='9216' heads='1'/>
> </video>
> ...
>
> With type='vga', libxl will then fail to start the domain
>
> libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
> videoram must be at least 16 MB for STDVGA on QEMU_XEN
>
Heh, thats "funny". At least this was the same thing I observed when creating
new guests with virt-manager. Maybe I blamed the wrong part of the stack. Or
they both do it. This lead the the second part of my changes which was not so
clear about whether it should or should not go upstream.
So from my side a fixup of some kind would be good but we can work on this in a
follow-up.
The problem is, some domains may not start after applying this patch.
Consider a domain with the following video config
<video>
<model type='vga' vram='9216' heads='1'/>
</video>
This would work pre-patch since libxl_domain_build_info->video_memkb
would be initialized to LIBXL_MEMKB_DEFAULT when calling
libxl_domain_build_info_init(). But the domain will fail to start
post-patch since video_memkb is set to an invalid value.
I know the current behavior is not correct either, but would be nice to
fix everything up in one series. I started on a second patch that would
validate input or set sane defaults in libxlDomainDeviceDefPostParse(),
but while testing realized that sane defaults depend on which qemu is
used. E.g. the minimum video_memkb values have doubled with QEMU_XEN vs
QEMU_XEN_TRADITIONAL - see $xen_root/tools/libxl/libxl_create.c,
libxl__domain_build_info_setdefault(). [I'll mention again that it is
unfortunate that QEMU_XEN vs QEMU_XEN_TRADITIONAL leaked through the
public API.]
I'm thinking of writing a utility function to detect the old vs new
qemu, hoping there is something in the help output or similar to
determine which one <emulator> is. Such a function would be useful for
David Scott's old patch to support arbitrary user-provided <emulator>
https://www.redhat.com/archives/libvir-list/2013-April/msg02119.html
But alas, this will have to wait until I return from vacation.
Regards,
Jim
8:50 a.m.
New subject: [libvirt] [PATCH 2/2] libxl: Fix up VRAM to minimum requirements
This is a bit debatable. On one side it hides configuration errors
in a way that makes them hard to spot. On the other side there is
at least one issue with (maybe some older versions) virt-manager.
Virt-manager sets VRAM directly, not using the default memory setting
but uses too small values for libxl. Worse, those versions do not seem
to allow to change VRAM from the GUI. So switching the video type to
VGA makes the guest fail to start until one manually adapts the VRAM
size in the XML definition.
With this change this would not happen but VRAM will be bigger than
the GUI says. This would not be that different from current Cirrus
behaviour. Only that in that case qemu seems to ignore the provided
size.
Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
---
src/libxl/libxl_conf.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2b5c469..9af8abe 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1316,13 +1316,38 @@ libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config
*d_config)
* type xen and vga is mapped to cirrus.
*/
if (def->nvideos) {
+ unsigned int min_vram = 8 * 1024;
+
switch (def->videos[0]->type) {
case VIR_DOMAIN_VIDEO_TYPE_VGA:
case VIR_DOMAIN_VIDEO_TYPE_XEN:
b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+ /*
+ * Libxl enforces a minimal VRAM size of 8M when using
+ * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+ * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+ * Avoid build failures and go with the minimum if less
+ * is specified.
+ */
+ switch (b_info->device_model_version) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ min_vram = 8 * 1024;
+ break;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ default:
+ min_vram = 16 * 1024;
+ }
break;
case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
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:
+ min_vram = 4 * 1024; /* Actually the max, too */
+ break;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ default:
+ min_vram = 8 * 1024;
+ }
break;
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1330,7 +1355,7 @@ libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config
*d_config)
_("video type not supported by libxl"));
return -1;
}
- b_info->video_memkb = def->videos[0]->vram ?
+ b_info->video_memkb = (def->videos[0]->vram >= min_vram) ?
def->videos[0]->vram :
LIBXL_MEMKB_DEFAULT;
} else {
--
1.7.9.5
5:26 p.m.
New subject: [libvirt] [PATCH 2/2] libxl: Fix up VRAM to minimum requirements
Stefan Bader wrote:
This is a bit debatable. On one side it hides configuration errors
in a way that makes them hard to spot. On the other side there is
at least one issue with (maybe some older versions) virt-manager.
Virt-manager sets VRAM directly, not using the default memory setting
but uses too small values for libxl.
It sounds like virt-manager should be fixed. I don't think the libxl
driver should contain this type of policy. It could report errors on
user-supplied configuration known not to work with libxl, but I don't
think it should be silently changing the configuration.
Regards,
Jim
Worse, those versions do not seem
to allow to change VRAM from the GUI. So switching the video type to
VGA makes the guest fail to start until one manually adapts the VRAM
size in the XML definition.
With this change this would not happen but VRAM will be bigger than
the GUI says. This would not be that different from current Cirrus
behaviour. Only that in that case qemu seems to ignore the provided
size.
Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com>
---
src/libxl/libxl_conf.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2b5c469..9af8abe 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1316,13 +1316,38 @@ libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config
*d_config)
* type xen and vga is mapped to cirrus.
*/
if (def->nvideos) {
+ unsigned int min_vram = 8 * 1024;
+
switch (def->videos[0]->type) {
case VIR_DOMAIN_VIDEO_TYPE_VGA:
case VIR_DOMAIN_VIDEO_TYPE_XEN:
b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+ /*
+ * Libxl enforces a minimal VRAM size of 8M when using
+ * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+ * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+ * Avoid build failures and go with the minimum if less
+ * is specified.
+ */
+ switch (b_info->device_model_version) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ min_vram = 8 * 1024;
+ break;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ default:
+ min_vram = 16 * 1024;
+ }
break;
case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
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:
+ min_vram = 4 * 1024; /* Actually the max, too */
+ break;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ default:
+ min_vram = 8 * 1024;
+ }
break;
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1330,7 +1355,7 @@ libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config
*d_config)
_("video type not supported by libxl"));
return -1;
}
- b_info->video_memkb = def->videos[0]->vram ?
+ b_info->video_memkb = (def->videos[0]->vram >= min_vram) ?
def->videos[0]->vram :
LIBXL_MEMKB_DEFAULT;
} else {
3594
days inactive
3653
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Jim Fehlig -
Stefan Bader