Hm. Since all OVMF virtual machines have a writeable pflash drive for the varstore, this issue apparently prevents snapshotting in such VMs. That's not good -- while for a "normal" VM we can tell the sysadmin to "just pick qcow2" for snapshotting, the format of the pflash drives is fixated in the libvirt code.

I guess this bug is not hard to solve in the technical sense, but it might become a compatibility / management mess.

- OVMF component: add a build-time dependency on qemu-img, and in the
  installation phase, run "qemu-img convert -O qcow2" on the varstore template.

- libvirt component: in the qemuBuildDomainLoaderCommandLine() function, pass
  format=qcow2 for *all* pflash drives that end up writable.

  This includes the pflash drive that corresponds to <nvram>, and also (in
  the non-split case, where the single drive contains both binary and
  varstore) the <loader readonly='no' type='pflash'>...</loader> element as
  well. (NB in RHEL we don't intend to support the non-split (ie. unified)
  case.)

- qemu-kvm(-rhev) component: nothing to do.

The management / compat issue here is how libvirt should handle a preexistent raw varstore after changing qemuBuildDomainLoaderCommandLine() to say "format=qcow2".

... Actually, I think this choice should be punted to the end-user instead, libvirt should only expose the capability. That is, both <loader> and <nvram> elements should grow a new attribute, "format", defaulting to "raw" (and only making sense for <loader> in case of type="pflash").

Then qemuBuildDomainLoaderCommandLine() would simply use those formats when building the command line. If a sysadmin wanted snapshots in an OVMF virtual machine, he would be told to convert his varstore to qcow2 manually.

... I realize the above is not fully consistent, especially because we might want to have different format defaults in upstream vs. RHEL. In any case, I reiterate that:
- this issue does not require a qemu-kvm(-rhev) code change; the component
  should be changed to libvirt
- the OVMF spec file can be updated to install a qcow2 formatted varstore
  template in addition to, or in place of, the current raw formatted varstore
  template (*)
- the code to change lives in qemuBuildDomainLoaderCommandLine(), but it might
  take more than that -- domain XML changes. I don't know how to design that
  in a compatible way.

For now I'm moving this over to libvirt.

Regarding the RHEL7 clone (because this is for RHEV), ie. bug 1180956, that one should be closed, because we don't ship two separate libvirt builds. The one libvirt package should be able to handle this for both qemu builds.

(*) Once the libvirt design is figured out, please open one OVMF and onve AAVMF bug, in order to address any installation-time format conversions, if necessary.

(In reply to Laszlo Ersek from comment #2)
> Hm. Since all OVMF virtual machines have a writeable pflash drive for the
> varstore, this issue apparently prevents snapshotting in such VMs. That's
> not good -- while for a "normal" VM we can tell the sysadmin to "just pick
> qcow2" for snapshotting, the format of the pflash drives is fixated in the
> libvirt code.
> 
> I guess this bug is not hard to solve in the technical sense, but it might
> become a compatibility / management mess.
> 
> - OVMF component: add a build-time dependency on qemu-img, and in the
>   installation phase, run "qemu-img convert -O qcow2" on the varstore
> template.

How would libvirt know that the varstore template is already qcow2 or the previous format? Or do we leave it for user to check and decide?

> 
> - libvirt component: in the qemuBuildDomainLoaderCommandLine() function, pass
>   format=qcow2 for *all* pflash drives that end up writable.
> 
>   This includes the pflash drive that corresponds to <nvram>, and also (in
>   the non-split case, where the single drive contains both binary and
>   varstore) the <loader readonly='no' type='pflash'>...</loader> element as
>   well. (NB in RHEL we don't intend to support the non-split (ie. unified)
>   case.)
> 
> - qemu-kvm(-rhev) component: nothing to do.
> 
> The management / compat issue here is how libvirt should handle a
> preexistent raw varstore after changing qemuBuildDomainLoaderCommandLine()
> to say "format=qcow2".
> 
> ... Actually, I think this choice should be punted to the end-user instead,
> libvirt should only expose the capability. That is, both <loader> and
> <nvram> elements should grow a new attribute, "format", defaulting to "raw"
> (and only making sense for <loader> in case of type="pflash").

Oh, I see. That answers my question above. Well, libvirt doesn't need any special capability to expose. Since ages libvirt XML works like this: users provide XML, libvirt parses it and then they should dump the XML back to check for missing attributes/elements (and therefore they know what's unsupported with the version they're running). So if libvirt is learnt new attribute, they'll just check it.

> 
> Then qemuBuildDomainLoaderCommandLine() would simply use those formats when
> building the command line. If a sysadmin wanted snapshots in an OVMF virtual
> machine, he would be told to convert his varstore to qcow2 manually.

Makes sense. Doing that in an rpm install macro would be shameful indeed.

> 
> ... I realize the above is not fully consistent, especially because we might
> want to have different format defaults in upstream vs. RHEL. In any case, I
> reiterate that:
> - this issue does not require a qemu-kvm(-rhev) code change; the component
>   should be changed to libvirt
> - the OVMF spec file can be updated to install a qcow2 formatted varstore
>   template in addition to, or in place of, the current raw formatted varstore
>   template (*)
> - the code to change lives in qemuBuildDomainLoaderCommandLine(), but it
> might
>   take more than that -- domain XML changes. I don't know how to design that
>   in a compatible way.
> 
> For now I'm moving this over to libvirt.
> 
> Regarding the RHEL7 clone (because this is for RHEV), ie. bug 1180956, that
> one should be closed, because we don't ship two separate libvirt builds. The
> one libvirt package should be able to handle this for both qemu builds.
> 
> (*) Once the libvirt design is figured out, please open one OVMF and onve
> AAVMF bug, in order to address any installation-time format conversions, if
> necessary.

Will do after my patches for libvirt are accepted.

Patch proposed upstream:

https://www.redhat.com/archives/libvir-list/2015-January/msg00394.html

I think BZ clones will be necessary for virt-install & virt-manager too... Esp. in RHEL, the default nvram format should be qcow2.

Of course a sysadmin can convert a raw varstore file after creation to qcow2, manually, rename it to the original name, and flip the new attribute from "raw" to "qcow2", if he/she wants snapshot / savevm to work. But, that's too much work for the user :); it should be the default selection in virt-install / virt-manager, if libvirtd reports this capability. Thanks!

Another try:

https://www.redhat.com/archives/libvir-list/2015-January/msg00444.html

... Okay, while reviewing Michal's v2, I actually made the effort to educate
myself about the "savevm" command. See "qemu-doc.texi", @node vm_snapshots:

> @node vm_snapshots
> @subsection VM snapshots
>
> VM snapshots are snapshots of the complete virtual machine including
> CPU state, RAM, device state and the content of all the writable
> disks. In order to use VM snapshots, you must have at least one non
> removable and writable block device using the @code{qcow2} disk image
> format. Normally this device is the first virtual hard drive.
>
> Use the monitor command @code{savevm} to create a new VM snapshot or
> replace an existing one. A human readable name can be assigned to each
> snapshot in addition to its numerical ID.
>
> Use @code{loadvm} to restore a VM snapshot and @code{delvm} to remove
> a VM snapshot. @code{info snapshots} lists the available snapshots
> with their associated information:
>
> @example
> (qemu) info snapshots
> Snapshot devices: hda
> Snapshot list (from hda):
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1         start                   41M 2006-08-06 12:38:02   00:00:14.954
> 2                                 40M 2006-08-06 12:43:29   00:00:18.633
> 3         msys                    40M 2006-08-06 12:44:04   00:00:23.514
> @end example
>
> A VM snapshot is made of a VM state info (its size is shown in
> @code{info snapshots}) and a snapshot of every writable disk image.
> The VM state info is stored in the first @code{qcow2} non removable
> and writable block device. The disk image snapshots are stored in
> every disk image. The size of a snapshot in a disk image is difficult
> to evaluate and is not shown by @code{info snapshots} because the
> associated disk sectors are shared among all the snapshots to save
> disk space (otherwise each snapshot would need a full copy of all the
> disk images).
>
> When using the (unrelated) @code{-snapshot} option
> (@ref{disk_images_snapshot_mode}), you can always make VM snapshots,
> but they are deleted as soon as you exit QEMU.
>
> VM snapshots currently have the following known limitations:
> @itemize
> @item
> They cannot cope with removable devices if they are removed or
> inserted after a snapshot is done.
> @item
> A few device drivers still have incomplete snapshot support so their
> state is not saved or restored properly (in particular USB).
> @end itemize

What an incredible, annoying mess is this?!

Yes, the state of pflash chips should be captured when creating a VM
snapshot. That's why I thought that the pflash drive should have qcow2
format.

But a pflash drive should *never* be selected for storing the VM
state info! That makes no sense at all.

The command line in comment #0 has six drives. The first four are excluded
from snapshots; they will not be snap-shot. (Or does "snapshot=off" mean
something else? The qemu manual page is completely useless here.) The 5th
drive is the read-only pflash drive with the OVMF binary. The 6th drive is
the read-write pflash drive with the varstore.

At the moment, do_savevm() bails out because the 6th drive, the *only*
writable and non-removable drive, doesn't support snapshots (because its
"raw").

However, if Michal's v2 libvirt patchset was applied, and the varstore drive
was qcow2, then qemu would dump the *entire VM state*, including memory and
device state, into the varstore drive (the 6th drive) under the command line
visible in comment #0. That's *completely* bogus; much worse than rejecting
the snapshot request.

This means:

(1) The pflash drive itself should be excluded from snapshotting (it
    shouldn't contain any qcow2 snapshot of its earlier state(s), it should
    remain raw). The VM state that is saved in whatever drive includes the
    RAMBlocks, and each pflash device has an associated RAMBlock. So the
    contents of the pflash chip will be saved anyway, as part of the VM
    state.

    When the VM state is loaded, the pflash_post_load() function will
    rewrite the full drive with the loaded RAMBlock contents.

(2) A pflash drive should *never* be considered as a candidate for *storing*
    VM state (memory, devices).

These items imply that Michal's current (v2) patchset is unnecessary :(
I'm so sorry!

A qemu patch will be needed after all (first in upstream). Namely, the
do_savevm() function must omit pflash drives from the verification step
(snapshot=off is insufficient for that, I tested it), and from the actual
snap-shooting. Plus, find_vmstate_bs() must skip pflash drives, when it
looks for a target drive to store the VM state (memory, devices). Something
like:

> diff --git a/savevm.c b/savevm.c
> index 08ec678..434d22b 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -42,6 +42,7 @@
>  #include "qemu/iov.h"
>  #include "block/snapshot.h"
>  #include "block/qapi.h"
> +#include "block/block_int.h"
>
>
>  #ifndef ETH_P_RARP
> @@ -1003,6 +1004,15 @@ static BlockDriverState *find_vmstate_bs(void)
>  {
>      BlockDriverState *bs = NULL;
>      while ((bs = bdrv_next(bs))) {
> +        /*
> +         * IF_PFLASH drives never accept snapshots; they back emulated flash
> +         * chips, not block devices.
> +         */
> +        if (bs->blk && blk_legacy_dinfo(bs->blk) &&
> +            blk_legacy_dinfo(bs->blk)->type == IF_PFLASH) {
> +            continue;
> +        }
> +
>          if (bdrv_can_snapshot(bs)) {
>              return bs;
>          }
> @@ -1051,11 +1061,19 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      struct tm tm;
>      const char *name = qdict_get_try_str(qdict, "name");
>
> -    /* Verify if there is a device that doesn't support snapshots and is writable */
> +    /*
> +     * Verify if there is a bdrv that doesn't support snapshots and is
> +     * writable.
> +     *
> +     * IF_PFLASH drives are only saved as part of VM state (RAMBlock); they're
> +     * not snap-shot. See pflash_post_load().
> +     */
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>
> -        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> +            (bs->blk && blk_legacy_dinfo(bs->blk) &&
> +             blk_legacy_dinfo(bs->blk)->type == IF_PFLASH)) {
>              continue;
>          }
>
> @@ -1120,6 +1138,11 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>
>      bs1 = NULL;
>      while ((bs1 = bdrv_next(bs1))) {
> +        if (bs1->blk && blk_legacy_dinfo(bs1->blk) &&
> +            blk_legacy_dinfo(bs1->blk)->type == IF_PFLASH) {
> +            continue;
> +        }
> +
>          if (bdrv_can_snapshot(bs1)) {
>              /* Write VM state size only to the image that contains the state */
>              sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);

(Except, this doesn't compile, because BlockDriverState is opaque.)

Clearly, this must be reviewed by a block subsys expert.

Definitely not 7.1 material (ie. way too big for an exception).

The VDAG describes snapshots:

Managing guest virtual machines with virsh
  Managing snapshots

Please document in this chapter that for OVMF, snapshot modes that end up in the "savevm" qemu monitor command are unsupported. As far as I understand the libvirt source code, an internal snapshot would be taken (with savevm) under the following circumstances (ie. this is what's not supported):

* the domain is currently running, AND
* '--disk-only' is absent, AND
* '--memspec ...,snapshot=external' is absent

In addition, it should be documented that the UEFI variable store's contents will only be part of the snapshot if:
* the domain is currently running, AND
* '--disk-only' is absent, AND
* '--memspec ...,snapshot=external' is present

... Unfortunately, reverting to an external snapshot is apparently not supported yet, which means that snapshots for OVMF guests are generally unavailable ATM.

*** Bug 1180956 has been marked as a duplicate of this bug. ***

See also http://thread.gmane.org/gmane.comp.emulators.qemu/386807

Actually, giving the limitation of --live in RHEL (where live snapshots are disabled), does it matter? Will users be able to hit this problem?

This also affects AAVMF, the AArch64 build of the ovmf component. Should this BZ be cloned for AArch64 in some way?