Re: [PATCH V3 7/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

Mark Rutland
2026-04-02T10:37:44Z

On Tue, Mar 31, 2026 at 05:58:00PM -0500, Rob Herring wrote:
> On Mon, Dec 16, 2024 at 10:58:29AM +0000, Mark Rutland wrote:
> > On Mon, Dec 16, 2024 at 09:38:31AM +0530, Anshuman Khandual wrote:
> > > Currently there can be maximum 16 breakpoints, and 16 watchpoints available
> > > on a given platform - as detected from ID_AA64DFR0_EL1.[BRPs|WRPs] register
> > > fields. But these breakpoint, and watchpoints can be extended further up to
> > > 64 via a new arch feature FEAT_Debugv8p9.
> > >
> > > This first enables banked access for the breakpoint and watchpoint register
> > > set via MDSELR_EL1, extended exceptions via MDSCR_EL1.EMBWE and determining
> > > available breakpoints and watchpoints in the platform from ID_AA64DFR1_EL1,
> > > when FEAT_Debugv8p9 is enabled.
> >
> > [...]
>
> Well, this series has landed on my plate...

Sorry about that; thanks for taking a look!

> > > +static u64 read_wb_reg(int reg, int n)
> > > +{
> > > + unsigned long flags;
> > > + u64 val;
> > > +
> > > + if (!is_debug_v8p9_enabled())
> > > + return __read_wb_reg(reg, n);
> > > +
> > > + /*
> > > + * Bank selection in MDSELR_EL1, followed by an indexed read from
> > > + * breakpoint (or watchpoint) registers cannot be interrupted, as
> > > + * that might cause misread from the wrong targets instead. Hence
> > > + * this requires mutual exclusion.
> > > + */
> > > + local_irq_save(flags);
> > > + write_sysreg_s(SYS_FIELD_PREP(MDSELR_EL1, BANK, n / MAX_PER_BANK), SYS_MDSELR_EL1);
> > > + isb();
> > > + val = __read_wb_reg(reg, n % MAX_PER_BANK);
> > > + local_irq_restore(flags);
> > > + return val;
> > > +}
> > > NOKPROBE_SYMBOL(read_wb_reg);
> >
> > I don't believe that disabling interrupts here is sufficient. On the
> > last version I asked about the case of racing with a watchpoint handler:
> >
> > | For example, what prevents watchpoint_handler() from firing in the
> > | middle of arch_install_hw_breakpoint() or
> > | arch_uninstall_hw_breakpoint()?
> >
> > ... and disabling interrupts cannot prevent that, because
> > local_irq_{save,restore}() do not affect the behaviour of watchpoints or
> > breakpoints.
>
> I think the answer is we just need NOKPROBE_SYMBOL() annotation on
> hw_breakpoint_control() (what arch_install_hw_breakpoint() and
> arch_uninstall_hw_breakpoint() wrap).

Ok. I couldn'y spot where we prevent placing HW breakpoints on
NOKPROBE_SYMBOL() functions, but if we do enforce that, something like
that sounds ok.

I suspect we'd need to make that noinstr to also prevent ftrace
instrumentation, unless ftrace also inhibits itself for
NOKPROBE_SYMBOL() functions.

> We also need that on __read_wb_reg
> and __read_wb_reg though I would think those are folded into the calling
> functions by the compiler. Interestly, the x86 code doesn't use the
> annotation at all.

IIUC, it looks like they *can* take debug NMIs during
arch_install_hw_breakpoint() and arch_uninstall_hw_breakpoint(), which
is why they have ordering constraints for modifying the percpu 'cpu_dr7'
variable, and their actual DR7 register (which IIUC has the enable
controls for each HW breakpoint).

That said, the use of 'bp_per_reg' looks suspect given their
arch_install_hw_breakpoint() and arch_uninstall_hw_breakpoint() modify
that non-atomically.

We could consider allowing breakpoints on those functions, but I'm not
sure whether that's possible for us, and (as noted below) it might be
better to transiently disable breakpoints/watchpoints.

IIRC on x86, breakpoint exceptions are taken *after* execution of the
instruction that triggered them, so the handler doesn't have to
manipulate single-step, and can safely ignore a breakpoint exception
without the risk of getting stuck taking the breakpoint repeatedly.

> I initially thought the IRQ disabling is also still needed as IRQ
> handlers can trigger breakpoints. However, the x86 version of
> arch_install_hw_breakpoint() contains a lockdep_assert_irqs_disabled(),
> so it seems for that case interrupts are already disabled. And in debug
> exceptions, we disable interrupts. So I think the interrupt disabling
> can be dropped.

I'd expect that the core perf code disables interrupts before calling
arch_install_hw_breakpoint() or arch_uninstall_hw_breakpoint(), and this
would be necessary for perf to serialise against IPIs that manipulate
the perf_event_context.

I agree that when we actually take the breakpoint, we'll mask all
exceptions, and so it's not necessary to mask IRQs there.

So a first step is probably to add that lockdep assert.

> > Please can you try to answer the questions I asked last time, i.e.
> >
> > | What prevents a race with an exception handler? e.g.
> > |
> > | * Does the structure of the code prevent that somehow?
>
> If you can't set a breakpoint/watchpoint in NOKPROBE_SYMBOL() annotated
> code, you can't race.

As above, I agree (with caveats), but I couldn't spot where this is
enforced.

> However, there's no such annotation for data. It looks like the kernel
> policy is "don't do that" or disable all breakpoints/watchpoints.

If we have to transiently disable watchpoints/breakpoints when
manipulating the relevant HW registers, that sounds fine to me.

> > |
> > | * What context(s) does this code execute in?
> > | - Are debug exceptions always masked?
>
> No.
>
> > | - Do we disable breakpoints/watchpoints around (some) manipulation of
> > | the relevant registers?
>
> Yes, with NOKPROBE_SYMBOL().

Thanks for digging into this; much appreciated!

Mark.