[PATCH v2] x86, traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP.

[PATCH v2] x86, traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP.

[Wang Nan]

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().

As core runs at ring 0 between early_trap_init() and trap_init(), there
is no chance to get a bad stack before trap_init().

As NMI is also enabled in trap_init(), we don't need to care about
is_debug_stack() and related things used in arch/x86/kernel/nmi.c.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
v1 -> v2: Correct grammar issues in comments.
---
 arch/x86/kernel/traps.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9d2073e..4281988 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,9 +925,17 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 /* Set of traps needed for early debugging. */
 void __init early_trap_init(void)
 {
-	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
+	/*
+	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
+	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
+	 * runs at ring 0 so it is impossible to hit an invalid stack.
+	 * Using the original stack works well enough at this early
+	 * stage. DEBUG_STACK will be equipped after cpu_init() in
+	 * trap_init().
+	 */
+	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
 	/* 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);
 #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().
+	 */
+	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
+	/* int3 can be called from all */
+	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
+
 	x86_init.irqs.trap_init();
 
 #ifdef CONFIG_X86_64
-- 
1.8.4

[tip:x86/asm] x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP

[tip-bot for Wang Nan]

Commit-ID:  b4d8327024637cb2a1f7910dcb5d0ad7a096f473
Gitweb:     http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
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().

As core runs at ring 0 between early_trap_init() and
trap_init(), there is no chance to get a bad stack before
trap_init().

As NMI is also enabled in trap_init(), we don't need to care
about is_debug_stack() and related things used in
arch/x86/kernel/nmi.c.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Cc: <dave.hansen@linux.intel.com>
Cc: <lizefan@huawei.com>
Cc: <luto@amacapital.net>
Cc: <oleg@redhat.com>
Link: http://lkml.kernel.org/r/1424929779-13174-1-git-send-email-wangnan0@huawei.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/traps.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9d2073e..4281988 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,9 +925,17 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 /* Set of traps needed for early debugging. */
 void __init early_trap_init(void)
 {
-	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
+	/*
+	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
+	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
+	 * runs at ring 0 so it is impossible to hit an invalid stack.
+	 * Using the original stack works well enough at this early
+	 * stage. DEBUG_STACK will be equipped after cpu_init() in
+	 * trap_init().
+	 */
+	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
 	/* 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);
 #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().
+	 */
+	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
+	/* int3 can be called from all */
+	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
+
 	x86_init.irqs.trap_init();
 
 #ifdef CONFIG_X86_64

Re: [tip:x86/asm] x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP

[Andy Lutomirski]

On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan <tipbot@zytor.com> wrote:
> Commit-ID:  b4d8327024637cb2a1f7910dcb5d0ad7a096f473
> Gitweb:     http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
> Author:     Wang Nan <wangnan0@huawei.com>
> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
> Committer:  Ingo Molnar <mingo@kernel.org>
> 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);

>         /* 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

Re: [tip:x86/asm] x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP

[Wang Nan]

On 2015/2/26 23:12, Andy Lutomirski wrote:
> On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan <tipbot@zytor.com> wrote:
>> Commit-ID:  b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>> Gitweb:     http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>> Author:     Wang Nan <wangnan0@huawei.com>
>> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
>> Committer:  Ingo Molnar <mingo@kernel.org>
>> 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.

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
> 

Re: [tip:x86/asm] x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP

[Andy Lutomirski]

On Thu, Feb 26, 2015 at 6:21 PM, Wang Nan <wangnan0@huawei.com> wrote:
> On 2015/2/26 23:12, Andy Lutomirski wrote:
>> On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan <tipbot@zytor.com> wrote:
>>> Commit-ID:  b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>>> Gitweb:     http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>>> Author:     Wang Nan <wangnan0@huawei.com>
>>> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
>>> Committer:  Ingo Molnar <mingo@kernel.org>
>>> 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

[PATCH] x86, traps: early_trap_init() cleanup.

[Wang Nan]

As early_trap_init() doesn't use IST, replace set_intr_gate_ist(..., 0)
and set_system_intr_gate_ist(..., 0) with their standard counterparts.

set_intr_gate() requires a trace_debug symbol which we don't have and
won't use. Use a small macro trick as a workaround.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---

Hi Andy Lutomirski,

   This patch is a bit tricky, but I think we don't need to define another
   helper for such a small problem. What's your opinion?

Thank you!

---
 arch/x86/kernel/traps.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4281988..c24434a 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,17 +925,22 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 /* Set of traps needed for early debugging. */
 void __init early_trap_init(void)
 {
+
 	/*
-	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
-	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
-	 * runs at ring 0 so it is impossible to hit an invalid stack.
-	 * Using the original stack works well enough at this early
-	 * stage. DEBUG_STACK will be equipped after cpu_init() in
-	 * trap_init().
+	 * Don't use IST to set DEBUG_STACK as it doesn't work until TSS is
+	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU runs at
+	 * ring 0 so it is impossible to hit an invalid stack.  Using the
+	 * original stack works well enough at this early stage. DEBUG_STACK
+	 * will be equipped after cpu_init() in trap_init().
+	 *
+	 * Since set_intr_gate() needs a trace_debug but we don't have it,
+	 * use the following #define as a workaround.
 	 */
-	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
+#define trace_debug debug
+	set_intr_gate(X86_TRAP_DB, debug);
+#undef trace_debug
 	/* int3 can be called from all */
-	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
+	set_system_intr_gate(X86_TRAP_BP, &int3);
 #ifdef CONFIG_X86_32
 	set_intr_gate(X86_TRAP_PF, page_fault);
 #endif
@@ -1015,7 +1020,7 @@ void __init trap_init(void)
 
 	/*
 	 * X86_TRAP_DB and X86_TRAP_BP have been set
-	 * in early_trap_init(). However, DEBUG_STACK works only after
+	 * in early_trap_init(). However, ITS works only after
 	 * cpu_init() loads TSS. See comments in early_trap_init().
 	 */
 	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
-- 
1.8.4

[PATCH v2] x86, traps: separate set_intr_gate() and cleanup early_trap_init().

[Wang Nan]

As early_trap_init() doesn't use IST, replace set_intr_gate_ist() and
set_system_intr_gate_ist() with their standard counterparts.

set_intr_gate() requires a trace_debug symbol which we don't have and
won't use. This patch seprates set_intr_gate() into 2 parts, and uses
base version in early_trap_init().

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---

Hi Andy Lutomirski,

   This patch should be less tricky than previous one [1]. I also tried
   to renaming all set_intr_gate(), but it causes too many code changes,
   so I think you will be satisfied with this one.

Thank you!

[1] https://lkml.org/lkml/2015/2/26/770

---
 arch/x86/include/asm/desc.h |  7 ++++++-
 arch/x86/kernel/traps.c     | 20 ++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index a94b82e..a0bf89f 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -376,11 +376,16 @@ static inline void _set_gate(int gate, unsigned type, void *addr,
  * Pentium F0 0F bugfix can have resulted in the mapped
  * IDT being write-protected.
  */
-#define set_intr_gate(n, addr)						\
+#define set_intr_gate_notrace(n, addr)					\
 	do {								\
 		BUG_ON((unsigned)n > 0xFF);				\
 		_set_gate(n, GATE_INTERRUPT, (void *)addr, 0, 0,	\
 			  __KERNEL_CS);					\
+	} while (0)
+
+#define set_intr_gate(n, addr)						\
+	do {								\
+		set_intr_gate_notrace(n, addr);				\
 		_trace_set_gate(n, GATE_INTERRUPT, (void *)trace_##addr,\
 				0, 0, __KERNEL_CS);			\
 	} while (0)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4281988..9965bd1 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -926,16 +926,20 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 void __init early_trap_init(void)
 {
 	/*
-	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
-	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
-	 * runs at ring 0 so it is impossible to hit an invalid stack.
-	 * Using the original stack works well enough at this early
-	 * stage. DEBUG_STACK will be equipped after cpu_init() in
+	 * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
+	 * is ready in cpu_init() <-- trap_init(). Before trap_init(),
+	 * CPU runs at ring 0 so it is impossible to hit an invalid
+	 * stack.  Using the original stack works well enough at this
+	 * early stage. DEBUG_STACK will be equipped after cpu_init() in
 	 * trap_init().
+	 *
+	 * We don't need to set trace_idt_table like set_intr_gate(),
+	 * since we don't have trace_debug and it will be reset to
+	 * 'debug' in trap_init() by set_intr_gate_ist().
 	 */
-	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
+	set_intr_gate_notrace(X86_TRAP_DB, debug);
 	/* int3 can be called from all */
-	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
+	set_system_intr_gate(X86_TRAP_BP, &int3);
 #ifdef CONFIG_X86_32
 	set_intr_gate(X86_TRAP_PF, page_fault);
 #endif
@@ -1015,7 +1019,7 @@ void __init trap_init(void)
 
 	/*
 	 * X86_TRAP_DB and X86_TRAP_BP have been set
-	 * in early_trap_init(). However, DEBUG_STACK works only after
+	 * in early_trap_init(). However, ITS works only after
 	 * cpu_init() loads TSS. See comments in early_trap_init().
 	 */
 	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
-- 
1.8.4

Re: [PATCH] x86, traps: early_trap_init() cleanup.

[Borislav Petkov]

On Fri, Feb 27, 2015 at 11:28:50AM +0800, Wang Nan wrote:
> As early_trap_init() doesn't use IST, replace set_intr_gate_ist(..., 0)
> and set_system_intr_gate_ist(..., 0) with their standard counterparts.
> 
> set_intr_gate() requires a trace_debug symbol which we don't have and
> won't use. Use a small macro trick as a workaround.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
> 
> Hi Andy Lutomirski,
> 
>    This patch is a bit tricky, but I think we don't need to define another
>    helper for such a small problem. What's your opinion?
> 
> Thank you!
> 
> ---
>  arch/x86/kernel/traps.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4281988..c24434a 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -925,17 +925,22 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
>  /* Set of traps needed for early debugging. */
>  void __init early_trap_init(void)
>  {
> +
>  	/*
> -	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
> -	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
> -	 * runs at ring 0 so it is impossible to hit an invalid stack.
> -	 * Using the original stack works well enough at this early
> -	 * stage. DEBUG_STACK will be equipped after cpu_init() in
> -	 * trap_init().
> +	 * Don't use IST to set DEBUG_STACK as it doesn't work until TSS is
> +	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU runs at
> +	 * ring 0 so it is impossible to hit an invalid stack.  Using the
> +	 * original stack works well enough at this early stage. DEBUG_STACK
> +	 * will be equipped after cpu_init() in trap_init().
> +	 *
> +	 * Since set_intr_gate() needs a trace_debug but we don't have it,
> +	 * use the following #define as a workaround.
>  	 */
> -	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
> +#define trace_debug debug

This goes to arch/x86/include/asm/traps.h like the rest of the trace_*
defines.

> +	set_intr_gate(X86_TRAP_DB, debug);
> +#undef trace_debug
>  	/* int3 can be called from all */
> -	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
> +	set_system_intr_gate(X86_TRAP_BP, &int3);
>  #ifdef CONFIG_X86_32
>  	set_intr_gate(X86_TRAP_PF, page_fault);
>  #endif

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2] x86, traps: separate set_intr_gate() and cleanup early_trap_init().

[Wang Nan]

Hi Andy Lutomirski,

Do you have any comment on this patch?

Thank you.

On 2015/2/27 12:19, Wang Nan wrote:
> As early_trap_init() doesn't use IST, replace set_intr_gate_ist() and
> set_system_intr_gate_ist() with their standard counterparts.
> 
> set_intr_gate() requires a trace_debug symbol which we don't have and
> won't use. This patch seprates set_intr_gate() into 2 parts, and uses
> base version in early_trap_init().
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
> 
> Hi Andy Lutomirski,
> 
>    This patch should be less tricky than previous one [1]. I also tried
>    to renaming all set_intr_gate(), but it causes too many code changes,
>    so I think you will be satisfied with this one.
> 
> Thank you!
> 
> [1] https://lkml.org/lkml/2015/2/26/770
> 
> ---
>  arch/x86/include/asm/desc.h |  7 ++++++-
>  arch/x86/kernel/traps.c     | 20 ++++++++++++--------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index a94b82e..a0bf89f 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -376,11 +376,16 @@ static inline void _set_gate(int gate, unsigned type, void *addr,
>   * Pentium F0 0F bugfix can have resulted in the mapped
>   * IDT being write-protected.
>   */
> -#define set_intr_gate(n, addr)						\
> +#define set_intr_gate_notrace(n, addr)					\
>  	do {								\
>  		BUG_ON((unsigned)n > 0xFF);				\
>  		_set_gate(n, GATE_INTERRUPT, (void *)addr, 0, 0,	\
>  			  __KERNEL_CS);					\
> +	} while (0)
> +
> +#define set_intr_gate(n, addr)						\
> +	do {								\
> +		set_intr_gate_notrace(n, addr);				\
>  		_trace_set_gate(n, GATE_INTERRUPT, (void *)trace_##addr,\
>  				0, 0, __KERNEL_CS);			\
>  	} while (0)
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4281988..9965bd1 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -926,16 +926,20 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
>  void __init early_trap_init(void)
>  {
>  	/*
> -	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
> -	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
> -	 * runs at ring 0 so it is impossible to hit an invalid stack.
> -	 * Using the original stack works well enough at this early
> -	 * stage. DEBUG_STACK will be equipped after cpu_init() in
> +	 * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
> +	 * is ready in cpu_init() <-- trap_init(). Before trap_init(),
> +	 * CPU runs at ring 0 so it is impossible to hit an invalid
> +	 * stack.  Using the original stack works well enough at this
> +	 * early stage. DEBUG_STACK will be equipped after cpu_init() in
>  	 * trap_init().
> +	 *
> +	 * We don't need to set trace_idt_table like set_intr_gate(),
> +	 * since we don't have trace_debug and it will be reset to
> +	 * 'debug' in trap_init() by set_intr_gate_ist().
>  	 */
> -	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
> +	set_intr_gate_notrace(X86_TRAP_DB, debug);
>  	/* int3 can be called from all */
> -	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
> +	set_system_intr_gate(X86_TRAP_BP, &int3);
>  #ifdef CONFIG_X86_32
>  	set_intr_gate(X86_TRAP_PF, page_fault);
>  #endif
> @@ -1015,7 +1019,7 @@ void __init trap_init(void)
>  
>  	/*
>  	 * X86_TRAP_DB and X86_TRAP_BP have been set
> -	 * in early_trap_init(). However, DEBUG_STACK works only after
> +	 * in early_trap_init(). However, ITS works only after
>  	 * cpu_init() loads TSS. See comments in early_trap_init().
>  	 */
>  	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
> 

Re: [PATCH v2] x86, traps: separate set_intr_gate() and cleanup early_trap_init().

[Andy Lutomirski]

On Thu, Feb 26, 2015 at 8:19 PM, Wang Nan <wangnan0@huawei.com> wrote:
> As early_trap_init() doesn't use IST, replace set_intr_gate_ist() and
> set_system_intr_gate_ist() with their standard counterparts.
>
> set_intr_gate() requires a trace_debug symbol which we don't have and
> won't use. This patch seprates set_intr_gate() into 2 parts, and uses
> base version in early_trap_init().
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>

Looks good to me.  Ingo?

--Andy

> ---
>
> Hi Andy Lutomirski,
>
>    This patch should be less tricky than previous one [1]. I also tried
>    to renaming all set_intr_gate(), but it causes too many code changes,
>    so I think you will be satisfied with this one.
>
> Thank you!
>
> [1] https://lkml.org/lkml/2015/2/26/770
>
> ---
>  arch/x86/include/asm/desc.h |  7 ++++++-
>  arch/x86/kernel/traps.c     | 20 ++++++++++++--------
>  2 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index a94b82e..a0bf89f 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -376,11 +376,16 @@ static inline void _set_gate(int gate, unsigned type, void *addr,
>   * Pentium F0 0F bugfix can have resulted in the mapped
>   * IDT being write-protected.
>   */
> -#define set_intr_gate(n, addr)                                         \
> +#define set_intr_gate_notrace(n, addr)                                 \
>         do {                                                            \
>                 BUG_ON((unsigned)n > 0xFF);                             \
>                 _set_gate(n, GATE_INTERRUPT, (void *)addr, 0, 0,        \
>                           __KERNEL_CS);                                 \
> +       } while (0)
> +
> +#define set_intr_gate(n, addr)                                         \
> +       do {                                                            \
> +               set_intr_gate_notrace(n, addr);                         \
>                 _trace_set_gate(n, GATE_INTERRUPT, (void *)trace_##addr,\
>                                 0, 0, __KERNEL_CS);                     \
>         } while (0)
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4281988..9965bd1 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -926,16 +926,20 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
>  void __init early_trap_init(void)
>  {
>         /*
> -        * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
> -        * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
> -        * runs at ring 0 so it is impossible to hit an invalid stack.
> -        * Using the original stack works well enough at this early
> -        * stage. DEBUG_STACK will be equipped after cpu_init() in
> +        * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
> +        * is ready in cpu_init() <-- trap_init(). Before trap_init(),
> +        * CPU runs at ring 0 so it is impossible to hit an invalid
> +        * stack.  Using the original stack works well enough at this
> +        * early stage. DEBUG_STACK will be equipped after cpu_init() in
>          * trap_init().
> +        *
> +        * We don't need to set trace_idt_table like set_intr_gate(),
> +        * since we don't have trace_debug and it will be reset to
> +        * 'debug' in trap_init() by set_intr_gate_ist().
>          */
> -       set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
> +       set_intr_gate_notrace(X86_TRAP_DB, debug);
>         /* int3 can be called from all */
> -       set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
> +       set_system_intr_gate(X86_TRAP_BP, &int3);
>  #ifdef CONFIG_X86_32
>         set_intr_gate(X86_TRAP_PF, page_fault);
>  #endif
> @@ -1015,7 +1019,7 @@ void __init trap_init(void)
>
>         /*
>          * X86_TRAP_DB and X86_TRAP_BP have been set
> -        * in early_trap_init(). However, DEBUG_STACK works only after
> +        * in early_trap_init(). However, ITS works only after
>          * cpu_init() loads TSS. See comments in early_trap_init().
>          */
>         set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
> --
> 1.8.4
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[tip:x86/asm] x86/traps: Separate set_intr_gate() and clean up early_trap_init()

[tip-bot for Wang Nan]

Commit-ID:  5eca7453d61003bf886992388f8cb407e6f0d051
Gitweb:     http://git.kernel.org/tip/5eca7453d61003bf886992388f8cb407e6f0d051
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Fri, 27 Feb 2015 12:19:49 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 5 Mar 2015 00:47:29 +0100

x86/traps: Separate set_intr_gate() and clean up early_trap_init()

As early_trap_init() doesn't use IST, replace
set_intr_gate_ist() and set_system_intr_gate_ist() with their
standard counterparts.

set_intr_gate() requires a trace_debug symbol which we don't
have and won't use. This patch separates set_intr_gate() into two
parts, and uses base version in early_trap_init().

Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Wang Nan <wangnan0@huawei.com>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Cc: <dave.hansen@linux.intel.com>
Cc: <lizefan@huawei.com>
Cc: <masami.hiramatsu.pt@hitachi.com>
Cc: <oleg@redhat.com>
Cc: <rostedt@goodmis.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1425010789-13714-1-git-send-email-wangnan0@huawei.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/desc.h |  7 ++++++-
 arch/x86/kernel/traps.c     | 20 ++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index a94b82e..a0bf89f 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -376,11 +376,16 @@ static inline void _set_gate(int gate, unsigned type, void *addr,
  * Pentium F0 0F bugfix can have resulted in the mapped
  * IDT being write-protected.
  */
-#define set_intr_gate(n, addr)						\
+#define set_intr_gate_notrace(n, addr)					\
 	do {								\
 		BUG_ON((unsigned)n > 0xFF);				\
 		_set_gate(n, GATE_INTERRUPT, (void *)addr, 0, 0,	\
 			  __KERNEL_CS);					\
+	} while (0)
+
+#define set_intr_gate(n, addr)						\
+	do {								\
+		set_intr_gate_notrace(n, addr);				\
 		_trace_set_gate(n, GATE_INTERRUPT, (void *)trace_##addr,\
 				0, 0, __KERNEL_CS);			\
 	} while (0)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4281988..9965bd1 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -926,16 +926,20 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 void __init early_trap_init(void)
 {
 	/*
-	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
-	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
-	 * runs at ring 0 so it is impossible to hit an invalid stack.
-	 * Using the original stack works well enough at this early
-	 * stage. DEBUG_STACK will be equipped after cpu_init() in
+	 * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
+	 * is ready in cpu_init() <-- trap_init(). Before trap_init(),
+	 * CPU runs at ring 0 so it is impossible to hit an invalid
+	 * stack.  Using the original stack works well enough at this
+	 * early stage. DEBUG_STACK will be equipped after cpu_init() in
 	 * trap_init().
+	 *
+	 * We don't need to set trace_idt_table like set_intr_gate(),
+	 * since we don't have trace_debug and it will be reset to
+	 * 'debug' in trap_init() by set_intr_gate_ist().
 	 */
-	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
+	set_intr_gate_notrace(X86_TRAP_DB, debug);
 	/* int3 can be called from all */
-	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
+	set_system_intr_gate(X86_TRAP_BP, &int3);
 #ifdef CONFIG_X86_32
 	set_intr_gate(X86_TRAP_PF, page_fault);
 #endif
@@ -1015,7 +1019,7 @@ void __init trap_init(void)
 
 	/*
 	 * X86_TRAP_DB and X86_TRAP_BP have been set
-	 * in early_trap_init(). However, DEBUG_STACK works only after
+	 * in early_trap_init(). However, ITS works only after
 	 * cpu_init() loads TSS. See comments in early_trap_init().
 	 */
 	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);