[Qemu-devel] [PATCH v14 0/3] PTimer fixes/features and ARM MPTimer conversion

[Qemu-devel] [PATCH v14 0/3] PTimer fixes/features and ARM MPTimer conversion

[Dmitry Osipenko]

Hello,

Current QEMU ARM MPTimer device model provides only a certain subset of the
emulation behavior, so this patch series is supposed to add missing parts by
converting the MPTimer to use generic ptimer helper. It fixes some important
ptimer bugs and provides new features that are required for the ARM MPTimer.

Emulation behavior is verified against the real HW by running specially
crafted MPTimer tests in both icount and non-icount modes:

https://gist.github.com/digetx/dbd46109503b1a91941a

Changelog:

    I ommitted old changelog since the rest of the precursor ptimer patches
    already been applied and the ARM MPTimer patch has Peter's Crosthwaite r-b.

    V14: Set the ptimer policy in the ptimer_init() instead of adding
         ptimer_set_policy(), keeping ptimer VMState unchanged and dropped
         hw_error() hardening asserts as per Peter's Maydell V13 review
         comments, addressed the rest of the review comments.

Dmitry Osipenko (3):
  hw/ptimer: Support running with counter = 0 by introducing new policy
    feature
  hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active
    timer
  arm_mptimer: Convert to use ptimer

 hw/arm/musicpal.c              |   2 +-
 hw/core/ptimer.c               |  49 ++++++++-------
 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/arm_mptimer.c         | 133 +++++++++++++++++++++--------------------
 hw/timer/arm_timer.c           |   2 +-
 hw/timer/aspeed_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/puv3_ost.c            |   2 +-
 hw/timer/sh_timer.c            |   2 +-
 hw/timer/slavio_timer.c        |   2 +-
 hw/timer/xilinx_timer.c        |   2 +-
 include/hw/ptimer.h            |   7 ++-
 include/hw/timer/arm_mptimer.h |   5 +-
 27 files changed, 133 insertions(+), 122 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Dmitry Osipenko]

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.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/arm/musicpal.c            |  2 +-
 hw/core/ptimer.c             | 45 +++++++++++++++++++++++---------------------
 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/arm_timer.c         |  2 +-
 hw/timer/aspeed_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/puv3_ost.c          |  2 +-
 hw/timer/sh_timer.c          |  2 +-
 hw/timer/slavio_timer.c      |  2 +-
 hw/timer/xilinx_timer.c      |  2 +-
 include/hw/ptimer.h          |  7 ++++++-
 25 files changed, 61 insertions(+), 52 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 7a4cc07..087696a 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -837,7 +837,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);
+    s->ptimer = ptimer_init(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 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;
     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) {
         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;
-    }
     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);
     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,
     .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;
 }
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index a4753e5..b135a5f 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -548,7 +548,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);
+        st->ptimer = ptimer_init(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 e14896e..b81901f 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -139,7 +139,7 @@ static m5206_timer_state *m5206_timer_init(qemu_irq irq)
 
     s = (m5206_timer_state *)g_malloc0(sizeof(m5206_timer_state));
     bh = qemu_bh_new(m5206_timer_trigger, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(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 2415557..9240ebf 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -183,7 +183,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
     for (i = 0; i < 2; i++) {
         s = (m5208_timer_state *)g_malloc0(sizeof(m5208_timer_state));
         bh = qemu_bh_new(m5208_timer_trigger, s);
-        s->timer = ptimer_init(bh);
+        s->timer = ptimer_init(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 98250e0..8f640b0 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -387,7 +387,7 @@ static void etsec_realize(DeviceState *dev, Error **errp)
 
 
     etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
-    etsec->ptimer = ptimer_init(etsec->bh);
+    etsec->ptimer = ptimer_init(etsec->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(etsec->ptimer, 100);
 }
 
diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 2052073..650a6c7 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -1345,7 +1345,7 @@ static int lan9118_init1(SysBusDevice *sbd)
     s->txp = &s->tx_packet;
 
     bh = qemu_bh_new(lan9118_tick, s);
-    s->timer = ptimer_init(bh);
+    s->timer = ptimer_init(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 3385e5d..22ceabe 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -267,7 +267,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]);
+        s->timer[i] = ptimer_init(bh[i], PTIMER_POLICY_DEFAULT);
     }
 }
 
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 111a16d..98fddd7 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -171,7 +171,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);
+    s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
     vmstate_register(NULL, -1, &vmstate_arm_timer, s);
     return s;
 }
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 4b94808..f48799d 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -356,7 +356,7 @@ static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
 
     t->id = id;
     bh = qemu_bh_new(aspeed_timer_expire, t);
-    t->timer = ptimer_init(bh);
+    t->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
 }
 
 static void aspeed_timer_realize(DeviceState *dev, Error **errp)
diff --git a/hw/timer/digic-timer.c b/hw/timer/digic-timer.c
index 0f21faf..e1fcf73 100644
--- a/hw/timer/digic-timer.c
+++ b/hw/timer/digic-timer.c
@@ -127,7 +127,7 @@ static void digic_timer_init(Object *obj)
 {
     DigicTimerState *s = DIGIC_TIMER(obj);
 
-    s->ptimer = ptimer_init(NULL);
+    s->ptimer = ptimer_init(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 36d8f46..8e18236 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -322,9 +322,9 @@ static int etraxfs_timer_init(SysBusDevice *dev)
     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);
-    t->ptimer_t1 = ptimer_init(t->bh_t1);
-    t->ptimer_wd = ptimer_init(t->bh_wd);
+    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);
 
     sysbus_init_irq(dev, &t->irq);
     sysbus_init_irq(dev, &t->nmi);
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index ae69345..0c18934 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1431,15 +1431,16 @@ 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]);
+    s->g_timer.ptimer_frc = ptimer_init(bh[0], PTIMER_POLICY_DEFAULT);
     memset(&s->g_timer.reg, 0, sizeof(struct gregs));
 
     /* Local timers */
     for (i = 0; i < 2; i++) {
         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]);
-        s->l_timer[i].ptimer_frc = ptimer_init(bh[1]);
+        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);
         s->l_timer[i].id = i;
     }
 
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index 0e9e2e9..f576507 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -390,7 +390,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);
+        s->timer[i].ptimer = ptimer_init(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 da4dd45..1a648c5 100644
--- a/hw/timer/exynos4210_rtc.c
+++ b/hw/timer/exynos4210_rtc.c
@@ -555,12 +555,12 @@ static void exynos4210_rtc_init(Object *obj)
     QEMUBH *bh;
 
     bh = qemu_bh_new(exynos4210_rtc_tick, s);
-    s->ptimer = ptimer_init(bh);
+    s->ptimer = ptimer_init(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);
+    s->ptimer_1Hz = ptimer_init(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 dd000f5..712d1ae 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -363,7 +363,7 @@ static int grlib_gptimer_init(SysBusDevice *dev)
 
         timer->unit   = unit;
         timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
-        timer->ptimer = ptimer_init(timer->bh);
+        timer->ptimer = ptimer_init(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 eddf348..f34d7f7 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -314,10 +314,10 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
                           0x00001000);
     sysbus_init_mmio(sbd, &s->iomem);
 
-    s->timer_reload = ptimer_init(NULL);
+    s->timer_reload = ptimer_init(NULL, PTIMER_POLICY_DEFAULT);
 
     bh = qemu_bh_new(imx_epit_cmp, s);
-    s->timer_cmp = ptimer_init(bh);
+    s->timer_cmp = ptimer_init(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 3c2f01a..b864ac3 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -440,7 +440,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);
+    s->timer = ptimer_init(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 3198355..a7a6b57 100644
--- a/hw/timer/lm32_timer.c
+++ b/hw/timer/lm32_timer.c
@@ -183,7 +183,7 @@ static int lm32_timer_init(SysBusDevice *dev)
     sysbus_init_irq(dev, &s->irq);
 
     s->bh = qemu_bh_new(timer_hit, s);
-    s->ptimer = ptimer_init(s->bh);
+    s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer, s->freq_hz);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &timer_ops, s,
diff --git a/hw/timer/milkymist-sysctl.c b/hw/timer/milkymist-sysctl.c
index 5f29480..818f2d9 100644
--- a/hw/timer/milkymist-sysctl.c
+++ b/hw/timer/milkymist-sysctl.c
@@ -280,8 +280,8 @@ static int milkymist_sysctl_init(SysBusDevice *dev)
 
     s->bh0 = qemu_bh_new(timer0_hit, s);
     s->bh1 = qemu_bh_new(timer1_hit, s);
-    s->ptimer0 = ptimer_init(s->bh0);
-    s->ptimer1 = ptimer_init(s->bh1);
+    s->ptimer0 = ptimer_init(s->bh0, PTIMER_POLICY_DEFAULT);
+    s->ptimer1 = ptimer_init(s->bh1, PTIMER_POLICY_DEFAULT);
     ptimer_set_freq(s->ptimer0, s->freq_hz);
     ptimer_set_freq(s->ptimer1, s->freq_hz);
 
diff --git a/hw/timer/puv3_ost.c b/hw/timer/puv3_ost.c
index 93650b7..0b3d717 100644
--- a/hw/timer/puv3_ost.c
+++ b/hw/timer/puv3_ost.c
@@ -125,7 +125,7 @@ static int puv3_ost_init(SysBusDevice *dev)
     sysbus_init_irq(dev, &s->irq);
 
     s->bh = qemu_bh_new(puv3_ost_tick, s);
-    s->ptimer = ptimer_init(s->bh);
+    s->ptimer = ptimer_init(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 255b2fc..9afb2d0 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -203,7 +203,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);
+    s->timer = ptimer_init(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 fb3e08b..bfee1f3 100644
--- a/hw/timer/slavio_timer.c
+++ b/hw/timer/slavio_timer.c
@@ -389,7 +389,7 @@ static int slavio_timer_init1(SysBusDevice *dev)
         tc->timer_index = i;
 
         bh = qemu_bh_new(slavio_timer_irq, tc);
-        s->cputimer[i].timer = ptimer_init(bh);
+        s->cputimer[i].timer = ptimer_init(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 2ea970d..59439c0 100644
--- a/hw/timer/xilinx_timer.c
+++ b/hw/timer/xilinx_timer.c
@@ -218,7 +218,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);
+        xt->ptimer = ptimer_init(xt->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(xt->ptimer, t->freq_hz);
     }
 
diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index e397db5..69dbdc1 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -12,11 +12,16 @@
 #include "qemu/timer.h"
 #include "migration/vmstate.h"
 
+/* Stop the timer if period/counter is equal to 0.  */
+#define PTIMER_POLICY_DEFAULT               0
+/* Counter = 0 triggers IRQ after one period.  */
+#define PTIMER_POLICY_CNT_0_DEFERRED_TRIG   (1 << 0)
+
 /* ptimer.c */
 typedef struct ptimer_state ptimer_state;
 typedef void (*ptimer_cb)(void *opaque);
 
-ptimer_state *ptimer_init(QEMUBH *bh);
+ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask);
 void ptimer_set_period(ptimer_state *s, int64_t period);
 void ptimer_set_freq(ptimer_state *s, uint32_t freq);
 uint64_t ptimer_get_limit(ptimer_state *s);
-- 
2.9.0

[Qemu-devel] [PATCH v14 2/3] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer

[Dmitry Osipenko]

Due to rounding down performed by ptimer_get_count, it returns counter - 1 for
the active timer. That's incorrect because counter should decrement only after
period been expired, not before. I.e. if running timer has been loaded with
value X, then timer counter should stay with X until period expired and
decrement after. Fix this by adding 1 to the counter value for the active and
unexpired timer.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/core/ptimer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 289e23e..7f89001 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -89,10 +89,10 @@ static void ptimer_tick(void *opaque)
 
 uint64_t ptimer_get_count(ptimer_state *s)
 {
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     uint64_t counter;
 
-    if (s->enabled && s->delta != 0) {
-        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    if (s->enabled && s->delta != 0 && now != s->last_event) {
         int64_t next = s->next_event;
         bool expired = (now - next >= 0);
         bool oneshot = (s->enabled == 2);
@@ -144,7 +144,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
                 if ((uint32_t)(period_frac << shift))
                     div += 1;
             }
-            counter = rem / div;
+            counter = rem / div + (expired ? 0 : 1);
 
             if (expired && counter != 0) {
                 /* Wrap around periodic counter.  */
-- 
2.9.0

[Qemu-devel] [PATCH v14 3/3] arm_mptimer: Convert to use ptimer

[Dmitry Osipenko]

Current ARM MPTimer implementation uses QEMUTimer for the actual timer,
this implementation isn't complete and mostly tries to duplicate of what
generic ptimer is already doing fine.

Conversion to ptimer brings the following benefits and fixes:
	- Simple timer pausing implementation
	- Fixes counter value preservation after stopping the timer
	- Properly handles prescaler != 0 / counter = 0 / load = 0 cases
	- Code simplification and reduction

Bump VMSD to version 3, since VMState is changed and is not compatible
with the previous implementation.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/timer/arm_mptimer.c         | 133 +++++++++++++++++++++--------------------
 include/hw/timer/arm_mptimer.h |   5 +-
 2 files changed, 70 insertions(+), 68 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index d66bbf0..3616c40 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -20,9 +20,10 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/ptimer.h"
 #include "hw/timer/arm_mptimer.h"
 #include "qapi/error.h"
-#include "qemu/timer.h"
+#include "qemu/main-loop.h"
 #include "qom/cpu.h"
 
 /* This device implements the per-cpu private timer and watchdog block
@@ -44,55 +45,54 @@ static inline void timerblock_update_irq(TimerBlock *tb)
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
-static inline uint32_t timerblock_scale(TimerBlock *tb)
+static inline uint32_t timerblock_scale(uint32_t control)
 {
-    return (((tb->control >> 8) & 0xff) + 1) * 10;
+    return (((control >> 8) & 0xff) + 1) * 10;
 }
 
-static void timerblock_reload(TimerBlock *tb, int restart)
+static inline void timerblock_set_count(struct ptimer_state *timer,
+                                        uint32_t control, uint64_t *count)
 {
-    if (tb->count == 0) {
-        return;
+    /* PTimer would immediately trigger interrupt for periodic timer
+     * when counter set to 0, MPtimer under certain condition only.
+     */
+    if ((control & 3) == 3 && (control & 0xff00) == 0 && *count == 0) {
+        *count = ptimer_get_limit(timer);
     }
-    if (restart) {
-        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    ptimer_set_count(timer, *count);
+}
+
+static inline void timerblock_run(struct ptimer_state *timer,
+                                  uint32_t control, uint32_t load)
+{
+    if ((control & 1) && ((control & 0xff00) || load != 0)) {
+        ptimer_run(timer, !(control & 2));
     }
-    tb->tick += (int64_t)tb->count * timerblock_scale(tb);
-    timer_mod(tb->timer, tb->tick);
 }
 
 static void timerblock_tick(void *opaque)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
     tb->status = 1;
-    if (tb->control & 2) {
-        tb->count = tb->load;
-        timerblock_reload(tb, 0);
-    } else {
-        tb->count = 0;
-    }
     timerblock_update_irq(tb);
+    /* Periodic timer with load = 0 and prescaler != 0 would re-trigger
+     * IRQ after one period, otherwise it either stops or wraps around.
+     */
+    if ((tb->control & 2) && (tb->control & 0xff00) &&
+            ptimer_get_limit(tb->timer) == 0) {
+        ptimer_run(tb->timer, 0);
+    }
 }
 
 static uint64_t timerblock_read(void *opaque, hwaddr addr,
                                 unsigned size)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
-    int64_t val;
     switch (addr) {
     case 0: /* Load */
-        return tb->load;
+        return ptimer_get_limit(tb->timer);
     case 4: /* Counter.  */
-        if (((tb->control & 1) == 0) || (tb->count == 0)) {
-            return 0;
-        }
-        /* Slow and ugly, but hopefully won't happen too often.  */
-        val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        val /= timerblock_scale(tb);
-        if (val < 0) {
-            val = 0;
-        }
-        return val;
+        return ptimer_get_count(tb->timer);
     case 8: /* Control.  */
         return tb->control;
     case 12: /* Interrupt status.  */
@@ -106,37 +106,45 @@ static void timerblock_write(void *opaque, hwaddr addr,
                              uint64_t value, unsigned size)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
-    int64_t old;
+    uint32_t control = tb->control;
     switch (addr) {
     case 0: /* Load */
-        tb->load = value;
-        /* Fall through.  */
-    case 4: /* Counter.  */
-        if ((tb->control & 1) && tb->count) {
-            /* Cancel the previous timer.  */
-            timer_del(tb->timer);
+        /* Setting load to 0 stops the timer without doing the tick if
+         * prescaler = 0.
+         */
+        if ((control & 1) && (control & 0xff00) == 0 && value == 0) {
+            ptimer_stop(tb->timer);
         }
-        tb->count = value;
-        if (tb->control & 1) {
-            timerblock_reload(tb, 1);
+        ptimer_set_limit(tb->timer, value, 1);
+        timerblock_run(tb->timer, control, value);
+        break;
+    case 4: /* Counter.  */
+        /* Setting counter to 0 stops the one-shot timer, or periodic with
+         * load == 0, without doing the tick if prescaler = 0.
+         */
+        if ((control & 1) && (control & 0xff00) == 0 && value == 0 &&
+                (!(control & 2) || ptimer_get_limit(tb->timer) == 0)) {
+            ptimer_stop(tb->timer);
         }
+        timerblock_set_count(tb->timer, control, &value);
+        timerblock_run(tb->timer, control, value);
         break;
     case 8: /* Control.  */
-        old = tb->control;
-        tb->control = value;
+        if ((control & 1) && !(value & 1)) {
+            ptimer_stop(tb->timer);
+        }
+        if ((control & 0xff00) != (value & 0xff00)) {
+            ptimer_set_period(tb->timer, timerblock_scale(value));
+        }
         if (value & 1) {
-            if ((old & 1) && (tb->count != 0)) {
-                /* Do nothing if timer is ticking right now.  */
-                break;
+            uint64_t count = ptimer_get_count(tb->timer);
+            /* Re-load periodic timer counter if needed.  */
+            if ((value & 2) && count == 0) {
+                timerblock_set_count(tb->timer, value, &count);
             }
-            if (tb->control & 2) {
-                tb->count = tb->load;
-            }
-            timerblock_reload(tb, 1);
-        } else if (old & 1) {
-            /* Shutdown the timer.  */
-            timer_del(tb->timer);
+            timerblock_run(tb->timer, value, count);
         }
+        tb->control = value;
         break;
     case 12: /* Interrupt status.  */
         tb->status &= ~value;
@@ -186,13 +194,12 @@ static const MemoryRegionOps timerblock_ops = {
 
 static void timerblock_reset(TimerBlock *tb)
 {
-    tb->count = 0;
-    tb->load = 0;
     tb->control = 0;
     tb->status = 0;
-    tb->tick = 0;
     if (tb->timer) {
-        timer_del(tb->timer);
+        ptimer_stop(tb->timer);
+        ptimer_set_limit(tb->timer, 0, 1);
+        ptimer_set_period(tb->timer, timerblock_scale(0));
     }
 }
 
@@ -238,7 +245,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
      */
     for (i = 0; i < s->num_cpu; i++) {
         TimerBlock *tb = &s->timerblock[i];
-        tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb);
+        QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
+        tb->timer = ptimer_init(bh, PTIMER_POLICY_CNT_0_DEFERRED_TRIG);
         sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);
@@ -248,26 +256,23 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_timerblock = {
     .name = "arm_mptimer_timerblock",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(count, TimerBlock),
-        VMSTATE_UINT32(load, TimerBlock),
         VMSTATE_UINT32(control, TimerBlock),
         VMSTATE_UINT32(status, TimerBlock),
-        VMSTATE_INT64(tick, TimerBlock),
-        VMSTATE_TIMER_PTR(timer, TimerBlock),
+        VMSTATE_PTIMER(timer, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static const VMStateDescription vmstate_arm_mptimer = {
     .name = "arm_mptimer",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
-                                     2, vmstate_timerblock, TimerBlock),
+                                     3, vmstate_timerblock, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
index b34cba0..c46d8d2 100644
--- a/include/hw/timer/arm_mptimer.h
+++ b/include/hw/timer/arm_mptimer.h
@@ -27,12 +27,9 @@
 
 /* State of a single timer or watchdog block */
 typedef struct {
-    uint32_t count;
-    uint32_t load;
     uint32_t control;
     uint32_t status;
-    int64_t tick;
-    QEMUTimer *timer;
+    struct ptimer_state *timer;
     qemu_irq irq;
     MemoryRegion iomem;
 } TimerBlock;
-- 
2.9.0

Re: [Qemu-devel] [PATCH v14 2/3] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer

[Dmitry Osipenko]

I started to port MPTimer tests to QTest that allows precise VM clock control. 
It helped to find out that the last period of the periodic timer is getting 
lost, i.e. when counter = 0 (before wrap around). So the counter should be 
loaded with limit + 1 after wrapping around. This worked before because we are 
returning counter = 0 for the expired timer, that expiration doesn't happen 
under QTest and some of the tests fail. I'll send new revision of the patch 
after receiving feedback to the "policy" patch and once QTests would be ready.

On 17.06.2016 16:17, Dmitry Osipenko wrote:
> Due to rounding down performed by ptimer_get_count, it returns counter - 1 for
> the active timer. That's incorrect because counter should decrement only after
> period been expired, not before. I.e. if running timer has been loaded with
> value X, then timer counter should stay with X until period expired and
> decrement after. Fix this by adding 1 to the counter value for the active and
> unexpired timer.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/core/ptimer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 289e23e..7f89001 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -89,10 +89,10 @@ static void ptimer_tick(void *opaque)
>
>  uint64_t ptimer_get_count(ptimer_state *s)
>  {
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>      uint64_t counter;
>
> -    if (s->enabled && s->delta != 0) {
> -        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    if (s->enabled && s->delta != 0 && now != s->last_event) {
>          int64_t next = s->next_event;
>          bool expired = (now - next >= 0);
>          bool oneshot = (s->enabled == 2);
> @@ -144,7 +144,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
>                  if ((uint32_t)(period_frac << shift))
>                      div += 1;
>              }
> -            counter = rem / div;
> +            counter = rem / div + (expired ? 0 : 1);
>
>              if (expired && counter != 0) {
>                  /* Wrap around periodic counter.  */
>


-- 
Dmitry

Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Peter Maydell]

On 17 June 2016 at 14:17, Dmitry Osipenko <digetx@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 <digetx@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

Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Dmitry Osipenko]

On 23.06.2016 16:49, Peter Maydell wrote:
> On 17 June 2016 at 14:17, Dmitry Osipenko <digetx@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.
> 

This patch isn't supposed to change behaviour for any of the current timers. I
think it is clearly expressed in the last sentence of the commit message. There
is one unintended behaviour change in this patch, it's my overlook [see below].

>> Signed-off-by: Dmitry Osipenko <digetx@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?
> 

I removed the in-place type conversion further:
>>      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?
> 

That's a good question. In the current ARM MPTimer conversion patch, I assume
that delta = 0 would stop the timer regardless of it's mode and ptimer user
would itself re-arm timer in a tick() handler if needed. However, after your
question, I think it's a bit unintuitive and needs to be changed to continuous
*ptimer* trigger in periodic mode in case of the deferred trigger policy.

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

Right, now I see that ptimer_trigger() is missed in case of running timer with a
default policy and delta = 0. Thanks for the note, will fix it. I'll craft
QTests for the ptimer, shouldn't take a lot of effort.

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

Agree :) I have hit it couple times during the work on the ARM MPTimer patch in
the past, decided that it may not worth a whole patch for a such simple one-liner.

>>      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?
> 

Ooops, good catch!

Thanks for the review!

-- 
Dmitry

Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Peter Maydell]

On 23 June 2016 at 17:32, Dmitry Osipenko <digetx@gmail.com> wrote:
> On 23.06.2016 16:49, Peter Maydell wrote:
>> On 17 June 2016 at 14:17, Dmitry Osipenko <digetx@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.
>>
>
> This patch isn't supposed to change behaviour for any of the current timers. I
> think it is clearly expressed in the last sentence of the commit message. There
> is one unintended behaviour change in this patch, it's my overlook [see below].

Right, but my point is that it is hard to tell that when I read
the patch to review it, and splitting this will make it easier.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Dmitry Osipenko]

On 23.06.2016 19:43, Peter Maydell wrote:
> On 23 June 2016 at 17:32, Dmitry Osipenko <digetx@gmail.com> wrote:
>> On 23.06.2016 16:49, Peter Maydell wrote:
>>> On 17 June 2016 at 14:17, Dmitry Osipenko <digetx@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.
>>>
>>
>> This patch isn't supposed to change behaviour for any of the current timers. I
>> think it is clearly expressed in the last sentence of the commit message. There
>> is one unintended behaviour change in this patch, it's my overlook [see below].
> 
> Right, but my point is that it is hard to tell that when I read
> the patch to review it, and splitting this will make it easier.
> 
> thanks
> -- PMM
> 

Ah, you are meaning to derive adding *new* "deferred trigger" policy into a
separate patch. I'll do it.

-- 
Dmitry