diff mbox

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

Message ID 1346808370-28376-1-git-send-email-xudong.hao@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hao, Xudong Sept. 5, 2012, 1:26 a.m. UTC
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(-)

Comments

Avi Kivity Sept. 5, 2012, 1:12 p.m. UTC | #1
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.
Hao, Xudong Sept. 6, 2012, 2:13 a.m. UTC | #2
> -----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.

-   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);


Thanks,
-Xudong


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity Sept. 6, 2012, 8:16 a.m. UTC | #3
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.
Hao, Xudong Sept. 10, 2012, 3:29 a.m. UTC | #4
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Avi Kivity
> Sent: Thursday, September 06, 2012 4:16 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/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.

Avi, I'm not sure if I fully understand of you. Do you mean enter guest with a fpu_active=0 and then fpu does not restore? If so, I will add fpu_active=1 in the no-lazy case.

-   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);
+       vcpu->fpu_active=1;
+   }
+   else
+       kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity Sept. 10, 2012, 8:07 a.m. UTC | #5
On 09/10/2012 06:29 AM, Hao, Xudong wrote:
>> 
>> 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.
> 
> Avi, I'm not sure if I fully understand of you. Do you mean enter guest with a fpu_active=0 and then fpu does not restore? 

Yes.

> If so, I will add fpu_active=1 in the no-lazy case.
> 
> -   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);
> +       vcpu->fpu_active=1;
> +   }
> +   else
> +       kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
> 

It doesn't help here.

  1 guest boot
  2 kvm_userspace_exit (deactivates fpu)
  3 XSETBV exit that sets xcr0.new_bit
  4 kvm_enter

There is no call to kvm_put_guest_fpu() between 3 and 4, you need
something in __kvm_set_xcr() to activate the fpu.

kvm_put_guest_fpu() doesn't need to activate the fpu then, just to avoid
deactivating it.

Note you also need to consider writes to xcr0 and cr4 that happen in the
reverse order due to live migration.
Hao, Xudong Sept. 11, 2012, 6:43 a.m. UTC | #6
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Monday, September 10, 2012 4:07 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
> 
> >
> > Avi, I'm not sure if I fully understand of you. Do you mean enter guest with a
> fpu_active=0 and then fpu does not restore?
> 
> Yes.
> 
> > If so, I will add fpu_active=1 in the no-lazy case.
> >
> > -   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);
> > +       vcpu->fpu_active=1;
> > +   }
> > +   else
> > +       kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
> >
> 
> It doesn't help here.
> 
>   1 guest boot
>   2 kvm_userspace_exit (deactivates fpu)
>   3 XSETBV exit that sets xcr0.new_bit
>   4 kvm_enter
> 
> There is no call to kvm_put_guest_fpu() between 3 and 4, you need
> something in __kvm_set_xcr() to activate the fpu.
> 

Yes, it's code path when enable xsave in guest, I'll add fpu activate there and remain v2 patch in kvm_put_guest_fpu().

@@ -554,6 +554,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
    xcr0 = xcr;
    if (kvm_x86_ops->get_cpl(vcpu) != 0)
        return 1;
+   if (xcr0 & ~((u64)KVM_XSTATE_LAZY))
+       kvm_x86_ops->fpu_activate(vcpu);
    if (!(xcr0 & XSTATE_FP))
        return 1;
    if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))

> Note you also need to consider writes to xcr0 and cr4 that happen in the
> reverse order due to live migration.
> 

I'm confused of this, doesn't setting cr4 firstly then xcr0?
Do you mean current live migration has a reverse order, or it must be a reverse order with my eager restore patch?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity Sept. 11, 2012, 7:54 a.m. UTC | #7
On 09/11/2012 09:43 AM, Hao, Xudong wrote:
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Monday, September 10, 2012 4:07 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
>> 
>> >
>> > Avi, I'm not sure if I fully understand of you. Do you mean enter guest with a
>> fpu_active=0 and then fpu does not restore?
>> 
>> Yes.
>> 
>> > If so, I will add fpu_active=1 in the no-lazy case.
>> >
>> > -   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);
>> > +       vcpu->fpu_active=1;
>> > +   }
>> > +   else
>> > +       kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>> >
>> 
>> It doesn't help here.
>> 
>>   1 guest boot
>>   2 kvm_userspace_exit (deactivates fpu)
>>   3 XSETBV exit that sets xcr0.new_bit
>>   4 kvm_enter
>> 
>> There is no call to kvm_put_guest_fpu() between 3 and 4, you need
>> something in __kvm_set_xcr() to activate the fpu.
>> 
> 
> Yes, it's code path when enable xsave in guest, I'll add fpu activate there and remain v2 patch in kvm_put_guest_fpu().
> 
> @@ -554,6 +554,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>     xcr0 = xcr;
>     if (kvm_x86_ops->get_cpl(vcpu) != 0)
>         return 1;
> +   if (xcr0 & ~((u64)KVM_XSTATE_LAZY))
> +       kvm_x86_ops->fpu_activate(vcpu);
>     if (!(xcr0 & XSTATE_FP))
>         return 1;
>     if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))
> 
>> Note you also need to consider writes to xcr0 and cr4 that happen in the
>> reverse order due to live migration.
>> 
> 
> I'm confused of this, doesn't setting cr4 firstly then xcr0?
> Do you mean current live migration has a reverse order, or it must be a reverse order with my eager restore patch?

I mean I want the code to work regardless of whether KVM_SET_SREGS or
KVM_SET_XCRS is called first.
diff mbox

Patch

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);
 }