[v2] hpet: recover timer offset correctly

Commit Message

Pavel Dovgalyuk Jan. 10, 2018, 9:33 a.m. UTC

> From: Juan Quintela [mailto:[e-mail]]
> "Pavel Dovgalyuk" <[e-mail]> wrote:
> >> From: Juan Quintela [mailto:[e-mail]]
> 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?


Pavel Dovgalyuk

Comments

Juan Quintela Jan. 10, 2018, 9:50 a.m. UTC | #1

"Pavel Dovgalyuk" <[e-mail]> wrote:
>> From: Juan Quintela [mailto:[e-mail]]
>> "Pavel Dovgalyuk" <[e-mail]> wrote:
>> >> From: Juan Quintela [mailto:[e-mail]]
>> 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 <[e-mail]>

Pavel Dovgalyuk Jan. 10, 2018, 10:10 a.m. UTC | #2

> -----Original Message-----
> From: Juan Quintela [mailto:[e-mail]]
> Sent: Wednesday, January 10, 2018 12:51 PM
> To: Pavel Dovgalyuk
> Cc: 'Pavel Dovgalyuk'; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2] hpet: recover timer offset correctly
> 
> "Pavel Dovgalyuk" <[e-mail]> wrote:
> >> From: Juan Quintela [mailto:[e-mail]]
> >> "Pavel Dovgalyuk" <[e-mail]> wrote:
> >> >> From: Juan Quintela [mailto:[e-mail]]
> >> 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 <[e-mail]>

Thanks.

Pavel Dovgalyuk

Patch

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 \