Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Peter Crosthwaite]

On Sat, Oct 24, 2015 at 5:21 AM, Dmitry Osipenko <address@hidden> wrote:
> Multiple issues here related to the timer with a corrected .limit value:
>
> 1) ptimer_get_count() returns incorrect value for the disabled timer after
> loading the counter with a small value, because corrected limit value
> is used instead of the original.
>
> For instance:
>     1) ptimer_stop(t)
>     2) ptimer_set_period(t, 1)
>     3) ptimer_set_limit(t, 0, 1)
>     4) ptimer_get_count(t) <-- would return 10000 instead of 0
>
> 2) ptimer_get_count() might return incorrect value for the timer running
> with a corrected limit value.
>
> For instance:
>     1) ptimer_stop(t)
>     2) ptimer_set_period(t, 1)
>     3) ptimer_set_limit(t, 10, 1)
>     4) ptimer_run(t)
>     5) ptimer_get_count(t) <-- might return value > 10
>
> 3) Neither ptimer_set_period() nor ptimer_set_freq() are correcting the
> limit value, so it is still possible to make timer timeout value
> arbitrary small.
>
> For instance:
>     1) ptimer_set_period(t, 10000)
>     2) ptimer_set_limit(t, 1, 0)
>     3) ptimer_set_period(t, 1) <-- bypass limit correction
>
> Fix all of the above issues by moving timeout value correction to the
> ptimer_reload(). Instead of changing limit value, set new ptimer struct
> member "next_event_corrected" when next_event was corrected and make
> ptimer_get_count() always return 1 for the timer running with corrected
> next_event value. Bump VMSD version since ptimer VM state is changed, but
> keep .minimum_version_id as it is backward compatible.
>
> Signed-off-by: Dmitry Osipenko <address@hidden>
> ---
>  hw/core/ptimer.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 8437bd6..2441943 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -19,6 +19,7 @@ struct ptimer_state
>      int64_t period;
>      int64_t last_event;
>      int64_t next_event;
> +    uint8_t next_event_corrected;
>      QEMUBH *bh;
>      QEMUTimer *timer;
>  };
> @@ -48,6 +49,23 @@ static void ptimer_reload(ptimer_state *s)
>      if (s->period_frac) {
>          s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
>      }
> +
> +    /*
> +     * Artificially limit timeout to something
> +     * achievable under QEMU.  Otherwise, QEMU spends all
> +     * its time generating timer interrupts, and there
> +     * is no forward progress.
> +     * About ten microseconds is the fastest that really works
> +     * on the current generation of host machines.
> +     */
> +
> +    s->next_event_corrected = 0;
> +
> +    if (!use_icount && (s->next_event - s->last_event < 10000)) {
> +        s->next_event = s->last_event + 10000;
> +        s->next_event_corrected = 1;
> +    }
> +
>      timer_mod(s->timer, s->next_event);
>  }
>
> @@ -76,6 +94,9 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              /* Prevent timer underflowing if it should already have
>                 triggered.  */
>              counter = 0;
> +        } else if (s->next_event_corrected) {
> +            /* Always return 1 when timer expire value was corrected.  */
> +            counter = 1;

This looks like a give-up without trying to get the correct value. If
the calculated value (using the normal-path logic below) is sane, you
should just use it. If it comes out bad then you should clamp to 1.

I am wondering whether this clamping policy (as in original code as
well) is correct at all though. The value of a free-running
short-interval periodic timer (poor mans random number generator)
without any actual interrupt generation will be affected by QEMUs
asynchronous handling of timer events. So if I set limit to 100, then
sample this timer every user keyboard stroke, I should get a uniform
distribution on [0,100]. Instead in am going to get lots of 1s. This
is more broken in the original code (as you state), as I will get >
100, but I think we have traded broken for slightly less broken. I
think the correct semantic is to completely ignoring rate limiting
except for the scheduling on event callbacks. That is, the timer
interval is not rate limited, instead the timer_mod interval
(next_event -last_event) just has a 10us clamp.

The means the original code semantic of returning counter = 0 for an
already triggered timer is wrong. It should handle in-the-past
wrap-arounds as wraparounds by triggering the timer and redoing the
math with the new interval values. So instead the logic would be
something like:

timer_val = -1;

for(;;) {

 if (!enabled) {
    return delta;
 }

 timer_val = (next-event - now) *  scaling();
 if (timer_val >=  0) {
     return timer_val;
 }
 /* Timer has actually expired but we missed it, reload it and try again */
 ptimer_tick();
}

ptimer_reload() then needs to be patched to make sure it always
timer_mod()s in the future, otherwise this loop could iterate a large
number of times.

This means that when the qemu_timer() actually ticks, a large number
or cycles may have occured, but we can justify that in that callback
event latency (usually interrupts) is undefined anyway and coalescing
of multiples may have happened as part of that. This usually matches
expectations of real guests where interrupt latency is ultimately
undefined.

Regards,
Peter

>          } else {
>              uint64_t rem;
>              uint64_t div;
> @@ -180,19 +201,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>     count = limit.  */
>  void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>  {
> -    /*
> -     * Artificially limit timeout rate to something
> -     * achievable under QEMU.  Otherwise, QEMU spends all
> -     * its time generating timer interrupts, and there
> -     * is no forward progress.
> -     * About ten microseconds is the fastest that really works
> -     * on the current generation of host machines.
> -     */
> -
> -    if (!use_icount && limit * s->period < 10000 && s->period) {
> -        limit = 10000 / s->period;
> -    }
> -
>      s->limit = limit;
>      if (reload)
>          s->delta = limit;
> @@ -204,7 +212,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, 
> int reload)
>
>  const VMStateDescription vmstate_ptimer = {
>      .name = "ptimer",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(enabled, ptimer_state),
> @@ -214,6 +222,7 @@ const VMStateDescription vmstate_ptimer = {
>          VMSTATE_INT64(period, ptimer_state),
>          VMSTATE_INT64(last_event, ptimer_state),
>          VMSTATE_INT64(next_event, ptimer_state),
> +        VMSTATE_UINT8(next_event_corrected, ptimer_state),
>          VMSTATE_TIMER_PTR(timer, ptimer_state),
>          VMSTATE_END_OF_LIST()
>      }
> --
> 2.6.1
>