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

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

[linmiaohe]

Paolo Bonzini <pbonzini@redhat.com> writes:
> The microcode version should be set just once, since it is essentially a CPU feature; so do it on vCPU creation rather than reset.
>
> 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@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Looks good. Thanks.
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

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

[Paolo Bonzini]

On 11/02/20 23:38, Sean Christopherson wrote:
> 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.

That was true of CPUID bits too until microcode started adding and
removing them, but I see your point. :)  What I was trying to convey as
"CPU feature" is that KVM will not change it arbitrarily when running;
it can only change as a result of userspace actions, KVM_SET_MSRS in
this case.  But yes, I will improve the text based on your version:

---
Do not initialize the microcode version at RESET or INIT, only on vCPU
creation.   Microcode updates are not lost during INIT, and exact
behavior across a warm RESET is not specified by the architecture.

Since we do not support a microcode update directly from the hypervisor,
but only as a result of userspace setting the microcode version MSR,
it's simpler for userspace if we do nothing in KVM and let userspace
emulate behavior for RESET as it sees fit.
---

Thanks,

Paolo

> 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:

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

[Sean Christopherson]

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@intel.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@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.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
> 

[PATCH] KVM: x86: do not reset microcode version on INIT or RESET

[Paolo Bonzini]

The microcode version should be set just once, since it is essentially
a CPU feature; so do it on vCPU creation rather than reset.

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@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.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