| Message ID | 20230606134917.3782215-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/timer/nrf51_timer: Don't lose time when timer is queried in tight loop | expand |
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
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
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; }
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 hw/timer/nrf51_timer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)