[libvirt] [PATCH 0/2] qemu: Support for QXL heads
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
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@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
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@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
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
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
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
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
participants (2)
-
Christophe Fergeau -
Martin Kletzander