[Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Dmitry Osipenko]

ptimer_get_count() returns incorrect value for the disabled timer after
reloading the counter with a small value, because corrected limit value
is used instead of the original.

For instance:
    1) ptimer_stop(t)
    2) ptimer_set_period(t, 1)
    3) ptimer_set_limit(t, 0, 1)
    4) ptimer_get_count(t) <-- would return 10000 instead of 0

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/core/ptimer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 8437bd6..abc3a20 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
    count = limit.  */
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 {
+    uint64_t count = limit;
+
     /*
      * Artificially limit timeout rate to something
      * achievable under QEMU.  Otherwise, QEMU spends all
@@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 
     s->limit = limit;
     if (reload)
-        s->delta = limit;
+        s->delta = count;
     if (s->enabled && reload) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         ptimer_reload(s);
-- 
2.6.0

[Qemu-devel] [PATCH v5 2/2] arm_mptimer: Convert to use ptimer

[Dmitry Osipenko]

Current ARM MPTimer implementation uses QEMUTimer for the actual timer,
this implementation isn't complete and mostly tries to duplicate of what
generic ptimer is already doing fine.

Conversion to ptimer brings the following benefits and fixes:
	- Simple timer pausing implementation
	- Fixes counter value preservation after stopping the timer
	- Code simplification and reduction

Bump VMSD to version 3, since VMState is changed and is not compatible
with the previuos implementation.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

Changelog:
    V2: Fixed changing periodic timer counter value "on the fly". I added a
        test to the gist to cover that issue.
    V3: Fixed starting the timer with load = 0 and counter != 0, added tests
        to the gist for this issue. Changed vmstate version for all VMSD's,
        since loadvm doesn't check version of nested VMSD.
    V4: Fixed spurious IT bit set for the timer starting in the periodic mode
        with counter = 0. Test added.
    V5: Code cleanup, now depends on ptimer_set_limit() fix.

Tests: https://gist.github.com/digetx/dbd46109503b1a91941a

 hw/timer/arm_mptimer.c         | 110 ++++++++++++++++++-----------------------
 include/hw/timer/arm_mptimer.h |   4 +-
 2 files changed, 49 insertions(+), 65 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 3e59c2a..c06da5e 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -19,8 +19,9 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "hw/ptimer.h"
 #include "hw/timer/arm_mptimer.h"
-#include "qemu/timer.h"
+#include "qemu/main-loop.h"
 #include "qom/cpu.h"
 
 /* This device implements the per-cpu private timer and watchdog block
@@ -47,28 +48,10 @@ static inline uint32_t timerblock_scale(TimerBlock *tb)
     return (((tb->control >> 8) & 0xff) + 1) * 10;
 }
 
-static void timerblock_reload(TimerBlock *tb, int restart)
-{
-    if (tb->count == 0) {
-        return;
-    }
-    if (restart) {
-        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    }
-    tb->tick += (int64_t)tb->count * timerblock_scale(tb);
-    timer_mod(tb->timer, tb->tick);
-}
-
 static void timerblock_tick(void *opaque)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
     tb->status = 1;
-    if (tb->control & 2) {
-        tb->count = tb->load;
-        timerblock_reload(tb, 0);
-    } else {
-        tb->count = 0;
-    }
     timerblock_update_irq(tb);
 }
 
@@ -76,21 +59,11 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
                                 unsigned size)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
-    int64_t val;
     switch (addr) {
     case 0: /* Load */
         return tb->load;
     case 4: /* Counter.  */
-        if (((tb->control & 1) == 0) || (tb->count == 0)) {
-            return 0;
-        }
-        /* Slow and ugly, but hopefully won't happen too often.  */
-        val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        val /= timerblock_scale(tb);
-        if (val < 0) {
-            val = 0;
-        }
-        return val;
+        return ptimer_get_count(tb->timer);
     case 8: /* Control.  */
         return tb->control;
     case 12: /* Interrupt status.  */
@@ -100,6 +73,19 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
     }
 }
 
+static void timerblock_run(TimerBlock *tb, uint64_t count, int set_count)
+{
+    if (set_count) {
+        if (((tb->control & 3) == 3) && (count == 0)) {
+            count = tb->load;
+        }
+        ptimer_set_count(tb->timer, count);
+    }
+    if ((tb->control & 1) && (count != 0)) {
+        ptimer_run(tb->timer, !(tb->control & 2));
+    }
+}
+
 static void timerblock_write(void *opaque, hwaddr addr,
                              uint64_t value, unsigned size)
 {
@@ -108,32 +94,34 @@ static void timerblock_write(void *opaque, hwaddr addr,
     switch (addr) {
     case 0: /* Load */
         tb->load = value;
-        /* Fall through.  */
-    case 4: /* Counter.  */
-        if ((tb->control & 1) && tb->count) {
-            /* Cancel the previous timer.  */
-            timer_del(tb->timer);
+        /* Setting load to 0 stops the timer.  */
+        if (tb->load == 0) {
+            ptimer_stop(tb->timer);
         }
-        tb->count = value;
-        if (tb->control & 1) {
-            timerblock_reload(tb, 1);
+        ptimer_set_limit(tb->timer, tb->load, 1);
+        timerblock_run(tb, tb->load, 0);
+        break;
+    case 4: /* Counter.  */
+        /* Setting counter to 0 stops the one-shot timer.  */
+        if (!(tb->control & 2) && (value == 0)) {
+            ptimer_stop(tb->timer);
         }
+        timerblock_run(tb, value, 1);
         break;
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (value & 1) {
-            if ((old & 1) && (tb->count != 0)) {
-                /* Do nothing if timer is ticking right now.  */
-                break;
-            }
-            if (tb->control & 2) {
-                tb->count = tb->load;
-            }
-            timerblock_reload(tb, 1);
-        } else if (old & 1) {
-            /* Shutdown the timer.  */
-            timer_del(tb->timer);
+        /* Timer mode switch requires ptimer to be stopped.  */
+        if ((old & 3) != (tb->control & 3)) {
+            ptimer_stop(tb->timer);
+        }
+        if (!(tb->control & 1)) {
+            break;
+        }
+        ptimer_set_period(tb->timer, timerblock_scale(tb));
+        if ((old & 3) != (tb->control & 3)) {
+            value = ptimer_get_count(tb->timer);
+            timerblock_run(tb, value, 1);
         }
         break;
     case 12: /* Interrupt status.  */
@@ -184,13 +172,12 @@ static const MemoryRegionOps timerblock_ops = {
 
 static void timerblock_reset(TimerBlock *tb)
 {
-    tb->count = 0;
     tb->load = 0;
     tb->control = 0;
     tb->status = 0;
-    tb->tick = 0;
     if (tb->timer) {
-        timer_del(tb->timer);
+        ptimer_stop(tb->timer);
+        ptimer_set_limit(tb->timer, 0, 1);
     }
 }
 
@@ -235,7 +222,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
      */
     for (i = 0; i < s->num_cpu; i++) {
         TimerBlock *tb = &s->timerblock[i];
-        tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb);
+        QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
+        tb->timer = ptimer_init(bh);
         sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);
@@ -245,26 +233,24 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_timerblock = {
     .name = "arm_mptimer_timerblock",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(count, TimerBlock),
         VMSTATE_UINT32(load, TimerBlock),
         VMSTATE_UINT32(control, TimerBlock),
         VMSTATE_UINT32(status, TimerBlock),
-        VMSTATE_INT64(tick, TimerBlock),
-        VMSTATE_TIMER_PTR(timer, TimerBlock),
+        VMSTATE_PTIMER(timer, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static const VMStateDescription vmstate_arm_mptimer = {
     .name = "arm_mptimer",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
-                                     2, vmstate_timerblock, TimerBlock),
+                                     3, vmstate_timerblock, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
index b34cba0..93db61b 100644
--- a/include/hw/timer/arm_mptimer.h
+++ b/include/hw/timer/arm_mptimer.h
@@ -27,12 +27,10 @@
 
 /* State of a single timer or watchdog block */
 typedef struct {
-    uint32_t count;
     uint32_t load;
     uint32_t control;
     uint32_t status;
-    int64_t tick;
-    QEMUTimer *timer;
+    struct ptimer_state *timer;
     qemu_irq irq;
     MemoryRegion iomem;
 } TimerBlock;
-- 
2.6.0

Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Peter Maydell]

On 14 October 2015 at 00:30, Dmitry Osipenko <digetx@gmail.com> wrote:
> ptimer_get_count() returns incorrect value for the disabled timer after
> reloading the counter with a small value, because corrected limit value
> is used instead of the original.
>
> For instance:
>     1) ptimer_stop(t)
>     2) ptimer_set_period(t, 1)
>     3) ptimer_set_limit(t, 0, 1)
>     4) ptimer_get_count(t) <-- would return 10000 instead of 0
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/core/ptimer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 8437bd6..abc3a20 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>     count = limit.  */
>  void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>  {
> +    uint64_t count = limit;
> +
>      /*
>       * Artificially limit timeout rate to something
>       * achievable under QEMU.  Otherwise, QEMU spends all
> @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>
>      s->limit = limit;
>      if (reload)
> -        s->delta = limit;
> +        s->delta = count;
>      if (s->enabled && reload) {
>          s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          ptimer_reload(s);

Doesn't this defeat the rate limiting if the timer is enabled,
though? ptimer_reload() sets the underlying timer based on
s->delta, so if s->delta isn't the rate-limited value then the
timer will be incorrectly set to a very close-in value.

I think we'll return "incorrect" values from ptimer_get_count()
in the "counter is running" case too, because we calculate those
by looking at when the underlying timer's due to expire, and
we set the expiry time based on the adjusted value.

What's the underlying model we should have for what values
we return from reading the count if we've decided to adjust
the actual timer expiry with the rate limit? Should the
count go down from the specified value and then just hang
at 1 until the extended timer expiry time hits? Or something
else? Clearly defining what we want to happen ought to make
it easier to review attempts to fix it...

(Calling ptimer_set_count() also bypasses the ratelimiting at
the moment, incidentally.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Dmitry Osipenko]

19.10.2015 20:06, Peter Maydell пишет:
> On 14 October 2015 at 00:30, Dmitry Osipenko <digetx@gmail.com> wrote:
>> ptimer_get_count() returns incorrect value for the disabled timer after
>> reloading the counter with a small value, because corrected limit value
>> is used instead of the original.
>>
>> For instance:
>>      1) ptimer_stop(t)
>>      2) ptimer_set_period(t, 1)
>>      3) ptimer_set_limit(t, 0, 1)
>>      4) ptimer_get_count(t) <-- would return 10000 instead of 0
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>   hw/core/ptimer.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>> index 8437bd6..abc3a20 100644
>> --- a/hw/core/ptimer.c
>> +++ b/hw/core/ptimer.c
>> @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>>      count = limit.  */
>>   void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>>   {
>> +    uint64_t count = limit;
>> +
>>       /*
>>        * Artificially limit timeout rate to something
>>        * achievable under QEMU.  Otherwise, QEMU spends all
>> @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>>
>>       s->limit = limit;
>>       if (reload)
>> -        s->delta = limit;
>> +        s->delta = count;
>>       if (s->enabled && reload) {
>>           s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>           ptimer_reload(s);
>
> Doesn't this defeat the rate limiting if the timer is enabled,
> though? ptimer_reload() sets the underlying timer based on
> s->delta, so if s->delta isn't the rate-limited value then the
> timer will be incorrectly set to a very close-in value.
>

Yes, it defeats the rate limiting for the first timer expire. As I understand, 
the idea of the rate limit correction aims periodic timer only.

"Otherwise, QEMU spends all its time generating timer interrupts, and there is 
no forward progress.", as stated in the comment.

I think it's not a problem to get "instant" timer trigger for the first expire.

> I think we'll return "incorrect" values from ptimer_get_count()
> in the "counter is running" case too, because we calculate those
> by looking at when the underlying timer's due to expire, and
> we set the expiry time based on the adjusted value.
>

That's a good point.

> What's the underlying model we should have for what values
> we return from reading the count if we've decided to adjust
> the actual timer expiry with the rate limit? Should the
> count go down from the specified value and then just hang
> at 1 until the extended timer expiry time hits? Or something
> else? Clearly defining what we want to happen ought to make
> it easier to review attempts to fix it...
>

What about the following:

Add additional ptimer struct member, say "limit_corrected", to check whether the 
limit was corrected or not.

ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
{
	.limit_corrected = 0;

	// on the limit correction:
	.limit_corrected = (limit == 0) ? 1 : 2;
	limit = 10000 / s->period;
}

ptimer_get_count()
{
	if (enabled) {
		if (expired || .limit_corrected == 1) {
			counter = 0;
		} else if (.limit_corrected == 2) {
			counter = 1;
		} else {
			// do the counter calculations ...
		}
	}
}

and clear .limit_corrected on the one-shot timer start. That would bump ptimer 
VMSD version, but keep .minimum_version_id.

> (Calling ptimer_set_count() also bypasses the ratelimiting at
> the moment, incidentally.)
>

Should be okay if my "aiming the periodic timer only" thought is correct.

> thanks
> -- PMM
>

-- 
Dmitry

Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Peter Maydell]

On 19 October 2015 at 21:01, Dmitry Osipenko <digetx@gmail.com> wrote:
> 19.10.2015 20:06, Peter Maydell пишет:
>>
>> Doesn't this defeat the rate limiting if the timer is enabled,
>> though? ptimer_reload() sets the underlying timer based on
>> s->delta, so if s->delta isn't the rate-limited value then the
>> timer will be incorrectly set to a very close-in value.
>>
>
> Yes, it defeats the rate limiting for the first timer expire. As I
> understand, the idea of the rate limit correction aims periodic timer only.
>
> "Otherwise, QEMU spends all its time generating timer interrupts, and there
> is no forward progress.", as stated in the comment.
>
> I think it's not a problem to get "instant" timer trigger for the first
> expire.

Yes, that makes sense. If you trigger a one-shot timer then we
should prefer accuracy, on the assumption that the next setting
of the one-shot timer will be further in the future. (The guest
could in theory still starve us of forward progress with an
infinite series of near-future one-shot timers, but we can worry
about that if we see it.)

>> I think we'll return "incorrect" values from ptimer_get_count()
>> in the "counter is running" case too, because we calculate those
>> by looking at when the underlying timer's due to expire, and
>> we set the expiry time based on the adjusted value.
>>
>
> That's a good point.
>
>> What's the underlying model we should have for what values
>> we return from reading the count if we've decided to adjust
>> the actual timer expiry with the rate limit? Should the
>> count go down from the specified value and then just hang
>> at 1 until the extended timer expiry time hits? Or something
>> else? Clearly defining what we want to happen ought to make
>> it easier to review attempts to fix it...
>>
>
> What about the following:
>
> Add additional ptimer struct member, say "limit_corrected", to check whether
> the limit was corrected or not.
>
> ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
> {
>         .limit_corrected = 0;
>
>         // on the limit correction:
>         .limit_corrected = (limit == 0) ? 1 : 2;

It's a bit confusing that a field that sounds like it ought
to be a bool is taking values 0/1/2.

>         limit = 10000 / s->period;
> }
>
> ptimer_get_count()
> {
>         if (enabled) {
>                 if (expired || .limit_corrected == 1) {
>                         counter = 0;
>                 } else if (.limit_corrected == 2) {
>                         counter = 1;
>                 } else {
>                         // do the counter calculations ...
>                 }
>         }
> }

Not completely sure I understand this. A comment about what
the various states limit_corrected can be in might help.

> and clear .limit_corrected on the one-shot timer start. That would bump
> ptimer VMSD version, but keep .minimum_version_id.

Given how widely ptimer is used, you should probably handle it
via a subsection, not by a version number bump.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Dmitry Osipenko]

19.10.2015 23:01, Dmitry Osipenko пишет:
> What about the following:
>
> Add additional ptimer struct member, say "limit_corrected", to check whether the
> limit was corrected or not.
>
> ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
> {
>      .limit_corrected = 0;
>
>      // on the limit correction:
>      .limit_corrected = (limit == 0) ? 1 : 2;
>      limit = 10000 / s->period;
> }
>
> ptimer_get_count()
> {
>      if (enabled) {
>          if (expired || .limit_corrected == 1) {
>              counter = 0;
>          } else if (.limit_corrected == 2) {
>              counter = 1;
>          } else {
>              // do the counter calculations ...
>          }
>      }
> }
>
> and clear .limit_corrected on the one-shot timer start. That would bump ptimer
> VMSD version, but keep .minimum_version_id.
>

However, that would break set_counter(). Also limit should be corrected on 
period / freq change. I'll work more on it.

-- 
Dmitry