diff mbox

[v4,0/7] RTC: New logic to emulate RTC

Message ID 4F68759E.10805@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini March 20, 2012, 12:18 p.m. UTC 
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

Comments

Zhang, Yang Z March 22, 2012, 12:23 a.m. UTC | #1 
> -----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
Paolo Bonzini March 22, 2012, 12:42 a.m. UTC | #2 
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
Zhang, Yang Z March 22, 2012, 3:03 a.m. UTC | #3 
-----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
Paolo Bonzini March 22, 2012, 3:05 a.m. UTC | #4 
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
Zhang, Yang Z March 22, 2012, 3:06 a.m. UTC | #5 
> -----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
diff mbox

Patch

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