* [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support
@ 2013-09-13 13:55 Paolo Bonzini
2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 1/2] x86: fix migration from pre-version 12 Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Paolo Bonzini @ 2013-09-13 13:55 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm
This series fixes two migration bugs concerning KVM's XSAVE ioctls.
The second right now is only a theoretical problem, since the only
XSAVE-specific state is AVX and all machines with XSAVE also have AVX.
In the future, this will ensure that machines with MPX or AVX-512
do not expose those XSAVE states when using e.g. "-cpu SandyBridge".
Please review.
Paolo Bonzini (2):
x86: fix migration from pre-version 12
x86: cpuid: reconstruct leaf 0Dh data
target-i386/cpu.c | 69 ++++++++++++++++++++++++++++++++++++++++++-------------
target-i386/cpu.h | 4 ++++
2 files changed, 57 insertions(+), 16 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH v2 uq/master 1/2] x86: fix migration from pre-version 12 2013-09-13 13:55 [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini @ 2013-09-13 13:55 ` Paolo Bonzini 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data Paolo Bonzini 2013-10-02 12:41 ` [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini 2 siblings, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2013-09-13 13:55 UTC (permalink / raw) To: qemu-devel; +Cc: kvm On KVM, the KVM_SET_XSAVE would be executed with a 0 xstate_bv, and not restore anything. Since FP and SSE data are always valid, set them in xstate_bv at reset time. In fact, that value is the same that KVM_GET_XSAVE returns on pre-XSAVE hosts. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target-i386/cpu.c | 1 + target-i386/cpu.h | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c36345e..ac83106 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2386,6 +2386,7 @@ static void x86_cpu_reset(CPUState *s) env->fpuc = 0x37f; env->mxcsr = 0x1f80; + env->xstate_bv = XSTATE_FP | XSTATE_SSE; env->pat = 0x0007040600070406ULL; env->msr_ia32_misc_enable = MSR_IA32_MISC_ENABLE_DEFAULT; diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 5723eff..d46ebdd 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -380,6 +380,10 @@ #define MSR_VM_HSAVE_PA 0xc0010117 +#define XSTATE_FP 1 +#define XSTATE_SSE 2 +#define XSTATE_YMM 4 + /* CPUID feature words */ typedef enum FeatureWord { FEAT_1_EDX, /* CPUID[1].EDX */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-09-13 13:55 [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 1/2] x86: fix migration from pre-version 12 Paolo Bonzini @ 2013-09-13 13:55 ` Paolo Bonzini 2013-10-02 15:21 ` Gleb Natapov 2013-10-02 12:41 ` [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini 2 siblings, 1 reply; 12+ messages in thread From: Paolo Bonzini @ 2013-09-13 13:55 UTC (permalink / raw) To: qemu-devel; +Cc: kvm The data in leaf 0Dh depends on information from other feature bits. Instead of passing it blindly from the host, compute it based on whether these feature bits are enabled. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target-i386/cpu.c | 63 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index ac83106..e6179f4 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { }; #undef REGISTER +typedef struct ExtSaveArea { + uint32_t feature, bits; + uint32_t offset, size; +} ExtSaveArea; + +static const ExtSaveArea ext_save_areas[] = { + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, + .offset = 0x100, .size = 0x240 }, +}; const char *get_register_name_32(unsigned int reg) { @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *edx = 0; } break; - case 0xD: + case 0xD: { + KVMState *s = cs->kvm_state; + uint64_t kvm_mask; + int i; + /* Processor Extended State */ - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; + *eax = 0; + *ebx = 0; + *ecx = 0; + *edx = 0; + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { break; } - if (kvm_enabled()) { - KVMState *s = cs->kvm_state; + kvm_mask = + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); - } else { - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; + if (count == 0) { + *ecx = 0x240; + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { + const ExtSaveArea *esa = &ext_save_areas[i]; + if ((env->features[esa->feature] & esa->bits) == esa->bits && + (kvm_mask & (1 << i)) != 0) { + if (i < 32) { + *eax |= 1 << i; + } else { + *edx |= 1 << (i - 32); + } + *ecx = MAX(*ecx, esa->offset + esa->size); + } + } + *eax |= kvm_mask & 3; + *ebx = *ecx; + } else if (count == 1) { + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); + } else if (count < ARRAY_SIZE(ext_save_areas)) { + const ExtSaveArea *esa = &ext_save_areas[count]; + if ((env->features[esa->feature] & esa->bits) == esa->bits && + (kvm_mask & (1 << count)) != 0) { + *eax = esa->offset; + *ebx = esa->size; + } } break; + } case 0x80000000: *eax = env->cpuid_xlevel; *ebx = env->cpuid_vendor1; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data Paolo Bonzini @ 2013-10-02 15:21 ` Gleb Natapov 2013-10-02 15:37 ` Paolo Bonzini 2013-10-02 15:54 ` [Qemu-devel] [PATCH v3 " Paolo Bonzini 0 siblings, 2 replies; 12+ messages in thread From: Gleb Natapov @ 2013-10-02 15:21 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, kvm On Fri, Sep 13, 2013 at 03:55:58PM +0200, Paolo Bonzini wrote: > The data in leaf 0Dh depends on information from other feature bits. > Instead of passing it blindly from the host, compute it based on > whether these feature bits are enabled. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target-i386/cpu.c | 63 +++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 47 insertions(+), 16 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index ac83106..e6179f4 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > }; > #undef REGISTER > > +typedef struct ExtSaveArea { > + uint32_t feature, bits; > + uint32_t offset, size; > +} ExtSaveArea; > + > +static const ExtSaveArea ext_save_areas[] = { > + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, > + .offset = 0x100, .size = 0x240 }, > +}; > > const char *get_register_name_32(unsigned int reg) > { > @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > *edx = 0; > } > break; > - case 0xD: > + case 0xD: { > + KVMState *s = cs->kvm_state; > + uint64_t kvm_mask; > + int i; > + > /* Processor Extended State */ > - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { > - *eax = 0; > - *ebx = 0; > - *ecx = 0; > - *edx = 0; > + *eax = 0; > + *ebx = 0; > + *ecx = 0; > + *edx = 0; > + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { > break; > } > - if (kvm_enabled()) { > - KVMState *s = cs->kvm_state; > + kvm_mask = > + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | > + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); > > - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); > - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); > - } else { > - *eax = 0; > - *ebx = 0; > - *ecx = 0; > - *edx = 0; > + if (count == 0) { > + *ecx = 0x240; > + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > + const ExtSaveArea *esa = &ext_save_areas[i]; > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > + (kvm_mask & (1 << i)) != 0) { > + if (i < 32) { > + *eax |= 1 << i; > + } else { > + *edx |= 1 << (i - 32); > + } > + *ecx = MAX(*ecx, esa->offset + esa->size); > + } > + } > + *eax |= kvm_mask & 3; Lets use define from previous patch. > + *ebx = *ecx; > + } else if (count == 1) { > + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > + } else if (count < ARRAY_SIZE(ext_save_areas)) { > + const ExtSaveArea *esa = &ext_save_areas[count]; > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > + (kvm_mask & (1 << count)) != 0) { > + *eax = esa->offset; > + *ebx = esa->size; Why do you hard code them instead of querying kernel? What if they depend on cpu type? (well if this happens we can forget about migration, but still...) > + } > } > break; > + } > case 0x80000000: > *eax = env->cpuid_xlevel; > *ebx = env->cpuid_vendor1; > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gleb. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-02 15:21 ` Gleb Natapov @ 2013-10-02 15:37 ` Paolo Bonzini 2013-10-02 15:39 ` Gleb Natapov 2013-10-02 15:54 ` [Qemu-devel] [PATCH v3 " Paolo Bonzini 1 sibling, 1 reply; 12+ messages in thread From: Paolo Bonzini @ 2013-10-02 15:37 UTC (permalink / raw) To: Gleb Natapov; +Cc: qemu-devel, kvm Il 02/10/2013 17:21, Gleb Natapov ha scritto: >> - if (kvm_enabled()) { >> - KVMState *s = cs->kvm_state; >> + kvm_mask = >> + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | >> + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); >> >> - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); >> - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); >> - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); >> - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); >> - } else { >> - *eax = 0; >> - *ebx = 0; >> - *ecx = 0; >> - *edx = 0; >> + if (count == 0) { >> + *ecx = 0x240; >> + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { >> + const ExtSaveArea *esa = &ext_save_areas[i]; >> + if ((env->features[esa->feature] & esa->bits) == esa->bits && >> + (kvm_mask & (1 << i)) != 0) { >> + if (i < 32) { >> + *eax |= 1 << i; >> + } else { >> + *edx |= 1 << (i - 32); >> + } >> + *ecx = MAX(*ecx, esa->offset + esa->size); >> + } >> + } >> + *eax |= kvm_mask & 3; > Lets use define from previous patch. Right. >> + *ebx = *ecx; >> + } else if (count == 1) { >> + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); >> + } else if (count < ARRAY_SIZE(ext_save_areas)) { >> + const ExtSaveArea *esa = &ext_save_areas[count]; >> + if ((env->features[esa->feature] & esa->bits) == esa->bits && >> + (kvm_mask & (1 << count)) != 0) { >> + *eax = esa->offset; >> + *ebx = esa->size; > Why do you hard code them instead of querying kernel? What if they > depend on cpu type? (well if this happens we can forget about > migration, but still...) HPA confirmed (on xen-devel) that they will not depend on the CPU type. All offsets are documented in the SDM and in the additional Skylake manual except for MPX, and he reported that he'd ask for MPX to be documented as well. As you said, if they changed it would be a total mess. I hardcoded them because this is not KVM-specific knowledge. TCG could in principle reuse the same code, just skipping the part where it masks away features not supported by KVM. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-02 15:37 ` Paolo Bonzini @ 2013-10-02 15:39 ` Gleb Natapov 0 siblings, 0 replies; 12+ messages in thread From: Gleb Natapov @ 2013-10-02 15:39 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, kvm On Wed, Oct 02, 2013 at 05:37:31PM +0200, Paolo Bonzini wrote: > Il 02/10/2013 17:21, Gleb Natapov ha scritto: > >> - if (kvm_enabled()) { > >> - KVMState *s = cs->kvm_state; > >> + kvm_mask = > >> + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | > >> + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); > >> > >> - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > >> - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > >> - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); > >> - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); > >> - } else { > >> - *eax = 0; > >> - *ebx = 0; > >> - *ecx = 0; > >> - *edx = 0; > >> + if (count == 0) { > >> + *ecx = 0x240; > >> + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > >> + const ExtSaveArea *esa = &ext_save_areas[i]; > >> + if ((env->features[esa->feature] & esa->bits) == esa->bits && > >> + (kvm_mask & (1 << i)) != 0) { > >> + if (i < 32) { > >> + *eax |= 1 << i; > >> + } else { > >> + *edx |= 1 << (i - 32); > >> + } > >> + *ecx = MAX(*ecx, esa->offset + esa->size); > >> + } > >> + } > >> + *eax |= kvm_mask & 3; > > Lets use define from previous patch. > > Right. > > >> + *ebx = *ecx; > >> + } else if (count == 1) { > >> + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > >> + } else if (count < ARRAY_SIZE(ext_save_areas)) { > >> + const ExtSaveArea *esa = &ext_save_areas[count]; > >> + if ((env->features[esa->feature] & esa->bits) == esa->bits && > >> + (kvm_mask & (1 << count)) != 0) { > >> + *eax = esa->offset; > >> + *ebx = esa->size; > > Why do you hard code them instead of querying kernel? What if they > > depend on cpu type? (well if this happens we can forget about > > migration, but still...) > > HPA confirmed (on xen-devel) that they will not depend on the CPU type. > All offsets are documented in the SDM and in the additional Skylake > manual except for MPX, and he reported that he'd ask for MPX to be > documented as well. As you said, if they changed it would be a total mess. > > I hardcoded them because this is not KVM-specific knowledge. TCG could > in principle reuse the same code, just skipping the part where it masks > away features not supported by KVM. > OK. Can you send new version with defines please? -- Gleb. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v3 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-02 15:21 ` Gleb Natapov 2013-10-02 15:37 ` Paolo Bonzini @ 2013-10-02 15:54 ` Paolo Bonzini 2013-10-02 16:03 ` Gleb Natapov 2013-10-03 9:59 ` Igor Mammedov 1 sibling, 2 replies; 12+ messages in thread From: Paolo Bonzini @ 2013-10-02 15:54 UTC (permalink / raw) To: qemu-devel; +Cc: gleb, kvm The data in leaf 0Dh depends on information from other feature bits. Instead of passing it blindly from the host, compute it based on whether these feature bits are enabled. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target-i386/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index ac83106..1addb18 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { }; #undef REGISTER +typedef struct ExtSaveArea { + uint32_t feature, bits; + uint32_t offset, size; +} ExtSaveArea; + +static const ExtSaveArea ext_save_areas[] = { + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, + .offset = 0x100, .size = 0x240 }, +}; const char *get_register_name_32(unsigned int reg) { @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *edx = 0; } break; - case 0xD: + case 0xD: { + KVMState *s = cs->kvm_state; + uint64_t kvm_mask; + int i; + /* Processor Extended State */ - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; + *eax = 0; + *ebx = 0; + *ecx = 0; + *edx = 0; + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { break; } - if (kvm_enabled()) { - KVMState *s = cs->kvm_state; + kvm_mask = + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); - } else { - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; + if (count == 0) { + *ecx = 0x240; + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { + const ExtSaveArea *esa = &ext_save_areas[i]; + if ((env->features[esa->feature] & esa->bits) == esa->bits && + (kvm_mask & (1 << i)) != 0) { + if (i < 32) { + *eax |= 1 << i; + } else { + *edx |= 1 << (i - 32); + } + *ecx = MAX(*ecx, esa->offset + esa->size); + } + } + *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); + *ebx = *ecx; + } else if (count == 1) { + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); + } else if (count < ARRAY_SIZE(ext_save_areas)) { + const ExtSaveArea *esa = &ext_save_areas[count]; + if ((env->features[esa->feature] & esa->bits) == esa->bits && + (kvm_mask & (1 << count)) != 0) { + *eax = esa->offset; + *ebx = esa->size; + } } break; + } case 0x80000000: *eax = env->cpuid_xlevel; *ebx = env->cpuid_vendor1; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-02 15:54 ` [Qemu-devel] [PATCH v3 " Paolo Bonzini @ 2013-10-02 16:03 ` Gleb Natapov 2013-10-03 9:59 ` Igor Mammedov 1 sibling, 0 replies; 12+ messages in thread From: Gleb Natapov @ 2013-10-02 16:03 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, kvm On Wed, Oct 02, 2013 at 05:54:57PM +0200, Paolo Bonzini wrote: > The data in leaf 0Dh depends on information from other feature bits. > Instead of passing it blindly from the host, compute it based on > whether these feature bits are enabled. > Applied both. Thanks. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target-i386/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 48 insertions(+), 17 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index ac83106..1addb18 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > }; > #undef REGISTER > > +typedef struct ExtSaveArea { > + uint32_t feature, bits; > + uint32_t offset, size; > +} ExtSaveArea; > + > +static const ExtSaveArea ext_save_areas[] = { > + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, > + .offset = 0x100, .size = 0x240 }, > +}; > > const char *get_register_name_32(unsigned int reg) > { > @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > *edx = 0; > } > break; > - case 0xD: > + case 0xD: { > + KVMState *s = cs->kvm_state; > + uint64_t kvm_mask; > + int i; > + > /* Processor Extended State */ > - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { > - *eax = 0; > - *ebx = 0; > - *ecx = 0; > - *edx = 0; > + *eax = 0; > + *ebx = 0; > + *ecx = 0; > + *edx = 0; > + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { > break; > } > - if (kvm_enabled()) { > - KVMState *s = cs->kvm_state; > + kvm_mask = > + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | > + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); > > - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); > - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); > - } else { > - *eax = 0; > - *ebx = 0; > - *ecx = 0; > - *edx = 0; > + if (count == 0) { > + *ecx = 0x240; > + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > + const ExtSaveArea *esa = &ext_save_areas[i]; > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > + (kvm_mask & (1 << i)) != 0) { > + if (i < 32) { > + *eax |= 1 << i; > + } else { > + *edx |= 1 << (i - 32); > + } > + *ecx = MAX(*ecx, esa->offset + esa->size); > + } > + } > + *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); > + *ebx = *ecx; > + } else if (count == 1) { > + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > + } else if (count < ARRAY_SIZE(ext_save_areas)) { > + const ExtSaveArea *esa = &ext_save_areas[count]; > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > + (kvm_mask & (1 << count)) != 0) { > + *eax = esa->offset; > + *ebx = esa->size; > + } > } > break; > + } > case 0x80000000: > *eax = env->cpuid_xlevel; > *ebx = env->cpuid_vendor1; > -- > 1.8.3.1 -- Gleb. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-02 15:54 ` [Qemu-devel] [PATCH v3 " Paolo Bonzini 2013-10-02 16:03 ` Gleb Natapov @ 2013-10-03 9:59 ` Igor Mammedov 2013-10-03 10:01 ` Gleb Natapov 1 sibling, 1 reply; 12+ messages in thread From: Igor Mammedov @ 2013-10-03 9:59 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, gleb, kvm On Wed, 2 Oct 2013 17:54:57 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > The data in leaf 0Dh depends on information from other feature bits. > Instead of passing it blindly from the host, compute it based on > whether these feature bits are enabled. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target-i386/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 48 insertions(+), 17 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index ac83106..1addb18 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > }; > #undef REGISTER > > +typedef struct ExtSaveArea { > + uint32_t feature, bits; > + uint32_t offset, size; > +} ExtSaveArea; > + > +static const ExtSaveArea ext_save_areas[] = { > + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, > + .offset = 0x100, .size = 0x240 }, > +}; > > const char *get_register_name_32(unsigned int reg) > { > @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > *edx = 0; > } > break; > - case 0xD: > + case 0xD: { > + KVMState *s = cs->kvm_state; > + uint64_t kvm_mask; > + int i; > + > /* Processor Extended State */ > - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { > - *eax = 0; > - *ebx = 0; > - *ecx = 0; > - *edx = 0; > + *eax = 0; > + *ebx = 0; > + *ecx = 0; > + *edx = 0; > + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { > break; > } > - if (kvm_enabled()) { > - KVMState *s = cs->kvm_state; > + kvm_mask = > + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | > + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); calling kvm_arch_get_supported_cpuid() without kvm_enabled() guard could regress TCG mode on non KVM host: kvm_arch_get_supported_cpuid -> get_supported_cpuid -> try_get_cpuid -> r = kvm_ioctl(s, KVM_GET_SUPPORTED_CPUID, cpuid); ... if (r < 0) { if (r == -E2BIG) { g_free(cpuid); return NULL; } else { fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n", strerror(-r)); exit(1); ^^^^^^^^^ guest suddenly dies > > - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); > - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); > - } else { > - *eax = 0; > - *ebx = 0; > - *ecx = 0; > - *edx = 0; > + if (count == 0) { > + *ecx = 0x240; > + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > + const ExtSaveArea *esa = &ext_save_areas[i]; > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > + (kvm_mask & (1 << i)) != 0) { > + if (i < 32) { > + *eax |= 1 << i; > + } else { > + *edx |= 1 << (i - 32); > + } > + *ecx = MAX(*ecx, esa->offset + esa->size); > + } > + } > + *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); > + *ebx = *ecx; > + } else if (count == 1) { > + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > + } else if (count < ARRAY_SIZE(ext_save_areas)) { > + const ExtSaveArea *esa = &ext_save_areas[count]; > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > + (kvm_mask & (1 << count)) != 0) { > + *eax = esa->offset; > + *ebx = esa->size; > + } > } > break; > + } > case 0x80000000: > *eax = env->cpuid_xlevel; > *ebx = env->cpuid_vendor1; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-03 9:59 ` Igor Mammedov @ 2013-10-03 10:01 ` Gleb Natapov 2013-10-03 10:14 ` Igor Mammedov 0 siblings, 1 reply; 12+ messages in thread From: Gleb Natapov @ 2013-10-03 10:01 UTC (permalink / raw) To: Igor Mammedov; +Cc: Paolo Bonzini, qemu-devel, kvm On Thu, Oct 03, 2013 at 11:59:24AM +0200, Igor Mammedov wrote: > On Wed, 2 Oct 2013 17:54:57 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > The data in leaf 0Dh depends on information from other feature bits. > > Instead of passing it blindly from the host, compute it based on > > whether these feature bits are enabled. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > target-i386/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 48 insertions(+), 17 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index ac83106..1addb18 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > > }; > > #undef REGISTER > > > > +typedef struct ExtSaveArea { > > + uint32_t feature, bits; > > + uint32_t offset, size; > > +} ExtSaveArea; > > + > > +static const ExtSaveArea ext_save_areas[] = { > > + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, > > + .offset = 0x100, .size = 0x240 }, > > +}; > > > > const char *get_register_name_32(unsigned int reg) > > { > > @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > *edx = 0; > > } > > break; > > - case 0xD: > > + case 0xD: { > > + KVMState *s = cs->kvm_state; > > + uint64_t kvm_mask; > > + int i; > > + > > /* Processor Extended State */ > > - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { > > - *eax = 0; > > - *ebx = 0; > > - *ecx = 0; > > - *edx = 0; > > + *eax = 0; > > + *ebx = 0; > > + *ecx = 0; > > + *edx = 0; > > + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { > > break; > > } > > - if (kvm_enabled()) { > > - KVMState *s = cs->kvm_state; > > + kvm_mask = > > + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | > > + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); > calling kvm_arch_get_supported_cpuid() without kvm_enabled() guard > could regress TCG mode on non KVM host: > But there is kvm_enabled() guard above. > kvm_arch_get_supported_cpuid -> get_supported_cpuid -> try_get_cpuid -> > r = kvm_ioctl(s, KVM_GET_SUPPORTED_CPUID, cpuid); > ... > if (r < 0) { > if (r == -E2BIG) { > g_free(cpuid); > return NULL; > } else { > fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n", > strerror(-r)); > exit(1); > ^^^^^^^^^ guest suddenly dies > > > > > - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > > - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > > - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); > > - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); > > - } else { > > - *eax = 0; > > - *ebx = 0; > > - *ecx = 0; > > - *edx = 0; > > + if (count == 0) { > > + *ecx = 0x240; > > + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > > + const ExtSaveArea *esa = &ext_save_areas[i]; > > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > > + (kvm_mask & (1 << i)) != 0) { > > + if (i < 32) { > > + *eax |= 1 << i; > > + } else { > > + *edx |= 1 << (i - 32); > > + } > > + *ecx = MAX(*ecx, esa->offset + esa->size); > > + } > > + } > > + *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); > > + *ebx = *ecx; > > + } else if (count == 1) { > > + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > > + } else if (count < ARRAY_SIZE(ext_save_areas)) { > > + const ExtSaveArea *esa = &ext_save_areas[count]; > > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > > + (kvm_mask & (1 << count)) != 0) { > > + *eax = esa->offset; > > + *ebx = esa->size; > > + } > > } > > break; > > + } > > case 0x80000000: > > *eax = env->cpuid_xlevel; > > *ebx = env->cpuid_vendor1; -- Gleb. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data 2013-10-03 10:01 ` Gleb Natapov @ 2013-10-03 10:14 ` Igor Mammedov 0 siblings, 0 replies; 12+ messages in thread From: Igor Mammedov @ 2013-10-03 10:14 UTC (permalink / raw) To: Gleb Natapov; +Cc: Paolo Bonzini, qemu-devel, kvm On Thu, 3 Oct 2013 13:01:54 +0300 Gleb Natapov <gleb@redhat.com> wrote: > On Thu, Oct 03, 2013 at 11:59:24AM +0200, Igor Mammedov wrote: > > On Wed, 2 Oct 2013 17:54:57 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > The data in leaf 0Dh depends on information from other feature bits. > > > Instead of passing it blindly from the host, compute it based on > > > whether these feature bits are enabled. > > > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > --- > > > target-i386/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++--------------- > > > 1 file changed, 48 insertions(+), 17 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index ac83106..1addb18 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > > > }; > > > #undef REGISTER > > > > > > +typedef struct ExtSaveArea { > > > + uint32_t feature, bits; > > > + uint32_t offset, size; > > > +} ExtSaveArea; > > > + > > > +static const ExtSaveArea ext_save_areas[] = { > > > + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, > > > + .offset = 0x100, .size = 0x240 }, > > > +}; > > > > > > const char *get_register_name_32(unsigned int reg) > > > { > > > @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > > *edx = 0; > > > } > > > break; > > > - case 0xD: > > > + case 0xD: { > > > + KVMState *s = cs->kvm_state; > > > + uint64_t kvm_mask; > > > + int i; > > > + > > > /* Processor Extended State */ > > > - if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { > > > - *eax = 0; > > > - *ebx = 0; > > > - *ecx = 0; > > > - *edx = 0; > > > + *eax = 0; > > > + *ebx = 0; > > > + *ecx = 0; > > > + *edx = 0; > > > + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { > > > break; > > > } > > > - if (kvm_enabled()) { > > > - KVMState *s = cs->kvm_state; > > > + kvm_mask = > > > + kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) | > > > + ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); > > calling kvm_arch_get_supported_cpuid() without kvm_enabled() guard > > could regress TCG mode on non KVM host: > > > But there is kvm_enabled() guard above. Ah, I'm sorry for noise. I've not noticed it in previous hunk. > > > kvm_arch_get_supported_cpuid -> get_supported_cpuid -> try_get_cpuid -> > > r = kvm_ioctl(s, KVM_GET_SUPPORTED_CPUID, cpuid); > > ... > > if (r < 0) { > > if (r == -E2BIG) { > > g_free(cpuid); > > return NULL; > > } else { > > fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n", > > strerror(-r)); > > exit(1); > > ^^^^^^^^^ guest suddenly dies > > > > > > > > - *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > > > - *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > > > - *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX); > > > - *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX); > > > - } else { > > > - *eax = 0; > > > - *ebx = 0; > > > - *ecx = 0; > > > - *edx = 0; > > > + if (count == 0) { > > > + *ecx = 0x240; > > > + for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > > > + const ExtSaveArea *esa = &ext_save_areas[i]; > > > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > > > + (kvm_mask & (1 << i)) != 0) { > > > + if (i < 32) { > > > + *eax |= 1 << i; > > > + } else { > > > + *edx |= 1 << (i - 32); > > > + } > > > + *ecx = MAX(*ecx, esa->offset + esa->size); > > > + } > > > + } > > > + *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); > > > + *ebx = *ecx; > > > + } else if (count == 1) { > > > + *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > > > + } else if (count < ARRAY_SIZE(ext_save_areas)) { > > > + const ExtSaveArea *esa = &ext_save_areas[count]; > > > + if ((env->features[esa->feature] & esa->bits) == esa->bits && > > > + (kvm_mask & (1 << count)) != 0) { > > > + *eax = esa->offset; > > > + *ebx = esa->size; > > > + } > > > } > > > break; > > > + } > > > case 0x80000000: > > > *eax = env->cpuid_xlevel; > > > *ebx = env->cpuid_vendor1; > > -- > Gleb. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support 2013-09-13 13:55 [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 1/2] x86: fix migration from pre-version 12 Paolo Bonzini 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data Paolo Bonzini @ 2013-10-02 12:41 ` Paolo Bonzini 2 siblings, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2013-10-02 12:41 UTC (permalink / raw) To: kvm; +Cc: qemu-devel, Gleb Natapov Il 13/09/2013 15:55, Paolo Bonzini ha scritto: > This series fixes two migration bugs concerning KVM's XSAVE ioctls. > The second right now is only a theoretical problem, since the only > XSAVE-specific state is AVX and all machines with XSAVE also have AVX. > In the future, this will ensure that machines with MPX or AVX-512 > do not expose those XSAVE states when using e.g. "-cpu SandyBridge". > > Please review. > > Paolo Bonzini (2): > x86: fix migration from pre-version 12 > x86: cpuid: reconstruct leaf 0Dh data > > target-i386/cpu.c | 69 ++++++++++++++++++++++++++++++++++++++++++------------- > target-i386/cpu.h | 4 ++++ > 2 files changed, 57 insertions(+), 16 deletions(-) > Ping? Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-10-03 10:15 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-13 13:55 [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 1/2] x86: fix migration from pre-version 12 Paolo Bonzini 2013-09-13 13:55 ` [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data Paolo Bonzini 2013-10-02 15:21 ` Gleb Natapov 2013-10-02 15:37 ` Paolo Bonzini 2013-10-02 15:39 ` Gleb Natapov 2013-10-02 15:54 ` [Qemu-devel] [PATCH v3 " Paolo Bonzini 2013-10-02 16:03 ` Gleb Natapov 2013-10-03 9:59 ` Igor Mammedov 2013-10-03 10:01 ` Gleb Natapov 2013-10-03 10:14 ` Igor Mammedov 2013-10-02 12:41 ` [Qemu-devel] [PATCH v2 uq/master 0/2] KVM: issues with XSAVE support Paolo Bonzini
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).