PTimer fixes and ARM MPTimer conversion
v8 cover PTimer fixes and ARM MPTimer conversion
Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
V2: Fixed changing periodic timer counter value "on the fly". I added a
test to the gist to cover that issue.
V3: Fixed starting the timer with load = 0 and counter != 0, added tests
to the gist for this issue. Changed vmstate version for all VMSD's, since loadvm doesn't check version of nested VMSD.
V4: Fixed spurious IT bit set for the timer starting in the periodic mode
with counter = 0. Test added.
V5: Code cleanup, now depends on ptimer_set_limit() fix.
V6: No code change, added test to check ptimer_get_count() with corrected
.limit value.
V7: No change.
V8: No change.
ARM MPTimer tests: https://gist.github.com/digetx/dbd46109503b1a91941a
Patch for ptimer is introduced since V5 of "ARM MPTimer conversion".
Changelog for the "ptimer fixes" patch:
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.
Dmitry Osipenko (4):
hw/ptimer: Fix issues caused by the adjusted timer limit value hw/ptimer: Perform tick and counter wrap around if timer already
expired
hw/ptimer: Update .delta on period/freq change
arm_mptimer: Convert to use ptimer
hw/core/ptimer.c | 94 ++++++++++++++++++++++++----------- hw/timer/arm_mptimer.c | 110 ++++++++++++++++++----------------------- include/hw/timer/arm_mptimer.h | 4 +- 3 files changed, 115 insertions(+), 93 deletions(-)
Signature · 1 line
2.6.4
PATCH v8 1/4 hw/ptimer: Fix issues caused by the adjusted timer limit value
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. Do the adjust for periodic timer only.
--- hw/core/ptimer.c | 59 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 23 deletions(-)
hw/core/ptimer.c +36 -23
@@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)static void ptimer_reload (ptimer_state *s) { -if (s->delta ==0 ) { +uint32_t period_frac = s->period_frac; +uint64_t period = s->period; +uint64_t delta = s->delta; +uint64_t limit = s->limit; + +if (delta ==0 ) {ptimer_trigger (s); - s->delta = s->limit; + delta = limit; } -if (s->delta ==0 || s->period ==0 ) { +if (delta ==0 || period ==0 ) {fprintf (stderr,"Timer with period zero, disabling\n" ); s->enabled =0 ;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 ) && (limit * period <10000 )) { + period =10000 / limit; + 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 + delta * period; +if (period_frac) { + s->next_event += ((int64_t )period_frac * delta) >>32 ; }timer_mod (s->timer, s->next_event); }
@@ -82,6 +101,8 @@ 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;/* We need to divide time by period, where time is stored in rem (64-bit integer) and period is stored in period/period_frac
@@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ +if ((s->enabled ==1 ) && (s->limit * period <10000 )) { + period =10000 / s->limit; + period_frac =0 ; + } + rem = s->next_event - now; - div = s->period; + div = period; clz1 =clz64 (rem); clz2 =clz64 (div);
@@ -103,13 +129,13 @@ uint64_t ptimer_get_count(ptimer_state *s) rem <<= shift; div <<= shift;if (shift >=32 ) { - div |= ((uint64_t )s->period_frac << (shift -32 )); + div |= ((uint64_t )period_frac << (shift -32 )); }else {if (shift !=0 ) - div |= (s->period_frac >> (32 - shift)); + div |= (period_frac >> (32 - shift));/* Look at remaining bits of period_frac and round div up if necessary. */ -if ((uint32_t )(s->period_frac << shift)) +if ((uint32_t )(period_frac << shift)) div +=1 ; } counter = rem / div;
@@ -181,19 +207,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.6.4
Re: [Qemu-arm] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value
Quoted 44 lines · 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 02)
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 > 103) Neither
ptimer_set_period()norptimer_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 correctionFix all of the above issues by adjusting timer period instead of the limit. Do the adjust for periodic timer only.
Signed-off-by Dmitry Osipenko <[email protected]>--- hw/core/ptimer.c | 59 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 23 deletions(-)hw/core/ptimer.c +0
-0
hw/core/ptimer.c static void ptimer_trigger(ptimer_state *s) · line 41
Quoted 9 lines
hw/core/ptimer.c +5
-1@@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)static void ptimer_reload (ptimer_state *s) { -if (s->delta ==0 ) { +uint32_t period_frac = s->period_frac; +uint64_t period = s->period; +uint64_t delta = s->delta; +uint64_t limit = s->limit; +
Localising these variables is out of scope of the change. I think you can just use s->foo and if you want to cleanup, it should be a separate patch.
Quoted 16 lines
hw/core/ptimer.c +7
-2+if (delta ==0 ) {ptimer_trigger (s); - s->delta = s->limit; + delta = limit; } -if (s->delta ==0 || s->period ==0 ) { +if (delta ==0 || period ==0 ) {fprintf (stderr,"Timer with period zero, disabling\n" ); s->enabled =0 ;return ; } +/* +* Artificially limit timeout rate to something +* achievable under QEMU. Otherwise, QEMU spends all +* its time generating timer interrupts, and there
hw/core/ptimer.c static void ptimer_trigger(ptimer_state *s) · line 65
Quoted 10 lines
hw/core/ptimer.c +10
-0+* is no forward progress. +* About ten microseconds is the fastest that really works +* on the current generation of host machines. +*/ + +if ((s->enabled ==1 ) && (limit * period <10000 )) { + period =10000 / limit; + period_frac =0 ; + } +
I think it should be ok to just update s->period and s->period_frac ...
Quoted 20 lines
hw/core/ptimer.c +5
-3s->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 + delta * period; +if (period_frac) { + s->next_event += ((int64_t )period_frac * delta) >>32 ; }timer_mod (s->timer, s->next_event); }@@ -82,6 +101,8 @@ 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;/* We need to divide time by period, where time is stored in rem (64-bit integer) and period is stored in period/period_frac
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 121
Quoted 8 lines
hw/core/ptimer.c +5
-0@@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ +if ((s->enabled ==1 ) && (s->limit * period <10000 )) { + period =10000 / s->limit; + period_frac =0 ; + } +
... and then this (and the local variables) become obsolete. Can get_count() blindly use the period and period_frac as used by ptimer_reload?
Quoted 27 lines
hw/core/ptimer.c +4
-4rem = s->next_event - now; - div = s->period; + div = period; clz1 =clz64 (rem); clz2 =clz64 (div);@@ -103,13 +129,13 @@ uint64_t ptimer_get_count(ptimer_state *s) rem <<= shift; div <<= shift;if (shift >=32 ) { - div |= ((uint64_t )s->period_frac << (shift -32 )); + div |= ((uint64_t )period_frac << (shift -32 )); }else {if (shift !=0 ) - div |= (s->period_frac >> (32 - shift)); + div |= (period_frac >> (32 - shift));/* Look at remaining bits of period_frac and round div up if necessary. */ -if ((uint32_t )(s->period_frac << shift)) +if ((uint32_t )(period_frac << shift)) div +=1 ; } counter = rem / div;@@ -181,19 +207,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) {
hw/core/ptimer.c void ptimer_set_freq(ptimer_state *s, uint32_t freq) · line 193
Quoted 10 lines
hw/core/ptimer.c +0
-10-/* -* 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) {
This original rate limiting code is gated on icount, so I think then new way should be the same.
Regards,
Peter
Quoted 9 lines
hw/core/ptimer.c +0
-3- limit =10000 / s->period; - } - s->limit = limit;if (reload) s->delta = limit;Signature · 1 line
2.6.4
Re: [Qemu-arm] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value
06.01.2016 15:15, Peter Crosthwaite пишет:
Quoted 6 lines
Quoted 6 lines
hw/core/ptimer.c +0
-0@@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)
Quoted 13 lines
hw/core/ptimer.c static void ptimer_trigger(ptimer_state *s) · line 41
Quoted 8 lines
hw/core/ptimer.c +5
-1static void ptimer_reload (ptimer_state *s) { -if (s->delta ==0 ) { +uint32_t period_frac = s->period_frac; +uint64_t period = s->period; +uint64_t delta = s->delta; +uint64_t limit = s->limit; +Localising these variables is out of scope of the change. I think you can just use
s->fooand if you want to cleanup, it should be a separate patch.
Okay
Quoted 12 lines
Quoted 12 lines
hw/core/ptimer.c +3
-2+if (delta ==0 ) {ptimer_trigger (s); - s->delta = s->limit; + delta = limit; } -if (s->delta ==0 || s->period ==0 ) { +if (delta ==0 || period ==0 ) {fprintf (stderr,"Timer with period zero, disabling\n" ); s->enabled =0 ;return ; }
Quoted 17 lines
Quoted 4 lines
hw/core/ptimer.c +4
-0+/* +* Artificially limit timeout rate to something +* achievable under QEMU. Otherwise, QEMU spends all +* its time generating timer interrupts, and there hw/core/ptimer.c static void ptimer_trigger(ptimer_state *s) · line 65
Quoted 10 lines
hw/core/ptimer.c +10
-0+* is no forward progress. +* About ten microseconds is the fastest that really works +* on the current generation of host machines. +*/ + +if ((s->enabled ==1 ) && (limit * period <10000 )) { + period =10000 / limit; + period_frac =0 ; + } +I think it should be ok to just update
s->periodands->period_frac...
No, then it would be irreversibly lost. What if'd decide to change the limit to some large value?
Quoted 64 lines
Quoted 20 lines
hw/core/ptimer.c +5
-3s->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 + delta * period; +if (period_frac) { + s->next_event += ((int64_t )period_frac * delta) >>32 ; }timer_mod (s->timer, s->next_event); }@@ -82,6 +101,8 @@ 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;/* We need to divide time by period, where time is stored in rem (64-bit integer) and period is stored in period/period_frac hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 121
Quoted 8 lines
hw/core/ptimer.c +5
-0@@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ +if ((s->enabled ==1 ) && (s->limit * period <10000 )) { + period =10000 / s->limit; + period_frac =0 ; + } +... and then this (and the local variables) become obsolete. Can
get_count()blindly use the period andperiod_fracas used byptimer_reload?Quoted 32 lines
hw/core/ptimer.c +4
-9rem = s->next_event - now; - div = s->period; + div = period; clz1 =clz64 (rem); clz2 =clz64 (div);@@ -103,13 +129,13 @@ uint64_t ptimer_get_count(ptimer_state *s) rem <<= shift; div <<= shift;if (shift >=32 ) { - div |= ((uint64_t )s->period_frac << (shift -32 )); + div |= ((uint64_t )period_frac << (shift -32 )); }else {if (shift !=0 ) - div |= (s->period_frac >> (32 - shift)); + div |= (period_frac >> (32 - shift));/* Look at remaining bits of period_frac and round div up if necessary. */ -if ((uint32_t )(s->period_frac << shift)) +if ((uint32_t )(period_frac << shift)) div +=1 ; } counter = rem / div;@@ -181,19 +207,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.
Quoted 9 lines
hw/core/ptimer.c void ptimer_set_freq(ptimer_state *s, uint32_t freq) · line 193
Quoted 5 lines
hw/core/ptimer.c +0
-5-* 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) {This original rate limiting code is gated on icount, so I think then new way should be the same.
Shoot :) That's second time I'm missing it. Good catch!
Quoted 12 lines
Regards,
PeterQuoted 9 lines
hw/core/ptimer.c +0
-4- limit =10000 / s->period; - } - s->limit = limit;if (reload) s->delta = limit; --2.6.4
Signature · 1 line
Dmitry
Re: [Qemu-devel] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value
Quoted 56 lines · Dmitry Osipenko
06.01.2016 15:15, Peter Crosthwaite пишет:
Quoted 7 lines
Quoted 7 lines
hw/core/ptimer.c +0
-0@@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)Quoted 14 lines
hw/core/ptimer.c static void ptimer_trigger(ptimer_state *s) · line 41
Quoted 8 lines
hw/core/ptimer.c +5
-1static void ptimer_reload (ptimer_state *s) { -if (s->delta ==0 ) { +uint32_t period_frac = s->period_frac; +uint64_t period = s->period; +uint64_t delta = s->delta; +uint64_t limit = s->limit; +Localising these variables is out of scope of the change. I think you can just use
s->fooand if you want to cleanup, it should be a separate patch.Okay
Quoted 21 lines
Quoted 21 lines
hw/core/ptimer.c +12
-2+if (delta ==0 ) {ptimer_trigger (s); - s->delta = s->limit; + delta = limit; } -if (s->delta ==0 || s->period ==0 ) { +if (delta ==0 || period ==0 ) {fprintf (stderr,"Timer with period zero, disabling\n" ); s->enabled =0 ;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. +*/ +Quoted 9 lines
hw/core/ptimer.c static void ptimer_trigger(ptimer_state *s) · line 65
Quoted 5 lines
hw/core/ptimer.c +5
-0+if ((s->enabled ==1 ) && (limit * period <10000 )) { + period =10000 / limit; + period_frac =0 ; + } +I think it should be ok to just update
s->periodands->period_frac...
Quoted 3 lines
No, then it would be irreversibly lost. What if'd decide to change the limit to some large value?
Ok makes sense.
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
hw/core/ptimer.c +5
-3s->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 + delta * period; +if (period_frac) { + s->next_event += ((int64_t )period_frac * delta) >>32 ; }timer_mod (s->timer, s->next_event); }@@ -82,6 +101,8 @@ 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;/* We need to divide time by period, where time is stored in
rem (64-bit integer) and period is stored in
period/period_frachw/core/ptimer.c +0
-0@@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */
Quoted 5 lines
Quoted 5 lines
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 121
Quoted 5 lines
hw/core/ptimer.c +5
-0+if ((s->enabled ==1 ) && (s->limit * period <10000 )) { + period =10000 / s->limit; + period_frac =0 ; + } +
As this now needs to be kept, another note I had is it should probably go before the block comment as the comment relates specifically to the math below.
Regards,
Peter
Quoted 69 lines
Quoted 40 lines
... and then this (and the local variables) become obsolete. Can
get_count()
blindly use the period andperiod_fracas used byptimer_reload?Quoted 34 lines
hw/core/ptimer.c +3
-3rem = s->next_event - now; - div = s->period; + div = period; clz1 =clz64 (rem); clz2 =clz64 (div);@@ -103,13 +129,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. */hw/core/ptimer.c +1
-7-if ((uint32_t )(s->period_frac << shift)) +if ((uint32_t )(period_frac << shift)) div +=1 ; } counter = rem / div;@@ -181,19 +207,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 Quoted 9 lines
hw/core/ptimer.c void ptimer_set_freq(ptimer_state *s, uint32_t freq) · line 193
Quoted 4 lines
hw/core/ptimer.c +0
-4-* on the current generation of host machines. -*/ - -if (!use_icount && limit * s->period <10000 && s->period) {This original rate limiting code is gated on icount, so I think then new way should be the same.
Shoot :) That's second time I'm missing it. Good catch!
Quoted 12 lines
Regards,
PeterQuoted 9 lines
hw/core/ptimer.c +0
-4- limit =10000 / s->period; - } - s->limit = limit;if (reload) s->delta = limit; --2.6.4
--
Dmitry
PATCH v8 2/4 hw/ptimer: Perform tick and counter wrap around if timer already expired
In reply to Dmitry Osipenkoptimer_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 addition, there is no reason to keep expired timer tick deferred, so just perform the tick from ptimer_get_count().
--- hw/core/ptimer.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
hw/core/ptimer.c +29 -6
@@ -85,15 +85,21 @@ static void ptimer_tick(void *opaque)uint64_t ptimer_get_count (ptimer_state *s) { +int enabled = s->enabled;int64_t now; +int64_t next;uint64_t counter; +int expired; +int oneshot; -if (s->enabled) { +if (enabled) { now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); + next = s->next_event; + expired = (now - next >=0 ); + oneshot = (enabled ==2 );/* Figure out the current counter value. */ -if (now - s->next_event >0 - || s->period ==0 ) { -/* Prevent timer underflowing if it should already have +if (s->period ==0 || (expired && oneshot)) { +/* Prevent one-shot timer underflowing if it should already have triggered. */ counter =0 ; }else {
@@ -114,12 +120,12 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ -if ((s->enabled ==1 ) && (s->limit * period <10000 )) { +if (!oneshot && (s->limit * period <10000 )) { period =10000 / s->limit; period_frac =0 ; } - rem = s->next_event - now; + rem = expired ? now - next : next - now; div = period; clz1 =clz64 (rem);
@@ -139,6 +145,23 @@ uint64_t ptimer_get_count(ptimer_state *s) div +=1 ; } counter = rem / div; + +if (expired) { +/* Wrap around periodic counter. */ + counter = s->delta = s->limit - counter % s->limit; + } + } + +if (expired) { +if (oneshot) { +ptimer_tick (s); + }else { +/* Don't use ptimer_tick() for the periodic timer since it +* would reset the delta value. +*/ +ptimer_trigger (s); +ptimer_reload (s); + } } }else { counter = s->delta;
Signature · 1 line
2.6.4
Re: [Qemu-arm] [PATCH v8 2/4] hw/ptimer: Perform tick and counter wrap around if timer already expired
Quoted 18 lines · 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 addition, there is no reason to keep expired timer tick deferred, so just perform the tick from
ptimer_get_count().Signed-off-by Dmitry Osipenko <[email protected]>--- hw/core/ptimer.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)hw/core/ptimer.c +0
-0
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 88
Quoted 4 lines
hw/core/ptimer.c +1
-0@@ -85,15 +85,21 @@ static void ptimer_tick(void *opaque)uint64_t ptimer_get_count (ptimer_state *s) { +int enabled = s->enabled;
Variable localisation not needed.
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 93
Quoted 5 lines
hw/core/ptimer.c +3
-0int64_t now; +int64_t next;uint64_t counter; +int expired; +int oneshot;
Variable defs can be localised to the if (enabled) (even though now in original code doesn't do that).
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 100
Quoted 8 lines
hw/core/ptimer.c +4
-1-if (s->enabled) { +if (enabled) { now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); + next = s->next_event; + expired = (now - next >=0 ); + oneshot = (enabled ==2 );/* Figure out the current counter value. */
This comment is now out of place.
Quoted 24 lines
hw/core/ptimer.c +4
-5-if (now - s->next_event >0 - || s->period ==0 ) { -/* Prevent timer underflowing if it should already have +if (s->period ==0 || (expired && oneshot)) { +/* Prevent one-shot timer underflowing if it should already have triggered. */ counter =0 ; }else {@@ -114,12 +120,12 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ -if ((s->enabled ==1 ) && (s->limit * period <10000 )) { +if (!oneshot && (s->limit * period <10000 )) { period =10000 / s->limit; period_frac =0 ; } - rem = s->next_event - now; + rem = expired ? now - next : next - now; div = period; clz1 =clz64 (rem);
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 151
Quoted 7 lines
hw/core/ptimer.c +4
-0@@ -139,6 +145,23 @@ uint64_t ptimer_get_count(ptimer_state *s) div +=1 ; } counter = rem / div; + +if (expired) { +/* Wrap around periodic counter. */ + counter = s->delta = s->limit - counter % s->limit;
Why do you update the delta here?
Also can you just get ptimer_reload to do the modulo math for you? If the timer is !oneshot and expired, then you call ptimer_reload anyway, which will update next_event. When the expired test returns false you can just reliably use the original logic involving now and next.
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 156
Quoted 5 lines
hw/core/ptimer.c +5
-0+ } + } + +if (expired) { +if (oneshot) {
This if-else has a lot of common structure with the one above. I think they could be merged.
Regards,
Peter
Quoted 14 lines
hw/core/ptimer.c +8
-0+ptimer_tick (s); + }else { +/* Don't use ptimer_tick() for the periodic timer since it +* would reset the delta value. +*/ +ptimer_trigger (s); +ptimer_reload (s); + } } }else { counter = s->delta;Signature · 1 line
2.6.4
Re: [Qemu-arm] [PATCH v8 2/4] hw/ptimer: Perform tick and counter wrap around if timer already expired
06.01.2016 15:17, Peter Crosthwaite пишет:
Quoted 26 lines · Dmitry Osipenko
Quoted 18 lines
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 addition, there is no reason to keep expired timer tick deferred, so just perform the tick from
ptimer_get_count().Signed-off-by Dmitry Osipenko <[email protected]>--- hw/core/ptimer.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)hw/core/ptimer.c +0
-0hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 88
Quoted 4 lines
hw/core/ptimer.c +1
-0@@ -85,15 +85,21 @@ static void ptimer_tick(void *opaque)uint64_t ptimer_get_count (ptimer_state *s) { +int enabled = s->enabled;Variable localisation not needed.
Quoted 9 lines
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 93
Quoted 5 lines
hw/core/ptimer.c +3
-0int64_t now; +int64_t next;uint64_t counter; +int expired; +int oneshot;Variable defs can be localised to the if (enabled) (even though now in original code doesn't do that).
Yeah, I just tried to keep original style here.
Quoted 11 lines
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 100
Quoted 8 lines
hw/core/ptimer.c +4
-1-if (s->enabled) { +if (enabled) { now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); + next = s->next_event; + expired = (now - next >=0 ); + oneshot = (enabled ==2 );/* Figure out the current counter value. */ This comment is now out of place.
Okay, will fix it.
Quoted 22 lines
Quoted 22 lines
hw/core/ptimer.c +4
-5-if (now - s->next_event >0 - || s->period ==0 ) { -/* Prevent timer underflowing if it should already have +if (s->period ==0 || (expired && oneshot)) { +/* Prevent one-shot timer underflowing if it should already have triggered. */ counter =0 ; }else {@@ -114,12 +120,12 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ -if ((s->enabled ==1 ) && (s->limit * period <10000 )) { +if (!oneshot && (s->limit * period <10000 )) { period =10000 / s->limit; period_frac =0 ; } - rem = s->next_event - now; + rem = expired ? now - next : next - now; div = period;
Quoted 12 lines
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 151
Quoted 9 lines
hw/core/ptimer.c +4
-0clz1 =clz64 (rem);@@ -139,6 +145,23 @@ uint64_t ptimer_get_count(ptimer_state *s) div +=1 ; } counter = rem / div; + +if (expired) { +/* Wrap around periodic counter. */ + counter = s->delta = s->limit - counter % s->limit;Why do you update the delta here?
Because we would want to schedule next tick based on current wrapped around counter value and not some arbitrary delta.
Quoted 5 lines
Also can you just get
ptimer_reloadto do the modulo math for you? If the timer is !oneshot and expired, then you callptimer_reloadanyway, which will updatenext_event. When the expired test returns false you can just reliably use the original logic involving now and next.
Yes, that's what I changed in V9. Have you received it?
https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00272.html
Quoted 9 lines
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 156
Quoted 5 lines
hw/core/ptimer.c +5
-0+ } + } + +if (expired) { +if (oneshot) {This if-else has a lot of common structure with the one above. I think they could be merged.
That's a good suggestion, will do it in V10. Thanks.
Quoted 17 lines
Regards,
PeterQuoted 14 lines
hw/core/ptimer.c +8
-1+ptimer_tick (s); + }else { +/* Don't use ptimer_tick() for the periodic timer since it +* would reset the delta value. +*/ +ptimer_trigger (s); +ptimer_reload (s); + } } }else { counter = s->delta; --2.6.4
Signature · 1 line
Dmitry
Re: [Qemu-devel] [PATCH v8 2/4] hw/ptimer: Perform tick and counter wrap around if timer already expired
Quoted 97 lines · Dmitry Osipenko
06.01.2016 15:17, Peter Crosthwaite пишет:
Quoted 36 lines · Dmitry Osipenko
Quoted 20 lines
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 addition, there is no reason to keep expired timer tick deferred, so just perform the tick from
ptimer_get_count().Signed-off-by Dmitry Osipenko <[email protected]>--- hw/core/ptimer.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)hw/core/ptimer.c +0
-0hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 88
Quoted 4 lines
hw/core/ptimer.c +1
-0@@ -85,15 +85,21 @@ static void ptimer_tick(void *opaque)uint64_t ptimer_get_count (ptimer_state *s) { +int enabled = s->enabled;Variable localisation not needed.
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 93
Quoted 5 lines
hw/core/ptimer.c +3
-0int64_t now; +int64_t next;uint64_t counter; +int expired; +int oneshot;Quoted 3 lines
Variable defs can be localised to the if (enabled) (even though now in original code doesn't do that).
Yeah, I just tried to keep original style here.
Quoted 10 lines
hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 100
Quoted 8 lines
hw/core/ptimer.c +4
-1-if (s->enabled) { +if (enabled) { now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); + next = s->next_event; + expired = (now - next >=0 ); + oneshot = (enabled ==2 );/* Figure out the current counter value. */ Quoted 2 lines
This comment is now out of place.
Okay, will fix it.
Quoted 23 lines
Quoted 23 lines
hw/core/ptimer.c +2
-3-if (now - s->next_event >0 - || s->period ==0 ) { -/* Prevent timer underflowing if it should already have +if (s->period ==0 || (expired && oneshot)) { +/* Prevent one-shot timer underflowing if it should already have
triggered. */ counter = 0; } else {hw/core/ptimer.c +2
-2@@ -114,12 +120,12 @@ uint64_t ptimer_get_count(ptimer_state *s)backwards. */ -if ((s->enabled ==1 ) && (s->limit * period <10000 )) { +if (!oneshot && (s->limit * period <10000 )) { period =10000 / s->limit; period_frac =0 ; } - rem = s->next_event - now; + rem = expired ? now - next : next - now; div = period;Quoted 13 lines
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 151
Quoted 9 lines
hw/core/ptimer.c +4
-0clz1 =clz64 (rem);@@ -139,6 +145,23 @@ uint64_t ptimer_get_count(ptimer_state *s) div +=1 ; } counter = rem / div; + +if (expired) { +/* Wrap around periodic counter. */ + counter = s->delta = s->limit - counter % s->limit;Why do you update the delta here?
Quoted 3 lines
Because we would want to schedule next tick based on current wrapped around counter value and not some arbitrary delta.
So looking at ptimer_reload(), the new schedule is done relative to the VM clock of the when the tick was expected to hit, not the current time. But this new delta is going to be relative to the now time and then used to update the next tick which will happen relative to next_event. Unless you stop or scale the timer, I don't think you need to do delta manipulation?
Quoted 10 lines
Quoted 5 lines
Also can you just get
ptimer_reloadto do the modulo math for you? If the timer is !oneshot and expired, then you callptimer_reloadanyway, which will updatenext_event. When the expired test returns false you can just reliably use the original logic involving now and next.Yes, that's what I changed in V9. Have you received it?
https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00272.html
Just had a look.
V9 still has the modulo I think?:
+ if (expired && (counter != 0)) {
+ /* Wrap around periodic counter. */
+ counter = s->delta = s->limit - counter % s->limit;
+ }
Regards,
Peter
Quoted 36 lines
Quoted 10 lines
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 156
Quoted 5 lines
hw/core/ptimer.c +5
-0+ } + } + +if (expired) { +if (oneshot) {This if-else has a lot of common structure with the one above. I think they could be merged.
That's a good suggestion, will do it in V10. Thanks.
Quoted 18 lines
Regards,
PeterQuoted 15 lines
hw/core/ptimer.c +3
-0+ptimer_tick (s); + }else { +/* Don't use ptimer_tick() for the periodic timer since it
hw/core/ptimer.c +5
-1+* would reset the delta value. +*/ +ptimer_trigger (s); +ptimer_reload (s); + } } }else { counter = s->delta; --2.6.4
--
Dmitry
Re: [Qemu-arm] [PATCH v8 2/4] hw/ptimer: Perform tick and counter wrap around if timer already expired
06.01.2016 16:59, Peter Crosthwaite пишет:
Quoted 13 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
hw/core/ptimer.c +4
-0+ +if (expired) { +/* Wrap around periodic counter. */ + counter = s->delta = s->limit - counter % s->limit;Quoted 6 lines
Quoted 2 lines
Why do you update the delta here?
Because we would want to schedule next tick based on current wrapped around counter value and not some arbitrary delta.
Quoted 7 lines
So looking at
ptimer_reload(), the new schedule is done relative to the VM clock of the when the tick was expected to hit, not the current time. But this new delta is going to be relative to the now time and then used to update the next tick which will happen relative tonext_event. Unless you stop or scale the timer, I don't think you need to do delta manipulation?
Yes, I missed that next_event would be set earlier (like you described) in case of expired timer. Thanks for the note, will fix it.
Quoted 16 lines
Quoted 10 lines
Quoted 5 lines
Also can you just get
ptimer_reloadto do the modulo math for you? If the timer is !oneshot and expired, then you callptimer_reloadanyway, which will updatenext_event. When the expired test returns false you can just reliably use the original logic involving now and next.Yes, that's what I changed in V9. Have you received it?
https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00272.html
Just had a look.
V9 still has the modulo I think?:
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 152
Quoted 4 lines
hw/core/ptimer.c +4
-0+if (expired && (counter !=0 )) { +/* Wrap around periodic counter. */ + counter = s->delta = s->limit - counter % s->limit; + }
Modulo is there, I just meant that V9 changed to call ptimer_reload() on counter == 0. As noted above, ptimer_reload would adjust next_event, so s->delta shouldn't be set to the wrapped around counter. However it should be set to the limit, since delta might been altered by ptimer_set_count.
Signature · 1 line
Dmitry
PATCH v8 3/4 hw/ptimer: Update .delta on period/freq change
In reply to Dmitry OsipenkoDelta 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.
--- hw/core/ptimer.c | 2 ++ 1 file changed, 2 insertions(+)
hw/core/ptimer.c +2 -0
@@ -207,6 +207,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) {
@@ -218,6 +219,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.6.4
Re: [Qemu-devel] [PATCH v8 3/4] hw/ptimer: Update .delta on period/freq change
Quoted 7 lines · 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]>
Quoted 27 lines
--- hw/core/ptimer.c | 2 ++ 1 file changed, 2 insertions(+)hw/core/ptimer.c +2
-0@@ -207,6 +207,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) {@@ -218,6 +219,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.6.4
PATCH v8 4/4 arm_mptimer: Convert to use ptimer
In reply to Dmitry OsipenkoCurrent 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.
--- 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.4
Re: [Qemu-devel] [PATCH v8 4/4] arm_mptimer: Convert to use ptimer
Quoted 86 lines · 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
- Code simplification and reductionBump 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 | 110 ++++++++++++++++++----------------------- include/hw/timer/arm_mptimer.h | 4 +- 2 files changed, 49 insertions(+), 65 deletions(-)hw/timer/arm_mptimer.c +3
-30@@ -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. */
hw/timer/arm_mptimer.c static uint64_t timerblock_read(void *opaque, hwaddr addr, · line 79
Quoted 7 lines
hw/timer/arm_mptimer.c +4
-0@@ -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 )) {
Parentheses around == expressions should not be needed.
hw/timer/arm_mptimer.c static uint64_t timerblock_read(void *opaque, hwaddr addr, · line 84
Quoted 5 lines
hw/timer/arm_mptimer.c +5
-0+ count = tb->load; + } +ptimer_set_count (tb->timer, count); + } +if ((tb->control &1 ) && (count !=0 )) {
This can check against tb->load instead of count to avoid dummy pass of tb->load to this fn ...
Quoted 22 lines
hw/timer/arm_mptimer.c +7
-7+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 ) {
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 109
Quoted 10 lines
hw/timer/arm_mptimer.c +8
-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 );
... then this would just need to be elsed.
Quoted 7 lines
hw/timer/arm_mptimer.c +0
-3break ;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. */
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 114
Quoted 10 lines
hw/timer/arm_mptimer.c +1
-9-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. */
Is it worth adding this to ptimer? It seems ptimer can now handle every other case of running configuration change except this one case.
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 124
Quoted 10 lines
hw/timer/arm_mptimer.c +10
-0+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 );
Is this reachable when the load value is still 0?
Following on from the suggested refactor before, I think timerblock_run should split off the count set to a new helper. Then this is
timerblock_setcount(tb, value);timerblock_run(tb);
and the boolean arg and dummy pass of tb->load as count are unneeded.
Regards,
Peter
Quoted 83 lines
hw/timer/arm_mptimer.c +10
-12}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.4
Re: [Qemu-arm] [PATCH v8 4/4] arm_mptimer: Convert to use ptimer
06.01.2016 16:17, Peter Crosthwaite пишет:
Quoted 159 lines · Dmitry Osipenko
Quoted 86 lines
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 reductionBump 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 | 110 ++++++++++++++++++----------------------- include/hw/timer/arm_mptimer.h | 4 +- 2 files changed, 49 insertions(+), 65 deletions(-)hw/timer/arm_mptimer.c +3
-30@@ -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. */ hw/timer/arm_mptimer.c static uint64_t timerblock_read(void *opaque, hwaddr addr, · line 79
Quoted 7 lines
hw/timer/arm_mptimer.c +4
-0@@ -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 )) {Parentheses around == expressions should not be needed.
hw/timer/arm_mptimer.c static uint64_t timerblock_read(void *opaque, hwaddr addr, · line 84
Quoted 5 lines
hw/timer/arm_mptimer.c +5
-0+ count = tb->load; + } +ptimer_set_count (tb->timer, count); + } +if ((tb->control &1 ) && (count !=0 )) {This can check against
tb->loadinstead of count to avoid dummy pass oftb->loadto this fn ...Quoted 22 lines
hw/timer/arm_mptimer.c +7
-7+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 ) {hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 109
Quoted 10 lines
hw/timer/arm_mptimer.c +8
-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 );... then this would just need to be elsed.
Quoted 7 lines
hw/timer/arm_mptimer.c +0
-3break ;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. */ hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 114
Quoted 10 lines
hw/timer/arm_mptimer.c +1
-9-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. */
Quoted 4 lines
Is it worth adding this to ptimer? It seems ptimer can now handle every other case of running configuration change except this one case.
Yeah, should make code cleaner as well.
Quoted 13 lines
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 124
Quoted 10 lines
hw/timer/arm_mptimer.c +10
-0+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 );Is this reachable when the load value is still 0?
Quoted 8 lines
Following on from the suggested refactor before, I think
timerblock_runshould split off the count set to a new helper. Then this is
timerblock_setcount(tb, value);timerblock_run(tb);and the boolean arg and dummy pass of
tb->loadas count are unneeded.
What about to just inline timerblock_run? It would allow compiler to eliminate dead code.
Signature · 1 line
Dmitry
Re: [Qemu-arm] [PATCH v8 4/4] arm_mptimer: Convert to use ptimer
In reply to Peter Crosthwaite06.01.2016 16:17, Peter Crosthwaite пишет:
Quoted 11 lines
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 124
Quoted 10 lines
hw/timer/arm_mptimer.c +10
-0+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 );
Quoted 2 lines
Is this reachable when the load value is still 0?
Yes, timer runs based on current counter value. You can load = 0 and set counter != 0 that would result in one-shot tick. Hmm... I need to fix it. Thanks for leading question!
Ughh... just noticed that test_timer_set_oneshot_count_to_0() on real HW sets IT bit if prescaler != 0. Looking into it..
Signature · 1 line
Dmitry