Re: [PATCH v3 1/6] migration: Allow immutable device state to be migrated early (i.e., before RAM)

[David Hildenbrand]

On 10.01.23 21:03, Peter Xu wrote:
> On Tue, Jan 10, 2023 at 12:52:32PM +0100, David Hildenbrand wrote:
> > The following seems to work,
>
> That looks much better at least from the diffstat pov (comparing to the
> existing patch 1+5 and the framework changes), thanks.
>
> > but makes analyze-migration.py angry:
> >
> > $ ./scripts/analyze-migration.py -f STATEFILE
> > Traceback (most recent call last):
> >    File "/home/dhildenb/git/qemu/./scripts/analyze-migration.py", line 605, in 
> > <module>
> >      dump.read(dump_memory = args.memory)
> >    File "/home/dhildenb/git/qemu/./scripts/analyze-migration.py", line 539, in 
> > read
> >      classdesc = self.section_classes[section_key]
> >                  ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
> > KeyError: ('0000:00:03.0/virtio-mem-early', 0)
> >
> >
> > We need the vmdesc to create info for the device.
>
> Migration may ignore the save entry if save_state() not provided in the
> "devices" section:
>
>          if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
>              continue;
>          }
>
> Could you try providing a shim save_state() for the new virtio-mem save
> entry?
>
> /*
>   * Shim function to make sure the save entry will be dumped into "devices"
>   * section, to make analyze-migration.py happy.
>   */
> static void virtio_mem_save_state_early(QEMUFile *file, void *opaque)
> {
> }
>
> Then:
>
> static const SaveVMHandlers vmstate_virtio_mem_device_early_ops = {
>      .save_setup = virtio_mem_save_setup_early,
>      .save_state = virtio_mem_save_state_early,
>      .load_state = virtio_mem_load_state_early,
> };
>
> I'm not 100% sure it'll work yet, but maybe worth trying.

It doesn't. virtio_mem_load_state_early() will get called twice (once 
with state saved during save_setup() and once with effectively nothing 
during save_state(), which breaks the whole migration).

vmdesc handling is also wrong, because analyze-migration.py gets 
confused because it receives data stored during save_setup() but vmdesc 
created during save_state() was told that there would be "nothing" to 
interpret ...

$ ./scripts/analyze-migration.py -f STATEFILE
{
    "ram (2)": {
        "section sizes": {
            "0000:00:03.0/mem0": "0x0000000f00000000",
            "pc.ram": "0x0000000100000000",
            "/rom@etc/acpi/tables": "0x0000000000020000",
            "pc.bios": "0x0000000000040000",
            "0000:00:02.0/e1000.rom": "0x0000000000040000",
            "pc.rom": "0x0000000000020000",
            "/rom@etc/table-loader": "0x0000000000001000",
            "/rom@etc/acpi/rsdp": "0x0000000000001000"
        }
    },
    "0000:00:03.0/virtio-mem-early (51)": {
        "data": ""
    }
}


Not sure if the whole thing becomes nicer when manually looking up the 
vmdesc ... because filling it will also requires manually storing the 
se->idstr and the se->instance_id, whereby both are migration-internal 
and not available inside save_setup().


Hm, I really prefer something like the simplified version that let's 
migration core deal with vmstate to be migrated during save_setup() 
phase. We could avoid the vmstate->immutable flag and simply use a 
separate function for registering the vmstate, like:

vmstate_register_immutable()
vmstate_register_early()
...

--
Thanks,

David / dhildenb