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é <address@hidden> 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 <address@hidden>
> > ---
> >   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