Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[]

[Eduardo Habkost]

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 <address@hidden>
> ---
>  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