| Message ID | 1416936942-32516-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 25, 2014 at 06:35:42PM +0100, Paolo Bonzini wrote: > These represent xsave-related capabilities of the processor, and KVM may > or may not support them. > > Add feature bits so that they are considered by "-cpu ...,enforce", and use > the new feature work instead of calling kvm_arch_get_supported_cpuid. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Cool, eliminating all kvm_arch_get_supported_cpuid() calls from cpu_x86_cpuid() is on my TODO-list. I just have one question below: > --- > target-i386/cpu.c | 28 +++++++++++++++++++++++++++- > target-i386/cpu.h | 6 ++++++ > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index e9df33e..a2dde11 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -274,6 +274,17 @@ static const char *cpuid_apm_edx_feature_name[] = { > NULL, NULL, NULL, NULL, > }; > > +static const char *cpuid_xsave_feature_name[] = { > + "xsaveopt", "xsavec", "xgetbv1", "xsaves", None of the above features introduce any new state that might need to be migrated, or will require other changes in QEMU to work, right? It looks like they don't introduce any extra state, but if they do, they need to be added to unmigratable_flags until migration support is implemented. If they require other QEMU changes, it would be nice if KVM reported them using KVM_CHECK_EXTENSION instead of GET_SUPPORTED_CPUID, so it wouldn't break "-cpu host". > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > +}; > + > #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) > #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ > CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) > @@ -391,6 +402,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > .tcg_features = TCG_APM_FEATURES, > .unmigratable_flags = CPUID_APM_INVTSC, > }, > + [FEAT_XSAVE] = { > + .feat_names = cpuid_xsave_feature_name, > + .cpuid_eax = 0xd, > + .cpuid_needs_ecx = true, .cpuid_ecx = 1, > + .cpuid_reg = R_EAX, > + .tcg_features = 0, > + }, > }; > > typedef struct X86RegisterInfo32 { > @@ -1018,6 +1036,8 @@ static X86CPUDefinition builtin_x86_defs[] = { > CPUID_EXT2_SYSCALL, > .features[FEAT_8000_0001_ECX] = > CPUID_EXT3_LAHF_LM, > + .features[FEAT_XSAVE] = > + CPUID_XSAVE_XSAVEOPT, > .xlevel = 0x8000000A, > .model_id = "Intel Xeon E312xx (Sandy Bridge)", > }, > @@ -1051,6 +1071,8 @@ static X86CPUDefinition builtin_x86_defs[] = { > CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | > CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | > CPUID_7_0_EBX_RTM, > + .features[FEAT_XSAVE] = > + CPUID_XSAVE_XSAVEOPT, > .xlevel = 0x8000000A, > .model_id = "Intel Core Processor (Haswell)", > }, > @@ -1085,6 +1107,8 @@ static X86CPUDefinition builtin_x86_defs[] = { > CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | > CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | > CPUID_7_0_EBX_SMAP, > + .features[FEAT_XSAVE] = > + CPUID_XSAVE_XSAVEOPT, > .xlevel = 0x8000000A, > .model_id = "Intel Core Processor (Broadwell)", > }, > @@ -1202,6 +1226,7 @@ static X86CPUDefinition builtin_x86_defs[] = { > CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE | > CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | CPUID_EXT3_SVM | > CPUID_EXT3_LAHF_LM, > + /* no xsaveopt! */ > .xlevel = 0x8000001A, > .model_id = "AMD Opteron 62xx class CPU", > }, > @@ -1236,6 +1261,7 @@ static X86CPUDefinition builtin_x86_defs[] = { > CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE | > CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | CPUID_EXT3_SVM | > CPUID_EXT3_LAHF_LM, > + /* no xsaveopt! */ > .xlevel = 0x8000001A, > .model_id = "AMD Opteron 63xx class CPU", > }, > @@ -2377,7 +2403,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); > *ebx = *ecx; > } else if (count == 1) { > - *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); > + *eax = env->features[FEAT_XSAVE]; > } else if (count < ARRAY_SIZE(ext_save_areas)) { > const ExtSaveArea *esa = &ext_save_areas[count]; > if ((env->features[esa->feature] & esa->bits) == esa->bits && > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 015f5b5..f9d74c7 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -411,6 +411,7 @@ typedef enum FeatureWord { > FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */ > FEAT_KVM, /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */ > FEAT_SVM, /* CPUID[8000_000A].EDX */ > + FEAT_XSAVE, /* CPUID[EAX=0xd,ECX=1].EAX */ > FEATURE_WORDS, > } FeatureWord; > > @@ -571,6 +572,11 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */ > #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */ > > +#define CPUID_XSAVE_XSAVEOPT (1U << 0) > +#define CPUID_XSAVE_XSAVEC (1U << 1) > +#define CPUID_XSAVE_XGETBV1 (1U << 2) > +#define CPUID_XSAVE_XSAVES (1U << 3) > + > /* CPUID[0x80000007].EDX flags: */ > #define CPUID_APM_INVTSC (1U << 8) > > -- > 1.8.3.1 >
On 25/11/2014 19:45, Eduardo Habkost wrote: >> > +static const char *cpuid_xsave_feature_name[] = { >> > + "xsaveopt", "xsavec", "xgetbv1", "xsaves", > None of the above features introduce any new state that might need to be > migrated, or will require other changes in QEMU to work, right? > > It looks like they don't introduce any extra state, but if they do, they > need to be added to unmigratable_flags until migration support is > implemented. > > If they require other QEMU changes, it would be nice if KVM reported > them using KVM_CHECK_EXTENSION instead of GET_SUPPORTED_CPUID, so it > wouldn't break "-cpu host". No, they don't. Paolo -- 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
On 25/11/2014 21:02, Paolo Bonzini wrote: > > > +static const char *cpuid_xsave_feature_name[] = { > > > + "xsaveopt", "xsavec", "xgetbv1", "xsaves", > > > > None of the above features introduce any new state that might need to be > > migrated, or will require other changes in QEMU to work, right? > > > > It looks like they don't introduce any extra state, but if they do, they > > need to be added to unmigratable_flags until migration support is > > implemented. > > > > If they require other QEMU changes, it would be nice if KVM reported > > them using KVM_CHECK_EXTENSION instead of GET_SUPPORTED_CPUID, so it > > wouldn't break "-cpu host". > > No, they don't. Actually, xsaves does but I don't think KVM_CHECK_EXTENSION is right. It's just another MSR, and we haven't used KVM_CHECK_EXTENSION for new MSRs and new XSAVE areas (last example: avx512). Since no hardware really exists for it, and KVM does not support it anyway, I think it's simplest to leave xsaves out for now. Is this right? Paolo -- 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
On Wed, Nov 26, 2014 at 10:20:12AM +0100, Paolo Bonzini wrote: > > > On 25/11/2014 21:02, Paolo Bonzini wrote: > > > > +static const char *cpuid_xsave_feature_name[] = { > > > > + "xsaveopt", "xsavec", "xgetbv1", "xsaves", > > > > > > None of the above features introduce any new state that might need to be > > > migrated, or will require other changes in QEMU to work, right? > > > > > > It looks like they don't introduce any extra state, but if they do, they > > > need to be added to unmigratable_flags until migration support is > > > implemented. > > > > > > If they require other QEMU changes, it would be nice if KVM reported > > > them using KVM_CHECK_EXTENSION instead of GET_SUPPORTED_CPUID, so it > > > wouldn't break "-cpu host". > > > > No, they don't. > > Actually, xsaves does but I don't think KVM_CHECK_EXTENSION is right. > It's just another MSR, and we haven't used KVM_CHECK_EXTENSION for new > MSRs and new XSAVE areas (last example: avx512). If the changes needed are only to support migration (this is the case if it's just another MSR handled by KVM, or additional XSAVE areas), GET_SUPPORTED_CPUID is still reasonable, because features that are unknown to QEMU are always considered unmigratable until we add the feature name to the feature_name arrays. (That's why we need to know if the feature introduces additional state when adding the feature names to the array.) If other changes are required to make the feature work even if no migration is required, then adding them to GET_SUPPORTED_CPUID would break "-cpu host" on older QEMUs. I don't think that's the case here, but I wanted to confirm. (Should we add those observations to Documentation/virtual/kvm/api.txt?) > > Since no hardware really exists for it, and KVM does not support it > anyway, I think it's simplest to leave xsaves out for now. Is this right? If unsure, it won't hurt to add the feature to unmigratable_features by now. Making QEMU aware of the feature name is still useful.
On 26/11/2014 12:40, Eduardo Habkost wrote: > On Wed, Nov 26, 2014 at 10:20:12AM +0100, Paolo Bonzini wrote: >> >> >> On 25/11/2014 21:02, Paolo Bonzini wrote: >>>>> +static const char *cpuid_xsave_feature_name[] = { >>>>> + "xsaveopt", "xsavec", "xgetbv1", "xsaves", >>>> >>>> None of the above features introduce any new state that might need to be >>>> migrated, or will require other changes in QEMU to work, right? >>>> >>>> It looks like they don't introduce any extra state, but if they do, they >>>> need to be added to unmigratable_flags until migration support is >>>> implemented. >>>> >>>> If they require other QEMU changes, it would be nice if KVM reported >>>> them using KVM_CHECK_EXTENSION instead of GET_SUPPORTED_CPUID, so it >>>> wouldn't break "-cpu host". >>> >>> No, they don't. >> >> Actually, xsaves does but I don't think KVM_CHECK_EXTENSION is right. >> It's just another MSR, and we haven't used KVM_CHECK_EXTENSION for new >> MSRs and new XSAVE areas (last example: avx512). > > If the changes needed are only to support migration (this is the case if > it's just another MSR handled by KVM, or additional XSAVE areas), > GET_SUPPORTED_CPUID is still reasonable, because features that are > unknown to QEMU are always considered unmigratable until we add the > feature name to the feature_name arrays. (That's why we need to know if > the feature introduces additional state when adding the feature names to > the array.) > > If other changes are required to make the feature work even if no > migration is required, then adding them to GET_SUPPORTED_CPUID would > break "-cpu host" on older QEMUs. I don't think that's the case here, > but I wanted to confirm. KVM may need more changes (I don't know, the details of the feature are not public yet), but a new userspace API is very unlikely based on Intel documentation. > (Should we add those observations to Documentation/virtual/kvm/api.txt?) > >> >> Since no hardware really exists for it, and KVM does not support it >> anyway, I think it's simplest to leave xsaves out for now. Is this right? > > If unsure, it won't hurt to add the feature to unmigratable_features by > now. Making QEMU aware of the feature name is still useful. Ok, thanks. Paolo -- 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
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e9df33e..a2dde11 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -274,6 +274,17 @@ static const char *cpuid_apm_edx_feature_name[] = { NULL, NULL, NULL, NULL, }; +static const char *cpuid_xsave_feature_name[] = { + "xsaveopt", "xsavec", "xgetbv1", "xsaves", + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, +}; + #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) @@ -391,6 +402,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .tcg_features = TCG_APM_FEATURES, .unmigratable_flags = CPUID_APM_INVTSC, }, + [FEAT_XSAVE] = { + .feat_names = cpuid_xsave_feature_name, + .cpuid_eax = 0xd, + .cpuid_needs_ecx = true, .cpuid_ecx = 1, + .cpuid_reg = R_EAX, + .tcg_features = 0, + }, }; typedef struct X86RegisterInfo32 { @@ -1018,6 +1036,8 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_EXT2_SYSCALL, .features[FEAT_8000_0001_ECX] = CPUID_EXT3_LAHF_LM, + .features[FEAT_XSAVE] = + CPUID_XSAVE_XSAVEOPT, .xlevel = 0x8000000A, .model_id = "Intel Xeon E312xx (Sandy Bridge)", }, @@ -1051,6 +1071,8 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RTM, + .features[FEAT_XSAVE] = + CPUID_XSAVE_XSAVEOPT, .xlevel = 0x8000000A, .model_id = "Intel Core Processor (Haswell)", }, @@ -1085,6 +1107,8 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP, + .features[FEAT_XSAVE] = + CPUID_XSAVE_XSAVEOPT, .xlevel = 0x8000000A, .model_id = "Intel Core Processor (Broadwell)", }, @@ -1202,6 +1226,7 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM, + /* no xsaveopt! */ .xlevel = 0x8000001A, .model_id = "AMD Opteron 62xx class CPU", }, @@ -1236,6 +1261,7 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM, + /* no xsaveopt! */ .xlevel = 0x8000001A, .model_id = "AMD Opteron 63xx class CPU", }, @@ -2377,7 +2403,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); *ebx = *ecx; } else if (count == 1) { - *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX); + *eax = env->features[FEAT_XSAVE]; } else if (count < ARRAY_SIZE(ext_save_areas)) { const ExtSaveArea *esa = &ext_save_areas[count]; if ((env->features[esa->feature] & esa->bits) == esa->bits && diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 015f5b5..f9d74c7 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -411,6 +411,7 @@ typedef enum FeatureWord { FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */ FEAT_KVM, /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */ FEAT_SVM, /* CPUID[8000_000A].EDX */ + FEAT_XSAVE, /* CPUID[EAX=0xd,ECX=1].EAX */ FEATURE_WORDS, } FeatureWord; @@ -571,6 +572,11 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */ #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */ +#define CPUID_XSAVE_XSAVEOPT (1U << 0) +#define CPUID_XSAVE_XSAVEC (1U << 1) +#define CPUID_XSAVE_XGETBV1 (1U << 2) +#define CPUID_XSAVE_XSAVES (1U << 3) + /* CPUID[0x80000007].EDX flags: */ #define CPUID_APM_INVTSC (1U << 8)
These represent xsave-related capabilities of the processor, and KVM may or may not support them. Add feature bits so that they are considered by "-cpu ...,enforce", and use the new feature work instead of calling kvm_arch_get_supported_cpuid. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target-i386/cpu.c | 28 +++++++++++++++++++++++++++- target-i386/cpu.h | 6 ++++++ 2 files changed, 33 insertions(+), 1 deletion(-)