[PATCH v2 for-8.0] target/arm: Fix generated code for cpreg reads when HSTR is active

[PATCH v2 for-8.0] target/arm: Fix generated code for cpreg reads when HSTR is active

[Peter Maydell]

In commit 049edada we added some code to handle HSTR_EL2 traps, which
we did as an inline "conditionally branch over a
gen_exception_insn()".  Unfortunately this fails to take account of
the fact that gen_exception_insn() will set s->base.is_jmp to
DISAS_NORETURN.  That means that at the end of the TB we won't
generate the necessary code to handle the "branched over the trap and
continued normal execution" codepath.  The result is that the TCG
main loop thinks that we stopped execution of the TB due to a
situation that only happens when icount is enabled, and hits an
assertion. Explicitly set is_jmp back to DISAS_NEXT so we generate
the correct code for when execution continues past this insn.

Note that this only happens for cpreg reads; writes will call
gen_lookup_tb() which generates a valid end-of-TB.

Fixes: 049edada ("target/arm: Make HSTR_EL2 traps take priority over UNDEF-at-EL1")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1551
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
v1->v2: just unconditionally set is_jmp to DISAS_NEXT.
---
 target/arm/tcg/translate.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 2cb9368b1ba..3c8401e9086 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -4623,6 +4623,12 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
             tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, over.label);
 
             gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
+            /*
+             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
+             * but since we're conditionally branching over it, we want
+             * to assume continue-to-next-instruction.
+             */
+            s->base.is_jmp = DISAS_NEXT;
             set_disas_label(s, over);
         }
     }
-- 
2.34.1

Re: [PATCH v2 for-8.0] target/arm: Fix generated code for cpreg reads when HSTR is active

[Richard Henderson]

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

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

On Thu, 30 Mar 2023, 03:19 Peter Maydell, <peter.maydell@linaro.org> wrote:

> In commit 049edada we added some code to handle HSTR_EL2 traps, which
> we did as an inline "conditionally branch over a
> gen_exception_insn()".  Unfortunately this fails to take account of
> the fact that gen_exception_insn() will set s->base.is_jmp to
> DISAS_NORETURN.  That means that at the end of the TB we won't
> generate the necessary code to handle the "branched over the trap and
> continued normal execution" codepath.  The result is that the TCG
> main loop thinks that we stopped execution of the TB due to a
> situation that only happens when icount is enabled, and hits an
> assertion. Explicitly set is_jmp back to DISAS_NEXT so we generate
> the correct code for when execution continues past this insn.
>
> Note that this only happens for cpreg reads; writes will call
> gen_lookup_tb() which generates a valid end-of-TB.
>
> Fixes: 049edada ("target/arm: Make HSTR_EL2 traps take priority over
> UNDEF-at-EL1")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1551
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: just unconditionally set is_jmp to DISAS_NEXT.
> ---
>  target/arm/tcg/translate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
> index 2cb9368b1ba..3c8401e9086 100644
> --- a/target/arm/tcg/translate.c
> +++ b/target/arm/tcg/translate.c
> @@ -4623,6 +4623,12 @@ static void do_coproc_insn(DisasContext *s, int
> cpnum, int is64,
>              tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, over.label);
>
>              gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
> +            /*
> +             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
> +             * but since we're conditionally branching over it, we want
> +             * to assume continue-to-next-instruction.
> +             */
> +            s->base.is_jmp = DISAS_NEXT;
>              set_disas_label(s, over);
>          }
>      }
> --
> 2.34.1
>
>

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