| Message ID | 97318fe5bbe95202ca2d96e72a31c597a148ea77.1464367869.git.digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On 27 May 2016 at 18:03, Dmitry Osipenko <digetx@gmail.com> wrote: > 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. 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). > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > hw/core/ptimer.c | 53 +++++++++++++++++++++++++++++++---------------------- > include/hw/ptimer.h | 6 ++++++ > 2 files changed, 37 insertions(+), 22 deletions(-) > > diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c > index 05b0c27..9bc70f5 100644 > --- a/hw/core/ptimer.c > +++ b/hw/core/ptimer.c > @@ -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"); 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 > } > - 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; > + Stray extra blank line. > 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. > 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() > } > }; > diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h > index e397db5..221f591 100644 > --- a/include/hw/ptimer.h > +++ b/include/hw/ptimer.h > @@ -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. > +}; > + > /* 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. > extern const VMStateDescription vmstate_ptimer; thanks -- PMM
On 06.06.2016 13:09, Peter Maydell wrote: > On 27 May 2016 at 18:03, Dmitry Osipenko <digetx@gmail.com> wrote: >> 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". > QOM property should fit well, thanks for the suggestion. > 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. >> + 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. > 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. >> @@ -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. > Ack. >> +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. >> +}; >> + >> /* 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. > Ack. Thanks a lot for the review!
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c index 05b0c27..9bc70f5 100644 --- a/hw/core/ptimer.c +++ b/hw/core/ptimer.c @@ -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() } }; diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h index e397db5..221f591 100644 --- a/include/hw/ptimer.h +++ b/include/hw/ptimer.h @@ -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;
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 <digetx@gmail.com> --- hw/core/ptimer.c | 53 +++++++++++++++++++++++++++++++---------------------- include/hw/ptimer.h | 6 ++++++ 2 files changed, 37 insertions(+), 22 deletions(-)