On 06/11/2014 10:56, Nadav Amit wrote: > >> On Nov 6, 2014, at 11:44, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: >> >> >> >> On 06/11/2014 10:13, Nadav Amit wrote: >>> >>>> On Nov 6, 2014, at 10:58, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: >>>> >>>> On 05/11/2014 21:31, Nadav Amit wrote: >>>>> You are correct, it does not appear clearly in the SDM, but that is what real hardware does. >>>>> If you look at bochs - http://code.metager.de/source/xref/bochs/bochs/cpu/init.cc - you’ll see they call >>>>> "BX_CPU_THIS_PTR xcr0.set32(0x1);” regardless to whether it is hardware or software reset (the latter happens on INIT). >>>> >>>> Fair enough. :) >>> Thanks. It is turning harder to find references for the crazy x86 behaviour. :) >> >> Indeed, I'll ask Intel to clarify this one too. >> >> The crazy thing is that AMD doesn't say anything, either! Their own >> manual just says "Hardware initializes XCR0 to 0000_0000_0000_0001h", >> but it doesn't say when. > I know. I looked at their manual too. > >> >>>> Does the patch in >>>> http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/129060 look good? >>> >>> Yes. However, re-reviewing the patches both my patch and yours actually do something slightly different than the spec: they clear XMM8-15 and YMM[128:…] which should not happen on INIT according to the spec. >> >> Yes, my patch just wanted to have the same effect as yours, but >> fpu_finit must remain in fx_alloc. Setting cr0 is also unnecessary, >> since vmx_vcpu_reset and svm_vcpu_reset both do this. >> >>> Fixing it might be a bit intrusive. What do you say? >> >> I think it's easy if we start with my version of the change: >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_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 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 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; >> } >> >> -int fx_init(struct kvm_vcpu *vcpu) >> +static int fx_init(struct kvm_vcpu *vcpu) >> { >> int err; >> >> @@ -6878,16 +6878,8 @@ int fx_init(struct kvm_vcpu *vcpu) >> >> fpu_finit(&vcpu->arch.guest_fpu); >> >> - /* >> - * Ensure guest xcr0 is valid for loading >> - */ >> - vcpu->arch.xcr0 = XSTATE_FP; >> - >> - vcpu->arch.cr0 |= X86_CR0_ET; > I think this line was removed by mistake. It is set already by vmx_vcpu_setup and svm_vcpu_setup. At startup we always set NW and CD in addition to ET. Paolo >> - >> return 0; >> } >> -EXPORT_SYMBOL_GPL(fx_init); >> >> static void fx_free(struct kvm_vcpu *vcpu) >> { >> @@ -7025,6 +7017,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu) >> vcpu->arch.regs_avail = ~0; >> vcpu->arch.regs_dirty = ~0; >> >> + /* >> + * Ensure guest xcr0 is valid for loading >> + */ >> + vcpu->arch.xcr0 = XSTATE_FP; >> + >> kvm_x86_ops->vcpu_reset(vcpu); >> } >> > I’ll give it a shot (in a couple of weeks). > > Thanks, > Nadav > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
