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

[Juan Quintela]

Anthony Liguori <address@hidden> 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.