From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48782 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q9GtN-0005Eu-Nl for qemu-devel@nongnu.org; Mon, 11 Apr 2011 09:11:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q9GtL-0005Bl-8u for qemu-devel@nongnu.org; Mon, 11 Apr 2011 09:10:56 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:51218) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9GtL-0005Bd-5t for qemu-devel@nongnu.org; Mon, 11 Apr 2011 09:10:55 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e8.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p3BCjeZ8019643 for ; Mon, 11 Apr 2011 08:45:42 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 558C238C8045 for ; Mon, 11 Apr 2011 09:10:45 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p3BDAs71403042 for ; Mon, 11 Apr 2011 09:10:54 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p3BDAsij000966 for ; Mon, 11 Apr 2011 10:10:54 -0300 Message-ID: <4DA2FDDC.2020409@us.ibm.com> Date: Mon, 11 Apr 2011 08:10:52 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v2 3/5] hpet 'driftfix': add fields to HPETTimer and VMStateDescription References: <1473344612.11808.1302510244411.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com> In-Reply-To: <1473344612.11808.1302510244411.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ulrich Obergfell Cc: jan kiszka , avi@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, gcosta@redhat.com On 04/11/2011 03:24 AM, Ulrich Obergfell wrote: >>> typedef struct HPETState { >>> @@ -248,7 +253,7 @@ static int hpet_post_load(void *opaque, int >>> version_id) >>> >>> static const VMStateDescription vmstate_hpet_timer = { >>> .name = "hpet_timer", >>> - .version_id = 1, >>> + .version_id = 3, >> Why jump from 1 to 3? >> >>> .minimum_version_id = 1, >>> .minimum_version_id_old = 1, >>> .fields = (VMStateField []) { >>> @@ -258,6 +263,11 @@ static const VMStateDescription >>> vmstate_hpet_timer = { >>> VMSTATE_UINT64(fsb, HPETTimer), >>> VMSTATE_UINT64(period, HPETTimer), >>> VMSTATE_UINT8(wrap_flag, HPETTimer), >>> + VMSTATE_UINT64_V(saved_period, HPETTimer, 3), >>> + VMSTATE_UINT64_V(ticks_not_accounted, HPETTimer, 3), >>> + VMSTATE_UINT32_V(irqs_to_inject, HPETTimer, 3), >>> + VMSTATE_UINT32_V(irq_rate, HPETTimer, 3), >>> + VMSTATE_UINT32_V(divisor, HPETTimer, 3), > > Anthony, > > I incremented the version ID of 'vmstate_hpet' from 2 to 3 to make sure > that migrations from a QEMU process that is capable of 'driftfix' to a > QEMU process that is _not_ capable of 'driftfix' will fail. I assigned > version ID 3 to 'vmstate_hpet_timer' and to the new fields in there too > to indicate that adding those fields was the reason why the version ID > of 'vmstate_hpet' was incremented to 3. > > As far as the flow of execution in vmstate_load_state() is concerned, I > think it does not matter whether the version ID of 'vmstate_hpet_timer' > and the new fields in there is 2 or 3 (as long as they are consistent). > When the 'while(field->name)' loop in vmstate_load_state() gets to the > following field in 'vmstate_hpet' ... > > VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0, > vmstate_hpet_timer, HPETTimer), > > ... it calls itself recursively ... > > if (field->flags& VMS_STRUCT) { > ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id); > > 'field->vmsd->version_id' is the version ID of 'vmstate_hpet_timer' [1]. > Hence 'vmstate_hpet_timer.version_id' is being checked against itself ... > > if (version_id> vmsd->version_id) { > return -EINVAL; > } > > ... and the version IDs of the new fields are also being checked against > 'vmstate_hpet_timer.version_id' ... > > if ((field->field_exists&& > field->field_exists(opaque, version_id)) || > (!field->field_exists&& > field->version_id<= version_id)) { > > If you want me to change the version ID of 'vmstate_hpet_timer' and the > new fields in there from 3 to 2, I can do that. It avoids surprises so I think it's a reasonable thing to do. But yes, your analysis is correct. Regards, Anthony Liguori > > Regards, > > Uli > > > [1] Ref.: commit fa3aad24d94a6cf894db52d83f72a399324a17bb