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

[Avi Kivity <avi@redhat.com>]

On 09/06/2012 05:13 AM, Hao, Xudong wrote:
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Wednesday, September 05, 2012 9:13 PM
>> To: Hao, Xudong
>> Cc: kvm@vger.kernel.org; Zhang, Xiantao; Joerg.Roedel@amd.com
>> Subject: Re: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU
>> 
>> On 09/05/2012 04:26 AM, Xudong Hao wrote:
>> > Enable KVM FPU fully eager restore, if there is other FPU state which isn't
>> > tracked by CR0.TS bit.
>> >
>> > 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/x86.c         |   13 ++++++++++++-
>> >  2 files changed, 16 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/x86.c b/arch/x86/kvm/x86.c
>> > index 20f2266..a632042 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -5969,7 +5969,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. 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 & ~((u64)KVM_XSTATE_LAZY)))
>> > +		kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>> >  	trace_kvm_fpu(0);
>> >  }
>> >
>> 
>> I think something is missing.  This patch prevents
>> KVM_REQ_DEACTIVATE_FPU, but the fpu may not be active when non-lazy bits
>> are added to xcr0 (or cr4.osxsave is enabled).  I think you need to
>> activate the fpu at that time as well.
>> 
> 
> Mmh, I thought fpu is active by default, but it's better to make fpu active explicitly here.
> If the following patch is fine, I'll make it another version.
> 

It is, but a previous pass through kvm_put_guest_fpu() could have
deactivated it.

> -   kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
> +   if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) &&
> +       (vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY)))
> +       kvm_x86_ops->fpu_activate(vcpu);
> +   else
> +       kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
> 

Doesn't help.  We can have:

host: deactivate fpu for some reason
guest: set cr4.osxsave, xcr0.bit3
host: enter guest with deactivated fpu
guest: touch fpu

result: host fpu corrupted.
-- 
error compiling committee.c: too many arguments to function