| Message ID | YgooFX2/nMFi6xhB@sig21.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm kgdb: fix breakpoint for thumb2 | expand |
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.
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
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; }
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
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);
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