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

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

[Zhang, Yang Z]

Changes in v2:
Add UIP check logic.
Add logic that next second tick will occur in exactly 500ms later after setting the clock

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.

--- 
Yang Zhang (4):
	RTC: Update the RTC clock only when reading it
	RTC: Add RTC update-ended interrupt support
	RTC: Add alarm support
	RTC: Add UIP(update in progress) check logic

	hw/mc146818rtc.c |  447 +++++++++++++++++++++++++++++++++++++-----------------
 	1 files changed, 308 insertions(+), 139 deletions(-)

best regards
yang

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

[Paolo Bonzini]

On 02/20/2012 01:24 AM, Zhang, Yang Z wrote:
> Changes in v2:
> Add UIP check logic.
> Add logic that next second tick will occur in exactly 500ms later after setting the clock
> 
> 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.
> 
> --- 
> Yang Zhang (4):
> 	RTC: Update the RTC clock only when reading it
> 	RTC: Add RTC update-ended interrupt support
> 	RTC: Add alarm support
> 	RTC: Add UIP(update in progress) check logic
> 
> 	hw/mc146818rtc.c |  447 +++++++++++++++++++++++++++++++++++++-----------------
>  	1 files changed, 308 insertions(+), 139 deletions(-)
> 
> best regards
> yang
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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.

Paolo

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

[Zhang, Yang Z]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, February 20, 2012 3:41 PM
> 
> On 02/20/2012 01:24 AM, Zhang, Yang Z wrote:
> > Changes in v2:
> > Add UIP check logic.
> > Add logic that next second tick will occur in exactly 500ms later
> > after setting the clock
> >
> > 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.
> >
> > ---
> > Yang Zhang (4):
> > 	RTC: Update the RTC clock only when reading it
> > 	RTC: Add RTC update-ended interrupt support
> > 	RTC: Add alarm support
> > 	RTC: Add UIP(update in progress) check logic
> >
> > 	hw/mc146818rtc.c |  447
> +++++++++++++++++++++++++++++++++++++-----------------
> >  	1 files changed, 308 insertions(+), 139 deletions(-)
> >
> > best regards
> > yang
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in the
> > body of a message to majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
> >
> 
> 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?

best regards
yang

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 3635 bytes --]

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

[-- Attachment #2: 0001-add-rtc-emulation-tests.patch --]
[-- Type: text/x-patch, Size: 10142 bytes --]

>From f8d5e45941562cd8259523bd225c81a9dd841308 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 22 Nov 2011 18:53:42 +0100
Subject: [PATCH] add rtc emulation tests

---
 config-x86-common.mak |    4 +-
 x86/rtc.c             |  294 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 297 insertions(+), 1 deletions(-)
 create mode 100644 x86/rtc.c

diff --git a/config-x86-common.mak b/config-x86-common.mak
index a093b8d..348c02a 100644
--- a/config-x86-common.mak
+++ b/config-x86-common.mak
@@ -34,7 +34,7 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
                $(TEST_DIR)/realmode.flat $(TEST_DIR)/msr.flat \
                $(TEST_DIR)/hypercall.flat $(TEST_DIR)/sieve.flat \
                $(TEST_DIR)/kvmclock_test.flat  $(TEST_DIR)/eventinj.flat \
-               $(TEST_DIR)/s3.flat
+               $(TEST_DIR)/s3.flat $(TEST_DIR)/rtc.flat
 
 ifdef API
 tests-common += api/api-sample
@@ -77,6 +77,8 @@ $(TEST_DIR)/idt_test.elf: $(cstart.o) $(TEST_DIR)/idt_test.o
 
 $(TEST_DIR)/xsave.elf: $(cstart.o) $(TEST_DIR)/xsave.o
 
+$(TEST_DIR)/rtc.elf: $(cstart.o) $(TEST_DIR)/rtc.o
+
 $(TEST_DIR)/rmap_chain.elf: $(cstart.o) $(TEST_DIR)/rmap_chain.o
 
 $(TEST_DIR)/svm.elf: $(cstart.o)
diff --git a/x86/rtc.c b/x86/rtc.c
new file mode 100644
index 0000000..03cf4dc
--- /dev/null
+++ b/x86/rtc.c
@@ -0,0 +1,294 @@
+#include "io.h"
+#include "processor.h"
+#include "libcflat.h"
+
+#define RTC_SECONDS             0
+#define RTC_SECONDS_ALARM       1
+#define RTC_MINUTES             2
+#define RTC_MINUTES_ALARM       3
+#define RTC_HOURS               4
+#define RTC_HOURS_ALARM         5
+#define RTC_ALARM_DONT_CARE    0xC0
+
+#define RTC_DAY_OF_WEEK         6
+#define RTC_DAY_OF_MONTH        7
+#define RTC_MONTH               8
+#define RTC_YEAR                9
+
+#define RTC_REG_A               10
+#define RTC_REG_B               11
+#define RTC_REG_C               12
+#define RTC_REG_D               13
+
+#define REG_A_UIP 0x80
+
+#define REG_B_SET  0x80
+#define REG_B_PIE  0x40
+#define REG_B_AIE  0x20
+#define REG_B_UIE  0x10
+#define REG_B_SQWE 0x08
+#define REG_B_DM   0x04
+#define REG_B_24H  0x02
+
+#define REG_C_UF   0x10
+#define REG_C_IRQF 0x80
+#define REG_C_PF   0x40
+#define REG_C_AF   0x20
+
+static inline int rtc_in(int reg)
+{
+    //asm volatile("cli");
+    outb(reg, 0x70);
+    unsigned char x = inb(0x71);
+    //asm volatile("sti");
+    return x;
+}
+
+static inline void rtc_out(int reg, int val)
+{
+    //asm volatile("cli");
+    outb(reg, 0x70);
+    outb(val, 0x71);
+    //asm volatile("sti");
+}
+
+static inline void rtc_en(int bit)
+{
+    rtc_in(RTC_REG_C);
+    rtc_out(RTC_REG_B, rtc_in(RTC_REG_B) | bit);
+}
+
+static inline void rtc_dis(int bit)
+{
+    rtc_out(RTC_REG_B, rtc_in(RTC_REG_B) & ~bit);
+}
+
+static inline int rtc_uip()
+{
+    return (rtc_in(RTC_REG_A) & REG_A_UIP) != 0;
+}
+
+static inline void rtc_intr_wait(int bit)
+{
+    unsigned char x;
+    do
+       x = rtc_in(RTC_REG_C);
+    while ((x & bit) != bit);
+}
+
+static inline void rtc_bin()
+{
+    rtc_en(REG_B_DM);
+}
+
+static inline void rtc_bcd()
+{
+    rtc_dis(REG_B_DM);
+}
+
+static inline void rtc_set(int h, int m, int s)
+{
+    rtc_en(REG_B_SET);
+    rtc_out(RTC_HOURS, h);
+    rtc_out(RTC_MINUTES, m);
+    rtc_out(RTC_SECONDS, s);
+    rtc_dis(REG_B_SET);
+}
+
+static inline void rtc_set_alarm(int h, int m, int s)
+{
+    rtc_out(RTC_HOURS_ALARM, h);
+    rtc_out(RTC_MINUTES_ALARM, m);
+    rtc_out(RTC_SECONDS_ALARM, s);
+}
+
+static void rtc_dump(char *s)
+{
+    printf ("%s, time %x:%x:%x | alarm %x:%x:%x | a=%x b=%x c=%x\n", s,
+            rtc_in(RTC_HOURS), rtc_in(RTC_MINUTES), rtc_in(RTC_SECONDS),
+            rtc_in(RTC_HOURS_ALARM), rtc_in(RTC_MINUTES_ALARM), rtc_in(RTC_SECONDS_ALARM),
+            rtc_in(RTC_REG_A), rtc_in(RTC_REG_B), rtc_in(RTC_REG_C));
+}
+
+static inline void rtc_check(int x)
+{
+    rtc_dump(x ? "ok" : "bad!");
+}
+
+static void rtc_reread(int b_set, int h_set, int b, int h)
+{
+    int m, s;
+    rtc_out(RTC_REG_B, b_set);
+    rtc_set(h_set, 0x20, 0x30);
+    rtc_out(RTC_REG_B, b);
+    if (((b_set ^ b) & REG_B_DM) == 0)
+        m = 0x20, s = 0x30;
+    else if (b_set & REG_B_DM)
+        m = 0x32, s = 0x48;
+    else
+        m = 20, s = 30;
+
+    int ok = (rtc_in(RTC_HOURS) == h &&
+              rtc_in(RTC_MINUTES) == m &&
+              rtc_in(RTC_SECONDS) == s);
+    if (!ok) printf("%x ", h_set), rtc_dump("bad!");
+}
+
+#define PM |0x80
+
+static unsigned char flags[4] = { REG_B_DM, 0, REG_B_DM|REG_B_24H, REG_B_24H };
+static char *flags_string[4] = { "bin/12h", "BCD/12h", "bin/24h", "BCD/24h" };
+static unsigned char hours[4][24] = {
+    { 12, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 
+      12 PM, 1 PM, 2 PM, 3 PM, 4 PM, 5 PM, 6 PM, 7 PM, 8 PM, 9 PM, 10 PM, 11 PM,
+    },
+    { 0x12, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0x10, 0x11, 
+      0x12 PM, 0x1 PM, 0x2 PM, 0x3 PM, 0x4 PM, 0x5 PM,
+      0x6 PM, 0x7 PM, 0x8 PM, 0x9 PM, 0x10 PM, 0x11 PM,
+    },
+    { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
+      12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23
+    },
+    { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0x10, 0x11,
+      0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x20, 0x21, 0x22, 0x23
+    }
+};
+
+int main()
+{
+    rtc_dis(REG_B_UIE | REG_B_PIE | REG_B_AIE);
+
+    rtc_dump("start");
+    printf("calibrating... ");
+    rtc_intr_wait(REG_C_UF);
+    long long cycles = rdtsc();
+    rtc_intr_wait(REG_C_UF);
+    long long cycles_per_sec = rdtsc() - cycles;
+    printf("%lld cycles/sec\n", cycles_per_sec);
+
+    /* UF is set when UIP resets.  */
+    while(rtc_uip());
+    rtc_intr_wait(REG_C_UF);
+    while(!rtc_uip());
+    while(rtc_uip());
+    rtc_check(rtc_in(RTC_REG_C) & REG_C_UF);
+
+    /* Test switching the DM and 24/12 bits.  */
+    for (int i = 0; i < 4; i++) {
+        /* The 24/12 bit cannot be changed without reinitializing the hours,
+         * so limit iteration to j < 2. */
+        for (int j = 0; j < 2; j++) {
+            printf ("set %s, get %s\n", flags_string[i], flags_string[i^j]);
+            for (int k = 0; k < 24; k++)
+                rtc_reread(flags[i], hours[i][k], flags[i^j], hours[i^j][k]);
+        }
+    }
+
+    /* Set bin/24hours */
+    rtc_bin();
+
+    /* Setting the clock resets UIP/UIE.  */
+    while(rtc_uip());
+    while(!rtc_uip());
+    rtc_en(REG_B_UIE);
+    rtc_check(rtc_uip());
+    rtc_check(rtc_in(RTC_REG_B) & REG_B_UIE);
+    rtc_set(0, 0, 0);
+    rtc_check(!rtc_uip());
+    rtc_check((rtc_in(RTC_REG_B) & REG_B_UIE) == 0);
+
+    /* The UIP bit should be set for at least 244 usec.  */
+    for (int i = 0; i < 5; i++) {
+        while(rtc_uip());
+        while(!rtc_uip());
+        cycles = rdtsc();
+        while(rtc_uip());
+        long long cycles_per_upd = rdtsc() - cycles;
+        printf("%lld cycles from UIP=1 to update (%lld usec)\n", cycles_per_upd,
+               cycles_per_upd * 1000000 / cycles_per_sec);
+        rtc_check(cycles_per_upd * 1000000 / cycles_per_sec > 244);
+
+        /* Too imprecise UIP time means the client will waste more time
+         * busy waiting.  Allow up to 3 ms (hardware does 2228 us).  */
+        rtc_check(cycles_per_upd * 1000000 / cycles_per_sec < 3000);
+    }
+
+    /* After completing a divider reset, the first update cycle begins
+     * one half-second later.
+     */
+    for (int i = 0; i < 5; i++) {
+        while(!rtc_uip());
+        rtc_out(RTC_REG_A, 0x70);
+        rtc_set(i, 0, 0);
+        rtc_out(RTC_REG_A, 0x20);
+        cycles = rdtsc();
+        while(!rtc_uip());
+        while(rtc_uip());
+        long long cycles_to_upd = rdtsc() - cycles;
+        rtc_check(rtc_in(RTC_SECONDS) == 1);
+        printf("%lld cycles from set to update (%lld msec)\n", cycles_to_upd,
+               cycles_to_upd * 1000 / cycles_per_sec);
+        rtc_check(cycles_to_upd * 1000 / cycles_per_sec > 400);
+        rtc_check(cycles_to_upd * 1000 / cycles_per_sec < 600);
+    }
+
+    /* Same as above, but test UF this time.  */
+    for (int i = 0; i < 5; i++) {
+        while(!rtc_uip());
+        rtc_out(RTC_REG_A, 0x70);
+        rtc_set(i, 0, 0);
+        rtc_out(RTC_REG_A, 0x20);
+        cycles = rdtsc();
+        rtc_intr_wait(REG_C_UF);
+        long long cycles_to_upd = rdtsc() - cycles;
+        rtc_check(rtc_in(RTC_SECONDS) == 1);
+        printf("%lld cycles from set to update interrupt (%lld msec)\n",
+               cycles_to_upd, cycles_to_upd * 1000 / cycles_per_sec);
+        rtc_check(cycles_to_upd * 1000 / cycles_per_sec > 400);
+        rtc_check(cycles_to_upd * 1000 / cycles_per_sec < 600);
+    }
+
+    /* Test consecutive alarms */
+    rtc_bin();
+    rtc_set(9, 0, 0);
+    rtc_in(RTC_REG_C);
+    for (int i = 2; i <= 5; i++) {
+        if (rtc_in(RTC_SECONDS) >= i)
+            break;
+        rtc_set_alarm(9, 0, i);
+        rtc_intr_wait(REG_C_AF);
+        rtc_check(rtc_in(RTC_SECONDS) == i);
+    }
+
+    /* Test spaced alarms in BCD mode */
+    rtc_bcd();
+    cycles = rdtsc();
+    rtc_set_alarm(9, 0, 0x10);
+    rtc_intr_wait(REG_C_AF);
+    long long cycles_to_alarm = rdtsc() - cycles;
+    rtc_check(rtc_in(RTC_SECONDS) == 0x10);
+    rtc_check(cycles_to_alarm / cycles_per_sec < 8);
+
+    /* Test spaced alarms in binary mode */
+    rtc_bin();
+    cycles = rdtsc();
+    rtc_set_alarm(9, 0, 18);
+    rtc_intr_wait(REG_C_AF);
+    cycles_to_alarm = rdtsc() - cycles;
+    rtc_check(rtc_in(RTC_SECONDS) == 18);
+    rtc_check(rtc_in(RTC_SECONDS_ALARM) == 18);
+    rtc_check(cycles_to_alarm / cycles_per_sec > 6);
+
+    /* Test consecutive alarms with wildcards */
+    for (int i = 0, secs = 18; i <= 3; i++) {
+        rtc_set_alarm(9, 0, 0xc0);
+        rtc_intr_wait(REG_C_AF);
+        rtc_check(rtc_in(RTC_SECONDS) == ++secs);
+        rtc_set_alarm(9, 0xc0, 0xc0);
+        rtc_intr_wait(REG_C_AF);
+        rtc_check(rtc_in(RTC_SECONDS) == ++secs);
+        rtc_set_alarm(0xc0, 0xc0, 0xc0);
+        rtc_intr_wait(REG_C_AF);
+        rtc_check(rtc_in(RTC_SECONDS) == ++secs);
+    }
+}
-- 
1.7.7.6

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

[Zhang, Yang Z]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> 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

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

[Paolo Bonzini]

On 02/23/2012 02:49 AM, Zhang, Yang Z wrote:
>> > 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. 

It doesn't seem hard to do, see my branch.

Paolo

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

[Paolo Bonzini]

On 02/23/2012 02:49 AM, Zhang, Yang Z wrote:
>> 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.

I'm not sure I understand, UIP and UF are indeed the same logic (UIP is
1 between the rising and falling edge; UF becomes 1 on the falling
edge), but they are also the same thing as the interrupt.  UIE only says
whether an interrupt is raised on the rising edge of UF.

So, merging the handling of UF and UIP makes sense, but separating the
two from the interrupt seems wrong.

Paolo

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

[Zhang, Yang Z]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> 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.
> 
Hi paolo
The DM and 24/12 test case assumes the changing of DM bit will reflect to RTC internal clock. But the datasheet said nothing will affect if you change it. Also, the current logic in qemu has the same assumption. Does this a bug or just by design?


best regards
yang

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

[Paolo Bonzini]

On 02/24/2012 01:55 AM, Zhang, Yang Z wrote:
> Hi paolo The DM and 24/12 test case assumes the changing of DM bit
> will reflect to RTC internal clock. But the datasheet said nothing
> will affect if you change it. Also, the current logic in qemu has the
> same assumption. Does this a bug or just by design?

Don't care about that part, it was more to test the BCD logic.

Paolo