hw_breakpoint.c -- a bad idea

[Jeffrey Merkey]

Allowing other subsystems to clobber the DR7 register is a bad idea
when debuggers are loaded.  I realize that this piece of code was
written for perf monitors, and poor KGDB and KDB have to use this
interface to set and clear hardware breakpoints.  Problem with this
architecture is that is you are using complex chaining of conditional
breakpoints in a debugger monitoring cross processor events, this code
has no way of knowing the state of subsystems using the breakpoints
while it blindly clears the DR7 register during a breakpoint
exception.   It needs to be either disabled or the ability of a
subsystem debugger to flag it as "owned" for a specific period of
time.

I realize that the way its coded is designed to share the DR
registers, but if a debugger is active, this code is completely
disabled anyway, so having it is pointless except for the perf
functions when no debugger is active.  I am debating adapting MDB to
use this interface rather than manage breakpoint registers itself, but
I don't see the point in dancing around this code and trying to stand
on my head and implement something that removes features and
capability from the debugger.  MDB supports complex SMP conditional
breakpoints, unlike KDB or KGDB.

This interface should be restructured to allow it to be completely
disabled if necessary by the debugger or owned and a snipet of code in
an event handler should not be blindly clobbering DR7 without knowing
the state of debuggers above it.  I can move the functionality into
this section but is kind of defeats the purpose of having a self
contained debugger.  A debugger should have minimal dependencies on OS
code and be as much a self contained bubble as possible.

At any rate I disable it whem MDB is loaded.  I will investigate a
batter way to do this to allow perf code to run as well.

Jeff
static int hw_breakpoint_handler(struct die_args *args)
{
        int i, cpu, rc = NOTIFY_STOP;
        struct perf_event *bp;
        unsigned long dr7, dr6;
        unsigned long *dr6_p;

        /* The DR6 value is pointed by args->err */
        dr6_p = (unsigned long *)ERR_PTR(args->err);
        dr6 = *dr6_p;

        /* If it's a single step, TRAP bits are random */
        if (dr6 & DR_STEP)
                return NOTIFY_DONE;

        /* Do an early return if no trap bits are set in DR6 */
        if ((dr6 & DR_TRAP_BITS) == 0)
                return NOTIFY_DONE;

        // if MDB is loaded turn off
#if defined(CONFIG_MDB) || defined(CONFIG_MDB_MODULE)
        if (disable_hw_bp_interface)
                return NOTIFY_DONE;
#endif

        get_debugreg(dr7, 7);
        /* Disable breakpoints during exception handling */

        set_debugreg(0UL, 7);   //   <---  fucking broken, breaks
proceed, conditional, and temp breakpoints in MDB

        /*
         * Assert that local interrupts are disabled
         * Reset the DRn bits in the virtualized register value.
         * The ptrace trigger routine will add in whatever is needed.
         */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/