LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86 debugctl MSR access interface
@ 2008-11-07 14:44 stephane eranian
  2008-11-10  5:58 ` Ananth N Mavinakayanahalli
  0 siblings, 1 reply; 2+ messages in thread
From: stephane eranian @ 2008-11-07 14:44 UTC (permalink / raw)
  To: linux-kernel; +Cc: ananth, anil.s.keshavamurthy, davem, Metzger, Markus T

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

Hello,

On Intel X86 processors, the DEBUGCTL msr contains lots of bits
to control various unrelated features, such as LBR, BTS, PMU.

Nowadays, several subsystems need access to this MSR to set or
clear the bits they care about. In 2.6.27, the new DS/BTS interface
was introduced and DEBUGCTL can now be cleared and restored
on context switches.

The KPROBE interface also uses DEBUGCTL.

In the context of perfmon, it can be interesting to also modify bits
in DEBUGCTL, such as FREEZE_PMU_ON_PMI but also the
bits controlling the LBR (bit 0 and 11). With perfmon, however,
there are situations, e.g. LBR, where the DEBUGCTL bits are
managed on a per-CPU level and not per-task level. Simply
overwriting DEBUGCTL on context switch may conflict with
per-cpu usage.

It appears that modifications to DEBUGCTL must be done using
a read-modify-write approach rather than pure write. To that extent,
I would like to propose the attached patch which modifies the
update_debugctlmsr() function to implement a read-modify-write
access. The caller passes the value of debugctl and a mask
of bits of interest. For instance, if you want to set bit 1 and clear bit 0,
then you call update_debugctlmsr(0x2, 0x3).

Please let me know what you think about this patch.

Thanks.

[-- Attachment #2: debugctl.diff --]
[-- Type: application/octet-stream, Size: 10811 bytes --]

diff --git a/arch/x86/include/asm/ds.h b/arch/x86/include/asm/ds.h
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 5ca01e3..c4eddd8 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -430,7 +430,7 @@ struct thread_struct {
 	/* Max allowed port in the bitmap, in bytes: */
 	unsigned		io_bitmap_max;
 /* MSR_IA32_DEBUGCTLMSR value to switch in if TIF_DEBUGCTLMSR is set.  */
-	unsigned long	debugctlmsr;
+	unsigned long	debugctlval;
 #ifdef CONFIG_X86_DS
 /* Debug Store context; see include/asm-x86/ds.h; goes into MSR_IA32_DS_AREA */
 	struct ds_context	*ds_ctx;
@@ -752,13 +752,21 @@ extern void switch_to_new_gdt(void);
 extern void cpu_init(void);
 extern void init_gdt(int cpu);
 
-static inline void update_debugctlmsr(unsigned long debugctlmsr)
+static inline void update_debugctlmsr(unsigned long val, unsigned long mask)
 {
+	unsigned long ctl;
+	unsigned long flags;
 #ifndef CONFIG_X86_DEBUGCTLMSR
 	if (boot_cpu_data.x86 < 6)
 		return;
 #endif
-	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);
+	local_irq_save(flags);
+
+	rdmsrl(MSR_IA32_DEBUGCTLMSR, ctl);
+	ctl = (ctl & ~mask) | (val & mask);
+	wrmsrl(MSR_IA32_DEBUGCTLMSR, ctl);
+
+	local_irq_restore(flags);
 }
 
 /*
diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index 6c27679..fcb034a 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -410,13 +410,13 @@ static void __kprobes set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
 static void __kprobes clear_btf(void)
 {
 	if (test_thread_flag(TIF_DEBUGCTLMSR))
-		update_debugctlmsr(0);
+		update_debugctlmsr(0, DEBUGCTLMSR_BTF);
 }
 
 static void __kprobes restore_btf(void)
 {
 	if (test_thread_flag(TIF_DEBUGCTLMSR))
-		update_debugctlmsr(current->thread.debugctlmsr);
+		update_debugctlmsr(DEBUGCTLMSR_BTF, DEBUGCTLMSR_BTF);
 }
 
 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index ce578b2..73ad19a 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -254,8 +254,6 @@ void exit_thread(void)
 #ifdef CONFIG_X86_DS
 	/* Free any DS contexts that have not been properly released. */
 	if (unlikely(current->thread.ds_ctx)) {
-		/* we clear debugctl to make sure DS is not used. */
-		update_debugctlmsr(0);
 		ds_free(current->thread.ds_ctx);
 	}
 #endif /* CONFIG_X86_DS */
@@ -424,8 +422,8 @@ int set_tsc_mode(unsigned int val)
 }
 
 #ifdef CONFIG_X86_DS
-static int update_debugctl(struct thread_struct *prev,
-			struct thread_struct *next, unsigned long debugctl)
+static void switch_ds_area(struct thread_struct *prev,
+			struct thread_struct *next)
 {
 	unsigned long ds_prev = 0;
 	unsigned long ds_next = 0;
@@ -435,20 +433,13 @@ static int update_debugctl(struct thread_struct *prev,
 	if (next->ds_ctx)
 		ds_next = (unsigned long)next->ds_ctx->ds;
 
-	if (ds_next != ds_prev) {
-		/* we clear debugctl to make sure DS
-		 * is not in use when we change it */
-		debugctl = 0;
-		update_debugctlmsr(0);
+	if (ds_next != ds_prev)
 		wrmsr(MSR_IA32_DS_AREA, ds_next, 0);
-	}
-	return debugctl;
 }
 #else
-static int update_debugctl(struct thread_struct *prev,
-			struct thread_struct *next, unsigned long debugctl)
+static void swich_ds_area(struct thread_struct *prev,
+			struct thread_struct *next)
 {
-	return debugctl;
 }
 #endif /* CONFIG_X86_DS */
 
@@ -457,7 +448,6 @@ __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		 struct tss_struct *tss)
 {
 	struct thread_struct *prev, *next;
-	unsigned long debugctl;
 
 	prev = &prev_p->thread;
 	next = &next_p->thread;
@@ -465,10 +455,13 @@ __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	if (test_tsk_thread_flag(prev_p, TIF_PERFMON_CTXSW))
 		pfm_ctxsw_out(prev_p, next_p);
 
-	debugctl = update_debugctl(prev, next, prev->debugctlmsr);
+	if (test_tsk_thread_flag(prev_p, TIF_DEBUGCTLMSR))
+		update_debugctlmsr(0, prev->debugctlval);
+
+	switch_ds_area(prev, next);
 
-	if (next->debugctlmsr != debugctl)
-		update_debugctlmsr(next->debugctlmsr);
+	if (test_tsk_thread_flag(next_p, TIF_DEBUGCTLMSR))
+		update_debugctlmsr(next->debugctlval, next->debugctlval);
 
 	if (test_tsk_thread_flag(next_p, TIF_PERFMON_CTXSW))
 		pfm_ctxsw_in(prev_p, next_p);
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 9c6ff7a..250a9fd 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -236,13 +236,17 @@ void exit_thread(void)
 		t->io_bitmap_max = 0;
 		put_cpu();
 	}
+
+	/* clear debugctl if used */
+	if (test_tsk_thread_flag(me, TIF_DEBUGCTLMSR)) {
+		update_debugctlmsr(0, t->debugctlval);
+		clear_tsk_thread_flag(me, TIF_DEBUGCTLMSR);
+	}
+
 #ifdef CONFIG_X86_DS
 	/* Free any DS contexts that have not been properly released. */
-	if (unlikely(t->ds_ctx)) {
-		/* we clear debugctl to make sure DS is not used. */
-		update_debugctlmsr(0);
+	if (unlikely(t->ds_ctx))
 		ds_free(t->ds_ctx);
-	}
 #endif /* CONFIG_X86_DS */
 
 	pfm_exit_thread();
@@ -475,7 +479,6 @@ static inline void __switch_to_xtra(struct task_struct *prev_p,
 				    struct tss_struct *tss)
 {
 	struct thread_struct *prev, *next;
-	unsigned long debugctl;
 
 	prev = &prev_p->thread,
 	next = &next_p->thread;
@@ -483,7 +486,8 @@ static inline void __switch_to_xtra(struct task_struct *prev_p,
 	if (test_tsk_thread_flag(prev_p, TIF_PERFMON_CTXSW))
 		pfm_ctxsw_out(prev_p, next_p);
 
-	debugctl = prev->debugctlmsr;
+	if (test_tsk_thread_flag(prev_p, TIF_DEBUGCTLMSR))
+		update_debugctlmsr(0, prev->debugctlval);
 
 #ifdef CONFIG_X86_DS
 	{
@@ -494,20 +498,13 @@ static inline void __switch_to_xtra(struct task_struct *prev_p,
 		if (next->ds_ctx)
 			ds_next = (unsigned long)next->ds_ctx->ds;
 
-		if (ds_next != ds_prev) {
-			/*
-			 * We clear debugctl to make sure DS
-			 * is not in use when we change it:
-			 */
-			debugctl = 0;
-			update_debugctlmsr(0);
+		if (ds_next != ds_prev)
 			wrmsrl(MSR_IA32_DS_AREA, ds_next);
-		}
 	}
 #endif /* CONFIG_X86_DS */
 
-	if (next->debugctlmsr != debugctl)
-		update_debugctlmsr(next->debugctlmsr);
+	if (test_tsk_thread_flag(next_p, TIF_DEBUGCTLMSR))
+		update_debugctlmsr(next->debugctlval, next->debugctlval);
 
 	if (test_tsk_thread_flag(next_p, TIF_PERFMON_CTXSW))
 		pfm_ctxsw_in(prev_p, next_p);
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 0a6d8c1..6c62c92 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -786,9 +786,9 @@ static int ptrace_bts_config(struct task_struct *child,
 		goto errout;
 
 	if (cfg.flags & PTRACE_BTS_O_TRACE)
-		child->thread.debugctlmsr |= bts_cfg.debugctl_mask;
+		child->thread.debugctlval |= bts_cfg.debugctl_mask;
 	else
-		child->thread.debugctlmsr &= ~bts_cfg.debugctl_mask;
+		child->thread.debugctlval &= ~bts_cfg.debugctl_mask;
 
 	if (cfg.flags & PTRACE_BTS_O_SCHED)
 		set_tsk_thread_flag(child, TIF_BTS_TRACE_TS);
@@ -798,7 +798,7 @@ static int ptrace_bts_config(struct task_struct *child,
 	error = sizeof(cfg);
 
 out:
-	if (child->thread.debugctlmsr)
+	if (child->thread.debugctlval)
 		set_tsk_thread_flag(child, TIF_DEBUGCTLMSR);
 	else
 		clear_tsk_thread_flag(child, TIF_DEBUGCTLMSR);
@@ -806,7 +806,7 @@ out:
 	return error;
 
 errout:
-	child->thread.debugctlmsr &= ~bts_cfg.debugctl_mask;
+	child->thread.debugctlval &= ~bts_cfg.debugctl_mask;
 	clear_tsk_thread_flag(child, TIF_BTS_TRACE_TS);
 	goto out;
 }
@@ -844,7 +844,7 @@ static int ptrace_bts_status(struct task_struct *child,
 		cfg.flags |= PTRACE_BTS_O_SIGNAL;
 
 	if (test_tsk_thread_flag(child, TIF_DEBUGCTLMSR) &&
-	    child->thread.debugctlmsr & bts_cfg.debugctl_mask)
+	    child->thread.debugctlval & bts_cfg.debugctl_mask)
 		cfg.flags |= PTRACE_BTS_O_TRACE;
 
 	if (test_tsk_thread_flag(child, TIF_BTS_TRACE_TS))
@@ -975,8 +975,8 @@ void ptrace_disable(struct task_struct *child)
 #ifdef CONFIG_X86_PTRACE_BTS
 	(void)ds_release_bts(child);
 
-	child->thread.debugctlmsr &= ~bts_cfg.debugctl_mask;
-	if (!child->thread.debugctlmsr)
+	child->thread.debugctlval &= ~bts_cfg.debugctl_mask;
+	if (!child->thread.debugctlval)
 		clear_tsk_thread_flag(child, TIF_DEBUGCTLMSR);
 
 	clear_tsk_thread_flag(child, TIF_BTS_TRACE_TS);
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index e8b9863..e4b35ee 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -161,17 +161,30 @@ static int enable_single_step(struct task_struct *child)
 /*
  * Install this value in MSR_IA32_DEBUGCTLMSR whenever child is running.
  */
-static void write_debugctlmsr(struct task_struct *child, unsigned long val)
+static inline void set_btf(struct task_struct *child)
 {
-	if (child->thread.debugctlmsr == val)
+	if (child->thread.debugctlval & DEBUGCTLMSR_BTF)
 		return;
 
-	child->thread.debugctlmsr = val;
+	child->thread.debugctlval |= DEBUGCTLMSR_BTF;
 
 	if (child != current)
 		return;
 
-	update_debugctlmsr(val);
+	update_debugctlmsr(child->thread.debugctlval, DEBUGCTLMSR_BTF);
+}
+
+static inline void clear_btf(struct task_struct *child)
+{
+	if (!(child->thread.debugctlval& DEBUGCTLMSR_BTF))
+		return;
+
+	 child->thread.debugctlval &= ~DEBUGCTLMSR_BTF;
+
+	if (child != current)
+		return;
+
+	update_debugctlmsr(child->thread.debugctlval, DEBUGCTLMSR_BTF);
 }
 
 /*
@@ -188,13 +201,10 @@ static void enable_step(struct task_struct *child, bool block)
 	 */
 	if (enable_single_step(child) && block) {
 		set_tsk_thread_flag(child, TIF_DEBUGCTLMSR);
-		write_debugctlmsr(child,
-				  child->thread.debugctlmsr | DEBUGCTLMSR_BTF);
+		set_btf(child);
 	} else {
-		write_debugctlmsr(child,
-				  child->thread.debugctlmsr & ~DEBUGCTLMSR_BTF);
-
-		if (!child->thread.debugctlmsr)
+		clear_btf(child);
+		if (!child->thread.debugctlval)
 			clear_tsk_thread_flag(child, TIF_DEBUGCTLMSR);
 	}
 }
@@ -214,10 +224,9 @@ void user_disable_single_step(struct task_struct *child)
 	/*
 	 * Make sure block stepping (BTF) is disabled.
 	 */
-	write_debugctlmsr(child,
-			  child->thread.debugctlmsr & ~DEBUGCTLMSR_BTF);
+	clear_btf(child);
 
-	if (!child->thread.debugctlmsr)
+	if (!child->thread.debugctlval)
 		clear_tsk_thread_flag(child, TIF_DEBUGCTLMSR);
 
 	/* Always clear TIF_SINGLESTEP... */
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 04d242a..dd9a86a 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -582,7 +582,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
 	 * The processor cleared BTF, so don't mark that we need it set.
 	 */
 	clear_tsk_thread_flag(tsk, TIF_DEBUGCTLMSR);
-	tsk->thread.debugctlmsr = 0;
+	tsk->thread.debugctlval &= ~DEBUGCTLMSR_BTF;
 
 	if (notify_die(DIE_DEBUG, "debug", regs, condition, error_code,
 						SIGTRAP) == NOTIFY_STOP)

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] x86 debugctl MSR access interface
  2008-11-07 14:44 [PATCH] x86 debugctl MSR access interface stephane eranian
@ 2008-11-10  5:58 ` Ananth N Mavinakayanahalli
  0 siblings, 0 replies; 2+ messages in thread
From: Ananth N Mavinakayanahalli @ 2008-11-10  5:58 UTC (permalink / raw)
  To: eranian
  Cc: linux-kernel, anil.s.keshavamurthy, davem, Metzger, Markus T,
	Masami Hiramatsu

On Fri, Nov 07, 2008 at 03:44:09PM +0100, stephane eranian wrote:
> Hello,
> 
> On Intel X86 processors, the DEBUGCTL msr contains lots of bits
> to control various unrelated features, such as LBR, BTS, PMU.
> 
> Nowadays, several subsystems need access to this MSR to set or
> clear the bits they care about. In 2.6.27, the new DS/BTS interface
> was introduced and DEBUGCTL can now be cleared and restored
> on context switches.
> 
> The KPROBE interface also uses DEBUGCTL.

Stephane,

I see no problems with the kprobes part of the patch.

Ananth

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2008-11-10  5:59 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-11-07 14:44 [PATCH] x86 debugctl MSR access interface stephane eranian
2008-11-10  5:58 ` Ananth N Mavinakayanahalli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).