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