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