Re: [PATCH v3 3/3] kernel: debug: Centralize dbg_[de]activate_sw_breakpoints

[Daniel Thompson]

On Mon, Sep 14, 2020 at 05:13:28PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Sep 14, 2020 at 6:02 AM Daniel Thompson
> <daniel.thompson@xxxxxxxxxx> wrote:
> >
> > During debug trap execution we expect dbg_deactivate_sw_breakpoints()
> > to be paired with an dbg_activate_sw_breakpoint(). Currently although
> > the calls are paired correctly they are needlessly smeared across three
> > different functions. Worse this also results in code to drive polled I/O
> > being called with breakpoints activated which, in turn, needlessly
> > increases the set of functions that will recursively trap if breakpointed.
> >
> > Fix this by moving the activation of breakpoints into the debug core.
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> > ---
> >  kernel/debug/debug_core.c       | 2 ++
> >  kernel/debug/gdbstub.c          | 1 -
> >  kernel/debug/kdb/kdb_debugger.c | 2 --
> >  3 files changed, 2 insertions(+), 3 deletions(-)
> 
> I like the idea, but previously the kgdb_arch_handle_exception() was
> always called after the SW breakpoints were activated.  Are you sure
> it's OK to swap those two orders across all architectures?

Pretty sure, yes.

However, given the poor attention to detail I demonstrated in patch 2/3,
I figure I'd better write out the full chain of reasoning if I want
you to trust me ;-) .

kgdb_arch_handle_exception() is already called frequently with
breakpoints disabled since it is basically a fallback that is called
whenever the architecture neutral parts of the gdb packet processing
cannot cope.

So your question becomes more specific: is it OK to swap orders when the
architecture code is handling a (c)ontinue or (s)tep packet[1]?

The reason the architecture neutral part cannot cope is because it
because *if* gdb has been instructed that the PC must be changed before
resuming then the architecture neutral code does not know how to effect
this. In other words the call to kgdb_arch_handle_exception() will
boil down to doing whatever the architecture equivalent of writing to
linux_regs->pc actually is (or return an error).

Updating the PC is safe whether or not breakpoints are deactivated
and activating the breakpoints if the architecture code actually
censors the resume is a bug (which we just fixed).


Daniel.


[1]
   The fallthroughs aren't a whole lot of fun to read but
   if gdb_cmd_exception_pass() provokes a fallthrough then it will
   have rewritten the packet as a (c)ontinue.