Re: [Qemu-arm] [PATCH v8 2/4] hw/ptimer: Perform tick and counter wrap around if timer already expired

[Dmitry Osipenko]

06.01.2016 15:17, Peter Crosthwaite пишет:
> On Tue, Jan 05, 2016 at 05:33:27AM +0300, Dmitry Osipenko wrote:
> > ptimer_get_count() might be called while QEMU timer already been expired.
> > In that case ptimer would return counter = 0, which might be undesirable
> > in case of polled timer. Do counter wrap around for periodic timer to keep
> > it distributed.
> >
> > In addition, there is no reason to keep expired timer tick deferred, so
> > just perform the tick from ptimer_get_count().
> >
> > Signed-off-by: Dmitry Osipenko <address@hidden>
> > ---
> >   hw/core/ptimer.c | 35 +++++++++++++++++++++++++++++------
> >   1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> > index 035af97..96a6c7a 100644
> > --- a/hw/core/ptimer.c
> > +++ b/hw/core/ptimer.c
> > @@ -85,15 +85,21 @@ static void ptimer_tick(void *opaque)
> >
> >   uint64_t ptimer_get_count(ptimer_state *s)
> >   {
> > +    int enabled = s->enabled;
>
> Variable localisation not needed.
>
> >       int64_t now;
> > +    int64_t next;
> >       uint64_t counter;
> > +    int expired;
> > +    int oneshot;
>
> Variable defs can be localised to the if (enabled) (even though now
> in original code doesn't do that).

Yeah, I just tried to keep original style here.

> > -    if (s->enabled) {
> > +    if (enabled) {
> >           now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        next = s->next_event;
> > +        expired = (now - next >= 0);
> > +        oneshot = (enabled == 2);
> >           /* Figure out the current counter value.  */
>
> This comment is now out of place.

Okay, will fix it.

> > -        if (now - s->next_event > 0
> > -            || s->period == 0) {
> > -            /* Prevent timer underflowing if it should already have
> > +        if (s->period == 0 || (expired && oneshot)) {
> > +            /* Prevent one-shot timer underflowing if it should already have
> >                  triggered.  */
> >               counter = 0;
> >           } else {
> > @@ -114,12 +120,12 @@ uint64_t ptimer_get_count(ptimer_state *s)
> >                  backwards.
> >               */
> >
> > -            if ((s->enabled == 1) && (s->limit * period < 10000)) {
> > +            if (!oneshot && (s->limit * period < 10000)) {
> >                   period = 10000 / s->limit;
> >                   period_frac = 0;
> >               }
> >
> > -            rem = s->next_event - now;
> > +            rem = expired ? now - next : next - now;
> >               div = period;
> >
> >               clz1 = clz64(rem);
> > @@ -139,6 +145,23 @@ uint64_t ptimer_get_count(ptimer_state *s)
> >                       div += 1;
> >               }
> >               counter = rem / div;
> > +
> > +            if (expired) {
> > +                /* Wrap around periodic counter.  */
> > +                counter = s->delta = s->limit - counter % s->limit;
>
> Why do you update the delta here?

Because we would want to schedule next tick based on current wrapped around 
counter value and not some arbitrary delta.

> Also can you just get ptimer_reload to do the modulo math for you? If the
> timer is !oneshot and expired, then you call ptimer_reload anyway,
> which will update next_event. When the expired test returns false
> you can just reliably use the original logic involving now and next.

Yes, that's what I changed in V9. Have you received it?

https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00272.html

> > +            }
> > +        }
> > +
> > +        if (expired) {
> > +            if (oneshot) {
>
> This if-else has a lot of common structure with the one above. I think
> they could be merged.

That's a good suggestion, will do it in V10. Thanks.

> Regards,
> Peter
>
> > +                ptimer_tick(s);
> > +            } else {
> > +                /* Don't use ptimer_tick() for the periodic timer since it
> > +                 * would reset the delta value.
> > +                 */
> > +                ptimer_trigger(s);
> > +                ptimer_reload(s);
> > +            }
> >           }
> >       } else {
> >           counter = s->delta;
> > --
> > 2.6.4


--
Dmitry