[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: The State of the SaveVM format
From: |
Juan Quintela |
Subject: |
[Qemu-devel] Re: The State of the SaveVM format |
Date: |
Wed, 09 Sep 2009 16:49:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
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.