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

[Joel Stanley <joel@jms.id.au>]

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?

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.

Cheers,

Joel