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

[Rob Herring]

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