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

2014-09-19 Thread Stefan Bader 
On 19.09.2014 05:01, Jim Fehlig wrote:
 Stefan Bader wrote:
 Re-pushing this as the old thread got rather stale.
 
 Thanks.
 
  Some of the
 VFB setup went in a bug fix. Not sure I missed a detail in rebasing
 bug the keyboard setting may be the only thing missing...
   
 
 Yes, agreed.
 
 -Stefan

 [v2: Check return code of VIR_STRDUP and fix indentation]
 [v3: Split out VRAM fixup and return error for unsupported video type]
 [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
 [v5: Rebased against head which already had some VFB setup code]

 From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@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.ba...@canonical.com
 ---
  src/libxl/libxl_conf.c | 50 
 ++
  1 file changed, 50 insertions(+)
   
 
 This patch suffers the same issues as the last version.  And when
 commenting on that version, I promised to work on a followup to address
 my concerns
 
 https://www.redhat.com/archives/libvir-list/2014-July/msg00931.html

Oh my, I have to admit I completely forgot about that.

 
 Your repost poked me into reworking my first attempt, the result of
 which is below.  I should probably look at a sensible split-up of these
 patches that would be easier to review, but in the meantime comments on
 my followup would be appreciated.
 
 With both patches, my tests are passing and my concerns are subdued :-).

There seem to be some parts of code suggesting support for anything beside Xen
(assumed to be alias to VGA) and Cirrus. As much as I know Xen handles only the
two (not qxl or anything else).

I believe the xen specific qemu variant was the only one called qemu-dm (and the
upstream variant always being qemu-system-i386). But checking the help message
probably is safer and not called that often so I should not worry about
performance).

I tried the variant you proposed and it looks to be ok. Still have to carry a
piece which is not going upstream if I wan't to paper over the virt-manager
issue. But at least now the failure is clearly showing what is wrong. So I am
happy enough. :)

-Stefan
 
 Regards,
 Jim
  
 




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-18 Thread Stefan Bader 
Re-pushing this as the old thread got rather stale. Some of the
VFB setup went in a bug fix. Not sure I missed a detail in rebasing
bug the keyboard setting may be the only thing missing...

-Stefan

[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
[v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
[v5: Rebased against head which already had some VFB setup code]

From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@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.ba...@canonical.com
---
 src/libxl/libxl_conf.c | 50 ++
 1 file changed, 50 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index acba69c..2727230 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1200,6 +1200,8 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
 if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb.sdl.xauthority)  
0)
 goto error;
 }
+if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
+goto error;
 }
 
 return 0;
@@ -1462,6 +1464,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static int
+libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
+return 0;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+break;
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+break;
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   %s,
+   _(video type not supported by libxl));
+return -1;
+}
+b_info-video_memkb = def-videos[0]-vram ?
+  def-videos[0]-vram :
+  LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+return 0;
+}
+
 int
 libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
virDomainDefPtr def,
@@ -1488,6 +1529,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 if (libxlMakePCIList(def, d_config)  0)
 return -1;
 
+/*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+if (libxlMakeVideo(def, d_config)  0)
+return -1;
+
 d_config-on_reboot = libxlActionFromVirLifecycle(def-onReboot);
 d_config-on_poweroff = libxlActionFromVirLifecycle(def-onPoweroff);
 d_config-on_crash = libxlActionFromVirLifecycleCrash(def-onCrash);
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-18 Thread Jim Fehlig 
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.ba...@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.ba...@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
 

From 5003420c1e4d22726c596594988169a37544f867 Mon Sep 17 00:00:00 2001
From: Jim Fehlig jfeh...@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 jfeh...@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 =

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

2014-07-17 Thread Stefan Bader 
On 16.07.2014 23:05, Jim Fehlig wrote:
 Stefan Bader wrote:
 being as bad with timely responses. Ok, so how about the following?

 One note: it could be the STRDUP's are not strictly needed. But
 to me it felt wrong to have two places refer to the same strings
 (as MakeVFB copies the struct containing the pointers).
 
 Agreed.  Without the STRDUP's, seems there is a potential for double
 free when libxl_device_vfb and libxl_domain_config objects are disposed.
 
  If this
 is not needed, then all changes now in MakeVFB probably can be
 dropped (except setting the keyboard layout, maybe; which I
 might miss ;)).

 -Stefan


 From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Thu, 27 Mar 2014 16:01:18 +0100
 Subject: [PATCH] libxl: Implement basic video device selection

 This started as an investigation into an issue where libvirt (using the
 libxl driver) and the Xen host, like an old couple, could not agree on
 who is responsible for selecting the VNC port to use.

 Things usually (and a bit surprisingly) did work because, just like that
 old couple, they had the same idea on what to do by default. However it
 was possible that this ended up in a big argument.

 The problem is that display information exists in two different places:
 in the vfbs list and in the build info. And for launching the device model,
 only the latter is used. But that never gets initialized from libvirt. So
 Xen allows the device model to select a default port while libvirt thinks
 it has told Xen that this is done by libvirt (though the vfbs config).

 While fixing that, I made a stab at actually evaluating the configuration
 of the video device. So that it is now possible to at least decide between
 a Cirrus or standard VGA emulation and to modify the VRAM within certain
 limits using libvirt.

 [v2: Check return code of VIR_STRDUP and fix indentation]
 [v3: Split out VRAM fixup and return error for unsupported video type]
 [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
   
 
 [meta-comment]
 libvirt prefers patch version history like this to be below  the '---'
 following your Signed-off-by, so as to not pollute the commit message.

Ah yeah, makes sense. I try to keep it in mind.

 
 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/libxl/libxl_conf.c |   63 
 ++--
  1 file changed, 61 insertions(+), 2 deletions(-)

 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index 8eeaf82..43cabcf 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
  libxl_domain_build_info *b_info = d_config-b_info;
  libxl_device_vfb vfb = d_config-vfbs[0];
  
 -if (libxl_defbool_val(vfb.vnc.enable))
 +if (libxl_defbool_val(vfb.vnc.enable)) {
  memcpy(b_info-u.hvm.vnc, vfb.vnc, sizeof(libxl_vnc_info));
 -else if (libxl_defbool_val(vfb.sdl.enable))
 +if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb.vnc.listen)  0)
 +goto error;
 +if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb.vnc.passwd)  0)
 +goto error;
 +} else if (libxl_defbool_val(vfb.sdl.enable)) {
  memcpy(b_info-u.hvm.sdl, vfb.sdl, sizeof(libxl_sdl_info));
 +if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb.sdl.display)  0)
 +goto error;
 +if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, 
 vfb.sdl.xauthority)  0)
 +goto error;
 +}
 +if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
 +goto error;
  }
  
  return 0;
 @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
  return NULL;
  }
  
 +static int
 +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
 +{
 +libxl_domain_build_info *b_info = d_config-b_info;
 +
 +if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
 +return 0;
 +
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +break;
 +case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
 +break;
 +default:
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +   %s,
 +   _(video type not supported by libxl));
 +return -1;
 +}
 +b_info-video_memkb =

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

2014-07-17 Thread Jim Fehlig 
Stefan Bader wrote:
 On 16.07.2014 23:05, Jim Fehlig wrote:
   
 While testing this, I noticed that libvirt will set vram to 9216 if not
 specified.  E.g.

 # cat test.xml
 ...
 video
   model type='vga'/
 /video
 ...
 # virsh define test.xml
 # virsh dumpxml test
 ...
 video
   model type='vga' vram='9216' heads='1'/
 /video
 ...

 With type='vga', libxl will then fail to start the domain

 libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
 videoram must be at least 16 MB for STDVGA on QEMU_XEN
 

 Heh, thats funny. At least this was the same thing I observed when creating
 new guests with virt-manager. Maybe I blamed the wrong part of the stack. Or
 they both do it. This lead the the second part of my changes which was not so
 clear about whether it should or should not go upstream.

 So from my side a fixup of some kind would be good but we can work on this in 
 a
 follow-up.
   

The problem is, some domains may not start after applying this patch. 
Consider a domain with the following video config

video
  model type='vga' vram='9216' heads='1'/
/video

This would work pre-patch since libxl_domain_build_info-video_memkb
would be initialized to LIBXL_MEMKB_DEFAULT when calling
libxl_domain_build_info_init().  But the domain will fail to start
post-patch since video_memkb is set to an invalid value.

I know the current behavior is not correct either, but would be nice to
fix everything up in one series.  I started on a second patch that would
validate input or set sane defaults in libxlDomainDeviceDefPostParse(),
but while testing realized that sane defaults depend on which qemu is
used.  E.g. the minimum video_memkb values have doubled with QEMU_XEN vs
QEMU_XEN_TRADITIONAL - see $xen_root/tools/libxl/libxl_create.c,
libxl__domain_build_info_setdefault().  [I'll mention again that it is
unfortunate that QEMU_XEN vs QEMU_XEN_TRADITIONAL leaked through the
public API.]

I'm thinking of writing a utility function to detect the old vs new
qemu, hoping there is something in the help output or similar to
determine which one emulator is.  Such a function would be useful for
David Scott's old patch to support arbitrary user-provided emulator

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

But alas, this will have to wait until I return from vacation.

Regards,
Jim


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-07-16 Thread Jim Fehlig 
Stefan Bader wrote:
 being as bad with timely responses. Ok, so how about the following?

 One note: it could be the STRDUP's are not strictly needed. But
 to me it felt wrong to have two places refer to the same strings
 (as MakeVFB copies the struct containing the pointers).

Agreed.  Without the STRDUP's, seems there is a potential for double
free when libxl_device_vfb and libxl_domain_config objects are disposed.

  If this
 is not needed, then all changes now in MakeVFB probably can be
 dropped (except setting the keyboard layout, maybe; which I
 might miss ;)).

 -Stefan


 From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Thu, 27 Mar 2014 16:01:18 +0100
 Subject: [PATCH] libxl: Implement basic video device selection

 This started as an investigation into an issue where libvirt (using the
 libxl driver) and the Xen host, like an old couple, could not agree on
 who is responsible for selecting the VNC port to use.

 Things usually (and a bit surprisingly) did work because, just like that
 old couple, they had the same idea on what to do by default. However it
 was possible that this ended up in a big argument.

 The problem is that display information exists in two different places:
 in the vfbs list and in the build info. And for launching the device model,
 only the latter is used. But that never gets initialized from libvirt. So
 Xen allows the device model to select a default port while libvirt thinks
 it has told Xen that this is done by libvirt (though the vfbs config).

 While fixing that, I made a stab at actually evaluating the configuration
 of the video device. So that it is now possible to at least decide between
 a Cirrus or standard VGA emulation and to modify the VRAM within certain
 limits using libvirt.

 [v2: Check return code of VIR_STRDUP and fix indentation]
 [v3: Split out VRAM fixup and return error for unsupported video type]
 [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
   

[meta-comment]
libvirt prefers patch version history like this to be below  the '---'
following your Signed-off-by, so as to not pollute the commit message.

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/libxl/libxl_conf.c |   63 
 ++--
  1 file changed, 61 insertions(+), 2 deletions(-)

 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index 8eeaf82..43cabcf 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
  libxl_domain_build_info *b_info = d_config-b_info;
  libxl_device_vfb vfb = d_config-vfbs[0];
  
 -if (libxl_defbool_val(vfb.vnc.enable))
 +if (libxl_defbool_val(vfb.vnc.enable)) {
  memcpy(b_info-u.hvm.vnc, vfb.vnc, sizeof(libxl_vnc_info));
 -else if (libxl_defbool_val(vfb.sdl.enable))
 +if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb.vnc.listen)  0)
 +goto error;
 +if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb.vnc.passwd)  0)
 +goto error;
 +} else if (libxl_defbool_val(vfb.sdl.enable)) {
  memcpy(b_info-u.hvm.sdl, vfb.sdl, sizeof(libxl_sdl_info));
 +if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb.sdl.display)  0)
 +goto error;
 +if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb.sdl.xauthority) 
  0)
 +goto error;
 +}
 +if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
 +goto error;
  }
  
  return 0;
 @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
  return NULL;
  }
  
 +static int
 +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
 +{
 +libxl_domain_build_info *b_info = d_config-b_info;
 +
 +if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
 +return 0;
 +
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +break;
 +case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
 +break;
 +default:
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +   %s,
 +   _(video type not supported by libxl));
 +return -1;
 +}
 +b_info-video_memkb = def-videos[0]-vram ?
 +  def-videos[0]-vram :
 +

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

2014-07-16 Thread Eric Blake 
On 07/16/2014 03:05 PM, Jim Fehlig wrote:

 +b_info-video_memkb = def-videos[0]-vram ?
 +  def-videos[0]-vram :
 +  LIBXL_MEMKB_DEFAULT;
   
 
 While testing this, I noticed that libvirt will set vram to 9216 if not
 specified.  E.g.

The 9216 default for qemu is absolutely stupid.  No real hardware has a
limit of 9M (8M or 16M are more likely).  Please feel free to not
perpetuate that stupidity into libxl.


 With type='vga', libxl will then fail to start the domain
 
 libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
 videoram must be at least 16 MB for STDVGA on QEMU_XEN
 
 This could be handled in libxlDomainDeviceDefPostParse(), where we can
 check for sane vram values for the various VIR_DOMAIN_VIDEO_TYPE_*, or
 set sane defaults if vram is not specified.

Sounds like for libxl, a sane default is 16M.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-07-01 Thread Stefan Bader 
being as bad with timely responses. Ok, so how about the following?

One note: it could be the STRDUP's are not strictly needed. But
to me it felt wrong to have two places refer to the same strings
(as MakeVFB copies the struct containing the pointers). If this
is not needed, then all changes now in MakeVFB probably can be
dropped (except setting the keyboard layout, maybe; which I
might miss ;)).

-Stefan


From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Thu, 27 Mar 2014 16:01:18 +0100
Subject: [PATCH] libxl: Implement basic video device selection

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
[v4: Re-arrange code and move VFB setup into libxlMakeVfbList]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   63 ++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8eeaf82..43cabcf 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
 libxl_domain_build_info *b_info = d_config-b_info;
 libxl_device_vfb vfb = d_config-vfbs[0];
 
-if (libxl_defbool_val(vfb.vnc.enable))
+if (libxl_defbool_val(vfb.vnc.enable)) {
 memcpy(b_info-u.hvm.vnc, vfb.vnc, sizeof(libxl_vnc_info));
-else if (libxl_defbool_val(vfb.sdl.enable))
+if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb.vnc.listen)  0)
+goto error;
+if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb.vnc.passwd)  0)
+goto error;
+} else if (libxl_defbool_val(vfb.sdl.enable)) {
 memcpy(b_info-u.hvm.sdl, vfb.sdl, sizeof(libxl_sdl_info));
+if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb.sdl.display)  0)
+goto error;
+if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb.sdl.xauthority)  
0)
+goto error;
+}
+if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
+goto error;
 }
 
 return 0;
@@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static int
+libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
+return 0;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+break;
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+break;
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   %s,
+   _(video type not supported by libxl));
+return -1;
+}
+b_info-video_memkb = def-videos[0]-vram ?
+  def-videos[0]-vram :
+  LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+return 0;
+}
+
 int
 libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
virDomainDefPtr def,
@@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 if (libxlMakePCIList(def, d_config)  0)
 return -1;
 
+/*
+ * Now that any potential VFBs are defined, it