Re: [PATCH 21/31] Add debugger entry points for MIPS

[Maciej W. Rozycki]

On Thu, 28 Jan 2016, Jeffrey Merkey wrote:

> This patch series adds an export which can be set by system debuggers to
> direct the hard lockup and soft lockup detector to trigger a breakpoint
> exception and enter a debugger if one is active.  It is assumed that if
> someone sets this variable, then an breakpoint handler of some sort will
> be actively loaded or registered via the notify die handler chain.
> 
> This addition is extremely useful for debugging hard and soft lockups
> real time and quickly from a console debugger.

 What's the intended use case for this hook and what do you call a console 
debugger?

 I'm asking because from the debugging perspective the Linux kernel is a 
bare metal application and for such the BREAK instruction is not the usual 
choice on the MIPS target.  This instruction is normally used for userland 
debugging, it traps to the kernel and is handled there.  Furthermore there 
are a few other applications of this instruction defined as a part of the 
MIPS ABI, also handled by the kernel, determined by the breakpoint code 
embedded with the instruction word, e.g. to trap integer division errors.  
See arch/mips/include/uapi/asm/break.h for the codes defined so far.

 All this means the trap may not be appropriate for debugging the kernel 
itself as you don't want to intercept it or the system won't run 
correctly.  You'd have to hook into the kernel itself to intercept it too.

 But if you do have a working debug environment already set up around this 
arrangement, then it might be fine after all.  In that case I think using 
a non-zero breakpoint code would make sense though, as 0 is often treated 
as a random (spurious) trap (it was used by IRIX though).  Or do you 
detect it by the symbol name somehow?

 I've got a couple of further notes on the patch itself below.

> diff --git a/arch/mips/include/asm/kdebug.h b/arch/mips/include/asm/kdebug.h
> index 8e3d08e..af5999e 100644
> --- a/arch/mips/include/asm/kdebug.h
> +++ b/arch/mips/include/asm/kdebug.h
> @@ -16,4 +16,16 @@ enum die_val {
>  	DIE_UPROBE_XOL,
>  };
>  
> +
> +void arch_breakpoint(void)
> +{
> +	__asm__ __volatile__(
> +		".globl breakinst\n\t"

 Please keep formatting consistent -- the rest of code below uses `\t' as 
the separator, so use it here as well.  We don't have an established 
inline assembly formatting style, so please just keep your chosen one 
consistent.

> +		".set\tnoreorder\n\t"
> +		"nop\n"
> +		"breakinst:\tbreak\n\t"
> +		"nop\n\t"
> +		".set\treorder");
> +}
> +
>  #endif /* _ASM_MIPS_KDEBUG_H */

 Why do you need these NOPs around the breakpoint?  You also need to mark 
`breakinst' as a function symbol (`.aent' might do) or otherwise you'll 
get garbled disassembly if this is built as microMIPS code.

  Maciej