From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758744AbZE2JCX (ORCPT ); Fri, 29 May 2009 05:02:23 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1758644AbZE2JBx (ORCPT ); Fri, 29 May 2009 05:01:53 -0400 Received: from e28smtp06.in.ibm.com ([59.145.155.6]:43728 "EHLO e28smtp06.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757344AbZE2JBu (ORCPT ); Fri, 29 May 2009 05:01:50 -0400 Date: Fri, 29 May 2009 14:31:46 +0530 From: "K.Prasad" To: David Gibson , Alan Stern , Frederic Weisbecker Cc: Steven Rostedt , Ingo Molnar , Linux Kernel Mailing List , Andrew Morton , Benjamin Herrenschmidt , maneesh@linux.vnet.ibm.com, Roland McGrath , Masami Hiramatsu Subject: Re: [Patch 06/12] Use the new wrapper routines to access debug registers in process/thread code Message-ID: <20090529090146.GA5353@in.ibm.com> Reply-To: prasad@linux.vnet.ibm.com References: <20090511114422.133566343@prasadkr_t60p.in.ibm.com> <20090511115344.GG25673@in.ibm.com> <20090528064238.GC3091@yookeroo.seuss> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090528064238.GC3091@yookeroo.seuss> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 28, 2009 at 04:42:38PM +1000, David Gibson wrote: > On Mon, May 11, 2009 at 05:23:44PM +0530, K.Prasad wrote: > > From: Alan Stern > > > > This patch enables the use of abstract debug registers in > > process-handling routines. > > [snip] > > > > + p->thread.io_bitmap_ptr = NULL; > > Why is manipulating the io_bitmap_ptr relevant to debug register > handling? I *re-read* the patch but was unable to find how this change had sneaked in. It shouldn't be there although it is harmless. Hi Frederic, I am attaching a new version of this patch 06/12 that: - removes the line that assigns NULL to "p->thread.io_bitmap_ptr" - Updates the comment in __switch_to() function which was stale (was relevant when 'last_debugged_task' was used to detect lazy debug register switching). Kindly integrate this version in lieu of the older version sent here: http://lkml.org/lkml/2009/5/21/149. Hi Alan, I'm retaining the "Reviewed-by: Alan Stern " tag on the patch even after the above changes and guess it is acceptable to you. Please let me know if you have any concerns about that. Thanks, K.Prasad Use the new wrapper routines to access debug registers in process/thread code This patch enables the use of abstract debug registers in process-handling routines. Original-patch-by: Alan Stern Signed-off-by: K.Prasad Reviewed-by: Alan Stern --- arch/x86/kernel/process.c | 23 ++++++----------------- arch/x86/kernel/process_32.c | 27 +++++++++++++++++++++++++++ arch/x86/kernel/process_64.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 17 deletions(-) Index: linux-2.6-tip.hbkpt/arch/x86/kernel/process.c =================================================================== --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/process.c +++ linux-2.6-tip.hbkpt/arch/x86/kernel/process.c @@ -17,6 +17,8 @@ #include #include #include +#include +#include unsigned long idle_halt; EXPORT_SYMBOL(idle_halt); @@ -48,6 +50,8 @@ void free_thread_xstate(struct task_stru kmem_cache_free(task_xstate_cachep, tsk->thread.xstate); tsk->thread.xstate = NULL; } + if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG))) + flush_thread_hw_breakpoint(tsk); WARN(tsk->thread.ds_ctx, "leaking DS context\n"); } @@ -106,14 +110,9 @@ void flush_thread(void) } #endif - clear_tsk_thread_flag(tsk, TIF_DEBUG); + if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG))) + flush_thread_hw_breakpoint(tsk); - tsk->thread.debugreg0 = 0; - tsk->thread.debugreg1 = 0; - tsk->thread.debugreg2 = 0; - tsk->thread.debugreg3 = 0; - tsk->thread.debugreg6 = 0; - tsk->thread.debugreg7 = 0; memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array)); /* * Forget coprocessor state.. @@ -195,16 +194,6 @@ void __switch_to_xtra(struct task_struct else if (next->debugctlmsr != prev->debugctlmsr) update_debugctlmsr(next->debugctlmsr); - if (test_tsk_thread_flag(next_p, TIF_DEBUG)) { - set_debugreg(next->debugreg0, 0); - set_debugreg(next->debugreg1, 1); - set_debugreg(next->debugreg2, 2); - set_debugreg(next->debugreg3, 3); - /* no 4 and 5 */ - set_debugreg(next->debugreg6, 6); - set_debugreg(next->debugreg7, 7); - } - if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^ test_tsk_thread_flag(next_p, TIF_NOTSC)) { /* prev and next are different */ Index: linux-2.6-tip.hbkpt/arch/x86/kernel/process_32.c =================================================================== --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/process_32.c +++ linux-2.6-tip.hbkpt/arch/x86/kernel/process_32.c @@ -58,6 +58,8 @@ #include #include #include +#include +#include asmlinkage void ret_from_fork(void) __asm__("ret_from_fork"); @@ -263,6 +265,11 @@ int copy_thread(unsigned long clone_flag task_user_gs(p) = get_user_gs(regs); tsk = current; + err = -ENOMEM; + if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG))) + if (copy_thread_hw_breakpoint(tsk, p, clone_flags)) + goto out; + if (unlikely(test_tsk_thread_flag(tsk, TIF_IO_BITMAP))) { p->thread.io_bitmap_ptr = kmemdup(tsk->thread.io_bitmap_ptr, IO_BITMAP_BYTES, GFP_KERNEL); @@ -282,10 +289,13 @@ int copy_thread(unsigned long clone_flag err = do_set_thread_area(p, -1, (struct user_desc __user *)childregs->si, 0); +out: if (err && p->thread.io_bitmap_ptr) { kfree(p->thread.io_bitmap_ptr); p->thread.io_bitmap_max = 0; } + if (err) + flush_thread_hw_breakpoint(p); clear_tsk_thread_flag(p, TIF_DS_AREA_MSR); p->thread.ds_ctx = NULL; @@ -424,6 +434,23 @@ __switch_to(struct task_struct *prev_p, lazy_load_gs(next->gs); percpu_write(current_task, next_p); + /* + * There's a problem with moving the arch_install_thread_hw_breakpoint() + * call before current is updated. Suppose a kernel breakpoint is + * triggered in between the two, the hw-breakpoint handler will see that + * the 'current' task does not have TIF_DEBUG flag set and will think it + * is leftover from an old task (lazy switching) and will erase it. Then + * until the next context switch, no user-breakpoints will be installed. + * + * The real problem is that it's impossible to update both current and + * physical debug registers at the same instant, so there will always be + * a window in which they disagree and a breakpoint might get triggered. + * Since we use lazy switching, we are forced to assume that a + * disagreement means that current is correct and the exception is due + * to lazy debug register switching. + */ + if (unlikely(test_tsk_thread_flag(next_p, TIF_DEBUG))) + arch_install_thread_hw_breakpoint(next_p); return prev_p; } Index: linux-2.6-tip.hbkpt/arch/x86/kernel/process_64.c =================================================================== --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/process_64.c +++ linux-2.6-tip.hbkpt/arch/x86/kernel/process_64.c @@ -52,6 +52,8 @@ #include #include #include +#include +#include asmlinkage extern void ret_from_fork(void); @@ -245,6 +247,8 @@ void release_thread(struct task_struct * BUG(); } } + if (unlikely(dead_task->thread.debugreg7)) + flush_thread_hw_breakpoint(dead_task); } static inline void set_32bit_tls(struct task_struct *t, int tls, u32 addr) @@ -306,6 +310,11 @@ int copy_thread(unsigned long clone_flag savesegment(es, p->thread.es); savesegment(ds, p->thread.ds); + err = -ENOMEM; + if (unlikely(test_tsk_thread_flag(me, TIF_DEBUG))) + if (copy_thread_hw_breakpoint(me, p, clone_flags)) + goto out; + if (unlikely(test_tsk_thread_flag(me, TIF_IO_BITMAP))) { p->thread.io_bitmap_ptr = kmalloc(IO_BITMAP_BYTES, GFP_KERNEL); if (!p->thread.io_bitmap_ptr) { @@ -344,6 +353,9 @@ out: kfree(p->thread.io_bitmap_ptr); p->thread.io_bitmap_max = 0; } + if (err) + flush_thread_hw_breakpoint(p); + return err; } @@ -489,6 +501,24 @@ __switch_to(struct task_struct *prev_p, */ if (tsk_used_math(next_p) && next_p->fpu_counter > 5) math_state_restore(); + /* + * There's a problem with moving the arch_install_thread_hw_breakpoint() + * call before current is updated. Suppose a kernel breakpoint is + * triggered in between the two, the hw-breakpoint handler will see that + * the 'current' task does not have TIF_DEBUG flag set and will think it + * is leftover from an old task (lazy switching) and will erase it. Then + * until the next context switch, no user-breakpoints will be installed. + * + * The real problem is that it's impossible to update both current and + * physical debug registers at the same instant, so there will always be + * a window in which they disagree and a breakpoint might get triggered. + * Since we use lazy switching, we are forced to assume that a + * disagreement means that current is correct and the exception is due + * to lazy debug register switching. + */ + if (unlikely(test_tsk_thread_flag(next_p, TIF_DEBUG))) + arch_install_thread_hw_breakpoint(next_p); + return prev_p; }