[libvirt] [PATCH v4 00/11] Add runnability info to query-cpu-definitions
This series extends query-cpu-definitions to include an extra field: "unavailable-features". The new field can be used to find out reasons that prevent the CPU model from running in the current host. This will return information based on the current machine and accelerator only. In the future we may extend these mechanisms to allow querying other machines and other accelerators without restarting QEMU, but it will require some reorganization of QEMU's main code. To be able to implement this more cleanly, the series rework some of the feature/property name code. This series can be seen in the git branch at: https://github.com/ehabkost/qemu-hacks.git work/query-cpu-definitions-runnable-info The series is based on my x86-next branch: https://github.com/ehabkost/qemu.git x86-next Changes v3 -> v4: * Removed patch "Define CPUID filtering functions before x86_cpu_list" * New patch: "tests: Add test case for x86 feature parsing compatibility" * New patch: "target-i386: Disable VME by default with TCG" * Disable VME by default on TCG to avoid returning bogus results for all CPU models in TCG mode * New patch: "target-i386: Make plus_features/minus_features QOM-based" * New patch: "target-i386: Remove underscores from property names" * New patch: "target-i386: Register properties for feature aliases manually" * New patch: "target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas" * New patch: "target-i386: x86_cpu_load_features() function" * On patch: "target-i386: Return runnability information on query-cpu-definitions" * Added code to handle unsupported XSAVE components cleanly * Use x86_cpu_load_features() function Changes v2 -> v3: * Small documentation reword * Suggested-by: Markus Armbruster <armbru@redhat.com> * Create a x86_cpu_feature_name() function, to isolate the code that returns the property name Changes v1 -> v2: * Fixed documentation to say "(since 2.7)" * Removed @runnable field, improved documentation Example command output: { "return": [ { "unavailable-features": [], "static": false, "name": "host" }, { "unavailable-features": [], "static": false, "name": "qemu64" }, { "unavailable-features": [], "static": false, "name": "qemu32" }, { "unavailable-features": ["npt", "sse4a", "3dnow", "3dnowext", "fxsr-opt", "mmxext"], "static": false, "name": "phenom" }, { "unavailable-features": [], "static": false, "name": "pentium3" }, { "unavailable-features": [], "static": false, "name": "pentium2" }, { "unavailable-features": [], "static": false, "name": "pentium" }, { "unavailable-features": [], "static": false, "name": "n270" }, { "unavailable-features": [], "static": false, "name": "kvm64" }, { "unavailable-features": [], "static": false, "name": "kvm32" }, { "unavailable-features": [], "static": false, "name": "coreduo" }, { "unavailable-features": [], "static": false, "name": "core2duo" }, { "unavailable-features": ["3dnow", "3dnowext", "mmxext"], "static": false, "name": "athlon" }, { "unavailable-features": [], "static": false, "name": "Westmere" }, { "unavailable-features": ["xgetbv1", "xsavec", "3dnowprefetch", "smap", "adx", "rdseed", "mpx", "rtm", "hle"], "static": false, "name": "Skylake-Client" }, { "unavailable-features": [], "static": false, "name": "SandyBridge" }, { "unavailable-features": [], "static": false, "name": "Penryn" }, { "unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", "misalignsse", "sse4a"], "static": false, "name": "Opteron_G5" }, { "unavailable-features": ["fma4", "xop", "3dnowprefetch", "misalignsse", "sse4a"], "static": false, "name": "Opteron_G4" }, { "unavailable-features": ["misalignsse", "sse4a"], "static": false, "name": "Opteron_G3" }, { "unavailable-features": [], "static": false, "name": "Opteron_G2" }, { "unavailable-features": [], "static": false, "name": "Opteron_G1" }, { "unavailable-features": [], "static": false, "name": "Nehalem" }, { "unavailable-features": [], "static": false, "name": "IvyBridge" }, { "unavailable-features": ["rtm", "hle"], "static": false, "name": "Haswell" }, { "unavailable-features": [], "static": false, "name": "Haswell-noTSX" }, { "unavailable-features": [], "static": false, "name": "Conroe" }, { "unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed", "rtm", "hle"], "static": false, "name": "Broadwell" }, { "unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed"], "static": false, "name": "Broadwell-noTSX" }, { "unavailable-features": [], "static": false, "name": "486" } ]} Cc: David Hildenbrand <dahi@linux.vnet.ibm.com> Cc: Michael Mueller <mimu@linux.vnet.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Jiri Denemark <jdenemar@redhat.com> Cc: libvir-list@redhat.com Eduardo Habkost (11): tests: Add test case for x86 feature parsing compatibility target-i386: List CPU models using subclass list target-i386: Disable VME by default with TCG target-i386: Make plus_features/minus_features QOM-based target-i386: Remove underscores from property names target-i386: Register properties for feature aliases manually target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas target-i386: Move warning code outside x86_cpu_filter_features() target-i386: x86_cpu_load_features() function qmp: Add runnability information to query-cpu-definitions target-i386: Return runnability information on query-cpu-definitions qapi-schema.json | 23 +- target-i386/cpu-qom.h | 4 + target-i386/cpu.c | 489 +++++++++++++++++++++++++----------------- tests/test-x86-cpuid-compat.c | 39 ++++ 4 files changed, 362 insertions(+), 193 deletions(-) -- 2.7.4
Add a new test case to ensure the existing behavior of the feature parsing code wlil be kept. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- tests/test-x86-cpuid-compat.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c index 83162a4..7cff2b5 100644 --- a/tests/test-x86-cpuid-compat.c +++ b/tests/test-x86-cpuid-compat.c @@ -3,6 +3,7 @@ #include "qapi/qmp/qlist.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qint.h" +#include "qapi/qmp/qbool.h" #include "libqtest.h" static char *get_cpu0_qom_path(void) @@ -34,6 +35,15 @@ static QObject *qom_get(const char *path, const char *prop) return ret; } +static bool qom_get_bool(const char *path, const char *prop) +{ + QBool *value = qobject_to_qbool(qom_get(path, prop)); + bool b = qbool_get_bool(value); + + QDECREF(value); + return b; +} + typedef struct CpuidTestArgs { const char *cmdline; const char *property; @@ -66,10 +76,39 @@ static void add_cpuid_test(const char *name, const char *cmdline, qtest_add_data_func(name, args, test_cpuid_prop); } +static void test_plus_minus(void) +{ + char *path; + + /* Rules: + * "-foo" overrides "+foo" + * "[+-]foo" overrides "foo=..." + * "foo_bar" should be translated to "foo-bar" + */ + qtest_start("-cpu pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on"); + path = get_cpu0_qom_path(); + + g_assert_false(qom_get_bool(path, "fpu")); + g_assert_false(qom_get_bool(path, "mce")); + g_assert_true(qom_get_bool(path, "cx8")); + + /* Test both the original and the alias feature names: */ + g_assert_true(qom_get_bool(path, "sse4-1")); + g_assert_true(qom_get_bool(path, "sse4.1")); + + g_assert_true(qom_get_bool(path, "sse4-2")); + g_assert_true(qom_get_bool(path, "sse4.2")); + + qtest_end(); + g_free(path); +} + int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); + qtest_add_func("x86/cpuid/parsing-plus-minus", test_plus_minus); + /* Original level values for CPU models: */ add_cpuid_test("x86/cpuid/phenom/level", "-cpu phenom", "level", 5); -- 2.7.4
On Thu, Sep 29, 2016 at 06:14:49PM -0300, Eduardo Habkost wrote:
Add a new test case to ensure the existing behavior of the feature parsing code wlil be kept.
s/wlil/will/
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Jonathan Neuschäfer
On 29/09/2016 23:14, Eduardo Habkost wrote:
+ * "-foo" overrides "+foo" + * "[+-]foo" overrides "foo=..."
Is this something that people are actually using? Can we detect it and deprecate it in 2.8, and drop it in 2.9? Paolo
+ * "foo_bar" should be translated to "foo-bar"
On Fri, Sep 30, 2016 at 09:55:33 +0200, Paolo Bonzini wrote:
On 29/09/2016 23:14, Eduardo Habkost wrote:
+ * "-foo" overrides "+foo" + * "[+-]foo" overrides "foo=..."
Is this something that people are actually using? Can we detect it and deprecate it in 2.8, and drop it in 2.9?
Libvirt uses -cpu Model,+foo,-bar style, but we do not mix mix -foo and +foo, or even [+-]foo and foo= if this is what you asked. Jirka
On Fri, Sep 30, 2016 at 09:55:33AM +0200, Paolo Bonzini wrote:
On 29/09/2016 23:14, Eduardo Habkost wrote:
+ * "-foo" overrides "+foo" + * "[+-]foo" overrides "foo=..."
Is this something that people are actually using? Can we detect it and deprecate it in 2.8, and drop it in 2.9?
We can, but I would like to keep the test cases there in 2.8, at least. I will update the test case to note that this is legacy behavior that we plan to remove in 2.9, and send a separate follow-up patch to detect when people mix both formats. -- Eduardo
On 30/09/2016 20:33, Eduardo Habkost wrote:
On Fri, Sep 30, 2016 at 09:55:33AM +0200, Paolo Bonzini wrote:
On 29/09/2016 23:14, Eduardo Habkost wrote:
+ * "-foo" overrides "+foo" + * "[+-]foo" overrides "foo=..."
Is this something that people are actually using? Can we detect it and deprecate it in 2.8, and drop it in 2.9?
We can, but I would like to keep the test cases there in 2.8, at least. I will update the test case to note that this is legacy behavior that we plan to remove in 2.9, and send a separate follow-up patch to detect when people mix both formats.
Yes, of course! Sounds like a very good plan. Paolo
Instead of using the builtin_x86_defs array, use the QOM subclass list to list CPU models on "-cpu ?" and "query-cpu-definitions". Signed-off-by: Andreas Färber <afaerber@suse.de> [ehabkost: copied code from a patch by Andreas: "target-i386: QOM'ify CPU", from March 2012] Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu-qom.h | 4 ++ target-i386/cpu.c | 103 ++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 78 insertions(+), 29 deletions(-) diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h index 5dde658..e724004 100644 --- a/target-i386/cpu-qom.h +++ b/target-i386/cpu-qom.h @@ -63,6 +63,10 @@ typedef struct X86CPUClass { bool kvm_required; + /* Optional description of CPU model. + * If unavailable, cpu_def->model_id is used */ + const char *model_description; + DeviceRealize parent_realize; void (*parent_reset)(CPUState *cpu); } X86CPUClass; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 0807e92..ac3646e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1628,6 +1628,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data) cpu_x86_fill_model_id(host_cpudef.model_id); xcc->cpu_def = &host_cpudef; + xcc->model_description = + "KVM processor with all supported host features " + "(only available in KVM mode)"; /* level, xlevel, xlevel2, and the feature words are initialized on * instance_init, because they require KVM to be initialized. @@ -2098,23 +2101,62 @@ static void listflags(FILE *f, fprintf_function print, const char **featureset) } } -/* generate CPU information. */ +/* Sort alphabetically by type name, listing kvm_required models last. */ +static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b) +{ + ObjectClass *class_a = (ObjectClass *)a; + ObjectClass *class_b = (ObjectClass *)b; + X86CPUClass *cc_a = X86_CPU_CLASS(class_a); + X86CPUClass *cc_b = X86_CPU_CLASS(class_b); + const char *name_a, *name_b; + + if (cc_a->kvm_required != cc_b->kvm_required) { + /* kvm_required items go last */ + return cc_a->kvm_required ? 1 : -1; + } else { + name_a = object_class_get_name(class_a); + name_b = object_class_get_name(class_b); + return strcmp(name_a, name_b); + } +} + +static GSList *get_sorted_cpu_model_list(void) +{ + GSList *list = object_class_get_list(TYPE_X86_CPU, false); + list = g_slist_sort(list, x86_cpu_list_compare); + return list; +} + +static void x86_cpu_list_entry(gpointer data, gpointer user_data) +{ + ObjectClass *oc = data; + X86CPUClass *cc = X86_CPU_CLASS(oc); + CPUListState *s = user_data; + char *name = x86_cpu_class_get_model_name(cc); + const char *desc = cc->model_description; + if (!desc) { + desc = cc->cpu_def->model_id; + } + + (*s->cpu_fprintf)(s->file, "x86 %16s %-48s\n", + name, desc); + g_free(name); +} + +/* list available CPU models and flags */ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) { - X86CPUDefinition *def; - char buf[256]; int i; + CPUListState s = { + .file = f, + .cpu_fprintf = cpu_fprintf, + }; + GSList *list; - for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) { - def = &builtin_x86_defs[i]; - snprintf(buf, sizeof(buf), "%s", def->name); - (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); - } -#ifdef CONFIG_KVM - (*cpu_fprintf)(f, "x86 %16s %-48s\n", "host", - "KVM processor with all supported host features " - "(only available in KVM mode)"); -#endif + (*cpu_fprintf)(f, "Available CPUs:\n"); + list = get_sorted_cpu_model_list(); + g_slist_foreach(list, x86_cpu_list_entry, &s); + g_slist_free(list); (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { @@ -2126,26 +2168,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) } } -CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp) +static void x86_cpu_definition_entry(gpointer data, gpointer user_data) { - CpuDefinitionInfoList *cpu_list = NULL; - X86CPUDefinition *def; - int i; + ObjectClass *oc = data; + X86CPUClass *cc = X86_CPU_CLASS(oc); + CpuDefinitionInfoList **cpu_list = user_data; + CpuDefinitionInfoList *entry; + CpuDefinitionInfo *info; - for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) { - CpuDefinitionInfoList *entry; - CpuDefinitionInfo *info; - - def = &builtin_x86_defs[i]; - info = g_malloc0(sizeof(*info)); - info->name = g_strdup(def->name); + info = g_malloc0(sizeof(*info)); + info->name = x86_cpu_class_get_model_name(cc); - entry = g_malloc0(sizeof(*entry)); - entry->value = info; - entry->next = cpu_list; - cpu_list = entry; - } + entry = g_malloc0(sizeof(*entry)); + entry->value = info; + entry->next = *cpu_list; + *cpu_list = entry; +} +CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp) +{ + CpuDefinitionInfoList *cpu_list = NULL; + GSList *list = get_sorted_cpu_model_list(); + g_slist_foreach(list, x86_cpu_definition_entry, &cpu_list); + g_slist_free(list); return cpu_list; } -- 2.7.4
VME is already disabled automatically when using TCG. So, instead of pretending it is there when reporting CPU model data on query-cpu-* QMP commands (making every CPU model to be reported as not runnable), we can disable it by default on all CPU models when using TCG. Do that by adding a tcg_default_props array that will work like kvm_default_props. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index ac3646e..3d3f64e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1550,6 +1550,14 @@ static PropValue kvm_default_props[] = { { NULL, NULL }, }; +/* TCG-specific defaults that override all CPU models when using TCG + */ +static PropValue tcg_default_props[] = { + { "vme", "off" }, + { NULL, NULL }, +}; + + void x86_cpu_change_kvm_default(const char *prop, const char *value) { PropValue *pv; @@ -2283,6 +2291,8 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) } x86_cpu_apply_props(cpu, kvm_default_props); + } else if (tcg_enabled()) { + x86_cpu_apply_props(cpu, tcg_default_props); } env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR; -- 2.7.4
Instead of using custom feature name lookup code for plus_features/minus_features, save the property names used in "[+-]feature" and use object_property_set_bool() to set them. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 108 +++++++++++------------------------------------------- 1 file changed, 22 insertions(+), 86 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 3d3f64e..4eaec0e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -650,85 +650,6 @@ void host_cpuid(uint32_t function, uint32_t count, *edx = vec[3]; } -#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c))) - -/* general substring compare of *[s1..e1) and *[s2..e2). sx is start of - * a substring. ex if !NULL points to the first char after a substring, - * otherwise the string is assumed to sized by a terminating nul. - * Return lexical ordering of *s1:*s2. - */ -static int sstrcmp(const char *s1, const char *e1, - const char *s2, const char *e2) -{ - for (;;) { - if (!*s1 || !*s2 || *s1 != *s2) - return (*s1 - *s2); - ++s1, ++s2; - if (s1 == e1 && s2 == e2) - return (0); - else if (s1 == e1) - return (*s2); - else if (s2 == e2) - return (*s1); - } -} - -/* compare *[s..e) to *altstr. *altstr may be a simple string or multiple - * '|' delimited (possibly empty) strings in which case search for a match - * within the alternatives proceeds left to right. Return 0 for success, - * non-zero otherwise. - */ -static int altcmp(const char *s, const char *e, const char *altstr) -{ - const char *p, *q; - - for (q = p = altstr; ; ) { - while (*p && *p != '|') - ++p; - if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p))) - return (0); - if (!*p) - return (1); - else - q = ++p; - } -} - -/* search featureset for flag *[s..e), if found set corresponding bit in - * *pval and return true, otherwise return false - */ -static bool lookup_feature(uint32_t *pval, const char *s, const char *e, - const char **featureset) -{ - uint32_t mask; - const char **ppc; - bool found = false; - - for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) { - if (*ppc && !altcmp(s, e, *ppc)) { - *pval |= mask; - found = true; - } - } - return found; -} - -static void add_flagname_to_bitmaps(const char *flagname, - FeatureWordArray words, - Error **errp) -{ - FeatureWord w; - for (w = 0; w < FEATURE_WORDS; w++) { - FeatureWordInfo *wi = &feature_word_info[w]; - if (lookup_feature(&words[w], flagname, NULL, wi->feat_names)) { - break; - } - } - if (w == FEATURE_WORDS) { - error_setg(errp, "CPU feature %s not found", flagname); - } -} - /* CPU class name definitions: */ #define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU @@ -2015,8 +1936,7 @@ static inline void feat2prop(char *s) * feat=on|feat even if the later is parsed after +-feat * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled) */ -static FeatureWordArray plus_features = { 0 }; -static FeatureWordArray minus_features = { 0 }; +static GList *plus_features, *minus_features; /* Parse "+feature,-feature,feature=foo" CPU feature string */ @@ -2047,10 +1967,14 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features, /* Compatibility syntax: */ if (featurestr[0] == '+') { - add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); + feat2prop(featurestr + 1); + plus_features = g_list_append(plus_features, + g_strdup(featurestr + 1)); continue; } else if (featurestr[0] == '-') { - add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); + feat2prop(featurestr + 1); + minus_features = g_list_append(minus_features, + g_strdup(featurestr + 1)); continue; } @@ -3066,6 +2990,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) Error *local_err = NULL; static bool ht_warned; FeatureWord w; + GList *l; if (xcc->kvm_required && !kvm_enabled()) { char *name = x86_cpu_class_get_model_name(xcc); @@ -3091,9 +3016,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) } } - for (w = 0; w < FEATURE_WORDS; w++) { - cpu->env.features[w] |= plus_features[w]; - cpu->env.features[w] &= ~minus_features[w]; + for (l = plus_features; l; l = l->next) { + const char *prop = l->data; + object_property_set_bool(OBJECT(cpu), true, prop, &local_err); + if (local_err) { + goto out; + } + } + + for (l = minus_features; l; l = l->next) { + const char *prop = l->data; + object_property_set_bool(OBJECT(cpu), false, prop, &local_err); + if (local_err) { + goto out; + } } if (!kvm_enabled() || !cpu->expose_kvm) { -- 2.7.4
Instead of translating the feature name entries when adding property names, store the actual property names in the feature name array. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4eaec0e..7795a7c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -279,11 +279,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { [FEAT_1_ECX] = { .feat_names = { "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor", - "ds_cpl", "vmx", "smx", "est", + "ds-cpl", "vmx", "smx", "est", "tm2", "ssse3", "cid", NULL, "fma", "cx16", "xtpr", "pdcm", - NULL, "pcid", "dca", "sse4.1|sse4_1", - "sse4.2|sse4_2", "x2apic", "movbe", "popcnt", + NULL, "pcid", "dca", "sse4.1|sse4-1", + "sse4.2|sse4-2", "x2apic", "movbe", "popcnt", "tsc-deadline", "aes", "xsave", "osxsave", "avx", "f16c", "rdrand", "hypervisor", }, @@ -303,7 +303,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */, NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */, "nx|xd", NULL, "mmxext", NULL /* mmx */, - NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb", "rdtscp", + NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp", NULL, "lm|i64", "3dnowext", "3dnow", }, .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX, @@ -311,13 +311,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, [FEAT_8000_0001_ECX] = { .feat_names = { - "lahf_lm", "cmp_legacy", "svm", "extapic", + "lahf-lm", "cmp-legacy", "svm", "extapic", "cr8legacy", "abm", "sse4a", "misalignsse", "3dnowprefetch", "osvw", "ibs", "xop", "skinit", "wdt", NULL, "lwp", - "fma4", "tce", NULL, "nodeid_msr", - NULL, "tbm", "topoext", "perfctr_core", - "perfctr_nb", NULL, NULL, NULL, + "fma4", "tce", NULL, "nodeid-msr", + NULL, "tbm", "topoext", "perfctr-core", + "perfctr-nb", NULL, NULL, NULL, NULL, NULL, NULL, NULL, }, .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX, @@ -339,8 +339,8 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, [FEAT_KVM] = { .feat_names = { - "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", - "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt", + "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock", + "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, @@ -400,9 +400,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, [FEAT_SVM] = { .feat_names = { - "npt", "lbrv", "svm_lock", "nrip_save", - "tsc_scale", "vmcb_clean", "flushbyasid", "decodeassists", - NULL, NULL, "pause_filter", NULL, + "npt", "lbrv", "svm-lock", "nrip-save", + "tsc-scale", "vmcb-clean", "flushbyasid", "decodeassists", + NULL, NULL, "pause-filter", NULL, "pfthreshold", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, @@ -414,7 +414,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, [FEAT_7_0_EBX] = { .feat_names = { - "fsgsbase", "tsc_adjust", NULL, "bmi1", + "fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep", "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL, @@ -3334,7 +3334,8 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu, names = g_strsplit(fi->feat_names[bitnr], "|", 0); - feat2prop(names[0]); + /* Property names should use "-" instead of "_" */ + assert(!strchr(names[0], '_')); x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr); for (i = 1; names[i]; i++) { -- 2.7.4
On Thu, Sep 29, 2016 at 18:14:53 -0300, Eduardo Habkost wrote:
Instead of translating the feature name entries when adding property names, store the actual property names in the feature name array.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4eaec0e..7795a7c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -279,11 +279,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { [FEAT_1_ECX] = { .feat_names = { "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor", - "ds_cpl", "vmx", "smx", "est", + "ds-cpl", "vmx", "smx", "est", "tm2", "ssse3", "cid", NULL, "fma", "cx16", "xtpr", "pdcm", - NULL, "pcid", "dca", "sse4.1|sse4_1", - "sse4.2|sse4_2", "x2apic", "movbe", "popcnt", + NULL, "pcid", "dca", "sse4.1|sse4-1", + "sse4.2|sse4-2", "x2apic", "movbe", "popcnt", "tsc-deadline", "aes", "xsave", "osxsave", "avx", "f16c", "rdrand", "hypervisor", },
It wasn't quite obvious to me where this means we can't use the names with underscores when talking to QEMU. So I tried it and apparently underscores are just silently translated to dashes. It's backward compatible this way. However, QEMU will always give us the names with dashes, which means we have even more differences between libvirt's feature names and QEMU's feature names. So assuming we'll have an interface for querying supported CPU properties (and their aliases), shouldn't the old underscore names be added as aliases? This way, we could actually know that "ds-cpl" means "ds_cpl". Jirka
On Fri, Sep 30, 2016 at 09:29:58AM +0200, Jiri Denemark wrote:
On Thu, Sep 29, 2016 at 18:14:53 -0300, Eduardo Habkost wrote:
Instead of translating the feature name entries when adding property names, store the actual property names in the feature name array.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4eaec0e..7795a7c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -279,11 +279,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { [FEAT_1_ECX] = { .feat_names = { "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor", - "ds_cpl", "vmx", "smx", "est", + "ds-cpl", "vmx", "smx", "est", "tm2", "ssse3", "cid", NULL, "fma", "cx16", "xtpr", "pdcm", - NULL, "pcid", "dca", "sse4.1|sse4_1", - "sse4.2|sse4_2", "x2apic", "movbe", "popcnt", + NULL, "pcid", "dca", "sse4.1|sse4-1", + "sse4.2|sse4-2", "x2apic", "movbe", "popcnt", "tsc-deadline", "aes", "xsave", "osxsave", "avx", "f16c", "rdrand", "hypervisor", },
It wasn't quite obvious to me where this means we can't use the names with underscores when talking to QEMU. So I tried it and apparently underscores are just silently translated to dashes. It's backward compatible this way. However, QEMU will always give us the names with dashes, which means we have even more differences between libvirt's feature names and QEMU's feature names. So assuming we'll have an interface for querying supported CPU properties (and their aliases), shouldn't the old underscore names be added as aliases? This way, we could actually know that "ds-cpl" means "ds_cpl".
Good idea. Instead of translating property names silently we can add aliases for the ones where underscores were actually used. I will look into it. -- Eduardo
Instead of keeping the aliases inside the feature name arrays and require parsing the strings, just register alias properties manually. This simplifies the property registration code and will simplify code that needs to look up property names for CPUID bits. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 7795a7c..c013ed0 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -278,12 +278,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, [FEAT_1_ECX] = { .feat_names = { - "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor", + "pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor", "ds-cpl", "vmx", "smx", "est", "tm2", "ssse3", "cid", NULL, "fma", "cx16", "xtpr", "pdcm", - NULL, "pcid", "dca", "sse4.1|sse4-1", - "sse4.2|sse4-2", "x2apic", "movbe", "popcnt", + NULL, "pcid", "dca", "sse4.1", + "sse4.2", "x2apic", "movbe", "popcnt", "tsc-deadline", "aes", "xsave", "osxsave", "avx", "f16c", "rdrand", "hypervisor", }, @@ -302,9 +302,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL /* cx8 */, NULL /* apic */, NULL, "syscall", NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */, NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */, - "nx|xd", NULL, "mmxext", NULL /* mmx */, - NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp", - NULL, "lm|i64", "3dnowext", "3dnow", + "nx", NULL, "mmxext", NULL /* mmx */, + NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp", + NULL, "lm", "3dnowext", "3dnow", }, .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX, .tcg_features = TCG_EXT2_FEATURES, @@ -3323,28 +3323,19 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu, FeatureWord w, int bitnr) { - Object *obj = OBJECT(cpu); - int i; - char **names; FeatureWordInfo *fi = &feature_word_info[w]; + const char *name = fi->feat_names[bitnr]; - if (!fi->feat_names[bitnr]) { + if (!name) { return; } - names = g_strsplit(fi->feat_names[bitnr], "|", 0); - /* Property names should use "-" instead of "_" */ - assert(!strchr(names[0], '_')); - x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr); - - for (i = 1; names[i]; i++) { - feat2prop(names[i]); - object_property_add_alias(obj, names[i], obj, names[0], - &error_abort); - } - - g_strfreev(names); + assert(!strchr(name, '_')); + /* aliases don't use "|" delimiters anymore, they are registered + * manually using object_property_add_alias() */ + assert(!strchr(name, '|')); + x86_cpu_register_bit_prop(cpu, name, &cpu->env.features[w], bitnr); } static void x86_cpu_initfn(Object *obj) @@ -3392,6 +3383,15 @@ static void x86_cpu_initfn(Object *obj) } } + /* Alias for feature properties: */ + object_property_add_alias(obj, "sse3", obj, "pni", &error_abort); + object_property_add_alias(obj, "pclmuldq", obj, "pclmulqdq", &error_abort); + object_property_add_alias(obj, "sse4-1", obj, "sse4.1", &error_abort); + object_property_add_alias(obj, "sse4-2", obj, "sse4.2", &error_abort); + object_property_add_alias(obj, "xd", obj, "nx", &error_abort); + object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", &error_abort); + object_property_add_alias(obj, "i64", obj, "lm", &error_abort); + x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort); } -- 2.7.4
Instead of treating the FP and SSE bits as special cases, add them to the x86_ext_save_areas array. This will simplify the code that calculates the supported xsave components and the size of the xsave area. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c013ed0..b36388e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -535,6 +535,20 @@ typedef struct ExtSaveArea { } ExtSaveArea; static const ExtSaveArea x86_ext_save_areas[] = { + [XSTATE_FP_BIT] = { + /* x87 FP state component is always enabled if XSAVE is supported */ + .feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE, + /* x87 state is in the legacy region of the XSAVE area */ + .offset = 0, + .size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader), + }, + [XSTATE_SSE_BIT] = { + /* SSE state component is always enabled if XSAVE is supported */ + .feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE, + /* SSE state is in the legacy region of the XSAVE area */ + .offset = 0, + .size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader), + }, [XSTATE_YMM_BIT] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX, .offset = offsetof(X86XSaveArea, avx_state), @@ -568,9 +582,9 @@ static const ExtSaveArea x86_ext_save_areas[] = { static uint32_t xsave_area_size(uint64_t mask) { int i; - uint64_t ret = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader); + uint64_t ret = 0; - for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { + for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; if ((mask >> i) & 1) { ret = MAX(ret, esa->offset + esa->size); @@ -2963,8 +2977,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu) return; } - mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK); - for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { + mask = 0; + for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; if (env->features[esa->feature] & esa->bits) { mask |= (1ULL << i); -- 2.7.4
x86_cpu_filter_features() will be reused by code that shouldn't print any warning. Move the warning code to a new x86_cpu_report_filtered_features() function, and call it from x86_cpu_realizefn(). Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v3 -> v4: * Made x86_cpu_filter_features() void, make x86_cpu_report_filtered_features() return true if some features were filtered --- target-i386/cpu.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index b36388e..b25657b 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2163,14 +2163,11 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, /* * Filters CPU feature words based on host availability of each feature. - * - * Returns: 0 if all flags are supported by the host, non-zero otherwise. */ -static int x86_cpu_filter_features(X86CPU *cpu) +static void x86_cpu_filter_features(X86CPU *cpu) { CPUX86State *env = &cpu->env; FeatureWord w; - int rv = 0; for (w = 0; w < FEATURE_WORDS; w++) { uint32_t host_feat = @@ -2178,15 +2175,22 @@ static int x86_cpu_filter_features(X86CPU *cpu) uint32_t requested_features = env->features[w]; env->features[w] &= host_feat; cpu->filtered_features[w] = requested_features & ~env->features[w]; - if (cpu->filtered_features[w]) { - if (cpu->check_cpuid || cpu->enforce_cpuid) { - report_unavailable_features(w, cpu->filtered_features[w]); - } - rv = 1; - } } +} - return rv; +/* Report list of filtered features to stderr. + * Returns true if some features were found to be filtered, false otherwise + */ +static bool x86_cpu_report_filtered_features(X86CPU *cpu) +{ + FeatureWord w; + uint32_t filtered = 0; + + for (w = 0; w < FEATURE_WORDS; w++) { + filtered |= cpu->filtered_features[w]; + report_unavailable_features(w, cpu->filtered_features[w]); + } + return filtered; } static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props) @@ -3082,12 +3086,15 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) env->cpuid_xlevel2 = env->cpuid_min_xlevel2; } - if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) { - error_setg(&local_err, - kvm_enabled() ? - "Host doesn't support requested features" : - "TCG doesn't support requested features"); - goto out; + x86_cpu_filter_features(cpu); + if (cpu->check_cpuid || cpu->enforce_cpuid) { + if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) { + error_setg(&local_err, + kvm_enabled() ? + "Host doesn't support requested features" : + "TCG doesn't support requested features"); + goto out; + } } /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on -- 2.7.4
When probing for CPU model information, we need to reuse the code that initializes CPUID fields, but not the remaining side-effects of x86_cpu_realizefn(). Move that code to a separate function that can be reused later. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes series v3 -> v4: * New patch added to series --- target-i386/cpu.c | 67 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index b25657b..e099892 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2993,34 +2993,14 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu) env->features[FEAT_XSAVE_COMP_HI] = mask >> 32; } -#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \ - (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \ - (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3) -#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \ - (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \ - (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3) -static void x86_cpu_realizefn(DeviceState *dev, Error **errp) +/* Load CPUID data based on configureured features + */ +static void x86_cpu_load_features(X86CPU *cpu, Error **errp) { - CPUState *cs = CPU(dev); - X86CPU *cpu = X86_CPU(dev); - X86CPUClass *xcc = X86_CPU_GET_CLASS(dev); CPUX86State *env = &cpu->env; - Error *local_err = NULL; - static bool ht_warned; FeatureWord w; GList *l; - - if (xcc->kvm_required && !kvm_enabled()) { - char *name = x86_cpu_class_get_model_name(xcc); - error_setg(&local_err, "CPU model '%s' requires KVM", name); - g_free(name); - goto out; - } - - if (cpu->apic_id == UNASSIGNED_APIC_ID) { - error_setg(errp, "apic-id property was not initialized properly"); - return; - } + Error *local_err = NULL; /*TODO: cpu->host_features incorrectly overwrites features * set using "feat=on|off". Once we fix this, we can convert @@ -3087,6 +3067,45 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) } x86_cpu_filter_features(cpu); + +out: + if (local_err != NULL) { + error_propagate(errp, local_err); + } +} + +#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \ + (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \ + (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3) +#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \ + (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \ + (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3) +static void x86_cpu_realizefn(DeviceState *dev, Error **errp) +{ + CPUState *cs = CPU(dev); + X86CPU *cpu = X86_CPU(dev); + X86CPUClass *xcc = X86_CPU_GET_CLASS(dev); + CPUX86State *env = &cpu->env; + Error *local_err = NULL; + static bool ht_warned; + + if (xcc->kvm_required && !kvm_enabled()) { + char *name = x86_cpu_class_get_model_name(xcc); + error_setg(&local_err, "CPU model '%s' requires KVM", name); + g_free(name); + goto out; + } + + if (cpu->apic_id == UNASSIGNED_APIC_ID) { + error_setg(errp, "apic-id property was not initialized properly"); + return; + } + + x86_cpu_load_features(cpu, &local_err); + if (local_err) { + goto out; + } + if (cpu->check_cpuid || cpu->enforce_cpuid) { if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) { error_setg(&local_err, -- 2.7.4
On 29/09/2016 23:14, Eduardo Habkost wrote:
+/* Load CPUID data based on configureured features + */
Typo ("configureured") and also unnecessarily breaking the comment on two lines. Paolo
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
Add a new optional field to query-cpu-definitions schema: "unavailable-features". It will contain a list of QOM properties that prevent the CPU model from running in the current host. Cc: David Hildenbrand <dahi@linux.vnet.ibm.com> Cc: Michael Mueller <mimu@linux.vnet.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Jiri Denemark <jdenemar@redhat.com> Cc: libvir-list@redhat.com Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v3 -> v4: * Changed doc to "since 2.8" as we missed 2.7 * Reported-by: Eric Blake <eblake@redhat.com> Changes v2 -> v3: * Small documentation reword * Suggested-by: Markus Armbruster <armbru@redhat.com> Changes v1 -> v2: * Remove @runnable field, non-empty @unavailable-features is enough to report CPU model as not runnable. * Documentation updates: * Changed to "(since 2.7)"; * Add more details about the exact meaning of unavailable-features, and what it would mean to see read-only QOM properties in the list, and that implementations can return "type" if there's no extra information available; --- qapi-schema.json | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/qapi-schema.json b/qapi-schema.json index c3dcf11..abb163b 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3114,10 +3114,31 @@ # QEMU version, machine type, machine options and accelerator options. # A static model is always migration-safe. (since 2.8) # +# @unavailable-features: #optional List of properties that prevent +# the CPU model from running in the current +# host. (since 2.8) +# +# @unavailable-features is a list of QOM property names that +# represent CPU model attributes that prevent the CPU from running. +# If the QOM property is read-only, that means there's no known +# way to make the CPU model run in the current host. Implementations +# that choose not to provide specific information return the +# property name "type". +# If the property is read-write, it means that it MAY be possible +# to run the CPU model in the current host if that property is +# changed. Management software can use it as hints to suggest or +# choose an alternative for the user, or just to generate meaningful +# error messages explaining why the CPU model can't be used. +# If @unavailable-features is an empty list, the CPU model is +# runnable using the current host and machine-type. +# If @unavailable-features is not present, runnability +# information for the CPU is not available. +# # Since: 1.2.0 ## { 'struct': 'CpuDefinitionInfo', - 'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool' } } + 'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool', + '*unavailable-features': [ 'str' ] } } ## # @query-cpu-definitions: -- 2.7.4
Fill the "unavailable-features" field on the x86 implementation of query-cpu-definitions. Cc: Jiri Denemark <jdenemar@redhat.com> Cc: libvir-list@redhat.com Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v3 -> v4: * Handle missing XSAVE components cleanly, but looking up the original feature that required it * Use x86_cpu_load_features() function Changes v2 -> v3: * Create a x86_cpu_feature_name() function, to isolate the code that returns the property name Changes v1 -> v2: * Updated to the new schema: no @runnable field, and always report @unavailable-features as present --- target-i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e099892..4dd3aee 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s) } } +/* Return the feature property name for a feature flag bit */ +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr) +{ + /* XSAVE components are automatically enabled by other features, + * so return the original feature name instead + */ + if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) { + int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr; + + if (comp < ARRAY_SIZE(x86_ext_save_areas) && + x86_ext_save_areas[comp].bits) { + w = x86_ext_save_areas[comp].feature; + bitnr = ctz32(x86_ext_save_areas[comp].bits); + } + } + + assert(bitnr < 32); + assert(w < FEATURE_WORDS); + return feature_word_info[w].feat_names[bitnr]; +} + /* Compatibily hack to maintain legacy +-feat semantic, * where +-feat overwrites any feature set by * feat=on|feat even if the later is parsed after +-feat @@ -2032,6 +2053,56 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features, } } +static void x86_cpu_load_features(X86CPU *cpu, Error **errp); + +/* Check for missing features that may prevent the CPU class from + * running using the current machine and accelerator. + */ +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, + strList **missing_feats) +{ + X86CPU *xc; + FeatureWord w; + Error *err = NULL; + strList **next = missing_feats; + + if (xcc->kvm_required && !kvm_enabled()) { + strList *new = g_new0(strList, 1); + new->value = g_strdup("kvm");; + *missing_feats = new; + return; + } + + xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc)))); + + x86_cpu_load_features(xc, &err); + if (err) { + /* Errors at x86_cpu_load_features should never happen, + * but in case it does, just report the model as not + * runnable at all using the "type" property. + */ + strList *new = g_new0(strList, 1); + new->value = g_strdup("type"); + *next = new; + next = &new->next; + } + + for (w = 0; w < FEATURE_WORDS; w++) { + uint32_t filtered = xc->filtered_features[w]; + int i; + for (i = 0; i < 32; i++) { + if (filtered & (1UL << i)) { + strList *new = g_new0(strList, 1); + new->value = g_strdup(x86_cpu_feature_name(w, i)); + *next = new; + next = &new->next; + } + } + } + + object_unref(OBJECT(xc)); +} + /* Print all cpuid feature names in featureset */ static void listflags(FILE *f, fprintf_function print, const char **featureset) @@ -2124,6 +2195,8 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data) info = g_malloc0(sizeof(*info)); info->name = x86_cpu_class_get_model_name(cc); + x86_cpu_class_check_missing_features(cc, &info->unavailable_features); + info->has_unavailable_features = true; entry = g_malloc0(sizeof(*entry)); entry->value = info; -- 2.7.4
On 29/09/2016 23:14, Eduardo Habkost wrote:
+/* Return the feature property name for a feature flag bit */ +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr) +{ + /* XSAVE components are automatically enabled by other features, + * so return the original feature name instead + */ + if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) { + int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr; + + if (comp < ARRAY_SIZE(x86_ext_save_areas) && + x86_ext_save_areas[comp].bits) { + w = x86_ext_save_areas[comp].feature; + bitnr = ctz32(x86_ext_save_areas[comp].bits); + } + } + + assert(bitnr < 32); + assert(w < FEATURE_WORDS); + return feature_word_info[w].feat_names[bitnr]; +} +
Could this be used to replace migratable_features? Paolo
On Fri, Sep 30, 2016 at 10:02:49AM +0200, Paolo Bonzini wrote:
On 29/09/2016 23:14, Eduardo Habkost wrote:
+/* Return the feature property name for a feature flag bit */ +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr) +{ + /* XSAVE components are automatically enabled by other features, + * so return the original feature name instead + */ + if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) { + int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr; + + if (comp < ARRAY_SIZE(x86_ext_save_areas) && + x86_ext_save_areas[comp].bits) { + w = x86_ext_save_areas[comp].feature; + bitnr = ctz32(x86_ext_save_areas[comp].bits); + } + } + + assert(bitnr < 32); + assert(w < FEATURE_WORDS); + return feature_word_info[w].feat_names[bitnr]; +} +
Could this be used to replace migratable_features?
It can. I will do it in a follow-up patch. -- Eduardo
participants (4)
-
Eduardo Habkost -
Jiri Denemark -
Jonathan Neuschäfer -
Paolo Bonzini