[PATCH 0/2] Convert sparc devices to new ptimer API

[PATCH 0/2] Convert sparc devices to new ptimer API

[Peter Maydell]

This patchset converts the devices used by sparc machines to the new
ptimer API.

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

The updates to the individual ptimer devices are straightforward:
we need to add begin/commit calls around the various places that
modify the ptimer state, and use the new ptimer_init() function
to create the timer.

Testing has been 'make check', and a quick smoke test of a sparc
linux boot image I had lying around, which obviously doesn't
exercise the devices very much, so more specific testing would
be appreciated. I'm happy for these patches to go in via the
sparc tree if you want, or I can collect them up with the other
ptimer-related changes I'm sending for other archs.

thanks
--PMM


Peter Maydell (2):
  hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API
  hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

 hw/timer/grlib_gptimer.c | 28 ++++++++++++++++++++++++----
 hw/timer/slavio_timer.c  | 20 ++++++++++++++++----
 2 files changed, 40 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/2] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API

[Peter Maydell]

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

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/grlib_gptimer.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index bb09268ea14..7a9371c0e30 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -29,7 +29,6 @@
 #include "hw/irq.h"
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 #include "trace.h"
@@ -63,7 +62,6 @@ typedef struct GPTimer     GPTimer;
 typedef struct GPTimerUnit GPTimerUnit;
 
 struct GPTimer {
-    QEMUBH *bh;
     struct ptimer_state *ptimer;
 
     qemu_irq     irq;
@@ -93,6 +91,17 @@ struct GPTimerUnit {
     uint32_t config;
 };
 
+static void grlib_gptimer_tx_begin(GPTimer *timer)
+{
+    ptimer_transaction_begin(timer->ptimer);
+}
+
+static void grlib_gptimer_tx_commit(GPTimer *timer)
+{
+    ptimer_transaction_commit(timer->ptimer);
+}
+
+/* Must be called within grlib_gptimer_tx_begin/commit block */
 static void grlib_gptimer_enable(GPTimer *timer)
 {
     assert(timer != NULL);
@@ -115,6 +124,7 @@ static void grlib_gptimer_enable(GPTimer *timer)
     ptimer_run(timer->ptimer, 1);
 }
 
+/* Must be called within grlib_gptimer_tx_begin/commit block */
 static void grlib_gptimer_restart(GPTimer *timer)
 {
     assert(timer != NULL);
@@ -141,7 +151,9 @@ static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
     trace_grlib_gptimer_set_scaler(scaler, value);
 
     for (i = 0; i < unit->nr_timers; i++) {
+        ptimer_transaction_begin(unit->timers[i].ptimer);
         ptimer_set_freq(unit->timers[i].ptimer, value);
+        ptimer_transaction_commit(unit->timers[i].ptimer);
     }
 }
 
@@ -266,8 +278,10 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
         switch (timer_addr) {
         case COUNTER_OFFSET:
             trace_grlib_gptimer_writel(id, addr, value);
+            grlib_gptimer_tx_begin(&unit->timers[id]);
             unit->timers[id].counter = value;
             grlib_gptimer_enable(&unit->timers[id]);
+            grlib_gptimer_tx_commit(&unit->timers[id]);
             return;
 
         case COUNTER_RELOAD_OFFSET:
@@ -291,6 +305,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
             /* gptimer_restart calls gptimer_enable, so if "enable" and "load"
                bits are present, we just have to call restart. */
 
+            grlib_gptimer_tx_begin(&unit->timers[id]);
             if (value & GPTIMER_LOAD) {
                 grlib_gptimer_restart(&unit->timers[id]);
             } else if (value & GPTIMER_ENABLE) {
@@ -301,6 +316,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
             value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
 
             unit->timers[id].config = value;
+            grlib_gptimer_tx_commit(&unit->timers[id]);
             return;
 
         default:
@@ -344,9 +360,11 @@ static void grlib_gptimer_reset(DeviceState *d)
         timer->counter = 0;
         timer->reload = 0;
         timer->config = 0;
+        ptimer_transaction_begin(timer->ptimer);
         ptimer_stop(timer->ptimer);
         ptimer_set_count(timer->ptimer, 0);
         ptimer_set_freq(timer->ptimer, unit->freq_hz);
+        ptimer_transaction_commit(timer->ptimer);
     }
 }
 
@@ -365,14 +383,16 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
         GPTimer *timer = &unit->timers[i];
 
         timer->unit   = unit;
-        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
-        timer->ptimer = ptimer_init_with_bh(timer->bh, PTIMER_POLICY_DEFAULT);
+        timer->ptimer = ptimer_init(grlib_gptimer_hit, timer,
+                                    PTIMER_POLICY_DEFAULT);
         timer->id     = i;
 
         /* One IRQ line for each timer */
         sysbus_init_irq(sbd, &timer->irq);
 
+        ptimer_transaction_begin(timer->ptimer);
         ptimer_set_freq(timer->ptimer, unit->freq_hz);
+        ptimer_transaction_commit(timer->ptimer);
     }
 
     memory_region_init_io(&unit->iomem, OBJECT(unit), &grlib_gptimer_ops,
-- 
2.20.1

Re: [PATCH 1/2] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:23 AM, Peter Maydell wrote:
> Switch the grlib_gptimer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/grlib_gptimer.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)

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


r~

Re: [PATCH 1/2] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:23 PM, Peter Maydell wrote:
> Switch the grlib_gptimer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/grlib_gptimer.c | 28 ++++++++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
> index bb09268ea14..7a9371c0e30 100644
> --- a/hw/timer/grlib_gptimer.c
> +++ b/hw/timer/grlib_gptimer.c
> @@ -29,7 +29,6 @@
>   #include "hw/irq.h"
>   #include "hw/ptimer.h"
>   #include "hw/qdev-properties.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   #include "trace.h"
> @@ -63,7 +62,6 @@ typedef struct GPTimer     GPTimer;
>   typedef struct GPTimerUnit GPTimerUnit;
>   
>   struct GPTimer {
> -    QEMUBH *bh;
>       struct ptimer_state *ptimer;
>   
>       qemu_irq     irq;
> @@ -93,6 +91,17 @@ struct GPTimerUnit {
>       uint32_t config;
>   };
>   
> +static void grlib_gptimer_tx_begin(GPTimer *timer)
> +{
> +    ptimer_transaction_begin(timer->ptimer);
> +}
> +
> +static void grlib_gptimer_tx_commit(GPTimer *timer)
> +{
> +    ptimer_transaction_commit(timer->ptimer);
> +}
> +
> +/* Must be called within grlib_gptimer_tx_begin/commit block */
>   static void grlib_gptimer_enable(GPTimer *timer)
>   {
>       assert(timer != NULL);
> @@ -115,6 +124,7 @@ static void grlib_gptimer_enable(GPTimer *timer)
>       ptimer_run(timer->ptimer, 1);
>   }
>   
> +/* Must be called within grlib_gptimer_tx_begin/commit block */
>   static void grlib_gptimer_restart(GPTimer *timer)
>   {
>       assert(timer != NULL);
> @@ -141,7 +151,9 @@ static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
>       trace_grlib_gptimer_set_scaler(scaler, value);
>   
>       for (i = 0; i < unit->nr_timers; i++) {
> +        ptimer_transaction_begin(unit->timers[i].ptimer);
>           ptimer_set_freq(unit->timers[i].ptimer, value);
> +        ptimer_transaction_commit(unit->timers[i].ptimer);
>       }
>   }
>   
> @@ -266,8 +278,10 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
>           switch (timer_addr) {
>           case COUNTER_OFFSET:
>               trace_grlib_gptimer_writel(id, addr, value);
> +            grlib_gptimer_tx_begin(&unit->timers[id]);
>               unit->timers[id].counter = value;
>               grlib_gptimer_enable(&unit->timers[id]);
> +            grlib_gptimer_tx_commit(&unit->timers[id]);
>               return;
>   
>           case COUNTER_RELOAD_OFFSET:
> @@ -291,6 +305,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
>               /* gptimer_restart calls gptimer_enable, so if "enable" and "load"
>                  bits are present, we just have to call restart. */
>   
> +            grlib_gptimer_tx_begin(&unit->timers[id]);
>               if (value & GPTIMER_LOAD) {
>                   grlib_gptimer_restart(&unit->timers[id]);
>               } else if (value & GPTIMER_ENABLE) {
> @@ -301,6 +316,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
>               value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
>   
>               unit->timers[id].config = value;
> +            grlib_gptimer_tx_commit(&unit->timers[id]);
>               return;
>   
>           default:
> @@ -344,9 +360,11 @@ static void grlib_gptimer_reset(DeviceState *d)
>           timer->counter = 0;
>           timer->reload = 0;
>           timer->config = 0;
> +        ptimer_transaction_begin(timer->ptimer);
>           ptimer_stop(timer->ptimer);
>           ptimer_set_count(timer->ptimer, 0);
>           ptimer_set_freq(timer->ptimer, unit->freq_hz);
> +        ptimer_transaction_commit(timer->ptimer);
>       }
>   }
>   
> @@ -365,14 +383,16 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
>           GPTimer *timer = &unit->timers[i];
>   
>           timer->unit   = unit;
> -        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
> -        timer->ptimer = ptimer_init_with_bh(timer->bh, PTIMER_POLICY_DEFAULT);
> +        timer->ptimer = ptimer_init(grlib_gptimer_hit, timer,
> +                                    PTIMER_POLICY_DEFAULT);
>           timer->id     = i;
>   
>           /* One IRQ line for each timer */
>           sysbus_init_irq(sbd, &timer->irq);
>   
> +        ptimer_transaction_begin(timer->ptimer);
>           ptimer_set_freq(timer->ptimer, unit->freq_hz);
> +        ptimer_transaction_commit(timer->ptimer);
>       }
>   
>       memory_region_init_io(&unit->iomem, OBJECT(unit), &grlib_gptimer_ops,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

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

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/slavio_timer.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
index 692d213897d..0e2efe6fe89 100644
--- a/hw/timer/slavio_timer.c
+++ b/hw/timer/slavio_timer.c
@@ -30,7 +30,6 @@
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "trace.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 /*
@@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
     saddr = addr >> 2;
     switch (saddr) {
     case TIMER_LIMIT:
+        ptimer_transaction_begin(t->timer);
         if (slavio_timer_is_user(tc)) {
             uint64_t count;
 
@@ -236,6 +236,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
                 }
             }
         }
+        ptimer_transaction_commit(t->timer);
         break;
     case TIMER_COUNTER:
         if (slavio_timer_is_user(tc)) {
@@ -247,7 +248,9 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
             t->reached = 0;
             count = ((uint64_t)t->counthigh) << 32 | t->count;
             trace_slavio_timer_mem_writel_limit(timer_index, count);
+            ptimer_transaction_begin(t->timer);
             ptimer_set_count(t->timer, LIMIT_TO_PERIODS(t->limit - count));
+            ptimer_transaction_commit(t->timer);
         } else {
             trace_slavio_timer_mem_writel_counter_invalid();
         }
@@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
     case TIMER_COUNTER_NORST:
         // set limit without resetting counter
         t->limit = val & TIMER_MAX_COUNT32;
+        ptimer_transaction_begin(t->timer);
         if (t->limit == 0) { /* free-run */
             ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0);
         } else {
             ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0);
         }
+        ptimer_transaction_commit(t->timer);
         break;
     case TIMER_STATUS:
+        ptimer_transaction_begin(t->timer);
         if (slavio_timer_is_user(tc)) {
             // start/stop user counter
             if (val & 1) {
@@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
             }
         }
         t->run = val & 1;
+        ptimer_transaction_commit(t->timer);
         break;
     case TIMER_MODE:
         if (timer_index == 0) {
@@ -282,6 +289,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
                 unsigned int processor = 1 << i;
                 CPUTimerState *curr_timer = &s->cputimer[i + 1];
 
+                ptimer_transaction_begin(curr_timer->timer);
                 // check for a change in timer mode for this processor
                 if ((val & processor) != (s->cputimer_mode & processor)) {
                     if (val & processor) { // counter -> user timer
@@ -308,6 +316,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
                         trace_slavio_timer_mem_writel_mode_counter(timer_index);
                     }
                 }
+                ptimer_transaction_commit(curr_timer->timer);
             }
         } else {
             trace_slavio_timer_mem_writel_mode_invalid();
@@ -367,10 +376,12 @@ static void slavio_timer_reset(DeviceState *d)
         curr_timer->count = 0;
         curr_timer->reached = 0;
         if (i <= s->num_cpus) {
+            ptimer_transaction_begin(curr_timer->timer);
             ptimer_set_limit(curr_timer->timer,
                              LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1);
             ptimer_run(curr_timer->timer, 0);
             curr_timer->run = 1;
+            ptimer_transaction_commit(curr_timer->timer);
         }
     }
     s->cputimer_mode = 0;
@@ -380,7 +391,6 @@ static void slavio_timer_init(Object *obj)
 {
     SLAVIO_TIMERState *s = SLAVIO_TIMER(obj);
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
-    QEMUBH *bh;
     unsigned int i;
     TimerContext *tc;
 
@@ -392,9 +402,11 @@ static void slavio_timer_init(Object *obj)
         tc->s = s;
         tc->timer_index = i;
 
-        bh = qemu_bh_new(slavio_timer_irq, tc);
-        s->cputimer[i].timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
+        s->cputimer[i].timer = ptimer_init(slavio_timer_irq, tc,
+                                           PTIMER_POLICY_DEFAULT);
+        ptimer_transaction_begin(s->cputimer[i].timer);
         ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD);
+        ptimer_transaction_commit(s->cputimer[i].timer);
 
         size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE;
         snprintf(timer_name, sizeof(timer_name), "timer-%i", i);
-- 
2.20.1

Re: [PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:23 AM, Peter Maydell wrote:
> Switch the slavio_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/slavio_timer.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)

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


r~

Re: [PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

Hi Peter,

On 10/17/19 3:23 PM, Peter Maydell wrote:
> Switch the slavio_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/slavio_timer.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
> index 692d213897d..0e2efe6fe89 100644
> --- a/hw/timer/slavio_timer.c
> +++ b/hw/timer/slavio_timer.c
> @@ -30,7 +30,6 @@
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
>   #include "trace.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   /*
> @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>       saddr = addr >> 2;
>       switch (saddr) {
>       case TIMER_LIMIT:
> +        ptimer_transaction_begin(t->timer);
>           if (slavio_timer_is_user(tc)) {
>               uint64_t count;


This part is odd since there is a check on t->timer != NULL later, and 
ptimer_transaction_commit() can't take NULL.

This won't happen if you restrict to ptimer_* calls:

-- >8 --
@@ -222,18 +222,22 @@ static void slavio_timer_mem_writel(void *opaque, 
hwaddr addr,
              t->reached = 0;
              count = ((uint64_t)t->counthigh << 32) | t->count;
              trace_slavio_timer_mem_writel_limit(timer_index, count);
+            ptimer_transaction_begin(t->timer);
              ptimer_set_count(t->timer, LIMIT_TO_PERIODS(t->limit - 
count));
+            ptimer_transaction_commit(t->timer);
          } else {
              // set limit, reset counter
              qemu_irq_lower(t->irq);
              t->limit = val & TIMER_MAX_COUNT32;
              if (t->timer) {
+                ptimer_transaction_begin(t->timer);
                  if (t->limit == 0) { /* free-run */
                      ptimer_set_limit(t->timer,
 
LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1);
                  } else {
                      ptimer_set_limit(t->timer, 
LIMIT_TO_PERIODS(t->limit), 1);
                  }
+                ptimer_transaction_commit(t->timer);
              }
          }
          break;
---

>   
> @@ -236,6 +236,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>                   }
>               }
>           }
> +        ptimer_transaction_commit(t->timer);
>           break;
>       case TIMER_COUNTER:
>           if (slavio_timer_is_user(tc)) {
> @@ -247,7 +248,9 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>               t->reached = 0;
>               count = ((uint64_t)t->counthigh) << 32 | t->count;
>               trace_slavio_timer_mem_writel_limit(timer_index, count);
> +            ptimer_transaction_begin(t->timer);
>               ptimer_set_count(t->timer, LIMIT_TO_PERIODS(t->limit - count));
> +            ptimer_transaction_commit(t->timer);
>           } else {
>               trace_slavio_timer_mem_writel_counter_invalid();
>           }
> @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>       case TIMER_COUNTER_NORST:
>           // set limit without resetting counter
>           t->limit = val & TIMER_MAX_COUNT32;
> +        ptimer_transaction_begin(t->timer);
>           if (t->limit == 0) { /* free-run */
>               ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0);
>           } else {
>               ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0);
>           }
> +        ptimer_transaction_commit(t->timer);
>           break;
>       case TIMER_STATUS:
> +        ptimer_transaction_begin(t->timer);
>           if (slavio_timer_is_user(tc)) {

I'd move the begin() here.

>               // start/stop user counter
>               if (val & 1) {
> @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>               }
>           }
>           t->run = val & 1;
> +        ptimer_transaction_commit(t->timer);
>           break;
>       case TIMER_MODE:
>           if (timer_index == 0) {
> @@ -282,6 +289,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>                   unsigned int processor = 1 << i;
>                   CPUTimerState *curr_timer = &s->cputimer[i + 1];
>   
> +                ptimer_transaction_begin(curr_timer->timer);
>                   // check for a change in timer mode for this processor
>                   if ((val & processor) != (s->cputimer_mode & processor)) {

Ditto, begin() here.

>                       if (val & processor) { // counter -> user timer
> @@ -308,6 +316,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>                           trace_slavio_timer_mem_writel_mode_counter(timer_index);
>                       }
>                   }
> +                ptimer_transaction_commit(curr_timer->timer);
>               }
>           } else {
>               trace_slavio_timer_mem_writel_mode_invalid();
> @@ -367,10 +376,12 @@ static void slavio_timer_reset(DeviceState *d)
>           curr_timer->count = 0;
>           curr_timer->reached = 0;
>           if (i <= s->num_cpus) {
> +            ptimer_transaction_begin(curr_timer->timer);
>               ptimer_set_limit(curr_timer->timer,
>                                LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 1);
>               ptimer_run(curr_timer->timer, 0);
>               curr_timer->run = 1;
> +            ptimer_transaction_commit(curr_timer->timer);
>           }
>       }
>       s->cputimer_mode = 0;
> @@ -380,7 +391,6 @@ static void slavio_timer_init(Object *obj)
>   {
>       SLAVIO_TIMERState *s = SLAVIO_TIMER(obj);
>       SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> -    QEMUBH *bh;
>       unsigned int i;
>       TimerContext *tc;
>   
> @@ -392,9 +402,11 @@ static void slavio_timer_init(Object *obj)
>           tc->s = s;
>           tc->timer_index = i;
>   
> -        bh = qemu_bh_new(slavio_timer_irq, tc);
> -        s->cputimer[i].timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
> +        s->cputimer[i].timer = ptimer_init(slavio_timer_irq, tc,
> +                                           PTIMER_POLICY_DEFAULT);
> +        ptimer_transaction_begin(s->cputimer[i].timer);
>           ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD);
> +        ptimer_transaction_commit(s->cputimer[i].timer);
>   
>           size = i == 0 ? SYS_TIMER_SIZE : CPU_TIMER_SIZE;
>           snprintf(timer_name, sizeof(timer_name), "timer-%i", i);
> 

Re: [PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 15:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Peter,
>
> On 10/17/19 3:23 PM, Peter Maydell wrote:
> > Switch the slavio_timer code away from bottom-half based ptimers to
> > the new transaction-based ptimer API.  This just requires adding
> > begin/commit calls around the various places that modify the ptimer
> > state, and using the new ptimer_init() function to create the timer.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/timer/slavio_timer.c | 20 ++++++++++++++++----
> >   1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
> > index 692d213897d..0e2efe6fe89 100644
> > --- a/hw/timer/slavio_timer.c
> > +++ b/hw/timer/slavio_timer.c
> > @@ -30,7 +30,6 @@
> >   #include "hw/sysbus.h"
> >   #include "migration/vmstate.h"
> >   #include "trace.h"
> > -#include "qemu/main-loop.h"
> >   #include "qemu/module.h"
> >
> >   /*
> > @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
> >       saddr = addr >> 2;
> >       switch (saddr) {
> >       case TIMER_LIMIT:
> > +        ptimer_transaction_begin(t->timer);
> >           if (slavio_timer_is_user(tc)) {
> >               uint64_t count;
>
>
> This part is odd since there is a check on t->timer != NULL later, and
> ptimer_transaction_commit() can't take NULL.

Hmm, I hadn't noticed that. I think the bug is the check
for NULL, though, beacuse the slavio_timer_init() function
always initializes all the timer pointers, so there's
no situation where the pointer can be non-NULL as far
as I can see. So I think I'd rather fix this by removing
the NULL pointer check...


> > @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
> >       case TIMER_COUNTER_NORST:
> >           // set limit without resetting counter
> >           t->limit = val & TIMER_MAX_COUNT32;
> > +        ptimer_transaction_begin(t->timer);
> >           if (t->limit == 0) { /* free-run */
> >               ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0);
> >           } else {
> >               ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0);
> >           }
> > +        ptimer_transaction_commit(t->timer);
> >           break;
> >       case TIMER_STATUS:
> > +        ptimer_transaction_begin(t->timer);
> >           if (slavio_timer_is_user(tc)) {
>
> I'd move the begin() here.

This would be awkward because then it won't neatly nest with
the commit call unless you add an extra if() for the
commit or otherwise rearrange/duplicate code...

> >               // start/stop user counter
> >               if (val & 1) {
> > @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
> >               }
> >           }
> >           t->run = val & 1;
> > +        ptimer_transaction_commit(t->timer);

...because the commit should come after we have finished
updating the timer state (t->run in this case), because
the trigger callback slavio_timer_irq() otherwise sees
inconsistent half-updated state if commit() calls it.

thanks
-- PMM

Re: [PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 5:00 PM, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 15:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 10/17/19 3:23 PM, Peter Maydell wrote:
>>> Switch the slavio_timer code away from bottom-half based ptimers to
>>> the new transaction-based ptimer API.  This just requires adding
>>> begin/commit calls around the various places that modify the ptimer
>>> state, and using the new ptimer_init() function to create the timer.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    hw/timer/slavio_timer.c | 20 ++++++++++++++++----
>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
>>> index 692d213897d..0e2efe6fe89 100644
>>> --- a/hw/timer/slavio_timer.c
>>> +++ b/hw/timer/slavio_timer.c
>>> @@ -30,7 +30,6 @@
>>>    #include "hw/sysbus.h"
>>>    #include "migration/vmstate.h"
>>>    #include "trace.h"
>>> -#include "qemu/main-loop.h"
>>>    #include "qemu/module.h"
>>>
>>>    /*
>>> @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>>>        saddr = addr >> 2;
>>>        switch (saddr) {
>>>        case TIMER_LIMIT:
>>> +        ptimer_transaction_begin(t->timer);
>>>            if (slavio_timer_is_user(tc)) {
>>>                uint64_t count;
>>
>>
>> This part is odd since there is a check on t->timer != NULL later, and
>> ptimer_transaction_commit() can't take NULL.
> 
> Hmm, I hadn't noticed that. I think the bug is the check
> for NULL, though, beacuse the slavio_timer_init() function
> always initializes all the timer pointers, so there's
> no situation where the pointer can be non-NULL as far
> as I can see. So I think I'd rather fix this by removing
> the NULL pointer check...

Yes, you are correct.

>>> @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>>>        case TIMER_COUNTER_NORST:
>>>            // set limit without resetting counter
>>>            t->limit = val & TIMER_MAX_COUNT32;
>>> +        ptimer_transaction_begin(t->timer);
>>>            if (t->limit == 0) { /* free-run */
>>>                ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0);
>>>            } else {
>>>                ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0);
>>>            }
>>> +        ptimer_transaction_commit(t->timer);
>>>            break;
>>>        case TIMER_STATUS:
>>> +        ptimer_transaction_begin(t->timer);
>>>            if (slavio_timer_is_user(tc)) {
>>
>> I'd move the begin() here.
> 
> This would be awkward because then it won't neatly nest with
> the commit call unless you add an extra if() for the
> commit or otherwise rearrange/duplicate code...
> 
>>>                // start/stop user counter
>>>                if (val & 1) {
>>> @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>>>                }
>>>            }
>>>            t->run = val & 1;
>>> +        ptimer_transaction_commit(t->timer);
> 
> ...because the commit should come after we have finished
> updating the timer state (t->run in this case), because
> the trigger callback slavio_timer_irq() otherwise sees
> inconsistent half-updated state if commit() calls it.

Oh, slavio_timer_irq() calls slavio_timer_get_out() which accesses the 
ptimer... OK I missed that.

Whew we need to be extra cautious with this API...

If possible I'd rather see the patch removing the NULL check previous to 
this one, anyway:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks,

Phil.

Re: [PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 16:22, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 10/17/19 5:00 PM, Peter Maydell wrote:
> > ...because the commit should come after we have finished
> > updating the timer state (t->run in this case), because
> > the trigger callback slavio_timer_irq() otherwise sees
> > inconsistent half-updated state if commit() calls it.
>
> Oh, slavio_timer_irq() calls slavio_timer_get_out() which accesses the
> ptimer... OK I missed that.
>
> Whew we need to be extra cautious with this API...

Yes. If the callback function is a trivial "just update
the interrupt register bit and set an irq line" one, like
about half the ptimer users, then it doesn't matter too
much where the commit call goes, but for those users who
do more complicated work in the timer callback it gets
a little trickier (and I didn't realise this wrinkle until
about halfway through doing the API conversion work).
It doesn't much matter where the begin call goes (an odd
asymmetry), but the commit call is effectively a "voluntarily
yield control to the callback function" and so its placement
can be important.

> If possible I'd rather see the patch removing the NULL check previous to
> this one, anyway:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks; I'll add the NULL-check cleanup in v2. Coverity will
probably complain otherwise.

thanks
-- PMM

Re: [PATCH 0/2] Convert sparc devices to new ptimer API

[Mark Cave-Ayland]

On 17/10/2019 14:23, Peter Maydell wrote:

> This patchset converts the devices used by sparc machines to the new
> ptimer API.
> 
> 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
> 
> The updates to the individual ptimer devices are straightforward:
> we need to add begin/commit calls around the various places that
> modify the ptimer state, and use the new ptimer_init() function
> to create the timer.
> 
> Testing has been 'make check', and a quick smoke test of a sparc
> linux boot image I had lying around, which obviously doesn't
> exercise the devices very much, so more specific testing would
> be appreciated. I'm happy for these patches to go in via the
> sparc tree if you want, or I can collect them up with the other
> ptimer-related changes I'm sending for other archs.
> 
> thanks
> --PMM

I've given these patches a spin on my OpenBIOS test images and I don't see any
obvious regressions, so for the sun4m (slavio) part:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Frederic, are you able to make sure that the leon3 parts don't cause any problems?
Currently I don't have any outstanding SPARC patches, so if you want to include them
in a ptimer-related PR then that's fine with me.


ATB,

Mark.