8:20 a.m.
So this is an old, tiny series that was posted some time ago; actually
a lot of time ago. I complained because of two things. First one
being that there are no tests, so this series has them in. The second
one was that this could break migration from old code since the
parameter was not used before. So I coded up a migration flag,
handled all the special cases and then started testing it. And while
testing it, I've found out that QEMU doesn't care at all about this
parameter being there and there is no point in keeping it
super-stable, especially when it can be changed on the fly. So I
removed all that code and ended up with tiny series very similar to
previous attempts by various people.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283207
Martin Kletzander (2):
qemu: Check for qxl's max_outputs parameter
qemu: Add support to QXL's max_outputs parameter
src/qemu/qemu_capabilities.c | 4 ++
src/qemu/qemu_capabilities.h | 2 +
src/qemu/qemu_command.c | 8 ++++
.../qemuxml2argv-video-qxl-heads.args | 28 +++++++++++++
.../qemuxml2argv-video-qxl-heads.xml | 47 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 8 ++++
.../qemuxml2xmlout-video-qxl-heads.xml | 47 ++++++++++++++++++++++
tests/qemuxml2xmltest.c | 2 +
8 files changed, 146 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-heads.xml
--
2.7.2
8:21 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Check for qxl's max_outputs parameter
There is max_outputs parameter for both qxl and qxl-vga and there is no
easy way of saying that we want the capability enabled only if both are
supported. So let's have two of them and only use them together.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 4 ++++
src/qemu/qemu_capabilities.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 355b76e1f82c..308249d40a50 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -319,6 +319,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"qxl.vram64_size_mb",
"qxl-vga.vram64_size_mb", /* 215 */
+ "qxl.max_outputs",
+ "qxl-vga.max_outputs",
);
@@ -1657,11 +1659,13 @@ static struct virQEMUCapsStringFlags
virQEMUCapsObjectPropsVmwareSvga[] = {
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxl[] = {
{ "vgamem_mb", QEMU_CAPS_QXL_VGAMEM },
{ "vram64_size_mb", QEMU_CAPS_QXL_VRAM64 },
+ { "max_outputs", QEMU_CAPS_QXL_MAX_OUTPUTS },
};
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
{ "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
{ "vram64_size_mb", QEMU_CAPS_QXL_VGA_VRAM64 },
+ { "max_outputs", QEMU_CAPS_QXL_VGA_MAX_OUTPUTS },
};
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioGpu[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 9aa79aa39593..ba31a569e98b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -349,6 +349,8 @@ typedef enum {
/* 215 */
QEMU_CAPS_QXL_VGA_VRAM64, /* -device qxl-vga.vram64_size_mb */
+ QEMU_CAPS_QXL_MAX_OUTPUTS, /* -device qxl,max-outputs= */
+ QEMU_CAPS_QXL_VGA_MAX_OUTPUTS, /* -device qxl-vga,max-outputs= */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.7.2
8:21 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add support to QXL's max_outputs parameter
Historically, we added heads=1 to videos, but for example for qxl, we
did not reflect that on the command line. Implementing that now could
mean that if user were to migrate from older to newer libvirt, the
command-line for qemu would differ. Well, it does, but since that is
not a problem for QEMU (and the migration is successful) this does not
need any additional migration code or flag.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_command.c | 8 ++++
.../qemuxml2argv-video-qxl-heads.args | 28 +++++++++++++
.../qemuxml2argv-video-qxl-heads.xml | 47 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 8 ++++
.../qemuxml2xmlout-video-qxl-heads.xml | 47 ++++++++++++++++++++++
tests/qemuxml2xmltest.c | 2 +
6 files changed, 140 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-heads.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 000c29d3e976..ea520d75dbfc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3361,6 +3361,14 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
/* QEMU accepts mebibytes for vgamem_mb. */
virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem /
1024);
}
+
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_MAX_OUTPUTS)) {
+ if (video->heads)
+ virBufferAsprintf(&buf, ",max_outputs=%u",
video->heads);
+ } else {
+ video->heads = 0;
+ }
} else if (video->vram &&
((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
new file mode 100644
index 000000000000..a939177088f9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,max_outputs=1,\
+bus=pci.0,addr=0x2 \
+-device qxl,id=video1,ram_size=67108864,vram_size=33554432,max_outputs=3,\
+bus=pci.0,addr=0x4 \
+-device qxl,id=video2,ram_size=67108864,vram_size=67108864,max_outputs=7,\
+bus=pci.0,addr=0x5 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
new file mode 100644
index 000000000000..45d08745e652
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
@@ -0,0 +1,47 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <video>
+ <model type='qxl' ram='65536' vram='32768'
vgamem='8192' heads='3'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x04' function='0x0'/>
+ </video>
+ <video>
+ <model type='qxl' ram='65536' vram='65536'
vgamem='8192' heads='7'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x05' function='0x0'/>
+ </video>
+ <video>
+ <model type='qxl' ram='65536' vram='65536'
vgamem='8192' heads='1' primary='yes'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x02' function='0x0'/>
+ </video>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d29073dda5e6..6ea53cb41002 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1887,6 +1887,14 @@ mymain(void)
DO_TEST("ppc64-usb-controller-legacy",
QEMU_CAPS_PIIX3_USB_UHCI);
+ DO_TEST("video-qxl-heads",
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_VGA_QXL,
+ QEMU_CAPS_DEVICE_QXL_VGA,
+ QEMU_CAPS_DEVICE_QXL,
+ QEMU_CAPS_QXL_MAX_OUTPUTS,
+ QEMU_CAPS_QXL_VGA_MAX_OUTPUTS);
+
DO_TEST_PARSE_FLAGS_ERROR("missing-machine",
VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS,
NONE);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-heads.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-heads.xml
new file mode 100644
index 000000000000..e7b18ab2b2ab
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-heads.xml
@@ -0,0 +1,47 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <video>
+ <model type='qxl' ram='65536' vram='65536'
vgamem='8192' heads='1' primary='yes'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x02' function='0x0'/>
+ </video>
+ <video>
+ <model type='qxl' ram='65536' vram='32768'
vgamem='8192' heads='3'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x04' function='0x0'/>
+ </video>
+ <video>
+ <model type='qxl' ram='65536' vram='65536'
vgamem='8192' heads='7'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x05' function='0x0'/>
+ </video>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 07356777b1c2..cc1b07b82478 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -761,6 +761,8 @@ mymain(void)
DO_TEST("virtio-input");
DO_TEST("virtio-input-passthrough");
+ DO_TEST("video-qxl-heads");
+
qemuTestDriverFree(&driver);
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
--
2.7.2
8:46 a.m.
Hey,
On Thu, Mar 10, 2016 at 03:20:59PM +0100, Martin Kletzander wrote:
So this is an old, tiny series that was posted some time ago;
actually
a lot of time ago. I complained because of two things. First one
being that there are no tests, so this series has them in. The second
one was that this could break migration from old code since the
parameter was not used before. So I coded up a migration flag,
handled all the special cases and then started testing it. And while
testing it, I've found out that QEMU doesn't care at all about this
parameter being there and there is no point in keeping it
super-stable, especially when it can be changed on the fly. So I
removed all that code and ended up with tiny series very similar to
previous attempts by various people.
I think the main problem was that if you upgrade from a QEMU which did
not care to a QEMU with the option and a libvirt which supports it,
suddenly your VMs are going to go from supporting multiple heads (4) to
only supporting one as older libvirt always add a 'heads=1' to the XML.
Christophe
8:52 a.m.
On Thu, Mar 10, 2016 at 03:46:42PM +0100, Christophe Fergeau wrote:
Hey,
On Thu, Mar 10, 2016 at 03:20:59PM +0100, Martin Kletzander wrote:
> So this is an old, tiny series that was posted some time ago; actually
> a lot of time ago. I complained because of two things. First one
> being that there are no tests, so this series has them in. The second
> one was that this could break migration from old code since the
> parameter was not used before. So I coded up a migration flag,
> handled all the special cases and then started testing it. And while
> testing it, I've found out that QEMU doesn't care at all about this
> parameter being there and there is no point in keeping it
> super-stable, especially when it can be changed on the fly. So I
> removed all that code and ended up with tiny series very similar to
> previous attempts by various people.
I think the main problem was that if you upgrade from a QEMU which did
not care to a QEMU with the option and a libvirt which supports it,
suddenly your VMs are going to go from supporting multiple heads (4) to
only supporting one as older libvirt always add a 'heads=1' to the XML.
I'm not sure how to handle this, but I was under the impression that
this can be changed on the fly. If libvirt doesn't support that, then
we should enable it. But anyway not adding max_outputs= to commandline
when migrating from older libvirt could break something the other way
around as well.
/me quickly searches reflog for that migration code...
Christophe
4:07 a.m.
On Thu, Mar 10, 2016 at 03:52:54PM +0100, Martin Kletzander wrote:
I'm not sure how to handle this, but I was under the impression
that
this can be changed on the fly. If libvirt doesn't support that, then
we should enable it.
From the spice side, this seems to be changeable on the fly.
Christophe
5:12 a.m.
On Tue, Mar 15, 2016 at 10:07:42AM +0100, Christophe Fergeau wrote:
On Thu, Mar 10, 2016 at 03:52:54PM +0100, Martin Kletzander wrote:
> I'm not sure how to handle this, but I was under the impression that
> this can be changed on the fly. If libvirt doesn't support that, then
> we should enable it.
From the spice side, this seems to be changeable on the fly.
So if the maximum is changeable on the fly then I believe we should not
add that to a command-line on the destination when migrating because it
might be already different to the number that was set when the VM
started. So let me know which one works better so I know what to
include in the next version where the deleted, restored, and then
cleaned up migration part will be.
Thanks,
Martin
3166
days inactive
3171
days old
6 comments
2 participants
participants (2)
-
Christophe Fergeau -
Martin Kletzander