Message-Id: <20250904100231.e0bb08d069db31520a6e0435@kernel.org>
Date: Thu, 4 Sep 2025 10:02:31 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Jinchao Wang <wangjinchao600@...il.com>
Cc: akpm@...ux-foundation.org, naveen@...nel.org, davem@...emloft.net,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 02/13] x86/HWBP: Add arch_reinstall_hw_breakpoint()
 for atomic updates

On Wed, 3 Sep 2025 15:58:44 +0800
Jinchao Wang <wangjinchao600@...il.com> wrote:

> On 9/2/25 22:11, Masami Hiramatsu (Google) wrote:
> > On Mon, 1 Sep 2025 18:23:44 +0800
> > Jinchao Wang <wangjinchao600@...il.com> wrote:
> > 
> >> On 9/1/25 15:06, Masami Hiramatsu (Google) wrote:
> >>> Hi Jinchao,
> >>>
> >> Hi Masami,
> >>
> >>> On Mon, 18 Aug 2025 20:26:07 +0800
> >>> Jinchao Wang <wangjinchao600@...il.com> wrote:
> >>>
> >>>> Add arch_reinstall_hw_breakpoint() to enable atomic context modification
> >>>> of hardware breakpoint parameters without deallocating and reallocating
> >>>> the breakpoint slot.
> >>>>
> >>>> The existing arch_install_hw_breakpoint() allocates a new debug register
> >>>> slot, while arch_uninstall_hw_breakpoint() deallocates it. However, some
> >>>> use cases require modifying breakpoint parameters (address, length, type)
> >>>> atomically without losing the allocated slot, particularly when operating
> >>>> in atomic contexts where allocation might fail or be unavailable.
> >>>>
> >>>> This is particularly useful for debugging tools like kstackwatch that
> >>>> need to dynamically update breakpoint targets in atomic contexts while
> >>>> maintaining consistent hardware state.
> >>>>
> >>>
> >>> I'm also trying to find this interface for my wprobe. So the idea is good.
> >>> But this looks hacky and only for x86. I think the interface should be
> >>> more generic and do not use this arch internal function directly.
> >>>
> >>
> >> I agree with your point about the architectural dependency. I have been
> >> considering this problem not only for the hardware breakpoint
> >> reinstallation,
> >> but also for other related parts of the series, such as canary finding and
> >> stack address resolving. These parts also rely on arch-specific code.
> > 
> > Yes, even though, the hw-breakpoint is an independent feature.
> > Directly using arch_*() functions (which are expected to be used
> > internally) introduces a hidden dependency between these two
> > components and looses maintainability.
> 
> Yes, I am trying to improve this in the v3 series.
> 
> > 
> >>> It seems that the slot is allocated by "type", thus, if this reinstall
> >>> hwbp without deallocate/allocate slot, it must NOT change the type.
> >>> See __modify_bp_slot. Also, provide CONFIG_HAVE_... option for checking
> >>> whether the architecture support that interface.
> >>>
> >> Regarding the slot allocation, I would like to clarify my point. I
> >> believe the
> >> event->attr.type should not be changed when reinstalling a hardware
> >> breakpoint, as this defines the fundamental nature of the event. The type
> >> must always be PERF_TYPE_BREAKPOINT.
> >>
> >> The event->attr.bp_type, however, can be changed. For example, from a
> >> HW_BREAKPOINT_W to a HW_BREAKPOINT_RW without needing to deallocate and
> >> reallocate the slot. This is useful for future applications, even though the
> >> current use case for KStackWatch only requires HW_BREAKPOINT_W.
> > 
> > I understand your point, so it also needs another wrapper which checks
> > the type is compatible on the architecture.
> > 
> 
> I think the wrapper should handle the type by type_slot, something like[1]:
> 

Ah, that's a good idea!

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 1db2c5e24d0e..6fed9521baf2 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -752,6 +752,7 @@ modify_user_hw_breakpoint_check(struct perf_event 
> *bp, struct perf_event_attr *a
>   {
>   	struct arch_hw_breakpoint hw = { };
>   	int err;
> +	enum bp_type_idx old_type_idx, new_type_idx;
> 
>   	err = hw_breakpoint_parse(bp, attr, &hw);
>   	if (err)
> @@ -766,7 +767,9 @@ modify_user_hw_breakpoint_check(struct perf_event 
> *bp, struct perf_event_attr *a
>   			return -EINVAL;
>   	}
> 
> -	if (bp->attr.bp_type != attr->bp_type) {
> +	old_type_idx = find_slot_idx(bp->attr.bp_type);
> +	new_type_idx = find_slot_idx(attr->bp_type);
> +	if (old_type_idx != new_type_idx) {
>   		err = modify_bp_slot(bp, bp->attr.bp_type, attr->bp_type);
>   		if (err)
>   			return err;
> 
> For kernel breakpoints, we might also consider introducing a
> modify_kernel_hw_breakpoint() helper, similar to
> modify_user_hw_breakpoint(), to encapsulate the kernel-specific case.
> 
> [1]https://lore.kernel.org/all/20250903075144.3722848-3-wangjinchao600@gmail.com/

Hmm, it seems that there is *user_hw_breakpoint() and *wide_hw_breakpoint()
(maybe it means system-wide) so it is better to call modify_wide_hw_breakpoint().
(anyway it is only for kernel address space...)

Thank you!

> 
> >>
> >> By the way, I have sent an updated series.
> >> https://lore.kernel.org/all/20250828073311.1116593-1-wangjinchao600@gmail.com/
> > 
> > Yeah, OK, let me review the series. Thanks for update!
> > 
> >>
> >> Thank you again for your valuable review.
> >> -- 
> >> Best regards,
> >> Jinchao
> > 
> > 
> 
> 
> -- 
> Best regards,
> Jinchao


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists