From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47611) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9y1t-0000iM-QZ for qemu-devel@nongnu.org; Tue, 20 Mar 2012 08:19:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9y1U-0003r1-Nw for qemu-devel@nongnu.org; Tue, 20 Mar 2012 08:19:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21419) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9y1U-0003qi-FV for qemu-devel@nongnu.org; Tue, 20 Mar 2012 08:18:44 -0400 Message-ID: <4F68759E.10805@redhat.com> Date: Tue, 20 Mar 2012 13:18:38 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/mixed; boundary="------------050606010701080203060904" Subject: Re: [Qemu-devel] [PATCH v4 0/7] RTC: New logic to emulate RTC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Zhang, Yang Z" Cc: "aliguori@us.ibm.com" , "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" This is a multi-part message in MIME format. --------------050606010701080203060904 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 --------------050606010701080203060904 Content-Type: text/x-patch; name="0001-fixes.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-fixes.patch" >>From 198afe37adb532738a55dd32ef8bd533c2493c65 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 20 Mar 2012 12:21:00 +0100 Subject: [PATCH] fixes Signed-off-by: Paolo Bonzini --- 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 --------------050606010701080203060904--