[qemu.git] target/imx: reload cmp timer outside of the reload ptimer transaction

Commit Message

~axelheider Oct. 19, 2022, 1:09 p.m. UTC

From: Axel Heider <[e-mail]>

When running seL4 tests (https://docs.sel4.systems/projects/sel4test)
on the sabrelight platform, the timer tests fail. The arm/imx6 EPIT
timer interrupt does not fire properly, instead of a e.g. second in
can take up to a minute to finally see the interrupt.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1263

Signed-off-by: Axel Heider <[e-mail]>
---
Fixed the comment style and the commit message.

> Do we also need to change the other places that call
> imx_epit_reload_compare_timer() in the handling of CR
> register writes ? (Those are a little more tricky.)

The current patch fixed the issue we are seeing. I'm not really
an expert on the QEMU code here and still try to understand
all details. Might also be that we never hit the other code paths
in the end.

 hw/timer/imx_epit.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Peter Maydell Oct. 25, 2022, 12:04 p.m. UTC | #1

On Mon, 24 Oct 2022 at 18:06, ~axelheider <[e-mail]> wrote:
>
> From: Axel Heider <[e-mail]>
>
> When running seL4 tests (https://docs.sel4.systems/projects/sel4test)
> on the sabrelight platform, the timer tests fail. The arm/imx6 EPIT
> timer interrupt does not fire properly, instead of a e.g. second in
> can take up to a minute to finally see the interrupt.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1263
>
> Signed-off-by: Axel Heider <[e-mail]>
> ---
> Fixed the comment style and the commit message.
>
> > Do we also need to change the other places that call
> > imx_epit_reload_compare_timer() in the handling of CR
> > register writes ? (Those are a little more tricky.)
>
> The current patch fixed the issue we are seeing. I'm not really
> an expert on the QEMU code here and still try to understand
> all details. Might also be that we never hit the other code paths
> in the end.

I suspect your guest happens to initialize the timer in
such a way that it doesn't matter if we don't get the
comparison stuff right on the write to the control
register: conceptually I think the CR write code has the
same bug where we call imx_epit_reload_compare_timer()
before we've finalized the value of the timer_reload value.

Anyway, this patch is definitely correct for the LR
register write, so I've applied it to target-arm.next.

thanks
-- PMM

Patch

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 2bf8c754b2..ec0fa440d7 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -275,10 +275,15 @@  static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
             /* If IOVW bit is set then set the timer value */
             ptimer_set_count(s->timer_reload, s->lr);
         }
-
+        /*
+         * Commit the change to s->timer_reload, so it can propagate. Otherwise
+         * the timer interrupt may not fire properly. The commit must happen
+         * before calling imx_epit_reload_compare_timer(), which reads
+         * s->timer_reload internally again.
+         */
+        ptimer_transaction_commit(s->timer_reload);
         imx_epit_reload_compare_timer(s);
         ptimer_transaction_commit(s->timer_cmp);
-        ptimer_transaction_commit(s->timer_reload);
         break;
 
     case 3: /* CMP */