* [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU @ 2012-09-05 1:26 Xudong Hao 2012-09-05 13:12 ` Avi Kivity 0 siblings, 1 reply; 9+ messages in thread From: Xudong Hao @ 2012-09-05 1:26 UTC (permalink / raw) To: avi; +Cc: kvm, xiantao.zhang, Joerg.Roedel, Xudong Hao 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); } -- 1.5.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-05 1:26 [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU Xudong Hao @ 2012-09-05 13:12 ` Avi Kivity 2012-09-06 2:13 ` Hao, Xudong 0 siblings, 1 reply; 9+ messages in thread From: Avi Kivity @ 2012-09-05 13:12 UTC (permalink / raw) To: Xudong Hao; +Cc: kvm, xiantao.zhang, Joerg.Roedel 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. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-05 13:12 ` Avi Kivity @ 2012-09-06 2:13 ` Hao, Xudong 2012-09-06 8:16 ` Avi Kivity 0 siblings, 1 reply; 9+ messages in thread From: Hao, Xudong @ 2012-09-06 2:13 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.com > -----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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-06 2:13 ` Hao, Xudong @ 2012-09-06 8:16 ` Avi Kivity 2012-09-10 3:29 ` Hao, Xudong 0 siblings, 1 reply; 9+ messages in thread From: Avi Kivity @ 2012-09-06 8:16 UTC (permalink / raw) To: Hao, Xudong; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-06 8:16 ` Avi Kivity @ 2012-09-10 3:29 ` Hao, Xudong 2012-09-10 8:07 ` Avi Kivity 0 siblings, 1 reply; 9+ messages in thread From: Hao, Xudong @ 2012-09-10 3:29 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.com > -----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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-10 3:29 ` Hao, Xudong @ 2012-09-10 8:07 ` Avi Kivity 2012-09-11 6:43 ` Hao, Xudong 0 siblings, 1 reply; 9+ messages in thread From: Avi Kivity @ 2012-09-10 8:07 UTC (permalink / raw) To: Hao, Xudong; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.com 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. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-10 8:07 ` Avi Kivity @ 2012-09-11 6:43 ` Hao, Xudong 2012-09-11 7:54 ` Avi Kivity 0 siblings, 1 reply; 9+ messages in thread From: Hao, Xudong @ 2012-09-11 6:43 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.com > -----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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-11 6:43 ` Hao, Xudong @ 2012-09-11 7:54 ` Avi Kivity 2012-09-11 9:20 ` Hao, Xudong 0 siblings, 1 reply; 9+ messages in thread From: Avi Kivity @ 2012-09-11 7:54 UTC (permalink / raw) To: Hao, Xudong; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.com 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. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU 2012-09-11 7:54 ` Avi Kivity @ 2012-09-11 9:20 ` Hao, Xudong 0 siblings, 0 replies; 9+ messages in thread From: Hao, Xudong @ 2012-09-11 9:20 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm@vger.kernel.org, Zhang, Xiantao, Joerg.Roedel@amd.com > -----Original Message----- > From: Avi Kivity [mailto:avi@redhat.com] > Sent: Tuesday, September 11, 2012 3:54 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/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. > Okay, I got it. fpu_active(vcpu) in __kvm_set_xcr () read guest cr0, so KVM_SET_XCRS depends on KVM_SET_SREGS in live migration case. Here only set fpu_active=1 in __kvm_set_xcr(), and clear TS bit in set_cr0 should solve this issue. --- 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; ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-09-11 9:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-05 1:26 [PATCH v2] kvm/fpu: Enable fully eager restore kvm FPU Xudong Hao 2012-09-05 13:12 ` Avi Kivity 2012-09-06 2:13 ` Hao, Xudong 2012-09-06 8:16 ` Avi Kivity 2012-09-10 3:29 ` Hao, Xudong 2012-09-10 8:07 ` Avi Kivity 2012-09-11 6:43 ` Hao, Xudong 2012-09-11 7:54 ` Avi Kivity 2012-09-11 9:20 ` Hao, Xudong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox