From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brjCH-0002oo-3R for qemu-devel@nongnu.org; Wed, 05 Oct 2016 06:13:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brjCB-0002Hf-W0 for qemu-devel@nongnu.org; Wed, 05 Oct 2016 06:13:08 -0400 Date: Wed, 5 Oct 2016 11:12:55 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161005101254.GD2036@work-vm> References: <1475519097-27611-1-git-send-email-duanj@linux.vnet.ibm.com> <1475519097-27611-2-git-send-email-duanj@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1475519097-27611-2-git-send-email-duanj@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [QEMU PATCH v5 1/6] migration: alternative way to set instance_id in SaveStateEntry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jianjun Duan Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, dmitry@daynix.com, peter.maydell@linaro.org, kraxel@redhat.com, mst@redhat.com, david@gibson.dropbear.id.au, pbonzini@redhat.com, veroniabahaa@gmail.com, quintela@redhat.com, amit.shah@redhat.com, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, aurelien@aurel32.net, leon.alrae@imgtec.com, blauwirbel@gmail.com, mark.cave-ayland@ilande.co.uk, mdroth@linux.vnet.ibm.com * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > In QOM(QEMU Object Model) migrated objects are identified with instance_id > which is calculated automatically using their path in the QOM composition > tree. For some objects, this path could change from source to target in > migration. To migrate such objects, we need to make sure the instance_id does > not change from source to target. We add a hook in DeviceClass to do customized > instance_id calculation in such cases. Can you explain a bit about why the path changes from source to destination; the path here should be a feature of the guest state not the host, and so I don't understand why it changes. > As a result, in these cases compat will not be set in the concerned > SaveStateEntry. This will prevent the inconsistent idstr to be sent over in > migration. We could have set alias_id in a similar way. But that will be > overloading the purpose of alias_id. > > The first application will be setting instance_id for DRC using its unique > index. Doing this makes the instance_id of DRC to be consistent across migration > and supports flexible management of DRC objects in migration. Is there a reason to use a custom instance_id rather than a custom idstr? > > Signed-off-by: Jianjun Duan > --- > include/hw/qdev-core.h | 6 ++++++ > migration/savevm.c | 20 ++++++++++++++++++-- > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 2c97347..a012e8e 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -139,6 +139,12 @@ typedef struct DeviceClass { > qdev_initfn init; /* TODO remove, once users are converted to realize */ > qdev_event exit; /* TODO remove, once users are converted to unrealize */ > const char *bus_type; > + > + /* When this field is set, qemu will use it to get an unique instance_id > + * instead of calculating an auto idstr and instanc_id for the relevant > + * SaveStateEntry > + */ > + int (*dev_get_instance_id)(DeviceState *dev); > } DeviceClass; > > typedef struct NamedGPIOList NamedGPIOList; > diff --git a/migration/savevm.c b/migration/savevm.c > index 33a2911..ef5c3d1 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -495,6 +495,11 @@ int register_savevm_live(DeviceState *dev, > void *opaque) > { > SaveStateEntry *se; > + /* when it is a device and it provides a way to get instance_id, > + * we will use it and skip setting idstr and compat. > + */ > + bool flag = (dev != NULL) && > + (DEVICE_GET_CLASS(dev)->dev_get_instance_id != NULL); We must be able to get a more descriptive name than 'flag'; how about 'has_custom_id' > se = g_new0(SaveStateEntry, 1); > se->version_id = version_id; > @@ -507,7 +512,7 @@ int register_savevm_live(DeviceState *dev, > se->is_ram = 1; > } > > - if (dev) { > + if (dev && !flag) { > char *id = qdev_get_dev_path(dev); > if (id) { > pstrcpy(se->idstr, sizeof(se->idstr), id); > @@ -523,6 +528,9 @@ int register_savevm_live(DeviceState *dev, > } > pstrcat(se->idstr, sizeof(se->idstr), idstr); > > + if (flag) { > + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); > + } > if (instance_id == -1) { > se->instance_id = calculate_new_instance_id(se->idstr); > } else { > @@ -580,6 +588,11 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, > int required_for_version) > { > SaveStateEntry *se; > + /* when it is a device and it provides a way to get instance_id, > + * we will use it and skip setting idstr and compat. > + */ > + bool flag = (dev != NULL) && > + (DEVICE_GET_CLASS(dev)->dev_get_instance_id != NULL); > > /* If this triggers, alias support can be dropped for the vmsd. */ > assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id); > @@ -591,7 +604,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, > se->vmsd = vmsd; > se->alias_id = alias_id; > > - if (dev) { > + if (dev && !flag) { > char *id = qdev_get_dev_path(dev); > if (id) { > pstrcpy(se->idstr, sizeof(se->idstr), id); > @@ -607,6 +620,9 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, > } > pstrcat(se->idstr, sizeof(se->idstr), vmsd->name); > > + if (flag) { > + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); > + } > if (instance_id == -1) { > se->instance_id = calculate_new_instance_id(se->idstr); > } else { > -- > 1.9.1 Dave > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK