From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934189AbeAJGcS (ORCPT + 1 other); Wed, 10 Jan 2018 01:32:18 -0500 Received: from mail-wm0-f43.google.com ([74.125.82.43]:41685 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752849AbeAJGcP (ORCPT ); Wed, 10 Jan 2018 01:32:15 -0500 X-Google-Smtp-Source: ACJfBosPY6EzWMRjRVLJHxdMjcq74d9m79BINSX/y8mm30MAmRhkTU2n9vNklizdbC+pTw+l4EbZ0g== Date: Wed, 10 Jan 2018 07:32:11 +0100 From: Ingo Molnar To: Thomas Gleixner Cc: LKML , Linus Torvalds , x86@kernel.org, Peter Zijlstra , Borislav Petkov , David Woodhouse , Tim Chen , Andrea Arcangeli , Andi Kleen , Greg KH , Dave Hansen , Andy Lutomirski , Arjan Van De Ven , Linus Torvalds Subject: Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control feature Message-ID: <20180110063211.z6w55gwxm5szmt2l@gmail.com> References: <20180110010652.404145126@linutronix.de> <20180110011350.759392067@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180110011350.759392067@linutronix.de> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: * Thomas Gleixner wrote: > From: Tim Chen > > CPUs can expose a MSR to control speculation. The initial function of this > MSR is to control Indirect Branch Speculation, which is required to > mitigate the Spectre_V2 attack on certain CPU generations. s/a MSR /an MSR > If CPUID(7).RDX[26] is set then MSR_IA32_SPEC_CTRL (0x48) is available and > bit 0 of that MSR controls whether Indirect Branch Speculation is > restricted or not. The control bit is named IBRS (Indirect Branch > Restricted Speculation). The IBSR bit can be unconditionally set to 1 > without clearing it before. Argh for inverted logic: why was the control bit defined for a _negated_ value, i.e. why does '0' mean "don't don't speculate"? And yes, I know what's behind it: this way 'IBRS' can be called a 'mitigation feature' that can be 'enabled', instead of calling it a 'broken CPU feature feature' that has to be disabled ... That's nonsense that causes confusion to no end: > If IBRS is set, near returns and near indirect jumps/calls will not allow > their predicted target address to be controlled by code that executed in a > less privileged prediction mode before the IBRS mode was last written with > a value of 1 or on another logical processor so long as all Return Stack > Buffer (RSB) entries from the previous less privileged prediction mode are > overwritten. > > Thus a near indirect jump/call/return may be affected by code in a less > privileged prediction mode that executed AFTER IBRS mode was last written > with a value of 1. > > Code executed by a sibling logical processor cannot control indirect > jump/call/return predicted target when IBRS is set > > IBRS is not required in order to isolate branch predictions for SMM or SGX > enclaves. > > Enabling IBRS can cause a measurable and depending on the workload > significant CPU performance penalty. This is bound to be really confusing due to the logic negation, in particular: > +#define SPEC_CTRL_DISABLE_IBRS (0UL << 0) > +#define SPEC_CTRL_ENABLE_IBRS (1UL << 0) "SPEC_CTRL_ENABLE_IBRS" will _disable_ speculation! Then that brokenness is propagated into higher code as well by the next patch, i.e. "SPECTRE_V2_CMD_IBRS" et al. This is totally brain-dead and should be inverted to follow natural logic instead. The lowest level hardware ABI will obviously stay broken, Why not define a sane name instead? Something like: CTRL_DISABLE_BR_SPECULATION CTRL_ENABLE_BR_SPECULATION and only explain the broken negated Intel naming and flag once in the comments for the definition and the lowest level MSR write. Thanks, Ingo