PTimer fixes/features and ARM MPTimer conversion

PATCH Superseded Dmitry Osipenko 2 participants 21 messages Jul 21, 2016 – Sep 6, 2016

Subsystems (4)

Arm i.MX and Layerscape Arm Samsung Exynos Arm/Zynq architecture Arm/Allwinner sunXi SoC support

lore

Share & tools

lore

Dmitry Osipenko

author

2016-07-21 14:32 UTC #1 lore raw

PATCH v15 1/15 hw/ptimer: Change ptimer_get_count to return "1" for the expired timer

In reply to Dmitry Osipenko

Software shouldn't see counter eq to "0" before timer IRQ trigger happened, as it could be wrong behaviour for some of the emulated platforms. In order to maintain deterministic behaviour, change returned counter value to "1" for the expired timer.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/core/ptimer.c +1 -1

index 30829ee..9dc2bb0 100644
@@ -96,7 +96,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
         if (expired) {
             /* Prevent timer underflowing if it should already have
                triggered.  */
-            counter = 0;
+            counter = 1;
         } else {
             uint64_t rem;
             uint64_t div;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #2 lore raw

PATCH v15 2/15 hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer

In reply to Dmitry Osipenko

Due to rounding down performed by ptimer_get_count, it returns counter - 1 for the enabled 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. Fix this by adding 1 to the counter value.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

hw/core/ptimer.c +4 -1

index 9dc2bb0..461b91c 100644
@@ -89,6 +89,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
     if (s->enabled) {
         int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         int64_t next = s->next_event;
+        int64_t last = s->last_event;
         bool expired = (now - next >= 0);
         bool oneshot = (s->enabled == 2);

@@ -97,6 +98,8 @@ uint64_t ptimer_get_count(ptimer_state *s)
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 1;
+        } else if (now == last) {
+            counter = s->delta;
         } else {
             uint64_t rem;
             uint64_t div;

@@ -139,7 +142,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
                 if ((uint32_t)(period_frac << shift))
                     div += 1;
             }
-            counter = rem / div;
+            counter = rem / div + 1;
         }
     } else {
         counter = s->delta;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #3 lore raw

PATCH v15 3/15 hw/ptimer: Actually stop timer in case of error

In reply to Dmitry Osipenko

Running with counter / period = 0 is treated as a error case, printing error message claiming that timer has been disabled. However, timer is only marked as disabled, keeping to tick till expired and triggering after being claimed as disabled. Stop the QEMU timer to avoid confusion.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 1 +
 1 file changed, 1 insertion(+)

hw/core/ptimer.c +1 -0

index 461b91c..1dd2d32 100644
@@ -44,6 +44,7 @@ static void ptimer_reload(ptimer_state *s)
     }
     if (s->delta == 0 || s->period == 0) {
         fprintf(stderr, "Timer with period zero, disabling\n");
+        timer_del(s->timer);
         s->enabled = 0;
         return;
     }

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #6 lore raw

PATCH v15 6/15 hw/ptimer: Add "wraparound after one period" policy

In reply to Dmitry Osipenko

Currently, periodic counter wraps around immediately once counter reaches "0", this is wrong behaviour for some of the timers, resulting in one period being lost. Add new ptimer policy that provides correct behaviour for such timers, so that counter stays with "0" for a one period before wrapping around.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c    | 43 ++++++++++++++++++++++++++++---------------
 include/hw/ptimer.h |  3 +++
 2 files changed, 31 insertions(+), 15 deletions(-)

hw/core/ptimer.c +28 ~~-15~~

index 87af293..c697c08 100644
@@ -34,22 +34,27 @@ static void ptimer_trigger(ptimer_state *s)
     }
 }
 
-static void ptimer_reload(ptimer_state *s)
+static void ptimer_reload(ptimer_state *s, int delta_adjust)
 {
     uint32_t period_frac = s->period_frac;
     uint64_t period = s->period;
+    uint64_t delta = s->delta;
 
-    if (s->delta == 0) {
+    if (delta == 0) {
         ptimer_trigger(s);
-        s->delta = s->limit;
+        delta = s->delta = s->limit;
     }
-    if (s->delta == 0 || s->period == 0) {
+    if (delta == 0 || s->period == 0) {
         fprintf(stderr, "Timer with period zero, disabling\n");
         timer_del(s->timer);
         s->enabled = 0;
         return;
     }
 
+    if (s->policy_mask & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) {
+        delta += delta_adjust;
+    }
+
     /*
      * Artificially limit timeout rate to something
      * achievable under QEMU.  Otherwise, QEMU spends all

@@ -59,15 +64,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);
 }

@@ -80,7 +85,7 @@ static void ptimer_tick(void *opaque)
     if (s->enabled == 2) {
         s->enabled = 0;
     } else {
-        ptimer_reload(s);
+        ptimer_reload(s, 1);
     }
 }

@@ -88,7 +93,7 @@ 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;
         int64_t last = s->last_event;

@@ -145,6 +150,14 @@ uint64_t ptimer_get_count(ptimer_state *s)
                     div += 1;
             }
             counter = rem / div + 1;
+
+            if (!oneshot && s->delta == s->limit) {
+                /* Before wrapping around, timer should stay with counter = 0
+                   for a one period.  The delta has been adjusted by +1 for
+                   the wrapped around counter, so taking a modulo of limit + 1
+                   gives that period.  */
+                counter %= s->limit + 1;
+            }
         }
     } else {
         counter = s->delta;

@@ -157,7 +170,7 @@ void ptimer_set_count(ptimer_state *s, uint64_t count)
     s->delta = count;
     if (s->enabled) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s);
+        ptimer_reload(s, 0);
     }
 }

@@ -172,7 +185,7 @@ void ptimer_run(ptimer_state *s, int oneshot)
     s->enabled = oneshot ? 2 : 1;
     if (was_disabled) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s);
+        ptimer_reload(s, 0);
     }
 }

@@ -196,7 +209,7 @@ void ptimer_set_period(ptimer_state *s, int64_t period)
     s->period_frac = 0;
     if (s->enabled) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s);
+        ptimer_reload(s, 0);
     }
 }

@@ -208,7 +221,7 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
     s->period_frac = (1000000000ll << 32) / freq;
     if (s->enabled) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s);
+        ptimer_reload(s, 0);
     }
 }

@@ -221,7 +234,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
         s->delta = limit;
     if (s->enabled && reload) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s);
+        ptimer_reload(s, 0);
     }
 }

include/hw/ptimer.h +3 -0

index 80564ea..e8de48d 100644
@@ -13,6 +13,9 @@
 #include "migration/vmstate.h"
 
 #define PTIMER_POLICY_DEFAULT               0
+/* Periodic timer counter stays with "0" for a one period before wrapping
+ * around.  */
+#define PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD (1 << 0)
 
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #7 lore raw

PATCH v15 7/15 tests: ptimer: Add tests for "wraparound after one period" policy

In reply to Dmitry Osipenko

PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD changes ptimer behaviour in a such way, that it would wrap around after one period instead of doing it immediately.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 tests/ptimer-test.c | 45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

tests/ptimer-test.c +31 ~~-14~~

index 5c87828..981414d 100644
@@ -188,6 +188,7 @@ static void check_periodic(gconstpointer arg)
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
+    bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
 
     triggered = false;

@@ -197,14 +198,14 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 10 + 100000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 10);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 0 : 10);
     g_assert_true(triggered);
 
     triggered = false;
 
     qemu_clock_step(2000000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 9);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 10 : 9);
     g_assert_false(triggered);
 
     ptimer_set_count(ptimer, 20);

@@ -216,7 +217,7 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 10);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 9);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 10 : 9);
     g_assert_true(triggered);
 
     ptimer_stop(ptimer);

@@ -224,7 +225,7 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 9);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 10 : 9);
     g_assert_false(triggered);
 
     ptimer_set_count(ptimer, 3);

@@ -232,14 +233,14 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 3 + 100000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 10);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 0 : 10);
     g_assert_true(triggered);
 
     triggered = false;
 
     qemu_clock_step(2000000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 9);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 10 : 9);
     g_assert_false(triggered);
 
     ptimer_set_count(ptimer, 0);

@@ -250,7 +251,7 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 12 + 100000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 8);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 9 : 8);
     g_assert_true(triggered);
 
     ptimer_stop(ptimer);

@@ -259,7 +260,7 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 12 + 100000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 8);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 9 : 8);
     g_assert_false(triggered);
 }

@@ -268,6 +269,7 @@ static void check_on_the_fly_mode_change(gconstpointer arg)
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
+    bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
 
     triggered = false;

@@ -284,7 +286,7 @@ static void check_on_the_fly_mode_change(gconstpointer arg)
 
     qemu_clock_step(2000000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 10);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 0 : 10);
     g_assert_true(triggered);
 
     triggered = false;

@@ -293,10 +295,10 @@ static void check_on_the_fly_mode_change(gconstpointer arg)
 
     ptimer_run(ptimer, 1);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 1);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 2 : 1);
     g_assert_false(triggered);
 
-    qemu_clock_step(2000000);
+    qemu_clock_step(2000000 * 3);
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
     g_assert_true(triggered);

@@ -386,6 +388,7 @@ static void check_run_with_delta_0(gconstpointer arg)
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
+    bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
 
     triggered = false;

@@ -428,7 +431,7 @@ static void check_run_with_delta_0(gconstpointer arg)
 
     qemu_clock_step(2000000 * 98);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 0 : 99);
     g_assert_true(triggered);
 
     ptimer_stop(ptimer);

@@ -437,7 +440,7 @@ static void check_run_with_delta_0(gconstpointer arg)
 static void add_ptimer_tests(uint8_t policy)
 {
     uint8_t *ppolicy = g_malloc(1);
-    char *policy_name = g_malloc(64);
+    char *policy_name = g_malloc0(256);
 
     *ppolicy = policy;

@@ -445,6 +448,10 @@ static void add_ptimer_tests(uint8_t policy)
         g_sprintf(policy_name, "default");
     }
 
+    if (policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) {
+        g_strlcat(policy_name, "wrap_after_one_period,", 256);
+    }
+
     qtest_add_data_func(
         g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
         ppolicy, check_set_count);

@@ -482,6 +489,16 @@ static void add_ptimer_tests(uint8_t policy)
         ppolicy, check_run_with_delta_0);
 }
 
+static void add_all_ptimer_policies_comb_tests(void)
+{
+    int last_policy = PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD;
+    int policy = PTIMER_POLICY_DEFAULT;
+
+    for (; policy < (last_policy << 1); policy++) {
+        add_ptimer_tests(policy);
+    }
+}
+
 int main(int argc, char **argv)
 {
     int i;

@@ -492,7 +509,7 @@ int main(int argc, char **argv)
         main_loop_tlg.tl[i] = g_new0(QEMUTimerList, 1);
     }
 
-    add_ptimer_tests(PTIMER_POLICY_DEFAULT);
+    add_all_ptimer_policies_comb_tests();
 
     return g_test_run();
 }

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #8 lore raw

PATCH v15 8/15 hw/ptimer: Add "continuous trigger" policy

In reply to Dmitry Osipenko

Currently, periodic timer that has load = delta = 0 performs trigger on timer reload and stops, printing a "period zero" error message. Introduce new policy that makes periodic timer to continuously trigger with a period interval in case of load = 0.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c    | 16 +++++++++++++++-
 include/hw/ptimer.h |  3 +++
 2 files changed, 18 insertions(+), 1 deletion(-)

hw/core/ptimer.c +15 -1

index c697c08..8578861 100644
@@ -44,7 +44,8 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
         ptimer_trigger(s);
         delta = s->delta = s->limit;
     }
-    if (delta == 0 || s->period == 0) {
+
+    if (s->period == 0) {
         fprintf(stderr, "Timer with period zero, disabling\n");
         timer_del(s->timer);
         s->enabled = 0;

@@ -55,6 +56,19 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
         delta += delta_adjust;
     }
 
+    if (delta == 0 && (s->policy_mask & PTIMER_POLICY_CONTINUOUS_TRIGGER)) {
+        if (s->enabled == 1) {
+            delta = 1;
+        }
+    }
+
+    if (delta == 0) {
+        fprintf(stderr, "Timer with delta zero, disabling\n");
+        timer_del(s->timer);
+        s->enabled = 0;
+        return;
+    }
+
     /*
      * Artificially limit timeout rate to something
      * achievable under QEMU.  Otherwise, QEMU spends all

include/hw/ptimer.h +3 -0

index e8de48d..c97f02e 100644
@@ -16,6 +16,9 @@
 /* Periodic timer counter stays with "0" for a one period before wrapping
  * around.  */
 #define PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD (1 << 0)
+/* Periodic timer that has load = 0 would continuously re-trigger every
+ * period.  */
+#define PTIMER_POLICY_CONTINUOUS_TRIGGER    (1 << 1)
 
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #5 lore raw

PATCH v15 5/15 tests: Add ptimer tests

In reply to Dmitry Osipenko

Ptimer is a generic countdown timer helper that is used by many timer device models as well as by the QEMU core. Add QTests for the ptimer.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 stubs/vmstate.c           |   5 +
 tests/Makefile.include    |   2 +
 tests/ptimer-test-stubs.c | 107 ++++++++++
 tests/ptimer-test.c       | 498 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/ptimer-test.h       |  20 ++
 5 files changed, 632 insertions(+)
 create mode 100644 tests/ptimer-test-stubs.c
 create mode 100644 tests/ptimer-test.c
 create mode 100644 tests/ptimer-test.h

stubs/vmstate.c +5 -0

index 6590627..94b831e 100644
@@ -3,6 +3,11 @@
 #include "migration/vmstate.h"
 
 const VMStateDescription vmstate_dummy = {};
+const VMStateInfo vmstate_info_uint8;
+const VMStateInfo vmstate_info_uint32;
+const VMStateInfo vmstate_info_uint64;
+const VMStateInfo vmstate_info_int64;
+const VMStateInfo vmstate_info_timer;
 
 int vmstate_register_with_alias_id(DeviceState *dev,
                                    int instance_id,

tests/Makefile.include +2 -0

index 2010b11..17056e9 100644
@@ -270,6 +270,7 @@ check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
+check-qtest-generic-y += tests/ptimer-test$(EXESUF)
 
 qapi-schema += alternate-any.json
 qapi-schema += alternate-array.json

@@ -620,6 +621,7 @@ tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
 tests/test-filter-redirector$(EXESUF): tests/test-filter-redirector.o $(qtest-obj-y)
 tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y)
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o
+tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o hw/core/ptimer.o
 
 ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil

tests/ptimer-test-stubs.c new file +107 -0

107 lines not on this page · raw message

tests/ptimer-test.c new file +498 -0

498 lines not on this page · raw message

tests/ptimer-test.h new file +20 -0

new file mode 100644
index 0000000..3814951
@@ -0,0 +1,20 @@
+/*
+ * QTest testcase for the ptimer
+ *
+ * Author: Dmitry Osipenko <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef PTIMER_TEST_H
+#define PTIMER_TEST_H
+
+extern int64_t ptimer_test_time_ns;
+
+struct QEMUTimerList {
+    QEMUTimer active_timers;
+};
+
+#endif

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #9 lore raw

PATCH v15 9/15 tests: ptimer: Add tests for "continuous trigger" policy

In reply to Dmitry Osipenko

PTIMER_POLICY_CONTINUOUS_TRIGGER makes periodic ptimer to re-trigger every period in case of load = delta = 0.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 tests/ptimer-test.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

tests/ptimer-test.c +65 -1

index 981414d..cdf8d28 100644
@@ -437,6 +437,58 @@ static void check_run_with_delta_0(gconstpointer arg)
     ptimer_stop(ptimer);
 }
 
+static void check_periodic_with_load_0(gconstpointer arg)
+{
+    const uint8_t *policy = arg;
+    QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
+    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    bool continuous_trigger = (*policy & PTIMER_POLICY_CONTINUOUS_TRIGGER);
+
+    triggered = false;
+
+    ptimer_set_period(ptimer, 2000000);
+    ptimer_run(ptimer, 0);
+
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+    g_assert_true(triggered);
+
+    triggered = false;
+
+    qemu_clock_step(2000000 + 100000);
+
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+
+    if (continuous_trigger) {
+        g_assert_true(triggered);
+    } else {
+        g_assert_false(triggered);
+    }
+
+    ptimer_stop(ptimer);
+}
+
+static void check_oneshot_with_load_0(gconstpointer arg)
+{
+    const uint8_t *policy = arg;
+    QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
+    ptimer_state *ptimer = ptimer_init(bh, *policy);
+
+    triggered = false;
+
+    ptimer_set_period(ptimer, 2000000);
+    ptimer_run(ptimer, 1);
+
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+    g_assert_true(triggered);
+
+    triggered = false;
+
+    qemu_clock_step(2000000 + 100000);
+
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+    g_assert_false(triggered);
+}
+
 static void add_ptimer_tests(uint8_t policy)
 {
     uint8_t *ppolicy = g_malloc(1);

@@ -452,6 +504,10 @@ static void add_ptimer_tests(uint8_t policy)
         g_strlcat(policy_name, "wrap_after_one_period,", 256);
     }
 
+    if (policy & PTIMER_POLICY_CONTINUOUS_TRIGGER) {
+        g_strlcat(policy_name, "continuous_trigger,", 256);
+    }
+
     qtest_add_data_func(
         g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
         ppolicy, check_set_count);

@@ -487,11 +543,19 @@ static void add_ptimer_tests(uint8_t policy)
     qtest_add_data_func(
         g_strdup_printf("/ptimer/run_with_delta_0 policy=%s", policy_name),
         ppolicy, check_run_with_delta_0);
+
+    qtest_add_data_func(
+        g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s", policy_name),
+        ppolicy, check_periodic_with_load_0);
+
+    qtest_add_data_func(
+        g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s", policy_name),
+        ppolicy, check_oneshot_with_load_0);
 }
 
 static void add_all_ptimer_policies_comb_tests(void)
 {
-    int last_policy = PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD;
+    int last_policy = PTIMER_POLICY_CONTINUOUS_TRIGGER;
     int policy = PTIMER_POLICY_DEFAULT;
 
     for (; policy < (last_policy << 1); policy++) {

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #10 lore raw

PATCH v15 10/15 hw/ptimer: Add "no immediate trigger" policy

In reply to Dmitry Osipenko

Performing trigger on setting (or starting to run with) counter = 0 could be a wrong behaviour for some of the timers, provide "no immediate trigger" policy to maintain correct behaviour for such timers.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c    | 9 ++++++++-
 include/hw/ptimer.h | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

hw/core/ptimer.c +8 -1

index 8578861..d37e8ab 100644
@@ -40,8 +40,11 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
     uint64_t period = s->period;
     uint64_t delta = s->delta;
 
-    if (delta == 0) {
+    if (delta == 0 && !(s->policy_mask & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER)) {
         ptimer_trigger(s);
+    }
+
+    if (delta == 0) {
         delta = s->delta = s->limit;
     }

@@ -62,6 +65,10 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
         }
     }
 
+    if (delta == 0 && (s->policy_mask & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER)) {
+        delta = 1;
+    }
+
     if (delta == 0) {
         fprintf(stderr, "Timer with delta zero, disabling\n");
         timer_del(s->timer);

include/hw/ptimer.h +3 -0

index c97f02e..ec76183 100644
@@ -19,6 +19,9 @@
 /* Periodic timer that has load = 0 would continuously re-trigger every
  * period.  */
 #define PTIMER_POLICY_CONTINUOUS_TRIGGER    (1 << 1)
+/* Starting to run with/setting counter = 0 won't perform immediate
+ * trigger.  */
+#define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER  (1 << 2)
 
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #0 lore raw

v15 cover PTimer fixes/features and ARM MPTimer conversion

Hello,

Currently, QEMU ARM MPTimer device model provides only a certain subset of the emulation behavior. This patch series is supposed to add missing parts by converting the MPTimer to use generic ptimer helper. It fixes some important ptimer bugs and provides new features that are required for the ARM MPTimer.

Changelog:
V15: As per Peter's Maydell request: split ptimer policy patches,
so that first "policy" patch only sets default policy to all of the timers without introducing new behaviour policies.

The "deferred trigger" ptimer policy from V14 is converted to "continuous trigger" as it is more intuitive.

New ptimer policies added.

New ptimer patch "Actually stop timer in case of error".

Fixed ARM MPTimer periodic/oneshot "on the fly" mode switch for the case when load = 0 and new mode has prescaler = 0.

Added QTests for each of the ptimer policies.

Added QTests for the ARM MPTimer.

V14: Set the ptimer policy in the ptimer_init() instead of adding
ptimer_set_policy(), keeping ptimer VMState unchanged and dropped hw_error() hardening asserts as per Peter's Maydell V13 review comments, addressed the rest of the review comments.

Dmitry Osipenko (15):
hw/ptimer: Change ptimer_get_count to return "1" for the expired timer hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active
timer
hw/ptimer: Actually stop timer in case of error
hw/ptimer: Introduce timer policy feature
tests: Add ptimer tests
hw/ptimer: Add "wraparound after one period" policy
tests: ptimer: Add tests for "wraparound after one period" policy hw/ptimer: Add "continuous trigger" policy
tests: ptimer: Add tests for "continuous trigger" policy hw/ptimer: Add "no immediate trigger" policy
tests: ptimer: Add tests for "no immediate trigger" policy hw/ptimer: Add "no immediate reload" policy
tests: ptimer: Add tests for "no immediate reload" policy
arm_mptimer: Convert to use ptimer
tests: Add tests for the ARM MPTimer

 hw/arm/musicpal.c              |    2 +-
 hw/core/ptimer.c               |   80 ++-
 hw/dma/xilinx_axidma.c         |    2 +-
 hw/m68k/mcf5206.c              |    2 +-
 hw/m68k/mcf5208.c              |    2 +-
 hw/net/fsl_etsec/etsec.c       |    2 +-
 hw/net/lan9118.c               |    2 +-
 hw/timer/allwinner-a10-pit.c   |    2 +-
 hw/timer/arm_mptimer.c         |  148 +++---
 hw/timer/arm_timer.c           |    2 +-
 hw/timer/digic-timer.c         |    2 +-
 hw/timer/etraxfs_timer.c       |    6 +-
 hw/timer/exynos4210_mct.c      |    7 +-
 hw/timer/exynos4210_pwm.c      |    2 +-
 hw/timer/exynos4210_rtc.c      |    4 +-
 hw/timer/grlib_gptimer.c       |    2 +-
 hw/timer/imx_epit.c            |    4 +-
 hw/timer/imx_gpt.c             |    2 +-
 hw/timer/lm32_timer.c          |    2 +-
 hw/timer/milkymist-sysctl.c    |    4 +-
 hw/timer/puv3_ost.c            |    2 +-
 hw/timer/sh_timer.c            |    2 +-
 hw/timer/slavio_timer.c        |    2 +-
 hw/timer/xilinx_timer.c        |    2 +-
 include/hw/ptimer.h            |   15 +-
 include/hw/timer/arm_mptimer.h |    5 +-
 stubs/vmstate.c                |    5 +
 tests/Makefile.include         |    4 +
 tests/ptimer-test-stubs.c      |  107 ++++
 tests/ptimer-test.c            |  659 ++++++++++++++++++++++++
 tests/ptimer-test.h            |   20 +
 tests/test-arm-mptimer.c       | 1079 ++++++++++++++++++++++++++++++++++++++++
 32 files changed, 2061 insertions(+), 120 deletions(-)
 create mode 100644 tests/ptimer-test-stubs.c
 create mode 100644 tests/ptimer-test.c
 create mode 100644 tests/ptimer-test.h
 create mode 100644 tests/test-arm-mptimer.c

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #4 lore raw

PATCH v15 4/15 hw/ptimer: Introduce timer policy feature

Some of the timer devices may have behaviour different from what ptimer provides. Introduce ptimer policy feature that allows ptimer users to change default and wrong timer behaviour, for example to continuously trigger periodic timer when load value is equal to "0".

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/arm/musicpal.c            | 2 +-
 hw/core/ptimer.c             | 4 +++-
 hw/dma/xilinx_axidma.c       | 2 +-
 hw/m68k/mcf5206.c            | 2 +-
 hw/m68k/mcf5208.c            | 2 +-
 hw/net/fsl_etsec/etsec.c     | 2 +-
 hw/net/lan9118.c             | 2 +-
 hw/timer/allwinner-a10-pit.c | 2 +-
 hw/timer/arm_timer.c         | 2 +-
 hw/timer/digic-timer.c       | 2 +-
 hw/timer/etraxfs_timer.c     | 6 +++---
 hw/timer/exynos4210_mct.c    | 7 ++++---
 hw/timer/exynos4210_pwm.c    | 2 +-
 hw/timer/exynos4210_rtc.c    | 4 ++--
 hw/timer/grlib_gptimer.c     | 2 +-
 hw/timer/imx_epit.c          | 4 ++--
 hw/timer/imx_gpt.c           | 2 +-
 hw/timer/lm32_timer.c        | 2 +-
 hw/timer/milkymist-sysctl.c  | 4 ++--
 hw/timer/puv3_ost.c          | 2 +-
 hw/timer/sh_timer.c          | 2 +-
 hw/timer/slavio_timer.c      | 2 +-
 hw/timer/xilinx_timer.c      | 2 +-
 include/hw/ptimer.h          | 4 +++-
 24 files changed, 36 insertions(+), 31 deletions(-)

hw/arm/musicpal.c +1 -1

index 7a4cc07..087696a 100644
@@ -837,7 +837,7 @@ static void mv88w8618_timer_init(SysBusDevice *dev, mv88w8618_timer_state *s,
     s->freq = freq;
 
     bh = qemu_bh_new(mv88w8618_timer_tick, s);
-    s->ptimer = ptimer_init(bh);
+    s->ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static uint64_t mv88w8618_pit_read(void *opaque, hwaddr offset,

hw/core/ptimer.c +3 -1

index 1dd2d32..87af293 100644
@@ -21,6 +21,7 @@ struct ptimer_state
     int64_t period;
     int64_t last_event;
     int64_t next_event;
+    uint8_t policy_mask;
     QEMUBH *bh;
     QEMUTimer *timer;
 };

@@ -246,12 +247,13 @@ const VMStateDescription vmstate_ptimer = {
     }
 };
 
-ptimer_state *ptimer_init(QEMUBH *bh)
+ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
 {
     ptimer_state *s;
 
     s = (ptimer_state *)g_malloc0(sizeof(ptimer_state));
     s->bh = bh;
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ptimer_tick, s);
+    s->policy_mask = policy_mask;
     return s;
 }

hw/dma/xilinx_axidma.c +1 -1

index a4753e5..b135a5f 100644
@@ -548,7 +548,7 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
 
         st->nr = i;
         st->bh = qemu_bh_new(timer_hit, st);
-        st->ptimer = ptimer_init(st->bh);
+        st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(st->ptimer, s->freqhz);
     }
     return;

hw/m68k/mcf5206.c +1 -1

index e14896e..b81901f 100644
@@ -139,7 +139,7 @@ static m5206_timer_state *m5206_timer_init(qemu_irq irq)
 
     s = (m5206_timer_state *)g_malloc0(sizeof(m5206_timer_state));
     bh = qemu_bh_new(m5206_timer_trigger, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
     s->irq = irq;
     m5206_timer_reset(s);
     return s;

hw/m68k/mcf5208.c +1 -1

index 2415557..9240ebf 100644
@@ -183,7 +183,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
     for (i = 0; i < 2; i++) {
         s = (m5208_timer_state *)g_malloc0(sizeof(m5208_timer_state));
         bh = qemu_bh_new(m5208_timer_trigger, s);
-        s->timer = ptimer_init(bh);
+        s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
         memory_region_init_io(&s->iomem, NULL, &m5208_timer_ops, s,
                               "m5208-timer", 0x00004000);
         memory_region_add_subregion(address_space, 0xfc080000 + 0x4000 * i,

hw/net/fsl_etsec/etsec.c +1 -1

index 98250e0..8f640b0 100644
@@ -387,7 +387,7 @@ static void etsec_realize(DeviceState *dev, Error **errp)
 
 
     etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
-    etsec->ptimer = ptimer_init(etsec->bh);
+    etsec->ptimer = ptimer_init(etsec->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(etsec->ptimer, 100);
 }

hw/net/lan9118.c +1 -1

index 2052073..650a6c7 100644
@@ -1345,7 +1345,7 @@ static int lan9118_init1(SysBusDevice *sbd)
     s->txp = &s->tx_packet;
 
     bh = qemu_bh_new(lan9118_tick, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->timer, 10000);
     ptimer_set_limit(s->timer, 0xffff, 1);

hw/timer/allwinner-a10-pit.c +1 -1

index 3385e5d..22ceabe 100644
@@ -267,7 +267,7 @@ static void a10_pit_init(Object *obj)
         tc->container = s;
         tc->index = i;
         bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
-        s->timer[i] = ptimer_init(bh[i]);
+        s->timer[i] = ptimer_init(bh[i], PTIMER_POLICY_DEFAULT);
     }
 }

hw/timer/arm_timer.c +1 -1

index 111a16d..98fddd7 100644
@@ -171,7 +171,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
     s->control = TIMER_CTRL_IE;
 
     bh = qemu_bh_new(arm_timer_tick, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
     vmstate_register(NULL, -1, &vmstate_arm_timer, s);
     return s;
 }

hw/timer/digic-timer.c +1 -1

index 0f21faf..e1fcf73 100644
@@ -127,7 +127,7 @@ static void digic_timer_init(Object *obj)
 {
     DigicTimerState *s = DIGIC_TIMER(obj);
 
-    s->ptimer = ptimer_init(NULL);
+    s->ptimer = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
 
     /*
      * FIXME: there is no documentation on Digic timer

hw/timer/etraxfs_timer.c +3 -3

index 36d8f46..8e18236 100644
@@ -322,9 +322,9 @@ static int etraxfs_timer_init(SysBusDevice *dev)
     t->bh_t0 = qemu_bh_new(timer0_hit, t);
     t->bh_t1 = qemu_bh_new(timer1_hit, t);
     t->bh_wd = qemu_bh_new(watchdog_hit, t);
-    t->ptimer_t0 = ptimer_init(t->bh_t0);
-    t->ptimer_t1 = ptimer_init(t->bh_t1);
-    t->ptimer_wd = ptimer_init(t->bh_wd);
+    t->ptimer_t0 = ptimer_init(t->bh_t0, PTIMER_POLICY_DEFAULT);
+    t->ptimer_t1 = ptimer_init(t->bh_t1, PTIMER_POLICY_DEFAULT);
+    t->ptimer_wd = ptimer_init(t->bh_wd, PTIMER_POLICY_DEFAULT);
 
     sysbus_init_irq(dev, &t->irq);
     sysbus_init_irq(dev, &t->nmi);

hw/timer/exynos4210_mct.c +4 -3

index ae69345..0c18934 100644
@@ -1431,15 +1431,16 @@ static void exynos4210_mct_init(Object *obj)
 
     /* Global timer */
     bh[0] = qemu_bh_new(exynos4210_gfrc_event, s);
-    s->g_timer.ptimer_frc = ptimer_init(bh[0]);
+    s->g_timer.ptimer_frc = ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
     memset(&s->g_timer.reg, 0, sizeof(struct gregs));
 
     /* Local timers */
     for (i = 0; i < 2; i++) {
         bh[0] = qemu_bh_new(exynos4210_ltick_event, &s->l_timer[i]);
         bh[1] = qemu_bh_new(exynos4210_lfrc_event, &s->l_timer[i]);
-        s->l_timer[i].tick_timer.ptimer_tick = ptimer_init(bh[0]);
-        s->l_timer[i].ptimer_frc = ptimer_init(bh[1]);
+        s->l_timer[i].tick_timer.ptimer_tick =
+                                   ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
+        s->l_timer[i].ptimer_frc = ptimer_init(bh[1], PTIMER_POLICY_DEFAULT);
         s->l_timer[i].id = i;
     }

hw/timer/exynos4210_pwm.c +1 -1

index 0e9e2e9..f576507 100644
@@ -390,7 +390,7 @@ static void exynos4210_pwm_init(Object *obj)
     for (i = 0; i < EXYNOS4210_PWM_TIMERS_NUM; i++) {
         bh = qemu_bh_new(exynos4210_pwm_tick, &s->timer[i]);
         sysbus_init_irq(dev, &s->timer[i].irq);
-        s->timer[i].ptimer = ptimer_init(bh);
+        s->timer[i].ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
         s->timer[i].id = i;
         s->timer[i].parent = s;
     }

hw/timer/exynos4210_rtc.c +2 -2

index da4dd45..1a648c5 100644
@@ -555,12 +555,12 @@ static void exynos4210_rtc_init(Object *obj)
     QEMUBH *bh;
 
     bh = qemu_bh_new(exynos4210_rtc_tick, s);
-    s->ptimer = ptimer_init(bh);
+    s->ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer, RTC_BASE_FREQ);
     exynos4210_rtc_update_freq(s, 0);
 
     bh = qemu_bh_new(exynos4210_rtc_1Hz_tick, s);
-    s->ptimer_1Hz = ptimer_init(bh);
+    s->ptimer_1Hz = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer_1Hz, RTC_BASE_FREQ);
 
     sysbus_init_irq(dev, &s->alm_irq);

hw/timer/grlib_gptimer.c +1 -1

index dd000f5..712d1ae 100644
@@ -363,7 +363,7 @@ static int grlib_gptimer_init(SysBusDevice *dev)
 
         timer->unit   = unit;
         timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
-        timer->ptimer = ptimer_init(timer->bh);
+        timer->ptimer = ptimer_init(timer->bh, PTIMER_POLICY_DEFAULT);
         timer->id     = i;
 
         /* One IRQ line for each timer */

hw/timer/imx_epit.c +2 -2

index eddf348..f34d7f7 100644
@@ -314,10 +314,10 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
                           0x00001000);
     sysbus_init_mmio(sbd, &s->iomem);
 
-    s->timer_reload = ptimer_init(NULL);
+    s->timer_reload = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
 
     bh = qemu_bh_new(imx_epit_cmp, s);
-    s->timer_cmp = ptimer_init(bh);
+    s->timer_cmp = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static void imx_epit_class_init(ObjectClass *klass, void *data)

hw/timer/imx_gpt.c +1 -1

index 82bc73c..f034d65 100644
@@ -461,7 +461,7 @@ static void imx_gpt_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
 
     bh = qemu_bh_new(imx_gpt_timeout, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static void imx_gpt_class_init(ObjectClass *klass, void *data)

hw/timer/lm32_timer.c +1 -1

index e45a65b..2a07b59 100644
@@ -184,7 +184,7 @@ static void lm32_timer_init(Object *obj)
     sysbus_init_irq(dev, &s->irq);
 
     s->bh = qemu_bh_new(timer_hit, s);
-    s->ptimer = ptimer_init(s->bh);
+    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
 
     memory_region_init_io(&s->iomem, obj, &timer_ops, s,
                           "timer", R_MAX * 4);

hw/timer/milkymist-sysctl.c +2 -2

index 2194832..4488590 100644
@@ -281,8 +281,8 @@ static void milkymist_sysctl_init(Object *obj)
 
     s->bh0 = qemu_bh_new(timer0_hit, s);
     s->bh1 = qemu_bh_new(timer1_hit, s);
-    s->ptimer0 = ptimer_init(s->bh0);
-    s->ptimer1 = ptimer_init(s->bh1);
+    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
+    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
 
     memory_region_init_io(&s->regs_region, obj, &sysctl_mmio_ops, s,
             "milkymist-sysctl", R_MAX * 4);

hw/timer/puv3_ost.c +1 -1

index 93650b7..0b3d717 100644
@@ -125,7 +125,7 @@ static int puv3_ost_init(SysBusDevice *dev)
     sysbus_init_irq(dev, &s->irq);
 
     s->bh = qemu_bh_new(puv3_ost_tick, s);
-    s->ptimer = ptimer_init(s->bh);
+    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer, 50 * 1000 * 1000);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &puv3_ost_ops, s, "puv3_ost",

hw/timer/sh_timer.c +1 -1

index 255b2fc..9afb2d0 100644
@@ -203,7 +203,7 @@ static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
     s->irq = irq;
 
     bh = qemu_bh_new(sh_timer_tick, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
 
     sh_timer_write(s, OFFSET_TCOR >> 2, s->tcor);
     sh_timer_write(s, OFFSET_TCNT >> 2, s->tcnt);

hw/timer/slavio_timer.c +1 -1

index fb3e08b..bfee1f3 100644
@@ -389,7 +389,7 @@ static int slavio_timer_init1(SysBusDevice *dev)
         tc->timer_index = i;
 
         bh = qemu_bh_new(slavio_timer_irq, tc);
-        s->cputimer[i].timer = ptimer_init(bh);
+        s->cputimer[i].timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD);
 
         size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE;

hw/timer/xilinx_timer.c +1 -1

index 2ea970d..59439c0 100644
@@ -218,7 +218,7 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
         xt->parent = t;
         xt->nr = i;
         xt->bh = qemu_bh_new(timer_hit, xt);
-        xt->ptimer = ptimer_init(xt->bh);
+        xt->ptimer = ptimer_init(xt->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(xt->ptimer, t->freq_hz);
     }

include/hw/ptimer.h +3 -1

index e397db5..80564ea 100644
@@ -12,11 +12,13 @@
 #include "qemu/timer.h"
 #include "migration/vmstate.h"
 
+#define PTIMER_POLICY_DEFAULT               0
+
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;
 typedef void (*ptimer_cb)(void *opaque);
 
-ptimer_state *ptimer_init(QEMUBH *bh);
+ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask);
 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);

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #11 lore raw

PATCH v15 11/15 tests: ptimer: Add tests for "no immediate trigger" policy

In reply to Dmitry Osipenko

PTIMER_POLICY_NO_IMMEDIATE_TRIGGER makes ptimer to not to trigger on starting to run with / setting counter to "0".

Signed-off-by Dmitry Osipenko <[email protected]>

---
 tests/ptimer-test.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 7 deletions(-)

tests/ptimer-test.c +51 -7

index cdf8d28..ba9ae39 100644
@@ -189,6 +189,7 @@ static void check_periodic(gconstpointer arg)
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
+    bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
 
     triggered = false;

@@ -245,7 +246,12 @@ static void check_periodic(gconstpointer arg)
 
     ptimer_set_count(ptimer, 0);
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 10);
-    g_assert_true(triggered);
+
+    if (no_immediate_trigger) {
+        g_assert_false(triggered);
+    } else {
+        g_assert_true(triggered);
+    }
 
     triggered = false;

@@ -389,6 +395,7 @@ static void check_run_with_delta_0(gconstpointer arg)
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
+    bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
 
     triggered = false;

@@ -396,7 +403,12 @@ static void check_run_with_delta_0(gconstpointer arg)
     ptimer_set_limit(ptimer, 99, 0);
     ptimer_run(ptimer, 1);
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
-    g_assert_true(triggered);
+
+    if (no_immediate_trigger) {
+        g_assert_false(triggered);
+    } else {
+        g_assert_true(triggered);
+    }
 
     triggered = false;

@@ -420,7 +432,12 @@ static void check_run_with_delta_0(gconstpointer arg)
     ptimer_set_count(ptimer, 0);
     ptimer_run(ptimer, 0);
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
-    g_assert_true(triggered);
+
+    if (no_immediate_trigger) {
+        g_assert_false(triggered);
+    } else {
+        g_assert_true(triggered);
+    }
 
     triggered = false;

@@ -443,6 +460,7 @@ static void check_periodic_with_load_0(gconstpointer arg)
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
     bool continuous_trigger = (*policy & PTIMER_POLICY_CONTINUOUS_TRIGGER);
+    bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
 
     triggered = false;

@@ -450,7 +468,12 @@ static void check_periodic_with_load_0(gconstpointer arg)
     ptimer_run(ptimer, 0);
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
-    g_assert_true(triggered);
+
+    if (no_immediate_trigger) {
+        g_assert_false(triggered);
+    } else {
+        g_assert_true(triggered);
+    }
 
     triggered = false;

@@ -458,7 +481,7 @@ static void check_periodic_with_load_0(gconstpointer arg)
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
 
-    if (continuous_trigger) {
+    if (continuous_trigger || no_immediate_trigger) {
         g_assert_true(triggered);
     } else {
         g_assert_false(triggered);

@@ -472,6 +495,7 @@ static void check_oneshot_with_load_0(gconstpointer arg)
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
     ptimer_state *ptimer = ptimer_init(bh, *policy);
+    bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
 
     triggered = false;

@@ -479,13 +503,29 @@ static void check_oneshot_with_load_0(gconstpointer arg)
     ptimer_run(ptimer, 1);
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
-    g_assert_true(triggered);
+
+    if (no_immediate_trigger) {
+        g_assert_false(triggered);
+    } else {
+        g_assert_true(triggered);
+    }
 
     triggered = false;
 
     qemu_clock_step(2000000 + 100000);
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+
+    if (no_immediate_trigger) {
+        g_assert_true(triggered);
+    } else {
+        g_assert_false(triggered);
+    }
+
+    triggered = false;
+
+    qemu_clock_step(2000000 + 100000);
+
     g_assert_false(triggered);
 }

@@ -508,6 +548,10 @@ static void add_ptimer_tests(uint8_t policy)
         g_strlcat(policy_name, "continuous_trigger,", 256);
     }
 
+    if (policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER) {
+        g_strlcat(policy_name, "no_immediate_trigger,", 256);
+    }
+
     qtest_add_data_func(
         g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
         ppolicy, check_set_count);

@@ -555,7 +599,7 @@ static void add_ptimer_tests(uint8_t policy)
 
 static void add_all_ptimer_policies_comb_tests(void)
 {
-    int last_policy = PTIMER_POLICY_CONTINUOUS_TRIGGER;
+    int last_policy = PTIMER_POLICY_NO_IMMEDIATE_TRIGGER;
     int policy = PTIMER_POLICY_DEFAULT;
 
     for (; policy < (last_policy << 1); policy++) {

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #12 lore raw

PATCH v15 12/15 hw/ptimer: Add "no immediate reload" policy

In reply to Dmitry Osipenko

Immediate counter re-load on setting (or on starting to run with) counter = 0 is a wrong behaviour for some of the timers. Add "no immediate reload" policy that provides correct behaviour for such timers.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c    | 8 +++++---
 include/hw/ptimer.h | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

hw/core/ptimer.c +5 -3

index d37e8ab..7747c65 100644
@@ -44,7 +44,7 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
         ptimer_trigger(s);
     }
 
-    if (delta == 0) {
+    if (delta == 0 && !(s->policy_mask & PTIMER_POLICY_NO_IMMEDIATE_RELOAD)) {
         delta = s->delta = s->limit;
     }

@@ -102,11 +102,13 @@ static void ptimer_tick(void *opaque)
 {
     ptimer_state *s = (ptimer_state *)opaque;
     ptimer_trigger(s);
-    s->delta = 0;
     if (s->enabled == 2) {
+        s->delta = 0;
         s->enabled = 0;
     } else {
-        ptimer_reload(s, 1);
+        int delta_adjust = (s->delta != 0 && s->limit != 0) ? 1 : 0;
+        s->delta = s->limit;
+        ptimer_reload(s, delta_adjust);
     }
 }

include/hw/ptimer.h +2 -0

index ec76183..3e49550 100644
@@ -22,6 +22,8 @@
 /* Starting to run with/setting counter = 0 won't perform immediate
  * trigger.  */
 #define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER  (1 << 2)
+/* Starting to run with/setting counter = 0 won't re-load counter.  */
+#define PTIMER_POLICY_NO_IMMEDIATE_RELOAD   (1 << 3)
 
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #13 lore raw

PATCH v15 13/15 tests: ptimer: Add tests for "no immediate reload" policy

In reply to Dmitry Osipenko

PTIMER_POLICY_NO_IMMEDIATE_RELOAD makes ptimer to not to re-load counter on setting counter value to "0" or starting to run with "0".

Signed-off-by Dmitry Osipenko <[email protected]>

---
 tests/ptimer-test.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

tests/ptimer-test.c +42 -6

index ba9ae39..c99f923 100644
@@ -190,6 +190,8 @@ static void check_periodic(gconstpointer arg)
     ptimer_state *ptimer = ptimer_init(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
     bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
+    bool no_immediate_reload = (*policy & PTIMER_POLICY_NO_IMMEDIATE_RELOAD);
+    bool continuous_trigger = (*policy & PTIMER_POLICY_CONTINUOUS_TRIGGER);
 
     triggered = false;

@@ -245,7 +247,7 @@ static void check_periodic(gconstpointer arg)
     g_assert_false(triggered);
 
     ptimer_set_count(ptimer, 0);
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 10);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, no_immediate_reload ? 0 : 10);
 
     if (no_immediate_trigger) {
         g_assert_false(triggered);

@@ -257,7 +259,14 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 12 + 100000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 9 : 8);
+    if (no_immediate_reload && (!continuous_trigger && !no_immediate_trigger)) {
+        g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+        g_assert_false(triggered);
+        return;
+    }
+
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==,
+                    8 + (wrap_policy ? 1 : 0) + (no_immediate_reload ? 1 : 0));
     g_assert_true(triggered);
 
     ptimer_stop(ptimer);

@@ -266,7 +275,8 @@ static void check_periodic(gconstpointer arg)
 
     qemu_clock_step(2000000 * 12 + 100000);
 
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, wrap_policy ? 9 : 8);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==,
+                    8 + (wrap_policy ? 1 : 0) + (no_immediate_reload ? 1 : 0));
     g_assert_false(triggered);
 }

@@ -396,13 +406,15 @@ static void check_run_with_delta_0(gconstpointer arg)
     ptimer_state *ptimer = ptimer_init(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
     bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
+    bool no_immediate_reload = (*policy & PTIMER_POLICY_NO_IMMEDIATE_RELOAD);
+    bool continuous_trigger = (*policy & PTIMER_POLICY_CONTINUOUS_TRIGGER);
 
     triggered = false;
 
     ptimer_set_period(ptimer, 2000000);
     ptimer_set_limit(ptimer, 99, 0);
     ptimer_run(ptimer, 1);
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, no_immediate_reload ? 0 : 99);
 
     if (no_immediate_trigger) {
         g_assert_false(triggered);

@@ -412,6 +424,11 @@ static void check_run_with_delta_0(gconstpointer arg)
 
     triggered = false;
 
+    if (no_immediate_reload) {
+        ptimer_set_count(ptimer, 99);
+        ptimer_run(ptimer, 1);
+    }
+
     qemu_clock_step(2000000 + 100000);
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 98);

@@ -431,7 +448,7 @@ static void check_run_with_delta_0(gconstpointer arg)
 
     ptimer_set_count(ptimer, 0);
     ptimer_run(ptimer, 0);
-    g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
+    g_assert_cmpuint(ptimer_get_count(ptimer), ==, no_immediate_reload ? 0 : 99);
 
     if (no_immediate_trigger) {
         g_assert_false(triggered);

@@ -441,6 +458,21 @@ static void check_run_with_delta_0(gconstpointer arg)
 
     triggered = false;
 
+    if (no_immediate_reload) {
+        qemu_clock_step(2000000 + 100000);
+
+        if (continuous_trigger || no_immediate_trigger) {
+            g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
+            g_assert_true(triggered);
+
+            triggered = false;
+        } else {
+            g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
+            g_assert_false(triggered);
+            return;
+        }
+    }
+
     qemu_clock_step(2000000 + 100000);
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 98);

@@ -552,6 +584,10 @@ static void add_ptimer_tests(uint8_t policy)
         g_strlcat(policy_name, "no_immediate_trigger,", 256);
     }
 
+    if (policy & PTIMER_POLICY_NO_IMMEDIATE_RELOAD) {
+        g_strlcat(policy_name, "no_immediate_reload,", 256);
+    }
+
     qtest_add_data_func(
         g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
         ppolicy, check_set_count);

@@ -599,7 +635,7 @@ static void add_ptimer_tests(uint8_t policy)
 
 static void add_all_ptimer_policies_comb_tests(void)
 {
-    int last_policy = PTIMER_POLICY_NO_IMMEDIATE_TRIGGER;
+    int last_policy = PTIMER_POLICY_NO_IMMEDIATE_RELOAD;
     int policy = PTIMER_POLICY_DEFAULT;
 
     for (; policy < (last_policy << 1); policy++) {

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #14 lore raw

PATCH v15 14/15 arm_mptimer: Convert to use ptimer

In reply to 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
- Properly 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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
 hw/timer/arm_mptimer.c         | 148 ++++++++++++++++++++++-------------------
 include/hw/timer/arm_mptimer.h |   5 +-
 2 files changed, 82 insertions(+), 71 deletions(-)

hw/timer/arm_mptimer.c +81 ~~-67~~

index d66bbf0..2a0fc78 100644
@@ -20,22 +20,32 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/ptimer.h"
 #include "hw/timer/arm_mptimer.h"
 #include "qapi/error.h"
-#include "qemu/timer.h"
+#include "qemu/main-loop.h"
 #include "qom/cpu.h"
 
+#define PTIMER_POLICY                       \
+    (PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |  \
+     PTIMER_POLICY_CONTINUOUS_TRIGGER    |  \
+     PTIMER_POLICY_NO_IMMEDIATE_TRIGGER  |  \
+     PTIMER_POLICY_NO_IMMEDIATE_RELOAD)
+
 /* This device implements the per-cpu private timer and watchdog block
  * which is used in both the ARM11MPCore and Cortex-A9MP.
  */
 
 static inline int get_current_cpu(ARMMPTimerState *s)
 {
-    if (current_cpu->cpu_index >= s->num_cpu) {
+    int cpu_id = current_cpu ? current_cpu->cpu_index : 0;
+
+    if (cpu_id >= s->num_cpu) {
         hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
-                 s->num_cpu, current_cpu->cpu_index);
+                 s->num_cpu, cpu_id);
     }
-    return current_cpu->cpu_index;
+
+    return cpu_id;
 }
 
 static inline void timerblock_update_irq(TimerBlock *tb)

@@ -44,33 +54,42 @@ 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 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;
+    /* 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) == 0 &&
+            ptimer_get_limit(tb->timer) == 0) {
+        ptimer_stop(tb->timer);
     }
+    tb->status = 1;
     timerblock_update_irq(tb);
 }

@@ -78,21 +97,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;
+        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.  */

@@ -106,37 +115,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 & 3) != (value & 3)) {
+            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;

@@ -186,13 +203,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));
     }
 }

@@ -238,7 +254,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, PTIMER_POLICY);
         sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);

@@ -248,26 +265,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()
     }
 };

include/hw/timer/arm_mptimer.h +1 -4

index b34cba0..c46d8d2 100644
@@ -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;

Signature · 1 line

2.9.2

Dmitry Osipenko

author

2016-07-21 14:32 UTC #15 lore raw

PATCH v15 15/15 tests: Add tests for the ARM MPTimer

In reply to Dmitry Osipenko

ARM MPTimer is a per-CPU core timer, essential part of the ARM Cortex-A9 MPCore. Add QTests for it.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 tests/Makefile.include   |    2 +
 tests/test-arm-mptimer.c | 1079 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 1081 insertions(+)
 create mode 100644 tests/test-arm-mptimer.c

tests/Makefile.include +2 -0

index 17056e9..56f4d0e 100644
@@ -257,6 +257,7 @@ check-qtest-arm-y += tests/ds1338-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
 check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
 gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
+check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
 check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
 check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)

@@ -622,6 +623,7 @@ tests/test-filter-redirector$(EXESUF): tests/test-filter-redirector.o $(qtest-ob
 tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y)
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o
 tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o hw/core/ptimer.o
+tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 
 ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil

tests/test-arm-mptimer.c new file +1079 -0

1,079 lines not on this page · raw message

Signature · 1 line

2.9.2

Peter Maydell

2016-09-05 18:15 UTC #16 lore raw

In reply to Dmitry Osipenko

Quoted 40 lines · Dmitry Osipenko

Hello,
Currently, QEMU ARM MPTimer device model provides only a certain subset of the emulation behavior. This patch series is supposed to add missing parts by converting the MPTimer to use generic ptimer helper. It fixes some important ptimer bugs and provides new features that are required for the ARM MPTimer.
Changelog:
V15: As per Peter's Maydell request: split ptimer policy patches,
so that first "policy" patch only sets default policy to all of the timers without introducing new behaviour policies.
The "deferred trigger" ptimer policy from V14 is converted to "continuous trigger" as it is more intuitive.
New ptimer policies added.
New ptimer patch "Actually stop timer in case of error".
Fixed ARM MPTimer periodic/oneshot "on the fly" mode switch for the case when load = 0 and new mode has prescaler = 0.
Added QTests for each of the ptimer policies.
Added QTests for the ARM MPTimer.
V14: Set the ptimer policy in the ptimer_init() instead of adding
ptimer_set_policy(), keeping ptimer VMState unchanged and dropped hw_error() hardening asserts as per Peter's Maydell V13 review comments, addressed the rest of the review comments.
Dmitry Osipenko (15):
hw/ptimer: Change ptimer_get_count to return "1" for the expired timer hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active
timer
hw/ptimer: Actually stop timer in case of error
hw/ptimer: Introduce timer policy feature
tests: Add ptimer tests
hw/ptimer: Add "wraparound after one period" policy
tests: ptimer: Add tests for "wraparound after one period" policy

Quoted 8 lines

hw/ptimer: Add "continuous trigger" policy
tests: ptimer: Add tests for "continuous trigger" policy hw/ptimer: Add "no immediate trigger" policy
tests: ptimer: Add tests for "no immediate trigger" policy hw/ptimer: Add "no immediate reload" policy
tests: ptimer: Add tests for "no immediate reload" policy
arm_mptimer: Convert to use ptimer
tests: Add tests for the ARM MPTimer

I tried "check this compiles and passes the tests for each commit", and what I found was that you get a lot of new warnings running 'make check' about "Timer with period zero, disabling", which were not there before. Could you look into this and fix it, please?

thanks
-- PMM

Dmitry Osipenko

author

2016-09-05 21:52 UTC #17 lore raw

Quoted 8 lines · Peter Maydell

Quoted 7 lines · Dmitry Osipenko
Hello,
Currently, QEMU ARM MPTimer device model provides only a certain subset of the emulation behavior. This patch series is supposed to add missing parts by converting the MPTimer to use generic ptimer helper. It fixes some important ptimer bugs and provides new features that are required for the ARM MPTimer.

[snip]

Quoted 9 lines

I tried "check this compiles and passes the tests for each commit", and what I found was that you get a lot of new warnings running 'make check' about "Timer with period zero, disabling", which were not there before. Could you look into this and fix it, please?
thanks
-- PMM

Sure, I'll silence those ptimer error messages by making them conditional of qtest_enabled(). The messages are actually harmless, however not very useful in the context of testing. Thanks a lot for returning to that series, I appreciate it.

Signature · 1 line

Dmitry

Peter Maydell

2016-09-05 22:12 UTC #18 lore raw

Quoted 9 lines · Dmitry Osipenko

Quoted 4 lines · Peter Maydell
I tried "check this compiles and passes the tests for each commit", and what I found was that you get a lot of new warnings running 'make check' about "Timer with period zero, disabling", which were not there before. Could you look into this and fix it, please?
Sure, I'll silence those ptimer error messages by making them conditional of qtest_enabled(). The messages are actually harmless, however not very useful in the context of testing. Thanks a lot for returning to that series, I appreciate it.

I had a look at the rest of the patches, but to be honest I found it very difficult to figure out whether any of the changes were making the right changes or the wrong changes. So it's not clear to me that "silence the warning if running under qtest" is right:
why is the warning being produced at all?

thanks
-- PMM

Dmitry Osipenko

author

2016-09-06 10:32 UTC #19 lore raw

Quoted 11 lines · Peter Maydell

Quoted 9 lines · Dmitry Osipenko
Quoted 4 lines · Peter Maydell
I tried "check this compiles and passes the tests for each commit", and what I found was that you get a lot of new warnings running 'make check' about "Timer with period zero, disabling", which were not there before. Could you look into this and fix it, please?
Sure, I'll silence those ptimer error messages by making them conditional of qtest_enabled(). The messages are actually harmless, however not very useful in the context of testing. Thanks a lot for returning to that series, I appreciate it.

Quoted 6 lines

I had a look at the rest of the patches, but to be honest I found it very difficult to figure out whether any of the changes were making the right changes or the wrong changes. So it's not clear to me that "silence the warning if running under qtest" is right:
why is the warning being produced at all?

The ptimer tests cover all ptimer behaviour cases, including the cases where ptimer stops because of the error condition. This helps to ensure that further applied ptimer patches (like new policies) are not affecting old ptimer (policy) behaviour, including those error cases. So the warning message is being emitted each time some of the ptimer tests checks the error condition behaviour.

If you have any thoughts on how to make review of this series easier for you, please tell. I'm open to suggestions.

BTW, I'm going to turn "Fix counter - 1 returned by ptimer_get_count for the active timer" patch into a ptimer policy. The patch is correct for all of the current ptimer users, however that "counter - 1" feature could be useful for some of the future added timers, like nios2 timer [0], and could be already used by some of the QEMU forks. So it might be better to retain old behaviour for the default policy.

[0] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg06423.html

Signature · 1 line

Dmitry

Peter Maydell

2016-09-06 10:37 UTC #20 lore raw

Quoted 8 lines · Dmitry Osipenko

Quoted 6 lines · Peter Maydell
I had a look at the rest of the patches, but to be honest I found it very difficult to figure out whether any of the changes were making the right changes or the wrong changes. So it's not clear to me that "silence the warning if running under qtest" is right:
why is the warning being produced at all?

Quoted 5 lines

The ptimer tests cover all ptimer behaviour cases, including the cases where ptimer stops because of the error condition. This helps to ensure that further applied ptimer patches (like new policies) are not affecting old ptimer (policy) behaviour, including those error cases. So the warning message is being emitted each time some of the ptimer tests checks the error condition behaviour.

Oh, I see. Right, that's certainly OK to suppress warnings for.

Quoted 9 lines

If you have any thoughts on how to make review of this series easier for you, please tell. I'm open to suggestions.
BTW, I'm going to turn "Fix counter - 1 returned by ptimer_get_count for the active timer" patch into a ptimer policy. The patch is correct for all of the current ptimer users, however that "counter - 1" feature could be useful for some of the future added timers, like nios2 timer [0], and could be already used by some of the QEMU forks. So it might be better to retain old behaviour for the default policy.

Yes, if the patchset didn't change any behaviour for timers which didn't request a non-default policy that would definitely help in making me more confident it was safe.

thanks
-- PMM