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

[Jeff Merkey]

On 2/1/16, Maciej W. Rozycki <macro@xxxxxxxxxx> wrote:
> 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
>

You can drop this patch series, I am rethinking a better way to do this.

Jeff