Re: [Qemu-devel] [PATCH 2/7] target/i386: introduce generic feature dependency mechanism

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Jul 05, 2019 at 11:12:11PM +0200, Paolo Bonzini wrote:
> On 05/07/19 22:52, Eduardo Habkost wrote:
> >> +typedef struct FeatureDep {
> >> +    uint16_t from, to;
> > 
> > Why uint16_t and not FeatureWord?
> 
> Ok.
> 
> >> +    uint64_t from_flag, to_flags;
> > 
> > There are other parts of the code that take a
> > FeatureWord/uint32_t pair (which will become uint64_t).  I'd wrap
> > this into a typedef.  I also miss documentation on the exact
> > meaning of those fields.
> > 
> >     typedef struct FeatureMask {
> >         FeatureWord w;
> >         uint64_t mask;
> >     };
> 
> Sounds good, I was optimizing the layout by putting small fields
> together.  Perhaps prematurely. :)
> 
> >> +    for (l = plus_features; l; l = l->next) {
> >> +        const char *prop = l->data;
> >> +        object_property_set_bool(OBJECT(cpu), true, prop, &local_err);
> >> +        if (local_err) {
> >> +            goto out;
> >> +        }
> >> +    }
> >> +
> >> +    for (l = minus_features; l; l = l->next) {
> >> +        const char *prop = l->data;
> >> +        object_property_set_bool(OBJECT(cpu), false, prop, &local_err);
> >> +        if (local_err) {
> >> +            goto out;
> >> +        }
> >> +    }
> > 
> > Maybe getting rid of plus_features/minus_features (as described
> > in the TODO comment below) will make things simpler.
> 
> This is just moving code.  I can look at getting rid of plus_features
> and minus_features but I was wary of the effects that global properties
> have on query_cpu_model_expansion.

Shouldn't be a problem, as query-cpu-model-expansion
documentation already advises against using "-cpu" when calling
it.


> 
> In any case, that would basically be rewriting "+foo" and "-foo" to
> "foo=on" and "foo=off" respectively, right?

I don't mean changing the command line interface, but just
changing the implementation of "+foo" and "-foo".

In theory the code was already fixed to make this safe, but I
agree this might be tricky.  Let's worry about
plus_features/minus_features later.


> 
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
> >> +        FeatureDep *d = &feature_dependencies[i];
> >> +        if ((env->user_features[d->from] & d->from_flag) &&
> >> +            !(env->features[d->from] & d->from_flag)) {
> > 
> > Why does it matter if the feature was cleared explicitly by the
> > user?
> 
> Because the feature set of named CPU models should be internally
> consistent.  I thought of this mechanism as a quick "clean up user's
> choices" pass to avoid having to remember a multitude of VMX features,
> for example it makes "-cpu host,-rdtscp" just work.

If named CPU models are already consistent, ignoring
user_features shouldn't make a difference, right?  It would also
be a useful mechanism to detect inconsistencies in internal CPU
model definitions.

I don't understand why the user_features check would be necessary
to make "-cpu host,-rdtscp" work.

> 
> >> +            uint64_t unavailable_features = env->features[d->to] & d->to_flags;
> >> +
> >> +            /* Not an error unless the dependent feature was added explicitly.  */
> >> +            mark_unavailable_features(cpu, d->to, unavailable_features & env->user_features[d->to],
> >> +                                      "This feature depends on other features that were not requested");
> >> +
> >> +            /* Prevent adding the feature in the loop below.  */
> >> +            env->user_features[d->to] |= d->to_flags;
> >> +            env->features[d->to] &= ~d->to_flags;
> >> +        }
> >> +    }
> > 
> > Maybe move this entire block inside x86_cpu_filter_features()?
> 
> It has to be done before expansion, so that env->user_features is set
> properly before -cpu host is expanded.

I don't get it.  It looks like you only need env->user_features
to be set above because you are handling dependencies before
cpu->max_features is handled.

If you handle dependencies at x86_cpu_filter_features() instead
(after cpu->max_features was already handled), you don't even
need to worry about setting user_features.

> 
> Paolo
> 
> >> +
> >>      /*TODO: Now cpu->max_features doesn't overwrite features
> >>       * set using QOM properties, and we can convert
> >>       * plus_features & minus_features to global properties
> >> @@ -5106,22 +5143,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> >>          }
> >>      }
> >>  
> >> -    for (l = plus_features; l; l = l->next) {
> >> -        const char *prop = l->data;
> >> -        object_property_set_bool(OBJECT(cpu), true, prop, &local_err);
> >> -        if (local_err) {
> >> -            goto out;
> >> -        }
> >> -    }
> >> -
> >> -    for (l = minus_features; l; l = l->next) {
> >> -        const char *prop = l->data;
> >> -        object_property_set_bool(OBJECT(cpu), false, prop, &local_err);
> >> -        if (local_err) {
> >> -            goto out;
> >> -        }
> >> -    }
> >> -
> >>      if (!kvm_enabled() || !cpu->expose_kvm) {
> >>          env->features[FEAT_KVM] = 0;
> >>      }
> >> -- 
> >> 1.8.3.1
> >>
> >>
> >>
> > 
> 

-- 
Eduardo