> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@xxxxxxxxxx] > Sent: Wednesday, February 22, 2012 7:19 PM > > 0) My alarm tests failed quite badly. :( I attach a patch for kvm-unit-tests > (repository at git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git). > The tests can be compiled simply with "make" and run with "qemu-kvm -kernel > /path/to/x86/rtc.flat -serial stdio -display none". Upstream QEMU fails some of > the tests. My branch rtc-cleanup at git://github.com/bonzini/qemu.git passes > them. The tests should take 30-40 seconds to run. You are right. There are something wrong with alarm setting. I have fixed it in next version. > Currently they hang very early because UF is not set. I attempted to fix that, > but ran into other problems. UIP seems not to be really in sync with the update > interrupt, because the 500 ms update tests pass when testing UIP, but not when > testing UF. (Another reason why I wanted to have the 500 ms > rule: it improves reliability of tests!) The current logic is not correct: It check the UIP with rtc clock and use a timer to set UF bit. Since the delay of the timer, those two do not in sync in some cases. Now, I separate UF logic from update interrupt. And base it on UIP. With this changed, it works. > 1) Alarm must also be handled like update. That is, the timer must be enabled > when AF=0 rather than when AIE=1. Again, this is not enough to fix the > problems. This is same as UF. The timer only for AIE not for AF. Also separate if from alarm logic and based it on UIP. > 2) Instead of using gettimeofday, you should use qemu_get_clock_ns(rtc_clock). > If host_clock == rtc_clock it is the same, see get_clock_realtime() in > qemu-timer.h. It also has the advantage that you can do all math in > nanoseconds and remove the get_ticks_per_sec() / USEC_PER_SEC factors. > > For example > > gettimeofday(&tv_now, NULL); > host_usec = tv_now.tv_sec * USEC_PER_SEC + tv_now.tv_usec; > > should be simply: > > host_nsec = qemu_get_clock_ns(rtc_clock); Good point! > 3) Please move the very complicated alarm logic to a separate function. Maybe I can add more annotate to explain it. > Ideally, the timer update functions should be as simple as this: > static void update_ended_timer_update(RTCState *s) { > if ((s->cmos_data[RTC_REG_C] & REG_C_UF) == 0 && > !(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > qemu_mod_timer(s->update_timer, rtc_update_time(s)); > } else { > qemu_del_timer(s->update_timer); > } > } > > /* handle alarm timer */ > static void alarm_timer_update(RTCState *s) { > uint64_t next_update_time; > uint64_t expire_time; > > if ((s->cmos_data[RTC_REG_C] & REG_C_AF) == 0 && > !(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > next_update_time = rtc_update_time(s); > expire_time = (rtc_alarm_sec(s) - 1) * NSEC_PER_SEC + > next_update_time; > qemu_mod_timer(s->alarm_timer, expire_time); > } else { > qemu_del_timer(s->alarm_timer); > } > } > > > 4) Related to this, my GCC reports a uninitialized variable at the += 1 here: > > } else if (cur_min == alarm_min) { > if ((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0) { > next_alarm_sec += 1; > } else if (cur_sec < alarm_sec) { > next_alarm_sec = alarm_sec - cur_sec; > } else { > min = alarm_min + MIN_PER_HOUR - cur_min; > next_alarm_sec = > alarm_sec + min * SEC_PER_MIN - cur_sec; > } > > I think it should be an "= 1" instead? Yes, it should be "=1". > 5) As a further cleanup, perhaps you can create hours_from_bcd and > hours_to_bcd functions to abstract the 12/24 setting. Good point! > 6) Setting the clock after 500 ms happens not on every set, but only when moving > out of divider reset (register A bits 5-7 moving from 110 or 111 to 010). As far as > I can read, SET prevents the registers from changing value, but keeps the internal > sub-second counters running. Do we really need this logic? It sounds like senseless. best regards yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
