[PATCH v1 0/2] clean-ups for sleep=off behaviour

[PATCH v1 0/2] clean-ups for sleep=off behaviour

[Alex Bennée]

Hi,

These fixes are more band-aids than anything else but they at least
improve on the original situation where running a zephyr build with:

  -icount shift=6,align=off,sleep=off

would lock the monitor hard as time rushed into the future. With these
patches time still runs to the future pretty fast as Zephyr spends
most of it's time in WFI waiting for input with a CVAL far in the
future. This does does raise the question of what sleep=off is even
for?

Alex Bennée (2):
  qemu-timer: gracefully handle the end of time
  target/arm: only set the nexttick timer if !ISTATUS

 include/sysemu/cpus.h |  2 ++
 softmmu/cpus.c        | 13 +++++++++++++
 target/arm/helper.c   | 35 ++++++++++++++++++++++-------------
 util/qemu-timer.c     | 16 ++++++++++++++++
 stubs/Makefile.objs   |  1 +
 5 files changed, 54 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v1 1/2] qemu-timer: gracefully handle the end of time

[Alex Bennée]

The previous behaviour was rather user hostile as timers set for
INT64_MAX would continually re-arm leaving the system locked in a loop
and the console unavailable. With this patch we detect the situation
and gracefully suspend the machine.

NB: we need a proper fix before the 23rd century.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/sysemu/cpus.h |  2 ++
 softmmu/cpus.c        | 13 +++++++++++++
 util/qemu-timer.c     | 16 ++++++++++++++++
 stubs/Makefile.objs   |  1 +
 4 files changed, 32 insertions(+)

diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 3c1da6a0183..7da3a5b60b5 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -15,6 +15,8 @@ void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
 extern int icount_align_option;
 
+void qemu_handle_outa_time(void);
+
 /* drift information for info jit command */
 extern int64_t max_delay;
 extern int64_t max_advance;
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index a802e899abb..46b6f346370 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -657,6 +657,19 @@ static bool shift_state_needed(void *opaque)
     return use_icount == 2;
 }
 
+/* Handler for reaching the end of time */
+void qemu_handle_outa_time(void)
+{
+    static bool reported = false;
+    if (runstate_is_running()) {
+        if (!reported) {
+            error_report("ran out of time, suspending emulation");
+            reported = true;
+        }
+        vm_stop(RUN_STATE_PAUSED);
+    }
+}
+
 /*
  * Subsection for warp timer migration is optional, because may not be created
  */
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index f62b4feecdb..06f6818eb3d 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -540,6 +540,22 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
      * done".
      */
     current_time = qemu_clock_get_ns(timer_list->clock->type);
+
+    /*
+     * Check to see if we have run out of time. Most of our time
+     * sources are nanoseconds since epoch (some time around the fall
+     * of Babylon 5, the start of the Enterprises five year mission
+     * and just before the arrival of the great evil ~ 2262CE).
+     * Although icount based time is ns since the start of emulation
+     * it is able to skip forward if the device is sleeping (think IoT
+     * device with a very long heartbeat). Either way we don't really
+     * handle running out of time so lets catch it and report it here.
+     */
+    if (current_time == INT64_MAX) {
+        qemu_handle_outa_time();
+        goto out;
+    }
+
     qemu_mutex_lock(&timer_list->active_timers_lock);
     while ((ts = timer_list->active_timers)) {
         if (!timer_expired_ns(ts, current_time)) {
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index d42046afe4e..d363d8b551b 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,6 @@
 stub-obj-y += blk-commit-all.o
 stub-obj-y += cmos.o
+stub-obj-y += cpus.o
 stub-obj-y += cpu-get-clock.o
 stub-obj-y += cpu-get-icount.o
 stub-obj-y += dump.o
-- 
2.20.1

Re: [PATCH v1 1/2] qemu-timer: gracefully handle the end of time

[Paolo Bonzini]

On 28/07/20 16:10, Alex Bennée wrote:
> +    /*
> +     * Check to see if we have run out of time. Most of our time
> +     * sources are nanoseconds since epoch (some time around the fall
> +     * of Babylon 5, the start of the Enterprises five year mission
> +     * and just before the arrival of the great evil ~ 2262CE).
> +     * Although icount based time is ns since the start of emulation
> +     * it is able to skip forward if the device is sleeping (think IoT
> +     * device with a very long heartbeat). Either way we don't really
> +     * handle running out of time so lets catch it and report it here.
> +     */
> +    if (current_time == INT64_MAX) {
> +        qemu_handle_outa_time();
> +        goto out;
> +    }
> +

Doing this here is a bit dangerous, I'd rather do nothing here and
detect the situation in cpus.c where we can do
qemu_system_shutdown_request() (and also do nothing).

Paolo

Re: [PATCH v1 1/2] qemu-timer: gracefully handle the end of time

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/07/20 16:10, Alex Bennée wrote:
>> +    /*
>> +     * Check to see if we have run out of time. Most of our time
>> +     * sources are nanoseconds since epoch (some time around the fall
>> +     * of Babylon 5, the start of the Enterprises five year mission
>> +     * and just before the arrival of the great evil ~ 2262CE).
>> +     * Although icount based time is ns since the start of emulation
>> +     * it is able to skip forward if the device is sleeping (think IoT
>> +     * device with a very long heartbeat). Either way we don't really
>> +     * handle running out of time so lets catch it and report it here.
>> +     */
>> +    if (current_time == INT64_MAX) {
>> +        qemu_handle_outa_time();
>> +        goto out;
>> +    }
>> +
>
> Doing this here is a bit dangerous, I'd rather do nothing here and
> detect the situation in cpus.c where we can do
> qemu_system_shutdown_request() (and also do nothing).

You mean in notify_aio_contexts()? Sure we can do that.

I also figured it might be worth cleaning up the return progress stuff
because AFAICT no one seems to care.

>
> Paolo


-- 
Alex Bennée

Re: [PATCH v1 1/2] qemu-timer: gracefully handle the end of time

[Paolo Bonzini]

On 28/07/20 18:08, Alex Bennée wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 28/07/20 16:10, Alex Bennée wrote:
>>> +    /*
>>> +     * Check to see if we have run out of time. Most of our time
>>> +     * sources are nanoseconds since epoch (some time around the fall
>>> +     * of Babylon 5, the start of the Enterprises five year mission
>>> +     * and just before the arrival of the great evil ~ 2262CE).
>>> +     * Although icount based time is ns since the start of emulation
>>> +     * it is able to skip forward if the device is sleeping (think IoT
>>> +     * device with a very long heartbeat). Either way we don't really
>>> +     * handle running out of time so lets catch it and report it here.
>>> +     */
>>> +    if (current_time == INT64_MAX) {
>>> +        qemu_handle_outa_time();
>>> +        goto out;
>>> +    }
>>> +
>>
>> Doing this here is a bit dangerous, I'd rather do nothing here and
>> detect the situation in cpus.c where we can do
>> qemu_system_shutdown_request() (and also do nothing).
> 
> You mean in notify_aio_contexts()? Sure we can do that.

Yes, that would work.  I think qemu_clock_deadline_ns_all would already
return -1 so that you'd have a zero-instruction budget from
tcg_get_icount_limit.

Paolo

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

[Alex Bennée]

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

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

[Peter Maydell]

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

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

[Alex Bennée]

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

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