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 <dig...@gmail.com>
> ---
> 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
>
