From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nadav Amit Subject: Re: [PATCH 08/21] KVM: x86: Reset FPU state during reset Date: Wed, 5 Nov 2014 15:20:34 +0200 Message-ID: References: <1414922101-17626-1-git-send-email-namit@cs.technion.ac.il> <1414922101-17626-9-git-send-email-namit@cs.technion.ac.il> <545A1264.5030002@redhat.com> Mime-Version: 1.0 (Mac OS X Mail 8.0 \(1990.1\)) Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nadav Amit , kvm@vger.kernel.org To: Paolo Bonzini Return-path: Received: from mail-wi0-f177.google.com ([209.85.212.177]:56519 "EHLO mail-wi0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753988AbaKENUh convert rfc822-to-8bit (ORCPT ); Wed, 5 Nov 2014 08:20:37 -0500 Received: by mail-wi0-f177.google.com with SMTP id ex7so1971906wid.10 for ; Wed, 05 Nov 2014 05:20:35 -0800 (PST) In-Reply-To: <545A1264.5030002@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: > On Nov 5, 2014, at 14:04, Paolo Bonzini wrote: >=20 >=20 >=20 > 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. >=20 > Actually it shouldn't be after INIT. XCR0 is not mentioned explicitl= y=20 > in Table 9-1 of the SDM (IA-32 Processor States Following Power-up,=20 > Reset, or INIT), but since MSR_IA32_XSS is not specified, I think XCR= 0=20 > should fall under "All other MSRs=94. I should have given a reference, since Intel SDM is a wild place - see = section 2.6 =93EXTENDED CONTROL REGISTERS (INCLUDING XCR0)=94 : "After = reset, all bits (except bit 0) in XCR0 are cleared to zero, XCR0[0] is = set to 1." >=20 >> Signed-off-by: Nadav Amit >> --- >> arch/x86/kvm/x86.c | 3 +++ >> 1 file changed, 3 insertions(+) >>=20 >> 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 =3D ~0; >> vcpu->arch.regs_dirty =3D ~0; >>=20 >> + /* should never fail, since fpu_alloc already done */ >> + fx_init(vcpu); >> + >> kvm_x86_ops->vcpu_reset(vcpu); >> } >>=20 >>=20 >=20 > Even then, I think this patch is not really nice... The call sequenc= e=20 > leading to kvm_vcpu_reset is: I agree. I did a lousy job this time=85 (made a hack, and forgot to unh= ack it). >=20 > 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) >=20 > The FPU state is not needed between kvm_arch_vcpu_init and=20 > kvm_arch_vcpu_setup. So we could simply move the reset from=20 > kvm_vcpu_init to kvm_vcpu_reset, like this: >=20 > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/k= vm_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 i= rq_source_id); >=20 > void kvm_inject_nmi(struct kvm_vcpu *vcpu); >=20 > -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_vc= pu *vcpu, struct kvm_fpu *fpu) > return 0; > } >=20 > -int fx_init(struct kvm_vcpu *vcpu) > +static int fx_init(struct kvm_vcpu *vcpu) > { > - int err; > - > - err =3D 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 =3D XSTATE_FP; > - > - vcpu->arch.cr0 |=3D X86_CR0_ET; > - > - return 0; > + return fpu_alloc(&vcpu->arch.guest_fpu); > } > -EXPORT_SYMBOL_GPL(fx_init); >=20 > static void fx_free(struct kvm_vcpu *vcpu) > { > @@ -7020,6 +7004,15 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu) > vcpu->arch.regs_avail =3D ~0; > vcpu->arch.regs_dirty =3D ~0; >=20 > + fpu_finit(&vcpu->arch.guest_fpu); > + > + /* > + * Ensure guest xcr0 is valid for loading > + */ > + vcpu->arch.xcr0 =3D XSTATE_FP; > + > + vcpu->arch.cr0 |=3D X86_CR0_ET; > + > kvm_x86_ops->vcpu_reset(vcpu); > } >=20 >=20 > However, as said above I'm not applying either patch, at least for no= w. Ok. I hope you will apply it in the future, since we need to workaround= it in our tests. ( I run some tests that stress the emulator, but this bug causes proble= ms even without this stress ). Thanks, Nadav