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

Rob Herring
2026-03-31T22:58:04Z

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

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

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.

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

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

> |
> | * 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().

Rob