| Message ID | 20191108110714.7475-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/cpumodel: Introduce "best" model variants | expand |
On Fri, Nov 08, 2019 at 12:07:14PM +0100, David Hildenbrand wrote: > For a specific CPU model, we have a lot of feature variability depending on > - The microcode version of the HW > - The hypervisor we're running on (LPAR vs. KVM vs. z/VM) > - The hypervisor version we're running on > - The KVM version > - KVM module parameters (especially, "nested=1") > - The accelerator > > Our default models are migration safe, however can only be changed > between QEMU releases (glued to QEMU machine). This somewhat collides > with the feature variability we have. E.g., the z13 model will not run > under TCG. There is the demand from higher levels in the stack to "have the > best CPU model possible on a given accelerator, firmware and HW", which > should especially include all features that fix security issues. > Especially, if we have a new feature due to a security flaw, we want to > have a way to backport this feature to older QEMU versions and a way to > automatically enable it when asked. > > This is where "best" CPU models come into play. If upper layers specify > "z14-best" on a z14, they will get the best possible feature set in that > configuration. "best" usually means "maximum features", besides deprecated > features. This will then, for example, include nested virtualization > ("SIE" feature) when KVM+HW support is enabled, or fixes via > microcode updates (e.g., spectre) > > "best" models are not migration safe. Upper layers can expand these > models to migration-safe and static variants, allowing them to be > migrated. > > Signed-off-by: David Hildenbrand <david@redhat.com> Makes sense to me, and the code looks good. I just have one question below: > --- [...] > +static void s390_best_cpu_model_initfn(Object *obj) > +{ > + const S390CPUModel *max_model; > + S390CPU *cpu = S390_CPU(obj); > + S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu); > + Error *local_err = NULL; > + int i; > + > + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { > + return; > + } > + > + max_model = get_max_cpu_model(&local_err); > + if (local_err) { > + /* we expect errors only under KVM, when actually querying the kernel */ > + g_assert(kvm_enabled()); > + error_report_err(local_err); > + return; > + } > + > + /* > + * Similar to baselining against the "max" model. However, features > + * are handled differently and are not used for the search for a definition. > + */ > + if (xcc->cpu_def->gen == max_model->def->gen) { > + if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) { > + return; > + } > + } else if (xcc->cpu_def->gen > max_model->def->gen) { > + return; > + } What exactly is expected to happen if we return from the function here? (In x86, we worked around the inability to report errors inside instance_init by adding another step to CPU object initialization called "CPU expansion", implemented by x86_cpu_expand_features().) > + > + /* The model is theoretically runnable, construct the features. */ > + cpu->model = g_new(S390CPUModel, 1); > + cpu->model->def = xcc->cpu_def; > + bitmap_copy(cpu->model->features, xcc->cpu_def->full_feat, S390_FEAT_MAX); > + > + /* Mask of features that are not available in the "max" model */ > + bitmap_and(cpu->model->features, cpu->model->features, max_model->features, > + S390_FEAT_MAX); > + > + /* Mask off deprecated features */ > + clear_bit(S390_FEAT_CONDITIONAL_SSKE, cpu->model->features); > + > + /* Make sure every model passes consistency checks */ > + for (i = 0; i < ARRAY_SIZE(cpu_feature_dependencies); i++) { > + if (!test_bit(cpu_feature_dependencies[i][1], cpu->model->features)) { > + clear_bit(cpu_feature_dependencies[i][0], cpu->model->features); > + } > + } > +} [...]
On 08.11.19 20:51, Eduardo Habkost wrote: > On Fri, Nov 08, 2019 at 12:07:14PM +0100, David Hildenbrand wrote: >> For a specific CPU model, we have a lot of feature variability depending on >> - The microcode version of the HW >> - The hypervisor we're running on (LPAR vs. KVM vs. z/VM) >> - The hypervisor version we're running on >> - The KVM version >> - KVM module parameters (especially, "nested=1") >> - The accelerator >> >> Our default models are migration safe, however can only be changed >> between QEMU releases (glued to QEMU machine). This somewhat collides >> with the feature variability we have. E.g., the z13 model will not run >> under TCG. There is the demand from higher levels in the stack to "have the >> best CPU model possible on a given accelerator, firmware and HW", which >> should especially include all features that fix security issues. >> Especially, if we have a new feature due to a security flaw, we want to >> have a way to backport this feature to older QEMU versions and a way to >> automatically enable it when asked. >> >> This is where "best" CPU models come into play. If upper layers specify >> "z14-best" on a z14, they will get the best possible feature set in that >> configuration. "best" usually means "maximum features", besides deprecated >> features. This will then, for example, include nested virtualization >> ("SIE" feature) when KVM+HW support is enabled, or fixes via >> microcode updates (e.g., spectre) >> >> "best" models are not migration safe. Upper layers can expand these >> models to migration-safe and static variants, allowing them to be >> migrated. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Makes sense to me, and the code looks good. I just have one > question below: > >> --- > [...] >> +static void s390_best_cpu_model_initfn(Object *obj) >> +{ >> + const S390CPUModel *max_model; >> + S390CPU *cpu = S390_CPU(obj); >> + S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu); >> + Error *local_err = NULL; >> + int i; >> + >> + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { >> + return; >> + } >> + >> + max_model = get_max_cpu_model(&local_err); >> + if (local_err) { >> + /* we expect errors only under KVM, when actually querying the kernel */ >> + g_assert(kvm_enabled()); >> + error_report_err(local_err); >> + return; >> + } >> + >> + /* >> + * Similar to baselining against the "max" model. However, features >> + * are handled differently and are not used for the search for a definition. >> + */ >> + if (xcc->cpu_def->gen == max_model->def->gen) { >> + if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) { >> + return; >> + } >> + } else if (xcc->cpu_def->gen > max_model->def->gen) { >> + return; >> + } > > What exactly is expected to happen if we return from the function > here? cpu->model is NULL. That fact (and xcc->is_best) is checked when the model is to be used (e.g., via qmp or when creating VCPUs), and a rather generic error is reported. This is suboptimal and ... > > (In x86, we worked around the inability to report errors inside > instance_init by adding another step to CPU object initialization > called "CPU expansion", implemented by > x86_cpu_expand_features().) ... doing something like that makes a lot of sense. We also have to rework this for the "host" and "max" model. I'll look into that when I'm back from holidays in one week. Thanks!
diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h index b809ec8418..73901d1410 100644 --- a/target/s390x/cpu-qom.h +++ b/target/s390x/cpu-qom.h @@ -52,6 +52,7 @@ typedef struct S390CPUClass { bool kvm_required; bool is_static; bool is_migration_safe; + bool is_best; const char *desc; DeviceRealize parent_realize; diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 57c06e5ea1..a379b4c15d 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -557,11 +557,16 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info, obj = object_new(object_class_get_name(oc)); cpu = S390_CPU(obj); - if (!cpu->model) { + if (!cpu->model && !S390_CPU_CLASS(oc)->is_best) { error_setg(errp, "Details about the host CPU model are not available, " "it cannot be used."); object_unref(obj); return; + } else if (!cpu->model) { + error_setg(errp, "There is not best CPU model that is runnable," + "therefore, it cannot be used."); + object_unref(obj); + return; } if (qdict) { @@ -932,7 +937,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp) return; } - if (!cpu->model) { + if (!cpu->model && !xcc->is_best) { /* no host model support -> perform compatibility stuff */ apply_cpu_model(NULL, errp); return; @@ -944,6 +949,11 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp) return; } + if (xcc->is_best && !cpu->model) { + error_setg(errp, "Selected CPU model is too new."); + return; + } + /* copy over properties that can vary */ cpu->model->lowest_ibc = max_model->lowest_ibc; cpu->model->cpu_id = max_model->cpu_id; @@ -1156,6 +1166,58 @@ static void s390_qemu_cpu_model_initfn(Object *obj) memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model)); } +static void s390_best_cpu_model_initfn(Object *obj) +{ + const S390CPUModel *max_model; + S390CPU *cpu = S390_CPU(obj); + S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu); + Error *local_err = NULL; + int i; + + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { + return; + } + + max_model = get_max_cpu_model(&local_err); + if (local_err) { + /* we expect errors only under KVM, when actually querying the kernel */ + g_assert(kvm_enabled()); + error_report_err(local_err); + return; + } + + /* + * Similar to baselining against the "max" model. However, features + * are handled differently and are not used for the search for a definition. + */ + if (xcc->cpu_def->gen == max_model->def->gen) { + if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) { + return; + } + } else if (xcc->cpu_def->gen > max_model->def->gen) { + return; + } + + /* The model is theoretically runnable, construct the features. */ + cpu->model = g_new(S390CPUModel, 1); + cpu->model->def = xcc->cpu_def; + bitmap_copy(cpu->model->features, xcc->cpu_def->full_feat, S390_FEAT_MAX); + + /* Mask of features that are not available in the "max" model */ + bitmap_and(cpu->model->features, cpu->model->features, max_model->features, + S390_FEAT_MAX); + + /* Mask off deprecated features */ + clear_bit(S390_FEAT_CONDITIONAL_SSKE, cpu->model->features); + + /* Make sure every model passes consistency checks */ + for (i = 0; i < ARRAY_SIZE(cpu_feature_dependencies); i++) { + if (!test_bit(cpu_feature_dependencies[i][1], cpu->model->features)) { + clear_bit(cpu_feature_dependencies[i][0], cpu->model->features); + } + } +} + static void s390_max_cpu_model_initfn(Object *obj) { const S390CPUModel *max_model; @@ -1235,6 +1297,20 @@ static void s390_base_cpu_model_class_init(ObjectClass *oc, void *data) xcc->desc = xcc->cpu_def->desc; } +static void s390_best_cpu_model_class_init(ObjectClass *oc, void *data) +{ + S390CPUClass *xcc = S390_CPU_CLASS(oc); + + /* + * The "best" models are neither static nor migration safe, similar to + * the "max" model. + */ + xcc->is_best = true; + xcc->cpu_def = (const S390CPUDef *) data; + xcc->desc = g_strdup_printf("%s with best features supported by the accelerator in the current host", + xcc->cpu_def->desc); +} + static void s390_cpu_model_class_init(ObjectClass *oc, void *data) { S390CPUClass *xcc = S390_CPU_CLASS(oc); @@ -1280,6 +1356,12 @@ static char *s390_base_cpu_type_name(const char *model_name) return g_strdup_printf(S390_CPU_TYPE_NAME("%s-base"), model_name); } +/* Generate type name for a best cpu model. Caller has to free the string. */ +static char *s390_best_cpu_type_name(const char *model_name) +{ + return g_strdup_printf(S390_CPU_TYPE_NAME("%s-best"), model_name); +} + ObjectClass *s390_cpu_class_by_name(const char *name) { char *typename = s390_cpu_type_name(name); @@ -1380,9 +1462,19 @@ static void register_types(void) .class_init = s390_cpu_model_class_init, .class_data = (void *) &s390_cpu_defs[i], }; + char *best_name = s390_best_cpu_type_name(s390_cpu_defs[i].name); + TypeInfo ti_best = { + .name = best_name, + .parent = TYPE_S390_CPU, + .instance_init = s390_best_cpu_model_initfn, + .instance_finalize = s390_cpu_model_finalize, + .class_init = s390_best_cpu_model_class_init, + .class_data = (void *) &s390_cpu_defs[i], + }; type_register_static(&ti_base); type_register_static(&ti); + type_register_static(&ti_best); g_free(base_name); g_free(name); }
For a specific CPU model, we have a lot of feature variability depending on - The microcode version of the HW - The hypervisor we're running on (LPAR vs. KVM vs. z/VM) - The hypervisor version we're running on - The KVM version - KVM module parameters (especially, "nested=1") - The accelerator Our default models are migration safe, however can only be changed between QEMU releases (glued to QEMU machine). This somewhat collides with the feature variability we have. E.g., the z13 model will not run under TCG. There is the demand from higher levels in the stack to "have the best CPU model possible on a given accelerator, firmware and HW", which should especially include all features that fix security issues. Especially, if we have a new feature due to a security flaw, we want to have a way to backport this feature to older QEMU versions and a way to automatically enable it when asked. This is where "best" CPU models come into play. If upper layers specify "z14-best" on a z14, they will get the best possible feature set in that configuration. "best" usually means "maximum features", besides deprecated features. This will then, for example, include nested virtualization ("SIE" feature) when KVM+HW support is enabled, or fixes via microcode updates (e.g., spectre) "best" models are not migration safe. Upper layers can expand these models to migration-safe and static variants, allowing them to be migrated. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/cpu-qom.h | 1 + target/s390x/cpu_models.c | 96 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 2 deletions(-)