Re: [RFC PATCH 02/13] x86/HWBP: Add arch_reinstall_hw_breakpoint() for atomic updates

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Mon, 1 Sep 2025 18:23:44 +0800
Jinchao Wang <wangjinchao600@gmail.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@gmail.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.

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

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


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