[PATCH] arm kgdb: fix breakpoint for thumb2

[PATCH] arm kgdb: fix breakpoint for thumb2

[Johannes Stezenbach]

Entering kdb via SysRq-G with CONFIG_THUMB2_KERNEL=y
on Cortex-A7 in Qemu results in an Ooops, and it is
not possible to continue because of "Catastrophic error detected".
The root cause is using an arm breakpoint instruction in
thumb code.

For the breakpoint instruction I used this reference:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdbserver/linux-aarch32-low.cc#l31
0xf7fxaxxx are UDF encoding T2 instructions
(32bit, permanently undefined).

Signed-off-by: Johannes Stezenbach <js@sig21.net>
---

Wrt arm GDB_BREAKINST maybe it is also needed to use something like

#ifdef CONFIG_AEABI
#define GDB_BREAKINST		0xe7f001f0
#else
#define GDB_BREAKINST		0xef9f0001
#endif

but I could not test if it makes a difference

diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h
index 8de1100d1067..7e6ed3171b54 100644
--- a/arch/arm/include/asm/kgdb.h
+++ b/arch/arm/include/asm/kgdb.h
@@ -34,16 +34,22 @@
  * make our lives much much simpler. :)
  */
 #define BREAK_INSTR_SIZE	4
+#ifdef CONFIG_THUMB2_KERNEL
+#define GDB_BREAKINST		0xf7f0a000
+#define KGDB_BREAKINST		0xf7f0a001
+#define KGDB_COMPILED_BREAK	0xf7f0a002
+#else
 #define GDB_BREAKINST		0xef9f0001
 #define KGDB_BREAKINST		0xe7ffdefe
 #define KGDB_COMPILED_BREAK	0xe7ffdeff
+#endif
 #define CACHE_FLUSH_IS_SAFE	1
 
 #ifndef	__ASSEMBLY__
 
 static inline void arch_kgdb_breakpoint(void)
 {
-	asm(__inst_arm(0xe7ffdeff));
+	asm(__inst_arm_thumb32(KGDB_COMPILED_BREAK, KGDB_COMPILED_BREAK));
 }
 
 extern void kgdb_handle_bus_error(void);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm kgdb: fix breakpoint for thumb2

[Russell King (Oracle)]

On Mon, Feb 14, 2022 at 10:59:49AM +0100, Johannes Stezenbach wrote:
> Entering kdb via SysRq-G with CONFIG_THUMB2_KERNEL=y
> on Cortex-A7 in Qemu results in an Ooops, and it is
> not possible to continue because of "Catastrophic error detected".
> The root cause is using an arm breakpoint instruction in
> thumb code.

This sounds like a bug in qemu.

0xe7ffdeXX is two 16-bit instructions:

   0:   e7ff            b.n     2 <.text+0x2>
   2:   def1            udf     #241    ; 0xf1

0xe7ff is a branch to the UDF instruction.

0xdeXX is a UDF (Permanently undefined) instruction which should raise a
undefined instruction trap. As per the Arm ARM on UDF: "Permanently
Undefined generates an Undefined Instruction exception."

The encoding is also a 32-bit UDF instruction:

   0:   e7ffdef1        udf     #64993  ; 0xfde1

which is exactly why these opcodes were chosen - so we can instrument
both ARM and Thumb code without caring which it is.

Qemu needs fixing if it complains about this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm kgdb: fix breakpoint for thumb2

[Johannes Stezenbach]

On Mon, Feb 14, 2022 at 10:13:43AM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 14, 2022 at 10:59:49AM +0100, Johannes Stezenbach wrote:
> > Entering kdb via SysRq-G with CONFIG_THUMB2_KERNEL=y
> > on Cortex-A7 in Qemu results in an Ooops, and it is
> > not possible to continue because of "Catastrophic error detected".
> > The root cause is using an arm breakpoint instruction in
> > thumb code.
> 
> This sounds like a bug in qemu.
> 
> 0xe7ffdeXX is two 16-bit instructions:
> 
>    0:   e7ff            b.n     2 <.text+0x2>
>    2:   def1            udf     #241    ; 0xf1
> 
> 0xe7ff is a branch to the UDF instruction.
> 
> 0xdeXX is a UDF (Permanently undefined) instruction which should raise a
> undefined instruction trap. As per the Arm ARM on UDF: "Permanently
> Undefined generates an Undefined Instruction exception."
> 
> The encoding is also a 32-bit UDF instruction:
> 
>    0:   e7ffdef1        udf     #64993  ; 0xfde1
> 
> which is exactly why these opcodes were chosen - so we can instrument
> both ARM and Thumb code without caring which it is.
> 
> Qemu needs fixing if it complains about this.

My apologies for the incomplete problem description.

Qemu is not complaining at all and correctly raises the invalid
instruction exception. But the kgdb_compiled_brkpt_hook undef_hook
only sees the 0xdef1 part and thus does not match.


Johannes

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm kgdb: fix breakpoint for thumb2

[Russell King (Oracle)]

On Mon, Feb 14, 2022 at 12:05:56PM +0100, Johannes Stezenbach wrote:
> On Mon, Feb 14, 2022 at 10:13:43AM +0000, Russell King (Oracle) wrote:
> > On Mon, Feb 14, 2022 at 10:59:49AM +0100, Johannes Stezenbach wrote:
> > > Entering kdb via SysRq-G with CONFIG_THUMB2_KERNEL=y
> > > on Cortex-A7 in Qemu results in an Ooops, and it is
> > > not possible to continue because of "Catastrophic error detected".
> > > The root cause is using an arm breakpoint instruction in
> > > thumb code.
> > 
> > This sounds like a bug in qemu.
> > 
> > 0xe7ffdeXX is two 16-bit instructions:
> > 
> >    0:   e7ff            b.n     2 <.text+0x2>
> >    2:   def1            udf     #241    ; 0xf1
> > 
> > 0xe7ff is a branch to the UDF instruction.
> > 
> > 0xdeXX is a UDF (Permanently undefined) instruction which should raise a
> > undefined instruction trap. As per the Arm ARM on UDF: "Permanently
> > Undefined generates an Undefined Instruction exception."
> > 
> > The encoding is also a 32-bit UDF instruction:
> > 
> >    0:   e7ffdef1        udf     #64993  ; 0xfde1
> > 
> > which is exactly why these opcodes were chosen - so we can instrument
> > both ARM and Thumb code without caring which it is.
> > 
> > Qemu needs fixing if it complains about this.
> 
> My apologies for the incomplete problem description.
> 
> Qemu is not complaining at all and correctly raises the invalid
> instruction exception. But the kgdb_compiled_brkpt_hook undef_hook
> only sees the 0xdef1 part and thus does not match.

The kgdb code needs to register an undef hook for the Thumb UDF
instruction that will fault, in addition to the ARM version. Probably
something like this (untested). Please let me know if this works.

diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
index 7bd30c0a4280..a3602cacda0b 100644
--- a/arch/arm/kernel/kgdb.c
+++ b/arch/arm/kernel/kgdb.c
@@ -154,22 +154,38 @@ static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int instr)
 	return 0;
 }
 
-static struct undef_hook kgdb_brkpt_hook = {
+static struct undef_hook kgdb_brkpt_arm_hook = {
 	.instr_mask		= 0xffffffff,
 	.instr_val		= KGDB_BREAKINST,
-	.cpsr_mask		= MODE_MASK,
+	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
 	.cpsr_val		= SVC_MODE,
 	.fn			= kgdb_brk_fn
 };
 
-static struct undef_hook kgdb_compiled_brkpt_hook = {
+static struct undef_hook kgdb_brkpt_thumb_hook = {
+	.instr_mask		= 0xffff,
+	.instr_val		= KGDB_BREAKINST & 0xffff,
+	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
+	.cpsr_val		= PSR_T_BIT | SVC_MODE,
+	.fn			= kgdb_brk_fn
+};
+
+static struct undef_hook kgdb_compiled_brkpt_arm_hook = {
 	.instr_mask		= 0xffffffff,
 	.instr_val		= KGDB_COMPILED_BREAK,
-	.cpsr_mask		= MODE_MASK,
+	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
 	.cpsr_val		= SVC_MODE,
 	.fn			= kgdb_compiled_brk_fn
 };
 
+static struct undef_hook kgdb_compiled_brkpt_thumb_hook = {
+	.instr_mask		= 0xffff,
+	.instr_val		= KGDB_COMPILED_BREAK & 0xffff,
+	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
+	.cpsr_val		= PSR_T_BIT | SVC_MODE,
+	.fn			= kgdb_compiled_brk_fn
+};
+
 static int __kgdb_notify(struct die_args *args, unsigned long cmd)
 {
 	struct pt_regs *regs = args->regs;
@@ -210,8 +226,10 @@ int kgdb_arch_init(void)
 	if (ret != 0)
 		return ret;
 
-	register_undef_hook(&kgdb_brkpt_hook);
-	register_undef_hook(&kgdb_compiled_brkpt_hook);
+	register_undef_hook(&kgdb_brkpt_arm_hook);
+	register_undef_hook(&kgdb_brkpt_thumb_hook);
+	register_undef_hook(&kgdb_compiled_brkpt_arm_hook);
+	register_undef_hook(&kgdb_compiled_brkpt_thumb_hook);
 
 	return 0;
 }

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm kgdb: fix breakpoint for thumb2

[Johannes Stezenbach]

On Mon, Feb 14, 2022 at 12:56:33PM +0000, Russell King (Oracle) wrote:
> The kgdb code needs to register an undef hook for the Thumb UDF
> instruction that will fault, in addition to the ARM version. Probably
> something like this (untested). Please let me know if this works.
>
> diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
> index 7bd30c0a4280..a3602cacda0b 100644
> --- a/arch/arm/kernel/kgdb.c
> +++ b/arch/arm/kernel/kgdb.c
> @@ -154,22 +154,38 @@ static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int instr)
>  	return 0;
>  }
>  
> -static struct undef_hook kgdb_brkpt_hook = {
> +static struct undef_hook kgdb_brkpt_arm_hook = {
>  	.instr_mask		= 0xffffffff,
>  	.instr_val		= KGDB_BREAKINST,
> -	.cpsr_mask		= MODE_MASK,
> +	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
>  	.cpsr_val		= SVC_MODE,
>  	.fn			= kgdb_brk_fn
>  };
>  
> -static struct undef_hook kgdb_compiled_brkpt_hook = {
> +static struct undef_hook kgdb_brkpt_thumb_hook = {
> +	.instr_mask		= 0xffff,
> +	.instr_val		= KGDB_BREAKINST & 0xffff,
> +	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
> +	.cpsr_val		= PSR_T_BIT | SVC_MODE,
> +	.fn			= kgdb_brk_fn
> +};
> +
> +static struct undef_hook kgdb_compiled_brkpt_arm_hook = {
>  	.instr_mask		= 0xffffffff,
>  	.instr_val		= KGDB_COMPILED_BREAK,
> -	.cpsr_mask		= MODE_MASK,
> +	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
>  	.cpsr_val		= SVC_MODE,
>  	.fn			= kgdb_compiled_brk_fn
>  };
>  
> +static struct undef_hook kgdb_compiled_brkpt_thumb_hook = {
> +	.instr_mask		= 0xffff,
> +	.instr_val		= KGDB_COMPILED_BREAK & 0xffff,
> +	.cpsr_mask		= PSR_T_BIT | MODE_MASK,
> +	.cpsr_val		= PSR_T_BIT | SVC_MODE,
> +	.fn			= kgdb_compiled_brk_fn
> +};
> +
>  static int __kgdb_notify(struct die_args *args, unsigned long cmd)
>  {
>  	struct pt_regs *regs = args->regs;
> @@ -210,8 +226,10 @@ int kgdb_arch_init(void)
>  	if (ret != 0)
>  		return ret;
>  
> -	register_undef_hook(&kgdb_brkpt_hook);
> -	register_undef_hook(&kgdb_compiled_brkpt_hook);
> +	register_undef_hook(&kgdb_brkpt_arm_hook);
> +	register_undef_hook(&kgdb_brkpt_thumb_hook);
> +	register_undef_hook(&kgdb_compiled_brkpt_arm_hook);
> +	register_undef_hook(&kgdb_compiled_brkpt_thumb_hook);
>  
>  	return 0;
>  }

It has compile errors, but with those fixed up your patch works nicely.

arch/arm/kernel/kgdb.c: In function ‘kgdb_arch_exit’:
arch/arm/kernel/kgdb.c:245:25: error: ‘kgdb_brkpt_hook’ undeclared (first use in this function); did you mean ‘kgdb_brkpt_arm_hook’?
  245 |  unregister_undef_hook(&kgdb_brkpt_hook);
      |                         ^~~~~~~~~~~~~~~
      |                         kgdb_brkpt_arm_hook
arch/arm/kernel/kgdb.c:245:25: note: each undeclared identifier is reported only once for each function it appears in
arch/arm/kernel/kgdb.c:246:25: error: ‘kgdb_compiled_brkpt_hook’ undeclared (first use in this function); did you mean ‘
kgdb_compiled_brkpt_arm_hook’?
  246 |  unregister_undef_hook(&kgdb_compiled_brkpt_hook);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~
      |                         kgdb_compiled_brkpt_arm_hook


Thanks,
Johannes

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel