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

[Paolo Bonzini]

On 20/12/2017 08:58, Pavel Dovgalyuk wrote:
> HPET saves its state by calculating the current time and recovers timer
> offset using this calculated value. But these calculations include
> divisions and multiplications. Therefore the timer state cannot be recovered
> precise enough.
> This patch introduces saving of the original value of the offset to
> preserve the determinism of the timer.
> 
> Signed-off-by: Maria Klimushenkova <address@hidden>
> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> 
> ---
>  hw/timer/hpet.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 577371b..4cf6a5b 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -221,7 +221,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);
> +    }
>  
>      return 0;
>  }
> @@ -252,7 +254,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 (version_id <= 2) {
> +        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;
> @@ -303,7 +308,7 @@ static const VMStateDescription vmstate_hpet_timer = {
>  
>  static const VMStateDescription vmstate_hpet = {
>      .name = "hpet",
> -    .version_id = 2,
> +    .version_id = 3,
>      .minimum_version_id = 1,
>      .pre_save = hpet_pre_save,
>      .pre_load = hpet_pre_load,
> @@ -312,6 +317,7 @@ static const VMStateDescription vmstate_hpet = {
>          VMSTATE_UINT64(config, HPETState),
>          VMSTATE_UINT64(isr, HPETState),
>          VMSTATE_UINT64(hpet_counter, HPETState),
> +        VMSTATE_UINT64_V(hpet_offset, HPETState, 3),
>          VMSTATE_UINT8_V(num_timers, HPETState, 2),
>          VMSTATE_VALIDATE("num_timers in range", hpet_validate_num_timers),
>          VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
> 

Queued, thanks.

However, note that Maria should have been the author in the patch.  I've
fixed that.

Paolo