"Pavel Dovgalyuk" <dovga...@ispras.ru> wrote:
> Ping?
>
> Pavel Dovgalyuk
>
>> -----Original Message-----
>> From: Pavel Dovgalyuk [mailto:pavel.dovga...@ispras.ru]
>> Sent: Wednesday, December 20, 2017 1:02 PM
>> To: qemu-devel@nongnu.org
>> Cc: quint...@redhat.com; m...@redhat.com; dgilb...@redhat.com;
>> maria.klimushenk...@ispras.ru;
>> dovga...@ispras.ru; pavel.dovga...@ispras.ru; pbonz...@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.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.
>> + }
>>
>> 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
}
};
