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

[Dmitry Osipenko <digetx@gmail.com>]

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 <digetx@gmail.com>
> ---
>   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