Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer

[Peter Crosthwaite]

On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <address@hidden> wrote:
> 30.01.2016 19:43, Dmitry Osipenko пишет:
>>
>> Due to rounding down performed by ptimer_get_count, it returns counter - 1
>> for
>> the active timer. That's incorrect because counter should decrement only
>> after
>> period been expired, not before. I.e. if running timer has been loaded
>> with
>> value X, then timer counter should stay with X until period expired and
>> decrement after. Fix this by adding 1 to the counter value for the active
>> and
>> unexpired timer.
>>
>> Signed-off-by: Dmitry Osipenko <address@hidden>
>> ---
>>   hw/core/ptimer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>> index 62f8cb1..b2044fb 100644
>> --- a/hw/core/ptimer.c
>> +++ b/hw/core/ptimer.c
>> @@ -136,7 +136,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>                   if ((uint32_t)(period_frac << shift))
>>                       div += 1;
>>               }
>> -            counter = rem / div;
>> +            counter = rem / div + (expired ? 0 : 1);
>>
>>               if (expired && counter != 0) {
>>                   /* Wrap around periodic counter.  */
>>
>
> Noticed one nit here:
>
> There is possibility to return timer counter = limit + 1, if the following
> ptimer calls execute in less than 1ns.
>
>         ptimer_run(t, 1);
>         // counter = 91, if set() count executed in less than 1ns
>         ptimer_set_count(t, 90);
>         counter = ptimer_get_count(t);
>
> Likely, it would be impossible to trigger that issue on a real current
> machine.
> But the fix is trivial, I'll incorporate it in V13 if it looks fine:
>
> ---
> @@ -76,20 +76,20 @@ static void ptimer_tick(void *opaque)
>  {
>      ptimer_state *s = (ptimer_state *)opaque;
>      s->delta = 0;
>      ptimer_reload(s);
>  }
>
>  uint64_t ptimer_get_count(ptimer_state *s)
>  {
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>      uint64_t counter;
>
> -    if (s->enabled && s->delta != 0) {
> -        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    if (s->enabled && s->delta != 0 && now != s->last_event) {
>          int64_t next = s->next_event;
>          bool expired = (now - next >= 0);
>          bool oneshot = (s->enabled == 2);
>
>          /* Figure out the current counter value.  */
>          if (expired && (oneshot || use_icount)) {
>              /* Prevent timer underflowing if it should already have
>                 triggered.  */
> @@ -131,17 +131,17 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              } else {
>                  if (shift != 0)
>                      div |= (period_frac >> (32 - shift));
>                  /* Look at remaining bits of period_frac and round div up
> if
>                     necessary.  */
>                  if ((uint32_t)(period_frac << shift))
>                      div += 1;
>              }
> -            counter = rem / div;
> +            counter = rem / div + (expired ? 0 : 1);
>

Sorry about the long delays. Im wondering what this has to do with
expiration though? the commit message suggests you want to change the
rounding scheme, so can that be done directly with DIV_ROUND_UP?

Regards,
Peter

>              if (expired && counter != 0) {
>                  /* Wrap around periodic counter.  */
>                  counter = s->limit - (counter - 1) % s->limit;
>              }
>          }
>      } else {
>          counter = s->delta;
>
>
> --
> Dmitry