Re: [PATCH 01/31] Add hard/soft lockup debugger entry points

[Jeffrey Merkey]

On 1/28/16, Chris Metcalf <cmetcalf@xxxxxxxxxx> wrote:
> On 01/28/2016 02:46 PM, 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.
>
> I'm concerned that you are duplicating the breakpoint instructions
> for all the platforms.  Could you make kgdb.h include kdebug.h and
> just move the arch_kgdb_breakpoint() implementations in kgdb.h to
> arch_breakpoint() in kdebug.h?  Then each platform can just put an
> appropriate define in kgdb.h, e.g. "#define arch_kgdb_breakpoint
> arch_breakpoint", unless (like mips) they have a more complicated
> requirement.
>
> I'm concerned that in some cases (e.g. arm64) there is a perfectly good
> breakpoint defined in kgdb.h but you are providing a no-op in kdebug.h.
> You should probably do another check across all the architectures for
> this case.
>
> You should probably add your no-op implementation of arch_breakpoint()
> to asm-generic/kdebug.h, and then add "generic-y" lines to the Kbuild
> files for the architectures that you are creating new empty files for.
> I'm a little ambivalent about the "silent no-op" implementation, but I'm
> not really sure there's a better option.
>
> For mips, I'm pretty sure you don't want to create a global "breakinst"
> symbol every time you insert a breakpoint into code.  I think this is an
> example of where you need to have a different implementation of
> arch_breakpoint() and arch_kgdb_breakpoint(), since mips makes its
> breakpoint magical by knowing what the address used for that specific
> instruction is, and you can't do that for arch_breakpoint().
>
> As a general rule, you probably want to provide header guards in new
> headers that you create, but if you just use asm-generic instead, it
> actually won't matter for this case.
>
> I should add that I didn't do a thorough review of the patch series,
> just a quick skim of a few of the architectures.
>
> --
> Chris Metcalf, EZChip Semiconductor
> http://www.ezchip.com
>
>

todo:

* Adding header guards (if file does not exist)
* reimplemeting arch_breakpoint
* move arch_breakpoint to asm-generic/kdebug.h
* implement as a macro if possible
* study MIPS breakpoint instruction.  Ask for help is not sure.
* fix syntax for INT3 instruction for x86 to use proper gas semantics
* fix kbuild test robot error for SPARC (move to asm-generic fixes)

List so far.

Jeff