From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MlOMq-0000ZT-1P for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:41:52 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MlOMl-0000W1-UJ for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:41:51 -0400 Received: from [199.232.76.173] (port=56853 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MlOMl-0000Vo-Ke for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:41:47 -0400 Received: from mail-qy0-f194.google.com ([209.85.221.194]:65241) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MlOMk-0003m6-Ig for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:41:46 -0400 Received: by qyk32 with SMTP id 32so3545226qyk.19 for ; Wed, 09 Sep 2009 07:41:45 -0700 (PDT) Message-ID: <4AA7BEA7.6080906@codemonkey.ws> Date: Wed, 09 Sep 2009 09:41:43 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] The State of the SaveVM format References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" 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