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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
--
Gleb.