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

[Eduardo Habkost]

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