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);
+
/*
* Uninstall the breakpoint contained in the given counter.
*
--
2.43.0
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? And why would we want to export guts like this?
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. > > 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.
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, -- Masami Hiramatsu (Google) <mhiramat@kernel.org>
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.
On 9/9/25 16:39, Jinchao Wang wrote: > > 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: >>>> ... >>>> >>>> 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. > > Hi Masami, I would like to ask for your advice regarding a development workflow issue. I have a patch series that refactors |arch_reinstall_hw_breakpoint|, which I know you are using in your |wprobe| series. My new feature, |kstackwatch|, is dependent on my patches and also on your wrapper patch for |modify_wide_hw_breakpoint_local|. Since we are working on different subsystems with different maintainers, I am concerned about how to handle the dependencies between our patches to avoid introducing trouble. What is the best practice for this type of cross-subsystem dependency? Should one of us carry all the patches until they are merged, or is there a standard procedure for this situation? Thank you for your guidance. -- Thanks, Jinchao
© 2016 - 2025 Red Hat, Inc.