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

[Anthony Liguori]

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