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

[Rob Herring <robh@kernel.org>]

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.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V3:
> 
> - Used SYS_FIELD_PREP() in read_wb_reg() and write_wb_reg()
> - Added MAX_PER_BANK based BUILD_BUG_ON() tests in arch_hw_breakpoint_init()
> - Dropped local variables i.e mdsel_bank and index
> - Derived bank and index from MAX_PER_BANK as required
> 
>  arch/arm64/include/asm/debug-monitors.h |  1 +
>  arch/arm64/include/asm/hw_breakpoint.h  | 47 ++++++++++++++++++------
>  arch/arm64/kernel/debug-monitors.c      | 15 +++++---
>  arch/arm64/kernel/hw_breakpoint.c       | 48 +++++++++++++++++++++++--
>  4 files changed, 95 insertions(+), 16 deletions(-)
> 

[...]

>  void enable_debug_monitors(enum dbg_active_el el)
>  {
> -	u32 mdscr, enable = 0;
> +	u64 mdscr, enable = 0;
>  
>  	WARN_ON(preemptible());
>  
> @@ -90,6 +91,9 @@ void enable_debug_monitors(enum dbg_active_el el)
>  	    this_cpu_inc_return(kde_ref_count) == 1)
>  		enable |= DBG_MDSCR_KDE;
>  
> +	if (is_debug_v8p9_enabled())
> +		enable |= DBG_MDSCR_EMBWE;

Just to record my own findings, this is in the wrong place. This will 
cause the below code to run every time, not just on the first enable or 
kernel or user mode breakpoints. it needs to be moved into the below if.

> +
>  	if (enable && debug_enabled) {
>  		mdscr = mdscr_read();
>  		mdscr |= enable;