[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 redhat com>
> ---
>  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 redhat com>

Christophe

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]