Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control feature

[Ingo Molnar <mingo@kernel.org>]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Tim Chen <tim.c.chen@linux.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