[Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Zhang, Yang Z]

Changes in v4:
Rebase to latest head.
Changing in patch 6: 
	Set the timer to one second earlier before target alarm when AF bit is clear. In version 3, in order to solve the async between UF, AF and UIP, the timer will keep running when UF or AF are clear. This is a little ugly, especially when a userspace program is using the alarm and we cannot achieve any power saving. In this version, when the AF bit is cleared, we will set the timer to one second earlier before the alarm. With this changing, we can avoid the unnecessary timer and keep the sync between UF, AF and UIP. Please help to review the patch 6.

Changes in v3:
Rebase to latest head.
Remove the logic to update time format when DM bit changed.
Allow to migrate from old version.
Solve the async when reading UF and UIP

Changes in v2:
Add UIP check logic.
Add logic that next second tick will occur in exactly 500ms later after reset divider

Current RTC emulation uses periodic timer(2 timers per second) to update RTC clock. And it will stop CPU staying at deep C-state for long period. Our experience shows the Pkg C6 residency reduced 6% when running 64 idle guest.
The following patch stop the two periodic timer and only updating RTC clock when guest try to read it.
--- 
Yang Zhang (7):
	RTC: Remove the logic to update time format when DM bit changed
	RTC: Update the RTC clock only when reading it
	RTC: Add UIP(update in progress) check logic
	RTC: Set internal millisecond register to 500ms when reset divider
	RTC: Add RTC update-ended interrupt support
	RTC: Add alarm support
	RTC: Allow to migrate from old version

    hw/mc146818rtc.c |  617 ++++++++++++++++++++++++++++++++++++++++-------------
    1 files changed, 465 insertions(+), 152 deletions(-)

best regards
yang

Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

Il 19/03/2012 07:13, Zhang, Yang Z ha scritto:
> Current RTC emulation uses periodic timer(2 timers per second) to
> update RTC clock. And it will stop CPU staying at deep C-state for
> long period. Our experience shows the Pkg C6 residency reduced 6%
> when running 64 idle guest. The following patch stop the two periodic
> timer and only updating RTC clock when guest try to read it.

I attach a patch that fixes some problems with divider reset and in
general simplifies the logic.  Even with the patch, however, I still see
failures in my test case unfortunately.  Probably there are rounding
errors somewhere.

Also (minor change) you need to use rtc_clock instead of host_clock.

I'm afraid that we're hitting a wall. :(  The problem I have is that the
logic in your patch is very complex and, without understanding it well,
I'm afraid we'll never be able to fix it completely (while the old
inefficient one is buggy but it _can_ be fixed).

By the way in general the SET bit and the divider should be handled in
the same way, because both stop the updates.  That is, in general there
should be a function like

static inline bool rtc_running(RTCState *s)
{
    return (!(s->cmos_data[RTC_REG_B] & REG_B_SET) &&
	    (s->cmos_data[RTC_REG_A] & 0x70) == 0x20);
}

that is used instead of testing REG_B_SET.  I pushed some work along
these lines at git://github.com/bonzini/qemu.git (branch rtc-intel), but
it does not really improve the situation with respect to rounding errors
so the bug must be elsewhere.

Paolo

[-- Attachment #2: 0001-fixes.patch --]
[-- Type: text/x-patch, Size: 3512 bytes --]

>From 198afe37adb532738a55dd32ef8bd533c2493c65 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 20 Mar 2012 12:21:00 +0100
Subject: [PATCH] fixes

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 22a9f40..f94ddbb 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -123,8 +123,6 @@ static void rtc_set_cmos(RTCState *s);
 static inline int rtc_from_bcd(RTCState *s, int a);
 static uint64_t get_next_alarm(RTCState *s);
 
-static int32_t divider_reset;
-
 static uint64_t get_guest_rtc_us(RTCState *s)
 {
     int64_t host_usec, offset_usec, guest_usec;
@@ -489,7 +487,8 @@ static void rtc_update_timer2(void *opaque)
     RTCState *s = opaque;
     int32_t alarm_fired;
 
-    if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
+    if (!(s->cmos_data[RTC_REG_B] & REG_B_SET) &&
+        (s->cmos_data[RTC_REG_A] & 0x70) == 0x20) {
         s->cmos_data[RTC_REG_C] |= REG_C_UF;
         if (check_alarm(s)) {
             s->cmos_data[RTC_REG_C] |= REG_C_AF;
@@ -561,16 +560,16 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data)
         case RTC_REG_A:
             /* when the divider reset is removed, the first update cycle
              * begins one-half second later*/
-            if (((s->cmos_data[RTC_REG_A] & 0x60) == 0x60) &&
-                                    ((data & 0x70) >> 4) <= 2) {
-                divider_reset = 1;
-                if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
-                    rtc_calibrate_time(s);
-                    rtc_set_offset(s, 500000);
-                    s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
-                    check_update_timer(s);
-                    divider_reset = 0;
-                }
+            if ((data & 0x60) == 0x60 &&
+                (s->cmos_data[RTC_REG_A] & 0x70) <= 0x20) {
+                rtc_calibrate_time(s);
+                rtc_set_cmos(s);
+                s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
+	    } else if ((s->cmos_data[RTC_REG_A] & 0x60) == 0x60 &&
+                       (data & 0x70) <= 0x20) {
+                rtc_set_time(s);
+                rtc_set_offset(s, 500000);
+                check_update_timer(s);
             }
             /* UIP bit is read only */
             s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) |
@@ -591,11 +590,7 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data)
                 /* if disabling set mode, update the time */
                 if (s->cmos_data[RTC_REG_B] & REG_B_SET) {
                     rtc_set_time(s);
-                    if (divider_reset == 1) {
-                        rtc_set_offset(s, 500000);
-                        s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
-                        divider_reset = 0;
-                    } else {
+                    if (((s->cmos_data[RTC_REG_A] & 0x70) >> 4) <= 2) {
                         rtc_set_offset(s, 0);
                     }
                 }
@@ -709,6 +704,9 @@ static int update_in_progress(RTCState *s)
     if (s->cmos_data[RTC_REG_B] & REG_B_SET) {
         return 0;
     }
+    if ((s->cmos_data[RTC_REG_A] & 0x60) == 0x60) {
+        return 0;
+    }
     guest_usec = get_guest_rtc_us(s);
     /* UIP bit will be set at last 244us of every second. */
     if ((guest_usec % USEC_PER_SEC) >= (USEC_PER_SEC - 244)) {
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Zhang, Yang Z]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> I attach a patch that fixes some problems with divider reset and in
> general simplifies the logic.  Even with the patch, however, I still see
> failures in my test case unfortunately.  Probably there are rounding
> errors somewhere.
Actually, I also see some failures during testing. And most of them are fail to pass the 244us update cycle checking. Since we are in emulation environment, we cannot ensure it always greater than 244us. So I think it should not be a bug.
Also, maybe there are some other bugs, I will do more testing.

> Also (minor change) you need to use rtc_clock instead of host_clock.
> 
> I'm afraid that we're hitting a wall. :(  The problem I have is that the
> logic in your patch is very complex and, without understanding it well,
> I'm afraid we'll never be able to fix it completely (while the old
> inefficient one is buggy but it _can_ be fixed).
> 
> By the way in general the SET bit and the divider should be handled in
> the same way, because both stop the updates.  That is, in general there
> should be a function like
> 
> static inline bool rtc_running(RTCState *s)
> {
>     return (!(s->cmos_data[RTC_REG_B] & REG_B_SET) &&
> 	    (s->cmos_data[RTC_REG_A] & 0x70) == 0x20);
> }
> 
> that is used instead of testing REG_B_SET.  I pushed some work along
> these lines at git://github.com/bonzini/qemu.git (branch rtc-intel), but
> it does not really improve the situation with respect to rounding errors
> so the bug must be elsewhere.
You are right, I missed this logic. Will add it in next version.

Best regrads
yang

Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Paolo Bonzini]

Il 22/03/2012 01:23, Zhang, Yang Z ha scritto:
> Actually, I also see some failures during testing. And most of them
> are fail to pass the 244us update cycle checking. Since we are in
> emulation environment, we cannot ensure it always greater than 244us.
> So I think it should not be a bug.

Yes, that's fine.  But I also see some cases in which the first upgrade
cycle after divider reset is not reported.  The second, 1.5 seconds
after divider reset, is fine.

Paolo

Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Zhang, Yang Z]

 -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> 
> Il 22/03/2012 01:23, Zhang, Yang Z ha scritto:
> > Actually, I also see some failures during testing. And most of them
> > are fail to pass the 244us update cycle checking. Since we are in
> > emulation environment, we cannot ensure it always greater than 244us.
> > So I think it should not be a bug.
> 
> Yes, that's fine.  But I also see some cases in which the first upgrade
> cycle after divider reset is not reported.  The second, 1.5 seconds
> after divider reset, is fine.
I think your patch can fix it. Right?

Best regards
yang

Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Paolo Bonzini]

Il 22/03/2012 04:03, Zhang, Yang Z ha scritto:
>> > Yes, that's fine.  But I also see some cases in which the first upgrade
>> > cycle after divider reset is not reported.  The second, 1.5 seconds
>> > after divider reset, is fine.
> I think your patch can fix it. Right?

No. :(  It makes it a lot less likely, but still occurs every 2-3 runs.

Paolo

Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC

[Zhang, Yang Z]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Thursday, March 22, 2012 11:05 AM
> To: Zhang, Yang Z
> Cc: qemu-devel@nongnu.org; aliguori@us.ibm.com; kvm@vger.kernel.org
> Subject: Re: [PATCH v4 0/7] RTC: New logic to emulate RTC
> 
> Il 22/03/2012 04:03, Zhang, Yang Z ha scritto:
> >> > Yes, that's fine.  But I also see some cases in which the first upgrade
> >> > cycle after divider reset is not reported.  The second, 1.5 seconds
> >> > after divider reset, is fine.
> > I think your patch can fix it. Right?
> 
> No. :(  It makes it a lot less likely, but still occurs every 2-3 runs.
Ok, I will look into it.

best regards
yang