diff mbox series

[PULL,04/52] hpet: recover timer offset correctly

Message ID 1515756676-3860-5-git-send-email-pbonzini@redhat.com
State New
Headers show
Series [PULL,01/52] scsi-generic: Add share-rw option | expand

Commit Message

Paolo Bonzini Jan. 12, 2018, 11:30 a.m. UTC 
From: Maria Klimushenkova <maria.klimushenkova@ispras.ru>

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>
Message-Id: <20171220100205.16625.84632.stgit@pasha-VirtualBox>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/timer/hpet.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

Comments

Pavel Dovgalyuk Jan. 12, 2018, 11:47 a.m. UTC | #1 
This revision is incorrect.
Here is the latest one: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg01751.html

Pavel Dovgalyuk


> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Sent: Friday, January 12, 2018 2:30 PM
> To: qemu-devel@nongnu.org
> Cc: Maria Klimushenkova; Pavel Dovgalyuk
> Subject: [PULL 04/52] hpet: recover timer offset correctly
> 
> From: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> 
> 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>
> Message-Id: <20171220100205.16625.84632.stgit@pasha-VirtualBox>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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
>      }
>  };
> --
> 1.8.3.1
>
Paolo Bonzini Jan. 12, 2018, 12:08 p.m. UTC | #2 
On 12/01/2018 12:47, Pavel Dovgalyuk wrote:
> This revision is incorrect.
> Here is the latest one: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg01751.html

_That_ version was posted on 10 Jan.

_This_ version was posted on 11 Jan.  So this one is the latest one; the
latest one is wrong too.

I wasted at least one hour this morning due to patches that didn't
compile, please *please* be more careful, and send a follow-up for the
problem with this version.

Paolo
Pavel Dovgalyuk Jan. 12, 2018, 12:18 p.m. UTC | #3 
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 12/01/2018 12:47, Pavel Dovgalyuk wrote:
> > This revision is incorrect.
> > Here is the latest one: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg01751.html
> 
> _That_ version was posted on 10 Jan.
> 
> _This_ version was posted on 11 Jan.  So this one is the latest one; the
> latest one is wrong too.

That's strange.
I posted this version on 20 Dec: https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg04172.html

Pavel Dovgalyuk
Paolo Bonzini Jan. 12, 2018, 12:21 p.m. UTC | #4 
On 12/01/2018 13:18, Pavel Dovgalyuk wrote:
>>> This revision is incorrect.
>>> Here is the latest one: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg01751.html
>> _That_ version was posted on 10 Jan.
>>
>> _This_ version was posted on 11 Jan.  So this one is the latest one; the
>> latest one is wrong too.
> That's strange.
> I posted this version on 20 Dec: https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg04172.html

Hmm, my mistake then...

Paolo
diff mbox series

Patch

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