On 20.09.2016 20:20, Peter Maydell wrote: > On 7 September 2016 at 14:22, Dmitry Osipenko <dig...@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. -- Dmitry