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

[Avi Kivity <avi@redhat.com>]

On 08/16/2012 12:48 PM, Hao, Xudong wrote:
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
>> Behalf Of Avi Kivity
>> Sent: Thursday, August 16, 2012 5:08 PM
>> To: Hao, Xudong
>> Cc: kvm@vger.kernel.org; Zhang, Xiantao
>> Subject: Re: [PATCH] kvm/fpu: Enable fully eager restore kvm FPU
>> 
>> On 08/16/2012 08:14 AM, Xudong Hao wrote:
>> > Enable KVM FPU fully eager restore, if there is other FPU state which isn't
>> tracked by
>> > CR0.TS bit.
>> >
>> > Tested with these cases:
>> > 1) SpecCPU2000 workload( 1 VM, 2 VMs)
>> > 2) Program for floating point caculate
>> 
>> Is the motivation performance or correctness?
>> 
> 
> It's not performance improvement, it could be treated as a correctness. I do not say current code has issue, but just as code comment, it's for the other FPU state.
> 
>> > +
>> >  struct kvm_memory_alias {
>> >  	__u32 slot;  /* this has a different namespace than memory slots */
>> >  	__u32 flags;
>> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> > index b6379e5..2e628e5 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -5966,7 +5966,18 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
>> >  	vcpu->guest_fpu_loaded = 0;
>> >  	fpu_save_init(&vcpu->arch.guest_fpu);
>> >  	++vcpu->stat.fpu_reload;
>> > -	kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>> > +	/*
>> > +	 * Currently KVM trigger FPU restore by #NM (via CR0.TS),
>> > +	 * till now only XCR0.bit0, XCR0.bit1, XCR0.bit2 is tracked
>> > +	 * by TS bit, there might be other FPU state is not tracked
>> > +	 * by TS bit.
>> 
>> Which state is that?
>> 
> 
> Except the last 3 bits, other bit are these state.
> 
>> > Here it only make FPU deactivate request and do
>> > +	 * FPU lazy restore for these cases: 1)xsave isn't enabled
>> > +	 * in guest, 2)all guest FPU states can be tracked by TS bit.
>> > +	 * For others, doing fully FPU eager restore.
>> > +	 */
>> > +	if (!kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) ||
>> > +		!(vcpu->arch.xcr0 & ~KVM_XSTATE_LAZY))
>> > +		kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>> >  	trace_kvm_fpu(0);
>> >  }
>> 
>> Is there no way to track accesses to this extended state?
>> 
> 
> Because I can't define the extended state now, so using this method. But just as I say, the extended state are NO-LAZY except the last 3 bit.

Ok.  Please check that ~KVM_XSTATE_LAZY expands to 64-bits correctly,
maybe we need to cast it to u64 before negating it.

Note that we limit xcr0 to the bits allowed by the host, so the currect
kernel is safe even on hardware with state that isn't tracked by cr0.ts.
 But it's better to be safe here.

Joerg, IIRC LWP uses one of these bits?  Should it be added to the mask?

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