[Qemu-devel] [PATCH] linux-user: Handle new ARM breakpoint instruction

[Qemu-devel] [PATCH] linux-user: Handle new ARM breakpoint instruction

[Hunter Laux]

This instruction space is guaranteed to be undefined.
ARM:   xxxx 0111 1111 xxxx xxxx xxxx 1111 xxxx
Thumb: 1101 1110 xxxx xxxx

The breakpoint instructions were selected from this instruction space.
Linux traps the illegal instruction and sends a SIGTRAP if it is a breakpoint.

Here is the Linux implementation:
http://lxr.free-electrons.com/source/arch/arm/kernel/ptrace.c#L221

Signed-off-by: Hunter Laux <hunterlaux@gmail.com>
---
 linux-user/main.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 900a17f..91f2681 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -688,11 +688,29 @@ void cpu_loop(CPUARMState *env)
                 uint32_t opcode;
                 int rc;
 
+                const uint32_t arm_bkpt_mask        = 0x0FFFFFFF;
+                const uint32_t arm_bkpt             = 0x07F001F0;
+                const uint32_t arm_bkpt_thumb_mask  = 0x0000FFFF;
+                const uint32_t arm_bkpt_thumb       = 0x0000DE01;
+                const uint32_t arm_bkpt_thumb2_mask = 0xFFFFFFFF;
+                const uint32_t arm_bkpt_thumb2      = 0xF7F0A000;
+
                 /* we handle the FPU emulation here, as Linux */
                 /* we get the opcode */
                 /* FIXME - what to do if get_user() fails? */
                 get_user_code_u32(opcode, env->regs[15], env->bswap_code);
 
+                if (env->thumb) {
+                    if ((opcode & arm_bkpt_thumb_mask) == arm_bkpt_thumb
+                        || (opcode & arm_bkpt_thumb2_mask) == arm_bkpt_thumb2) {
+                        goto excp_debug;
+                    }
+                } else {
+                    if ((opcode & arm_bkpt_mask) == arm_bkpt) {
+                        goto excp_debug;
+                    }
+                }
+
                 rc = EmulateAll(opcode, &ts->fpa, env);
                 if (rc == 0) { /* illegal instruction */
                     info.si_signo = SIGILL;
-- 
1.9.1

Re: [Qemu-devel] [PATCH] linux-user: Handle new ARM breakpoint instruction

[Peter Maydell]

On 25 June 2014 05:08, Hunter Laux <hunterlaux@gmail.com> wrote:
> This instruction space is guaranteed to be undefined.
> ARM:   xxxx 0111 1111 xxxx xxxx xxxx 1111 xxxx
> Thumb: 1101 1110 xxxx xxxx
>
> The breakpoint instructions were selected from this instruction space.
> Linux traps the illegal instruction and sends a SIGTRAP if it is a breakpoint.
>
> Here is the Linux implementation:
> http://lxr.free-electrons.com/source/arch/arm/kernel/ptrace.c#L221
>
> Signed-off-by: Hunter Laux <hunterlaux@gmail.com>
> ---
>  linux-user/main.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 900a17f..91f2681 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -688,11 +688,29 @@ void cpu_loop(CPUARMState *env)
>                  uint32_t opcode;
>                  int rc;
>
> +                const uint32_t arm_bkpt_mask        = 0x0FFFFFFF;
> +                const uint32_t arm_bkpt             = 0x07F001F0;
> +                const uint32_t arm_bkpt_thumb_mask  = 0x0000FFFF;
> +                const uint32_t arm_bkpt_thumb       = 0x0000DE01;
> +                const uint32_t arm_bkpt_thumb2_mask = 0xFFFFFFFF;
> +                const uint32_t arm_bkpt_thumb2      = 0xF7F0A000;
> +
>                  /* we handle the FPU emulation here, as Linux */
>                  /* we get the opcode */
>                  /* FIXME - what to do if get_user() fails? */
>                  get_user_code_u32(opcode, env->regs[15], env->bswap_code);
>
> +                if (env->thumb) {
> +                    if ((opcode & arm_bkpt_thumb_mask) == arm_bkpt_thumb
> +                        || (opcode & arm_bkpt_thumb2_mask) == arm_bkpt_thumb2) {
> +                        goto excp_debug;
> +                    }
> +                } else {
> +                    if ((opcode & arm_bkpt_mask) == arm_bkpt) {
> +                        goto excp_debug;
> +                    }
> +                }
> +
>                  rc = EmulateAll(opcode, &ts->fpa, env);
>                  if (rc == 0) { /* illegal instruction */
>                      info.si_signo = SIGILL;

This shouldn't be necessary, because our instruction decoder
causes the BKPT instructions to generate an EXCP_BKPT
(see target-arm/translate.c). So we should never go through
this code path for these instructions...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Handle new ARM breakpoint instruction

[Hunter Laux]

[-- Attachment #1: Type: text/plain, Size: 3175 bytes --]

Isn't the instruction decoder really the wrong place to do that since that
exception is handled differently in user space and kernel space. It's my
understanding that the instruction decoder is also shared by machine
emulation. If you're doing machine emulation you'll still want to throw
EXCP_UDEF because that's what Linux is expecting.

http://lxr.free-electrons.com/source/arch/avr32/kernel/traps.c#L212

It fixed by signal 4 problem during my SBCL build, but there might be a
better way to fix this. I think they use every trick in the book to get
that thing working.

-Hunter Laux
On Jun 25, 2014 2:18 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote:

> On 25 June 2014 05:08, Hunter Laux <hunterlaux@gmail.com> wrote:
> > This instruction space is guaranteed to be undefined.
> > ARM:   xxxx 0111 1111 xxxx xxxx xxxx 1111 xxxx
> > Thumb: 1101 1110 xxxx xxxx
> >
> > The breakpoint instructions were selected from this instruction space.
> > Linux traps the illegal instruction and sends a SIGTRAP if it is a
> breakpoint.
> >
> > Here is the Linux implementation:
> > http://lxr.free-electrons.com/source/arch/arm/kernel/ptrace.c#L221
> >
> > Signed-off-by: Hunter Laux <hunterlaux@gmail.com>
> > ---
> >  linux-user/main.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 900a17f..91f2681 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -688,11 +688,29 @@ void cpu_loop(CPUARMState *env)
> >                  uint32_t opcode;
> >                  int rc;
> >
> > +                const uint32_t arm_bkpt_mask        = 0x0FFFFFFF;
> > +                const uint32_t arm_bkpt             = 0x07F001F0;
> > +                const uint32_t arm_bkpt_thumb_mask  = 0x0000FFFF;
> > +                const uint32_t arm_bkpt_thumb       = 0x0000DE01;
> > +                const uint32_t arm_bkpt_thumb2_mask = 0xFFFFFFFF;
> > +                const uint32_t arm_bkpt_thumb2      = 0xF7F0A000;
> > +
> >                  /* we handle the FPU emulation here, as Linux */
> >                  /* we get the opcode */
> >                  /* FIXME - what to do if get_user() fails? */
> >                  get_user_code_u32(opcode, env->regs[15],
> env->bswap_code);
> >
> > +                if (env->thumb) {
> > +                    if ((opcode & arm_bkpt_thumb_mask) == arm_bkpt_thumb
> > +                        || (opcode & arm_bkpt_thumb2_mask) ==
> arm_bkpt_thumb2) {
> > +                        goto excp_debug;
> > +                    }
> > +                } else {
> > +                    if ((opcode & arm_bkpt_mask) == arm_bkpt) {
> > +                        goto excp_debug;
> > +                    }
> > +                }
> > +
> >                  rc = EmulateAll(opcode, &ts->fpa, env);
> >                  if (rc == 0) { /* illegal instruction */
> >                      info.si_signo = SIGILL;
>
> This shouldn't be necessary, because our instruction decoder
> causes the BKPT instructions to generate an EXCP_BKPT
> (see target-arm/translate.c). So we should never go through
> this code path for these instructions...
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 4500 bytes --]

Re: [Qemu-devel] [PATCH] linux-user: Handle new ARM breakpoint instruction

[Peter Maydell]

On 25 June 2014 17:11, Hunter Laux <hunterlaux@gmail.com> wrote:
> Isn't the instruction decoder really the wrong place to do that since that
> exception is handled differently in user space and kernel space. It's my
> understanding that the instruction decoder is also shared by machine
> emulation. If you're doing machine emulation you'll still want to throw
> EXCP_UDEF because that's what Linux is expecting.

Actually the BKPT instruction generates a Prefetch Abort, not
an Undefined Instruction.

They really are different in some senses because if your kernel
is 64 bit then they generate different syndrome register values.
If your kernel is 32 bit then we will eventually generate a Prefetch
Abort from EXCP_BKPT. The key point here is that the EXCP_*
values are for the benefit of QEMU; they are not architectural,
they're just ways we use to provide information about what
happened; the code which deals with them will then use that
to do the right thing, which may involve generating one of the
architecturally defined exceptions. (Another example here is
EXCP_STREX, which is purely a way to say "hey, the linux-user
main loop needs to do some work for us now".)

In fact looking at the kernel code I was confused by your
subject line. These:
ARM:   xxxx 0111 1111 xxxx xxxx xxxx 1111 xxxx
Thumb: 1101 1110 xxxx xxxx

are not the "ARM breakpoint instruction" (which would be BKPT).
They're just some random UNDEF patterns the kernel has
decided to treat as causing SIGTRAP. So your code is OK
but your commit message could be clearer :-)

I think also I would factor the check out into its own function:

/* Return true if the instruction matches one of the patterns
 * in the "permanently undefined" encoding space which
 * Linux treats as if they were breakpoint instructions.
 */
static bool is_kernel_bkpt(CPUARMState *env, uint32_t opcode)
{
    ...
}

Then the main cpu_loop() calling code is a little simpler.

thanks
-- PMM