All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
@ 2016-01-30 16:43 Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit value Dmitry Osipenko
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

Changelog for ARM MPTimer QEMUTimer to ptimer conversion:

    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.

    V6: No code change, added test to check ptimer_get_count() with corrected
        .limit value.

    V7: No change.

    V8: No change.

    V9: No change.

    V10: Correctly handle cases when counter = load = 0 and prescaler != 0,
         i.e. triggering interrupt in that case. Call ptimer_* only when
         certain MPTimer state was changed, like prescaler change. Factor out
         timerblock_set_count from timerblock_run and inline both.
         Tests updated.

    V11: Fixed missed periodic timer stopping on setting counter => 0 with
         load = 0 and prescaler = 0.

    v12: Timer isn't doing uninterruptible IRQ, but ticks continuously.
         On setting counter/load to 0 with prescaler != 0, timer would trigger
         IRQ after one period. Verified on real HW, tests updated.

ARM MPTimer tests: https://gist.github.com/digetx/dbd46109503b1a91941a


Patches for ptimer are introduced since V5 of "ARM MPTimer conversion".

Changelog for the ptimer patches:

    V5: Only fixed ptimer_set_limit() for the disabled timer.

    V6: As was pointed by Peter Maydell, there are other issues beyond
        ptimer_set_limit(), so V6 supposed to cover all those issues.

    V7: Added accidentally removed !use_icount check.
        Added missed "else" statement.

    V8: Adjust period instead of the limit and do it for periodic timer only
        (.limit adjusting bug). Added patch/fix for freq/period change and
        ptimer_get_count() improvement.

    V9: Don't do wrap around if counter == 0, otherwise polled periodic
        timer won't ever return counter = 0.

    V10: Addressed V8/9 review comments.
         Adjust timer period based on delta instead of limit.
         Don't wrap around when in icount mode.
         New patches: "on the fly" mode switch, silence error msg when
                      delta = load = 0, introduce ptimer_get_limit.

    V11: Dropped timer tick from "Perform tick and counter wrap around if
         timer already expired" patch since it would cause bogus tick after
         QEMU been reset if ptimer was stopped and it's QEMUtimer expired
         during reset.
         Patch "Legalize running with delta = load = 0" now explicitly
         forbids period = 0.

    v12: Fixed missed abort on setting freq > 1000000000.
         New patches:
            "Fix counter - 1 returned by ptimer_get_count for the active timer"
            "Perform delayed tick instead of immediate if delta = 0"

Dmitry Osipenko (9):
  hw/ptimer: Fix issues caused by the adjusted timer limit value
  hw/ptimer: Perform counter wrap around if timer already expired
  hw/ptimer: Update .delta on period/freq change
  hw/ptimer: Support "on the fly" timer mode switch
  hw/ptimer: Introduce ptimer_get_limit
  hw/ptimer: Legalize running with delta = load = 0 and abort on period
    = 0
  hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active
    timer
  hw/ptimer: Perform delayed tick instead of immediate if delta = 0
  arm_mptimer: Convert to use ptimer

 hw/core/ptimer.c               | 114 ++++++++++++++++++++---------------
 hw/timer/arm_mptimer.c         | 133 +++++++++++++++++++++--------------------
 include/hw/ptimer.h            |   1 +
 include/hw/timer/arm_mptimer.h |   5 +-
 4 files changed, 138 insertions(+), 115 deletions(-)

-- 
2.7.0

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit value
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already expired Dmitry Osipenko
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

Multiple issues here related to the timer with a adjusted .limit value:

1) ptimer_get_count() returns incorrect counter value for the disabled
timer after loading the counter with a small value, because adjusted 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

2) ptimer_get_count() might return incorrect value for the timer running
with a adjusted limit value.

For instance:
    1) ptimer_stop(t)
    2) ptimer_set_period(t, 1)
    3) ptimer_set_limit(t, 10, 1)
    4) ptimer_run(t)
    5) ptimer_get_count(t) <-- might return value > 10

3) Neither ptimer_set_period() nor ptimer_set_freq() are adjusting the
limit value, so it is still possible to make timer timeout value
arbitrary small.

For instance:
    1) ptimer_set_period(t, 10000)
    2) ptimer_set_limit(t, 1, 0)
    3) ptimer_set_period(t, 1) <-- bypass limit correction

Fix all of the above issues by adjusting timer period instead of the limit.
Perform the adjustment for periodic timer only. Use the delta value instead
of the limit to make decision whether adjustment is required, as limit could
be altered while timer is running, resulting in incorrect value returned by
ptimer_get_count.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/core/ptimer.c | 51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index edf077c..5edcd71 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -34,6 +34,9 @@ static void ptimer_trigger(ptimer_state *s)
 
 static void ptimer_reload(ptimer_state *s)
 {
+    uint32_t period_frac = s->period_frac;
+    uint64_t period = s->period;
+
     if (s->delta == 0) {
         ptimer_trigger(s);
         s->delta = s->limit;
@@ -44,10 +47,24 @@ static void ptimer_reload(ptimer_state *s)
         return;
     }
 
+    /*
+     * Artificially limit timeout rate to something
+     * achievable under QEMU.  Otherwise, QEMU spends all
+     * its time generating timer interrupts, and there
+     * is no forward progress.
+     * About ten microseconds is the fastest that really works
+     * on the current generation of host machines.
+     */
+
+    if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) {
+        period = 10000 / s->delta;
+        period_frac = 0;
+    }
+
     s->last_event = s->next_event;
-    s->next_event = s->last_event + s->delta * s->period;
-    if (s->period_frac) {
-        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
+    s->next_event = s->last_event + s->delta * period;
+    if (period_frac) {
+        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
     }
     timer_mod(s->timer, s->next_event);
 }
@@ -82,6 +99,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
             uint64_t div;
             int clz1, clz2;
             int shift;
+            uint32_t period_frac = s->period_frac;
+            uint64_t period = s->period;
+
+            if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+                period = 10000 / s->delta;
+                period_frac = 0;
+            }
 
             /* We need to divide time by period, where time is stored in
                rem (64-bit integer) and period is stored in period/period_frac
@@ -94,7 +118,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
             */
 
             rem = s->next_event - now;
-            div = s->period;
+            div = period;
 
             clz1 = clz64(rem);
             clz2 = clz64(div);
@@ -103,13 +127,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
             rem <<= shift;
             div <<= shift;
             if (shift >= 32) {
-                div |= ((uint64_t)s->period_frac << (shift - 32));
+                div |= ((uint64_t)period_frac << (shift - 32));
             } else {
                 if (shift != 0)
-                    div |= (s->period_frac >> (32 - shift));
+                    div |= (period_frac >> (32 - shift));
                 /* Look at remaining bits of period_frac and round div up if 
                    necessary.  */
-                if ((uint32_t)(s->period_frac << shift))
+                if ((uint32_t)(period_frac << shift))
                     div += 1;
             }
             counter = rem / div;
@@ -181,19 +205,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
    count = limit.  */
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 {
-    /*
-     * Artificially limit timeout rate to something
-     * achievable under QEMU.  Otherwise, QEMU spends all
-     * its time generating timer interrupts, and there
-     * is no forward progress.
-     * About ten microseconds is the fastest that really works
-     * on the current generation of host machines.
-     */
-
-    if (!use_icount && limit * s->period < 10000 && s->period) {
-        limit = 10000 / s->period;
-    }
-
     s->limit = limit;
     if (reload)
         s->delta = limit;
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already expired
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit value Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change Dmitry Osipenko
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

ptimer_get_count() might be called while QEMU timer already been expired.
In that case ptimer would return counter = 0, which might be undesirable
in case of polled timer. Do counter wrap around for periodic timer to keep
it distributed. In order to achieve more accurate emulation behaviour of
certain hardware, don't perform wrap around when in icount mode and return
counter = 0 in that case (that doesn't affect polled counter distribution).

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/core/ptimer.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 5edcd71..d4452d3 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -83,14 +83,16 @@ static void ptimer_tick(void *opaque)
 
 uint64_t ptimer_get_count(ptimer_state *s)
 {
-    int64_t now;
     uint64_t counter;
 
     if (s->enabled) {
-        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        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 (now - s->next_event > 0
-            || s->period == 0) {
+        if (s->period == 0 || (expired && (oneshot || use_icount))) {
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 0;
@@ -102,7 +104,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
             uint32_t period_frac = s->period_frac;
             uint64_t period = s->period;
 
-            if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+            if (!oneshot && (s->delta * period < 10000) && !use_icount) {
                 period = 10000 / s->delta;
                 period_frac = 0;
             }
@@ -117,7 +119,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
                backwards.
             */
 
-            rem = s->next_event - now;
+            rem = expired ? now - next : next - now;
             div = period;
 
             clz1 = clz64(rem);
@@ -137,6 +139,11 @@ uint64_t ptimer_get_count(ptimer_state *s)
                     div += 1;
             }
             counter = rem / div;
+
+            if (expired && counter != 0) {
+                /* Wrap around periodic counter.  */
+                counter = s->limit - (counter - 1) % s->limit;
+            }
         }
     } else {
         counter = s->delta;
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit value Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already expired Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 4/9] hw/ptimer: Support "on the fly" timer mode switch Dmitry Osipenko
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

Delta value must be updated on period/freq change, otherwise running timer
would be restarted (counter reloaded with old delta). Only m68k/mcf520x
and arm/arm_timer devices are currently doing freq change correctly, i.e.
stopping the timer. Perform delta update to fix affected devices and
eliminate potential further mistakes.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/core/ptimer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index d4452d3..f58790a 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -189,6 +189,7 @@ void ptimer_stop(ptimer_state *s)
 /* Set counter increment interval in nanoseconds.  */
 void ptimer_set_period(ptimer_state *s, int64_t period)
 {
+    s->delta = ptimer_get_count(s);
     s->period = period;
     s->period_frac = 0;
     if (s->enabled) {
@@ -200,6 +201,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)
 {
+    s->delta = ptimer_get_count(s);
     s->period = 1000000000ll / freq;
     s->period_frac = (1000000000ll << 32) / freq;
     if (s->enabled) {
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 4/9] hw/ptimer: Support "on the fly" timer mode switch
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (2 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 5/9] hw/ptimer: Introduce ptimer_get_limit Dmitry Osipenko
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

Allow switching between periodic <-> oneshot modes while timer is running.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/core/ptimer.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index f58790a..78ae443 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -162,16 +162,17 @@ void ptimer_set_count(ptimer_state *s, uint64_t count)
 
 void ptimer_run(ptimer_state *s, int oneshot)
 {
-    if (s->enabled) {
-        return;
-    }
-    if (s->period == 0) {
+    bool was_disabled = !s->enabled;
+
+    if (was_disabled && s->period == 0) {
         fprintf(stderr, "Timer with period zero, disabling\n");
         return;
     }
     s->enabled = oneshot ? 2 : 1;
-    s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    ptimer_reload(s);
+    if (was_disabled) {
+        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        ptimer_reload(s);
+    }
 }
 
 /* Pause a timer.  Note that this may cause it to "lose" time, even if it
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 5/9] hw/ptimer: Introduce ptimer_get_limit
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (3 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 4/9] hw/ptimer: Support "on the fly" timer mode switch Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 6/9] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0 Dmitry Osipenko
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

Currently ptimer users are used to store copy of the limit value, because
ptimer doesn't provide facility to retrieve the limit. Let's provide it.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/core/ptimer.c    | 5 +++++
 include/hw/ptimer.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 78ae443..bf62fdd 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -224,6 +224,11 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
     }
 }
 
+uint64_t ptimer_get_limit(ptimer_state *s)
+{
+    return s->limit;
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
     .version_id = 1,
diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index 8ebacbb..e397db5 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -19,6 +19,7 @@ typedef void (*ptimer_cb)(void *opaque);
 ptimer_state *ptimer_init(QEMUBH *bh);
 void ptimer_set_period(ptimer_state *s, int64_t period);
 void ptimer_set_freq(ptimer_state *s, uint32_t freq);
+uint64_t ptimer_get_limit(ptimer_state *s);
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload);
 uint64_t ptimer_get_count(ptimer_state *s);
 void ptimer_set_count(ptimer_state *s, uint64_t count);
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 6/9] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (4 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 5/9] hw/ptimer: Introduce ptimer_get_limit Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer Dmitry Osipenko
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

Currently ptimer would print error message and clear enable flag for an
arming timer that has delta = load = 0. That actually could be a valid case
for some hardware, like instant IRQ trigger for oneshot timer or continuous
in periodic mode. Support those cases by removing the error message and
stopping the timer when delta = 0. Abort execution when period = 0 instead
of printing the error message, otherwise there is a chance to miss that error.

In addition, don't re-load oneshot timer when delta = 0 and remove duplicated
code from ptimer_tick(), since ptimer_reload would invoke trigger and stop
the timer.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/core/ptimer.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index bf62fdd..62f8cb1 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -39,11 +39,14 @@ static void ptimer_reload(ptimer_state *s)
 
     if (s->delta == 0) {
         ptimer_trigger(s);
+    }
+
+    if (s->delta == 0 && s->enabled == 1) {
         s->delta = s->limit;
     }
-    if (s->delta == 0 || s->period == 0) {
-        fprintf(stderr, "Timer with period zero, disabling\n");
-        s->enabled = 0;
+
+    if (s->delta == 0) {
+        ptimer_stop(s);
         return;
     }
 
@@ -72,27 +75,22 @@ 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->enabled = 0;
-    } else {
-        ptimer_reload(s);
-    }
+    ptimer_reload(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;
@@ -164,12 +162,10 @@ 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;
-    }
     s->enabled = oneshot ? 2 : 1;
+
     if (was_disabled) {
+        g_assert(s->period != 0);
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         ptimer_reload(s);
     }
@@ -190,6 +186,7 @@ void ptimer_stop(ptimer_state *s)
 /* Set counter increment interval in nanoseconds.  */
 void ptimer_set_period(ptimer_state *s, int64_t period)
 {
+    g_assert(period != 0);
     s->delta = ptimer_get_count(s);
     s->period = period;
     s->period_frac = 0;
@@ -202,6 +199,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 <= 1000000000);
     s->delta = ptimer_get_count(s);
     s->period = 1000000000ll / freq;
     s->period_frac = (1000000000ll << 32) / freq;
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (5 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 6/9] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0 Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-02-02 15:19   ` Dmitry Osipenko
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0 Dmitry Osipenko
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

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.  */
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (6 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-03-08 21:08   ` Peter Crosthwaite
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 9/9] arm_mptimer: Convert to use ptimer Dmitry Osipenko
  2016-03-16 14:36 ` [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Peter Maydell
  9 siblings, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

It might be necessary by some emulated HW to perform the tick after one
period if delta = 0. Given that it is much less churny to implement immediate
tick by the ptimer user itself, let's make ptimer do the delayed tick.

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

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index b2044fb..bcd090c 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -36,19 +36,7 @@ static void ptimer_reload(ptimer_state *s)
 {
     uint32_t period_frac = s->period_frac;
     uint64_t period = s->period;
-
-    if (s->delta == 0) {
-        ptimer_trigger(s);
-    }
-
-    if (s->delta == 0 && s->enabled == 1) {
-        s->delta = s->limit;
-    }
-
-    if (s->delta == 0) {
-        ptimer_stop(s);
-        return;
-    }
+    uint64_t delta = MAX(1, s->delta);
 
     /*
      * Artificially limit timeout rate to something
@@ -59,15 +47,15 @@ static void ptimer_reload(ptimer_state *s)
      * on the current generation of host machines.
      */
 
-    if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) {
-        period = 10000 / s->delta;
+    if (s->enabled == 1 && (delta * period < 10000) && !use_icount) {
+        period = 10000 / delta;
         period_frac = 0;
     }
 
     s->last_event = s->next_event;
-    s->next_event = s->last_event + s->delta * period;
+    s->next_event = s->last_event + delta * period;
     if (period_frac) {
-        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
+        s->next_event += ((int64_t)period_frac * delta) >> 32;
     }
     timer_mod(s->timer, s->next_event);
 }
@@ -75,8 +63,16 @@ static void ptimer_reload(ptimer_state *s)
 static void ptimer_tick(void *opaque)
 {
     ptimer_state *s = (ptimer_state *)opaque;
-    s->delta = 0;
-    ptimer_reload(s);
+
+    s->delta = (s->enabled == 1) ? s->limit : 0;
+
+    if (s->delta == 0) {
+        s->enabled = 0;
+    } else {
+        ptimer_reload(s);
+    }
+
+    ptimer_trigger(s);
 }
 
 uint64_t ptimer_get_count(ptimer_state *s)
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH v12 9/9] arm_mptimer: Convert to use ptimer
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (7 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0 Dmitry Osipenko
@ 2016-01-30 16:43 ` Dmitry Osipenko
  2016-03-08 21:03   ` Peter Crosthwaite
  2016-03-16 14:36 ` [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Peter Maydell
  9 siblings, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2016-01-30 16:43 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

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
	- Correctly handles prescaler != 0 / counter = 0 / load = 0 cases
	- Code simplification and reduction

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

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/timer/arm_mptimer.c         | 133 +++++++++++++++++++++--------------------
 include/hw/timer/arm_mptimer.h |   5 +-
 2 files changed, 70 insertions(+), 68 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 5dfab66..f002458 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
@@ -42,55 +43,54 @@ static inline void timerblock_update_irq(TimerBlock *tb)
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
-static inline uint32_t timerblock_scale(TimerBlock *tb)
+static inline uint32_t timerblock_scale(uint32_t control)
 {
-    return (((tb->control >> 8) & 0xff) + 1) * 10;
+    return (((control >> 8) & 0xff) + 1) * 10;
 }
 
-static void timerblock_reload(TimerBlock *tb, int restart)
+static inline void timerblock_set_count(struct ptimer_state *timer,
+                                        uint32_t control, uint64_t *count)
 {
-    if (tb->count == 0) {
-        return;
+    /* PTimer would immediately trigger interrupt for periodic timer
+     * when counter set to 0, MPtimer under certain condition only.
+     */
+    if ((control & 3) == 3 && (control & 0xff00) == 0 && *count == 0) {
+        *count = ptimer_get_limit(timer);
     }
-    if (restart) {
-        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    ptimer_set_count(timer, *count);
+}
+
+static inline void timerblock_run(struct ptimer_state *timer,
+                                  uint32_t control, uint32_t load)
+{
+    if ((control & 1) && ((control & 0xff00) || load != 0)) {
+        ptimer_run(timer, !(control & 2));
     }
-    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);
+    /* Periodic timer with load = 0 and prescaler != 0 would re-trigger
+     * IRQ after one period, otherwise it either stops or wraps around.
+     */
+    if ((tb->control & 2) && (tb->control & 0xff00) &&
+            ptimer_get_limit(tb->timer) == 0) {
+        ptimer_run(tb->timer, 0);
+    }
 }
 
 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;
+        return ptimer_get_limit(tb->timer);
     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.  */
@@ -104,37 +104,45 @@ static void timerblock_write(void *opaque, hwaddr addr,
                              uint64_t value, unsigned size)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
-    int64_t old;
+    uint32_t control = tb->control;
     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 without doing the tick if
+         * prescaler = 0.
+         */
+        if ((control & 1) && (control & 0xff00) == 0 && value == 0) {
+            ptimer_stop(tb->timer);
         }
-        tb->count = value;
-        if (tb->control & 1) {
-            timerblock_reload(tb, 1);
+        ptimer_set_limit(tb->timer, value, 1);
+        timerblock_run(tb->timer, control, value);
+        break;
+    case 4: /* Counter.  */
+        /* Setting counter to 0 stops the one-shot timer, or periodic with
+         * load == 0, without doing the tick if prescaler = 0.
+         */
+        if ((control & 1) && (control & 0xff00) == 0 && value == 0 &&
+                (!(control & 2) || ptimer_get_limit(tb->timer) == 0)) {
+            ptimer_stop(tb->timer);
         }
+        timerblock_set_count(tb->timer, control, &value);
+        timerblock_run(tb->timer, control, value);
         break;
     case 8: /* Control.  */
-        old = tb->control;
-        tb->control = value;
+        if ((control & 1) && !(value & 1)) {
+            ptimer_stop(tb->timer);
+        }
+        if ((control & 0xff00) != (value & 0xff00)) {
+            ptimer_set_period(tb->timer, timerblock_scale(value));
+        }
         if (value & 1) {
-            if ((old & 1) && (tb->count != 0)) {
-                /* Do nothing if timer is ticking right now.  */
-                break;
+            uint64_t count = ptimer_get_count(tb->timer);
+            /* Re-load periodic timer counter if needed.  */
+            if ((value & 2) && count == 0) {
+                timerblock_set_count(tb->timer, value, &count);
             }
-            if (tb->control & 2) {
-                tb->count = tb->load;
-            }
-            timerblock_reload(tb, 1);
-        } else if (old & 1) {
-            /* Shutdown the timer.  */
-            timer_del(tb->timer);
+            timerblock_run(tb->timer, value, count);
         }
+        tb->control = value;
         break;
     case 12: /* Interrupt status.  */
         tb->status &= ~value;
@@ -184,13 +192,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);
+        ptimer_set_period(tb->timer, timerblock_scale(0));
     }
 }
 
@@ -236,7 +243,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);
@@ -246,26 +254,23 @@ 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..c46d8d2 100644
--- a/include/hw/timer/arm_mptimer.h
+++ b/include/hw/timer/arm_mptimer.h
@@ -27,12 +27,9 @@
 
 /* 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.7.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer Dmitry Osipenko
@ 2016-02-02 15:19   ` Dmitry Osipenko
  2016-02-02 15:24     ` Dmitry Osipenko
  2016-03-08  3:43     ` Peter Crosthwaite
  0 siblings, 2 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-02-02 15:19 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer
  2016-02-02 15:19   ` Dmitry Osipenko
@ 2016-02-02 15:24     ` Dmitry Osipenko
  2016-03-08  3:43     ` Peter Crosthwaite
  1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-02-02 15:24 UTC (permalink / raw)
  To: QEMU Developers, qemu-arm; +Cc: Peter Maydell, Peter Crosthwaite

02.02.2016 18:19, Dmitry Osipenko пишет:
> There is possibility to return timer counter = limit + 1, if the following
> ptimer calls execute in less than 1ns.

s/limit/delta/

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer
  2016-02-02 15:19   ` Dmitry Osipenko
  2016-02-02 15:24     ` Dmitry Osipenko
@ 2016-03-08  3:43     ` Peter Crosthwaite
  2016-03-08 19:11       ` Dmitry Osipenko
  1 sibling, 1 reply; 25+ messages in thread
From: Peter Crosthwaite @ 2016-03-08  3:43 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: Peter Maydell, qemu-arm, QEMU Developers

On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <digetx@gmail.com> 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 <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);
>

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer
  2016-03-08  3:43     ` Peter Crosthwaite
@ 2016-03-08 19:11       ` Dmitry Osipenko
  0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-03-08 19:11 UTC (permalink / raw)
  To: Peter Crosthwaite; +Cc: Peter Maydell, qemu-arm, QEMU Developers

08.03.2016 06:43, Peter Crosthwaite пишет:
> On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <digetx@gmail.com> 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 <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);
>>
>
> 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

Hi Peter,

The expiration check is needed because timer should stay with 0 for a one period 
before doing wrap around. Using DIV_ROUND_UP directly, timer would wrap around 
immediately once now > next (i.e. after 1 ns) regardless of the period value, 
and DIV_ROUND_UP can't be used because it would cause integer overflow of (n) + 
(d) due to the left shifting of the "rem" and "div".

#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))


No worries, I can wait as much as needed :)

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 9/9] arm_mptimer: Convert to use ptimer
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 9/9] arm_mptimer: Convert to use ptimer Dmitry Osipenko
@ 2016-03-08 21:03   ` Peter Crosthwaite
  0 siblings, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2016-03-08 21:03 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: Peter Maydell, qemu-arm, QEMU Developers

On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> 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
>         - Correctly handles prescaler != 0 / counter = 0 / load = 0 cases
>         - Code simplification and reduction
>
> Bump VMSD to version 3, since VMState is changed and is not compatible
> with the previous implementation.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Im a little rusty on some of the corner cases that have come out of
your test suite, but the general idea is solid, and it adds those
missing features that got the whole discussion started.

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  hw/timer/arm_mptimer.c         | 133 +++++++++++++++++++++--------------------
>  include/hw/timer/arm_mptimer.h |   5 +-
>  2 files changed, 70 insertions(+), 68 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 5dfab66..f002458 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
> @@ -42,55 +43,54 @@ static inline void timerblock_update_irq(TimerBlock *tb)
>  }
>
>  /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
> -static inline uint32_t timerblock_scale(TimerBlock *tb)
> +static inline uint32_t timerblock_scale(uint32_t control)
>  {
> -    return (((tb->control >> 8) & 0xff) + 1) * 10;
> +    return (((control >> 8) & 0xff) + 1) * 10;
>  }
>
> -static void timerblock_reload(TimerBlock *tb, int restart)
> +static inline void timerblock_set_count(struct ptimer_state *timer,
> +                                        uint32_t control, uint64_t *count)
>  {
> -    if (tb->count == 0) {
> -        return;
> +    /* PTimer would immediately trigger interrupt for periodic timer
> +     * when counter set to 0, MPtimer under certain condition only.
> +     */
> +    if ((control & 3) == 3 && (control & 0xff00) == 0 && *count == 0) {
> +        *count = ptimer_get_limit(timer);
>      }
> -    if (restart) {
> -        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    ptimer_set_count(timer, *count);
> +}
> +
> +static inline void timerblock_run(struct ptimer_state *timer,
> +                                  uint32_t control, uint32_t load)
> +{
> +    if ((control & 1) && ((control & 0xff00) || load != 0)) {
> +        ptimer_run(timer, !(control & 2));
>      }
> -    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);
> +    /* Periodic timer with load = 0 and prescaler != 0 would re-trigger
> +     * IRQ after one period, otherwise it either stops or wraps around.
> +     */
> +    if ((tb->control & 2) && (tb->control & 0xff00) &&
> +            ptimer_get_limit(tb->timer) == 0) {
> +        ptimer_run(tb->timer, 0);
> +    }
>  }
>
>  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;
> +        return ptimer_get_limit(tb->timer);
>      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.  */
> @@ -104,37 +104,45 @@ static void timerblock_write(void *opaque, hwaddr addr,
>                               uint64_t value, unsigned size)
>  {
>      TimerBlock *tb = (TimerBlock *)opaque;
> -    int64_t old;
> +    uint32_t control = tb->control;
>      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 without doing the tick if
> +         * prescaler = 0.
> +         */
> +        if ((control & 1) && (control & 0xff00) == 0 && value == 0) {
> +            ptimer_stop(tb->timer);
>          }
> -        tb->count = value;
> -        if (tb->control & 1) {
> -            timerblock_reload(tb, 1);
> +        ptimer_set_limit(tb->timer, value, 1);
> +        timerblock_run(tb->timer, control, value);
> +        break;
> +    case 4: /* Counter.  */
> +        /* Setting counter to 0 stops the one-shot timer, or periodic with
> +         * load == 0, without doing the tick if prescaler = 0.
> +         */
> +        if ((control & 1) && (control & 0xff00) == 0 && value == 0 &&
> +                (!(control & 2) || ptimer_get_limit(tb->timer) == 0)) {
> +            ptimer_stop(tb->timer);
>          }
> +        timerblock_set_count(tb->timer, control, &value);
> +        timerblock_run(tb->timer, control, value);
>          break;
>      case 8: /* Control.  */
> -        old = tb->control;
> -        tb->control = value;
> +        if ((control & 1) && !(value & 1)) {
> +            ptimer_stop(tb->timer);
> +        }
> +        if ((control & 0xff00) != (value & 0xff00)) {
> +            ptimer_set_period(tb->timer, timerblock_scale(value));
> +        }
>          if (value & 1) {
> -            if ((old & 1) && (tb->count != 0)) {
> -                /* Do nothing if timer is ticking right now.  */
> -                break;
> +            uint64_t count = ptimer_get_count(tb->timer);
> +            /* Re-load periodic timer counter if needed.  */
> +            if ((value & 2) && count == 0) {
> +                timerblock_set_count(tb->timer, value, &count);
>              }
> -            if (tb->control & 2) {
> -                tb->count = tb->load;
> -            }
> -            timerblock_reload(tb, 1);
> -        } else if (old & 1) {
> -            /* Shutdown the timer.  */
> -            timer_del(tb->timer);
> +            timerblock_run(tb->timer, value, count);
>          }
> +        tb->control = value;
>          break;
>      case 12: /* Interrupt status.  */
>          tb->status &= ~value;
> @@ -184,13 +192,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);
> +        ptimer_set_period(tb->timer, timerblock_scale(0));
>      }
>  }
>
> @@ -236,7 +243,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);
> @@ -246,26 +254,23 @@ 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..c46d8d2 100644
> --- a/include/hw/timer/arm_mptimer.h
> +++ b/include/hw/timer/arm_mptimer.h
> @@ -27,12 +27,9 @@
>
>  /* 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.7.0
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0 Dmitry Osipenko
@ 2016-03-08 21:08   ` Peter Crosthwaite
  2016-03-09 19:56     ` Dmitry Osipenko
  2016-03-16 18:34     ` Dmitry Osipenko
  0 siblings, 2 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2016-03-08 21:08 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: Peter Maydell, qemu-arm, QEMU Developers

On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> It might be necessary by some emulated HW to perform the tick after one
> period if delta = 0. Given that it is much less churny to implement immediate
> tick by the ptimer user itself, let's make ptimer do the delayed tick.
>

Isn't this related to previous patch? It is kind of a rounding problem
that will vary from timer to timer. Some timers may interpret the
"tick" as the wrap-around back to the load value (even if that is 0)
while others interpret the tick as the transition to zero (makes more
sense for a one-shot). It's hard to set a universal policy here.

But is this a non-issue when we consider that event latency (usually
interrupt latency) is undefined anyway?

Regards,
Peter

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/core/ptimer.c | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index b2044fb..bcd090c 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -36,19 +36,7 @@ static void ptimer_reload(ptimer_state *s)
>  {
>      uint32_t period_frac = s->period_frac;
>      uint64_t period = s->period;
> -
> -    if (s->delta == 0) {
> -        ptimer_trigger(s);
> -    }
> -
> -    if (s->delta == 0 && s->enabled == 1) {
> -        s->delta = s->limit;
> -    }
> -
> -    if (s->delta == 0) {
> -        ptimer_stop(s);
> -        return;
> -    }
> +    uint64_t delta = MAX(1, s->delta);
>
>      /*
>       * Artificially limit timeout rate to something
> @@ -59,15 +47,15 @@ static void ptimer_reload(ptimer_state *s)
>       * on the current generation of host machines.
>       */
>
> -    if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) {
> -        period = 10000 / s->delta;
> +    if (s->enabled == 1 && (delta * period < 10000) && !use_icount) {
> +        period = 10000 / delta;
>          period_frac = 0;
>      }
>
>      s->last_event = s->next_event;
> -    s->next_event = s->last_event + s->delta * period;
> +    s->next_event = s->last_event + delta * period;
>      if (period_frac) {
> -        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
> +        s->next_event += ((int64_t)period_frac * delta) >> 32;
>      }
>      timer_mod(s->timer, s->next_event);
>  }
> @@ -75,8 +63,16 @@ static void ptimer_reload(ptimer_state *s)
>  static void ptimer_tick(void *opaque)
>  {
>      ptimer_state *s = (ptimer_state *)opaque;
> -    s->delta = 0;
> -    ptimer_reload(s);
> +
> +    s->delta = (s->enabled == 1) ? s->limit : 0;
> +
> +    if (s->delta == 0) {
> +        s->enabled = 0;
> +    } else {
> +        ptimer_reload(s);
> +    }
> +
> +    ptimer_trigger(s);
>  }
>
>  uint64_t ptimer_get_count(ptimer_state *s)
> --
> 2.7.0
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0
  2016-03-08 21:08   ` Peter Crosthwaite
@ 2016-03-09 19:56     ` Dmitry Osipenko
  2016-03-16 18:34     ` Dmitry Osipenko
  1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-03-09 19:56 UTC (permalink / raw)
  To: Peter Crosthwaite; +Cc: Peter Maydell, qemu-arm, QEMU Developers

09.03.2016 00:08, Peter Crosthwaite пишет:
> On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> It might be necessary by some emulated HW to perform the tick after one
>> period if delta = 0. Given that it is much less churny to implement immediate
>> tick by the ptimer user itself, let's make ptimer do the delayed tick.
>>
>
> Isn't this related to previous patch? It is kind of a rounding problem
> that will vary from timer to timer. Some timers may interpret the
> "tick" as the wrap-around back to the load value (even if that is 0)
> while others interpret the tick as the transition to zero (makes more
> sense for a one-shot). It's hard to set a universal policy here.

If by previous patch you are meaning "Fix counter - 1 ...", then no, it's a 
distinct issue/feature. I endeavored to add some sort of "flags" to ptimer in 
order to support various policies, but quickly realized that it doesn't really 
worth an effort since there are no users of those policies (currently "delta = 
0" is a forbidden case) and ptimer VMSD version would be bumped.

It's not possible (doesn't make sense) for ptimer user to implement policy 
provided by this patch by itself. Other policies could be easily supported in 
future if desired (or implemented by ptimer user).

>
> But is this a non-issue when we consider that event latency (usually
> interrupt latency) is undefined anyway?
>

It's not an issue for a timer that has quite short period. Some of the 
arm_mptimer tests would fail (in icount mode) if that policy not implemented.

BTW, I think "deferred" might be better than "delayed", however not sure :)

> Regards,
> Peter

>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>   hw/core/ptimer.c | 34 +++++++++++++++-------------------
>>   1 file changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>> index b2044fb..bcd090c 100644
>> --- a/hw/core/ptimer.c
>> +++ b/hw/core/ptimer.c
>> @@ -36,19 +36,7 @@ static void ptimer_reload(ptimer_state *s)
>>   {
>>       uint32_t period_frac = s->period_frac;
>>       uint64_t period = s->period;
>> -
>> -    if (s->delta == 0) {
>> -        ptimer_trigger(s);
>> -    }
>> -
>> -    if (s->delta == 0 && s->enabled == 1) {
>> -        s->delta = s->limit;
>> -    }
>> -
>> -    if (s->delta == 0) {
>> -        ptimer_stop(s);
>> -        return;
>> -    }
>> +    uint64_t delta = MAX(1, s->delta);
>>
>>       /*
>>        * Artificially limit timeout rate to something
>> @@ -59,15 +47,15 @@ static void ptimer_reload(ptimer_state *s)
>>        * on the current generation of host machines.
>>        */
>>
>> -    if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) {
>> -        period = 10000 / s->delta;
>> +    if (s->enabled == 1 && (delta * period < 10000) && !use_icount) {
>> +        period = 10000 / delta;
>>           period_frac = 0;
>>       }
>>
>>       s->last_event = s->next_event;
>> -    s->next_event = s->last_event + s->delta * period;
>> +    s->next_event = s->last_event + delta * period;
>>       if (period_frac) {
>> -        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
>> +        s->next_event += ((int64_t)period_frac * delta) >> 32;
>>       }
>>       timer_mod(s->timer, s->next_event);
>>   }
>> @@ -75,8 +63,16 @@ static void ptimer_reload(ptimer_state *s)
>>   static void ptimer_tick(void *opaque)
>>   {
>>       ptimer_state *s = (ptimer_state *)opaque;
>> -    s->delta = 0;
>> -    ptimer_reload(s);
>> +
>> +    s->delta = (s->enabled == 1) ? s->limit : 0;
>> +
>> +    if (s->delta == 0) {
>> +        s->enabled = 0;
>> +    } else {
>> +        ptimer_reload(s);
>> +    }
>> +
>> +    ptimer_trigger(s);
>>   }
>>
>>   uint64_t ptimer_get_count(ptimer_state *s)
>> --
>> 2.7.0
>>


-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
                   ` (8 preceding siblings ...)
  2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 9/9] arm_mptimer: Convert to use ptimer Dmitry Osipenko
@ 2016-03-16 14:36 ` Peter Maydell
  2016-03-16 15:33   ` Dmitry Osipenko
  2016-03-28 13:22   ` Dmitry Osipenko
  9 siblings, 2 replies; 25+ messages in thread
From: Peter Maydell @ 2016-03-16 14:36 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

On 30 January 2016 at 16:43, Dmitry Osipenko <digetx@gmail.com> wrote:
> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:

So, where are we with this series? It looked from the mailing list
threads as if there were still a few things Peter C hadn't got
closure on, but we're rapidly running out of time before hard
freeze :-(  I'd really rather not have to push it out by yet
another release, especially since it got posted back in January..

thanks
-- PMM

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-03-16 14:36 ` [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Peter Maydell
@ 2016-03-16 15:33   ` Dmitry Osipenko
  2016-03-28 13:22   ` Dmitry Osipenko
  1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-03-16 15:33 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

16.03.2016 17:36, Peter Maydell пишет:
> On 30 January 2016 at 16:43, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
>
> So, where are we with this series? It looked from the mailing list
> threads as if there were still a few things Peter C hadn't got
> closure on, but we're rapidly running out of time before hard
> freeze :-(  I'd really rather not have to push it out by yet
> another release, especially since it got posted back in January..
>
> thanks
> -- PMM
>

Well, I think the series is in quite good shape. Peter C got busy with other 
things recently, so review process isn't going fast. Maybe you could give a 
quick glance at the last two remaining patches?

I'll re-test the series on the current master and send out final V13 then.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0
  2016-03-08 21:08   ` Peter Crosthwaite
  2016-03-09 19:56     ` Dmitry Osipenko
@ 2016-03-16 18:34     ` Dmitry Osipenko
  1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-03-16 18:34 UTC (permalink / raw)
  To: Peter Crosthwaite; +Cc: Peter Maydell, qemu-arm, QEMU Developers

09.03.2016 00:08, Peter Crosthwaite пишет:
> On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> It might be necessary by some emulated HW to perform the tick after one
>> period if delta = 0. Given that it is much less churny to implement immediate
>> tick by the ptimer user itself, let's make ptimer do the delayed tick.
>>
>
> Isn't this related to previous patch? It is kind of a rounding problem

Ah, you probably meant "Legalize running with delta = load = 0 and abort on 
period = 0" patch. Yes, they could be squashed.

I chose wrong policy at first time, further added MPtimer tests revealed it.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-03-16 14:36 ` [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Peter Maydell
  2016-03-16 15:33   ` Dmitry Osipenko
@ 2016-03-28 13:22   ` Dmitry Osipenko
  2016-03-29 19:00     ` Peter Maydell
  1 sibling, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2016-03-28 13:22 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

Hello Peter,

16.03.2016 17:36, Peter Maydell пишет:
> On 30 January 2016 at 16:43, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
>
> So, where are we with this series? It looked from the mailing list
> threads as if there were still a few things Peter C hadn't got
> closure on, but we're rapidly running out of time before hard
> freeze :-(  I'd really rather not have to push it out by yet
> another release, especially since it got posted back in January..
>
> thanks
> -- PMM
>

It still pending for the review and the due date is tomorrow, maybe you would 
want to pick some of the PTimer patches for 2.6.

PTimer fixes ready for submission:
[PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit value
[PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already expired
[PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change

The rest of the patches are either prerequisites for the ARM MPtimer conversion 
or pending for the review, we will work them out once Peter Crosthwaite would 
get some spare time.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-03-28 13:22   ` Dmitry Osipenko
@ 2016-03-29 19:00     ` Peter Maydell
  2016-03-29 20:17       ` Dmitry Osipenko
  0 siblings, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2016-03-29 19:00 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

On 28 March 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote:
> Hello Peter,
>
> 16.03.2016 17:36, Peter Maydell пишет:
>>
>> On 30 January 2016 at 16:43, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>>> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
>>
>>
>> So, where are we with this series? It looked from the mailing list
>> threads as if there were still a few things Peter C hadn't got
>> closure on, but we're rapidly running out of time before hard
>> freeze :-(  I'd really rather not have to push it out by yet
>> another release, especially since it got posted back in January..

> It still pending for the review and the due date is tomorrow, maybe you
> would want to pick some of the PTimer patches for 2.6.
>
> PTimer fixes ready for submission:
> [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit
> value
> [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already
> expired
> [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change

I'm afraid at this point I really don't want to apply changes
to code like ptimer that affects multiple platforms; this will
have to go to 2.7 :-(

thanks
-- PMM

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-03-29 19:00     ` Peter Maydell
@ 2016-03-29 20:17       ` Dmitry Osipenko
  2016-05-24 16:20         ` Peter Maydell
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2016-03-29 20:17 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

29.03.2016 22:00, Peter Maydell пишет:
> On 28 March 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Hello Peter,
>>
>> 16.03.2016 17:36, Peter Maydell пишет:
>>>
>>> On 30 January 2016 at 16:43, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>
>>>> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
>>>
>>>
>>> So, where are we with this series? It looked from the mailing list
>>> threads as if there were still a few things Peter C hadn't got
>>> closure on, but we're rapidly running out of time before hard
>>> freeze :-(  I'd really rather not have to push it out by yet
>>> another release, especially since it got posted back in January..
>
>> It still pending for the review and the due date is tomorrow, maybe you
>> would want to pick some of the PTimer patches for 2.6.
>>
>> PTimer fixes ready for submission:
>> [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit
>> value
>> [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already
>> expired
>> [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change
>
> I'm afraid at this point I really don't want to apply changes
> to code like ptimer that affects multiple platforms; this will
> have to go to 2.7 :-(

Okay, this is fine too :)

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-03-29 20:17       ` Dmitry Osipenko
@ 2016-05-24 16:20         ` Peter Maydell
  2016-05-24 17:15           ` Dmitry Osipenko
  0 siblings, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2016-05-24 16:20 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

On 29 March 2016 at 21:17, Dmitry Osipenko <digetx@gmail.com> wrote:
> 29.03.2016 22:00, Peter Maydell пишет:
>> On 28 March 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote:
>>> PTimer fixes ready for submission:
>>> [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit
>>> value
>>> [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already
>>> expired
>>> [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change
>>
>>
>> I'm afraid at this point I really don't want to apply changes
>> to code like ptimer that affects multiple platforms; this will
>> have to go to 2.7 :-(

> Okay, this is fine too :)

Can I ask you to rebase this series on master and resend it,
please (or at least whichever parts of it you think are ready
to go in) ? I'm sorry it's been kind of left in limbo for so
long but I promise to look at it when you resend it :-(

thanks
-- PMM

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion
  2016-05-24 16:20         ` Peter Maydell
@ 2016-05-24 17:15           ` Dmitry Osipenko
  0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2016-05-24 17:15 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers

On 24.05.2016 19:20, Peter Maydell wrote:
> On 29 March 2016 at 21:17, Dmitry Osipenko <digetx@gmail.com> wrote:
>> 29.03.2016 22:00, Peter Maydell пишет:
>>> On 28 March 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>> PTimer fixes ready for submission:
>>>> [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit
>>>> value
>>>> [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already
>>>> expired
>>>> [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change
>>>
>>>
>>> I'm afraid at this point I really don't want to apply changes
>>> to code like ptimer that affects multiple platforms; this will
>>> have to go to 2.7 :-(
>
>> Okay, this is fine too :)
>
> Can I ask you to rebase this series on master and resend it,
> please (or at least whichever parts of it you think are ready
> to go in) ? I'm sorry it's been kind of left in limbo for so
> long but I promise to look at it when you resend it :-(
>
> thanks
> -- PMM
>

Sure, I already was going to do it this or next week :) Thanks for remembering 
about it!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2016-05-24 17:15 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-30 16:43 [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 1/9] hw/ptimer: Fix issues caused by the adjusted timer limit value Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 2/9] hw/ptimer: Perform counter wrap around if timer already expired Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 3/9] hw/ptimer: Update .delta on period/freq change Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 4/9] hw/ptimer: Support "on the fly" timer mode switch Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 5/9] hw/ptimer: Introduce ptimer_get_limit Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 6/9] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0 Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer Dmitry Osipenko
2016-02-02 15:19   ` Dmitry Osipenko
2016-02-02 15:24     ` Dmitry Osipenko
2016-03-08  3:43     ` Peter Crosthwaite
2016-03-08 19:11       ` Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0 Dmitry Osipenko
2016-03-08 21:08   ` Peter Crosthwaite
2016-03-09 19:56     ` Dmitry Osipenko
2016-03-16 18:34     ` Dmitry Osipenko
2016-01-30 16:43 ` [Qemu-devel] [PATCH v12 9/9] arm_mptimer: Convert to use ptimer Dmitry Osipenko
2016-03-08 21:03   ` Peter Crosthwaite
2016-03-16 14:36 ` [Qemu-devel] [PATCH v12 0/9] PTimer fixes/features and ARM MPTimer conversion Peter Maydell
2016-03-16 15:33   ` Dmitry Osipenko
2016-03-28 13:22   ` Dmitry Osipenko
2016-03-29 19:00     ` Peter Maydell
2016-03-29 20:17       ` Dmitry Osipenko
2016-05-24 16:20         ` Peter Maydell
2016-05-24 17:15           ` Dmitry Osipenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.