Skip to contentSkip to conversation

HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests

PATCH In review K.Prasad 2 participants 5 messages Aug 26, 2009 – Aug 28, 2009 1 message not in the archive

Subsystems (2)
x86 Generic include/asm header files
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

K.Prasad

author
#1 lore raw

PATCH 1/3 HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests

Allow kernel-space hw-breakpoints to be restricted only for a subset of CPUs using a cpumask field in 'struct hw_breakpoint'.

Signed-off-by K.Prasad <[email protected]>
---
 arch/x86/kernel/hw_breakpoint.c     |    6 +
 include/asm-generic/hw_breakpoint.h |    2 
 kernel/hw_breakpoint.c              |  119 +++++++++++++++++++++++++-----------
 3 files changed, 92 insertions(+), 35 deletions(-)
arch/x86/kernel/hw_breakpoint.c +5 -1
Index: linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
===================================================================
@@ -78,7 +78,7 @@ void arch_update_kernel_hw_breakpoint(vo
 	set_debugreg(0UL, 7);
 
 	for (i = hbp_kernel_pos; i < HBP_NUM; i++) {
-		per_cpu(this_hbp_kernel[i], cpu) = bp = hbp_kernel[i];
+		bp = per_cpu(this_hbp_kernel[i], cpu);
 		if (bp) {
 			temp_kdr7 |= encode_dr7(i, bp->info.len, bp->info.type);
 			set_debugreg(bp->info.address, i);

@@ -207,6 +207,10 @@ int arch_validate_hwbkpt_settings(struct
 	unsigned int align;
 	int ret = -EINVAL;
 
+	/* User-space breakpoints cannot be restricted to a subset of CPUs */
+	if (tsk && bp->cpumask)
+		return ret;
+
 	switch (bp->info.type) {
 	/*
 	 * Ptrace-refactoring code
include/asm-generic/hw_breakpoint.h +2 -0
Index: linux-2.6-tip.hbkpt/include/asm-generic/hw_breakpoint.h
===================================================================
@@ -8,6 +8,7 @@
 #ifdef	__KERNEL__
 #include <linux/list.h>
 #include <linux/types.h>
+#include <linux/cpumask.h>
 #include <linux/kallsyms.h>
 
 /**

@@ -102,6 +103,7 @@
  */
 struct hw_breakpoint {
 	void (*triggered)(struct hw_breakpoint *, struct pt_regs *);
+	const cpumask_t *cpumask;
 	struct arch_hw_breakpoint info;
 };
kernel/hw_breakpoint.c +85 -34

170 lines not on this page · raw message

author
#2 lore raw

PATCH 2/3 HW-BKPT: Allow kernel breakpoints to be modified through a new API

In reply to a message not in the archive

Introduce modify_kernel_hw_breakpoint() API that can quickly change the characteristics (such as address, length, type) of a kernel-space breakpoint without having to unregister first and then re-register it.

Signed-off-by K.Prasad <[email protected]>
---
 include/asm-generic/hw_breakpoint.h |    2 +
 kernel/hw_breakpoint.c              |   49 ++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)
include/asm-generic/hw_breakpoint.h +2 -0
Index: linux-2.6-tip.hbkpt/include/asm-generic/hw_breakpoint.h
===================================================================
@@ -133,6 +133,8 @@ extern void unregister_user_hw_breakpoin
  * Kernel breakpoints are not associated with any particular thread.
  */
 extern int register_kernel_hw_breakpoint(struct hw_breakpoint *bp);
+extern int modify_kernel_hw_breakpoint(struct hw_breakpoint *old_bp,
+					struct hw_breakpoint *new_bp);
 extern void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp);
 
 extern unsigned int hbp_kernel_pos;
kernel/hw_breakpoint.c +49 -0

55 lines not on this page · raw message

author
#3 lore raw

PATCH 3/3 HW-BKPT: Enable/disable the breakpoints when still registered

In reply to a message not in the archive

Allow breakpoints to be enabled/disabled without yielding the breakpoint request through a new API - enable_hw_breakpoint().

Signed-off-by K.Prasad <[email protected]>
---
 arch/x86/kernel/hw_breakpoint.c     |   12 ++++++----
 include/asm-generic/hw_breakpoint.h |    9 +++++++
 kernel/hw_breakpoint.c              |   41 ++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 5 deletions(-)
arch/x86/kernel/hw_breakpoint.c +7 -5
Index: linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
===================================================================
@@ -79,10 +79,11 @@ void arch_update_kernel_hw_breakpoint(vo
 
 	for (i = hbp_kernel_pos; i < HBP_NUM; i++) {
 		bp = per_cpu(this_hbp_kernel[i], cpu);
-		if (bp) {
+		if (!bp)
+			continue;
+		set_debugreg(bp->info.address, i);
+		if (bp->enabled)
 			temp_kdr7 |= encode_dr7(i, bp->info.len, bp->info.type);
-			set_debugreg(bp->info.address, i);
-		}
 	}
 
 	/* No need to set DR6. Update the debug registers with kernel-space

@@ -282,8 +283,9 @@ void arch_update_user_hw_breakpoint(int 
 	thread->debugreg7 &= ~dr7_masks[pos];
 	if (bp) {
 		thread->debugreg[pos] = bp->info.address;
-		thread->debugreg7 |= encode_dr7(pos, bp->info.len,
-							bp->info.type);
+		if (bp->enabled)
+			thread->debugreg7 |= encode_dr7(pos, bp->info.len,
+								bp->info.type);
 	} else
 		thread->debugreg[pos] = 0;
 }
include/asm-generic/hw_breakpoint.h +9 -0
Index: linux-2.6-tip.hbkpt/include/asm-generic/hw_breakpoint.h
===================================================================
@@ -102,6 +102,12 @@
  * ----------------------------------------------------------------------
  */
 struct hw_breakpoint {
+	/*
+	 * Denotes if a breakpoint is currently enabled in physical debug
+	 * registers. Not to be set directly by the end-user. Must be
+	 * operated through enable_hw_breakpoint() API only.
+	 */
+	unsigned int enabled;
 	void (*triggered)(struct hw_breakpoint *, struct pt_regs *);
 	const cpumask_t *cpumask;
 	struct arch_hw_breakpoint info;

@@ -137,6 +143,9 @@ extern int modify_kernel_hw_breakpoint(s
 					struct hw_breakpoint *new_bp);
 extern void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp);
 
+extern void enable_hw_breakpoint(struct hw_breakpoint *bp,
+				struct task_struct *tsk, unsigned int enabled);
+
 extern unsigned int hbp_kernel_pos;
 
 #endif	/* __KERNEL__ */
kernel/hw_breakpoint.c +41 -0

53 lines not on this page · raw message

#4 lore raw

Re: [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered

On Thu, Aug 27, 2009 at 01:45:06AM +0530, K.Prasad wrote:

...

include/asm-generic/hw_breakpoint.h line 110

Quoted 7 lines

struct hw_breakpoint {

include/asm-generic/hw_breakpoint.h +6 -0
+	/*
+	 * Denotes if a breakpoint is currently enabled in physical debug
+	 * registers. Not to be set directly by the end-user. Must be
+	 * operated through enable_hw_breakpoint() API only.
+	 */
+	unsigned int enabled;

bool?

...

Quoted 21 lines
kernel/hw_breakpoint.c +21 -0
+void enable_hw_breakpoint(struct hw_breakpoint *bp, struct task_struct *tsk,
+							unsigned int enabled)
+{
+	int i;
+	struct thread_struct *thread = &(tsk->thread);
+
+	spin_lock_bh(&hw_breakpoint_lock);
+
+	bp->enabled = enabled;
+	/* Enable/Disable the kernel-space breakpoint */
+	if (!tsk) {
+		if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
+			arch_update_kernel_hw_breakpoint(NULL);
+		smp_call_function_many(bp->cpumask,
+				arch_update_kernel_hw_breakpoint, NULL, 1);
+		goto out;
+	}
+
+	/* Enable/disable the user-space breakpoint */
+	for (i = 0; i < hbp_kernel_pos; i++) {
+		if (thread->hbp[i] != bp)

kernel/hw_breakpoint.c ret_path: · line 506

Quoted 10 lines
kernel/hw_breakpoint.c +10 -0
+			continue;
+		arch_update_user_hw_breakpoint(i, tsk);
+		if (tsk == current)
+			arch_install_thread_hw_breakpoint(tsk);
+		break;
+	}
+out:
+	spin_unlock_bh(&hw_breakpoint_lock);
+}
+EXPORT_SYMBOL_GPL(enable_hw_breakpoint);

Not sure if its cleaner to have enable_hw_breakpoint() and disable_hw_breakpoint() rather than one overloaded call.

Ananth

author
#5 lore raw

Re: [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered

Quoted 14 lines · Ananth N Mavinakayanahalli

On Thu, Aug 27, 2009 at 01:45:06AM +0530, K.Prasad wrote:

...

include/asm-generic/hw_breakpoint.h line 110

Quoted 7 lines

struct hw_breakpoint {

include/asm-generic/hw_breakpoint.h +6 -0
+	/*
+	 * Denotes if a breakpoint is currently enabled in physical debug
+	 * registers. Not to be set directly by the end-user. Must be
+	 * operated through enable_hw_breakpoint() API only.
+	 */
+	unsigned int enabled;

bool?

Quoted 2 lines

...

A 'bool' data-type doesn't really provide advantages in terms of the bytes consumed as far as I can see...leaving it as 'unsigned int' would allow any positive integer to be considered as a 'enable' request.

However considering the change due to the second comment, and for the fact that the data-type is more intuitive I'll change them to 'bool'.

Quoted 26 lines
Quoted 26 lines
kernel/hw_breakpoint.c +26 -0
+void enable_hw_breakpoint(struct hw_breakpoint *bp, struct task_struct *tsk,
+							unsigned int enabled)
+{
+	int i;
+	struct thread_struct *thread = &(tsk->thread);
+
+	spin_lock_bh(&hw_breakpoint_lock);
+
+	bp->enabled = enabled;
+	/* Enable/Disable the kernel-space breakpoint */
+	if (!tsk) {
+		if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
+			arch_update_kernel_hw_breakpoint(NULL);
+		smp_call_function_many(bp->cpumask,
+				arch_update_kernel_hw_breakpoint, NULL, 1);
+		goto out;
+	}
+
+	/* Enable/disable the user-space breakpoint */
+	for (i = 0; i < hbp_kernel_pos; i++) {
+		if (thread->hbp[i] != bp)
+			continue;
+		arch_update_user_hw_breakpoint(i, tsk);
+		if (tsk == current)
+			arch_install_thread_hw_breakpoint(tsk);
+		break;
Quoted 9 lines

kernel/hw_breakpoint.c ret_path: · line 506

Quoted 5 lines
kernel/hw_breakpoint.c +5 -0
+	}
+out:
+	spin_unlock_bh(&hw_breakpoint_lock);
+}
+EXPORT_SYMBOL_GPL(enable_hw_breakpoint);

Not sure if its cleaner to have enable_hw_breakpoint() and disable_hw_breakpoint() rather than one overloaded call.

The plan was to have two separate interfaces at first, but then it turned out that the code was largely similar...tempting them to be clubbed together.

But after looking at 'struct pmu' (whose callbacks are intended to map to these interfaces), I see that two distinct interfaces would be better (there's a enable/disable callback in struct pmu too).

I shall post a new version of this patch including more changes.

Thanks,
K.Prasad