Re: [PATCH 1/4] linux-user/arm: BKPT should cause SIGTRAP, not be a syscall

[Philippe Mathieu-Daudé]

On 4/20/20 11:22 PM, Peter Maydell wrote:
> In linux-user/arm/cpu-loop.c we incorrectly treat EXCP_BKPT similarly
> to EXCP_SWI, which means that if the guest executes a BKPT insn then
> QEMU will perform a syscall for it (which syscall depends on what
> value happens to be in r7...). The correct behaviour is that the
> guest process should take a SIGTRAP.
> 
> This code has been like this (more or less) since commit
> 06c949e62a098f in 2006 which added BKPT in the first place.  This is
> probably because at the time the same code path was used to handle
> both Linux syscalls and semihosting calls, and (on M profile) BKPT
> with a suitable magic number is used for semihosting calls.  But
> these days we've moved handling of semihosting out to an entirely
> different codepath, so we can fix this bug by simply removing this
> handling of EXCP_BKPT and instead making it deliver a SIGTRAP like
> EXCP_DEBUG (as we do already on aarch64).
> 
> Reported-by: <address@hidden>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1873898
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  linux-user/arm/cpu_loop.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index cf618daa1ca..82d0dd3c312 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -295,32 +295,17 @@ void cpu_loop(CPUARMState *env)
>              }
>              break;
>          case EXCP_SWI:
> -        case EXCP_BKPT:
>              {
>                  env->eabi = 1;
>                  /* system call */
> -                if (trapnr == EXCP_BKPT) {
> -                    if (env->thumb) {
> -                        /* FIXME - what to do if get_user() fails? */
> -                        get_user_code_u16(insn, env->regs[15], env);
> -                        n = insn & 0xff;
> -                        env->regs[15] += 2;
> -                    } else {
> -                        /* FIXME - what to do if get_user() fails? */
> -                        get_user_code_u32(insn, env->regs[15], env);
> -                        n = (insn & 0xf) | ((insn >> 4) & 0xff0);
> -                        env->regs[15] += 4;
> -                    }
> +                if (env->thumb) {
> +                    /* FIXME - what to do if get_user() fails? */
> +                    get_user_code_u16(insn, env->regs[15] - 2, env);
> +                    n = insn & 0xff;
>                  } else {
> -                    if (env->thumb) {
> -                        /* FIXME - what to do if get_user() fails? */
> -                        get_user_code_u16(insn, env->regs[15] - 2, env);
> -                        n = insn & 0xff;
> -                    } else {
> -                        /* FIXME - what to do if get_user() fails? */
> -                        get_user_code_u32(insn, env->regs[15] - 4, env);
> -                        n = insn & 0xffffff;
> -                    }
> +                    /* FIXME - what to do if get_user() fails? */
> +                    get_user_code_u32(insn, env->regs[15] - 4, env);
> +                    n = insn & 0xffffff;
>                  }

I couldn't find a git-diff option to display this change in an obvious way.

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>  
>                  if (n == ARM_NR_cacheflush) {
> @@ -396,6 +381,7 @@ void cpu_loop(CPUARMState *env)
>              }
>              break;
>          case EXCP_DEBUG:
> +        case EXCP_BKPT:
>          excp_debug:
>              info.si_signo = TARGET_SIGTRAP;
>              info.si_errno = 0;
>