[PATCH v3 0/3] mc146818rtc: fix timer interrupt reinjection

[PATCH v3 0/3] mc146818rtc: fix timer interrupt reinjection

[Philippe Mathieu-Daudé]

Since v2:
split Marcelo's patch in 3 to ease review, no logical change.

v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg651016.html
v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg650803.html

Marcelo Tosatti (1):
  mc146818rtc: fix timer interrupt reinjection

Philippe Mathieu-Daudé (2):
  mc146818rtc: Simplify by reordering if() statement
  mc146818rtc: Tidy up indentation

 hw/timer/mc146818rtc.c | 52 +++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

-- 
2.21.0

[PATCH v3 1/3] mc146818rtc: Simplify by reordering if() statement

[Philippe Mathieu-Daudé]

Reorder the if() statement, so further changes in this function
will be easier to read.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/timer/mc146818rtc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6cb378751b..2b6371faac 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -203,7 +203,12 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
 
     period = rtc_periodic_clock_ticks(s);
 
-    if (period) {
+    if (!period) {
+        s->irq_coalesced = 0;
+        timer_del(s->periodic_timer);
+        return;
+    }
+
         /* compute 32 khz clock */
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
@@ -263,10 +268,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
         next_irq_clock = cur_clock + period - lost_clock;
         s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1;
         timer_mod(s->periodic_timer, s->next_periodic_time);
-    } else {
-        s->irq_coalesced = 0;
-        timer_del(s->periodic_timer);
-    }
 }
 
 static void rtc_periodic_timer(void *opaque)
-- 
2.21.0

[PATCH v3 2/3] mc146818rtc: Tidy up indentation

[Philippe Mathieu-Daudé]

Re-indent, no functional change.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Patch easier to review with 'diff -b' (git show -b)
---
 hw/timer/mc146818rtc.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 2b6371faac..8da7fd1a50 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -209,22 +209,21 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
         return;
     }
 
-        /* compute 32 khz clock */
-        cur_clock =
-            muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
+    /* compute 32 khz clock */
+    cur_clock = muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
 
-        /*
-        * if the periodic timer's update is due to period re-configuration,
-        * we should count the clock since last interrupt.
-        */
-        if (old_period) {
-            int64_t last_periodic_clock, next_periodic_clock;
+    /*
+     * if the periodic timer's update is due to period re-configuration,
+     * we should count the clock since last interrupt.
+     */
+    if (old_period) {
+        int64_t last_periodic_clock, next_periodic_clock;
 
-            next_periodic_clock = muldiv64(s->next_periodic_time,
-                                    RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
-            last_periodic_clock = next_periodic_clock - old_period;
-            lost_clock = cur_clock - last_periodic_clock;
-            assert(lost_clock >= 0);
+        next_periodic_clock = muldiv64(s->next_periodic_time,
+                                RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
+        last_periodic_clock = next_periodic_clock - old_period;
+        lost_clock = cur_clock - last_periodic_clock;
+        assert(lost_clock >= 0);
         }
 
         /*
@@ -256,18 +255,18 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
                 rtc_coalesced_timer_update(s);
             }
         } else {
-           /*
+            /*
              * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
              * is not used, we should make the time progress anyway.
              */
             lost_clock = MIN(lost_clock, period);
         }
 
-        assert(lost_clock >= 0 && lost_clock <= period);
+    assert(lost_clock >= 0 && lost_clock <= period);
 
-        next_irq_clock = cur_clock + period - lost_clock;
-        s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1;
-        timer_mod(s->periodic_timer, s->next_periodic_time);
+    next_irq_clock = cur_clock + period - lost_clock;
+    s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1;
+    timer_mod(s->periodic_timer, s->next_periodic_time);
 }
 
 static void rtc_periodic_timer(void *opaque)
-- 
2.21.0

Re: [PATCH v3 2/3] mc146818rtc: Tidy up indentation

[Paolo Bonzini]

On 24/10/19 14:24, Philippe Mathieu-Daudé wrote:
> -        * if the periodic timer's update is due to period re-configuration,
> -        * we should count the clock since last interrupt.
> -        */
> -        if (old_period) {
> -            int64_t last_periodic_clock, next_periodic_clock;
> +    /*
> +     * if the periodic timer's update is due to period re-configuration,
> +     * we should count the clock since last interrupt.
> +     */
> +    if (old_period) {
> +        int64_t last_periodic_clock, next_periodic_clock;
>  
> -            next_periodic_clock = muldiv64(s->next_periodic_time,
> -                                    RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> -            last_periodic_clock = next_periodic_clock - old_period;
> -            lost_clock = cur_clock - last_periodic_clock;
> -            assert(lost_clock >= 0);
> +        next_periodic_clock = muldiv64(s->next_periodic_time,
> +                                RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> +        last_periodic_clock = next_periodic_clock - old_period;
> +        lost_clock = cur_clock - last_periodic_clock;
> +        assert(lost_clock >= 0);
>          }
>  
>          /*

Still not entirely tidy, is it?  I understand making Marcelo's fix just
move a brace, but in general you can review with "git show -b" to see
more clearly what's going on.  Therefore, it would make the most sense
to have just two patches, one reversing the if and one fixing the bug
(and both of them having indentation changes).

However, I'm preparing the pull request now so I think I'll just keep
Marcelo's version.

Paolo

Re: [PATCH v3 2/3] mc146818rtc: Tidy up indentation

[Philippe Mathieu-Daudé]

On 10/24/19 3:01 PM, Paolo Bonzini wrote:
> On 24/10/19 14:24, Philippe Mathieu-Daudé wrote:
>> -        * if the periodic timer's update is due to period re-configuration,
>> -        * we should count the clock since last interrupt.
>> -        */
>> -        if (old_period) {
>> -            int64_t last_periodic_clock, next_periodic_clock;
>> +    /*
>> +     * if the periodic timer's update is due to period re-configuration,
>> +     * we should count the clock since last interrupt.
>> +     */
>> +    if (old_period) {
>> +        int64_t last_periodic_clock, next_periodic_clock;
>>   
>> -            next_periodic_clock = muldiv64(s->next_periodic_time,
>> -                                    RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>> -            last_periodic_clock = next_periodic_clock - old_period;
>> -            lost_clock = cur_clock - last_periodic_clock;
>> -            assert(lost_clock >= 0);
>> +        next_periodic_clock = muldiv64(s->next_periodic_time,
>> +                                RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>> +        last_periodic_clock = next_periodic_clock - old_period;
>> +        lost_clock = cur_clock - last_periodic_clock;
>> +        assert(lost_clock >= 0);
>>           }
>>   
>>           /*
> 
> Still not entirely tidy, is it?  I understand making Marcelo's fix just
> move a brace, but in general you can review with "git show -b" to see
> more clearly what's going on.  Therefore, it would make the most sense
> to have just two patches, one reversing the if and one fixing the bug
> (and both of them having indentation changes).
> 
> However, I'm preparing the pull request now so I think I'll just keep
> Marcelo's version.

OK.

[PATCH v3 3/3] mc146818rtc: fix timer interrupt reinjection

[Philippe Mathieu-Daudé]

From: Marcelo Tosatti <mtosatti@redhat.com>

commit 369b41359af46bded5799c9ef8be2b641d92e043 broke timer interrupt
reinjection when there is no period change by the guest.

In that case, old_period is 0, which ends up zeroing irq_coalesced
(counter of reinjected interrupts).

The consequence is Windows 7 is unable to synchronize time via NTP.
Easily reproducible by playing a fullscreen video with cirrus and VNC.

Fix by not updating s->irq_coalesced when old_period is 0.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Message-Id: <20191010123008.GA19158@amt.cnet>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/timer/mc146818rtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 8da7fd1a50..adbc3b9d57 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -224,7 +224,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
         last_periodic_clock = next_periodic_clock - old_period;
         lost_clock = cur_clock - last_periodic_clock;
         assert(lost_clock >= 0);
-        }
 
         /*
          * s->irq_coalesced can change for two reasons:
@@ -261,6 +260,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
              */
             lost_clock = MIN(lost_clock, period);
         }
+    }
 
     assert(lost_clock >= 0 && lost_clock <= period);
 
-- 
2.21.0