[MODERATED] Re: [patch 04/11] [PATCH v2 04/10] Linux Patch #4

[Borislav Petkov <bp@suse.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.
> 
> 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.
> 
> 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.
> 
> This allows us to deal with future extensions to the SPEC_CTRL
> interface if any at all.
> 
> 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(-)
> 
> 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 = native_read_msr(MSR_IA32_SPEC_CTRL);
>  
> -	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...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--