Re: [PATCH v3] KVM: x86: Make register state after reset conform to specification

[Gleb Natapov <gleb@redhat.com>]

On Tue, Dec 04, 2012 at 01:13:30PM +0100, Julian Stecklina wrote:
> VMX behaves now as SVM wrt to FPU initialization. Code has been moved to
> generic code path. General-purpose registers are now cleared on reset and
> INIT.  SVM code properly initializes EDX.
> 
Looks good overall, small bug bellow and regenerate the patch against
git://git.kernel.org/pub/scm/virt/kvm/kvm.git queue branch please.

> Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de>
> ---
>  arch/x86/kvm/cpuid.c |  1 +
>  arch/x86/kvm/svm.c   | 14 ++++++--------
>  arch/x86/kvm/vmx.c   |  7 -------
>  arch/x86/kvm/x86.c   |  8 ++++++++
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0595f13..aa468c2 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -659,6 +659,7 @@ void kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
>  	} else
>  		*eax = *ebx = *ecx = *edx = 0;
>  }
> +EXPORT_SYMBOL_GPL(kvm_cpuid);
>  
>  void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
>  {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index baead95..6c50121 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -20,6 +20,7 @@
>  #include "mmu.h"
>  #include "kvm_cache_regs.h"
>  #include "x86.h"
> +#include "cpuid.h"
>  
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> @@ -1190,6 +1191,8 @@ static void init_vmcb(struct vcpu_svm *svm)
>  static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> +	u32 dummy;
> +	u32 eax = 1;
>  
>  	init_vmcb(svm);
>  
> @@ -1198,8 +1201,9 @@ static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
>  		svm->vmcb->save.cs.base = svm->vcpu.arch.sipi_vector << 12;
>  		svm->vmcb->save.cs.selector = svm->vcpu.arch.sipi_vector << 8;
>  	}
> -	vcpu->arch.regs_avail = ~0;
> -	vcpu->arch.regs_dirty = ~0;
> +
> +	kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy);
> +	kvm_register_write(vcpu, VCPU_REGS_RDX, eax);
>  
>  	return 0;
>  }
> @@ -1257,10 +1261,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
>  	init_vmcb(svm);
>  	kvm_write_tsc(&svm->vcpu, 0);
>  
> -	err = fx_init(&svm->vcpu);
> -	if (err)
> -		goto free_page4;
> -
>  	svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE;
>  	if (kvm_vcpu_is_bsp(&svm->vcpu))
>  		svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP;
> @@ -1269,8 +1269,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
>  
>  	return &svm->vcpu;
>  
> -free_page4:
> -	__free_page(hsave_page);
>  free_page3:
>  	__free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER);
>  free_page2:
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ff66a3b..85cecfe 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3942,8 +3942,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>  	u64 msr;
>  	int ret;
>  
> -	vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP));
> -
>  	vmx->rmode.vm86_active = 0;
>  
>  	vmx->soft_vnmi_blocked = 0;
> @@ -3955,10 +3953,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>  		msr |= MSR_IA32_APICBASE_BSP;
>  	kvm_set_apic_base(&vmx->vcpu, msr);
>  
> -	ret = fx_init(&vmx->vcpu);
> -	if (ret != 0)
> -		goto out;
> -
>  	vmx_segment_cache_clear(vmx);
>  
>  	seg_setup(VCPU_SREG_CS);
> @@ -3999,7 +3993,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>  		kvm_rip_write(vcpu, 0xfff0);
>  	else
>  		kvm_rip_write(vcpu, 0);
> -	kvm_register_write(vcpu, VCPU_REGS_RSP, 0);
>  
>  	vmcs_writel(GUEST_DR7, 0x400);
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2966c84..2e031bf 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6066,6 +6066,10 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
>  
>  	kvm_pmu_reset(vcpu);
>  
> +	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
> +	vcpu->arch.regs_avail = ~0;
> +	vcpu->arch.regs_dirty = ~0;
> +
>  	return kvm_x86_ops->vcpu_reset(vcpu);
>  }
>  
> @@ -6229,6 +6233,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  	if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL))
>  		goto fail_free_mce_banks;
>  
> +	r = fx_init(vcpu);
> +	if (r)
> +		goto fail_free_mce_banks;
> +
You need to free vcpu->arch.wbinvd_dirty_mask in case fx_init() fails.

>  	kvm_async_pf_hash_reset(vcpu);
>  	kvm_pmu_init(vcpu);
>  
> -- 
> 1.7.11.7

--
			Gleb.