Re: [Qemu-devel] [QEMU RFC PATCH v4 1/6] migration: alternative way to set instance_id in SaveStateEntry

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5102 bytes --]

On Wed, Jun 08, 2016 at 04:06:52PM -0700, Jianjun Duan 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.
> 
> 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.
> 
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>

Ah, ok.  I misunderstood your earlier comment - I didn't realise you
needed a change to the migration core in order to set the instance
ids.

That said, I think being able to customize the instance ids could give
better ability to future-proof migration data, at least for certain
buses and devices.

Juan, Dave, what do you think?

> ---
>  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 1ce02b2..28173a7 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 6c21231..f7e7c68 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -494,6 +494,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);
>  
>      se = g_new0(SaveStateEntry, 1);
>      se->version_id = version_id;
> @@ -506,7 +511,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);
> @@ -522,6 +527,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 {
> @@ -579,6 +587,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);
> @@ -590,7 +603,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);
> @@ -606,6 +619,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 {

-- 
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: 819 bytes --]