[Top][All Lists]

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

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

From: Anthony Liguori
Subject: Re: [Qemu-devel] The State of the SaveVM format
Date: Wed, 09 Sep 2009 09:41:43 -0500
User-agent: Thunderbird (X11/20090825)

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);
   if (version_id == 3)
   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 =
     .... 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.


Anthony Liguori

reply via email to

[Prev in Thread] Current Thread [Next in Thread]