Message ID | 4F68759E.10805@redhat.com |
---|---|
State | New |
Headers | show |
> -----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
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
-----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
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
> -----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
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