* [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 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).