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

Mark Rutland
2024-12-16T10:58:36Z

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.