[Qemu-devel] [PATCH v5 00/12] Add runnability info to query-cpu-definitions

[Qemu-devel] [PATCH v5 00/12] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

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 v4 -> v5:
* New patch: "target-i386: Register aliases for feature names with underscores"
* On patch: "tests: Add test case for x86 feature parsing compatibility":
  * Fix typo on commit message
    Reported-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
  * Add comment noting that the "[+-]feature" compatibility mode
    will be removed soon
* On patch: "target-i386: Make plus_features/minus_features QOM-based":
  * Removed feat2prop() call on , as we now have property aliases
    for the old names containing underscores
* On patch: "target-i386: Remove underscores from feat_names arrays":
  * Remove the feat2prop() call from the alias registration
    loop, too
  * Commit message update to enumerate all code that uses
    feat_names
* On patch: "target-i386: x86_cpu_load_features() function":
  * Fix typo on x86_cpu_load_features() comment
    Reported-by: Paolo Bonzini <pbonzini@redhat.com>

Diff v4 ->v5:

  diff --git a/target-i386/cpu.c b/target-i386/cpu.c
  index 4dd3aee..620889f 100644
  --- a/target-i386/cpu.c
  +++ b/target-i386/cpu.c
  @@ -2002,12 +2002,10 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,

           /* Compatibility syntax: */
           if (featurestr[0] == '+') {
  -            feat2prop(featurestr + 1);
               plus_features = g_list_append(plus_features,
                                             g_strdup(featurestr + 1));
               continue;
           } else if (featurestr[0] == '-') {
  -            feat2prop(featurestr + 1);
               minus_features = g_list_append(minus_features,
                                              g_strdup(featurestr + 1));
               continue;
  @@ -3066,8 +3064,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
       env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
   }

  -/* Load CPUID data based on configureured features
  - */
  +/* Load CPUID data based on configured features */
   static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
   {
       CPUX86State *env = &cpu->env;
  @@ -3443,7 +3440,10 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
           return;
       }

  -    /* Property names should use "-" instead of "_" */
  +    /* Property names should use "-" instead of "_".
  +     * Old names containing underscores are registered as aliases
  +     * using object_property_add_alias()
  +     */
       assert(!strchr(name, '_'));
       /* aliases don't use "|" delimiters anymore, they are registered
        * manually using object_property_add_alias() */
  @@ -3496,7 +3496,6 @@ 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);
  @@ -3505,6 +3504,28 @@ static void x86_cpu_initfn(Object *obj)
       object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", &error_abort);
       object_property_add_alias(obj, "i64", obj, "lm", &error_abort);

  +    object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", &error_abort);
  +    object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", &error_abort);
  +    object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", &error_abort);
  +    object_property_add_alias(obj, "lahf_lm", obj, "lahf-lm", &error_abort);
  +    object_property_add_alias(obj, "cmp_legacy", obj, "cmp-legacy", &error_abort);
  +    object_property_add_alias(obj, "nodeid_msr", obj, "nodeid-msr", &error_abort);
  +    object_property_add_alias(obj, "perfctr_core", obj, "perfctr-core", &error_abort);
  +    object_property_add_alias(obj, "perfctr_nb", obj, "perfctr-nb", &error_abort);
  +    object_property_add_alias(obj, "kvm_nopiodelay", obj, "kvm-nopiodelay", &error_abort);
  +    object_property_add_alias(obj, "kvm_mmu", obj, "kvm-mmu", &error_abort);
  +    object_property_add_alias(obj, "kvm_asyncpf", obj, "kvm-asyncpf", &error_abort);
  +    object_property_add_alias(obj, "kvm_steal_time", obj, "kvm-steal-time", &error_abort);
  +    object_property_add_alias(obj, "kvm_pv_eoi", obj, "kvm-pv-eoi", &error_abort);
  +    object_property_add_alias(obj, "kvm_pv_unhalt", obj, "kvm-pv-unhalt", &error_abort);
  +    object_property_add_alias(obj, "svm_lock", obj, "svm-lock", &error_abort);
  +    object_property_add_alias(obj, "nrip_save", obj, "nrip-save", &error_abort);
  +    object_property_add_alias(obj, "tsc_scale", obj, "tsc-scale", &error_abort);
  +    object_property_add_alias(obj, "vmcb_clean", obj, "vmcb-clean", &error_abort);
  +    object_property_add_alias(obj, "pause_filter", obj, "pause-filter", &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);
  +
       x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
   }

  diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
  index 7cff2b5..260dd27 100644
  --- a/tests/test-x86-cpuid-compat.c
  +++ b/tests/test-x86-cpuid-compat.c
  @@ -81,9 +81,14 @@ static void test_plus_minus(void)
       char *path;

       /* Rules:
  -     * "-foo" overrides "+foo"
  -     * "[+-]foo" overrides "foo=..."
  -     * "foo_bar" should be translated to "foo-bar"
  +     * 1)"-foo" overrides "+foo"
  +     * 2) "[+-]foo" overrides "foo=..."
  +     * 3) Old feature names with underscores (e.g. "sse4_2")
  +     *    should keep working
  +     *
  +     * Note: rules 1 and 2 are planned to be removed soon, but we
  +     * need to keep compatibility for a while until we start
  +     * warning users about it.
        */
       qtest_start("-cpu pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on");
       path = get_cpu0_qom_path();

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 (12):
  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: Register aliases for feature names with underscores
  target-i386: Make plus_features/minus_features QOM-based
  target-i386: Remove underscores from feat_names arrays
  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             | 510 ++++++++++++++++++++++++++----------------
 tests/test-x86-cpuid-compat.c |  44 ++++
 4 files changed, 388 insertions(+), 193 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v5 01/12] tests: Add test case for x86 feature parsing compatibility

[Eduardo Habkost]

Add a new test case to ensure the existing behavior of the
feature parsing code will be kept.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes series v4 -> v5:
* Fix typo on commit message
  Reported-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
* Add comment noting that the "[+-]feature" compatibility mode
  will be removed soon

Changes series v3 -> v4:
* New patch added to series
---
 tests/test-x86-cpuid-compat.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index 83162a4..260dd27 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,44 @@ 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:
+     * 1)"-foo" overrides "+foo"
+     * 2) "[+-]foo" overrides "foo=..."
+     * 3) Old feature names with underscores (e.g. "sse4_2")
+     *    should keep working
+     *
+     * Note: rules 1 and 2 are planned to be removed soon, but we
+     * need to keep compatibility for a while until we start
+     * warning users about it.
+     */
+    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

[Qemu-devel] [PATCH v5 02/12] target-i386: List CPU models using subclass list

[Eduardo Habkost]

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

[Qemu-devel] [PATCH v5 03/12] target-i386: Disable VME by default with TCG

[Eduardo Habkost]

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

[Qemu-devel] [PATCH v5 04/12] target-i386: Register aliases for feature names with underscores

[Eduardo Habkost]

Registering the actual names containing underscores as aliases
will allow management software to be aware that the old
compatibility names are suported, and will make feat2prop() calls
unnecessary when using feature names.

Also, this will help us avoid making the code support underscores
on feature names that never had them in the first place. e.g.
"+tsc_deadline" was never supported and doesn't need to be
translated to "+tsc-deadline".

In other word: this will require less magic translation of
strings, and simple 1:1 match between the config options and
actual QOM properties.

Note that the underscores are still present in the
FeatureWordInfo::feat_names arrays, because
add_flagname_to_bitmaps() needs them to be kept. The next patches
will remove add_flagname_to_bitmaps() and will allow us to
finally remove the aliases from feat_names.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3d3f64e..6b5bf59 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3455,6 +3455,28 @@ static void x86_cpu_initfn(Object *obj)
         }
     }
 
+    object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", &error_abort);
+    object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", &error_abort);
+    object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", &error_abort);
+    object_property_add_alias(obj, "lahf_lm", obj, "lahf-lm", &error_abort);
+    object_property_add_alias(obj, "cmp_legacy", obj, "cmp-legacy", &error_abort);
+    object_property_add_alias(obj, "nodeid_msr", obj, "nodeid-msr", &error_abort);
+    object_property_add_alias(obj, "perfctr_core", obj, "perfctr-core", &error_abort);
+    object_property_add_alias(obj, "perfctr_nb", obj, "perfctr-nb", &error_abort);
+    object_property_add_alias(obj, "kvm_nopiodelay", obj, "kvm-nopiodelay", &error_abort);
+    object_property_add_alias(obj, "kvm_mmu", obj, "kvm-mmu", &error_abort);
+    object_property_add_alias(obj, "kvm_asyncpf", obj, "kvm-asyncpf", &error_abort);
+    object_property_add_alias(obj, "kvm_steal_time", obj, "kvm-steal-time", &error_abort);
+    object_property_add_alias(obj, "kvm_pv_eoi", obj, "kvm-pv-eoi", &error_abort);
+    object_property_add_alias(obj, "kvm_pv_unhalt", obj, "kvm-pv-unhalt", &error_abort);
+    object_property_add_alias(obj, "svm_lock", obj, "svm-lock", &error_abort);
+    object_property_add_alias(obj, "nrip_save", obj, "nrip-save", &error_abort);
+    object_property_add_alias(obj, "tsc_scale", obj, "tsc-scale", &error_abort);
+    object_property_add_alias(obj, "vmcb_clean", obj, "vmcb-clean", &error_abort);
+    object_property_add_alias(obj, "pause_filter", obj, "pause-filter", &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);
+
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v5 05/12] target-i386: Make plus_features/minus_features QOM-based

[Eduardo Habkost]

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.

We don't need a feat2prop() call because we now have alias
properties for the old names containing underscores.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v4 -> v5:
* Removed feat2prop() call, as we now have property aliases for
  the old names containing underscores

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 106 +++++++++++-------------------------------------------
 1 file changed, 20 insertions(+), 86 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6b5bf59..cef4ecd 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,12 @@ 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);
+            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);
+            minus_features = g_list_append(minus_features,
+                                           g_strdup(featurestr + 1));
             continue;
         }
 
@@ -3066,6 +2988,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 +3014,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

[Qemu-devel] [PATCH v5 06/12] target-i386: Remove underscores from feat_names arrays

[Eduardo Habkost]

Instead of translating the feature name entries when adding
property names, store the actual property names in the feature
name array.

For reference, here is the full list of functions that use
FeatureWordInfo::feat_names:

* x86_cpu_get_migratable_flags(): not affected, as it just
  check for non-NULL values.
* report_unavailable_features(): informative only. It will
  start printing feature names with hyphens.
* x86_cpu_list(): informative only. It will start printing
  feature names with hyphens
* x86_cpu_register_feature_bit_props(): not affected, as it
  was already calling feat2prop(). Now we can remove the
  feat2prop() calls safely.

So, the only user-visible effect of this patch are the new names
being used in help and error messages for users.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v4 -> v5:
* Remove the feat2prop() call from the alias registration
  loop, too
* Commit message update to enumerate all code that uses
  feat_names

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cef4ecd..ec65aef 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,
@@ -3332,11 +3332,15 @@ 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 "_".
+     * Old names containing underscores are registered as aliases
+     * using object_property_add_alias()
+     */
+    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]);
+        assert(!strchr(names[i], '_'));
         object_property_add_alias(obj, names[i], obj, names[0],
                                   &error_abort);
     }
-- 
2.7.4

[Qemu-devel] [PATCH v5 07/12] target-i386: Register properties for feature aliases manually

[Eduardo Habkost]

Instead of keeping the aliases inside the feature name arrays and
require parsing the strings, just register alias properties
manually. This simplifies the code for property registration and
lookup.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v4 -> v5:
* Refresh after the previous patches were changed

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ec65aef..c018122 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,
@@ -3321,31 +3321,22 @@ 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 "_".
      * Old names containing underscores are registered as aliases
      * using object_property_add_alias()
      */
-    assert(!strchr(names[0], '_'));
-    x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr);
-
-    for (i = 1; names[i]; i++) {
-        assert(!strchr(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)
@@ -3393,6 +3384,14 @@ static void x86_cpu_initfn(Object *obj)
         }
     }
 
+    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);
+
     object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", &error_abort);
     object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", &error_abort);
     object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", &error_abort);
-- 
2.7.4

[Qemu-devel] [PATCH v5 08/12] target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas

[Eduardo Habkost]

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 c018122..477990d 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);
@@ -2961,8 +2975,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

[Qemu-devel] [PATCH v5 09/12] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

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 477990d..636e629 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2161,14 +2161,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 =
@@ -2176,15 +2173,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)
@@ -3080,12 +3084,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

[Qemu-devel] [PATCH v5 10/12] target-i386: x86_cpu_load_features() function

[Eduardo Habkost]

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 v4 -> v5:
* Fix typo on x86_cpu_load_features() comment
  Reported-by: Paolo Bonzini <pbonzini@redhat.com>

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 66 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 636e629..e4b45e3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2991,34 +2991,13 @@ 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 configured 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
@@ -3085,6 +3064,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

[Qemu-devel] [PATCH v5 11/12] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

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

[Qemu-devel] [PATCH v5 12/12] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

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 e4b45e3..620889f 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
@@ -2030,6 +2051,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)
@@ -2122,6 +2193,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

Re: [Qemu-devel] [libvirt] [PATCH v5 04/12] target-i386: Register aliases for feature names with underscores

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On 09/30/2016 01:49 PM, Eduardo Habkost wrote:
> Registering the actual names containing underscores as aliases
> will allow management software to be aware that the old
> compatibility names are suported, and will make feat2prop() calls

s/suported/supported/

> unnecessary when using feature names.
> 
> Also, this will help us avoid making the code support underscores
> on feature names that never had them in the first place. e.g.
> "+tsc_deadline" was never supported and doesn't need to be
> translated to "+tsc-deadline".
> 
> In other word: this will require less magic translation of
> strings, and simple 1:1 match between the config options and
> actual QOM properties.
> 
> Note that the underscores are still present in the
> FeatureWordInfo::feat_names arrays, because
> add_flagname_to_bitmaps() needs them to be kept. The next patches
> will remove add_flagname_to_bitmaps() and will allow us to
> finally remove the aliases from feat_names.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [libvirt] [PATCH v5 04/12] target-i386: Register aliases for feature names with underscores

[Eduardo Habkost]

On Fri, Sep 30, 2016 at 02:56:42PM -0500, Eric Blake wrote:
> On 09/30/2016 01:49 PM, Eduardo Habkost wrote:
> > Registering the actual names containing underscores as aliases
> > will allow management software to be aware that the old
> > compatibility names are suported, and will make feat2prop() calls
> 
> s/suported/supported/

Fixed, thanks.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 00/12] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

On Fri, Sep 30, 2016 at 03:49:34PM -0300, Eduardo Habkost wrote:
[...]
> Eduardo Habkost (12):
>   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: Register aliases for feature names with underscores
>   target-i386: Make plus_features/minus_features QOM-based
>   target-i386: Remove underscores from feat_names arrays
>   target-i386: Register properties for feature aliases manually
>   target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas

Patches 1-8 (above) were applied to x86-next. I will send v6 of
9-12 (below), with a small change.

>   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             | 510 ++++++++++++++++++++++++++----------------
>  tests/test-x86-cpuid-compat.c |  44 ++++
>  4 files changed, 388 insertions(+), 193 deletions(-)
> 
-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 11/12] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Fri, Sep 30, 2016 at 03:49:45PM -0300, Eduardo Habkost wrote:
> 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>

Applied this to x86-next.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 05/12] target-i386: Make plus_features/minus_features QOM-based

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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.
>
> We don't need a feat2prop() call because we now have alias
> properties for the old names containing underscores.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v4 -> v5:
> * Removed feat2prop() call, as we now have property aliases for
>   the old names containing underscores
>
> Changes series v3 -> v4:
> * New patch added to series
> ---
>  target-i386/cpu.c | 106 +++++++++++-------------------------------------------
>  1 file changed, 20 insertions(+), 86 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 6b5bf59..cef4ecd 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
[...]
> @@ -2047,10 +1967,12 @@ 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);
> +            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);
> +            minus_features = g_list_append(minus_features,
> +                                           g_strdup(featurestr + 1));
>              continue;
>          }
>  

Since removes the only assignments to local_err other than its
initialization to null, it can't become non-null anymore.  Coverity
points out:

    *** CID 1365201:  Possible Control flow issues  (DEADCODE)
    /target-i386/cpu.c: 2050 in x86_cpu_parse_featurestr()
    2044             prop->value = g_strdup(val);
    2045             prop->errp = &error_fatal;
    2046             qdev_prop_register_global(prop);
    2047         }
    2048     
    2049         if (local_err) {
    >>>     CID 1365201:  Possible Control flow issues  (DEADCODE)
    >>>     Execution cannot reach this statement: "error_propagate(errp, local...".
    2050             error_propagate(errp, local_err);
    2051         }
    2052     }
    2053     
    2054     static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
    2055     static int x86_cpu_filter_features(X86CPU *cpu);

[...]

Please clean this up.