Eduardo Habkost <ehabkost@redhat.com> writes:
On Tue, Dec 13, 2016 at 11:16:01AM +0100, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
Implement query-cpu-model-expansion for target-i386.
The code needs to be careful to handle non-migration-safe features ("pmu" and "host-cache-info") according to the expansion type.
Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- tests/Makefile.include | 3 + monitor.c | 4 +- target-i386/cpu.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 200 insertions(+), 2 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include index 63c4347..c7bbfca 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -251,6 +251,9 @@ check-qtest-x86_64-y += $(check-qtest-i386-y) gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
+check-simpleqtest-x86_64-y += $(SRC_PATH)/tests/query-cpu-model-test.py +check-simpleqtest-i386-y += $(SRC_PATH)/tests/query-cpu-model-test.py + check-qtest-alpha-y = tests/boot-serial-test$(EXESUF)
check-qtest-mips-y = tests/endianness-test$(EXESUF) diff --git a/monitor.c b/monitor.c index 0841d43..90c12b3 100644 --- a/monitor.c +++ b/monitor.c @@ -983,8 +983,10 @@ static void qmp_unregister_commands_hack(void) #ifndef TARGET_ARM qmp_unregister_command("query-gic-capabilities"); #endif -#if !defined(TARGET_S390X) +#if !defined(TARGET_S390X) && !defined(TARGET_I386) qmp_unregister_command("query-cpu-model-expansion"); +#endif +#if !defined(TARGET_S390X) qmp_unregister_command("query-cpu-model-baseline"); qmp_unregister_command("query-cpu-model-comparison"); #endif diff --git a/target-i386/cpu.c b/target-i386/cpu.c index bf4ac09..198014a 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -29,10 +29,14 @@ #include "qemu/option.h" #include "qemu/config-file.h" #include "qapi/qmp/qerror.h" +#include "qapi/qmp/qstring.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qmp/qbool.h"
#include "qapi-types.h" #include "qapi-visit.h" #include "qapi/visitor.h" +#include "qom/qom-qobject.h" #include "sysemu/arch_init.h"
#if defined(CONFIG_KVM) @@ -2259,7 +2263,7 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props) } }
-/* Load data from X86CPUDefinition +/* Load data from X86CPUDefinition into a X86CPU object */ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) { @@ -2268,6 +2272,11 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) char host_vendor[CPUID_VENDOR_SZ + 1]; FeatureWord w;
+ /*NOTE: any property set by this function should be returned by
Space between /* and comment text, please.
Also, consider adding wings to both ends of multi-line comments.
Will do.
+ * x86_cpu_to_dict(), so CPU model data returned by + * query-cpu-model-expansion is always complete. + */ + /* CPU models only set _minimum_ values for level/xlevel: */ object_property_set_int(OBJECT(cpu), def->level, "min-level", errp); object_property_set_int(OBJECT(cpu), def->xlevel, "min-xlevel", errp); @@ -2312,6 +2321,190 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
}
+/* Convert CPU model data from X86CPU object to a property dictionary + * that can recreate exactly the same CPU model. + * + * This function does the opposite of x86_cpu_load_def(). Any + * property changed by x86_cpu_load_def() or instance_init + * methods should be returned by this function too. + */ +static void x86_cpu_to_dict(X86CPU *cpu, QDict *props, Error **errp) +{ + Object *obj = OBJECT(cpu); + FeatureWord w; + PropValue *pv; + + /* This code could simply iterate over all writeable properties in the + * CPU object, and return all of them. But then the aliases properties
"alias properties"?
Will fix it.
+ * would be returned as well. Returning only the known features + * is more reliable. + */ + qdict_put_obj(props, "min-level", + object_property_get_qobject(obj, "min-level", errp)); + qdict_put_obj(props, "min-xlevel", + object_property_get_qobject(obj, "min-xlevel", errp)); + + qdict_put_obj(props, "family", + object_property_get_qobject(obj, "family", errp)); + qdict_put_obj(props, "model", + object_property_get_qobject(obj, "model", errp)); + qdict_put_obj(props, "stepping", + object_property_get_qobject(obj, "stepping", errp)); + qdict_put_obj(props, "model-id", + object_property_get_qobject(obj, "model-id", errp));
Incorrect use of the error API: if more than one call fails, the second failure will trip the assertion in error_setv().
If errors should not happen here, use &error_abort.
If errors need to be handled, see "Receive and accumulate multiple errors" in error.h.
Oops. I will fix it.
Consider "more than four, use a for":
static char *prop_name[] = { "min-level", "min-xlevel", "family", "model", "stepping", "model-id", NULL };
for (pname = prop_name; *pname, pname++) { qdict_put_obj(props, *pname, object_property_get_qobject(obj, *pname, &error_abort)); }
Good idea, I will do it.
+ + for (w = 0; w < FEATURE_WORDS; w++) { + FeatureWordInfo *fi = &feature_word_info[w]; + int bit; + for (bit = 0; bit < 32; bit++) { + if (!fi->feat_names[bit]) { + continue; + } + qdict_put_obj(props, fi->feat_names[bit], + object_property_get_qobject(obj, fi->feat_names[bit], + errp)); + } + } + + for (pv = kvm_default_props; pv->prop; pv++) { + qdict_put_obj(props, pv->prop, + object_property_get_qobject(obj, pv->prop, errp)); + } + for (pv = tcg_default_props; pv->prop; pv++) { + qdict_put_obj(props, pv->prop, + object_property_get_qobject(obj, pv->prop, errp)); + } + + qdict_put_obj(props, "vendor", + object_property_get_qobject(obj, "vendor", errp)); + + /* Set by "host": */ + qdict_put_obj(props, "lmce", + object_property_get_qobject(obj, "lmce", errp)); + qdict_put_obj(props, "pmu", + object_property_get_qobject(obj, "pmu", errp)); + + + /* Other properties configurable by the user: */ + qdict_put_obj(props, "host-cache-info", + object_property_get_qobject(obj, "host-cache-info", errp)); +} + +static void object_apply_props(Object *obj, QDict *props, Error **errp) +{ + const QDictEntry *prop; + Error *err = NULL; + + for (prop = qdict_first(props); prop; prop = qdict_next(props, prop)) { + object_property_set_qobject(obj, qdict_entry_value(prop), + qdict_entry_key(prop), &err); + if (err) { + break; + } + } + + error_propagate(errp, err); +} + +static X86CPU *x86_cpu_from_model(CpuModelInfo *model, Error **errp) +{ + X86CPU *xc = NULL; + X86CPUClass *xcc; + Error *err = NULL; + + xcc = X86_CPU_CLASS(cpu_class_by_name(TYPE_X86_CPU, model->name)); + if (xcc == NULL) { + error_setg(&err, "CPU model '%s' not found", model->name); + goto out; + } + + xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc)))); + if (model->has_props) { + QDict *d = qobject_to_qdict(model->props); + if (!d) { + error_setg(&err, "model.props must be a dictionary"); + goto out;
How can this happen?
'props' is 'any' in the QAPI schema, because (it looks like) QAPI doesn't have a 'dict' type.
I see.
+ } + object_apply_props(OBJECT(xc), d, &err); + if (err) { + goto out; + } + } + + x86_cpu_expand_features(xc, &err); + if (err) { + goto out; + } + +out: + if (err) { + error_propagate(errp, err); + object_unref(OBJECT(xc)); + xc = NULL; + } + return xc; +} + +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType type, + CpuModelInfo *model, + Error **errp) +{ + X86CPU *xc = NULL; + Error *err = NULL; + CpuModelExpansionInfo *ret = g_new0(CpuModelExpansionInfo, 1); + QDict *props; + + xc = x86_cpu_from_model(model, &err); + if (err) { + goto out; + } + + /* We currently always do full expansion */
This comment made me go "wait, we do full expansion even when @type is CPU_MODEL_EXPANSION_TYPE_STATIC?" Looks like we first to full expansion, then correct the result according to type.
The comment is a leftover from a previous version where we didn't even check expansion type. I will remove it (or clarify it).
+ ret->model = g_new0(CpuModelInfo, 1); + ret->model->name = g_strdup("base"); /* the only static model */ + props = qdict_new(); + ret->model->props = QOBJECT(props); + ret->model->has_props = true; + x86_cpu_to_dict(xc, props, &err); + + /* Some features (pmu, host-cache-info) are not migration-safe, + * and are handled differently depending on expansion type: + */ + if (type == CPU_MODEL_EXPANSION_TYPE_STATIC) {
Single space after ==, please.
Will fix.
+ /* static expansion force migration-unsafe features off: */ + ret->q_static = ret->migration_safe = true; + qdict_del(props, "pmu"); + qdict_del(props, "host-cache-info"); + } else if (type == CPU_MODEL_EXPANSION_TYPE_FULL) { + QObject *o; + /* full expansion clear the static/migration-safe flags + * to indicate migration-unsafe features are on: + */ + ret->q_static = true; + ret->migration_safe = true; + + o = qdict_get(props, "pmu"); + if (o && qbool_get_bool(qobject_to_qbool(o))) { + ret->q_static = ret->migration_safe = false; + } + o = qdict_get(props, "host-cache-info"); + if (o && qbool_get_bool(qobject_to_qbool(o))) { + ret->q_static = ret->migration_safe = false; + } + } else { + error_setg(&err, "The requested expansion type is not supported.");
How can this happen?
If it can, it bombs when x86_cpu_to_dict() already set an error (see "use of the error API" above).
This can happen if we change the QAPI schema to support another expansion type in the future.
I'd make this an assertion, because it's a programming error.
+ } + +out: + object_unref(OBJECT(xc)); + if (err) { + error_propagate(errp, err); + qapi_free_CpuModelExpansionInfo(ret); + ret = NULL; + } + return ret; +} + X86CPU *cpu_x86_init(const char *cpu_model) { return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));