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

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

Thanks,

Phil.