hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()
PATCH v5 1/2 hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()
ptimer_get_count() returns incorrect value for the disabled timer after reloading the counter with a small value, because corrected limit value is used instead of the original.
For instance:
1) ptimer_stop(t)
2) ptimer_set_period(t, 1)
3) ptimer_set_limit(t, 0, 1)
4) ptimer_get_count(t) <-- would return 10000 instead of 0
--- hw/core/ptimer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
hw/core/ptimer.c +3 -1
@@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)count = limit. */ void ptimer_set_limit (ptimer_state *s,uint64_t limit,int reload) { +uint64_t count = limit; +/* * Artificially limit timeout rate to something * achievable under QEMU. Otherwise, QEMU spends all
@@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) s->limit = limit;if (reload) - s->delta = limit; + s->delta = count;if (s->enabled && reload) { s->next_event =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );ptimer_reload (s);
Signature · 1 line
2.6.0
PATCH v5 2/2 arm_mptimer: Convert to use ptimer
Current ARM MPTimer implementation uses QEMUTimer for the actual timer, this implementation isn't complete and mostly tries to duplicate of what generic ptimer is already doing fine.
Conversion to ptimer brings the following benefits and fixes:
- Simple timer pausing implementation
- Fixes counter value preservation after stopping the timer
- Code simplification and reduction
Bump VMSD to version 3, since VMState is changed and is not compatible with the previuos implementation.
---
Changelog:
V2: Fixed changing periodic timer counter value "on the fly". I added a
test to the gist to cover that issue.
V3: Fixed starting the timer with load = 0 and counter != 0, added tests
to the gist for this issue. Changed vmstate version for all VMSD's, since loadvm doesn't check version of nested VMSD.
V4: Fixed spurious IT bit set for the timer starting in the periodic mode
with counter = 0. Test added.
V5: Code cleanup, now depends on ptimer_set_limit() fix.
Tests: https://gist.github.com/digetx/dbd46109503b1a91941a
hw/timer/arm_mptimer.c | 110 ++++++++++++++++++----------------------- include/hw/timer/arm_mptimer.h | 4 +- 2 files changed, 49 insertions(+), 65 deletions(-)
hw/timer/arm_mptimer.c +48 -62
@@ -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 () } };
include/hw/timer/arm_mptimer.h +1 -3
@@ -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 ;
Signature · 1 line
2.6.0
Quoted 29 lines · Dmitry Osipenko
ptimer_get_count()returns incorrect value for the disabled timer after reloading the counter with a small value, because corrected limit value is used instead of the original.For instance:
1)ptimer_stop(t)
2)ptimer_set_period(t, 1)
3)ptimer_set_limit(t, 0, 1)
4)ptimer_get_count(t)<-- would return 10000 instead of 0Signed-off-by Dmitry Osipenko <[email protected]>--- hw/core/ptimer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)hw/core/ptimer.c +2
-0@@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)count = limit. */ void ptimer_set_limit (ptimer_state *s,uint64_t limit,int reload) { +uint64_t count = limit; +/* * Artificially limit timeout rate to something * achievable under QEMU. Otherwise, QEMU spends all
hw/core/ptimer.c void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) · line 203
Quoted 8 lines
hw/core/ptimer.c +1
-1@@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) s->limit = limit;if (reload) - s->delta = limit; + s->delta = count;if (s->enabled && reload) { s->next_event =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );ptimer_reload (s);
Doesn't this defeat the rate limiting if the timer is enabled, though? ptimer_reload() sets the underlying timer based on s->delta, so if s->delta isn't the rate-limited value then the timer will be incorrectly set to a very close-in value.
I think we'll return "incorrect" values from ptimer_get_count() in the "counter is running" case too, because we calculate those by looking at when the underlying timer's due to expire, and we set the expiry time based on the adjusted value.
What's the underlying model we should have for what values we return from reading the count if we've decided to adjust the actual timer expiry with the rate limit? Should the count go down from the specified value and then just hang at 1 until the extended timer expiry time hits? Or something else? Clearly defining what we want to happen ought to make it easier to review attempts to fix it...
(Calling ptimer_set_count() also bypasses the ratelimiting at the moment, incidentally.)
thanks
-- PMM
19.10.2015 20:06, Peter Maydell пишет:
Quoted 31 lines · Dmitry Osipenko
Quoted 30 lines
ptimer_get_count()returns incorrect value for the disabled timer after reloading the counter with a small value, because corrected limit value is used instead of the original.For instance:
1)ptimer_stop(t)
2)ptimer_set_period(t, 1)
3)ptimer_set_limit(t, 0, 1)
4)ptimer_get_count(t)<-- would return 10000 instead of 0Signed-off-by Dmitry Osipenko <[email protected]>--- hw/core/ptimer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)hw/core/ptimer.c +2
-0@@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)count = limit. */ void ptimer_set_limit (ptimer_state *s,uint64_t limit,int reload) { +uint64_t count = limit; +/* * Artificially limit timeout rate to something * achievable under QEMU. Otherwise, QEMU spends all @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
Quoted 13 lines
hw/core/ptimer.c void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) · line 203
Quoted 7 lines
hw/core/ptimer.c +1
-1s->limit = limit;if (reload) - s->delta = limit; + s->delta = count;if (s->enabled && reload) { s->next_event =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );ptimer_reload (s);Doesn't this defeat the rate limiting if the timer is enabled, though?
ptimer_reload()sets the underlying timer based ons->delta, so ifs->deltaisn't the rate-limited value then the timer will be incorrectly set to a very close-in value.
Yes, it defeats the rate limiting for the first timer expire. As I understand, the idea of the rate limit correction aims periodic timer only.
"Otherwise, QEMU spends all its time generating timer interrupts, and there is no forward progress.", as stated in the comment.
I think it's not a problem to get "instant" timer trigger for the first expire.
Quoted 5 lines
I think we'll return "incorrect" values from
ptimer_get_count()in the "counter is running" case too, because we calculate those by looking at when the underlying timer's due to expire, and we set the expiry time based on the adjusted value.
That's a good point.
Quoted 8 lines
What's the underlying model we should have for what values we return from reading the count if we've decided to adjust the actual timer expiry with the rate limit? Should the count go down from the specified value and then just hang at 1 until the extended timer expiry time hits? Or something else? Clearly defining what we want to happen ought to make it easier to review attempts to fix it...
What about the following:
Add additional ptimer struct member, say "limit_corrected", to check whether the limit was corrected or not.
ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
{
.limit_corrected = 0;
// on the limit correction:
.limit_corrected = (limit == 0) ? 1 : 2;
limit = 10000 / s->period;
}ptimer_get_count()
{
if (enabled) {
if (expired || .limit_corrected == 1) {
counter = 0;
} else if (.limit_corrected == 2) {
counter = 1;
} else {
// do the counter calculations ...
}
}
}and clear .limit_corrected on the one-shot timer start. That would bump ptimer VMSD version, but keep .minimum_version_id.
Quoted 3 lines
(Calling
ptimer_set_count()also bypasses the ratelimiting at the moment, incidentally.)
Should be okay if my "aiming the periodic timer only" thought is correct.
Quoted 3 lines
thanks
-- PMM
Signature · 1 line
Dmitry
Quoted 8 lines · Dmitry Osipenko
19.10.2015 20:06, Peter Maydell пишет:
Quoted 6 lines
Doesn't this defeat the rate limiting if the timer is enabled, though?
ptimer_reload()sets the underlying timer based ons->delta, so ifs->deltaisn't the rate-limited value then the timer will be incorrectly set to a very close-in value.
Quoted 8 lines
Yes, it defeats the rate limiting for the first timer expire. As I understand, the idea of the rate limit correction aims periodic timer only.
"Otherwise, QEMU spends all its time generating timer interrupts, and there is no forward progress.", as stated in the comment.
I think it's not a problem to get "instant" timer trigger for the first expire.
Yes, that makes sense. If you trigger a one-shot timer then we should prefer accuracy, on the assumption that the next setting of the one-shot timer will be further in the future. (The guest could in theory still starve us of forward progress with an infinite series of near-future one-shot timers, but we can worry about that if we see it.)
Quoted 22 lines
Quoted 5 lines
I think we'll return "incorrect" values from
ptimer_get_count()in the "counter is running" case too, because we calculate those by looking at when the underlying timer's due to expire, and we set the expiry time based on the adjusted value.That's a good point.
Quoted 8 lines
What's the underlying model we should have for what values we return from reading the count if we've decided to adjust the actual timer expiry with the rate limit? Should the count go down from the specified value and then just hang at 1 until the extended timer expiry time hits? Or something else? Clearly defining what we want to happen ought to make it easier to review attempts to fix it...
What about the following:
Add additional ptimer struct member, say "
limit_corrected", to check whether the limit was corrected or not.
Quoted 6 lines
ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload){ .limit_corrected = 0; // on the limit correction: .limit_corrected = (limit == 0) ? 1 : 2;
It's a bit confusing that a field that sounds like it ought to be a bool is taking values 0/1/2.
Quoted 4 lines
limit = 10000 / s->period; }
ptimer_get_count()
Quoted 11 lines
{
if (enabled) {
if (expired || .limit_corrected == 1) {
counter = 0;
} else if (.limit_corrected == 2) {
counter = 1;
} else {
// do the counter calculations ...
}
}
}Not completely sure I understand this. A comment about what the various states limit_corrected can be in might help.
Quoted 2 lines
and clear .limit_corrected on the one-shot timer start. That would bump ptimer VMSD version, but keep .minimum_version_id.
Given how widely ptimer is used, you should probably handle it via a subsection, not by a version number bump.
thanks
-- PMM
19.10.2015 23:01, Dmitry Osipenko пишет:
Quoted 27 lines
What about the following:
Add additional ptimer struct member, say "
limit_corrected", to check whether the limit was corrected or not.
ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload){ .limit_corrected = 0; // on the limit correction: .limit_corrected = (limit == 0) ? 1 : 2; limit = 10000 / s->period; }
ptimer_get_count(){ if (enabled) { if (expired || .limit_corrected == 1) { counter = 0; } else if (.limit_corrected == 2) { counter = 1; } else { // do the counter calculations ... } } }
Quoted 3 lines
and clear .limit_corrected on the one-shot timer start. That would bump ptimer VMSD version, but keep .minimum_version_id.
However, that would break set_counter(). Also limit should be corrected on period / freq change. I'll work more on it.
Signature · 1 line
Dmitry