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

Florian Bezdeka <[email protected]> writes:

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

I don't see any reason to disable in-band preemption before handling the
fault entry, especially with a potential stage switch. The ordering you
used is correct.

-- 
Philippe.