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

From: Paolo Bonzini <pbonzini@redhat.com>
To: Nadav Amit <nadav.amit@gmail.com>
Cc: Nadav Amit <namit@cs.technion.ac.il>, kvm@vger.kernel.org
Subject: Re: [PATCH 08/21] KVM: x86: Reset FPU state during reset
Date: Thu, 06 Nov 2014 11:44:48 +0100	[thread overview]
Message-ID: <545B5120.3000801@redhat.com> (raw)
In-Reply-To: <F9F361A5-CB6A-4002-921A-707214B56A70@gmail.com>

On 06/11/2014 10:56, Nadav Amit wrote:
> 
>> On Nov 6, 2014, at 11:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>>
>> On 06/11/2014 10:13, Nadav Amit wrote:
>>>
>>>> On Nov 6, 2014, at 10:58, Paolo Bonzini <pbonzini@redhat.com> 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
> 