[libvirt] [PATCH] libxl: fix default vram setting for implicit video device

Commit 6879be48 moved adding of an implicit video device after XML parsing. As a result, libxlDomainDeviceDefPostParse() is no longer called to set the default vram when adding an implicit device. Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the default vram, but it returns 0 if the domain virtType is VIR_DOMAIN_VIRT_XEN. Attempting to start an HVM domain with vram=0 results in error: unsupported configuration: videoram must be at least 4MB for CIRRUS The default vram setting for Xen HVM domains depends on the device model used (qemu-xen vs qemu-traditional), hence setting the default is deferred to libxlDomainDeviceDefPostParse(). This patch addresses the problem during creation of the video device. If vram is 0, it is assumed unset and the default (depending on qemu-xen vs qemu-traditional) is applied. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1334557 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index d927b37..1973ded 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1916,6 +1916,40 @@ libxlMakePCIList(virDomainDefPtr def, libxl_domain_config *d_config) return -1; } +static unsigned int +libxlVideoDefaultRAM(const virDomainVideoType type, int dm_type) +{ + unsigned int ret = 0; + + switch (type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) + ret = 16 * 1024; + else + ret = 8 * 1024; + break; + + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) + ret = 8 * 1024; + else + ret = 4 * 1024; + break; + +#ifdef LIBXL_HAVE_QXL + case VIR_DOMAIN_VIDEO_TYPE_QXL: + ret = 128 * 1024; + break; +#endif + + default: + break; + } + + return ret; +} + static int libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) @@ -1931,6 +1965,10 @@ libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) * on the first graphics device (display). */ if (def->nvideos) { + if (def->videos[0]->vram == 0) + def->videos[0]->vram = libxlVideoDefaultRAM(def->videos[0]->type, + dm_type); + switch (def->videos[0]->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: case VIR_DOMAIN_VIDEO_TYPE_XEN: -- 2.1.4

On Tue, May 10, 2016 at 11:01:17PM -0600, Jim Fehlig wrote:
Commit 6879be48 moved adding of an implicit video device after XML parsing. As a result, libxlDomainDeviceDefPostParse() is no longer called to set the default vram when adding an implicit device. Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the default vram, but it returns 0 if the domain virtType is VIR_DOMAIN_VIRT_XEN.
I think it would be nicer if PostParse would add a proper device with vram set correctly. I have proposed a series that fills (v)ram in post-parse even for implicit devices, would that fix the problem? cover.1462963982.git.jtomko@redhat.com https://www.redhat.com/archives/libvir-list/2016-May/msg00728.html Jan
Attempting to start an HVM domain with vram=0 results in
error: unsupported configuration: videoram must be at least 4MB for CIRRUS
The default vram setting for Xen HVM domains depends on the device model used (qemu-xen vs qemu-traditional), hence setting the default is deferred to libxlDomainDeviceDefPostParse().
This patch addresses the problem during creation of the video device. If vram is 0, it is assumed unset and the default (depending on qemu-xen vs qemu-traditional) is applied.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1334557 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)

On 05/11/2016 06:01 AM, Jim Fehlig wrote:
Commit 6879be48 moved adding of an implicit video device after XML parsing. As a result, libxlDomainDeviceDefPostParse() is no longer called to set the default vram when adding an implicit device. Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the default vram, but it returns 0 if the domain virtType is VIR_DOMAIN_VIRT_XEN. Attempting to start an HVM domain with vram=0 results in
error: unsupported configuration: videoram must be at least 4MB for CIRRUS
The default vram setting for Xen HVM domains depends on the device model used (qemu-xen vs qemu-traditional), hence setting the default is deferred to libxlDomainDeviceDefPostParse().
This patch addresses the problem during creation of the video device. If vram is 0, it is assumed unset and the default (depending on qemu-xen vs qemu-traditional) is applied.
Hmm, What about letting (xen) libxl decide that? If vram is set to 0 we would set video_memkb to LIBXL_MEMKB_DEFAULT and have (xen) libxl decide the defaults and latter update def with the value libxl set back? Altough these values are well established defaults, these might change on libxl in which case prevent domain creation.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1334557 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index d927b37..1973ded 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1916,6 +1916,40 @@ libxlMakePCIList(virDomainDefPtr def, libxl_domain_config *d_config) return -1; }
+static unsigned int +libxlVideoDefaultRAM(const virDomainVideoType type, int dm_type) +{ + unsigned int ret = 0; + + switch (type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) + ret = 16 * 1024; + else + ret = 8 * 1024; + break; + + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) + ret = 8 * 1024; + else + ret = 4 * 1024; + break; + +#ifdef LIBXL_HAVE_QXL + case VIR_DOMAIN_VIDEO_TYPE_QXL: + ret = 128 * 1024; + break; +#endif + + default: + break; + } + + return ret; +} + static int libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
@@ -1931,6 +1965,10 @@ libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) * on the first graphics device (display). */ if (def->nvideos) { + if (def->videos[0]->vram == 0) + def->videos[0]->vram = libxlVideoDefaultRAM(def->videos[0]->type, + dm_type); + switch (def->videos[0]->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: case VIR_DOMAIN_VIDEO_TYPE_XEN:

On 05/11/2016 05:22 AM, Joao Martins wrote:
On 05/11/2016 06:01 AM, Jim Fehlig wrote:
Commit 6879be48 moved adding of an implicit video device after XML parsing. As a result, libxlDomainDeviceDefPostParse() is no longer called to set the default vram when adding an implicit device. Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the default vram, but it returns 0 if the domain virtType is VIR_DOMAIN_VIRT_XEN. Attempting to start an HVM domain with vram=0 results in
error: unsupported configuration: videoram must be at least 4MB for CIRRUS
The default vram setting for Xen HVM domains depends on the device model used (qemu-xen vs qemu-traditional), hence setting the default is deferred to libxlDomainDeviceDefPostParse().
This patch addresses the problem during creation of the video device. If vram is 0, it is assumed unset and the default (depending on qemu-xen vs qemu-traditional) is applied. Hmm, What about letting (xen) libxl decide that? If vram is set to 0 we would set video_memkb to LIBXL_MEMKB_DEFAULT and have (xen) libxl decide the defaults and latter update def with the value libxl set back?
Yeah, that sounds like a good cleanup for the existing code. But I think fixing the logic broken by commit 6879be48 is also in order, which Jan has kindly done in place of this hack https://www.redhat.com/archives/libvir-list/2016-May/msg00728.html
Altough these values are well established defaults, these might change on libxl in which case prevent domain creation.
Yep. Good reason for a follow-up patch along the lines of your suggestion :-). Regards, Jim

On 05/11/2016 11:21 PM, Jim Fehlig wrote:
On 05/11/2016 05:22 AM, Joao Martins wrote:
On 05/11/2016 06:01 AM, Jim Fehlig wrote:
Commit 6879be48 moved adding of an implicit video device after XML parsing. As a result, libxlDomainDeviceDefPostParse() is no longer called to set the default vram when adding an implicit device. Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the default vram, but it returns 0 if the domain virtType is VIR_DOMAIN_VIRT_XEN. Attempting to start an HVM domain with vram=0 results in
error: unsupported configuration: videoram must be at least 4MB for CIRRUS
The default vram setting for Xen HVM domains depends on the device model used (qemu-xen vs qemu-traditional), hence setting the default is deferred to libxlDomainDeviceDefPostParse().
This patch addresses the problem during creation of the video device. If vram is 0, it is assumed unset and the default (depending on qemu-xen vs qemu-traditional) is applied. Hmm, What about letting (xen) libxl decide that? If vram is set to 0 we would set video_memkb to LIBXL_MEMKB_DEFAULT and have (xen) libxl decide the defaults and latter update def with the value libxl set back?
Yeah, that sounds like a good cleanup for the existing code. But I think fixing the logic broken by commit 6879be48 is also in order, which Jan has kindly done in place of this hack
https://www.redhat.com/archives/libvir-list/2016-May/msg00728.html
Yeap, I saw it shortly after my reply.
Altough these values are well established defaults, these might change on libxl in which case prevent domain creation.
Yep. Good reason for a follow-up patch along the lines of your suggestion :-). Ah, sounds good!
Regards, Jim
participants (3)
-
Jim Fehlig
-
Joao Martins
-
Ján Tomko