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

[Marc-André Lureau]

Hi

On Fri, Dec 13, 2019 at 12:18 AM Dr. David Alan Gilbert
<address@hidden> wrote:
>
> 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).

None of the qdev devices using vmsd use a hard-coded instance id
afaik. In device_set_realized(), vmstate_register_with_alias_id() is
called with instance_id = -1, so it relies on
calculate_new_instance_id(se->idstr)

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


-- 
Marc-André Lureau