| Message ID | 586d1643bf90bab81322ae34b9b68c10458249a9.1473252818.git.digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7 September 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote: > Currently, periodic counter wraps around immediately once counter reaches > "0", this is wrong behaviour for some of the timers, resulting in one period > being lost. Add new ptimer policy that provides correct behaviour for such > timers, so that counter stays with "0" for a one period before wrapping > around. This says it's just adding a new policy... > @@ -91,7 +96,7 @@ uint64_t ptimer_get_count(ptimer_state *s) > { > uint64_t counter; > > - if (s->enabled) { > + if (s->enabled && s->delta != 0) { > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > int64_t next = s->next_event; > bool expired = (now - next >= 0); > @@ -145,6 +150,14 @@ uint64_t ptimer_get_count(ptimer_state *s) > div += 1; > } > counter = rem / div; > + > + if (!oneshot && s->delta == s->limit) { > + /* Before wrapping around, timer should stay with counter = 0 > + for a one period. The delta has been adjusted by +1 for > + the wrapped around counter, so taking a modulo of limit + 1 > + gives that period. */ > + counter %= s->limit + 1; > + } > } > } else { > counter = s->delta; ...but the changes in this function look like they affect behaviour even if that policy flag isn't set. thanks -- PMM
On 20.09.2016 20:20, Peter Maydell wrote: > On 7 September 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote: >> Currently, periodic counter wraps around immediately once counter reaches >> "0", this is wrong behaviour for some of the timers, resulting in one period >> being lost. Add new ptimer policy that provides correct behaviour for such >> timers, so that counter stays with "0" for a one period before wrapping >> around. > > This says it's just adding a new policy... > >> @@ -91,7 +96,7 @@ uint64_t ptimer_get_count(ptimer_state *s) >> { >> uint64_t counter; >> >> - if (s->enabled) { >> + if (s->enabled && s->delta != 0) { >> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> int64_t next = s->next_event; >> bool expired = (now - next >= 0); >> @@ -145,6 +150,14 @@ uint64_t ptimer_get_count(ptimer_state *s) >> div += 1; >> } >> counter = rem / div; >> + >> + if (!oneshot && s->delta == s->limit) { >> + /* Before wrapping around, timer should stay with counter = 0 >> + for a one period. The delta has been adjusted by +1 for >> + the wrapped around counter, so taking a modulo of limit + 1 >> + gives that period. */ >> + counter %= s->limit + 1; >> + } >> } >> } else { >> counter = s->delta; > > ...but the changes in this function look like they affect > behaviour even if that policy flag isn't set. > No, the delta value is adjusted only when PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD is set. + if (s->policy_mask & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) { + delta += delta_adjust; + } + That adjustment is applied only on reload of the periodic timer. @@ -83,7 +88,7 @@ static void ptimer_tick(void *opaque) if (s->enabled == 2) { s->enabled = 0; } else { - ptimer_reload(s); + ptimer_reload(s, 1); } } All other ptimer_reload's are ptimer_reload(s, 0). The periodic timer is reloaded with the limit value. When s->delta == s->limit, we can assume that timer is free running. When delta is adjusted by +1 on reload, the counter = limit + 1 (counter value is calculated based on elapsed QEMU time) and that "counter %= s->limit + 1" modulo gives counter = 0 while in fact the counter = adjusted delta (limit + 1). When delta is *not* adjusted, the modulo returns the actual counter value, since counter value is always less than s->limit + 1. So, this patch doesn't affect old behaviour at all. Looking more at it now, I think "counter %= s->limit + 1" should be changed to "counter %= s->limit" in this patch and +1 added in the "no counter round down" patch, since the counter value is always rounded down here. Instead of the "staying with counter = 0 for a one period" it would wraparound to the limit value if "wraparound after one period" policy is used. I'll change it in V17.
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c index c45c835..e9b2e15 100644 --- a/hw/core/ptimer.c +++ b/hw/core/ptimer.c @@ -35,16 +35,17 @@ static void ptimer_trigger(ptimer_state *s) } } -static void ptimer_reload(ptimer_state *s) +static void ptimer_reload(ptimer_state *s, int delta_adjust) { uint32_t period_frac = s->period_frac; uint64_t period = s->period; + uint64_t delta = s->delta; - if (s->delta == 0) { + if (delta == 0) { ptimer_trigger(s); - s->delta = s->limit; + delta = s->delta = s->limit; } - if (s->delta == 0 || s->period == 0) { + if (delta == 0 || s->period == 0) { if (!qtest_enabled()) { fprintf(stderr, "Timer with period zero, disabling\n"); } @@ -53,6 +54,10 @@ static void ptimer_reload(ptimer_state *s) return; } + if (s->policy_mask & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) { + delta += delta_adjust; + } + /* * Artificially limit timeout rate to something * achievable under QEMU. Otherwise, QEMU spends all @@ -62,15 +67,15 @@ static void ptimer_reload(ptimer_state *s) * on the current generation of host machines. */ - if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) { - period = 10000 / s->delta; + if (s->enabled == 1 && (delta * period < 10000) && !use_icount) { + period = 10000 / delta; period_frac = 0; } s->last_event = s->next_event; - s->next_event = s->last_event + s->delta * period; + s->next_event = s->last_event + delta * period; if (period_frac) { - s->next_event += ((int64_t)period_frac * s->delta) >> 32; + s->next_event += ((int64_t)period_frac * delta) >> 32; } timer_mod(s->timer, s->next_event); } @@ -83,7 +88,7 @@ static void ptimer_tick(void *opaque) if (s->enabled == 2) { s->enabled = 0; } else { - ptimer_reload(s); + ptimer_reload(s, 1); } } @@ -91,7 +96,7 @@ uint64_t ptimer_get_count(ptimer_state *s) { uint64_t counter; - if (s->enabled) { + if (s->enabled && s->delta != 0) { int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); int64_t next = s->next_event; bool expired = (now - next >= 0); @@ -145,6 +150,14 @@ uint64_t ptimer_get_count(ptimer_state *s) div += 1; } counter = rem / div; + + if (!oneshot && s->delta == s->limit) { + /* Before wrapping around, timer should stay with counter = 0 + for a one period. The delta has been adjusted by +1 for + the wrapped around counter, so taking a modulo of limit + 1 + gives that period. */ + counter %= s->limit + 1; + } } } else { counter = s->delta; @@ -157,7 +170,7 @@ void ptimer_set_count(ptimer_state *s, uint64_t count) s->delta = count; if (s->enabled) { s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ptimer_reload(s); + ptimer_reload(s, 0); } } @@ -174,7 +187,7 @@ void ptimer_run(ptimer_state *s, int oneshot) s->enabled = oneshot ? 2 : 1; if (was_disabled) { s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ptimer_reload(s); + ptimer_reload(s, 0); } } @@ -198,7 +211,7 @@ void ptimer_set_period(ptimer_state *s, int64_t period) s->period_frac = 0; if (s->enabled) { s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ptimer_reload(s); + ptimer_reload(s, 0); } } @@ -210,7 +223,7 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq) s->period_frac = (1000000000ll << 32) / freq; if (s->enabled) { s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ptimer_reload(s); + ptimer_reload(s, 0); } } @@ -223,7 +236,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) s->delta = limit; if (s->enabled && reload) { s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ptimer_reload(s); + ptimer_reload(s, 0); } } diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h index 7b0fc13..a13a12e 100644 --- a/include/hw/ptimer.h +++ b/include/hw/ptimer.h @@ -34,6 +34,10 @@ * counter = counter value at the moment of change (.i.e. one less). */ #define PTIMER_POLICY_DEFAULT 0 +/* Periodic timer counter stays with "0" for a one period before wrapping + * around. */ +#define PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD (1 << 0) + /* ptimer.c */ typedef struct ptimer_state ptimer_state; typedef void (*ptimer_cb)(void *opaque);
Currently, periodic counter wraps around immediately once counter reaches "0", this is wrong behaviour for some of the timers, resulting in one period being lost. Add new ptimer policy that provides correct behaviour for such timers, so that counter stays with "0" for a one period before wrapping around. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- hw/core/ptimer.c | 43 ++++++++++++++++++++++++++++--------------- include/hw/ptimer.h | 4 ++++ 2 files changed, 32 insertions(+), 15 deletions(-)