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

[Rob Herring]

On Thu, Apr 2, 2026 at 5:37 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> 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.

Uh, actually you are right. I think we need the equivalent to this in
arch_build_bp_info():

    case HW_BREAKPOINT_X:
        /*
         * We don't allow kernel breakpoints in places that are not
         * acceptable for kprobes.  On non-kprobes kernels, we don't
         * allow kernel breakpoints at all.
         */
        if (attr->bp_addr >= TASK_SIZE_MAX) {
            if (within_kprobe_blacklist(attr->bp_addr))
                return -EINVAL;
        }

The 2nd sentence is enforced by within_kprobe_blacklist() returning
true for !CONFIG_KPROBES. Seems like a reasonable restriction, but it
would be a change. Unfortunately, we can't move this to non-arch code
as some arches support h/w BP without supporting kprobes.

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

If we use NOKPROBE_SYMBOL() along with nokprobe_inline (which just
forces inlining functions), then ftrace could only be used on the
entry or exit of
arch_install_hw_breakpoint()/arch_uninstall_hw_breakpoint() which I
think would be fine.

>
> > 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.

You don't believe the comment saying counter->ctx->lock is held?

> 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.

Even if possible, is it all that useful? Easier to just disable than
reason whether it would work or not IMO.


> 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.

For wp/bp_on_reg, the ordering is 'data access, h/w accesses'. I think
we just need a barrier to enforce that ordering so the data access
(and then watchpoint) don't trigger in the middle of the h/w accesses.
Any guidance on the flavor of dsb here? (And is there any guarantee
that the access is visible to the watchpoint h/w after a dsb
completes?)

Rob