Re: [Qemu-ppc] [QEMU PATCH v5 1/6] migration: alternative way to set instance_id in SaveStateEntry

[Dr. David Alan Gilbert]

* Jianjun Duan (address@hidden) 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 <address@hidden>
> ---
>  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 / address@hidden / Manchester, UK