From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBos8h8iPl11vmezPlnaP6IVPZYOnLWL2FxYOmfDWyuRV2dyhTUJdM+Wgmko6d3q+sGTUOASR ARC-Seal: i=1; a=rsa-sha256; t=1516305124; cv=none; d=google.com; s=arc-20160816; b=X0bE3DUgsS85Pnd0meDamZy58bzczSVZgy4SxWyRIkO3JlTeVBEB44q9P0wjINF6sL noEIyjxxe3kVMj+8BuGRF3Hy+5Ni//BSGV1ilX6WQv7sU7nf+MQAvqsFF9XN+TrMU7ge P9yw67k0yAh0sMk6v92qi5nmwjYAhG1g8pRbpmMqCU8DFT9v7aOcRjDhbJQgVE3e7+Xb 83LzF2ZKhZARET43qg59J7VgYwUXPgvYKylgSP/l0boRe0UkwBvJ9jA7t8XxQ/xubgG3 Q76a/BKqVnSYrRQvEprjrJALqMma1FrGF7dQrY2QrN+P/dmH6f1/YAGWWpNHIrlwqWqH V4Gg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=FLtgzebhBcaePbC+zx5/LuqN6WTV3cuEQdH8CkbxFAI=; b=G/bv3gfKEeFHAUq6jwgKPgzIW/HsBh6s9UMZ+3eAlc1q5D9/nk8xM0XljCmgghdQNX u2spPZq6Sk2bbmowoT77cx0OEy+RAWabk+yBvLBlcfaYiqocm2r1PSTcfba7nVhe05NK o2ziGB4UVSBrN9h61qDH/fMrW3pbiv0rHOsxJ/GhxXWNtDh8hSYR4R/tgKLt3eI8PEpz IYrs3126CpMMAAhnQJ/TIjcrS5J+/BlFDxA6ETn4nceccbs0oL5I0gJpypQ2utWguX53 TTLZeOopgWRbXGd2qqNJ0FVkxbmQamFiJ40ZWqUPMnfta8Q9eJ78krbr1HpTbLbqCciE 0nsw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of prvs=549b675a5=andrew.cooper3@citrix.com designates 185.25.65.24 as permitted sender) smtp.mailfrom=prvs=549b675a5=Andrew.Cooper3@citrix.com Authentication-Results: mx.google.com; spf=pass (google.com: domain of prvs=549b675a5=andrew.cooper3@citrix.com designates 185.25.65.24 as permitted sender) smtp.mailfrom=prvs=549b675a5=Andrew.Cooper3@citrix.com X-IronPort-AV: E=Sophos;i="5.46,378,1511827200"; d="scan'208,217";a="66296163" Subject: Re: [PATCH 28/35] x86/idle: Control Indirect Branch Speculation in idle To: Peter Zijlstra , David Woodhouse , Thomas Gleixner , Josh Poimboeuf CC: , Dave Hansen , Ashok Raj , Tim Chen , Andy Lutomirski , Linus Torvalds , Greg KH , Andrea Arcangeli , Andi Kleen , Arjan Van De Ven , Dan Williams , Paolo Bonzini , Jun Nakajima , Asit Mallick , Jason Baron References: <20180118134800.711245485@infradead.org> <20180118140153.138069275@infradead.org> From: Andrew Cooper Message-ID: <812b9561-3f05-1ad0-861e-a93dc1ca5f6f@citrix.com> Date: Thu, 18 Jan 2018 19:52:02 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180118140153.138069275@infradead.org> Content-Type: multipart/alternative; boundary="------------8E86E6BB09B25FBCD2BA17B6" Content-Language: en-GB X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-LABELS: =?utf-8?b?IlxcSW1wb3J0YW50Ig==?= X-GMAIL-THRID: =?utf-8?q?1589961162070750560?= X-GMAIL-MSGID: =?utf-8?q?1589961162070750560?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: --------------8E86E6BB09B25FBCD2BA17B6 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit On 18/01/18 13:48, Peter Zijlstra wrote: > From: Thomas Gleixner > > Indirect Branch Speculation (IBS) is controlled per physical core. If one > thread disables it then it's disabled for the core. If a thread enters idle > it makes sense to reenable IBS so the sibling thread can run with full > speculation enabled in user space. > > This makes only sense in mwait_idle_with_hints() because mwait_idle() can > serve an interrupt immediately before speculation can be stopped again. SKL > which requires IBRS should use mwait_idle_with_hints() so this is a non > issue and in the worst case a missed optimization. > > [peterz: fixed stop_indirect_branch_speculation placement] > > Originally-by: Tim Chen > Signed-off-by: Thomas Gleixner > Signed-off-by: Peter Zijlstra (Intel) > --- > arch/x86/include/asm/mwait.h | 14 ++++++++++++++ > arch/x86/kernel/process.c | 14 ++++++++++++++ > 2 files changed, 28 insertions(+) > > --- a/arch/x86/include/asm/mwait.h > +++ b/arch/x86/include/asm/mwait.h > @@ -6,6 +6,7 @@ > #include > > #include > +#include > > #define MWAIT_SUBSTATE_MASK 0xf > #define MWAIT_CSTATE_MASK 0xf > @@ -106,9 +107,22 @@ static inline void mwait_idle_with_hints > mb(); > } > > + /* > + * Indirect Branch Speculation (IBS) is controlled per > + * physical core. If one thread disables it, then it's > + * disabled on all threads of the core. The kernel disables > + * it on entry from user space. Reenable it on the thread > + * which goes idle so the other thread has a chance to run > + * with full speculation enabled in userspace. > + */ > + restart_indirect_branch_speculation(); > __monitor((void *)¤t_thread_info()->flags, 0, 0); > if (!need_resched()) > __mwait(eax, ecx); > + /* > + * Stop IBS again to protect kernel execution. > + */ > + stop_indirect_branch_speculation(); Be very careful.  The safety of this on Skylake+ depends on there not being a ret instruction which can be speculatively reached between the two WRMSRs used to play with MSR_SPEC_CTRL. You need to guarantee that the net call tree of these five functions is forced always inline.  A plain "static inline" function is not good enough (and there are several here), if the compiler chooses to out-of-line it, and a real function call is definitely a problem. I accidentally introduced a vulnerability into Xen in my first attempt at this.  (I'm still trying to decide whether reimplementing it in asm would be better long term). ~Andrew --------------8E86E6BB09B25FBCD2BA17B6 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
On 18/01/18 13:48, Peter Zijlstra wrote:
From: Thomas Gleixner <tglx@linutronix.de>

Indirect Branch Speculation (IBS) is controlled per physical core. If one
thread disables it then it's disabled for the core. If a thread enters idle
it makes sense to reenable IBS so the sibling thread can run with full
speculation enabled in user space.

This makes only sense in mwait_idle_with_hints() because mwait_idle() can
serve an interrupt immediately before speculation can be stopped again. SKL
which requires IBRS should use mwait_idle_with_hints() so this is a non
issue and in the worst case a missed optimization.

[peterz: fixed stop_indirect_branch_speculation placement]

Originally-by: Tim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/mwait.h |   14 ++++++++++++++
 arch/x86/kernel/process.c    |   14 ++++++++++++++
 2 files changed, 28 insertions(+)

--- a/arch/x86/include/asm/mwait.h
+++ b/arch/x86/include/asm/mwait.h
@@ -6,6 +6,7 @@
 #include <linux/sched/idle.h>
 
 #include <asm/cpufeature.h>
+#include <asm/nospec-branch.h>
 
 #define MWAIT_SUBSTATE_MASK		0xf
 #define MWAIT_CSTATE_MASK		0xf
@@ -106,9 +107,22 @@ static inline void mwait_idle_with_hints
 			mb();
 		}
 
+		/*
+		 * Indirect Branch Speculation (IBS) is controlled per
+		 * physical core. If one thread disables it, then it's
+		 * disabled on all threads of the core. The kernel disables
+		 * it on entry from user space. Reenable it on the thread
+		 * which goes idle so the other thread has a chance to run
+		 * with full speculation enabled in userspace.
+		 */
+		restart_indirect_branch_speculation();
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		if (!need_resched())
 			__mwait(eax, ecx);
+		/*
+		 * Stop IBS again to protect kernel execution.
+		 */
+		stop_indirect_branch_speculation();

Be very careful.  The safety of this on Skylake+ depends on there not being a ret instruction which can be speculatively reached between the two WRMSRs used to play with MSR_SPEC_CTRL.

You need to guarantee that the net call tree of these five functions is forced always inline.  A plain "static inline" function is not good enough (and there are several here), if the compiler chooses to out-of-line it, and a real function call is definitely a problem.

I accidentally introduced a vulnerability into Xen in my first attempt at this.  (I'm still trying to decide whether reimplementing it in asm would be better long term).

~Andrew
--------------8E86E6BB09B25FBCD2BA17B6--