Re: [PATCH v4 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Dr. David Alan Gilbert]

Apologies for the delay.

* Marc-André Lureau (address@hidden) wrote:
> Hi
> 
> On Sun, Dec 1, 2019 at 10:10 PM Peter Maydell <address@hidden> wrote:
> >
> > On Sun, 1 Dec 2019 at 17:27, Marc-André Lureau
> > <address@hidden> wrote:
> > >
> > > Hi
> > >
> > > On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell <address@hidden> wrote:
> > > >
> > > > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau
> > > > <address@hidden> wrote:
> > > > >
> > > > > - "serial: register vmsd with DeviceClass"
> > > > >
> > > > > This is standard qdev-ification, however it breaks backward migration,
> > > > > but that's just how qdev_set_legacy_instance_id() works.
> > > >
> > > > I don't understand this part. Surely the whole point
> > > > of setting a legacy instance ID is exactly to preserve
> > > > migration compatibility? If it doesn't do that then what
> > > > does setting legacy ID value do?
> > > >
> > >
> > > It works in old->new direction only, because new code can match the
> > > legacy instance id.
> > >
> > > But when going from new->old, the legacy instance id is lost, as it
> > > uses new 0-based instance_id.
> >
> > I still don't understand. My mental model of the situation is:
> >
> >  * in the old (current) version of the code, the instance ID
> >    is some random thing resulting from what the old code does
> 
> right
> 
> >  * in the new version of the code, we use qdev_set_legacy_instance_id,
> >    and so instead of using the ID you'd naturally get as a
> >    written-from-scratch qdev device, it uses the legacy value
> >    you pass in
> 
> no, it only sets the SaveStateEntry.alias_id, which is only used
> during incoming migration in find_se().
> 
> Iow, it only works old->new.
> 
> >  * thus the device/board in both old and new versions of QEMU
> >    uses the same value and migration in both directions works
> 
> sadly no
> 
> >
> > I don't understand why we would ever be using a "new 0-based
> > instance_id" -- it seems to me that the whole point of setting
> > a legacy ID value is that we will use it always, and I don't
> > understand how the board code can know that it's going to be
> > the target of an old->new migration as opposed to being the
> > source of a new->old migration such that it can end up with
> > a different ID value in the latter case.
> 
> The target will find the "legacy" alias with find_se() on incoming
> migration, but any new outgoing migration will use the new 0-based
> instance_id
> 
> >
> > If qdev_set_legacy_instance_id() doesn't work the way I
> > think it does above, what *does* it do ?
> 
> just set the old alias_id for incoming migration.
> 
> David, is that correct?

Yes, I think it is.
However, I'm curious which devices you're finding are explicitly setting
their id's;  there aren't many - although there are some that probably
should!
For example, running an x86 image with:
   -device isa-parallel,chardev=... -device isa-serial -device isa-serial 
-trace enable=qemu_loadvm_state_section_startfull

shows:
qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, 
uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u"

165217@1576179638.856300:qemu_loadvm_state_section_startfull 41(serial) 0 3
165217@1576179638.856307:qemu_loadvm_state_section_startfull 42(serial) 1 3
165217@1576179638.856311:qemu_loadvm_state_section_startfull 43(parallel_isa) 0 
1

so those two serial devices are instances '0' and '1' I think by luck of
their command line order, rather than having specified their base
address (which would have been safer).

Dave



> thanks
> 
> 
> -- 
> Marc-André Lureau
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK