[PATCH 0/3] Convert ppc and microblaze devices to new ptimer API

[PATCH 0/3] Convert ppc and microblaze devices to new ptimer API

[Peter Maydell]

This patchset converts the devices used by ppc and microblaze
machines to the new ptimer API. (xilinx_timer is used by both,
hence putting both archs in the same patchset).

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' only, 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
ppc 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 (3):
  hw/net/fsl_etsec/etsec.c: Switch to transaction-based ptimer API
  hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
  hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

 hw/net/fsl_etsec/etsec.h |  1 -
 hw/dma/xilinx_axidma.c   |  9 +++++----
 hw/net/fsl_etsec/etsec.c |  9 +++++----
 hw/timer/xilinx_timer.c  | 13 ++++++++-----
 4 files changed, 18 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH 1/3] hw/net/fsl_etsec/etsec.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the fsl_etsec 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/net/fsl_etsec/etsec.h | 1 -
 hw/net/fsl_etsec/etsec.c | 9 +++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index 09d05c21338..7951c3ad65f 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -141,7 +141,6 @@ typedef struct eTSEC {
     uint16_t phy_control;
 
     /* Polling */
-    QEMUBH *bh;
     struct ptimer_state *ptimer;
 
     /* Whether we should flush the rx queue when buffer becomes available. */
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index d9b3e8c691e..717de76569a 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -34,7 +34,6 @@
 #include "etsec.h"
 #include "registers.h"
 #include "qemu/log.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 /* #define HEX_DUMP */
@@ -195,9 +194,11 @@ static void write_dmactrl(eTSEC          *etsec,
 
     if (!(value & DMACTRL_WOP)) {
         /* Start polling */
+        ptimer_transaction_begin(etsec->ptimer);
         ptimer_stop(etsec->ptimer);
         ptimer_set_count(etsec->ptimer, 1);
         ptimer_run(etsec->ptimer, 1);
+        ptimer_transaction_commit(etsec->ptimer);
     }
 }
 
@@ -391,10 +392,10 @@ static void etsec_realize(DeviceState *dev, Error **errp)
                               object_get_typename(OBJECT(dev)), dev->id, etsec);
     qemu_format_nic_info_str(qemu_get_queue(etsec->nic), etsec->conf.macaddr.a);
 
-
-    etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
-    etsec->ptimer = ptimer_init_with_bh(etsec->bh, PTIMER_POLICY_DEFAULT);
+    etsec->ptimer = ptimer_init(etsec_timer_hit, etsec, PTIMER_POLICY_DEFAULT);
+    ptimer_transaction_begin(etsec->ptimer);
     ptimer_set_freq(etsec->ptimer, 100);
+    ptimer_transaction_commit(etsec->ptimer);
 }
 
 static void etsec_instance_init(Object *obj)
-- 
2.20.1

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

[Peter Maydell]

Switch the xilinx_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/xilinx_timer.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
index 92dbff304d9..7191ea54f58 100644
--- a/hw/timer/xilinx_timer.c
+++ b/hw/timer/xilinx_timer.c
@@ -28,7 +28,6 @@
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 #define D(x)
@@ -52,7 +51,6 @@
 
 struct xlx_timer
 {
-    QEMUBH *bh;
     ptimer_state *ptimer;
     void *parent;
     int nr; /* for debug.  */
@@ -134,6 +132,7 @@ timer_read(void *opaque, hwaddr addr, unsigned int size)
     return r;
 }
 
+/* Must be called inside ptimer transaction block */
 static void timer_enable(struct xlx_timer *xt)
 {
     uint64_t count;
@@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,
                 value &= ~TCSR_TINT;
 
             xt->regs[addr] = value & 0x7ff;
-            if (value & TCSR_ENT)
+            if (value & TCSR_ENT) {
+                ptimer_transaction_begin(xt->ptimer);
                 timer_enable(xt);
+                ptimer_transaction_commit(xt->ptimer);
+            }
             break;
  
         default:
@@ -220,9 +222,10 @@ 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_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);
+        xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);
+        ptimer_transaction_begin(xt->ptimer);
         ptimer_set_freq(xt->ptimer, t->freq_hz);
+        ptimer_transaction_commit(xt->ptimer);
     }
 
     memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",
-- 
2.20.1

[PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index e035d1f7504..fb3a978e282 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -31,7 +31,6 @@
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 #include "hw/stream.h"
@@ -104,7 +103,6 @@ enum {
 };
 
 struct Stream {
-    QEMUBH *bh;
     ptimer_state *ptimer;
     qemu_irq irq;
 
@@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s)
     unsigned int comp_delay;
 
     /* Start the delayed timer.  */
+    ptimer_transaction_begin(s->ptimer);
     comp_delay = s->regs[R_DMACR] >> 24;
     if (comp_delay) {
         ptimer_stop(s->ptimer);
@@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s)
         s->regs[R_DMASR] |= DMASR_IOC_IRQ;
         stream_reload_complete_cnt(s);
     }
+    ptimer_transaction_commit(s->ptimer);
 }
 
 static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
@@ -551,9 +551,10 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
         struct Stream *st = &s->streams[i];
 
         st->nr = i;
-        st->bh = qemu_bh_new(timer_hit, st);
-        st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
+        st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT);
+        ptimer_transaction_begin(st->ptimer);
         ptimer_set_freq(st->ptimer, s->freqhz);
+        ptimer_transaction_commit(st->ptimer);
     }
     return;
 
-- 
2.20.1

Re: [PATCH 1/3] hw/net/fsl_etsec/etsec.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:21 AM, Peter Maydell wrote:
> Switch the fsl_etsec 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/net/fsl_etsec/etsec.h | 1 -
>  hw/net/fsl_etsec/etsec.c | 9 +++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)

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


r~

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

[Richard Henderson]

On 10/17/19 6:21 AM, Peter Maydell wrote:
> Switch the xilinx_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/xilinx_timer.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

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


r~

Re: [PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:21 AM, Peter Maydell wrote:
> Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

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


r~

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

[Philippe Mathieu-Daudé]

Hi Peter,

On 10/17/19 3:21 PM, Peter Maydell wrote:
> Switch the xilinx_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/xilinx_timer.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
> index 92dbff304d9..7191ea54f58 100644
> --- a/hw/timer/xilinx_timer.c
> +++ b/hw/timer/xilinx_timer.c
> @@ -28,7 +28,6 @@
>   #include "hw/ptimer.h"
>   #include "hw/qdev-properties.h"
>   #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   #define D(x)
> @@ -52,7 +51,6 @@
>   
>   struct xlx_timer
>   {
> -    QEMUBH *bh;
>       ptimer_state *ptimer;
>       void *parent;
>       int nr; /* for debug.  */
> @@ -134,6 +132,7 @@ timer_read(void *opaque, hwaddr addr, unsigned int size)
>       return r;
>   }
>   
> +/* Must be called inside ptimer transaction block */
>   static void timer_enable(struct xlx_timer *xt)
>   {
>       uint64_t count;
> @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,
>                   value &= ~TCSR_TINT;
>   
>               xt->regs[addr] = value & 0x7ff;
> -            if (value & TCSR_ENT)
> +            if (value & TCSR_ENT) {
> +                ptimer_transaction_begin(xt->ptimer);
>                   timer_enable(xt);
> +                ptimer_transaction_commit(xt->ptimer);

Why not move these inside timer_enable()?

> +            }
>               break;
>    
>           default:
> @@ -220,9 +222,10 @@ 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_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);
> +        xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);
> +        ptimer_transaction_begin(xt->ptimer);
>           ptimer_set_freq(xt->ptimer, t->freq_hz);
> +        ptimer_transaction_commit(xt->ptimer);
>       }
>   
>       memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",
> 

Re: [PATCH 1/3] hw/net/fsl_etsec/etsec.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:21 PM, Peter Maydell wrote:
> Switch the fsl_etsec 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/net/fsl_etsec/etsec.h | 1 -
>   hw/net/fsl_etsec/etsec.c | 9 +++++----
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 09d05c21338..7951c3ad65f 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -141,7 +141,6 @@ typedef struct eTSEC {
>       uint16_t phy_control;
>   
>       /* Polling */
> -    QEMUBH *bh;
>       struct ptimer_state *ptimer;
>   
>       /* Whether we should flush the rx queue when buffer becomes available. */
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index d9b3e8c691e..717de76569a 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -34,7 +34,6 @@
>   #include "etsec.h"
>   #include "registers.h"
>   #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   /* #define HEX_DUMP */
> @@ -195,9 +194,11 @@ static void write_dmactrl(eTSEC          *etsec,
>   
>       if (!(value & DMACTRL_WOP)) {
>           /* Start polling */
> +        ptimer_transaction_begin(etsec->ptimer);
>           ptimer_stop(etsec->ptimer);
>           ptimer_set_count(etsec->ptimer, 1);
>           ptimer_run(etsec->ptimer, 1);
> +        ptimer_transaction_commit(etsec->ptimer);
>       }
>   }
>   
> @@ -391,10 +392,10 @@ static void etsec_realize(DeviceState *dev, Error **errp)
>                                 object_get_typename(OBJECT(dev)), dev->id, etsec);
>       qemu_format_nic_info_str(qemu_get_queue(etsec->nic), etsec->conf.macaddr.a);
>   
> -
> -    etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
> -    etsec->ptimer = ptimer_init_with_bh(etsec->bh, PTIMER_POLICY_DEFAULT);
> +    etsec->ptimer = ptimer_init(etsec_timer_hit, etsec, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(etsec->ptimer);
>       ptimer_set_freq(etsec->ptimer, 100);
> +    ptimer_transaction_commit(etsec->ptimer);
>   }
>   
>   static void etsec_instance_init(Object *obj)
> 

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

Re: [PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

Hi Peter,

On 10/17/19 3:21 PM, Peter Maydell wrote:
> Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index e035d1f7504..fb3a978e282 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -31,7 +31,6 @@
>   #include "hw/ptimer.h"
>   #include "hw/qdev-properties.h"
>   #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   #include "hw/stream.h"
> @@ -104,7 +103,6 @@ enum {
>   };
>   
>   struct Stream {
> -    QEMUBH *bh;
>       ptimer_state *ptimer;
>       qemu_irq irq;
>   
> @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s)
>       unsigned int comp_delay;
>   
>       /* Start the delayed timer.  */
> +    ptimer_transaction_begin(s->ptimer);
>       comp_delay = s->regs[R_DMACR] >> 24;
>       if (comp_delay) {
>           ptimer_stop(s->ptimer);
> @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s)
>           s->regs[R_DMASR] |= DMASR_IOC_IRQ;
>           stream_reload_complete_cnt(s);
>       }
> +    ptimer_transaction_commit(s->ptimer);

I'd restrict the transaction here within the if() statement:

-- >8 --
@@ -244,9 +244,11 @@ static void stream_complete(struct Stream *s)
      /* Start the delayed timer.  */
      comp_delay = s->regs[R_DMACR] >> 24;
      if (comp_delay) {
+        ptimer_transaction_begin(s->ptimer);
          ptimer_stop(s->ptimer);
          ptimer_set_count(s->ptimer, comp_delay);
          ptimer_run(s->ptimer, 1);
+        ptimer_transaction_commit(s->ptimer);
      }

      s->complete_cnt--;
---

>   }
>   
>   static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
> @@ -551,9 +551,10 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
>           struct Stream *st = &s->streams[i];
>   
>           st->nr = i;
> -        st->bh = qemu_bh_new(timer_hit, st);
> -        st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
> +        st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT);
> +        ptimer_transaction_begin(st->ptimer);
>           ptimer_set_freq(st->ptimer, s->freqhz);
> +        ptimer_transaction_commit(st->ptimer);
>       }
>       return;
>   
> 

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

[Peter Maydell]

On Thu, 17 Oct 2019 at 15:57, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Peter,
>
> On 10/17/19 3:21 PM, Peter Maydell wrote:
> > +/* Must be called inside ptimer transaction block */
> >   static void timer_enable(struct xlx_timer *xt)
> >   {
> >       uint64_t count;
> > @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,
> >                   value &= ~TCSR_TINT;
> >
> >               xt->regs[addr] = value & 0x7ff;
> > -            if (value & TCSR_ENT)
> > +            if (value & TCSR_ENT) {
> > +                ptimer_transaction_begin(xt->ptimer);
> >                   timer_enable(xt);
> > +                ptimer_transaction_commit(xt->ptimer);
>
> Why not move these inside timer_enable()?

Because timer_enable() is called from the callback
function timer_hit(). Since callback functions are called
from within a begin/commit block, if we called begin
again in timer_enable() it would assert().

thanks
-- PMM

Re: [PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 16:01, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Peter,
>
> On 10/17/19 3:21 PM, Peter Maydell wrote:
> > Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index e035d1f7504..fb3a978e282 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -31,7 +31,6 @@
> >   #include "hw/ptimer.h"
> >   #include "hw/qdev-properties.h"
> >   #include "qemu/log.h"
> > -#include "qemu/main-loop.h"
> >   #include "qemu/module.h"
> >
> >   #include "hw/stream.h"
> > @@ -104,7 +103,6 @@ enum {
> >   };
> >
> >   struct Stream {
> > -    QEMUBH *bh;
> >       ptimer_state *ptimer;
> >       qemu_irq irq;
> >
> > @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s)
> >       unsigned int comp_delay;
> >
> >       /* Start the delayed timer.  */
> > +    ptimer_transaction_begin(s->ptimer);
> >       comp_delay = s->regs[R_DMACR] >> 24;
> >       if (comp_delay) {
> >           ptimer_stop(s->ptimer);
> > @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s)
> >           s->regs[R_DMASR] |= DMASR_IOC_IRQ;
> >           stream_reload_complete_cnt(s);
> >       }
> > +    ptimer_transaction_commit(s->ptimer);
>
> I'd restrict the transaction here within the if() statement:
>
> -- >8 --
> @@ -244,9 +244,11 @@ static void stream_complete(struct Stream *s)
>       /* Start the delayed timer.  */
>       comp_delay = s->regs[R_DMACR] >> 24;
>       if (comp_delay) {
> +        ptimer_transaction_begin(s->ptimer);
>           ptimer_stop(s->ptimer);
>           ptimer_set_count(s->ptimer, comp_delay);
>           ptimer_run(s->ptimer, 1);
> +        ptimer_transaction_commit(s->ptimer);
>       }
>
>       s->complete_cnt--;

The timer_hit callback function itself writes to
s->complete_cnt, so we don't want to allow it to
be called (via the commit()) before stream_complete()
is done with changing that state.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 10/17/19 5:03 PM, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 15:57, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 10/17/19 3:21 PM, Peter Maydell wrote:
>>> +/* Must be called inside ptimer transaction block */
>>>    static void timer_enable(struct xlx_timer *xt)
>>>    {
>>>        uint64_t count;
>>> @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,
>>>                    value &= ~TCSR_TINT;
>>>
>>>                xt->regs[addr] = value & 0x7ff;
>>> -            if (value & TCSR_ENT)
>>> +            if (value & TCSR_ENT) {
>>> +                ptimer_transaction_begin(xt->ptimer);
>>>                    timer_enable(xt);
>>> +                ptimer_transaction_commit(xt->ptimer);
>>
>> Why not move these inside timer_enable()?
> 
> Because timer_enable() is called from the callback
> function timer_hit(). Since callback functions are called
> from within a begin/commit block, if we called begin
> again in timer_enable() it would assert().

Yes, now I understood the point of this new API ;)

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

Re: [PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 5:06 PM, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 16:01, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 10/17/19 3:21 PM, Peter Maydell wrote:
>>> Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++----
>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>>> index e035d1f7504..fb3a978e282 100644
>>> --- a/hw/dma/xilinx_axidma.c
>>> +++ b/hw/dma/xilinx_axidma.c
>>> @@ -31,7 +31,6 @@
>>>    #include "hw/ptimer.h"
>>>    #include "hw/qdev-properties.h"
>>>    #include "qemu/log.h"
>>> -#include "qemu/main-loop.h"
>>>    #include "qemu/module.h"
>>>
>>>    #include "hw/stream.h"
>>> @@ -104,7 +103,6 @@ enum {
>>>    };
>>>
>>>    struct Stream {
>>> -    QEMUBH *bh;
>>>        ptimer_state *ptimer;
>>>        qemu_irq irq;
>>>
>>> @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s)
>>>        unsigned int comp_delay;
>>>
>>>        /* Start the delayed timer.  */
>>> +    ptimer_transaction_begin(s->ptimer);
>>>        comp_delay = s->regs[R_DMACR] >> 24;
>>>        if (comp_delay) {
>>>            ptimer_stop(s->ptimer);
>>> @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s)
>>>            s->regs[R_DMASR] |= DMASR_IOC_IRQ;
>>>            stream_reload_complete_cnt(s);
>>>        }
>>> +    ptimer_transaction_commit(s->ptimer);
>>
>> I'd restrict the transaction here within the if() statement:
>>
>> -- >8 --
>> @@ -244,9 +244,11 @@ static void stream_complete(struct Stream *s)
>>        /* Start the delayed timer.  */
>>        comp_delay = s->regs[R_DMACR] >> 24;
>>        if (comp_delay) {
>> +        ptimer_transaction_begin(s->ptimer);
>>            ptimer_stop(s->ptimer);
>>            ptimer_set_count(s->ptimer, comp_delay);
>>            ptimer_run(s->ptimer, 1);
>> +        ptimer_transaction_commit(s->ptimer);
>>        }
>>
>>        s->complete_cnt--;
> 
> The timer_hit callback function itself writes to
> s->complete_cnt, so we don't want to allow it to
> be called (via the commit()) before stream_complete()
> is done with changing that state.

Indeed we have timer_hit() -> stream_reload_complete_cnt() which uses 
s->complete_cnt.

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

Re: [PATCH 1/3] hw/net/fsl_etsec/etsec.c: Switch to transaction-based ptimer API

[Alistair Francis]

On Thu, Oct 17, 2019 at 6:42 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Switch the fsl_etsec 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/net/fsl_etsec/etsec.h | 1 -
>  hw/net/fsl_etsec/etsec.c | 9 +++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 09d05c21338..7951c3ad65f 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -141,7 +141,6 @@ typedef struct eTSEC {
>      uint16_t phy_control;
>
>      /* Polling */
> -    QEMUBH *bh;
>      struct ptimer_state *ptimer;
>
>      /* Whether we should flush the rx queue when buffer becomes available. */
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index d9b3e8c691e..717de76569a 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -34,7 +34,6 @@
>  #include "etsec.h"
>  #include "registers.h"
>  #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>  #include "qemu/module.h"
>
>  /* #define HEX_DUMP */
> @@ -195,9 +194,11 @@ static void write_dmactrl(eTSEC          *etsec,
>
>      if (!(value & DMACTRL_WOP)) {
>          /* Start polling */
> +        ptimer_transaction_begin(etsec->ptimer);
>          ptimer_stop(etsec->ptimer);
>          ptimer_set_count(etsec->ptimer, 1);
>          ptimer_run(etsec->ptimer, 1);
> +        ptimer_transaction_commit(etsec->ptimer);
>      }
>  }
>
> @@ -391,10 +392,10 @@ static void etsec_realize(DeviceState *dev, Error **errp)
>                                object_get_typename(OBJECT(dev)), dev->id, etsec);
>      qemu_format_nic_info_str(qemu_get_queue(etsec->nic), etsec->conf.macaddr.a);
>
> -
> -    etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
> -    etsec->ptimer = ptimer_init_with_bh(etsec->bh, PTIMER_POLICY_DEFAULT);
> +    etsec->ptimer = ptimer_init(etsec_timer_hit, etsec, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(etsec->ptimer);
>      ptimer_set_freq(etsec->ptimer, 100);
> +    ptimer_transaction_commit(etsec->ptimer);
>  }
>
>  static void etsec_instance_init(Object *obj)
> --
> 2.20.1
>
>

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

[Alistair Francis]

On Thu, Oct 17, 2019 at 6:50 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Switch the xilinx_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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/timer/xilinx_timer.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
> index 92dbff304d9..7191ea54f58 100644
> --- a/hw/timer/xilinx_timer.c
> +++ b/hw/timer/xilinx_timer.c
> @@ -28,7 +28,6 @@
>  #include "hw/ptimer.h"
>  #include "hw/qdev-properties.h"
>  #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>  #include "qemu/module.h"
>
>  #define D(x)
> @@ -52,7 +51,6 @@
>
>  struct xlx_timer
>  {
> -    QEMUBH *bh;
>      ptimer_state *ptimer;
>      void *parent;
>      int nr; /* for debug.  */
> @@ -134,6 +132,7 @@ timer_read(void *opaque, hwaddr addr, unsigned int size)
>      return r;
>  }
>
> +/* Must be called inside ptimer transaction block */
>  static void timer_enable(struct xlx_timer *xt)
>  {
>      uint64_t count;
> @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,
>                  value &= ~TCSR_TINT;
>
>              xt->regs[addr] = value & 0x7ff;
> -            if (value & TCSR_ENT)
> +            if (value & TCSR_ENT) {
> +                ptimer_transaction_begin(xt->ptimer);
>                  timer_enable(xt);
> +                ptimer_transaction_commit(xt->ptimer);
> +            }
>              break;
>
>          default:
> @@ -220,9 +222,10 @@ 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_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);
> +        xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);
> +        ptimer_transaction_begin(xt->ptimer);
>          ptimer_set_freq(xt->ptimer, t->freq_hz);
> +        ptimer_transaction_commit(xt->ptimer);
>      }
>
>      memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",
> --
> 2.20.1
>
>

Re: [PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Alistair Francis]

On Thu, Oct 17, 2019 at 6:53 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Switch the xilinx_axidma 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/dma/xilinx_axidma.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index e035d1f7504..fb3a978e282 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -31,7 +31,6 @@
>  #include "hw/ptimer.h"
>  #include "hw/qdev-properties.h"
>  #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>  #include "qemu/module.h"
>
>  #include "hw/stream.h"
> @@ -104,7 +103,6 @@ enum {
>  };
>
>  struct Stream {
> -    QEMUBH *bh;
>      ptimer_state *ptimer;
>      qemu_irq irq;
>
> @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s)
>      unsigned int comp_delay;
>
>      /* Start the delayed timer.  */
> +    ptimer_transaction_begin(s->ptimer);
>      comp_delay = s->regs[R_DMACR] >> 24;
>      if (comp_delay) {
>          ptimer_stop(s->ptimer);
> @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s)
>          s->regs[R_DMASR] |= DMASR_IOC_IRQ;
>          stream_reload_complete_cnt(s);
>      }
> +    ptimer_transaction_commit(s->ptimer);
>  }
>
>  static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
> @@ -551,9 +551,10 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
>          struct Stream *st = &s->streams[i];
>
>          st->nr = i;
> -        st->bh = qemu_bh_new(timer_hit, st);
> -        st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT);
> +        st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT);
> +        ptimer_transaction_begin(st->ptimer);
>          ptimer_set_freq(st->ptimer, s->freqhz);
> +        ptimer_transaction_commit(st->ptimer);
>      }
>      return;
>
> --
> 2.20.1
>
>

Re: [PATCH 0/3] Convert ppc and microblaze devices to new ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 14:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset converts the devices used by ppc and microblaze
> machines to the new ptimer API. (xilinx_timer is used by both,
> hence putting both archs in the same patchset).
>
> 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' only, 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
> ppc tree if you want, or I can collect them up with the other
> ptimer-related changes I'm sending for other archs.

I'll put these in via target-arm.next, since they've all
been reviewed now and nobody's suggested a preference
for some other route.

thanks
-- PMM

Re: [PATCH 0/3] Convert ppc and microblaze devices to new ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 14:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset converts the devices used by ppc and microblaze
> machines to the new ptimer API. (xilinx_timer is used by both,
> hence putting both archs in the same patchset).

Apologies for the duplicate cover-letter: messed up the
git-send-email command line the first time and sent just
the cover letter without the actual patches.

-- PMM

[PATCH 0/3] Convert ppc and microblaze devices to new ptimer API

[Peter Maydell]

This patchset converts the devices used by ppc and microblaze
machines to the new ptimer API. (xilinx_timer is used by both,
hence putting both archs in the same patchset).

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' only, 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
ppc 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 (3):
  hw/net/fsl_etsec/etsec.c: Switch to transaction-based ptimer API
  hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
  hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

 hw/net/fsl_etsec/etsec.h |  1 -
 hw/dma/xilinx_axidma.c   |  9 +++++----
 hw/net/fsl_etsec/etsec.c |  9 +++++----
 hw/timer/xilinx_timer.c  | 13 ++++++++-----
 4 files changed, 18 insertions(+), 14 deletions(-)

-- 
2.20.1