Re: [Qemu-devel] [PATCH v2] hpet: recover timer offset correctly

[Juan Quintela <quintela@redhat.com>]

"Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
>> From: Juan Quintela [mailto:quintela@redhat.com]

>> >>      return 0;
>> >>  }
>> >>
>> >> @@ -252,7 +257,10 @@ static int hpet_post_load(void *opaque, int version_id)
>> >>      HPETState *s = opaque;
>> >>
>> >>      /* Recalculate the offset between the main counter and guest time */
>> >> -    s->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> >> +    if (!s->hpet_offset_loaded) {
>> >> +        s->hpet_offset = ticks_to_ns(s->hpet_counter)
>> >> +                        - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> >> +    }
>> 
>> So, at this point it is going to always be false.
>
> No, because post load (below) sets it to true.
>
>> >> +static int hpet_offset_post_load(void *opaque, int version_id)
>> >> +{
>> >> +    HPETState *s = opaque;
>> >> +
>> >> +    s->hpet_offset_loaded = true;
>> >> +    return 0;
>> >> +}
>> >> +
>> >>  static bool hpet_rtc_irq_level_needed(void *opaque)
>> >>  {
>> >>      HPETState *s = opaque;
>> >> @@ -285,6 +301,17 @@ static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>> >>      }
>> >>  };
>> >>
>> >> +static const VMStateDescription vmstate_hpet_offset = {
>> >> +    .name = "hpet/offset",
>> >> +    .version_id = 1,
>> >> +    .minimum_version_id = 1,
>> >> +    .post_load = hpet_offset_post_load,
>> 
>> You are missing here a .needed function.  Se how
>
> Because .needed is optional.

If you *don't* use a needed function then please just increase the
version.  You are just breaking compatibility anyways.  The whole point
of subsections is that they are optional.  If they are mandatory (this
case), then they bring no advantage at all.

>
>> - You want to transport hpet_offset just in the cases that hpet_is_enabled()?
>
> No, we want to preserve backwards compatibility.

So, let me understand this again.

old qemu -> new qemu

   what do you want to do here. i.e.
   new->hpet_counter = .... old->hpet_counter...
   new->hpet_offset = ... old->hpet_offset...

   You can use whatever you want.

new qemu -> old qemu
   right now, you break it always

   we *always* sent the subsection.  Is there any case where it makes
   sense to migrate to the old version.


>> I think that the following patch does what you want, no?  And it is a
>> bit simpler.
>
> It is simpler, but won't work for migrations from old version to the new one.
> hpet_counter becomes invalid in such case.

What value do you want hpet_offset to have when migrating from old
versions?

My version does:

new -> new (hpet_enabled)
   dst->hpet_counter = src->hpet_counter
   dst->hpet_offset = src->hpet_offset

new -> new ( ! hpet_enabled)
   dst->hpet_counter = src->hpet_counter
   dst->hpet_offset = <undefined really>

new -> old (hpet_enabled)
   breaks migration

new -> old ( ! hpet_enabled)
   dst->hpet_counter = src->hpet_counter
   dst->hpet_offset = <undefined really>

old -> old
   dst->hpet_counter= src->hpet_get_tick();
   dst->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);


Your version does:

new -> new (hpet_enabled)
   dst->hpet_counter = hpet_get_ticks(s) /* on source */
   dst->hpet_offset = src->hpet_offset

new -> new ( ! hpet_enabled)
   dst->hpet_counter = src->hpet_counter
   dst->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

new -> old
   breaks always migration independently of hpet_enabled

old -> old
   dst->hpet_counter= src->hpet_get_tick();
   dst->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

So, what are the values that we want to have here really?

What dave is asked for your previous version is that you disable the
section for old machine types.  Look at how to use DEFINE_PROP_* for
this use case. 

Later, Juan.

>> Head:     master Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request' into
>> staging
>> Merge:    qemu/master Merge remote-tracking branch 'remotes/elmarco/tags/dump-pull-request'
>> into staging
>> Tag:      v2.11.0 (454)
>> 
>> Unstaged changes (1)
>> modified   hw/timer/hpet.c
>> @@ -216,16 +216,6 @@ static void update_irq(struct HPETTimer *timer, int set)
>>      }
>>  }
>> 
>> -static int hpet_pre_save(void *opaque)
>> -{
>> -    HPETState *s = opaque;
>> -
>> -    /* save current counter value */
>> -    s->hpet_counter = hpet_get_ticks(s);
>> -
>> -    return 0;
>> -}
>> -
>>  static int hpet_pre_load(void *opaque)
>>  {
>>      HPETState *s = opaque;
>> @@ -251,9 +241,6 @@ static int hpet_post_load(void *opaque, int version_id)
>>  {
>>      HPETState *s = opaque;
>> 
>> -    /* Recalculate the offset between the main counter and guest time */
>> -    s->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> -
>>      /* Push number of timers into capability returned via HPET_ID */
>>      s->capability &= ~HPET_ID_NUM_TIM_MASK;
>>      s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
>> @@ -285,6 +272,24 @@ static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>>      }
>>  };
>> 
>> +static bool hpet_offset_needed(void *opaque)
>> +{
>> +    HPETState *s = opaque;
>> +
>> +    return hpet_enabled(s);
>> +}
>> +
>> +static const VMStateDescription vmstate_hpet_offset = {
>> +    .name = "hpet/offset",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = hpet_offset_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(hpet_offset, HPETState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>  static const VMStateDescription vmstate_hpet_timer = {
>>      .name = "hpet_timer",
>>      .version_id = 1,
>> @@ -320,6 +325,7 @@ static const VMStateDescription vmstate_hpet = {
>>      },
>>      .subsections = (const VMStateDescription*[]) {
>>          &vmstate_hpet_rtc_irq_level,
>> +        &vmstate_hpet_offset,
>>          NULL
>>      }
>>  };
>> 
>
>
> Pavel Dovgalyuk