Re: [Qemu-devel] [PATCH v10 1/7] hw/ptimer: Fix issues caused by the adjusted timer limit value

[Peter Crosthwaite]

On Sat, Jan 9, 2016 at 9:39 AM, Dmitry Osipenko <address@hidden> wrote:
> Multiple issues here related to the timer with a adjusted .limit value:
>
> 1) ptimer_get_count() returns incorrect counter value for the disabled
> timer after loading the counter with a small value, because adjusted 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 adjusted 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 adjusting 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 adjusting timer period instead of the limit.
> Perform the adjustment for periodic timer only. Use the delta value instead
> of the limit to make decision whether adjustment is required, as limit could
> be altered while timer is running, resulting in incorrect value returned by
> ptimer_get_count.
>
> Signed-off-by: Dmitry Osipenko <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  hw/core/ptimer.c | 51 +++++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index edf077c..c3d31cb 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -34,6 +34,9 @@ static void ptimer_trigger(ptimer_state *s)
>
>  static void ptimer_reload(ptimer_state *s)
>  {
> +    uint32_t period_frac = s->period_frac;
> +    uint64_t period = s->period;
> +
>      if (s->delta == 0) {
>          ptimer_trigger(s);
>          s->delta = s->limit;
> @@ -44,10 +47,24 @@ static void ptimer_reload(ptimer_state *s)
>          return;
>      }
>
> +    /*
> +     * 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 ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
> +        period = 10000 / s->delta;
> +        period_frac = 0;
> +    }
> +
>      s->last_event = s->next_event;
> -    s->next_event = s->last_event + s->delta * s->period;
> -    if (s->period_frac) {
> -        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
> +    s->next_event = s->last_event + s->delta * period;
> +    if (period_frac) {
> +        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
>      }
>      timer_mod(s->timer, s->next_event);
>  }
> @@ -82,6 +99,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              uint64_t div;
>              int clz1, clz2;
>              int shift;
> +            uint32_t period_frac = s->period_frac;
> +            uint64_t period = s->period;
> +
> +            if ((s->enabled == 1) && !use_icount && (s->delta * period < 
> 10000)) {
> +                period = 10000 / s->delta;
> +                period_frac = 0;
> +            }
>
>              /* We need to divide time by period, where time is stored in
>                 rem (64-bit integer) and period is stored in 
> period/period_frac
> @@ -94,7 +118,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              */
>
>              rem = s->next_event - now;
> -            div = s->period;
> +            div = period;
>
>              clz1 = clz64(rem);
>              clz2 = clz64(div);
> @@ -103,13 +127,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              rem <<= shift;
>              div <<= shift;
>              if (shift >= 32) {
> -                div |= ((uint64_t)s->period_frac << (shift - 32));
> +                div |= ((uint64_t)period_frac << (shift - 32));
>              } else {
>                  if (shift != 0)
> -                    div |= (s->period_frac >> (32 - shift));
> +                    div |= (period_frac >> (32 - shift));
>                  /* Look at remaining bits of period_frac and round div up if
>                     necessary.  */
> -                if ((uint32_t)(s->period_frac << shift))
> +                if ((uint32_t)(period_frac << shift))
>                      div += 1;
>              }
>              counter = rem / div;
> @@ -181,19 +205,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;
> --
> 2.6.4
>