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. In any case, that would basically be rewriting "+foo" and "-foo" to "foo=on" and "foo=off" respectively, right? >> + >> + 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. >> + 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. 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 >> >> >> >