lkml.org 
[lkml]   [2015]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [tip:x86/asm] x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP
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


\
 
 \ /
  Last update: 2015-02-27 03:41    [W:0.075 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site