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

[Andreas Färber]

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.

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 []) {

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg