Re: [PATCH v2 05/18] x86/hw_breakpoint: introduce arch_reinstall_hw_breakpoint() for atomic context

[Jinchao Wang <wangjinchao600@gmail.com>]

On 9/9/25 16:00, Masami Hiramatsu (Google) wrote:
> On Mon, 8 Sep 2025 13:20:51 +0800
> Jinchao Wang <wangjinchao600@gmail.com> wrote:
>
>> On Thu, Sep 04, 2025 at 08:38:32AM +0200, Peter Zijlstra wrote:
>>> On Thu, Sep 04, 2025 at 08:21:02AM +0800, Jinchao Wang wrote:
>>>> Introduce arch_reinstall_hw_breakpoint() to update hardware breakpoint
>>>> parameters (address, length, type) without freeing and reallocating the
>>>> debug register slot.
>>>>
>>>> This allows atomic updates in contexts where memory allocation is not
>>>> permitted, such as kprobe handlers.
>>>>
>>>> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
>>>> ---
>>>>   arch/x86/include/asm/hw_breakpoint.h |  1 +
>>>>   arch/x86/kernel/hw_breakpoint.c      | 50 ++++++++++++++++++++++++++++
>>>>   2 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
>>>> index 0bc931cd0698..bb7c70ad22fe 100644
>>>> --- a/arch/x86/include/asm/hw_breakpoint.h
>>>> +++ b/arch/x86/include/asm/hw_breakpoint.h
>>>> @@ -59,6 +59,7 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
>>>>   
>>>>   
>>>>   int arch_install_hw_breakpoint(struct perf_event *bp);
>>>> +int arch_reinstall_hw_breakpoint(struct perf_event *bp);
>>>>   void arch_uninstall_hw_breakpoint(struct perf_event *bp);
>>>>   void hw_breakpoint_pmu_read(struct perf_event *bp);
>>>>   void hw_breakpoint_pmu_unthrottle(struct perf_event *bp);
>>>> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
>>>> index b01644c949b2..89135229ed21 100644
>>>> --- a/arch/x86/kernel/hw_breakpoint.c
>>>> +++ b/arch/x86/kernel/hw_breakpoint.c
>>>> @@ -132,6 +132,56 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>>>>   	return 0;
>>>>   }
>>>>   
>>>> +/*
>>>> + * Reinstall a hardware breakpoint on the current CPU.
>>>> + *
>>>> + * This function is used to re-establish a perf counter hardware breakpoint.
>>>> + * It finds the debug address register slot previously allocated for the
>>>> + * breakpoint and re-enables it by writing the address to the debug register
>>>> + * and setting the corresponding bits in the debug control register (DR7).
>>>> + *
>>>> + * It is expected that the breakpoint's event context lock is already held
>>>> + * and interrupts are disabled, ensuring atomicity and safety from other
>>>> + * event handlers.
>>>> + */
>>>> +int arch_reinstall_hw_breakpoint(struct perf_event *bp)
>>>> +{
>>>> +	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
>>>> +	unsigned long *dr7;
>>>> +	int i;
>>>> +
>>>> +	lockdep_assert_irqs_disabled();
>>>> +
>>>> +	for (i = 0; i < HBP_NUM; i++) {
>>>> +		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
>>>> +
>>>> +		if (*slot == bp)
>>>> +			break;
>>>> +	}
>>>> +
>>>> +	if (WARN_ONCE(i == HBP_NUM, "Can't find a matching breakpoint slot"))
>>>> +		return -EINVAL;
>>>> +
>>>> +	set_debugreg(info->address, i);
>>>> +	__this_cpu_write(cpu_debugreg[i], info->address);
>>>> +
>>>> +	dr7 = this_cpu_ptr(&cpu_dr7);
>>>> +	*dr7 |= encode_dr7(i, info->len, info->type);
>>>> +
>>>> +	/*
>>>> +	 * Ensure we first write cpu_dr7 before we set the DR7 register.
>>>> +	 * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
>>>> +	 */
>>>> +	barrier();
>>>> +
>>>> +	set_debugreg(*dr7, 7);
>>>> +	if (info->mask)
>>>> +		amd_set_dr_addr_mask(info->mask, i);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(arch_reinstall_hw_breakpoint);
>>> Yeah, I think not. For one this is an almost verbatim copy of
>>> arch_install_hw_breakpoint() with zero re-use. Surely you've been taught
>>> better?
>> I introduced this to modify bp_addr in atomic context in my RFC series.
>> I thought it was clearer to split the introduction and refactor.
>> And then It was used in the wprobe series, so I left it as introduced
>> in the RFC series.
>>
>> I agree your suggestion is right. I am willing to refactor after wprobe.
> I'm OK to refactor it to reuse arch_install_hw_breakpoint().
> My point is to have CONFIG_HAVE_REINSTALL_HW_BREAKPOINT so that
> we can easily implement the dependency for other features which
> requires this feature in Kconfig level.
>
>>> And why would we want to export guts like this?
>> I wanted to introduce a real-time stack corruption debugging tool,
>> which needs a helper to change bp_addr in atomic context (kprobe handler).
>> And wprobe needs it also.
> I agree with Peter, it should not expose the architecture
> dependent code directly. Instead, we need a wrapper.
>
> Thank you,
Understood, I will use the wrapper instead.