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

[Dmitry Osipenko]

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);

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


--
Dmitry