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

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Jul 02, 2019 at 05:01:16PM +0200, Paolo Bonzini wrote:
> Sometimes a CPU feature does not make sense unless another is
> present.  In the case of VMX features, KVM does not even allow
> setting the VMX controls to some invalid combinations.
> 
> Therefore, this patch adds a generic mechanism that looks for bits
> that the user explicitly cleared, and uses them to remove other bits
> from the expanded CPU definition.  If these dependent bits were also
> explicitly *set* by the user, this will be a warning for "-cpu check"
> and an error for "-cpu enforce".  If not, then the dependent bits are
> cleared silently, for convenience.
> 
> With VMX features, this will be used so that for example
> "-cpu host,-rdrand" will also hide support for RDRAND exiting.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 9149d0d..412e834 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -799,10 +799,6 @@ typedef struct FeatureWordInfo {
>          /* If type==MSR_FEATURE_WORD */
>          struct {
>              uint32_t index;
> -            struct {   /*CPUID that enumerate this MSR*/
> -                FeatureWord cpuid_class;
> -                uint32_t    cpuid_flag;
> -            } cpuid_dep;
>          } msr;
>      };
>      uint32_t tcg_features; /* Feature flags supported by TCG */
> @@ -1197,10 +1193,6 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          },
>          .msr = {
>              .index = MSR_IA32_ARCH_CAPABILITIES,
> -            .cpuid_dep = {
> -                FEAT_7_0_EDX,
> -                CPUID_7_0_EDX_ARCH_CAPABILITIES
> -            }
>          },
>      },
>      [FEAT_CORE_CAPABILITY] = {
> @@ -1217,14 +1209,26 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          },
>          .msr = {
>              .index = MSR_IA32_CORE_CAPABILITY,
> -            .cpuid_dep = {
> -                FEAT_7_0_EDX,
> -                CPUID_7_0_EDX_CORE_CAPABILITY,
> -            },
>          },
>      },
>  };
>  
> +typedef struct FeatureDep {
> +    uint16_t from, to;

Why uint16_t and not FeatureWord?

> +    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;
    };
    
    
    typedef struct FeatureDependency {
       /*
        * Features in @to may be present only if _all_ features in @from
        * present too.
        */
       FeatureMask from, to;
    };
    
    static FeatureDep feature_dependencies[] = {
        {
            .from = { FEAT_7_0_EDX, CPUID_7_0_EDX_ARCH_CAPABILITIES
            .to =   { FEAT_ARCH_CAPABILITIES, ~0ull },
        },
        {
            .from = { FEAT_7_0_EDX, CPUID_7_0_EDX_CORE_CAPABILITY },
            .to =   { FEAT_CORE_CAPABILITY, ~0ull },
        },
    };


> +} FeatureDep;
> +
> +static FeatureDep feature_dependencies[] = {
> +    {
> +        .from = FEAT_7_0_EDX,            .from_flag = CPUID_7_0_EDX_ARCH_CAPABILITIES,
> +        .to = FEAT_ARCH_CAPABILITIES,    .to_flags = ~0ull,
> +    },
> +    {
> +        .from = FEAT_7_0_EDX,            .from_flag = CPUID_7_0_EDX_CORE_CAPABILITY,
> +        .to = FEAT_CORE_CAPABILITY,      .to_flags = ~0ull,
> +    },
> +};
> +
>  typedef struct X86RegisterInfo32 {
>      /* Name of register */
>      const char *name;
> @@ -5086,9 +5090,42 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>  {
>      CPUX86State *env = &cpu->env;
>      FeatureWord w;
> +    int i;
>      GList *l;
>      Error *local_err = NULL;
>  
> +    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.

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

> +            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()?

> +
>      /*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
> 
> 
> 

-- 
Eduardo