Re: [libvirt] [PATCH v3 2/2] qemu: Add support to QXL's max_outputs parameter

Hey, On Thu, Mar 24, 2016 at 05:01:52PM +0100, Martin Kletzander wrote: > 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. In order for that not to happen a > migration cookie flag is introduced. > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 8 +++ > src/qemu/qemu_migration.c | 64 ++++++++++++++++++++-- > .../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 + > 7 files changed, 198 insertions(+), 6 deletions(-) > 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 0331789b6b59..f6b102e96bb2 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3985,6 +3985,14 @@ qemuBuildDeviceVideoStr(const virDomainDef *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)) { This test could be more similar to the one for vgamem/vram64 above for consistency, and be if ((video->primary && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS)) || (!video->primary && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_MAX_OUTPUTS))) { } Since both caps will always be set/not set, this is not going to make a difference, and your version is easier to read imo.

> + if (video->heads) > + virBufferAsprintf(&buf, ",max_outputs=%u", video->heads); > + } else { > + video->heads = 0; > + } Is it typical to modify the domain def content from within qemu_command.c ?

> } else if (video->vram && > ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA && > virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) || > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 8bc76bf1671d..f83b6362d228 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -87,6 +87,7 @@ enum qemuMigrationCookieFlags { > QEMU_MIGRATION_COOKIE_FLAG_NBD, > QEMU_MIGRATION_COOKIE_FLAG_STATS, > QEMU_MIGRATION_COOKIE_FLAG_MEMORY_HOTPLUG, > + QEMU_MIGRATION_COOKIE_FLAG_VIDEO_HEADS, > > QEMU_MIGRATION_COOKIE_FLAG_LAST > }; > @@ -100,7 +101,8 @@ VIR_ENUM_IMPL(qemuMigrationCookieFlag, > "network", > "nbd", > "statistics", > - "memory-hotplug"); > + "memory-hotplug", > + "video-heads"); > > enum qemuMigrationCookieFeatures { > QEMU_MIGRATION_COOKIE_GRAPHICS = (1 << QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS), > @@ -110,6 +112,7 @@ enum qemuMigrationCookieFeatures { > QEMU_MIGRATION_COOKIE_NBD = (1 << QEMU_MIGRATION_COOKIE_FLAG_NBD), > QEMU_MIGRATION_COOKIE_STATS = (1 << QEMU_MIGRATION_COOKIE_FLAG_STATS), > QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG = (1 << QEMU_MIGRATION_COOKIE_FLAG_MEMORY_HOTPLUG), > + QEMU_MIGRATION_COOKIE_VIDEO_HEADS = (1 << QEMU_MIGRATION_COOKIE_FLAG_VIDEO_HEADS), > }; > > typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics; > @@ -1386,6 +1389,9 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, > if (flags & QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG) > mig->flagsMandatory |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG; > > + if (flags & QEMU_MIGRATION_COOKIE_VIDEO_HEADS) > + mig->flagsMandatory |= QEMU_MIGRATION_COOKIE_VIDEO_HEADS; > + > if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig))) > return -1; > > @@ -3091,6 +3097,7 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, > qemuDomainObjPrivatePtr priv = vm->privateData; > virCapsPtr caps = NULL; > unsigned int cookieFlags = QEMU_MIGRATION_COOKIE_LOCKSTATE; > + size_t i = 0, j = 0; > > VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, dname=%s," > " cookieout=%p, cookieoutlen=%p," > @@ -3131,8 +3138,6 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, > > if (nmigrate_disks) { > if (has_drive_mirror) { > - size_t i, j; > - Nit: I would keep these variables here, and declare a new size_t i; variable in the QXL_MAX_OUTPUTS block. Smaller scope to look at when one wants to look at where/how the 'i' variable is used.

> /* Check user requested only known disk targets. */ > for (i = 0; i < nmigrate_disks; i++) { > for (j = 0; j < vm->def->ndisks; j++) { > @@ -3177,6 +3182,27 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, > vm->newDef && virDomainDefHasMemoryHotplug(vm->newDef))) > cookieFlags |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG; > > + /* > + * If there is at least one QXL video with number of heads > + * defined, we need to indicate that we now know how to > + * properly specify that on the command-line. Destination > + * that cannot do this will properly block such migration and > + * new enough destination will know that the value can be kept > + * and not reset. > + */ > + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QXL_MAX_OUTPUTS) && > + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS)) { > + for (i = 0; i < vm->def->nvideos; i++) { > + virDomainVideoDefPtr vid = vm->def->videos[i]; > + > + if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL && > + vid->heads) { > + cookieFlags |= QEMU_MIGRATION_COOKIE_VIDEO_HEADS; > + break; > + } > + } > + } > + > if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) > goto cleanup; > > @@ -3404,6 +3430,32 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm, > } > > static int > +qemuMigratePrepareDomain(virConnectPtr dconn, > + virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + qemuMigrationCookiePtr mig) > +{ > + size_t i = 0; > + > + if (!(mig->flags & QEMU_MIGRATION_COOKIE_VIDEO_HEADS)) { > + /* > + * Source didn't know how to properly specify number of heads > + * for QXL video, so in order to migrate with ABI kept, leave > + * the value unspecified. > + */ > + for (i = 0; i < vm->def->nvideos; i++) { > + virDomainVideoDefPtr vid = vm->def->videos[i]; > + > + if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) > + vid->heads = 0; > + } > + } > + > + return qemuProcessPrepareDomain(dconn, driver, vm, > + VIR_QEMU_PROCESS_START_AUTODESTROY); > +} > + > +static int > qemuMigrationPrepareAny(virQEMUDriverPtr driver, > virConnectPtr dconn, > const char *cookiein, > @@ -3546,7 +3598,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, > QEMU_MIGRATION_COOKIE_LOCKSTATE | > QEMU_MIGRATION_COOKIE_NBD | > - QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG))) > + QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG | > + QEMU_MIGRATION_COOKIE_VIDEO_HEADS))) > goto cleanup; > > if (STREQ_NULLABLE(protocol, "rdma") && > @@ -3590,8 +3643,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > goto stopjob; > dataFD[0] = -1; /* the FD is now owned by incoming */ > > - if (qemuProcessPrepareDomain(dconn, driver, vm, > - VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) > + if (qemuMigratePrepareDomain(dconn, driver, vm, mig) < 0) > goto stopjob; > > if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) > 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> Maybe one of the tests could check what happens if heads is not specified ? (though iirc libvirt always adds it to the XML?)

Not acking as I'm not familiar with the migration code at all, but Reviewed-by: Christophe Fergeau <cfergeau(a)redhat.com&gt; Christophe