Re: [PATCH v2 2/2] x86/kgdb: fix hang on failed breakpoint removal
From: Florian Rommel
Date: Tue Aug 13 2024 - 11:06:02 EST
Hi Thomas,
Thanks for the feedback.
On Mon, 2024-08-12 at 23:04 +0200, Thomas Gleixner wrote:
> Either you know it or not. Speculation is reserved for CPUs.
I have now checked it, it is always the static_key patching mechanism
when I reproduce the issue (in QEMU with a varying number of CPUs), but
we can skip this statement.
>
> > {
> > if (exception == 3 && kgdb_isremovedbreak(regs->ip - 1)) {
> > + struct kgdb_bkpt *bpt;
> > + int i, error;
> > +
> > regs->ip -= 1;
> > +
> > + /*
> > + * Try to remove the spurious int3 instruction.
> > + * These int3s can result from failed breakpoint removals
> > + * in kgdb_arch_remove_breakpoint.
> > + */
> > + for (bpt = NULL, i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> > + if (kgdb_break[i].bpt_addr == regs->ip &&
> > + kgdb_break[i].state == BP_REMOVED &&
> > + (kgdb_break[i].type == BP_BREAKPOINT ||
> > + kgdb_break[i].type == BP_POKE_BREAKPOINT)) {
> > + bpt = &kgdb_break[i];
> > + break;
> > + }
> > + }
>
> Seriously? The KGBD core already walked that array in
> kgdb_isremovedbreak() just so you can walk it again here.
>
> struct kkgdb_bkpt *kgdb_get_removed_breakpoint(unsigned long addr)
> {
> struct kgdb_bkpt *bp = kgdb_break;
>
> for (int i = 0; i < KGDB_MAX_BREAKPOINTS; i++, bp++) {
> if (bp>.state == BP_REMOVED && bp->kgdb_bpt_addr == addr)
> return bp;
> }
> return NULL;
> }
>
> bool kgdb_isremovedbreak(unsigned long addr)
> {
> return !!kgdb_get_removed_breakpoint(addr);
> }
>
> bool kgdb_rewind_exception(int exception, struct pt_regs *regs)
> {
> struct kgdb_bkpt *bp;
>
> if (exception != 3)
> return false;
>
> bp = kgdb_get_removed_breakpoint(--regs->ip);
> if (!bp || !bp->type == BP_BREAKPOINT)
> return false;
>
> Hmm?
Ok, ok, looks much better. My intention was to keep the changes in the
generic debug core minimal, especially since efficiency is not
important here.. but I see, it should be done right.
Best regards,
Flo