Re: [Qemu-devel] [PATCH for-4.1?] target/arm: Deliver BKPT/BRK exceptions to correct exception level

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/30/19 3:25 PM, Peter Maydell wrote:
> Most Arm architectural debug exceptions (eg watchpoints) are ignored
> if the configured "debug exception level" is below the current
> exception level (so for example EL1 can't arrange to get debug exceptions
> for EL2 execution). Exceptions generated by the BRK or BPKT instructions
> are a special case -- they must always cause an exception, so if
> we're executing above the debug exception level then we
> must take them to the current exception level.
> 
> This fixes a bug where executing BRK at EL2 could result in an
> exception being taken at EL1 (which is strictly forbidden by the
> architecture).
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838277
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> At this point in the release cycle I'm not sure we should put this
> into 4.1 -- it is definitely a bug but it's not a regression as
> we've been wrong like this for multiple releases, pretty much
> since we put in the debug handling code I suspect.

The fix is quite trivial, and the user reported using a release, so
having it in the next release would be nice.
Or as usual, wait for 'last-minute-bugfix-that-postpone-another-rc' and
squeeze this fix in.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>  target/arm/op_helper.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 1ab91f915e4..5e1625a1c8a 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -370,6 +370,9 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
>   */
>  void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
>  {
> +    int debug_el = arm_debug_target_el(env);
> +    int cur_el = arm_current_el(env);
> +
>      /* FSR will only be used if the debug target EL is AArch32. */
>      env->exception.fsr = arm_debug_exception_fsr(env);
>      /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
> @@ -377,7 +380,18 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
>       * exception/security level.
>       */
>      env->exception.vaddress = 0;
> -    raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
> +    /*
> +     * Other kinds of architectural debug exception are ignored if
> +     * they target an exception level below the current one (in QEMU
> +     * this is checked by arm_generate_debug_exceptions()). Breakpoint
> +     * instructions are special because they always generate an exception
> +     * to somewhere: if they can't go to the configured debug exception
> +     * level they are taken to the current exception level.
> +     */
> +    if (debug_el < cur_el) {
> +        debug_el = cur_el;
> +    }
> +    raise_exception(env, EXCP_BKPT, syndrome, debug_el);
>  }
>  
>  uint32_t HELPER(cpsr_read)(CPUARMState *env)
>