Re: [RFC PATCH 6/6] KVM: PPC: Book3E: Enhance FPU laziness

[Scott Wood <scottwood@freescale.com>]

On 06/05/2013 04:14:21 AM, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, June 05, 2013 1:54 AM
> > To: Caraman Mihai Claudiu-B02008
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org; Caraman Mihai Claudiu-B02008
> > Subject: Re: [RFC PATCH 6/6] KVM: PPC: Book3E: Enhance FPU laziness
> >
> > On 06/03/2013 03:54:28 PM, Mihai Caraman wrote:
> > > Adopt AltiVec approach to increase laziness by calling
> > > kvmppc_load_guest_fp()
> > > just before returning to guest instaed of each sched in.
> > >
> > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> >
> > If you did this *before* adding Altivec it would have saved a  
> question
> > in an earlier patch. :-)
> 
> I kept asking myself about the order and in the end I decided that  
> this is
> an improvement originated from AltiVec work. FPU may be further  
> cleaned up
> (get rid of active state, etc).
> 
> >
> > > ---
> > >  arch/powerpc/kvm/booke.c  |    1 +
> > >  arch/powerpc/kvm/e500mc.c |    2 --
> > >  2 files changed, 1 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > > index 019496d..5382238 100644
> > > --- a/arch/powerpc/kvm/booke.c
> > > +++ b/arch/powerpc/kvm/booke.c
> > > @@ -1258,6 +1258,7 @@ int kvmppc_handle_exit(struct kvm_run *run,
> > > struct kvm_vcpu *vcpu,
> > >  		} else {
> > >  			kvmppc_lazy_ee_enable();
> > >  			kvmppc_load_guest_altivec(vcpu);
> > > +			kvmppc_load_guest_fp(vcpu);
> > >  		}
> > >  	}
> > >
> >
> > You should probably do these before kvmppc_lazy_ee_enable().
> 
> Why? I wanted to look like part of lightweight_exit.

We want to minimize the portion of the code that runs with interrupts  
disabled while telling tracers that interrupts are enabled.  We want to  
minimize the C code run with lazy EE in an inconsistent state.

The same applies to kvm_vcpu_run()...

> > Actually, I don't think this is a good idea at all.  As I understand
> > it, you're not supposed to take kernel ownersship of floating point  
> in
> > non-atomic context, because an interrupt could itself call
> > enable_kernel_fp().
> 
> So lightweight_exit isn't executed in atomic context?

Ignore this, I misread what the patch was doing.  I thought you were  
doing the opposite you did. :-P

As such, this patch appears to fix the thing I was complaining about --  
before, we could have taken an interrupt after kvmppc_core_vcpu_load(),  
and that interrupt could have claimed the floating point (unlikely with  
the kernel as is, but you never know what could happen in the future or  
out-of-tree...).

> Will be lazyee fixes including  kvmppc_fix_ee_before_entry() in 3.10?
> 64-bit Book3E KVM is unreliable without them. Should we disable e5500  
> too
> for 3.10?

I hope so...  I meant to ask Gleb to take them while Alex was away, but  
I forgot about them. :-P

Alex, are you back from vacation yet?

-Scott