Re: [PATCH] libxl: Implement basic video device selection

* 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