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

[David Hildenbrand <david@redhat.com>]

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