| Message ID | 20170412095111.11728-5-xiaoguangrong@tencent.com |
|---|---|
| State | New |
| Headers | show |
On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: > From: Xiao Guangrong <xiaoguangrong@tencent.com> > > Move the x86 specific code in periodic_timer_update() to a common place, > the actual logic is not changed > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> > --- > hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++-------------------- > 1 file changed, 66 insertions(+), 46 deletions(-) > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c > index 3bf559d..d7b7c56 100644 > --- a/hw/timer/mc146818rtc.c > +++ b/hw/timer/mc146818rtc.c > @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque) > > rtc_coalesced_timer_update(s); > } > + > +static int64_t > +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) > +{ > + if (period != s->period) { > + int64_t scale_lost_clock; > + int current_irq_coalesced = s->irq_coalesced; > + > + s->irq_coalesced = (current_irq_coalesced * s->period) / period; > + > + /* > + * calculate the lost clock after it is scaled which should be > + * compensated in the next interrupt. > + */ > + scale_lost_clock = current_irq_coalesced * s->period - > + s->irq_coalesced * period; > + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " > + "are compensated.\n", current_irq_coalesced, > + s->irq_coalesced, scale_lost_clock); > + lost_clock += scale_lost_clock; > + s->period = period; This should be moved up to the caller. Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is zero. So I *think* what you get is equivalent to if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) { return; } /* ... comment ... */ lost_interrupt = (s->irq_coalesced * s->period) / period; lost_clock += (s->irq_coalesced * s->period) % period; lost_interrupt += lost_clock / period; lost_clock %= period; s->irq_coalesced = load_interrupt; rtc_coalesced_timer_update(s); or equivalently: lost_clock += s->irq_coalesced * s->period; s->irq_coalesced = lost_clock / period; lost_clock %= period; rtc_coalesced_timer_update(s); I think you should probably merge these three patches and document the resulting logic, because it's simpler than building it a patch at a time. Thanks, Paolo > + } > + > + /* > + * if more than period clocks were passed, i.e, the timer interrupt > + * has been lost, we should catch up the time. > + */ > + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW && > + (lost_clock / period)) { > + int lost_interrupt = lost_clock / period; > + > + s->irq_coalesced += lost_interrupt; > + lost_clock -= lost_interrupt * period; > + if (lost_interrupt) { > + DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs " > + "increased to %d\n", lost_interrupt, s->irq_coalesced); > + rtc_coalesced_timer_update(s); > + } > + } > + > + return lost_clock; > +} > + > +static void arch_periodic_timer_disable(RTCState *s) > +{ > + s->irq_coalesced = 0; > +} > +#else > +static int64_t > +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) > +{ > + return lost_clock; > +} > + > +static void arch_periodic_timer_disable(RTCState *s) > +{ > +} > #endif > > static int period_code_to_clock(int period_code) > @@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period) > if (period_code != 0 > && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) { > period = period_code_to_clock(period_code); > -#ifdef TARGET_I386 > - if (period != s->period) { > - int current_irq_coalesced = s->irq_coalesced; > - > - s->irq_coalesced = (current_irq_coalesced * s->period) / period; > > - /* > - * calculate the lost clock after it is scaled which should be > - * compensated in the next interrupt. > - */ > - lost_clock += current_irq_coalesced * s->period - > - s->irq_coalesced * period; > - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " > - "are compensated.\n", > - current_irq_coalesced, s->irq_coalesced, lost_clock); > - } > - s->period = period; > -#endif > /* compute 32 khz clock */ > cur_clock = > muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND); > @@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period) > > /* calculate the clock since last interrupt. */ > lost_clock += cur_clock - last_periodic_clock; > - > -#ifdef TARGET_I386 > - /* > - * if more than period clocks were passed, i.e, the timer interrupt > - * has been lost, we should catch up the time. > - */ > - if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW && > - (lost_clock / period)) { > - int lost_interrupt = lost_clock / period; > - > - s->irq_coalesced += lost_interrupt; > - lost_clock -= lost_interrupt * period; > - if (lost_interrupt) { > - DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs " > - "increased to %d\n", lost_interrupt, > - s->irq_coalesced); > - rtc_coalesced_timer_update(s); > - } > - } else > -#endif > - /* > - * 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 = arch_periodic_timer_update(s, period, lost_clock); > + > + /* > + * we should make the time progress anyway. > + */ > + lost_clock = MIN(lost_clock, period); > + assert(lost_clock >= 0); > + > next_irq_clock = cur_clock + period - lost_clock; > s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND, > RTC_CLOCK_RATE) + 1; > timer_mod(s->periodic_timer, s->next_periodic_time); > } else { > -#ifdef TARGET_I386 > - s->irq_coalesced = 0; > -#endif > + arch_periodic_timer_disable(s); > timer_del(s->periodic_timer); > } > } >
On 05/03/2017 11:39 PM, Paolo Bonzini wrote: > > > On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: >> From: Xiao Guangrong <xiaoguangrong@tencent.com> >> >> Move the x86 specific code in periodic_timer_update() to a common place, >> the actual logic is not changed >> >> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> >> --- >> hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++-------------------- >> 1 file changed, 66 insertions(+), 46 deletions(-) >> >> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c >> index 3bf559d..d7b7c56 100644 >> --- a/hw/timer/mc146818rtc.c >> +++ b/hw/timer/mc146818rtc.c >> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque) >> >> rtc_coalesced_timer_update(s); >> } >> + >> +static int64_t >> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) >> +{ >> + if (period != s->period) { >> + int64_t scale_lost_clock; >> + int current_irq_coalesced = s->irq_coalesced; >> + >> + s->irq_coalesced = (current_irq_coalesced * s->period) / period; >> + >> + /* >> + * calculate the lost clock after it is scaled which should be >> + * compensated in the next interrupt. >> + */ >> + scale_lost_clock = current_irq_coalesced * s->period - >> + s->irq_coalesced * period; >> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " >> + "are compensated.\n", current_irq_coalesced, >> + s->irq_coalesced, scale_lost_clock); >> + lost_clock += scale_lost_clock; >> + s->period = period; > > This should be moved up to the caller. It should not. As you pointed out below, all these code are only needed for LOST_TICK_POLICY_SLEW that is x86 specific. Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW, Unnecessary branch checks will little slow down other architectures, but i think it is acceptable, right? :) > > Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is > zero. So I *think* what you get is equivalent to > > if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) { > return; > } > > /* ... comment ... */ > lost_interrupt = (s->irq_coalesced * s->period) / period; > lost_clock += (s->irq_coalesced * s->period) % period; > lost_interrupt += lost_clock / period; > lost_clock %= period; > > s->irq_coalesced = load_interrupt; > rtc_coalesced_timer_update(s); > > or equivalently: > > lost_clock += s->irq_coalesced * s->period; > > s->irq_coalesced = lost_clock / period; > lost_clock %= period; > rtc_coalesced_timer_update(s); > Exactly right, it is much better, will apply it. > I think you should probably merge these three patches and document the > resulting logic, because it's simpler than building it a patch at a time. Okay, i will consider it carefully in the next version. Thank you, Paolo!
On 04/05/2017 05:25, Xiao Guangrong wrote: > > > On 05/03/2017 11:39 PM, Paolo Bonzini wrote: >> >> >> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: >>> From: Xiao Guangrong <xiaoguangrong@tencent.com> >>> >>> Move the x86 specific code in periodic_timer_update() to a common place, >>> the actual logic is not changed >>> >>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> >>> --- >>> hw/timer/mc146818rtc.c | 112 >>> +++++++++++++++++++++++++++++-------------------- >>> 1 file changed, 66 insertions(+), 46 deletions(-) >>> >>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c >>> index 3bf559d..d7b7c56 100644 >>> --- a/hw/timer/mc146818rtc.c >>> +++ b/hw/timer/mc146818rtc.c >>> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque) >>> rtc_coalesced_timer_update(s); >>> } >>> + >>> +static int64_t >>> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) >>> +{ >>> + if (period != s->period) { >>> + int64_t scale_lost_clock; >>> + int current_irq_coalesced = s->irq_coalesced; >>> + >>> + s->irq_coalesced = (current_irq_coalesced * s->period) / >>> period; >>> + >>> + /* >>> + * calculate the lost clock after it is scaled which should be >>> + * compensated in the next interrupt. >>> + */ >>> + scale_lost_clock = current_irq_coalesced * s->period - >>> + s->irq_coalesced * period; >>> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld >>> clocks " >>> + "are compensated.\n", current_irq_coalesced, >>> + s->irq_coalesced, scale_lost_clock); >>> + lost_clock += scale_lost_clock; >>> + s->period = period; >> >> This should be moved up to the caller. > > It should not. As you pointed out below, all these code are only needed > for LOST_TICK_POLICY_SLEW that is x86 specific. > > Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without > "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW, > Unnecessary branch checks will little slow down other architectures, > but i think it is acceptable, right? :) Yeah, the #ifdef TARGET_I386 is only needed because of the APIC interface. This one doesn't really need the #ifdef. But you're right that it shouldn't be moved to the caller. Paolo >> >> Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is >> zero. So I *think* what you get is equivalent to >> >> if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) { >> return; >> } >> >> /* ... comment ... */ >> lost_interrupt = (s->irq_coalesced * s->period) / period; >> lost_clock += (s->irq_coalesced * s->period) % period; >> lost_interrupt += lost_clock / period; >> lost_clock %= period; >> >> s->irq_coalesced = load_interrupt; >> rtc_coalesced_timer_update(s); >> >> or equivalently: >> >> lost_clock += s->irq_coalesced * s->period; >> >> s->irq_coalesced = lost_clock / period; >> lost_clock %= period; >> rtc_coalesced_timer_update(s); >> > > Exactly right, it is much better, will apply it. > >> I think you should probably merge these three patches and document the >> resulting logic, because it's simpler than building it a patch at a time. > > Okay, i will consider it carefully in the next version. > > Thank you, Paolo! >
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c index 3bf559d..d7b7c56 100644 --- a/hw/timer/mc146818rtc.c +++ b/hw/timer/mc146818rtc.c @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque) rtc_coalesced_timer_update(s); } + +static int64_t +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) +{ + if (period != s->period) { + int64_t scale_lost_clock; + int current_irq_coalesced = s->irq_coalesced; + + s->irq_coalesced = (current_irq_coalesced * s->period) / period; + + /* + * calculate the lost clock after it is scaled which should be + * compensated in the next interrupt. + */ + scale_lost_clock = current_irq_coalesced * s->period - + s->irq_coalesced * period; + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " + "are compensated.\n", current_irq_coalesced, + s->irq_coalesced, scale_lost_clock); + lost_clock += scale_lost_clock; + s->period = period; + } + + /* + * if more than period clocks were passed, i.e, the timer interrupt + * has been lost, we should catch up the time. + */ + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW && + (lost_clock / period)) { + int lost_interrupt = lost_clock / period; + + s->irq_coalesced += lost_interrupt; + lost_clock -= lost_interrupt * period; + if (lost_interrupt) { + DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs " + "increased to %d\n", lost_interrupt, s->irq_coalesced); + rtc_coalesced_timer_update(s); + } + } + + return lost_clock; +} + +static void arch_periodic_timer_disable(RTCState *s) +{ + s->irq_coalesced = 0; +} +#else +static int64_t +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) +{ + return lost_clock; +} + +static void arch_periodic_timer_disable(RTCState *s) +{ +} #endif static int period_code_to_clock(int period_code) @@ -175,24 +232,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period) if (period_code != 0 && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) { period = period_code_to_clock(period_code); -#ifdef TARGET_I386 - if (period != s->period) { - int current_irq_coalesced = s->irq_coalesced; - - s->irq_coalesced = (current_irq_coalesced * s->period) / period; - /* - * calculate the lost clock after it is scaled which should be - * compensated in the next interrupt. - */ - lost_clock += current_irq_coalesced * s->period - - s->irq_coalesced * period; - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " - "are compensated.\n", - current_irq_coalesced, s->irq_coalesced, lost_clock); - } - s->period = period; -#endif /* compute 32 khz clock */ cur_clock = muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND); @@ -219,42 +259,22 @@ periodic_timer_update(RTCState *s, int64_t current_time, int old_period) /* calculate the clock since last interrupt. */ lost_clock += cur_clock - last_periodic_clock; - -#ifdef TARGET_I386 - /* - * if more than period clocks were passed, i.e, the timer interrupt - * has been lost, we should catch up the time. - */ - if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW && - (lost_clock / period)) { - int lost_interrupt = lost_clock / period; - - s->irq_coalesced += lost_interrupt; - lost_clock -= lost_interrupt * period; - if (lost_interrupt) { - DPRINTF_C("cmos: compensate %d interrupts, coalesced irqs " - "increased to %d\n", lost_interrupt, - s->irq_coalesced); - rtc_coalesced_timer_update(s); - } - } else -#endif - /* - * 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 = arch_periodic_timer_update(s, period, lost_clock); + + /* + * we should make the time progress anyway. + */ + lost_clock = MIN(lost_clock, period); + assert(lost_clock >= 0); + next_irq_clock = cur_clock + period - lost_clock; s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND, RTC_CLOCK_RATE) + 1; timer_mod(s->periodic_timer, s->next_periodic_time); } else { -#ifdef TARGET_I386 - s->irq_coalesced = 0; -#endif + arch_periodic_timer_disable(s); timer_del(s->periodic_timer); } }