Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update

[Xiao Guangrong]

On 05/03/2017 11:39 PM, Paolo Bonzini wrote:
> On 12/04/2017 11:51, address@hidden wrote:
> > From: Xiao Guangrong <address@hidden>
> >
> > Move the x86 specific code in periodic_timer_update() to a common place,
> > the actual logic is not changed
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >   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!