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

["Pavel Dovgalyuk" <dovgaluk@ispras.ru>]

> From: Juan Quintela [mailto:quintela@redhat.com]
> >> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> >> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> >> ---
> >>  hw/timer/hpet.c |   32 ++++++++++++++++++++++++++++++--
> >>  1 file changed, 30 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> >> index 577371b..4904a60 100644
> >> --- a/hw/timer/hpet.c
> >> +++ b/hw/timer/hpet.c
> >> @@ -70,6 +70,7 @@ typedef struct HPETState {
> >>
> >>      MemoryRegion iomem;
> >>      uint64_t hpet_offset;
> >> +    bool hpet_offset_loaded;
> >>      qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
> >>      uint32_t flags;
> >>      uint8_t rtc_irq_level;
> >> @@ -221,7 +222,9 @@ static int hpet_pre_save(void *opaque)
> >>      HPETState *s = opaque;
> >>
> >>      /* save current counter value */
> >> -    s->hpet_counter = hpet_get_ticks(s);
> >> +    if (hpet_enabled(s)) {
> >> +        s->hpet_counter = hpet_get_ticks(s);
> 
> Why do we want to save it only when hpet is enabled?  We used to save it always.

Because it may be read by the guest.
Therefore hpet_counter should not be affected by the events not caused by the guest.

> 
> >> +    }
> >>
> >>      return 0;
> >>  }
> >> @@ -232,6 +235,8 @@ static int hpet_pre_load(void *opaque)
> >>
> >>      /* version 1 only supports 3, later versions will load the actual value */
> >>      s->num_timers = HPET_MIN_TIMERS;
> >> +    /* for checking whether the hpet_offset section is loaded */
> >> +    s->hpet_offset_loaded = false;
> 
> This is made false everytime that we start incoming migration.

Right.

> 
> >>      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.

> - You want to transport hpet_offset just in the cases that hpet_is_enabled()?

No, we want to preserve backwards compatibility.

> 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.

> 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