Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753667AbbB0CeL (ORCPT ); Thu, 26 Feb 2015 21:34:11 -0500 Received: from mail-la0-f52.google.com ([209.85.215.52]:43571 "EHLO mail-la0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751287AbbB0CeK (ORCPT ); Thu, 26 Feb 2015 21:34:10 -0500 MIME-Version: 1.0 In-Reply-To: <54EFD4A2.2000702@huawei.com> References: <1424929779-13174-1-git-send-email-wangnan0@huawei.com> <54EFD4A2.2000702@huawei.com> From: Andy Lutomirski Date: Thu, 26 Feb 2015 18:33:47 -0800 Message-ID: Subject: Re: [tip:x86/asm] x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP To: Wang Nan Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Dave Hansen , Masami Hiramatsu , "linux-kernel@vger.kernel.org" , Oleg Nesterov , Li Zefan , Steven Rostedt , "linux-tip-commits@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3102 Lines: 83 On Thu, Feb 26, 2015 at 6:21 PM, Wang Nan wrote: > On 2015/2/26 23:12, Andy Lutomirski wrote: >> On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan wrote: >>> Commit-ID: b4d8327024637cb2a1f7910dcb5d0ad7a096f473 >>> Gitweb: http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473 >>> Author: Wang Nan >>> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800 >>> Committer: Ingo Molnar >>> CommitDate: Thu, 26 Feb 2015 12:29:20 +0100 >>> >>> x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP >>> >>> Before this patch early_trap_init() installs DEBUG_STACK for >>> X86_TRAP_BP and X86_TRAP_DB. However, DEBUG_STACK doesn't work >>> correctly until cpu_init() <-- trap_init(). >>> >>> This patch passes 0 to set_intr_gate_ist() and >>> set_system_intr_gate_ist() instead of DEBUG_STACK to let it use >>> same stack as kernel, and installs DEBUG_STACK for them in >>> trap_init(). >>> >> >> Sorry, I'm late to the party. This patch, while technically correct >> AFAICT, is really ugly, because the whole point is that it *doesn't* >> use ist. In other words: >> >>> + set_intr_gate_ist(X86_TRAP_DB, &debug, 0); >> >> That should be set_intr_gate(X86_TRAP_DB, &debug); >> > > I considered this, but found that set_intr_gate() and set_intr_gate_ist(..., 0) > behaviors differently: set_intr_gate() -> _trace_set_gate() requires 'trace_##addr' > to be installed to trace_idt_table, while set_intr_gate_ist(..., 0) puts 'addr'. > into trace_idt_table() through _set_gate(). > Therefore, if we want to use set_intr_gate() we need to provide a trace_debug entry > for it, which will be overwritten in trap_init() so it is in fact a useless symbol. Yuck. Then IMO we should have a separate helper for this. Better yet, we should rename set_intr_gate, because this distinction could easily be overlooked. > > Anyway, I'd like to post a v3 patch follow your advise. Please keep an eye on it. > > Thank you! > >>> /* int3 can be called from all */ >>> - set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK); >>> + set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0); >> >> Similarly, this should be set_system_gate. >> >>> #ifdef CONFIG_X86_32 >>> set_intr_gate(X86_TRAP_PF, page_fault); >>> #endif >>> @@ -1005,6 +1013,15 @@ void __init trap_init(void) >>> */ >>> cpu_init(); >>> >>> + /* >>> + * X86_TRAP_DB and X86_TRAP_BP have been set >>> + * in early_trap_init(). However, DEBUG_STACK works only after >>> + * cpu_init() loads TSS. See comments in early_trap_init(). >> >> It's not that DEBUG_STACK only works after the TSS is loaded; it's >> that the IST mechanism only works after TSS is loaded. >> >> --Andy >> > > -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/