Re: [Qemu-devel] [PATCH] hw/timer: Add value matching support to aspeed_timer

[Andrew Jeffery]

On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
> On 27 May 2016 at 06:08, Andrew Jeffery <address@hidden> wrote:
> > 
> > Value matching allows Linux to boot with CONFIG_NO_HZ_IDLE=y on the
> > palmetto-bmc machine. Two match registers are provided for each timer.
> > 
> > Signed-off-by: Andrew Jeffery <address@hidden>
> > ---
> > 
> > The change pulls out ptimer in favour of the regular timer infrastructure. 
> > As a
> > consequence it implements the conversions between ticks and time which 
> > feels a
> > little tedious. Any comments there would be appreciated.
> Couple of minor comments below.
> 
> > 
> >  hw/timer/aspeed_timer.c         | 135 
> > ++++++++++++++++++++++++++++++----------
> >  include/hw/timer/aspeed_timer.h |   6 +-
> >  2 files changed, 105 insertions(+), 36 deletions(-)
> > 
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > index 4b94808821b4..905de0f788b2 100644
> > --- a/hw/timer/aspeed_timer.c
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -9,13 +9,12 @@
> >   * the COPYING file in the top-level directory.
> >   */
> > 
> > +#include 
> >  #include "qemu/osdep.h"
> osdep.h must always be the first include.

Thanks for picking that up.

> 
> > 
> > -#include "hw/ptimer.h"
> >  #include "hw/sysbus.h"
> >  #include "hw/timer/aspeed_timer.h"
> >  #include "qemu-common.h"
> >  #include "qemu/bitops.h"
> > -#include "qemu/main-loop.h"
> >  #include "qemu/timer.h"
> >  #include "qemu/log.h"
> >  #include "trace.h"
> > @@ -77,21 +76,99 @@ static inline bool timer_can_pulse(AspeedTimer *t)
> >      return t->id >= TIMER_FIRST_CAP_PULSE;
> >  }
> > 
> > +static inline bool timer_external_clock(AspeedTimer *t)
> > +{
> > +    return timer_ctrl_status(t, op_external_clock);
> > +}
> > +
> > +static double clock_rates[] = { TIMER_CLOCK_APB_HZ, TIMER_CLOCK_EXT_HZ };
> > +
> > +static inline double calculate_rate(struct AspeedTimer *t)
> > +{
> > +    return clock_rates[timer_external_clock(t)];
> > +}
> > +
> > +static inline double calculate_period(struct AspeedTimer *t)
> > +{
> > +    return NANOSECONDS_PER_SECOND / calculate_rate(t);
> > +}
> > +
> > +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t 
> > now_ns)
> > +{
> > +    uint64_t delta_ns = now_ns - MIN(now_ns, t->start);
> > +    uint32_t ticks = (uint32_t) floor(delta_ns / calculate_period(t));
> Do we really need to do this with floating point ?

I went with floating point to avoid accumulating errors from truncation
by integer division. At eg 24MHz truncation increases the tick rate by
approximately 1 in 63. We're using floor() here, but that only
truncates at the end of the calculation, so the fractional current
tick.

Having said that, grep'ing around under {,include/}hw/ doesn't show a
lot of use of floating point. It looks like we are explicitly avoiding
it, however with a quick search I didn't find it mentioned in any of
the docs. What's the reasoning? Should I use a fixed-point approach
like ptimer?

> 
> > 
> > +
> > +    return t->reload - MIN(t->reload, ticks);
> > +}
> > +
> > +static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t 
> > ticks)
> > +{
> > +    uint64_t delta_ns;
> > +
> > +    ticks = MIN(t->reload, ticks);
> > +    delta_ns = (uint64_t) floor((t->reload - ticks) * calculate_period(t));
> > +
> > +    return t->start + delta_ns;
> > +}
> > +
> > +static uint64_t calculate_next(struct AspeedTimer *t)
> > +{
> > +    uint64_t now;
> > +    uint64_t next;
> > +    int i;
> > +    /* We don't know the relationship between the values in the match
> > +     * registers, so sort using MAX/MIN/zero. We sort in that order as the
> > +     * timer counts down to zero. */
> > +    uint64_t seq[] = {
> > +        MAX(t->match[0], t->match[1]),
> > +        MIN(t->match[0], t->match[1]),
> > +        0,
> > +    };
> > +
> > +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    for (i = 0; i < ARRAY_SIZE(seq); i++) {
> > +        next = calculate_time(t, seq[i]);
> > +        if (now < next) {
> > +            return next;
> > +        }
> > +    }
> > +
> > +    {
> > +        uint64_t reload_ns;
> > +
> > +        reload_ns = (uint64_t) floor(t->reload * calculate_period(t));
> > +        t->start = now - ((now - t->start) % reload_ns);
> > +    }
> > +
> > +    return calculate_next(t);
> Why is this recursing ?

The common case is that we return during iterating over seq array i.e.
we're anticipating another match event (either from the match values or
the timer reaching zero). If not then we've overrun, so we reinitialise
the timer by resetting the 'start' reference point then searching again
for the next event (iterating over seq). As the search for the next
event is encoded in the function, I've recursed to reuse it.

Would you prefer a loop here?

Considering the two approaches (recursion vs loop), if TCO doesn't
apply we could blow the stack, and with loops or TCO it's possible we
could spin here indefinitely if the timer period is shorter than the
time it takes to recalculate. Arguably, not crashing is better so I'm
happy to rework it.

As an aside, the approach for reinitialising start skips countdown
periods that were completely missed through high service latency, and
there will be no interrupts issued for the missing events. Is that
reasonable?

> 
> > 
> > +}
> > +
> >  static void aspeed_timer_expire(void *opaque)
> >  {
> >      AspeedTimer *t = opaque;
> > +    bool interrupt = false;
> > +    uint32_t ticks;
> > 
> > -    /* Only support interrupts on match values of zero for the moment - 
> > this is
> > -     * sufficient to boot an aspeed_defconfig Linux kernel.
> > -     *
> > -     * TODO: matching on arbitrary values (see e.g. hw/timer/a9gtimer.c)
> > -     */
> > -    bool match = !(t->match[0] && t->match[1]);
> > -    bool interrupt = timer_overflow_interrupt(t) || match;
> > -    if (timer_enabled(t) && interrupt) {
> > +    if (!timer_enabled(t)) {
> > +        return;
> > +    }
> > +
> > +    ticks = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> > +
> > +    if (!ticks) {
> > +        interrupt = timer_overflow_interrupt(t) || !t->match[0] || 
> > !t->match[1];
> > +    } else if (ticks <= MIN(t->match[0], t->match[1])) {
> > +        interrupt = true;
> > +    } else if (ticks <= MAX(t->match[0], t->match[1])) {
> > +        interrupt = true;
> > +    }
> > +
> > +    if (interrupt) {
> >          t->level = !t->level;
> >          qemu_set_irq(t->irq, t->level);
> >      }
> > +
> > +    timer_mod(&t->timer, calculate_next(t));
> >  }
> > 
> >  static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
> > @@ -100,7 +177,7 @@ static uint64_t aspeed_timer_get_value(AspeedTimer *t, 
> > int reg)
> > 
> >      switch (reg) {
> >      case TIMER_REG_STATUS:
> > -        value = ptimer_get_count(t->timer);
> > +        value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> >          break;
> >      case TIMER_REG_RELOAD:
> >          value = t->reload;
> > @@ -160,24 +237,21 @@ static void 
> > aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> >      switch (reg) {
> >      case TIMER_REG_STATUS:
> >          if (timer_enabled(t)) {
> > -            ptimer_set_count(t->timer, value);
> > +            uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +            int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, 
> > now);
> > +
> > +            t->start += delta * calculate_period(t);
> > +            timer_mod(&t->timer, calculate_next(t));
> >          }
> >          break;
> >      case TIMER_REG_RELOAD:
> >          t->reload = value;
> > -        ptimer_set_limit(t->timer, value, 1);
> >          break;
> >      case TIMER_REG_MATCH_FIRST:
> >      case TIMER_REG_MATCH_SECOND:
> > -        if (value) {
> > -            /* Non-zero match values are unsupported. As such an interrupt 
> > will
> > -             * always be triggered when the timer reaches zero even if the
> > -             * overflow interrupt control bit is clear.
> > -             */
> > -            qemu_log_mask(LOG_UNIMP, "%s: Match value unsupported by 
> > device: "
> > -                    "0x%" PRIx32 "\n", __func__, value);
> > -        } else {
> > -            t->match[reg - 2] = value;
> > +        t->match[reg - 2] = value;
> > +        if (timer_enabled(t)) {
> > +            timer_mod(&t->timer, calculate_next(t));
> >          }
> >          break;
> >      default:
> > @@ -196,21 +270,16 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, 
> > bool enable)
> >  {
> >      trace_aspeed_timer_ctrl_enable(t->id, enable);
> >      if (enable) {
> > -        ptimer_run(t->timer, 0);
> > +        t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        timer_mod(&t->timer, calculate_next(t));
> >      } else {
> > -        ptimer_stop(t->timer);
> > -        ptimer_set_limit(t->timer, t->reload, 1);
> > +        timer_del(&t->timer);
> >      }
> >  }
> > 
> >  static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
> >  {
> >      trace_aspeed_timer_ctrl_external_clock(t->id, enable);
> > -    if (enable) {
> > -        ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
> > -    } else {
> > -        ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
> > -    }
> > 
> >  }
> > 
> >  static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool 
> > enable)
> > @@ -351,12 +420,10 @@ static const MemoryRegionOps aspeed_timer_ops = {
> > 
> >  static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
> >  {
> > -    QEMUBH *bh;
> >      AspeedTimer *t = &s->timers[id];
> > 
> >      t->id = id;
> > -    bh = qemu_bh_new(aspeed_timer_expire, t);
> > -    t->timer = ptimer_init(bh);
> > +    timer_init_ns(&t->timer, QEMU_CLOCK_VIRTUAL, aspeed_timer_expire, t);
> >  }
> > 
> >  static void aspeed_timer_realize(DeviceState *dev, Error **errp)
> > @@ -404,7 +471,7 @@ static const VMStateDescription vmstate_aspeed_timer = {
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UINT8(id, AspeedTimer),
> >          VMSTATE_INT32(level, AspeedTimer),
> > -        VMSTATE_PTIMER(timer, AspeedTimer),
> > +        VMSTATE_TIMER(timer, AspeedTimer),
> >          VMSTATE_UINT32(reload, AspeedTimer),
> >          VMSTATE_UINT32_ARRAY(match, AspeedTimer, 2),
> >          VMSTATE_END_OF_LIST()
> You need to bump the vmstate version_id and minimum_version_id if
> you change the format (and then the version number you use in the
> VMSTATE_STRUCT_ARRAY that refers to this one).

Will do, thanks for clarifying.

Cheers,

Andrew

signature.asc
Description: This is a digitally signed message part