* [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled
2017-11-28 14:35 7% Suspend-to-ram/resume regression with commit ca37e57bbe0c Jarkko Nikula
@ 2017-11-29 7:09 4% ` Ingo Molnar
0 siblings, 0 replies; 2+ results
From: Ingo Molnar @ 2017-11-29 7:09 UTC (permalink / raw)
To: Jarkko Nikula
Cc: linux-kernel, Andy Lutomirski, Thomas Gleixner, Peter Zijlstra,
Linus Torvalds, Borislav Petkov
* Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> Hi
>
> Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it to
> commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
> native_load_gs_index()").
>
> I noticed it on Intel Kabylake (core) and Apollolake (atom) based prototype
> machines. Symptoms are that machine appears to enter into suspend but
> resumes instantly and hangs. Unfortunately no logs.
>
> If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.
Hm, that commit looks broken with irq-tracing enabled.
Does the patch below fix it?
In fact the exception handler itself appears to have broken GS handling as well -
I suspect it never triggers in practice, because it was broken forever.
Andy, do you concur?
On a related note, we should definitely extend the 'intended GS state' annotation
comments I did in this patch to all SWAPGS instances - this way code review has a
much higher chance of finding discrepancies between intent and actual code.
Thanks,
Ingo
=================>
>From 769dbd33a272214c48c0fc5a17bed9c1597e222f Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 29 Nov 2017 07:43:27 +0100
Subject: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled
Jarkko Nikula reported a S2R resume hang regression and bisected it back to:
ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
Turns out the GS handling of that patch is wrong: when IRQ state tracing is
enabled it calls a kernel function (as part of the TRACE_IRQS_*() functionality),
but we have not switched to the kernel GS yet ...
Fix the SWAPGS handling and also annotate every affected SWAPGS
instance to document the intended state of GS.
Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Bisected-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/0fede9f9-88b0-a6e7-1027-dfb2019b8ef2@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/entry/entry_64.S | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f81d50d7ceac..c0b52df8ee4f 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -945,16 +945,16 @@ idtentry simd_coprocessor_error do_simd_coprocessor_error has_error_code=0
*/
ENTRY(native_load_gs_index)
FRAME_BEGIN
+ SWAPGS /* switch from user GS to kernel GS */
pushfq
DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
TRACE_IRQS_OFF
- SWAPGS
.Lgs_change:
movl %edi, %gs
2: ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
- SWAPGS
TRACE_IRQS_FLAGS (%rsp)
popfq
+ SWAPGS /* switch from kernel GS to user GS */
FRAME_END
ret
ENDPROC(native_load_gs_index)
@@ -964,7 +964,7 @@ EXPORT_SYMBOL(native_load_gs_index)
.section .fixup, "ax"
/* running with kernelgs */
bad_gs:
- SWAPGS /* switch back to user gs */
+ SWAPGS /* switch back to user GS, to modify GS */
.macro ZAP_GS
/* This can't be a string because the preprocessor needs to see it. */
movl $__USER_DS, %eax
@@ -973,6 +973,7 @@ EXPORT_SYMBOL(native_load_gs_index)
ALTERNATIVE "", "ZAP_GS", X86_BUG_NULL_SEG
xorl %eax, %eax
movl %eax, %gs
+ SWAPGS /* switch to kernel GS again before continuing */
jmp 2b
.previous
^ permalink raw reply related [relevance 4%]
* Suspend-to-ram/resume regression with commit ca37e57bbe0c
@ 2017-11-28 14:35 7% Jarkko Nikula
2017-11-29 7:09 4% ` [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled Ingo Molnar
0 siblings, 1 reply; 2+ results
From: Jarkko Nikula @ 2017-11-28 14:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Andy Lutomirski, Ingo Molnar
Hi
Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it
to commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
native_load_gs_index()").
I noticed it on Intel Kabylake (core) and Apollolake (atom) based
prototype machines. Symptoms are that machine appears to enter into
suspend but resumes instantly and hangs. Unfortunately no logs.
If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.
--
Jarkko
^ permalink raw reply [relevance 7%]
Results 1-2 of 2 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2017-11-28 14:35 7% Suspend-to-ram/resume regression with commit ca37e57bbe0c Jarkko Nikula
2017-11-29 7:09 4% ` [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled Ingo Molnar
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).