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.