From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adN2l-000847-41 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 14:11:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adN2j-0006YR-SH for qemu-devel@nongnu.org; Tue, 08 Mar 2016 14:11:43 -0500 References: <77d17ad13f7b20deb433ca7a7b3d7cf40429665e.1454169735.git.digetx@gmail.com> <56B0C90B.7000300@gmail.com> From: Dmitry Osipenko Message-ID: <56DF23E1.9080901@gmail.com> Date: Tue, 8 Mar 2016 22:11:29 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , qemu-arm , QEMU Developers 08.03.2016 06:43, Peter Crosthwaite пишет: > On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko wrote: >> 30.01.2016 19:43, Dmitry Osipenko пишет: >>> >>> Due to rounding down performed by ptimer_get_count, it returns counter - 1 >>> for >>> the active timer. That's incorrect because counter should decrement only >>> after >>> period been expired, not before. I.e. if running timer has been loaded >>> with >>> value X, then timer counter should stay with X until period expired and >>> decrement after. Fix this by adding 1 to the counter value for the active >>> and >>> unexpired timer. >>> >>> Signed-off-by: Dmitry Osipenko >>> --- >>> hw/core/ptimer.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c >>> index 62f8cb1..b2044fb 100644 >>> --- a/hw/core/ptimer.c >>> +++ b/hw/core/ptimer.c >>> @@ -136,7 +136,7 @@ uint64_t ptimer_get_count(ptimer_state *s) >>> if ((uint32_t)(period_frac << shift)) >>> div += 1; >>> } >>> - counter = rem / div; >>> + counter = rem / div + (expired ? 0 : 1); >>> >>> if (expired && counter != 0) { >>> /* Wrap around periodic counter. */ >>> >> >> Noticed one nit here: >> >> There is possibility to return timer counter = limit + 1, if the following >> ptimer calls execute in less than 1ns. >> >> ptimer_run(t, 1); >> // counter = 91, if set() count executed in less than 1ns >> ptimer_set_count(t, 90); >> counter = ptimer_get_count(t); >> >> Likely, it would be impossible to trigger that issue on a real current >> machine. >> But the fix is trivial, I'll incorporate it in V13 if it looks fine: >> >> --- >> @@ -76,20 +76,20 @@ static void ptimer_tick(void *opaque) >> { >> ptimer_state *s = (ptimer_state *)opaque; >> s->delta = 0; >> ptimer_reload(s); >> } >> >> uint64_t ptimer_get_count(ptimer_state *s) >> { >> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> uint64_t counter; >> >> - if (s->enabled && s->delta != 0) { >> - int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> + if (s->enabled && s->delta != 0 && now != s->last_event) { >> int64_t next = s->next_event; >> bool expired = (now - next >= 0); >> bool oneshot = (s->enabled == 2); >> >> /* Figure out the current counter value. */ >> if (expired && (oneshot || use_icount)) { >> /* Prevent timer underflowing if it should already have >> triggered. */ >> @@ -131,17 +131,17 @@ uint64_t ptimer_get_count(ptimer_state *s) >> } else { >> if (shift != 0) >> div |= (period_frac >> (32 - shift)); >> /* Look at remaining bits of period_frac and round div up >> if >> necessary. */ >> if ((uint32_t)(period_frac << shift)) >> div += 1; >> } >> - counter = rem / div; >> + counter = rem / div + (expired ? 0 : 1); >> > > Sorry about the long delays. Im wondering what this has to do with > expiration though? the commit message suggests you want to change the > rounding scheme, so can that be done directly with DIV_ROUND_UP? > > Regards, > Peter > >> if (expired && counter != 0) { >> /* Wrap around periodic counter. */ >> counter = s->limit - (counter - 1) % s->limit; >> } >> } >> } else { >> counter = s->delta; >> >> >> -- >> Dmitry Hi Peter, The expiration check is needed because timer should stay with 0 for a one period before doing wrap around. Using DIV_ROUND_UP directly, timer would wrap around immediately once now > next (i.e. after 1 ns) regardless of the period value, and DIV_ROUND_UP can't be used because it would cause integer overflow of (n) + (d) due to the left shifting of the "rem" and "div". #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d)) No worries, I can wait as much as needed :) -- Dmitry