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: Thu, 6 Nov 2014 11:56:27 +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> <545A3A6C.3010302@redhat.com> <545B3843.8090501@redhat.com> <47A17AC1-9FAC-467B-9DBC-76D530D8F131@gmail.com> <545B42FF.4000302@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-wg0-f43.google.com ([74.125.82.43]:49693 "EHLO mail-wg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751761AbaKFJ4c convert rfc822-to-8bit (ORCPT ); Thu, 6 Nov 2014 04:56:32 -0500 Received: by mail-wg0-f43.google.com with SMTP id y10so736500wgg.30 for ; Thu, 06 Nov 2014 01:56:30 -0800 (PST) In-Reply-To: <545B42FF.4000302@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: > On Nov 6, 2014, at 11:44, Paolo Bonzini wrote: >=20 >=20 >=20 > On 06/11/2014 10:13, Nadav Amit wrote: >>=20 >>> On Nov 6, 2014, at 10:58, Paolo Bonzini wrote= : >>>=20 >>> On 05/11/2014 21:31, Nadav Amit wrote: >>>> You are correct, it does not appear clearly in the SDM, but that i= s what real hardware does. >>>> If you look at bochs - http://code.metager.de/source/xref/bochs/bo= chs/cpu/init.cc - you=92ll see they call >>>> "BX_CPU_THIS_PTR xcr0.set32(0x1);=94 regardless to whether it is h= ardware or software reset (the latter happens on INIT). >>>=20 >>> Fair enough. :) >> Thanks. It is turning harder to find references for the crazy x86 be= haviour. :) >=20 > Indeed, I'll ask Intel to clarify this one too. >=20 > The crazy thing is that AMD doesn't say anything, either! Their own=20 > manual just says "Hardware initializes XCR0 to 0000_0000_0000_0001h",= =20 > but it doesn't say when. I know. I looked at their manual too. >=20 >>> Does the patch in >>> http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/129060 lo= ok good? >>=20 >> Yes. However, re-reviewing the patches both my patch and yours actua= lly do something slightly different than the spec: they clear XMM8-15 a= nd YMM[128:=85] which should not happen on INIT according to the spec. >=20 > Yes, my patch just wanted to have the same effect as yours, but=20 > fpu_finit must remain in fx_alloc. Setting cr0 is also unnecessary,=20 > since vmx_vcpu_reset and svm_vcpu_reset both do this. >=20 >> Fixing it might be a bit intrusive. What do you say? >=20 > I think it's easy if we start with my version of the change: >=20 > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/k= vm_host.h > index dc932d388c43..aba13df4e0ec 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -915,8 +915,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 e0260ccd78a4..0ef4c0b27248 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6868,7 +6868,7 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu= *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; >=20 > @@ -6878,16 +6878,8 @@ int fx_init(struct kvm_vcpu *vcpu) >=20 > fpu_finit(&vcpu->arch.guest_fpu); >=20 > - /* > - * Ensure guest xcr0 is valid for loading > - */ > - vcpu->arch.xcr0 =3D XSTATE_FP; > - > - vcpu->arch.cr0 |=3D X86_CR0_ET; I think this line was removed by mistake. > - > return 0; > } > -EXPORT_SYMBOL_GPL(fx_init); >=20 > static void fx_free(struct kvm_vcpu *vcpu) > { > @@ -7025,6 +7017,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu) > vcpu->arch.regs_avail =3D ~0; > vcpu->arch.regs_dirty =3D ~0; >=20 > + /* > + * Ensure guest xcr0 is valid for loading > + */ > + vcpu->arch.xcr0 =3D XSTATE_FP; > + > kvm_x86_ops->vcpu_reset(vcpu); > } >=20 I=92ll give it a shot (in a couple of weeks). Thanks, Nadav