[RFC 0/4] transaction-based ptimer API

[RFC 0/4] transaction-based ptimer API

[Peter Maydell]

Currently the ptimer design uses a QEMU bottom-half as its mechanism
for calling back into the device model using the ptimer when the
timer has expired.  Unfortunately this design is fatally flawed,
because it means that there is a lag between the ptimer updating its
own state and the device callback function updating device state, and
guest accesses to device registers between the two can return
inconsistent device state. This was reported as a bug in a specific
timer device but it's a problem with the generic ptimer code:
https://bugs.launchpad.net/qemu/+bug/1777777

This RFC patchset introduces a change to the ptimer API so that
instead of using a bottom-half for the trigger a device can choose to
use a new 'transaction' based approach.  In this design (suggested by
RTH) all calls 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.

I'm sending this out as an RFC to get opinions on the new API
before I proceed to converting all the users of ptimer. (My
plan is that the final patchset will start with these patches,
have another couple of dozen patches changing the other timer
devices using ptimer, and then have a "delete the BH API/code"
patch. Or we could split the "change to new API" parts into
smaller per-architecture ones.)

Patch 1 is some new trace events I added to the arm_timer code
while I was investigating the original bug report.
Patch 2 is just a function rename so we can keep the nicer
"ptimer_init()" name for the new API.
Patch 3 adds the new API and its implementation.
Patch 4 is a sample conversion of a ptimer-using device,
which fixes the reported bug:
https://bugs.launchpad.net/qemu/+bug/1777777

thanks
-- PMM

Peter Maydell (4):
  hw/timer/arm_timer: Add trace events
  ptimer: Rename ptimer_init() to ptimer_init_with_bh()
  ptimer: Provide new transaction-based API
  hw/timer/arm_timer.c: Switch to transaction-based ptimer API

 include/hw/ptimer.h              |  77 ++++++++++++++++++--
 hw/arm/musicpal.c                |   2 +-
 hw/core/ptimer.c                 | 116 +++++++++++++++++++++++++++----
 hw/dma/xilinx_axidma.c           |   2 +-
 hw/m68k/mcf5206.c                |   2 +-
 hw/m68k/mcf5208.c                |   2 +-
 hw/net/fsl_etsec/etsec.c         |   2 +-
 hw/net/lan9118.c                 |   2 +-
 hw/timer/allwinner-a10-pit.c     |   2 +-
 hw/timer/altera_timer.c          |   2 +-
 hw/timer/arm_mptimer.c           |   6 +-
 hw/timer/arm_timer.c             |  43 +++++++++---
 hw/timer/cmsdk-apb-dualtimer.c   |   2 +-
 hw/timer/cmsdk-apb-timer.c       |   2 +-
 hw/timer/digic-timer.c           |   2 +-
 hw/timer/etraxfs_timer.c         |   6 +-
 hw/timer/exynos4210_mct.c        |   7 +-
 hw/timer/exynos4210_pwm.c        |   2 +-
 hw/timer/exynos4210_rtc.c        |   4 +-
 hw/timer/grlib_gptimer.c         |   2 +-
 hw/timer/imx_epit.c              |   4 +-
 hw/timer/imx_gpt.c               |   2 +-
 hw/timer/lm32_timer.c            |   2 +-
 hw/timer/milkymist-sysctl.c      |   4 +-
 hw/timer/mss-timer.c             |   2 +-
 hw/timer/puv3_ost.c              |   2 +-
 hw/timer/sh_timer.c              |   2 +-
 hw/timer/slavio_timer.c          |   2 +-
 hw/timer/xilinx_timer.c          |   2 +-
 hw/watchdog/cmsdk-apb-watchdog.c |   2 +-
 tests/ptimer-test.c              |  22 +++---
 hw/timer/trace-events            |   7 ++
 32 files changed, 263 insertions(+), 75 deletions(-)

-- 
2.20.1

[RFC 1/4] hw/timer/arm_timer: Add trace events

[Peter Maydell]

Add some basic trace events to the arm_timer device.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c  | 27 +++++++++++++++++++++------
 hw/timer/trace-events |  7 +++++++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index c2e6211188b..283ae1e74a9 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -17,6 +17,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qemu/log.h"
+#include "trace.h"
 
 /* Common timer implementation.  */
 
@@ -43,7 +44,10 @@ typedef struct {
 static void arm_timer_update(arm_timer_state *s)
 {
     /* Update interrupts.  */
-    if (s->int_level && (s->control & TIMER_CTRL_IE)) {
+    int level = s->int_level && (s->control & TIMER_CTRL_IE);
+
+    trace_sp804_arm_timer_update(level);
+    if (level) {
         qemu_irq_raise(s->irq);
     } else {
         qemu_irq_lower(s->irq);
@@ -216,17 +220,21 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
     SP804State *s = (SP804State *)opaque;
+    uint64_t res;
 
     if (offset < 0x20) {
-        return arm_timer_read(s->timer[0], offset);
+        res = arm_timer_read(s->timer[0], offset);
+        goto out;
     }
     if (offset < 0x40) {
-        return arm_timer_read(s->timer[1], offset - 0x20);
+        res = arm_timer_read(s->timer[1], offset - 0x20);
+        goto out;
     }
 
     /* TimerPeriphID */
     if (offset >= 0xfe0 && offset <= 0xffc) {
-        return sp804_ids[(offset - 0xfe0) >> 2];
+        res = sp804_ids[(offset - 0xfe0) >> 2];
+        goto out;
     }
 
     switch (offset) {
@@ -236,12 +244,17 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
         qemu_log_mask(LOG_UNIMP,
                       "%s: integration test registers unimplemented\n",
                       __func__);
-        return 0;
+        res = 0;
+        goto out;
     }
 
     qemu_log_mask(LOG_GUEST_ERROR,
                   "%s: Bad offset %x\n", __func__, (int)offset);
-    return 0;
+    res = 0;
+
+out:
+    trace_sp804_read(offset, res);
+    return res;
 }
 
 static void sp804_write(void *opaque, hwaddr offset,
@@ -249,6 +262,8 @@ static void sp804_write(void *opaque, hwaddr offset,
 {
     SP804State *s = (SP804State *)opaque;
 
+    trace_sp804_write(offset, value);
+
     if (offset < 0x20) {
         arm_timer_write(s->timer[0], offset, value);
         return;
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index db02a9142cd..600b002c7bf 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -87,3 +87,10 @@ pl031_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
 pl031_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
 pl031_alarm_raised(void) "alarm raised"
 pl031_set_alarm(uint32_t ticks) "alarm set for %u ticks"
+
+# arm_timer.c
+sp804_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
+sp804_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
+# Note that this trace event can't distinguish which of the two timers
+# in the sp804 is being updated
+sp804_arm_timer_update(int level) "level %d"
-- 
2.20.1

Re: [RFC 1/4] hw/timer/arm_timer: Add trace events

[Richard Henderson]

On 10/4/19 4:48 AM, Peter Maydell wrote:
> Add some basic trace events to the arm_timer device.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/arm_timer.c  | 27 +++++++++++++++++++++------
>  hw/timer/trace-events |  7 +++++++
>  2 files changed, 28 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[RFC 2/4] ptimer: Rename ptimer_init() to ptimer_init_with_bh()

[Peter Maydell]

Currently the ptimer design uses a QEMU bottom-half as its
mechanism for calling back into the device model using the
ptimer when the timer has expired. Unfortunately this design
is fatally flawed, because it means that there is a lag
between the ptimer updating its own state and the device
callback function updating device state, and guest accesses
to device registers between the two can return inconsistent
device state.

We want to replace the bottom-half design with one where
the guest device's callback is called either immediately
(when the ptimer triggers by timeout) or when the device
model code closes a transaction-begin/end section (when the
ptimer triggers because the device model changed the
ptimer's count value or other state). As the first step,
rename ptimer_init() to ptimer_init_with_bh(), to free up
the ptimer_init() name for the new API. We can then convert
all the ptimer users away from ptimer_init_with_bh() before
removing it entirely.

(Commit created with
 git grep -l ptimer_init | xargs sed -i -e 's/ptimer_init/ptimer_init_with_bh/'
and three overlong lines folded by hand.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/ptimer.h              | 11 ++++++-----
 hw/arm/musicpal.c                |  2 +-
 hw/core/ptimer.c                 |  2 +-
 hw/dma/xilinx_axidma.c           |  2 +-
 hw/m68k/mcf5206.c                |  2 +-
 hw/m68k/mcf5208.c                |  2 +-
 hw/net/fsl_etsec/etsec.c         |  2 +-
 hw/net/lan9118.c                 |  2 +-
 hw/timer/allwinner-a10-pit.c     |  2 +-
 hw/timer/altera_timer.c          |  2 +-
 hw/timer/arm_mptimer.c           |  6 +++---
 hw/timer/arm_timer.c             |  2 +-
 hw/timer/cmsdk-apb-dualtimer.c   |  2 +-
 hw/timer/cmsdk-apb-timer.c       |  2 +-
 hw/timer/digic-timer.c           |  2 +-
 hw/timer/etraxfs_timer.c         |  6 +++---
 hw/timer/exynos4210_mct.c        |  7 ++++---
 hw/timer/exynos4210_pwm.c        |  2 +-
 hw/timer/exynos4210_rtc.c        |  4 ++--
 hw/timer/grlib_gptimer.c         |  2 +-
 hw/timer/imx_epit.c              |  4 ++--
 hw/timer/imx_gpt.c               |  2 +-
 hw/timer/lm32_timer.c            |  2 +-
 hw/timer/milkymist-sysctl.c      |  4 ++--
 hw/timer/mss-timer.c             |  2 +-
 hw/timer/puv3_ost.c              |  2 +-
 hw/timer/sh_timer.c              |  2 +-
 hw/timer/slavio_timer.c          |  2 +-
 hw/timer/xilinx_timer.c          |  2 +-
 hw/watchdog/cmsdk-apb-watchdog.c |  2 +-
 tests/ptimer-test.c              | 22 +++++++++++-----------
 31 files changed, 56 insertions(+), 54 deletions(-)

diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index 9c770552290..2fb9ba1915e 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -72,7 +72,7 @@
  * ptimer_set_count() or ptimer_set_limit() will not trigger the timer
  * (though it will cause a reload). Only a counter decrement to "0"
  * will cause a trigger. Not compatible with NO_IMMEDIATE_TRIGGER;
- * ptimer_init() will assert() that you don't set both.
+ * ptimer_init_with_bh() will assert() that you don't set both.
  */
 #define PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT (1 << 5)
 
@@ -81,7 +81,7 @@ typedef struct ptimer_state ptimer_state;
 typedef void (*ptimer_cb)(void *opaque);
 
 /**
- * ptimer_init - Allocate and return a new ptimer
+ * ptimer_init_with_bh - Allocate and return a new ptimer
  * @bh: QEMU bottom half which is run on timer expiry
  * @policy: PTIMER_POLICY_* bits specifying behaviour
  *
@@ -89,13 +89,13 @@ typedef void (*ptimer_cb)(void *opaque);
  * The ptimer takes ownership of @bh and will delete it
  * when the ptimer is eventually freed.
  */
-ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask);
+ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask);
 
 /**
  * ptimer_free - Free a ptimer
  * @s: timer to free
  *
- * Free a ptimer created using ptimer_init() (including
+ * Free a ptimer created using ptimer_init_with_bh() (including
  * deleting the bottom half which it is using).
  */
 void ptimer_free(ptimer_state *s);
@@ -178,7 +178,8 @@ void ptimer_set_count(ptimer_state *s, uint64_t count);
  * @oneshot: non-zero if this timer should only count down once
  *
  * Start a ptimer counting down; when it reaches zero the bottom half
- * passed to ptimer_init() will be invoked. If the @oneshot argument is zero,
+ * passed to ptimer_init_with_bh() will be invoked.
+ * If the @oneshot argument is zero,
  * the counter value will then be reloaded from the limit and it will
  * start counting down again. If @oneshot is non-zero, then the counter
  * will disable itself when it reaches zero.
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 246cbb13363..b3624d5e280 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -849,7 +849,7 @@ static void mv88w8618_timer_init(SysBusDevice *dev, mv88w8618_timer_state *s,
     s->freq = freq;
 
     bh = qemu_bh_new(mv88w8618_timer_tick, s);
-    s->ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static uint64_t mv88w8618_pit_read(void *opaque, hwaddr offset,
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index d58e2dfdb08..f0d3ce11398 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -358,7 +358,7 @@ const VMStateDescription vmstate_ptimer = {
     }
 };
 
-ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
+ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask)
 {
     ptimer_state *s;
 
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index a254275b64e..e035d1f7504 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -552,7 +552,7 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
 
         st->nr = i;
         st->bh = qemu_bh_new(timer_hit, st);
-        st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
+        st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(st->ptimer, s->freqhz);
     }
     return;
diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index a9c2c95b0d1..a49096367cb 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -141,7 +141,7 @@ static m5206_timer_state *m5206_timer_init(qemu_irq irq)
 
     s = g_new0(m5206_timer_state, 1);
     bh = qemu_bh_new(m5206_timer_trigger, s);
-    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
     s->irq = irq;
     m5206_timer_reset(s);
     return s;
diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index 012710d057d..45c28b75a17 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -192,7 +192,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
     for (i = 0; i < 2; i++) {
         s = g_new0(m5208_timer_state, 1);
         bh = qemu_bh_new(m5208_timer_trigger, s);
-        s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+        s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
         memory_region_init_io(&s->iomem, NULL, &m5208_timer_ops, s,
                               "m5208-timer", 0x00004000);
         memory_region_add_subregion(address_space, 0xfc080000 + 0x4000 * i,
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 8451c17fb8f..d9b3e8c691e 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -393,7 +393,7 @@ static void etsec_realize(DeviceState *dev, Error **errp)
 
 
     etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
-    etsec->ptimer = ptimer_init(etsec->bh, PTIMER_POLICY_DEFAULT);
+    etsec->ptimer = ptimer_init_with_bh(etsec->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(etsec->ptimer, 100);
 }
 
diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 8bba2a80568..0ea51433dca 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -1350,7 +1350,7 @@ static void lan9118_realize(DeviceState *dev, Error **errp)
     s->txp = &s->tx_packet;
 
     bh = qemu_bh_new(lan9118_tick, s);
-    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->timer, 10000);
     ptimer_set_limit(s->timer, 0xffff, 1);
 }
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index ca5a9050591..28d055e42f3 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -271,7 +271,7 @@ static void a10_pit_init(Object *obj)
         tc->container = s;
         tc->index = i;
         bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
-        s->timer[i] = ptimer_init(bh[i], PTIMER_POLICY_DEFAULT);
+        s->timer[i] = ptimer_init_with_bh(bh[i], PTIMER_POLICY_DEFAULT);
     }
 }
 
diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c
index 936b31311d2..ee32e0ec1ff 100644
--- a/hw/timer/altera_timer.c
+++ b/hw/timer/altera_timer.c
@@ -184,7 +184,7 @@ static void altera_timer_realize(DeviceState *dev, Error **errp)
     }
 
     t->bh = qemu_bh_new(timer_hit, t);
-    t->ptimer = ptimer_init(t->bh, PTIMER_POLICY_DEFAULT);
+    t->ptimer = ptimer_init_with_bh(t->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(t->ptimer, t->freq_hz);
 
     memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t,
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 9f63abef10e..2a54a011431 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -228,7 +228,7 @@ static void arm_mptimer_reset(DeviceState *dev)
     }
 }
 
-static void arm_mptimer_init(Object *obj)
+static void arm_mptimer_init_with_bh(Object *obj)
 {
     ARMMPTimerState *s = ARM_MPTIMER(obj);
 
@@ -261,7 +261,7 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < s->num_cpu; i++) {
         TimerBlock *tb = &s->timerblock[i];
         QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
-        tb->timer = ptimer_init(bh, PTIMER_POLICY);
+        tb->timer = ptimer_init_with_bh(bh, PTIMER_POLICY);
         sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);
@@ -311,7 +311,7 @@ static const TypeInfo arm_mptimer_info = {
     .name          = TYPE_ARM_MPTIMER,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(ARMMPTimerState),
-    .instance_init = arm_mptimer_init,
+    .instance_init = arm_mptimer_init_with_bh,
     .class_init    = arm_mptimer_class_init,
 };
 
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 283ae1e74a9..848fbcb0e25 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -177,7 +177,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
     s->control = TIMER_CTRL_IE;
 
     bh = qemu_bh_new(arm_timer_tick, s);
-    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
     vmstate_register(NULL, -1, &vmstate_arm_timer, s);
     return s;
 }
diff --git a/hw/timer/cmsdk-apb-dualtimer.c b/hw/timer/cmsdk-apb-dualtimer.c
index 5e2352dd326..44d23c80364 100644
--- a/hw/timer/cmsdk-apb-dualtimer.c
+++ b/hw/timer/cmsdk-apb-dualtimer.c
@@ -453,7 +453,7 @@ static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp)
         QEMUBH *bh = qemu_bh_new(cmsdk_dualtimermod_tick, m);
 
         m->parent = s;
-        m->timer = ptimer_init(bh,
+        m->timer = ptimer_init_with_bh(bh,
                                PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |
                                PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT |
                                PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
diff --git a/hw/timer/cmsdk-apb-timer.c b/hw/timer/cmsdk-apb-timer.c
index c83e26566a9..c9ce9770cef 100644
--- a/hw/timer/cmsdk-apb-timer.c
+++ b/hw/timer/cmsdk-apb-timer.c
@@ -218,7 +218,7 @@ static void cmsdk_apb_timer_realize(DeviceState *dev, Error **errp)
     }
 
     bh = qemu_bh_new(cmsdk_apb_timer_tick, s);
-    s->timer = ptimer_init(bh,
+    s->timer = ptimer_init_with_bh(bh,
                            PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |
                            PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT |
                            PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
diff --git a/hw/timer/digic-timer.c b/hw/timer/digic-timer.c
index 021c4ef714f..b111e1fe643 100644
--- a/hw/timer/digic-timer.c
+++ b/hw/timer/digic-timer.c
@@ -129,7 +129,7 @@ static void digic_timer_init(Object *obj)
 {
     DigicTimerState *s = DIGIC_TIMER(obj);
 
-    s->ptimer = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init_with_bh(NULL, PTIMER_POLICY_DEFAULT);
 
     /*
      * FIXME: there is no documentation on Digic timer
diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index d62025b8797..ab27fe1895b 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -328,9 +328,9 @@ static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
     t->bh_t0 = qemu_bh_new(timer0_hit, t);
     t->bh_t1 = qemu_bh_new(timer1_hit, t);
     t->bh_wd = qemu_bh_new(watchdog_hit, t);
-    t->ptimer_t0 = ptimer_init(t->bh_t0, PTIMER_POLICY_DEFAULT);
-    t->ptimer_t1 = ptimer_init(t->bh_t1, PTIMER_POLICY_DEFAULT);
-    t->ptimer_wd = ptimer_init(t->bh_wd, PTIMER_POLICY_DEFAULT);
+    t->ptimer_t0 = ptimer_init_with_bh(t->bh_t0, PTIMER_POLICY_DEFAULT);
+    t->ptimer_t1 = ptimer_init_with_bh(t->bh_t1, PTIMER_POLICY_DEFAULT);
+    t->ptimer_wd = ptimer_init_with_bh(t->bh_wd, PTIMER_POLICY_DEFAULT);
 
     sysbus_init_irq(sbd, &t->irq);
     sysbus_init_irq(sbd, &t->nmi);
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 77b9af05f41..9f2e8dd0a42 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1429,7 +1429,7 @@ static void exynos4210_mct_init(Object *obj)
 
     /* Global timer */
     bh[0] = qemu_bh_new(exynos4210_gfrc_event, s);
-    s->g_timer.ptimer_frc = ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
+    s->g_timer.ptimer_frc = ptimer_init_with_bh(bh[0], PTIMER_POLICY_DEFAULT);
     memset(&s->g_timer.reg, 0, sizeof(struct gregs));
 
     /* Local timers */
@@ -1437,8 +1437,9 @@ static void exynos4210_mct_init(Object *obj)
         bh[0] = qemu_bh_new(exynos4210_ltick_event, &s->l_timer[i]);
         bh[1] = qemu_bh_new(exynos4210_lfrc_event, &s->l_timer[i]);
         s->l_timer[i].tick_timer.ptimer_tick =
-                                   ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
-        s->l_timer[i].ptimer_frc = ptimer_init(bh[1], PTIMER_POLICY_DEFAULT);
+            ptimer_init_with_bh(bh[0], PTIMER_POLICY_DEFAULT);
+        s->l_timer[i].ptimer_frc =
+            ptimer_init_with_bh(bh[1], PTIMER_POLICY_DEFAULT);
         s->l_timer[i].id = i;
     }
 
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index b7fad2ad449..aa5dca68ef7 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -393,7 +393,7 @@ static void exynos4210_pwm_init(Object *obj)
     for (i = 0; i < EXYNOS4210_PWM_TIMERS_NUM; i++) {
         bh = qemu_bh_new(exynos4210_pwm_tick, &s->timer[i]);
         sysbus_init_irq(dev, &s->timer[i].irq);
-        s->timer[i].ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+        s->timer[i].ptimer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
         s->timer[i].id = i;
         s->timer[i].parent = s;
     }
diff --git a/hw/timer/exynos4210_rtc.c b/hw/timer/exynos4210_rtc.c
index ea689042297..d5d7c91fb15 100644
--- a/hw/timer/exynos4210_rtc.c
+++ b/hw/timer/exynos4210_rtc.c
@@ -558,12 +558,12 @@ static void exynos4210_rtc_init(Object *obj)
     QEMUBH *bh;
 
     bh = qemu_bh_new(exynos4210_rtc_tick, s);
-    s->ptimer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer, RTC_BASE_FREQ);
     exynos4210_rtc_update_freq(s, 0);
 
     bh = qemu_bh_new(exynos4210_rtc_1Hz_tick, s);
-    s->ptimer_1Hz = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer_1Hz = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer_1Hz, RTC_BASE_FREQ);
 
     sysbus_init_irq(dev, &s->alm_irq);
diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index 32dbf870d4b..bb09268ea14 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -366,7 +366,7 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
 
         timer->unit   = unit;
         timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
-        timer->ptimer = ptimer_init(timer->bh, PTIMER_POLICY_DEFAULT);
+        timer->ptimer = ptimer_init_with_bh(timer->bh, PTIMER_POLICY_DEFAULT);
         timer->id     = i;
 
         /* One IRQ line for each timer */
diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index f54e059910b..39810ac8b03 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -317,10 +317,10 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
                           0x00001000);
     sysbus_init_mmio(sbd, &s->iomem);
 
-    s->timer_reload = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
+    s->timer_reload = ptimer_init_with_bh(NULL, PTIMER_POLICY_DEFAULT);
 
     bh = qemu_bh_new(imx_epit_cmp, s);
-    s->timer_cmp = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->timer_cmp = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static void imx_epit_class_init(ObjectClass *klass, void *data)
diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index 49a441f4517..c535d191292 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -492,7 +492,7 @@ static void imx_gpt_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
 
     bh = qemu_bh_new(imx_gpt_timeout, s);
-    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static void imx_gpt_class_init(ObjectClass *klass, void *data)
diff --git a/hw/timer/lm32_timer.c b/hw/timer/lm32_timer.c
index ac3edaff4f8..f79f818d22c 100644
--- a/hw/timer/lm32_timer.c
+++ b/hw/timer/lm32_timer.c
@@ -187,7 +187,7 @@ static void lm32_timer_init(Object *obj)
     sysbus_init_irq(dev, &s->irq);
 
     s->bh = qemu_bh_new(timer_hit, s);
-    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);
 
     memory_region_init_io(&s->iomem, obj, &timer_ops, s,
                           "timer", R_MAX * 4);
diff --git a/hw/timer/milkymist-sysctl.c b/hw/timer/milkymist-sysctl.c
index 958350767ae..aeba889bba4 100644
--- a/hw/timer/milkymist-sysctl.c
+++ b/hw/timer/milkymist-sysctl.c
@@ -285,8 +285,8 @@ static void milkymist_sysctl_init(Object *obj)
 
     s->bh0 = qemu_bh_new(timer0_hit, s);
     s->bh1 = qemu_bh_new(timer1_hit, s);
-    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
-    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
+    s->ptimer0 = ptimer_init_with_bh(s->bh0, PTIMER_POLICY_DEFAULT);
+    s->ptimer1 = ptimer_init_with_bh(s->bh1, PTIMER_POLICY_DEFAULT);
 
     memory_region_init_io(&s->regs_region, obj, &sysctl_mmio_ops, s,
             "milkymist-sysctl", R_MAX * 4);
diff --git a/hw/timer/mss-timer.c b/hw/timer/mss-timer.c
index 45f1cf42f9e..a34c2402b00 100644
--- a/hw/timer/mss-timer.c
+++ b/hw/timer/mss-timer.c
@@ -229,7 +229,7 @@ static void mss_timer_init(Object *obj)
         struct Msf2Timer *st = &t->timers[i];
 
         st->bh = qemu_bh_new(timer_hit, st);
-        st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
+        st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(st->ptimer, t->freq_hz);
         sysbus_init_irq(SYS_BUS_DEVICE(obj), &st->irq);
     }
diff --git a/hw/timer/puv3_ost.c b/hw/timer/puv3_ost.c
index 6fe370049b5..0898da5ce97 100644
--- a/hw/timer/puv3_ost.c
+++ b/hw/timer/puv3_ost.c
@@ -129,7 +129,7 @@ static void puv3_ost_realize(DeviceState *dev, Error **errp)
     sysbus_init_irq(sbd, &s->irq);
 
     s->bh = qemu_bh_new(puv3_ost_tick, s);
-    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer, 50 * 1000 * 1000);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &puv3_ost_ops, s, "puv3_ost",
diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index adcc0c138e7..48a81b4dc79 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -204,7 +204,7 @@ static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
     s->irq = irq;
 
     bh = qemu_bh_new(sh_timer_tick, s);
-    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
 
     sh_timer_write(s, OFFSET_TCOR >> 2, s->tcor);
     sh_timer_write(s, OFFSET_TCNT >> 2, s->tcnt);
diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
index 38fd32b62a0..692d213897d 100644
--- a/hw/timer/slavio_timer.c
+++ b/hw/timer/slavio_timer.c
@@ -393,7 +393,7 @@ static void slavio_timer_init(Object *obj)
         tc->timer_index = i;
 
         bh = qemu_bh_new(slavio_timer_irq, tc);
-        s->cputimer[i].timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
+        s->cputimer[i].timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD);
 
         size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE;
diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
index 355518232cd..92dbff304d9 100644
--- a/hw/timer/xilinx_timer.c
+++ b/hw/timer/xilinx_timer.c
@@ -221,7 +221,7 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
         xt->parent = t;
         xt->nr = i;
         xt->bh = qemu_bh_new(timer_hit, xt);
-        xt->ptimer = ptimer_init(xt->bh, PTIMER_POLICY_DEFAULT);
+        xt->ptimer = ptimer_init_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(xt->ptimer, t->freq_hz);
     }
 
diff --git a/hw/watchdog/cmsdk-apb-watchdog.c b/hw/watchdog/cmsdk-apb-watchdog.c
index 6bf43f943fb..e42c3ebd29d 100644
--- a/hw/watchdog/cmsdk-apb-watchdog.c
+++ b/hw/watchdog/cmsdk-apb-watchdog.c
@@ -329,7 +329,7 @@ static void cmsdk_apb_watchdog_realize(DeviceState *dev, Error **errp)
     }
 
     bh = qemu_bh_new(cmsdk_apb_watchdog_tick, s);
-    s->timer = ptimer_init(bh,
+    s->timer = ptimer_init_with_bh(bh,
                            PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD |
                            PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT |
                            PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
diff --git a/tests/ptimer-test.c b/tests/ptimer-test.c
index 5b20e91599e..a3c82d1d147 100644
--- a/tests/ptimer-test.c
+++ b/tests/ptimer-test.c
@@ -68,7 +68,7 @@ static void check_set_count(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
 
     triggered = false;
 
@@ -82,7 +82,7 @@ static void check_set_limit(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
 
     triggered = false;
 
@@ -102,7 +102,7 @@ static void check_oneshot(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool no_round_down = (*policy & PTIMER_POLICY_NO_COUNTER_ROUND_DOWN);
 
     triggered = false;
@@ -205,7 +205,7 @@ static void check_periodic(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
     bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
     bool no_immediate_reload = (*policy & PTIMER_POLICY_NO_IMMEDIATE_RELOAD);
@@ -373,7 +373,7 @@ static void check_on_the_fly_mode_change(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
     bool no_round_down = (*policy & PTIMER_POLICY_NO_COUNTER_ROUND_DOWN);
 
@@ -420,7 +420,7 @@ static void check_on_the_fly_period_change(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool no_round_down = (*policy & PTIMER_POLICY_NO_COUNTER_ROUND_DOWN);
 
     triggered = false;
@@ -453,7 +453,7 @@ static void check_on_the_fly_freq_change(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool no_round_down = (*policy & PTIMER_POLICY_NO_COUNTER_ROUND_DOWN);
 
     triggered = false;
@@ -486,7 +486,7 @@ static void check_run_with_period_0(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
 
     triggered = false;
 
@@ -504,7 +504,7 @@ static void check_run_with_delta_0(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool wrap_policy = (*policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD);
     bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
     bool no_immediate_reload = (*policy & PTIMER_POLICY_NO_IMMEDIATE_RELOAD);
@@ -610,7 +610,7 @@ static void check_periodic_with_load_0(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool continuous_trigger = (*policy & PTIMER_POLICY_CONTINUOUS_TRIGGER);
     bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
     bool trig_only_on_dec = (*policy & PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT);
@@ -670,7 +670,7 @@ static void check_oneshot_with_load_0(gconstpointer arg)
 {
     const uint8_t *policy = arg;
     QEMUBH *bh = qemu_bh_new(ptimer_trigger, NULL);
-    ptimer_state *ptimer = ptimer_init(bh, *policy);
+    ptimer_state *ptimer = ptimer_init_with_bh(bh, *policy);
     bool no_immediate_trigger = (*policy & PTIMER_POLICY_NO_IMMEDIATE_TRIGGER);
     bool trig_only_on_dec = (*policy & PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT);
 
-- 
2.20.1

Re: [RFC 2/4] ptimer: Rename ptimer_init() to ptimer_init_with_bh()

[Richard Henderson]

On 10/4/19 4:48 AM, Peter Maydell wrote:
> Currently the ptimer design uses a QEMU bottom-half as its
> mechanism for calling back into the device model using the
> ptimer when the timer has expired. Unfortunately this design
> is fatally flawed, because it means that there is a lag
> between the ptimer updating its own state and the device
> callback function updating device state, and guest accesses
> to device registers between the two can return inconsistent
> device state.
> 
> We want to replace the bottom-half design with one where
> the guest device's callback is called either immediately
> (when the ptimer triggers by timeout) or when the device
> model code closes a transaction-begin/end section (when the
> ptimer triggers because the device model changed the
> ptimer's count value or other state). As the first step,
> rename ptimer_init() to ptimer_init_with_bh(), to free up
> the ptimer_init() name for the new API. We can then convert
> all the ptimer users away from ptimer_init_with_bh() before
> removing it entirely.
> 
> (Commit created with
>  git grep -l ptimer_init | xargs sed -i -e 's/ptimer_init/ptimer_init_with_bh/'
> and three overlong lines folded by hand.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[RFC 3/4] ptimer: Provide new transaction-based API

[Peter Maydell]

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().

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>
---
 include/hw/ptimer.h |  66 +++++++++++++++++++++++++
 hw/core/ptimer.c    | 114 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 169 insertions(+), 11 deletions(-)

diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index 2fb9ba1915e..b9cd301ab02 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -91,6 +91,32 @@ typedef void (*ptimer_cb)(void *opaque);
  */
 ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask);
 
+/**
+ * ptimer_init - Allocate and return a new ptimer
+ * @callback: function to call on ptimer expiry
+ * @callback_data: opaque pointer passed to @callback
+ * @policy: PTIMER_POLICY_* bits specifying behaviour
+ *
+ * The ptimer returned must be freed using ptimer_free().
+ *
+ * If a ptimer is created using this API then will use the
+ * transaction-based API for modifying ptimer state: 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.
+ */
+ptimer_state *ptimer_init(ptimer_cb callback,
+                          void *callback_opaque,
+                          uint8_t policy_mask);
+
 /**
  * ptimer_free - Free a ptimer
  * @s: timer to free
@@ -100,6 +126,28 @@ ptimer_state *ptimer_init_with_bh(QEMUBH *bh, uint8_t policy_mask);
  */
 void ptimer_free(ptimer_state *s);
 
+/**
+ * ptimer_transaction_begin() - Start a ptimer modification transaction
+ *
+ * This function must be called before making any calls to functions
+ * which modify the ptimer's state (see the ptimer_init() documentation
+ * for a list of these), and must always have a matched call to
+ * ptimer_transaction_commit().
+ * It is an error to call this function for a BH-based ptimer;
+ * attempting to do this will trigger an assert.
+ */
+void ptimer_transaction_begin(ptimer_state *s);
+
+/**
+ * ptimer_transaction_commit() - Commit a ptimer modification transaction
+ *
+ * This function must be called after calls to functions which modify
+ * the ptimer's state, and completes the update of the ptimer. If the
+ * ptimer state now means that we should trigger the timer expiry
+ * callback, it will be called directly.
+ */
+void ptimer_transaction_commit(ptimer_state *s);
+
 /**
  * ptimer_set_period - Set counter increment interval in nanoseconds
  * @s: ptimer to configure
@@ -108,6 +156,9 @@ void ptimer_free(ptimer_state *s);
  * Note that if your counter behaviour is specified as having a
  * particular frequency rather than a period then ptimer_set_freq()
  * may be more appropriate.
+ *
+ * This function will assert if it is called outside a
+ * ptimer_transaction_begin/commit block, unless this is a bottom-half ptimer.
  */
 void ptimer_set_period(ptimer_state *s, int64_t period);
 
@@ -121,6 +172,9 @@ void ptimer_set_period(ptimer_state *s, int64_t period);
  * as setting the frequency then this function is more appropriate,
  * because it allows specifying an effective period which is
  * precise to fractions of a nanosecond, avoiding rounding errors.
+ *
+ * This function will assert if it is called outside a
+ * ptimer_transaction_begin/commit block, unless this is a bottom-half ptimer.
  */
 void ptimer_set_freq(ptimer_state *s, uint32_t freq);
 
@@ -148,6 +202,9 @@ uint64_t ptimer_get_limit(ptimer_state *s);
  * Set the limit value of the down-counter. The @reload flag can
  * be used to emulate the behaviour of timers which immediately
  * reload the counter when their reload register is written to.
+ *
+ * This function will assert if it is called outside a
+ * ptimer_transaction_begin/commit block, unless this is a bottom-half ptimer.
  */
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload);
 
@@ -169,6 +226,9 @@ uint64_t ptimer_get_count(ptimer_state *s);
  * Set the value of the down-counter. If the counter is currently
  * enabled this will arrange for a timer callback at the appropriate
  * point in the future.
+ *
+ * This function will assert if it is called outside a
+ * ptimer_transaction_begin/commit block, unless this is a bottom-half ptimer.
  */
 void ptimer_set_count(ptimer_state *s, uint64_t count);
 
@@ -183,6 +243,9 @@ void ptimer_set_count(ptimer_state *s, uint64_t count);
  * the counter value will then be reloaded from the limit and it will
  * start counting down again. If @oneshot is non-zero, then the counter
  * will disable itself when it reaches zero.
+ *
+ * This function will assert if it is called outside a
+ * ptimer_transaction_begin/commit block, unless this is a bottom-half ptimer.
  */
 void ptimer_run(ptimer_state *s, int oneshot);
 
@@ -195,6 +258,9 @@ void ptimer_run(ptimer_state *s, int oneshot);
  *
  * Note that this can cause it to "lose" time, even if it is immediately
  * restarted.
+ *
+ * This function will assert if it is called outside a
+ * ptimer_transaction_begin/commit block, unless this is a bottom-half ptimer.
  */
 void ptimer_stop(ptimer_state *s);
 
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index f0d3ce11398..89228132650 100644
--- a/hw/core/ptimer.c
+++ b/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,6 +49,9 @@ 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)
@@ -263,10 +276,15 @@ 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;
+        }
     }
 }
 
@@ -274,6 +292,8 @@ 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,8 +302,12 @@ 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;
+        }
     }
 }
 
@@ -291,35 +315,50 @@ void ptimer_run(ptimer_state *s, int oneshot)
    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;
+        }
     }
 }
 
@@ -327,12 +366,17 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
    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 +385,22 @@ 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;
+}
+
+void ptimer_transaction_commit(ptimer_state *s)
+{
+    assert(s->in_transaction);
+    s->in_transaction = false;
+    if (s->need_reload) {
+        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        ptimer_reload(s, 0);
+    }
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
     .version_id = 1,
@@ -377,9 +437,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);
 }
-- 
2.20.1

Re: [RFC 3/4] ptimer: Provide new transaction-based API

[Peter Maydell]

On Fri, 4 Oct 2019 at 12:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Provide the new transaction-based API.


> +void ptimer_transaction_begin(ptimer_state *s)
> +{
> +    assert(!s->in_transaction && s->callback);
> +    s->in_transaction = true;

As I was working on converting the ptimer tests to the new
API I noticed a silly bug here -- we should set
       s->need_reload = false;
in ptimer_transaction_begin(). Otherwise one transaction has
decided to do a reload, we'll do a reload in commit every time,
including when that's bogus (eg when the timer is disabled)...

thanks
-- PMM

Re: [RFC 3/4] ptimer: Provide new transaction-based API

[Richard Henderson]

On 10/4/19 4:48 AM, Peter Maydell wrote:
> 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().
> 
> 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>

With the need_reload fix, as you noted, and one other nit,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


> +/**
> + * ptimer_init - Allocate and return a new ptimer
> + * @callback: function to call on ptimer expiry
> + * @callback_data: opaque pointer passed to @callback

Comment mismatch with...

> +ptimer_state *ptimer_init(ptimer_cb callback,
> +                          void *callback_opaque,

the declaration and all the uses.  Either name works for me.


r~

[RFC 4/4] hw/timer/arm_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the arm_timer.c code away from bottom-half based ptimers
to the new transaction-based ptimer API. This just requires
adding begin/commit calls around the various arms of
arm_timer_write() that modify the ptimer state, and using the
new ptimer_init() function to create the timer.

Fixes: https://bugs.launchpad.net/qemu/+bug/1777777
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 848fbcb0e25..255def3deec 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -14,7 +14,6 @@
 #include "hw/irq.h"
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qemu/log.h"
 #include "trace.h"
@@ -79,7 +78,10 @@ static uint32_t arm_timer_read(void *opaque, hwaddr offset)
     }
 }
 
-/* Reset the timer limit after settings have changed.  */
+/*
+ * Reset the timer limit after settings have changed.
+ * May only be called from inside a ptimer transaction block.
+ */
 static void arm_timer_recalibrate(arm_timer_state *s, int reload)
 {
     uint32_t limit;
@@ -106,13 +108,16 @@ static void arm_timer_write(void *opaque, hwaddr offset,
     switch (offset >> 2) {
     case 0: /* TimerLoad */
         s->limit = value;
+        ptimer_transaction_begin(s->timer);
         arm_timer_recalibrate(s, 1);
+        ptimer_transaction_commit(s->timer);
         break;
     case 1: /* TimerValue */
         /* ??? Linux seems to want to write to this readonly register.
            Ignore it.  */
         break;
     case 2: /* TimerControl */
+        ptimer_transaction_begin(s->timer);
         if (s->control & TIMER_CTRL_ENABLE) {
             /* Pause the timer if it is running.  This may cause some
                inaccuracy dure to rounding, but avoids a whole lot of other
@@ -132,13 +137,16 @@ static void arm_timer_write(void *opaque, hwaddr offset,
             /* Restart the timer if still enabled.  */
             ptimer_run(s->timer, (s->control & TIMER_CTRL_ONESHOT) != 0);
         }
+        ptimer_transaction_commit(s->timer);
         break;
     case 3: /* TimerIntClr */
         s->int_level = 0;
         break;
     case 6: /* TimerBGLoad */
         s->limit = value;
+        ptimer_transaction_begin(s->timer);
         arm_timer_recalibrate(s, 0);
+        ptimer_transaction_commit(s->timer);
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -170,14 +178,12 @@ static const VMStateDescription vmstate_arm_timer = {
 static arm_timer_state *arm_timer_init(uint32_t freq)
 {
     arm_timer_state *s;
-    QEMUBH *bh;
 
     s = (arm_timer_state *)g_malloc0(sizeof(arm_timer_state));
     s->freq = freq;
     s->control = TIMER_CTRL_IE;
 
-    bh = qemu_bh_new(arm_timer_tick, s);
-    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_DEFAULT);
     vmstate_register(NULL, -1, &vmstate_arm_timer, s);
     return s;
 }
-- 
2.20.1

Re: [RFC 4/4] hw/timer/arm_timer.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/4/19 4:48 AM, Peter Maydell wrote:
> Switch the arm_timer.c code away from bottom-half based ptimers
> to the new transaction-based ptimer API. This just requires
> adding begin/commit calls around the various arms of
> arm_timer_write() that modify the ptimer state, and using the
> new ptimer_init() function to create the timer.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1777777
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/arm_timer.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [RFC 0/4] transaction-based ptimer API

[Paolo Bonzini]

On 04/10/19 13:48, Peter Maydell wrote:
> 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.

Could ptimer_stop/ptimer_run act as the begin and commit functions?
That is, ptimer_set_* can be called only between ptimer_stop and ptimer_run?

Thanks,

Paolo

Re: [RFC 0/4] transaction-based ptimer API

[Peter Maydell]

On Fri, 4 Oct 2019 at 12:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/10/19 13:48, Peter Maydell wrote:
> > 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.
>
> Could ptimer_stop/ptimer_run act as the begin and commit functions?
> That is, ptimer_set_* can be called only between ptimer_stop and ptimer_run?

No, because stop/run causes the ptimer to "lose time"
(we stop the underlying timer and restart it). It's
very common for a device to want to change the ptimer
properties without a stop/restart -- "set the ptimer
count value when the guest writes to the device's counter
register" is the common one. Of the three begin/commit
blocks in the arm_timer.c conversion, only one of those
involves calls to stop/run, and even there we only
call stop/run if the write to the control register
modified the enable bit.

thanks
-- PMM

Re: [RFC 0/4] transaction-based ptimer API

[Paolo Bonzini]

On 04/10/19 14:00, Peter Maydell wrote:
> No, because stop/run causes the ptimer to "lose time"
> (we stop the underlying timer and restart it). It's
> very common for a device to want to change the ptimer
> properties without a stop/restart -- "set the ptimer
> count value when the guest writes to the device's counter
> register" is the common one. Of the three begin/commit
> blocks in the arm_timer.c conversion, only one of those
> involves calls to stop/run, and even there we only
> call stop/run if the write to the control register
> modified the enable bit.

Ok, thanks.

Paolo