[libvirt] [PATCH for-2.9 00/17] target-i386: Implement query-cpu-model-expansion
This series implements query-cpu-model-expansion on target-i386. QAPI / interface changes ------------------------ When implementing this, I have noticed that the "host" CPU model in i386 includes some migration-unsafe features that can't be translated to any migration-safe representation: "pmu", and "host-cache-info". To be able to handle the migration-unsafe features, I have extended the query-cpu-model-expansion definition to be clear about what happens to those features when the CPU model is expanded (in short: static expansion removes them, full expansion keeps them). I also added "static" and "migration-safe" fields to the return value of query-cpu-model-expansion, so callers can know if the the expanded representation is static and migration-safe. Test code --------- I have added a Python test script for the feature, that will try multiple combinations of the expansion operation, and see if the returned data keeps matches some constratins. The test script works with the s390x query-cpu-model-expansion command, except that: 1) I couldn't test it with KVM; 2) qtest.py error handling when QEMU refuses to run is unreliable (so the script needs runnability information to be availble in TCG mode, too, to skip not-runnable CPU models and avoid problems). Future versions of the test script could run a arch-specific CPUID-dump guest binary, and validate data seen by the guest directly. While we don't do that, the script validates all QOM properties on the CPU objects looking for unexpected changes. At least in the case of x86, the QOM properties will include lots of the CPUID data seen by the guest, giving us decent coverage. Patches from other series ------------------------- This series includes patches from other series, just to help on the implementation of the test code: * "qmp: Report QOM type name on query-cpu-definitions", that was already submitted for 2.9 * qemu.py, qtest.py, and tests/Makefile.include changes --- Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: David Hildenbrand <david@redhat.com> Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> Cc: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Eric Blake <eblake@redhat.com> Eduardo Habkost (17): qmp: Report QOM type name on query-cpu-definitions qemu.py: Make logging optional qtest.py: Support QTEST_LOG environment variable qtest.py: make logging optional qtest.py: Make 'binary' parameter optional tests: Add rules to non-gtester qtest test cases target-i386: Reorganize and document CPUID initialization steps target-i386: Support "-cpu host" on TCG too target-i386: Move "host" properties to base class target-i386: Allow short strings to be used as vendor ID target-i386: Remove AMD feature flag aliases from Opteron models target-i386: Return migration-safe field on query-cpu-definitions cpu: Support comma escaping when parsing -cpu qapi: add static/migration-safe info to query-cpu-model-expansion target-i386: Define static "base" CPU model tests: query-cpu-model-test.py test code target-i386: Implement query-cpu-model-expansion QMP command qapi-schema.json | 30 ++- scripts/qemu.py | 25 ++- scripts/qtest.py | 15 +- target-i386/cpu-qom.h | 8 +- tests/Makefile.include | 40 +++- tests/query-cpu-model-test.py | 421 ++++++++++++++++++++++++++++++++++++ tests/test-x86-cpuid-compat.c | 19 ++ monitor.c | 4 +- qom/cpu.c | 32 ++- target-arm/helper.c | 1 + target-i386/cpu.c | 481 +++++++++++++++++++++++++++++++----------- target-ppc/translate_init.c | 1 + target-s390x/cpu_models.c | 5 + 13 files changed, 928 insertions(+), 154 deletions(-) create mode 100755 tests/query-cpu-model-test.py -- 2.7.4
On x86, "-cpu host" enables some features that can't be represented by a static CPU model definition: cache info passthrough ("host-cache-info") and PMU passthrough ("pmu"). This means a type=static expansion of "host" can't include those features. A type=full expansion of "host", on the other hand, can include those features, but then the returned data won't be a static CPU model representation. Add a note to the documentation explaining that when using CPU models that include non-migration-safe features, users need to choose being precision and safety: a precise expansion of the CPU model (full) won't be safe (static), (because they would include pmu=on and host-cache-info=on), and a safe (static) expansion of the CPU model won't be precise. Architectures where CPU model expansion is always migration-safe (e.g. s390x) can simply do what they already do, and set 'migration-safe' and 'static' to true. Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: David Hildenbrand <david@redhat.com> Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> Cc: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Eric Blake <eblake@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- qapi-schema.json | 25 ++++++++++++++++++++++++- target-s390x/cpu_models.c | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/qapi-schema.json b/qapi-schema.json index 8d113f8..a102534 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3291,6 +3291,15 @@ # migration-safe, but allows tooling to get an insight and work with # model details. # +# Note: When a non-migration-safe CPU model is expanded in static mode, some +# features enabled by the CPU model may be omitted, because they can't be +# implemented by a static CPU model definition (e.g. cache info passthrough and +# PMU passthrough in x86). If you need an accurate representation of the +# features enabled by a non-migration-safe CPU model, use @full. If you need a +# static representation that will keep ABI compatibility even when changing QEMU +# version or machine-type, use @static (but keep in mind that some features may +# be omitted). +# # Since: 2.8.0 ## { 'enum': 'CpuModelExpansionType', @@ -3304,10 +3313,24 @@ # # @model: the expanded CpuModelInfo. # +# @migration-safe: the expanded CPU model in @model is a migration-safe +# CPU model. See @CpuDefinitionInfo.migration-safe. +# If expansion type was @static, this is always true. +# (since 2.9) +# +# @static: the expanded CPU model in @model is a static CPU model. +# See @CpuDefinitionInfo.static. If expansion type was @static, +# this is always true. +# (since 2.9) +# +# query-cpu-model-expansion with static expansion type should always +# return a static and migration-safe expansion. +# # Since: 2.8.0 ## { 'struct': 'CpuModelExpansionInfo', - 'data': { 'model': 'CpuModelInfo' } } + 'data': { 'model': 'CpuModelInfo', 'static': 'bool', + 'migration-safe': 'bool' } } ## diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c index 5b66d33..f934add 100644 --- a/target-s390x/cpu_models.c +++ b/target-s390x/cpu_models.c @@ -448,6 +448,10 @@ CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType type /* convert it back to a static representation */ expansion_info = g_malloc0(sizeof(*expansion_info)); expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); + + /* We always expand to a static and migration-safe CpuModelInfo */ + expansion_info->q_static = true; + expansion_info->migration_safe = true; cpu_info_from_model(expansion_info->model, &s390_model, delta_changes); return expansion_info; } -- 2.7.4
I'm not familiar with CPU model expansion, but here goes anyway. Eduardo Habkost <ehabkost@redhat.com> writes:
On x86, "-cpu host" enables some features that can't be represented by a static CPU model definition: cache info passthrough ("host-cache-info") and PMU passthrough ("pmu"). This means a type=static expansion of "host" can't include those features.
A type=full expansion of "host", on the other hand, can include those features, but then the returned data won't be a static CPU model representation.
Add a note to the documentation explaining that when using CPU models that include non-migration-safe features, users need to choose being precision and safety: a precise expansion of the CPU
s/being/between/
model (full) won't be safe (static), (because they would include
s/they/it/
pmu=on and host-cache-info=on), and a safe (static) expansion of the CPU model won't be precise.
Architectures where CPU model expansion is always migration-safe (e.g. s390x) can simply do what they already do, and set 'migration-safe' and 'static' to true.
This patch does that exactly for s390x. The next patch looks like it does something for x86. What about other targets? I recommend to have this commit message explain briefly what this patch does, what later patches in this series do, and what's left for later (if anything).
Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: David Hildenbrand <david@redhat.com> Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> Cc: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Eric Blake <eblake@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- qapi-schema.json | 25 ++++++++++++++++++++++++- target-s390x/cpu_models.c | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/qapi-schema.json b/qapi-schema.json index 8d113f8..a102534 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3291,6 +3291,15 @@ # migration-safe, but allows tooling to get an insight and work with # model details. # +# Note: When a non-migration-safe CPU model is expanded in static mode, some +# features enabled by the CPU model may be omitted, because they can't be +# implemented by a static CPU model definition (e.g. cache info passthrough and +# PMU passthrough in x86). If you need an accurate representation of the +# features enabled by a non-migration-safe CPU model, use @full. If you need a +# static representation that will keep ABI compatibility even when changing QEMU +# version or machine-type, use @static (but keep in mind that some features may +# be omitted). +# # Since: 2.8.0 ## { 'enum': 'CpuModelExpansionType', @@ -3304,10 +3313,24 @@ # # @model: the expanded CpuModelInfo. # +# @migration-safe: the expanded CPU model in @model is a migration-safe +# CPU model. See @CpuDefinitionInfo.migration-safe. +# If expansion type was @static, this is always true. +# (since 2.9) +# +# @static: the expanded CPU model in @model is a static CPU model. +# See @CpuDefinitionInfo.static. If expansion type was @static, +# this is always true. +# (since 2.9) +# +# query-cpu-model-expansion with static expansion type should always +# return a static and migration-safe expansion. +# # Since: 2.8.0 ## { 'struct': 'CpuModelExpansionInfo', - 'data': { 'model': 'CpuModelInfo' } } + 'data': { 'model': 'CpuModelInfo', 'static': 'bool', + 'migration-safe': 'bool' } }
## diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c index 5b66d33..f934add 100644 --- a/target-s390x/cpu_models.c +++ b/target-s390x/cpu_models.c @@ -448,6 +448,10 @@ CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType type /* convert it back to a static representation */ expansion_info = g_malloc0(sizeof(*expansion_info)); expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); + + /* We always expand to a static and migration-safe CpuModelInfo */ + expansion_info->q_static = true; + expansion_info->migration_safe = true; cpu_info_from_model(expansion_info->model, &s390_model, delta_changes); return expansion_info; }
On Tue, Dec 13, 2016 at 10:47:38AM +0100, Markus Armbruster wrote:
I'm not familiar with CPU model expansion, but here goes anyway.
Eduardo Habkost <ehabkost@redhat.com> writes:
On x86, "-cpu host" enables some features that can't be represented by a static CPU model definition: cache info passthrough ("host-cache-info") and PMU passthrough ("pmu"). This means a type=static expansion of "host" can't include those features.
A type=full expansion of "host", on the other hand, can include those features, but then the returned data won't be a static CPU model representation.
Add a note to the documentation explaining that when using CPU models that include non-migration-safe features, users need to choose being precision and safety: a precise expansion of the CPU
s/being/between/
model (full) won't be safe (static), (because they would include
s/they/it/
Oops. Thanks!
pmu=on and host-cache-info=on), and a safe (static) expansion of the CPU model won't be precise.
Architectures where CPU model expansion is always migration-safe (e.g. s390x) can simply do what they already do, and set 'migration-safe' and 'static' to true.
This patch does that exactly for s390x. The next patch looks like it does something for x86. What about other targets? I recommend to have this commit message explain briefly what this patch does, what later patches in this series do, and what's left for later (if anything).
s390x is the only architecture implementing query-cpu-model-expansion by now. I will add a comment clarifying that, anyway.
Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: David Hildenbrand <david@redhat.com> Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> Cc: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Eric Blake <eblake@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- qapi-schema.json | 25 ++++++++++++++++++++++++- target-s390x/cpu_models.c | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/qapi-schema.json b/qapi-schema.json index 8d113f8..a102534 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3291,6 +3291,15 @@ # migration-safe, but allows tooling to get an insight and work with # model details. # +# Note: When a non-migration-safe CPU model is expanded in static mode, some +# features enabled by the CPU model may be omitted, because they can't be +# implemented by a static CPU model definition (e.g. cache info passthrough and +# PMU passthrough in x86). If you need an accurate representation of the +# features enabled by a non-migration-safe CPU model, use @full. If you need a +# static representation that will keep ABI compatibility even when changing QEMU +# version or machine-type, use @static (but keep in mind that some features may +# be omitted). +# # Since: 2.8.0 ## { 'enum': 'CpuModelExpansionType', @@ -3304,10 +3313,24 @@ # # @model: the expanded CpuModelInfo. # +# @migration-safe: the expanded CPU model in @model is a migration-safe +# CPU model. See @CpuDefinitionInfo.migration-safe. +# If expansion type was @static, this is always true. +# (since 2.9) +# +# @static: the expanded CPU model in @model is a static CPU model. +# See @CpuDefinitionInfo.static. If expansion type was @static, +# this is always true. +# (since 2.9) +# +# query-cpu-model-expansion with static expansion type should always +# return a static and migration-safe expansion. +# # Since: 2.8.0 ## { 'struct': 'CpuModelExpansionInfo', - 'data': { 'model': 'CpuModelInfo' } } + 'data': { 'model': 'CpuModelInfo', 'static': 'bool', + 'migration-safe': 'bool' } }
## diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c index 5b66d33..f934add 100644 --- a/target-s390x/cpu_models.c +++ b/target-s390x/cpu_models.c @@ -448,6 +448,10 @@ CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType type /* convert it back to a static representation */ expansion_info = g_malloc0(sizeof(*expansion_info)); expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); + + /* We always expand to a static and migration-safe CpuModelInfo */ + expansion_info->q_static = true; + expansion_info->migration_safe = true; cpu_info_from_model(expansion_info->model, &s390_model, delta_changes); return expansion_info; }
-- Eduardo
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 + * 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 + * 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)); + + 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; + } + 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 */ + 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) { + /* 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."); + } + +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)); -- 2.7.4
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.
+ * 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"?
+ * 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. 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)); }
+ + 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?
+ } + 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.
+ 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.
+ /* 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).
+ } + +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));
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.
+ } + 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.
+ } + +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));
-- Eduardo
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));
On Tue, Dec 13, 2016 at 08:20:39PM +0100, Markus Armbruster wrote: [...]
+ if (type == CPU_MODEL_EXPANSION_TYPE_STATIC) { + /* 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.
I don't think it's a programming error. For example, if one day the ppc maintainers decide they need a new expansion type for some arch-specific requirement they have, they won't need to touch the x86 and s390x code when changing the schema. -- Eduardo
Am 02.12.2016 um 22:17 schrieb Eduardo Habkost:
This series implements query-cpu-model-expansion on target-i386.
QAPI / interface changes ------------------------
When implementing this, I have noticed that the "host" CPU model in i386 includes some migration-unsafe features that can't be translated to any migration-safe representation: "pmu", and "host-cache-info".
To be able to handle the migration-unsafe features, I have extended the query-cpu-model-expansion definition to be clear about what happens to those features when the CPU model is expanded (in short: static expansion removes them, full expansion keeps them).
I think this makes sense. The important part is to really keep static expansion static :)
I also added "static" and "migration-safe" fields to the return value of query-cpu-model-expansion, so callers can know if the the expanded representation is static and migration-safe.
Is static really needed? I can understand why migration-safe might be of interest, but can't see how "static" could help (I mean we have static expansion for this purpose). Do you have anything special in mind regarding exposing "static"?
Test code ---------
I have added a Python test script for the feature, that will try multiple combinations of the expansion operation, and see if the returned data keeps matches some constratins.
The test script works with the s390x query-cpu-model-expansion command, except that: 1) I couldn't test it with KVM; 2) qtest.py error handling when QEMU refuses to run is unreliable (so the script needs runnability information to be availble in TCG mode, too, to skip not-runnable CPU models and avoid problems).
Everything except "host" should behave completely the same on s390x with or without KVM being active. So with !KVM tests we can already cover most of the interesting parts. Thanks for taking care of s390x.
Future versions of the test script could run a arch-specific CPUID-dump guest binary, and validate data seen by the guest directly. While we don't do that, the script validates all QOM properties on the CPU objects looking for unexpected changes. At least in the case of x86, the QOM properties will include lots of the CPUID data seen by the guest, giving us decent coverage.
Something like that would be cool. Unfortunately, e.g. on s390x some CPUID-like data (e.g. STFL(E) and SCP info) is only available from kernel space. So we can't simply run a user space script. However, something like a kernel module would be possible (or even something like the s390x pc-bios to simply read and dump the data). -- David
On Mon, Dec 05, 2016 at 04:15:38PM +0100, David Hildenbrand wrote:
Am 02.12.2016 um 22:17 schrieb Eduardo Habkost:
This series implements query-cpu-model-expansion on target-i386.
QAPI / interface changes ------------------------
When implementing this, I have noticed that the "host" CPU model in i386 includes some migration-unsafe features that can't be translated to any migration-safe representation: "pmu", and "host-cache-info".
To be able to handle the migration-unsafe features, I have extended the query-cpu-model-expansion definition to be clear about what happens to those features when the CPU model is expanded (in short: static expansion removes them, full expansion keeps them).
I think this makes sense. The important part is to really keep static expansion static :)
I also added "static" and "migration-safe" fields to the return value of query-cpu-model-expansion, so callers can know if the the expanded representation is static and migration-safe.
Is static really needed? I can understand why migration-safe might be of interest, but can't see how "static" could help (I mean we have static expansion for this purpose). Do you have anything special in mind regarding exposing "static"?
I didn't have any specific use case in mind. My main worry is to avoid letting management software make any assumptions about the returned data. If we don't add an explicit "static" field, a client might make some wrong assumptions just because some conditions are known to be always true on existing implementations. e.g.: a client could incorrectly assume that "query-cpu-model-expansion type=full" of a migration-safe model would always be static.
Test code ---------
I have added a Python test script for the feature, that will try multiple combinations of the expansion operation, and see if the returned data keeps matches some constratins.
The test script works with the s390x query-cpu-model-expansion command, except that: 1) I couldn't test it with KVM; 2) qtest.py error handling when QEMU refuses to run is unreliable (so the script needs runnability information to be availble in TCG mode, too, to skip not-runnable CPU models and avoid problems).
Everything except "host" should behave completely the same on s390x with or without KVM being active. So with !KVM tests we can already cover most of the interesting parts. Thanks for taking care of s390x.
Future versions of the test script could run a arch-specific CPUID-dump guest binary, and validate data seen by the guest directly. While we don't do that, the script validates all QOM properties on the CPU objects looking for unexpected changes. At least in the case of x86, the QOM properties will include lots of the CPUID data seen by the guest, giving us decent coverage.
Something like that would be cool. Unfortunately, e.g. on s390x some CPUID-like data (e.g. STFL(E) and SCP info) is only available from kernel space. So we can't simply run a user space script. However, something like a kernel module would be possible (or even something like the s390x pc-bios to simply read and dump the data).
I meant a kernel binary, above. On x86, there are existing test cases on the autotest repository that run a very small kernel[1] that simply dumps CPUID data to the serial console. We could reuse it, or we could add a CPUID command to the qtest protocol. I'm not sure what's the best solution. We can try to use a similar approach on s390x to reuse code in the test script, but we can add arch-specific code to it if necessary. [1] https://github.com/autotest/tp-qemu/tree/master/qemu/deps/cpuid/src -- Eduardo
Is static really needed? I can understand why migration-safe might be of interest, but can't see how "static" could help (I mean we have static expansion for this purpose). Do you have anything special in mind regarding exposing "static"?
I didn't have any specific use case in mind. My main worry is to avoid letting management software make any assumptions about the returned data. If we don't add an explicit "static" field, a client might make some wrong assumptions just because some conditions are known to be always true on existing implementations. e.g.: a client could incorrectly assume that "query-cpu-model-expansion type=full" of a migration-safe model would always be static.
I think "migration-safe" makes sense, as the doc currently states for "@full": "The produced model is not guaranteed to be migration-safe". For static expansion, this information is somewhat superfluous, as "static CPU models are migration-safe", but it doesn't hurt. (and this is also what you properly documented :) ) But I don't think that "static" will be of any use. If you want a static model, go for "static" expansion. (I don't really see any use case here, but if you do, keep it)
Something like that would be cool. Unfortunately, e.g. on s390x some CPUID-like data (e.g. STFL(E) and SCP info) is only available from kernel space. So we can't simply run a user space script. However, something like a kernel module would be possible (or even something like the s390x pc-bios to simply read and dump the data).
I meant a kernel binary, above. On x86, there are existing test cases on the autotest repository that run a very small kernel[1] that simply dumps CPUID data to the serial console. We could reuse it, or we could add a CPUID command to the qtest protocol. I'm not sure what's the best solution.
We can try to use a similar approach on s390x to reuse code in the test script, but we can add arch-specific code to it if necessary.
Ah, alright, so I wasn't aware of that because there is no autotest for s390x (at least not that I know of ;) ). Thanks for the hint.
[1] https://github.com/autotest/tp-qemu/tree/master/qemu/deps/cpuid/src
-- David
On Mon, Dec 05, 2016 at 07:13:36PM +0100, David Hildenbrand wrote:
Is static really needed? I can understand why migration-safe might be of interest, but can't see how "static" could help (I mean we have static expansion for this purpose). Do you have anything special in mind regarding exposing "static"?
I didn't have any specific use case in mind. My main worry is to avoid letting management software make any assumptions about the returned data. If we don't add an explicit "static" field, a client might make some wrong assumptions just because some conditions are known to be always true on existing implementations. e.g.: a client could incorrectly assume that "query-cpu-model-expansion type=full" of a migration-safe model would always be static.
I think "migration-safe" makes sense, as the doc currently states for "@full": "The produced model is not guaranteed to be migration-safe".
For static expansion, this information is somewhat superfluous, as "static CPU models are migration-safe", but it doesn't hurt. (and this is also what you properly documented :) )
But I don't think that "static" will be of any use. If you want a static model, go for "static" expansion. (I don't really see any use case here, but if you do, keep it)
Your point is valid, and the field is probably not strictly necessary. But considering that the interface is not trivial, I think it is a good thing to have extra metadata that can help clients validate their assumptions. Even if it's useful in practice only for debugging, it would be a good reason to add it, IMO. I first considered only updating the documentation to make the guarantees explicit (so people won't assume that type=full expansion is always a superset of static, or that type=static expansion is always precise/ABI-compatible and will never lose any features). But then I thought that returning extra metadata would help people using the interface (and help us validate the implementation on the test script). Anyway, the next version of this series will have the new fields added by a separate patch. This way we can evaluate and discuss the interface changes separately. -- Eduardo
participants (3)
-
David Hildenbrand -
Eduardo Habkost -
Markus Armbruster