| Message ID | 77d17ad13f7b20deb433ca7a7b3d7cf40429665e.1454169735.git.digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <digetx@gmail.com> > --- > 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); if (expired && counter != 0) { /* Wrap around periodic counter. */ counter = s->limit - (counter - 1) % s->limit; } } } else { counter = s->delta;
02.02.2016 18:19, Dmitry Osipenko ?????: > There is possibility to return timer counter = limit + 1, if the following > ptimer calls execute in less than 1ns. s/limit/delta/
On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <digetx@gmail.com> 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 <digetx@gmail.com> >> --- >> 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
08.03.2016 06:43, Peter Crosthwaite ?????: > On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <digetx@gmail.com> 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 <digetx@gmail.com> >>> --- >>> 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 :)
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. */
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 <digetx@gmail.com> --- hw/core/ptimer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)