[PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[Jinchao Wang]

Consolidate breakpoint management into a single helper function to
reduce code duplication. This introduces new static helpers for
slot management and debug register manipulation.

Also, add `<linux/types.h>` to the header file to fix a build
dependency.

Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
---
 arch/x86/include/asm/hw_breakpoint.h |   7 +-
 arch/x86/kernel/hw_breakpoint.c      | 151 ++++++++++++++++-----------
 2 files changed, 96 insertions(+), 62 deletions(-)

diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
index 0bc931cd0698..bd437a30dbf2 100644
--- a/arch/x86/include/asm/hw_breakpoint.h
+++ b/arch/x86/include/asm/hw_breakpoint.h
@@ -3,8 +3,8 @@
 #define	_I386_HW_BREAKPOINT_H
 
 #include <uapi/asm/hw_breakpoint.h>
-
 #define	__ARCH_HW_BREAKPOINT_H
+#include <linux/types.h>
 
 /*
  * The name should probably be something dealt in
@@ -18,6 +18,11 @@ struct arch_hw_breakpoint {
 	u8		type;
 };
 
+enum bp_slot_action {
+	BP_SLOT_ACTION_INSTALL,
+	BP_SLOT_ACTION_UNINSTALL,
+};
+
 #include <linux/kdebug.h>
 #include <linux/percpu.h>
 #include <linux/list.h>
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index b01644c949b2..1736063a82b7 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -48,7 +48,6 @@ static DEFINE_PER_CPU(unsigned long, cpu_debugreg[HBP_NUM]);
  */
 static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM]);
 
-
 static inline unsigned long
 __encode_dr7(int drnum, unsigned int len, unsigned int type)
 {
@@ -84,54 +83,115 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type)
 	return (dr7 >> (bpnum * DR_ENABLE_SIZE)) & 0x3;
 }
 
-/*
- * Install a perf counter breakpoint.
- *
- * We seek a free debug address register and use it for this
- * breakpoint. Eventually we enable it in the debug control register.
- *
- * Atomic: we hold the counter->ctx->lock and we only handle variables
- * and registers local to this cpu.
- */
-int arch_install_hw_breakpoint(struct perf_event *bp)
+static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	unsigned long *dr7;
-	int i;
+	struct perf_event *old_bp;
+	struct perf_event *new_bp;
+	int slot;
+
+	switch (action) {
+	case BP_SLOT_ACTION_INSTALL:
+		old_bp = NULL;
+		new_bp = bp;
+		break;
+	case BP_SLOT_ACTION_UNINSTALL:
+		old_bp = bp;
+		new_bp = NULL;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	lockdep_assert_irqs_disabled();
 
-	for (i = 0; i < HBP_NUM; i++) {
-		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
+	for (slot = 0; slot < HBP_NUM; slot++) {
+		struct perf_event **curr = this_cpu_ptr(&bp_per_reg[slot]);
 
-		if (!*slot) {
-			*slot = bp;
-			break;
+		if (*curr == old_bp) {
+			*curr = new_bp;
+			return slot;
 		}
 	}
 
-	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
-		return -EBUSY;
+	if (old_bp) {
+		WARN_ONCE(1, "Can't find matching breakpoint slot");
+		return -EINVAL;
+	}
 
-	set_debugreg(info->address, i);
-	__this_cpu_write(cpu_debugreg[i], info->address);
+	WARN_ONCE(1, "No free breakpoint slots");
+	return -EBUSY;
+}
 
-	dr7 = this_cpu_ptr(&cpu_dr7);
-	*dr7 |= encode_dr7(i, info->len, info->type);
+static void setup_hwbp(struct arch_hw_breakpoint *info, int slot, bool enable)
+{
+	unsigned long dr7;
+
+	set_debugreg(info->address, slot);
+	__this_cpu_write(cpu_debugreg[slot], info->address);
+
+	dr7 = this_cpu_read(cpu_dr7);
+	if (enable)
+		dr7 |= encode_dr7(slot, info->len, info->type);
+	else
+		dr7 &= ~__encode_dr7(slot, 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.
+	 * Enabling:
+	 *   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.
 	 */
+	if (enable)
+		this_cpu_write(cpu_dr7, dr7);
+
 	barrier();
 
-	set_debugreg(*dr7, 7);
+	set_debugreg(dr7, 7);
+
 	if (info->mask)
-		amd_set_dr_addr_mask(info->mask, i);
+		amd_set_dr_addr_mask(enable ? info->mask : 0, slot);
+
+	/*
+	 * Disabling:
+	 *   Ensure the write to cpu_dr7 is after we've set the DR7 register.
+	 *   This ensures an NMI never see cpu_dr7 0 when DR7 is not.
+	 */
+	if (!enable)
+		this_cpu_write(cpu_dr7, dr7);
+}
+
+static int arch_manage_bp(struct perf_event *bp, enum bp_slot_action action)
+{
+	struct arch_hw_breakpoint *info;
+	bool install = true;
+	int slot;
+
+	if (action == BP_SLOT_ACTION_UNINSTALL)
+		install = false;
+
+	slot = manage_bp_slot(bp, action);
+	if (slot < 0)
+		return slot;
+
+	info = counter_arch_bp(bp);
+	setup_hwbp(info, slot, install);
 
 	return 0;
 }
 
+/*
+ * Install a perf counter breakpoint.
+ *
+ * We seek a free debug address register and use it for this
+ * breakpoint. Eventually we enable it in the debug control register.
+ *
+ * Atomic: we hold the counter->ctx->lock and we only handle variables
+ * and registers local to this cpu.
+ */
+int arch_install_hw_breakpoint(struct perf_event *bp)
+{
+	return arch_manage_bp(bp, BP_SLOT_ACTION_INSTALL);
+}
+
 /*
  * Uninstall the breakpoint contained in the given counter.
  *
@@ -143,38 +203,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
  */
 void arch_uninstall_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) {
-			*slot = NULL;
-			break;
-		}
-	}
-
-	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
-		return;
-
-	dr7 = this_cpu_read(cpu_dr7);
-	dr7 &= ~__encode_dr7(i, info->len, info->type);
-
-	set_debugreg(dr7, 7);
-	if (info->mask)
-		amd_set_dr_addr_mask(0, i);
-
-	/*
-	 * Ensure the write to cpu_dr7 is after we've set the DR7 register.
-	 * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
-	 */
-	barrier();
-
-	this_cpu_write(cpu_dr7, dr7);
+	arch_manage_bp(bp, BP_SLOT_ACTION_UNINSTALL);
 }
 
 static int arch_bp_generic_len(int x86_len)
-- 
2.43.0

[PATCH 2/2] x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint

[Jinchao Wang]

The new `arch_reinstall_hw_breakpoint` function can be used in an
atomic context, unlike the more expensive free and re-allocation path.

This patch adds `BP_SLOT_ACTION_REINSTALL` to the `enum` and updates the
`manage_bp_slot` helper to correctly handle the action. This allows
callers to efficiently re-establish an existing breakpoint.

Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
---
 arch/x86/include/asm/hw_breakpoint.h |  2 ++
 arch/x86/kernel/hw_breakpoint.c      | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
index bd437a30dbf2..d1cc6db8a59f 100644
--- a/arch/x86/include/asm/hw_breakpoint.h
+++ b/arch/x86/include/asm/hw_breakpoint.h
@@ -20,6 +20,7 @@ struct arch_hw_breakpoint {
 
 enum bp_slot_action {
 	BP_SLOT_ACTION_INSTALL,
+	BP_SLOT_ACTION_REINSTALL,
 	BP_SLOT_ACTION_UNINSTALL,
 };
 
@@ -64,6 +65,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 1736063a82b7..99223695cee8 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -94,6 +94,10 @@ static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
 		old_bp = NULL;
 		new_bp = bp;
 		break;
+	case BP_SLOT_ACTION_REINSTALL:
+		old_bp = bp;
+		new_bp = bp;
+		break;
 	case BP_SLOT_ACTION_UNINSTALL:
 		old_bp = bp;
 		new_bp = NULL;
@@ -192,6 +196,23 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	return arch_manage_bp(bp, BP_SLOT_ACTION_INSTALL);
 }
 
+/*
+ * 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)
+{
+	return arch_manage_bp(bp, BP_SLOT_ACTION_REINSTALL);
+}
+
 /*
  * Uninstall the breakpoint contained in the given counter.
  *
-- 
2.43.0

Re: [PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[Dave Hansen]

On 9/10/25 02:39, Jinchao Wang wrote:
> Consolidate breakpoint management into a single helper function to
> reduce code duplication. This introduces new static helpers for
> slot management and debug register manipulation.
> 
> Also, add `<linux/types.h>` to the header file to fix a build
> dependency.
> 
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> ---
>  arch/x86/include/asm/hw_breakpoint.h |   7 +-
>  arch/x86/kernel/hw_breakpoint.c      | 151 ++++++++++++++++-----------
>  2 files changed, 96 insertions(+), 62 deletions(-)

That diffstat doesn't look like it's reducing code duplication.

Re: [PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[H. Peter Anvin]

On September 10, 2025 7:47:06 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>On 9/10/25 02:39, Jinchao Wang wrote:
>> Consolidate breakpoint management into a single helper function to
>> reduce code duplication. This introduces new static helpers for
>> slot management and debug register manipulation.
>> 
>> Also, add `<linux/types.h>` to the header file to fix a build
>> dependency.
>> 
>> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
>> ---
>>  arch/x86/include/asm/hw_breakpoint.h |   7 +-
>>  arch/x86/kernel/hw_breakpoint.c      | 151 ++++++++++++++++-----------
>>  2 files changed, 96 insertions(+), 62 deletions(-)
>
>That diffstat doesn't look like it's reducing code duplication.

He does add a *lot* of comments.

Re: [PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[Jinchao Wang]

On 9/10/25 22:56, H. Peter Anvin wrote:
> On September 10, 2025 7:47:06 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>> On 9/10/25 02:39, Jinchao Wang wrote:
>>> Consolidate breakpoint management into a single helper function to
>>> reduce code duplication. This introduces new static helpers for
>>> slot management and debug register manipulation.
>>>
>>> Also, add `<linux/types.h>` to the header file to fix a build
>>> dependency.
>>>
>>> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
>>> ---
>>>   arch/x86/include/asm/hw_breakpoint.h |   7 +-
>>>   arch/x86/kernel/hw_breakpoint.c      | 151 ++++++++++++++++-----------
>>>   2 files changed, 96 insertions(+), 62 deletions(-)
>> That diffstat doesn't look like it's reducing code duplication.
> He does add a *lot* of comments.
The diffstat was misleading. My refactoring consolidated duplicated
logic into reusable functions, which reduces the final code size.
The original refactoring can be found at:
https://lore.kernel.org/lkml/20250904002126.1514566-6-wangjinchao600@gmail.com
Measurements of the `hw_breakpoint.o`file were taken with 
`CONFIG_DEBUG_INFO`
enabled, using these commands:
```
objcopy --strip-debug arch/x86/kernel/hw_breakpoint.o tmp.o
du -b arch/x86/kernel/hw_breakpoint.o
du -b tmp.o
```
This table compares the code size in bytes. The `refactor`column indicates
if the refactoring was applied (`1`) or not (`0`). The `function`column
indicates if `arch_reinstall_hw_breakpoint`was included (`1`) or not (`0`).
```
+----------+------------+------------+----------+----------+
| refactor | function | unstripped | stripped | percent |
+----------+------------+------------+----------+----------+
| 0 | 0 | 275320 | 11976 | 100% |
| 1 | 0 | 275736 | 11448 | 96% |
| 0 | 1 | 278720 | 13520 | 100% |
| 1 | 1 | 276392 | 11760 | 87% |
+----------+------------+------------+----------+----------+
```
1.**Overall Refactoring Benefit**: The refactored version (`refactor 1`) 
with
`arch_reinstall_hw_breakpoint`is 11760 bytes, a **13%**reduction from the
original's 13520 bytes.
2.**Unification Target**: Adding `arch_reinstall_hw_breakpoint`to the
un-refactored code increased the size by **1544 bytes**. Adding it to the
refactored code only increased the size by **312 bytes**. This demonstrates
that the refactoring makes future code additions more efficient.

-- 
Thanks,
Jinchao

Re: [PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[Masami Hiramatsu]

On Wed, 10 Sep 2025 17:39:34 +0800
Jinchao Wang <wangjinchao600@gmail.com> wrote:

> Consolidate breakpoint management into a single helper function to
> reduce code duplication. This introduces new static helpers for
> slot management and debug register manipulation.
> 
> Also, add `<linux/types.h>` to the header file to fix a build
> dependency.

Looks good to me. Just some nitpicks.

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

> 
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> ---
>  arch/x86/include/asm/hw_breakpoint.h |   7 +-
>  arch/x86/kernel/hw_breakpoint.c      | 151 ++++++++++++++++-----------
>  2 files changed, 96 insertions(+), 62 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
> index 0bc931cd0698..bd437a30dbf2 100644
> --- a/arch/x86/include/asm/hw_breakpoint.h
> +++ b/arch/x86/include/asm/hw_breakpoint.h
> @@ -3,8 +3,8 @@
>  #define	_I386_HW_BREAKPOINT_H
>  
>  #include <uapi/asm/hw_breakpoint.h>
> -

nit: Why this line is removed?

>  #define	__ARCH_HW_BREAKPOINT_H
> +#include <linux/types.h>
>  
>  /*
>   * The name should probably be something dealt in
> @@ -18,6 +18,11 @@ struct arch_hw_breakpoint {
>  	u8		type;
>  };
>  
> +enum bp_slot_action {
> +	BP_SLOT_ACTION_INSTALL,
> +	BP_SLOT_ACTION_UNINSTALL,
> +};
> +
>  #include <linux/kdebug.h>
>  #include <linux/percpu.h>
>  #include <linux/list.h>
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index b01644c949b2..1736063a82b7 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -48,7 +48,6 @@ static DEFINE_PER_CPU(unsigned long, cpu_debugreg[HBP_NUM]);
>   */
>  static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM]);
>  
> -

Ditto.

>  static inline unsigned long
>  __encode_dr7(int drnum, unsigned int len, unsigned int type)
>  {
> @@ -84,54 +83,115 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type)
>  	return (dr7 >> (bpnum * DR_ENABLE_SIZE)) & 0x3;
>  }
>  
> -/*
> - * Install a perf counter breakpoint.
> - *
> - * We seek a free debug address register and use it for this
> - * breakpoint. Eventually we enable it in the debug control register.
> - *
> - * Atomic: we hold the counter->ctx->lock and we only handle variables
> - * and registers local to this cpu.
> - */
> -int arch_install_hw_breakpoint(struct perf_event *bp)
> +static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
>  {
> -	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> -	unsigned long *dr7;
> -	int i;
> +	struct perf_event *old_bp;
> +	struct perf_event *new_bp;
> +	int slot;
> +
> +	switch (action) {
> +	case BP_SLOT_ACTION_INSTALL:
> +		old_bp = NULL;
> +		new_bp = bp;
> +		break;
> +	case BP_SLOT_ACTION_UNINSTALL:
> +		old_bp = bp;
> +		new_bp = NULL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	lockdep_assert_irqs_disabled();
>  
> -	for (i = 0; i < HBP_NUM; i++) {
> -		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> +	for (slot = 0; slot < HBP_NUM; slot++) {
> +		struct perf_event **curr = this_cpu_ptr(&bp_per_reg[slot]);
>  
> -		if (!*slot) {
> -			*slot = bp;
> -			break;
> +		if (*curr == old_bp) {
> +			*curr = new_bp;
> +			return slot;
>  		}
>  	}
>  
> -	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
> -		return -EBUSY;
> +	if (old_bp) {
> +		WARN_ONCE(1, "Can't find matching breakpoint slot");
> +		return -EINVAL;
> +	}
>  
> -	set_debugreg(info->address, i);
> -	__this_cpu_write(cpu_debugreg[i], info->address);
> +	WARN_ONCE(1, "No free breakpoint slots");
> +	return -EBUSY;
> +}
>  
> -	dr7 = this_cpu_ptr(&cpu_dr7);
> -	*dr7 |= encode_dr7(i, info->len, info->type);
> +static void setup_hwbp(struct arch_hw_breakpoint *info, int slot, bool enable)
> +{
> +	unsigned long dr7;
> +
> +	set_debugreg(info->address, slot);
> +	__this_cpu_write(cpu_debugreg[slot], info->address);
> +
> +	dr7 = this_cpu_read(cpu_dr7);
> +	if (enable)
> +		dr7 |= encode_dr7(slot, info->len, info->type);
> +	else
> +		dr7 &= ~__encode_dr7(slot, 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.
> +	 * Enabling:
> +	 *   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.
>  	 */
> +	if (enable)
> +		this_cpu_write(cpu_dr7, dr7);
> +
>  	barrier();
>  
> -	set_debugreg(*dr7, 7);
> +	set_debugreg(dr7, 7);
> +
>  	if (info->mask)
> -		amd_set_dr_addr_mask(info->mask, i);
> +		amd_set_dr_addr_mask(enable ? info->mask : 0, slot);
> +
> +	/*
> +	 * Disabling:
> +	 *   Ensure the write to cpu_dr7 is after we've set the DR7 register.
> +	 *   This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> +	 */
> +	if (!enable)
> +		this_cpu_write(cpu_dr7, dr7);
> +}
> +
> +static int arch_manage_bp(struct perf_event *bp, enum bp_slot_action action)
> +{
> +	struct arch_hw_breakpoint *info;
> +	bool install = true;
> +	int slot;
> +
> +	if (action == BP_SLOT_ACTION_UNINSTALL)
> +		install = false;

This looks a bit unnecessary. 

> +
> +	slot = manage_bp_slot(bp, action);
> +	if (slot < 0)
> +		return slot;
> +
> +	info = counter_arch_bp(bp);
> +	setup_hwbp(info, slot, install);

since you can do

	setup_hwbp(info, slot,
		   action != BP_SLOT_ACTION_UNINSTALL);

Thank you,


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

Re: [PATCH 2/2] x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint

[Masami Hiramatsu]

On Wed, 10 Sep 2025 17:39:35 +0800
Jinchao Wang <wangjinchao600@gmail.com> wrote:

> The new `arch_reinstall_hw_breakpoint` function can be used in an
> atomic context, unlike the more expensive free and re-allocation path.
> 
> This patch adds `BP_SLOT_ACTION_REINSTALL` to the `enum` and updates the
> `manage_bp_slot` helper to correctly handle the action. This allows
> callers to efficiently re-establish an existing breakpoint.
> 

Ah, this also looks good to me.

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

Thanks,

> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> ---
>  arch/x86/include/asm/hw_breakpoint.h |  2 ++
>  arch/x86/kernel/hw_breakpoint.c      | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
> index bd437a30dbf2..d1cc6db8a59f 100644
> --- a/arch/x86/include/asm/hw_breakpoint.h
> +++ b/arch/x86/include/asm/hw_breakpoint.h
> @@ -20,6 +20,7 @@ struct arch_hw_breakpoint {
>  
>  enum bp_slot_action {
>  	BP_SLOT_ACTION_INSTALL,
> +	BP_SLOT_ACTION_REINSTALL,
>  	BP_SLOT_ACTION_UNINSTALL,
>  };
>  
> @@ -64,6 +65,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 1736063a82b7..99223695cee8 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -94,6 +94,10 @@ static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
>  		old_bp = NULL;
>  		new_bp = bp;
>  		break;
> +	case BP_SLOT_ACTION_REINSTALL:
> +		old_bp = bp;
> +		new_bp = bp;
> +		break;
>  	case BP_SLOT_ACTION_UNINSTALL:
>  		old_bp = bp;
>  		new_bp = NULL;
> @@ -192,6 +196,23 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>  	return arch_manage_bp(bp, BP_SLOT_ACTION_INSTALL);
>  }
>  
> +/*
> + * 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)
> +{
> +	return arch_manage_bp(bp, BP_SLOT_ACTION_REINSTALL);
> +}
> +
>  /*
>   * Uninstall the breakpoint contained in the given counter.
>   *
> -- 
> 2.43.0
> 


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

Re: [PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall

[Jinchao Wang]

On Thu, Sep 11, 2025 at 05:03:45PM +0900, Masami Hiramatsu wrote:
> On Wed, 10 Sep 2025 17:39:34 +0800
> Jinchao Wang <wangjinchao600@gmail.com> wrote:
> 
> > Consolidate breakpoint management into a single helper function to
> > reduce code duplication. This introduces new static helpers for
> > slot management and debug register manipulation.
> > 
> > Also, add `<linux/types.h>` to the header file to fix a build
> > dependency.
> 
> Looks good to me. Just some nitpicks.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> > 
> > Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> > ---
> >  arch/x86/include/asm/hw_breakpoint.h |   7 +-
> >  arch/x86/kernel/hw_breakpoint.c      | 151 ++++++++++++++++-----------
> >  2 files changed, 96 insertions(+), 62 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
> > index 0bc931cd0698..bd437a30dbf2 100644
> > --- a/arch/x86/include/asm/hw_breakpoint.h
> > +++ b/arch/x86/include/asm/hw_breakpoint.h
> > @@ -3,8 +3,8 @@
> >  #define	_I386_HW_BREAKPOINT_H
> >  
> >  #include <uapi/asm/hw_breakpoint.h>
> > -
> 
> nit: Why this line is removed?
sharp eye, will restore to its original state.
> 
> >  #define	__ARCH_HW_BREAKPOINT_H
> > +#include <linux/types.h>
> >  
> >  /*
> >   * The name should probably be something dealt in
> > @@ -18,6 +18,11 @@ struct arch_hw_breakpoint {
> >  	u8		type;
> >  };
> >  
> > +enum bp_slot_action {
> > +	BP_SLOT_ACTION_INSTALL,
> > +	BP_SLOT_ACTION_UNINSTALL,
> > +};
> > +
> >  #include <linux/kdebug.h>
> >  #include <linux/percpu.h>
> >  #include <linux/list.h>
> > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> > index b01644c949b2..1736063a82b7 100644
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
> > @@ -48,7 +48,6 @@ static DEFINE_PER_CPU(unsigned long, cpu_debugreg[HBP_NUM]);
> >   */
> >  static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM]);
> >  
> > -
> 
> Ditto.
There were double-space lines, so delete one.
> 
> >  static inline unsigned long
> >  __encode_dr7(int drnum, unsigned int len, unsigned int type)
> >  {
> > @@ -84,54 +83,115 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type)
> >  	return (dr7 >> (bpnum * DR_ENABLE_SIZE)) & 0x3;
> >  }
> >  
> > -/*
> > - * Install a perf counter breakpoint.
> > - *
> > - * We seek a free debug address register and use it for this
> > - * breakpoint. Eventually we enable it in the debug control register.
> > - *
> > - * Atomic: we hold the counter->ctx->lock and we only handle variables
> > - * and registers local to this cpu.
> > - */
> > -int arch_install_hw_breakpoint(struct perf_event *bp)
> > +static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
> >  {
> > -	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> > -	unsigned long *dr7;
> > -	int i;
> > +	struct perf_event *old_bp;
> > +	struct perf_event *new_bp;
> > +	int slot;
> > +
> > +	switch (action) {
> > +	case BP_SLOT_ACTION_INSTALL:
> > +		old_bp = NULL;
> > +		new_bp = bp;
> > +		break;
> > +	case BP_SLOT_ACTION_UNINSTALL:
> > +		old_bp = bp;
> > +		new_bp = NULL;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >  
> >  	lockdep_assert_irqs_disabled();
> >  
> > -	for (i = 0; i < HBP_NUM; i++) {
> > -		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> > +	for (slot = 0; slot < HBP_NUM; slot++) {
> > +		struct perf_event **curr = this_cpu_ptr(&bp_per_reg[slot]);
> >  
> > -		if (!*slot) {
> > -			*slot = bp;
> > -			break;
> > +		if (*curr == old_bp) {
> > +			*curr = new_bp;
> > +			return slot;
> >  		}
> >  	}
> >  
> > -	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
> > -		return -EBUSY;
> > +	if (old_bp) {
> > +		WARN_ONCE(1, "Can't find matching breakpoint slot");
> > +		return -EINVAL;
> > +	}
> >  
> > -	set_debugreg(info->address, i);
> > -	__this_cpu_write(cpu_debugreg[i], info->address);
> > +	WARN_ONCE(1, "No free breakpoint slots");
> > +	return -EBUSY;
> > +}
> >  
> > -	dr7 = this_cpu_ptr(&cpu_dr7);
> > -	*dr7 |= encode_dr7(i, info->len, info->type);
> > +static void setup_hwbp(struct arch_hw_breakpoint *info, int slot, bool enable)
> > +{
> > +	unsigned long dr7;
> > +
> > +	set_debugreg(info->address, slot);
> > +	__this_cpu_write(cpu_debugreg[slot], info->address);
> > +
> > +	dr7 = this_cpu_read(cpu_dr7);
> > +	if (enable)
> > +		dr7 |= encode_dr7(slot, info->len, info->type);
> > +	else
> > +		dr7 &= ~__encode_dr7(slot, 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.
> > +	 * Enabling:
> > +	 *   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.
> >  	 */
> > +	if (enable)
> > +		this_cpu_write(cpu_dr7, dr7);
> > +
> >  	barrier();
> >  
> > -	set_debugreg(*dr7, 7);
> > +	set_debugreg(dr7, 7);
> > +
> >  	if (info->mask)
> > -		amd_set_dr_addr_mask(info->mask, i);
> > +		amd_set_dr_addr_mask(enable ? info->mask : 0, slot);
> > +
> > +	/*
> > +	 * Disabling:
> > +	 *   Ensure the write to cpu_dr7 is after we've set the DR7 register.
> > +	 *   This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> > +	 */
> > +	if (!enable)
> > +		this_cpu_write(cpu_dr7, dr7);
> > +}
> > +
> > +static int arch_manage_bp(struct perf_event *bp, enum bp_slot_action action)
> > +{
> > +	struct arch_hw_breakpoint *info;
> > +	bool install = true;
> > +	int slot;
> > +
> > +	if (action == BP_SLOT_ACTION_UNINSTALL)
> > +		install = false;
> 
> This looks a bit unnecessary. 
> 
> > +
> > +	slot = manage_bp_slot(bp, action);
> > +	if (slot < 0)
> > +		return slot;
> > +
> > +	info = counter_arch_bp(bp);
> > +	setup_hwbp(info, slot, install);
> 
> since you can do
> 
> 	setup_hwbp(info, slot,
> 		   action != BP_SLOT_ACTION_UNINSTALL);
> 
> Thank you,
Thanks, I will follow your advice in the next patch.
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

-- 
Jinchao