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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
Christophe
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list