Re: [Qemu-arm] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value

[Peter Crosthwaite]

On Tue, Jan 05, 2016 at 05:33:26AM +0300, Dmitry Osipenko 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.
> Do the adjust for periodic timer only.
> 
> Signed-off-by: Dmitry Osipenko <address@hidden>
> ---
>  hw/core/ptimer.c | 59 
> ++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index edf077c..035af97 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)
>  
>  static void ptimer_reload(ptimer_state *s)
>  {
> -    if (s->delta == 0) {
> +    uint32_t period_frac = s->period_frac;
> +    uint64_t period = s->period;
> +    uint64_t delta = s->delta;
> +    uint64_t limit = s->limit;
> +

Localising these variables is out of scope of the change. I think you can
just use s->foo and if you want to cleanup, it should be a separate
patch.

> +    if (delta == 0) {
>          ptimer_trigger(s);
> -        s->delta = s->limit;
> +        delta = limit;
>      }
> -    if (s->delta == 0 || s->period == 0) {
> +    if (delta == 0 || period == 0) {
>          fprintf(stderr, "Timer with period zero, disabling\n");
>          s->enabled = 0;
>          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) && (limit * period < 10000)) {
> +        period = 10000 / limit;
> +        period_frac = 0;
> +    }
> +

I think it should be ok to just update s->period and s->period_frac ...

>      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 + delta * period;
> +    if (period_frac) {
> +        s->next_event += ((int64_t)period_frac * delta) >> 32;
>      }
>      timer_mod(s->timer, s->next_event);
>  }
> @@ -82,6 +101,8 @@ 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;
>  
>              /* We need to divide time by period, where time is stored in
>                 rem (64-bit integer) and period is stored in 
> period/period_frac
> @@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>                 backwards.
>              */
>  
> +            if ((s->enabled == 1) && (s->limit * period < 10000)) {
> +                period = 10000 / s->limit;
> +                period_frac = 0;
> +            }
> +

... and then this (and the local variables) become obsolete. Can get_count()
blindly use the period and period_frac as used by ptimer_reload?

>              rem = s->next_event - now;
> -            div = s->period;
> +            div = period;
>  
>              clz1 = clz64(rem);
>              clz2 = clz64(div);
> @@ -103,13 +129,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 +207,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) {

This original rate limiting code is gated on icount, so I think then
new way should be the same.

Regards,
Peter

> -        limit = 10000 / s->period;
> -    }
> -
>      s->limit = limit;
>      if (reload)
>          s->delta = limit;
> -- 
> 2.6.4
>