Re: [Qemu-devel] [PATCH v2] i386: Allow cpuid bit override

[Michael Roth]

Quoting Michael Roth (2017-04-18 16:01:20)
> Quoting Eduardo Habkost (2017-04-13 12:36:25)
> > On Tue, Mar 28, 2017 at 01:27:06PM +0200, Alexander Graf wrote:
> > > KVM has a feature bitmap of CPUID bits that it knows works for guests.
> > > QEMU removes bits that are not part of that bitmap automatically on VM
> > > start.
> > > 
> > > However, some times we just don't list features in that list because
> > > they don't make sense for normal scenarios, but may be useful in specific,
> > > targeted workloads.
> > > 
> > > For that purpose, add a new =force option to all CPUID feature flags in
> > > the CPU property. With that we can override the accel filtering and give
> > > users full control over the CPUID feature bits exposed into guests.
> > > 
> > > Signed-off-by: Alexander Graf <address@hidden>
> > > 
> > > ---
> > > 
> > > v1 -> v2:
> > > 
> > >   - Base on "i386: Replace uint32_t* with FeatureWord on feature 
> > > getter/setter"
> > > ---
> > >  target/i386/cpu.c | 25 ++++++++++++++++++++++---
> > >  target/i386/cpu.h |  3 +++
> > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 13c0985..6105fc5 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function 
> > > cpu_fprintf)
> > >      g_slist_foreach(list, x86_cpu_list_entry, &s);
> > >      g_slist_free(list);
> > >  
> > > -    (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
> > > +    (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n");
> > >      for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
> > >          FeatureWordInfo *fw = &feature_word_info[i];
> > >  
> > > @@ -3464,6 +3464,7 @@ static int x86_cpu_filter_features(X86CPU *cpu)
> > >              x86_cpu_get_supported_feature_word(w, false);
> > >          uint32_t requested_features = env->features[w];
> > >          env->features[w] &= host_feat;
> > > +        env->features[w] |= cpu->forced_features[w];
> > >          cpu->filtered_features[w] = requested_features & 
> > > ~env->features[w];
> > >          if (cpu->filtered_features[w]) {
> > >              rv = 1;
> > > @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, 
> > > Visitor *v, const char *name,
> > >      X86CPU *cpu = X86_CPU(obj);
> > >      BitProperty *fp = opaque;
> > >      uint32_t f = cpu->env.features[fp->w];
> > > +    uint32_t ff = cpu->forced_features[fp->w];
> > >      bool value = (f & fp->mask) == fp->mask;
> > > -    visit_type_bool(v, name, &value, errp);
> > > +    bool forced = (ff & fp->mask) == fp->mask;
> > > +    char str[] = "force";
> > > +    char *strval = str;
> > > +
> > > +    if (forced) {
> > > +        visit_type_str(v, name, &strval, errp);
> > > +    } else {
> > > +        visit_type_bool(v, name, &value, errp);
> > 
> > Interesting approach. This means we keep returning a boolean
> > value on the property getter (which is important to not break
> > compatibility), but return a string in case the feature was set
> > to "force".
> > 
> > We probably should update the 'type' field of the property, as it
> > won't be a real "bool" property anymore.
> > 
> > I won't say I love that solution, but it seems to work. I'm CCing
> > the QAPI maintainers to see what they think about it.
> > 
> > (For reference: in the v1 thread I have suggested introducing a
> > new enum type in the QAPI schema, but this would break
> > compatibility with existing management code that expects the
> > property to return a boolean value [like the users of
> > query-cpu-model-expansion]).
> > 
> > > +    }
> > >  }
> > >  
> > >  static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char 
> > > *name,
> > > @@ -3717,6 +3727,7 @@ static void x86_cpu_set_bit_prop(Object *obj, 
> > > Visitor *v, const char *name,
> > >      X86CPU *cpu = X86_CPU(obj);
> > >      BitProperty *fp = opaque;
> > >      Error *local_err = NULL;
> > > +    char *strval = NULL;
> > >      bool value;
> > >  
> > >      if (dev->realized) {
> > > @@ -3724,7 +3735,15 @@ static void x86_cpu_set_bit_prop(Object *obj, 
> > > Visitor *v, const char *name,
> > >          return;
> > >      }
> > >  
> > > -    visit_type_bool(v, name, &value, &local_err);
> > > +    visit_type_str(v, name, &strval, &local_err);
> > > +    if (!local_err && !strcmp(strval, "force")) {
> > > +        value = true;
> > > +        cpu->forced_features[fp->w] |= fp->mask;
> > > +    } else {
> > > +        local_err = NULL;
> > > +        visit_type_bool(v, name, &value, &local_err);
> > > +    }
> > 
> > I'm not sure this is valid usage of the visitor API. If the
> > visit_type_str() call succeeds, isn't the visitor allowed to
> > consume the original value, and return something completely
> > different when visit_type_bool() is called?
> 
> That was indeed one of the design goals, to the extent that
> you could even take a stream of data types and actually build
> up a dictionary based on the ordering visited (e.g. the
> once-proposed BER visitor for migration and why QAPI uses
> OrderedDict when generating visitors).
> 
> I'm not sure how important that is anymore, but even setting
> that aside that we still have the issue of not handling the
> data type as declared, and relying on error-handling to probe
> it, which doesn't necessarily leave the visitor in a recoverable
> state where we can continue visiting. I think it might be possible
> to rely on the "alternate" QAPI type to handle this case. Maybe
> something like this?
> 
>     GenericAlternate *alt = NULL;
>     visit_start_alternate(v, name, &alt, sizeof(*alt), false, &local_err);
>     if (local_err) {
>         goto out;
>     }
>     if (!alt) {
>         goto out_obj;
>     }
>     switch (alt->type) {
>     case QTYPE_QBOOL:
>         visit_type_bool(v, name, &value, &local_err);
>         break;
>     case QTYPE_QSTRING:
>         visit_type_str(v, name, &strval, &local_err);
>         if (!local_err && !strcmp(strval, "force")) {
>             value = true;
>             cpu->forced_features[fp->w] |= fp->mask;
>         }
>         break;
>     default:
>         error_setg(&local_err, "invalid type");
>     }
> 
> out_obj:
>     visit_end_alternate(v, (void **)obj);
> out:
>     ...
> 
> This would only work for setters/qobject-input-visitor ATM though since
> qobject-output-visitor doesn't support the visit_start_alternate()
> interface. I'm not sure why it wasn't enabled on the output side, maybe
> Eric knows if doing so would be feasible for this situation?

Forgot to Cc: Eric

> 
> > 
> > > +
> > >      if (local_err) {
> > >          error_propagate(errp, local_err);
> > >          return;
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index c4602ca..69efe6c 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -1230,6 +1230,9 @@ struct X86CPU {
> > >      /* Features that were filtered out because of missing host 
> > > capabilities */
> > >      uint32_t filtered_features[FEATURE_WORDS];
> > >  
> > > +    /* Features that are force enabled despite incompatible accel */
> > > +    uint32_t forced_features[FEATURE_WORDS];
> > > +
> > >      /* Enable PMU CPUID bits. This can't be enabled by default yet 
> > > because
> > >       * it doesn't have ABI stability guarantees, as it passes all PMU 
> > > CPUID
> > >       * bits returned by GET_SUPPORTED_CPUID (that depend on host CPU and 
> > > kernel
> > > -- 
> > > 1.8.5.6
> > > 
> > > 
> > 
> > -- 
> > Eduardo
> >