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

[Paolo Bonzini]

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.

Paolo