On 19.09.2014 21:23, Jim Fehlig wrote:

Allow the Xen drivers to determine default vram values. Sane default vaules depend on the device model being used, so the drivers are in the best position to determine the defaults.

For the legacy xen driver, it is best to maintain the existing logic for setting default vram values to ensure there are no regressions. The libxl driver currently does not support configuring a video device. Support will be added in a subsequent patch, where the benefit of this change will be reaped.

Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/conf/domain_conf.c | 4 ++++ src/xen/xen_driver.c | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bb4a4cb..1de2650 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9696,6 +9696,10 @@ int virDomainVideoDefaultRAM(const virDomainDef *def, int type) { + /* Defer setting default 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/xen/xen_driver.c b/src/xen/xen_driver.c index 11ae8f9..7438ebb 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -353,6 +353,25 @@ 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: + /* Original Xen PVFB hardcoded to 4 MB */ + dev->data.video->vram = 4 * 1024; + break; + + case VIR_DOMAIN_VIDEO_TYPE_QXL: + /* Use 64M as the minimal video video memory for qxl device */

Indentation's off.

+ return 64 * 1024; + } + } + return 0; }

Michal

On 19.09.2014 21:23, Jim Fehlig wrote:

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).

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/libxl/libxl_conf.c | 32 ++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 3 +++ 2 files changed, 35 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 0a781f9..ff3f6b5 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" @@ -796,6 +797,37 @@ libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard) }

+#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; +} + + int libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) { 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,

Well, we've tried hard to move away from parsing 'qemu -help' output. But on the other hand, this is trivial compared to virQEMUCaps code we've developed. So I'd give green flag to this. Michal

John Ferlan wrote:

Coverity complains... On 09/19/2014 03:23 PM, Jim Fehlig wrote:

...

From: Stefan Bader <stefan.bader@canonical.com>

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> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_domain.c | 22 ++++++++++++++ 2 files changed, 96 insertions(+)

<...snip...>

...

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 557fc20..f2cd07b 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -510,6 +510,28 @@ 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); + + switch (dev->data.video->type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case 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; + }

There's no break here..

...

+ case 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; + }

And of course here - not that it matters yet

Opps. Thanks for catching those. Patch sent https://www.redhat.com/archives/libvir-list/2014-October/msg00486.html Regards, Jim

On 19.09.2014 21:23, Jim Fehlig wrote:

With the introduction of the libxlDomainGetEmulatorType function, it is trivial to support a user-specfied <emulator> in the libxl driver. This patch is based loosely on David Scott's old patch to do the same

https://www.redhat.com/archives/libvir-list/2013-April/msg02119.html

Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 09211f8..882dcff 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -705,6 +705,21 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, if (VIR_STRDUP(b_info->u.hvm.boot, bootorder) < 0) goto error;

+ if (def->emulator) { + if (!virFileExists(def->emulator)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("emulator '%s' not found"), + def->emulator); + goto error; + } + + VIR_FREE(b_info->device_model); + if (VIR_STRDUP(b_info->device_model, def->emulator) < 0) + goto error; + + b_info->device_model_version = libxlDomainGetEmulatorType(def); + } + if (def->nserials) { if (def->nserials > 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

Do we need virFileIsExecutable too? Michal