* [PATCH 0/2] KVM_SET_XCRS fixes
@ 2013-10-17 14:50 Paolo Bonzini
2013-10-17 14:50 ` [PATCH 1/2] KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paolo Bonzini @ 2013-10-17 14:50 UTC (permalink / raw)
To: linux-kernel; +Cc: gleb, agraf, chao.zhou, magnus, kvm
The first patch fixes bugs 63121 and 63131 (yeah, all kernel bugs
end with 1). The second patch fixes a typo (the same typo exists
in QEMU).
Paolo Bonzini (2):
KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE
KVM: x86: fix KVM_SET_XCRS loop
arch/x86/kvm/x86.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE 2013-10-17 14:50 [PATCH 0/2] KVM_SET_XCRS fixes Paolo Bonzini @ 2013-10-17 14:50 ` Paolo Bonzini 2013-10-17 14:50 ` [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop Paolo Bonzini 2013-10-31 8:32 ` [PATCH 0/2] KVM_SET_XCRS fixes Gleb Natapov 2 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2013-10-17 14:50 UTC (permalink / raw) To: linux-kernel; +Cc: gleb, agraf, chao.zhou, magnus, kvm The KVM_SET_XCRS ioctl must accept anything that KVM_GET_XCRS could return. XCR0's bit 0 is always 1 in real processors with XSAVE, and KVM_GET_XCRS will always leave bit 0 set even if the emulated processor does not have XSAVE. So, KVM_SET_XCRS must ignore that bit when checking for attempts to enable unsupported save states. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- Introduced between v2 and v3, when I stopped hardcoding XSTATE_FPSSE in guest_supported_xcr0. arch/x86/kvm/x86.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c951c71..f4e1391 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -577,6 +577,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu) int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr) { u64 xcr0; + u64 valid_bits; /* Only support XCR_XFEATURE_ENABLED_MASK(xcr0) now */ if (index != XCR_XFEATURE_ENABLED_MASK) @@ -586,8 +587,16 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr) return 1; if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE)) return 1; - if (xcr0 & ~vcpu->arch.guest_supported_xcr0) + + /* + * Do not allow the guest to set bits that we do not support + * saving. However, xcr0 bit 0 is always set, even if the + * emulated CPU does not support XSAVE (see fx_init). + */ + valid_bits = vcpu->arch.guest_supported_xcr0 | XSTATE_FP; + if (xcr0 & ~valid_bits) return 1; + kvm_put_guest_xcr0(vcpu); vcpu->arch.xcr0 = xcr0; return 0; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop 2013-10-17 14:50 [PATCH 0/2] KVM_SET_XCRS fixes Paolo Bonzini 2013-10-17 14:50 ` [PATCH 1/2] KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE Paolo Bonzini @ 2013-10-17 14:50 ` Paolo Bonzini 2013-10-18 0:04 ` Marcelo Tosatti 2013-10-31 8:32 ` [PATCH 0/2] KVM_SET_XCRS fixes Gleb Natapov 2 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2013-10-17 14:50 UTC (permalink / raw) To: linux-kernel; +Cc: gleb, agraf, chao.zhou, magnus, kvm The loop was always using 0 as the index. This means that any rubbish after the first element of the array went undetected. It seems reasonable to assume that no KVM userspace did that. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/x86.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index f4e1391..f91dff2 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3062,9 +3062,9 @@ static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu, for (i = 0; i < guest_xcrs->nr_xcrs; i++) /* Only support XCR0 currently */ - if (guest_xcrs->xcrs[0].xcr == XCR_XFEATURE_ENABLED_MASK) { + if (guest_xcrs->xcrs[i].xcr == XCR_XFEATURE_ENABLED_MASK) { r = __kvm_set_xcr(vcpu, XCR_XFEATURE_ENABLED_MASK, - guest_xcrs->xcrs[0].value); + guest_xcrs->xcrs[i].value); break; } if (r) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop 2013-10-17 14:50 ` [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop Paolo Bonzini @ 2013-10-18 0:04 ` Marcelo Tosatti 2013-10-18 6:46 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Marcelo Tosatti @ 2013-10-18 0:04 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, gleb, agraf, chao.zhou, magnus, kvm On Thu, Oct 17, 2013 at 04:50:47PM +0200, Paolo Bonzini wrote: > The loop was always using 0 as the index. This means that > any rubbish after the first element of the array went undetected. > It seems reasonable to assume that no KVM userspace did that. It is not a typo, look at __kvm_set_xcr when setting guest_xcrs->xcrs[i].value, with i != 0. The code is not prepared to deal with XCR != 0 (because its not implemented in hw). > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/x86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index f4e1391..f91dff2 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3062,9 +3062,9 @@ static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu, > > for (i = 0; i < guest_xcrs->nr_xcrs; i++) > /* Only support XCR0 currently */ > - if (guest_xcrs->xcrs[0].xcr == XCR_XFEATURE_ENABLED_MASK) { > + if (guest_xcrs->xcrs[i].xcr == XCR_XFEATURE_ENABLED_MASK) { > r = __kvm_set_xcr(vcpu, XCR_XFEATURE_ENABLED_MASK, > - guest_xcrs->xcrs[0].value); > + guest_xcrs->xcrs[i].value); > break; > } > if (r) > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop 2013-10-18 0:04 ` Marcelo Tosatti @ 2013-10-18 6:46 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2013-10-18 6:46 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: linux-kernel, gleb, agraf, chao.zhou, magnus, kvm Il 18/10/2013 02:04, Marcelo Tosatti ha scritto: > On Thu, Oct 17, 2013 at 04:50:47PM +0200, Paolo Bonzini wrote: >> The loop was always using 0 as the index. This means that >> any rubbish after the first element of the array went undetected. >> It seems reasonable to assume that no KVM userspace did that. > > It is not a typo, look at __kvm_set_xcr when setting > guest_xcrs->xcrs[i].value, with i != 0. i is not the index of the XCR register, it's the index in the array. The index is currently hardcoded to 0 when it is passed to __kvm_set_xcr; but very reasonably __kvm_set_xcr returns 1 when index != 0. IMO even the "if" in kvm_vcpu_ioctl_x86_set_xcrs is wrong: setting XCR above XCR0 should fail KVM_SET_XCRS, while currently is ignored. The body of the loop should be simply: r = __kvm_set_xcr(vcpu, guest_xcrs->xcrs[i].xcr, guest_xcrs->xcrs[i].value); if (r) break; Paolo > The code is not prepared to deal with XCR != 0 (because its not > implemented in hw). > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> arch/x86/kvm/x86.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index f4e1391..f91dff2 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -3062,9 +3062,9 @@ static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu, >> >> for (i = 0; i < guest_xcrs->nr_xcrs; i++) >> /* Only support XCR0 currently */ >> - if (guest_xcrs->xcrs[0].xcr == XCR_XFEATURE_ENABLED_MASK) { >> + if (guest_xcrs->xcrs[i].xcr == XCR_XFEATURE_ENABLED_MASK) { >> r = __kvm_set_xcr(vcpu, XCR_XFEATURE_ENABLED_MASK, >> - guest_xcrs->xcrs[0].value); >> + guest_xcrs->xcrs[i].value); >> break; >> } >> if (r) >> -- >> 1.8.3.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] KVM_SET_XCRS fixes 2013-10-17 14:50 [PATCH 0/2] KVM_SET_XCRS fixes Paolo Bonzini 2013-10-17 14:50 ` [PATCH 1/2] KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE Paolo Bonzini 2013-10-17 14:50 ` [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop Paolo Bonzini @ 2013-10-31 8:32 ` Gleb Natapov 2 siblings, 0 replies; 6+ messages in thread From: Gleb Natapov @ 2013-10-31 8:32 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, agraf, chao.zhou, magnus, kvm On Thu, Oct 17, 2013 at 04:50:45PM +0200, Paolo Bonzini wrote: > The first patch fixes bugs 63121 and 63131 (yeah, all kernel bugs > end with 1). The second patch fixes a typo (the same typo exists > in QEMU). > > Paolo Bonzini (2): > KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE > KVM: x86: fix KVM_SET_XCRS loop > > arch/x86/kvm/x86.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > Reviewed-by: Gleb Natapov <gleb@redhat.com> -- Gleb. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-31 8:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-17 14:50 [PATCH 0/2] KVM_SET_XCRS fixes Paolo Bonzini 2013-10-17 14:50 ` [PATCH 1/2] KVM: x86: fix KVM_SET_XCRS for CPUs that do not support XSAVE Paolo Bonzini 2013-10-17 14:50 ` [PATCH 2/2] KVM: x86: fix KVM_SET_XCRS loop Paolo Bonzini 2013-10-18 0:04 ` Marcelo Tosatti 2013-10-18 6:46 ` Paolo Bonzini 2013-10-31 8:32 ` [PATCH 0/2] KVM_SET_XCRS fixes Gleb Natapov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.