* [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
@ 2012-09-12 8:10 Xudong Hao
2012-09-13 16:26 ` Marcelo Tosatti
2012-09-13 16:38 ` Avi Kivity
0 siblings, 2 replies; 16+ messages in thread
From: Xudong Hao @ 2012-09-12 8:10 UTC (permalink / raw)
To: avi; +Cc: kvm, xiantao.zhang, Xudong Hao
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.
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;
vcpu->arch.xcr0 = xcr0;
vcpu->guest_xcr0_loaded = 0;
return 0;
@@ -5969,7 +5971,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);
}
--
1.5.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-12 8:10 [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU Xudong Hao
@ 2012-09-13 16:26 ` Marcelo Tosatti
2012-09-13 16:29 ` Marcelo Tosatti
2012-09-13 16:38 ` Avi Kivity
1 sibling, 1 reply; 16+ messages in thread
From: Marcelo Tosatti @ 2012-09-13 16:26 UTC (permalink / raw)
To: Xudong Hao, Avi Kivity; +Cc: kvm, xiantao.zhang
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?
> 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;
> vcpu->arch.xcr0 = xcr0;
> vcpu->guest_xcr0_loaded = 0;
> return 0;
> @@ -5969,7 +5971,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);
> }
>
> --
> 1.5.5
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-13 16:26 ` Marcelo Tosatti
@ 2012-09-13 16:29 ` Marcelo Tosatti
2012-09-13 16:40 ` Avi Kivity
2012-09-14 8:54 ` Hao, Xudong
0 siblings, 2 replies; 16+ messages in thread
From: Marcelo Tosatti @ 2012-09-13 16:29 UTC (permalink / raw)
To: Xudong Hao, Avi Kivity; +Cc: kvm, xiantao.zhang
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.
> > vcpu->arch.xcr0 = xcr0;
> > vcpu->guest_xcr0_loaded = 0;
> > return 0;
> > @@ -5969,7 +5971,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);
> > }
> >
> > --
> > 1.5.5
> >
> > --
> > 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
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-12 8:10 [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU Xudong Hao
2012-09-13 16:26 ` Marcelo Tosatti
@ 2012-09-13 16:38 ` Avi Kivity
2012-09-14 8:52 ` Hao, Xudong
1 sibling, 1 reply; 16+ messages in thread
From: Avi Kivity @ 2012-09-13 16:38 UTC (permalink / raw)
To: Xudong Hao; +Cc: kvm, xiantao.zhang
On 09/12/2012 11:10 AM, 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.
>
> v2 changes from v1:
> - Expand KVM_XSTATE_LAZY to 64 bits before negating it.
>
> 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);
>
Why? The guest may wish to receive #NM faults.
> 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;
> vcpu->arch.xcr0 = xcr0;
> vcpu->guest_xcr0_loaded = 0;
> return 0;
> @@ -5969,7 +5971,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
"currently", "till now", don't tell someone reading the code in six
months anything. Just say how the code works.
> + * 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);
> }
>
>
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-13 16:29 ` Marcelo Tosatti
@ 2012-09-13 16:40 ` Avi Kivity
2012-09-17 2:07 ` Hao, Xudong
2012-09-14 8:54 ` Hao, Xudong
1 sibling, 1 reply; 16+ messages in thread
From: Avi Kivity @ 2012-09-13 16:40 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Xudong Hao, kvm, xiantao.zhang
On 09/13/2012 07:29 PM, Marcelo Tosatti wrote:
> 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.
Better to have a helper function (lazy_fpu_allowed(), for example).
Variables raise the question of whether they are maintained correctly.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-13 16:38 ` Avi Kivity
@ 2012-09-14 8:52 ` Hao, Xudong
0 siblings, 0 replies; 16+ messages in thread
From: Hao, Xudong @ 2012-09-14 8:52 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm@vger.kernel.org, Zhang, Xiantao
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Avi Kivity
> Sent: Friday, September 14, 2012 12:39 AM
> To: Hao, Xudong
> Cc: kvm@vger.kernel.org; Zhang, Xiantao
> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
>
> On 09/12/2012 11:10 AM, 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.
> >
> > v2 changes from v1:
> > - Expand KVM_XSTATE_LAZY to 64 bits before negating it.
> >
> > 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);
> >
>
> Why? The guest may wish to receive #NM faults.
>
Hmm, I wanted to clear TS bit to avoid vmexit if fpu_active=1, but missing to consider the guest inside.
> > 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;
> > vcpu->arch.xcr0 = xcr0;
> > vcpu->guest_xcr0_loaded = 0;
> > return 0;
> > @@ -5969,7 +5971,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
>
> "currently", "till now", don't tell someone reading the code in six
> months anything. Just say how the code works.
>
Okay.
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-13 16:29 ` Marcelo Tosatti
2012-09-13 16:40 ` Avi Kivity
@ 2012-09-14 8:54 ` Hao, Xudong
2012-09-14 9:35 ` Marcelo Tosatti
1 sibling, 1 reply; 16+ messages in thread
From: Hao, Xudong @ 2012-09-14 8:54 UTC (permalink / raw)
To: Marcelo Tosatti, Avi Kivity; +Cc: kvm@vger.kernel.org, Zhang, Xiantao
> -----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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-14 8:54 ` Hao, Xudong
@ 2012-09-14 9:35 ` Marcelo Tosatti
0 siblings, 0 replies; 16+ messages in thread
From: Marcelo Tosatti @ 2012-09-14 9:35 UTC (permalink / raw)
To: Hao, Xudong; +Cc: Avi Kivity, kvm@vger.kernel.org, Zhang, Xiantao
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-13 16:40 ` Avi Kivity
@ 2012-09-17 2:07 ` Hao, Xudong
2012-09-17 13:30 ` Marcelo Tosatti
0 siblings, 1 reply; 16+ messages in thread
From: Hao, Xudong @ 2012-09-17 2:07 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti; +Cc: kvm@vger.kernel.org, Zhang, Xiantao
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Friday, September 14, 2012 12:40 AM
> To: Marcelo Tosatti
> Cc: Hao, Xudong; kvm@vger.kernel.org; Zhang, Xiantao
> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
>
> On 09/13/2012 07:29 PM, Marcelo Tosatti wrote:
> > 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.
>
> Better to have a helper function (lazy_fpu_allowed(), for example).
> Variables raise the question of whether they are maintained correctly.
>
I realized to modifying the fpu_active variable is incorrect, it must update exception bitmap.
To avoid the cr0 and xcrs setting order for live migrate case, how about calling fpu_activate() in kvm_set_xcr()? I can add code comments in this function calling.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index be6d549..e4646d9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
kvm_inject_gp(vcpu, 0);
return 1;
}
+ if (xcr & ~((u64)KVM_XSTATE_LAZY))
+ /* Allow fpu eager restore */
+ kvm_x86_ops->fpu_activate(vcpu);
return 0;
}
Thanks,
-Xudong
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-17 2:07 ` Hao, Xudong
@ 2012-09-17 13:30 ` Marcelo Tosatti
2012-09-18 1:08 ` Hao, Xudong
0 siblings, 1 reply; 16+ messages in thread
From: Marcelo Tosatti @ 2012-09-17 13:30 UTC (permalink / raw)
To: Hao, Xudong; +Cc: Avi Kivity, kvm@vger.kernel.org, Zhang, Xiantao
On Mon, Sep 17, 2012 at 02:07:43AM +0000, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Avi Kivity [mailto:avi@redhat.com]
> > Sent: Friday, September 14, 2012 12:40 AM
> > To: Marcelo Tosatti
> > Cc: Hao, Xudong; kvm@vger.kernel.org; Zhang, Xiantao
> > Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
> >
> > On 09/13/2012 07:29 PM, Marcelo Tosatti wrote:
> > > 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.
> >
> > Better to have a helper function (lazy_fpu_allowed(), for example).
> > Variables raise the question of whether they are maintained correctly.
> >
>
> I realized to modifying the fpu_active variable is incorrect, it must update exception bitmap.
> To avoid the cr0 and xcrs setting order for live migrate case, how about calling fpu_activate() in kvm_set_xcr()? I can add code comments in this function calling.
The objective of the change is to disable lazy fpu loading (that is,
host fpu loaded in guest and vice-versa), when some bit except the
initial tree bits set in guest XCR0 (initial tree being XSTATE_FP|XSTATE_SSE|
XSTATE_YMM). Yes?
If i get that right, then the suggestion seems to be:
static bool lazy_fpu_allowed()
{
return (vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
}
On guest entry:
if (!lazy_fpu_allowed(vcpu))
kvm_x86_ops->fpu_activate(vcpu);
if (vcpu->fpu_active)
kvm_load_guest_fpu(vcpu);
Does that make sense?
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index be6d549..e4646d9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
> kvm_inject_gp(vcpu, 0);
> return 1;
> }
> + if (xcr & ~((u64)KVM_XSTATE_LAZY))
> + /* Allow fpu eager restore */
> + kvm_x86_ops->fpu_activate(vcpu);
> return 0;
> }
>
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-17 13:30 ` Marcelo Tosatti
@ 2012-09-18 1:08 ` Hao, Xudong
2012-09-19 10:23 ` Avi Kivity
0 siblings, 1 reply; 16+ messages in thread
From: Hao, Xudong @ 2012-09-18 1:08 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm@vger.kernel.org, Zhang, Xiantao
> -----Original Message-----
> From: Marcelo Tosatti [mailto:mtosatti@redhat.com]
> Sent: Monday, September 17, 2012 9:31 PM
> To: Hao, Xudong
> Cc: Avi Kivity; kvm@vger.kernel.org; Zhang, Xiantao
> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
>
> On Mon, Sep 17, 2012 at 02:07:43AM +0000, Hao, Xudong wrote:
> > > -----Original Message-----
> > > From: Avi Kivity [mailto:avi@redhat.com]
> > > Sent: Friday, September 14, 2012 12:40 AM
> > > To: Marcelo Tosatti
> > > Cc: Hao, Xudong; kvm@vger.kernel.org; Zhang, Xiantao
> > > Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
> > >
> > > On 09/13/2012 07:29 PM, Marcelo Tosatti wrote:
> > > > 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.
> > >
> > > Better to have a helper function (lazy_fpu_allowed(), for example).
> > > Variables raise the question of whether they are maintained correctly.
> > >
> >
> > I realized to modifying the fpu_active variable is incorrect, it must update
> exception bitmap.
> > To avoid the cr0 and xcrs setting order for live migrate case, how about
> calling fpu_activate() in kvm_set_xcr()? I can add code comments in this
> function calling.
>
> The objective of the change is to disable lazy fpu loading (that is,
> host fpu loaded in guest and vice-versa), when some bit except the
> initial tree bits set in guest XCR0 (initial tree being XSTATE_FP|XSTATE_SSE|
> XSTATE_YMM). Yes?
>
Yes, it's just the object.
> If i get that right, then the suggestion seems to be:
>
> static bool lazy_fpu_allowed()
> {
> return (vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
> }
>
That may be:
static bool lazy_fpu_allowed()
{
return !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
}
> On guest entry:
> if (!lazy_fpu_allowed(vcpu))
> kvm_x86_ops->fpu_activate(vcpu);
Yes, we can add it into guest entry: kvm_set_xcr(). Avi, other comments?
> if (vcpu->fpu_active)
> kvm_load_guest_fpu(vcpu);
>
>
> Does that make sense?
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-18 1:08 ` Hao, Xudong
@ 2012-09-19 10:23 ` Avi Kivity
2012-09-20 1:43 ` Hao, Xudong
0 siblings, 1 reply; 16+ messages in thread
From: Avi Kivity @ 2012-09-19 10:23 UTC (permalink / raw)
To: Hao, Xudong; +Cc: Marcelo Tosatti, kvm@vger.kernel.org, Zhang, Xiantao
On 09/18/2012 04:08 AM, Hao, Xudong wrote:
>>
>> The objective of the change is to disable lazy fpu loading (that is,
>> host fpu loaded in guest and vice-versa),
Not vice versa. We allow the guest fpu loaded in the host, but save it
on heavyweight exit or task switch.
when some bit except the
>> initial tree bits set in guest XCR0 (initial tree being XSTATE_FP|XSTATE_SSE|
>> XSTATE_YMM). Yes?
>>
>
> Yes, it's just the object.
>
>> If i get that right, then the suggestion seems to be:
>>
>> static bool lazy_fpu_allowed()
>> {
>> return (vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
>> }
>>
>
> That may be:
>
> static bool lazy_fpu_allowed()
> {
> return !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
> }
Shouldn't it depend on cr4.osxsave as well?
>
>> On guest entry:
>> if (!lazy_fpu_allowed(vcpu))
>> kvm_x86_ops->fpu_activate(vcpu);
>
But we already have that:
if (vcpu->fpu_active)
kvm_load_guest_fpu(vcpu);
so why not manage fpu_active to be always set when needed? I don't want
more checks in the entry path.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-19 10:23 ` Avi Kivity
@ 2012-09-20 1:43 ` Hao, Xudong
2012-09-20 9:19 ` Avi Kivity
0 siblings, 1 reply; 16+ messages in thread
From: Hao, Xudong @ 2012-09-20 1:43 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm@vger.kernel.org, Zhang, Xiantao
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Wednesday, September 19, 2012 6:24 PM
> To: Hao, Xudong
> Cc: Marcelo Tosatti; kvm@vger.kernel.org; Zhang, Xiantao
> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
> > That may be:
> >
> > static bool lazy_fpu_allowed()
> > {
> > return !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
> > }
>
> Shouldn't it depend on cr4.osxsave as well?
>
It do need to check cr4.osxsave due to a separate function.
static bool lazy_fpu_allowed(struct kvm_vcpu *vcpu)
{
return !kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) ||
!(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
}
> >
> >> On guest entry:
> >> if (!lazy_fpu_allowed(vcpu))
> >> kvm_x86_ops->fpu_activate(vcpu);
> >
>
> But we already have that:
>
> if (vcpu->fpu_active)
> kvm_load_guest_fpu(vcpu);
>
> so why not manage fpu_active to be always set when needed? I don't want
> more checks in the entry path.
>
I means add fpu_active() in kvm_set_xcr(), not in guest entry. Then the fpu_active will be set always when guest initialize xstate.
@@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
kvm_inject_gp(vcpu, 0);
return 1;
}
+ if (!lazy_fpu_allowed(vcpu))
+ kvm_x86_ops->fpu_activate(vcpu);
return 0;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-20 1:43 ` Hao, Xudong
@ 2012-09-20 9:19 ` Avi Kivity
2012-09-21 8:47 ` Hao, Xudong
0 siblings, 1 reply; 16+ messages in thread
From: Avi Kivity @ 2012-09-20 9:19 UTC (permalink / raw)
To: Hao, Xudong; +Cc: Marcelo Tosatti, kvm@vger.kernel.org, Zhang, Xiantao
On 09/20/2012 04:43 AM, Hao, Xudong wrote:
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Wednesday, September 19, 2012 6:24 PM
>> To: Hao, Xudong
>> Cc: Marcelo Tosatti; kvm@vger.kernel.org; Zhang, Xiantao
>> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
>> > That may be:
>> >
>> > static bool lazy_fpu_allowed()
>> > {
>> > return !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
>> > }
>>
>> Shouldn't it depend on cr4.osxsave as well?
>>
>
> It do need to check cr4.osxsave due to a separate function.
>
> static bool lazy_fpu_allowed(struct kvm_vcpu *vcpu)
> {
> return !kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) ||
> !(vcpu->arch.xcr0 & ~((u64)KVM_XSTATE_LAZY));
> }
Yes.
>
>> >
>> >> On guest entry:
>> >> if (!lazy_fpu_allowed(vcpu))
>> >> kvm_x86_ops->fpu_activate(vcpu);
>> >
>>
>> But we already have that:
>>
>> if (vcpu->fpu_active)
>> kvm_load_guest_fpu(vcpu);
>>
>> so why not manage fpu_active to be always set when needed? I don't want
>> more checks in the entry path.
>>
> I means add fpu_active() in kvm_set_xcr(), not in guest entry. Then the fpu_active will be set always when guest initialize xstate.
>
> @@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
> kvm_inject_gp(vcpu, 0);
> return 1;
> }
> + if (!lazy_fpu_allowed(vcpu))
> + kvm_x86_ops->fpu_activate(vcpu);
> return 0;
>
And of course on cr4 update. So a function update_lazy_fpu() to be
called from both places is needed.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-20 9:19 ` Avi Kivity
@ 2012-09-21 8:47 ` Hao, Xudong
2012-09-23 9:03 ` Avi Kivity
0 siblings, 1 reply; 16+ messages in thread
From: Hao, Xudong @ 2012-09-21 8:47 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm@vger.kernel.org, Zhang, Xiantao
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Thursday, September 20, 2012 5:20 PM
> To: Hao, Xudong
> Cc: Marcelo Tosatti; kvm@vger.kernel.org; Zhang, Xiantao
> Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
> >> >> On guest entry:
> >> >> if (!lazy_fpu_allowed(vcpu))
> >> >> kvm_x86_ops->fpu_activate(vcpu);
> >> >
> >>
> >> But we already have that:
> >>
> >> if (vcpu->fpu_active)
> >> kvm_load_guest_fpu(vcpu);
> >>
> >> so why not manage fpu_active to be always set when needed? I don't
> want
> >> more checks in the entry path.
> >>
> > I means add fpu_active() in kvm_set_xcr(), not in guest entry. Then the
> fpu_active will be set always when guest initialize xstate.
> >
> > @@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index,
> u64 xcr)
> > kvm_inject_gp(vcpu, 0);
> > return 1;
> > }
> > + if (!lazy_fpu_allowed(vcpu))
> > + kvm_x86_ops->fpu_activate(vcpu);
> > return 0;
> >
>
> And of course on cr4 update. So a function update_lazy_fpu() to be
> called from both places is needed.
>
Complete consideration, thanks.
So I will define a function update_lazy_fpu(), insert it into kvm_set_xcr() and handle_cr(). Comments?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
2012-09-21 8:47 ` Hao, Xudong
@ 2012-09-23 9:03 ` Avi Kivity
0 siblings, 0 replies; 16+ messages in thread
From: Avi Kivity @ 2012-09-23 9:03 UTC (permalink / raw)
To: Hao, Xudong; +Cc: Marcelo Tosatti, kvm@vger.kernel.org, Zhang, Xiantao
On 09/21/2012 11:47 AM, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Avi Kivity [mailto:avi@redhat.com]
> > Sent: Thursday, September 20, 2012 5:20 PM
> > To: Hao, Xudong
> > Cc: Marcelo Tosatti; kvm@vger.kernel.org; Zhang, Xiantao
> > Subject: Re: [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU
> > >> >> On guest entry:
> > >> >> if (!lazy_fpu_allowed(vcpu))
> > >> >> kvm_x86_ops->fpu_activate(vcpu);
> > >> >
> > >>
> > >> But we already have that:
> > >>
> > >> if (vcpu->fpu_active)
> > >> kvm_load_guest_fpu(vcpu);
> > >>
> > >> so why not manage fpu_active to be always set when needed? I don't
> > want
> > >> more checks in the entry path.
> > >>
> > > I means add fpu_active() in kvm_set_xcr(), not in guest entry. Then the
> > fpu_active will be set always when guest initialize xstate.
> > >
> > > @@ -574,6 +574,9 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index,
> > u64 xcr)
> > > kvm_inject_gp(vcpu, 0);
> > > return 1;
> > > }
> > > + if (!lazy_fpu_allowed(vcpu))
> > > + kvm_x86_ops->fpu_activate(vcpu);
> > > return 0;
> > >
> >
> > And of course on cr4 update. So a function update_lazy_fpu() to be
> > called from both places is needed.
> >
>
> Complete consideration, thanks.
>
> So I will define a function update_lazy_fpu(), insert it into kvm_set_xcr() and handle_cr(). Comments?
Sounds good.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-09-23 9:03 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-12 8:10 [PATCH v3] kvm/fpu: Enable fully eager restore kvm FPU Xudong Hao
2012-09-13 16:26 ` Marcelo Tosatti
2012-09-13 16:29 ` Marcelo Tosatti
2012-09-13 16:40 ` Avi Kivity
2012-09-17 2:07 ` Hao, Xudong
2012-09-17 13:30 ` Marcelo Tosatti
2012-09-18 1:08 ` Hao, Xudong
2012-09-19 10:23 ` Avi Kivity
2012-09-20 1:43 ` Hao, Xudong
2012-09-20 9:19 ` Avi Kivity
2012-09-21 8:47 ` Hao, Xudong
2012-09-23 9:03 ` Avi Kivity
2012-09-14 8:54 ` Hao, Xudong
2012-09-14 9:35 ` Marcelo Tosatti
2012-09-13 16:38 ` Avi Kivity
2012-09-14 8:52 ` Hao, Xudong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).