Re: [PATCH v3 2/4] x86/speculation: Support "Enhanced IBRS" on future CPUs

From: David Woodhouse <dwmw2@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: karahmed@amazon.de, x86@kernel.org, kvm@vger.kernel.org,
	torvalds@linux-foundation.org, pbonzini@redhat.com,
	linux-kernel@vger.kernel.org, bp@alien8.de, peterz@infradead.org,
	jmattson@google.com, rkrcmar@redhat.com,
	arjan.van.de.ven@intel.com, dave.hansen@intel.com,
	mingo@kernel.org
Subject: Re: [PATCH v3 2/4] x86/speculation: Support "Enhanced IBRS" on future CPUs
Date: Tue, 20 Feb 2018 08:53:45 +0000	[thread overview]
Message-ID: <1519116825.7876.112.camel@infradead.org> (raw)
In-Reply-To: <alpine.DEB.2.21.1802200854010.24268@nanos.tec.linutronix.de>

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On Tue, 2018-02-20 at 09:31 +0100, Thomas Gleixner wrote:
> > @@ -237,6 +239,16 @@ static void __init spectre_v2_select_mitigation(void)
> >  
> >  	case SPECTRE_V2_CMD_FORCE:
> >  	case SPECTRE_V2_CMD_AUTO:
> > +		if (boot_cpu_has(X86_FEATURE_ARCH_CAPABILITIES)) {
> > +			u64 ia32_cap = 0;
> > +
> > +			rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> > +			if (ia32_cap & ARCH_CAP_IBRS_ALL) {
>
> Hmm. We already read the MSR in cpu/common.c to check for the RDCL_NO
> bit. Shouldn't we just read it once and set a feature bit for IBRS_ALL?

Yeah. See my comment to Dave where he was going to add a *third* place
that does the same for a different bit. I think we should probably just
turn these into cpufeature bits like the 'scattered' ones.

> > @@ -3387,13 +3387,14 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  
> >  		vmx->spec_ctrl = data;
> >  
> > -		if (!data)
> > +		if (!data && !spectre_v2_ibrs_all())
> >  			break;
> >  
> >  		/*
> >  		 * For non-nested:
> >  		 * When it's written (to non-zero) for the first time, pass
> > -		 * it through.
> > +		 * it through unless we have IBRS_ALL and it should just be
> > +		 * set for ever.
>
> A non zero write is going to disable the intercept only when IBRS_ALL
> is on. The comment says is should be set forever, i.e. not changeable by
> the guest. So the condition should be:
> 
> 		if (!data || spectre_v2_ibrs_all())
> 			break;
> Hmm?

Yes, good catch. Thanks.

However, Paolo is very insistent that taking the trap every time is
actually a lot *slower* than really frobbing IBRS on certain
microarchitectures, so my hand-waving "pfft, what did they expect?" is
not acceptable.

Which I think puts us back to the "throwing the toys out of the pram"
solution; demanding that Intel give us a new feature bit for "IBRS_ALL,
and the bit in the MSR is a no-op". Which was going to be true for
*most* new CPUs anyway. (Note: a blacklist for those few CPUs on which
it *isn't* true might also suffice instead of a new feature bit.)

Unless someone really wants to implement the atomic MSR save and
restore on vmexit, and make it work with nesting, and make the whole
thing sufficiently simple that we don't throw our toys out of the pram
anyway when we see it?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]