[PATCH 0/5] libxl: user-specified domain config improvements

[PATCH 0/5] libxl: user-specified domain config improvements

[Jim Fehlig]

This is essentially a V2 of Stefan's patch to add support
for user-specified video device in the libxl driver.

https://www.redhat.com/archives/libvir-list/2014-September/msg01157.html

It goes a bit further and adds support for user-specfied <emulator>,
which was trivial to add after introducing libxlDomainGetEmulatorType
in patch 3.  To ease review, the series has been split up as follows:

- Missed keymap config from commit 4dfc34c3 split out to patch 1
- Patch 2 added to defer setting default vram value to the Xen drivers
- Patch 3 added to detect old qemu-dm vs qemu with xen support
- Patch 4 is a slightly reworked version of Stefan's patch
- Patch 5 added to support user-specified <emulator>

Jim Fehlig (4):
  libxl: Copy user-specified keymap to libxl build info struct
  Xen: Defer setting default vram value to Xen drivers
  libxl: Add function to determine device model type
  libxl: Support user-specified <emulator>

Stefan Bader (1):
  libxl: Implement basic video device selection

 src/conf/domain_conf.c   |   4 ++
 src/libxl/libxl_conf.c   | 124 +++++++++++++++++++++++++++++++++++++++++++++++
 src/libxl/libxl_conf.h   |   3 ++
 src/libxl/libxl_domain.c |  22 +++++++++
 src/xen/xen_driver.c     |  19 ++++++++
 5 files changed, 172 insertions(+)

-- 
1.8.4.5

[PATCH 1/5] libxl: Copy user-specified keymap to libxl build info struct

[Jim Fehlig]

Commit 4dfc34c3 missed copying the user-specified keymap to
libxl_domain_build_info struct when creating a VFB device.

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

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index acba69c..0a781f9 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1200,6 +1200,9 @@ 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;
-- 
1.8.4.5

[PATCH 2/5] Xen: Defer setting default vram value to Xen drivers

[Jim Fehlig]

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 */
+            return 64 * 1024;
+        }
+    }
+
     return 0;
 }
 
-- 
1.8.4.5

[PATCH 3/5] libxl: Add function to determine device model type

[Jim Fehlig]

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,
-- 
1.8.4.5

[PATCH 4/5] libxl: Implement basic video device selection

[Jim Fehlig]

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(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index ff3f6b5..09211f8 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1433,6 +1433,72 @@ libxlMakePCIList(virDomainDefPtr def, libxl_domain_config *d_config)
     return -1;
 }
 
+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;
+
+    /*
+     * Take the first defined video device (graphics card) to display
+     * on the first graphics device (display).
+     */
+    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;
+            if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+                if (def->videos[0]->vram < 16 * 1024) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                   _("videoram must be at least 16MB for VGA"));
+                    return -1;
+                }
+            } else {
+                if (def->videos[0]->vram < 8 * 1024) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                   _("videoram must be at least 8MB for VGA"));
+                    return -1;
+                }
+            }
+            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 < 8 * 1024) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                   _("videoram must be at least 8MB for CIRRUS"));
+                    return -1;
+                }
+            } else {
+                if (def->videos[0]->vram < 4 * 1024) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                   _("videoram must be at least 4MB for CIRRUS"));
+                    return -1;
+                }
+            }
+            break;
+
+        default:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("video type %s is not supported by libxl"),
+                           virDomainVideoTypeToString(def->videos[0]->type));
+            return -1;
+        }
+        /* vram validated for each video type, now set it */
+        b_info->video_memkb = def->videos[0]->vram;
+    } else {
+        libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+    }
+
+    return 0;
+}
+
 int
 libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info)
 {
@@ -1523,6 +1589,14 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
     if (libxlMakePCIList(def, d_config) < 0)
         return -1;
 
+    /*
+     * Now that any potential VFBs are defined, 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);
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;
+                }
+        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;
+            }
+        }
+    }
+
     return 0;
 }
 
-- 
1.8.4.5

[PATCH 5/5] libxl: Support user-specified <emulator>

[Jim Fehlig]

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,
-- 
1.8.4.5

Re: [libvirt] [PATCH 0/5] libxl: user-specified domain config improvements

[Michal Privoznik]

On 19.09.2014 21:23, Jim Fehlig wrote:
> This is essentially a V2 of Stefan's patch to add support
> for user-specified video device in the libxl driver.
>
> https://www.redhat.com/archives/libvir-list/2014-September/msg01157.html
>
> It goes a bit further and adds support for user-specfied <emulator>,
> which was trivial to add after introducing libxlDomainGetEmulatorType
> in patch 3.  To ease review, the series has been split up as follows:
>
> - Missed keymap config from commit 4dfc34c3 split out to patch 1
> - Patch 2 added to defer setting default vram value to the Xen drivers
> - Patch 3 added to detect old qemu-dm vs qemu with xen support
> - Patch 4 is a slightly reworked version of Stefan's patch
> - Patch 5 added to support user-specified <emulator>
>
> Jim Fehlig (4):
>    libxl: Copy user-specified keymap to libxl build info struct
>    Xen: Defer setting default vram value to Xen drivers
>    libxl: Add function to determine device model type
>    libxl: Support user-specified <emulator>
>
> Stefan Bader (1):
>    libxl: Implement basic video device selection
>
>   src/conf/domain_conf.c   |   4 ++
>   src/libxl/libxl_conf.c   | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>   src/libxl/libxl_conf.h   |   3 ++
>   src/libxl/libxl_domain.c |  22 +++++++++
>   src/xen/xen_driver.c     |  19 ++++++++
>   5 files changed, 172 insertions(+)
>

ACK series, but please see my inline comments.

Michal

Re: [libvirt] [PATCH 5/5] libxl: Support user-specified <emulator>

[Michal Privoznik]

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

Re: [libvirt] [PATCH 3/5] libxl: Add function to determine device model type

[Michal Privoznik]

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

Re: [libvirt] [PATCH 2/5] Xen: Defer setting default vram value to Xen drivers

[Michal Privoznik]

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

Re: [libvirt] [PATCH 3/5] libxl: Add function to determine device model type

[Jim Fehlig]

Michal Privoznik wrote:
> 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.

Yep, understood.

> But on the other hand, this is trivial compared to virQEMUCaps code
> we've developed.

Right :).  This is only detecting if the qemu is Xen's old fork.  I
really don't know of any other way to do that.

> So I'd give green flag to this.

Thanks!

Regards,
Jim

Re: [libvirt] [PATCH 0/5] libxl: user-specified domain config improvements

[Jim Fehlig]

Michal Privoznik wrote:
> On 19.09.2014 21:23, Jim Fehlig wrote:
>> This is essentially a V2 of Stefan's patch to add support
>> for user-specified video device in the libxl driver.
>>
>> https://www.redhat.com/archives/libvir-list/2014-September/msg01157.html
>>
>> It goes a bit further and adds support for user-specfied <emulator>,
>> which was trivial to add after introducing libxlDomainGetEmulatorType
>> in patch 3.  To ease review, the series has been split up as follows:
>>
>> - Missed keymap config from commit 4dfc34c3 split out to patch 1
>> - Patch 2 added to defer setting default vram value to the Xen drivers
>> - Patch 3 added to detect old qemu-dm vs qemu with xen support
>> - Patch 4 is a slightly reworked version of Stefan's patch
>> - Patch 5 added to support user-specified <emulator>
>>
>> Jim Fehlig (4):
>>    libxl: Copy user-specified keymap to libxl build info struct
>>    Xen: Defer setting default vram value to Xen drivers
>>    libxl: Add function to determine device model type
>>    libxl: Support user-specified <emulator>
>>
>> Stefan Bader (1):
>>    libxl: Implement basic video device selection
>>
>>   src/conf/domain_conf.c   |   4 ++
>>   src/libxl/libxl_conf.c   | 124
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   src/libxl/libxl_conf.h   |   3 ++
>>   src/libxl/libxl_domain.c |  22 +++++++++
>>   src/xen/xen_driver.c     |  19 ++++++++
>>   5 files changed, 172 insertions(+)
>>
>
> ACK series, but please see my inline comments.

Thanks!  I've pushed 1-4, but will repost 5.  I found a small bug while
testing your suggestion to use virFileIsExecutable.

Regards,
Jim

Re: [libvirt] [PATCH 4/5] libxl: Implement basic video device selection

[John Ferlan]

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

> +        }
> +    }
> +
>      return 0;
>  }
>  
> 

Re: [libvirt] [PATCH 4/5] libxl: Implement basic video device selection

[Jim Fehlig]

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