Re: [Qemu-devel] [PATCH v2 3/5] hpet 'driftfix': add fields to HPETTimer and VMStateDescription

[Anthony Liguori <aliguori@us.ibm.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