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

Message ID 20191017132351.4762-3-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • Convert sparc devices to new ptimer API
Related show

Commit Message

Peter Maydell Oct. 17, 2019, 1:23 p.m.
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(-)

-- 
2.20.1

Comments

Richard Henderson Oct. 17, 2019, 2:21 p.m. | #1
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~
Philippe Mathieu-Daudé Oct. 17, 2019, 2:54 p.m. | #2
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);

>
Peter Maydell Oct. 17, 2019, 3 p.m. | #3
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
Philippe Mathieu-Daudé Oct. 17, 2019, 3:22 p.m. | #4
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.
Peter Maydell Oct. 17, 2019, 3:31 p.m. | #5
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

Patch

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);