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

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

[Pavel Dovgalyuk]

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 <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);
+    }
 
     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;
     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);
+    }
 
     /* Push number of timers into capability returned via HPET_ID */
     s->capability &= ~HPET_ID_NUM_TIM_MASK;
@@ -267,6 +275,14 @@ static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+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,
+    .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 +347,7 @@ static const VMStateDescription vmstate_hpet = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_hpet_rtc_irq_level,
+        &vmstate_hpet_offset,
         NULL
     }
 };

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

[Pavel Dovgalyuk]

Ping?

Pavel Dovgalyuk

> -----Original Message-----
> From: Pavel Dovgalyuk [mailto:Pavel.Dovgaluk@ispras.ru]
> Sent: Wednesday, December 20, 2017 1:02 PM
> To: qemu-devel@nongnu.org
> Cc: quintela@redhat.com; mst@redhat.com; dgilbert@redhat.com; maria.klimushenkova@ispras.ru;
> dovgaluk@ispras.ru; pavel.dovgaluk@ispras.ru; pbonzini@redhat.com
> Subject: [PATCH v2] hpet: recover timer offset correctly
> 
> 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 <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);
> +    }
> 
>      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;
>      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);
> +    }
> 
>      /* Push number of timers into capability returned via HPET_ID */
>      s->capability &= ~HPET_ID_NUM_TIM_MASK;
> @@ -267,6 +275,14 @@ static int hpet_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> +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,
> +    .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 +347,7 @@ static const VMStateDescription vmstate_hpet = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_hpet_rtc_irq_level,
> +        &vmstate_hpet_offset,
>          NULL
>      }
>  };

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

[Juan Quintela]

"Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
> Ping?
>
> Pavel Dovgalyuk
>
>> -----Original Message-----
>> From: Pavel Dovgalyuk [mailto:Pavel.Dovgaluk@ispras.ru]
>> Sent: Wednesday, December 20, 2017 1:02 PM
>> To: qemu-devel@nongnu.org
>> Cc: quintela@redhat.com; mst@redhat.com; dgilbert@redhat.com; maria.klimushenkova@ispras.ru;
>> dovgaluk@ispras.ru; pavel.dovgaluk@ispras.ru; pbonzini@redhat.com
>> Subject: [PATCH v2] hpet: recover timer offset correctly
>> 
>> 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.

Hi

How have you tested this?


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

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

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

>> 
>>      /* Push number of timers into capability returned via HPET_ID */
>>      s->capability &= ~HPET_ID_NUM_TIM_MASK;
>> @@ -267,6 +275,14 @@ static int hpet_post_load(void *opaque, int version_id)
>>      return 0;
>>  }
>> 
>> +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
vmstate_hpet_rtc_irq_level_needed().
I am commeting from migration/vmstate point of view.  I don't understand
hpet well enough to comment about that.

Just to be sure that I am understanding what you want/need.

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

So your _needed() function needs to do something like
   return hpet_enabled();


So, we have two cases:
- hpet is enabled -> we need your changes, but then we can just have
hpet_counter directly



>> +    .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 +347,7 @@ static const VMStateDescription vmstate_hpet = {
>>      },
>>      .subsections = (const VMStateDescription*[]) {
>>          &vmstate_hpet_rtc_irq_level,
>> +        &vmstate_hpet_offset,
>>          NULL
>>      }
>>  };

I think that the following patch does what you want, no?  And it is a
bit simpler.

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
     }
 };

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

[Pavel Dovgalyuk]

> 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

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

[Juan Quintela]

"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

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

[Pavel Dovgalyuk]

> From: Juan Quintela [mailto:quintela@redhat.com]
> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
> >> From: Juan Quintela [mailto:quintela@redhat.com]
> 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.

Thanks, I thought that the sections are skipped automatically when there is no code for loading
them.

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

How do you like this one?

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 577371b..539586f 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_saved;
     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);
+    }
 
     return 0;
 }
@@ -252,7 +255,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_CLOC
+    if (!s->hpet_offset_saved) {
+        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;
@@ -267,6 +273,13 @@ static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool hpet_offset_needed(void *opaque)
+{
+    HPETState *s = opaque;
+
+    return s->hpet_offset_saved;
+}
+
 static bool hpet_rtc_irq_level_needed(void *opaque)
 {
     HPETState *s = opaque;
@@ -285,6 +298,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,
+    .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 +344,7 @@ static const VMStateDescription vmstate_hpet = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_hpet_rtc_irq_level,
+        &vmstate_hpet_offset,
         NULL
     }
 };
@@ -762,6 +787,7 @@ static Property hpet_device_properties[] = {
     DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
     DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
     DEFINE_PROP_UINT32(HPET_INTCAP, HPETState, intcap, 0),
+    DEFINE_PROP_BOOL("hpet-offset-saved", HPETState, hpet_offset_saved, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 263de97..8897302 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -13,6 +13,10 @@
         .driver   = "virtio-tablet-device",\
         .property = "wheel-axis",\
         .value    = "false",\
+    },{\
+        .driver   = "hpet",\
+        .property = "hpet-offset-saved",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_9 \

Pavel Dovgalyuk

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

[Juan Quintela]

"Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
>> From: Juan Quintela [mailto:quintela@redhat.com]
>> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
>> >> From: Juan Quintela [mailto:quintela@redhat.com]
>> 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.
>
> Thanks, I thought that the sections are skipped automatically when
> there is no code for loading
> them.
>
>> 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.
>
> How do you like this one?

Much better, thanks.

> +static bool hpet_offset_needed(void *opaque)
> +{
> +    HPETState *s = opaque;
> +
> +    return s->hpet_offset_saved;
> +}
> +

If this is only one optimization, this test is ok.  If it makes things
go worse, you can add something there like && hpet_enabled() or
whatever.  Remember that I don't understand HPET.


> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 263de97..8897302 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -13,6 +13,10 @@
>          .driver   = "virtio-tablet-device",\
>          .property = "wheel-axis",\
>          .value    = "false",\
> +    },{\
> +        .driver   = "hpet",\
> +        .property = "hpet-offset-saved",\
> +        .value    = "off",\
>      },
>  
>  #define HW_COMPAT_2_9 \

This should be on 2_11 not 2_10 O:-)

But for the vmstate bits:

Reviewed-by: Juan Quintela <quintela@redhat.com>

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

[Pavel Dovgalyuk]

> -----Original Message-----
> From: Juan Quintela [mailto:quintela@redhat.com]
> Sent: Wednesday, January 10, 2018 12:51 PM
> To: Pavel Dovgalyuk
> Cc: 'Pavel Dovgalyuk'; qemu-devel@nongnu.org; mst@redhat.com; dgilbert@redhat.com;
> maria.klimushenkova@ispras.ru; pbonzini@redhat.com
> Subject: Re: [PATCH v2] hpet: recover timer offset correctly
> 
> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
> >> From: Juan Quintela [mailto:quintela@redhat.com]
> >> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> wrote:
> >> >> From: Juan Quintela [mailto:quintela@redhat.com]
> >> 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.
> >
> > Thanks, I thought that the sections are skipped automatically when
> > there is no code for loading
> > them.
> >
> >> 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.
> >
> > How do you like this one?
> 
> Much better, thanks.
> 
> > +static bool hpet_offset_needed(void *opaque)
> > +{
> > +    HPETState *s = opaque;
> > +
> > +    return s->hpet_offset_saved;
> > +}
> > +
> 
> If this is only one optimization, this test is ok.  If it makes things
> go worse, you can add something there like && hpet_enabled() or
> whatever.  Remember that I don't understand HPET.

Right. Please check the new version of the patch.

> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 263de97..8897302 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -13,6 +13,10 @@
> >          .driver   = "virtio-tablet-device",\
> >          .property = "wheel-axis",\
> >          .value    = "false",\
> > +    },{\
> > +        .driver   = "hpet",\
> > +        .property = "hpet-offset-saved",\
> > +        .value    = "off",\
> >      },
> >
> >  #define HW_COMPAT_2_9 \
> 
> This should be on 2_11 not 2_10 O:-)
> 
> But for the vmstate bits:
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks.

Pavel Dovgalyuk