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
