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

[Richard Henderson <richard.henderson@linaro.org>]

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

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.


r~