[PATCH 1/3] libxl: Use id from virDomainObj inside the driver

[PATCH 1/3] libxl: Use id from virDomainObj inside the driver

[Stefan Bader]

There is a domain id in the virDomain structure as well as in the
virDomainObj structure. While the former can become stale the latter
is kept up to date. So it is safer to always (virDomainObjPtr)->def->id
internally.

This will fix issues seen when managing Xen guests through libvirt from
virt-manager (not being able to get domain info after define or reboot).
This was caused both though libxlDomainGetInfo() only but there were
a lot of places that might potentially cause issues, too.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 src/libxl/libxl_driver.c |   75 +++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index fc97db4..b5061df 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -770,10 +770,10 @@ libxlDomainSuspend(virDomainPtr dom)
     priv = vm->privateData;
 
     if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
-        if (libxl_domain_pause(priv->ctx, dom->id) != 0) {
+        if (libxl_domain_pause(priv->ctx, vm->def->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Failed to suspend domain '%d' with libxenlight"),
-                           dom->id);
+                           vm->def->id);
             goto endjob;
         }
 
@@ -829,10 +829,10 @@ libxlDomainResume(virDomainPtr dom)
     priv = vm->privateData;
 
     if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
-        if (libxl_domain_unpause(priv->ctx, dom->id) != 0) {
+        if (libxl_domain_unpause(priv->ctx, vm->def->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Failed to resume domain '%d' with libxenlight"),
-                           dom->id);
+                           vm->def->id);
             goto endjob;
         }
 
@@ -883,10 +883,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
     }
 
     priv = vm->privateData;
-    if (libxl_domain_shutdown(priv->ctx, dom->id) != 0) {
+    if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to shutdown domain '%d' with libxenlight"),
-                       dom->id);
+                       vm->def->id);
         goto cleanup;
     }
 
@@ -930,10 +930,10 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     }
 
     priv = vm->privateData;
-    if (libxl_domain_reboot(priv->ctx, dom->id) != 0) {
+    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to reboot domain '%d' with libxenlight"),
-                       dom->id);
+                       vm->def->id);
         goto cleanup;
     }
     ret = 0;
@@ -974,7 +974,7 @@ libxlDomainDestroyFlags(virDomainPtr dom,
     priv = vm->privateData;
     if (libxl_domain_destroy(priv->ctx, vm->def->id, NULL) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Failed to destroy domain '%d'"), dom->id);
+                       _("Failed to destroy domain '%d'"), vm->def->id);
         goto cleanup;
     }
 
@@ -1105,10 +1105,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
 
         if (flags & VIR_DOMAIN_MEM_LIVE) {
             priv = vm->privateData;
-            if (libxl_domain_setmaxmem(priv->ctx, dom->id, newmem) < 0) {
+            if (libxl_domain_setmaxmem(priv->ctx, vm->def->id, newmem) < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Failed to set maximum memory for domain '%d'"
-                                 " with libxenlight"), dom->id);
+                                 " with libxenlight"), vm->def->id);
                 goto endjob;
             }
         }
@@ -1138,13 +1138,13 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
             priv = vm->privateData;
             /* Unlock virDomainObj while ballooning memory */
             virObjectUnlock(vm);
-            res = libxl_set_memory_target(priv->ctx, dom->id, newmem, 0,
+            res = libxl_set_memory_target(priv->ctx, vm->def->id, newmem, 0,
                                           /* force */ 1);
             virObjectLock(vm);
             if (res < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Failed to set memory for domain '%d'"
-                                 " with libxenlight"), dom->id);
+                                 " with libxenlight"), vm->def->id);
                 goto endjob;
             }
         }
@@ -1202,9 +1202,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
         info->memory = vm->def->mem.cur_balloon;
         info->maxMem = vm->def->mem.max_balloon;
     } else {
-        if (libxl_domain_info(priv->ctx, &d_info, dom->id) != 0) {
+        if (libxl_domain_info(priv->ctx, &d_info, vm->def->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("libxl_domain_info failed for domain '%d'"), dom->id);
+                           _("libxl_domain_info failed for domain '%d'"),
+                           vm->def->id);
             goto cleanup;
         }
         info->cpuTime = d_info.cpu_time;
@@ -1483,11 +1484,11 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
 
     if (!(flags & VIR_DUMP_LIVE) &&
         virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
-        if (libxl_domain_pause(priv->ctx, dom->id) != 0) {
+        if (libxl_domain_pause(priv->ctx, vm->def->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Before dumping core, failed to suspend domain '%d'"
                              " with libxenlight"),
-                           dom->id);
+                           vm->def->id);
             goto endjob;
         }
         virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP);
@@ -1496,20 +1497,20 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
 
     /* Unlock virDomainObj while dumping core */
     virObjectUnlock(vm);
-    ret = libxl_domain_core_dump(priv->ctx, dom->id, to, NULL);
+    ret = libxl_domain_core_dump(priv->ctx, vm->def->id, to, NULL);
     virObjectLock(vm);
     if (ret != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to dump core of domain '%d' with libxenlight"),
-                       dom->id);
+                       vm->def->id);
         ret = -1;
         goto unpause;
     }
 
     if (flags & VIR_DUMP_CRASH) {
-        if (libxl_domain_destroy(priv->ctx, dom->id, NULL) < 0) {
+        if (libxl_domain_destroy(priv->ctx, vm->def->id, NULL) < 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Failed to destroy domain '%d'"), dom->id);
+                           _("Failed to destroy domain '%d'"), vm->def->id);
             goto unpause;
         }
 
@@ -1524,10 +1525,10 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
 
  unpause:
     if (virDomainObjIsActive(vm) && paused) {
-        if (libxl_domain_unpause(priv->ctx, dom->id) != 0) {
+        if (libxl_domain_unpause(priv->ctx, vm->def->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("After dumping core, failed to resume domain '%d' with"
-                             " libxenlight"), dom->id);
+                             " libxenlight"), vm->def->id);
         } else {
             virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
                                  VIR_DOMAIN_RUNNING_UNPAUSED);
@@ -1786,19 +1787,19 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
         break;
 
     case VIR_DOMAIN_VCPU_LIVE:
-        if (libxl_set_vcpuonline(priv->ctx, dom->id, &map) != 0) {
+        if (libxl_set_vcpuonline(priv->ctx, vm->def->id, &map) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Failed to set vcpus for domain '%d'"
-                             " with libxenlight"), dom->id);
+                             " with libxenlight"), vm->def->id);
             goto endjob;
         }
         break;
 
     case VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG:
-        if (libxl_set_vcpuonline(priv->ctx, dom->id, &map) != 0) {
+        if (libxl_set_vcpuonline(priv->ctx, vm->def->id, &map) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Failed to set vcpus for domain '%d'"
-                             " with libxenlight"), dom->id);
+                             " with libxenlight"), vm->def->id);
             goto endjob;
         }
         def->vcpus = nvcpus;
@@ -1934,7 +1935,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
         libxlDomainObjPrivatePtr priv;
 
         priv = vm->privateData;
-        if (libxl_set_vcpuaffinity(priv->ctx, dom->id, vcpu, &map) != 0) {
+        if (libxl_set_vcpuaffinity(priv->ctx, vm->def->id, vcpu, &map) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Failed to pin vcpu '%d' with libxenlight"),
                            vcpu);
@@ -2099,11 +2100,11 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo,
     }
 
     priv = vm->privateData;
-    if ((vcpuinfo = libxl_list_vcpu(priv->ctx, dom->id, &maxcpu,
+    if ((vcpuinfo = libxl_list_vcpu(priv->ctx, vm->def->id, &maxcpu,
                                     &hostcpus)) == NULL) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to list vcpus for domain '%d' with libxenlight"),
-                       dom->id);
+                       vm->def->id);
         goto cleanup;
     }
 
@@ -3608,7 +3609,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
     default:
         virReportError(VIR_ERR_INTERNAL_ERROR,
                    _("Failed to get scheduler id for domain '%d'"
-                     " with libxenlight"), dom->id);
+                     " with libxenlight"), vm->def->id);
         goto cleanup;
     }
 
@@ -3659,10 +3660,10 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
         goto cleanup;
     }
 
-    if (libxl_domain_sched_params_get(priv->ctx, dom->id, &sc_info) != 0) {
+    if (libxl_domain_sched_params_get(priv->ctx, vm->def->id, &sc_info) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to get scheduler parameters for domain '%d'"
-                         " with libxenlight"), dom->id);
+                         " with libxenlight"), vm->def->id);
         goto cleanup;
     }
 
@@ -3740,10 +3741,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom,
         goto endjob;
     }
 
-    if (libxl_domain_sched_params_get(priv->ctx, dom->id, &sc_info) != 0) {
+    if (libxl_domain_sched_params_get(priv->ctx, vm->def->id, &sc_info) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to get scheduler parameters for domain '%d'"
-                         " with libxenlight"), dom->id);
+                         " with libxenlight"), vm->def->id);
         goto endjob;
     }
 
@@ -3756,10 +3757,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom,
             sc_info.cap = params[i].value.ui;
     }
 
-    if (libxl_domain_sched_params_set(priv->ctx, dom->id, &sc_info) != 0) {
+    if (libxl_domain_sched_params_set(priv->ctx, vm->def->id, &sc_info) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to set scheduler parameters for domain '%d'"
-                         " with libxenlight"), dom->id);
+                         " with libxenlight"), vm->def->id);
         goto endjob;
     }
 
-- 
1.7.9.5

Re: [libvirt] [PATCH 1/3] libxl: Use id from virDomainObj inside the driver

[Michal Privoznik]

On 27.03.2014 17:55, Stefan Bader wrote:
> There is a domain id in the virDomain structure as well as in the
> virDomainObj structure. While the former can become stale the latter
> is kept up to date. So it is safer to always (virDomainObjPtr)->def->id
> internally.
>
> This will fix issues seen when managing Xen guests through libvirt from
> virt-manager (not being able to get domain info after define or reboot).
> This was caused both though libxlDomainGetInfo() only but there were
> a lot of places that might potentially cause issues, too.
>
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>   src/libxl/libxl_driver.c |   75 +++++++++++++++++++++++-----------------------
>   1 file changed, 38 insertions(+), 37 deletions(-)

Oh, you've sent the patch again. This time as a part of bigger set. 
Anyway, my ACK stands.

Michal

[PATCH 2/3] libxl: Set disk format for empty cdrom device

[Stefan Bader]

The XML config for a CDROM device can be without a source path,
indicating that there is no media present. Without this change
the libxl driver fails to start a guest in that case because
the libxl library checks for the LIBXL_DISK_FORMAT_EMPTY format
type and tries to stat the NULL pointer that gets passed on.

> libxl: error: libxl_device.c:265:libxl__device_disk_set_backend:
> Disk vdev=hdc failed to stat: (null): Bad address

Signed-off-by: Stefan Bader <stefan.bader@canonical.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 de6f7ce..b8de72a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -827,6 +827,9 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
     x_disk->removable = 1;
     x_disk->readwrite = !l_disk->readonly;
     x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0;
+    /* An empty CDROM must have the empty format, otherwise libxl fails. */
+    if (x_disk->is_cdrom && !x_disk->pdev_path)
+        x_disk->format = LIBXL_DISK_FORMAT_EMPTY;
     if (l_disk->transient) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("libxenlight does not support transient disks"));
-- 
1.7.9.5

Re: [libvirt] [PATCH 2/3] libxl: Set disk format for empty cdrom device

[Michal Privoznik]

On 27.03.2014 17:55, Stefan Bader wrote:
> The XML config for a CDROM device can be without a source path,
> indicating that there is no media present. Without this change
> the libxl driver fails to start a guest in that case because
> the libxl library checks for the LIBXL_DISK_FORMAT_EMPTY format
> type and tries to stat the NULL pointer that gets passed on.
>
>> libxl: error: libxl_device.c:265:libxl__device_disk_set_backend:
>> Disk vdev=hdc failed to stat: (null): Bad address
>
> Signed-off-by: Stefan Bader <stefan.bader@canonical.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 de6f7ce..b8de72a 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -827,6 +827,9 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
>       x_disk->removable = 1;
>       x_disk->readwrite = !l_disk->readonly;
>       x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0;
> +    /* An empty CDROM must have the empty format, otherwise libxl fails. */
> +    if (x_disk->is_cdrom && !x_disk->pdev_path)
> +        x_disk->format = LIBXL_DISK_FORMAT_EMPTY;
>       if (l_disk->transient) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("libxenlight does not support transient disks"));
>

ACK

Michal

[PATCH 3/3] libxl: Implement basic video device selection

[Stefan Bader]

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 |   85 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b8de72a..fdd2726 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1298,6 +1298,82 @@ libxlMakeCapabilities(libxl_ctx *ctx)
     return NULL;
 }
 
+static void
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+    libxl_domain_build_info *b_info = &d_config->b_info;
+
+    /*
+     * 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) {
+        unsigned int min_vram = 8 * 1024;
+
+        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;
+                /*
+                 * Libxl enforces a minimal VRAM size of 8M when using
+                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+                 * Avoid build failures and go with the minimum if less
+                 * is specified.
+                 */
+                switch (b_info->device_model_version) {
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                        min_vram = 8 * 1024;
+                        break;
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    default:
+                        min_vram = 16 * 1024;
+                }
+                break;
+            default:
+                /*
+                 * Ignore any other device type and use Cirrus. Again fix
+                 * up the minimal VRAM to what libxl expects.
+                 */
+                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+                switch (b_info->device_model_version) {
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                        min_vram = 4 * 1024; /* Actually the max, too */
+                        break;
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    default:
+                        min_vram = 8 * 1024;
+                }
+        }
+        b_info->video_memkb = (def->videos[0]->vram > min_vram) ?
+                               def->videos[0]->vram :
+                               LIBXL_MEMKB_DEFAULT;
+    } else {
+        libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+    }
+
+    /*
+     * When making the list of displays, only VNC and SDL types were
+     * taken into account. So it seems sensible to connect the default
+     * video device to the first in the vfb list.
+     *
+     * FIXME: Copy the structures and fixing the strings feels a bit dirty.
+     */
+    if (d_config->num_vfbs) {
+        libxl_device_vfb *vfb0 = &d_config->vfbs[0];
+
+	b_info->u.hvm.vnc = vfb0->vnc;
+        VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen);
+        VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd);
+        b_info->u.hvm.sdl = vfb0->sdl;
+        VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display);
+        VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority);
+        VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap);
+    }
+}
+
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
                        virDomainObjPtr vm, libxl_domain_config *d_config)
@@ -1325,6 +1401,15 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
     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 (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
+        libxlSetBuildGraphics(def, d_config);
+
     d_config->on_reboot = def->onReboot;
     d_config->on_poweroff = def->onPoweroff;
     d_config->on_crash = def->onCrash;
-- 
1.7.9.5

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

[Michal Privoznik]

On 27.03.2014 17:55, Stefan Bader wrote:
> 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 |   85 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 85 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b8de72a..fdd2726 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1298,6 +1298,82 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>       return NULL;
>   }
>
> +static void
> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> +    libxl_domain_build_info *b_info = &d_config->b_info;
> +
> +    /*
> +     * 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) {
> +        unsigned int min_vram = 8 * 1024;
> +
> +        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;
> +                /*
> +                 * Libxl enforces a minimal VRAM size of 8M when using
> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> +                 * Avoid build failures and go with the minimum if less
> +                 * is specified.
> +                 */
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 8 * 1024;
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 16 * 1024;
> +                }
> +                break;
> +            default:
> +                /*
> +                 * Ignore any other device type and use Cirrus. Again fix
> +                 * up the minimal VRAM to what libxl expects.
> +                 */
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 4 * 1024; /* Actually the max, too */
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 8 * 1024;
> +                }
> +        }
> +        b_info->video_memkb = (def->videos[0]->vram > min_vram) ?
> +                               def->videos[0]->vram :
> +                               LIBXL_MEMKB_DEFAULT;
> +    } else {
> +        libxl_defbool_set(&b_info->u.hvm.nographic, 1);
> +    }
> +
> +    /*
> +     * When making the list of displays, only VNC and SDL types were
> +     * taken into account. So it seems sensible to connect the default
> +     * video device to the first in the vfb list.
> +     *
> +     * FIXME: Copy the structures and fixing the strings feels a bit dirty.
> +     */
> +    if (d_config->num_vfbs) {
> +        libxl_device_vfb *vfb0 = &d_config->vfbs[0];
> +
> +	b_info->u.hvm.vnc = vfb0->vnc;
> +        VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen);
> +        VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd);
> +        b_info->u.hvm.sdl = vfb0->sdl;
> +        VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display);
> +        VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority);
> +        VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap);

You need to check for the return value of VIR_STRDUP(). Moreover, the 
indentation seems off. Looking forward to v2.

Michal

Re: libxl fixes/improvements for libvirt

[Jim Fehlig]

On 03/27/2014 10:55 AM, Stefan Bader wrote:
> Here several changes which improve the handling of Xen for me:

Thanks for the improvements Stefan.  I'm currently on bereavement leave and will 
look at these patches when returning, unless someone beats me to it.

Regards,
Jim

>
> * 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
>    This is a re-send as I initially submitted that as a reply to some
>    discussion. Starting from the visibly broken libxlDomainGetInfo when
>    creating or rebooting a guest with virt-manager, I replaced all usage
>    of dom->id with the more stable vm->def->id.
> * 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
>    Fixes start of a guest after ejecting the iso image from a virtual
>    CDROM drive (again using virt-manager).
> * 0003-libxl-Implement-basic-video-device-selection.patch
>    Should fix the occasional disagreement about the used VNC port and
>    adds the ability to select the standard VGA graphics from Xen.
>    VRAM size seemed to work with that. Only for Cirrus, while the qemu
>    command-line looks good, ones seems to end up with 32M.
>
> Note to people on the Xen list: I wonder whether libxenlight internally
> should somehow should fix up the situation where a caller sets up the
> video devices in the vfbs list but does not sync that with the information
> in the build info. Question probably is what the semantics should be.
>
> -Stefan
>
> .

Re: [libvirt] libxl fixes/improvements for libvirt

[Michal Privoznik]

On 27.03.2014 17:55, Stefan Bader wrote:
> Here several changes which improve the handling of Xen for me:
>
> * 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
>    This is a re-send as I initially submitted that as a reply to some
>    discussion. Starting from the visibly broken libxlDomainGetInfo when
>    creating or rebooting a guest with virt-manager, I replaced all usage
>    of dom->id with the more stable vm->def->id.
> * 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
>    Fixes start of a guest after ejecting the iso image from a virtual
>    CDROM drive (again using virt-manager).
> * 0003-libxl-Implement-basic-video-device-selection.patch
>    Should fix the occasional disagreement about the used VNC port and
>    adds the ability to select the standard VGA graphics from Xen.
>    VRAM size seemed to work with that. Only for Cirrus, while the qemu
>    command-line looks good, ones seems to end up with 32M.
>
> Note to people on the Xen list: I wonder whether libxenlight internally
> should somehow should fix up the situation where a caller sets up the
> video devices in the vfbs list but does not sync that with the information
> in the build info. Question probably is what the semantics should be.
>
> -Stefan
>

Just for the record, I've pushed the first two patches. The last one has 
some issues, so I rather see it's second version.

Michal

Re: [libvirt] libxl fixes/improvements for libvirt

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 1669 bytes --]

On 03.04.2014 17:45, Michal Privoznik wrote:
> On 27.03.2014 17:55, Stefan Bader wrote:
>> Here several changes which improve the handling of Xen for me:
>>
>> * 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
>>    This is a re-send as I initially submitted that as a reply to some
>>    discussion. Starting from the visibly broken libxlDomainGetInfo when
>>    creating or rebooting a guest with virt-manager, I replaced all usage
>>    of dom->id with the more stable vm->def->id.
>> * 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
>>    Fixes start of a guest after ejecting the iso image from a virtual
>>    CDROM drive (again using virt-manager).
>> * 0003-libxl-Implement-basic-video-device-selection.patch
>>    Should fix the occasional disagreement about the used VNC port and
>>    adds the ability to select the standard VGA graphics from Xen.
>>    VRAM size seemed to work with that. Only for Cirrus, while the qemu
>>    command-line looks good, ones seems to end up with 32M.
>>
>> Note to people on the Xen list: I wonder whether libxenlight internally
>> should somehow should fix up the situation where a caller sets up the
>> video devices in the vfbs list but does not sync that with the information
>> in the build info. Question probably is what the semantics should be.
>>
>> -Stefan
>>
> 
> Just for the record, I've pushed the first two patches. The last one has some
> issues, so I rather see it's second version.
> 
> Michal
> 
Ack, thanks. I will send out an updated version as soon as I get it done. Next
on my list though I cannot exactly tell when "next" happens. :)

-Stefan


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2] libxl: Implement basic video device selection

[Stefan Bader]

Ok, I think indentation should be ok like that and I added error
handling for VIR_STRDUP. I think just returning -1 like the other
places should be ok as the only caller looks to handle the disposal
of the config structure.

-Stefan

---

>From dfe579003e91137ecd824d2a08bcdc8f18725857 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 |   93 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b8de72a..042f4da 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1298,6 +1298,89 @@ libxlMakeCapabilities(libxl_ctx *ctx)
     return NULL;
 }
 
+static int
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+    libxl_domain_build_info *b_info = &d_config->b_info;
+
+    /*
+     * 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) {
+        unsigned int min_vram = 8 * 1024;
+
+        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;
+                /*
+                 * Libxl enforces a minimal VRAM size of 8M when using
+                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+                 * Avoid build failures and go with the minimum if less
+                 * is specified.
+                 */
+                switch (b_info->device_model_version) {
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                        min_vram = 8 * 1024;
+                        break;
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    default:
+                        min_vram = 16 * 1024;
+                }
+                break;
+            default:
+                /*
+                 * Ignore any other device type and use Cirrus. Again fix
+                 * up the minimal VRAM to what libxl expects.
+                 */
+                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+                switch (b_info->device_model_version) {
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                        min_vram = 4 * 1024; /* Actually the max, too */
+                        break;
+                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    default:
+                        min_vram = 8 * 1024;
+                }
+        }
+        b_info->video_memkb = (def->videos[0]->vram > min_vram) ?
+                               def->videos[0]->vram :
+                               LIBXL_MEMKB_DEFAULT;
+    } else {
+        libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+    }
+
+    /*
+     * When making the list of displays, only VNC and SDL types were
+     * taken into account. So it seems sensible to connect the default
+     * video device to the first in the vfb list.
+     *
+     * FIXME: Copy the structures and fixing the strings feels a bit dirty.
+     */
+    if (d_config->num_vfbs) {
+        libxl_device_vfb *vfb0 = &d_config->vfbs[0];
+
+        b_info->u.hvm.vnc = vfb0->vnc;
+        if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen) < 0)
+            return -1;
+        if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd) < 0)
+            return -1;
+        b_info->u.hvm.sdl = vfb0->sdl;
+        if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display) < 0)
+            return -1;
+        if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority) < 0)
+            return -1;
+        if (VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
                        virDomainObjPtr vm, libxl_domain_config *d_config)
@@ -1325,6 +1408,16 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
     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 (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
+        if (libxlSetBuildGraphics(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

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

[Ian Campbell]

On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
> +    /*
> +     * 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) {
> +        unsigned int min_vram = 8 * 1024;
> +
> +        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;
> +                /*
> +                 * Libxl enforces a minimal VRAM size of 8M when using
> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> +                 * Avoid build failures and go with the minimum if less
> +                 * is specified.

Build failures? Do you mean "domain build" rather than "libvirt build"?

I'm not sure about this fixing up the GIGO from the user side, but
that's a libvirt policy decision I suppose? If it were me I would just
let libxl provide the error and expect people to fix their domain config
rather than silently giving them something other than what they asked
for. What if increasing the VRAM causes a cascading failure due e.g. to
lack of memory? That's going to be tricky to debug I think!

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

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 2349 bytes --]

On 04.04.2014 11:48, Ian Campbell wrote:
> On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
>> +    /*
>> +     * 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) {
>> +        unsigned int min_vram = 8 * 1024;
>> +
>> +        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;
>> +                /*
>> +                 * Libxl enforces a minimal VRAM size of 8M when using
>> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
>> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
>> +                 * Avoid build failures and go with the minimum if less
>> +                 * is specified.
> 
> Build failures? Do you mean "domain build" rather than "libvirt build"?
> 
> I'm not sure about this fixing up the GIGO from the user side, but
> that's a libvirt policy decision I suppose? If it were me I would just
> let libxl provide the error and expect people to fix their domain config
> rather than silently giving them something other than what they asked
> for. What if increasing the VRAM causes a cascading failure due e.g. to
> lack of memory? That's going to be tricky to debug I think!

In the end its a start a domain with such a config. Which seems to be what I
would end up with in my testing with an admittedly older version of virt-manager
connecting to a libvirtd running an initial version of this patch without that part.
The error seen on the front-end was something along the lines of "failed to get
enough memory to start the guest" (the libxl log on the other side had the
better error message). And the gui always reduces the memory below the minimum
for both the options (VGA and "Xen").
That is the reason I went for "meh, go for the minimum anyways".

And btw, it is already confusing enough as with cirrus, I get a 9M by default
which is passed on to qemu on the command line and then ignored by it and one
gets 32M in any way...

-Stefan

> 
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
> On 04.04.2014 11:48, Ian Campbell wrote:
> > On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
> >> +    /*
> >> +     * 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) {
> >> +        unsigned int min_vram = 8 * 1024;
> >> +
> >> +        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;
> >> +                /*
> >> +                 * Libxl enforces a minimal VRAM size of 8M when using
> >> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> >> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> >> +                 * Avoid build failures and go with the minimum if less
> >> +                 * is specified.
> > 
> > Build failures? Do you mean "domain build" rather than "libvirt build"?
> > 
> > I'm not sure about this fixing up the GIGO from the user side, but
> > that's a libvirt policy decision I suppose? If it were me I would just
> > let libxl provide the error and expect people to fix their domain config
> > rather than silently giving them something other than what they asked
> > for. What if increasing the VRAM causes a cascading failure due e.g. to
> > lack of memory? That's going to be tricky to debug I think!
> 
> In the end its a start a domain with such a config. Which seems to be what I
> would end up with in my testing with an admittedly older version of virt-manager
> connecting to a libvirtd running an initial version of this patch without that part.
> The error seen on the front-end was something along the lines of "failed to get
> enough memory to start the guest" (the libxl log on the other side had the
> better error message). And the gui always reduces the memory below the minimum
> for both the options (VGA and "Xen").
> That is the reason I went for "meh, go for the minimum anyways".

Does the libvirt protocol require the client to provide all the sizes
etc with no provision for asking the server side to pick a sane default?

> And btw, it is already confusing enough as with cirrus, I get a 9M by default
> which is passed on to qemu on the command line and then ignored by it and one
> gets 32M in any way...

Lovely!

Ian.

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

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 2718 bytes --]

On 04.04.2014 12:31, Stefan Bader wrote:
> On 04.04.2014 11:48, Ian Campbell wrote:
>> On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
>>> +    /*
>>> +     * 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) {
>>> +        unsigned int min_vram = 8 * 1024;
>>> +
>>> +        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;
>>> +                /*
>>> +                 * Libxl enforces a minimal VRAM size of 8M when using
>>> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
>>> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
>>> +                 * Avoid build failures and go with the minimum if less
>>> +                 * is specified.
>>
>> Build failures? Do you mean "domain build" rather than "libvirt build"?
>>
>> I'm not sure about this fixing up the GIGO from the user side, but
>> that's a libvirt policy decision I suppose? If it were me I would just
>> let libxl provide the error and expect people to fix their domain config
>> rather than silently giving them something other than what they asked
>> for. What if increasing the VRAM causes a cascading failure due e.g. to
>> lack of memory? That's going to be tricky to debug I think!
> 
> In the end its a start a domain with such a config. Which seems to be what I

*sigh* That wasn't much better English. Yes, I meant a domain build or a failure
to start a domain...

> would end up with in my testing with an admittedly older version of virt-manager
> connecting to a libvirtd running an initial version of this patch without that part.
> The error seen on the front-end was something along the lines of "failed to get
> enough memory to start the guest" (the libxl log on the other side had the
> better error message). And the gui always reduces the memory below the minimum
> for both the options (VGA and "Xen").
> That is the reason I went for "meh, go for the minimum anyways".
> 
> And btw, it is already confusing enough as with cirrus, I get a 9M by default
> which is passed on to qemu on the command line and then ignored by it and one
> gets 32M in any way...
> 
> -Stefan
> 
>>
>>
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 3178 bytes --]

On 04.04.2014 12:34, Ian Campbell wrote:
> On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
>> On 04.04.2014 11:48, Ian Campbell wrote:
>>> On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
>>>> +    /*
>>>> +     * 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) {
>>>> +        unsigned int min_vram = 8 * 1024;
>>>> +
>>>> +        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;
>>>> +                /*
>>>> +                 * Libxl enforces a minimal VRAM size of 8M when using
>>>> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
>>>> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
>>>> +                 * Avoid build failures and go with the minimum if less
>>>> +                 * is specified.
>>>
>>> Build failures? Do you mean "domain build" rather than "libvirt build"?
>>>
>>> I'm not sure about this fixing up the GIGO from the user side, but
>>> that's a libvirt policy decision I suppose? If it were me I would just
>>> let libxl provide the error and expect people to fix their domain config
>>> rather than silently giving them something other than what they asked
>>> for. What if increasing the VRAM causes a cascading failure due e.g. to
>>> lack of memory? That's going to be tricky to debug I think!
>>
>> In the end its a start a domain with such a config. Which seems to be what I
>> would end up with in my testing with an admittedly older version of virt-manager
>> connecting to a libvirtd running an initial version of this patch without that part.
>> The error seen on the front-end was something along the lines of "failed to get
>> enough memory to start the guest" (the libxl log on the other side had the
>> better error message). And the gui always reduces the memory below the minimum
>> for both the options (VGA and "Xen").
>> That is the reason I went for "meh, go for the minimum anyways".
> 
> Does the libvirt protocol require the client to provide all the sizes
> etc with no provision for asking the server side to pick a sane default?

I think libvirt itself would be ok with a config that does not enforce a certain
amount of VRAM (libvirt devs correct me if I am talking non-sense here).
But together with the virt-manager front-end that tries to be "helpful" halfway.
I get a incorrect setting which I cannot change from that version of the gui.
Again, never versions might be better.
Just practically I expect some crazy people (like me *cough*) to try this from
an older Desktop release...

> 
>> And btw, it is already confusing enough as with cirrus, I get a 9M by default
>> which is passed on to qemu on the command line and then ignored by it and one
>> gets 32M in any way...
> 
> Lovely!
> 
> Ian.
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Daniel P. Berrange]

On Fri, Apr 04, 2014 at 11:34:17AM +0100, Ian Campbell wrote:
> On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
> > On 04.04.2014 11:48, Ian Campbell wrote:
> > > On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
> > >> +    /*
> > >> +     * 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) {
> > >> +        unsigned int min_vram = 8 * 1024;
> > >> +
> > >> +        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;
> > >> +                /*
> > >> +                 * Libxl enforces a minimal VRAM size of 8M when using
> > >> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> > >> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> > >> +                 * Avoid build failures and go with the minimum if less
> > >> +                 * is specified.
> > > 
> > > Build failures? Do you mean "domain build" rather than "libvirt build"?
> > > 
> > > I'm not sure about this fixing up the GIGO from the user side, but
> > > that's a libvirt policy decision I suppose? If it were me I would just
> > > let libxl provide the error and expect people to fix their domain config
> > > rather than silently giving them something other than what they asked
> > > for. What if increasing the VRAM causes a cascading failure due e.g. to
> > > lack of memory? That's going to be tricky to debug I think!
> > 
> > In the end its a start a domain with such a config. Which seems to be what I
> > would end up with in my testing with an admittedly older version of virt-manager
> > connecting to a libvirtd running an initial version of this patch without that part.
> > The error seen on the front-end was something along the lines of "failed to get
> > enough memory to start the guest" (the libxl log on the other side had the
> > better error message). And the gui always reduces the memory below the minimum
> > for both the options (VGA and "Xen").
> > That is the reason I went for "meh, go for the minimum anyways".
> 
> Does the libvirt protocol require the client to provide all the sizes
> etc with no provision for asking the server side to pick a sane default?

The XML does not have to include the VGA ram size. If it is omitted the
we fill in a default value after initial parsing is done. That defualt
can be hypervisor specific

> 
> > And btw, it is already confusing enough as with cirrus, I get a 9M by default
> > which is passed on to qemu on the command line and then ignored by it and one
> > gets 32M in any way...

Yeah, we were silently ignoring ram size for cirrus since QEMU doesn't let
it be configured. Only QXL and I think stdvga is configurable.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Ian Campbell]

On Fri, 2014-04-04 at 14:51 +0200, Daniel P. Berrange wrote:
> On Fri, Apr 04, 2014 at 11:34:17AM +0100, Ian Campbell wrote:
> > On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
> > > On 04.04.2014 11:48, Ian Campbell wrote:
> > > > On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
> > > >> +    /*
> > > >> +     * 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) {
> > > >> +        unsigned int min_vram = 8 * 1024;
> > > >> +
> > > >> +        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;
> > > >> +                /*
> > > >> +                 * Libxl enforces a minimal VRAM size of 8M when using
> > > >> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> > > >> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> > > >> +                 * Avoid build failures and go with the minimum if less
> > > >> +                 * is specified.
> > > > 
> > > > Build failures? Do you mean "domain build" rather than "libvirt build"?
> > > > 
> > > > I'm not sure about this fixing up the GIGO from the user side, but
> > > > that's a libvirt policy decision I suppose? If it were me I would just
> > > > let libxl provide the error and expect people to fix their domain config
> > > > rather than silently giving them something other than what they asked
> > > > for. What if increasing the VRAM causes a cascading failure due e.g. to
> > > > lack of memory? That's going to be tricky to debug I think!
> > > 
> > > In the end its a start a domain with such a config. Which seems to be what I
> > > would end up with in my testing with an admittedly older version of virt-manager
> > > connecting to a libvirtd running an initial version of this patch without that part.
> > > The error seen on the front-end was something along the lines of "failed to get
> > > enough memory to start the guest" (the libxl log on the other side had the
> > > better error message). And the gui always reduces the memory below the minimum
> > > for both the options (VGA and "Xen").
> > > That is the reason I went for "meh, go for the minimum anyways".
> > 
> > Does the libvirt protocol require the client to provide all the sizes
> > etc with no provision for asking the server side to pick a sane default?
> 
> The XML does not have to include the VGA ram size. If it is omitted the
> we fill in a default value after initial parsing is done.

I guess the issue is that whatever client Stefan is using is including
the VGA ram size, with a value which it turns out is not allowed.

> That defualt can be hypervisor specific

Great!

Ian.
> > > And btw, it is already confusing enough as with cirrus, I get a 9M by default
> > > which is passed on to qemu on the command line and then ignored by it and one
> > > gets 32M in any way...
> 
> Yeah, we were silently ignoring ram size for cirrus since QEMU doesn't let
> it be configured. Only QXL and I think stdvga is configurable.
> 
> 
> Regards,
> Daniel

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

[Daniel P. Berrange]

On Fri, Apr 04, 2014 at 01:56:09PM +0100, Ian Campbell wrote:
> On Fri, 2014-04-04 at 14:51 +0200, Daniel P. Berrange wrote:
> > On Fri, Apr 04, 2014 at 11:34:17AM +0100, Ian Campbell wrote:
> > > On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
> > > > On 04.04.2014 11:48, Ian Campbell wrote:
> > > > > On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
> > > > >> +    /*
> > > > >> +     * 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) {
> > > > >> +        unsigned int min_vram = 8 * 1024;
> > > > >> +
> > > > >> +        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;
> > > > >> +                /*
> > > > >> +                 * Libxl enforces a minimal VRAM size of 8M when using
> > > > >> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> > > > >> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> > > > >> +                 * Avoid build failures and go with the minimum if less
> > > > >> +                 * is specified.
> > > > > 
> > > > > Build failures? Do you mean "domain build" rather than "libvirt build"?
> > > > > 
> > > > > I'm not sure about this fixing up the GIGO from the user side, but
> > > > > that's a libvirt policy decision I suppose? If it were me I would just
> > > > > let libxl provide the error and expect people to fix their domain config
> > > > > rather than silently giving them something other than what they asked
> > > > > for. What if increasing the VRAM causes a cascading failure due e.g. to
> > > > > lack of memory? That's going to be tricky to debug I think!
> > > > 
> > > > In the end its a start a domain with such a config. Which seems to be what I
> > > > would end up with in my testing with an admittedly older version of virt-manager
> > > > connecting to a libvirtd running an initial version of this patch without that part.
> > > > The error seen on the front-end was something along the lines of "failed to get
> > > > enough memory to start the guest" (the libxl log on the other side had the
> > > > better error message). And the gui always reduces the memory below the minimum
> > > > for both the options (VGA and "Xen").
> > > > That is the reason I went for "meh, go for the minimum anyways".
> > > 
> > > Does the libvirt protocol require the client to provide all the sizes
> > > etc with no provision for asking the server side to pick a sane default?
> > 
> > The XML does not have to include the VGA ram size. If it is omitted the
> > we fill in a default value after initial parsing is done.
> 
> I guess the issue is that whatever client Stefan is using is including
> the VGA ram size, with a value which it turns out is not allowed.

Yeah, that's probably a historical accident in virt-manager trying to guess
the default values itself rather than delegating.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 3973 bytes --]

On 04.04.2014 14:56, Ian Campbell wrote:
> On Fri, 2014-04-04 at 14:51 +0200, Daniel P. Berrange wrote:
>> On Fri, Apr 04, 2014 at 11:34:17AM +0100, Ian Campbell wrote:
>>> On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
>>>> On 04.04.2014 11:48, Ian Campbell wrote:
>>>>> On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
>>>>>> +    /*
>>>>>> +     * 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) {
>>>>>> +        unsigned int min_vram = 8 * 1024;
>>>>>> +
>>>>>> +        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;
>>>>>> +                /*
>>>>>> +                 * Libxl enforces a minimal VRAM size of 8M when using
>>>>>> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
>>>>>> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
>>>>>> +                 * Avoid build failures and go with the minimum if less
>>>>>> +                 * is specified.
>>>>>
>>>>> Build failures? Do you mean "domain build" rather than "libvirt build"?
>>>>>
>>>>> I'm not sure about this fixing up the GIGO from the user side, but
>>>>> that's a libvirt policy decision I suppose? If it were me I would just
>>>>> let libxl provide the error and expect people to fix their domain config
>>>>> rather than silently giving them something other than what they asked
>>>>> for. What if increasing the VRAM causes a cascading failure due e.g. to
>>>>> lack of memory? That's going to be tricky to debug I think!
>>>>
>>>> In the end its a start a domain with such a config. Which seems to be what I
>>>> would end up with in my testing with an admittedly older version of virt-manager
>>>> connecting to a libvirtd running an initial version of this patch without that part.
>>>> The error seen on the front-end was something along the lines of "failed to get
>>>> enough memory to start the guest" (the libxl log on the other side had the
>>>> better error message). And the gui always reduces the memory below the minimum
>>>> for both the options (VGA and "Xen").
>>>> That is the reason I went for "meh, go for the minimum anyways".
>>>
>>> Does the libvirt protocol require the client to provide all the sizes
>>> etc with no provision for asking the server side to pick a sane default?
>>
>> The XML does not have to include the VGA ram size. If it is omitted the
>> we fill in a default value after initial parsing is done.
> 
> I guess the issue is that whatever client Stefan is using is including
> the VGA ram size, with a value which it turns out is not allowed.

Right and the current fixup code is in there because I am too lazy to be
bothered to use virsh to fix up the vram size all the times. And in some way I
expected other users to be of the same mind. Or even not to find out what went
wrong at all.
I am open to let this drop on the upstream side and only carry the delta
locally. Whichever sounds more suitable for the upstream maintainers.

-Stefan
> 
>> That defualt can be hypervisor specific
> 
> Great!
> 
> Ian.
>>>> And btw, it is already confusing enough as with cirrus, I get a 9M by default
>>>> which is passed on to qemu on the command line and then ignored by it and one
>>>> gets 32M in any way...
>>
>> Yeah, we were silently ignoring ram size for cirrus since QEMU doesn't let
>> it be configured. Only QXL and I think stdvga is configurable.
>>
>>
>> Regards,
>> Daniel
> 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Daniel P. Berrange]

On Fri, Apr 04, 2014 at 11:36:39AM +0200, Stefan Bader wrote:
> +static int
> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> +    libxl_domain_build_info *b_info = &d_config->b_info;
> +
> +    /*
> +     * 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) {
> +        unsigned int min_vram = 8 * 1024;
> +
> +        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;
> +                /*
> +                 * Libxl enforces a minimal VRAM size of 8M when using
> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> +                 * Avoid build failures and go with the minimum if less
> +                 * is specified.
> +                 */
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 8 * 1024;
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 16 * 1024;
> +                }
> +                break;
> +            default:
> +                /*
> +                 * Ignore any other device type and use Cirrus. Again fix
> +                 * up the minimal VRAM to what libxl expects.
> +                 */

We shouldn't do that 'default'. For any device type that Xen can't support
we should report  VIR_ERR_CONFIG_UNSUPPORTED.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 2572 bytes --]

On 04.04.2014 15:17, Daniel P. Berrange wrote:
> On Fri, Apr 04, 2014 at 11:36:39AM +0200, Stefan Bader wrote:
>> +static int
>> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
>> +{
>> +    libxl_domain_build_info *b_info = &d_config->b_info;
>> +
>> +    /*
>> +     * 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) {
>> +        unsigned int min_vram = 8 * 1024;
>> +
>> +        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;
>> +                /*
>> +                 * Libxl enforces a minimal VRAM size of 8M when using
>> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
>> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
>> +                 * Avoid build failures and go with the minimum if less
>> +                 * is specified.
>> +                 */
>> +                switch (b_info->device_model_version) {
>> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
>> +                        min_vram = 8 * 1024;
>> +                        break;
>> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>> +                    default:
>> +                        min_vram = 16 * 1024;
>> +                }
>> +                break;
>> +            default:
>> +                /*
>> +                 * Ignore any other device type and use Cirrus. Again fix
>> +                 * up the minimal VRAM to what libxl expects.
>> +                 */
> 
> We shouldn't do that 'default'. For any device type that Xen can't support
> we should report  VIR_ERR_CONFIG_UNSUPPORTED.

Ok, I could remove that. At some point it might be possible to enhance that
again. But that requires more careful thinking. At least with
device_model_version QEMU_XEN (not QEMU_XEN_TRADITIONAL and not to confuse with
device_model which just sets the path ;)) at least in theory there should be the
same support as for kvm... Anyway, just loud thinking.

Would you also rather want the VRAM fixup to be removed as well? I probably wait
a bit more for other feedback and then would do a v3...

-Stefan
> 
> Regards,
> Daniel
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Daniel P. Berrange]

On Fri, Apr 04, 2014 at 03:36:38PM +0200, Stefan Bader wrote:
> On 04.04.2014 15:17, Daniel P. Berrange wrote:
> > On Fri, Apr 04, 2014 at 11:36:39AM +0200, Stefan Bader wrote:
> >> +static int
> >> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
> >> +{
> >> +    libxl_domain_build_info *b_info = &d_config->b_info;
> >> +
> >> +    /*
> >> +     * 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) {
> >> +        unsigned int min_vram = 8 * 1024;
> >> +
> >> +        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;
> >> +                /*
> >> +                 * Libxl enforces a minimal VRAM size of 8M when using
> >> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> >> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> >> +                 * Avoid build failures and go with the minimum if less
> >> +                 * is specified.
> >> +                 */
> >> +                switch (b_info->device_model_version) {
> >> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> >> +                        min_vram = 8 * 1024;
> >> +                        break;
> >> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> >> +                    default:
> >> +                        min_vram = 16 * 1024;
> >> +                }
> >> +                break;
> >> +            default:
> >> +                /*
> >> +                 * Ignore any other device type and use Cirrus. Again fix
> >> +                 * up the minimal VRAM to what libxl expects.
> >> +                 */
> > 
> > We shouldn't do that 'default'. For any device type that Xen can't support
> > we should report  VIR_ERR_CONFIG_UNSUPPORTED.
> 
> Ok, I could remove that. At some point it might be possible to enhance that
> again. But that requires more careful thinking. At least with
> device_model_version QEMU_XEN (not QEMU_XEN_TRADITIONAL and not to confuse with
> device_model which just sets the path ;)) at least in theory there should be the
> same support as for kvm... Anyway, just loud thinking.
> 
> Would you also rather want the VRAM fixup to be removed as well? I probably wait
> a bit more for other feedback and then would do a v3...

I'd suggest, do a first patch which removes the 'default:' case and sets an
error and honours the VRAM size. Then do a second patch which adds the VRAM
workaround, so we can discuss it separately from the main enablement.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|