Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Peter Maydell]

On 17 June 2016 at 14:17, Dmitry Osipenko <address@hidden> wrote:
> Currently ptimer prints error message and stops the running timer that has
> delta (counter) = 0, this is an incorrect behaviour for some of the ptimer
> users. There are different variants of how particular timer could handle that
> case besides stopping the timer, like immediate or deferred IRQ trigger.
> Introduce policy feature that provides ptimer with an information about the
> correct behaviour.
>
> Implement the "counter = 0 triggers IRQ after one period" policy, as it is
> known to be used by the ARM MPTimer and set "default" policy to all ptimer
> users, maintaining old behaviour till they get fixed.

Could you split this into:
 (1) a patch which just adds the new argument to ptimer_init() and
     updates all its callers
 (2) a patch which adds support for setting the policy option to
     something other than the default value

and also make sure that we only do one change per patch -- there
seem to be several different behaviour changes tangled up in
one patch here.

I think that will be easier to review.

> Signed-off-by: Dmitry Osipenko <address@hidden>

> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 05b0c27..289e23e 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_mask;
>      QEMUBH *bh;
>      QEMUTimer *timer;
>  };
> @@ -35,14 +36,15 @@ static void ptimer_trigger(ptimer_state *s)
>
>  static void ptimer_reload(ptimer_state *s)
>  {
> -    uint32_t period_frac = s->period_frac;
> +    int64_t period_frac = s->period_frac;

Why does this variable change type?

>      uint64_t period = s->period;
> +    uint64_t delta = s->delta;
>
> -    if (s->delta == 0) {
> -        ptimer_trigger(s);
> -        s->delta = s->limit;
> +    if (delta == 0 && (s->policy_mask & PTIMER_POLICY_CNT_0_DEFERRED_TRIG)) {
> +        delta = 1;
>      }
> -    if (s->delta == 0 || s->period == 0) {
> +
> +    if (delta == 0 || period == 0) {
>          fprintf(stderr, "Timer with period zero, disabling\n");
>          s->enabled = 0;
>          return;
> @@ -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 += (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) {

This seems to be a change not guarded by a check of a policy bit?

>          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,10 +170,6 @@ 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;
> -    }

If the default policy value was provided, we shouldn't change behaviour.

>      s->enabled = oneshot ? 2 : 1;
>      if (was_disabled) {
>          s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -203,6 +204,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 <= 1000000000ll);

This seems to be an unrelated change.

>      s->delta = ptimer_get_count(s);
>      s->period = 1000000000ll / freq;
>      s->period_frac = (1000000000ll << 32) / freq;
> @@ -232,8 +234,8 @@ uint64_t ptimer_get_limit(ptimer_state *s)
>
>  const VMStateDescription vmstate_ptimer = {
>      .name = "ptimer",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,

Why are we bumping the version ID here?

>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(enabled, ptimer_state),
>          VMSTATE_UINT64(limit, ptimer_state),
> @@ -247,12 +249,13 @@ const VMStateDescription vmstate_ptimer = {
>      }
>  };
>
> -ptimer_state *ptimer_init(QEMUBH *bh)
> +ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
>  {
>      ptimer_state *s;
>
>      s = (ptimer_state *)g_malloc0(sizeof(ptimer_state));
>      s->bh = bh;
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ptimer_tick, s);
> +    s->policy_mask = policy_mask;
>      return s;
>  }

thanks
-- PMM