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

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

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.

[...]

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

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?
| 
| * What context(s) does this code execute in?
|   - Are debug exceptions always masked?
|   - Do we disable breakpoints/watchpoints around (some) manipulation of
|     the relevant registers?

... and the question form the earlier comment, i.e.

| Is the existing code correct?

I suspect that the existing code might not have the necessary mutual
exclusion in all cases, but it's difficult to trigger an issue by
accident. Is there any way a handler could race with some other
manipulation of watchpoints/breakpoints such that some data structure
gets corrupted, or such that the kernel deadlocks?

Mark.