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

[Jinchao Wang <wangjinchao600@gmail.com>]

On 9/3/25 15:58, Jinchao Wang wrote:
> On 9/2/25 22:11, Masami Hiramatsu (Google) wrote:
>> 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.
> 
> 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]:
> ...
> 
> 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.
> 

Hi Masami,
I have a new idea, indroducing hw_breakpoint_modify_local():
   - call hw_breakpoint_arch_parse to validate bp_{type,addr,len},
     eliminating the need for a separate type-only check
   - use a__weak arch_reinstall_hw_breakpoint for other archs

If other archs require additional checks, they could add that logic to 
hw_breakpoint_modify_local or their hw_breakpoint_arch_parse().

Seems clear and simple enough.

Regarding the arch dependency problems you mentioned, I have solved them 
and sent a v2 series:
https://lore.kernel.org/all/20250904002126.1514566-7-wangjinchao600@gmail.com/

>>
>>>
>>> Thank you again for your valuable review.
>>> -- 
>>> Best regards,
>>> Jinchao
>>
>>
> 
> 


-- 
Best regards,
Jinchao