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

[Paolo Bonzini]

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
>>
>>
>>
>