| Message ID | DE8DF0795D48FD4CA783C40EC8292335013EF14B@SHSMSX101.ccr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
Il 29/11/2013 14:17, Liu, Jinsong ha scritto: > From aac033473bc88befe39a9add99820c0a7118ac90 Mon Sep 17 00:00:00 2001 > From: root <root@ljs.(none)> > Date: Fri, 22 Nov 2013 00:24:35 +0800 > Subject: [PATCH 2/2] target-i386: Intel MPX support > > Expose cpuid leaf (0xd, 3) and (0xd, 4) to guest. > Fix ebx and re-calculate ecx of cpuid leaf (0xd, 0). There is no reason to get the size and offset from the host. Peter Anvin confirmed that the sizes and offsets will never change (as should be the case for migration to work across different CPU versions). In fact, the size and offset is documented for every XSAVE feature except MPX in the copy I have of the Intel documentation. Please get the size and offset from the documentation, if it has been updated, or from a real host, and hardcode them in QEMU. Paolo > Signed-off-by: Liu Jinsong <jinsong.liu@intel.com> > --- > target-i386/cpu.c | 34 ++++++++++++++++++++++++++-------- > target-i386/cpu.h | 1 + > 2 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 544b57f..7d04f28 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -330,12 +330,12 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > > 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 = 0x240, .size = 0x100 }, > + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX }, > + [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, > + [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, > }; > > const char *get_register_name_32(unsigned int reg) > @@ -2204,9 +2204,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); > > if (count == 0) { > - *ecx = 0x240; > + *ebx = *ecx = 0x240; > for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { > + uint32_t offset, size; > const ExtSaveArea *esa = &ext_save_areas[i]; > + > if ((env->features[esa->feature] & esa->bits) == esa->bits && > (kvm_mask & (1 << i)) != 0) { > if (i < 32) { > @@ -2214,19 +2216,35 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > } else { > *edx |= 1 << (i - 32); > } > - *ecx = MAX(*ecx, esa->offset + esa->size); > + > + size = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EAX); > + offset = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX); > + *ecx = MAX(*ecx, offset + size); > + > + /* > + * EBX here just in order to > + * 1. keep compatible with old qemu version, take AVX > + * into account; > + * 2. keep compatible with old kernel version. Currently > + * KVM has bug when expose cpuid 0xd to guest (include > + * static value when guest booting and dynamic value > + * when guest enables XCR0 features. EBX here can > + * co-work with old buggy and new updated KVM, keep > + * same value independent to CPU and kernel version. > + */ > + if (i == 2) > + *ebx = MAX(*ebx, offset + 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->size; > - *ebx = esa->offset; > + *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); > + *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); > } > } > break; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index ea373e8..9a838d1 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -545,6 +545,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EBX_ERMS (1 << 9) > #define CPUID_7_0_EBX_INVPCID (1 << 10) > #define CPUID_7_0_EBX_RTM (1 << 11) > +#define CPUID_7_0_EBX_MPX (1 << 14) > #define CPUID_7_0_EBX_RDSEED (1 << 18) > #define CPUID_7_0_EBX_ADX (1 << 19) > #define CPUID_7_0_EBX_SMAP (1 << 20) > -- > 1.7.1 >
Paolo Bonzini wrote: > Il 29/11/2013 14:17, Liu, Jinsong ha scritto: >> From aac033473bc88befe39a9add99820c0a7118ac90 Mon Sep 17 00:00:00 >> 2001 >> From: root <root@ljs.(none)> >> Date: Fri, 22 Nov 2013 00:24:35 +0800 >> Subject: [PATCH 2/2] target-i386: Intel MPX support >> >> Expose cpuid leaf (0xd, 3) and (0xd, 4) to guest. >> Fix ebx and re-calculate ecx of cpuid leaf (0xd, 0). > > There is no reason to get the size and offset from the host. Peter > Anvin confirmed that the sizes and offsets will never change (as > should be the case for migration to work across different CPU > versions). In fact, the size and offset is documented for every > XSAVE feature except MPX in the copy I have of the Intel > documentation. If the sizes and offsets will never change, what's the bad effect of getting them from host? > > Please get the size and offset from the documentation, if it has been > updated, or from a real host, and hardcode them in QEMU. > Hmm, the problem is what I get is not equal to real test :( For example, I was told XSTATE_BNDCSR_SIZE is 0x40, but real test shows it's 0x10. Maybe getting from real h/w is not bad than hardcode it? Thanks Jinsong > >> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com> >> --- >> target-i386/cpu.c | 34 ++++++++++++++++++++++++++-------- >> target-i386/cpu.h | 1 + 2 files changed, 27 insertions(+), 8 >> deletions(-) >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index 544b57f..7d04f28 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -330,12 +330,12 @@ X86RegisterInfo32 >> x86_reg_info_32[CPU_NB_REGS32] = { >> >> 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 = 0x240, .size = 0x100 }, >> + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX }, >> + [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, >> + [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, >> }; >> >> const char *get_register_name_32(unsigned int reg) >> @@ -2204,9 +2204,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t >> index, uint32_t count, >> ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); >> >> if (count == 0) { >> - *ecx = 0x240; >> + *ebx = *ecx = 0x240; >> for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { >> + uint32_t offset, size; >> const ExtSaveArea *esa = &ext_save_areas[i]; + >> if ((env->features[esa->feature] & esa->bits) == >> esa->bits && (kvm_mask & (1 << i)) != 0) { >> if (i < 32) { >> @@ -2214,19 +2216,35 @@ void cpu_x86_cpuid(CPUX86State *env, >> uint32_t index, uint32_t count, >> } else { *edx |= 1 << (i - 32); } >> - *ecx = MAX(*ecx, esa->offset + esa->size); + >> + size = kvm_arch_get_supported_cpuid(s, 0xd, i, >> R_EAX); + offset = >> kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX); + >> *ecx = MAX(*ecx, offset + size); + + /* >> + * EBX here just in order to >> + * 1. keep compatible with old qemu version, >> take AVX + * into account; >> + * 2. keep compatible with old kernel version. >> Currently + * KVM has bug when expose cpuid >> 0xd to guest (include + * static value when >> guest booting and dynamic value + * when >> guest enables XCR0 features. EBX here can + * >> co-work with old buggy and new updated KVM, keep + >> * same value independent to CPU and kernel version. + >> */ + if (i == 2) + *ebx = >> MAX(*ebx, offset + 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->size; >> - *ebx = esa->offset; >> + *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, >> R_EAX); + *ebx = kvm_arch_get_supported_cpuid(s, 0xd, >> count, R_EBX); } } >> break; >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >> index ea373e8..9a838d1 100644 >> --- a/target-i386/cpu.h >> +++ b/target-i386/cpu.h >> @@ -545,6 +545,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; >> #define CPUID_7_0_EBX_ERMS (1 << 9) >> #define CPUID_7_0_EBX_INVPCID (1 << 10) >> #define CPUID_7_0_EBX_RTM (1 << 11) >> +#define CPUID_7_0_EBX_MPX (1 << 14) >> #define CPUID_7_0_EBX_RDSEED (1 << 18) >> #define CPUID_7_0_EBX_ADX (1 << 19) >> #define CPUID_7_0_EBX_SMAP (1 << 20) >> -- >> 1.7.1
Il 29/11/2013 15:50, Liu, Jinsong ha scritto: > > There is no reason to get the size and offset from the host. Peter > > Anvin confirmed that the sizes and offsets will never change (as > > should be the case for migration to work across different CPU > > versions). In fact, the size and offset is documented for every > > XSAVE feature except MPX in the copy I have of the Intel > > documentation. > > If the sizes and offsets will never change, what's the bad effect of getting them from host? In case TCG gets AVX/MPX support later, you will not be able to get CPUID values from the host. The leaf 0xd code was written so that it would work for both KVM and TCG. When QEMU got AVX support, we decided not to treat XSAVE data as opaque blobs, and instead "unmarshal" data out of it into the CPUX86State struct and back. This is again useful for TCG, but it also makes for easier interpretation of migration state. You will have to rely on precise sizes and offsets in the marshaling/unmarshaling code of kvm_get_xsave/kvm_put_xsave, so it is not a big problem to have them here as well. Paolo
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 544b57f..7d04f28 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -330,12 +330,12 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { 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 = 0x240, .size = 0x100 }, + [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX }, + [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, + [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, }; const char *get_register_name_32(unsigned int reg) @@ -2204,9 +2204,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); if (count == 0) { - *ecx = 0x240; + *ebx = *ecx = 0x240; for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) { + uint32_t offset, size; const ExtSaveArea *esa = &ext_save_areas[i]; + if ((env->features[esa->feature] & esa->bits) == esa->bits && (kvm_mask & (1 << i)) != 0) { if (i < 32) { @@ -2214,19 +2216,35 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, } else { *edx |= 1 << (i - 32); } - *ecx = MAX(*ecx, esa->offset + esa->size); + + size = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EAX); + offset = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX); + *ecx = MAX(*ecx, offset + size); + + /* + * EBX here just in order to + * 1. keep compatible with old qemu version, take AVX + * into account; + * 2. keep compatible with old kernel version. Currently + * KVM has bug when expose cpuid 0xd to guest (include + * static value when guest booting and dynamic value + * when guest enables XCR0 features. EBX here can + * co-work with old buggy and new updated KVM, keep + * same value independent to CPU and kernel version. + */ + if (i == 2) + *ebx = MAX(*ebx, offset + 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->size; - *ebx = esa->offset; + *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX); + *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX); } } break; diff --git a/target-i386/cpu.h b/target-i386/cpu.h index ea373e8..9a838d1 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -545,6 +545,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_7_0_EBX_ERMS (1 << 9) #define CPUID_7_0_EBX_INVPCID (1 << 10) #define CPUID_7_0_EBX_RTM (1 << 11) +#define CPUID_7_0_EBX_MPX (1 << 14) #define CPUID_7_0_EBX_RDSEED (1 << 18) #define CPUID_7_0_EBX_ADX (1 << 19) #define CPUID_7_0_EBX_SMAP (1 << 20)