> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On
> Behalf Of Avi Kivity
> Sent: Thursday, September 06, 2012 4:16 PM
> To: Hao, Xudong
> Cc: [email protected]; Zhang, Xiantao; [email protected]
> 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:[email protected]]
> >> Sent: Wednesday, September 05, 2012 9:13 PM
> >> To: Hao, Xudong
> >> Cc: [email protected]; Zhang, Xiantao; [email protected]
> >> 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 <[email protected]>
> >> > ---
> >> > 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
