Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Peter Maydell]

On 14 October 2015 at 00:30, Dmitry Osipenko <address@hidden> wrote:
> ptimer_get_count() returns incorrect value for the disabled timer after
> reloading 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
>
> Signed-off-by: Dmitry Osipenko <address@hidden>
> ---
>  hw/core/ptimer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 8437bd6..abc3a20 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>     count = limit.  */
>  void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>  {
> +    uint64_t count = limit;
> +
>      /*
>       * Artificially limit timeout rate to something
>       * achievable under QEMU.  Otherwise, QEMU spends all
> @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, 
> int reload)
>
>      s->limit = limit;
>      if (reload)
> -        s->delta = limit;
> +        s->delta = count;
>      if (s->enabled && reload) {
>          s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          ptimer_reload(s);

Doesn't this defeat the rate limiting if the timer is enabled,
though? ptimer_reload() sets the underlying timer based on
s->delta, so if s->delta isn't the rate-limited value then the
timer will be incorrectly set to a very close-in value.

I think we'll return "incorrect" values from ptimer_get_count()
in the "counter is running" case too, because we calculate those
by looking at when the underlying timer's due to expire, and
we set the expiry time based on the adjusted value.

What's the underlying model we should have for what values
we return from reading the count if we've decided to adjust
the actual timer expiry with the rate limit? Should the
count go down from the specified value and then just hang
at 1 until the extended timer expiry time hits? Or something
else? Clearly defining what we want to happen ought to make
it easier to review attempts to fix it...

(Calling ptimer_set_count() also bypasses the ratelimiting at
the moment, incidentally.)

thanks
-- PMM