* [Qemu-devel] [PATCH v6 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout
2015-10-23 20:03 [Qemu-devel] [PATCH v6 0/2] PTimer fix and ARM MPTimer conversion Dmitry Osipenko
@ 2015-10-23 20:03 ` Dmitry Osipenko
2015-10-23 20:03 ` [Qemu-devel] [PATCH v6 2/2] arm_mptimer: Convert to use ptimer Dmitry Osipenko
1 sibling, 0 replies; 3+ messages in thread
From: Dmitry Osipenko @ 2015-10-23 20:03 UTC (permalink / raw)
To: QEMU Developers; +Cc: Peter Maydell, Peter Crosthwaite
Multiple issues here related to the timer with a corrected .limit value:
1) ptimer_get_count() returns incorrect value for the disabled timer after
loading the counter with a small value, because corrected limit value
is used instead of the original.
For instance:
1) ptimer_stop(t)
2) ptimer_set_period(t, 1)
3) ptimer_set_limit(t, 0, 1)
4) ptimer_get_count(t) <-- would return 10000 instead of 0
2) ptimer_get_count() might return incorrect value for the timer running
with a corrected 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 correcting 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 moving timeout value correction to the
ptimer_reload(). Instead of changing limit value, set new ptimer struct
member "next_event_corrected" when next_event was corrected and make
ptimer_get_count() always return 1 for the timer running with corrected
next_event value. Bump VMSD version since ptimer VM state is changed, but
keep .minimum_version_id as it is backward compatible.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
hw/core/ptimer.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 8437bd6..0732b20 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -19,6 +19,7 @@ struct ptimer_state
int64_t period;
int64_t last_event;
int64_t next_event;
+ uint8_t next_event_corrected;
QEMUBH *bh;
QEMUTimer *timer;
};
@@ -48,6 +49,22 @@ static void ptimer_reload(ptimer_state *s)
if (s->period_frac) {
s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
}
+
+ /*
+ * Artificially limit timeout 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.
+ */
+
+ s->next_event_corrected = !!(s->next_event - s->last_event < 10000);
+
+ if (s->next_event_corrected) {
+ s->next_event = s->last_event + 10000;
+ }
+
timer_mod(s->timer, s->next_event);
}
@@ -76,6 +93,9 @@ uint64_t ptimer_get_count(ptimer_state *s)
/* Prevent timer underflowing if it should already have
triggered. */
counter = 0;
+ } if (s->next_event_corrected) {
+ /* Always return 1 when timer expire value was corrected. */
+ counter = 1;
} else {
uint64_t rem;
uint64_t div;
@@ -180,19 +200,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;
@@ -204,7 +211,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
const VMStateDescription vmstate_ptimer = {
.name = "ptimer",
- .version_id = 1,
+ .version_id = 2,
.minimum_version_id = 1,
.fields = (VMStateField[]) {
VMSTATE_UINT8(enabled, ptimer_state),
@@ -214,6 +221,7 @@ const VMStateDescription vmstate_ptimer = {
VMSTATE_INT64(period, ptimer_state),
VMSTATE_INT64(last_event, ptimer_state),
VMSTATE_INT64(next_event, ptimer_state),
+ VMSTATE_UINT8(next_event_corrected, ptimer_state),
VMSTATE_TIMER_PTR(timer, ptimer_state),
VMSTATE_END_OF_LIST()
}
--
2.6.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [Qemu-devel] [PATCH v6 2/2] arm_mptimer: Convert to use ptimer
2015-10-23 20:03 [Qemu-devel] [PATCH v6 0/2] PTimer fix and ARM MPTimer conversion Dmitry Osipenko
2015-10-23 20:03 ` [Qemu-devel] [PATCH v6 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout Dmitry Osipenko
@ 2015-10-23 20:03 ` Dmitry Osipenko
1 sibling, 0 replies; 3+ messages in thread
From: Dmitry Osipenko @ 2015-10-23 20:03 UTC (permalink / raw)
To: QEMU Developers; +Cc: Peter Maydell, Peter Crosthwaite
Current ARM MPTimer implementation uses QEMUTimer for the actual timer,
this implementation isn't complete and mostly tries to duplicate of what
generic ptimer is already doing fine.
Conversion to ptimer brings the following benefits and fixes:
- Simple timer pausing implementation
- Fixes counter value preservation after stopping the timer
- Code simplification and reduction
Bump VMSD to version 3, since VMState is changed and is not compatible
with the previous implementation.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
hw/timer/arm_mptimer.c | 110 ++++++++++++++++++-----------------------
include/hw/timer/arm_mptimer.h | 4 +-
2 files changed, 49 insertions(+), 65 deletions(-)
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 3e59c2a..c06da5e 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -19,8 +19,9 @@
* with this program; if not, see <http://www.gnu.org/licenses/>.
*/
+#include "hw/ptimer.h"
#include "hw/timer/arm_mptimer.h"
-#include "qemu/timer.h"
+#include "qemu/main-loop.h"
#include "qom/cpu.h"
/* This device implements the per-cpu private timer and watchdog block
@@ -47,28 +48,10 @@ static inline uint32_t timerblock_scale(TimerBlock *tb)
return (((tb->control >> 8) & 0xff) + 1) * 10;
}
-static void timerblock_reload(TimerBlock *tb, int restart)
-{
- if (tb->count == 0) {
- return;
- }
- if (restart) {
- tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- }
- tb->tick += (int64_t)tb->count * timerblock_scale(tb);
- timer_mod(tb->timer, tb->tick);
-}
-
static void timerblock_tick(void *opaque)
{
TimerBlock *tb = (TimerBlock *)opaque;
tb->status = 1;
- if (tb->control & 2) {
- tb->count = tb->load;
- timerblock_reload(tb, 0);
- } else {
- tb->count = 0;
- }
timerblock_update_irq(tb);
}
@@ -76,21 +59,11 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
unsigned size)
{
TimerBlock *tb = (TimerBlock *)opaque;
- int64_t val;
switch (addr) {
case 0: /* Load */
return tb->load;
case 4: /* Counter. */
- if (((tb->control & 1) == 0) || (tb->count == 0)) {
- return 0;
- }
- /* Slow and ugly, but hopefully won't happen too often. */
- val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- val /= timerblock_scale(tb);
- if (val < 0) {
- val = 0;
- }
- return val;
+ return ptimer_get_count(tb->timer);
case 8: /* Control. */
return tb->control;
case 12: /* Interrupt status. */
@@ -100,6 +73,19 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
}
}
+static void timerblock_run(TimerBlock *tb, uint64_t count, int set_count)
+{
+ if (set_count) {
+ if (((tb->control & 3) == 3) && (count == 0)) {
+ count = tb->load;
+ }
+ ptimer_set_count(tb->timer, count);
+ }
+ if ((tb->control & 1) && (count != 0)) {
+ ptimer_run(tb->timer, !(tb->control & 2));
+ }
+}
+
static void timerblock_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
@@ -108,32 +94,34 @@ static void timerblock_write(void *opaque, hwaddr addr,
switch (addr) {
case 0: /* Load */
tb->load = value;
- /* Fall through. */
- case 4: /* Counter. */
- if ((tb->control & 1) && tb->count) {
- /* Cancel the previous timer. */
- timer_del(tb->timer);
+ /* Setting load to 0 stops the timer. */
+ if (tb->load == 0) {
+ ptimer_stop(tb->timer);
}
- tb->count = value;
- if (tb->control & 1) {
- timerblock_reload(tb, 1);
+ ptimer_set_limit(tb->timer, tb->load, 1);
+ timerblock_run(tb, tb->load, 0);
+ break;
+ case 4: /* Counter. */
+ /* Setting counter to 0 stops the one-shot timer. */
+ if (!(tb->control & 2) && (value == 0)) {
+ ptimer_stop(tb->timer);
}
+ timerblock_run(tb, value, 1);
break;
case 8: /* Control. */
old = tb->control;
tb->control = value;
- if (value & 1) {
- if ((old & 1) && (tb->count != 0)) {
- /* Do nothing if timer is ticking right now. */
- break;
- }
- if (tb->control & 2) {
- tb->count = tb->load;
- }
- timerblock_reload(tb, 1);
- } else if (old & 1) {
- /* Shutdown the timer. */
- timer_del(tb->timer);
+ /* Timer mode switch requires ptimer to be stopped. */
+ if ((old & 3) != (tb->control & 3)) {
+ ptimer_stop(tb->timer);
+ }
+ if (!(tb->control & 1)) {
+ break;
+ }
+ ptimer_set_period(tb->timer, timerblock_scale(tb));
+ if ((old & 3) != (tb->control & 3)) {
+ value = ptimer_get_count(tb->timer);
+ timerblock_run(tb, value, 1);
}
break;
case 12: /* Interrupt status. */
@@ -184,13 +172,12 @@ static const MemoryRegionOps timerblock_ops = {
static void timerblock_reset(TimerBlock *tb)
{
- tb->count = 0;
tb->load = 0;
tb->control = 0;
tb->status = 0;
- tb->tick = 0;
if (tb->timer) {
- timer_del(tb->timer);
+ ptimer_stop(tb->timer);
+ ptimer_set_limit(tb->timer, 0, 1);
}
}
@@ -235,7 +222,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
*/
for (i = 0; i < s->num_cpu; i++) {
TimerBlock *tb = &s->timerblock[i];
- tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb);
+ QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
+ tb->timer = ptimer_init(bh);
sysbus_init_irq(sbd, &tb->irq);
memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
"arm_mptimer_timerblock", 0x20);
@@ -245,26 +233,24 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
static const VMStateDescription vmstate_timerblock = {
.name = "arm_mptimer_timerblock",
- .version_id = 2,
- .minimum_version_id = 2,
+ .version_id = 3,
+ .minimum_version_id = 3,
.fields = (VMStateField[]) {
- VMSTATE_UINT32(count, TimerBlock),
VMSTATE_UINT32(load, TimerBlock),
VMSTATE_UINT32(control, TimerBlock),
VMSTATE_UINT32(status, TimerBlock),
- VMSTATE_INT64(tick, TimerBlock),
- VMSTATE_TIMER_PTR(timer, TimerBlock),
+ VMSTATE_PTIMER(timer, TimerBlock),
VMSTATE_END_OF_LIST()
}
};
static const VMStateDescription vmstate_arm_mptimer = {
.name = "arm_mptimer",
- .version_id = 2,
- .minimum_version_id = 2,
+ .version_id = 3,
+ .minimum_version_id = 3,
.fields = (VMStateField[]) {
VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
- 2, vmstate_timerblock, TimerBlock),
+ 3, vmstate_timerblock, TimerBlock),
VMSTATE_END_OF_LIST()
}
};
diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
index b34cba0..93db61b 100644
--- a/include/hw/timer/arm_mptimer.h
+++ b/include/hw/timer/arm_mptimer.h
@@ -27,12 +27,10 @@
/* State of a single timer or watchdog block */
typedef struct {
- uint32_t count;
uint32_t load;
uint32_t control;
uint32_t status;
- int64_t tick;
- QEMUTimer *timer;
+ struct ptimer_state *timer;
qemu_irq irq;
MemoryRegion iomem;
} TimerBlock;
--
2.6.1
^ permalink raw reply related [flat|nested] 3+ messages in thread