* [PATCH] mc146818rtc: fix timer interrupt reinjection @ 2019-10-09 18:40 Marcelo Tosatti 2019-10-09 21:13 ` Paolo Bonzini 0 siblings, 1 reply; 3+ messages in thread From: Marcelo Tosatti @ 2019-10-09 18:40 UTC (permalink / raw) To: qemu-devel; +Cc: Tai Yunfang, Xiao Guangrong, Vadim Rozenfeld, Paolo Bonzini 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> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c index 6cb378751b..aabffa8c67 100644 --- a/hw/timer/mc146818rtc.c +++ b/hw/timer/mc146818rtc.c @@ -241,7 +241,10 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) s->period = period; lost_clock += old_irq_coalesced * old_period; - s->irq_coalesced = lost_clock / s->period; + if (old_period) { + s->irq_coalesced = lost_clock / s->period; + } + lost_clock %= s->period; if (old_irq_coalesced != s->irq_coalesced || old_period != s->period) { ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mc146818rtc: fix timer interrupt reinjection 2019-10-09 18:40 [PATCH] mc146818rtc: fix timer interrupt reinjection Marcelo Tosatti @ 2019-10-09 21:13 ` Paolo Bonzini 2019-10-10 12:28 ` Marcelo Tosatti 0 siblings, 1 reply; 3+ messages in thread From: Paolo Bonzini @ 2019-10-09 21:13 UTC (permalink / raw) To: Marcelo Tosatti, qemu-devel; +Cc: Tai Yunfang, Xiao Guangrong, Vadim Rozenfeld On 09/10/19 20:40, Marcelo Tosatti wrote: > s->period = period; > lost_clock += old_irq_coalesced * old_period; > - s->irq_coalesced = lost_clock / s->period; > + if (old_period) { > + s->irq_coalesced = lost_clock / s->period; > + } > + > lost_clock %= s->period; > if (old_irq_coalesced != s->irq_coalesced || > old_period != s->period) { I think none of the code in the "if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {" matters if old_period == 0 (and lost_clock will always be 0). So perhaps we should place all that big "if" under the existing "if (old_period)"? Or even something like: diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c index 6cb378751b..3337a8da98 100644 --- a/hw/timer/mc146818rtc.c +++ b/hw/timer/mc146818rtc.c @@ -202,25 +202,33 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) int64_t cur_clock, next_irq_clock, lost_clock = 0; period = rtc_periodic_clock_ticks(s); + if (old_period && old_period == period) { + return; + } - if (period) { - /* compute 32 khz clock */ - cur_clock = - muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND); + if (!period) { + s->irq_coalesced = 0; + timer_del(s->periodic_timer); + return; - /* - * 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); - } + } + + /* 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; + + 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); /* * s->irq_coalesced can change for two reasons: @@ -243,13 +251,10 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) lost_clock += old_irq_coalesced * old_period; s->irq_coalesced = lost_clock / s->period; lost_clock %= s->period; - if (old_irq_coalesced != s->irq_coalesced || - old_period != s->period) { - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " - "period scaled from %d to %d\n", old_irq_coalesced, - s->irq_coalesced, old_period, s->period); - rtc_coalesced_timer_update(s); - } + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " + "period scaled from %d to %d\n", old_irq_coalesced, + s->irq_coalesced, old_period, s->period); + rtc_coalesced_timer_update(s); } else { /* * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW @@ -257,16 +262,12 @@ 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); - - 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); } + + 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) Best read with "diff -b". Paolo ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mc146818rtc: fix timer interrupt reinjection 2019-10-09 21:13 ` Paolo Bonzini @ 2019-10-10 12:28 ` Marcelo Tosatti 0 siblings, 0 replies; 3+ messages in thread From: Marcelo Tosatti @ 2019-10-10 12:28 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Tai Yunfang, Xiao Guangrong, qemu-devel, Vadim Rozenfeld On Wed, Oct 09, 2019 at 11:13:16PM +0200, Paolo Bonzini wrote: > On 09/10/19 20:40, Marcelo Tosatti wrote: > > s->period = period; > > lost_clock += old_irq_coalesced * old_period; > > - s->irq_coalesced = lost_clock / s->period; > > + if (old_period) { > > + s->irq_coalesced = lost_clock / s->period; > > + } > > + > > lost_clock %= s->period; > > if (old_irq_coalesced != s->irq_coalesced || > > old_period != s->period) { > > I think none of the code in the "if (s->lost_tick_policy == > LOST_TICK_POLICY_SLEW) {" matters if old_period == 0 (and lost_clock > will always be 0). So perhaps we should place all that big "if" under > the existing "if (old_period)"? > > Or even something like: This skips reprogramming the timer if the period is equal. Sending v2 based on your suggestions and patch below. > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c > index 6cb378751b..3337a8da98 100644 > --- a/hw/timer/mc146818rtc.c > +++ b/hw/timer/mc146818rtc.c > @@ -202,25 +202,33 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) > int64_t cur_clock, next_irq_clock, lost_clock = 0; > > period = rtc_periodic_clock_ticks(s); > + if (old_period && old_period == period) { > + return; > + } > > - if (period) { > - /* compute 32 khz clock */ > - cur_clock = > - muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND); > + if (!period) { > + s->irq_coalesced = 0; > + timer_del(s->periodic_timer); > + return; > > - /* > - * 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); > - } > + } > + > + /* 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; > + > + 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); > > /* > * s->irq_coalesced can change for two reasons: > @@ -243,13 +251,10 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) > lost_clock += old_irq_coalesced * old_period; > s->irq_coalesced = lost_clock / s->period; > lost_clock %= s->period; > - if (old_irq_coalesced != s->irq_coalesced || > - old_period != s->period) { > - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " > - "period scaled from %d to %d\n", old_irq_coalesced, > - s->irq_coalesced, old_period, s->period); > - rtc_coalesced_timer_update(s); > - } > + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " > + "period scaled from %d to %d\n", old_irq_coalesced, > + s->irq_coalesced, old_period, s->period); > + rtc_coalesced_timer_update(s); > } else { > /* > * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW > @@ -257,16 +262,12 @@ 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); > - > - 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); > } > + > + 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) > > > Best read with "diff -b". > > Paolo ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-10 12:56 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-09 18:40 [PATCH] mc146818rtc: fix timer interrupt reinjection Marcelo Tosatti 2019-10-09 21:13 ` Paolo Bonzini 2019-10-10 12:28 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).