linux-kernel - Re: [PATCH] KVM: x86: do not reset microcode version on INIT or RESET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200211223837.GC21526@linux.intel.com>
Date:   Tue, 11 Feb 2020 14:38:37 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: do not reset microcode version on INIT or RESET

On Tue, Feb 11, 2020 at 07:04:39PM +0100, Paolo Bonzini wrote:
> The microcode version should be set just once, since it is essentially
> a CPU feature; so do it on vCPU creation rather than reset.

I wouldn't call it a CPU feature, CPU features generally can't be
arbitrarily changed while running.  I'd prefer to have a changelog that
at least somewhat ties the change to hardware behavior. 

  Do not initialize the microcode version at RESET or INIT.   Microcode
  updates are not lost during INIT, and exact behavior across a warm RESET
  is microarchitectural, i.e. defer to userspace to emulate behavior for
  RESET as it sees fit.

For the code:

Reviewed-by: Sean Christopherson <sean.j.christopherson@...el.com>

> Userspace can tie the fix to the availability of MSR_IA32_UCODE_REV in
> the list of emulated MSRs.
> 
> Reported-by: Alex Williamson <alex.williamson@...hat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/svm.c     | 2 +-
>  arch/x86/kvm/vmx/vmx.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index a7e63b613837..280f6d024e84 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2185,7 +2185,6 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	u32 dummy;
>  	u32 eax = 1;
>  
> -	vcpu->arch.microcode_version = 0x01000065;
>  	svm->spec_ctrl = 0;
>  	svm->virt_spec_ctrl = 0;
>  
> @@ -2276,6 +2275,7 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
>  	init_vmcb(svm);
>  
>  	svm_init_osvw(vcpu);
> +	vcpu->arch.microcode_version = 0x01000065;
>  
>  	return 0;
>  
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9a6664886f2e..d625b4b0e7b4 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4238,7 +4238,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  
>  	vmx->msr_ia32_umwait_control = 0;
>  
> -	vcpu->arch.microcode_version = 0x100000000ULL;
>  	vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
>  	vmx->hv_deadline_tsc = -1;
>  	kvm_set_cr8(vcpu, 0);
> @@ -6763,6 +6762,7 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
>  	vmx->nested.posted_intr_nv = -1;
>  	vmx->nested.current_vmptr = -1ull;
>  
> +	vcpu->arch.microcode_version = 0x100000000ULL;
>  	vmx->msr_ia32_feature_control_valid_bits = FEAT_CTL_LOCKED;
>  
>  	/*
> -- 
> 1.8.3.1
>