* [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-12 9:51 ` guangrong.xiao
0 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread* [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-04-12 9:51 ` guangrong.xiao
-1 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Currently, the timer is updated whenever RegA or RegB is written
even if the periodic timer related configuration is not changed
This patch optimizes it slightly to make the update happen only
if its period or enable-status is changed, also later patches are
depend on this optimization
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
hw/timer/mc146818rtc.c | 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
^ permalink raw reply related [flat|nested] 59+ messages in thread* [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed
@ 2017-04-12 9:51 ` guangrong.xiao
0 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
From: Xiao Guangrong <xiaoguangrong@tencent.com>
Currently, the timer is updated whenever RegA or RegB is written
even if the periodic timer related configuration is not changed
This patch optimizes it slightly to make the update happen only
if its period or enable-status is changed, also later patches are
depend on this optimization
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
hw/timer/mc146818rtc.c | 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
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-05-03 15:42 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:42 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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:
>
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed
@ 2017-05-03 15:42 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:42 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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:
>
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed
2017-05-03 15:42 ` [Qemu-devel] " Paolo Bonzini
@ 2017-05-04 3:27 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 3:27 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed
@ 2017-05-04 3:27 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 3:27 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-04-12 9:51 ` guangrong.xiao
-1 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* [Qemu-devel] [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq
@ 2017-04-12 9:51 ` guangrong.xiao
0 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-05-03 15:15 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:15 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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);
> --
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq
@ 2017-05-03 15:15 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:15 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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);
> --
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq
2017-05-03 15:15 ` [Qemu-devel] " Paolo Bonzini
@ 2017-05-04 2:51 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 2:51 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq
@ 2017-05-04 2:51 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 2:51 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-04-12 9:51 ` guangrong.xiao
-1 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
@ 2017-04-12 9:51 ` guangrong.xiao
0 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-05-03 15:32 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:32 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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);
> + }
> +
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
@ 2017-05-03 15:32 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:32 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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);
> + }
> +
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
2017-05-03 15:32 ` [Qemu-devel] " Paolo Bonzini
@ 2017-05-04 2:54 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 2:54 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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. :)
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
@ 2017-05-04 2:54 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 2:54 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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. :)
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
2017-05-04 2:54 ` [Qemu-devel] " Xiao Guangrong
@ 2017-05-04 12:02 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-04 12:02 UTC (permalink / raw)
To: Xiao Guangrong, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt
@ 2017-05-04 12:02 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-04 12:02 UTC (permalink / raw)
To: Xiao Guangrong, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-04-12 9:51 ` guangrong.xiao
-1 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
@ 2017-04-12 9:51 ` guangrong.xiao
0 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-05-03 15:39 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:39 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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);
> }
> }
>
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
@ 2017-05-03 15:39 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-03 15:39 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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);
> }
> }
>
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
2017-05-03 15:39 ` [Qemu-devel] " Paolo Bonzini
@ 2017-05-04 3:25 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 3:25 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
@ 2017-05-04 3:25 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-05-04 3:25 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
2017-05-04 3:25 ` [Qemu-devel] " Xiao Guangrong
@ 2017-05-04 7:08 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-04 7:08 UTC (permalink / raw)
To: Xiao Guangrong, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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!
>
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update
@ 2017-05-04 7:08 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-05-04 7:08 UTC (permalink / raw)
To: Xiao Guangrong, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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!
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 5/5] mc146818rtc: embrace all x86 specific code
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-04-12 9:51 ` guangrong.xiao
-1 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread* [Qemu-devel] [PATCH 5/5] mc146818rtc: embrace all x86 specific code
@ 2017-04-12 9:51 ` guangrong.xiao
0 siblings, 0 replies; 59+ messages in thread
From: guangrong.xiao @ 2017-04-12 9:51 UTC (permalink / raw)
To: pbonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao
@ 2017-04-13 6:37 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-04-13 6:37 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 6:37 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-04-13 6:37 UTC (permalink / raw)
To: guangrong.xiao, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 6:37 ` [Qemu-devel] " Paolo Bonzini
@ 2017-04-13 8:39 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 8:39 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 8:39 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 8:39 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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!
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 8:39 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-13 8:52 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 8:52 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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. :(
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 8:52 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 8:52 UTC (permalink / raw)
To: Paolo Bonzini, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, 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. :(
^ permalink raw reply [flat|nested] 59+ messages in thread
* 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 8:52 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-13 9:05 ` Zhanghailiang
-1 siblings, 0 replies; 59+ messages in thread
From: Zhanghailiang @ 2017-04-13 9:05 UTC (permalink / raw)
To: Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 9:05 ` Zhanghailiang
0 siblings, 0 replies; 59+ messages in thread
From: Zhanghailiang @ 2017-04-13 9:05 UTC (permalink / raw)
To: Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 9:05 ` [Qemu-devel] " Zhanghailiang
@ 2017-04-13 9:18 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 9:18 UTC (permalink / raw)
To: Zhanghailiang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: qemu-devel, kvm, yunfangtai, 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?
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 9:18 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 9:18 UTC (permalink / raw)
To: Zhanghailiang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: qemu-devel, kvm, yunfangtai, 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?
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 9:18 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-13 9:29 ` Hailiang Zhang
-1 siblings, 0 replies; 59+ messages in thread
From: Hailiang Zhang @ 2017-04-13 9:29 UTC (permalink / raw)
To: Xiao Guangrong, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
>
> .
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 9:29 ` Hailiang Zhang
0 siblings, 0 replies; 59+ messages in thread
From: Hailiang Zhang @ 2017-04-13 9:29 UTC (permalink / raw)
To: Xiao Guangrong, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
>
> .
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 9:29 ` [Qemu-devel] " Hailiang Zhang
@ 2017-04-13 9:35 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 9:35 UTC (permalink / raw)
To: Hailiang Zhang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, 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.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 9:35 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-13 9:35 UTC (permalink / raw)
To: Hailiang Zhang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, 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.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 9:35 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-13 9:38 ` Hailiang Zhang
-1 siblings, 0 replies; 59+ messages in thread
From: Hailiang Zhang @ 2017-04-13 9:38 UTC (permalink / raw)
To: Xiao Guangrong, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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.
>
>
> .
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-13 9:38 ` Hailiang Zhang
0 siblings, 0 replies; 59+ messages in thread
From: Hailiang Zhang @ 2017-04-13 9:38 UTC (permalink / raw)
To: Xiao Guangrong, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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.
>
>
> .
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 9:38 ` [Qemu-devel] " Hailiang Zhang
@ 2017-04-19 2:02 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-19 2:02 UTC (permalink / raw)
To: Hailiang Zhang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, 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? :)
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-19 2:02 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-19 2:02 UTC (permalink / raw)
To: Hailiang Zhang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, 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? :)
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-19 2:02 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-19 10:41 ` Hailiang Zhang
-1 siblings, 0 replies; 59+ messages in thread
From: Hailiang Zhang @ 2017-04-19 10:41 UTC (permalink / raw)
To: Xiao Guangrong, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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 :)
>
>
> .
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-19 10:41 ` Hailiang Zhang
0 siblings, 0 replies; 59+ messages in thread
From: Hailiang Zhang @ 2017-04-19 10:41 UTC (permalink / raw)
To: Xiao Guangrong, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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 :)
>
>
> .
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-19 10:41 ` [Qemu-devel] " Hailiang Zhang
@ 2017-04-19 11:13 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-19 11:13 UTC (permalink / raw)
To: Hailiang Zhang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, 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? :)
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-19 11:13 ` Xiao Guangrong
0 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-19 11:13 UTC (permalink / raw)
To: Hailiang Zhang, Xiao Guangrong, Paolo Bonzini, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, 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? :)
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-19 11:13 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-19 16:44 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-04-19 16:44 UTC (permalink / raw)
To: Xiao Guangrong, Hailiang Zhang, Xiao Guangrong, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] 答复: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-19 16:44 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-04-19 16:44 UTC (permalink / raw)
To: Xiao Guangrong, Hailiang Zhang, Xiao Guangrong, mst, mtosatti
Cc: xuquan8, qemu-devel, kvm, yunfangtai, Xiao Guangrong
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-13 8:52 ` [Qemu-devel] " Xiao Guangrong
@ 2017-04-14 5:09 ` Paolo Bonzini
-1 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-04-14 5:09 UTC (permalink / raw)
To: Xiao Guangrong, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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. :(
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
@ 2017-04-14 5:09 ` Paolo Bonzini
0 siblings, 0 replies; 59+ messages in thread
From: Paolo Bonzini @ 2017-04-14 5:09 UTC (permalink / raw)
To: Xiao Guangrong, mst, mtosatti; +Cc: qemu-devel, kvm, yunfangtai, Xiao Guangrong
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. :(
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] mc146818rtc: fix Windows VM clock faster
2017-04-14 5:09 ` [Qemu-devel] " Paolo Bonzini
(?)
@ 2017-04-14 6:07 ` Xiao Guangrong
-1 siblings, 0 replies; 59+ messages in thread
From: Xiao Guangrong @ 2017-04-14 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Xiao Guangrong, mst, mtosatti
Cc: Xiao Guangrong, yunfangtai, qemu-devel, kvm
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. :)
^ permalink raw reply [flat|nested] 59+ messages in thread