Re: [PATCH v3] ARM, ARM64, LONGARCH, XTENSA: Delay HW BP notification to task_work()

[Ada Couprie Diaz <ada.coupriediaz@arm.com>]

Hi Sebastian,

Sorry for the long wait, finally taking a look at this !
(+Linus Walleij for the `arch/arm/` side)

On 01/10/2026 15:35, Sebastian Andrzej Siewior wrote:
> Waiman, Luis, Ada reported that HW breakpoints on ARM64 trigger
> "sleeping while atomic" warnings on PREEMPT_RT. The hardware event is
> delivered with disabled interrupts and perf intrastrucure expects
> disabled interrupts while the overflow callback is invoked.
>
> The callback then sends a SIGTRAP signal for which it acquires
> sighand_struct::siglock, a spinlock_t which becomes a sleeping lock and
> must not be acquired in atomic context.
>
> Delay the event callback until the return to userland.
> Add perf_arch_hwbp_notify(), a generic perf callback which delayes the
> actual callback invocation to task_work_add() callback. This callback
> invokes the architecture defines callback arch_hwbp_send_sig().
Typo : `[...] architecture defined [...]`
> This requires struct callback_head and the functions require
> ARCH_NEED_PERF_HW_NOTIF to be defined.
>
> This was reported against ARM64. ARM, LongARCH and Xtensa follow the
> same pattern are also converted. Xtensa is the only not supporting
> PREEMPT_RT but now we have all architectures using the same pattern.
>
> Reported-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
> Reported-by: Waiman Long <longman@redhat.com>
> Closes: https://lore.kernel.org/all/aho0eqjMESuHxECr@redhat.com/
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>
> v2…v3: https://lore.kernel.org/all/20260814085118.OPEA_Ssn@linutronix.de/
>   - Add Xtensa for completion
>   - sashiko complains and wants TWA_SIGNAL instead TWA_RESUME. His
>     argument is that a syscall will trap via get_user() and loop forever
>     instead making progress. This is wrong IMHO. ARM64 will single step
>     over the watchpoint and continue execution. The only downside is that
>     userland will get notified after the syscall completed. So my theory.
>     Using TWA_SIGNAL is worse: Assume we have a watchpoint on UADDR and
>     are in a futex() syscall. The get_user() invocation will trigger the
>     exception, the debug handler will step over and queue a signal. The
>     futex code will notice this and return ERESTARTNOINTR. A signal will
>     be sent, the syscall restarts, traps onto UADDR again, the loop
>     continues. But this should be case now, too…
>     Now that I look into arch_build_bp_info() and do actual testing I must
>     say arm64 does not support mixed breakpoints. This means there is no
>     breakpoint in kernel on a userland address. \o/

For the record, the comment on `task_work_add()` reads :
> @TWA_SIGNAL works like signals, in that the it will interrupt the targeted
> task and run the task_work, regardless of whether the task is currently
> running in the kernel or userspace.
> [...]
> @TWA_RESUME work is run only when the task exits the kernel and returns to
> user mode, or before entering guest mode.

At least on arm64, we are explicitly not preemptible while handling
hardware breakpoint/watchpoint exceptions. (See `debug_exception_enter()`
in `arch/arm64/kernel/entry-common.c`). So `TWA_RESUME` is definitely
the behaviour we want in my opinion.

>
> v1…v2: https://lore.kernel.org/all/20260713144939.FuCj9yvZ@linutronix.de/
>   - sashiko complained that a memory breakpoint might trigger several
>     times before a signal is sent if the syscall touches the memory (via
>     get_user()) more than once before returning back. This would lead to
>     list corruption in task_work_add(). To handle this, there is now a
>     variable which is set via xchg before task_work_add() and cleared
>     after the signal has been sent.
>
>   arch/arm/include/asm/hw_breakpoint.h       |  1 +
>   arch/arm/kernel/ptrace.c                   |  6 ++---
>   arch/arm64/include/asm/hw_breakpoint.h     |  1 +
>   arch/arm64/kernel/ptrace.c                 |  6 ++---
>   arch/loongarch/include/asm/hw_breakpoint.h |  1 +
>   arch/loongarch/kernel/ptrace.c             |  6 ++---
>   arch/xtensa/include/asm/hw_breakpoint.h    |  1 +
>   arch/xtensa/kernel/ptrace.c                |  6 ++---
>   include/linux/hw_breakpoint.h              |  3 +++
>   include/linux/perf_event.h                 |  4 ++++
>   kernel/events/core.c                       | 26 ++++++++++++++++++++++
>   11 files changed, 45 insertions(+), 16 deletions(-)
>
> [...]
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 915c6fd3f0845..4e0cea7f59e4c 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -215,6 +215,10 @@ struct hw_perf_event {
>   
>   	/* Last sync'ed generation of filters */
>   	unsigned long			addr_filters_gen;
> +#ifdef ARCH_NEED_PERF_HW_NOTIF
> +	struct callback_head		arch_hw_notif;
> +	int				arch_hw_notif_busy;
> +#endif
>   
>   /*
>    * hw_perf_event::state flags; used to track the PERF_EF_* state.
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 634d2ccbab82d..9b38a14880361 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -13381,6 +13381,28 @@ static void account_event(struct perf_event *event)
>   	account_pmu_sb_event(event);
>   }
>   
> +#ifdef ARCH_NEED_PERF_HW_NOTIF
> +static void perf_arch_hwbp_send_sig(struct callback_head *head)
> +{
> +	struct perf_event *bp;
> +
> +	bp = container_of(head, struct perf_event, hw.arch_hw_notif);
> +	arch_hwbp_send_sig(bp);
> +	xchg_relaxed(&bp->hw.arch_hw_notif_busy, 0);
> +	put_event(bp);
> +}
> +
> +void perf_arch_hwbp_notify(struct perf_event *bp, struct perf_sample_data *data,
> +			   struct pt_regs *regs)
> +{
> +	if (WARN_ON_ONCE(!atomic_long_inc_not_zero(&bp->refcount)))
> +		return;
> +	if (xchg_relaxed(&bp->hw.arch_hw_notif_busy, 1) ||
> +	    WARN_ON_ONCE(task_work_add(current, &bp->hw.arch_hw_notif, TWA_RESUME)))
> +		put_event(bp);
> +}
> +#endif

I find the function names a bit counter-intuitive, compared to the other
arch-specific perf functions.
Given the name `perf_arch_...`, I would have expected them to be defined
in arch code, rather than in the generic perf code.
 From what I can see, usually perf functions calling an arch-specific
function lack the `_arch_` infix of their `arch_` counterpart.

I do not know very well what we expect in perf, so I might be off-base,
but would calling them `perf_hwbp_send_sig()` and `perf_hwbp_notify()`
make sense ?


Otherwise, it looks good to me on the arm64 side !
I had a look on the arm side as well, given the debug handling architecture
is similar, and I think it is OK on there as well, though I wouldn't mind
a more experienced arm review :)

Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>

I also tested the patch with pNMI and CONFIG_PREEMPT_RT on arm64 : I can
confirm that the atomic sleep warning is gone and everything works
as expected !

Tested-by: Ada Couprie Diaz <ada.coupriediaz@arm.com> (arm64)

Thanks a lot for looking into this, combined with[0] the hardware debug
handling should be much cleaner ! :)
Kind regards,
Ada

[0]: https://lore.kernel.org/r/20260907163101.131569-1-ada.coupriediaz@arm.com