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

[Paolo Bonzini]

On 02/21/2012 01:00 AM, Zhang, Yang Z wrote:
>> > Thanks, this looks much better!  I'll run it through some tests.
>> > 
>> > We also should try to keep migration working from older versions using the
>> > load_old callback.
> Sure, I missed it. Will add it to the version 3. 
> Any other comments?

Here they are.

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.

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!)

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.

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);

3) Please move the very complicated alarm logic to a separate function.
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?

5) As a further cleanup, perhaps you can create hours_from_bcd and
hours_to_bcd functions to abstract the 12/24 setting.

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.

Paolo

0001-add-rtc-emulation-tests.patch
Description: Text Data