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

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

[Paolo Bonzini]

Based on discussions with Peng Hao.  It is not clear to me what
causes UIP to get stuck in his case, but these changes test one
such scenario.

Paolo

Paolo Bonzini (4):
  rtc-test: cleanup register_b_set_flag test
  rtc-test: introduce more update tests
  mc146818rtc: simplify check_update_timer
  mc146818rtc: implement UIP latching as intended

 hw/timer/mc146818rtc.c |  37 ++++++------
 tests/rtc-test.c       | 156 +++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 145 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] rtc-test: cleanup register_b_set_flag test

[Paolo Bonzini]

Introduce set_datetime_bcd/assert_datetime_bcd, and handle
UIP correctly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/rtc-test.c | 76 ++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/tests/rtc-test.c b/tests/rtc-test.c
index e78f701..798cf5e 100644
--- a/tests/rtc-test.c
+++ b/tests/rtc-test.c
@@ -17,6 +17,8 @@
 #include "qemu/timer.h"
 #include "hw/timer/mc146818rtc_regs.h"
 
+#define UIP_HOLD_LENGTH           (8 * NANOSECONDS_PER_SECOND / 32768)
+
 static uint8_t base = 0x70;
 
 static int bcd2dec(int value)
@@ -297,16 +299,30 @@ static void alarm_time(void)
     g_assert(cmos_read(RTC_REG_C) == 0);
 }
 
+static void set_time_regs(int h, int m, int s)
+{
+    cmos_write(RTC_HOURS, h);
+    cmos_write(RTC_MINUTES, m);
+    cmos_write(RTC_SECONDS, s);
+}
+
 static void set_time(int mode, int h, int m, int s)
 {
-    /* set BCD 12 hour mode */
     cmos_write(RTC_REG_B, mode);
-
     cmos_write(RTC_REG_A, 0x76);
+    set_time_regs(h, m, s);
+    cmos_write(RTC_REG_A, 0x26);
+}
+
+static void set_datetime_bcd(int h, int min, int s, int d, int m, int y)
+{
     cmos_write(RTC_HOURS, h);
-    cmos_write(RTC_MINUTES, m);
+    cmos_write(RTC_MINUTES, min);
     cmos_write(RTC_SECONDS, s);
-    cmos_write(RTC_REG_A, 0x26);
+    cmos_write(RTC_YEAR, y & 0xFF);
+    cmos_write(RTC_CENTURY, y >> 8);
+    cmos_write(RTC_MONTH, m);
+    cmos_write(RTC_DAY_OF_MONTH, d);
 }
 
 #define assert_time(h, m, s) \
@@ -316,6 +332,17 @@ static void set_time(int mode, int h, int m, int s)
         g_assert_cmpint(cmos_read(RTC_SECONDS), ==, s); \
     } while(0)
 
+#define assert_datetime_bcd(h, min, s, d, m, y) \
+    do { \
+        g_assert_cmpint(cmos_read(RTC_HOURS), ==, h); \
+        g_assert_cmpint(cmos_read(RTC_MINUTES), ==, min); \
+        g_assert_cmpint(cmos_read(RTC_SECONDS), ==, s); \
+        g_assert_cmpint(cmos_read(RTC_DAY_OF_MONTH), ==, d); \
+        g_assert_cmpint(cmos_read(RTC_MONTH), ==, m); \
+        g_assert_cmpint(cmos_read(RTC_YEAR), ==, (y & 0xFF)); \
+        g_assert_cmpint(cmos_read(RTC_CENTURY), ==, (y >> 8)); \
+    } while(0)
+
 static void basic_12h_bcd(void)
 {
     /* set BCD 12 hour mode */
@@ -506,41 +533,30 @@ static void fuzz_registers(void)
 
 static void register_b_set_flag(void)
 {
+    if (cmos_read(RTC_REG_A) & REG_A_UIP) {
+        clock_step(UIP_HOLD_LENGTH + NANOSECONDS_PER_SECOND / 5);
+    }
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
+
     /* Enable binary-coded decimal (BCD) mode and SET flag in Register B*/
     cmos_write(RTC_REG_B, REG_B_24H | REG_B_SET);
 
-    cmos_write(RTC_REG_A, 0x76);
-    cmos_write(RTC_YEAR, 0x11);
-    cmos_write(RTC_CENTURY, 0x20);
-    cmos_write(RTC_MONTH, 0x02);
-    cmos_write(RTC_DAY_OF_MONTH, 0x02);
-    cmos_write(RTC_HOURS, 0x02);
-    cmos_write(RTC_MINUTES, 0x04);
-    cmos_write(RTC_SECONDS, 0x58);
-    cmos_write(RTC_REG_A, 0x26);
+    set_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
 
-    /* Since SET flag is still enabled, these are equality checks. */
-    g_assert_cmpint(cmos_read(RTC_HOURS), ==, 0x02);
-    g_assert_cmpint(cmos_read(RTC_MINUTES), ==, 0x04);
-    g_assert_cmpint(cmos_read(RTC_SECONDS), ==, 0x58);
-    g_assert_cmpint(cmos_read(RTC_DAY_OF_MONTH), ==, 0x02);
-    g_assert_cmpint(cmos_read(RTC_MONTH), ==, 0x02);
-    g_assert_cmpint(cmos_read(RTC_YEAR), ==, 0x11);
-    g_assert_cmpint(cmos_read(RTC_CENTURY), ==, 0x20);
+    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
+
+    /* Since SET flag is still enabled, time does not advance. */
+    clock_step(1000000000LL);
+    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
 
     /* Disable SET flag in Register B */
     cmos_write(RTC_REG_B, cmos_read(RTC_REG_B) & ~REG_B_SET);
 
-    g_assert_cmpint(cmos_read(RTC_HOURS), ==, 0x02);
-    g_assert_cmpint(cmos_read(RTC_MINUTES), ==, 0x04);
+    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
 
-    /* Since SET flag is disabled, this is an inequality check.
-     * We (reasonably) assume that no (sexagesimal) overflow occurs. */
-    g_assert_cmpint(cmos_read(RTC_SECONDS), >=, 0x58);
-    g_assert_cmpint(cmos_read(RTC_DAY_OF_MONTH), ==, 0x02);
-    g_assert_cmpint(cmos_read(RTC_MONTH), ==, 0x02);
-    g_assert_cmpint(cmos_read(RTC_YEAR), ==, 0x11);
-    g_assert_cmpint(cmos_read(RTC_CENTURY), ==, 0x20);
+    /* Since SET flag is disabled, the clock now advances.  */
+    clock_step(1000000000LL);
+    assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
 }
 
 #define RTC_PERIOD_CODE1    13   /* 8 Hz */
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] rtc-test: introduce more update tests

[Paolo Bonzini]

Test divider reset and UIP behavior.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/rtc-test.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

diff --git a/tests/rtc-test.c b/tests/rtc-test.c
index 798cf5e..d7a96cb 100644
--- a/tests/rtc-test.c
+++ b/tests/rtc-test.c
@@ -325,6 +325,30 @@ static void set_datetime_bcd(int h, int min, int s, int d, int m, int y)
     cmos_write(RTC_DAY_OF_MONTH, d);
 }
 
+static void set_datetime_dec(int h, int min, int s, int d, int m, int y)
+{
+    cmos_write(RTC_HOURS, h);
+    cmos_write(RTC_MINUTES, min);
+    cmos_write(RTC_SECONDS, s);
+    cmos_write(RTC_YEAR, y % 100);
+    cmos_write(RTC_CENTURY, y / 100);
+    cmos_write(RTC_MONTH, m);
+    cmos_write(RTC_DAY_OF_MONTH, d);
+}
+
+static void set_datetime(int mode, int h, int min, int s, int d, int m, int y)
+{
+    cmos_write(RTC_REG_B, mode);
+
+    cmos_write(RTC_REG_A, 0x76);
+    if (mode & REG_B_DM) {
+        set_datetime_dec(h, min, s, d, m, y);
+    } else {
+        set_datetime_bcd(h, min, s, d, m, y);
+    }
+    cmos_write(RTC_REG_A, 0x26);
+}
+
 #define assert_time(h, m, s) \
     do { \
         g_assert_cmpint(cmos_read(RTC_HOURS), ==, h); \
@@ -559,6 +583,60 @@ static void register_b_set_flag(void)
     assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
 }
 
+static void divider_reset(void)
+{
+    /* Enable binary-coded decimal (BCD) mode in Register B*/
+    cmos_write(RTC_REG_B, REG_B_24H);
+
+    /* Enter divider reset */
+    cmos_write(RTC_REG_A, 0x76);
+    set_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
+
+    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
+
+    /* Since divider reset flag is still enabled, these are equality checks. */
+    clock_step(1000000000LL);
+    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
+
+    /* The first update ends 500 ms after divider reset */
+    cmos_write(RTC_REG_A, 0x26);
+    clock_step(500000000LL - UIP_HOLD_LENGTH - 1);
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
+    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
+
+    clock_step(1);
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, !=, 0);
+    clock_step(UIP_HOLD_LENGTH);
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
+
+    assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
+}
+
+static void uip_stuck(void)
+{
+    set_datetime(REG_B_24H, 0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
+
+    /* The first update ends 500 ms after divider reset */
+    (void)cmos_read(RTC_REG_C);
+    clock_step(500000000LL);
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
+    assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
+
+    /* UF is now set.  */
+    cmos_write(RTC_HOURS_ALARM, 0x02);
+    cmos_write(RTC_MINUTES_ALARM, 0xC0);
+    cmos_write(RTC_SECONDS_ALARM, 0xC0);
+
+    /* Because the alarm will fire soon, reading register A will latch UIP.  */
+    clock_step(1000000000LL - UIP_HOLD_LENGTH / 2);
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, !=, 0);
+
+    /* Move the alarm far away.  This must not cause UIP to remain stuck!  */
+    cmos_write(RTC_HOURS_ALARM, 0x03);
+    clock_step(UIP_HOLD_LENGTH);
+    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
+}
+
 #define RTC_PERIOD_CODE1    13   /* 8 Hz */
 #define RTC_PERIOD_CODE2    15   /* 2 Hz */
 
@@ -625,7 +703,9 @@ int main(int argc, char **argv)
     qtest_add_func("/rtc/basic/bcd-12h", basic_12h_bcd);
     qtest_add_func("/rtc/set-year/20xx", set_year_20xx);
     qtest_add_func("/rtc/set-year/1980", set_year_1980);
-    qtest_add_func("/rtc/misc/register_b_set_flag", register_b_set_flag);
+    qtest_add_func("/rtc/update/register_b_set_flag", register_b_set_flag);
+    qtest_add_func("/rtc/update/divider-reset", divider_reset);
+    qtest_add_func("/rtc/update/uip-stuck", uip_stuck);
     qtest_add_func("/rtc/misc/fuzz-registers", fuzz_registers);
     qtest_add_func("/rtc/periodic/interrupt", periodic_timer);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/4] rtc-test: introduce more update tests

[Philippe Mathieu-Daudé]

On 07/25/2017 10:48 AM, Paolo Bonzini wrote:
> Test divider reset and UIP behavior.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/rtc-test.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/rtc-test.c b/tests/rtc-test.c
> index 798cf5e..d7a96cb 100644
> --- a/tests/rtc-test.c
> +++ b/tests/rtc-test.c
> @@ -325,6 +325,30 @@ static void set_datetime_bcd(int h, int min, int s, int d, int m, int y)
>       cmos_write(RTC_DAY_OF_MONTH, d);
>   }
>   

I'm not sure why this function is in tests/ ...

> +static void set_datetime_dec(int h, int min, int s, int d, int m, int y)
> +{

Following is set_time_regs()

> +    cmos_write(RTC_HOURS, h);
> +    cmos_write(RTC_MINUTES, min);
> +    cmos_write(RTC_SECONDS, s);

Maybe we can use here:

if (mode & REG_B_DM /* dec */) {

> +    cmos_write(RTC_YEAR, y % 100);
> +    cmos_write(RTC_CENTURY, y / 100);

} else /* bcd */ {
...
}

instead of having 2 set_datetime()

> +    cmos_write(RTC_MONTH, m);
> +    cmos_write(RTC_DAY_OF_MONTH, d);
> +}
> +
> +static void set_datetime(int mode, int h, int min, int s, int d, int m, int y)
> +{
> +    cmos_write(RTC_REG_B, mode);
> +
> +    cmos_write(RTC_REG_A, 0x76);
> +    if (mode & REG_B_DM) {
> +        set_datetime_dec(h, min, s, d, m, y);
> +    } else {
> +        set_datetime_bcd(h, min, s, d, m, y);
> +    }
> +    cmos_write(RTC_REG_A, 0x26);
> +}
> +
>   #define assert_time(h, m, s) \
>       do { \
>           g_assert_cmpint(cmos_read(RTC_HOURS), ==, h); \
> @@ -559,6 +583,60 @@ static void register_b_set_flag(void)
>       assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
>   }
>   
> +static void divider_reset(void)
> +{
> +    /* Enable binary-coded decimal (BCD) mode in Register B*/
> +    cmos_write(RTC_REG_B, REG_B_24H);
> +
> +    /* Enter divider reset */
> +    cmos_write(RTC_REG_A, 0x76);
> +    set_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
> +
> +    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
> +
> +    /* Since divider reset flag is still enabled, these are equality checks. */
> +    clock_step(1000000000LL);
> +    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
> +
> +    /* The first update ends 500 ms after divider reset */
> +    cmos_write(RTC_REG_A, 0x26);
> +    clock_step(500000000LL - UIP_HOLD_LENGTH - 1);
> +    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
> +    assert_datetime_bcd(0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
> +
> +    clock_step(1);
> +    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, !=, 0);
> +    clock_step(UIP_HOLD_LENGTH);
> +    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
> +
> +    assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
> +}
> +
> +static void uip_stuck(void)
> +{
> +    set_datetime(REG_B_24H, 0x02, 0x04, 0x58, 0x02, 0x02, 0x2011);
> +
> +    /* The first update ends 500 ms after divider reset */
> +    (void)cmos_read(RTC_REG_C);
> +    clock_step(500000000LL);
> +    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
> +    assert_datetime_bcd(0x02, 0x04, 0x59, 0x02, 0x02, 0x2011);
> +
> +    /* UF is now set.  */
> +    cmos_write(RTC_HOURS_ALARM, 0x02);
> +    cmos_write(RTC_MINUTES_ALARM, 0xC0);
> +    cmos_write(RTC_SECONDS_ALARM, 0xC0);
> +
> +    /* Because the alarm will fire soon, reading register A will latch UIP.  */
> +    clock_step(1000000000LL - UIP_HOLD_LENGTH / 2);
> +    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, !=, 0);
> +
> +    /* Move the alarm far away.  This must not cause UIP to remain stuck!  */
> +    cmos_write(RTC_HOURS_ALARM, 0x03);
> +    clock_step(UIP_HOLD_LENGTH);
> +    g_assert_cmpint(cmos_read(RTC_REG_A) & REG_A_UIP, ==, 0);
> +}
> +
>   #define RTC_PERIOD_CODE1    13   /* 8 Hz */
>   #define RTC_PERIOD_CODE2    15   /* 2 Hz */
>   
> @@ -625,7 +703,9 @@ int main(int argc, char **argv)
>       qtest_add_func("/rtc/basic/bcd-12h", basic_12h_bcd);
>       qtest_add_func("/rtc/set-year/20xx", set_year_20xx);
>       qtest_add_func("/rtc/set-year/1980", set_year_1980);
> -    qtest_add_func("/rtc/misc/register_b_set_flag", register_b_set_flag);
> +    qtest_add_func("/rtc/update/register_b_set_flag", register_b_set_flag);
> +    qtest_add_func("/rtc/update/divider-reset", divider_reset);
> +    qtest_add_func("/rtc/update/uip-stuck", uip_stuck);
>       qtest_add_func("/rtc/misc/fuzz-registers", fuzz_registers);
>       qtest_add_func("/rtc/periodic/interrupt", periodic_timer);
>   
> 

Re: [Qemu-devel] [PATCH 2/4] rtc-test: introduce more update tests

[Paolo Bonzini]

On 26/07/2017 03:28, Philippe Mathieu-Daudé wrote:
> On 07/25/2017 10:48 AM, Paolo Bonzini wrote:
>> Test divider reset and UIP behavior.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   tests/rtc-test.c | 82
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 81 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/rtc-test.c b/tests/rtc-test.c
>> index 798cf5e..d7a96cb 100644
>> --- a/tests/rtc-test.c
>> +++ b/tests/rtc-test.c
>> @@ -325,6 +325,30 @@ static void set_datetime_bcd(int h, int min, int
>> s, int d, int m, int y)
>>       cmos_write(RTC_DAY_OF_MONTH, d);
>>   }
>>   
> 
> I'm not sure why this function is in tests/ ...

Not sure I understand?

>> +static void set_datetime_dec(int h, int min, int s, int d, int m, int y)
>> +{
> 
> Following is set_time_regs()
> 
>> +    cmos_write(RTC_HOURS, h);
>> +    cmos_write(RTC_MINUTES, min);
>> +    cmos_write(RTC_SECONDS, s);
> 
> Maybe we can use here:
> 
> if (mode & REG_B_DM /* dec */) {
> 
>> +    cmos_write(RTC_YEAR, y % 100);
>> +    cmos_write(RTC_CENTURY, y / 100);
> 
> } else /* bcd */ {
> ...
> }
> 
> instead of having 2 set_datetime()

Because this function does not have a "mode" argument.

Paolo

[Qemu-devel] [PATCH 3/4] mc146818rtc: simplify check_update_timer

[Paolo Bonzini]

Move all the optimized cases together, since they all have UF=1 in
common.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/timer/mc146818rtc.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 1b8d3d7..ffb2c6a 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -291,26 +291,14 @@ static void check_update_timer(RTCState *s)
 
     /* From the data sheet: "Holding the dividers in reset prevents
      * interrupts from operating, while setting the SET bit allows"
-     * them to occur.  However, it will prevent an alarm interrupt
-     * from occurring, because the time of day is not updated.
+     * them to occur.
      */
     if ((s->cmos_data[RTC_REG_A] & 0x60) == 0x60) {
         timer_del(s->update_timer);
         return;
     }
-    if ((s->cmos_data[RTC_REG_C] & REG_C_UF) &&
-        (s->cmos_data[RTC_REG_B] & REG_B_SET)) {
-        timer_del(s->update_timer);
-        return;
-    }
-    if ((s->cmos_data[RTC_REG_C] & REG_C_UF) &&
-        (s->cmos_data[RTC_REG_C] & REG_C_AF)) {
-        timer_del(s->update_timer);
-        return;
-    }
 
     guest_nsec = get_guest_rtc_ns(s) % NANOSECONDS_PER_SECOND;
-    /* if UF is clear, reprogram to next second */
     next_update_time = qemu_clock_get_ns(rtc_clock)
         + NANOSECONDS_PER_SECOND - guest_nsec;
 
@@ -321,7 +309,17 @@ 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 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.
+         */
+        if ((s->cmos_data[RTC_REG_B] & REG_B_SET) ||
+            (s->cmos_data[RTC_REG_C] & REG_C_AF)) {
+            timer_del(s->update_timer);
+            return;
+        }
+
         /* UF is set, but AF is clear.  Program the timer to target
          * the alarm time.  */
         next_update_time = s->next_alarm_time;
-- 
1.8.3.1

[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 <pbonzini@redhat.com>
---
 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

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH 0/4] mc146818rtc: implement UIP latching as intended
Message-id: 1500990514-30326-1-git-send-email-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c07cd6a mc146818rtc: implement UIP latching as intended
f4817e4 mc146818rtc: simplify check_update_timer
fba1b07 rtc-test: introduce more update tests
cac9365 rtc-test: cleanup register_b_set_flag test

=== OUTPUT BEGIN ===
Checking PATCH 1/4: rtc-test: cleanup register_b_set_flag test...
ERROR: space required before the open parenthesis '('
#73: FILE: tests/rtc-test.c:344:
+    } while(0)

total: 1 errors, 0 warnings, 115 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/4: rtc-test: introduce more update tests...
Checking PATCH 3/4: mc146818rtc: simplify check_update_timer...
Checking PATCH 4/4: mc146818rtc: implement UIP latching as intended...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org