On Wed, Jan 11, 2023 at 02:48:09PM +0100, David Hildenbrand wrote:
> 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).
Oh hold on.. so load_state() to pair save_setup()? Maybe you should pair it
with load_setup(), which I just noticed.. Then the load_state() needs to
be another shim like save_state().
>
> 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": ""
> }
> }
Yeah this is expected, because the whole data stream within the setup phase
is a black box and not described by anything. "ram" can display correctly
only because it's hard coded in the python script, I think. The trick
above can only make the script not break but not teaching the script to
also check for the early vmsd.
But that's one step forward and IMHO it's fine for special devices. For
example, even with your initial patch, I think the static analyzer (aka,
qemu -dump-vmstate) will also ignore your new vmsd anyway because it's not
directly bound to the DeviceState* but registered separately. So no ideal
solution so far, afaict, without more work done on this aspect.
>
>
> 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()
> ...
I agree, this looks useful. It's just that the devices that need this will
be rare, and unfortunately some of them already implemented the stream in
other ways so maybe non-trivial to convert them.
Not against it if you want to keep exploring, but if so please avoid using
the priority field, also I'd hope the early vmsd will be saved within
qemu_savevm_state_setup() so maybe it can be another alternative to
save_setup().
Maybe it's still easier we keep going with the save_setup() and the shim
functions above, if it works for you.
--
Peter Xu
