[Qemu-devel] [PATCH 4/4] mc146818rtc: implement UIP latching as intended

[Paolo Bonzini]

In some cases, the guest can observe the wrong ordering of UIP and
interrupts.  This can happen if the VCPU exit is timed like this
(the interrupt is supposed to happen at time t):

           iothread                 VCPU
                                  ... wait for interrupt ...
t-100ns                           read register A
t          wake up, take BQL
t+100ns                             update_in_progress
                                      return false
                                    return UIP=0
           trigger interrupt

The interrupt is late; the VCPU expected the falling edge of UIP to
happen after the interrupt.  update_in_progress is already trying to
cover this case by latching UIP if the timer is going to fire soon,
and the fix is documented in the commit message for commit 56038ef623
("RTC: Update the RTC clock only when reading it", 2012-09-10).  It
cannot be tested with qtest, because its timing of interrupts vs. reads
is exact.

However, the implementation was incorrect because UIP cmos_ioport_read
cleared register A instead of leaving that to rtc_update_timer.  Fixing
the implementation of cmos_ioport_read to match the commit message,
however, breaks the "uip-stuck" test case from the previous patch.
To fix it, skip update timer optimizations if UIP has been latched.

Signed-off-by: Paolo Bonzini <address@hidden>
---
 hw/timer/mc146818rtc.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index ffb2c6a..82843ed 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -294,6 +294,7 @@ static void check_update_timer(RTCState *s)
      * them to occur.
      */
     if ((s->cmos_data[RTC_REG_A] & 0x60) == 0x60) {
+        assert((s->cmos_data[RTC_REG_A] & REG_A_UIP) == 0);
         timer_del(s->update_timer);
         return;
     }
@@ -309,8 +310,12 @@ static void check_update_timer(RTCState *s)
     s->next_alarm_time = next_update_time +
                          (next_alarm_sec - 1) * NANOSECONDS_PER_SECOND;
 
-    /* If UF is already set, we might be able to optimize. */
-    if (s->cmos_data[RTC_REG_C] & REG_C_UF) {
+    /* If update_in_progress latched the UIP bit, we must keep the timer
+     * programmed to the next second, so that UIP is cleared.  Otherwise,
+     * if UF is already set, we might be able to optimize.
+     */
+    if (!(s->cmos_data[RTC_REG_A] & REG_A_UIP) &&
+        (s->cmos_data[RTC_REG_C] & REG_C_UF)) {
         /* If AF cannot change (i.e. either it is set already, or SET=1
          * and then the time of day is not updated), nothing to do.
          */
@@ -725,12 +730,10 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr 
addr,
             ret = s->cmos_data[s->cmos_index];
             break;
         case RTC_REG_A:
+            ret = s->cmos_data[s->cmos_index];
             if (update_in_progress(s)) {
-                s->cmos_data[s->cmos_index] |= REG_A_UIP;
-            } else {
-                s->cmos_data[s->cmos_index] &= ~REG_A_UIP;
+                ret |= REG_A_UIP;
             }
-            ret = s->cmos_data[s->cmos_index];
             break;
         case RTC_REG_C:
             ret = s->cmos_data[s->cmos_index];
-- 
1.8.3.1