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

[PATCH 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.

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>
---
saving and restoring is_jmp around the call seems super
clunky -- is there a better way ? I think mostly we avoid
this by not doing conditional exception-generation in
inline TCG code...
---
 target/arm/tcg/translate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 2cb9368b1ba..bb502165c39 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -4617,12 +4617,20 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
             /* T4 and T14 are RES0 so never cause traps */
             TCGv_i32 t;
             DisasLabel over = gen_disas_label(s);
+            DisasJumpType old_is_jmp;
 
             t = load_cpu_offset(offsetoflow32(CPUARMState, cp15.hstr_el2));
             tcg_gen_andi_i32(t, t, 1u << maskbit);
             tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, over.label);
 
+            /*
+             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
+             * but since we're conditionally branching over it, we want
+             * to retain the existing value.
+             */
+            old_is_jmp = s->base.is_jmp;
             gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
+            s->base.is_jmp = old_is_jmp;
             set_disas_label(s, over);
         }
     }
-- 
2.34.1

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

[Richard Henderson]

On 3/28/23 09:28, Peter Maydell 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.
> 
> 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>
> ---
> saving and restoring is_jmp around the call seems super
> clunky -- is there a better way ? I think mostly we avoid
> this by not doing conditional exception-generation in
> inline TCG code...
> ---
>   target/arm/tcg/translate.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

You could also do

     /* Branch over conditional exception: continue. */
     if (s->base.is_jmp == DISAS_NORETURN) {
         s->base.is_jmp = DISAS_NEXT;
     }

within set_disas_label.  Any other is_jmp value will be preserved to exit the TB "normally".

This is similar to hppa nullify_end().  For a moment I thought we already had something 
similar for arm conditional illegal insns, but I see those handled via code at the end of 
arm_tr_tb_stop.


r~

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

[Richard Henderson]

On 3/28/23 09:28, Peter Maydell wrote:
> +            /*
> +             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
> +             * but since we're conditionally branching over it, we want
> +             * to retain the existing value.
> +             */
> +            old_is_jmp = s->base.is_jmp;
>               gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
> +            s->base.is_jmp = old_is_jmp;

A third solution is to simply set is_jmp = DISAS_NEXT here.


r~

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

[Peter Maydell]

On Tue, 28 Mar 2023 at 18:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/28/23 09:28, Peter Maydell wrote:
> > +            /*
> > +             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
> > +             * but since we're conditionally branching over it, we want
> > +             * to retain the existing value.
> > +             */
> > +            old_is_jmp = s->base.is_jmp;
> >               gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
> > +            s->base.is_jmp = old_is_jmp;
>
> A third solution is to simply set is_jmp = DISAS_NEXT here.

I wasn't confident enough that the previous is_jmp had
to be DISAS_NEXT to do that -- there are a lot of
different values and it's not clear to me which are ones you
might find lying around in is_jmp at the start of an insn.

I like the set_disas_label() idea, but maybe for 8.1 at this
point...

thanks
-- PMM

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

[Richard Henderson]

On 3/28/23 11:27, Peter Maydell wrote:
> On Tue, 28 Mar 2023 at 18:27, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 3/28/23 09:28, Peter Maydell wrote:
>>> +            /*
>>> +             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
>>> +             * but since we're conditionally branching over it, we want
>>> +             * to retain the existing value.
>>> +             */
>>> +            old_is_jmp = s->base.is_jmp;
>>>                gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
>>> +            s->base.is_jmp = old_is_jmp;
>>
>> A third solution is to simply set is_jmp = DISAS_NEXT here.
> 
> I wasn't confident enough that the previous is_jmp had
> to be DISAS_NEXT to do that -- there are a lot of
> different values and it's not clear to me which are ones you
> might find lying around in is_jmp at the start of an insn.

At the very start of an insn, you will *only* find DISAS_NEXT.
Anything else will have exited the TB for the previous insn.


r~

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

[Richard Henderson]

On 3/28/23 11:27, Peter Maydell wrote:
> On Tue, 28 Mar 2023 at 18:27, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 3/28/23 09:28, Peter Maydell wrote:
>>> +            /*
>>> +             * gen_exception_insn() will set is_jmp to DISAS_NORETURN,
>>> +             * but since we're conditionally branching over it, we want
>>> +             * to retain the existing value.
>>> +             */
>>> +            old_is_jmp = s->base.is_jmp;
>>>                gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
>>> +            s->base.is_jmp = old_is_jmp;
>>
>> A third solution is to simply set is_jmp = DISAS_NEXT here.
> 
> I wasn't confident enough that the previous is_jmp had
> to be DISAS_NEXT to do that -- there are a lot of
> different values and it's not clear to me which are ones you
> might find lying around in is_jmp at the start of an insn.
> 
> I like the set_disas_label() idea, but maybe for 8.1 at this
> point...

Anyway, if you'd like to stay with your current patch for 8.0, it's not wrong, so

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


r~