On Sat, Jan 9, 2016 at 9:39 AM, Dmitry Osipenko <dig...@gmail.com> 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 <dig...@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.pe...@gmail.com>
> ---
> 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
>
