* Question about vmstate_register(), dc->vmsd and instance_id
@ 2022-03-17 13:58 Daniel Henrique Barboza
2022-03-17 15:04 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-03-17 13:58 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson
Hi,
I've been looking into converting some vmstate_register() calls to use dc->vmsd,
using as a base the docs in docs/devel/migration.rst. This doc mentions that we
can either register the vmsd by using vmstate_register() or we can use dc->vmsd
for qdev-based devices.
When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
drc_realize()) I found this:
vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
drc);
spapr_drc_index() is an unique identifier for these DRC devices and it's being used
as instance_id. It is not clear to me how we can keep using this same instance_id when
using the dc->vmsd alternative. By looking a bit into migration files I understood
that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
Another related question is the role of instance_id per se. I understand that this
value is used to identify SaveStateEntries in migration/savevm.c and it's autogenerated
if the caller does not provide it. And there's also this comment from
register_savevm_live():
/* TODO: Individual devices generally have very little idea about the rest
of the system, so instance_id should be removed/replaced.
Meanwhile pass -1 as instance_id if you do not already have a clearly
distinguishing id for all instances of your device class. */
Given that this is a 13 year old comment from Anthony Liguori I wanted to confirm its
validity. Is there a long term goal of getting rid of instance_id? Can I ignore its
role when converting these calls to dc->vmsd?
Thanks,
Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about vmstate_register(), dc->vmsd and instance_id
2022-03-17 13:58 Question about vmstate_register(), dc->vmsd and instance_id Daniel Henrique Barboza
@ 2022-03-17 15:04 ` Peter Maydell
2022-03-17 16:29 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2022-03-17 15:04 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-devel, David Gibson
On Thu, 17 Mar 2022 at 14:03, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
> I've been looking into converting some vmstate_register() calls to use dc->vmsd,
> using as a base the docs in docs/devel/migration.rst. This doc mentions that we
> can either register the vmsd by using vmstate_register() or we can use dc->vmsd
> for qdev-based devices.
>
> When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
> drc_realize()) I found this:
>
> vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
> drc);
>
> spapr_drc_index() is an unique identifier for these DRC devices and it's being used
> as instance_id. It is not clear to me how we can keep using this same instance_id when
> using the dc->vmsd alternative. By looking a bit into migration files I understood
> that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
Not entirely. It is the intended common setup, but because changing
the ID value breaks migration compatibility there is a mechanism
for saying "my device is special and needs to set the instance ID
to something else" -- qdev_set_legacy_instance_id().
> Given that this is a 13 year old comment from Anthony Liguori I wanted to confirm its
> validity. Is there a long term goal of getting rid of instance_id? Can I ignore its
> role when converting these calls to dc->vmsd?
Only if you're prepared to break migration compatibility, I think.
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about vmstate_register(), dc->vmsd and instance_id
2022-03-17 15:04 ` Peter Maydell
@ 2022-03-17 16:29 ` Dr. David Alan Gilbert
2022-03-18 3:43 ` David Gibson
2022-03-23 21:39 ` Daniel Henrique Barboza
0 siblings, 2 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2022-03-17 16:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: Daniel Henrique Barboza, qemu-devel, David Gibson
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 17 Mar 2022 at 14:03, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
> > I've been looking into converting some vmstate_register() calls to use dc->vmsd,
> > using as a base the docs in docs/devel/migration.rst. This doc mentions that we
> > can either register the vmsd by using vmstate_register() or we can use dc->vmsd
> > for qdev-based devices.
> >
> > When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
> > drc_realize()) I found this:
> >
> > vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
> > drc);
> >
> > spapr_drc_index() is an unique identifier for these DRC devices and it's being used
> > as instance_id. It is not clear to me how we can keep using this same instance_id when
> > using the dc->vmsd alternative. By looking a bit into migration files I understood
> > that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
>
> Not entirely. It is the intended common setup, but because changing
> the ID value breaks migration compatibility there is a mechanism
> for saying "my device is special and needs to set the instance ID
> to something else" -- qdev_set_legacy_instance_id().
Yes, this is normally only an issue for 'system' or memory mapped
devices; for things hung off a bus that has it's own device naming,
then each instance of a device has it's own device due to the bus name
so instance_id's aren't used. Where you've got a few of the
same device with the same name, and no bus for them to be named by, then
the instance_id is used to uniquify them.
Dave
> > Given that this is a 13 year old comment from Anthony Liguori I wanted to confirm its
> > validity. Is there a long term goal of getting rid of instance_id? Can I ignore its
> > role when converting these calls to dc->vmsd?
>
> Only if you're prepared to break migration compatibility, I think.
>
> -- PMM
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about vmstate_register(), dc->vmsd and instance_id
2022-03-17 16:29 ` Dr. David Alan Gilbert
@ 2022-03-18 3:43 ` David Gibson
2022-03-18 19:51 ` Daniel Henrique Barboza
2022-03-23 21:39 ` Daniel Henrique Barboza
1 sibling, 1 reply; 7+ messages in thread
From: David Gibson @ 2022-03-18 3:43 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Peter Maydell, Daniel Henrique Barboza, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2376 bytes --]
On Thu, Mar 17, 2022 at 04:29:14PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > On Thu, 17 Mar 2022 at 14:03, Daniel Henrique Barboza
> > <danielhb413@gmail.com> wrote:
> > > I've been looking into converting some vmstate_register() calls to use dc->vmsd,
> > > using as a base the docs in docs/devel/migration.rst. This doc mentions that we
> > > can either register the vmsd by using vmstate_register() or we can use dc->vmsd
> > > for qdev-based devices.
> > >
> > > When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
> > > drc_realize()) I found this:
> > >
> > > vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
> > > drc);
> > >
> > > spapr_drc_index() is an unique identifier for these DRC devices and it's being used
> > > as instance_id. It is not clear to me how we can keep using this same instance_id when
> > > using the dc->vmsd alternative. By looking a bit into migration files I understood
> > > that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
> >
> > Not entirely. It is the intended common setup, but because changing
> > the ID value breaks migration compatibility there is a mechanism
> > for saying "my device is special and needs to set the instance ID
> > to something else" -- qdev_set_legacy_instance_id().
>
> Yes, this is normally only an issue for 'system' or memory mapped
> devices; for things hung off a bus that has it's own device naming,
> then each instance of a device has it's own device due to the bus name
> so instance_id's aren't used. Where you've got a few of the
> same device with the same name, and no bus for them to be named by, then
> the instance_id is used to uniquify them.
Thanks for the information. I remember deciding at the time that just
using vmsd wouldn't work for the DRCs because we needed this fixed
index. At the time either qdev_set_legacy_instance_id() didn't exist,
or I didn't know about it, hence the explicit vmstate_register() call
so that an explicit instance id could be supplied.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about vmstate_register(), dc->vmsd and instance_id
2022-03-18 3:43 ` David Gibson
@ 2022-03-18 19:51 ` Daniel Henrique Barboza
2022-03-19 9:43 ` David Gibson
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-03-18 19:51 UTC (permalink / raw)
To: David Gibson, Dr. David Alan Gilbert; +Cc: Peter Maydell, qemu-devel
On 3/18/22 00:43, David Gibson wrote:
> On Thu, Mar 17, 2022 at 04:29:14PM +0000, Dr. David Alan Gilbert wrote:
>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>> On Thu, 17 Mar 2022 at 14:03, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>> I've been looking into converting some vmstate_register() calls to use dc->vmsd,
>>>> using as a base the docs in docs/devel/migration.rst. This doc mentions that we
>>>> can either register the vmsd by using vmstate_register() or we can use dc->vmsd
>>>> for qdev-based devices.
>>>>
>>>> When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
>>>> drc_realize()) I found this:
>>>>
>>>> vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>>>> drc);
>>>>
>>>> spapr_drc_index() is an unique identifier for these DRC devices and it's being used
>>>> as instance_id. It is not clear to me how we can keep using this same instance_id when
>>>> using the dc->vmsd alternative. By looking a bit into migration files I understood
>>>> that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
>>>
>>> Not entirely. It is the intended common setup, but because changing
>>> the ID value breaks migration compatibility there is a mechanism
>>> for saying "my device is special and needs to set the instance ID
>>> to something else" -- qdev_set_legacy_instance_id().
>>
>> Yes, this is normally only an issue for 'system' or memory mapped
>> devices; for things hung off a bus that has it's own device naming,
>> then each instance of a device has it's own device due to the bus name
>> so instance_id's aren't used. Where you've got a few of the
>> same device with the same name, and no bus for them to be named by, then
>> the instance_id is used to uniquify them.
Thanks for the info. qdev_set_legacy_instance_id() was the missing piece I was
looking for to continue with the dc->vmsd transition I'd like to do.
>
> Thanks for the information. I remember deciding at the time that just
> using vmsd wouldn't work for the DRCs because we needed this fixed
> index. At the time either qdev_set_legacy_instance_id() didn't exist,
> or I didn't know about it, hence the explicit vmstate_register() call
> so that an explicit instance id could be supplied.
>
This is the commit that introduced DRC migration:
commit a50919dddf148b0a2008db4a0593dbe69e1059c0
Author: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
Date: Mon May 22 16:35:49 2017 -0300
hw/ppc: migrating the DRC state of hotplugged devices
I'd say you can cut yourself some slack this time. Blame that guy instead.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about vmstate_register(), dc->vmsd and instance_id
2022-03-18 19:51 ` Daniel Henrique Barboza
@ 2022-03-19 9:43 ` David Gibson
0 siblings, 0 replies; 7+ messages in thread
From: David Gibson @ 2022-03-19 9:43 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: Peter Maydell, Dr. David Alan Gilbert, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3321 bytes --]
On Fri, Mar 18, 2022 at 04:51:10PM -0300, Daniel Henrique Barboza wrote:
>
>
> On 3/18/22 00:43, David Gibson wrote:
> > On Thu, Mar 17, 2022 at 04:29:14PM +0000, Dr. David Alan Gilbert wrote:
> > > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > > On Thu, 17 Mar 2022 at 14:03, Daniel Henrique Barboza
> > > > <danielhb413@gmail.com> wrote:
> > > > > I've been looking into converting some vmstate_register() calls to use dc->vmsd,
> > > > > using as a base the docs in docs/devel/migration.rst. This doc mentions that we
> > > > > can either register the vmsd by using vmstate_register() or we can use dc->vmsd
> > > > > for qdev-based devices.
> > > > >
> > > > > When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
> > > > > drc_realize()) I found this:
> > > > >
> > > > > vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
> > > > > drc);
> > > > >
> > > > > spapr_drc_index() is an unique identifier for these DRC devices and it's being used
> > > > > as instance_id. It is not clear to me how we can keep using this same instance_id when
> > > > > using the dc->vmsd alternative. By looking a bit into migration files I understood
> > > > > that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
> > > >
> > > > Not entirely. It is the intended common setup, but because changing
> > > > the ID value breaks migration compatibility there is a mechanism
> > > > for saying "my device is special and needs to set the instance ID
> > > > to something else" -- qdev_set_legacy_instance_id().
> > >
> > > Yes, this is normally only an issue for 'system' or memory mapped
> > > devices; for things hung off a bus that has it's own device naming,
> > > then each instance of a device has it's own device due to the bus name
> > > so instance_id's aren't used. Where you've got a few of the
> > > same device with the same name, and no bus for them to be named by, then
> > > the instance_id is used to uniquify them.
>
>
> Thanks for the info. qdev_set_legacy_instance_id() was the missing piece I was
> looking for to continue with the dc->vmsd transition I'd like to do.
>
>
> >
> > Thanks for the information. I remember deciding at the time that just
> > using vmsd wouldn't work for the DRCs because we needed this fixed
> > index. At the time either qdev_set_legacy_instance_id() didn't exist,
> > or I didn't know about it, hence the explicit vmstate_register() call
> > so that an explicit instance id could be supplied.
> >
>
> This is the commit that introduced DRC migration:
>
>
> commit a50919dddf148b0a2008db4a0593dbe69e1059c0
> Author: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> Date: Mon May 22 16:35:49 2017 -0300
>
> hw/ppc: migrating the DRC state of hotplugged devices
>
>
> I'd say you can cut yourself some slack this time. Blame that guy
> instead.
Man, not that guy again! ;-)
I think I must have done something similar with some other migration
component.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about vmstate_register(), dc->vmsd and instance_id
2022-03-17 16:29 ` Dr. David Alan Gilbert
2022-03-18 3:43 ` David Gibson
@ 2022-03-23 21:39 ` Daniel Henrique Barboza
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-03-23 21:39 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Peter Maydell; +Cc: qemu-devel, David Gibson
On 3/17/22 13:29, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On Thu, 17 Mar 2022 at 14:03, Daniel Henrique Barboza
>> <danielhb413@gmail.com> wrote:
>>> I've been looking into converting some vmstate_register() calls to use dc->vmsd,
>>> using as a base the docs in docs/devel/migration.rst. This doc mentions that we
>>> can either register the vmsd by using vmstate_register() or we can use dc->vmsd
>>> for qdev-based devices.
>>>
>>> When trying to convert this vmstate() call for the qdev alternative (hw/ppc/spapr_drc.c,
>>> drc_realize()) I found this:
>>>
>>> vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>>> drc);
>>>
>>> spapr_drc_index() is an unique identifier for these DRC devices and it's being used
>>> as instance_id. It is not clear to me how we can keep using this same instance_id when
>>> using the dc->vmsd alternative. By looking a bit into migration files I understood
>>> that if dc->vmsd is being used the instance_id is always autogenerated. Is that correct?
>>
>> Not entirely. It is the intended common setup, but because changing
>> the ID value breaks migration compatibility there is a mechanism
>> for saying "my device is special and needs to set the instance ID
>> to something else" -- qdev_set_legacy_instance_id().
>
> Yes, this is normally only an issue for 'system' or memory mapped
> devices; for things hung off a bus that has it's own device naming,
> then each instance of a device has it's own device due to the bus name
> so instance_id's aren't used. Where you've got a few of the
> same device with the same name, and no bus for them to be named by, then
> the instance_id is used to uniquify them.
(long reply inc)
So, qdev_set_legacy_instance_id() doesn't set 'instance_id' as I've expected but rather
'alias_id'. The function will set dev->instance_id_alias, which is then used in device_set_realized()
as follows:
if (qdev_get_vmsd(dev)) {
if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
VMSTATE_INSTANCE_ID_ANY,
qdev_get_vmsd(dev), dev,
dev->instance_id_alias,
dev->alias_required_for_version,
&local_err) < 0) {
goto post_realize_fail;
}
}
instance_id is set to VMSTATE_INSTANCE_ID_ANY, meaning that is going to be autogenerated. The
SaveStateEntry (SE) will be generated with se->alias_id = (custom value we set) and
se->instance_id = autogenerated.
The migration stream transmits se->instance_id but not se->alias_id. When loading the migration
in the destination, find_se() will make a search using the received instance_id from the source
and compare it to both se->instance_id and se->alias_id from the destination.
If I try to convert an existing migratable device that is setting instance_id via vmstate_register()
to use qdev's dc->vmsd, if the existing code is already setting instance_id via vmstate_register(),
I end up breaking backward migration. This is what happened in patch
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg05617.html where I attempted this
conversion.
The code before the patch (B) has the following SEs for the device I changed:
===== spapr_iommu: se->instanceid = 0x80000000 se->alias_id = 0xffffffff ====
===== spapr_iommu: se->instanceid = 0x80000001 se->alias_id = 0xffffffff ====
And the code after the patch (A):
===== spapr_iommu: se->instanceid = 0x0 se->alias_id = 0x80000000 ====
===== spapr_iommu: se->instanceid = 0x1 se->alias_id = 0x80000001 ====
Migrating a pseries guest from B to A works because the new code, although using a different
instance_id, is matching with its alias_id. This is the output in A using the following trace:
trace_qemu_loadvm_state_section_startfull(section_id, idstr, instance_id, version_id);
qemu_loadvm_state_section_startfull 15(vty@71000000/spapr_vty) 0 1
qemu_loadvm_state_section_startfull 16(nvram@71000001/spapr_nvram) 0 1
qemu_loadvm_state_section_startfull 560(spapr_iommu) 2147483648 2
qemu_loadvm_state_section_startfull 561(spapr_iommu) 2147483649 2
(...)
But the backward migration, A to B, doesn't work:
qemu_loadvm_state_section_startfull 560 (spapr_iommu) 0 2
qemu-system-ppc64: Unknown savevm section or instance 'spapr_iommu' 0. Make sure that your current
VM setup matches your saved VM setup, including any hotplugged devices
qemu-system-ppc64: load of migration failed: Invalid argument
The failure happens because the code without the patch is trying to match an instance_id = 0
(which A is now autogenerating) to its se->instance_id = 0x80000000 | se->alias = 0xffffffff.
The match fails and the error is thrown.
It seems that what I'm trying to do, convert vmstate_register() calls to qdev's dc->vmsd, when
the existing code is setting custom instance_ids in vmstate_register(), is not feasible to be
done without breaking backward migration. At least with the current qdev APIs.
qdev_set_legacy_instance_id() helps to allow older guests to migrate to newer QEMUs, but not
the other way around.
Am I missing something here?
Thanks,
Daniel
>
> Dave
>
>>> Given that this is a 13 year old comment from Anthony Liguori I wanted to confirm its
>>> validity. Is there a long term goal of getting rid of instance_id? Can I ignore its
>>> role when converting these calls to dc->vmsd?
>>
>> Only if you're prepared to break migration compatibility, I think.
>>
>> -- PMM
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-23 21:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-17 13:58 Question about vmstate_register(), dc->vmsd and instance_id Daniel Henrique Barboza
2022-03-17 15:04 ` Peter Maydell
2022-03-17 16:29 ` Dr. David Alan Gilbert
2022-03-18 3:43 ` David Gibson
2022-03-18 19:51 ` Daniel Henrique Barboza
2022-03-19 9:43 ` David Gibson
2022-03-23 21:39 ` Daniel Henrique Barboza
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.