Re: [Qemu-arm] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer

[Dmitry Osipenko]

08.03.2016 06:43, Peter Crosthwaite пишет:
> On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <address@hidden> wrote:
> > 30.01.2016 19:43, Dmitry Osipenko пишет:
> > > Due to rounding down performed by ptimer_get_count, it returns counter - 1
> > > for
> > > the active timer. That's incorrect because counter should decrement only
> > > after
> > > period been expired, not before. I.e. if running timer has been loaded
> > > with
> > > value X, then timer counter should stay with X until period expired and
> > > decrement after. Fix this by adding 1 to the counter value for the active
> > > and
> > > unexpired timer.
> > >
> > > Signed-off-by: Dmitry Osipenko <address@hidden>
> > > ---
> > >    hw/core/ptimer.c | 2 +-
> > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> > > index 62f8cb1..b2044fb 100644
> > > --- a/hw/core/ptimer.c
> > > +++ b/hw/core/ptimer.c
> > > @@ -136,7 +136,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
> > >                    if ((uint32_t)(period_frac << shift))
> > >                        div += 1;
> > >                }
> > > -            counter = rem / div;
> > > +            counter = rem / div + (expired ? 0 : 1);
> > >
> > >                if (expired && counter != 0) {
> > >                    /* Wrap around periodic counter.  */
> >
> > Noticed one nit here:
> >
> > There is possibility to return timer counter = limit + 1, if the following
> > ptimer calls execute in less than 1ns.
> >
> >          ptimer_run(t, 1);
> >          // counter = 91, if set() count executed in less than 1ns
> >          ptimer_set_count(t, 90);
> >          counter = ptimer_get_count(t);
> >
> > Likely, it would be impossible to trigger that issue on a real current
> > machine.
> > But the fix is trivial, I'll incorporate it in V13 if it looks fine:
> >
> > ---
> > @@ -76,20 +76,20 @@ static void ptimer_tick(void *opaque)
> >   {
> >       ptimer_state *s = (ptimer_state *)opaque;
> >       s->delta = 0;
> >       ptimer_reload(s);
> >   }
> >
> >   uint64_t ptimer_get_count(ptimer_state *s)
> >   {
> > +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >       uint64_t counter;
> >
> > -    if (s->enabled && s->delta != 0) {
> > -        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    if (s->enabled && s->delta != 0 && now != s->last_event) {
> >           int64_t next = s->next_event;
> >           bool expired = (now - next >= 0);
> >           bool oneshot = (s->enabled == 2);
> >
> >           /* Figure out the current counter value.  */
> >           if (expired && (oneshot || use_icount)) {
> >               /* Prevent timer underflowing if it should already have
> >                  triggered.  */
> > @@ -131,17 +131,17 @@ uint64_t ptimer_get_count(ptimer_state *s)
> >               } else {
> >                   if (shift != 0)
> >                       div |= (period_frac >> (32 - shift));
> >                   /* Look at remaining bits of period_frac and round div up
> > if
> >                      necessary.  */
> >                   if ((uint32_t)(period_frac << shift))
> >                       div += 1;
> >               }
> > -            counter = rem / div;
> > +            counter = rem / div + (expired ? 0 : 1);
>
> Sorry about the long delays. Im wondering what this has to do with
> expiration though? the commit message suggests you want to change the
> rounding scheme, so can that be done directly with DIV_ROUND_UP?
>
> Regards,
> Peter
>
> >               if (expired && counter != 0) {
> >                   /* Wrap around periodic counter.  */
> >                   counter = s->limit - (counter - 1) % s->limit;
> >               }
> >           }
> >       } else {
> >           counter = s->delta;
> >
> >
> > --
> > Dmitry

Hi Peter,

The expiration check is needed because timer should stay with 0 for a one period 
before doing wrap around. Using DIV_ROUND_UP directly, timer would wrap around 
immediately once now > next (i.e. after 1 ns) regardless of the period value, 
and DIV_ROUND_UP can't be used because it would cause integer overflow of (n) + 
(d) due to the left shifting of the "rem" and "div".

#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))


No worries, I can wait as much as needed :)

--
Dmitry