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
>From f8d5e45941562cd8259523bd225c81a9dd841308 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
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
