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

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

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
See https://gitlab.com/qemu-project/qemu/-/issues/1263
When running the seL4 tests
(https://docs.sel4.systems/projects/sel4test), on the sabrelight
platform the timer test fails (and thus it's disabled by default).
Investigation has shown that the arm/imx6 EPIT timer interrupt does not
fire properly, instead of a second in can take up to a minute to finally
see the interrupt.

 hw/timer/imx_epit.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 2bf8c754b2..0b13c1eab0 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -276,9 +276,12 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
             ptimer_set_count(s->timer_reload, s->lr);
         }
 
+        // commit s->timer_reload before imx_epit_reload_compare_timer
+        // as timer_reload is read in imx_epit_reload_compare_timer
+        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 */
-- 
2.34.5

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

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

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 <axel.heider@hensoldt.net>
---
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(-)

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 */
-- 
2.34.5

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

[Peter Maydell]

On Fri, 21 Oct 2022 at 20:18, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
> See https://gitlab.com/qemu-project/qemu/-/issues/1263
> When running the seL4 tests
> (https://docs.sel4.systems/projects/sel4test), on the sabrelight
> platform the timer test fails (and thus it's disabled by default).
> Investigation has shown that the arm/imx6 EPIT timer interrupt does not
> fire properly, instead of a second in can take up to a minute to finally
> see the interrupt.
>
>  hw/timer/imx_epit.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 2bf8c754b2..0b13c1eab0 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -276,9 +276,12 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
>              ptimer_set_count(s->timer_reload, s->lr);
>          }
>
> +        // commit s->timer_reload before imx_epit_reload_compare_timer
> +        // as timer_reload is read in imx_epit_reload_compare_timer

QEMU coding style requires /* ... */ for comments, not //
(with the /* and */ on lines of their own if it's a multiline
comment.

> +        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;

Yes, I see what's happening here. It's OK to commit the timer_reload
timer transaction first because that timer doesn't care about the
timer_cmp timer.

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

thanks
-- PMM

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

[Peter Maydell]

On Mon, 24 Oct 2022 at 13:37, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 21 Oct 2022 at 20:18, ~axelheider <axelheider@git.sr.ht> wrote:
> >
> > From: Axel Heider <axel.heider@hensoldt.net>
> >
> > Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> > ---
> > See https://gitlab.com/qemu-project/qemu/-/issues/1263
> > When running the seL4 tests
> > (https://docs.sel4.systems/projects/sel4test), on the sabrelight
> > platform the timer test fails (and thus it's disabled by default).
> > Investigation has shown that the arm/imx6 EPIT timer interrupt does not
> > fire properly, instead of a second in can take up to a minute to finally
> > see the interrupt.

Oh yes, this sort of information should ideally be in the commit
message proper -- the commit message is the place to explain what
you're changing and why. You can make the gitlab issue auto-close
by putting a line

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

into the commit message.

thanks
-- PMM

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

[Peter Maydell]

On Mon, 24 Oct 2022 at 18:06, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> 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 <axel.heider@hensoldt.net>
> ---
> 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