On Sat, Jul 06, 2019 at 12:07:50AM +0200, Paolo Bonzini wrote:
> On 05/07/19 23:41, Eduardo Habkost wrote:
> >>>> + 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.
>
> Ok, I can drop that check.
>
> >> 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.
>
> I think you're right, but on the other hand setting user_features is
> cleaner. Effectively the dependent features have been disabled because
> of something the user told QEMU. So on one hand I can move the loop to
> x86_cpu_filter_features, on the other hand I'd prefer to set
> user_features and then it feels more like expansion (e.g. of vmx-ept=off
> to vmx-ept=off,vmx-unrestricted-guest=off) than filtering.
I don't disagree if you really want to set user_features for
consistency. Considering it part of expansion and not filtering
makes sense, too.
The only point I disagree with is the handling feature dependency
before the "if (cpu->max_features)" block. e.g. if a feature is
being disabled by x86_cpu_get_supported_feature_word(), we also
need to disable features that depend on it.
--
Eduardo
