Re: [PATCH] target/arm: Take an exception if PSTATE.IL is set

[Richard Henderson]

On 8/17/21 6:21 AM, Peter Maydell wrote:
> In v8A, the PSTATE.IL bit is set for various kinds of illegal
> exception return or mode-change attempts.  We already set PSTATE.IL
> (or its AArch32 equivalent CPSR.IL) in all those cases, but we
> weren't implementing the part of the behaviour where attempting to
> execute an instruction with PSTATE.IL takes an immediate exception
> with an appropriate syndrome value.
>
> Add a new TB flags bit tracking PSTATE.IL/CPSR.IL, and generate code
> to take an exception instead of whatever the instruction would have
> been.
>
> PSTATE.IL and CPSR.IL change only on exception entry, attempted
> exception exit, and various AArch32 mode changes via cpsr_write().
> These places generally already rebuild the hflags, so the only place
> we need an extra rebuild_hflags call is in the illegal-return
> codepath of the AArch64 exception_return helper.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Obviously correct guest code is never going to set PSTATE.IL, but
> it's pretty confusing to debug bugs in guest OSes if we just plough
> on executing code rather than taking the illegal-state exception.  We
> had a user point this bug out to us earlier this year I think
> (probably on IRC since I can't find anything about it in my email).

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

> +    if (s->pstate_il) {
> +        /*
> +         * Illegal execution state. This has priority over BTI
> +         * exceptions, but comes after instruction abort exceptions.
> +         */
> +        gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
> +                           syn_illegalstate(), default_exception_el(s));
> +    }

Missing return after exception.

> @@ -9045,6 +9045,15 @@ static void disas_arm_insn(DisasContext *s, unsigned int 
> insn)
>           return;
>       }
>   
> +    if (s->pstate_il) {
> +        /*
> +         * Illegal execution state. This has priority over BTI
> +         * exceptions, but comes after instruction abort exceptions.
> +         */
> +        gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
> +                           syn_illegalstate(), default_exception_el(s));
> +    }
...
> @@ -9576,6 +9586,15 @@ static void thumb_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>       }
>       dc->insn = insn;
>   
> +    if (dc->pstate_il) {
> +        /*
> +         * Illegal execution state. This has priority over BTI
> +         * exceptions, but comes after instruction abort exceptions.
> +         */
> +        gen_exception_insn(dc, dc->pc_curr, EXCP_UDEF,
> +                           syn_illegalstate(), default_exception_el(dc));
> +    }

Likewise.

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


r~