PTimer fixes/features and ARM MPTimer conversion

PATCH Applied Dmitry Osipenko 2 participants 13 messages May 27, 2016 – Jun 6, 2016

lore

Share & tools

lore

Dmitry Osipenko

author

2016-05-27 17:04 UTC #0 lore raw

v13 cover PTimer fixes/features and ARM MPTimer conversion

Hello,

Current QEMU ARM MPTimer device model provides only a certain subset of the emulation behavior, so 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.

Emulation behavior is verified against the real HW by running specially crafted MPTimer tests in both icount and non-icount modes:

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

Changelog for the 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.

v13: Rebased on recent master, use new ptimer policy feature.

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"

v13: Squashed the following two patches from v12:

hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0 hw/ptimer: Perform delayed tick instead of immediate if delta = 0

into

hw/ptimer: Support running with counter = 0 by introducing new policy feature

The second patch has no r-b, I omitted Peter's Crosthwaite r-b from the first patch. I think it's not valid now, otherwise please let me know.

Dmitry Osipenko (8):
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: Support running with counter = 0 by introducing new policy
feature
hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active
timer
arm_mptimer: Convert to use ptimer

 hw/core/ptimer.c               | 131 ++++++++++++++++++++++++---------------
 hw/timer/arm_mptimer.c         | 135 ++++++++++++++++++++++-------------------
 include/hw/ptimer.h            |   7 +++
 include/hw/timer/arm_mptimer.h |   5 +-
 4 files changed, 162 insertions(+), 116 deletions(-)

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:04 UTC #5 lore raw

PATCH v13 5/8 hw/ptimer: Introduce ptimer_get_limit

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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
 hw/core/ptimer.c    | 5 +++++
 include/hw/ptimer.h | 1 +
 2 files changed, 6 insertions(+)

hw/core/ptimer.c +5 -0

index d0b2f38..05b0c27 100644
@@ -225,6 +225,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,

include/hw/ptimer.h +1 -0

index 8ebacbb..e397db5 100644
@@ -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);

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:04 UTC #10 lore raw

PATCH v13 7/8 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 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 <[email protected]>

---
 hw/core/ptimer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

hw/core/ptimer.c +3 -3

index 9bc70f5..c9f2604 100644
@@ -89,10 +89,10 @@ static void ptimer_tick(void *opaque)
 
 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);

@@ -144,7 +144,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.  */

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:04 UTC #6 lore raw

PATCH v13 6/8 hw/ptimer: Support running with counter = 0 by introducing new policy feature

In reply to Dmitry Osipenko

Currently ptimer prints error message and clears enable flag for an arming timer that has delta = load = 0. That actually is a valid case for most of the timers, like instant IRQ trigger for oneshot timer or continuous in periodic mode. There are different possible policies of how particular timer could interpret setting counter to 0, like:

1) Immediate stop with triggering the interrupt in oneshot mode. Immediate wraparound with triggering the interrupt in continuous mode.

2) Immediate stop without triggering the interrupt in oneshot mode. Immediate wraparound without triggering the interrupt in continuous mode.

3) Stopping with triggering the interrupt after one period in oneshot mode. Wraparound with triggering the interrupt in continuous mode after one period.

4) Stopping without triggering the interrupt after one period in oneshot mode. Wraparound without triggering the interrupt in continuous mode after one period.

As well as handling oneshot/continuous modes differently.

Given that, currently, running the timer with counter/period equal to 0 is treated as a error case, it's not obvious what policies should be supported by ptimer. Let's implement the third policy for now, as it is known to be used by the ARM MPTimer.

Explicitly forbid running with counter/period == 0 in all cases by aborting QEMU execution instead of printing the error message.

Bump VMSD version since a new VMState member is added.

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

---
 hw/core/ptimer.c    | 53 +++++++++++++++++++++++++++++++----------------------
 include/hw/ptimer.h |  6 ++++++
 2 files changed, 37 insertions(+), 22 deletions(-)

hw/core/ptimer.c +31 ~~-22~~

index 05b0c27..9bc70f5 100644
@@ -21,6 +21,7 @@ struct ptimer_state
     int64_t period;
     int64_t last_event;
     int64_t next_event;
+    uint8_t policy;
     QEMUBH *bh;
     QEMUTimer *timer;
 };

@@ -37,15 +38,16 @@ static void ptimer_reload(ptimer_state *s)
 {
     uint32_t period_frac = s->period_frac;
     uint64_t period = s->period;
+    uint64_t delta = MAX(1, s->delta);
 
-    if (s->delta == 0) {
-        ptimer_trigger(s);
-        s->delta = s->limit;
+    if (s->delta == 0 && s->policy == UNIMPLEMENTED) {
+        hw_error("ptimer: Running with counter=0 is unimplemented by " \
+                 "this timer, fix it!\n");
     }
-    if (s->delta == 0 || s->period == 0) {
-        fprintf(stderr, "Timer with period zero, disabling\n");
-        s->enabled = 0;
-        return;
+
+    if (period == 0) {
+        hw_error("ptimer: Timer tries to run with period=0, behaviour is " \
+                 "undefined, fix it!\n");
     }
 
     /*

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

@@ -73,27 +75,30 @@ 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->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)
 {
     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;

@@ -165,11 +170,8 @@ 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) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         ptimer_reload(s);

@@ -203,6 +205,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;

@@ -230,10 +233,15 @@ uint64_t ptimer_get_limit(ptimer_state *s)
     return s->limit;
 }
 
+void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy)
+{
+    s->policy = policy;
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(enabled, ptimer_state),
         VMSTATE_UINT64(limit, ptimer_state),

@@ -243,6 +251,7 @@ const VMStateDescription vmstate_ptimer = {
         VMSTATE_INT64(last_event, ptimer_state),
         VMSTATE_INT64(next_event, ptimer_state),
         VMSTATE_TIMER_PTR(timer, ptimer_state),
+        VMSTATE_UINT8(policy, ptimer_state),
         VMSTATE_END_OF_LIST()
     }
 };

include/hw/ptimer.h +6 -0

index e397db5..221f591 100644
@@ -12,6 +12,11 @@
 #include "qemu/timer.h"
 #include "migration/vmstate.h"
 
+enum ptimer_policy {
+    UNIMPLEMENTED,
+    SET_CNT_TO_0_TRIGGERS_IRQ_AFTER_ONE_PERIOD,
+};
+
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;
 typedef void (*ptimer_cb)(void *opaque);

@@ -25,6 +30,7 @@ uint64_t ptimer_get_count(ptimer_state *s);
 void ptimer_set_count(ptimer_state *s, uint64_t count);
 void ptimer_run(ptimer_state *s, int oneshot);
 void ptimer_stop(ptimer_state *s);
+void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy);
 
 extern const VMStateDescription vmstate_ptimer;

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:04 UTC #11 lore raw

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

Use new ptimer policy feature to correctly emulate running with/setting counter = 0.

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

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

---
 hw/timer/arm_mptimer.c         | 135 ++++++++++++++++++++++-------------------
 include/hw/timer/arm_mptimer.h |   5 +-
 2 files changed, 72 insertions(+), 68 deletions(-)

hw/timer/arm_mptimer.c +71 ~~-64~~

index d66bbf0..bffc506 100644
@@ -20,9 +20,10 @@
  */
 
 #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"
 
 /* This device implements the per-cpu private timer and watchdog block

@@ -44,55 +45,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.  */

@@ -106,37 +106,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;

@@ -186,13 +194,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 +245,10 @@ 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_set_policy(tb->timer,
+                          SET_CNT_TO_0_TRIGGERS_IRQ_AFTER_ONE_PERIOD);
         sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);

@@ -248,26 +258,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.8.3

Dmitry Osipenko

author

2016-05-27 17:04 UTC #4 lore raw

PATCH v13 4/8 hw/ptimer: Support "on the fly" timer mode switch

In reply to Dmitry Osipenko

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

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

Reviewed-by Peter Crosthwaite <[email protected]>

---
 hw/core/ptimer.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

hw/core/ptimer.c +7 -6

index 76ebe9b..d0b2f38 100644
@@ -163,16 +163,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

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:05 UTC #1 lore raw

PATCH v13 1/8 hw/ptimer: Fix issues caused by the adjusted timer limit value

In reply to Dmitry Osipenko

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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
 hw/core/ptimer.c | 51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

hw/core/ptimer.c +31 ~~-20~~

index 153c835..16d7dd5 100644
@@ -35,6 +35,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;

@@ -45,10 +48,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);
 }

@@ -83,6 +100,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

@@ -95,7 +119,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);

@@ -104,13 +128,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;

@@ -182,19 +206,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;

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:05 UTC #2 lore raw

PATCH v13 2/8 hw/ptimer: Perform counter wrap around if timer already expired

In reply to Dmitry Osipenko

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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
 hw/core/ptimer.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

hw/core/ptimer.c +13 -6

index 16d7dd5..7e6fc2d 100644
@@ -84,14 +84,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;

@@ -103,7 +105,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;
             }

@@ -118,7 +120,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);

@@ -138,6 +140,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;

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-27 17:05 UTC #3 lore raw

PATCH v13 3/8 hw/ptimer: Update .delta on period/freq change

In reply to Dmitry Osipenko

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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

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

hw/core/ptimer.c +2 -0

index 7e6fc2d..76ebe9b 100644
@@ -190,6 +190,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) {

@@ -201,6 +202,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) {

Signature · 1 line

2.8.3

Dmitry Osipenko

author

2016-05-28 14:13 UTC #7 lore raw

Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter = 0 by introducing new policy feature

In reply to Dmitry Osipenko

Just a small note:

It's more like RFC. I don't think 8 bits would be enough for all possible policies and I don't know whether it is possible to maintain backward compatibility by keeping VMSD .minimum_version_id. "loadvm" refused to accept old ptimer VMState, I haven't looked at it much.

Signature · 1 line

Dmitry

Peter Maydell

2016-06-06 10:10 UTC #8 lore raw

Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter = 0 by introducing new policy feature

In reply to Dmitry Osipenko

Quoted 27 lines · Dmitry Osipenko

Currently ptimer prints error message and clears enable flag for an arming timer that has delta = load = 0. That actually is a valid case for most of the timers, like instant IRQ trigger for oneshot timer or continuous in periodic mode. There are different possible policies of how particular timer could interpret setting counter to 0, like:
1) Immediate stop with triggering the interrupt in oneshot mode. Immediate wraparound with triggering the interrupt in continuous mode.
2) Immediate stop without triggering the interrupt in oneshot mode. Immediate wraparound without triggering the interrupt in continuous mode.
3) Stopping with triggering the interrupt after one period in oneshot mode. Wraparound with triggering the interrupt in continuous mode after one period.
4) Stopping without triggering the interrupt after one period in oneshot mode. Wraparound without triggering the interrupt in continuous mode after one period.
As well as handling oneshot/continuous modes differently.
Given that, currently, running the timer with counter/period equal to 0 is treated as a error case, it's not obvious what policies should be supported by ptimer. Let's implement the third policy for now, as it is known to be used by the ARM MPTimer.

Quoted 4 lines

Explicitly forbid running with counter/period == 0 in all cases by aborting QEMU execution instead of printing the error message.
Bump VMSD version since a new VMState member is added.

I'm afraid you can't do that -- the ptimer code is used by a lot of platforms including some which do not permit migration compatibility breaks (at least sparc, for instance).

If you want to add this feature you need to at least make it seamlessly support migration from the old version using a vmstate subsection.

Better would be to make the policy a QOM property -- then it is constant for the lifetime of the timer and doesn't need to be migrated at all. (There's no reason to support timers which change policy at runtime, I hope). The default property value should be "same behaviour as previously".

This whole change would be more solidly supported if it came with a clear description of the bug being fixed (ie what device is currently using the default-but-wrong behaviour and is going to be fixed by setting a different policy).

Quoted 20 lines

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

---
 hw/core/ptimer.c    | 53 +++++++++++++++++++++++++++++++----------------------
 include/hw/ptimer.h |  6 ++++++
 2 files changed, 37 insertions(+), 22 deletions(-)

hw/core/ptimer.c +1 -0

index 05b0c27..9bc70f5 100644
@@ -21,6 +21,7 @@ struct ptimer_state
     int64_t period;
     int64_t last_event;
     int64_t next_event;
+    uint8_t policy;
     QEMUBH *bh;
     QEMUTimer *timer;
 };

@@ -37,15 +38,16 @@ static void ptimer_reload(ptimer_state *s)
 {

hw/core/ptimer.c static void ptimer_reload(ptimer_state *s) · line 45

Quoted 10 lines

hw/core/ptimer.c +4 -3

     uint32_t period_frac = s->period_frac;
     uint64_t period = s->period;
+    uint64_t delta = MAX(1, s->delta);
 
-    if (s->delta == 0) {
-        ptimer_trigger(s);
-        s->delta = s->limit;
+    if (s->delta == 0 && s->policy == UNIMPLEMENTED) {
+        hw_error("ptimer: Running with counter=0 is unimplemented by " \
+                 "this timer, fix it!\n");

hw_error() is effectively a verbose assert. Have you checked that all the users of ptimer do the right thing? If not, we can't assert() until we've fixed them all. Similarly below. Note that what we had previously was just a warning-and-continue

Quoted 69 lines

hw/core/ptimer.c +16 ~~-13~~

     }
-    if (s->delta == 0 || s->period == 0) {
-        fprintf(stderr, "Timer with period zero, disabling\n");
-        s->enabled = 0;
-        return;
+
+    if (period == 0) {
+        hw_error("ptimer: Timer tries to run with period=0, behaviour is " \
+                 "undefined, fix it!\n");
     }
 
     /*

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

@@ -73,27 +75,30 @@ 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->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)
 {
     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;

hw/core/ptimer.c void ptimer_run(ptimer_state *s, int oneshot) · line 174

Quoted 9 lines

hw/core/ptimer.c +1 -4

@@ -165,11 +170,8 @@ 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;
+

Stray extra blank line.

hw/core/ptimer.c void ptimer_set_period(ptimer_state *s, int64_t period) · line 208

Quoted 8 lines

hw/core/ptimer.c +1 -0

     if (was_disabled) {
         s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         ptimer_reload(s);

@@ -203,6 +205,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);

This probably needs an LL suffix like the others below.

Quoted 34 lines

hw/core/ptimer.c +8 -2

     s->delta = ptimer_get_count(s);
     s->period = 1000000000ll / freq;
     s->period_frac = (1000000000ll << 32) / freq;

@@ -230,10 +233,15 @@ uint64_t ptimer_get_limit(ptimer_state *s)
     return s->limit;
 }
 
+void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy)
+{
+    s->policy = policy;
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(enabled, ptimer_state),
         VMSTATE_UINT64(limit, ptimer_state),

@@ -243,6 +251,7 @@ const VMStateDescription vmstate_ptimer = {
         VMSTATE_INT64(last_event, ptimer_state),
         VMSTATE_INT64(next_event, ptimer_state),
         VMSTATE_TIMER_PTR(timer, ptimer_state),
+        VMSTATE_UINT8(policy, ptimer_state),
         VMSTATE_END_OF_LIST()
     }
 };

include/hw/ptimer.h +0 -0

index e397db5..221f591 100644

include/hw/ptimer.h line 17

Quoted 6 lines

include/hw/ptimer.h +3 -0

@@ -12,6 +12,11 @@
 #include "qemu/timer.h"
 #include "migration/vmstate.h"
 
+enum ptimer_policy {
+    UNIMPLEMENTED,
+    SET_CNT_TO_0_TRIGGERS_IRQ_AFTER_ONE_PERIOD,

Given that enums aren't namespaced, "UNIMPLEMENTED" is too broad; you need some PTIMER_ prefixes. Also the second one of these is too long a name.

include/hw/ptimer.h uint64_t ptimer_get_count(ptimer_state *s); · line 33

Quoted 10 lines

include/hw/ptimer.h +3 -0

+};
+
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;
 typedef void (*ptimer_cb)(void *opaque);

@@ -25,6 +30,7 @@ uint64_t ptimer_get_count(ptimer_state *s);
 void ptimer_set_count(ptimer_state *s, uint64_t count);
 void ptimer_run(ptimer_state *s, int oneshot);
 void ptimer_stop(ptimer_state *s);
+void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy);

Documentation comment headers for new global functions, please.

include/hw/ptimer.h uint64_t ptimer_get_count(ptimer_state *s); · line 35

Quoted 1 line

include/hw/ptimer.h +0 -0

 extern const VMStateDescription vmstate_ptimer;

thanks
-- PMM

Peter Maydell

Applied

2016-06-06 13:44 UTC #12 lore raw

In reply to Dmitry Osipenko

Quoted 11 lines · Dmitry Osipenko

Hello,
Current QEMU ARM MPTimer device model provides only a certain subset of the emulation behavior, so 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.
Emulation behavior is verified against the real HW by running specially crafted MPTimer tests in both icount and non-icount modes:
https://gist.github.com/digetx/dbd46109503b1a91941a

I've applied patches 1-5 to target-arm.next; thanks.

-- PMM

Dmitry Osipenko

author

2016-06-06 17:25 UTC #9 lore raw

Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter = 0 by introducing new policy feature

In reply to Peter Maydell

Quoted 7 lines · Peter Maydell

Quoted 1 line · Dmitry Osipenko
Bump VMSD version since a new VMState member is added.
I'm afraid you can't do that -- the ptimer code is used by a lot of platforms including some which do not permit migration compatibility breaks (at least sparc, for instance).

Quoted 9 lines

If you want to add this feature you need to at least make it seamlessly support migration from the old version using a vmstate subsection.
Better would be to make the policy a QOM property -- then it is constant for the lifetime of the timer and doesn't need to be migrated at all. (There's no reason to support timers which change policy at runtime, I hope). The default property value should be "same behaviour as previously".

QOM property should fit well, thanks for the suggestion.

Quoted 5 lines

This whole change would be more solidly supported if it came with a clear description of the bug being fixed (ie what device is currently using the default-but-wrong behaviour and is going to be fixed by setting a different policy).

I'll extend the commit description, thanks for the note.

Quoted 7 lines

hw/core/ptimer.c static void ptimer_reload(ptimer_state *s) · line 45
Quoted 3 lines
hw/core/ptimer.c +3 -0
+    if (s->delta == 0 && s->policy == UNIMPLEMENTED) {
+        hw_error("ptimer: Running with counter=0 is unimplemented by " \
+                 "this timer, fix it!\n");
hw_error() is effectively a verbose assert. Have you checked that all the users of ptimer do the right thing? If not, we can't assert() until we've fixed them all. Similarly below.

No, I haven't examined all the users thoroughly, will take look through them for this issue.

In my opinion it's better to know that your QEMU machine doesn't behave correctly and just fix it.

Quoted 2 lines

Note that what we had previously was just a warning-and-continue

Yes, and it was literally "forever" there: "committed on 23 May 2007". Seems just nobody cared to fix/improve it since then.

I'll keep old "warning-and-continue" in this patch and derive "hw_error" change into a separate patch.

Quoted 4 lines

hw/core/ptimer.c +0 -0

@@ -165,11 +170,8 @@ void ptimer_run(ptimer_state *s, int oneshot)
 {
     bool was_disabled = !s->enabled;

Quoted 9 lines

hw/core/ptimer.c void ptimer_run(ptimer_state *s, int oneshot) · line 174
Quoted 6 lines
hw/core/ptimer.c +1 -4
-    if (was_disabled && s->period == 0) {
-        fprintf(stderr, "Timer with period zero, disabling\n");
-        return;
-    }
     s->enabled = oneshot ? 2 : 1;
+
Stray extra blank line.

Ack.

Quoted 8 lines

include/hw/ptimer.h line 17
Quoted 3 lines
include/hw/ptimer.h +3 -0
+enum ptimer_policy {
+    UNIMPLEMENTED,
+    SET_CNT_TO_0_TRIGGERS_IRQ_AFTER_ONE_PERIOD,
Given that enums aren't namespaced, "UNIMPLEMENTED" is too broad; you need some PTIMER_ prefixes. Also the second one of these is too long a name.

Ack.

Quoted 4 lines

Quoted 4 lines
include/hw/ptimer.h +2 -0
+};
+
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;

Quoted 9 lines

include/hw/ptimer.h uint64_t ptimer_get_count(ptimer_state *s); · line 33
Quoted 6 lines
typedef void (*ptimer_cb)(void *opaque);
include/hw/ptimer.h +1 -0
@@ -25,6 +30,7 @@ uint64_t ptimer_get_count(ptimer_state *s);
 void ptimer_set_count(ptimer_state *s, uint64_t count);
 void ptimer_run(ptimer_state *s, int oneshot);
 void ptimer_stop(ptimer_state *s);
+void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy);
Documentation comment headers for new global functions, please.

Ack. Thanks a lot for the review!

Signature · 1 line

Dmitry