Re: [PATCH] hw/timer/nrf51_timer: Don't lose time when timer is queried in tight loop

[Peter Maydell]

On Wed, 7 Jun 2023 at 12:26, Joel Stanley <joel@jms.id.au> wrote:
>
> On Tue, 6 Jun 2023 at 13:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > The nrf51_timer has a free-running counter which we implement using
> > the pattern of using two fields (update_counter_ns, counter) to track
> > the last point at which we calculated the counter value, and the
> > counter value at that time.  Then we can find the current counter
> > value by converting the difference in wall-clock time between then
> > and now to a tick count that we need to add to the counter value.
> >
> > Unfortunately the nrf51_timer's implementation of this has a bug
> > which means it loses time every time update_counter() is called.
> > After updating s->counter it always sets s->update_counter_ns to
> > 'now', even though the actual point when s->counter hit the new value
> > will be some point in the past (half a tick, say).  In the worst case
> > (guest code in a tight loop reading the counter, icount mode) the
> > counter is continually queried less than a tick after it was last
> > read, so s->counter never advances but s->update_counter_ns does, and
> > the guest never makes forward progress.
> >
> > The fix for this is to only advance update_counter_ns to the
> > timestamp of the last tick, not all the way to 'now'.  (This is the
> > pattern used in hw/misc/mps2-fpgaio.c's counter.)
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > The hang in icount mode was discovered by the Zephyr folks as part
> > of their investigation into
> > https://github.com/zephyrproject-rtos/zephyr/issues/57512
>
> Did you get an image to test with?

Yes, somewhere in the comments on one of the pullrequests
associated with that issue one of the Zephyr devs helpfully
provided a test image.

> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> >
> >  hw/timer/nrf51_timer.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/timer/nrf51_timer.c b/hw/timer/nrf51_timer.c
> > index 42be79c7363..50c6772383e 100644
> > --- a/hw/timer/nrf51_timer.c
> > +++ b/hw/timer/nrf51_timer.c
> > @@ -45,7 +45,12 @@ static uint32_t update_counter(NRF51TimerState *s, 
> > int64_t now)
> >      uint32_t ticks = ns_to_ticks(s, now - s->update_counter_ns);
> >
> >      s->counter = (s->counter + ticks) % BIT(bitwidths[s->bitmode]);
> > -    s->update_counter_ns = now;
> > +    /*
> > +     * Only advance the sync time to the timestamp of the last tick,
> > +     * not all the way to 'now', so we don't lose time if we do
> > +     * multiple resyncs in a single tick.
> > +     */
> > +    s->update_counter_ns += ticks_to_ns(s, ticks);
> >      return ticks;
>
> We're still returning the number of ticks up to 'now'. Should we
> instead return the elapsed ticks? If not, we will expire the timer
> early.

Hmm, I'm not sure what you mean here. The tick count at the
'update_counter_ns' time and the tick count at 'now' should be
identical (because there can't ever be an entire tick difference
between them).

thanks
-- PMM