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

[Eduardo Habkost <ehabkost@redhat.com>]

On Sat, Aug 18, 2018 at 05:01:45PM +0800, Robert Hoo wrote:
> On Fri, 2018-08-17 at 10:28 -0300, Eduardo Habkost wrote:
> > 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.hu@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.
> > 
> 1 question: I think caller should g_free() the returned str after it
> warn_report(), right?
> > > +{
> > > +    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.)
> 
> The first part is easy to do. the latter, "to warn the user in some way
> if RSBA is set in the host but not in the guest", in
> kvm_arch_get_supported_msr_feature(), how can I know if guest has
> enabled IA32_ARCH_CAPABILITIES.RSBA or not? or you mean check in some
> other place, where is it?

Probably X86CPU realize function is an obvious place, but the
main problem is that we don't have an appropriate channel to
report that warning except stderr (making the warning useless for
users that are not running QEMU directly).

We also need to ensure QEMU is doing its job before it complains
to the user: before printing a warning about unsafe
configuration, we should try to make QEMU enable safe
configuration by default.

-- 
Eduardo