| Message ID | 1539578845-37944-3-git-send-email-robert.hu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: QEMU side support on MSR based features | expand |
On Mon, Oct 15, 2018 at 12:47:24PM +0800, Robert Hoo wrote: > Add FeatureWordType indicator in struct FeatureWordInfo. > Change feature_word_info[] accordingly. > Change existing functions that refer to feature_word_info[] accordingly. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > target/i386/cpu.c | 188 +++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 136 insertions(+), 52 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index c88876d..d191b9c 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > /* missing: > CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */ > > +typedef enum FeatureWordType { > + CPUID_FEATURE_WORD, > + MSR_FEATURE_WORD, > +} FeatureWordType; > + > typedef struct FeatureWordInfo { > + FeatureWordType type; > /* feature flags names are taken from "Intel Processor Identification and > * the CPUID Instruction" and AMD's "CPUID Specification". > * In cases of disagreement between feature naming conventions, > * aliases may be added. > */ > const char *feat_names[32]; > - uint32_t cpuid_eax; /* Input EAX for CPUID */ > - bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ > - uint32_t cpuid_ecx; /* Input ECX value for CPUID */ > - int cpuid_reg; /* output register (R_* constant) */ > + union { > + /* If type==CPUID_FEATURE_WORD */ > + struct { > + uint32_t eax; /* Input EAX for CPUID */ > + bool needs_ecx; /* CPUID instruction uses ECX as input */ > + uint32_t ecx; /* Input ECX value for CPUID */ > + int reg; /* output register (R_* constant) */ > + } cpuid; > + /* If type==MSR_FEATURE_WORD */ > + struct { > + uint32_t index; > + struct { /*CPUID that enumerate this MSR*/ > + FeatureWord cpuid_class; > + uint32_t cpuid_flag; > + } cpuid_dep; > + } msr; > + }; > uint32_t tcg_features; /* Feature flags supported by TCG */ > uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */ > uint32_t migratable_flags; /* Feature flags known to be migratable */ > @@ -790,6 +809,7 @@ typedef struct FeatureWordInfo { > > static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > [FEAT_1_EDX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "fpu", "vme", "de", "pse", > "tsc", "msr", "pae", "mce", > @@ -800,10 +820,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > "fxsr", "sse", "sse2", "ss", > "ht" /* Intel htt */, "tm", "ia64", "pbe", > }, > - .cpuid_eax = 1, .cpuid_reg = R_EDX, > + .cpuid = {.eax = 1, .reg = R_EDX, }, > .tcg_features = TCG_FEATURES, > }, > [FEAT_1_ECX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor", > "ds-cpl", "vmx", "smx", "est", > @@ -814,7 +835,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > "tsc-deadline", "aes", "xsave", NULL /* osxsave */, > "avx", "f16c", "rdrand", "hypervisor", > }, > - .cpuid_eax = 1, .cpuid_reg = R_ECX, > + .cpuid = { .eax = 1, .reg = R_ECX, }, > .tcg_features = TCG_EXT_FEATURES, > }, > /* Feature names that are already defined on feature_name[] but > @@ -823,6 +844,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD. > */ > [FEAT_8000_0001_EDX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */, > NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */, > @@ -833,10 +855,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp", > NULL, "lm", "3dnowext", "3dnow", > }, > - .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX, > + .cpuid = { .eax = 0x80000001, .reg = R_EDX, }, > .tcg_features = TCG_EXT2_FEATURES, > }, > [FEAT_8000_0001_ECX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "lahf-lm", "cmp-legacy", "svm", "extapic", > "cr8legacy", "abm", "sse4a", "misalignsse", > @@ -847,7 +870,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > "perfctr-nb", NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX, > + .cpuid = { .eax = 0x80000001, .reg = R_ECX, }, > .tcg_features = TCG_EXT3_FEATURES, > /* > * TOPOEXT is always allowed but can't be enabled blindly by > @@ -857,6 +880,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > .no_autoenable_flags = CPUID_EXT3_TOPOEXT, > }, > [FEAT_C000_0001_EDX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL, NULL, "xstore", "xstore-en", > NULL, NULL, "xcrypt", "xcrypt-en", > @@ -867,10 +891,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX, > + .cpuid = { .eax = 0xC0000001, .reg = R_EDX, }, > .tcg_features = TCG_EXT4_FEATURES, > }, > [FEAT_KVM] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock", > "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt", > @@ -881,10 +906,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > "kvmclock-stable-bit", NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX, > + .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EAX, }, > .tcg_features = TCG_KVM_FEATURES, > }, > [FEAT_KVM_HINTS] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "kvm-hint-dedicated", NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > @@ -895,7 +921,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EDX, > + .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EDX, }, > .tcg_features = TCG_KVM_FEATURES, > /* > * KVM hints aren't auto-enabled by -cpu host, they need to be > @@ -904,6 +930,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > .no_autoenable_flags = ~0U, > }, > [FEAT_HYPERV_EAX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL /* hv_msr_vp_runtime_access */, NULL /* hv_msr_time_refcount_access */, > NULL /* hv_msr_synic_access */, NULL /* hv_msr_stimer_access */, > @@ -918,9 +945,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x40000003, .cpuid_reg = R_EAX, > + .cpuid = { .eax = 0x40000003, .reg = R_EAX, }, > }, > [FEAT_HYPERV_EBX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL /* hv_create_partitions */, NULL /* hv_access_partition_id */, > NULL /* hv_access_memory_pool */, NULL /* hv_adjust_message_buffers */, > @@ -934,9 +962,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x40000003, .cpuid_reg = R_EBX, > + .cpuid = { .eax = 0x40000003, .reg = R_EBX, }, > }, > [FEAT_HYPERV_EDX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL /* hv_mwait */, NULL /* hv_guest_debugging */, > NULL /* hv_perf_monitor */, NULL /* hv_cpu_dynamic_part */, > @@ -949,9 +978,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x40000003, .cpuid_reg = R_EDX, > + .cpuid = { .eax = 0x40000003, .reg = R_EDX, }, > }, > [FEAT_SVM] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "npt", "lbrv", "svm-lock", "nrip-save", > "tsc-scale", "vmcb-clean", "flushbyasid", "decodeassists", > @@ -962,10 +992,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX, > + .cpuid = { .eax = 0x8000000A, .reg = R_EDX, }, > .tcg_features = TCG_SVM_FEATURES, > }, > [FEAT_7_0_EBX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "fsgsbase", "tsc-adjust", NULL, "bmi1", > "hle", "avx2", NULL, "smep", > @@ -976,12 +1007,15 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > "clwb", "intel-pt", "avx512pf", "avx512er", > "avx512cd", "sha-ni", "avx512bw", "avx512vl", > }, > - .cpuid_eax = 7, > - .cpuid_needs_ecx = true, .cpuid_ecx = 0, > - .cpuid_reg = R_EBX, > + .cpuid = { > + .eax = 7, > + .needs_ecx = true, .ecx = 0, > + .reg = R_EBX, > + }, > .tcg_features = TCG_7_0_EBX_FEATURES, > }, > [FEAT_7_0_ECX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL, "avx512vbmi", "umip", "pku", > NULL /* ospke */, NULL, "avx512vbmi2", NULL, > @@ -992,12 +1026,15 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, "cldemote", NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 7, > - .cpuid_needs_ecx = true, .cpuid_ecx = 0, > - .cpuid_reg = R_ECX, > + .cpuid = { > + .eax = 7, > + .needs_ecx = true, .ecx = 0, > + .reg = R_ECX, > + }, > .tcg_features = TCG_7_0_ECX_FEATURES, > }, > [FEAT_7_0_EDX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL, NULL, "avx512-4vnniw", "avx512-4fmaps", > NULL, NULL, NULL, NULL, > @@ -1008,13 +1045,16 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, "spec-ctrl", NULL, > NULL, "arch-capabilities", NULL, "ssbd", > }, > - .cpuid_eax = 7, > - .cpuid_needs_ecx = true, .cpuid_ecx = 0, > - .cpuid_reg = R_EDX, > + .cpuid = { > + .eax = 7, > + .needs_ecx = true, .ecx = 0, > + .reg = R_EDX, > + }, > .tcg_features = TCG_7_0_EDX_FEATURES, > .unmigratable_flags = CPUID_7_0_EDX_ARCH_CAPABILITIES, > }, > [FEAT_8000_0007_EDX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > @@ -1025,12 +1065,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x80000007, > - .cpuid_reg = R_EDX, > + .cpuid = { .eax = 0x80000007, .reg = R_EDX, }, > .tcg_features = TCG_APM_FEATURES, > .unmigratable_flags = CPUID_APM_INVTSC, > }, > [FEAT_8000_0008_EBX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > @@ -1041,12 +1081,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > "amd-ssbd", "virt-ssbd", "amd-no-ssb", NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0x80000008, > - .cpuid_reg = R_EBX, > + .cpuid = { .eax = 0x80000008, .reg = R_EBX, }, > .tcg_features = 0, > .unmigratable_flags = 0, > }, > [FEAT_XSAVE] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > "xsaveopt", "xsavec", "xgetbv1", "xsaves", > NULL, NULL, NULL, NULL, > @@ -1057,12 +1097,15 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 0xd, > - .cpuid_needs_ecx = true, .cpuid_ecx = 1, > - .cpuid_reg = R_EAX, > + .cpuid = { > + .eax = 0xd, > + .needs_ecx = true, .ecx = 1, > + .reg = R_EAX, > + }, > .tcg_features = TCG_XSAVE_FEATURES, > }, > [FEAT_6_EAX] = { > + .type = CPUID_FEATURE_WORD, > .feat_names = { > NULL, NULL, "arat", NULL, > NULL, NULL, NULL, NULL, > @@ -1073,13 +1116,16 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, NULL, NULL, > }, > - .cpuid_eax = 6, .cpuid_reg = R_EAX, > + .cpuid = { .eax = 6, .reg = R_EAX, }, > .tcg_features = TCG_6_EAX_FEATURES, > }, > [FEAT_XSAVE_COMP_LO] = { > - .cpuid_eax = 0xD, > - .cpuid_needs_ecx = true, .cpuid_ecx = 0, > - .cpuid_reg = R_EAX, > + .type = CPUID_FEATURE_WORD, > + .cpuid = { > + .eax = 0xD, > + .needs_ecx = true, .ecx = 0, > + .reg = R_EAX, > + }, > .tcg_features = ~0U, > .migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK | > XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK | > @@ -1087,9 +1133,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > XSTATE_PKRU_MASK, > }, > [FEAT_XSAVE_COMP_HI] = { > - .cpuid_eax = 0xD, > - .cpuid_needs_ecx = true, .cpuid_ecx = 0, > - .cpuid_reg = R_EDX, > + .type = CPUID_FEATURE_WORD, > + .cpuid = { > + .eax = 0xD, > + .needs_ecx = true, .ecx = 0, > + .reg = R_EDX, > + }, > .tcg_features = ~0U, > }, > }; > @@ -2975,21 +3024,41 @@ static const TypeInfo host_x86_cpu_type_info = { > > #endif > > +static char *feature_word_description(FeatureWordInfo *f, uint32_t bit) > +{ > + assert(f->type == CPUID_FEATURE_WORD || f->type == MSR_FEATURE_WORD); > + > + switch (f->type) { > + case CPUID_FEATURE_WORD: > + { > + const char *reg = get_register_name_32(f->cpuid.reg); > + assert(reg); > + return g_strdup_printf("CPUID.%02XH:%s", > + f->cpuid.eax, reg); > + } > + case MSR_FEATURE_WORD: > + return g_strdup_printf("MSR(%02XH)", > + f->msr.index); > + } > + > + return NULL; > +} > + > static void report_unavailable_features(FeatureWord w, uint32_t mask) > { > FeatureWordInfo *f = &feature_word_info[w]; > int i; > + char *feat_word_str; > > for (i = 0; i < 32; ++i) { > if ((1UL << i) & mask) { > - const char *reg = get_register_name_32(f->cpuid_reg); > - assert(reg); > - warn_report("%s doesn't support requested feature: " > - "CPUID.%02XH:%s%s%s [bit %d]", > + feat_word_str = feature_word_description(f, i); > + warn_report("%s doesn't support requested feature: %s%s%s [bit %d]", > accel_uses_host_cpuid() ? "host" : "TCG", > - f->cpuid_eax, reg, > + feat_word_str, > f->feat_names[i] ? "." : "", > f->feat_names[i] ? f->feat_names[i] : "", i); > + g_free(feat_word_str); > } > } > } > @@ -3233,11 +3302,18 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > > for (w = 0; w < FEATURE_WORDS; w++) { > FeatureWordInfo *wi = &feature_word_info[w]; > + /* > + * We didn't have MSR features when "feature-words" was > + * introduced. Therefore skipped other type entries. > + */ > + if (wi->type != CPUID_FEATURE_WORD) { > + continue; > + } > X86CPUFeatureWordInfo *qwi = &word_infos[w]; > - qwi->cpuid_input_eax = wi->cpuid_eax; > - qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx; > - qwi->cpuid_input_ecx = wi->cpuid_ecx; > - qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum; > + qwi->cpuid_input_eax = wi->cpuid.eax; > + qwi->has_cpuid_input_ecx = wi->cpuid.needs_ecx; > + qwi->cpuid_input_ecx = wi->cpuid.ecx; > + qwi->cpuid_register = x86_reg_info_32[wi->cpuid.reg].qapi_enum; > qwi->features = array[w]; > > /* List will be in reverse order, but order shouldn't matter */ > @@ -3610,12 +3686,19 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, > bool migratable_only) > { > FeatureWordInfo *wi = &feature_word_info[w]; > - uint32_t r; > + uint32_t r = 0; > > if (kvm_enabled()) { > - r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, > - wi->cpuid_ecx, > - wi->cpuid_reg); > + switch (wi->type) { > + case CPUID_FEATURE_WORD: > + r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid.eax, > + wi->cpuid.ecx, > + wi->cpuid.reg); > + break; > + case MSR_FEATURE_WORD: > + r = kvm_arch_get_supported_msr_feature(kvm_state, wi->msr.index); > + break; > + } > } else if (hvf_enabled()) { I just noticed this issue: You need to check wi->type for the other accelerators too, otherwise we're going to call hvf_get_supported_cpuid() with bogus EAX/ECX values. I suggest the fixup below. The rest of the patch looks good to me. diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 2b58bca62e..bcfe6928c1 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -3729,6 +3729,9 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, break; } } else if (hvf_enabled()) { + if (wi->type != CPUID_FEATURE_WORD) { + return 0; + } r = hvf_get_supported_cpuid(wi->cpuid_eax, wi->cpuid_ecx, wi->cpuid_reg); > r = hvf_get_supported_cpuid(wi->cpuid_eax, > wi->cpuid_ecx, > @@ -4680,9 +4763,10 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu, FeatureWord w) > { > CPUX86State *env = &cpu->env; > FeatureWordInfo *fi = &feature_word_info[w]; > - uint32_t eax = fi->cpuid_eax; > + uint32_t eax = fi->cpuid.eax; > uint32_t region = eax & 0xF0000000; > > + assert(feature_word_info[w].type == CPUID_FEATURE_WORD); > if (!env->features[w]) { > return; > } > -- > 1.8.3.1 > >
On Mon, Oct 15, 2018 at 12:47:24PM +0800, Robert Hoo wrote: > Add FeatureWordType indicator in struct FeatureWordInfo. > Change feature_word_info[] accordingly. > Change existing functions that refer to feature_word_info[] accordingly. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > target/i386/cpu.c | 188 +++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 136 insertions(+), 52 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index c88876d..d191b9c 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > /* missing: > CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */ > > +typedef enum FeatureWordType { > + CPUID_FEATURE_WORD, > + MSR_FEATURE_WORD, > +} FeatureWordType; > + > typedef struct FeatureWordInfo { > + FeatureWordType type; > /* feature flags names are taken from "Intel Processor Identification and > * the CPUID Instruction" and AMD's "CPUID Specification". > * In cases of disagreement between feature naming conventions, > * aliases may be added. > */ > const char *feat_names[32]; > - uint32_t cpuid_eax; /* Input EAX for CPUID */ > - bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ > - uint32_t cpuid_ecx; /* Input ECX value for CPUID */ > - int cpuid_reg; /* output register (R_* constant) */ > + union { > + /* If type==CPUID_FEATURE_WORD */ > + struct { > + uint32_t eax; /* Input EAX for CPUID */ > + bool needs_ecx; /* CPUID instruction uses ECX as input */ > + uint32_t ecx; /* Input ECX value for CPUID */ > + int reg; /* output register (R_* constant) */ > + } cpuid; > + /* If type==MSR_FEATURE_WORD */ > + struct { > + uint32_t index; > + struct { /*CPUID that enumerate this MSR*/ > + FeatureWord cpuid_class; > + uint32_t cpuid_flag; > + } cpuid_dep; Aren't you going to use this field anywhere? Probably we want to prevent the VM from starting if a bit is set in the feature world but the cpuid_dep bit is not set. e.g.: qemu-system-x86_64 -cpu Skylake-Client,-arch-capabilities,+rsba probably should fail to start. > + } msr; > + }; [...]
On Wed, 2018-10-24 at 07:16 -0300, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 12:47:24PM +0800, Robert Hoo wrote: > > Add FeatureWordType indicator in struct FeatureWordInfo. > > Change feature_word_info[] accordingly. > > Change existing functions that refer to feature_word_info[] > > accordingly. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > target/i386/cpu.c | 188 +++++++++++++++++++++++++++++++++++++++--- > > ------------ > > 1 file changed, 136 insertions(+), 52 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index c88876d..d191b9c 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char > > *dst, uint32_t vendor1, > > /* missing: > > CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */ > > > > +typedef enum FeatureWordType { > > + CPUID_FEATURE_WORD, > > + MSR_FEATURE_WORD, > > +} FeatureWordType; > > + > > typedef struct FeatureWordInfo { > > + FeatureWordType type; > > /* feature flags names are taken from "Intel Processor > > Identification and > > * the CPUID Instruction" and AMD's "CPUID Specification". > > * In cases of disagreement between feature naming > > conventions, > > * aliases may be added. > > */ > > const char *feat_names[32]; > > - uint32_t cpuid_eax; /* Input EAX for CPUID */ > > - bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input > > */ > > - uint32_t cpuid_ecx; /* Input ECX value for CPUID */ > > - int cpuid_reg; /* output register (R_* constant) */ > > + union { > > + /* If type==CPUID_FEATURE_WORD */ > > + struct { > > + uint32_t eax; /* Input EAX for CPUID */ > > + bool needs_ecx; /* CPUID instruction uses ECX as input > > */ > > + uint32_t ecx; /* Input ECX value for CPUID */ > > + int reg; /* output register (R_* constant) */ > > + } cpuid; > > + /* If type==MSR_FEATURE_WORD */ > > + struct { > > + uint32_t index; > > + struct { /*CPUID that enumerate this MSR*/ > > + FeatureWord cpuid_class; > > + uint32_t cpuid_flag; > > + } cpuid_dep; > > Aren't you going to use this field anywhere? Probably we want to > prevent the VM from starting if a bit is set in the feature world > but the cpuid_dep bit is not set. > > e.g.: > qemu-system-x86_64 -cpu Skylake-Client,-arch-capabilities,+rsba > probably should fail to start. How about in x86_cpu_filter_features() filters the MSR feature word, if its dependent CPUID feature bit is not set? The filter results will be record in cpu->filtered_features, and print out, like other filtered features. > > > + } msr; > > + }; > > [...] >
On Thu, Oct 25, 2018 at 11:06:59AM +0800, Robert Hoo wrote: > On Wed, 2018-10-24 at 07:16 -0300, Eduardo Habkost wrote: [...] > > > + struct { /*CPUID that enumerate this MSR*/ > > > + FeatureWord cpuid_class; > > > + uint32_t cpuid_flag; > > > + } cpuid_dep; > > > > Aren't you going to use this field anywhere? Probably we want to > > prevent the VM from starting if a bit is set in the feature world > > but the cpuid_dep bit is not set. > > > > e.g.: > > qemu-system-x86_64 -cpu Skylake-Client,-arch-capabilities,+rsba > > probably should fail to start. > > How about in x86_cpu_filter_features() filters the MSR feature word, if > its dependent CPUID feature bit is not set? > The filter results will be record in cpu->filtered_features, and print > out, like other filtered features. Sounds good to me.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index c88876d..d191b9c 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, /* missing: CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */ +typedef enum FeatureWordType { + CPUID_FEATURE_WORD, + MSR_FEATURE_WORD, +} FeatureWordType; + typedef struct FeatureWordInfo { + FeatureWordType type; /* feature flags names are taken from "Intel Processor Identification and * the CPUID Instruction" and AMD's "CPUID Specification". * In cases of disagreement between feature naming conventions, * aliases may be added. */ const char *feat_names[32]; - uint32_t cpuid_eax; /* Input EAX for CPUID */ - bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ - uint32_t cpuid_ecx; /* Input ECX value for CPUID */ - int cpuid_reg; /* output register (R_* constant) */ + union { + /* If type==CPUID_FEATURE_WORD */ + struct { + uint32_t eax; /* Input EAX for CPUID */ + bool needs_ecx; /* CPUID instruction uses ECX as input */ + uint32_t ecx; /* Input ECX value for CPUID */ + int reg; /* output register (R_* constant) */ + } cpuid; + /* If type==MSR_FEATURE_WORD */ + struct { + uint32_t index; + struct { /*CPUID that enumerate this MSR*/ + FeatureWord cpuid_class; + uint32_t cpuid_flag; + } cpuid_dep; + } msr; + }; uint32_t tcg_features; /* Feature flags supported by TCG */ uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */ uint32_t migratable_flags; /* Feature flags known to be migratable */ @@ -790,6 +809,7 @@ typedef struct FeatureWordInfo { static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { [FEAT_1_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "fpu", "vme", "de", "pse", "tsc", "msr", "pae", "mce", @@ -800,10 +820,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { "fxsr", "sse", "sse2", "ss", "ht" /* Intel htt */, "tm", "ia64", "pbe", }, - .cpuid_eax = 1, .cpuid_reg = R_EDX, + .cpuid = {.eax = 1, .reg = R_EDX, }, .tcg_features = TCG_FEATURES, }, [FEAT_1_ECX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor", "ds-cpl", "vmx", "smx", "est", @@ -814,7 +835,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { "tsc-deadline", "aes", "xsave", NULL /* osxsave */, "avx", "f16c", "rdrand", "hypervisor", }, - .cpuid_eax = 1, .cpuid_reg = R_ECX, + .cpuid = { .eax = 1, .reg = R_ECX, }, .tcg_features = TCG_EXT_FEATURES, }, /* Feature names that are already defined on feature_name[] but @@ -823,6 +844,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD. */ [FEAT_8000_0001_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */, NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */, @@ -833,10 +855,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp", NULL, "lm", "3dnowext", "3dnow", }, - .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX, + .cpuid = { .eax = 0x80000001, .reg = R_EDX, }, .tcg_features = TCG_EXT2_FEATURES, }, [FEAT_8000_0001_ECX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "lahf-lm", "cmp-legacy", "svm", "extapic", "cr8legacy", "abm", "sse4a", "misalignsse", @@ -847,7 +870,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { "perfctr-nb", NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX, + .cpuid = { .eax = 0x80000001, .reg = R_ECX, }, .tcg_features = TCG_EXT3_FEATURES, /* * TOPOEXT is always allowed but can't be enabled blindly by @@ -857,6 +880,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .no_autoenable_flags = CPUID_EXT3_TOPOEXT, }, [FEAT_C000_0001_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, "xstore", "xstore-en", NULL, NULL, "xcrypt", "xcrypt-en", @@ -867,10 +891,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX, + .cpuid = { .eax = 0xC0000001, .reg = R_EDX, }, .tcg_features = TCG_EXT4_FEATURES, }, [FEAT_KVM] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock", "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt", @@ -881,10 +906,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { "kvmclock-stable-bit", NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX, + .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EAX, }, .tcg_features = TCG_KVM_FEATURES, }, [FEAT_KVM_HINTS] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "kvm-hint-dedicated", NULL, NULL, NULL, NULL, NULL, NULL, NULL, @@ -895,7 +921,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EDX, + .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EDX, }, .tcg_features = TCG_KVM_FEATURES, /* * KVM hints aren't auto-enabled by -cpu host, they need to be @@ -904,6 +930,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .no_autoenable_flags = ~0U, }, [FEAT_HYPERV_EAX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL /* hv_msr_vp_runtime_access */, NULL /* hv_msr_time_refcount_access */, NULL /* hv_msr_synic_access */, NULL /* hv_msr_stimer_access */, @@ -918,9 +945,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x40000003, .cpuid_reg = R_EAX, + .cpuid = { .eax = 0x40000003, .reg = R_EAX, }, }, [FEAT_HYPERV_EBX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL /* hv_create_partitions */, NULL /* hv_access_partition_id */, NULL /* hv_access_memory_pool */, NULL /* hv_adjust_message_buffers */, @@ -934,9 +962,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x40000003, .cpuid_reg = R_EBX, + .cpuid = { .eax = 0x40000003, .reg = R_EBX, }, }, [FEAT_HYPERV_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL /* hv_mwait */, NULL /* hv_guest_debugging */, NULL /* hv_perf_monitor */, NULL /* hv_cpu_dynamic_part */, @@ -949,9 +978,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x40000003, .cpuid_reg = R_EDX, + .cpuid = { .eax = 0x40000003, .reg = R_EDX, }, }, [FEAT_SVM] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "npt", "lbrv", "svm-lock", "nrip-save", "tsc-scale", "vmcb-clean", "flushbyasid", "decodeassists", @@ -962,10 +992,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX, + .cpuid = { .eax = 0x8000000A, .reg = R_EDX, }, .tcg_features = TCG_SVM_FEATURES, }, [FEAT_7_0_EBX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep", @@ -976,12 +1007,15 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { "clwb", "intel-pt", "avx512pf", "avx512er", "avx512cd", "sha-ni", "avx512bw", "avx512vl", }, - .cpuid_eax = 7, - .cpuid_needs_ecx = true, .cpuid_ecx = 0, - .cpuid_reg = R_EBX, + .cpuid = { + .eax = 7, + .needs_ecx = true, .ecx = 0, + .reg = R_EBX, + }, .tcg_features = TCG_7_0_EBX_FEATURES, }, [FEAT_7_0_ECX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, "avx512vbmi", "umip", "pku", NULL /* ospke */, NULL, "avx512vbmi2", NULL, @@ -992,12 +1026,15 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, "cldemote", NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 7, - .cpuid_needs_ecx = true, .cpuid_ecx = 0, - .cpuid_reg = R_ECX, + .cpuid = { + .eax = 7, + .needs_ecx = true, .ecx = 0, + .reg = R_ECX, + }, .tcg_features = TCG_7_0_ECX_FEATURES, }, [FEAT_7_0_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, "avx512-4vnniw", "avx512-4fmaps", NULL, NULL, NULL, NULL, @@ -1008,13 +1045,16 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, "spec-ctrl", NULL, NULL, "arch-capabilities", NULL, "ssbd", }, - .cpuid_eax = 7, - .cpuid_needs_ecx = true, .cpuid_ecx = 0, - .cpuid_reg = R_EDX, + .cpuid = { + .eax = 7, + .needs_ecx = true, .ecx = 0, + .reg = R_EDX, + }, .tcg_features = TCG_7_0_EDX_FEATURES, .unmigratable_flags = CPUID_7_0_EDX_ARCH_CAPABILITIES, }, [FEAT_8000_0007_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, @@ -1025,12 +1065,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x80000007, - .cpuid_reg = R_EDX, + .cpuid = { .eax = 0x80000007, .reg = R_EDX, }, .tcg_features = TCG_APM_FEATURES, .unmigratable_flags = CPUID_APM_INVTSC, }, [FEAT_8000_0008_EBX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, @@ -1041,12 +1081,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { "amd-ssbd", "virt-ssbd", "amd-no-ssb", NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0x80000008, - .cpuid_reg = R_EBX, + .cpuid = { .eax = 0x80000008, .reg = R_EBX, }, .tcg_features = 0, .unmigratable_flags = 0, }, [FEAT_XSAVE] = { + .type = CPUID_FEATURE_WORD, .feat_names = { "xsaveopt", "xsavec", "xgetbv1", "xsaves", NULL, NULL, NULL, NULL, @@ -1057,12 +1097,15 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 0xd, - .cpuid_needs_ecx = true, .cpuid_ecx = 1, - .cpuid_reg = R_EAX, + .cpuid = { + .eax = 0xd, + .needs_ecx = true, .ecx = 1, + .reg = R_EAX, + }, .tcg_features = TCG_XSAVE_FEATURES, }, [FEAT_6_EAX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, "arat", NULL, NULL, NULL, NULL, NULL, @@ -1073,13 +1116,16 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, - .cpuid_eax = 6, .cpuid_reg = R_EAX, + .cpuid = { .eax = 6, .reg = R_EAX, }, .tcg_features = TCG_6_EAX_FEATURES, }, [FEAT_XSAVE_COMP_LO] = { - .cpuid_eax = 0xD, - .cpuid_needs_ecx = true, .cpuid_ecx = 0, - .cpuid_reg = R_EAX, + .type = CPUID_FEATURE_WORD, + .cpuid = { + .eax = 0xD, + .needs_ecx = true, .ecx = 0, + .reg = R_EAX, + }, .tcg_features = ~0U, .migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK | XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK | @@ -1087,9 +1133,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { XSTATE_PKRU_MASK, }, [FEAT_XSAVE_COMP_HI] = { - .cpuid_eax = 0xD, - .cpuid_needs_ecx = true, .cpuid_ecx = 0, - .cpuid_reg = R_EDX, + .type = CPUID_FEATURE_WORD, + .cpuid = { + .eax = 0xD, + .needs_ecx = true, .ecx = 0, + .reg = R_EDX, + }, .tcg_features = ~0U, }, }; @@ -2975,21 +3024,41 @@ static const TypeInfo host_x86_cpu_type_info = { #endif +static char *feature_word_description(FeatureWordInfo *f, uint32_t bit) +{ + assert(f->type == CPUID_FEATURE_WORD || f->type == MSR_FEATURE_WORD); + + switch (f->type) { + case CPUID_FEATURE_WORD: + { + const char *reg = get_register_name_32(f->cpuid.reg); + assert(reg); + return g_strdup_printf("CPUID.%02XH:%s", + f->cpuid.eax, reg); + } + case MSR_FEATURE_WORD: + return g_strdup_printf("MSR(%02XH)", + f->msr.index); + } + + return NULL; +} + static void report_unavailable_features(FeatureWord w, uint32_t mask) { FeatureWordInfo *f = &feature_word_info[w]; int i; + char *feat_word_str; for (i = 0; i < 32; ++i) { if ((1UL << i) & mask) { - const char *reg = get_register_name_32(f->cpuid_reg); - assert(reg); - warn_report("%s doesn't support requested feature: " - "CPUID.%02XH:%s%s%s [bit %d]", + feat_word_str = feature_word_description(f, i); + warn_report("%s doesn't support requested feature: %s%s%s [bit %d]", accel_uses_host_cpuid() ? "host" : "TCG", - f->cpuid_eax, reg, + feat_word_str, f->feat_names[i] ? "." : "", f->feat_names[i] ? f->feat_names[i] : "", i); + g_free(feat_word_str); } } } @@ -3233,11 +3302,18 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, for (w = 0; w < FEATURE_WORDS; w++) { FeatureWordInfo *wi = &feature_word_info[w]; + /* + * We didn't have MSR features when "feature-words" was + * introduced. Therefore skipped other type entries. + */ + if (wi->type != CPUID_FEATURE_WORD) { + continue; + } X86CPUFeatureWordInfo *qwi = &word_infos[w]; - qwi->cpuid_input_eax = wi->cpuid_eax; - qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx; - qwi->cpuid_input_ecx = wi->cpuid_ecx; - qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum; + qwi->cpuid_input_eax = wi->cpuid.eax; + qwi->has_cpuid_input_ecx = wi->cpuid.needs_ecx; + qwi->cpuid_input_ecx = wi->cpuid.ecx; + qwi->cpuid_register = x86_reg_info_32[wi->cpuid.reg].qapi_enum; qwi->features = array[w]; /* List will be in reverse order, but order shouldn't matter */ @@ -3610,12 +3686,19 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, bool migratable_only) { FeatureWordInfo *wi = &feature_word_info[w]; - uint32_t r; + uint32_t r = 0; if (kvm_enabled()) { - r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, - wi->cpuid_ecx, - wi->cpuid_reg); + switch (wi->type) { + case CPUID_FEATURE_WORD: + r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid.eax, + wi->cpuid.ecx, + wi->cpuid.reg); + break; + case MSR_FEATURE_WORD: + r = kvm_arch_get_supported_msr_feature(kvm_state, wi->msr.index); + break; + } } else if (hvf_enabled()) { r = hvf_get_supported_cpuid(wi->cpuid_eax, wi->cpuid_ecx, @@ -4680,9 +4763,10 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu, FeatureWord w) { CPUX86State *env = &cpu->env; FeatureWordInfo *fi = &feature_word_info[w]; - uint32_t eax = fi->cpuid_eax; + uint32_t eax = fi->cpuid.eax; uint32_t region = eax & 0xF0000000; + assert(feature_word_info[w].type == CPUID_FEATURE_WORD); if (!env->features[w]) { return; }
Add FeatureWordType indicator in struct FeatureWordInfo. Change feature_word_info[] accordingly. Change existing functions that refer to feature_word_info[] accordingly. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- target/i386/cpu.c | 188 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 136 insertions(+), 52 deletions(-)