Patchwork [RFC] spapr: support time base offset migration

login
register
mail settings
Submitter Alexey Kardashevskiy
Date Sept. 3, 2013, 9:07 a.m.
Message ID <5225A6C5.8030407@ozlabs.ru>
Download mbox | patch
Permalink /patch/272178/
State New
Headers show

Comments

Alexey Kardashevskiy - Sept. 3, 2013, 9:07 a.m.
On 09/03/2013 06:42 PM, Andreas Färber wrote:
> Am 03.09.2013 09:31, schrieb Alexey Kardashevskiy:
>> This allows guests to have a different timebase origin from the host.
>>
>> This is needed for migration, where a guest can migrate from one host
>> to another and the two hosts might have a different timebase origin.
>> However, the timebase seen by the guest must not go backwards, and
>> should go forwards only by a small amount corresponding to the time
>> taken for the migration.
>>
>> This is only supported for recent POWER hardware which has the TBU40
>> (timebase upper 40 bits) register. That includes POWER6, 7, 8 but not
>> 970.
>>
>> This adds kvm_access_one_reg() to access a special register which is not
>> in env->spr.
>>
>> The feature must be present in the host kernel.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>> This is an RFC but not a final patch. Can break something but I just do not see what.
>>
>> ---
>>  hw/ppc/ppc.c         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/ppc.h |  4 ++++
>>  target-ppc/kvm.c     | 23 +++++++++++++++++++++++
>>  target-ppc/machine.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  trace-events         |  3 +++
>>  5 files changed, 123 insertions(+)
>>
>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>> index 1e3cab3..7d08c9a 100644
>> --- a/hw/ppc/ppc.c
>> +++ b/hw/ppc/ppc.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/loader.h"
>>  #include "sysemu/kvm.h"
>>  #include "kvm_ppc.h"
>> +#include "trace.h"
>>  
>>  //#define PPC_DEBUG_IRQ
>>  #define PPC_DEBUG_TB
>> @@ -796,6 +797,54 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
>>      cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
>>  }
>>  
>> +/*
>> + * Calculate timebase on the destination side of migration
>> + *
>> + * We calculate new timebase offset as shown below:
>> + * 1) Gtb2 = Gtb1 + max(tod2 - tod1, 0)
>> + *    Gtb2 = tb2 + off2
>> + *    Gtb1 = tb1 + off1
>> + * 2) tb2 + off2 = tb1 + off1 + max(tod2 - tod1, 0)
>> + * 3) off2 = tb1 - tb2 + off1 + max(tod2 - tod1, 0)
>> + *
>> + * where:
>> + * Gtb2 - destination guest timebase
>> + * tb2 - destination host timebase
>> + * off2 - destination timebase offset
>> + * tod2 - destination time of the day
>> + * Gtb1 - source guest timebase
>> + * tb1 - source host timebase
>> + * off1 - source timebase offset
>> + * tod1 - source time of the day
>> + *
>> + * The result we want is in @off2
>> + *
>> + * Two conditions must be met for @off2:
>> + * 1) off2 must be multiple of 2^24 ticks as it will be set via TBU40 SPR
>> + * 2) Gtb2 >= Gtb1
>> + */
>> +void cpu_ppc_adjust_tb_offset(ppc_tb_t *tb_env)
>> +{
>> +    uint64_t tb2, tod2, off2;
>> +    int ratio = tb_env->tb_freq / 1000000;
>> +    struct timeval tv;
>> +
>> +    tb2 = cpu_get_real_ticks();
>> +    gettimeofday(&tv, NULL);
>> +    tod2 = tv.tv_sec * 1000000 + tv.tv_usec;
>> +
>> +    off2 = tb_env->timebase - tb2 + tb_env->tb_offset;
>> +    if (tod2 > tb_env->time_of_the_day) {
>> +        off2 += (tod2 - tb_env->time_of_the_day) * ratio;
>> +    }
>> +    off2 = ROUND_UP(off2, 1 << 24);
>> +
>> +    trace_ppc_tb_adjust(tb_env->tb_offset, off2,
>> +                        (int64_t)off2 - tb_env->tb_offset);
>> +
>> +    tb_env->tb_offset = off2;
>> +}
>> +
>>  /* Set up (once) timebase frequency (in Hz) */
>>  clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>>  {
>> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
>> index 132ab97..235871c 100644
>> --- a/include/hw/ppc/ppc.h
>> +++ b/include/hw/ppc/ppc.h
>> @@ -32,6 +32,9 @@ struct ppc_tb_t {
>>      uint64_t purr_start;
>>      void *opaque;
>>      uint32_t flags;
>> +    /* Cached values for live migration purposes */
>> +    uint64_t timebase;
>> +    uint64_t time_of_the_day;
>>  };
>>  
>>  /* PPC Timers flags */
>> @@ -46,6 +49,7 @@ struct ppc_tb_t {
>>                                                 */
>>  
>>  uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset);
>> +void cpu_ppc_adjust_tb_offset(ppc_tb_t *tb_env);
>>  clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq);
>>  /* Embedded PowerPC DCR management */
>>  typedef uint32_t (*dcr_read_cb)(void *opaque, int dcrn);
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 7af9e3d..93df955 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -35,6 +35,7 @@
>>  #include "hw/sysbus.h"
>>  #include "hw/ppc/spapr.h"
>>  #include "hw/ppc/spapr_vio.h"
>> +#include "hw/ppc/ppc.h"
>>  #include "sysemu/watchdog.h"
>>  
>>  //#define DEBUG_KVM
>> @@ -761,6 +762,22 @@ static int kvm_put_vpa(CPUState *cs)
>>  }
>>  #endif /* TARGET_PPC64 */
>>  
>> +static int kvm_access_one_reg(CPUState *cs, bool set, __u64 id, void *addr)
>> +{
>> +    struct kvm_one_reg reg = {
>> +        .id = id,
>> +        .addr = (uintptr_t)addr,
>> +    };
>> +    int ret = kvm_vcpu_ioctl(cs, set ? KVM_SET_ONE_REG : KVM_GET_ONE_REG, &reg);
>> +
>> +    if (ret) {
>> +        DPRINTF("Unable to %s time base offset to KVM: %s\n",
>> +                set ? "set" : "get", strerror(errno));
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  int kvm_arch_put_registers(CPUState *cs, int level)
>>  {
>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>> @@ -873,6 +890,9 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>                  DPRINTF("Warning: Unable to set VPA information to KVM\n");
>>              }
>>          }
>> +
>> +        kvm_access_one_reg(cs, 1, KVM_REG_PPC_TB_OFFSET,
>> +                           &env->tb_env->tb_offset);
>>  #endif /* TARGET_PPC64 */
>>      }
>>  
>> @@ -1082,6 +1102,9 @@ int kvm_arch_get_registers(CPUState *cs)
>>                  DPRINTF("Warning: Unable to get VPA information from KVM\n");
>>              }
>>          }
>> +
>> +        kvm_access_one_reg(cs, 0, KVM_REG_PPC_TB_OFFSET,
>> +                           &env->tb_env->tb_offset);
>>  #endif
>>      }
>>  
>> 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
>> @@ -1,5 +1,6 @@
>>  #include "hw/hw.h"
>>  #include "hw/boards.h"
>> +#include "hw/ppc/ppc.h"
>>  #include "sysemu/kvm.h"
>>  #include "helper_regs.h"
>>  
>> @@ -459,6 +460,45 @@ static const VMStateDescription vmstate_tlbmas = {
>>      }
>>  };
>>  
>> +static void timebase_pre_save(void *opaque)
>> +{
>> +    ppc_tb_t *tb_env = opaque;
>> +    struct timeval tv;
>> +
>> +    gettimeofday(&tv, NULL);
>> +    tb_env->time_of_the_day = tv.tv_sec * 1000000 + tv.tv_usec;
>> +    tb_env->timebase = cpu_get_real_ticks();
>> +}
>> +
>> +static int timebase_post_load(void *opaque, int version_id)
>> +{
>> +    ppc_tb_t *tb_env = opaque;
>> +
>> +    if (!tb_env) {
>> +        printf("NO TB!\n");
>> +        return -1;
>> +    }
>> +    cpu_ppc_adjust_tb_offset(tb_env);
>> +
>> +    return 0;
>> +}
>> +
>> +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.

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.
Or the source won't sent what the destination expects and we do not want
this destination guest to continue?
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?

Thanks!



alexey@ka1:~/pcipassthru/qemu$ git diff
Andreas Färber - Sept. 3, 2013, 9:22 a.m.
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


> alexey@ka1:~/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 Kardashevskiy - Sept. 4, 2013, 1:13 a.m.
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
> 
> 
>> alexey@ka1:~/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 []) {
>
Alexander Graf - Sept. 4, 2013, 1:27 a.m.
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

Patch

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