Re: [PATCH Dovetail v3 3/6] arm: irq_pipeline: Fix dovetail_fault_{entry,exit} bypass in hw_breakpoint_pending

On Mon, 2026-06-22 at 10:05 +0200, Florian Bezdeka wrote:
> HW breakpoint / watchpoint handling was bypassing the
> dovetail_fault_{entry,exit} machinery. As a result it could happen that
> the inband IRQ mask was touched from the OOB stage.
> 
> There is one more problem in the HW bp/wp handling related to
> interrupts_enabled(). This one will be fixed in a separate patch.
> 
> Signed-off-by: Florian Bezdeka <[email protected]>
> ---
>  arch/arm/include/asm/dovetail.h         | 1 +
>  arch/arm/include/asm/trace/exceptions.h | 3 ++-
>  arch/arm/kernel/hw_breakpoint.c         | 6 ++++++
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/dovetail.h b/arch/arm/include/asm/dovetail.h
> index 795a1fb903ec29f4c2fb7b25e70980796804acc0..eeb5d16ad032044a2587822f6b35074d664a9bbc 100644
> --- a/arch/arm/include/asm/dovetail.h
> +++ b/arch/arm/include/asm/dovetail.h
> @@ -16,6 +16,7 @@
>  #define ARM_TRAP_VFP		6	/* VFP floating point exception */
>  #define ARM_TRAP_UNDEFINSTR	7	/* Undefined instruction */
>  #define ARM_TRAP_ALIGNMENT	8	/* Unaligned access exception */
> +#define ARM_TRAP_HW_BREAK	9	/* HW break or watchpoint exception */
>  
>  #if !defined(__ASSEMBLY__)
>  
> diff --git a/arch/arm/include/asm/trace/exceptions.h b/arch/arm/include/asm/trace/exceptions.h
> index bdb666b3da4e364e0d7e33337be3e8ad8aeedb88..f0164e55efc31a4f363764d2c5c84761f3ea34d4 100644
> --- a/arch/arm/include/asm/trace/exceptions.h
> +++ b/arch/arm/include/asm/trace/exceptions.h
> @@ -21,7 +21,8 @@
>  			__trace_trap(ARM_TRAP_FPU),		\
>  			__trace_trap(ARM_TRAP_VFP),		\
>  			__trace_trap(ARM_TRAP_UNDEFINSTR),	\
> -			__trace_trap(ARM_TRAP_ALIGNMENT))
> +			__trace_trap(ARM_TRAP_ALIGNMENT),	\
> +			__trace_trap(ARM_TRAP_HW_BREAK))
>  
>  DECLARE_EVENT_CLASS(ARM_trap_event,
>  	TP_PROTO(int trapnr, struct pt_regs *regs),
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index cd4b34c96e35e9e63e9a1ade1aeb415c22d32b00..6266380737dd88dca7fe8fde14b786a52ec42635 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -26,6 +26,7 @@
>  #include <asm/current.h>
>  #include <asm/hw_breakpoint.h>
>  #include <asm/traps.h>
> +#include <asm/trap_entry.h>
>  
>  /* Breakpoint currently in use for each BRP. */
>  static DEFINE_PER_CPU(struct perf_event *, bp_on_reg[ARM_MAX_BRP]);
> @@ -942,9 +943,12 @@ static void hw_breakpoint_cfi_handler(struct pt_regs *regs)
>  static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
>  				 struct pt_regs *regs)
>  {
> +	unsigned long irqflags;
>  	int ret = 0;
>  	u32 dscr;
>  
> +	irqflags = dovetail_fault_entry(ARM_TRAP_HW_BREAK, regs);
> +
>  	preempt_disable();

Philippe, is that the right ordering or should I move the
dovetail_fault_entry() code below the preempt_disable() call?

Normally we want preemption enabled as long as possible and I couldn't
find a reason why the oob notify mechanism needs it already disabled,
but as always I might have missed something.

>  
>  	if (interrupts_enabled(regs))
> @@ -973,6 +977,8 @@ static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
>  
>  	preempt_enable();
>  
> +	dovetail_fault_exit(ARM_TRAP_HW_BREAK, regs, irqflags);
> +
>  	return ret;
>  }
>  
> 
> -- 
> 2.54.0