From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brpJk-0001V3-4T for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:45:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brpJf-0001Mu-5C for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:45:16 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:41235) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brpJe-0001ME-SN for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:45:11 -0400 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u95GiLsd065550 for ; Wed, 5 Oct 2016 12:45:09 -0400 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0a-001b2d01.pphosted.com with ESMTP id 25w2gucycc-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 05 Oct 2016 12:45:09 -0400 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 5 Oct 2016 10:45:08 -0600 References: <1475519097-27611-1-git-send-email-duanj@linux.vnet.ibm.com> <1475519097-27611-2-git-send-email-duanj@linux.vnet.ibm.com> <20161005101254.GD2036@work-vm> From: Jianjun Duan Date: Wed, 5 Oct 2016 09:44:57 -0700 MIME-Version: 1.0 In-Reply-To: <20161005101254.GD2036@work-vm> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Message-Id: <9dc23ca2-9ca6-9b2e-9c22-257d11e234d1@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: "Dr. David Alan Gilbert" 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 Please see comments below: On 10/05/2016 03:12 AM, Dr. David Alan Gilbert wrote: > * 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. Please see the discussion with David in the previous versions: http://lists.nongnu.org/archive/html/qemu-ppc/2016-06/msg00062.html >> 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 It can be done either way. But it is easier to deal with a integer than a string. >> >> 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 > Thanks, Jianjun