As soon as the kernel will support the XSAVE extensions in Skylake
processors, we will want both userspace and the hypervisor to run
guests without showing any trace of the new features (because
support for them in the hypervisor will come later).
This series does exactly this. Patches 1 and 3 ensures that userspace
does not get access to features not supported in the hypervisor.
Patch 2 does the same for the guest.
The effect of these patches is already visible if you use the QEMU
patches posted last week together with a command line such as
"-cpu SandyBridge,-avx". Previously, AVX state was always sent
by QEMU during migration. If both the QEMU and hypervisor changes are
applied, instead, disabling AVX will also prevent the AVX state from
being transferred.
Paolo
v1->v2: new patches 1 and 3, patch 2 mostly rewritten
Paolo Bonzini (3):
KVM: x86: mask unsupported XSAVE entries from leaf 0Dh index 0
KVM: x86: prevent setting unsupported XSAVE states
KVM: x86: only copy XSAVE state for the supported features
arch/x86/include/asm/kvm_host.h | 2 ++
arch/x86/kvm/cpuid.c | 37 ++++++++++++++++++++++++++++++++++++-
arch/x86/kvm/x86.c | 26 ++++++++++++++++++++------
arch/x86/kvm/x86.h | 1 +
4 files changed, 59 insertions(+), 7 deletions(-)
--
1.8.3.1
XSAVE entries that KVM does not support are reported by
KVM_GET_SUPPORTED_CPUID for leaf 0Dh index 0 if the host supports them;
they should be left out unless there is also hypervisor support for them.
Sub-leafs are correctly handled in supported_xcr0_bit, fix index 0
to match.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/cpuid.c | 4 +++-
arch/x86/kvm/x86.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b110fe6..a03a9fa 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -182,7 +182,7 @@ static bool supported_xcr0_bit(unsigned bit)
{
u64 mask = ((u64)1 << bit);
- return mask & (XSTATE_FP | XSTATE_SSE | XSTATE_YMM) & host_xcr0;
+ return mask & KVM_SUPPORTED_XCR0 & host_xcr0;
}
#define F(x) bit(X86_FEATURE_##x)
@@ -383,6 +383,8 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
case 0xd: {
int idx, i;
+ entry->eax &= host_xcr0 & KVM_SUPPORTED_XCR0;
+ entry->edx &= (host_xcr0 & KVM_SUPPORTED_XCR0) >> 32;
entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
for (idx = 1, i = 1; idx < 64; ++idx) {
if (*nent >= maxnent)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index e224f7a..587fb9e 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -122,6 +122,7 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
gva_t addr, void *val, unsigned int bytes,
struct x86_exception *exception);
+#define KVM_SUPPORTED_XCR0 (XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
extern u64 host_xcr0;
extern struct static_key kvm_no_apic_vcpu;
--
1.8.3.1
A guest can still attempt to save and restore XSAVE states even if they
have been masked in CPUID leaf 0Dh. This usually is not visible to
the guest, but is still wrong: "Any attempt to set a reserved bit (as
determined by the contents of EAX and EDX after executing CPUID with
EAX=0DH, ECX= 0H) in XCR0 for a given processor will result in a #GP
exception".
The patch also performs the same checks as __kvm_set_xcr in KVM_SET_XSAVE.
This catches migration from newer to older kernel/processor before the
guest starts running.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/cpuid.c | 7 +++++++
arch/x86/kvm/x86.c | 17 ++++++++++++++---
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c76ff74..1553542 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -389,6 +389,7 @@ struct kvm_vcpu_arch {
struct fpu guest_fpu;
u64 xcr0;
+ u64 supported_xcr0;
struct kvm_pio_request pio;
void *pio_data;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a03a9fa..fa754a8 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -46,6 +46,13 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
apic->lapic_timer.timer_mode_mask = 1 << 17;
}
+ best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
+ if (!best)
+ return;
+ vcpu->arch.supported_xcr0 =
+ (best->eax | ((u64)best->edx << 32)) &
+ host_xcr0 & KVM_SUPPORTED_XCR0;
+
kvm_pmu_cpuid_update(vcpu);
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e5ca72a..cc8c403 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -586,7 +586,7 @@ 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 & ~host_xcr0)
+ if (xcr0 & ~vcpu->arch.supported_xcr0)
return 1;
kvm_put_guest_xcr0(vcpu);
vcpu->arch.xcr0 = xcr0;
@@ -3003,10 +3003,19 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
u64 xstate_bv =
*(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)];
- if (cpu_has_xsave)
+ if (cpu_has_xsave) {
+ /*
+ * Here we allow setting states that are not present in
+ * CPUID leaf 0xD, index 0, EDX:EAX. This is for compatibility
+ * with old userspace.
+ */
+ if (xstate_bv & ~KVM_SUPPORTED_XCR0)
+ return -EINVAL;
+ if (xstate_bv & ~host_xcr0)
+ return -EINVAL;
memcpy(&vcpu->arch.guest_fpu.state->xsave,
guest_xsave->region, xstate_size);
- else {
+ } else {
if (xstate_bv & ~XSTATE_FPSSE)
return -EINVAL;
memcpy(&vcpu->arch.guest_fpu.state->fxsave,
@@ -6940,6 +6948,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
vcpu->arch.ia32_tsc_adjust_msr = 0x0;
vcpu->arch.pv_time_enabled = false;
+
+ vcpu->arch.supported_xcr0 = XSTATE_FPSSE;
+
kvm_async_pf_hash_reset(vcpu);
kvm_pmu_init(vcpu);
--
1.8.3.1
This makes the interface more deterministic for userspace, which can expect
(after configuring only the features it supports) to get exactly the same
state from the kernel, independent of the host CPU and kernel version.
Suggested-by: Gleb Natapov <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/cpuid.c | 28 +++++++++++++++++++++++++++-
arch/x86/kvm/x86.c | 9 ++++++---
3 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1553542..a21dcee 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -390,6 +390,7 @@ struct kvm_vcpu_arch {
struct fpu guest_fpu;
u64 xcr0;
u64 supported_xcr0;
+ u32 xstate_size;
struct kvm_pio_request pio;
void *pio_data;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index fa754a8..00f5923e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -23,6 +23,26 @@
#include "mmu.h"
#include "trace.h"
+static u32 xstate_required_size(u64 xstate_bv)
+{
+ int feature_bit = 0;
+ u32 ret = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
+
+ xstate_bv &= ~XSTATE_FPSSE;
+ while (xstate_bv) {
+ if (xstate_bv & 0x1) {
+ u32 eax, ebx, ecx, edx;
+ cpuid_count(0xD, feature_bit, &eax, &ebx, &ecx, &edx);
+ ret = max(ret, eax + ebx);
+ }
+
+ xstate_bv >>= 1;
+ feature_bit++;
+ }
+
+ return ret;
+}
+
void kvm_update_cpuid(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
@@ -46,12 +66,18 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
apic->lapic_timer.timer_mode_mask = 1 << 17;
}
+ /*
+ * Adjust which states will be returned by KVM_GET_XSAVE.
+ * x87 and SSE state is always returned, the others are only
+ * returned if they are exposed to the guest.
+ */
best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
if (!best)
return;
vcpu->arch.supported_xcr0 =
- (best->eax | ((u64)best->edx << 32)) &
+ (best->eax | ((u64)best->edx << 32) | XSTATE_FPSSE) &
host_xcr0 & KVM_SUPPORTED_XCR0;
+ vcpu->arch.xstate_size = xstate_required_size(vcpu->arch.supported_xcr0);
kvm_pmu_cpuid_update(vcpu);
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cc8c403..7c36099 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2984,11 +2984,13 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
struct kvm_xsave *guest_xsave)
{
- if (cpu_has_xsave)
+ if (cpu_has_xsave) {
+ *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] &=
+ vcpu->arch.supported_xcr0;
memcpy(guest_xsave->region,
&vcpu->arch.guest_fpu.state->xsave,
- xstate_size);
- else {
+ vcpu->arch.xstate_size);
+ } else {
memcpy(guest_xsave->region,
&vcpu->arch.guest_fpu.state->fxsave,
sizeof(struct i387_fxsave_struct));
@@ -6950,6 +6952,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
vcpu->arch.pv_time_enabled = false;
vcpu->arch.supported_xcr0 = XSTATE_FPSSE;
+ vcpu->arch.xstate_size = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
kvm_async_pf_hash_reset(vcpu);
kvm_pmu_init(vcpu);
--
1.8.3.1
On Mon, Sep 16, 2013 at 04:26:30PM +0200, Paolo Bonzini wrote:
> A guest can still attempt to save and restore XSAVE states even if they
> have been masked in CPUID leaf 0Dh. This usually is not visible to
> the guest, but is still wrong: "Any attempt to set a reserved bit (as
> determined by the contents of EAX and EDX after executing CPUID with
> EAX=0DH, ECX= 0H) in XCR0 for a given processor will result in a #GP
> exception".
>
> The patch also performs the same checks as __kvm_set_xcr in KVM_SET_XSAVE.
> This catches migration from newer to older kernel/processor before the
> guest starts running.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/cpuid.c | 7 +++++++
> arch/x86/kvm/x86.c | 17 ++++++++++++++---
> 3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c76ff74..1553542 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -389,6 +389,7 @@ struct kvm_vcpu_arch {
>
> struct fpu guest_fpu;
> u64 xcr0;
> + u64 supported_xcr0;
>
Lets prefix it with guest_.
> struct kvm_pio_request pio;
> void *pio_data;
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a03a9fa..fa754a8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -46,6 +46,13 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
> apic->lapic_timer.timer_mode_mask = 1 << 17;
> }
>
> + best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
> + if (!best)
> + return;
If QMEU will set AVX in 0xD and then remove cpuid leaf at all
supported_xcr0 will still have it.
> + vcpu->arch.supported_xcr0 =
> + (best->eax | ((u64)best->edx << 32)) &
> + host_xcr0 & KVM_SUPPORTED_XCR0;
> +
> kvm_pmu_cpuid_update(vcpu);
> }
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5ca72a..cc8c403 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -586,7 +586,7 @@ 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 & ~host_xcr0)
> + if (xcr0 & ~vcpu->arch.supported_xcr0)
> return 1;
> kvm_put_guest_xcr0(vcpu);
> vcpu->arch.xcr0 = xcr0;
> @@ -3003,10 +3003,19 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
> u64 xstate_bv =
> *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)];
>
> - if (cpu_has_xsave)
> + if (cpu_has_xsave) {
> + /*
> + * Here we allow setting states that are not present in
> + * CPUID leaf 0xD, index 0, EDX:EAX. This is for compatibility
> + * with old userspace.
> + */
> + if (xstate_bv & ~KVM_SUPPORTED_XCR0)
> + return -EINVAL;
> + if (xstate_bv & ~host_xcr0)
> + return -EINVAL;
> memcpy(&vcpu->arch.guest_fpu.state->xsave,
> guest_xsave->region, xstate_size);
> - else {
> + } else {
> if (xstate_bv & ~XSTATE_FPSSE)
> return -EINVAL;
> memcpy(&vcpu->arch.guest_fpu.state->fxsave,
> @@ -6940,6 +6948,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>
> vcpu->arch.ia32_tsc_adjust_msr = 0x0;
> vcpu->arch.pv_time_enabled = false;
> +
> + vcpu->arch.supported_xcr0 = XSTATE_FPSSE;
> +
Why is this needed? It will make make __kvm_set_xcr() succeed if attempt
is made to set SSE bit when it is not supported in cpuid. This may not
be an issue in practice, but for clarity it is better for supported_xcr0
to contain only what is supported in guest's cpuid bits and handle the
fact that FP/SSE state should always be copied to/from userspace in
kvm_vcpu_ioctl_x86_(set|get)_xsave functions.
> kvm_async_pf_hash_reset(vcpu);
> kvm_pmu_init(vcpu);
>
> --
> 1.8.3.1
>
--
Gleb.
Il 22/09/2013 11:33, Gleb Natapov ha scritto:
> On Mon, Sep 16, 2013 at 04:26:30PM +0200, Paolo Bonzini wrote:
>> A guest can still attempt to save and restore XSAVE states even if they
>> have been masked in CPUID leaf 0Dh. This usually is not visible to
>> the guest, but is still wrong: "Any attempt to set a reserved bit (as
>> determined by the contents of EAX and EDX after executing CPUID with
>> EAX=0DH, ECX= 0H) in XCR0 for a given processor will result in a #GP
>> exception".
>>
>> The patch also performs the same checks as __kvm_set_xcr in KVM_SET_XSAVE.
>> This catches migration from newer to older kernel/processor before the
>> guest starts running.
>>
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 +
>> arch/x86/kvm/cpuid.c | 7 +++++++
>> arch/x86/kvm/x86.c | 17 ++++++++++++++---
>> 3 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index c76ff74..1553542 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -389,6 +389,7 @@ struct kvm_vcpu_arch {
>>
>> struct fpu guest_fpu;
>> u64 xcr0;
>> + u64 supported_xcr0;
>>
> Lets prefix it with guest_.
Ok.
>> struct kvm_pio_request pio;
>> void *pio_data;
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index a03a9fa..fa754a8 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -46,6 +46,13 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>> apic->lapic_timer.timer_mode_mask = 1 << 17;
>> }
>>
>> + best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
>> + if (!best)
>> + return;
> If QMEU will set AVX in 0xD and then remove cpuid leaf at all
> supported_xcr0 will still have it.
Right. Will fix.
>> + vcpu->arch.supported_xcr0 =
>> + (best->eax | ((u64)best->edx << 32)) &
>> + host_xcr0 & KVM_SUPPORTED_XCR0;
>> +
>> kvm_pmu_cpuid_update(vcpu);
>> }
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index e5ca72a..cc8c403 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -586,7 +586,7 @@ 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 & ~host_xcr0)
>> + if (xcr0 & ~vcpu->arch.supported_xcr0)
>> return 1;
>> kvm_put_guest_xcr0(vcpu);
>> vcpu->arch.xcr0 = xcr0;
>> @@ -3003,10 +3003,19 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>> u64 xstate_bv =
>> *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)];
>>
>> - if (cpu_has_xsave)
>> + if (cpu_has_xsave) {
>> + /*
>> + * Here we allow setting states that are not present in
>> + * CPUID leaf 0xD, index 0, EDX:EAX. This is for compatibility
>> + * with old userspace.
>> + */
>> + if (xstate_bv & ~KVM_SUPPORTED_XCR0)
>> + return -EINVAL;
>> + if (xstate_bv & ~host_xcr0)
>> + return -EINVAL;
>> memcpy(&vcpu->arch.guest_fpu.state->xsave,
>> guest_xsave->region, xstate_size);
>> - else {
>> + } else {
>> if (xstate_bv & ~XSTATE_FPSSE)
>> return -EINVAL;
>> memcpy(&vcpu->arch.guest_fpu.state->fxsave,
>> @@ -6940,6 +6948,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>>
>> vcpu->arch.ia32_tsc_adjust_msr = 0x0;
>> vcpu->arch.pv_time_enabled = false;
>> +
>> + vcpu->arch.supported_xcr0 = XSTATE_FPSSE;
>> +
> Why is this needed? It will make make __kvm_set_xcr() succeed if attempt
> is made to set SSE bit when it is not supported in cpuid. This may not
> be an issue in practice, but for clarity it is better for supported_xcr0
> to contain only what is supported in guest's cpuid bits and handle the
> fact that FP/SSE state should always be copied to/from userspace in
> kvm_vcpu_ioctl_x86_(set|get)_xsave functions.
I don't think it makes sense to disable SSE but not XSAVE. Linux even
has this:
if ((pcntxt_mask & XSTATE_FPSSE) != XSTATE_FPSSE) {
pr_err("FP/SSE not shown under xsave features 0x%llx\n",
pcntxt_mask);
BUG();
}
I preferred this to adding dubiously-testable code that probed CPUID for
SSE support (or should I check FXSR instead?)
Paolo
>> kvm_async_pf_hash_reset(vcpu);
>> kvm_pmu_init(vcpu);
>>
>> --
>> 1.8.3.1
>>
>
> --
> Gleb.
>
On Mon, Sep 23, 2013 at 12:22:37PM +0200, Paolo Bonzini wrote:
> >> @@ -6940,6 +6948,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> >>
> >> vcpu->arch.ia32_tsc_adjust_msr = 0x0;
> >> vcpu->arch.pv_time_enabled = false;
> >> +
> >> + vcpu->arch.supported_xcr0 = XSTATE_FPSSE;
> >> +
> > Why is this needed? It will make make __kvm_set_xcr() succeed if attempt
> > is made to set SSE bit when it is not supported in cpuid. This may not
> > be an issue in practice, but for clarity it is better for supported_xcr0
> > to contain only what is supported in guest's cpuid bits and handle the
> > fact that FP/SSE state should always be copied to/from userspace in
> > kvm_vcpu_ioctl_x86_(set|get)_xsave functions.
>
> I don't think it makes sense to disable SSE but not XSAVE. Linux even
> has this:
>
> if ((pcntxt_mask & XSTATE_FPSSE) != XSTATE_FPSSE) {
> pr_err("FP/SSE not shown under xsave features 0x%llx\n",
> pcntxt_mask);
> BUG();
> }
>
That's why I said it may not be an issue in practice. What makes the
code confusing to me is that it is not clear what .supported_xcr0
contains. Why not make it contain only guest cpuid bits and use
XSTATE_FPSSE explicitly in get_xsave. I mean drop this from next patch:
vcpu->arch.supported_xcr0 =
- (best->eax | ((u64)best->edx << 32)) &
+ (best->eax | ((u64)best->edx << 32) | XSTATE_FPSSE) &
host_xcr0 & KVM_SUPPORTED_XCR0;
And change
*(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] != vcpu->arch.supported_xcr0;
to
*(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] != (vcpu->arch.supported_xcr0 | XSTATE_FPSSE);
> I preferred this to adding dubiously-testable code that probed CPUID for
> SSE support (or should I check FXSR instead?)
>
Not sure I understand.
--
Gleb.
Il 23/09/2013 17:09, Gleb Natapov ha scritto:
> On Mon, Sep 23, 2013 at 12:22:37PM +0200, Paolo Bonzini wrote:
>>>> @@ -6940,6 +6948,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>>>>
>>>> vcpu->arch.ia32_tsc_adjust_msr = 0x0;
>>>> vcpu->arch.pv_time_enabled = false;
>>>> +
>>>> + vcpu->arch.supported_xcr0 = XSTATE_FPSSE;
>>>> +
>>> Why is this needed? It will make make __kvm_set_xcr() succeed if attempt
>>> is made to set SSE bit when it is not supported in cpuid. This may not
>>> be an issue in practice, but for clarity it is better for supported_xcr0
>>> to contain only what is supported in guest's cpuid bits and handle the
>>> fact that FP/SSE state should always be copied to/from userspace in
>>> kvm_vcpu_ioctl_x86_(set|get)_xsave functions.
>>
>> I don't think it makes sense to disable SSE but not XSAVE. Linux even
>> has this:
>>
>> if ((pcntxt_mask & XSTATE_FPSSE) != XSTATE_FPSSE) {
>> pr_err("FP/SSE not shown under xsave features 0x%llx\n",
>> pcntxt_mask);
>> BUG();
>> }
>>
> That's why I said it may not be an issue in practice. What makes the
> code confusing to me is that it is not clear what .supported_xcr0
> contains. Why not make it contain only guest cpuid bits and use
> XSTATE_FPSSE explicitly in get_xsave. I mean drop this from next patch:
>
> vcpu->arch.supported_xcr0 =
> - (best->eax | ((u64)best->edx << 32)) &
> + (best->eax | ((u64)best->edx << 32) | XSTATE_FPSSE) &
> host_xcr0 & KVM_SUPPORTED_XCR0;
>
> And change
> *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] != vcpu->arch.supported_xcr0;
> to
> *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] != (vcpu->arch.supported_xcr0 | XSTATE_FPSSE);
Ok.
>
>> I preferred this to adding dubiously-testable code that probed CPUID for
>> SSE support (or should I check FXSR instead?)
I didn't understand you wanted the "| XSTATE_FPSSE" moved, I thought you
wanted to check for SSE bits in userspace (similar to AVX). I wasn't
sure whether to tie this to the SSE bit, or to FXSR. But what you
propose makes sense, I'll test it.
Paolo