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

[Mark Rutland <mark.rutland@arm.com>]

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.