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
