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
