* [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-05-04 11:59 guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 1/5] mc146818rtc: update periodic timer only if it is needed guangrong.xiao
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: guangrong.xiao @ 2017-05-04 11:59 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Changelog in v2:
Thanks to Paolo's review, the changes in this version are:
1) merge patch 2, 3, 4 together
2) use a nice way ((value ^ new_value) & BIT_MASK) to check if updating
for periodic timer is needed
3) use a smarter way to calculate coalesced irqs and lost_clock
4) make sure only x86 can enable LOST_TICK_POLICY_SLEW
5) make the code depends on LOST_TICK_POLICY_SLEW rather than
TARGET_I386
We noticed that the clock on some windows VMs, e.g, Window7 and window8
is really faster and the issue can be easily reproduced by staring the
VM with '-rtc base=localtime,clock=vm,driftfix=slew -no-hpet' and
running attached code in the guest
The root cause is that the clock will be lost if the periodic period is
changed as currently code counts the next periodic time like this:
next_irq_clock = (cur_clock & ~(period - 1)) + period;
consider the case if cur_clock = 0x11FF and period = 0x100, then the
next_irq_clock is 0x1200, however, there is only 1 clock left to trigger
the next irq. Unfortunately, Windows guests (at least Windows7) change
the period very frequently if it runs the attached code, so that the
lost clock is accumulated, the wall-time become faster and faster
The main idea to fix the issue is we use a accurate clock period to
calculate the next irq:
next_irq_clock = cur_clock + period;
After that, it is really convenient to compensate clock if it is needed
The code running in windows VM is attached:
// TimeInternalTest.cpp : Defines the entry point for the console application.
//
#include "stdafx.h"
#pragma comment(lib, "winmm")
#include <stdio.h>
#include <windows.h>
#define SWITCH_PEROID 13
int _tmain(int argc, _TCHAR* argv[])
{
if (argc != 2)
{
printf("parameter error!\n");
printf("USAGE: *.exe time(ms)\n");
printf("example: *.exe 40\n");
return 0;
}
else
{
DWORD internal = atoi((char *)argv[1]);
DWORD count = 0;
while (1)
{
count++;
timeBeginPeriod(1);
DWORD start = timeGetTime();
Sleep(internal);
timeEndPeriod(1);
if ((count % SWITCH_PEROID) == 0) {
Sleep(1);
}
}
}
return 0;
}
Tai Yunfang (1):
mc146818rtc: precisely count the clock for periodic timer
Xiao Guangrong (4):
mc146818rtc: update periodic timer only if it is needed
mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on
TARGET_I386
mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
mc146818rtc: embrace all x86 specific code
hw/timer/mc146818rtc.c | 204 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 144 insertions(+), 60 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 1/5] mc146818rtc: update periodic timer only if it is needed
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
@ 2017-05-04 11:59 ` guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer guangrong.xiao
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: guangrong.xiao @ 2017-05-04 11:59 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Currently, the timer is updated whenever RegA or RegB is written
even if the periodic timer related configuration is not changed
This patch optimizes it slightly to make the update happen only
if its period or enable-status is changed, also later patches are
depend on this optimization
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
hw/timer/mc146818rtc.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 4165450..5cccb2a 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -391,6 +391,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
RTCState *s = opaque;
+ bool update_periodic_timer;
if ((addr & 1) == 0) {
s->cmos_index = data & 0x7f;
@@ -423,6 +424,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
}
break;
case RTC_REG_A:
+ update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & 0x0f;
+
if ((data & 0x60) == 0x60) {
if (rtc_running(s)) {
rtc_update_time(s);
@@ -445,10 +448,17 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
/* UIP bit is read only */
s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) |
(s->cmos_data[RTC_REG_A] & REG_A_UIP);
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+
+ if (update_periodic_timer) {
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ }
+
check_update_timer(s);
break;
case RTC_REG_B:
+ update_periodic_timer = (s->cmos_data[RTC_REG_B] ^ data)
+ & REG_B_PIE;
+
if (data & REG_B_SET) {
/* update cmos to when the rtc was stopping */
if (rtc_running(s)) {
@@ -475,7 +485,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
qemu_irq_lower(s->irq);
}
s->cmos_data[RTC_REG_B] = data;
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+
+ if (update_periodic_timer) {
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ }
+
check_update_timer(s);
break;
case RTC_REG_C:
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 1/5] mc146818rtc: update periodic timer only if it is needed guangrong.xiao
@ 2017-05-04 11:59 ` guangrong.xiao
2017-05-04 12:31 ` Paolo Bonzini
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 3/5] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386 guangrong.xiao
` (3 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: guangrong.xiao @ 2017-05-04 11:59 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Tai Yunfang <yunfangtai@tencent.com>
There are two issues in current code:
1) If the period is changed by re-configuring RegA, the coalesced
irq will be scaled to reflect the new period, however, it
calculates the new interrupt number like this:
s->irq_coalesced = (s->irq_coalesced * s->period) / period;
There are some clocks will be lost if they are not enough to
be squeezed to a single new period that will cause the VM clock
slower
In order to fix the issue, we calculate the interrupt window
based on the precise clock rather than period, then the clocks
lost during period is scaled can be compensated properly
2) If periodic_timer_update() is called due to RegA reconfiguration,
i.e, the period is updated, current time is not the start point
for the next periodic timer, instead, which should start from the
last interrupt, otherwise, the clock in VM will become slow
This patch takes the clocks from last interrupt to current clock
into account and compensates the clocks for the next interrupt,
especially，if a complete interrupt was lost in this window, the
time can be caught up by LOST_TICK_POLICY_SLEW
[ Xiao: redesign the algorithm based on Yunfang's original work. ]
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
Signed-off-by: Tai Yunfang <yunfangtai@tencent.com>
---
hw/timer/mc146818rtc.c | 116 ++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 96 insertions(+), 20 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 5cccb2a..14bde1a 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -146,31 +146,104 @@ static void rtc_coalesced_timer(void *opaque)
}
#endif
-/* handle periodic timer */
-static void periodic_timer_update(RTCState *s, int64_t current_time)
+static int period_code_to_clock(int period_code)
+{
+ /* periodic timer is disabled. */
+ if (!period_code) {
+ return 0;
+ }
+
+ if (period_code <= 2) {
+ period_code += 7;
+ }
+
+ /* period in 32 Khz cycles */
+ return 1 << (period_code - 1);
+}
+
+/*
+ * handle periodic timer. @old_period indicates the periodic timer update
+ * is just due to period adjustment.
+ */
+static void
+periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
{
int period_code, period;
- int64_t cur_clock, next_irq_clock;
+ int64_t cur_clock, next_irq_clock, lost_clock = 0;
period_code = s->cmos_data[RTC_REG_A] & 0x0f;
if (period_code != 0
&& (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
- if (period_code <= 2)
- period_code += 7;
- /* period in 32 Khz cycles */
- period = 1 << (period_code - 1);
-#ifdef TARGET_I386
- if (period != s->period) {
- s->irq_coalesced = (s->irq_coalesced * s->period) / period;
- DPRINTF_C("cmos: coalesced irqs scaled to %d\n", s->irq_coalesced);
- }
- s->period = period;
-#endif
+ period = period_code_to_clock(period_code);
+
/* compute 32 khz clock */
cur_clock =
muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
- next_irq_clock = (cur_clock & ~(period - 1)) + period;
+ /*
+ * if the periodic timer's update is due to period re-configuration,
+ * we should count the clock since last interrupt.
+ */
+ if (old_period) {
+ int64_t last_periodic_clock;
+
+ last_periodic_clock = muldiv64(s->next_periodic_time,
+ RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
+ /*
+ * if the next interrupt has not happened yet, we recall the last
+ * interrupt based on the original period.
+ */
+ if (last_periodic_clock > cur_clock) {
+ last_periodic_clock -= period_code_to_clock(old_period);
+
+ /* the last interrupt must have happened. */
+ assert(cur_clock >= last_periodic_clock);
+ }
+
+ /* calculate the clock since last interrupt. */
+ lost_clock = cur_clock - last_periodic_clock;
+ }
+
+#ifdef TARGET_I386
+ /*
+ * recalculate the coalesced irqs for two reasons:
+ * a) the lost_clock is more that a period, i,e. the timer
+ * interrupt has been lost, we should catch up the time.
+ *
+ * b) the period may be reconfigured, under this case, when
+ * switching from a shorter to a longer period, scale down
+ * the missing ticks since we expect the OS handler to
+ * treat the delayed ticks as longer. Any leftovers are
+ * put back into lost_clock.
+ * When switching to a shorter period, scale up the missing
+ * ticks since we expect the OS handler to treat the delayed
+ * ticks as shorter.
+ */
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+ uint32_t cur_irq_coalesced = s->irq_coalesced;
+ uint32_t cur_period = s->period;
+
+ lost_clock += cur_irq_coalesced * cur_period;
+ s->irq_coalesced = lost_clock / period;
+ lost_clock %= period;
+ s->period = period;
+ if ((cur_irq_coalesced != s->irq_coalesced) ||
+ (cur_period |= s->period)) {
+ DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
+ "period scaled from %d to %d\n", cur_irq_coalesced,
+ s->irq_coalesced, cur_period, s->period);
+ rtc_coalesced_timer_update(s);
+ }
+ }
+#endif
+ /*
+ * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
+ * is not used, we should make the time progress anyway.
+ */
+ lost_clock = MIN(lost_clock, period);
+ assert(lost_clock >= 0);
+
+ next_irq_clock = cur_clock + period - lost_clock;
s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND,
RTC_CLOCK_RATE) + 1;
timer_mod(s->periodic_timer, s->next_periodic_time);
@@ -186,7 +259,7 @@ static void rtc_periodic_timer(void *opaque)
{
RTCState *s = opaque;
- periodic_timer_update(s, s->next_periodic_time);
+ periodic_timer_update(s, s->next_periodic_time, 0);
s->cmos_data[RTC_REG_C] |= REG_C_PF;
if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
@@ -391,6 +464,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
RTCState *s = opaque;
+ int cur_period;
bool update_periodic_timer;
if ((addr & 1) == 0) {
@@ -424,6 +498,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
}
break;
case RTC_REG_A:
+ cur_period = s->cmos_data[RTC_REG_A] & 0xf;
update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & 0x0f;
if ((data & 0x60) == 0x60) {
@@ -450,7 +525,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
(s->cmos_data[RTC_REG_A] & REG_A_UIP);
if (update_periodic_timer) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
+ cur_period);
}
check_update_timer(s);
@@ -487,7 +563,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
s->cmos_data[RTC_REG_B] = data;
if (update_periodic_timer) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
}
check_update_timer(s);
@@ -757,7 +833,7 @@ static int rtc_post_load(void *opaque, int version_id)
uint64_t now = qemu_clock_get_ns(rtc_clock);
if (now < s->next_periodic_time ||
now > (s->next_periodic_time + get_max_clock_jump())) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
}
}
@@ -822,7 +898,7 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
int64_t now = *(int64_t *)data;
rtc_set_date_from_host(ISA_DEVICE(s));
- periodic_timer_update(s, now);
+ periodic_timer_update(s, now, 0);
check_update_timer(s);
#ifdef TARGET_I386
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 1/5] mc146818rtc: update periodic timer only if it is needed guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer guangrong.xiao
@ 2017-05-04 11:59 ` guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 4/5] mc146818rtc: drop unnecessary '#ifdef TARGET_I386' guangrong.xiao
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: guangrong.xiao @ 2017-05-04 11:59 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Any tick policy specified on other platforms rather on TARGET_I386
will fall back to LOST_TICK_POLICY_DISCARD silently, this patch makes
sure only TARGET_I386 can enable LOST_TICK_POLICY_SLEW
After that, we can enable LOST_TICK_POLICY_SLEW in the common code
which need not use '#ifdef TARGET_I386' to make these code be x86
specific anymore
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
hw/timer/mc146818rtc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 14bde1a..7f2e975 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -976,19 +976,19 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
rtc_set_date_from_host(isadev);
-#ifdef TARGET_I386
switch (s->lost_tick_policy) {
+#ifdef TARGET_I386
case LOST_TICK_POLICY_SLEW:
s->coalesced_timer =
timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
break;
+#endif
case LOST_TICK_POLICY_DISCARD:
break;
default:
error_setg(errp, "Invalid lost tick policy.");
return;
}
-#endif
s->periodic_timer = timer_new_ns(rtc_clock, rtc_periodic_timer, s);
s->update_timer = timer_new_ns(rtc_clock, rtc_update_timer, s);
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
` (2 preceding siblings ...)
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 3/5] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386 guangrong.xiao
@ 2017-05-04 11:59 ` guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 5/5] mc146818rtc: embrace all x86 specific code guangrong.xiao
2017-05-04 14:00 ` [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster no-reply
5 siblings, 0 replies; 9+ messages in thread
From: guangrong.xiao @ 2017-05-04 11:59 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
If the code purely depends on LOST_TICK_POLICY_SLEW, we can simply
drop '#ifdef TARGET_I386' as only x86 can enable this tick policy
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
hw/timer/mc146818rtc.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 7f2e975..8fc9313 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -112,7 +112,6 @@ static uint64_t get_guest_rtc_ns(RTCState *s)
guest_clock - s->last_update + s->offset;
}
-#ifdef TARGET_I386
static void rtc_coalesced_timer_update(RTCState *s)
{
if (s->irq_coalesced == 0) {
@@ -126,6 +125,7 @@ static void rtc_coalesced_timer_update(RTCState *s)
}
}
+#ifdef TARGET_I386
static void rtc_coalesced_timer(void *opaque)
{
RTCState *s = opaque;
@@ -204,7 +204,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
lost_clock = cur_clock - last_periodic_clock;
}
-#ifdef TARGET_I386
/*
* recalculate the coalesced irqs for two reasons:
* a) the lost_clock is more that a period, i,e. the timer
@@ -235,7 +234,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
rtc_coalesced_timer_update(s);
}
}
-#endif
+
/*
* no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
* is not used, we should make the time progress anyway.
@@ -248,9 +247,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
RTC_CLOCK_RATE) + 1;
timer_mod(s->periodic_timer, s->next_periodic_time);
} else {
-#ifdef TARGET_I386
s->irq_coalesced = 0;
-#endif
timer_del(s->periodic_timer);
}
}
@@ -837,13 +834,11 @@ static int rtc_post_load(void *opaque, int version_id)
}
}
-#ifdef TARGET_I386
if (version_id >= 2) {
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
rtc_coalesced_timer_update(s);
}
}
-#endif
return 0;
}
@@ -900,11 +895,10 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
rtc_set_date_from_host(ISA_DEVICE(s));
periodic_timer_update(s, now, 0);
check_update_timer(s);
-#ifdef TARGET_I386
+
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
rtc_coalesced_timer_update(s);
}
-#endif
}
/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
@@ -925,12 +919,10 @@ static void rtc_reset(void *opaque)
qemu_irq_lower(s->irq);
-#ifdef TARGET_I386
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
s->irq_coalesced = 0;
s->irq_reinject_on_ack_count = 0;
}
-#endif
}
static const MemoryRegionOps cmos_ops = {
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] mc146818rtc: embrace all x86 specific code
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
` (3 preceding siblings ...)
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 4/5] mc146818rtc: drop unnecessary '#ifdef TARGET_I386' guangrong.xiao
@ 2017-05-04 11:59 ` guangrong.xiao
2017-05-04 14:00 ` [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster no-reply
5 siblings, 0 replies; 9+ messages in thread
From: guangrong.xiao @ 2017-05-04 11:59 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Introduce a function, rtc_policy_slew_deliver_irq(), which delivers
irq if LOST_TICK_POLICY_SLEW is used, as which is only supported on
x86, other platforms call it will trigger a assert
After that, we can move the x86 specific code to the common place
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
hw/timer/mc146818rtc.c | 60 ++++++++++++++++++++++++++------------------------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 8fc9313..987160c 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -125,17 +125,34 @@ static void rtc_coalesced_timer_update(RTCState *s)
}
}
+static QLIST_HEAD(, RTCState) rtc_devices =
+ QLIST_HEAD_INITIALIZER(rtc_devices);
+
#ifdef TARGET_I386
+void qmp_rtc_reset_reinjection(Error **errp)
+{
+ RTCState *s;
+
+ QLIST_FOREACH(s, &rtc_devices, link) {
+ s->irq_coalesced = 0;
+ }
+}
+
+static bool rtc_policy_slew_deliver_irq(RTCState *s)
+{
+ apic_reset_irq_delivered();
+ qemu_irq_raise(s->irq);
+ return apic_get_irq_delivered();
+}
+
static void rtc_coalesced_timer(void *opaque)
{
RTCState *s = opaque;
if (s->irq_coalesced != 0) {
- apic_reset_irq_delivered();
s->cmos_data[RTC_REG_C] |= 0xc0;
DPRINTF_C("cmos: injecting from timer\n");
- qemu_irq_raise(s->irq);
- if (apic_get_irq_delivered()) {
+ if (rtc_policy_slew_deliver_irq(s)) {
s->irq_coalesced--;
DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
s->irq_coalesced);
@@ -144,6 +161,12 @@ static void rtc_coalesced_timer(void *opaque)
rtc_coalesced_timer_update(s);
}
+#else
+static bool rtc_policy_slew_deliver_irq(RTCState *s)
+{
+ assert(0);
+ return false;
+}
#endif
static int period_code_to_clock(int period_code)
@@ -260,21 +283,17 @@ static void rtc_periodic_timer(void *opaque)
s->cmos_data[RTC_REG_C] |= REG_C_PF;
if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
-#ifdef TARGET_I386
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT)
- s->irq_reinject_on_ack_count = 0;
- apic_reset_irq_delivered();
- qemu_irq_raise(s->irq);
- if (!apic_get_irq_delivered()) {
+ s->irq_reinject_on_ack_count = 0;
+ if (!rtc_policy_slew_deliver_irq(s)) {
s->irq_coalesced++;
rtc_coalesced_timer_update(s);
DPRINTF_C("cmos: coalesced irqs increased to %d\n",
s->irq_coalesced);
}
} else
-#endif
- qemu_irq_raise(s->irq);
+ qemu_irq_raise(s->irq);
}
}
@@ -616,20 +635,6 @@ static void rtc_get_time(RTCState *s, struct tm *tm)
rtc_from_bcd(s, s->cmos_data[RTC_CENTURY]) * 100 - 1900;
}
-static QLIST_HEAD(, RTCState) rtc_devices =
- QLIST_HEAD_INITIALIZER(rtc_devices);
-
-#ifdef TARGET_I386
-void qmp_rtc_reset_reinjection(Error **errp)
-{
- RTCState *s;
-
- QLIST_FOREACH(s, &rtc_devices, link) {
- s->irq_coalesced = 0;
- }
-}
-#endif
-
static void rtc_set_time(RTCState *s)
{
struct tm tm;
@@ -749,22 +754,19 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr addr,
if (ret & (REG_C_UF | REG_C_AF)) {
check_update_timer(s);
}
-#ifdef TARGET_I386
+
if(s->irq_coalesced &&
(s->cmos_data[RTC_REG_B] & REG_B_PIE) &&
s->irq_reinject_on_ack_count < RTC_REINJECT_ON_ACK_COUNT) {
s->irq_reinject_on_ack_count++;
s->cmos_data[RTC_REG_C] |= REG_C_IRQF | REG_C_PF;
- apic_reset_irq_delivered();
DPRINTF_C("cmos: injecting on ack\n");
- qemu_irq_raise(s->irq);
- if (apic_get_irq_delivered()) {
+ if (rtc_policy_slew_deliver_irq(s)) {
s->irq_coalesced--;
DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
s->irq_coalesced);
}
}
-#endif
break;
default:
ret = s->cmos_data[s->cmos_index];
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer guangrong.xiao
@ 2017-05-04 12:31 ` Paolo Bonzini
2017-05-05 6:29 ` Xiao Guangrong
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2017-05-04 12:31 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
On 04/05/2017 13:59, guangrong.xiao@gmail.com wrote:
> From: Tai Yunfang <yunfangtai@tencent.com>
>
> There are two issues in current code:
> 1) If the period is changed by re-configuring RegA, the coalesced
> irq will be scaled to reflect the new period, however, it
> calculates the new interrupt number like this:
> s->irq_coalesced = (s->irq_coalesced * s->period) / period;
>
> There are some clocks will be lost if they are not enough to
> be squeezed to a single new period that will cause the VM clock
> slower
>
> In order to fix the issue, we calculate the interrupt window
> based on the precise clock rather than period, then the clocks
> lost during period is scaled can be compensated properly
>
> 2) If periodic_timer_update() is called due to RegA reconfiguration,
> i.e, the period is updated, current time is not the start point
> for the next periodic timer, instead, which should start from the
> last interrupt, otherwise, the clock in VM will become slow
>
> This patch takes the clocks from last interrupt to current clock
> into account and compensates the clocks for the next interrupt,
> especially，if a complete interrupt was lost in this window, the
> time can be caught up by LOST_TICK_POLICY_SLEW
>
> [ Xiao: redesign the algorithm based on Yunfang's original work. ]
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> Signed-off-by: Tai Yunfang <yunfangtai@tencent.com>
> ---
> hw/timer/mc146818rtc.c | 116 ++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 96 insertions(+), 20 deletions(-)
>
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 5cccb2a..14bde1a 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -146,31 +146,104 @@ static void rtc_coalesced_timer(void *opaque)
> }
> #endif
>
> -/* handle periodic timer */
> -static void periodic_timer_update(RTCState *s, int64_t current_time)
> +static int period_code_to_clock(int period_code)
> +{
> + /* periodic timer is disabled. */
> + if (!period_code) {
> + return 0;
> + }
> +
> + if (period_code <= 2) {
> + period_code += 7;
> + }
> +
> + /* period in 32 Khz cycles */
> + return 1 << (period_code - 1);
> +}
> +
> +/*
> + * handle periodic timer. @old_period indicates the periodic timer update
> + * is just due to period adjustment.
> + */
> +static void
> +periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
We need old_period to distinguish reconfiguration from interrupts ("if
(old_period)" below), but it can be a bool. Instead of passing the old
period value, we can use s->period which is
period_code_to_clock(old_period).
That is, just do
old_period = s->period;
s->period = rtc_periodic_clock_ticks(s);
/*
* if the periodic timer's update is due to period re-configuration,
* the clock should count since last interrupt.
*/
if (s->period != 0) {
...
if (!from_interrupt) {
}
...
}
where rtc_periodic_clock_ticks takes into account both regA and regB,
returning the result in 32 kHZ ticks.
> {
> int period_code, period;
> - int64_t cur_clock, next_irq_clock;
> + int64_t cur_clock, next_irq_clock, lost_clock = 0;
>
> period_code = s->cmos_data[RTC_REG_A] & 0x0f;
> if (period_code != 0
> && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
> - if (period_code <= 2)
> - period_code += 7;
> - /* period in 32 Khz cycles */
> - period = 1 << (period_code - 1);
> -#ifdef TARGET_I386
> - if (period != s->period) {
> - s->irq_coalesced = (s->irq_coalesced * s->period) / period;
> - DPRINTF_C("cmos: coalesced irqs scaled to %d\n", s->irq_coalesced);
> - }
> - s->period = period;
> -#endif
> + period = period_code_to_clock(period_code);
Since we have old_period, we can assign s->period here.
> /* compute 32 khz clock */
> cur_clock =
> muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>
> - next_irq_clock = (cur_clock & ~(period - 1)) + period;
> + /*
> + * if the periodic timer's update is due to period re-configuration,
> + * we should count the clock since last interrupt.
> + */
> + if (old_period) {
> + int64_t last_periodic_clock;
> +
> + last_periodic_clock = muldiv64(s->next_periodic_time,
> + RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> + /*
> + * if the next interrupt has not happened yet, we recall the last
> + * interrupt based on the original period.
> + */
> + if (last_periodic_clock > cur_clock) {
> + last_periodic_clock -= period_code_to_clock(old_period);
This becomes "last_periodic_clock -= old_period".
But, I'm not sure how it can happen that last_periodic_clock <=
cur_clock. More precisely, if it happened, you have lost a tick and you
should add it to s->irq_coalesced it below. The simplest way to do it,
is to *always* do the subtraction.
> +
> + /* the last interrupt must have happened. */
> + assert(cur_clock >= last_periodic_clock);
> + }
> +
> + /* calculate the clock since last interrupt. */
> + lost_clock = cur_clock - last_periodic_clock;
> + }
I think the "assert(lost_clock >= 0)" should be here. Then you can also
remove the "the last interrupt must have happened" assertion, since the
two test exactly the same formula:
lost_clock >= 0
cur_clock - last_periodic_clock >= 0
cur_clock >= last_periodic_clock.
Putting everything together, this becomes:
if (!from_interrupt) {
int64_t next_periodic_clock, last_periodic_clock;
next_periodic_clock = muldiv64(s->next_periodic_time,
RTC_CLOCK_RATE,
NANOSECONDS_PER_SECOND);
last_periodic_clock = next_periodic_clock - old_period;
lost_clock = cur_clock - last_periodic_clock;
assert(lost_clock >= 0);
}
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
...
} else {
lost_clock = MIN(lost_clock, s->period);
}
assert(lost_clock >= 0 && lost_clock < s->period);
> + /*
> +#ifdef TARGET_I386
> + /*
> + * recalculate the coalesced irqs for two reasons:
> + * a) the lost_clock is more that a period, i,e. the timer
> + * interrupt has been lost, we should catch up the time.
> + *
> + * b) the period may be reconfigured, under this case, when
> + * switching from a shorter to a longer period, scale down
> + * the missing ticks since we expect the OS handler to
> + * treat the delayed ticks as longer. Any leftovers are
> + * put back into lost_clock.
> + * When switching to a shorter period, scale up the missing
> + * ticks since we expect the OS handler to treat the delayed
> + * ticks as shorter.
> + */
> + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
> + uint32_t cur_irq_coalesced = s->irq_coalesced;
> + uint32_t cur_period = s->period;
These are really "old" values rather than current.
> +
> + lost_clock += cur_irq_coalesced * cur_period;
> + s->irq_coalesced = lost_clock / period;
> + lost_clock %= period;
> + s->period = period;
> + if ((cur_irq_coalesced != s->irq_coalesced) ||
> + (cur_period |= s->period)) {
Typo here, so this becomes:
uint32_t old_irq_coalesced = s->irq_coalesced;
lost_clock += old_irq_coalesced * old_period;
s->irq_coalesced = lost_clock / s->period;
lost_clock %= s->period;
if (old_irq_coalesced != s->irq_coalesced ||
old_period != s->period) {
DPRINTF ...
rtc_coalesced_timer_update(s);
}
> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
> + "period scaled from %d to %d\n", cur_irq_coalesced,
> + s->irq_coalesced, cur_period, s->period);
> + rtc_coalesced_timer_update(s);
> + }
> + }
> +#endif
> + /*
> + * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
> + * is not used, we should make the time progress anyway.
> + */
> + lost_clock = MIN(lost_clock, period);
See above---let's put it under "else", since it is unnecessary in the
SLEW case.
This is already much more understandable, and all of it makes a lot of
sense. The next version should be perfect. :)
Paolo
> + assert(lost_clock >= 0);
> +
> + next_irq_clock = cur_clock + period - lost_clock;
> s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND,
> RTC_CLOCK_RATE) + 1;
> timer_mod(s->periodic_timer, s->next_periodic_time);
> @@ -186,7 +259,7 @@ static void rtc_periodic_timer(void *opaque)
> {
> RTCState *s = opaque;
>
> - periodic_timer_update(s, s->next_periodic_time);
> + periodic_timer_update(s, s->next_periodic_time, 0);
> s->cmos_data[RTC_REG_C] |= REG_C_PF;
> if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
> s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> @@ -391,6 +464,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> uint64_t data, unsigned size)
> {
> RTCState *s = opaque;
> + int cur_period;
> bool update_periodic_timer;
>
> if ((addr & 1) == 0) {
> @@ -424,6 +498,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> }
> break;
> case RTC_REG_A:
> + cur_period = s->cmos_data[RTC_REG_A] & 0xf;
> update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & 0x0f;
>
> if ((data & 0x60) == 0x60) {
> @@ -450,7 +525,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> (s->cmos_data[RTC_REG_A] & REG_A_UIP);
>
> if (update_periodic_timer) {
> - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
> + cur_period);
> }
>
> check_update_timer(s);
> @@ -487,7 +563,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> s->cmos_data[RTC_REG_B] = data;
>
> if (update_periodic_timer) {
> - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
> }
>
> check_update_timer(s);
> @@ -757,7 +833,7 @@ static int rtc_post_load(void *opaque, int version_id)
> uint64_t now = qemu_clock_get_ns(rtc_clock);
> if (now < s->next_periodic_time ||
> now > (s->next_periodic_time + get_max_clock_jump())) {
> - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
> }
> }
>
> @@ -822,7 +898,7 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
> int64_t now = *(int64_t *)data;
>
> rtc_set_date_from_host(ISA_DEVICE(s));
> - periodic_timer_update(s, now);
> + periodic_timer_update(s, now, 0);
> check_update_timer(s);
> #ifdef TARGET_I386
> if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
` (4 preceding siblings ...)
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 5/5] mc146818rtc: embrace all x86 specific code guangrong.xiao
@ 2017-05-04 14:00 ` no-reply
5 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2017-05-04 14:00 UTC (permalink / raw)
To: guangrong.xiao
Cc: famz, pbonzini, mst, mtosatti, xiaoguangrong, yunfangtai,
qemu-devel, kvm
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster
Message-id: 20170504115948.3048-1-xiaoguangrong@tencent.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/20170504125432.21653-1-marcandre.lureau@redhat.com -> patchew/20170504125432.21653-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
f61e071 mc146818rtc: embrace all x86 specific code
dec1091 mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
9d50ad1 mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
d192a0f mc146818rtc: precisely count the clock for periodic timer
21b28e7 mc146818rtc: update periodic timer only if it is needed
=== OUTPUT BEGIN ===
Checking PATCH 1/5: mc146818rtc: update periodic timer only if it is needed...
Checking PATCH 2/5: mc146818rtc: precisely count the clock for periodic timer...
ERROR: do not use assignment in if condition
#143: FILE: hw/timer/mc146818rtc.c:230:
+ if ((cur_irq_coalesced != s->irq_coalesced) ||
total: 1 errors, 0 warnings, 174 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/5: mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386...
Checking PATCH 4/5: mc146818rtc: drop unnecessary '#ifdef TARGET_I386'...
Checking PATCH 5/5: mc146818rtc: embrace all x86 specific code...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer
2017-05-04 12:31 ` Paolo Bonzini
@ 2017-05-05 6:29 ` Xiao Guangrong
0 siblings, 0 replies; 9+ messages in thread
From: Xiao Guangrong @ 2017-05-05 6:29 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
On 05/04/2017 08:31 PM, Paolo Bonzini wrote:
>
>
> On 04/05/2017 13:59, guangrong.xiao@gmail.com wrote:
>> From: Tai Yunfang <yunfangtai@tencent.com>
>>
>> There are two issues in current code:
>> 1) If the period is changed by re-configuring RegA, the coalesced
>> irq will be scaled to reflect the new period, however, it
>> calculates the new interrupt number like this:
>> s->irq_coalesced = (s->irq_coalesced * s->period) / period;
>>
>> There are some clocks will be lost if they are not enough to
>> be squeezed to a single new period that will cause the VM clock
>> slower
>>
>> In order to fix the issue, we calculate the interrupt window
>> based on the precise clock rather than period, then the clocks
>> lost during period is scaled can be compensated properly
>>
>> 2) If periodic_timer_update() is called due to RegA reconfiguration,
>> i.e, the period is updated, current time is not the start point
>> for the next periodic timer, instead, which should start from the
>> last interrupt, otherwise, the clock in VM will become slow
>>
>> This patch takes the clocks from last interrupt to current clock
>> into account and compensates the clocks for the next interrupt,
>> especially，if a complete interrupt was lost in this window, the
>> time can be caught up by LOST_TICK_POLICY_SLEW
>>
>> [ Xiao: redesign the algorithm based on Yunfang's original work. ]
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> Signed-off-by: Tai Yunfang <yunfangtai@tencent.com>
>> ---
>> hw/timer/mc146818rtc.c | 116 ++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 96 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>> index 5cccb2a..14bde1a 100644
>> --- a/hw/timer/mc146818rtc.c
>> +++ b/hw/timer/mc146818rtc.c
>> @@ -146,31 +146,104 @@ static void rtc_coalesced_timer(void *opaque)
>> }
>> #endif
>>
>> -/* handle periodic timer */
>> -static void periodic_timer_update(RTCState *s, int64_t current_time)
>> +static int period_code_to_clock(int period_code)
>> +{
>> + /* periodic timer is disabled. */
>> + if (!period_code) {
>> + return 0;
>> + }
>> +
>> + if (period_code <= 2) {
>> + period_code += 7;
>> + }
>> +
>> + /* period in 32 Khz cycles */
>> + return 1 << (period_code - 1);
>> +}
>> +
>> +/*
>> + * handle periodic timer. @old_period indicates the periodic timer update
>> + * is just due to period adjustment.
>> + */
>> +static void
>> +periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
>
> We need old_period to distinguish reconfiguration from interrupts ("if
> (old_period)" below), but it can be a bool. Instead of passing the old
> period value, we can use s->period which is
> period_code_to_clock(old_period).
>
> That is, just do
>
> old_period = s->period;
> s->period = rtc_periodic_clock_ticks(s);
>
> /*
> * if the periodic timer's update is due to period re-configuration,
> * the clock should count since last interrupt.
> */
> if (s->period != 0) {
> ...
> if (!from_interrupt) {
> }
> ...
> }
>
> where rtc_periodic_clock_ticks takes into account both regA and regB,
> returning the result in 32 kHZ ticks.
>
>> {
>> int period_code, period;
>> - int64_t cur_clock, next_irq_clock;
>> + int64_t cur_clock, next_irq_clock, lost_clock = 0;
>>
>> period_code = s->cmos_data[RTC_REG_A] & 0x0f;
>> if (period_code != 0
>> && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
>> - if (period_code <= 2)
>> - period_code += 7;
>> - /* period in 32 Khz cycles */
>> - period = 1 << (period_code - 1);
>> -#ifdef TARGET_I386
>> - if (period != s->period) {
>> - s->irq_coalesced = (s->irq_coalesced * s->period) / period;
>> - DPRINTF_C("cmos: coalesced irqs scaled to %d\n", s->irq_coalesced);
>> - }
>> - s->period = period;
>> -#endif
>> + period = period_code_to_clock(period_code);
>
>
> Since we have old_period, we can assign s->period here.
>
>> /* compute 32 khz clock */
>> cur_clock =
>> muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>>
>> - next_irq_clock = (cur_clock & ~(period - 1)) + period;
>> + /*
>> + * if the periodic timer's update is due to period re-configuration,
>> + * we should count the clock since last interrupt.
>> + */
>> + if (old_period) {
>> + int64_t last_periodic_clock;
>> +
>> + last_periodic_clock = muldiv64(s->next_periodic_time,
>> + RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>> + /*
>> + * if the next interrupt has not happened yet, we recall the last
>> + * interrupt based on the original period.
>> + */
>> + if (last_periodic_clock > cur_clock) {
>> + last_periodic_clock -= period_code_to_clock(old_period);
>
> This becomes "last_periodic_clock -= old_period".
>
> But, I'm not sure how it can happen that last_periodic_clock <=
> cur_clock. More precisely, if it happened, you have lost a tick and you
> should add it to s->irq_coalesced it below. The simplest way to do it,
> is to *always* do the subtraction.
>
Yes, i agree with you.
>
>> +
>> + /* the last interrupt must have happened. */
>> + assert(cur_clock >= last_periodic_clock);
>> + }
>> +
>> + /* calculate the clock since last interrupt. */
>> + lost_clock = cur_clock - last_periodic_clock;
>> + }
>
> I think the "assert(lost_clock >= 0)" should be here. Then you can also
> remove the "the last interrupt must have happened" assertion, since the
> two test exactly the same formula:
>
> lost_clock >= 0
> cur_clock - last_periodic_clock >= 0
> cur_clock >= last_periodic_clock.
>
> Putting everything together, this becomes:
>
> if (!from_interrupt) {
the lost-clock needs to be adjusted only for the enable-to-enable case,
i.e, if the periodic_timer is changed from disabled to enabled, nothing
need to be compensated.
So i am going to change this if statement to:
if (reconfig && old_period) {
......
}
> int64_t next_periodic_clock, last_periodic_clock;
>
> next_periodic_clock = muldiv64(s->next_periodic_time,
> RTC_CLOCK_RATE,
> NANOSECONDS_PER_SECOND);
> last_periodic_clock = next_periodic_clock - old_period;
> lost_clock = cur_clock - last_periodic_clock;
> assert(lost_clock >= 0);
> }
>
> if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
> ...
> } else {
> lost_clock = MIN(lost_clock, s->period);
> }
> assert(lost_clock >= 0 && lost_clock < s->period);
All of you suggestions look nice to me and very elaborate, however,
there is a issue that you use s->period on all platforms, in the
current code, only x86 is using it. So that will break live migration
between two !x86 VMs if one applied this pathset, another one does not.
If do not object, i will keep the old_period part in this patchset
and apply all you other suggestions. :)
Thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-05 6:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-04 11:59 [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 1/5] mc146818rtc: update periodic timer only if it is needed guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 2/5] mc146818rtc: precisely count the clock for periodic timer guangrong.xiao
2017-05-04 12:31 ` Paolo Bonzini
2017-05-05 6:29 ` Xiao Guangrong
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 3/5] mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386 guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 4/5] mc146818rtc: drop unnecessary '#ifdef TARGET_I386' guangrong.xiao
2017-05-04 11:59 ` [Qemu-devel] [PATCH v2 5/5] mc146818rtc: embrace all x86 specific code guangrong.xiao
2017-05-04 14:00 ` [Qemu-devel] [PATCH v2 0/5] mc146818rtc: fix Windows VM clock faster no-reply
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).