Re: [PATCH v3 07/33] serial: register vmsd with DeviceClass

[Marc-André Lureau]

Hi

On Wed, Nov 20, 2019 at 10:54 PM Dr. David Alan Gilbert
<address@hidden> wrote:
>
> * Marc-André Lureau (address@hidden) wrote:
> > On Tue, Nov 19, 2019 at 2:35 PM Peter Maydell <address@hidden> wrote:
> > >
> > > On Tue, 19 Nov 2019 at 10:23, Marc-André Lureau
> > > <address@hidden> wrote:
> > > > On Mon, Nov 18, 2019 at 6:22 PM Peter Maydell <address@hidden> wrote:
> > > > > Did you test whether migration still works from a QEMU
> > > > > version without this patch to one with it? (The migration
> > > >
> > > > Yes, I thought I did test correctly, but I realized testing with x86
> > > > isn't correct.
> > > >
> > > > So with arm/musicpal for ex, I can migrate from before->after, however
> > > > after->before won't work. Is that ok?
> > >
> > > Broadly speaking, the only case where we care about not
> > > breaking cross-version migration is where we have a versioned
> > > machine type. So musicpal doesn't matter too much. Beyond
> > > that, yes, generally before->after is more important than
> > > after->before. I have a feeling Red Hat downstream cares about
> > > after->before migration at least for x86 but you or your colleagues
> > > would know that better than me :-)
> > >
> > > > > vmstate code is too complicated for me to be able to figure
> > > > > out whether passing the 'dev' pointer makes a difference
> > > > > to whot it names the state sections and whether the
> > > > > 'qdev_set_legacy_instance_id' suffices to avoid problems.)
> > > >
> > > > I don't see a way to fix after->before, because the instance id is
> > > > initially 0 with the new code, and the old code expect a different
> > > > value.
> > >
> > > Can you explain how the instance ID stuff works? I was
> > > expecting that the result of setting the legacy instance ID
> > > would just be that the new version would always have
> > > the older setting, so if it works for old->new it would also
> > > work for new->old. But as I say I don't understand this bit
> > > of the migration code.
> >
> > From what I understand, the alias_id is only used in
> > savevm.c:find_se(), and thus can only be used to match against
> > "legacy" instance id values. On new code, instance_id is generated
> > incrementally from 0 with calculate_new_instance_id(), based on
> > "qdev-path/vmsd-name".
>
> I think there are cases here there's no qdev path that's viable;
> e.g. for ISA devices, the ID is set to the ISA IO base:
>
> hw/char/serial-isa.c
> 79:    qdev_set_legacy_instance_id(dev, isa->iobase, 3);
>
> (In serial_isa_realizefn )
>
> but to be honest I'd have to trace this out and see what values the
> devices are actually using to be sure.

There is no qdev path, because ISA bus doesn't have get_dev_path()
implemented for some reason.

However, vmstate_register_with_alias_id() will use
calculate_new_instance_id(se->idstr) in this case.

>
> (And yes, please don't break backwards migration; otherwise I'll
> end up having to figure out a fix).

My understanding is that qdev_set_legacy_instance_id() always broke
backward migration.

To keep backward migration to work, we would need a mechanism to
"force" to use legacy instance id.

Would it be acceptable to have a patch that does that when the
original VM state uses legacy instance id?
If you start the VM with the new code path, and try to migrate to the
old / legacy it would fail. But migrating existing old VM back and
forth between old/new would work.





-- 
Marc-André Lureau