ptimer: Provide new transaction-based API

Provide the new transaction-based API. If a ptimer is created
using ptimer_init() rather than ptimer_init_with_bh(), then
instead of providing a QEMUBH, it provides a pointer to the
callback function directly, and has opted into the transaction
API. All calls to functions which modify ptimer state:
 - ptimer_set_period()
 - ptimer_set_freq()
 - ptimer_set_limit()
 - ptimer_set_count()
 - ptimer_run()
 - ptimer_stop()
must be between matched calls to ptimer_transaction_begin()
and ptimer_transaction_commit(). When ptimer_transaction_commit()
is called it will evaluate the state of the timer after all the
changes in the transaction, and call the callback if necessary.

In the old API the individual update functions generally would
call ptimer_trigger() immediately, which would schedule the QEMUBH.
In the new API the update functions will instead defer the
"set s->next_event and call ptimer_reload()" work to
ptimer_transaction_commit().

Because ptimer_trigger() can now immediately call into the
device code which may then call other ptimer functions that
update ptimer_state fields, we must be more careful in
ptimer_reload() not to cache fields from ptimer_state across
the ptimer_trigger() call. (This was harmless with the QEMUBH
mechanism as the BH would not be invoked until much later.)

We use assertions to check that:
 * the functions modifying ptimer state are not called outside
   a transaction block
 * ptimer_transaction_begin() and _commit() calls are paired
 * the transaction API is not used with a QEMUBH ptimer

There is some slight repetition of code:
 * most of the set functions have similar looking "if s->bh
   call ptimer_reload, otherwise set s->need_reload" code
 * ptimer_init() and ptimer_init_with_bh() have similar code
We deliberately don't try to avoid this repetition, because
it will all be deleted when the QEMUBH version of the API
is removed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 20191008171740.9679-3-peter.maydell@linaro.org

hw/core/ptimer.c

@@ -31,6 +31,16 @@ struct ptimer_state
     uint8_t policy_mask;
     QEMUBH *bh;
     QEMUTimer *timer;
+    ptimer_cb callback;
+    void *callback_opaque;
+    /*
+     * These track whether we're in a transaction block, and if we
+     * need to do a timer reload when the block finishes. They don't
+     * need to be migrated because migration can never happen in the
+     * middle of a transaction block.
+     */
+    bool in_transaction;
+    bool need_reload;
 };
 
 /* Use a bottom-half routine to avoid reentrancy issues.  */
@@ -39,13 +49,16 @@ static void ptimer_trigger(ptimer_state *s)
     if (s->bh) {
         replay_bh_schedule_event(s->bh);
     }
+    if (s->callback) {
+        s->callback(s->callback_opaque);
+    }
 }
 
 static void ptimer_reload(ptimer_state *s, int delta_adjust)
 {
-    uint32_t period_frac = s->period_frac;
-    uint64_t period = s->period;
-    uint64_t delta = s->delta;
+    uint32_t period_frac;
+    uint64_t period;
+    uint64_t delta;
     bool suppress_trigger = false;
 
     /*
@@ -58,11 +71,20 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
         (s->policy_mask & PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT)) {
         suppress_trigger = true;
     }
-    if (delta == 0 && !(s->policy_mask & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER)
+    if (s->delta == 0 && !(s->policy_mask & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER)
         && !suppress_trigger) {
         ptimer_trigger(s);
     }
 
+    /*
+     * Note that ptimer_trigger() might call the device callback function,
+     * which can then modify timer state, so we must not cache any fields
+     * from ptimer_state until after we have called it.
+     */
+    delta = s->delta;
+    period = s->period;
+    period_frac = s->period_frac;
+
     if (delta == 0 && !(s->policy_mask & PTIMER_POLICY_NO_IMMEDIATE_RELOAD)) {
         delta = s->delta = s->limit;
     }
@@ -136,6 +158,15 @@ static void ptimer_tick(void *opaque)
     ptimer_state *s = (ptimer_state *)opaque;
     bool trigger = true;
 
+    /*
+     * We perform all the tick actions within a begin/commit block
+     * because the callback function that ptimer_trigger() calls
+     * might make calls into the ptimer APIs that provoke another
+     * trigger, and we want that to cause the callback function
+     * to be called iteratively, not recursively.
+     */
+    ptimer_transaction_begin(s);
+
     if (s->enabled == 2) {
         s->delta = 0;
         s->enabled = 0;
@@ -164,6 +195,8 @@ static void ptimer_tick(void *opaque)
     if (trigger) {
         ptimer_trigger(s);
     }
+
+    ptimer_transaction_commit(s);
 }
 
 uint64_t ptimer_get_count(ptimer_state *s)
@@ -263,17 +296,24 @@ uint64_t ptimer_get_count(ptimer_state *s)
 
 void ptimer_set_count(ptimer_state *s, uint64_t count)
 {
+    assert(s->in_transaction || !s->callback);
     s->delta = count;
     if (s->enabled) {
-        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s, 0);
+        if (!s->callback) {
+            s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            ptimer_reload(s, 0);
+        } else {
+            s->need_reload = true;
+        }
     }
 }
 
 void ptimer_run(ptimer_state *s, int oneshot)
 {
     bool was_disabled = !s->enabled;
 
+    assert(s->in_transaction || !s->callback);
+
     if (was_disabled && s->period == 0) {
         if (!qtest_enabled()) {
             fprintf(stderr, "Timer with period zero, disabling\n");
@@ -282,57 +322,81 @@ void ptimer_run(ptimer_state *s, int oneshot)
     }
     s->enabled = oneshot ? 2 : 1;
     if (was_disabled) {
-        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s, 0);
+        if (!s->callback) {
+            s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            ptimer_reload(s, 0);
+        } else {
+            s->need_reload = true;
+        }
     }
 }
 
 /* Pause a timer.  Note that this may cause it to "lose" time, even if it
    is immediately restarted.  */
 void ptimer_stop(ptimer_state *s)
 {
+    assert(s->in_transaction || !s->callback);
+
     if (!s->enabled)
         return;
 
     s->delta = ptimer_get_count(s);
     timer_del(s->timer);
     s->enabled = 0;
+    if (s->callback) {
+        s->need_reload = false;
+    }
 }
 
 /* Set counter increment interval in nanoseconds.  */
 void ptimer_set_period(ptimer_state *s, int64_t period)
 {
+    assert(s->in_transaction || !s->callback);
     s->delta = ptimer_get_count(s);
     s->period = period;
     s->period_frac = 0;
     if (s->enabled) {
-        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s, 0);
+        if (!s->callback) {
+            s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            ptimer_reload(s, 0);
+        } else {
+            s->need_reload = true;
+        }
     }
 }
 
 /* Set counter frequency in Hz.  */
 void ptimer_set_freq(ptimer_state *s, uint32_t freq)
 {
+    assert(s->in_transaction || !s->callback);
     s->delta = ptimer_get_count(s);
     s->period = 1000000000ll / freq;
     s->period_frac = (1000000000ll << 32) / freq;
     if (s->enabled) {
-        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s, 0);
+        if (!s->callback) {
+            s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            ptimer_reload(s, 0);
+        } else {
+            s->need_reload = true;
+        }
     }
 }
 
 /* Set the initial countdown value.  If reload is nonzero then also set
    count = limit.  */
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 {
+    assert(s->in_transaction || !s->callback);
     s->limit = limit;
     if (reload)
         s->delta = limit;
     if (s->enabled && reload) {
-        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        ptimer_reload(s, 0);
+        if (!s->callback) {
+            s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            ptimer_reload(s, 0);
+        } else {
+            s->need_reload = true;
+        }
     }
 }
 
@@ -341,6 +405,32 @@ uint64_t ptimer_get_limit(ptimer_state *s)
     return s->limit;
 }
 
+void ptimer_transaction_begin(ptimer_state *s)
+{
+    assert(!s->in_transaction || !s->callback);
+    s->in_transaction = true;
+    s->need_reload = false;
+}
+
+void ptimer_transaction_commit(ptimer_state *s)
+{
+    assert(s->in_transaction);
+    /*
+     * We must loop here because ptimer_reload() can call the callback
+     * function, which might then update ptimer state in a way that
+     * means we need to do another reload and possibly another callback.
+     * A disabled timer never needs reloading (and if we don't check
+     * this then we loop forever if ptimer_reload() disables the timer).
+     */
+    while (s->need_reload && s->enabled) {
+        s->need_reload = false;
+        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        ptimer_reload(s, 0);
+    }
+    /* Now we've finished reload we can leave the transaction block. */
+    s->in_transaction = false;
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
     .version_id = 1,
@@ -377,9 +467,41 @@ ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask)
     return s;
 }
 
+ptimer_state *ptimer_init(ptimer_cb callback, void *callback_opaque,
+                          uint8_t policy_mask)
+{
+    ptimer_state *s;
+
+    /*
+     * The callback function is mandatory; so we use it to distinguish
+     * old-style QEMUBH ptimers from new transaction API ptimers.
+     * (ptimer_init_with_bh() allows a NULL bh pointer and at least
+     * one device (digic-timer) passes NULL, so it's not the case
+     * that either s->bh != NULL or s->callback != NULL.)
+     */
+    assert(callback);
+
+    s = g_new0(ptimer_state, 1);
+    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ptimer_tick, s);
+    s->policy_mask = policy_mask;
+    s->callback = callback;
+    s->callback_opaque = callback_opaque;
+
+    /*
+     * These two policies are incompatible -- trigger-on-decrement implies
+     * a timer trigger when the count becomes 0, but no-immediate-trigger
+     * implies a trigger when the count stops being 0.
+     */
+    assert(!((policy_mask & PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT) &&
+             (policy_mask & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER)));
+    return s;
+}
+
 void ptimer_free(ptimer_state *s)
 {
-    qemu_bh_delete(s->bh);
+    if (s->bh) {
+        qemu_bh_delete(s->bh);
+    }
     timer_free(s->timer);
     g_free(s);
 }