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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Sep 14, 2012 at 08:54:40AM +0000, Hao, Xudong wrote:
> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> > Behalf Of Marcelo Tosatti
> > Sent: Friday, September 14, 2012 12:29 AM
> > To: Hao, Xudong; Avi Kivity
> > Cc: kvm@vger.kernel.org; Zhang, Xiantao
> > Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
> > 
> > On Thu, Sep 13, 2012 at 01:26:36PM -0300, Marcelo Tosatti wrote:
> > > On Wed, Sep 12, 2012 at 04:10:24PM +0800, Xudong Hao wrote:
> > > > Enable KVM FPU fully eager restore, if there is other FPU state which isn't
> > > > tracked by CR0.TS bit.
> > > >
> > > > v3 changes from v2:
> > > > - Make fpu active explicitly while guest xsave is enabling and non-lazy xstate
> > bit
> > > > exist.
> > >
> > > How about a "guest_xcr0_can_lazy_saverestore" bool to control this?
> > > It only needs to be updated when guest xcr0 is updated.
> > >
> > > That seems cleaner. Avi?
> > 
> > Reasoning below.
> > 
> > > > v2 changes from v1:
> > > > - Expand KVM_XSTATE_LAZY to 64 bits before negating it.
> > > >
> > > > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > > > ---
> > > >  arch/x86/include/asm/kvm.h |    4 ++++
> > > >  arch/x86/kvm/vmx.c         |    2 ++
> > > >  arch/x86/kvm/x86.c         |   15 ++++++++++++++-
> > > >  3 files changed, 20 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
> > > > index 521bf25..4c27056 100644
> > > > --- a/arch/x86/include/asm/kvm.h
> > > > +++ b/arch/x86/include/asm/kvm.h
> > > > @@ -8,6 +8,8 @@
> > > >
> > > >  #include <linux/types.h>
> > > >  #include <linux/ioctl.h>
> > > > +#include <asm/user.h>
> > > > +#include <asm/xsave.h>
> > > >
> > > >  /* Select x86 specific features in <linux/kvm.h> */
> > > >  #define __KVM_HAVE_PIT
> > > > @@ -30,6 +32,8 @@
> > > >  /* Architectural interrupt line count. */
> > > >  #define KVM_NR_INTERRUPTS 256
> > > >
> > > > +#define KVM_XSTATE_LAZY	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
> > > > +
> > > >  struct kvm_memory_alias {
> > > >  	__u32 slot;  /* this has a different namespace than memory slots */
> > > >  	__u32 flags;
> > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > > index 248c2b4..853e875 100644
> > > > --- a/arch/x86/kvm/vmx.c
> > > > +++ b/arch/x86/kvm/vmx.c
> > > > @@ -3028,6 +3028,8 @@ static void vmx_set_cr0(struct kvm_vcpu *vcpu,
> > unsigned long cr0)
> > > >
> > > >  	if (!vcpu->fpu_active)
> > > >  		hw_cr0 |= X86_CR0_TS | X86_CR0_MP;
> > > > +	else
> > > > +		hw_cr0 &= ~(X86_CR0_TS | X86_CR0_MP);
> > > >
> > > >  	vmcs_writel(CR0_READ_SHADOW, cr0);
> > > >  	vmcs_writel(GUEST_CR0, hw_cr0);
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index 20f2266..183cf60 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -560,6 +560,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32
> > index, u64 xcr)
> > > >  		return 1;
> > > >  	if (xcr0 & ~host_xcr0)
> > > >  		return 1;
> > > > +	if (xcr0 & ~((u64)KVM_XSTATE_LAZY))
> > > > +		vcpu->fpu_active = 1;
> > 
> > This is confusing. The variable allows to decrease the number of places
> > the decision is made.
> > 
> 
> Hi, Marcelo, What does it mean?

See Avi's reply. Better create a function 

lazy_fpu_allowed

Which can be used to decide whether or not to allow 
guest owning FPU state.