[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180110063211.z6w55gwxm5szmt2l@gmail.com>
Date: Wed, 10 Jan 2018 07:32:11 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linus Torvalds <torvalds@...uxfoundation.org>, x86@...nel.org,
Peter Zijlstra <peterz@...radead.org>,
Borislav Petkov <bp@...en8.de>,
David Woodhouse <dwmw@...zon.co.uk>,
Tim Chen <tim.c.chen@...ux.intel.com>,
Andrea Arcangeli <aarcange@...hat.com>,
Andi Kleen <ak@...ux.intel.com>,
Greg KH <gregkh@...uxfoundation.org>,
Dave Hansen <dave.hansen@...el.com>,
Andy Lutomirski <luto@...nel.org>,
Arjan Van De Ven <arjan.van.de.ven@...el.com>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control
feature
* Thomas Gleixner <tglx@...utronix.de> wrote:
> From: Tim Chen <tim.c.chen@...ux.intel.com>
>
> 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
Powered by blists - more mailing lists