Re: [Qemu-devel] [PATCH v2 0/4] RTC: New logic to emulate RTC

[Zhang, Yang Z]

> -----Original Message-----
> From: Paolo Bonzini [mailto:address@hidden
> 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