3:17 p.m.
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(a)de.ibm.com>
Cc: Christian Borntraeger <borntraeger(a)de.ibm.com>
Cc: David Hildenbrand <david(a)redhat.com>
Cc: libvir-list(a)redhat.com
Cc: Jiri Denemark <jdenemar(a)redhat.com>
Cc: "Jason J. Herne" <jjherne(a)linux.vnet.ibm.com>
Cc: Markus Armbruster <armbru(a)redhat.com>
Cc: Richard Henderson <rth(a)twiddle.net>
Cc: Igor Mammedov <imammedo(a)redhat.com>
Cc: Eric Blake <eblake(a)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
3:18 p.m.
New subject: [libvirt] [PATCH for-2.9 14/17] qapi: add static/migration-safe info to query-cpu-model-expansion
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(a)de.ibm.com>
Cc: Christian Borntraeger <borntraeger(a)de.ibm.com>
Cc: David Hildenbrand <david(a)redhat.com>
Cc: libvir-list(a)redhat.com
Cc: Jiri Denemark <jdenemar(a)redhat.com>
Cc: "Jason J. Herne" <jjherne(a)linux.vnet.ibm.com>
Cc: Markus Armbruster <armbru(a)redhat.com>
Cc: Eric Blake <eblake(a)redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost(a)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
3:47 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 14/17] qapi: add static/migration-safe info to query-cpu-model-expansion
I'm not familiar with CPU model expansion, but here goes anyway.
Eduardo Habkost <ehabkost(a)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(a)de.ibm.com>
Cc: Christian Borntraeger <borntraeger(a)de.ibm.com>
Cc: David Hildenbrand <david(a)redhat.com>
Cc: libvir-list(a)redhat.com
Cc: Jiri Denemark <jdenemar(a)redhat.com>
Cc: "Jason J. Herne" <jjherne(a)linux.vnet.ibm.com>
Cc: Markus Armbruster <armbru(a)redhat.com>
Cc: Eric Blake <eblake(a)redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost(a)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;
}
6:46 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 14/17] qapi: add static/migration-safe info to query-cpu-model-expansion
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(a)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(a)de.ibm.com>
> Cc: Christian Borntraeger <borntraeger(a)de.ibm.com>
> Cc: David Hildenbrand <david(a)redhat.com>
> Cc: libvir-list(a)redhat.com
> Cc: Jiri Denemark <jdenemar(a)redhat.com>
> Cc: "Jason J. Herne" <jjherne(a)linux.vnet.ibm.com>
> Cc: Markus Armbruster <armbru(a)redhat.com>
> Cc: Eric Blake <eblake(a)redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost(a)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
3:18 p.m.
New subject: [libvirt] [PATCH for-2.9 17/17] target-i386: Implement query-cpu-model-expansion QMP command
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(a)redhat.com
Cc: Jiri Denemark <jdenemar(a)redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost(a)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
4:16 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 17/17] target-i386: Implement query-cpu-model-expansion QMP command
Eduardo Habkost <ehabkost(a)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(a)redhat.com
Cc: Jiri Denemark <jdenemar(a)redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost(a)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));
6:55 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 17/17] target-i386: Implement query-cpu-model-expansion QMP command
On Tue, Dec 13, 2016 at 11:16:01AM +0100, Markus Armbruster wrote:
Eduardo Habkost <ehabkost(a)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(a)redhat.com
> Cc: Jiri Denemark <jdenemar(a)redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost(a)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
1:20 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 17/17] target-i386: Implement query-cpu-model-expansion QMP command
Eduardo Habkost <ehabkost(a)redhat.com> writes:
On Tue, Dec 13, 2016 at 11:16:01AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost(a)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(a)redhat.com
> > Cc: Jiri Denemark <jdenemar(a)redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost(a)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));
3:11 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 17/17] target-i386: Implement query-cpu-model-expansion QMP command
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
9:15 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 00/17] target-i386: Implement query-cpu-model-expansion
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
11:11 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 00/17] target-i386: Implement query-cpu-model-expansion
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
12:13 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 00/17] target-i386: Implement query-cpu-model-expansion
> 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.
--
David
5:45 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH for-2.9 00/17] target-i386: Implement query-cpu-model-expansion
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
2905
days inactive
2916
days old
12 comments
3 participants
participants (3)
-
David Hildenbrand -
Eduardo Habkost -
Markus Armbruster