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>
---
 target/arm/helper.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c69a2baf1d3..ec1b84cf0fd 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2683,7 +2683,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
         uint64_t count = gt_get_countervalue(&cpu->env);
         /* Note that this must be unsigned 64 bit arithmetic: */
         int istatus = count - offset >= gt->cval;
-        uint64_t nexttick;
+        uint64_t nexttick = 0;
         int irqstate;
 
         gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
@@ -2692,21 +2692,30 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
         qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
 
         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]);
         } else {
             /* Next transition is when we hit cval */
             nexttick = gt->cval + offset;
-        }
-        /* Note that the desired next expiry time might be beyond the
-         * signed-64-bit range of a QEMUTimer -- in this case we just
-         * set the timer for as far in the future as possible. When the
-         * timer expires we will reset the timer for any remaining period.
-         */
-        if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
-            timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
-        } else {
-            timer_mod(cpu->gt_timer[timeridx], nexttick);
+
+            /*
+             * It is possible the next tick is beyond the
+             * signed-64-bit range of a QEMUTimer but currently the
+             * timer system doesn't support a run time of more the 292
+             * odd years so we set it to INT_MAX in this case.
+             */
+            if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
+                timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
+            } else {
+                timer_mod(cpu->gt_timer[timeridx], nexttick);
+            }
         }
         trace_arm_gt_recalc(timeridx, irqstate, nexttick);
     } else {
-- 
2.20.1

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>
> ---
>  target/arm/helper.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c69a2baf1d3..ec1b84cf0fd 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2683,7 +2683,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          uint64_t count = gt_get_countervalue(&cpu->env);
>          /* Note that this must be unsigned 64 bit arithmetic: */
>          int istatus = count - offset >= gt->cval;
> -        uint64_t nexttick;
> +        uint64_t nexttick = 0;
>          int irqstate;
>
>          gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
> @@ -2692,21 +2692,30 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
>
>          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"...

>          } else {
>              /* Next transition is when we hit cval */
>              nexttick = gt->cval + offset;
> -        }
> -        /* Note that the desired next expiry time might be beyond the
> -         * signed-64-bit range of a QEMUTimer -- in this case we just
> -         * set the timer for as far in the future as possible. When the
> -         * timer expires we will reset the timer for any remaining period.
> -         */
> -        if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
> -            timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
> -        } else {
> -            timer_mod(cpu->gt_timer[timeridx], nexttick);
> +
> +            /*
> +             * It is possible the next tick is beyond the
> +             * signed-64-bit range of a QEMUTimer but currently the
> +             * timer system doesn't support a run time of more the 292
> +             * odd years so we set it to INT_MAX in this case.
> +             */
> +            if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
> +                timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);

...but here we handle the similar case by "set a timeout for
INT64_MAX" ?

> +            } else {
> +                timer_mod(cpu->gt_timer[timeridx], nexttick);
> +            }
>          }
>          trace_arm_gt_recalc(timeridx, irqstate, nexttick);
>      } else {
> --

thanks
-- PMM

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>
>> ---
>>  target/arm/helper.c | 35 ++++++++++++++++++++++-------------
>>  1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index c69a2baf1d3..ec1b84cf0fd 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -2683,7 +2683,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>>          uint64_t count = gt_get_countervalue(&cpu->env);
>>          /* Note that this must be unsigned 64 bit arithmetic: */
>>          int istatus = count - offset >= gt->cval;
>> -        uint64_t nexttick;
>> +        uint64_t nexttick = 0;
>>          int irqstate;
>>
>>          gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
>> @@ -2692,21 +2692,30 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>>          qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
>>
>>          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.

>
>>          } else {
>>              /* Next transition is when we hit cval */
>>              nexttick = gt->cval + offset;
>> -        }
>> -        /* Note that the desired next expiry time might be beyond the
>> -         * signed-64-bit range of a QEMUTimer -- in this case we just
>> -         * set the timer for as far in the future as possible. When the
>> -         * timer expires we will reset the timer for any remaining period.
>> -         */
>> -        if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
>> -            timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
>> -        } else {
>> -            timer_mod(cpu->gt_timer[timeridx], nexttick);
>> +
>> +            /*
>> +             * It is possible the next tick is beyond the
>> +             * signed-64-bit range of a QEMUTimer but currently the
>> +             * timer system doesn't support a run time of more the 292
>> +             * odd years so we set it to INT_MAX in this case.
>> +             */
>> +            if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
>> +                timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
>
> ...but here we handle the similar case by "set a timeout for
> INT64_MAX" ?

Yeah we could just swallow it up and report something to say it's not
going to happen because it's beyond the horizon of what QEMUTimer can
deal with.

>
>> +            } else {
>> +                timer_mod(cpu->gt_timer[timeridx], nexttick);
>> +            }
>>          }
>>          trace_arm_gt_recalc(timeridx, irqstate, nexttick);
>>      } else {
>> --
>
> thanks
> -- PMM


-- 
Alex Bennée

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