* Re: [PATCH] libxl: Implement basic video device selection [not found] <1411048512-13045-1-git-send-email-stefan.bader@canonical.com> @ 2014-09-19 3:01 ` Jim Fehlig 2014-09-19 12:52 ` Stefan Bader 0 siblings, 1 reply; 7+ messages in thread From: Jim Fehlig @ 2014-09-19 3:01 UTC (permalink / raw) To: Stefan Bader; +Cc: libvir-list, xen-devel [-- Attachment #1: Type: text/plain, Size: 2547 bytes --] Stefan Bader wrote: > Re-pushing this as the old thread got rather stale. Thanks. > Some of the > VFB setup went in a bug fix. Not sure I missed a detail in rebasing > bug the keyboard setting may be the only thing missing... > Yes, agreed. > -Stefan > > [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] > [v5: Rebased against head which already had some VFB setup code] > > >From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001 > From: Stefan Bader <stefan.bader@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. > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > src/libxl/libxl_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > This patch suffers the same issues as the last version. And when commenting on that version, I promised to work on a followup to address my concerns https://www.redhat.com/archives/libvir-list/2014-July/msg00931.html Your repost poked me into reworking my first attempt, the result of which is below. I should probably look at a sensible split-up of these patches that would be easier to review, but in the meantime comments on my followup would be appreciated. With both patches, my tests are passing and my concerns are subdued :-). Regards, Jim [-- Attachment #2: xen-vram-followup.patch --] [-- Type: text/x-diff, Size: 9149 bytes --] >From 5003420c1e4d22726c596594988169a37544f867 Mon Sep 17 00:00:00 2001 From: Jim Fehlig <jfehlig@suse.com> Date: Thu, 17 Jul 2014 12:00:31 -0600 Subject: [PATCH 2/2] Xen: Improve handling of video device vram The minimum vram values supported by libxl depend upon the device model used. E.g. the minimum values are doubled when using QEMU_XEN, as compared to the old QEMU_XEN_TRADITIONAL. This patch introduces a function to detect whether the specified emulator is QEMU_XEN or QEMU_XEN_TRADITIONAL. Detection is based on the string "Options specific to the Xen version:" in '$qemu -help' output. AFAIK, the only qemu containing that string in help output is the old Xen fork (aka qemu-dm). The detection function is then used to sanity check user-provided vram values, and set appropriate defaults when not provided. For the latter, virDomainVideoDefaultRAM was changed to defer setting the default to the Xen drivers. Note: QEMU_XEN means a qemu that contains support for Xen. QEMU_XEN_TRADITIONAL means Xen's old forked qemu 0.10.2 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/conf/domain_conf.c | 4 +++ src/libxl/libxl_conf.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++-- src/libxl/libxl_conf.h | 3 ++ src/libxl/libxl_domain.c | 21 +++++++++++++ src/xen/xen_driver.c | 18 +++++++++++ 5 files changed, 122 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ccec1c..41be4f4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9683,6 +9683,10 @@ int virDomainVideoDefaultRAM(const virDomainDef *def, int type) { + /* Defer setting vram to the Xen drivers */ + if (def->virtType == VIR_DOMAIN_VIRT_XEN) + return 0; + switch (type) { /* Weird, QEMU defaults to 9 MB ??! */ case VIR_DOMAIN_VIDEO_TYPE_VGA: diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index ce9fafd..5505de8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -40,6 +40,7 @@ #include "viralloc.h" #include "viruuid.h" #include "capabilities.h" +#include "vircommand.h" #include "libxl_domain.h" #include "libxl_conf.h" #include "libxl_utils.h" @@ -492,6 +493,38 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) return 0; } + +#define LIBXL_QEMU_DM_STR "Options specific to the Xen version:" + +int +libxlDomainGetEmulatorType(const virDomainDef *def) +{ + int ret = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; + virCommandPtr cmd = NULL; + char *output = NULL; + + if (STREQ(def->os.type, "hvm")) { + if (def->emulator) { + cmd = virCommandNew(def->emulator); + + virCommandAddArgList(cmd, "-help", NULL); + virCommandSetOutputBuffer(cmd, &output); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (strstr(output, LIBXL_QEMU_DM_STR)) + ret = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; + } + } + + cleanup: + VIR_FREE(output); + virCommandFree(cmd); + return ret; +} + + static int libxlMakeDomCreateInfo(libxl_ctx *ctx, virDomainDefPtr def, @@ -1469,6 +1502,7 @@ static int libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) { libxl_domain_build_info *b_info = &d_config->b_info; + int dm_type = libxlDomainGetEmulatorType(def); if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM) return 0; @@ -1484,9 +1518,51 @@ libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) case VIR_DOMAIN_VIDEO_TYPE_VGA: case VIR_DOMAIN_VIDEO_TYPE_XEN: b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD; + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) { + if (def->videos[0]->vram == 0) { + b_info->video_memkb = 16 * 1024; + } else if (def->videos[0]->vram < 16 * 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("videoram must be at least 16MB for VGA")); + return -1; + } else { + b_info->video_memkb = def->videos[0]->vram; + } + } else { + if (def->videos[0]->vram == 0) { + b_info->video_memkb = 8 * 1024; + } else if (def->videos[0]->vram < 8 * 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("videoram must be at least 8MB for VGA")); + return -1; + } else { + b_info->video_memkb = def->videos[0]->vram; + } + } break; case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) { + if (def->videos[0]->vram == 0) { + b_info->video_memkb = 8 * 1024; + } else if (def->videos[0]->vram < 8 * 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("videoram must be at least 8MB for CIRRUS")); + return -1; + } else { + b_info->video_memkb = def->videos[0]->vram; + } + } else { + if (def->videos[0]->vram == 0) { + b_info->video_memkb = 4 * 1024; + } else if (def->videos[0]->vram < 4 * 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("videoram must be at least 4MB for CIRRUS")); + return -1; + } else { + b_info->video_memkb = def->videos[0]->vram; + } + } break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -1494,9 +1570,6 @@ libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) virDomainVideoTypeToString(def->videos[0]->type)); 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); } diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index da66b4e..25f77ea 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -163,6 +163,9 @@ virCapsPtr libxlMakeCapabilities(libxl_ctx *ctx); int +libxlDomainGetEmulatorType(const virDomainDef *def); + +int libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev); int libxlMakeNic(virDomainDefPtr def, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 557fc20..6515918 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -510,6 +510,27 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; } + if (dev->type == VIR_DOMAIN_DEVICE_VIDEO && STREQ(def->os.type, "hvm")) { + int dm_type = libxlDomainGetEmulatorType(def); + + if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_VGA || + dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_XEN) { + if (dev->data.video->vram == 0) { + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) + dev->data.video->vram = 16 * 1024; + else + dev->data.video->vram = 8 * 1024; + } + } else if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS) { + if (dev->data.video->vram == 0) { + if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) + dev->data.video->vram = 8 * 1024; + else + dev->data.video->vram = 4 * 1024; + } + } + } + return 0; } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 11ae8f9..84ae7f5 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -353,6 +353,24 @@ xenDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } + if (dev->type == VIR_DOMAIN_DEVICE_VIDEO && dev->data.video->vram == 0) { + switch (dev->data.video->type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + case VIR_DOMAIN_VIDEO_TYPE_VMVGA: + dev->data.video->vram = 9 * 1024; + break; + + case VIR_DOMAIN_VIDEO_TYPE_XEN: + dev->data.video->vram = 4 * 1024; + break; + + case VIR_DOMAIN_VIDEO_TYPE_QXL: + /* Use 64M as the minimal video video memory for qxl device */ + return 64 * 1024; + } + } + return 0; } -- 1.8.4.5 [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libxl: Implement basic video device selection 2014-09-19 3:01 ` [PATCH] libxl: Implement basic video device selection Jim Fehlig @ 2014-09-19 12:52 ` Stefan Bader 0 siblings, 0 replies; 7+ messages in thread From: Stefan Bader @ 2014-09-19 12:52 UTC (permalink / raw) To: Jim Fehlig; +Cc: libvir-list, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 3522 bytes --] On 19.09.2014 05:01, Jim Fehlig wrote: > Stefan Bader wrote: >> Re-pushing this as the old thread got rather stale. > > Thanks. > >> Some of the >> VFB setup went in a bug fix. Not sure I missed a detail in rebasing >> bug the keyboard setting may be the only thing missing... >> > > Yes, agreed. > >> -Stefan >> >> [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] >> [v5: Rebased against head which already had some VFB setup code] >> >> >From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001 >> From: Stefan Bader <stefan.bader@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. >> >> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> >> --- >> src/libxl/libxl_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> > > This patch suffers the same issues as the last version. And when > commenting on that version, I promised to work on a followup to address > my concerns > > https://www.redhat.com/archives/libvir-list/2014-July/msg00931.html Oh my, I have to admit I completely forgot about that. > > Your repost poked me into reworking my first attempt, the result of > which is below. I should probably look at a sensible split-up of these > patches that would be easier to review, but in the meantime comments on > my followup would be appreciated. > > With both patches, my tests are passing and my concerns are subdued :-). There seem to be some parts of code suggesting support for anything beside Xen (assumed to be alias to VGA) and Cirrus. As much as I know Xen handles only the two (not qxl or anything else). I believe the xen specific qemu variant was the only one called qemu-dm (and the upstream variant always being qemu-system-i386). But checking the help message probably is safer and not called that often so I should not worry about performance). I tried the variant you proposed and it looks to be ok. Still have to carry a piece which is not going upstream if I wan't to paper over the virt-manager issue. But at least now the failure is clearly showing what is wrong. So I am happy enough. :) -Stefan > > Regards, > Jim > > [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] libxl: Implement basic video device selection @ 2014-09-18 13:55 Stefan Bader 0 siblings, 0 replies; 7+ messages in thread From: Stefan Bader @ 2014-09-18 13:55 UTC (permalink / raw) To: libvir-list, xen-devel; +Cc: jfehlig Re-pushing this as the old thread got rather stale. Some of the VFB setup went in a bug fix. Not sure I missed a detail in rebasing bug the keyboard setting may be the only thing missing... -Stefan [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] [v5: Rebased against head which already had some VFB setup code] >From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001 From: Stefan Bader <stefan.bader@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. Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/libxl/libxl_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index acba69c..2727230 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1200,6 +1200,8 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, 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; @@ -1462,6 +1464,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, @@ -1488,6 +1529,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 = libxlActionFromVirLifecycle(def->onReboot); d_config->on_poweroff = libxlActionFromVirLifecycle(def->onPoweroff); d_config->on_crash = libxlActionFromVirLifecycleCrash(def->onCrash); -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <5387B626.3040407@suse.com>]
* [PATCH] libxl: Implement basic video device selection [not found] <5387B626.3040407@suse.com> @ 2014-07-01 7:58 ` Stefan Bader 2014-07-16 21:05 ` Jim Fehlig [not found] ` <53C6E91E.9010500@suse.com> 0 siblings, 2 replies; 7+ messages in thread From: Stefan Bader @ 2014-07-01 7:58 UTC (permalink / raw) To: jfehlig; +Cc: libvir-list, xen-devel 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@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@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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libxl: Implement basic video device selection 2014-07-01 7:58 ` Stefan Bader @ 2014-07-16 21:05 ` Jim Fehlig [not found] ` <53C6E91E.9010500@suse.com> 1 sibling, 0 replies; 7+ messages in thread From: Jim Fehlig @ 2014-07-16 21:05 UTC (permalink / raw) To: Stefan Bader; +Cc: libvir-list, xen-devel 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@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@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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <53C6E91E.9010500@suse.com>]
* Re: [PATCH] libxl: Implement basic video device selection [not found] ` <53C6E91E.9010500@suse.com> @ 2014-07-17 9:30 ` Stefan Bader 2014-07-17 21:31 ` Jim Fehlig 0 siblings, 1 reply; 7+ messages in thread From: Stefan Bader @ 2014-07-17 9:30 UTC (permalink / raw) To: Jim Fehlig; +Cc: libvir-list, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 7905 bytes --] 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@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@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; >> [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 884 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libxl: Implement basic video device selection 2014-07-17 9:30 ` Stefan Bader @ 2014-07-17 21:31 ` Jim Fehlig 0 siblings, 0 replies; 7+ messages in thread From: Jim Fehlig @ 2014-07-17 21:31 UTC (permalink / raw) To: Stefan Bader; +Cc: libvir-list, xen-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-19 12:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1411048512-13045-1-git-send-email-stefan.bader@canonical.com>
2014-09-19 3:01 ` [PATCH] libxl: Implement basic video device selection Jim Fehlig
2014-09-19 12:52 ` Stefan Bader
2014-09-18 13:55 Stefan Bader
[not found] <5387B626.3040407@suse.com>
2014-07-01 7:58 ` Stefan Bader
2014-07-16 21:05 ` Jim Fehlig
[not found] ` <53C6E91E.9010500@suse.com>
2014-07-17 9:30 ` Stefan Bader
2014-07-17 21:31 ` Jim Fehlig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).