From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752524AbdK2HJ5 (ORCPT ); Wed, 29 Nov 2017 02:09:57 -0500 Received: from mail-wr0-f180.google.com ([209.85.128.180]:42942 "EHLO mail-wr0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751355AbdK2HJz (ORCPT ); Wed, 29 Nov 2017 02:09:55 -0500 X-Google-Smtp-Source: AGs4zMbXx5Lr8MUP6eV/zvwuWrq52E1re/QrGGBft9DjJVvALpGqMPWRm/BOgZ6qll3YujH/O90nHw== Date: Wed, 29 Nov 2017 08:09:51 +0100 From: Ingo Molnar To: Jarkko Nikula Cc: linux-kernel , Andy Lutomirski , Thomas Gleixner , Peter Zijlstra , Linus Torvalds , Borislav Petkov Subject: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled Message-ID: <20171129070951.hjjjpbyilzaak4ig@gmail.com> References: <0fede9f9-88b0-a6e7-1027-dfb2019b8ef2@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0fede9f9-88b0-a6e7-1027-dfb2019b8ef2@linux.intel.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Jarkko Nikula 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 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 Bisected-by: Jarkko Nikula Cc: Andy Lutomirski Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Link: http://lkml.kernel.org/r/0fede9f9-88b0-a6e7-1027-dfb2019b8ef2@linux.intel.com Signed-off-by: Ingo Molnar --- 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