[Qemu-devel] [PATCH 00/22 v3] target-i386: convert CPU features into properties

[Qemu-devel] [PATCH 00/22 v3] target-i386: convert CPU features into properties

[Igor Mammedov]

git tree for testing:
  https://github.com/imammedo/qemu/tree/x86-cpu-properties.v3

Depends on:
  [Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names
    http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html

Rebased on top of todays qemu tree.

Igor Mammedov (22):
  target-i386: return Error from cpu_x86_find_by_name()
  target-i386: cpu_x86_register(): report error from property setter
  target-i386: if x86_cpu_realize() failed report error and do cleanup
  target-i386: filter out not TCG features if running without kvm at
    realize time
  target-i386: move out CPU features initialization in separate func
  target-i386: xlevel should be more than 0x80000000, move fixup into
    setter
  target-i386: convert cpuid features into properties
  target-i386: add stubs for
    hyperv_(vapic_recommended|relaxed_timing_enabled|get_spinlock_retries)()
  target-i386: convert 'hv_spinlocks' feature into property
  target-i386: convert 'hv_relaxed' feature into property
  target-i386: convert 'hv_vapic' feature into property
  target-i386: convert 'check' and 'enforce' features into properties
  add visitor for parsing hz[KMG] input string
  target-i386: use visit_type_hz to parse tsc_freq property value
  target-i386: introduce vendor-override property
  target-i386: use define for cpuid vendor string size
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: parse cpu_model string into set of stringified
    properties
  target-i386: use properties to set/unset user specified features on
    CPU
  target-i386: move init of "hypervisor" feature into CPU initializer
    from cpudef
  target-i386: move default init of cpuid_kvm_features bitmap into CPU
    initializer from cpudef
  target-i386: cleanup cpu_x86_find_by_name(), only fill x86_def_t in
    it

 qapi/qapi-visit-core.c      |   11 +
 qapi/qapi-visit-core.h      |    2 +
 qapi/string-input-visitor.c |   22 ++
 target-i386/cpu.c           |  733 ++++++++++++++++++++++++++-----------------
 target-i386/cpu.h           |   10 +-
 target-i386/helper.c        |    9 +-
 target-i386/hyperv.h        |    9 +-
 7 files changed, 507 insertions(+), 289 deletions(-)

[Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Igor Mammedov]

it will allow to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Don Slutz <Don@CloudSwitch.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
v2:
    - style change, add braces (reqested by Blue Swirl)
    - removed unused error_is_set(errp) in properties set loop
---
 target-i386/cpu.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c4e6163..e1e2346 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1241,6 +1242,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1264,6 +1266,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1350,8 +1355,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1401,6 +1408,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;
-- 
1.7.1

[Qemu-devel] [PATCH 02/22] target-i386: cpu_x86_register(): report error from property setter

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 v2:
   - replace "if (error_is_set(&error))" with "if (error)"
---
 target-i386/cpu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e1e2346..8d3edbf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1410,7 +1410,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
 
 out:
-    if (error_is_set(&error)) {
+    if (error) {
+        fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
         return -1;
     }
-- 
1.7.1

[Qemu-devel] [PATCH 03/22] target-i386: if x86_cpu_realize() failed report error and do cleanup

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Andreas Färber <afaerber@suse.de>
---
 v2:
   - replaced "if (error_is_set(&error))" with "if (error)"
---
 target-i386/helper.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 8a5da3d..3038198 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1151,6 +1151,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
     CPUX86State *env;
+    Error *error = NULL;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
@@ -1161,8 +1162,12 @@ X86CPU *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
 
-    x86_cpu_realize(OBJECT(cpu), NULL);
-
+    x86_cpu_realize(OBJECT(cpu), &error);
+    if (error) {
+        error_free(error);
+        object_delete(OBJECT(cpu));
+        return NULL;
+    }
     return cpu;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 04/22] target-i386: filter out not TCG features if running without kvm at realize time

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8d3edbf..f9900af 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1396,17 +1396,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
     }
 
-    if (!kvm_enabled()) {
-        env->cpuid_features &= TCG_FEATURES;
-        env->cpuid_ext_features &= TCG_EXT_FEATURES;
-        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
-#ifdef TARGET_X86_64
-            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
-#endif
-            );
-        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
-        env->cpuid_svm_features &= TCG_SVM_FEATURES;
-    }
+
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
 
 out:
@@ -1865,6 +1855,19 @@ static void mce_init(X86CPU *cpu)
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+
+    if (!kvm_enabled()) {
+        env->cpuid_features &= TCG_FEATURES;
+        env->cpuid_ext_features &= TCG_EXT_FEATURES;
+        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
+#ifdef TARGET_X86_64
+            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
+#endif
+            );
+        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
+        env->cpuid_svm_features &= TCG_SVM_FEATURES;
+    }
 
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
-- 
1.7.1

[Qemu-devel] [PATCH 05/22] target-i386: move out CPU features initialization in separate func

[Igor Mammedov]

later it could be used in cpu_x86_find_by_name() to init
CPU from found cpu_def

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
v2:
   - rebased on top of  "i386: cpu: remove duplicate feature names"
      http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
---
 target-i386/cpu.c |   84 +++++++++++++++++++++++++++-------------------------
 1 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f9900af..50efe5f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,6 +1086,49 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
+{
+    CPUX86State *env = &cpu->env;
+
+    if (def->vendor1) {
+        env->cpuid_vendor1 = def->vendor1;
+        env->cpuid_vendor2 = def->vendor2;
+        env->cpuid_vendor3 = def->vendor3;
+    } else {
+        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
+        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
+        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
+    }
+    env->cpuid_vendor_override = def->vendor_override;
+    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
+    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
+    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
+    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
+    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
+    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
+                            "tsc-frequency", errp);
+    env->cpuid_features = def->features;
+    env->cpuid_ext_features = def->ext_features;
+    env->cpuid_ext2_features = def->ext2_features;
+    env->cpuid_ext3_features = def->ext3_features;
+    env->cpuid_kvm_features = def->kvm_features;
+    env->cpuid_svm_features = def->svm_features;
+    env->cpuid_ext4_features = def->ext4_features;
+    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
+    env->cpuid_xlevel2 = def->xlevel2;
+
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
+    }
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
@@ -1349,7 +1392,6 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
-    CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
 
@@ -1359,45 +1401,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto out;
     }
 
-    if (def->vendor1) {
-        env->cpuid_vendor1 = def->vendor1;
-        env->cpuid_vendor2 = def->vendor2;
-        env->cpuid_vendor3 = def->vendor3;
-    } else {
-        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
-        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
-        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
-    }
-    env->cpuid_vendor_override = def->vendor_override;
-    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
-    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
-    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
-    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
-    env->cpuid_features = def->features;
-    env->cpuid_ext_features = def->ext_features;
-    env->cpuid_ext2_features = def->ext2_features;
-    env->cpuid_ext3_features = def->ext3_features;
-    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
-    env->cpuid_kvm_features = def->kvm_features;
-    env->cpuid_svm_features = def->svm_features;
-    env->cpuid_ext4_features = def->ext4_features;
-    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
-    env->cpuid_xlevel2 = def->xlevel2;
-    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
-                            "tsc-frequency", &error);
-
-    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-     * CPUID[1].EDX.
-     */
-    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
-            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
-            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
-        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
-        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
-    }
-
-
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    cpudef_2_x86_cpu(cpu, def, &error);
 
 out:
     if (error) {
-- 
1.7.1

[Qemu-devel] [PATCH 06/22] target-i386: xlevel should be more than 0x80000000, move fixup into setter

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 50efe5f..a713960 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -973,8 +973,17 @@ static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque,
                                  const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
+    uint32_t value;
 
-    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
+    visit_type_uint32(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (value < 0x80000000) {
+        value += 0x80000000;
+    }
+    cpu->env.cpuid_xlevel = value;
 }
 
 static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
@@ -1229,9 +1238,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                     fprintf(stderr, "bad numerical value %s\n", val);
                     goto error;
                 }
-                if (numvalue < 0x80000000) {
-                    numvalue += 0x80000000;
-                }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
                 if (strlen(val) != 12) {
-- 
1.7.1

[Qemu-devel] [PATCH 07/22] target-i386: convert cpuid features into properties

[Igor Mammedov]

add property accessors for cpuid feature bits defined by
*_feature_name arrays.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
    as suggested by Eduardo Habkost
v3:
  * check if property exists before adding it
  * rebased on top of  "i386: cpu: remove duplicate feature names"
          http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
    place ext2_feature_name for AMD case into setter, so that not to
    clutter x86_cpu_realize() with property specific code.
  * fix for convert cpuid features
---
 target-i386/cpu.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 114 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a713960..213368b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -833,6 +833,114 @@ static int check_features_against_host(x86_def_t *guest_def)
     return rv;
 }
 
+static bool is_feature_set(const char *name, const uint32_t featbitmap,
+                                  const char **featureset)
+{
+    uint32_t bit;
+
+    for (bit = 0; bit < 32; ++bit) {
+        if (featureset[bit] && !altcmp(name, NULL, featureset[bit])) {
+            if (featbitmap & (1 << bit)) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    bool value = true;
+
+    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
+       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
+       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
+       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
+       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
+       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
+        value = false;
+    }
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    uint32_t mask = 0;
+    uint32_t *dst_features;
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (lookup_feature(&mask, name, NULL, feature_name)) {
+        dst_features = &env->cpuid_features;
+    } else if (lookup_feature(&mask, name, NULL, ext_feature_name)) {
+        dst_features = &env->cpuid_ext_features;
+    } else if (lookup_feature(&mask, name, NULL, ext2_feature_name)) {
+        dst_features = &env->cpuid_ext2_features;
+    } else if (lookup_feature(&mask, name, NULL, ext3_feature_name)) {
+        dst_features = &env->cpuid_ext3_features;
+    } else if (lookup_feature(&mask, name, NULL, kvm_feature_name)) {
+        dst_features = &env->cpuid_kvm_features;
+    } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) {
+        dst_features = &env->cpuid_svm_features;
+    } else {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
+        return;
+    }
+
+    if (value) {
+        *dst_features |= mask;
+    } else {
+        *dst_features &= ~mask;
+    }
+
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (dst_features == &env->cpuid_features &&
+            env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= *dst_features & CPUID_EXT2_AMD_ALIASES;
+    }
+}
+
+static void x86_register_cpuid_properties(Object *obj, const char **featureset)
+{
+    uint32_t bit;
+
+    for (bit = 0; bit < 32; ++bit) {
+        if (featureset[bit]) {
+            char *feature_name, *save_ptr;
+            char buf[32];
+            if (strlen(featureset[bit]) > sizeof(buf) - 1) {
+                abort();
+            }
+            pstrcpy(buf, sizeof(buf), featureset[bit]);
+            feature_name = strtok_r(buf, "|", &save_ptr);
+            while (feature_name) {
+                if (!object_property_find(obj, feature_name, NULL)) {
+                    object_property_add(obj, feature_name, "bool",
+                                    x86_cpuid_get_feature,
+                                    x86_cpuid_set_feature, NULL, NULL, NULL);
+                }
+                feature_name = strtok_r(NULL, "|", &save_ptr);
+            }
+        }
+    }
+}
+
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
 {
@@ -1920,6 +2028,12 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    x86_register_cpuid_properties(obj, feature_name);
+    x86_register_cpuid_properties(obj, ext_feature_name);
+    x86_register_cpuid_properties(obj, ext2_feature_name);
+    x86_register_cpuid_properties(obj, ext3_feature_name);
+    x86_register_cpuid_properties(obj, kvm_feature_name);
+    x86_register_cpuid_properties(obj, svm_feature_name);
 
     env->cpuid_apic_id = env->cpu_index;
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 07/22] target-i386: convert cpuid features into properties

[H. Peter Anvin]

On 09/26/2012 01:32 PM, Igor Mammedov wrote:
> +
> +static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
> +                                         const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    bool value = true;
> +
> +    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
> +       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
> +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
> +        value = false;
> +    }
> +

If you're going to do a full-blown restructuring of the CPUID handling,
how about actually turning this into a composite object, specifically an
array, instead of this insane collection of arguments (which needs to be
augmented)?

	-hpa

Re: [Qemu-devel] [PATCH 07/22] target-i386: convert cpuid features into properties

[Eduardo Habkost]

On Wed, Sep 26, 2012 at 02:31:25PM -0700, H. Peter Anvin wrote:
> On 09/26/2012 01:32 PM, Igor Mammedov wrote:
> > +
> > +static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
> > +                                         const char *name, Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    CPUX86State *env = &cpu->env;
> > +    bool value = true;
> > +
> > +    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
> > +       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
> > +       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
> > +       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
> > +       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
> > +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
> > +        value = false;
> > +    }
> > +
> 
> If you're going to do a full-blown restructuring of the CPUID handling,
> how about actually turning this into a composite object, specifically an
> array, instead of this insane collection of arguments (which needs to be
> augmented)?

I have a series that does that, based on this one, that I will clean up
and submit after the other changes get in.

https://github.com/ehabkost/qemu-hacks/commits/work/cpuid-refactor-v0.22-2012-08-31

-- 
Eduardo

[Qemu-devel] [PATCH 07/22 v4] target-i386: convert cpuid features into properties

[Igor Mammedov]

add property accessors for cpuid feature bits defined by
*_feature_name arrays.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
    as suggested by Eduardo Habkost
v3:
  * check if property exists before adding it
  * rebased on top of  "i386: cpu: remove duplicate feature names"
          http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
    place ext2_feature_name for AMD case into setter, so that not to
    clutter x86_cpu_realize() with property specific code.
  * fix for convert cpuid features
v4:
  * Mingw doesn't have strtok_r(), use g_strsplit() instead.
---
 target-i386/cpu.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 111 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a713960..f990be2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -833,6 +833,111 @@ static int check_features_against_host(x86_def_t *guest_def)
     return rv;
 }
 
+static bool is_feature_set(const char *name, const uint32_t featbitmap,
+                                  const char **featureset)
+{
+    uint32_t bit;
+
+    for (bit = 0; bit < 32; ++bit) {
+        if (featureset[bit] && !altcmp(name, NULL, featureset[bit])) {
+            if (featbitmap & (1 << bit)) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    bool value = true;
+
+    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
+       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
+       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
+       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
+       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
+       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
+        value = false;
+    }
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    uint32_t mask = 0;
+    uint32_t *dst_features;
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (lookup_feature(&mask, name, NULL, feature_name)) {
+        dst_features = &env->cpuid_features;
+    } else if (lookup_feature(&mask, name, NULL, ext_feature_name)) {
+        dst_features = &env->cpuid_ext_features;
+    } else if (lookup_feature(&mask, name, NULL, ext2_feature_name)) {
+        dst_features = &env->cpuid_ext2_features;
+    } else if (lookup_feature(&mask, name, NULL, ext3_feature_name)) {
+        dst_features = &env->cpuid_ext3_features;
+    } else if (lookup_feature(&mask, name, NULL, kvm_feature_name)) {
+        dst_features = &env->cpuid_kvm_features;
+    } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) {
+        dst_features = &env->cpuid_svm_features;
+    } else {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
+        return;
+    }
+
+    if (value) {
+        *dst_features |= mask;
+    } else {
+        *dst_features &= ~mask;
+    }
+
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (dst_features == &env->cpuid_features &&
+            env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= *dst_features & CPUID_EXT2_AMD_ALIASES;
+    }
+}
+
+static void x86_register_cpuid_properties(Object *obj, const char **featureset)
+{
+    uint32_t bit;
+
+    for (bit = 0; bit < 32; ++bit) {
+        if (featureset[bit]) {
+            char **feature_name;
+            int i;
+
+            feature_name = g_strsplit(featureset[bit], "|", 0);
+            for (i = 0; feature_name[i]; ++i) {
+                if (!object_property_find(obj, feature_name[i], NULL)) {
+                    object_property_add(obj, feature_name[i], "bool",
+                                    x86_cpuid_get_feature,
+                                    x86_cpuid_set_feature, NULL, NULL, NULL);
+                }
+            }
+            g_strfreev(feature_name);
+        }
+    }
+}
+
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
 {
@@ -1920,6 +2025,12 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    x86_register_cpuid_properties(obj, feature_name);
+    x86_register_cpuid_properties(obj, ext_feature_name);
+    x86_register_cpuid_properties(obj, ext2_feature_name);
+    x86_register_cpuid_properties(obj, ext3_feature_name);
+    x86_register_cpuid_properties(obj, kvm_feature_name);
+    x86_register_cpuid_properties(obj, svm_feature_name);
 
     env->cpuid_apic_id = env->cpu_index;
 
-- 
1.7.1

[Qemu-devel] [PATCH 18/22 v3] target-i386: parse cpu_model string into set of stringified properties

[Igor Mammedov]

cpu_model string does represent features in following format:
 ([+-]feat)|(feat=foo)|(feat)
which makes it impossible directly use property infrastructure
to set features on CPU.
This patch introduces parser that splits CPU name from cpu_model and
converts legacy features string into canonized set of strings that
is compatible with property manipulation infrastructure.

PS:
  * later it could be used as a hook to convert legacy command line
    features to global properties. Then marked as deprecated and
    removed with -cpu option in the future.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v2:
  * compiler complains that it's unused function but I guess it is
    easier for review this way, for pull req I'll squash it into next
    patch
  * fix spelling error
  * initialize sptr, due to a CentOS6 compiler warning, that
    breakes build when -Werror is set. Suggested-by: Don Slutz
 v3:
  * Mingw doesn't have strtok_r(), use g_strsplit() instead of it.
        Suggested-by: Blue Swirl
---
 target-i386/cpu.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ad1f088..9cf25f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1339,6 +1339,57 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     }
 }
 
+/* convert legacy cpumodel string to string cpu_name and
+ * a uniform set of custom features that will be applied to CPU
+ * using object_property_parse()
+ */
+static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
+                                        QDict **features, Error **errp)
+{
+    int i;
+    gchar **feat_array = g_strsplit(cpu_model, ",", 0);
+    *features = qdict_new();
+
+    g_assert(feat_array[0] != NULL);
+    *cpu_name = g_strdup(feat_array[0]);
+
+    for (i = 1; feat_array[i]; ++i) {
+        gchar *featurestr = feat_array[i];
+        char *val;
+        if (featurestr[0] == '+') {
+            /*
+             * preseve legacy behaviour, if feature was disabled once
+             * do not allow to enable it again
+             */
+            if (!qdict_haskey(*features, featurestr + 1)) {
+                qdict_put(*features, featurestr + 1, qstring_from_str("on"));
+            }
+        } else if (featurestr[0] == '-') {
+            qdict_put(*features, featurestr + 1, qstring_from_str("off"));
+        } else {
+            val = strchr(featurestr, '=');
+            if (val) {
+                *val = 0; val++;
+                if (!strcmp(featurestr, "vendor")) {
+                    qdict_put(*features, "vendor-override",
+                              qstring_from_str("on"));
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                } else if (!strcmp(featurestr, "tsc_freq")) {
+                    qdict_put(*features, "tsc-frequency",
+                              qstring_from_str(val));
+                } else {
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                }
+            } else {
+                qdict_put(*features, featurestr, qstring_from_str("on"));
+            }
+        }
+    }
+
+    g_strfreev(feat_array);
+    return;
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
-- 
1.7.1

[Qemu-devel] [PATCH 08/22] target-i386: add stubs for hyperv_(vapic_recommended|relaxed_timing_enabled|get_spinlock_retries)()

[Igor Mammedov]

It's needed for the next 3 patches to avoid build breakage when qemu is built
with --disable-kvm option.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/hyperv.h |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-i386/hyperv.h b/target-i386/hyperv.h
index bacb1d4..7bd4d9e 100644
--- a/target-i386/hyperv.h
+++ b/target-i386/hyperv.h
@@ -30,16 +30,19 @@
 void hyperv_enable_vapic_recommended(bool val);
 void hyperv_enable_relaxed_timing(bool val);
 void hyperv_set_spinlock_retries(int val);
+bool hyperv_vapic_recommended(void);
+bool hyperv_relaxed_timing_enabled(void);
+int hyperv_get_spinlock_retries(void);
 #else
 static inline void hyperv_enable_vapic_recommended(bool val) { }
 static inline void hyperv_enable_relaxed_timing(bool val) { }
 static inline void hyperv_set_spinlock_retries(int val) { }
+static inline bool hyperv_vapic_recommended(void) { return false; }
+static inline bool hyperv_relaxed_timing_enabled(void) { return false; }
+static inline int  hyperv_get_spinlock_retries(void) { return 0; }
 #endif
 
 bool hyperv_enabled(void);
 bool hyperv_hypercall_available(void);
-bool hyperv_vapic_recommended(void);
-bool hyperv_relaxed_timing_enabled(void);
-int hyperv_get_spinlock_retries(void);
 
 #endif /* QEMU_HW_HYPERV_H */
-- 
1.7.1

[Qemu-devel] [PATCH 09/22] target-i386: convert 'hv_spinlocks' feature into property

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 213368b..b215b53 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1203,6 +1203,32 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+#if !defined(CONFIG_USER_ONLY)
+static void x86_get_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    int64_t value = hyperv_get_spinlock_retries();
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void x86_set_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    int64_t value;
+
+    visit_type_int(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (!value) {
+        error_set(errp, QERR_PROPERTY_VALUE_BAD, "", name, "0");
+        return;
+    }
+    hyperv_set_spinlock_retries(value);
+}
+#endif
+
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
@@ -2028,6 +2054,11 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+#if !defined(CONFIG_USER_ONLY)
+    object_property_add(obj, "hv_spinlocks", "int",
+                        x86_get_hv_spinlocks,
+                        x86_set_hv_spinlocks, NULL, NULL, NULL);
+#endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
     x86_register_cpuid_properties(obj, ext2_feature_name);
-- 
1.7.1

[Qemu-devel] [PATCH 10/22] target-i386: convert 'hv_relaxed' feature into property

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b215b53..079a8e7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1227,6 +1227,26 @@ static void x86_set_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
     }
     hyperv_set_spinlock_retries(value);
 }
+
+static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value = hyperv_relaxed_timing_enabled();
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    hyperv_enable_relaxed_timing(value);
+}
 #endif
 
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
@@ -2058,6 +2078,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "hv_spinlocks", "int",
                         x86_get_hv_spinlocks,
                         x86_set_hv_spinlocks, NULL, NULL, NULL);
+    object_property_add(obj, "hv_relaxed", "bool",
+                        x86_get_hv_relaxed,
+                        x86_set_hv_relaxed, NULL, NULL, NULL);
 #endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
-- 
1.7.1

[Qemu-devel] [PATCH 11/22] target-i386: convert 'hv_vapic' feature into property

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 079a8e7..42f06bf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1247,6 +1247,26 @@ static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
     }
     hyperv_enable_relaxed_timing(value);
 }
+
+static void x86_get_hv_vapic(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value = hyperv_vapic_recommended();
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    hyperv_enable_vapic_recommended(value);
+}
 #endif
 
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
@@ -2081,6 +2101,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "hv_relaxed", "bool",
                         x86_get_hv_relaxed,
                         x86_set_hv_relaxed, NULL, NULL, NULL);
+    object_property_add(obj, "hv_vapic", "bool",
+                        x86_set_hv_vapic,
+                        x86_get_hv_vapic, NULL, NULL, NULL);
 #endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
-- 
1.7.1

[Qemu-devel] [PATCH 12/22] target-i386: convert 'check' and 'enforce' features into properties

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
--
v2:
  * restore original behavior, check features against host before
    they might be filtered out by TCG masks. spotted-by: Eduardo Habkost
---
 target-i386/cpu.c |   68 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 42f06bf..e663ec1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -114,8 +114,8 @@ typedef struct model_features_t {
     uint32_t cpuid;
     } model_features_t;
 
-int check_cpuid = 0;
-int enforce_cpuid = 0;
+bool check_cpuid;
+bool enforce_cpuid;
 
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
@@ -807,19 +807,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
  * their way to the guest.  Note: ft[].check_feat ideally should be
  * specified via a guest_def field to suppress report of extraneous flags.
  */
-static int check_features_against_host(x86_def_t *guest_def)
+static int check_features_against_host(X86CPU *cpu)
 {
+    CPUX86State *env = &cpu->env;
     x86_def_t host_def;
     uint32_t mask;
     int rv, i;
     struct model_features_t ft[] = {
-        {&guest_def->features, &host_def.features,
+        {&env->cpuid_features, &host_def.features,
             ~0, feature_name, 0x00000000},
-        {&guest_def->ext_features, &host_def.ext_features,
+        {&env->cpuid_ext_features, &host_def.ext_features,
             ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
-        {&guest_def->ext2_features, &host_def.ext2_features,
+        {&env->cpuid_ext2_features, &host_def.ext2_features,
             ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
-        {&guest_def->ext3_features, &host_def.ext3_features,
+        {&env->cpuid_ext3_features, &host_def.ext3_features,
             ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
 
     cpu_x86_fill_host(&host_def);
@@ -1269,6 +1270,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque,
 }
 #endif
 
+static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    visit_type_bool(v, &check_cpuid, name, errp);
+}
+
+static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    check_cpuid = value;
+}
+
+static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    visit_type_bool(v, &enforce_cpuid, name, errp);
+}
+
+static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    enforce_cpuid = value;
+    object_property_set_bool(obj, value, "check", errp);
+}
+
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
@@ -1480,10 +1518,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     x86_cpu_def->ext3_features &= ~minus_ext3_features;
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
-    if (check_cpuid) {
-        if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
-            goto error;
-    }
     g_free(s);
     return 0;
 
@@ -2041,6 +2075,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
 
+    if (check_cpuid && check_features_against_host(cpu)
+        && enforce_cpuid) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
@@ -2094,6 +2134,12 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    object_property_add(obj, "check", "bool",
+                        x86_cpuid_get_check,
+                        x86_cpuid_set_check, NULL, NULL, NULL);
+    object_property_add(obj, "enforce", "bool",
+                        x86_cpuid_get_enforce,
+                        x86_cpuid_set_enforce, NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
     object_property_add(obj, "hv_spinlocks", "int",
                         x86_get_hv_spinlocks,
-- 
1.7.1

[Qemu-devel] [PATCH 13/22] add visitor for parsing hz[KMG] input string

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Andreas Färber <afaerber@suse.de>
---
v2:
  * replaced _hz suffix for frequency visitor by _freq suffix
    suggested-by: Andreas Färber
  * fixed typo & extra space spotted-by: Andreas Färber
  * initialize val, due to a silly CentOS6 compiler warning, that
    breakes build when -Werror is set. suggested-by: Don Slutz
---
 qapi/qapi-visit-core.c      |   11 +++++++++++
 qapi/qapi-visit-core.h      |    2 ++
 qapi/string-input-visitor.c |   22 ++++++++++++++++++++++
 3 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 7a82b63..5c8705e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -311,3 +311,14 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[],
     g_free(enum_str);
     *obj = value;
 }
+
+void visit_type_freq(Visitor *v, int64_t *obj, const char *name, Error **errp)
+{
+    if (!error_is_set(errp)) {
+        if (v->type_freq) {
+            v->type_freq(v, obj, name, errp);
+        } else {
+            v->type_int(v, obj, name, errp);
+        }
+    }
+}
diff --git a/qapi/qapi-visit-core.h b/qapi/qapi-visit-core.h
index 60aceda..e5e7dd7 100644
--- a/qapi/qapi-visit-core.h
+++ b/qapi/qapi-visit-core.h
@@ -62,6 +62,7 @@ struct Visitor
     void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
     /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
     void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+    void (*type_freq)(Visitor *v, int64_t *obj, const char *name, Error **errp);
 };
 
 void visit_start_handle(Visitor *v, void **obj, const char *kind,
@@ -91,5 +92,6 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
 void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
+void visit_type_freq(Visitor *v, int64_t *obj, const char *name, Error **errp);
 
 #endif
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 497eb9a..74fe395 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -110,6 +110,27 @@ static void parse_start_optional(Visitor *v, bool *present,
     *present = true;
 }
 
+static void parse_type_freq(Visitor *v, int64_t *obj, const char *name,
+                            Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    char *endp = (char *) siv->string;
+    long long val = 0;
+
+    errno = 0;
+    if (siv->string) {
+        val = strtosz_suffix_unit(siv->string, &endp,
+                             STRTOSZ_DEFSUFFIX_B, 1000);
+    }
+    if (!siv->string || val == -1 || *endp) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
+              "a value representable as a non-negative int64");
+        return;
+    }
+
+    *obj = val;
+}
+
 Visitor *string_input_get_visitor(StringInputVisitor *v)
 {
     return &v->visitor;
@@ -132,6 +153,7 @@ StringInputVisitor *string_input_visitor_new(const char *str)
     v->visitor.type_str = parse_type_str;
     v->visitor.type_number = parse_type_number;
     v->visitor.start_optional = parse_start_optional;
+    v->visitor.type_freq = parse_type_freq;
 
     v->string = str;
     return v;
-- 
1.7.1

[Qemu-devel] [PATCH 14/22] target-i386: use visit_type_hz to parse tsc_freq property value

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
v2:
  * use visit_type_freq() which replaced visit_type_hz()
---
 target-i386/cpu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e663ec1..d36410a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1191,7 +1191,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     const int64_t max = INT64_MAX;
     int64_t value;
 
-    visit_type_int(v, &value, name, errp);
+    visit_type_freq(v, &value, name, errp);
     if (error_is_set(errp)) {
         return;
     }
-- 
1.7.1

[Qemu-devel] [PATCH 15/22] target-i386: introduce vendor-override property

[Igor Mammedov]

currently 'cpuid_vendor_override' can be set only via cmd line cpu_model
string. But setting it in 'vendor' property prevents using 'vendor'
property on its own without setting cpuid_vendor_override.

So fix/remove enabling cpuid_vendor_override from "vendor" property setter.
It's up-to cpu_model string parser to maintain legacy behavior when user
overrides vendor on command line.

v2:
  - convert cpuid_vendor_override to bool to reflect its real usage

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   31 +++++++++++++++++++++++++++++--
 target-i386/cpu.h |    2 +-
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d36410a..4f333e9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1133,7 +1133,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
         env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
         env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
     }
-    env->cpuid_vendor_override = 1;
 }
 
 static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
@@ -1307,6 +1306,31 @@ static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque,
     object_property_set_bool(obj, value, "check", errp);
 }
 
+static void
+x86_cpuid_get_vendor_override(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+
+    visit_type_bool(v, &env->cpuid_vendor_override, name, errp);
+}
+
+static void
+x86_cpuid_set_vendor_override(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    env->cpuid_vendor_override = value;
+}
+
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
@@ -1320,7 +1344,7 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
         env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
         env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
     }
-    env->cpuid_vendor_override = def->vendor_override;
+    object_property_set_bool(OBJECT(cpu), true, "vendor-override", errp);
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
     object_property_set_int(OBJECT(cpu), def->model, "model", errp);
@@ -2128,6 +2152,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_str(obj, "vendor",
                             x86_cpuid_get_vendor,
                             x86_cpuid_set_vendor, NULL);
+    object_property_add(obj, "vendor-override", "bool",
+                        x86_cpuid_get_vendor_override,
+                        x86_cpuid_set_vendor_override, NULL, NULL, NULL);
     object_property_add_str(obj, "model-id",
                             x86_cpuid_get_model_id,
                             x86_cpuid_set_model_id, NULL);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 4995084..4f525ee 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -774,7 +774,7 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
+    bool cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.1

[Qemu-devel] [PATCH 16/22] target-i386: use define for cpuid vendor string size

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |    6 +++---
 target-i386/cpu.h |    2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4f333e9..5c7c5a5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1102,13 +1102,13 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
     char *value;
     int i;
 
-    value = (char *)g_malloc(12 + 1);
+    value = (char *)g_malloc(CPUID_VENDOR_SZ + 1);
     for (i = 0; i < 4; i++) {
         value[i    ] = env->cpuid_vendor1 >> (8 * i);
         value[i + 4] = env->cpuid_vendor2 >> (8 * i);
         value[i + 8] = env->cpuid_vendor3 >> (8 * i);
     }
-    value[12] = '\0';
+    value[CPUID_VENDOR_SZ] = '\0';
     return value;
 }
 
@@ -1119,7 +1119,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
     CPUX86State *env = &cpu->env;
     int i;
 
-    if (strlen(value) != 12) {
+    if (strlen(value) != CPUID_VENDOR_SZ) {
         error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
                   "vendor", value);
         return;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 4f525ee..b6bcdf1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -474,6 +474,8 @@
 #define CPUID_SVM_PAUSEFILTER  (1 << 10)
 #define CPUID_SVM_PFTHRESHOLD  (1 << 12)
 
+#define CPUID_VENDOR_SZ      12
+
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
-- 
1.7.1

[Qemu-devel] [PATCH 17/22] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

Vendor property setter takes string as vendor value but cpudefs
use uint32_t vendor[123] fields to define vendor value. It makes it
difficult to unify and use property setter for values from cpudefs.

Simplify code by using vendor property setter, vendor[123] fields
are converted into vendor[13] array to keep its value. And vendor
property setter is used to access/set value on CPU.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
[ehabkost: rebase on top of my unduplicate-features branch]
[ehabkost: fix the new CPU models to use the string .vendor field, too,
 on the CPU model array]
[ehabkost: keep CPUID_VENDOR_AMD_[123] #defines, as they are used
 in the AMD CPU feature alias handling]
---
 target-i386/cpu.c |   92 ++++++++++++++---------------------------------------
 target-i386/cpu.h |    6 ++--
 2 files changed, 27 insertions(+), 71 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5c7c5a5..b7a9cd6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -234,7 +234,7 @@ typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
     uint32_t level;
-    uint32_t vendor1, vendor2, vendor3;
+    char vendor[CPUID_VENDOR_SZ + 1];
     int family;
     int model;
     int stepping;
@@ -298,9 +298,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu64",
         .level = 4,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -317,9 +315,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "phenom",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 16,
         .model = 2,
         .stepping = 3,
@@ -363,9 +359,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm64",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -464,9 +458,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "athlon",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -498,9 +490,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Conroe",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -518,9 +508,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Penryn",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -539,9 +527,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Nehalem",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -560,9 +546,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Westmere",
         .level = 11,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 44,
         .stepping = 1,
@@ -582,9 +566,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "SandyBridge",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 42,
         .stepping = 1,
@@ -607,9 +589,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G1",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -631,9 +611,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G2",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -657,9 +635,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G3",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -685,9 +661,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G4",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 1,
         .stepping = 2,
@@ -734,13 +708,16 @@ static int cpu_x86_fill_model_id(char *str)
 static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+    int i;
 
     x86_cpu_def->name = "host";
     host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->level = eax;
-    x86_cpu_def->vendor1 = ebx;
-    x86_cpu_def->vendor2 = edx;
-    x86_cpu_def->vendor3 = ecx;
+    for (i = 0; i < 4; i++) {
+        x86_cpu_def->vendor[i] = ebx >> (8 * i);
+        x86_cpu_def->vendor[i + 4] = edx >> (8 * i);
+        x86_cpu_def->vendor[i + 8] = ecx >> (8 * i);
+    }
 
     host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
@@ -765,9 +742,7 @@ static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
-    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
-        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
-        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
+    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
         host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         if (eax >= 0xC0000001) {
             /* Support VIA max extended level */
@@ -1335,15 +1310,8 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
 
-    if (def->vendor1) {
-        env->cpuid_vendor1 = def->vendor1;
-        env->cpuid_vendor2 = def->vendor2;
-        env->cpuid_vendor3 = def->vendor3;
-    } else {
-        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
-        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
-        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
-    }
+    object_property_set_str(OBJECT(cpu), def->vendor[0] ?
+                            def->vendor : CPUID_VENDOR_INTEL, "vendor", errp);
     object_property_set_bool(OBJECT(cpu), true, "vendor-override", errp);
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
@@ -1377,7 +1345,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
-    unsigned int i;
     x86_def_t *def;
 
     char *s = g_strdup(cpu_model);
@@ -1476,18 +1443,7 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                if (strlen(val) != 12) {
-                    fprintf(stderr, "vendor string must be 12 chars long\n");
-                    goto error;
-                }
-                x86_cpu_def->vendor1 = 0;
-                x86_cpu_def->vendor2 = 0;
-                x86_cpu_def->vendor3 = 0;
-                for(i = 0; i < 4; i++) {
-                    x86_cpu_def->vendor1 |= ((uint8_t)val[i    ]) << (8 * i);
-                    x86_cpu_def->vendor2 |= ((uint8_t)val[i + 4]) << (8 * i);
-                    x86_cpu_def->vendor3 |= ((uint8_t)val[i + 8]) << (8 * i);
-                }
+                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
                 x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index b6bcdf1..5265c5a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -479,14 +479,14 @@
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+#define CPUID_VENDOR_INTEL "GenuineIntel"
 
 #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
 #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
 #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
 
-#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_VIA   "CentaurHauls"
 
 #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
-- 
1.7.1

[Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Igor Mammedov]

cpu_model string does represent features in following format:
 ([+-]feat)|(feat=foo)|(feat)
which makes it impossible directly use property infrastructure
to set features on CPU.
This patch introduces parser that splits CPU name from cpu_model and
converts legacy features string into canonized set of strings that
is compatible with property manipulation infrastructure.

PS:
  * later it could be used as a hook to convert legacy command line
    features to global properties. Then marked as deprecated and
    removed with -cpu option in the future.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v2:
  * compiler complains that it's unused function but I guess it is
    easier for review this way, for pull req I'll squash it into next
    patch
  * fix spelling error
  * initialize sptr, due to a CentOS6 compiler warning, that
    breakes build when -Werror is set. suggested-by: Don Slutz
---
 target-i386/cpu.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b7a9cd6..afa12ca 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1342,6 +1342,58 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     }
 }
 
+/* convert legacy cpumodel string to string cpu_name and
+ * a uniform set of custom features that will be applied to CPU
+ * using object_property_parse()
+ */
+static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
+                                        QDict **features, Error **errp)
+{
+
+    char *s = g_strdup(cpu_model);
+    char *featurestr, *sptr = NULL;
+
+    *cpu_name = strtok_r(s, ",", &sptr);
+    *features = qdict_new();
+
+    featurestr = strtok_r(NULL, ",", &sptr);
+    while (featurestr) {
+        char *val;
+        if (featurestr[0] == '+') {
+            /*
+             * preseve legacy behaviour, if feature was disabled once
+             * do not allow to enable it again
+             */
+            if (!qdict_haskey(*features, featurestr + 1)) {
+                qdict_put(*features, featurestr + 1, qstring_from_str("on"));
+            }
+        } else if (featurestr[0] == '-') {
+            qdict_put(*features, featurestr + 1, qstring_from_str("off"));
+        } else {
+            val = strchr(featurestr, '=');
+            if (val) {
+                *val = 0; val++;
+                if (!strcmp(featurestr, "vendor")) {
+                    qdict_put(*features, "vendor-override",
+                              qstring_from_str("on"));
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                } else if (!strcmp(featurestr, "tsc_freq")) {
+                    qdict_put(*features, "tsc-frequency",
+                              qstring_from_str(val));
+                } else {
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                }
+            } else {
+                qdict_put(*features, featurestr, qstring_from_str("on"));
+            }
+        }
+
+        featurestr = strtok_r(NULL, ",", &sptr);
+    }
+
+    return;
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Blue Swirl]

On Wed, Sep 26, 2012 at 8:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> cpu_model string does represent features in following format:
>  ([+-]feat)|(feat=foo)|(feat)
> which makes it impossible directly use property infrastructure
> to set features on CPU.
> This patch introduces parser that splits CPU name from cpu_model and
> converts legacy features string into canonized set of strings that
> is compatible with property manipulation infrastructure.
>
> PS:
>   * later it could be used as a hook to convert legacy command line
>     features to global properties. Then marked as deprecated and
>     removed with -cpu option in the future.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  v2:
>   * compiler complains that it's unused function but I guess it is
>     easier for review this way, for pull req I'll squash it into next
>     patch
>   * fix spelling error
>   * initialize sptr, due to a CentOS6 compiler warning, that
>     breakes build when -Werror is set. suggested-by: Don Slutz
> ---
>  target-i386/cpu.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b7a9cd6..afa12ca 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1342,6 +1342,58 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>      }
>  }
>
> +/* convert legacy cpumodel string to string cpu_name and
> + * a uniform set of custom features that will be applied to CPU
> + * using object_property_parse()
> + */
> +static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
> +                                        QDict **features, Error **errp)
> +{
> +
> +    char *s = g_strdup(cpu_model);
> +    char *featurestr, *sptr = NULL;
> +
> +    *cpu_name = strtok_r(s, ",", &sptr);

This would break build since strtok_r() isn't available on Mingw, it's
not Posix either. How about g_strsplit()?

> +    *features = qdict_new();
> +
> +    featurestr = strtok_r(NULL, ",", &sptr);
> +    while (featurestr) {
> +        char *val;
> +        if (featurestr[0] == '+') {
> +            /*
> +             * preseve legacy behaviour, if feature was disabled once
> +             * do not allow to enable it again
> +             */
> +            if (!qdict_haskey(*features, featurestr + 1)) {
> +                qdict_put(*features, featurestr + 1, qstring_from_str("on"));
> +            }
> +        } else if (featurestr[0] == '-') {
> +            qdict_put(*features, featurestr + 1, qstring_from_str("off"));
> +        } else {
> +            val = strchr(featurestr, '=');
> +            if (val) {
> +                *val = 0; val++;
> +                if (!strcmp(featurestr, "vendor")) {
> +                    qdict_put(*features, "vendor-override",
> +                              qstring_from_str("on"));
> +                    qdict_put(*features, featurestr, qstring_from_str(val));
> +                } else if (!strcmp(featurestr, "tsc_freq")) {
> +                    qdict_put(*features, "tsc-frequency",
> +                              qstring_from_str(val));
> +                } else {
> +                    qdict_put(*features, featurestr, qstring_from_str(val));
> +                }
> +            } else {
> +                qdict_put(*features, featurestr, qstring_from_str("on"));
> +            }
> +        }
> +
> +        featurestr = strtok_r(NULL, ",", &sptr);
> +    }
> +
> +    return;
> +}
> +
>  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>                                  const char *cpu_model, Error **errp)
>  {
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Igor Mammedov]

On Sun, 30 Sep 2012 12:16:27 +0000
Blue Swirl <blauwirbel@gmail.com> wrote:

> This would break build since strtok_r() isn't available on Mingw, it's
> not Posix either. How about g_strsplit()?

Thanks! Updated series at:
https://github.com/imammedo/qemu/tree/x86-cpu-properties.WIP

I'll post fixed [7/22] and [18/22] patches as followup to this thread.

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Eric Blake]

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

On 09/30/2012 06:16 AM, Blue Swirl wrote:
> On Wed, Sep 26, 2012 at 8:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> cpu_model string does represent features in following format:
>>  ([+-]feat)|(feat=foo)|(feat)
>> which makes it impossible directly use property infrastructure
>> to set features on CPU.
>> This patch introduces parser that splits CPU name from cpu_model and
>> converts legacy features string into canonized set of strings that
>> is compatible with property manipulation infrastructure.
>>

>> +
>> +    *cpu_name = strtok_r(s, ",", &sptr);
> 
> This would break build since strtok_r() isn't available on Mingw, it's

Correct.  Microsoft is stuck in the past when it comes to standard
conformance.

> not Posix either.

Huh? http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtok_r.html
strtok_r has been part of POSIX since pthreads were added.

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Paolo Bonzini]

Il 02/10/2012 13:28, Eric Blake ha scritto:
>>> >> +    *cpu_name = strtok_r(s, ",", &sptr);
>> > 
>> > This would break build since strtok_r() isn't available on Mingw, it's
> Correct.  Microsoft is stuck in the past when it comes to standard
> conformance.

Luckily, in the common case of a non-varying delimiter g_strsplit is just
as simple:

gchar **
g_strsplit                      (const gchar *string,
                                 const gchar *delimiter,
                                 gint max_tokens);

Splits a string into a maximum of max_tokens pieces, using the given delimiter.
If max_tokens is reached, the remainder of string is appended to the last token.

As a special case, the result of splitting the empty string "" is an empty vector,
not a vector containing a single string. The reason for this special case is that
being able to represent a empty vector is typically more useful than consistent
handling of empty elements. If you do need to represent empty elements, you'll
need to check for the empty string before calling g_strsplit().

string :
	a string to split.

delimiter :
	a string which specifies the places at which to split the string. The delimiter
        is not included in any of the resulting strings, unless max_tokens is reached.

max_tokens :
	the maximum number of pieces to split string into. If this is less than 1, the
        string is split completely.

Returns :
	a newly-allocated NULL-terminated array of strings. Use g_strfreev() to free it.

[Qemu-devel] [PATCH 18/22 v3] target-i386: parse cpu_model string into set of stringified properties

[Igor Mammedov]

cpu_model string does represent features in following format:
 ([+-]feat)|(feat=foo)|(feat)
which makes it impossible directly use property infrastructure
to set features on CPU.
This patch introduces parser that splits CPU name from cpu_model and
converts legacy features string into canonized set of strings that
is compatible with property manipulation infrastructure.

PS:
  * later it could be used as a hook to convert legacy command line
    features to global properties. Then marked as deprecated and
    removed with -cpu option in the future.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v2:
  * compiler complains that it's unused function but I guess it is
    easier for review this way, for pull req I'll squash it into next
    patch
  * fix spelling error
  * initialize sptr, due to a CentOS6 compiler warning, that
    breakes build when -Werror is set. Suggested-by: Don Slutz
 v3:
  * Mingw doesn't have strtok_r(), use g_strsplit() instead of it.
        Suggested-by: Blue Swirl
---
 target-i386/cpu.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ad1f088..9cf25f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1339,6 +1339,57 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     }
 }
 
+/* convert legacy cpumodel string to string cpu_name and
+ * a uniform set of custom features that will be applied to CPU
+ * using object_property_parse()
+ */
+static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
+                                        QDict **features, Error **errp)
+{
+    int i;
+    gchar **feat_array = g_strsplit(cpu_model, ",", 0);
+    *features = qdict_new();
+
+    g_assert(feat_array[0] != NULL);
+    *cpu_name = g_strdup(feat_array[0]);
+
+    for (i = 1; feat_array[i]; ++i) {
+        gchar *featurestr = feat_array[i];
+        char *val;
+        if (featurestr[0] == '+') {
+            /*
+             * preseve legacy behaviour, if feature was disabled once
+             * do not allow to enable it again
+             */
+            if (!qdict_haskey(*features, featurestr + 1)) {
+                qdict_put(*features, featurestr + 1, qstring_from_str("on"));
+            }
+        } else if (featurestr[0] == '-') {
+            qdict_put(*features, featurestr + 1, qstring_from_str("off"));
+        } else {
+            val = strchr(featurestr, '=');
+            if (val) {
+                *val = 0; val++;
+                if (!strcmp(featurestr, "vendor")) {
+                    qdict_put(*features, "vendor-override",
+                              qstring_from_str("on"));
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                } else if (!strcmp(featurestr, "tsc_freq")) {
+                    qdict_put(*features, "tsc-frequency",
+                              qstring_from_str(val));
+                } else {
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                }
+            } else {
+                qdict_put(*features, featurestr, qstring_from_str("on"));
+            }
+        }
+    }
+
+    g_strfreev(feat_array);
+    return;
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Blue Swirl]

On Tue, Oct 2, 2012 at 11:28 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/30/2012 06:16 AM, Blue Swirl wrote:
>> On Wed, Sep 26, 2012 at 8:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>> cpu_model string does represent features in following format:
>>>  ([+-]feat)|(feat=foo)|(feat)
>>> which makes it impossible directly use property infrastructure
>>> to set features on CPU.
>>> This patch introduces parser that splits CPU name from cpu_model and
>>> converts legacy features string into canonized set of strings that
>>> is compatible with property manipulation infrastructure.
>>>
>
>>> +
>>> +    *cpu_name = strtok_r(s, ",", &sptr);
>>
>> This would break build since strtok_r() isn't available on Mingw, it's
>
> Correct.  Microsoft is stuck in the past when it comes to standard
> conformance.
>
>> not Posix either.
>
> Huh? http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtok_r.html
> strtok_r has been part of POSIX since pthreads were added.

Sorry, I missed that strtok_r() was sharing the entry with strtok().

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

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Igor Mammedov]

On Wed, 3 Oct 2012 20:09:52 +0000
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Tue, Oct 2, 2012 at 11:28 AM, Eric Blake <eblake@redhat.com> wrote:
> > On 09/30/2012 06:16 AM, Blue Swirl wrote:
> >> On Wed, Sep 26, 2012 at 8:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> >>> cpu_model string does represent features in following format:
> >>>  ([+-]feat)|(feat=foo)|(feat)
> >>> which makes it impossible directly use property infrastructure
> >>> to set features on CPU.
> >>> This patch introduces parser that splits CPU name from cpu_model and
> >>> converts legacy features string into canonized set of strings that
> >>> is compatible with property manipulation infrastructure.
> >>>
> >
> >>> +
> >>> +    *cpu_name = strtok_r(s, ",", &sptr);
> >>
> >> This would break build since strtok_r() isn't available on Mingw, it's
> >
> > Correct.  Microsoft is stuck in the past when it comes to standard
> > conformance.
> >
> >> not Posix either.
> >
> > Huh? http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtok_r.html
> > strtok_r has been part of POSIX since pthreads were added.
> 
> Sorry, I missed that strtok_r() was sharing the entry with strtok().
Mingw maintainers rejected to implement it though:
http://sourceforge.net/tracker/?func=detail&aid=2673480&group_id=2435&atid=352435

Anyway, I've already replace it with g_strsplit(), resulting patch became even
simpler.

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


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 19/22] target-i386: use properties to set/unset user specified features on CPU

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
  * fix error of initializing from incorrect cpu model definition
    use x86_cpu_def instead of def. spotted-by: Eduardo Habkost
  * add missing env in cpu_x86_find_by_name()
  * added cpu_x86_set_props() to make following code movement more
    clean. suggested-by: Eduardo Habkost
  * init name and feature to NULL, to avoid freeing uninitialized mem
---
 target-i386/cpu.c |  184 ++++++++++------------------------------------------
 1 files changed, 36 insertions(+), 148 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index afa12ca..759268d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -214,22 +214,6 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
     return found;
 }
 
-static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
-                                    uint32_t *ext_features,
-                                    uint32_t *ext2_features,
-                                    uint32_t *ext3_features,
-                                    uint32_t *kvm_features,
-                                    uint32_t *svm_features)
-{
-    if (!lookup_feature(features, flagname, NULL, feature_name) &&
-        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
-        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
-        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
-        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
-        !lookup_feature(svm_features, flagname, NULL, svm_feature_name))
-            fprintf(stderr, "CPU feature %s not found\n", flagname);
-}
-
 typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
@@ -1325,7 +1309,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_ext_features = def->ext_features;
     env->cpuid_ext2_features = def->ext2_features;
     env->cpuid_ext3_features = def->ext3_features;
-    env->cpuid_kvm_features = def->kvm_features;
     env->cpuid_svm_features = def->svm_features;
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
@@ -1394,22 +1377,34 @@ static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
     return;
 }
 
+/* Set features on X86CPU object based on a QDict */
+static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
+        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
+        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
+                              qdict_entry_key(ent), errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
     x86_def_t *def;
 
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
-    /* Features to be added*/
-    uint32_t plus_features = 0, plus_ext_features = 0;
-    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
-    /* Features to be removed */
-    uint32_t minus_features = 0, minus_ext_features = 0;
-    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
-    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
-    uint32_t numvalue;
+    CPUX86State *env = &cpu->env;
+    QDict *features = NULL;
+    char *name = NULL;
+
+    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
+    if (error_is_set(errp)) {
+        goto error;
+    }
 
     for (def = x86_defs; def; def = def->next)
         if (name && !strcmp(name, def->name))
@@ -1422,8 +1417,10 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
+    cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
+
 #if defined(CONFIG_KVM)
-    plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
+    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_NOP_IO_DELAY) | 
         (1 << KVM_FEATURE_MMU_OP) |
         (1 << KVM_FEATURE_CLOCKSOURCE2) |
@@ -1431,130 +1428,23 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
 #else
-    plus_kvm_features = 0;
+    env->cpuid_kvm_features = 0;
 #endif
 
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features, &plus_svm_features);
-
-    featurestr = strtok(NULL, ",");
-
-    while (featurestr) {
-        char *val;
-        if (featurestr[0] == '+') {
-            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
-                            &plus_ext_features, &plus_ext2_features,
-                            &plus_ext3_features, &plus_kvm_features,
-                            &plus_svm_features);
-        } else if (featurestr[0] == '-') {
-            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
-                            &minus_ext_features, &minus_ext2_features,
-                            &minus_ext3_features, &minus_kvm_features,
-                            &minus_svm_features);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
-            if (!strcmp(featurestr, "family")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff + 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->family = numvalue;
-            } else if (!strcmp(featurestr, "model")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->model = numvalue;
-            } else if (!strcmp(featurestr, "stepping")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->stepping = numvalue ;
-            } else if (!strcmp(featurestr, "level")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->level = numvalue;
-            } else if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->xlevel = numvalue;
-            } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
-            } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
-            } else if (!strcmp(featurestr, "tsc_freq")) {
-                int64_t tsc_freq;
-                char *err;
-
-                tsc_freq = strtosz_suffix_unit(val, &err,
-                                               STRTOSZ_DEFSUFFIX_B, 1000);
-                if (tsc_freq < 0 || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
-            } else if (!strcmp(featurestr, "hv_spinlocks")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                hyperv_set_spinlock_retries(numvalue);
-            } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
-                goto error;
-            }
-        } else if (!strcmp(featurestr, "check")) {
-            check_cpuid = 1;
-        } else if (!strcmp(featurestr, "enforce")) {
-            check_cpuid = enforce_cpuid = 1;
-        } else if (!strcmp(featurestr, "hv_relaxed")) {
-            hyperv_enable_relaxed_timing(true);
-        } else if (!strcmp(featurestr, "hv_vapic")) {
-            hyperv_enable_vapic_recommended(true);
-        } else {
-            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
-            goto error;
-        }
+    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
-        featurestr = strtok(NULL, ",");
+    cpu_x86_set_props(cpu, features, errp);
+    QDECREF(features);
+    if (error_is_set(errp)) {
+        goto error;
     }
-    x86_cpu_def->features |= plus_features;
-    x86_cpu_def->ext_features |= plus_ext_features;
-    x86_cpu_def->ext2_features |= plus_ext2_features;
-    x86_cpu_def->ext3_features |= plus_ext3_features;
-    x86_cpu_def->kvm_features |= plus_kvm_features;
-    x86_cpu_def->svm_features |= plus_svm_features;
-    x86_cpu_def->features &= ~minus_features;
-    x86_cpu_def->ext_features &= ~minus_ext_features;
-    x86_cpu_def->ext2_features &= ~minus_ext2_features;
-    x86_cpu_def->ext3_features &= ~minus_ext3_features;
-    x86_cpu_def->kvm_features &= ~minus_kvm_features;
-    x86_cpu_def->svm_features &= ~minus_svm_features;
-    g_free(s);
+
+    g_free(name);
     return 0;
 
 error:
-    g_free(s);
+    g_free(name);
+    QDECREF(features);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1647,8 +1537,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto out;
     }
 
-    cpudef_2_x86_cpu(cpu, def, &error);
-
 out:
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
-- 
1.7.1

[Qemu-devel] [PATCH 20/22] target-i386: move init of "hypervisor" feature into CPU initializer from cpudef

[Igor Mammedov]

"hypervisor" CPU feature is unconditionally enabled/overridden even if it's cleared
in cpudef. Moving it inside CPU initializer from cpudef will help to
split cpu_x86_find_by_name() into default init and user settable properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 759268d..c2e093d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1313,6 +1313,7 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
     env->cpuid_xlevel2 = def->xlevel2;
+    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
      * CPUID[1].EDX.
@@ -1431,8 +1432,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     env->cpuid_kvm_features = 0;
 #endif
 
-    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
-
     cpu_x86_set_props(cpu, features, errp);
     QDECREF(features);
     if (error_is_set(errp)) {
-- 
1.7.1

[Qemu-devel] [PATCH 21/22] target-i386: move default init of cpuid_kvm_features bitmap into CPU initializer from cpudef

[Igor Mammedov]

Moving it inside CPU initializer from cpudef will help to split
cpu_x86_find_by_name() into default init and user settable properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
[ehabkost: rebase on top of latest qemu.git master, where the bitmap
initialization is now different]
[imammedo: fix whitespace]
---
 target-i386/cpu.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c2e093d..d452347 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1313,6 +1313,17 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
     env->cpuid_xlevel2 = def->xlevel2;
+#if defined(CONFIG_KVM)
+    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
+        (1 << KVM_FEATURE_NOP_IO_DELAY) |
+        (1 << KVM_FEATURE_MMU_OP) |
+        (1 << KVM_FEATURE_CLOCKSOURCE2) |
+        (1 << KVM_FEATURE_ASYNC_PF) |
+        (1 << KVM_FEATURE_STEAL_TIME) |
+        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
+#else
+    env->cpuid_kvm_features = 0;
+#endif
     object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
@@ -1398,7 +1409,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 {
     x86_def_t *def;
 
-    CPUX86State *env = &cpu->env;
     QDict *features = NULL;
     char *name = NULL;
 
@@ -1420,18 +1430,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 
     cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
 
-#if defined(CONFIG_KVM)
-    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
-        (1 << KVM_FEATURE_NOP_IO_DELAY) | 
-        (1 << KVM_FEATURE_MMU_OP) |
-        (1 << KVM_FEATURE_CLOCKSOURCE2) |
-        (1 << KVM_FEATURE_ASYNC_PF) | 
-        (1 << KVM_FEATURE_STEAL_TIME) |
-        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-#else
-    env->cpuid_kvm_features = 0;
-#endif
-
     cpu_x86_set_props(cpu, features, errp);
     QDECREF(features);
     if (error_is_set(errp)) {
-- 
1.7.1

[Qemu-devel] [PATCH 22/22] target-i386: cleanup cpu_x86_find_by_name(), only fill x86_def_t in it

[Igor Mammedov]

Do in cpu_x86_find_by_name() only what name implies. i.e. leave only
cpudef search and copy/fill passed in x86_def_t structure.

and move out of it cpu_model parsing and CPU initializing into
cpu_x86_register(). Plus add hints to where blocks should go when
cpu_x86_register() is disbanded.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   55 +++++++++++++++++++++++-----------------------------
 1 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d452347..b8f431a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1409,43 +1409,21 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 {
     x86_def_t *def;
 
-    QDict *features = NULL;
-    char *name = NULL;
-
-    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
-    if (error_is_set(errp)) {
-        goto error;
-    }
-
-    for (def = x86_defs; def; def = def->next)
-        if (name && !strcmp(name, def->name))
+    for (def = x86_defs; def; def = def->next) {
+        if (!strcmp(cpu_model, def->name)) {
             break;
-    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
+        }
+    }
+    if (kvm_enabled() && strcmp(cpu_model, "host") == 0) {
         cpu_x86_fill_host(x86_cpu_def);
     } else if (!def) {
-        goto error;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, cpu_model);
+        return -1;
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
-    cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
-
-    cpu_x86_set_props(cpu, features, errp);
-    QDECREF(features);
-    if (error_is_set(errp)) {
-        goto error;
-    }
-
-    g_free(name);
     return 0;
-
-error:
-    g_free(name);
-    QDECREF(features);
-    if (!error_is_set(errp)) {
-        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
-    }
-    return -1;
 }
 
 /* generate a composite string into buf of all cpuid names in featureset
@@ -1527,14 +1505,29 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
+    QDict *features = NULL;
+    char *name = NULL;
 
-    memset(def, 0, sizeof(*def));
+    /* for CPU subclasses should go into cpu_x86_init() before object_new() */
+    compat_normalize_cpu_model(cpu_model, &name, &features, &error);
+    if (error_is_set(&error)) {
+        goto out;
+    }
 
-    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+    /* this block should be replaced by CPU subclasses */
+    memset(def, 0, sizeof(*def));
+    if (cpu_x86_find_by_name(cpu, def, name, &error) < 0) {
         goto out;
     }
+    cpudef_2_x86_cpu(cpu, def, &error);
+
+    /* for CPU subclasses should go between object_new() and
+     * x86_cpu_realize() */
+    cpu_x86_set_props(cpu, features, &error);
 
 out:
+    QDECREF(features);
+    g_free(name);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-- 
1.7.1

[Qemu-devel] [PATCH 00/22 v2] target-i386: convert CPU features into properties

[Igor Mammedov]

Build and run tested in FC17 host with x86_64-linux-user, x86_64-softmmu
targets

git tree for testing:
  https://github.com/imammedo/qemu/tree/x86-cpu-properties.v2

Igor Mammedov (22):
  target-i386: return Error from cpu_x86_find_by_name()
  target-i386: cpu_x86_register(): report error from property setter
  target-i386: if x86_cpu_realize() failed report error and do cleanup
  target-i386: filter out not TCG features if running without kvm at
    realize time
  target-i386: move out CPU features initialization in separate func
  target-i386: xlevel should be more than 0x80000000, move fixup into
    setter
  target-i386: convert cpuid features into properties
  target-i386: add stubs for
    hyperv_(vapic_recommended|relaxed_timing_enabled|get_spinlock_retries)()
  target-i386: convert 'hv_spinlocks' feature into property
  target-i386: convert 'hv_relaxed' feature into property
  target-i386: convert 'hv_vapic' feature into property
  target-i386: convert 'check' and 'enforce' features into properties
  add visitor for parsing hz[KMG] input string
  target-i386: use visit_type_hz to parse tsc_freq property value
  target-i386: introduce vendor-override property
  target-i386: use define for cpuid vendor string size
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: parse cpu_model string into set of stringified
    properties
  target-i386: use properties to set/unset user specified features on
    CPU
  target-i386: move init of "hypervisor" feature into CPU initializer
    from cpudef
  target-i386: move default init of cpuid_kvm_features bitmap into CPU
    initializer from cpudef
  target-i386: cleanup cpu_x86_find_by_name(), only fill x86_def_t in
    it

 qapi/qapi-visit-core.c      |  11 +
 qapi/qapi-visit-core.h      |   2 +
 qapi/string-input-visitor.c |  22 ++
 target-i386/cpu.c           | 723 +++++++++++++++++++++++++++-----------------
 target-i386/cpu.h           |  10 +-
 target-i386/helper.c        |   9 +-
 target-i386/hyperv.h        |   9 +-
 7 files changed, 498 insertions(+), 288 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Igor Mammedov]

cpu_model string does represent features in following format:
 ([+-]feat)|(feat=foo)|(feat)
which makes it impossible directly use property infrastructure
to set features on CPU.
This patch introduces parser that splits CPU name from cpu_model and
converts legacy features string into canonized set of strings that
is compatible with property manipulation infrastructure.

PS:
  * later it could be used as a hook to convert legacy command line
    features to global properties. Then marked as deprecated and
    removed with -cpu option in the future.
  * compiler complains that it's unused function but I guess it is
    easier for review this way

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0543e62..2c9cd6b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1332,6 +1332,58 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_xlevel2 = def->xlevel2;
 }
 
+/* convert legacy cpumodel string to string cpu_name and
+ * a uniforms set of custom features that will be applied to CPU
+ * using object_property_parse()
+ */
+static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
+                                        QDict **features, Error **errp)
+{
+
+    char *s = g_strdup(cpu_model);
+    char *featurestr, *sptr;
+
+    *cpu_name = strtok_r(s, ",", &sptr);
+    *features = qdict_new();
+
+    featurestr = strtok_r(NULL, ",", &sptr);
+    while (featurestr) {
+        char *val;
+        if (featurestr[0] == '+') {
+            /*
+             * preseve legacy behaviour, if feature was disabled once
+             * do not allow to enable it again
+             */
+            if (!qdict_haskey(*features, featurestr + 1)) {
+                qdict_put(*features, featurestr + 1, qstring_from_str("on"));
+            }
+        } else if (featurestr[0] == '-') {
+            qdict_put(*features, featurestr + 1, qstring_from_str("off"));
+        } else {
+            val = strchr(featurestr, '=');
+            if (val) {
+                *val = 0; val++;
+                if (!strcmp(featurestr, "vendor")) {
+                    qdict_put(*features, "vendor-override",
+                              qstring_from_str("on"));
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                } else if (!strcmp(featurestr, "tsc_freq")) {
+                    qdict_put(*features, "tsc-frequency",
+                              qstring_from_str(val));
+                } else {
+                    qdict_put(*features, featurestr, qstring_from_str(val));
+                }
+            } else {
+                qdict_put(*features, featurestr, qstring_from_str("on"));
+            }
+        }
+
+        featurestr = strtok_r(NULL, ",", &sptr);
+    }
+
+    return;
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties

[Don Slutz]

On 09/07/12 16:55, Igor Mammedov wrote:
> cpu_model string does represent features in following format:
>   ([+-]feat)|(feat=foo)|(feat)
> which makes it impossible directly use property infrastructure
> to set features on CPU.
> This patch introduces parser that splits CPU name from cpu_model and
> converts legacy features string into canonized set of strings that
> is compatible with property manipulation infrastructure.
>
> PS:
>    * later it could be used as a hook to convert legacy command line
>      features to global properties. Then marked as deprecated and
>      removed with -cpu option in the future.
>    * compiler complains that it's unused function but I guess it is
>      easier for review this way
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   target-i386/cpu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0543e62..2c9cd6b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1332,6 +1332,58 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>       env->cpuid_xlevel2 = def->xlevel2;
>   }
>   
> +/* convert legacy cpumodel string to string cpu_name and
> + * a uniforms set of custom features that will be applied to CPU
> + * using object_property_parse()
> + */
> +static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
> +                                        QDict **features, Error **errp)
> +{
> +
> +    char *s = g_strdup(cpu_model);
> +    char *featurestr, *sptr;
> +
> +    *cpu_name = strtok_r(s, ",", &sptr);
I get:

cc1: warnings being treated as errors
/root/qemu-cpu-v2/target-i386/cpu.c: In function 'cpu_x86_register':
/root/qemu-cpu-v2/target-i386/cpu.c:1341: error: 'sptr' may be used 
uninitialized in this function
/root/qemu-cpu-v2/target-i386/cpu.c:1341: note: 'sptr' was declared here

And the change:

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7964a3..af50a8f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1338,7 +1338,7 @@ static void compat_normalize_cpu_model(const char 
*cpu_model, char **cpu_name,
  {

      char *s = g_strdup(cpu_model);
-    char *featurestr, *sptr;
+    char *featurestr, *sptr = NULL;

      *cpu_name = strtok_r(s, ",", &sptr);
      *features = qdict_new();

fixes this for me.

> +    *features = qdict_new();
> +
> +    featurestr = strtok_r(NULL, ",", &sptr);
> +    while (featurestr) {
> +        char *val;
> +        if (featurestr[0] == '+') {
> +            /*
> +             * preseve legacy behaviour, if feature was disabled once
> +             * do not allow to enable it again
> +             */
> +            if (!qdict_haskey(*features, featurestr + 1)) {
> +                qdict_put(*features, featurestr + 1, qstring_from_str("on"));
> +            }
> +        } else if (featurestr[0] == '-') {
> +            qdict_put(*features, featurestr + 1, qstring_from_str("off"));
> +        } else {
> +            val = strchr(featurestr, '=');
> +            if (val) {
> +                *val = 0; val++;
> +                if (!strcmp(featurestr, "vendor")) {
> +                    qdict_put(*features, "vendor-override",
> +                              qstring_from_str("on"));
> +                    qdict_put(*features, featurestr, qstring_from_str(val));
> +                } else if (!strcmp(featurestr, "tsc_freq")) {
> +                    qdict_put(*features, "tsc-frequency",
> +                              qstring_from_str(val));
> +                } else {
> +                    qdict_put(*features, featurestr, qstring_from_str(val));
> +                }
> +            } else {
> +                qdict_put(*features, featurestr, qstring_from_str("on"));
> +            }
> +        }
> +
> +        featurestr = strtok_r(NULL, ",", &sptr);
> +    }
> +
> +    return;
> +}
> +
>   static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>                                   const char *cpu_model, Error **errp)
>   {
  -Don Slutz