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~