On Fri, Aug 10, 2018 at 10:06:29PM +0800, Robert Hoo wrote: > Add an util function feature_word_description(), which help construct the > string > describing the feature word (both CPUID and MSR types). > > report_unavailable_features(): add MSR_FEATURE_WORD type support. > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. > x86_cpu_adjust_feat_level(): assert the requested feature must be > CPUID_FEATURE_WORD type. > > Signed-off-by: Robert Hoo <robert...@linux.intel.com> > --- > target/i386/cpu.c | 77 > +++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 58 insertions(+), 19 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index f7c70d9..21ed599 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { > > #endif > > +/* > +*caller should have input str no less than 64 byte length. > +*/ > +#define FEATURE_WORD_DESCPTION_LEN 64 > +static int feature_word_description(char str[], FeatureWordInfo *f, > + uint32_t bit)
I agree with Eric: g_strup_printf() would be much simpler here. > +{ > + int ret; > + > + 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); > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > + "CPUID.%02XH:%s%s%s [bit %d]", > + f->cpuid.eax, reg, > + f->feat_names[bit] ? "." : "", > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > + break; > + } > + case MSR_FEATURE_WORD: > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > + "MSR(%xH).%s [bit %d]", > + f->msr.index, > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); What about leaving the (f->feat_names[bit] ? ... : ...) part in report_unavailable_features() and just make this function return "CPUID[...]" or "MSR[...]"? > + break; > + } > + return ret > 0; > +} > + > static void report_unavailable_features(FeatureWord w, uint32_t mask) > { > FeatureWordInfo *f = &feature_word_info[w]; > int i; > + char feat_word_dscrp_str[FEATURE_WORD_DESCPTION_LEN]; > > 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]", > + feature_word_description(feat_word_dscrp_str, f, i); > + warn_report("%s doesn't support requested feature: %s", > accel_uses_host_cpuid() ? "host" : "TCG", > - f->cpuid_eax, reg, > - f->feat_names[i] ? "." : "", > - f->feat_names[i] ? f->feat_names[i] : "", i); > + feat_word_dscrp_str); > } > } > } > @@ -3276,17 +3306,17 @@ static void x86_cpu_get_feature_words(Object *obj, > Visitor *v, > { > uint32_t *array = (uint32_t *)opaque; > FeatureWord w; > - X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; > - X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; > + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS_NUM_CPUID] = { }; > + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS_NUM_CPUID] = { }; > X86CPUFeatureWordInfoList *list = NULL; > > - for (w = 0; w < FEATURE_WORDS; w++) { > + for (w = 0; w < FEATURE_WORDS_NUM_CPUID; w++) { > FeatureWordInfo *wi = &feature_word_info[w]; > 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; Looks OK, but I would add an assert(wi->type == CPUID_FEATURE_WORD) on every block of code that uses the wi->cpuid field. I would also get rid of FEATURE_WORDS_NUM_CPUID and FEATURE_WORDS_FIRST_MSR to reduce the chance of future mistakes. We can use FEATURE_WORDS in this loop and just check wi->type on each iteration. > qwi->features = array[w]; > > /* List will be in reverse order, but order shouldn't matter */ > @@ -3659,12 +3689,20 @@ 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; I'm not sure this is correct in the case of IA32_ARCH_CAPABILITIES.RSBA: we do want to be able to set RSBA even if the host doesn't have it set. Probably you need to handle IA32_ARCH_CAPABILITIES.RSBA as a special case inside kvm_arch_get_supported_msr_feature(). (And we do want to warn the user in some way if RSBA is set in the host but not in the guest. But that's a separate problem.) > + } > } else if (hvf_enabled()) { > r = hvf_get_supported_cpuid(wi->cpuid_eax, > wi->cpuid_ecx, > @@ -4732,9 +4770,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 > > -- Eduardo