* [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly
@ 2017-12-20 7:58 Pavel Dovgalyuk
2017-12-20 9:03 ` Paolo Bonzini
2017-12-20 9:17 ` Dr. David Alan Gilbert
0 siblings, 2 replies; 6+ messages in thread
From: Pavel Dovgalyuk @ 2017-12-20 7:58 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, mst, dgilbert, maria.klimushenkova, dovgaluk,
pavel.dovgaluk, pbonzini
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 | 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,
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly
2017-12-20 7:58 [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly Pavel Dovgalyuk
@ 2017-12-20 9:03 ` Paolo Bonzini
2017-12-20 9:17 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2017-12-20 9:03 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel
Cc: quintela, mst, dgilbert, maria.klimushenkova, dovgaluk
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 <maria.klimushenkova@ispras.ru>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly
2017-12-20 7:58 [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly Pavel Dovgalyuk
2017-12-20 9:03 ` Paolo Bonzini
@ 2017-12-20 9:17 ` Dr. David Alan Gilbert
2017-12-20 9:21 ` Pavel Dovgalyuk
1 sibling, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2017-12-20 9:17 UTC (permalink / raw)
To: Pavel Dovgalyuk
Cc: qemu-devel, quintela, mst, maria.klimushenkova, dovgaluk, pbonzini
* Pavel Dovgalyuk (Pavel.Dovgaluk@ispras.ru) 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.
Please put the extra data in a subsection and tie the subsection to a
property enabled by the machine type. That avoids breaking backwards
migration compatibility.
Dave
> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>
> ---
> 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,
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly
2017-12-20 9:17 ` Dr. David Alan Gilbert
@ 2017-12-20 9:21 ` Pavel Dovgalyuk
2017-12-20 9:40 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Dovgalyuk @ 2017-12-20 9:21 UTC (permalink / raw)
To: 'Dr. David Alan Gilbert', 'Pavel Dovgalyuk'
Cc: qemu-devel, quintela, mst, maria.klimushenkova, pbonzini
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> * Pavel Dovgalyuk (Pavel.Dovgaluk@ispras.ru) 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.
>
> Please put the extra data in a subsection and tie the subsection to a
> property enabled by the machine type. That avoids breaking backwards
> migration compatibility.
I thought about it and haven't found how to preserve the backward migration compatibility
without breaking everything else.
Loading old (or compatible) snapshot without changing the version will corrupt the offset in
post_load.
Pavel Dovgalyuk
>
> > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> >
> > ---
> > 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,
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly
2017-12-20 9:21 ` Pavel Dovgalyuk
@ 2017-12-20 9:40 ` Dr. David Alan Gilbert
2017-12-20 10:02 ` Pavel Dovgalyuk
0 siblings, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2017-12-20 9:40 UTC (permalink / raw)
To: Pavel Dovgalyuk
Cc: 'Pavel Dovgalyuk',
qemu-devel, quintela, mst, maria.klimushenkova, pbonzini
* Pavel Dovgalyuk (dovgaluk@ispras.ru) wrote:
> > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > * Pavel Dovgalyuk (Pavel.Dovgaluk@ispras.ru) 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.
> >
> > Please put the extra data in a subsection and tie the subsection to a
> > property enabled by the machine type. That avoids breaking backwards
> > migration compatibility.
>
> I thought about it and haven't found how to preserve the backward migration compatibility
> without breaking everything else.
> Loading old (or compatible) snapshot without changing the version will corrupt the offset in
> post_load.
There are lots of ways; for example you could initialise hpet_offset to
a dummy value in a pre-load and spot that in the post-load instead of
the version_id, or set a flag in a post-load on the subsection.
Dave
> Pavel Dovgalyuk
> >
> > > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > >
> > > ---
> > > 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,
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly
2017-12-20 9:40 ` Dr. David Alan Gilbert
@ 2017-12-20 10:02 ` Pavel Dovgalyuk
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Dovgalyuk @ 2017-12-20 10:02 UTC (permalink / raw)
To: 'Dr. David Alan Gilbert'
Cc: 'Pavel Dovgalyuk',
qemu-devel, quintela, mst, maria.klimushenkova, pbonzini
> * Pavel Dovgalyuk (dovgaluk@ispras.ru) wrote:
> > > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > > * Pavel Dovgalyuk (Pavel.Dovgaluk@ispras.ru) 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.
> > >
> > > Please put the extra data in a subsection and tie the subsection to a
> > > property enabled by the machine type. That avoids breaking backwards
> > > migration compatibility.
> >
> > I thought about it and haven't found how to preserve the backward migration compatibility
> > without breaking everything else.
> > Loading old (or compatible) snapshot without changing the version will corrupt the offset in
> > post_load.
>
> There are lots of ways; for example you could initialise hpet_offset to
> a dummy value in a pre-load and spot that in the post-load instead of
> the version_id, or set a flag in a post-load on the subsection.
Thanks. Please check the second version.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-20 10:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-20 7:58 [Qemu-devel] [ PATCH ] hpet: recover timer offset correctly Pavel Dovgalyuk
2017-12-20 9:03 ` Paolo Bonzini
2017-12-20 9:17 ` Dr. David Alan Gilbert
2017-12-20 9:21 ` Pavel Dovgalyuk
2017-12-20 9:40 ` Dr. David Alan Gilbert
2017-12-20 10:02 ` Pavel Dovgalyuk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.