From mboxrd@z Thu Jan  1 00:00:00 1970
Return-path: <bp@suse.de>
Received: from mx2.suse.de ([195.135.220.15])	by Galois.linutronix.de with
 esmtps (TLS1.0:DHE_RSA_CAMELLIA_256_CBC_SHA1:256)	(Exim 4.80)	(envelope-from
 <bp@suse.de>)	id 1f9YhN-0000eG-Qk	for speck@linutronix.de; Fri, 20 Apr 2018
 18:15:48 +0200
Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254])
	by mx2.suse.de (Postfix) with ESMTP id 6A470AEA6
	for <speck@linutronix.de>; Fri, 20 Apr 2018 16:15:40 +0000 (UTC)
Date: Fri, 20 Apr 2018 18:15:33 +0200
From: Borislav Petkov <bp@suse.de>
Subject: [MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4
Message-ID: <20180420161533.GK13977@pd.tnic>
References: <20180420022613.057637144@localhost.localdomain>
MIME-Version: 1.0
In-Reply-To: <20180420022613.057637144@localhost.localdomain>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
To: speck@linutronix.de
List-ID: <speck.linutronix.de>

On Thu, Apr 19, 2018 at 10:25:44PM -0400, speck for konrad.wilk_at_oracle.com=
 wrote:
> KVM/SVM/VMX/x86/spectre_v2: Support the combination of guest IBRS and ours.
>=20
> A guest may modify the SPEC_CTRL MSR from the value used by the
> kernel. Since we don't use IBRS, this means a value of zero
> is what we need in the host.
>=20
> But the 336996-Speculative-Execution-Side-Channel-Mitigations.pdf
> refers to the other bits as reserved so we should respect the
> boot time SPEC_CTRL value and use that.
>=20
> This allows us to deal with future extensions to the SPEC_CTRL
> interface if any at all.
>=20
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v2: New patch
> ---
>  arch/x86/kvm/svm.c | 6 +++---
>  arch/x86/kvm/vmx.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
>=20
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index be9c839e2c89..f666b4c21559 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5401,7 +5401,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>  	 * is no need to worry about the conditional branch over the wrmsr
>  	 * being speculatively taken.
>  	 */
> -	if (svm->spec_ctrl)
> +	if (svm->spec_ctrl || need_spec_ctrl_acc())
>  		native_wrmsrl(MSR_IA32_SPEC_CTRL, svm->spec_ctrl);

Instead of those silly set/clear helpers, why not do this:

	if (svm->spec_ctrl)
		x86_enable_ibrs();

and hide in x86_enable_ibrs() all that logic of checking
x86_spec_ctrl_base and writing the MSR and whatever else we will need in
the future?

This way you don't need to export helper functions any bit defines
around the place - just the enable/disable functions which hide the
whole logic?

>  	asm volatile (
> @@ -5514,8 +5514,8 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>  	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
>  		svm->spec_ctrl =3D native_read_msr(MSR_IA32_SPEC_CTRL);
> =20
> -	if (svm->spec_ctrl)
> -		native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> +	if (svm->spec_ctrl || need_spec_ctrl_acc())
> +		native_wrmsrl(MSR_IA32_SPEC_CTRL, clear_spec_ctrl(SPEC_CTRL_IBRS));

Same here:

	if (svm->spec_ctrl)
		x86_disable_ibrs();

And so on...

--=20
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imend=C3=B6rffer, Jane Smithard, Graham Norton, HR=
B 21284 (AG N=C3=BCrnberg)
--=20