[Qemu-devel] The State of the SaveVM format

[Qemu-devel] The State of the SaveVM format

[Juan Quintela]

A Sad History of a Doomed Format
--------------------------------

When the world was old, there was a V1 of savevm format (nothing more
to tell about it).

Then appeared version 2. It was a very simple format.  Only fields were:

- instance_id
- version_id
- record_len

You can see it at savevm.c::qemu_loadvm_state_v2()

ToDo: Create an image with v2, and see if we can still read it,
      otherwise, remove support to load v2 format.

And then v3 appeared

    commit 9366f4186025e1d8fc3bebd41fb714521c170b6f
    Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
    Date:   Mon Oct 6 14:53:52 2008 +0000
    Introduce v3 of savevm protocol

Features:
     * Support for progressive save of sections (for live checkpoint/migration)
     * An asynchronous API for doing save
     * Support for interleaving multiple progressive save sections
       (for future support of memory hot-add/storage migration)
     * Fully streaming format
     * Strong section version checking

At this point, all the save/load of images were done in plain C with
functions that did anything that they wanted.  Life was nice and good
while things worked.  When they didn't worked, you only knew that they
didn't worked.  No info at all why.  Qemu SaveVM format was an opaque
thing  that only a corrected configured qemu is able to read.

Fast Forward to the present, and it appears VMState.  What does it?
It allows you to specify the state as a table, and then the save
function walks the table and save all the fields.  The load function
walks the table and loads all the fields.  Save and Load functions are
obviously always on sync, because they are done walking the same table.
And life was good .... Ooops, no, it was not good.

The problems is what to do from here:
- We can have a very simple VMState format that only allows storing
  simple types (int32_t, uint64_t, timers, buffers of uint8_t, ...)
  Arrays of valid types
  Structs of valid types
  And that is it.  Advantage of this approach, it is very simple to
  create/test/whatever.  Disadvantage: it can't express all the things
  that were done in plain C.  Everybody agrees that we don't want to
  support everything that was done in plain C in the old way.  What we
  are discussing is "how many" things do we want to support.  Notice
  that  we can support _everything_ that we were doing with plain C.
  Anytime that you want to do something strange, you just need to write
  your own marshaling functions and you are done.  You do there
  anything that you want.

  We are here at how we want to develop the format.  People that has
  expressed opinions so far are:
  - Gerd: You do a very simple format, and if the old state can't be
          expressed in simple VMState, you just use the old load
          function.  This maintains VMState clean, and you can load
          everything that was done before. Eventually, we remove the
          old load state functions when we don't support so old format.
  - Anthony: If we leave the old load state functions, they will be
          around forever.  He wants to complicate^Wimprove VMState
          to be able to express everything that was done in plain C.
          Reason: It is better to only have _one_ set of functions.
  - Paul?: I think he told that testing that we can load old state
          is impossible, and it is better to just remove the ability
          of load from old versions (I think this was Paul position, but
          discussion was a month ago, and my memory is not perfect)

I guess that if I am misinterpreting anyone;, they will let you know,
don't worry :) As you can see, what we are searching here is the less
bad solution, All have advantages and disadvantages, and none is
"perfect" or obviously better than the others.

ToDo: Port all devices (for instance of a typical pc) to current simple
      VMState and see how many things we are missing (Beware: Dragons in
      virtio)

Another day, another problem, this time called: Optional features.

How do we deal with optional features?
- We add feature bits (something like PCI does with optional features,
  the exact implementation is not important).  When we add an optional
  feature  to a driver, we just implement the save function as:
   - if we are using the feature, we add the feature bit indicating that
     we are using the feature, and we save the state for that feature.
   - at load time: If we find a feature that we don't understand, we
     just abort the load.
   - at load time: if you miss a feature that you need -> you also abort
  This has a nice advantage, if you load the state from old qemu, you
  don't use the new feature, and you save the state -> you can still
  load the state in old qemu (this is a nice theory, we don't know how
  it would work on practice).  Another advantage is that you can code
  and test each option separately. Michael S. Tsirkin likes this mode.

- The other position: Optional features? Such a thing don't exist :)
  Why?  Because if there are not optional features, you always know
  with only version + name of device if you support it or not (with
  optional features, you have another failure mode: you can find
  a feature that you don't understand in the middle of loading the state
  that can't happen if there is not optional features.

  But, we really, really want optional features (they throw msix support
  again).  No problem, you just create _another device:

   VMStateDescription vmstate_virtio-net = ...

   VMStateDescription vmstate_virtio-net_msix =
     VMSTATE_STRUCT(vmstate_net);
     .... msix bits

  You explicitly tells what optional features you want to use.  Notice
  that you can convince qdev to make the right thing:
    --device net,model=virtio,msix=on  (loads virtio-net-msix)
    --device net,model=virtio,msix=off  (loads plain virtio-net)

  Advantages, you only support the combinations that made sense, you
  explicitly state what they are, and VMState continues to be simple.
  Why don't use optional features?  Because then test matrix explodes
  exponentially, for each optional feature, you multiply by two the
  number of tests that you have to do.  Disadvantage is that obviously
  you end having more devices (although they can be implemented in the
  same file and share almost all the code, see how vga-pci and vga-isa
  share almost all the code).

  Not having optional features, have another interesting property.
  Versions of a device are linear in the sense that each new version is a
  superset of the previous one (i.e. the same fields than the previous one
  plus some more).  This makes support for loading of old versions way
  easier.  Here put Juan (i.e. me) and I think that in the past Gerd
  liked something like this.

To help make a decision here, it is a good idea to look at all the
devices and see if when they add more fields for a new version, they do
typically as:
- they add optional features
- they add them because now the simulation is better/whatever (they are
  _not_ optional)

Notice that again, both approaches have advantages and disadvantages, it
just depend of what your priorities are :)

More problems: Going from newer versions to old versions
- I think that everybody thinks that this is a nice to have, but that it
  will took a lot to make it work, and there are more urgent things to
  do.

Notice that there are plans for VMState to do more interesting things
like:
- Be able to show the values in a saved image
- See if a VM is able to load a vmstate (i.e. it has the needed devices
  at the needed versions)
- .....

That ones are independent of what we decided for the previous problems.

Comments?  Things that I missed for the discussion?

Later, Juan.

[Qemu-devel] Re: The State of the SaveVM format

[Michael S. Tsirkin]

On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
> More problems: Going from newer versions to old versions
> - I think that everybody thinks that this is a nice to have, but that it
>   will took a lot to make it work, and there are more urgent things to
>   do.

It's actually easy if you make whatever is new in the new version an
optional feature.  Then to save in an old format, qemu is started with
the feature disabled.

-- 
MST

[Qemu-devel] Re: The State of the SaveVM format

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
>> More problems: Going from newer versions to old versions
>> - I think that everybody thinks that this is a nice to have, but that it
>>   will took a lot to make it work, and there are more urgent things to
>>   do.
>
> It's actually easy if you make whatever is new in the new version an
> optional feature.  Then to save in an old format, qemu is started with
> the feature disabled.

That only works if invariants are maintained.  That is not true in
general.  And furthermore, you are deciding that if you find a bug, and
fix needs a new field -> just live with the bug.  If you can recreate
the state without the new field, then _why_ did you added it in the 1st place?

Later, Juan.

[Qemu-devel] Re: The State of the SaveVM format

[Michael S. Tsirkin]

On Wed, Sep 09, 2009 at 11:22:50AM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
> >> More problems: Going from newer versions to old versions
> >> - I think that everybody thinks that this is a nice to have, but that it
> >>   will took a lot to make it work, and there are more urgent things to
> >>   do.
> >
> > It's actually easy if you make whatever is new in the new version an
> > optional feature.  Then to save in an old format, qemu is started with
> > the feature disabled.
> 
> That only works if invariants are maintained.  That is not true in
> general.  And furthermore, you are deciding that if you find a bug, and
> fix needs a new field -> just live with the bug.

Clarification: add an *option* to live with the bug, off by default.

> If you can recreate the state without the new field, then _why_ did
> you added it in the 1st place?
> 
> Later, Juan.

[Qemu-devel] Re: The State of the SaveVM format

[Michael S. Tsirkin]

On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
> Notice that there are plans for VMState to do more interesting things
> like:
> - Be able to show the values in a saved image
> - See if a VM is able to load a vmstate (i.e. it has the needed devices
>   at the needed versions)
> - .....
> 
> That ones are independent of what we decided for the previous problems.
> 
> Comments?  Things that I missed for the discussion?
> 
> Later, Juan.

Another idea was to switch to some standard format, like qdev machine
description format or xml, so we don't have to maintain our own.

All state besides the physical memory dump is normally very
small, so this won't have much overhead.

This is possible if we give up on backwards compatibility completely.

-- 
MST

[Qemu-devel] Re: The State of the SaveVM format

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
>> Notice that there are plans for VMState to do more interesting things
>> like:
>> - Be able to show the values in a saved image
>> - See if a VM is able to load a vmstate (i.e. it has the needed devices
>>   at the needed versions)
>> - .....
>> 
>> That ones are independent of what we decided for the previous problems.
>> 
>> Comments?  Things that I missed for the discussion?
>> 
>> Later, Juan.
>
> Another idea was to switch to some standard format, like qdev machine
> description format or xml, so we don't have to maintain our own.

Are you serious here?  Qemu don't trust the avalavility
autoconf/automake/gnulib/glib/... and other useful libraries, and now we
are gonig to require an xml library or qdev machine format (a library
that don't do releases and is not packaged) for somethnig core of qemu?

> All state besides the physical memory dump is normally very
> small, so this won't have much overhead.

> This is possible if we give up on backwards compatibility completely.

If we give up on backwards compatiblity, we fix at least half of the
problems, independently of swiching to a new format :)

Later, Juan.

[Qemu-devel] Re: The State of the SaveVM format

[Michael S. Tsirkin]

On Wed, Sep 09, 2009 at 11:26:43AM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
> >> Notice that there are plans for VMState to do more interesting things
> >> like:
> >> - Be able to show the values in a saved image
> >> - See if a VM is able to load a vmstate (i.e. it has the needed devices
> >>   at the needed versions)
> >> - .....
> >> 
> >> That ones are independent of what we decided for the previous problems.
> >> 
> >> Comments?  Things that I missed for the discussion?
> >> 
> >> Later, Juan.
> >
> > Another idea was to switch to some standard format, like qdev machine
> > description format or xml, so we don't have to maintain our own.
> 
> Are you serious here?  Qemu don't trust the avalavility
> autoconf/automake/gnulib/glib/... and other useful libraries, and now we
> are gonig to require an xml library or qdev machine format (a library
> that don't do releases and is not packaged) for somethnig core of qemu?

I mean the qdev that we have in qemu.  We already know how to
deserialize properties, and need to serialize them to dump machine
description out.


> > All state besides the physical memory dump is normally very
> > small, so this won't have much overhead.
> 
> > This is possible if we give up on backwards compatibility completely.
> 
> If we give up on backwards compatiblity, we fix at least half of the
> problems, independently of swiching to a new format :)
> 
> Later, Juan.

Re: [Qemu-devel] The State of the SaveVM format

[Gerd Hoffmann]

On 09/09/09 10:47, Juan Quintela wrote:
>    We are here at how we want to develop the format.  People that has
>    expressed opinions so far are:
>    - Gerd: You do a very simple format, and if the old state can't be
>            expressed in simple VMState, you just use the old load
>            function.  This maintains VMState clean, and you can load
>            everything that was done before. Eventually, we remove the
>            old load state functions when we don't support so old format.

I still think we should try to keep it as simple as possible.

>    - Anthony: If we leave the old load state functions, they will be
>            around forever.  He wants to complicate^Wimprove VMState
>            to be able to express everything that was done in plain C.
>            Reason: It is better to only have _one_ set of functions.

I think we can drop the load functions some day.  We'll need a release 
(say 0.13) which is completely converted to vmstate but still can read 
old state via old functions.  Then zap them and document that you can't 
go from 0.10 to 0.15 directly, but have to use 0.13 inbetween to port 
the saved vmstate over to the new format.

> How do we deal with optional features?
> - We add feature bits (something like PCI does with optional features,
>    the exact implementation is not important).  When we add an optional
>    feature  to a driver, we just implement the save function as:
>     - if we are using the feature, we add the feature bit indicating that
>       we are using the feature, and we save the state for that feature.
>     - at load time: If we find a feature that we don't understand, we
>       just abort the load.
>     - at load time: if you miss a feature that you need ->  you also abort
>    This has a nice advantage, if you load the state from old qemu, you
>    don't use the new feature, and you save the state ->  you can still
>    load the state in old qemu (this is a nice theory, we don't know how
>    it would work on practice).  Another advantage is that you can code
>    and test each option separately. Michael S. Tsirkin likes this mode.

Hmm.  We could make the format much more verbose.  Instead of storing 
just the values and a section version we store for each field:

   (1) the name
   (2) the type
   (3) for arrays the length
   (4) maybe field flags
   (5) the actual value

It loadvm then will read the field name, looks it up in vmstate, checks 
the type, if ok load the values to the specified state struct offset.

We probably want to add a default value for all fields to vmstate then.

Advantages:
   * We don't need section versions any more (except for the jump from
     the old to the vmstate based section format), adding fields just
     works.  Fields not present in the snapshot are filled with the
     default.
   * We maybe can use the vmstate default values to implement a generic
     device-reset function.
   * Dumping the snapshot in human-readable form works without access
     to all the vmstate structs describing the section layout.

Not sure how to handle stuff like msix.  I'd tend to save all state 
unconditionally.  We could add a is_default field flag which savevm will 
write to signal 'actual value == default value'.  In case loadvm finds a 
unknown field it then can ignore it if is_default is set, otherwise 
complain that it can't deal with the unknown state.  msix would have to 
make sure all state fields have the default values in case msix is not 
in use.

cheers,
  Gerd

Re: [Qemu-devel] The State of the SaveVM format

[Anthony Liguori]

Hi Juan,

Juan Quintela wrote:
> The problems is what to do from here:
> - We can have a very simple VMState format that only allows storing
>   simple types (int32_t, uint64_t, timers, buffers of uint8_t, ...)
>   Arrays of valid types
>   Structs of valid types
>   And that is it.  Advantage of this approach, it is very simple to
>   create/test/whatever.  Disadvantage: it can't express all the things
>   that were done in plain C.  Everybody agrees that we don't want to
>   support everything that was done in plain C in the old way.  What we
>   are discussing is "how many" things do we want to support.  Notice
>   that  we can support _everything_ that we were doing with plain C.
>   Anytime that you want to do something strange, you just need to write
>   your own marshaling functions and you are done.  You do there
>   anything that you want.
>
>   We are here at how we want to develop the format.  People that has
>   expressed opinions so far are:
>   - Gerd: You do a very simple format, and if the old state can't be
>           expressed in simple VMState, you just use the old load
>           function.  This maintains VMState clean, and you can load
>           everything that was done before. Eventually, we remove the
>           old load state functions when we don't support so old format.
>   - Anthony: If we leave the old load state functions, they will be
>           around forever.  He wants to complicate^Wimprove VMState
>           to be able to express everything that was done in plain C.
>           Reason: It is better to only have _one_ set of functions.
>   

This is not quite an accurate representation.

Today, you make no attempt to support older versions even if their 
format is quite sane.  Take ps2_kbd as an example.

The new format (v3) is:

        VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, 
PS2State),
        VMSTATE_INT32(scan_enabled, PS2KbdState),
        VMSTATE_INT32(translate, PS2KbdState),
        VMSTATE_INT32_V(scancode_set, PS2KbdState,3),

This is nice and should support v2 and v3.  However, you still point to 
the old savevm function which is:


static int ps2_kbd_load_old(QEMUFile* f, void* opaque, int version_id)
{
    PS2KbdState *s = (PS2KbdState*)opaque;

    if (version_id != 2 && version_id != 3)
        return -EINVAL;

    vmstate_load_state(f, &vmstate_ps2_common, &s->common, version_id);
    s->scan_enabled=qemu_get_be32(f);
    s->translate=qemu_get_be32(f);
    if (version_id == 3)
        s->scancode_set=qemu_get_be32(f);
    else
        s->scancode_set=2;
    return 0;
}

Which has to be an error.  But this is the real problem with leaving the 
old functions.  It encourages sloppiness.

Let's look at a more complex example (piix_pci):

        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
        VMSTATE_UINT8(smm_enabled, PCII440FXState),

This is v3.  You have an old load function that duplicates this 
functionality but has an additional field:

    if (version_id == 2)
        for (i = 0; i < 4; i++)
            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);

All I'm suggesting, is that instead of leaving that old function, you 
introduce:

static void marshal_pci_irq_levels(void *opaque, const char *name, 
size_t offset, int load, int version)
{
    if (version == 2) {
        for (i = 0; i < 4; i++)
            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
   }
}

        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
        VMSTATE_UINT8(smm_enabled, PCII440FXState),
        VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, PCII440FXState, 
marshal_pci_irq_levels, 2)

This avoids bit rot of the old load functions and makes the whole thing 
more robust.  My contention is that there simply isn't a lot of these 
special cases so it's not a lot more work overall.
> Another day, another problem, this time called: Optional features.
>
> How do we deal with optional features?
> - We add feature bits (something like PCI does with optional features,
>   the exact implementation is not important).  When we add an optional
>   feature  to a driver, we just implement the save function as:
>    - if we are using the feature, we add the feature bit indicating that
>      we are using the feature, and we save the state for that feature.
>    - at load time: If we find a feature that we don't understand, we
>      just abort the load.
>    - at load time: if you miss a feature that you need -> you also abort
>   This has a nice advantage, if you load the state from old qemu, you
>   don't use the new feature, and you save the state -> you can still
>   load the state in old qemu (this is a nice theory, we don't know how
>   it would work on practice).  Another advantage is that you can code
>   and test each option separately. Michael S. Tsirkin likes this mode.
>
> - The other position: Optional features? Such a thing don't exist :)
>   Why?  Because if there are not optional features, you always know
>   with only version + name of device if you support it or not (with
>   optional features, you have another failure mode: you can find
>   a feature that you don't understand in the middle of loading the state
>   that can't happen if there is not optional features.
>
>   But, we really, really want optional features (they throw msix support
>   again).  No problem, you just create _another device:
>
>    VMStateDescription vmstate_virtio-net = ...
>
>    VMStateDescription vmstate_virtio-net_msix =
>      VMSTATE_STRUCT(vmstate_net);
>      .... msix bits
>
>   You explicitly tells what optional features you want to use.  Notice
>   that you can convince qdev to make the right thing:
>     --device net,model=virtio,msix=on  (loads virtio-net-msix)
>     --device net,model=virtio,msix=off  (loads plain virtio-net)
>
>   Advantages, you only support the combinations that made sense, you
>   explicitly state what they are, and VMState continues to be simple.
>   Why don't use optional features?  Because then test matrix explodes
>   exponentially, for each optional feature, you multiply by two the
>   number of tests that you have to do.  Disadvantage is that obviously
>   you end having more devices (although they can be implemented in the
>   same file and share almost all the code, see how vga-pci and vga-isa
>   share almost all the code).
>
>   Not having optional features, have another interesting property.
>   Versions of a device are linear in the sense that each new version is a
>   superset of the previous one (i.e. the same fields than the previous one
>   plus some more).  This makes support for loading of old versions way
>   easier.  Here put Juan (i.e. me) and I think that in the past Gerd
>   liked something like this.
>   

I think the discussion around optional features is orthogonal to how to 
support older savevm formats so let's keep it separate.  I generally 
share your concern about test matrix explosion.

Regards,

Anthony Liguori

[Qemu-devel] Re: The State of the SaveVM format

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> Hi Juan,

> This is not quite an accurate representation.
Sorry.

> Today, you make no attempt to support older versions even if their
> format is quite sane.  Take ps2_kbd as an example.
>
> The new format (v3) is:
>
>        VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common,
> PS2State),
>        VMSTATE_INT32(scan_enabled, PS2KbdState),
>        VMSTATE_INT32(translate, PS2KbdState),
>        VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>
> This is nice and should support v2 and v3.  However, you still point
> to the old savevm function which is:

>    if (version_id == 3)
>        s->scancode_set=qemu_get_be32(f);
>    else
>        s->scancode_set=2;

Problem here is this value, there is no way to set default values
different of zero.  That is why there is still the old function.

> Which has to be an error.  But this is the real problem with leaving
> the old functions.  It encourages sloppiness.

I don't like to leave old versions either.  Just we have to get a
balance between leaving old versions or get new ones.

> Let's look at a more complex example (piix_pci):
>
>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>
> This is v3.  You have an old load function that duplicates this
> functionality but has an additional field:
>
>    if (version_id == 2)
>        for (i = 0; i < 4; i++)
>            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>
> All I'm suggesting, is that instead of leaving that old function, you
> introduce:
>
> static void marshal_pci_irq_levels(void *opaque, const char *name,
> size_t offset, int load, int version)
> {
>    if (version == 2) {
>        for (i = 0; i < 4; i++)
>            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>   }
> }
>
>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>        VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels,
> PCII440FXState, marshal_pci_irq_levels, 2)
>
> This avoids bit rot of the old load functions and makes the whole
> thing more robust.  My contention is that there simply isn't a lot of
> these special cases so it's not a lot more work overall.

Ok, will take a look at doing the _own_ marshaling, it has other
features, now a field becomes optional, that is the next item.

[ Optional features stuff removed ]

> I think the discussion around optional features is orthogonal to how
> to support older savevm formats so let's keep it separate.  I
> generally share your concern about test matrix explosion.

Ok. I am trying to port all the pc.c stuff to new VMState without adding
more marshaling (we already have the old load functions).  After I
finish with that, we can look at the remaining cases and look at a
course of action?

> Regards,
>
> Anthony Liguori

Thanks, Juan.

[Qemu-devel] Re: The State of the SaveVM format

[Anthony Liguori]

Juan Quintela wrote:
>>    if (version_id == 3)
>>        s->scancode_set=qemu_get_be32(f);
>>    else
>>        s->scancode_set=2;
>>     
>
> Problem here is this value, there is no way to set default values
> different of zero.  That is why there is still the old function.
>   

We do this with qdev properties, certainly we could do it with savevm too?

The advantage of doing this with savevm is that it potentially allows 
for the init and the reset functions to be simplified/eliminated.  Even 
fields that are present in all versions should have the ability to have 
default values.

In fact, semantically, a field should be zero unless a default value is 
specified.

>> I think the discussion around optional features is orthogonal to how
>> to support older savevm formats so let's keep it separate.  I
>> generally share your concern about test matrix explosion.
>>     
>
> Ok. I am trying to port all the pc.c stuff to new VMState without adding
> more marshaling (we already have the old load functions).  After I
> finish with that, we can look at the remaining cases and look at a
> course of action?
>   

Sounds good to me.

Regards,

Anthony Liguori

Re: [Qemu-devel] The State of the SaveVM format

[Gerd Hoffmann]

   Hi,

> Today, you make no attempt to support older versions even if their
> format is quite sane. Take ps2_kbd as an example.
>
> The new format (v3) is:
>
> VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, PS2State),
> VMSTATE_INT32(scan_enabled, PS2KbdState),
> VMSTATE_INT32(translate, PS2KbdState),
> VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>
> This is nice and should support v2 and v3.

It doesn't ...

> if (version_id == 3)
> s->scancode_set=qemu_get_be32(f);
> else
> s->scancode_set=2;

... setting scancode_set when loading v2 is missing.

I think vmstate fields need a default value to handle cases like this 
one without having to keep the old load function.

> Which has to be an error. But this is the real problem with leaving the
> old functions. It encourages sloppiness.

I think we can kill most of the old load functions.  I'd keep the old 
ones only in case emulating the old load function with vmstate would 
make it unreasonable complex.

> static void marshal_pci_irq_levels(void *opaque, const char *name,
> size_t offset, int load, int version)
> {
> if (version == 2) {
> for (i = 0; i < 4; i++)
> d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
> }
> }

> VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, PCII440FXState,
> marshal_pci_irq_levels, 2)

No.  I don't want any free-form C code in vmstate.  That will kill quite 
a few of the vmstate advantages.  Imagine a tool dumping snapshot data. 
  What this tool should do when it finds such a FUNC field?  It has 
absolutely no idea what is in there ...

cheers,
   Gerd

Re: [Qemu-devel] The State of the SaveVM format

[Anthony Liguori]

Gerd Hoffmann wrote:
>> Which has to be an error. But this is the real problem with leaving the
>> old functions. It encourages sloppiness.
>
> I think we can kill most of the old load functions.  I'd keep the old 
> ones only in case emulating the old load function with vmstate would 
> make it unreasonable complex.

That would be more palatable.

>> static void marshal_pci_irq_levels(void *opaque, const char *name,
>> size_t offset, int load, int version)
>> {
>> if (version == 2) {
>> for (i = 0; i < 4; i++)
>> d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>> }
>> }
>
>> VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, PCII440FXState,
>> marshal_pci_irq_levels, 2)
>
> No.  I don't want any free-form C code in vmstate.  That will kill 
> quite a few of the vmstate advantages.  Imagine a tool dumping 
> snapshot data.  What this tool should do when it finds such a FUNC 
> field?  It has absolutely no idea what is in there ...

Well in this example, we could eliminate the function by just marking 
each of these fields as just being version 2.  One approach would be to 
have a version mask associated with each field.  That would provide an 
explicit bitmap of which versions were supported.  > N becomes (-1 << 
n), < N becomes ((1 << n) - 1), etc.  It also lets us support these 
weird cases where a field was present in v2, but not in v3 by just using 
an explicit mask.

We can use a uint64_t to start with and that will last us 32 years with 
a 6 month release cycle or 16 years with a 3 month release cycle.

Regards,

Anthony Liguori

Re: [Qemu-devel] The State of the SaveVM format

[Anthony Liguori]

Gerd Hoffmann wrote:
>> Which has to be an error. But this is the real problem with leaving the
>> old functions. It encourages sloppiness.
>
> I think we can kill most of the old load functions.  I'd keep the old 
> ones only in case emulating the old load function with vmstate would 
> make it unreasonable complex.

That would be more palatable.

>> static void marshal_pci_irq_levels(void *opaque, const char *name,
>> size_t offset, int load, int version)
>> {
>> if (version == 2) {
>> for (i = 0; i < 4; i++)
>> d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>> }
>> }
>
>> VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, PCII440FXState,
>> marshal_pci_irq_levels, 2)
>
> No.  I don't want any free-form C code in vmstate.  That will kill 
> quite a few of the vmstate advantages.  Imagine a tool dumping 
> snapshot data.  What this tool should do when it finds such a FUNC 
> field?  It has absolutely no idea what is in there ...

Well in this example, we could eliminate the function by just marking 
each of these fields as just being version 2.  One approach would be to 
have a version mask associated with each field.  That would provide an 
explicit bitmap of which versions were supported.  > N becomes (-1 << 
n), < N becomes ((1 << n) - 1), etc.  It also lets us support these 
weird cases where a field was present in v2, but not in v3 by just using 
an explicit mask.

We can use a uint64_t to start with and that will last us 32 years with 
a 6 month release cycle or 16 years with a 3 month release cycle.

Regards,

Anthony Liguori

Re: [Qemu-devel] The State of the SaveVM format

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

[...]
>> static void marshal_pci_irq_levels(void *opaque, const char *name,
>> size_t offset, int load, int version)
>> {
>> if (version == 2) {
>> for (i = 0; i < 4; i++)
>> d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>> }
>> }
>
>> VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, PCII440FXState,
>> marshal_pci_irq_levels, 2)
>
> No.  I don't want any free-form C code in vmstate.  That will kill
> quite a few of the vmstate advantages.
[...]

Seconded.

Re: [Qemu-devel] The State of the SaveVM format

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Hi Juan,
>
> Juan Quintela wrote:
>> The problems is what to do from here:
>> - We can have a very simple VMState format that only allows storing
>>   simple types (int32_t, uint64_t, timers, buffers of uint8_t, ...)
>>   Arrays of valid types
>>   Structs of valid types
>>   And that is it.  Advantage of this approach, it is very simple to
>>   create/test/whatever.  Disadvantage: it can't express all the things
>>   that were done in plain C.  Everybody agrees that we don't want to
>>   support everything that was done in plain C in the old way.  What we
>>   are discussing is "how many" things do we want to support.  Notice
>>   that  we can support _everything_ that we were doing with plain C.
>>   Anytime that you want to do something strange, you just need to write
>>   your own marshaling functions and you are done.  You do there
>>   anything that you want.
>>
>>   We are here at how we want to develop the format.  People that has
>>   expressed opinions so far are:
>>   - Gerd: You do a very simple format, and if the old state can't be
>>           expressed in simple VMState, you just use the old load
>>           function.  This maintains VMState clean, and you can load
>>           everything that was done before. Eventually, we remove the
>>           old load state functions when we don't support so old format.
>>   - Anthony: If we leave the old load state functions, they will be
>>           around forever.  He wants to complicate^Wimprove VMState
>>           to be able to express everything that was done in plain C.
>>           Reason: It is better to only have _one_ set of functions.
>>   
>
> This is not quite an accurate representation.
>
> Today, you make no attempt to support older versions even if their
> format is quite sane.  Take ps2_kbd as an example.
>
> The new format (v3) is:
>
>        VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common,
> PS2State),
>        VMSTATE_INT32(scan_enabled, PS2KbdState),
>        VMSTATE_INT32(translate, PS2KbdState),
>        VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>
> This is nice and should support v2 and v3.  However, you still point
> to the old savevm function which is:
>
>
> static int ps2_kbd_load_old(QEMUFile* f, void* opaque, int version_id)
> {
>    PS2KbdState *s = (PS2KbdState*)opaque;
>
>    if (version_id != 2 && version_id != 3)
>        return -EINVAL;
>
>    vmstate_load_state(f, &vmstate_ps2_common, &s->common, version_id);
>    s->scan_enabled=qemu_get_be32(f);
>    s->translate=qemu_get_be32(f);
>    if (version_id == 3)
>        s->scancode_set=qemu_get_be32(f);
>    else
>        s->scancode_set=2;
>    return 0;
> }
>
> Which has to be an error.  But this is the real problem with leaving
> the old functions.  It encourages sloppiness.
>
> Let's look at a more complex example (piix_pci):
>
>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>
> This is v3.  You have an old load function that duplicates this
> functionality but has an additional field:
>
>    if (version_id == 2)
>        for (i = 0; i < 4; i++)
>            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>
> All I'm suggesting, is that instead of leaving that old function, you
> introduce:
>
> static void marshal_pci_irq_levels(void *opaque, const char *name,
> size_t offset, int load, int version)
> {
>    if (version == 2) {
>        for (i = 0; i < 4; i++)
>            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>   }
> }
>
>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>        VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels,
> PCII440FXState, marshal_pci_irq_levels, 2)
>
> This avoids bit rot of the old load functions and makes the whole
> thing more robust.  My contention is that there simply isn't a lot of
> these special cases so it's not a lot more work overall.
[...]

Bitrot is only an issue if we keep supporting the old formats for much
longer.

I'd like to challenge the idea that we need to drag along the old
formats for an indefinite period of time.  Is the restriction that to
upgrade from version N to version M, you have to go through version N+1
really so onerous?

A couple of releases down the road, the pre-vmstate formats will be
ancient history.  If we complicate vmstate now to shoehorn pre-vmstate
formats into vmstate, that ancient history will continue to haunt us.
Complicating a program is far easier than the other direction.

Moreover, I'm rather wary of efforts to replace working code (the
existing load functions) by new code that is supposed to do precisely
the same.  It means replacing the devil I know by some untested devil I
don't know.

Re: [Qemu-devel] The State of the SaveVM format

[Anthony Liguori]

Markus Armbruster wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>   
>>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>>        VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels,
>> PCII440FXState, marshal_pci_irq_levels, 2)
>>
>> This avoids bit rot of the old load functions and makes the whole
>> thing more robust.  My contention is that there simply isn't a lot of
>> these special cases so it's not a lot more work overall.
>>     
> [...]
>
> Bitrot is only an issue if we keep supporting the old formats for much
> longer.
>
> I'd like to challenge the idea that we need to drag along the old
> formats for an indefinite period of time.  Is the restriction that to
> upgrade from version N to version M, you have to go through version N+1
> really so onerous?
>   

Honestly, this is a really hard one for me.  On the one hand, migration 
compatibility is a very important feature for users.  I think we ought 
to try our best to support it.

But it's certainly not lost on me that this is going to be very 
difficult if not impossible to achieve in practice.  I'd like us to give 
it the best shot we can.  I don't like the idea of admitting defeat from 
the start and not even attempting to provide backwards compatibility.

> A couple of releases down the road, the pre-vmstate formats will be
> ancient history.

It's been shipped for RHEL5.4.  As is true for any enterprise distro, 
that means it will remain important to me for quite some time :-)

>   If we complicate vmstate now to shoehorn pre-vmstate
> formats into vmstate, that ancient history will continue to haunt us.
> Complicating a program is far easier than the other direction.
>   

Let's take it one step at a time.  There is an awful lot of areas where 
we can support older versions without adding complications.  Let's 
approach the complicated ones one at a time.

> Moreover, I'm rather wary of efforts to replace working code (the
> existing load functions) by new code that is supposed to do precisely
> the same.  It means replacing the devil I know by some untested devil I
> don't know.
>   

Understood, but what concerns me is that the old code path goes from 
being partially tested all of the time to being never tested in the 
common case.

The other big concern I have is that I would really like to eliminate 
QEMUFile as it's a rather hideous abstraction.  The advantage of 
converting old load functions to vmstate is that it makes it an awful 
lot easier to get rid of QEMUFile.

Regards,

Anthony Liguori

Re: [Qemu-devel] The State of the SaveVM format

[Michael S. Tsirkin]

On Wed, Sep 09, 2009 at 09:02:07PM -0500, Anthony Liguori wrote:
>>   If we complicate vmstate now to shoehorn pre-vmstate
>> formats into vmstate, that ancient history will continue to haunt us.
>> Complicating a program is far easier than the other direction.
>>   
>
> Let's take it one step at a time.  There is an awful lot of areas where  
> we can support older versions without adding complications.  Let's  
> approach the complicated ones one at a time.

I'm not sure I understand this talk about "pre-vmstate formats".
I thought vmstate patches were, at least for the most part, trying
to reimplement existing format with the table-driver design?

If that's not so and we are changing the format now, is it too late to
consider some standard serialization format rather than rolling our own?

-- 
MST

[Qemu-devel] Re: The State of the SaveVM format

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Sep 09, 2009 at 09:02:07PM -0500, Anthony Liguori wrote:
>>>   If we complicate vmstate now to shoehorn pre-vmstate
>>> formats into vmstate, that ancient history will continue to haunt us.
>>> Complicating a program is far easier than the other direction.
>>>   
>>
>> Let's take it one step at a time.  There is an awful lot of areas where  
>> we can support older versions without adding complications.  Let's  
>> approach the complicated ones one at a time.
>
> I'm not sure I understand this talk about "pre-vmstate formats".
> I thought vmstate patches were, at least for the most part, trying
> to reimplement existing format with the table-driver design?
>
> If that's not so and we are changing the format now, is it too late to
> consider some standard serialization format rather than rolling our own?

We are using previous format.  At some point we should move to other
format.  When/what is still not decided.  Each time at its time.  Once
we have everything using vmstate, we have a declarative description of
the state.  Going for tables with names + types to any format is just an
exercise of walking the tables and writing a pretty-printer and a parser.

Later, Juan.

[Qemu-devel] Re: The State of the SaveVM format

[Michael S. Tsirkin]

On Thu, Sep 10, 2009 at 02:55:07PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Sep 09, 2009 at 09:02:07PM -0500, Anthony Liguori wrote:
> >>>   If we complicate vmstate now to shoehorn pre-vmstate
> >>> formats into vmstate, that ancient history will continue to haunt us.
> >>> Complicating a program is far easier than the other direction.
> >>>   
> >>
> >> Let's take it one step at a time.  There is an awful lot of areas where  
> >> we can support older versions without adding complications.  Let's  
> >> approach the complicated ones one at a time.
> >
> > I'm not sure I understand this talk about "pre-vmstate formats".
> > I thought vmstate patches were, at least for the most part, trying
> > to reimplement existing format with the table-driver design?
> >
> > If that's not so and we are changing the format now, is it too late to
> > consider some standard serialization format rather than rolling our own?
> 
> We are using previous format.  At some point we should move to other
> format.  When/what is still not decided.  Each time at its time.

Ah, that's what I thought.

> Once
> we have everything using vmstate, we have a declarative description of
> the state.  Going for tables with names + types to any format is just an
> exercise of walking the tables and writing a pretty-printer and a parser.
> 
> Later, Juan.

Good point. But if we do intend to switch formats because of vmstate and
separately switch to a standard format, it might be easier on users if
we do a single switch, in the same release cycle.

-- 
MST

[Qemu-devel] Re: The State of the SaveVM format

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Sep 10, 2009 at 02:55:07PM +0200, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Sep 09, 2009 at 09:02:07PM -0500, Anthony Liguori wrote:
>> >>>   If we complicate vmstate now to shoehorn pre-vmstate
>> >>> formats into vmstate, that ancient history will continue to haunt us.
>> >>> Complicating a program is far easier than the other direction.
>> >>>   
>> >>
>> >> Let's take it one step at a time.  There is an awful lot of areas where  
>> >> we can support older versions without adding complications.  Let's  
>> >> approach the complicated ones one at a time.
>> >
>> > I'm not sure I understand this talk about "pre-vmstate formats".
>> > I thought vmstate patches were, at least for the most part, trying
>> > to reimplement existing format with the table-driver design?
>> >
>> > If that's not so and we are changing the format now, is it too late to
>> > consider some standard serialization format rather than rolling our own?
>> 
>> We are using previous format.  At some point we should move to other
>> format.  When/what is still not decided.  Each time at its time.
>
> Ah, that's what I thought.
>
>> Once
>> we have everything using vmstate, we have a declarative description of
>> the state.  Going for tables with names + types to any format is just an
>> exercise of walking the tables and writing a pretty-printer and a parser.
>> 
>> Later, Juan.
>
> Good point. But if we do intend to switch formats because of vmstate and
> separately switch to a standard format, it might be easier on users if
> we do a single switch, in the same release cycle.

Switch will be done for 0.12, 0.11 should have been released already.
1st switch everything to vmstate, and then compare/study/decide formats.

Later, Juan.