Re: [PATCH v4 6/6] arm64: hw_breakpoint: Enable FEAT_Debugv8p9

[Will Deacon]

On Tue, Apr 07, 2026 at 09:29:48AM -0500, Rob Herring (Arm) wrote:
> From: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> 
> 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. These breakpoints and watchpoints can be extended further up to
> 64 via a new arch feature FEAT_Debugv8p9.
> 
> Checking for FEAT_Debugv8p9 alone is not enough to enable the support.
> It is also necessary to determine if there are more than 16 breakpoints
> or watchpoints. The behavior with FEAT_Debugv8p9 and <=16 breakpoints
> and watchpoints is IMPDEF.
> 
> The addition of the MDSELR_EL1 to set the bank index makes the register
> accesses non-atomic. However, the combination of all the breakpoint code
> being in the kprobe blacklist and breakpoint install/uninstall being
> protected by perf locking (IRQs disabled and context lock) will prevent
> debug exceptions during accesses and serialize the accesses.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> Signed-off-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
> ---
> v4:
>  - Update commit message.
>  - Configure MDSCR_EL1_EMBWE on CPU reset/hotplug instead of every time
>    breakpoints are enabled/disabled.
>  - Drop unnecessary IRQ save and restore on register accesses.
>  - Stash checking whether FEAT_Debugv8p9 is used rather than reading
>    feature register on every register access.
>  - Check that we're greater than or equal to Debug_v8p9 not just equal
>    to.
>  - Use is_debug_v8p9_enabled() in get_num_brps/get_num_wrps(). Handle
>    the case when FEAT_Debugv8p9 is present, but the number of BP/WP
>    are <16. It is IMPDEF if ID_AA64DFR1_EL1 is used in this case. It is
>    also IMPDEF if MDSELR_EL1 is accessible. TF-A doesn't enable access
>    to MDSELR_EL1 in this case.
>  - Mark register access functions nokprobe.
> ---
>  arch/arm64/include/asm/hw_breakpoint.h | 47 ++++++++++++++++++++++++++--------
>  arch/arm64/kernel/debug-monitors.c     | 16 ++++++++----
>  arch/arm64/kernel/hw_breakpoint.c      | 41 +++++++++++++++++++++++++++--
>  3 files changed, 87 insertions(+), 17 deletions(-)

[...]

> @@ -138,19 +147,37 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>  /* Determine number of BRP registers available. */
>  static inline int get_num_brps(void)
>  {
> -	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> -	return 1 +
> -		cpuid_feature_extract_unsigned_field(dfr0,
> -						ID_AA64DFR0_EL1_BRPs_SHIFT);
> +	u64 dfr0, dfr1;
> +	int brps;
> +
> +	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> +	brps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRPs_SHIFT);
> +	if (is_debug_v8p9_enabled() && brps == 15) {
> +		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
> +		brps = cpuid_feature_extract_unsigned_field_width(dfr1,
> +								  ID_AA64DFR1_EL1_BRPs_SHIFT, 8);
> +		if (!brps)
> +			return 16;
> +	}
> +	return 1 + brps;
>  }
>  
>  /* Determine number of WRP registers available. */
>  static inline int get_num_wrps(void)
>  {
> -	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> -	return 1 +
> -		cpuid_feature_extract_unsigned_field(dfr0,
> -						ID_AA64DFR0_EL1_WRPs_SHIFT);
> +	u64 dfr0, dfr1;
> +	int wrps;
> +
> +	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> +	wrps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_WRPs_SHIFT);
> +	if (is_debug_v8p9_enabled() && wrps == 15) {
> +		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
> +		wrps = cpuid_feature_extract_unsigned_field_width(dfr1,
> +								  ID_AA64DFR1_EL1_WRPs_SHIFT, 8);
> +		if (!wrps)
> +			return 16;
> +	}
> +	return 1 + wrps;
>  }

[...]

> @@ -990,6 +1024,7 @@ static int __init arch_hw_breakpoint_init(void)
>  
>  	core_num_brps = get_num_brps();
>  	core_num_wrps = get_num_wrps();
> +	has_debug_v8p9 = (core_num_brps > 16) || (core_num_wrps > 16);

nit: FEAT_Debugv8p9 is advertised by ID_AA64DFR0_EL1.DebugVer and so
this should probably be called something else (e.g. 'has_register_banks').

Have you tested this in a guest? My reading is that MDCR_EL2.EMBWE will
be zero, but the ID registers can still advertise > 16 registers and
so I don't think this will work properly because writes to MDSELR_EL1
will either be trapped or ignored. It definitely feels like the KVM
piece of the puzzle is missing here and I think that it probably has to
be in place before we expose ID_AA64DFR1_EL1.

I'm also surprised not to see any ptrace changes in this series.
Specifically:

  1. I don't think we should expose any of this to compat tasks (see
     compat_ptrace_hbp_get_resource_info()) unless the 32-bit kernel is
     going to do that.

  2. 'struct user_hwdebug_state' retains a fixed length array of 16 for
     the debug regs and so the new registers aren't accessible in the
     REGSET_HW_{BREAK,WATCH} regsets. That means GDB can't use them and
     it also means they won't be included in coredumps iirc. I'm not sure
     whether we can safely extend the structure, so we might need to add
     some new ones...

Will