Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Peter Maydell]

On 27 May 2016 at 18:03, Dmitry Osipenko <address@hidden> wrote:
> Currently ptimer prints error message and clears enable flag for an arming
> timer that has delta = load = 0. That actually is a valid case for most
> of the timers, like instant IRQ trigger for oneshot timer or continuous in
> periodic mode. There are different possible policies of how particular timer
> could interpret setting counter to 0, like:
>
> 1) Immediate stop with triggering the interrupt in oneshot mode. Immediate
>    wraparound with triggering the interrupt in continuous mode.
>
> 2) Immediate stop without triggering the interrupt in oneshot mode. Immediate
>    wraparound without triggering the interrupt in continuous mode.
>
> 3) Stopping with triggering the interrupt after one period in oneshot mode.
>    Wraparound with triggering the interrupt in continuous mode after one
>    period.
>
> 4) Stopping without triggering the interrupt after one period in oneshot mode.
>    Wraparound without triggering the interrupt in continuous mode after one
>    period.
>
> As well as handling oneshot/continuous modes differently.
>
> Given that, currently, running the timer with counter/period equal to 0 is
> treated as a error case, it's not obvious what policies should be supported
> by ptimer. Let's implement the third policy for now, as it is known to be
> used by the ARM MPTimer.
>
> Explicitly forbid running with counter/period == 0 in all cases by aborting
> QEMU execution instead of printing the error message.
>
> Bump VMSD version since a new VMState member is added.

I'm afraid you can't do that -- the ptimer code is used by a lot of
platforms including some which do not permit migration compatibility
breaks (at least sparc, for instance).

If you want to add this feature you need to at least make it seamlessly
support migration from the old version using a vmstate subsection.

Better would be to make the policy a QOM property -- then it is
constant for the lifetime of the timer and doesn't need to be migrated
at all. (There's no reason to support timers which change policy at
runtime, I hope). The default property value should be "same behaviour
as previously".

This whole change would be more solidly supported if it came with
a clear description of the bug being fixed (ie what device is currently
using the default-but-wrong behaviour and is going to be fixed by
setting a different policy).

> Signed-off-by: Dmitry Osipenko <address@hidden>
> ---
>  hw/core/ptimer.c    | 53 
> +++++++++++++++++++++++++++++++----------------------
>  include/hw/ptimer.h |  6 ++++++
>  2 files changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 05b0c27..9bc70f5 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -21,6 +21,7 @@ struct ptimer_state
>      int64_t period;
>      int64_t last_event;
>      int64_t next_event;
> +    uint8_t policy;
>      QEMUBH *bh;
>      QEMUTimer *timer;
>  };
> @@ -37,15 +38,16 @@ static void ptimer_reload(ptimer_state *s)
>  {
>      uint32_t period_frac = s->period_frac;
>      uint64_t period = s->period;
> +    uint64_t delta = MAX(1, s->delta);
>
> -    if (s->delta == 0) {
> -        ptimer_trigger(s);
> -        s->delta = s->limit;
> +    if (s->delta == 0 && s->policy == UNIMPLEMENTED) {
> +        hw_error("ptimer: Running with counter=0 is unimplemented by " \
> +                 "this timer, fix it!\n");

hw_error() is effectively a verbose assert. Have you checked that
all the users of ptimer do the right thing? If not, we can't
assert() until we've fixed them all. Similarly below.
Note that what we had previously was just a warning-and-continue

>      }
> -    if (s->delta == 0 || s->period == 0) {
> -        fprintf(stderr, "Timer with period zero, disabling\n");
> -        s->enabled = 0;
> -        return;
> +
> +    if (period == 0) {
> +        hw_error("ptimer: Timer tries to run with period=0, behaviour is " \
> +                 "undefined, fix it!\n");
>      }
>
>      /*
> @@ -57,15 +59,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);
>  }
> @@ -73,27 +75,30 @@ static void ptimer_reload(ptimer_state *s)
>  static void ptimer_tick(void *opaque)
>  {
>      ptimer_state *s = (ptimer_state *)opaque;
> -    ptimer_trigger(s);
> -    s->delta = 0;
> -    if (s->enabled == 2) {
> +
> +    s->delta = (s->enabled == 1) ? s->limit : 0;
> +
> +    if (s->delta == 0) {
>          s->enabled = 0;
>      } else {
>          ptimer_reload(s);
>      }
> +
> +    ptimer_trigger(s);
>  }
>
>  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);
>          bool oneshot = (s->enabled == 2);
>
>          /* Figure out the current counter value.  */
> -        if (s->period == 0 || (expired && (oneshot || use_icount))) {
> +        if (expired && (oneshot || use_icount)) {
>              /* Prevent timer underflowing if it should already have
>                 triggered.  */
>              counter = 0;
> @@ -165,11 +170,8 @@ void ptimer_run(ptimer_state *s, int oneshot)
>  {
>      bool was_disabled = !s->enabled;
>
> -    if (was_disabled && s->period == 0) {
> -        fprintf(stderr, "Timer with period zero, disabling\n");
> -        return;
> -    }
>      s->enabled = oneshot ? 2 : 1;
> +

Stray extra blank line.

>      if (was_disabled) {
>          s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          ptimer_reload(s);
> @@ -203,6 +205,7 @@ void ptimer_set_period(ptimer_state *s, int64_t period)
>  /* Set counter frequency in Hz.  */
>  void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>  {
> +    g_assert(freq != 0 && freq <= 1000000000);

This probably needs an LL suffix like the others below.

>      s->delta = ptimer_get_count(s);
>      s->period = 1000000000ll / freq;
>      s->period_frac = (1000000000ll << 32) / freq;
> @@ -230,10 +233,15 @@ uint64_t ptimer_get_limit(ptimer_state *s)
>      return s->limit;
>  }
>
> +void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy)
> +{
> +    s->policy = policy;
> +}
> +
>  const VMStateDescription vmstate_ptimer = {
>      .name = "ptimer",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(enabled, ptimer_state),
>          VMSTATE_UINT64(limit, ptimer_state),
> @@ -243,6 +251,7 @@ const VMStateDescription vmstate_ptimer = {
>          VMSTATE_INT64(last_event, ptimer_state),
>          VMSTATE_INT64(next_event, ptimer_state),
>          VMSTATE_TIMER_PTR(timer, ptimer_state),
> +        VMSTATE_UINT8(policy, ptimer_state),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
> index e397db5..221f591 100644
> --- a/include/hw/ptimer.h
> +++ b/include/hw/ptimer.h
> @@ -12,6 +12,11 @@
>  #include "qemu/timer.h"
>  #include "migration/vmstate.h"
>
> +enum ptimer_policy {
> +    UNIMPLEMENTED,
> +    SET_CNT_TO_0_TRIGGERS_IRQ_AFTER_ONE_PERIOD,

Given that enums aren't namespaced, "UNIMPLEMENTED" is too broad;
you need some PTIMER_ prefixes. Also the second one of these is
too long a name.

> +};
> +
>  /* ptimer.c */
>  typedef struct ptimer_state ptimer_state;
>  typedef void (*ptimer_cb)(void *opaque);
> @@ -25,6 +30,7 @@ uint64_t ptimer_get_count(ptimer_state *s);
>  void ptimer_set_count(ptimer_state *s, uint64_t count);
>  void ptimer_run(ptimer_state *s, int oneshot);
>  void ptimer_stop(ptimer_state *s);
> +void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy);

Documentation comment headers for new global functions, please.

>  extern const VMStateDescription vmstate_ptimer;

thanks
-- PMM