From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoDtO-0002Ow-KZ for qemu-devel@nongnu.org; Mon, 19 Oct 2015 13:06:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoDtK-0003CD-FZ for qemu-devel@nongnu.org; Mon, 19 Oct 2015 13:06:38 -0400 Received: from mail-vk0-f44.google.com ([209.85.213.44]:34315) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoDtK-0003Bw-8o for qemu-devel@nongnu.org; Mon, 19 Oct 2015 13:06:34 -0400 Received: by vkat63 with SMTP id t63so108159263vka.1 for ; Mon, 19 Oct 2015 10:06:33 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1444779048-10096-1-git-send-email-digetx@gmail.com> References: <1444779048-10096-1-git-send-email-digetx@gmail.com> From: Peter Maydell Date: Mon, 19 Oct 2015 18:06:14 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dmitry Osipenko Cc: Peter Crosthwaite , QEMU Developers On 14 October 2015 at 00:30, Dmitry Osipenko 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 > --- > 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