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