[Qemu-devel] Re: [PATCH RFC 0/5] New VMState table based load/save infrastructure

[Juan Quintela <quintela@redhat.com>]

Reply-to: quintela@redhat.com
Gerd Hoffmann <kraxel@redhat.com> wrote:
>> What do I want to discuss:
>> a- do we want to be able to load state form old versions?
>>     I am not sure that we are going to get this right for complex devices,
>>     and I don't completely see how we can have any kind of testing here.
>>     There was already a discussion about this on the list.
>
> Yes, we want.  And I think the right approach to do that is to simply
> use the old, existing code and switch based on version_id.  To stick
> with the apic example:  When loadvm finds a v2 (or older) apic
> section, go call apic_load.  When loadvm finds a v3 (or newer) apic
> section, call the new generic load code with the apic vmstatefield
> list.

Liked it, will implement it in next round.

>> b- Is this the right approach?  What more do we want/need?
>>     For instance, implementing struct save support, and calling
>>     other "sub-descriptions" is not difficult, we just have to decide
>>     if we want it.
>
> Yes, we want.  PCI devices call a generic function to save pci
> state. We want a common pci vmstatefield list too and have some way to
> refer to them from the device tables.

That is the reason that I wanted to maintain the ->info field.  See the
other email.

>> c- In the current approach, we have loops to send arrays,  I think that one
>>     got already done better on new approarch.  But we don't have support
>>     for ifs (see hw/ide.c
>>         if (s->identify_set) {
>>          qemu_get_buffer(f, (uint8_t *)s->identify_data, 512);
>>      }
>>     Do we want support for things like that?
>
> No.  We want fixed-sized sections, bonus points for a 'size' field in
> the header.  Just save everything unconditionally.  The current
> save/load functions do way too much stuff conditionally.  Saving a few
> bytes simply isn't worth the complexity of getting that right.

Problem with this is when things can be NULL :p
>From msix.c

    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
        return;
    }

void msix_save(PCIDevice *dev, QEMUFile *f)
{
    unsigned n = dev->msix_entries_nr;

    qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
}

msix_table_page is not NULL only if QEMU_PCI_CAP_MSIX is present.
I think that optional fields are needed, or a better way of dealing with
things like this.

>> d- how aggresive should the new design be?  i.e. be able to be compatible with
>>     old design is good, or can we start with a clean sheet and just remove the
>>     gotchas of the previous design?
>
> Handle compatibility by keeping the old load functions and start over
> with a clean sheet.

Liked this idea, makes things way, way easier.  I was looking at
virtio_net_load() and had a head-ache.  With your approach, things
become really easy.

Thaks for the review, Juan