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 <pbonz...@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
