From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60052) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VH1f3-0001NS-3z for qemu-devel@nongnu.org; Tue, 03 Sep 2013 21:13:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VH1ev-00028d-PB for qemu-devel@nongnu.org; Tue, 03 Sep 2013 21:13:33 -0400 Received: from mail-pa0-f49.google.com ([209.85.220.49]:46120) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VH1ev-00028X-GL for qemu-devel@nongnu.org; Tue, 03 Sep 2013 21:13:25 -0400 Received: by mail-pa0-f49.google.com with SMTP id ld10so7172894pab.22 for ; Tue, 03 Sep 2013 18:13:24 -0700 (PDT) Message-ID: <5226892D.408@ozlabs.ru> Date: Wed, 04 Sep 2013 11:13:17 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1378193502-4968-1-git-send-email-aik@ozlabs.ru> <5225A0F6.40506@suse.de> <5225A6C5.8030407@ozlabs.ru> <5225AA73.5090901@suse.de> In-Reply-To: <5225AA73.5090901@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH] spapr: support time base offset migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Juan Quintela , Alexander Graf , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paolo Bonzini , Paul Mackerras , David Gibson 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