Re: [PATCH v3 3/4] hw/timer/bcm2835: Support the timer COMPARE registers

[Philippe Mathieu-Daudé]

On 10/3/20 7:17 PM, Richard Henderson wrote:
> On 10/2/20 11:42 AM, Philippe Mathieu-Daudé wrote:
> > @@ -78,16 +71,29 @@ static void bcm2835_systmr_write(void *opaque, hwaddr 
> > offset,
> >                                    uint64_t value, unsigned size)
> >   {
> >       BCM2835SystemTimerState *s = BCM2835_SYSTIMER(opaque);
> > +    int index;
> > +    uint64_t now;
> > +    uint64_t triggers_delay_us;
> >   
> >       trace_bcm2835_systmr_write(offset, value);
> >       switch (offset) {
> >       case A_CTRL_STATUS:
> >           s->reg.ctrl_status &= ~value; /* Ack */
> > -        bcm2835_systmr_update_irq(s);
> > +        for (index = 0; index < ARRAY_SIZE(s->tmr); index++) {
> > +            if (extract32(value, index, 1)) {
> > +                trace_bcm2835_systmr_irq_ack(index);
> > +                qemu_set_irq(s->tmr[index].irq, 0);
> > +            }
>
> I think it might be instructive to have the parameter be uint64_t value64, and
> the immediately do
>
>      uint32_t value = value64;
>
> That matches up better with extract32, the trace arguments...
>
> > +        }
> >           break;
> >       case A_COMPARE0 ... A_COMPARE3:
> > -        s->reg.compare[(offset - A_COMPARE0) >> 2] = value;
> > -        bcm2835_systmr_update_compare(s, (offset - A_COMPARE0) >> 2);
> > +        index = (offset - A_COMPARE0) >> 2;
> > +        s->reg.compare[index] = value;
> > +        now = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL);
> > +        /* Compare lower 32-bits of the free-running counter. */
> > +        triggers_delay_us = value - (now & UINT32_MAX);
> > +        trace_bcm2835_systmr_run(index, triggers_delay_us);
> > +        timer_mod(&s->tmr[index].timer, now + triggers_delay_us);
>
> ... and here.
>
> Also, the arithmetic looks off.
>
> Consider when you want a long timeout, and pass in a value slightly below now.
>   So, e.g.
>
>    now   = 0xabcdffffffff;
>    value = 0x0000fffffffe;
>
> since triggers_delay_us is uint64_t, that comparison becomes
>
>    triggers_delay_us = 0x0000fffffffe - 0xffffffff;
>                      = 0xffffffffffffffff;
>
> Then you add back in now, and do *not* get a value in the future:
>
>      now + triggers_delay_us
>    = 0xabcdffffffff + 0xffffffffffffffff
>    = 0xabcdfffffffe

Thanks for the example of wrong behavior...

> What I think you want is
>
>    uint32_t triggers_delay_us = value - now
>                               = 0xffffffff;
>
> which then zero-extends when you add back to now to get
>
>      now + triggers_delay_us
>    = 0xabcdffffffff + 0xffffffff
>    = 0xabcefffffffe
>
> which is indeed in the future.

... and the correct one :)

I'll correct as suggested.

Thanks!

Phil.