Re: [PATCH v1 2/2] target/arm: only set the nexttick timer if !ISTATUS

[Peter Maydell]

On Tue, 28 Jul 2020 at 17:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 28 Jul 2020 at 15:10, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> Otherwise we have an unfortunate interaction with -count sleep=off
> >> which means we fast forward time when we don't need to. The easiest
> >> way to trigger it was to attach to the gdbstub and place a break point
> >> at the timers IRQ routine. Once the timer fired setting the next event
> >> at INT_MAX then qemu_start_warp_timer would skip to the end.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---

> >>          if (istatus) {
> >> -            /* Next transition is when count rolls back over to zero */
> >> -            nexttick = UINT64_MAX;
> >> +            /*
> >> +             * The IRQ status of the timer will persist until:
> >> +             *   - CVAL is changed or
> >> +             *   - ENABLE is changed
> >> +             *
> >> +             * There is no point re-arming the timer for some far
> >> +             * flung future - currently it just is.
> >> +             */
> >> +            timer_del(cpu->gt_timer[timeridx]);
> >
> > Why do we delete the timer for this case of "next time we need to
> > know is massively in the future"...
>
> It's not really - it's happening now and it will continue to happen
> until the IRQ is serviced or we change the CVAL at which point we can
> calculate the next time we need it.

It is far-future: the counter can roll all the way round and back over
to zero, as the old comment states. (Other reasons for things to change
get handled via other code paths: it's only the "at some defined time
in the future a change happens" cases that we need to set a timer for).
It's the same situation as below, I think (though I don't know why we
used UINT64_MAX for one and INT64_MAX for the other).

-- PMM