Re: [PATCH 08/21] KVM: x86: Reset FPU state during reset

[Paolo Bonzini <pbonzini@redhat.com>]

On 02/11/2014 10:54, Nadav Amit wrote:
> When resetting the VCPU, the FPU should be reset as well (e.g., XCR0 state).
> Call fx_init during reset as well.

Actually it shouldn't be after INIT.  XCR0 is not mentioned explicitly 
in Table 9-1 of the SDM (IA-32 Processor States Following Power-up, 
Reset, or INIT), but since MSR_IA32_XSS is not specified, I think XCR0 
should fall under "All other MSRs".

> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 773c17e..9b90ea7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7020,6 +7020,9 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
>  	vcpu->arch.regs_avail = ~0;
>  	vcpu->arch.regs_dirty = ~0;
>  
> +	/* should never fail, since fpu_alloc already done */
> +	fx_init(vcpu);
> +
>  	kvm_x86_ops->vcpu_reset(vcpu);
>  }
>  
> 

Even then, I think this patch is not really nice...  The call sequence 
leading to kvm_vcpu_reset is:

kvm_vm_ioctl_create_vcpu
        kvm_arch_vcpu_create
                kvm_vcpu_init
                        kvm_arch_vcpu_init
                                fx_init (does fpu_alloc)
        kvm_arch_vcpu_setup
                kvm_vcpu_reset
                        fx_init (no fpu_alloc)

The FPU state is not needed between kvm_arch_vcpu_init and 
kvm_arch_vcpu_setup.  So we could simply move the reset from 
kvm_vcpu_init to kvm_vcpu_reset, like this:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 904535fe825e..eaa3be26dfdc 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -914,8 +914,6 @@ void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
 
 void kvm_inject_nmi(struct kvm_vcpu *vcpu);
 
-int fx_init(struct kvm_vcpu *vcpu);
-
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes);
 int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 773c17ec42dd..a0566efbb77f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6863,26 +6863,10 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 	return 0;
 }
 
-int fx_init(struct kvm_vcpu *vcpu)
+static int fx_init(struct kvm_vcpu *vcpu)
 {
-	int err;
-
-	err = fpu_alloc(&vcpu->arch.guest_fpu);
-	if (err)
-		return err;
-
-	fpu_finit(&vcpu->arch.guest_fpu);
-
-	/*
-	 * Ensure guest xcr0 is valid for loading
-	 */
-	vcpu->arch.xcr0 = XSTATE_FP;
-
-	vcpu->arch.cr0 |= X86_CR0_ET;
-
-	return 0;
+	return fpu_alloc(&vcpu->arch.guest_fpu);
 }
-EXPORT_SYMBOL_GPL(fx_init);
 
 static void fx_free(struct kvm_vcpu *vcpu)
 {
@@ -7020,6 +7004,15 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
 	vcpu->arch.regs_avail = ~0;
 	vcpu->arch.regs_dirty = ~0;
 
+	fpu_finit(&vcpu->arch.guest_fpu);
+
+	/*
+	 * Ensure guest xcr0 is valid for loading
+	 */
+	vcpu->arch.xcr0 = XSTATE_FP;
+
+	vcpu->arch.cr0 |= X86_CR0_ET;
+
 	kvm_x86_ops->vcpu_reset(vcpu);
 }
 

However, as said above I'm not applying either patch, at least for now.

Paolo