[PATCH 0/3] target/avr: Fix skips vs interrupts

[PATCH 0/3] target/avr: Fix skips vs interrupts

[Richard Henderson]

Fixes https://gitlab.com/qemu-project/qemu/-/issues/1118

r~

Richard Henderson (3):
  target/avr: Call avr_cpu_do_interrupt directly
  target/avr: Only execute one interrupt at a time
  target/avr: Disable interrupts when env->skip set

 target/avr/helper.c    | 23 ++++++++++++++---------
 target/avr/translate.c | 26 ++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly

[Richard Henderson]

There is no need to go through cc->tcg_ops when
we know what value that must have.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index 82284f8997..9614ccf3e4 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -29,14 +29,13 @@
 bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     bool ret = false;
-    CPUClass *cc = CPU_GET_CLASS(cs);
     AVRCPU *cpu = AVR_CPU(cs);
     CPUAVRState *env = &cpu->env;
 
     if (interrupt_request & CPU_INTERRUPT_RESET) {
         if (cpu_interrupts_enabled(env)) {
             cs->exception_index = EXCP_RESET;
-            cc->tcg_ops->do_interrupt(cs);
+            avr_cpu_do_interrupt(cs);
 
             cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
 
@@ -47,7 +46,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
             int index = ctz32(env->intsrc);
             cs->exception_index = EXCP_INT(index);
-            cc->tcg_ops->do_interrupt(cs);
+            avr_cpu_do_interrupt(cs);
 
             env->intsrc &= env->intsrc - 1; /* clear the interrupt */
             if (!env->intsrc) {
-- 
2.34.1

[PATCH 2/3] target/avr: Only execute one interrupt at a time

[Richard Henderson]

We cannot deliver two interrupts simultaneously;
the first interrupt handler must execute first.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index 9614ccf3e4..34f1cbffb2 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -28,7 +28,6 @@
 
 bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
-    bool ret = false;
     AVRCPU *cpu = AVR_CPU(cs);
     CPUAVRState *env = &cpu->env;
 
@@ -38,8 +37,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             avr_cpu_do_interrupt(cs);
 
             cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
-
-            ret = true;
+            return true;
         }
     }
     if (interrupt_request & CPU_INTERRUPT_HARD) {
@@ -52,11 +50,10 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             if (!env->intsrc) {
                 cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
             }
-
-            ret = true;
+            return true;
         }
     }
-    return ret;
+    return false;
 }
 
 void avr_cpu_do_interrupt(CPUState *cs)
-- 
2.34.1

[PATCH 3/3] target/avr: Disable interrupts when env->skip set

[Richard Henderson]

This bit is not saved across interrupts, so we must
delay delivering the interrupt until the skip has
been processed.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1118
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.c    |  9 +++++++++
 target/avr/translate.c | 26 ++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index 34f1cbffb2..156dde4e92 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -31,6 +31,15 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     AVRCPU *cpu = AVR_CPU(cs);
     CPUAVRState *env = &cpu->env;
 
+    /*
+     * We cannot separate a skip from the next instruction,
+     * as the skip would not be preserved across the interrupt.
+     * Separating the two insn normally only happens at page boundaries.
+     */
+    if (env->skip) {
+        return false;
+    }
+
     if (interrupt_request & CPU_INTERRUPT_RESET) {
         if (cpu_interrupts_enabled(env)) {
             cs->exception_index = EXCP_RESET;
diff --git a/target/avr/translate.c b/target/avr/translate.c
index dc9c3d6bcc..026753c963 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -2971,8 +2971,18 @@ static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
     if (skip_label) {
         canonicalize_skip(ctx);
         gen_set_label(skip_label);
-        if (ctx->base.is_jmp == DISAS_NORETURN) {
+
+        switch (ctx->base.is_jmp) {
+        case DISAS_NORETURN:
             ctx->base.is_jmp = DISAS_CHAIN;
+            break;
+        case DISAS_NEXT:
+            if (ctx->base.tb->flags & TB_FLAGS_SKIP) {
+                ctx->base.is_jmp = DISAS_TOO_MANY;
+            }
+            break;
+        default:
+            break;
         }
     }
 
@@ -2989,6 +2999,11 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     bool nonconst_skip = canonicalize_skip(ctx);
+    /*
+     * Because we disable interrupts while env->skip is set,
+     * we must return to the main loop to re-evaluate afterward.
+     */
+    bool force_exit = ctx->base.tb->flags & TB_FLAGS_SKIP;
 
     switch (ctx->base.is_jmp) {
     case DISAS_NORETURN:
@@ -2997,7 +3012,7 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     case DISAS_NEXT:
     case DISAS_TOO_MANY:
     case DISAS_CHAIN:
-        if (!nonconst_skip) {
+        if (!nonconst_skip && !force_exit) {
             /* Note gen_goto_tb checks singlestep.  */
             gen_goto_tb(ctx, 1, ctx->npc);
             break;
@@ -3005,8 +3020,11 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
         tcg_gen_movi_tl(cpu_pc, ctx->npc);
         /* fall through */
     case DISAS_LOOKUP:
-        tcg_gen_lookup_and_goto_ptr();
-        break;
+        if (!force_exit) {
+            tcg_gen_lookup_and_goto_ptr();
+            break;
+        }
+        /* fall through */
     case DISAS_EXIT:
         tcg_gen_exit_tb(NULL, 0);
         break;
-- 
2.34.1

Re: [PATCH 2/3] target/avr: Only execute one interrupt at a time

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>

On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> We cannot deliver two interrupts simultaneously;
> the first interrupt handler must execute first.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/helper.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 9614ccf3e4..34f1cbffb2 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -28,7 +28,6 @@
>
>  bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
> -    bool ret = false;
>      AVRCPU *cpu = AVR_CPU(cs);
>      CPUAVRState *env = &cpu->env;
>
> @@ -38,8 +37,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>              avr_cpu_do_interrupt(cs);
>
>              cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
> -
> -            ret = true;
> +            return true;
>          }
>      }
>      if (interrupt_request & CPU_INTERRUPT_HARD) {
> @@ -52,11 +50,10 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>              if (!env->intsrc) {
>                  cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
>              }
> -
> -            ret = true;
> +            return true;
>          }
>      }
> -    return ret;
> +    return false;
>  }
>
>  void avr_cpu_do_interrupt(CPUState *cs)
> --
> 2.34.1
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 2257 bytes --]

Re: [PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>

On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> There is no need to go through cc->tcg_ops when
> we know what value that must have.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/helper.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 82284f8997..9614ccf3e4 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -29,14 +29,13 @@
>  bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      bool ret = false;
> -    CPUClass *cc = CPU_GET_CLASS(cs);
>      AVRCPU *cpu = AVR_CPU(cs);
>      CPUAVRState *env = &cpu->env;
>
>      if (interrupt_request & CPU_INTERRUPT_RESET) {
>          if (cpu_interrupts_enabled(env)) {
>              cs->exception_index = EXCP_RESET;
> -            cc->tcg_ops->do_interrupt(cs);
> +            avr_cpu_do_interrupt(cs);
>
>              cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
>
> @@ -47,7 +46,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>          if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
>              int index = ctz32(env->intsrc);
>              cs->exception_index = EXCP_INT(index);
> -            cc->tcg_ops->do_interrupt(cs);
> +            avr_cpu_do_interrupt(cs);
>
>              env->intsrc &= env->intsrc - 1; /* clear the interrupt */
>              if (!env->intsrc) {
> --
> 2.34.1
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 2349 bytes --]

Re: [PATCH 3/3] target/avr: Disable interrupts when env->skip set

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>

On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> This bit is not saved across interrupts, so we must
> delay delivering the interrupt until the skip has
> been processed.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1118
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/helper.c    |  9 +++++++++
>  target/avr/translate.c | 26 ++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 34f1cbffb2..156dde4e92 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -31,6 +31,15 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>      AVRCPU *cpu = AVR_CPU(cs);
>      CPUAVRState *env = &cpu->env;
>
> +    /*
> +     * We cannot separate a skip from the next instruction,
> +     * as the skip would not be preserved across the interrupt.
> +     * Separating the two insn normally only happens at page boundaries.
> +     */
> +    if (env->skip) {
> +        return false;
> +    }
> +
>      if (interrupt_request & CPU_INTERRUPT_RESET) {
>          if (cpu_interrupts_enabled(env)) {
>              cs->exception_index = EXCP_RESET;
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index dc9c3d6bcc..026753c963 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -2971,8 +2971,18 @@ static void avr_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
>      if (skip_label) {
>          canonicalize_skip(ctx);
>          gen_set_label(skip_label);
> -        if (ctx->base.is_jmp == DISAS_NORETURN) {
> +
> +        switch (ctx->base.is_jmp) {
> +        case DISAS_NORETURN:
>              ctx->base.is_jmp = DISAS_CHAIN;
> +            break;
> +        case DISAS_NEXT:
> +            if (ctx->base.tb->flags & TB_FLAGS_SKIP) {
> +                ctx->base.is_jmp = DISAS_TOO_MANY;
> +            }
> +            break;
> +        default:
> +            break;
>          }
>      }
>
> @@ -2989,6 +2999,11 @@ static void avr_tr_tb_stop(DisasContextBase
> *dcbase, CPUState *cs)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      bool nonconst_skip = canonicalize_skip(ctx);
> +    /*
> +     * Because we disable interrupts while env->skip is set,
> +     * we must return to the main loop to re-evaluate afterward.
> +     */
> +    bool force_exit = ctx->base.tb->flags & TB_FLAGS_SKIP;
>
>      switch (ctx->base.is_jmp) {
>      case DISAS_NORETURN:
> @@ -2997,7 +3012,7 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase,
> CPUState *cs)
>      case DISAS_NEXT:
>      case DISAS_TOO_MANY:
>      case DISAS_CHAIN:
> -        if (!nonconst_skip) {
> +        if (!nonconst_skip && !force_exit) {
>              /* Note gen_goto_tb checks singlestep.  */
>              gen_goto_tb(ctx, 1, ctx->npc);
>              break;
> @@ -3005,8 +3020,11 @@ static void avr_tr_tb_stop(DisasContextBase
> *dcbase, CPUState *cs)
>          tcg_gen_movi_tl(cpu_pc, ctx->npc);
>          /* fall through */
>      case DISAS_LOOKUP:
> -        tcg_gen_lookup_and_goto_ptr();
> -        break;
> +        if (!force_exit) {
> +            tcg_gen_lookup_and_goto_ptr();
> +            break;
> +        }
> +        /* fall through */
>      case DISAS_EXIT:
>          tcg_gen_exit_tb(NULL, 0);
>          break;
> --
> 2.34.1
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 4701 bytes --]

Re: [PATCH 0/3] target/avr: Fix skips vs interrupts

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>

On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> Fixes https://gitlab.com/qemu-project/qemu/-/issues/1118
>
> r~
>
> Richard Henderson (3):
>   target/avr: Call avr_cpu_do_interrupt directly
>   target/avr: Only execute one interrupt at a time
>   target/avr: Disable interrupts when env->skip set
>
>  target/avr/helper.c    | 23 ++++++++++++++---------
>  target/avr/translate.c | 26 ++++++++++++++++++++++----
>  2 files changed, 36 insertions(+), 13 deletions(-)
>
> --
> 2.34.1
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 1191 bytes --]

Re: [PATCH 0/3] target/avr: Fix skips vs interrupts

[Philippe Mathieu-Daudé via]

On 26/8/22 22:55, Richard Henderson wrote:
> Fixes https://gitlab.com/qemu-project/qemu/-/issues/1118

Thanks!

> 
> Richard Henderson (3):
>    target/avr: Call avr_cpu_do_interrupt directly
>    target/avr: Only execute one interrupt at a time
>    target/avr: Disable interrupts when env->skip set

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>