Re: [Qemu-devel] [RFC PATCH] spapr: support time base offset migration

[Alexey Kardashevskiy]

On 09/03/2013 07:22 PM, Andreas Färber wrote:
> Am 03.09.2013 11:07, schrieb Alexey Kardashevskiy:
>> On 09/03/2013 06:42 PM, Andreas Färber wrote:
>>> Am 03.09.2013 09:31, schrieb Alexey Kardashevskiy:
>>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>>>> index 12e1512..d1ffc7f 100644
>>>> --- a/target-ppc/machine.c
>>>> +++ b/target-ppc/machine.c
> [...]
>>>> +static const VMStateDescription vmstate_timebase = {
>>>> +    .name = "cpu/timebase",
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .minimum_version_id_old = 1,
>>>> +    .pre_save = timebase_pre_save,
>>>> +    .post_load = timebase_post_load,
>>>> +    .fields      = (VMStateField []) {
>>>> +        VMSTATE_UINT64(timebase, ppc_tb_t),
>>>> +        VMSTATE_INT64(tb_offset, ppc_tb_t),
>>>> +        VMSTATE_UINT64(time_of_the_day, ppc_tb_t),
>>>> +        VMSTATE_UINT32_EQUAL(tb_freq, ppc_tb_t),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    },
>>>> +};
>>>> +
>>>>  const VMStateDescription vmstate_ppc_cpu = {
>>>>      .name = "cpu",
>>>>      .version_id = 5,
>>>> @@ -498,6 +538,10 @@ const VMStateDescription vmstate_ppc_cpu = {
>>>>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>>>>          VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>>>>          VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
>>>> +
>>>> +        /* Time offset */
>>>> +        VMSTATE_STRUCT_POINTER(env.tb_env, PowerPCCPU,
>>>> +                               vmstate_timebase, ppc_tb_t *),
>>>>          VMSTATE_END_OF_LIST()
>>>>      },
>>>>      .subsections = (VMStateSubsection []) {
>>>
>>> Breaks the migration format. ;) You need to bump version_id and use a
>>> macro that accepts the version the field was added in as argument.
>>
>> Risking of being called ignorant, I'll still ask - is the patch below what
>> you mean? I could not find VMSTATE_STRUCT_POINTER_V and I do not believe it
>> is not there already.
> 
> Usually the way we do it is to have VMSTATE_STRUCT_POINTER() call
> VMSTATE_STRUCT_POINTER_V() and thus VMSTATE_STRUCT_POINTER_TEST() call a
> new VMSTATE_STRUCT_POINTER_TEST_V(), to avoid code duplication of the
> core array entry. CC'ing Juan. Please do the VMState preparation in a
> separate patch.
>
> ppc usage looks fine.
> 
>> btw why would it break? Just asking. Is it because the source may send what
>> the destination cannot handle? Named fields should stop the migration the
>> same way as version mismatch would have done.
> 
> Nope, field names do not get transmitted, only the section names.
> (Otherwise random code refactorings could break the migration format.)
> 
>> Or the source won't sent what the destination expects and we do not want
>> this destination guest to continue?
> 
> There's an incoming stream of data from either live migration or a file,
> and QEMU must decide whether it can read and how to interpret the raw
> bytestream. It shouldn't just read random bytes into a new field when
> they were written from a different field.
> 
>> Once I was told that migration between different versions of QEMU is not
>> supported - so what is the point of .version_id field at all?
> 
> Not sure who told such a thing and in what context. On x86 we try to
> avoid version_id bumps by adding subsections to allow migration in both
> ways (including from newer to older) but for ppc, arm and all others we
> do require that new fields are marked as such. Whether migration is
> officially supported is a different matter from the VMState wire format.


Why is the approach different for x86 and ppc here? I can convert
VMSTATE_STRUCT_POINTER to a subsection, why should not I? Or ppc is not
mature enough and therefore there is no need to keep compatibility? Thanks.


> 
> Regards,
> Andreas
> 
> 
>> address@hidden:~/pcipassthru/qemu$ git diff
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 1c31b5d..7b624bf 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -499,6 +499,15 @@ extern const VMStateInfo vmstate_info_bitmap;
>>  #define VMSTATE_STRUCT_POINTER(_field, _state, _vmsd, _type)          \
>>      VMSTATE_STRUCT_POINTER_TEST(_field, _state, NULL, _vmsd, _type)
>>
>> +#define VMSTATE_STRUCT_POINTER_V(_field, _state,  _vmsd, _type, _version) { 
>> \
>> +    .name         = (stringify(_field)),                             \
>> +    .version_id = (_version),                                        \
>> +    .vmsd         = &(_vmsd),                                        \
>> +    .size         = sizeof(_type),                                   \
>> +    .flags        = VMS_STRUCT|VMS_POINTER,                          \
>> +    .offset       = vmstate_offset_value(_state, _field, _type),     \
>> +}
>> +
>>  #define VMSTATE_STRUCT_ARRAY(_field, _state, _num, _version, _vmsd, _type) \
>>      VMSTATE_STRUCT_ARRAY_TEST(_field, _state, _num, NULL, _version,   \
>>              _vmsd, _type)
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index b4f447c..f79f38e 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -501,7 +501,7 @@ static const VMStateDescription vmstate_timebase = {
>>
>>  const VMStateDescription vmstate_ppc_cpu = {
>>      .name = "cpu",
>> -    .version_id = 5,
>> +    .version_id = 6,
>>      .minimum_version_id = 5,
>>      .minimum_version_id_old = 4,
>>      .load_state_old = cpu_load_old,
>> @@ -540,8 +540,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>>          VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
>>
>>          /* Time offset */
>> -        VMSTATE_STRUCT_POINTER(env.tb_env, PowerPCCPU,
>> -                               vmstate_timebase, ppc_tb_t *),
>> +        VMSTATE_STRUCT_POINTER_V(env.tb_env, PowerPCCPU,
>> +                                 vmstate_timebase, ppc_tb_t *, 6),
>>          VMSTATE_END_OF_LIST()
>>      },
>>      .subsections = (VMStateSubsection []) {
> 


-- 
Alexey