Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU

[Avi Kivity <avi@redhat.com>]

On 09/20/2012 04:43 AM, Hao, Xudong wrote:
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Wednesday, September 19, 2012 6:24 PM
>> To: Hao, Xudong
>> Cc: Marcelo Tosatti; kvm@vger.kernel.org; Zhang, Xiantao
>> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
>> > That may be:
>> >
>> > static bool lazy_fpu_allowed()
>> > {
>> > 	return !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
>> > }
>> 
>> Shouldn't it depend on cr4.osxsave as well?
>> 
> 
> It do need to check cr4.osxsave due to a separate function.
> 
> static bool lazy_fpu_allowed(struct kvm_vcpu *vcpu)
> {
> 	return !kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) ||
>           !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
> }

Yes.

> 
>> >
>> >> On guest entry:
>> >>         if (!lazy_fpu_allowed(vcpu))
>> >>                 kvm_x86_ops->fpu_activate(vcpu);
>> >
>> 
>> But we already have that:
>> 
>> 	if (vcpu->fpu_active)
>> 		kvm_load_guest_fpu(vcpu);
>> 
>> so why not manage fpu_active to be always set when needed?  I don't want
>> more checks in the entry path.
>>
> I means add fpu_active() in kvm_set_xcr(), not in guest entry. Then the fpu_active will be set always when guest initialize xstate.
>  
> @@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>         kvm_inject_gp(vcpu, 0);
>         return 1;
>     }
> +   if (!lazy_fpu_allowed(vcpu))
> +       kvm_x86_ops->fpu_activate(vcpu);
>     return 0;
> 

And of course on cr4 update.  So a function update_lazy_fpu() to be
called from both places is needed.


-- 
error compiling committee.c: too many arguments to function