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

[Jianjun Duan]

On 10/05/2016 03:12 AM, Dr. David Alan Gilbert wrote:
> * 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'
Sure. Will wait for more comments and address them together.

>>      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
> 
Thanks,
Jianjun