[PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

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: properly count the time for the next interrupt

Xiao Guangrong (4):
  mc146818rtc: update periodic timer only if it is needed
  mc146818rtc: fix clock lost after scaling coalesced irq
  mc146818rtc: move x86 specific code out of periodic_timer_update
  mc146818rtc: embrace all x86 specific code

 hw/timer/mc146818rtc.c | 370 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 272 insertions(+), 98 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

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: properly count the time for the next interrupt

Xiao Guangrong (4):
  mc146818rtc: update periodic timer only if it is needed
  mc146818rtc: fix clock lost after scaling coalesced irq
  mc146818rtc: move x86 specific code out of periodic_timer_update
  mc146818rtc: embrace all x86 specific code

 hw/timer/mc146818rtc.c | 370 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 272 insertions(+), 98 deletions(-)

-- 
2.9.3

[PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[guangrong.xiao]

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 | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 4165450..749e206 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
     check_update_timer(s);
 }
 
+static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
+{
+    uint8_t orig = s->cmos_data[RTC_REG_A];
+
+    return (orig & 0x0f) != (data & 0x0f);
+}
+
+static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
+{
+    uint8_t orig = s->cmos_data[RTC_REG_B];
+
+    return (orig & REG_B_PIE) != (data & REG_B_PIE);
+}
+
 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 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
             }
             break;
         case RTC_REG_A:
+            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);
+
             if ((data & 0x60) == 0x60) {
                 if (rtc_running(s)) {
                     rtc_update_time(s);
@@ -445,10 +462,16 @@ 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 = rtc_periodic_timer_updated_by_regB(s, data);
+
             if (data & REG_B_SET) {
                 /* update cmos to when the rtc was stopping */
                 if (rtc_running(s)) {
@@ -475,7 +498,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

[Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[guangrong.xiao]

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 | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 4165450..749e206 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
     check_update_timer(s);
 }
 
+static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
+{
+    uint8_t orig = s->cmos_data[RTC_REG_A];
+
+    return (orig & 0x0f) != (data & 0x0f);
+}
+
+static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
+{
+    uint8_t orig = s->cmos_data[RTC_REG_B];
+
+    return (orig & REG_B_PIE) != (data & REG_B_PIE);
+}
+
 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 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
             }
             break;
         case RTC_REG_A:
+            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);
+
             if ((data & 0x60) == 0x60) {
                 if (rtc_running(s)) {
                     rtc_update_time(s);
@@ -445,10 +462,16 @@ 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 = rtc_periodic_timer_updated_by_regB(s, data);
+
             if (data & REG_B_SET) {
                 /* update cmos to when the rtc was stopping */
                 if (rtc_running(s)) {
@@ -475,7 +498,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

[PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

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

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 749e206..649678c 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -146,23 +146,46 @@ static void rtc_coalesced_timer(void *opaque)
 }
 #endif
 
+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 */
 static void periodic_timer_update(RTCState *s, int64_t current_time)
 {
     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);
+        period = period_code_to_clock(period_code);
 #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);
+            int current_irq_coalesced = s->irq_coalesced;
+
+            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
+
+            /*
+             * calculate the lost clock after it is scaled which should be
+             * compensated in the next interrupt.
+             */
+            lost_clock += current_irq_coalesced * s->period -
+                            s->irq_coalesced * period;
+            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
+                      "are compensated.\n",
+                      current_irq_coalesced, s->irq_coalesced, lost_clock);
         }
         s->period = period;
 #endif
@@ -170,7 +193,7 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
 
-        next_irq_clock = (cur_clock & ~(period - 1)) + period;
+        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);
-- 
2.9.3

[Qemu-devel] [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

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

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 749e206..649678c 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -146,23 +146,46 @@ static void rtc_coalesced_timer(void *opaque)
 }
 #endif
 
+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 */
 static void periodic_timer_update(RTCState *s, int64_t current_time)
 {
     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);
+        period = period_code_to_clock(period_code);
 #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);
+            int current_irq_coalesced = s->irq_coalesced;
+
+            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
+
+            /*
+             * calculate the lost clock after it is scaled which should be
+             * compensated in the next interrupt.
+             */
+            lost_clock += current_irq_coalesced * s->period -
+                            s->irq_coalesced * period;
+            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
+                      "are compensated.\n",
+                      current_irq_coalesced, s->irq_coalesced, lost_clock);
         }
         s->period = period;
 #endif
@@ -170,7 +193,7 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
 
-        next_irq_clock = (cur_clock & ~(period - 1)) + period;
+        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);
-- 
2.9.3

[PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[guangrong.xiao]

From: Tai Yunfang <yunfangtai@tencent.com>

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: Tai Yunfang <yunfangtai@tencent.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 7 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 649678c..3bf559d 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -161,8 +161,12 @@ static int period_code_to_clock(int period_code)
     return 1 << (period_code - 1);
 }
 
-/* handle periodic timer */
-static void periodic_timer_update(RTCState *s, int64_t current_time)
+/*
+ * 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, lost_clock = 0;
@@ -193,6 +197,56 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
 
+        /*
+        * 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
+            /*
+             * if more than period clocks were passed, i.e, the timer interrupt
+             * has been lost, we should catch up the time.
+             */
+            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
+                (lost_clock / period)) {
+                int lost_interrupt = lost_clock / period;
+
+                s->irq_coalesced += lost_interrupt;
+                lost_clock -= lost_interrupt * period;
+                if (lost_interrupt) {
+                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
+                              "increased to %d\n", lost_interrupt,
+                              s->irq_coalesced);
+                    rtc_coalesced_timer_update(s);
+                }
+            } else
+#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;
@@ -209,7 +263,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;
@@ -428,6 +482,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) {
@@ -461,6 +516,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 = rtc_periodic_timer_updated_by_regA(s, data);
 
             if ((data & 0x60) == 0x60) {
@@ -487,7 +543,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);
@@ -523,7 +580,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);
@@ -793,7 +850,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);
         }
     }
 
@@ -858,7 +915,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

[Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[guangrong.xiao]

From: Tai Yunfang <yunfangtai@tencent.com>

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: Tai Yunfang <yunfangtai@tencent.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 7 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 649678c..3bf559d 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -161,8 +161,12 @@ static int period_code_to_clock(int period_code)
     return 1 << (period_code - 1);
 }
 
-/* handle periodic timer */
-static void periodic_timer_update(RTCState *s, int64_t current_time)
+/*
+ * 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, lost_clock = 0;
@@ -193,6 +197,56 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
 
+        /*
+        * 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
+            /*
+             * if more than period clocks were passed, i.e, the timer interrupt
+             * has been lost, we should catch up the time.
+             */
+            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
+                (lost_clock / period)) {
+                int lost_interrupt = lost_clock / period;
+
+                s->irq_coalesced += lost_interrupt;
+                lost_clock -= lost_interrupt * period;
+                if (lost_interrupt) {
+                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
+                              "increased to %d\n", lost_interrupt,
+                              s->irq_coalesced);
+                    rtc_coalesced_timer_update(s);
+                }
+            } else
+#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;
@@ -209,7 +263,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;
@@ -428,6 +482,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) {
@@ -461,6 +516,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 = rtc_periodic_timer_updated_by_regA(s, data);
 
             if ((data & 0x60) == 0x60) {
@@ -487,7 +543,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);
@@ -523,7 +580,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);
@@ -793,7 +850,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);
         }
     }
 
@@ -858,7 +915,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

[PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Move the x86 specific code in periodic_timer_update() to a common place,
the actual logic is not changed

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 46 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 3bf559d..d7b7c56 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
 
     rtc_coalesced_timer_update(s);
 }
+
+static int64_t
+arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
+{
+    if (period != s->period) {
+        int64_t scale_lost_clock;
+        int current_irq_coalesced = s->irq_coalesced;
+
+        s->irq_coalesced = (current_irq_coalesced * s->period) / period;
+
+        /*
+         * calculate the lost clock after it is scaled which should be
+         * compensated in the next interrupt.
+         */
+        scale_lost_clock = current_irq_coalesced * s->period -
+                            s->irq_coalesced * period;
+        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
+                  "are compensated.\n", current_irq_coalesced,
+                  s->irq_coalesced, scale_lost_clock);
+        lost_clock += scale_lost_clock;
+        s->period = period;
+    }
+
+    /*
+     * if more than period clocks were passed, i.e, the timer interrupt
+     * has been lost, we should catch up the time.
+     */
+    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
+       (lost_clock / period)) {
+        int lost_interrupt = lost_clock / period;
+
+        s->irq_coalesced += lost_interrupt;
+        lost_clock -= lost_interrupt * period;
+        if (lost_interrupt) {
+            DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
+                      "increased to %d\n", lost_interrupt, s->irq_coalesced);
+            rtc_coalesced_timer_update(s);
+        }
+    }
+
+    return lost_clock;
+}
+
+static void arch_periodic_timer_disable(RTCState *s)
+{
+    s->irq_coalesced = 0;
+}
+#else
+static int64_t
+arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
+{
+    return lost_clock;
+}
+
+static void arch_periodic_timer_disable(RTCState *s)
+{
+}
 #endif
 
 static int period_code_to_clock(int period_code)
@@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
     if (period_code != 0
         && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
         period = period_code_to_clock(period_code);
-#ifdef TARGET_I386
-        if (period != s->period) {
-            int current_irq_coalesced = s->irq_coalesced;
-
-            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
 
-            /*
-             * calculate the lost clock after it is scaled which should be
-             * compensated in the next interrupt.
-             */
-            lost_clock += current_irq_coalesced * s->period -
-                            s->irq_coalesced * period;
-            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
-                      "are compensated.\n",
-                      current_irq_coalesced, s->irq_coalesced, lost_clock);
-        }
-        s->period = period;
-#endif
         /* compute 32 khz clock */
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
@@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
 
             /* calculate the clock since last interrupt. */
             lost_clock += cur_clock - last_periodic_clock;
-
-#ifdef TARGET_I386
-            /*
-             * if more than period clocks were passed, i.e, the timer interrupt
-             * has been lost, we should catch up the time.
-             */
-            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
-                (lost_clock / period)) {
-                int lost_interrupt = lost_clock / period;
-
-                s->irq_coalesced += lost_interrupt;
-                lost_clock -= lost_interrupt * period;
-                if (lost_interrupt) {
-                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
-                              "increased to %d\n", lost_interrupt,
-                              s->irq_coalesced);
-                    rtc_coalesced_timer_update(s);
-                }
-            } else
-#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);
         }
 
+        lost_clock = arch_periodic_timer_update(s, period, lost_clock);
+
+        /*
+         * 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);
     } else {
-#ifdef TARGET_I386
-        s->irq_coalesced = 0;
-#endif
+        arch_periodic_timer_disable(s);
         timer_del(s->periodic_timer);
     }
 }
-- 
2.9.3

[Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Move the x86 specific code in periodic_timer_update() to a common place,
the actual logic is not changed

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 46 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 3bf559d..d7b7c56 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
 
     rtc_coalesced_timer_update(s);
 }
+
+static int64_t
+arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
+{
+    if (period != s->period) {
+        int64_t scale_lost_clock;
+        int current_irq_coalesced = s->irq_coalesced;
+
+        s->irq_coalesced = (current_irq_coalesced * s->period) / period;
+
+        /*
+         * calculate the lost clock after it is scaled which should be
+         * compensated in the next interrupt.
+         */
+        scale_lost_clock = current_irq_coalesced * s->period -
+                            s->irq_coalesced * period;
+        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
+                  "are compensated.\n", current_irq_coalesced,
+                  s->irq_coalesced, scale_lost_clock);
+        lost_clock += scale_lost_clock;
+        s->period = period;
+    }
+
+    /*
+     * if more than period clocks were passed, i.e, the timer interrupt
+     * has been lost, we should catch up the time.
+     */
+    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
+       (lost_clock / period)) {
+        int lost_interrupt = lost_clock / period;
+
+        s->irq_coalesced += lost_interrupt;
+        lost_clock -= lost_interrupt * period;
+        if (lost_interrupt) {
+            DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
+                      "increased to %d\n", lost_interrupt, s->irq_coalesced);
+            rtc_coalesced_timer_update(s);
+        }
+    }
+
+    return lost_clock;
+}
+
+static void arch_periodic_timer_disable(RTCState *s)
+{
+    s->irq_coalesced = 0;
+}
+#else
+static int64_t
+arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
+{
+    return lost_clock;
+}
+
+static void arch_periodic_timer_disable(RTCState *s)
+{
+}
 #endif
 
 static int period_code_to_clock(int period_code)
@@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
     if (period_code != 0
         && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
         period = period_code_to_clock(period_code);
-#ifdef TARGET_I386
-        if (period != s->period) {
-            int current_irq_coalesced = s->irq_coalesced;
-
-            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
 
-            /*
-             * calculate the lost clock after it is scaled which should be
-             * compensated in the next interrupt.
-             */
-            lost_clock += current_irq_coalesced * s->period -
-                            s->irq_coalesced * period;
-            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
-                      "are compensated.\n",
-                      current_irq_coalesced, s->irq_coalesced, lost_clock);
-        }
-        s->period = period;
-#endif
         /* compute 32 khz clock */
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
@@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
 
             /* calculate the clock since last interrupt. */
             lost_clock += cur_clock - last_periodic_clock;
-
-#ifdef TARGET_I386
-            /*
-             * if more than period clocks were passed, i.e, the timer interrupt
-             * has been lost, we should catch up the time.
-             */
-            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
-                (lost_clock / period)) {
-                int lost_interrupt = lost_clock / period;
-
-                s->irq_coalesced += lost_interrupt;
-                lost_clock -= lost_interrupt * period;
-                if (lost_interrupt) {
-                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
-                              "increased to %d\n", lost_interrupt,
-                              s->irq_coalesced);
-                    rtc_coalesced_timer_update(s);
-                }
-            } else
-#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);
         }
 
+        lost_clock = arch_periodic_timer_update(s, period, lost_clock);
+
+        /*
+         * 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);
     } else {
-#ifdef TARGET_I386
-        s->irq_coalesced = 0;
-#endif
+        arch_periodic_timer_disable(s);
         timer_del(s->periodic_timer);
     }
 }
-- 
2.9.3

[PATCH 5/5] mc146818rtc: embrace all x86 specific code

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

This patch introduces proper hooks in the common code then moves
x86 specific code into a single '#ifdef TARGET_I386'

The real logic is not touched

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 197 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 122 insertions(+), 75 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index d7b7c56..5cb4621 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -112,6 +112,9 @@ static uint64_t get_guest_rtc_ns(RTCState *s)
         guest_clock - s->last_update + s->offset;
 }
 
+static QLIST_HEAD(, RTCState) rtc_devices =
+    QLIST_HEAD_INITIALIZER(rtc_devices);
+
 #ifdef TARGET_I386
 static void rtc_coalesced_timer_update(RTCState *s)
 {
@@ -145,6 +148,15 @@ static void rtc_coalesced_timer(void *opaque)
     rtc_coalesced_timer_update(s);
 }
 
+void qmp_rtc_reset_reinjection(Error **errp)
+{
+    RTCState *s;
+
+    QLIST_FOREACH(s, &rtc_devices, link) {
+        s->irq_coalesced = 0;
+    }
+}
+
 static int64_t
 arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
 {
@@ -191,6 +203,83 @@ static void arch_periodic_timer_disable(RTCState *s)
 {
     s->irq_coalesced = 0;
 }
+
+static void arch_rtc_periodic_timer(RTCState *s)
+{
+    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_coalesced++;
+            rtc_coalesced_timer_update(s);
+            DPRINTF_C("cmos: coalesced irqs increased to %d\n",
+                      s->irq_coalesced);
+        }
+    } else {
+        qemu_irq_raise(s->irq);
+    }
+}
+
+static void arch_read_regC(RTCState *s)
+{
+    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()) {
+            s->irq_coalesced--;
+            DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
+                      s->irq_coalesced);
+        }
+    }
+}
+
+static void arch_rtc_post_load(RTCState *s, int version_id)
+{
+    if (version_id >= 2) {
+        if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+            rtc_coalesced_timer_update(s);
+        }
+    }
+}
+
+static void arch_rtc_notify_clock_reset(RTCState *s)
+{
+    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+        rtc_coalesced_timer_update(s);
+    }
+}
+
+static void arch_rtc_reset(RTCState *s)
+{
+    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+        s->irq_coalesced = 0;
+        s->irq_reinject_on_ack_count = 0;
+    }
+}
+
+static bool arch_rtc_realizefn(RTCState *s, Error **errp)
+{
+    switch (s->lost_tick_policy) {
+    case LOST_TICK_POLICY_SLEW:
+        s->coalesced_timer =
+            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
+        break;
+    case LOST_TICK_POLICY_DISCARD:
+        break;
+    default:
+        error_setg(errp, "Invalid lost tick policy.");
+        return false;
+    }
+
+    return true;
+}
 #else
 static int64_t
 arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
@@ -201,6 +290,32 @@ arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
 static void arch_periodic_timer_disable(RTCState *s)
 {
 }
+
+static void arch_rtc_periodic_timer(RTCState *s)
+{
+    qemu_irq_raise(s->irq);
+}
+
+static void arch_read_regC(RTCState *s)
+{
+}
+
+static void arch_rtc_post_load(RTCState *s, int version_id)
+{
+}
+
+static void arch_rtc_notify_clock_reset(RTCState *s)
+{
+}
+
+static void arch_rtc_reset(RTCState *s)
+{
+}
+
+static bool arch_rtc_realizefn(RTCState *s, Error **errp)
+{
+    return true;
+}
 #endif
 
 static int period_code_to_clock(int period_code)
@@ -287,21 +402,7 @@ 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_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);
+        arch_rtc_periodic_timer(s);
     }
 }
 
@@ -656,20 +757,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;
@@ -789,22 +876,8 @@ 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()) {
-                    s->irq_coalesced--;
-                    DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
-                              s->irq_coalesced);
-                }
-            }
-#endif
+
+            arch_read_regC(s);
             break;
         default:
             ret = s->cmos_data[s->cmos_index];
@@ -874,13 +947,7 @@ 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
+    arch_rtc_post_load(s, version_id);
     return 0;
 }
 
@@ -937,11 +1004,7 @@ 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
+    arch_rtc_notify_clock_reset(s);
 }
 
 /* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
@@ -961,13 +1024,7 @@ static void rtc_reset(void *opaque)
     check_update_timer(s);
 
     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
+    arch_rtc_reset(s);
 }
 
 static const MemoryRegionOps cmos_ops = {
@@ -1013,19 +1070,9 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 
     rtc_set_date_from_host(isadev);
 
-#ifdef TARGET_I386
-    switch (s->lost_tick_policy) {
-    case LOST_TICK_POLICY_SLEW:
-        s->coalesced_timer =
-            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
-        break;
-    case LOST_TICK_POLICY_DISCARD:
-        break;
-    default:
-        error_setg(errp, "Invalid lost tick policy.");
+    if (!arch_rtc_realizefn(s, errp)) {
         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

[Qemu-devel] [PATCH 5/5] mc146818rtc: embrace all x86 specific code

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

This patch introduces proper hooks in the common code then moves
x86 specific code into a single '#ifdef TARGET_I386'

The real logic is not touched

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 197 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 122 insertions(+), 75 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index d7b7c56..5cb4621 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -112,6 +112,9 @@ static uint64_t get_guest_rtc_ns(RTCState *s)
         guest_clock - s->last_update + s->offset;
 }
 
+static QLIST_HEAD(, RTCState) rtc_devices =
+    QLIST_HEAD_INITIALIZER(rtc_devices);
+
 #ifdef TARGET_I386
 static void rtc_coalesced_timer_update(RTCState *s)
 {
@@ -145,6 +148,15 @@ static void rtc_coalesced_timer(void *opaque)
     rtc_coalesced_timer_update(s);
 }
 
+void qmp_rtc_reset_reinjection(Error **errp)
+{
+    RTCState *s;
+
+    QLIST_FOREACH(s, &rtc_devices, link) {
+        s->irq_coalesced = 0;
+    }
+}
+
 static int64_t
 arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
 {
@@ -191,6 +203,83 @@ static void arch_periodic_timer_disable(RTCState *s)
 {
     s->irq_coalesced = 0;
 }
+
+static void arch_rtc_periodic_timer(RTCState *s)
+{
+    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_coalesced++;
+            rtc_coalesced_timer_update(s);
+            DPRINTF_C("cmos: coalesced irqs increased to %d\n",
+                      s->irq_coalesced);
+        }
+    } else {
+        qemu_irq_raise(s->irq);
+    }
+}
+
+static void arch_read_regC(RTCState *s)
+{
+    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()) {
+            s->irq_coalesced--;
+            DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
+                      s->irq_coalesced);
+        }
+    }
+}
+
+static void arch_rtc_post_load(RTCState *s, int version_id)
+{
+    if (version_id >= 2) {
+        if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+            rtc_coalesced_timer_update(s);
+        }
+    }
+}
+
+static void arch_rtc_notify_clock_reset(RTCState *s)
+{
+    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+        rtc_coalesced_timer_update(s);
+    }
+}
+
+static void arch_rtc_reset(RTCState *s)
+{
+    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+        s->irq_coalesced = 0;
+        s->irq_reinject_on_ack_count = 0;
+    }
+}
+
+static bool arch_rtc_realizefn(RTCState *s, Error **errp)
+{
+    switch (s->lost_tick_policy) {
+    case LOST_TICK_POLICY_SLEW:
+        s->coalesced_timer =
+            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
+        break;
+    case LOST_TICK_POLICY_DISCARD:
+        break;
+    default:
+        error_setg(errp, "Invalid lost tick policy.");
+        return false;
+    }
+
+    return true;
+}
 #else
 static int64_t
 arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
@@ -201,6 +290,32 @@ arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
 static void arch_periodic_timer_disable(RTCState *s)
 {
 }
+
+static void arch_rtc_periodic_timer(RTCState *s)
+{
+    qemu_irq_raise(s->irq);
+}
+
+static void arch_read_regC(RTCState *s)
+{
+}
+
+static void arch_rtc_post_load(RTCState *s, int version_id)
+{
+}
+
+static void arch_rtc_notify_clock_reset(RTCState *s)
+{
+}
+
+static void arch_rtc_reset(RTCState *s)
+{
+}
+
+static bool arch_rtc_realizefn(RTCState *s, Error **errp)
+{
+    return true;
+}
 #endif
 
 static int period_code_to_clock(int period_code)
@@ -287,21 +402,7 @@ 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_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);
+        arch_rtc_periodic_timer(s);
     }
 }
 
@@ -656,20 +757,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;
@@ -789,22 +876,8 @@ 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()) {
-                    s->irq_coalesced--;
-                    DPRINTF_C("cmos: coalesced irqs decreased to %d\n",
-                              s->irq_coalesced);
-                }
-            }
-#endif
+
+            arch_read_regC(s);
             break;
         default:
             ret = s->cmos_data[s->cmos_index];
@@ -874,13 +947,7 @@ 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
+    arch_rtc_post_load(s, version_id);
     return 0;
 }
 
@@ -937,11 +1004,7 @@ 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
+    arch_rtc_notify_clock_reset(s);
 }
 
 /* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
@@ -961,13 +1024,7 @@ static void rtc_reset(void *opaque)
     check_update_timer(s);
 
     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
+    arch_rtc_reset(s);
 }
 
 static const MemoryRegionOps cmos_ops = {
@@ -1013,19 +1070,9 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 
     rtc_set_date_from_host(isadev);
 
-#ifdef TARGET_I386
-    switch (s->lost_tick_policy) {
-    case LOST_TICK_POLICY_SLEW:
-        s->coalesced_timer =
-            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
-        break;
-    case LOST_TICK_POLICY_DISCARD:
-        break;
-    default:
-        error_setg(errp, "Invalid lost tick policy.");
+    if (!arch_rtc_realizefn(s, errp)) {
         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

Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
> 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

Very interesting.

However, I think that the above should be exactly how the RTC should
work.  The original RTC circuit had 22 divider stages (see page 13 of
the datasheet[1], at the bottom right), and the periodic interrupt taps
the rising edge of one of the dividers (page 16, second paragraph).  The
datasheet also never mentions a comparator being used to trigger the
periodic interrupts.

Have you checked that this Windows bug doesn't happen on real hardware
too?  Or is the combination of driftfix=slew and changing periods that
is a problem?

The series also does more than this fix (or workaround), so I will
review it anyway.

[1] http://www.nxp.com/assets/documents/data/en/data-sheets/MC146818.pdf

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
> 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

Very interesting.

However, I think that the above should be exactly how the RTC should
work.  The original RTC circuit had 22 divider stages (see page 13 of
the datasheet[1], at the bottom right), and the periodic interrupt taps
the rising edge of one of the dividers (page 16, second paragraph).  The
datasheet also never mentions a comparator being used to trigger the
periodic interrupts.

Have you checked that this Windows bug doesn't happen on real hardware
too?  Or is the combination of driftfix=slew and changing periods that
is a problem?

The series also does more than this fix (or workaround), so I will
review it anyway.

[1] http://www.nxp.com/assets/documents/data/en/data-sheets/MC146818.pdf

Thanks,

Paolo

Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>
>
> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>> 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
>
> Very interesting.
>

Yes, indeed.

> However, I think that the above should be exactly how the RTC should
> work.  The original RTC circuit had 22 divider stages (see page 13 of
> the datasheet[1], at the bottom right), and the periodic interrupt taps
> the rising edge of one of the dividers (page 16, second paragraph).  The
> datasheet also never mentions a comparator being used to trigger the
> periodic interrupts.
>

That was my thought before, however, after more test, i am not sure if
re-configuring RegA changes these divider stages internal...

> Have you checked that this Windows bug doesn't happen on real hardware
> too?  Or is the combination of driftfix=slew and changing periods that
> is a problem?
>

I have two physical windows 7 machines, both of them have
'useplatformclock = off' and ntp disabled, the wall time is really
accurate. The difference is that the physical machines are using Intel
Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
issue is easily be reproduced just in ~10 mins.

Our test mostly focus on 'driftfix=slew' and after this patchset the
time is accurate and stable.

I will do the test for dropping 'slew' and see what will happen...

> The series also does more than this fix (or workaround), so I will
> review it anyway.
>

Thank you, Paolo!

Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>
>
> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>> 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
>
> Very interesting.
>

Yes, indeed.

> However, I think that the above should be exactly how the RTC should
> work.  The original RTC circuit had 22 divider stages (see page 13 of
> the datasheet[1], at the bottom right), and the periodic interrupt taps
> the rising edge of one of the dividers (page 16, second paragraph).  The
> datasheet also never mentions a comparator being used to trigger the
> periodic interrupts.
>

That was my thought before, however, after more test, i am not sure if
re-configuring RegA changes these divider stages internal...

> Have you checked that this Windows bug doesn't happen on real hardware
> too?  Or is the combination of driftfix=slew and changing periods that
> is a problem?
>

I have two physical windows 7 machines, both of them have
'useplatformclock = off' and ntp disabled, the wall time is really
accurate. The difference is that the physical machines are using Intel
Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
issue is easily be reproduced just in ~10 mins.

Our test mostly focus on 'driftfix=slew' and after this patchset the
time is accurate and stable.

I will do the test for dropping 'slew' and see what will happen...

> The series also does more than this fix (or workaround), so I will
> review it anyway.
>

Thank you, Paolo!

Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>
>
> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>> 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
>>
>> Very interesting.
>>
>
> Yes, indeed.
>
>> However, I think that the above should be exactly how the RTC should
>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>> the datasheet[1], at the bottom right), and the periodic interrupt taps
>> the rising edge of one of the dividers (page 16, second paragraph).  The
>> datasheet also never mentions a comparator being used to trigger the
>> periodic interrupts.
>>
>
> That was my thought before, however, after more test, i am not sure if
> re-configuring RegA changes these divider stages internal...
>
>> Have you checked that this Windows bug doesn't happen on real hardware
>> too?  Or is the combination of driftfix=slew and changing periods that
>> is a problem?
>>
>
> I have two physical windows 7 machines, both of them have
> 'useplatformclock = off' and ntp disabled, the wall time is really
> accurate. The difference is that the physical machines are using Intel
> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
> issue is easily be reproduced just in ~10 mins.
>
> Our test mostly focus on 'driftfix=slew' and after this patchset the
> time is accurate and stable.
>
> I will do the test for dropping 'slew' and see what will happen...
>

Well, the time is easily observed to be faster if 'driftfix=slew' is
not used. :(

Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>
>
> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>> 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
>>
>> Very interesting.
>>
>
> Yes, indeed.
>
>> However, I think that the above should be exactly how the RTC should
>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>> the datasheet[1], at the bottom right), and the periodic interrupt taps
>> the rising edge of one of the dividers (page 16, second paragraph).  The
>> datasheet also never mentions a comparator being used to trigger the
>> periodic interrupts.
>>
>
> That was my thought before, however, after more test, i am not sure if
> re-configuring RegA changes these divider stages internal...
>
>> Have you checked that this Windows bug doesn't happen on real hardware
>> too?  Or is the combination of driftfix=slew and changing periods that
>> is a problem?
>>
>
> I have two physical windows 7 machines, both of them have
> 'useplatformclock = off' and ntp disabled, the wall time is really
> accurate. The difference is that the physical machines are using Intel
> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
> issue is easily be reproduced just in ~10 mins.
>
> Our test mostly focus on 'driftfix=slew' and after this patchset the
> time is accurate and stable.
>
> I will do the test for dropping 'slew' and see what will happen...
>

Well, the time is easily observed to be faster if 'driftfix=slew' is
not used. :(

答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Zhanghailiang]

Hi,

-----邮件原件-----
发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代表 Xiao Guangrong
发送时间: 2017年4月13日 16:53
收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org; yunfangtai@tencent.com; Xiao Guangrong
主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster



On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>
>
> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>> 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
>>
>> Very interesting.
>>
>
> Yes, indeed.
>
>> However, I think that the above should be exactly how the RTC should 
>> work.  The original RTC circuit had 22 divider stages (see page 13 of 
>> the datasheet[1], at the bottom right), and the periodic interrupt 
>> taps the rising edge of one of the dividers (page 16, second 
>> paragraph).  The datasheet also never mentions a comparator being 
>> used to trigger the periodic interrupts.
>>
>
> That was my thought before, however, after more test, i am not sure if 
> re-configuring RegA changes these divider stages internal...
>
>> Have you checked that this Windows bug doesn't happen on real 
>> hardware too?  Or is the combination of driftfix=slew and changing 
>> periods that is a problem?
>>
>
> I have two physical windows 7 machines, both of them have 
> 'useplatformclock = off' and ntp disabled, the wall time is really 
> accurate. The difference is that the physical machines are using Intel
> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the 
> issue is easily be reproduced just in ~10 mins.
>
> Our test mostly focus on 'driftfix=slew' and after this patchset the 
> time is accurate and stable.
>
> I will do the test for dropping 'slew' and see what will happen...
>

> Well, the time is easily observed to be faster if 'driftfix=slew' is not used. :(

You mean, it only fixes the one case which with the ' driftfix=slew ' is used ?
We encountered this problem too, I have tried to fix it long time ago.  
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
(It seems that your solution is more useful)
But it seems that it is impossible to fix, we need to emulate the behaviors of real hardware, 
but we didn't find any clear description about it. And it seems that other virtualization platforms
have this problem too:
VMware:
https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
Heper-v:
https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/


Hailiang

[Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Zhanghailiang]

Hi,

-----邮件原件-----
发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代表 Xiao Guangrong
发送时间: 2017年4月13日 16:53
收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org; yunfangtai@tencent.com; Xiao Guangrong
主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster



On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>
>
> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>> 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
>>
>> Very interesting.
>>
>
> Yes, indeed.
>
>> However, I think that the above should be exactly how the RTC should 
>> work.  The original RTC circuit had 22 divider stages (see page 13 of 
>> the datasheet[1], at the bottom right), and the periodic interrupt 
>> taps the rising edge of one of the dividers (page 16, second 
>> paragraph).  The datasheet also never mentions a comparator being 
>> used to trigger the periodic interrupts.
>>
>
> That was my thought before, however, after more test, i am not sure if 
> re-configuring RegA changes these divider stages internal...
>
>> Have you checked that this Windows bug doesn't happen on real 
>> hardware too?  Or is the combination of driftfix=slew and changing 
>> periods that is a problem?
>>
>
> I have two physical windows 7 machines, both of them have 
> 'useplatformclock = off' and ntp disabled, the wall time is really 
> accurate. The difference is that the physical machines are using Intel
> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the 
> issue is easily be reproduced just in ~10 mins.
>
> Our test mostly focus on 'driftfix=slew' and after this patchset the 
> time is accurate and stable.
>
> I will do the test for dropping 'slew' and see what will happen...
>

> Well, the time is easily observed to be faster if 'driftfix=slew' is not used. :(

You mean, it only fixes the one case which with the ' driftfix=slew ' is used ?
We encountered this problem too, I have tried to fix it long time ago.  
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
(It seems that your solution is more useful)
But it seems that it is impossible to fix, we need to emulate the behaviors of real hardware, 
but we didn't find any clear description about it. And it seems that other virtualization platforms
have this problem too:
VMware:
https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
Heper-v:
https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/


Hailiang

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 05:05 PM, Zhanghailiang wrote:
> Hi,
>
> -----邮件原件-----
> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代表 Xiao Guangrong
> 发送时间: 2017年4月13日 16:53
> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org; yunfangtai@tencent.com; Xiao Guangrong
> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>
>
>
> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>
>>
>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>> 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
>>>
>>> Very interesting.
>>>
>>
>> Yes, indeed.
>>
>>> However, I think that the above should be exactly how the RTC should
>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>> taps the rising edge of one of the dividers (page 16, second
>>> paragraph).  The datasheet also never mentions a comparator being
>>> used to trigger the periodic interrupts.
>>>
>>
>> That was my thought before, however, after more test, i am not sure if
>> re-configuring RegA changes these divider stages internal...
>>
>>> Have you checked that this Windows bug doesn't happen on real
>>> hardware too?  Or is the combination of driftfix=slew and changing
>>> periods that is a problem?
>>>
>>
>> I have two physical windows 7 machines, both of them have
>> 'useplatformclock = off' and ntp disabled, the wall time is really
>> accurate. The difference is that the physical machines are using Intel
>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>> issue is easily be reproduced just in ~10 mins.
>>
>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>> time is accurate and stable.
>>
>> I will do the test for dropping 'slew' and see what will happen...
>>
>
>> Well, the time is easily observed to be faster if 'driftfix=slew' is not used. :(
>
> You mean, it only fixes the one case which with the ' driftfix=slew ' is used ?

No. for both.

> We encountered this problem too, I have tried to fix it long time ago.
> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
> (It seems that your solution is more useful)
> But it seems that it is impossible to fix, we need to emulate the behaviors of real hardware,
> but we didn't find any clear description about it. And it seems that other virtualization platforms

That is the issue, the hardware spec does not detail how the clock is
counted when the timer interval is changed. What we can do at this time
is that speculate it from the behaviors. Current RTC is completely
unusable anyway.


> have this problem too:
> VMware:
> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
> Heper-v:
> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/

Hmm, slower clock is understandable, does really the Windows7 on hyperV
have faster clock? Did you meet it?

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 05:05 PM, Zhanghailiang wrote:
> Hi,
>
> -----邮件原件-----
> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代表 Xiao Guangrong
> 发送时间: 2017年4月13日 16:53
> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org; yunfangtai@tencent.com; Xiao Guangrong
> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>
>
>
> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>
>>
>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>> 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
>>>
>>> Very interesting.
>>>
>>
>> Yes, indeed.
>>
>>> However, I think that the above should be exactly how the RTC should
>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>> taps the rising edge of one of the dividers (page 16, second
>>> paragraph).  The datasheet also never mentions a comparator being
>>> used to trigger the periodic interrupts.
>>>
>>
>> That was my thought before, however, after more test, i am not sure if
>> re-configuring RegA changes these divider stages internal...
>>
>>> Have you checked that this Windows bug doesn't happen on real
>>> hardware too?  Or is the combination of driftfix=slew and changing
>>> periods that is a problem?
>>>
>>
>> I have two physical windows 7 machines, both of them have
>> 'useplatformclock = off' and ntp disabled, the wall time is really
>> accurate. The difference is that the physical machines are using Intel
>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>> issue is easily be reproduced just in ~10 mins.
>>
>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>> time is accurate and stable.
>>
>> I will do the test for dropping 'slew' and see what will happen...
>>
>
>> Well, the time is easily observed to be faster if 'driftfix=slew' is not used. :(
>
> You mean, it only fixes the one case which with the ' driftfix=slew ' is used ?

No. for both.

> We encountered this problem too, I have tried to fix it long time ago.
> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
> (It seems that your solution is more useful)
> But it seems that it is impossible to fix, we need to emulate the behaviors of real hardware,
> but we didn't find any clear description about it. And it seems that other virtualization platforms

That is the issue, the hardware spec does not detail how the clock is
counted when the timer interval is changed. What we can do at this time
is that speculate it from the behaviors. Current RTC is completely
unusable anyway.


> have this problem too:
> VMware:
> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
> Heper-v:
> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/

Hmm, slower clock is understandable, does really the Windows7 on hyperV
have faster clock? Did you meet it?

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Hailiang Zhang]

On 2017/4/13 17:18, Xiao Guangrong wrote:
>
> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>> Hi,
>>
>> -----邮件原件-----
>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代表 Xiao Guangrong
>> 发送时间: 2017年4月13日 16:53
>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org; yunfangtai@tencent.com; Xiao Guangrong
>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>
>>
>>
>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>
>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>
>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>> 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
>>>> Very interesting.
>>>>
>>> Yes, indeed.
>>>
>>>> However, I think that the above should be exactly how the RTC should
>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>> taps the rising edge of one of the dividers (page 16, second
>>>> paragraph).  The datasheet also never mentions a comparator being
>>>> used to trigger the periodic interrupts.
>>>>
>>> That was my thought before, however, after more test, i am not sure if
>>> re-configuring RegA changes these divider stages internal...
>>>
>>>> Have you checked that this Windows bug doesn't happen on real
>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>> periods that is a problem?
>>>>
>>> I have two physical windows 7 machines, both of them have
>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>> accurate. The difference is that the physical machines are using Intel
>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>> issue is easily be reproduced just in ~10 mins.
>>>
>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>> time is accurate and stable.
>>>
>>> I will do the test for dropping 'slew' and see what will happen...
>>>
>>> Well, the time is easily observed to be faster if 'driftfix=slew' is not used. :(
>> You mean, it only fixes the one case which with the ' driftfix=slew ' is used ?
> No. for both.
>
>> We encountered this problem too, I have tried to fix it long time ago.
>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>> (It seems that your solution is more useful)
>> But it seems that it is impossible to fix, we need to emulate the behaviors of real hardware,
>> but we didn't find any clear description about it. And it seems that other virtualization platforms
> That is the issue, the hardware spec does not detail how the clock is
> counted when the timer interval is changed. What we can do at this time
> is that speculate it from the behaviors. Current RTC is completely
> unusable anyway.
>
>
>> have this problem too:
>> VMware:
>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>> Heper-v:
>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
> Hmm, slower clock is understandable, does really the Windows7 on hyperV
> have faster clock? Did you meet it?

I don't know, we didn't test it, besides, I'd like to know how long did your testcase run before
you judge it is stable with 'driftfix=slew'  option? (My previous patch can't fix it completely but
only narrows the gap between timer in guest and real timer.)

Hailiang
>
> .
>

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Hailiang Zhang]

On 2017/4/13 17:18, Xiao Guangrong wrote:
>
> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>> Hi,
>>
>> -----邮件原件-----
>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代表 Xiao Guangrong
>> 发送时间: 2017年4月13日 16:53
>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org; yunfangtai@tencent.com; Xiao Guangrong
>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>
>>
>>
>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>
>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>
>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>> 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
>>>> Very interesting.
>>>>
>>> Yes, indeed.
>>>
>>>> However, I think that the above should be exactly how the RTC should
>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>> taps the rising edge of one of the dividers (page 16, second
>>>> paragraph).  The datasheet also never mentions a comparator being
>>>> used to trigger the periodic interrupts.
>>>>
>>> That was my thought before, however, after more test, i am not sure if
>>> re-configuring RegA changes these divider stages internal...
>>>
>>>> Have you checked that this Windows bug doesn't happen on real
>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>> periods that is a problem?
>>>>
>>> I have two physical windows 7 machines, both of them have
>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>> accurate. The difference is that the physical machines are using Intel
>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>> issue is easily be reproduced just in ~10 mins.
>>>
>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>> time is accurate and stable.
>>>
>>> I will do the test for dropping 'slew' and see what will happen...
>>>
>>> Well, the time is easily observed to be faster if 'driftfix=slew' is not used. :(
>> You mean, it only fixes the one case which with the ' driftfix=slew ' is used ?
> No. for both.
>
>> We encountered this problem too, I have tried to fix it long time ago.
>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>> (It seems that your solution is more useful)
>> But it seems that it is impossible to fix, we need to emulate the behaviors of real hardware,
>> but we didn't find any clear description about it. And it seems that other virtualization platforms
> That is the issue, the hardware spec does not detail how the clock is
> counted when the timer interval is changed. What we can do at this time
> is that speculate it from the behaviors. Current RTC is completely
> unusable anyway.
>
>
>> have this problem too:
>> VMware:
>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>> Heper-v:
>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
> Hmm, slower clock is understandable, does really the Windows7 on hyperV
> have faster clock? Did you meet it?

I don't know, we didn't test it, besides, I'd like to know how long did your testcase run before
you judge it is stable with 'driftfix=slew'  option? (My previous patch can't fix it completely but
only narrows the gap between timer in guest and real timer.)

Hailiang
>
> .
>

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>
>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>> Hi,
>>>
>>> -----邮件原件-----
>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>> 代表 Xiao Guangrong
>>> 发送时间: 2017年4月13日 16:53
>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>> yunfangtai@tencent.com; Xiao Guangrong
>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>
>>>
>>>
>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>
>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>
>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>> 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
>>>>> Very interesting.
>>>>>
>>>> Yes, indeed.
>>>>
>>>>> However, I think that the above should be exactly how the RTC should
>>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>> used to trigger the periodic interrupts.
>>>>>
>>>> That was my thought before, however, after more test, i am not sure if
>>>> re-configuring RegA changes these divider stages internal...
>>>>
>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>> periods that is a problem?
>>>>>
>>>> I have two physical windows 7 machines, both of them have
>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>> accurate. The difference is that the physical machines are using Intel
>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>> issue is easily be reproduced just in ~10 mins.
>>>>
>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>> time is accurate and stable.
>>>>
>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>
>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>> not used. :(
>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>> is used ?
>> No. for both.
>>
>>> We encountered this problem too, I have tried to fix it long time ago.
>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>> (It seems that your solution is more useful)
>>> But it seems that it is impossible to fix, we need to emulate the
>>> behaviors of real hardware,
>>> but we didn't find any clear description about it. And it seems that
>>> other virtualization platforms
>> That is the issue, the hardware spec does not detail how the clock is
>> counted when the timer interval is changed. What we can do at this time
>> is that speculate it from the behaviors. Current RTC is completely
>> unusable anyway.
>>
>>
>>> have this problem too:
>>> VMware:
>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>> Heper-v:
>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>
>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>> have faster clock? Did you meet it?
>
> I don't know, we didn't test it, besides, I'd like to know how long did
> your testcase run before
> you judge it is stable with 'driftfix=slew'  option? (My previous patch
> can't fix it completely but
> only narrows the gap between timer in guest and real timer.)

More than 12 hours.

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>
>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>> Hi,
>>>
>>> -----邮件原件-----
>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>> 代表 Xiao Guangrong
>>> 发送时间: 2017年4月13日 16:53
>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>> yunfangtai@tencent.com; Xiao Guangrong
>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>
>>>
>>>
>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>
>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>
>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>> 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
>>>>> Very interesting.
>>>>>
>>>> Yes, indeed.
>>>>
>>>>> However, I think that the above should be exactly how the RTC should
>>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>> used to trigger the periodic interrupts.
>>>>>
>>>> That was my thought before, however, after more test, i am not sure if
>>>> re-configuring RegA changes these divider stages internal...
>>>>
>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>> periods that is a problem?
>>>>>
>>>> I have two physical windows 7 machines, both of them have
>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>> accurate. The difference is that the physical machines are using Intel
>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>> issue is easily be reproduced just in ~10 mins.
>>>>
>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>> time is accurate and stable.
>>>>
>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>
>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>> not used. :(
>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>> is used ?
>> No. for both.
>>
>>> We encountered this problem too, I have tried to fix it long time ago.
>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>> (It seems that your solution is more useful)
>>> But it seems that it is impossible to fix, we need to emulate the
>>> behaviors of real hardware,
>>> but we didn't find any clear description about it. And it seems that
>>> other virtualization platforms
>> That is the issue, the hardware spec does not detail how the clock is
>> counted when the timer interval is changed. What we can do at this time
>> is that speculate it from the behaviors. Current RTC is completely
>> unusable anyway.
>>
>>
>>> have this problem too:
>>> VMware:
>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>> Heper-v:
>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>
>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>> have faster clock? Did you meet it?
>
> I don't know, we didn't test it, besides, I'd like to know how long did
> your testcase run before
> you judge it is stable with 'driftfix=slew'  option? (My previous patch
> can't fix it completely but
> only narrows the gap between timer in guest and real timer.)

More than 12 hours.

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Hailiang Zhang]

On 2017/4/13 17:35, Xiao Guangrong wrote:
>
> On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
>> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>>> Hi,
>>>>
>>>> -----邮件原件-----
>>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>> 代表 Xiao Guangrong
>>>> 发送时间: 2017年4月13日 16:53
>>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>>> yunfangtai@tencent.com; Xiao Guangrong
>>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>>
>>>>
>>>>
>>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>>> 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
>>>>>> Very interesting.
>>>>>>
>>>>> Yes, indeed.
>>>>>
>>>>>> However, I think that the above should be exactly how the RTC should
>>>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>>> used to trigger the periodic interrupts.
>>>>>>
>>>>> That was my thought before, however, after more test, i am not sure if
>>>>> re-configuring RegA changes these divider stages internal...
>>>>>
>>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>>> periods that is a problem?
>>>>>>
>>>>> I have two physical windows 7 machines, both of them have
>>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>>> accurate. The difference is that the physical machines are using Intel
>>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>>> issue is easily be reproduced just in ~10 mins.
>>>>>
>>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>>> time is accurate and stable.
>>>>>
>>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>>
>>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>>> not used. :(
>>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>>> is used ?
>>> No. for both.
>>>
>>>> We encountered this problem too, I have tried to fix it long time ago.
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>>> (It seems that your solution is more useful)
>>>> But it seems that it is impossible to fix, we need to emulate the
>>>> behaviors of real hardware,
>>>> but we didn't find any clear description about it. And it seems that
>>>> other virtualization platforms
>>> That is the issue, the hardware spec does not detail how the clock is
>>> counted when the timer interval is changed. What we can do at this time
>>> is that speculate it from the behaviors. Current RTC is completely
>>> unusable anyway.
>>>
>>>
>>>> have this problem too:
>>>> VMware:
>>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>>> Heper-v:
>>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>>
>>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>>> have faster clock? Did you meet it?
>> I don't know, we didn't test it, besides, I'd like to know how long did
>> your testcase run before
>> you judge it is stable with 'driftfix=slew'  option? (My previous patch
>> can't fix it completely but
>> only narrows the gap between timer in guest and real timer.)
> More than 12 hours.

Great, I'll test and look into it ... thanks.

>
>
> .
>

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Hailiang Zhang]

On 2017/4/13 17:35, Xiao Guangrong wrote:
>
> On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
>> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>>> Hi,
>>>>
>>>> -----邮件原件-----
>>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>> 代表 Xiao Guangrong
>>>> 发送时间: 2017年4月13日 16:53
>>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>>> yunfangtai@tencent.com; Xiao Guangrong
>>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>>
>>>>
>>>>
>>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>>> 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
>>>>>> Very interesting.
>>>>>>
>>>>> Yes, indeed.
>>>>>
>>>>>> However, I think that the above should be exactly how the RTC should
>>>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>>> used to trigger the periodic interrupts.
>>>>>>
>>>>> That was my thought before, however, after more test, i am not sure if
>>>>> re-configuring RegA changes these divider stages internal...
>>>>>
>>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>>> periods that is a problem?
>>>>>>
>>>>> I have two physical windows 7 machines, both of them have
>>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>>> accurate. The difference is that the physical machines are using Intel
>>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>>> issue is easily be reproduced just in ~10 mins.
>>>>>
>>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>>> time is accurate and stable.
>>>>>
>>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>>
>>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>>> not used. :(
>>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>>> is used ?
>>> No. for both.
>>>
>>>> We encountered this problem too, I have tried to fix it long time ago.
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>>> (It seems that your solution is more useful)
>>>> But it seems that it is impossible to fix, we need to emulate the
>>>> behaviors of real hardware,
>>>> but we didn't find any clear description about it. And it seems that
>>>> other virtualization platforms
>>> That is the issue, the hardware spec does not detail how the clock is
>>> counted when the timer interval is changed. What we can do at this time
>>> is that speculate it from the behaviors. Current RTC is completely
>>> unusable anyway.
>>>
>>>
>>>> have this problem too:
>>>> VMware:
>>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>>> Heper-v:
>>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>>
>>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>>> have faster clock? Did you meet it?
>> I don't know, we didn't test it, besides, I'd like to know how long did
>> your testcase run before
>> you judge it is stable with 'driftfix=slew'  option? (My previous patch
>> can't fix it completely but
>> only narrows the gap between timer in guest and real timer.)
> More than 12 hours.

Great, I'll test and look into it ... thanks.

>
>
> .
>

Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 13/04/2017 16:52, Xiao Guangrong wrote:
> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>> However, I think that the above should be exactly how the RTC should
>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>> the datasheet[1], at the bottom right), and the periodic interrupt taps
>>> the rising edge of one of the dividers (page 16, second paragraph).  The
>>> datasheet also never mentions a comparator being used to trigger the
>>> periodic interrupts.
>>>
>>
>> That was my thought before, however, after more test, i am not sure if
>> re-configuring RegA changes these divider stages internal...

It's unlikely because there is a separate divider reset command.  But
Hailiang found the same problem, and Bochs does the same implementation
as you.

It's even possible (not sure how likely) that the original MC146818 RTC
had the bug, but recent Super I/O chips fixed it to work around the
problem with Windows.

Thanks,

Paolo

>>> Have you checked that this Windows bug doesn't happen on real hardware
>>> too?  Or is the combination of driftfix=slew and changing periods that
>>> is a problem?
>>>
>>
>> I have two physical windows 7 machines, both of them have
>> 'useplatformclock = off' and ntp disabled, the wall time is really
>> accurate. The difference is that the physical machines are using Intel
>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>> issue is easily be reproduced just in ~10 mins.
>>
>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>> time is accurate and stable.
>>
>> I will do the test for dropping 'slew' and see what will happen...
>>
> 
> Well, the time is easily observed to be faster if 'driftfix=slew' is
> not used. :(
> 

Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 13/04/2017 16:52, Xiao Guangrong wrote:
> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>> However, I think that the above should be exactly how the RTC should
>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>> the datasheet[1], at the bottom right), and the periodic interrupt taps
>>> the rising edge of one of the dividers (page 16, second paragraph).  The
>>> datasheet also never mentions a comparator being used to trigger the
>>> periodic interrupts.
>>>
>>
>> That was my thought before, however, after more test, i am not sure if
>> re-configuring RegA changes these divider stages internal...

It's unlikely because there is a separate divider reset command.  But
Hailiang found the same problem, and Bochs does the same implementation
as you.

It's even possible (not sure how likely) that the original MC146818 RTC
had the bug, but recent Super I/O chips fixed it to work around the
problem with Windows.

Thanks,

Paolo

>>> Have you checked that this Windows bug doesn't happen on real hardware
>>> too?  Or is the combination of driftfix=slew and changing periods that
>>> is a problem?
>>>
>>
>> I have two physical windows 7 machines, both of them have
>> 'useplatformclock = off' and ntp disabled, the wall time is really
>> accurate. The difference is that the physical machines are using Intel
>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>> issue is easily be reproduced just in ~10 mins.
>>
>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>> time is accurate and stable.
>>
>> I will do the test for dropping 'slew' and see what will happen...
>>
> 
> Well, the time is easily observed to be faster if 'driftfix=slew' is
> not used. :(
> 

Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/14/2017 01:09 PM, Paolo Bonzini wrote:
>
>
> On 13/04/2017 16:52, Xiao Guangrong wrote:
>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>> However, I think that the above should be exactly how the RTC should
>>>> work.  The original RTC circuit had 22 divider stages (see page 13 of
>>>> the datasheet[1], at the bottom right), and the periodic interrupt taps
>>>> the rising edge of one of the dividers (page 16, second paragraph).  The
>>>> datasheet also never mentions a comparator being used to trigger the
>>>> periodic interrupts.
>>>>
>>>
>>> That was my thought before, however, after more test, i am not sure if
>>> re-configuring RegA changes these divider stages internal...
>
> It's unlikely because there is a separate divider reset command.  But
> Hailiang found the same problem, and Bochs does the same implementation
> as you.
>

Happy to see the same approach is being used in Bochs. Thanks for your
check. :)

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 05:38 PM, Hailiang Zhang wrote:
> On 2017/4/13 17:35, Xiao Guangrong wrote:
>>
>> On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
>>> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>>>> Hi,
>>>>>
>>>>> -----邮件原件-----
>>>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>>> 代表 Xiao Guangrong
>>>>> 发送时间: 2017年4月13日 16:53
>>>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>>>> yunfangtai@tencent.com; Xiao Guangrong
>>>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>>>
>>>>>
>>>>>
>>>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>>>> 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
>>>>>>> Very interesting.
>>>>>>>
>>>>>> Yes, indeed.
>>>>>>
>>>>>>> However, I think that the above should be exactly how the RTC should
>>>>>>> work.  The original RTC circuit had 22 divider stages (see page
>>>>>>> 13 of
>>>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>>>> used to trigger the periodic interrupts.
>>>>>>>
>>>>>> That was my thought before, however, after more test, i am not
>>>>>> sure if
>>>>>> re-configuring RegA changes these divider stages internal...
>>>>>>
>>>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>>>> periods that is a problem?
>>>>>>>
>>>>>> I have two physical windows 7 machines, both of them have
>>>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>>>> accurate. The difference is that the physical machines are using
>>>>>> Intel
>>>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>>>> issue is easily be reproduced just in ~10 mins.
>>>>>>
>>>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>>>> time is accurate and stable.
>>>>>>
>>>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>>>
>>>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>>>> not used. :(
>>>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>>>> is used ?
>>>> No. for both.
>>>>
>>>>> We encountered this problem too, I have tried to fix it long time ago.
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>>>> (It seems that your solution is more useful)
>>>>> But it seems that it is impossible to fix, we need to emulate the
>>>>> behaviors of real hardware,
>>>>> but we didn't find any clear description about it. And it seems that
>>>>> other virtualization platforms
>>>> That is the issue, the hardware spec does not detail how the clock is
>>>> counted when the timer interval is changed. What we can do at this time
>>>> is that speculate it from the behaviors. Current RTC is completely
>>>> unusable anyway.
>>>>
>>>>
>>>>> have this problem too:
>>>>> VMware:
>>>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>>>> Heper-v:
>>>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>>>
>>>>>
>>>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>>>> have faster clock? Did you meet it?
>>> I don't know, we didn't test it, besides, I'd like to know how long did
>>> your testcase run before
>>> you judge it is stable with 'driftfix=slew'  option? (My previous patch
>>> can't fix it completely but
>>> only narrows the gap between timer in guest and real timer.)
>> More than 12 hours.
>
> Great, I'll test and look into it ... thanks.
>

Hi Hailiang,

Does this patchset work for you? :)

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/13/2017 05:38 PM, Hailiang Zhang wrote:
> On 2017/4/13 17:35, Xiao Guangrong wrote:
>>
>> On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
>>> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>>>> Hi,
>>>>>
>>>>> -----邮件原件-----
>>>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>>> 代表 Xiao Guangrong
>>>>> 发送时间: 2017年4月13日 16:53
>>>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>>>> yunfangtai@tencent.com; Xiao Guangrong
>>>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>>>
>>>>>
>>>>>
>>>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>>>> 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
>>>>>>> Very interesting.
>>>>>>>
>>>>>> Yes, indeed.
>>>>>>
>>>>>>> However, I think that the above should be exactly how the RTC should
>>>>>>> work.  The original RTC circuit had 22 divider stages (see page
>>>>>>> 13 of
>>>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>>>> used to trigger the periodic interrupts.
>>>>>>>
>>>>>> That was my thought before, however, after more test, i am not
>>>>>> sure if
>>>>>> re-configuring RegA changes these divider stages internal...
>>>>>>
>>>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>>>> periods that is a problem?
>>>>>>>
>>>>>> I have two physical windows 7 machines, both of them have
>>>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>>>> accurate. The difference is that the physical machines are using
>>>>>> Intel
>>>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>>>> issue is easily be reproduced just in ~10 mins.
>>>>>>
>>>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>>>> time is accurate and stable.
>>>>>>
>>>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>>>
>>>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>>>> not used. :(
>>>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>>>> is used ?
>>>> No. for both.
>>>>
>>>>> We encountered this problem too, I have tried to fix it long time ago.
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>>>> (It seems that your solution is more useful)
>>>>> But it seems that it is impossible to fix, we need to emulate the
>>>>> behaviors of real hardware,
>>>>> but we didn't find any clear description about it. And it seems that
>>>>> other virtualization platforms
>>>> That is the issue, the hardware spec does not detail how the clock is
>>>> counted when the timer interval is changed. What we can do at this time
>>>> is that speculate it from the behaviors. Current RTC is completely
>>>> unusable anyway.
>>>>
>>>>
>>>>> have this problem too:
>>>>> VMware:
>>>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>>>> Heper-v:
>>>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>>>
>>>>>
>>>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>>>> have faster clock? Did you meet it?
>>> I don't know, we didn't test it, besides, I'd like to know how long did
>>> your testcase run before
>>> you judge it is stable with 'driftfix=slew'  option? (My previous patch
>>> can't fix it completely but
>>> only narrows the gap between timer in guest and real timer.)
>> More than 12 hours.
>
> Great, I'll test and look into it ... thanks.
>

Hi Hailiang,

Does this patchset work for you? :)

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Hailiang Zhang]

On 2017/4/19 10:02, Xiao Guangrong wrote:
>
> On 04/13/2017 05:38 PM, Hailiang Zhang wrote:
>> On 2017/4/13 17:35, Xiao Guangrong wrote:
>>> On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
>>>> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>>>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> -----邮件原件-----
>>>>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>>>> 代表 Xiao Guangrong
>>>>>> 发送时间: 2017年4月13日 16:53
>>>>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>>>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>>>>> yunfangtai@tencent.com; Xiao Guangrong
>>>>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>>>>> 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
>>>>>>>> Very interesting.
>>>>>>>>
>>>>>>> Yes, indeed.
>>>>>>>
>>>>>>>> However, I think that the above should be exactly how the RTC should
>>>>>>>> work.  The original RTC circuit had 22 divider stages (see page
>>>>>>>> 13 of
>>>>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>>>>> used to trigger the periodic interrupts.
>>>>>>>>
>>>>>>> That was my thought before, however, after more test, i am not
>>>>>>> sure if
>>>>>>> re-configuring RegA changes these divider stages internal...
>>>>>>>
>>>>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>>>>> periods that is a problem?
>>>>>>>>
>>>>>>> I have two physical windows 7 machines, both of them have
>>>>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>>>>> accurate. The difference is that the physical machines are using
>>>>>>> Intel
>>>>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>>>>> issue is easily be reproduced just in ~10 mins.
>>>>>>>
>>>>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>>>>> time is accurate and stable.
>>>>>>>
>>>>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>>>>
>>>>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>>>>> not used. :(
>>>>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>>>>> is used ?
>>>>> No. for both.
>>>>>
>>>>>> We encountered this problem too, I have tried to fix it long time ago.
>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>>>>> (It seems that your solution is more useful)
>>>>>> But it seems that it is impossible to fix, we need to emulate the
>>>>>> behaviors of real hardware,
>>>>>> but we didn't find any clear description about it. And it seems that
>>>>>> other virtualization platforms
>>>>> That is the issue, the hardware spec does not detail how the clock is
>>>>> counted when the timer interval is changed. What we can do at this time
>>>>> is that speculate it from the behaviors. Current RTC is completely
>>>>> unusable anyway.
>>>>>
>>>>>
>>>>>> have this problem too:
>>>>>> VMware:
>>>>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>>>>> Heper-v:
>>>>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>>>>
>>>>>>
>>>>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>>>>> have faster clock? Did you meet it?
>>>> I don't know, we didn't test it, besides, I'd like to know how long did
>>>> your testcase run before
>>>> you judge it is stable with 'driftfix=slew'  option? (My previous patch
>>>> can't fix it completely but
>>>> only narrows the gap between timer in guest and real timer.)
>>> More than 12 hours.
>> Great, I'll test and look into it ... thanks.
>>
> Hi Hailiang,
>
> Does this patchset work for you? :)

Yes, i think it works for us, nice work :)

>
>
> .
>

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Hailiang Zhang]

On 2017/4/19 10:02, Xiao Guangrong wrote:
>
> On 04/13/2017 05:38 PM, Hailiang Zhang wrote:
>> On 2017/4/13 17:35, Xiao Guangrong wrote:
>>> On 04/13/2017 05:29 PM, Hailiang Zhang wrote:
>>>> On 2017/4/13 17:18, Xiao Guangrong wrote:
>>>>> On 04/13/2017 05:05 PM, Zhanghailiang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> -----邮件原件-----
>>>>>> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>>>> 代表 Xiao Guangrong
>>>>>> 发送时间: 2017年4月13日 16:53
>>>>>> 收件人: Paolo Bonzini; mst@redhat.com; mtosatti@redhat.com
>>>>>> 抄送: qemu-devel@nongnu.org; kvm@vger.kernel.org;
>>>>>> yunfangtai@tencent.com; Xiao Guangrong
>>>>>> 主题: Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04/13/2017 04:39 PM, Xiao Guangrong wrote:
>>>>>>> On 04/13/2017 02:37 PM, Paolo Bonzini wrote:
>>>>>>>> On 12/04/2017 17:51, guangrong.xiao@gmail.com wrote:
>>>>>>>>> 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
>>>>>>>> Very interesting.
>>>>>>>>
>>>>>>> Yes, indeed.
>>>>>>>
>>>>>>>> However, I think that the above should be exactly how the RTC should
>>>>>>>> work.  The original RTC circuit had 22 divider stages (see page
>>>>>>>> 13 of
>>>>>>>> the datasheet[1], at the bottom right), and the periodic interrupt
>>>>>>>> taps the rising edge of one of the dividers (page 16, second
>>>>>>>> paragraph).  The datasheet also never mentions a comparator being
>>>>>>>> used to trigger the periodic interrupts.
>>>>>>>>
>>>>>>> That was my thought before, however, after more test, i am not
>>>>>>> sure if
>>>>>>> re-configuring RegA changes these divider stages internal...
>>>>>>>
>>>>>>>> Have you checked that this Windows bug doesn't happen on real
>>>>>>>> hardware too?  Or is the combination of driftfix=slew and changing
>>>>>>>> periods that is a problem?
>>>>>>>>
>>>>>>> I have two physical windows 7 machines, both of them have
>>>>>>> 'useplatformclock = off' and ntp disabled, the wall time is really
>>>>>>> accurate. The difference is that the physical machines are using
>>>>>>> Intel
>>>>>>> Q87 LPC chipset which is mc146818rtc compatible. However, on VM, the
>>>>>>> issue is easily be reproduced just in ~10 mins.
>>>>>>>
>>>>>>> Our test mostly focus on 'driftfix=slew' and after this patchset the
>>>>>>> time is accurate and stable.
>>>>>>>
>>>>>>> I will do the test for dropping 'slew' and see what will happen...
>>>>>>>
>>>>>>> Well, the time is easily observed to be faster if 'driftfix=slew' is
>>>>>>> not used. :(
>>>>>> You mean, it only fixes the one case which with the ' driftfix=slew '
>>>>>> is used ?
>>>>> No. for both.
>>>>>
>>>>>> We encountered this problem too, I have tried to fix it long time ago.
>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06937.html.
>>>>>> (It seems that your solution is more useful)
>>>>>> But it seems that it is impossible to fix, we need to emulate the
>>>>>> behaviors of real hardware,
>>>>>> but we didn't find any clear description about it. And it seems that
>>>>>> other virtualization platforms
>>>>> That is the issue, the hardware spec does not detail how the clock is
>>>>> counted when the timer interval is changed. What we can do at this time
>>>>> is that speculate it from the behaviors. Current RTC is completely
>>>>> unusable anyway.
>>>>>
>>>>>
>>>>>> have this problem too:
>>>>>> VMware:
>>>>>> https://www.vmware.com/files/pdf/Timekeeping-In-VirtualMachines.pdf
>>>>>> Heper-v:
>>>>>> https://blogs.msdn.microsoft.com/virtual_pc_guy/2010/11/19/time-synchronization-in-hyper-v/
>>>>>>
>>>>>>
>>>>> Hmm, slower clock is understandable, does really the Windows7 on hyperV
>>>>> have faster clock? Did you meet it?
>>>> I don't know, we didn't test it, besides, I'd like to know how long did
>>>> your testcase run before
>>>> you judge it is stable with 'driftfix=slew'  option? (My previous patch
>>>> can't fix it completely but
>>>> only narrows the gap between timer in guest and real timer.)
>>> More than 12 hours.
>> Great, I'll test and look into it ... thanks.
>>
> Hi Hailiang,
>
> Does this patchset work for you? :)

Yes, i think it works for us, nice work :)

>
>
> .
>

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/19/2017 06:41 PM, Hailiang Zhang wrote:

>> Hi Hailiang,
>>
>> Does this patchset work for you? :)
>
> Yes, i think it works for us, nice work :)

Appreciate your test, Hailiang!

Paolo, any comment? :)

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Xiao Guangrong]

On 04/19/2017 06:41 PM, Hailiang Zhang wrote:

>> Hi Hailiang,
>>
>> Does this patchset work for you? :)
>
> Yes, i think it works for us, nice work :)

Appreciate your test, Hailiang!

Paolo, any comment? :)

Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 19/04/2017 13:13, Xiao Guangrong wrote:
> 
> 
> On 04/19/2017 06:41 PM, Hailiang Zhang wrote:
> 
>>> Hi Hailiang,
>>>
>>> Does this patchset work for you? :)
>>
>> Yes, i think it works for us, nice work :)
> 
> Appreciate your test, Hailiang!
> 
> Paolo, any comment? :)

Will review as soon as 2.9 gets out. :)

Paolo

Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 19/04/2017 13:13, Xiao Guangrong wrote:
> 
> 
> On 04/19/2017 06:41 PM, Hailiang Zhang wrote:
> 
>>> Hi Hailiang,
>>>
>>> Does this patchset work for you? :)
>>
>> Yes, i think it works for us, nice work :)
> 
> Appreciate your test, Hailiang!
> 
> Paolo, any comment? :)

Will review as soon as 2.9 gets out. :)

Paolo

Re: [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> +            int current_irq_coalesced = s->irq_coalesced;
> +
> +            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
> +
> +            /*
> +             * calculate the lost clock after it is scaled which should be
> +             * compensated in the next interrupt.
> +             */
> +            lost_clock += current_irq_coalesced * s->period -
> +                            s->irq_coalesced * period;

This is:

   lost_clock = current_irq_coalesced * s->period -
	(current_irq_coalesced * s->period) / period * period;

i.e.

   /* 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 next_irq_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.
    */
   lost_clock = (s->irq_coalesced * s->period) % period;
   s->irq_coalesced = (s->irq_coalesced * s->period) / period;

Is this correct?

Paolo

> +            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> +                      "are compensated.\n",
> +                      current_irq_coalesced, s->irq_coalesced, lost_clock);
>          }
>          s->period = period;
>  #endif
> @@ -170,7 +193,7 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
>          cur_clock =
>              muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>  
> -        next_irq_clock = (cur_clock & ~(period - 1)) + period;
> +        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);
> -- 

Re: [Qemu-devel] [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> +            int current_irq_coalesced = s->irq_coalesced;
> +
> +            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
> +
> +            /*
> +             * calculate the lost clock after it is scaled which should be
> +             * compensated in the next interrupt.
> +             */
> +            lost_clock += current_irq_coalesced * s->period -
> +                            s->irq_coalesced * period;

This is:

   lost_clock = current_irq_coalesced * s->period -
	(current_irq_coalesced * s->period) / period * period;

i.e.

   /* 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 next_irq_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.
    */
   lost_clock = (s->irq_coalesced * s->period) % period;
   s->irq_coalesced = (s->irq_coalesced * s->period) / period;

Is this correct?

Paolo

> +            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> +                      "are compensated.\n",
> +                      current_irq_coalesced, s->irq_coalesced, lost_clock);
>          }
>          s->period = period;
>  #endif
> @@ -170,7 +193,7 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
>          cur_clock =
>              muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>  
> -        next_irq_clock = (cur_clock & ~(period - 1)) + period;
> +        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);
> -- 

Re: [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> +#ifdef TARGET_I386
> +            /*
> +             * if more than period clocks were passed, i.e, the timer interrupt
> +             * has been lost, we should catch up the time.
> +             */
> +            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> +                (lost_clock / period)) {
> +                int lost_interrupt = lost_clock / period;
> +
> +                s->irq_coalesced += lost_interrupt;
> +                lost_clock -= lost_interrupt * period;
> +                if (lost_interrupt) {
> +                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> +                              "increased to %d\n", lost_interrupt,
> +                              s->irq_coalesced);
> +                    rtc_coalesced_timer_update(s);
> +                }

I think you should merge these two patches, since both of them
essentially update the number of coalesced ticks and then split it
between s->irq_coalesced and lost_clock.

Paolo

> +            } else
> +#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);
> +        }
> +

Re: [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> +#ifdef TARGET_I386
> +            /*
> +             * if more than period clocks were passed, i.e, the timer interrupt
> +             * has been lost, we should catch up the time.
> +             */
> +            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> +                (lost_clock / period)) {
> +                int lost_interrupt = lost_clock / period;
> +
> +                s->irq_coalesced += lost_interrupt;
> +                lost_clock -= lost_interrupt * period;
> +                if (lost_interrupt) {
> +                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> +                              "increased to %d\n", lost_interrupt,
> +                              s->irq_coalesced);
> +                    rtc_coalesced_timer_update(s);
> +                }

I think you should merge these two patches, since both of them
essentially update the number of coalesced ticks and then split it
between s->irq_coalesced and lost_clock.

Paolo

> +            } else
> +#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);
> +        }
> +

Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> Move the x86 specific code in periodic_timer_update() to a common place,
> the actual logic is not changed
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 66 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 3bf559d..d7b7c56 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>  
>      rtc_coalesced_timer_update(s);
>  }
> +
> +static int64_t
> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
> +{
> +    if (period != s->period) {
> +        int64_t scale_lost_clock;
> +        int current_irq_coalesced = s->irq_coalesced;
> +
> +        s->irq_coalesced = (current_irq_coalesced * s->period) / period;
> +
> +        /*
> +         * calculate the lost clock after it is scaled which should be
> +         * compensated in the next interrupt.
> +         */
> +        scale_lost_clock = current_irq_coalesced * s->period -
> +                            s->irq_coalesced * period;
> +        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> +                  "are compensated.\n", current_irq_coalesced,
> +                  s->irq_coalesced, scale_lost_clock);
> +        lost_clock += scale_lost_clock;
> +        s->period = period;

This should be moved up to the caller.

Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
zero.  So I *think* what you get is equivalent to

   if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
       return;
   }

   /* ... comment ... */
   lost_interrupt = (s->irq_coalesced * s->period) / period;
   lost_clock += (s->irq_coalesced * s->period) % period;
   lost_interrupt += lost_clock / period;
   lost_clock %= period;

   s->irq_coalesced = load_interrupt;
   rtc_coalesced_timer_update(s);

or equivalently:

   lost_clock += s->irq_coalesced * s->period;

   s->irq_coalesced = lost_clock / period;
   lost_clock %= period;
   rtc_coalesced_timer_update(s);

I think you should probably merge these three patches and document the
resulting logic, because it's simpler than building it a patch at a time.

Thanks,

Paolo

> +    }
> +
> +    /*
> +     * if more than period clocks were passed, i.e, the timer interrupt
> +     * has been lost, we should catch up the time.
> +     */
> +    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> +       (lost_clock / period)) {
> +        int lost_interrupt = lost_clock / period;
> +
> +        s->irq_coalesced += lost_interrupt;
> +        lost_clock -= lost_interrupt * period;
> +        if (lost_interrupt) {
> +            DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> +                      "increased to %d\n", lost_interrupt, s->irq_coalesced);
> +            rtc_coalesced_timer_update(s);
> +        }
> +    }
> +
> +    return lost_clock;
> +}
> +
> +static void arch_periodic_timer_disable(RTCState *s)
> +{
> +    s->irq_coalesced = 0;
> +}
> +#else
> +static int64_t
> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
> +{
> +    return lost_clock;
> +}
> +
> +static void arch_periodic_timer_disable(RTCState *s)
> +{
> +}
>  #endif
>  
>  static int period_code_to_clock(int period_code)
> @@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
>      if (period_code != 0
>          && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
>          period = period_code_to_clock(period_code);
> -#ifdef TARGET_I386
> -        if (period != s->period) {
> -            int current_irq_coalesced = s->irq_coalesced;
> -
> -            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>  
> -            /*
> -             * calculate the lost clock after it is scaled which should be
> -             * compensated in the next interrupt.
> -             */
> -            lost_clock += current_irq_coalesced * s->period -
> -                            s->irq_coalesced * period;
> -            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> -                      "are compensated.\n",
> -                      current_irq_coalesced, s->irq_coalesced, lost_clock);
> -        }
> -        s->period = period;
> -#endif
>          /* compute 32 khz clock */
>          cur_clock =
>              muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> @@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
>  
>              /* calculate the clock since last interrupt. */
>              lost_clock += cur_clock - last_periodic_clock;
> -
> -#ifdef TARGET_I386
> -            /*
> -             * if more than period clocks were passed, i.e, the timer interrupt
> -             * has been lost, we should catch up the time.
> -             */
> -            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> -                (lost_clock / period)) {
> -                int lost_interrupt = lost_clock / period;
> -
> -                s->irq_coalesced += lost_interrupt;
> -                lost_clock -= lost_interrupt * period;
> -                if (lost_interrupt) {
> -                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> -                              "increased to %d\n", lost_interrupt,
> -                              s->irq_coalesced);
> -                    rtc_coalesced_timer_update(s);
> -                }
> -            } else
> -#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);
>          }
>  
> +        lost_clock = arch_periodic_timer_update(s, period, lost_clock);
> +
> +        /*
> +         * 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);
>      } else {
> -#ifdef TARGET_I386
> -        s->irq_coalesced = 0;
> -#endif
> +        arch_periodic_timer_disable(s);
>          timer_del(s->periodic_timer);
>      }
>  }
> 

Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> Move the x86 specific code in periodic_timer_update() to a common place,
> the actual logic is not changed
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 66 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 3bf559d..d7b7c56 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>  
>      rtc_coalesced_timer_update(s);
>  }
> +
> +static int64_t
> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
> +{
> +    if (period != s->period) {
> +        int64_t scale_lost_clock;
> +        int current_irq_coalesced = s->irq_coalesced;
> +
> +        s->irq_coalesced = (current_irq_coalesced * s->period) / period;
> +
> +        /*
> +         * calculate the lost clock after it is scaled which should be
> +         * compensated in the next interrupt.
> +         */
> +        scale_lost_clock = current_irq_coalesced * s->period -
> +                            s->irq_coalesced * period;
> +        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> +                  "are compensated.\n", current_irq_coalesced,
> +                  s->irq_coalesced, scale_lost_clock);
> +        lost_clock += scale_lost_clock;
> +        s->period = period;

This should be moved up to the caller.

Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
zero.  So I *think* what you get is equivalent to

   if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
       return;
   }

   /* ... comment ... */
   lost_interrupt = (s->irq_coalesced * s->period) / period;
   lost_clock += (s->irq_coalesced * s->period) % period;
   lost_interrupt += lost_clock / period;
   lost_clock %= period;

   s->irq_coalesced = load_interrupt;
   rtc_coalesced_timer_update(s);

or equivalently:

   lost_clock += s->irq_coalesced * s->period;

   s->irq_coalesced = lost_clock / period;
   lost_clock %= period;
   rtc_coalesced_timer_update(s);

I think you should probably merge these three patches and document the
resulting logic, because it's simpler than building it a patch at a time.

Thanks,

Paolo

> +    }
> +
> +    /*
> +     * if more than period clocks were passed, i.e, the timer interrupt
> +     * has been lost, we should catch up the time.
> +     */
> +    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> +       (lost_clock / period)) {
> +        int lost_interrupt = lost_clock / period;
> +
> +        s->irq_coalesced += lost_interrupt;
> +        lost_clock -= lost_interrupt * period;
> +        if (lost_interrupt) {
> +            DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> +                      "increased to %d\n", lost_interrupt, s->irq_coalesced);
> +            rtc_coalesced_timer_update(s);
> +        }
> +    }
> +
> +    return lost_clock;
> +}
> +
> +static void arch_periodic_timer_disable(RTCState *s)
> +{
> +    s->irq_coalesced = 0;
> +}
> +#else
> +static int64_t
> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
> +{
> +    return lost_clock;
> +}
> +
> +static void arch_periodic_timer_disable(RTCState *s)
> +{
> +}
>  #endif
>  
>  static int period_code_to_clock(int period_code)
> @@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
>      if (period_code != 0
>          && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
>          period = period_code_to_clock(period_code);
> -#ifdef TARGET_I386
> -        if (period != s->period) {
> -            int current_irq_coalesced = s->irq_coalesced;
> -
> -            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>  
> -            /*
> -             * calculate the lost clock after it is scaled which should be
> -             * compensated in the next interrupt.
> -             */
> -            lost_clock += current_irq_coalesced * s->period -
> -                            s->irq_coalesced * period;
> -            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> -                      "are compensated.\n",
> -                      current_irq_coalesced, s->irq_coalesced, lost_clock);
> -        }
> -        s->period = period;
> -#endif
>          /* compute 32 khz clock */
>          cur_clock =
>              muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> @@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
>  
>              /* calculate the clock since last interrupt. */
>              lost_clock += cur_clock - last_periodic_clock;
> -
> -#ifdef TARGET_I386
> -            /*
> -             * if more than period clocks were passed, i.e, the timer interrupt
> -             * has been lost, we should catch up the time.
> -             */
> -            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
> -                (lost_clock / period)) {
> -                int lost_interrupt = lost_clock / period;
> -
> -                s->irq_coalesced += lost_interrupt;
> -                lost_clock -= lost_interrupt * period;
> -                if (lost_interrupt) {
> -                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
> -                              "increased to %d\n", lost_interrupt,
> -                              s->irq_coalesced);
> -                    rtc_coalesced_timer_update(s);
> -                }
> -            } else
> -#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);
>          }
>  
> +        lost_clock = arch_periodic_timer_update(s, period, lost_clock);
> +
> +        /*
> +         * 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);
>      } else {
> -#ifdef TARGET_I386
> -        s->irq_coalesced = 0;
> -#endif
> +        arch_periodic_timer_disable(s);
>          timer_del(s->periodic_timer);
>      }
>  }
> 

Re: [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> 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 | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 4165450..749e206 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
>      check_update_timer(s);
>  }
>  
> +static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
> +{
> +    uint8_t orig = s->cmos_data[RTC_REG_A];
> +
> +    return (orig & 0x0f) != (data & 0x0f);
> +}
> +
> +static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
> +{
> +    uint8_t orig = s->cmos_data[RTC_REG_B];
> +
> +    return (orig & REG_B_PIE) != (data & REG_B_PIE);
> +}
> +
>  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 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>              }
>              break;
>          case RTC_REG_A:
> +            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);

I think you can change it to a...

>              if ((data & 0x60) == 0x60) {
>                  if (rtc_running(s)) {
>                      rtc_update_time(s);
> @@ -445,10 +462,16 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>              /* UIP bit is read only */

... inlined update_periodic_timer assignment here:

    update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_A_PERIOD;

>              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 = rtc_periodic_timer_updated_by_regB(s, data);
> +
>              if (data & REG_B_SET) {
>                  /* update cmos to when the rtc was stopping */
>                  if (rtc_running(s)) {
> @@ -475,7 +498,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>                  qemu_irq_lower(s->irq);
>              }

Same here:

    update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_B_PIE;

Thanks,

Paolo

>              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:
> 

Re: [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[Paolo Bonzini]

On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> 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 | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 4165450..749e206 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
>      check_update_timer(s);
>  }
>  
> +static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
> +{
> +    uint8_t orig = s->cmos_data[RTC_REG_A];
> +
> +    return (orig & 0x0f) != (data & 0x0f);
> +}
> +
> +static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
> +{
> +    uint8_t orig = s->cmos_data[RTC_REG_B];
> +
> +    return (orig & REG_B_PIE) != (data & REG_B_PIE);
> +}
> +
>  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 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>              }
>              break;
>          case RTC_REG_A:
> +            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);

I think you can change it to a...

>              if ((data & 0x60) == 0x60) {
>                  if (rtc_running(s)) {
>                      rtc_update_time(s);
> @@ -445,10 +462,16 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>              /* UIP bit is read only */

... inlined update_periodic_timer assignment here:

    update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_A_PERIOD;

>              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 = rtc_periodic_timer_updated_by_regB(s, data);
> +
>              if (data & REG_B_SET) {
>                  /* update cmos to when the rtc was stopping */
>                  if (rtc_running(s)) {
> @@ -475,7 +498,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>                  qemu_irq_lower(s->irq);
>              }

Same here:

    update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_B_PIE;

Thanks,

Paolo

>              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:
> 

Re: [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq

[Xiao Guangrong]

On 05/03/2017 11:15 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> +            int current_irq_coalesced = s->irq_coalesced;
>> +
>> +            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>> +
>> +            /*
>> +             * calculate the lost clock after it is scaled which should be
>> +             * compensated in the next interrupt.
>> +             */
>> +            lost_clock += current_irq_coalesced * s->period -
>> +                            s->irq_coalesced * period;
> 
> This is:
> 
>     lost_clock = current_irq_coalesced * s->period -
> 	(current_irq_coalesced * s->period) / period * period;
> 
> i.e.
> 
>     /* 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 next_irq_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.
>      */
>     lost_clock = (s->irq_coalesced * s->period) % period;
>     s->irq_coalesced = (s->irq_coalesced * s->period) / period;
> 
> Is this correct?
> 

Yes, it is correct, it looks smarter, will apply it in the next version.

Thanks!

Re: [Qemu-devel] [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq

[Xiao Guangrong]

On 05/03/2017 11:15 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> +            int current_irq_coalesced = s->irq_coalesced;
>> +
>> +            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>> +
>> +            /*
>> +             * calculate the lost clock after it is scaled which should be
>> +             * compensated in the next interrupt.
>> +             */
>> +            lost_clock += current_irq_coalesced * s->period -
>> +                            s->irq_coalesced * period;
> 
> This is:
> 
>     lost_clock = current_irq_coalesced * s->period -
> 	(current_irq_coalesced * s->period) / period * period;
> 
> i.e.
> 
>     /* 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 next_irq_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.
>      */
>     lost_clock = (s->irq_coalesced * s->period) % period;
>     s->irq_coalesced = (s->irq_coalesced * s->period) / period;
> 
> Is this correct?
> 

Yes, it is correct, it looks smarter, will apply it in the next version.

Thanks!

Re: [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[Xiao Guangrong]

On 05/03/2017 11:32 PM, Paolo Bonzini wrote:
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> +#ifdef TARGET_I386
>> +            /*
>> +             * if more than period clocks were passed, i.e, the timer interrupt
>> +             * has been lost, we should catch up the time.
>> +             */
>> +            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
>> +                (lost_clo / period)) {
>> +                int lost_interrupt = lost_clock / period;
>> +
>> +                s->irq_coalesced += lost_interrupt;
>> +                lost_clock -= lost_interrupt * period;
>> +                if (lost_interrupt) {
>> +                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
>> +                              "increased to %d\n", lost_interrupt,
>> +                              s->irq_coalesced);
>> +                    rtc_coalesced_timer_update(s);
>> +                }
> 
> I think you should merge these two patches, since both of them
> essentially update the number of coalesced ticks and then split it
> between s->irq_coalesced and lost_clock.

I thought these two patches fix two different issues, one for clock
lost for coalesced-irq and another for period reconfiguration. Your
suggestion sounds reasonable indeed, will merged them in the next
version. :)

Re: [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[Xiao Guangrong]

On 05/03/2017 11:32 PM, Paolo Bonzini wrote:
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> +#ifdef TARGET_I386
>> +            /*
>> +             * if more than period clocks were passed, i.e, the timer interrupt
>> +             * has been lost, we should catch up the time.
>> +             */
>> +            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
>> +                (lost_clo / period)) {
>> +                int lost_interrupt = lost_clock / period;
>> +
>> +                s->irq_coalesced += lost_interrupt;
>> +                lost_clock -= lost_interrupt * period;
>> +                if (lost_interrupt) {
>> +                    DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs "
>> +                              "increased to %d\n", lost_interrupt,
>> +                              s->irq_coalesced);
>> +                    rtc_coalesced_timer_update(s);
>> +                }
> 
> I think you should merge these two patches, since both of them
> essentially update the number of coalesced ticks and then split it
> between s->irq_coalesced and lost_clock.

I thought these two patches fix two different issues, one for clock
lost for coalesced-irq and another for period reconfiguration. Your
suggestion sounds reasonable indeed, will merged them in the next
version. :)

Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Xiao Guangrong]

On 05/03/2017 11:39 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> Move the x86 specific code in periodic_timer_update() to a common place,
>> the actual logic is not changed
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++--------------------
>>   1 file changed, 66 insertions(+), 46 deletions(-)
>>
>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>> index 3bf559d..d7b7c56 100644
>> --- a/hw/timer/mc146818rtc.c
>> +++ b/hw/timer/mc146818rtc.c
>> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>>   
>>       rtc_coalesced_timer_update(s);
>>   }
>> +
>> +static int64_t
>> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
>> +{
>> +    if (period != s->period) {
>> +        int64_t scale_lost_clock;
>> +        int current_irq_coalesced = s->irq_coalesced;
>> +
>> +        s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>> +
>> +        /*
>> +         * calculate the lost clock after it is scaled which should be
>> +         * compensated in the next interrupt.
>> +         */
>> +        scale_lost_clock = current_irq_coalesced * s->period -
>> +                            s->irq_coalesced * period;
>> +        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
>> +                  "are compensated.\n", current_irq_coalesced,
>> +                  s->irq_coalesced, scale_lost_clock);
>> +        lost_clock += scale_lost_clock;
>> +        s->period = period;
> 
> This should be moved up to the caller.

It should not. As you pointed out below, all these code are only needed
for LOST_TICK_POLICY_SLEW that is x86 specific.

Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without
"#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW,
Unnecessary branch checks will little slow down other architectures,
but i think it is acceptable, right? :)

> 
> Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
> zero.  So I *think* what you get is equivalent to
> 
>     if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
>         return;
>     }
> 
>     /* ... comment ... */
>     lost_interrupt = (s->irq_coalesced * s->period) / period;
>     lost_clock += (s->irq_coalesced * s->period) % period;
>     lost_interrupt += lost_clock / period;
>     lost_clock %= period;
> 
>     s->irq_coalesced = load_interrupt;
>     rtc_coalesced_timer_update(s);
> 
> or equivalently:
> 
>     lost_clock += s->irq_coalesced * s->period;
> 
>     s->irq_coalesced = lost_clock / period;
>     lost_clock %= period;
>     rtc_coalesced_timer_update(s);
> 

Exactly right, it is much better, will apply it.

> I think you should probably merge these three patches and document the
> resulting logic, because it's simpler than building it a patch at a time.

Okay, i will consider it carefully in the next version.

Thank you, Paolo!

Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Xiao Guangrong]

On 05/03/2017 11:39 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> Move the x86 specific code in periodic_timer_update() to a common place,
>> the actual logic is not changed
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++--------------------
>>   1 file changed, 66 insertions(+), 46 deletions(-)
>>
>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>> index 3bf559d..d7b7c56 100644
>> --- a/hw/timer/mc146818rtc.c
>> +++ b/hw/timer/mc146818rtc.c
>> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>>   
>>       rtc_coalesced_timer_update(s);
>>   }
>> +
>> +static int64_t
>> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
>> +{
>> +    if (period != s->period) {
>> +        int64_t scale_lost_clock;
>> +        int current_irq_coalesced = s->irq_coalesced;
>> +
>> +        s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>> +
>> +        /*
>> +         * calculate the lost clock after it is scaled which should be
>> +         * compensated in the next interrupt.
>> +         */
>> +        scale_lost_clock = current_irq_coalesced * s->period -
>> +                            s->irq_coalesced * period;
>> +        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
>> +                  "are compensated.\n", current_irq_coalesced,
>> +                  s->irq_coalesced, scale_lost_clock);
>> +        lost_clock += scale_lost_clock;
>> +        s->period = period;
> 
> This should be moved up to the caller.

It should not. As you pointed out below, all these code are only needed
for LOST_TICK_POLICY_SLEW that is x86 specific.

Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without
"#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW,
Unnecessary branch checks will little slow down other architectures,
but i think it is acceptable, right? :)

> 
> Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
> zero.  So I *think* what you get is equivalent to
> 
>     if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
>         return;
>     }
> 
>     /* ... comment ... */
>     lost_interrupt = (s->irq_coalesced * s->period) / period;
>     lost_clock += (s->irq_coalesced * s->period) % period;
>     lost_interrupt += lost_clock / period;
>     lost_clock %= period;
> 
>     s->irq_coalesced = load_interrupt;
>     rtc_coalesced_timer_update(s);
> 
> or equivalently:
> 
>     lost_clock += s->irq_coalesced * s->period;
> 
>     s->irq_coalesced = lost_clock / period;
>     lost_clock %= period;
>     rtc_coalesced_timer_update(s);
> 

Exactly right, it is much better, will apply it.

> I think you should probably merge these three patches and document the
> resulting logic, because it's simpler than building it a patch at a time.

Okay, i will consider it carefully in the next version.

Thank you, Paolo!

Re: [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[Xiao Guangrong]

On 05/03/2017 11:42 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> 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 | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>> index 4165450..749e206 100644
>> --- a/hw/timer/mc146818rtc.c
>> +++ b/hw/timer/mc146818rtc.c
>> @@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
>>       check_update_timer(s);
>>   }
>>   
>> +static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
>> +{
>> +    uint8_t orig = s->cmos_data[RTC_REG_A];
>> +
>> +    return (orig & 0x0f) != (data & 0x0f);
>> +}
>> +
>> +static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
>> +{
>> +    uint8_t orig = s->cmos_data[RTC_REG_B];
>> +
>> +    return (orig & REG_B_PIE) != (data & REG_B_PIE);
>> +}
>> +
>>   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 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>               }
>>               break;
>>           case RTC_REG_A:
>> +            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);
> 
> I think you can change it to a...
> 
>>               if ((data & 0x60) == 0x60) {
>>                   if (rtc_running(s)) {
>>                       rtc_update_time(s);
>> @@ -445,10 +462,16 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>               /* UIP bit is read only */
> 
> ... inlined update_periodic_timer assignment here:
> 
>      update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_A_PERIOD;
> 
>>               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 = rtc_periodic_timer_updated_by_regB(s, data);
>> +
>>               if (data & REG_B_SET) {
>>                   /* update cmos to when the rtc was stopping */
>>                   if (rtc_running(s)) {
>> @@ -475,7 +498,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>                   qemu_irq_lower(s->irq);
>>               }
> 
> Same here:
> 
>      update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_B_PIE;
> 

Okay, will do, thanks!

Re: [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[Xiao Guangrong]

On 05/03/2017 11:42 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> 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 | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>> index 4165450..749e206 100644
>> --- a/hw/timer/mc146818rtc.c
>> +++ b/hw/timer/mc146818rtc.c
>> @@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
>>       check_update_timer(s);
>>   }
>>   
>> +static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
>> +{
>> +    uint8_t orig = s->cmos_data[RTC_REG_A];
>> +
>> +    return (orig & 0x0f) != (data & 0x0f);
>> +}
>> +
>> +static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
>> +{
>> +    uint8_t orig = s->cmos_data[RTC_REG_B];
>> +
>> +    return (orig & REG_B_PIE) != (data & REG_B_PIE);
>> +}
>> +
>>   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 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>               }
>>               break;
>>           case RTC_REG_A:
>> +            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);
> 
> I think you can change it to a...
> 
>>               if ((data & 0x60) == 0x60) {
>>                   if (rtc_running(s)) {
>>                       rtc_update_time(s);
>> @@ -445,10 +462,16 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>               /* UIP bit is read only */
> 
> ... inlined update_periodic_timer assignment here:
> 
>      update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_A_PERIOD;
> 
>>               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 = rtc_periodic_timer_updated_by_regB(s, data);
>> +
>>               if (data & REG_B_SET) {
>>                   /* update cmos to when the rtc was stopping */
>>                   if (rtc_running(s)) {
>> @@ -475,7 +498,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>                   qemu_irq_lower(s->irq);
>>               }
> 
> Same here:
> 
>      update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_B_PIE;
> 

Okay, will do, thanks!

Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Paolo Bonzini]

On 04/05/2017 05:25, Xiao Guangrong wrote:
> 
> 
> On 05/03/2017 11:39 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>
>>> Move the x86 specific code in periodic_timer_update() to a common place,
>>> the actual logic is not changed
>>>
>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>>> ---
>>>   hw/timer/mc146818rtc.c | 112
>>> +++++++++++++++++++++++++++++--------------------
>>>   1 file changed, 66 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>>> index 3bf559d..d7b7c56 100644
>>> --- a/hw/timer/mc146818rtc.c
>>> +++ b/hw/timer/mc146818rtc.c
>>> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>>>         rtc_coalesced_timer_update(s);
>>>   }
>>> +
>>> +static int64_t
>>> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
>>> +{
>>> +    if (period != s->period) {
>>> +        int64_t scale_lost_clock;
>>> +        int current_irq_coalesced = s->irq_coalesced;
>>> +
>>> +        s->irq_coalesced = (current_irq_coalesced * s->period) /
>>> period;
>>> +
>>> +        /*
>>> +         * calculate the lost clock after it is scaled which should be
>>> +         * compensated in the next interrupt.
>>> +         */
>>> +        scale_lost_clock = current_irq_coalesced * s->period -
>>> +                            s->irq_coalesced * period;
>>> +        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld
>>> clocks "
>>> +                  "are compensated.\n", current_irq_coalesced,
>>> +                  s->irq_coalesced, scale_lost_clock);
>>> +        lost_clock += scale_lost_clock;
>>> +        s->period = period;
>>
>> This should be moved up to the caller.
> 
> It should not. As you pointed out below, all these code are only needed
> for LOST_TICK_POLICY_SLEW that is x86 specific.
> 
> Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without
> "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW,
> Unnecessary branch checks will little slow down other architectures,
> but i think it is acceptable, right? :)

Yeah, the #ifdef TARGET_I386 is only needed because of the APIC
interface.  This one doesn't really need the #ifdef.  But you're right
that it shouldn't be moved to the caller.

Paolo

>>
>> Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
>> zero.  So I *think* what you get is equivalent to
>>
>>     if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
>>         return;
>>     }
>>
>>     /* ... comment ... */
>>     lost_interrupt = (s->irq_coalesced * s->period) / period;
>>     lost_clock += (s->irq_coalesced * s->period) % period;
>>     lost_interrupt += lost_clock / period;
>>     lost_clock %= period;
>>
>>     s->irq_coalesced = load_interrupt;
>>     rtc_coalesced_timer_update(s);
>>
>> or equivalently:
>>
>>     lost_clock += s->irq_coalesced * s->period;
>>
>>     s->irq_coalesced = lost_clock / period;
>>     lost_clock %= period;
>>     rtc_coalesced_timer_update(s);
>>
> 
> Exactly right, it is much better, will apply it.
> 
>> I think you should probably merge these three patches and document the
>> resulting logic, because it's simpler than building it a patch at a time.
> 
> Okay, i will consider it carefully in the next version.
> 
> Thank you, Paolo!
> 

Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Paolo Bonzini]

On 04/05/2017 05:25, Xiao Guangrong wrote:
> 
> 
> On 05/03/2017 11:39 PM, Paolo Bonzini wrote:
>>
>>
>> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>
>>> Move the x86 specific code in periodic_timer_update() to a common place,
>>> the actual logic is not changed
>>>
>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>>> ---
>>>   hw/timer/mc146818rtc.c | 112
>>> +++++++++++++++++++++++++++++--------------------
>>>   1 file changed, 66 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>>> index 3bf559d..d7b7c56 100644
>>> --- a/hw/timer/mc146818rtc.c
>>> +++ b/hw/timer/mc146818rtc.c
>>> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque)
>>>         rtc_coalesced_timer_update(s);
>>>   }
>>> +
>>> +static int64_t
>>> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock)
>>> +{
>>> +    if (period != s->period) {
>>> +        int64_t scale_lost_clock;
>>> +        int current_irq_coalesced = s->irq_coalesced;
>>> +
>>> +        s->irq_coalesced = (current_irq_coalesced * s->period) /
>>> period;
>>> +
>>> +        /*
>>> +         * calculate the lost clock after it is scaled which should be
>>> +         * compensated in the next interrupt.
>>> +         */
>>> +        scale_lost_clock = current_irq_coalesced * s->period -
>>> +                            s->irq_coalesced * period;
>>> +        DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld
>>> clocks "
>>> +                  "are compensated.\n", current_irq_coalesced,
>>> +                  s->irq_coalesced, scale_lost_clock);
>>> +        lost_clock += scale_lost_clock;
>>> +        s->period = period;
>>
>> This should be moved up to the caller.
> 
> It should not. As you pointed out below, all these code are only needed
> for LOST_TICK_POLICY_SLEW that is x86 specific.
> 
> Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without
> "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW,
> Unnecessary branch checks will little slow down other architectures,
> but i think it is acceptable, right? :)

Yeah, the #ifdef TARGET_I386 is only needed because of the APIC
interface.  This one doesn't really need the #ifdef.  But you're right
that it shouldn't be moved to the caller.

Paolo

>>
>> Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is
>> zero.  So I *think* what you get is equivalent to
>>
>>     if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) {
>>         return;
>>     }
>>
>>     /* ... comment ... */
>>     lost_interrupt = (s->irq_coalesced * s->period) / period;
>>     lost_clock += (s->irq_coalesced * s->period) % period;
>>     lost_interrupt += lost_clock / period;
>>     lost_clock %= period;
>>
>>     s->irq_coalesced = load_interrupt;
>>     rtc_coalesced_timer_update(s);
>>
>> or equivalently:
>>
>>     lost_clock += s->irq_coalesced * s->period;
>>
>>     s->irq_coalesced = lost_clock / period;
>>     lost_clock %= period;
>>     rtc_coalesced_timer_update(s);
>>
> 
> Exactly right, it is much better, will apply it.
> 
>> I think you should probably merge these three patches and document the
>> resulting logic, because it's simpler than building it a patch at a time.
> 
> Okay, i will consider it carefully in the next version.
> 
> Thank you, Paolo!
> 

Re: [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[Paolo Bonzini]

On 04/05/2017 04:54, Xiao Guangrong wrote:
>>>
>>> +            /*
>>> +             * if more than period clocks were passed, i.e, the
>>> timer interrupt
>>> +             * has been lost, we should catch up the time.
>>> +             */
>>> +            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
>>> +                (lost_clo / period)) {
>>> +                int lost_interrupt = lost_clock / period;
>>> +
>>> +                s->irq_coalesced += lost_interrupt;
>>> +                lost_clock -= lost_interrupt * period;
>>> +                if (lost_interrupt) {
>>> +                    DPRINTF_C("cmos: compensate %d interrupts,
>>> coalesced irqs "
>>> +                              "increased to %d\n", lost_interrupt,
>>> +                              s->irq_coalesced);
>>> +                    rtc_coalesced_timer_update(s);
>>> +                }
>>
>> I think you should merge these two patches, since both of them
>> essentially update the number of coalesced ticks and then split it
>> between s->irq_coalesced and lost_clock.
> 
> I thought these two patches fix two different issues, one for clock
> lost for coalesced-irq and another for period reconfiguration. Your
> suggestion sounds reasonable indeed, will merged them in the next
> version. :)

Yunfang's patch in turn has two parts, one that is only for SLEW and one
that is not.  You can keep the first separate, and merge the second with
yours.

Paolo

Re: [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt

[Paolo Bonzini]

On 04/05/2017 04:54, Xiao Guangrong wrote:
>>>
>>> +            /*
>>> +             * if more than period clocks were passed, i.e, the
>>> timer interrupt
>>> +             * has been lost, we should catch up the time.
>>> +             */
>>> +            if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
>>> +                (lost_clo / period)) {
>>> +                int lost_interrupt = lost_clock / period;
>>> +
>>> +                s->irq_coalesced += lost_interrupt;
>>> +                lost_clock -= lost_interrupt * period;
>>> +                if (lost_interrupt) {
>>> +                    DPRINTF_C("cmos: compensate %d interrupts,
>>> coalesced irqs "
>>> +                              "increased to %d\n", lost_interrupt,
>>> +                              s->irq_coalesced);
>>> +                    rtc_coalesced_timer_update(s);
>>> +                }
>>
>> I think you should merge these two patches, since both of them
>> essentially update the number of coalesced ticks and then split it
>> between s->irq_coalesced and lost_clock.
> 
> I thought these two patches fix two different issues, one for clock
> lost for coalesced-irq and another for period reconfiguration. Your
> suggestion sounds reasonable indeed, will merged them in the next
> version. :)

Yunfang's patch in turn has two parts, one that is only for SLEW and one
that is not.  You can keep the first separate, and merge the second with
yours.

Paolo