From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 08/21] KVM: x86: Reset FPU state during reset Date: Wed, 05 Nov 2014 13:04:52 +0100 Message-ID: <545A1264.5030002@redhat.com> References: <1414922101-17626-1-git-send-email-namit@cs.technion.ac.il> <1414922101-17626-9-git-send-email-namit@cs.technion.ac.il> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, nadav.amit@gmail.com To: Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47534 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754004AbaKEMFF (ORCPT ); Wed, 5 Nov 2014 07:05:05 -0500 In-Reply-To: <1414922101-17626-9-git-send-email-namit@cs.technion.ac.il> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > 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