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

[Dmitry Osipenko]

On 23.06.2016 16:49, Peter Maydell wrote:
> 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.
> 

This patch isn't supposed to change behaviour for any of the current timers. I
think it is clearly expressed in the last sentence of the commit message. There
is one unintended behaviour change in this patch, it's my overlook [see below].

>> 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?
> 

I removed the in-place type conversion further:
>>      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?
> 

That's a good question. In the current ARM MPTimer conversion patch, I assume
that delta = 0 would stop the timer regardless of it's mode and ptimer user
would itself re-arm timer in a tick() handler if needed. However, after your
question, I think it's a bit unintuitive and needs to be changed to continuous
*ptimer* trigger in periodic mode in case of the deferred trigger policy.

>>          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.
> 

Right, now I see that ptimer_trigger() is missed in case of running timer with a
default policy and delta = 0. Thanks for the note, will fix it. I'll craft
QTests for the ptimer, shouldn't take a lot of effort.

>>      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.
> 

Agree :) I have hit it couple times during the work on the ARM MPTimer patch in
the past, decided that it may not worth a whole patch for a such simple 
one-liner.

>>      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?
> 

Ooops, good catch!

Thanks for the review!

-- 
Dmitry