| Message ID | 1562079681-19204-3-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: support VMX features in "-cpu" | expand |
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 > > >
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 >> >> >> >
On Fri, Jul 05, 2019 at 11:12:11PM +0200, Paolo Bonzini wrote: > 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. Shouldn't be a problem, as query-cpu-model-expansion documentation already advises against using "-cpu" when calling it. > > In any case, that would basically be rewriting "+foo" and "-foo" to > "foo=on" and "foo=off" respectively, right? I don't mean changing the command line interface, but just changing the implementation of "+foo" and "-foo". In theory the code was already fixed to make this safe, but I agree this might be tricky. Let's worry about plus_features/minus_features later. > > >> + > >> + 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. I don't understand why the user_features check would be necessary to make "-cpu host,-rdtscp" 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. 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. > > 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 > >> > >> > >> > > >
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
On Sat, Jul 06, 2019 at 12:07:50AM +0200, Paolo Bonzini wrote: > 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. I don't disagree if you really want to set user_features for consistency. Considering it part of expansion and not filtering makes sense, too. The only point I disagree with is the handling feature dependency before the "if (cpu->max_features)" block. e.g. if a feature is being disabled by x86_cpu_get_supported_feature_word(), we also need to disable features that depend on it.
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; + uint64_t from_flag, to_flags; +} 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; + } + } + + 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)) { + 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; + } + } + /*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; }
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(-)