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

[Alexander Graf]

On 04.09.2013, at 03:13, Alexey Kardashevskiy wrote:

> 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.

Just bump the version.


Alex