On 17 June 2016 at 14:17, Dmitry Osipenko <dig...@gmail.com> wrote:
> Currently ptimer prints error message and stops the running timer that has
> delta (counter) = 0, this is an incorrect behaviour for some of the ptimer
> users. There are different variants of how particular timer could handle that
> case besides stopping the timer, like immediate or deferred IRQ trigger.
> Introduce policy feature that provides ptimer with an information about the
> correct behaviour.
>
> Implement the "counter = 0 triggers IRQ after one period" policy, as it is
> known to be used by the ARM MPTimer and set "default" policy to all ptimer
> users, maintaining old behaviour till they get fixed.
Could you split this into:
(1) a patch which just adds the new argument to ptimer_init() and
updates all its callers
(2) a patch which adds support for setting the policy option to
something other than the default value
and also make sure that we only do one change per patch -- there
seem to be several different behaviour changes tangled up in
one patch here.
I think that will be easier to review.
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 05b0c27..289e23e 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_mask;
> QEMUBH *bh;
> QEMUTimer *timer;
> };
> @@ -35,14 +36,15 @@ static void ptimer_trigger(ptimer_state *s)
>
> static void ptimer_reload(ptimer_state *s)
> {
> - uint32_t period_frac = s->period_frac;
> + int64_t period_frac = s->period_frac;
Why does this variable change type?
> uint64_t period = s->period;
> + uint64_t delta = s->delta;
>
> - if (s->delta == 0) {
> - ptimer_trigger(s);
> - s->delta = s->limit;
> + if (delta == 0 && (s->policy_mask & PTIMER_POLICY_CNT_0_DEFERRED_TRIG)) {
> + delta = 1;
> }
> - if (s->delta == 0 || s->period == 0) {
> +
> + if (delta == 0 || period == 0) {
> fprintf(stderr, "Timer with period zero, disabling\n");
> s->enabled = 0;
> return;
> @@ -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 += (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) {
This seems to be a change not guarded by a check of a policy bit?
> 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,10 +170,6 @@ 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;
> - }
If the default policy value was provided, we shouldn't change behaviour.
> s->enabled = oneshot ? 2 : 1;
> if (was_disabled) {
> s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -203,6 +204,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 <= 1000000000ll);
This seems to be an unrelated change.
> s->delta = ptimer_get_count(s);
> s->period = 1000000000ll / freq;
> s->period_frac = (1000000000ll << 32) / freq;
> @@ -232,8 +234,8 @@ uint64_t ptimer_get_limit(ptimer_state *s)
>
> const VMStateDescription vmstate_ptimer = {
> .name = "ptimer",
> - .version_id = 1,
> - .minimum_version_id = 1,
> + .version_id = 2,
> + .minimum_version_id = 2,
Why are we bumping the version ID here?
> .fields = (VMStateField[]) {
> VMSTATE_UINT8(enabled, ptimer_state),
> VMSTATE_UINT64(limit, ptimer_state),
> @@ -247,12 +249,13 @@ const VMStateDescription vmstate_ptimer = {
> }
> };
>
> -ptimer_state *ptimer_init(QEMUBH *bh)
> +ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
> {
> ptimer_state *s;
>
> s = (ptimer_state *)g_malloc0(sizeof(ptimer_state));
> s->bh = bh;
> s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ptimer_tick, s);
> + s->policy_mask = policy_mask;
> return s;
> }
thanks
-- PMM
