> From: Juan Quintela [mailto:quint...@redhat.com]
> >> Signed-off-by: Maria Klimushenkova <maria.klimushenk...@ispras.ru>
> >> Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@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
