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

[Qemu-devel] [PATCH 00/21] 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

changes since RFC are documented in idividual patch descriptions
Thanks Andreas, Blue, Eduardo for reviews and suggestions.

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

Igor Mammedov (21):
  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: 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           | 691 +++++++++++++++++++++++++++-----------------
 target-i386/cpu.h           |  14 +-
 target-i386/helper.c        |   9 +-
 6 files changed, 479 insertions(+), 270 deletions(-)

-- 
1.7.11.2

[Qemu-devel] [PATCH 01/21] 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>
--
v2:
    style change, add braces (reqested by Blue Swirl)
---
 target-i386/cpu.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6d5d0d6..49286ca 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -859,7 +859,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;
@@ -1004,6 +1005,11 @@ 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;
         }
+
+        if (error_is_set(errp)) {
+            goto error;
+        }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1027,6 +1033,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;
 }
 
@@ -1155,8 +1164,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;
@@ -1195,6 +1206,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.11.2

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 49286ca..b655dbc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1209,6 +1209,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
 out:
     if (error_is_set(&error)) {
+        fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
         return -1;
     }
-- 
1.7.11.2

[Qemu-devel] [PATCH 03/21] 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>
---
 target-i386/helper.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 8a5da3d..a0e4c89 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_is_set(&error)) {
+        error_free(error);
+        object_delete(OBJECT(cpu));
+        return NULL;
+    }
     return cpu;
 }
 
-- 
1.7.11.2

[Qemu-devel] [PATCH 04/21] 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>
---
 target-i386/cpu.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b655dbc..cc6ce48 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1194,17 +1194,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     env->cpuid_xlevel2 = def->xlevel2;
     object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
                             "tsc-frequency", &error);
-    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:
@@ -1768,6 +1757,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.11.2

[Qemu-devel] [PATCH 05/21] 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>
---
 target-i386/cpu.c | 62 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cc6ce48..f31462a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -859,6 +859,39 @@ 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;
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
@@ -1158,7 +1191,6 @@ CpuDefinitionInfoList *qmp_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;
 
@@ -1168,33 +1200,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);
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    cpudef_2_x86_cpu(cpu, def, &error);
 
 out:
     if (error_is_set(&error)) {
-- 
1.7.11.2

[Qemu-devel] [PATCH 06/21] 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 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f31462a..37ba5ef 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -746,8 +746,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)
@@ -982,9 +991,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.11.2

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

[Igor Mammedov]

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
---
 target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 37ba5ef..440e724 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -606,6 +606,101 @@ 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;
+    }
+}
+
+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) {
+                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)
 {
@@ -1824,6 +1919,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.11.2

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 440e724..777b8ce 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -963,6 +963,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;
@@ -1919,6 +1945,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.11.2

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 777b8ce..edf277e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -987,6 +987,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)
@@ -1949,6 +1969,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.11.2

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index edf277e..4e7f22b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1007,6 +1007,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)
@@ -1972,6 +1992,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.11.2

[Qemu-devel] [PATCH 11/21] 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 file changed, 57 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4e7f22b..33326cc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -107,8 +107,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)
@@ -580,19 +580,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);
@@ -1029,6 +1030,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;
@@ -1224,10 +1262,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;
 
@@ -1932,6 +1966,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;
@@ -1985,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);
+    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.11.2

[Qemu-devel] [PATCH 12/21] add visitor for parsing hz[KMG] input string

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
--
v2:
  * replaced _hz suffix for frequency visitor by _freq suffix
    suggested-by: Andreas Färber
  * fixed typo, spotted-by: Andreas Färber
---
 qapi/qapi-visit-core.c      | 11 +++++++++++
 qapi/qapi-visit-core.h      |  2 ++
 qapi/string-input-visitor.c | 22 ++++++++++++++++++++++
 3 files changed, 35 insertions(+)

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..9ec0895 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;
+
+    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.11.2

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
v2:
  * use visit_type_freq() which replaced visit_type_hz()
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 33326cc..3952368 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -951,7 +951,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     const int64_t max = INT_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.11.2

[Qemu-devel] [PATCH 14/21] 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 3952368..cb6ada0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -893,7 +893,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)
@@ -1067,6 +1066,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;
@@ -1080,7 +1104,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);
@@ -2019,6 +2043,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 60f9e97..979682a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -739,7 +739,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.11.2

[Qemu-devel] [PATCH 15/21] 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 cb6ada0..af6c9a3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -862,13 +862,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;
 }
 
@@ -879,7 +879,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 979682a..5c75704 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -440,6 +440,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.11.2

[Qemu-devel] [PATCH 16/21] 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.

To allow 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>
---
 target-i386/cpu.c | 76 +++++++++++++------------------------------------------
 target-i386/cpu.h | 10 +++-----
 2 files changed, 20 insertions(+), 66 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index af6c9a3..d24ef84 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -227,7 +227,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;
@@ -293,9 +293,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,
@@ -312,9 +310,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,
@@ -358,9 +354,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,
@@ -459,9 +453,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,
@@ -507,13 +499,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);
@@ -538,9 +533,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 */
@@ -1095,15 +1088,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);
@@ -1127,7 +1113,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);
@@ -1216,18 +1201,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),
@@ -1361,10 +1335,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
             (*cpu_fprintf)(f, "x86 %16s\n", buf);
         }
         if (dump) {
-            memcpy(buf, &def->vendor1, sizeof (def->vendor1));
-            memcpy(buf + 4, &def->vendor2, sizeof (def->vendor2));
-            memcpy(buf + 8, &def->vendor3, sizeof (def->vendor3));
-            buf[12] = '\0';
+            pstrcpy(buf, sizeof(def->vendor), def->vendor);
             (*cpu_fprintf)(f,
                 "  family %d model %d stepping %d level %d xlevel 0x%x"
                 " vendor \"%s\"\n",
@@ -1437,17 +1408,6 @@ out:
 }
 
 #if !defined(CONFIG_USER_ONLY)
-/* copy vendor id string to 32 bit register, nul pad as needed
- */
-static void cpyid(const char *s, uint32_t *id)
-{
-    char *d = (char *)id;
-    char i;
-
-    for (i = sizeof (*id); i--; )
-        *d++ = *s ? *s++ : '\0';
-}
-
 /* interpret radix and convert from string to arbitrary scalar,
  * otherwise flag failure
  */
@@ -1499,9 +1459,7 @@ static int cpudef_setfield(const char *name, const char *str, void *opaque)
     } else if (!strcmp(name, "level")) {
         setscalar(&def->level, str, &err)
     } else if (!strcmp(name, "vendor")) {
-        cpyid(&str[0], &def->vendor1);
-        cpyid(&str[4], &def->vendor2);
-        cpyid(&str[8], &def->vendor3);
+        pstrcpy(def->vendor, sizeof(def->vendor), str);
     } else if (!strcmp(name, "family")) {
         setscalar(&def->family, str, &err)
     } else if (!strcmp(name, "model")) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5c75704..5543496 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -445,14 +445,10 @@
 #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_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
+#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.11.2

[Qemu-devel] [PATCH 17/21] 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 d24ef84..98b6bbd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1110,6 +1110,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.2

[Qemu-devel] [PATCH 18/21] 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
---
 target-i386/cpu.c | 184 ++++++++++--------------------------------------------
 1 file changed, 34 insertions(+), 150 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 98b6bbd..73dbf32 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -207,22 +207,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;
@@ -1103,7 +1087,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;
@@ -1162,22 +1145,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;
+    char *name;
+
+    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))
@@ -1190,133 +1185,24 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
-    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
+    cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
 
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features, &plus_svm_features);
+    /* not supported bits will be filtered out later */
+    env->cpuid_kvm_features = ~0;
 
-    featurestr = strtok(NULL, ",");
+    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
-    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;
-        }
-
-        if (error_is_set(errp)) {
-            goto error;
-        }
+    cpu_x86_set_props(cpu, features, errp);
+    QDECREF(features);
+    if (error_is_set(errp)) {
+        goto error;
+    }
 
-        featurestr = strtok(NULL, ",");
-    }
-    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);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1448,8 +1334,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto out;
     }
 
-    cpudef_2_x86_cpu(cpu, def, &error);
-
 out:
     if (error_is_set(&error)) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
-- 
1.7.11.2

[Qemu-devel] [PATCH 19/21] 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 73dbf32..59ebe40 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1091,6 +1091,8 @@ 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);
 }
 
 /* convert legacy cpumodel string to string cpu_name and
@@ -1190,8 +1192,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     /* not supported bits will be filtered out later */
     env->cpuid_kvm_features = ~0;
 
-    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.11.2

[Qemu-devel] [PATCH 20/21] 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>
---
 target-i386/cpu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 59ebe40..75cbf48 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1092,6 +1092,9 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
     env->cpuid_xlevel2 = def->xlevel2;
 
+    /* not supported bits will be filtered out later */
+    env->cpuid_kvm_features = ~0;
+
     object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 }
 
@@ -1167,7 +1170,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;
     char *name;
 
@@ -1189,9 +1191,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);
 
-    /* not supported bits will be filtered out later */
-    env->cpuid_kvm_features = ~0;
-
     cpu_x86_set_props(cpu, features, errp);
     QDECREF(features);
     if (error_is_set(errp)) {
-- 
1.7.11.2

[Qemu-devel] [PATCH 21/21] 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 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 75cbf48..ee2a90f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1170,42 +1170,26 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 {
     x86_def_t *def;
 
-    QDict *features;
-    char *name;
-
-    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
-    if (error_is_set(errp)) {
-        goto error;
+    if (!cpu_model) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cpu_model", "NULL");
+        return -1;
     }
 
-    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);
-    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
@@ -1326,14 +1310,29 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
+    QDict *features;
+    char *name;
 
-    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_is_set(&error)) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-- 
1.7.11.2

Re: [Qemu-devel] [PATCH 12/21] add visitor for parsing hz[KMG] input string

[Andreas Färber]

Am 15.08.2012 18:13, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> --
> v2:
>   * replaced _hz suffix for frequency visitor by _freq suffix
>     suggested-by: Andreas Färber
>   * fixed typo, spotted-by: Andreas Färber
> ---
>  qapi/qapi-visit-core.c      | 11 +++++++++++
>  qapi/qapi-visit-core.h      |  2 ++
>  qapi/string-input-visitor.c | 22 ++++++++++++++++++++++
>  3 files changed, 35 insertions(+)
> 
> 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..9ec0895 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)

Tiny glitch here, one space too much it seems.

If you fix that:
Acked-by: Andreas Färber <afaerber@suse.de>

> +{
> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> +    char *endp = (char *) siv->string;
> +    long long val;
> +
> +    errno = 0;
> +    if (siv->string) {
> +        val = strtosz_suffix_unit(siv->string, &endp,
> +                             STRTOSZ_DEFSUFFIX_B, 1000);

Please also check here...

> +    }
> +    if (!siv->string || val == -1 || *endp) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +              "a value representable as a non-negative int64");

...and here.

> +        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;

Optionally reorder this, so that it's alongside the other type_*?

>  
>      v->string = str;
>      return v;
> 

Thanks,
Andreas


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 12/21] add visitor for parsing hz[KMG] input string

[Igor Mammedov]

On Wed, 15 Aug 2012 18:44:59 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 15.08.2012 18:13, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > --
> > v2:
> >   * replaced _hz suffix for frequency visitor by _freq suffix
> >     suggested-by: Andreas Färber
> >   * fixed typo, spotted-by: Andreas Färber
> > ---
> >  qapi/qapi-visit-core.c      | 11 +++++++++++
> >  qapi/qapi-visit-core.h      |  2 ++
> >  qapi/string-input-visitor.c | 22 ++++++++++++++++++++++
> >  3 files changed, 35 insertions(+)
> > 
> > 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..9ec0895 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)
> 
> Tiny glitch here, one space too much it seems.
> 
> If you fix that:
> Acked-by: Andreas Färber <afaerber@suse.de>
Done.
Thanks!
updated tree at: https://github.com/imammedo/qemu/tree/x86-cpu-properties.WIP

> 
> > +{
> > +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> > +    char *endp = (char *) siv->string;
> > +    long long val;
> > +
> > +    errno = 0;
> > +    if (siv->string) {
> > +        val = strtosz_suffix_unit(siv->string, &endp,
> > +                             STRTOSZ_DEFSUFFIX_B, 1000);
> 
> Please also check here...
> 
> > +    }
> > +    if (!siv->string || val == -1 || *endp) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
> > +              "a value representable as a non-negative int64");
> 
> ...and here.
> 
> > +        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;
> 
> Optionally reorder this, so that it's alongside the other type_*?
> 
> >  
> >      v->string = str;
> >      return v;
> > 
> 
> Thanks,
> Andreas
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 08/21] target-i386: convert 'hv_spinlocks' feature into property

[Eduardo Habkost]

On 15/08/2012, at 13:13, Igor Mammedov <imammedo@redhat.com> wrote:

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> target-i386/cpu.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 440e724..777b8ce 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -963,6 +963,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();

This breaks build if compiling without KVM support, as hyperv_get_spinlock_retries() is available only if CONFIG_KVM is set (hyperv_set_spinlock_retries(), on the other hand, is defined as an empty function if CONFIG_KVM is not set).


> +
> +    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;
> @@ -1919,6 +1945,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.11.2
> 

-- 
Eduardo	

Re: [Qemu-devel] [PATCH 08/21] target-i386: convert 'hv_spinlocks' feature into property

[Igor Mammedov]

On Wed, 15 Aug 2012 17:43:31 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 
> On 15/08/2012, at 13:13, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > target-i386/cpu.c | 31 +++++++++++++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 440e724..777b8ce 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -963,6 +963,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();
> 
> This breaks build if compiling without KVM support, as hyperv_get_spinlock_retries() is available only if CONFIG_KVM is set (hyperv_set_spinlock_retries(), on the other hand, is defined as an empty function if CONFIG_KVM is not set).
Fixed by https://github.com/imammedo/qemu/commit/3172153a73c6912bd90a7226a0343c6601ed1d04
on x86-cpu-properties.WIP branch.

Thanks,
  Igor
> 
> > +
> > +    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;
> > @@ -1919,6 +1945,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.11.2
> > 
> 
> -- 
> Eduardo	
> 
> 

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

[Eduardo Habkost]

On Wed, Aug 15, 2012 at 06:13:20PM +0200, Igor Mammedov wrote:
> build and run tested in FC17 host with x86_64-linux-user, x86_64-softmmu
> targets
> 
> changes since RFC are documented in idividual patch descriptions
> Thanks Andreas, Blue, Eduardo for reviews and suggestions.
> 
> git tree for testing: https://github.com/imammedo/qemu/tree/x86-cpu-properties_v1

FYI, I have rebase my work in progress on top of this, and on top of
other series that are on the list. The result is at:

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

On the branch above, the series are in this order:

- "-cpu help" fixes from Peter Maydell
- CPU DeviceState series from Anthony
- My "move CPU models to C" series
- Igor CPU properties series
- A new (unsubmitted) version of my CPU model classes series
- Work in progress branch (that I didn't submit yet) to simplify the
  handling of feature bits and feature names

> 
> Igor Mammedov (21):
>   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: 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           | 691 +++++++++++++++++++++++++++-----------------
>  target-i386/cpu.h           |  14 +-
>  target-i386/helper.c        |   9 +-
>  6 files changed, 479 insertions(+), 270 deletions(-)
> 
> -- 
> 1.7.11.2
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> 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
> ---
>  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 37ba5ef..440e724 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -606,6 +606,101 @@ 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;
> +    }

Some feature names are duplicated on feature_names and
ext_feature_names. On AMD CPU models, we have to set both, on Intel
models we need to set the bits only on cpuid_features.

Maybe it's better to:

1) eliminate the duplication and set the names only on feature_name
   array;
2) At the end of CPU initialization, set the features on
   cpuid_ext2_features as copies of the corresponding cpuid_features
   bits if CPU vendor == AMD (or, maybe, if some boolean
   "ext2_features_aliases" flag is set, to make it not
   vendor-dependent).


> +
> +    if (value) {
> +        *dst_features |= mask;
> +    } else {
> +        *dst_features &= ~mask;
> +    }
> +}
> +
> +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) {
> +                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);

What happens when object_property_add() is called twice with the same
feature name?

> +            }
> +        }
> +    }
> +}
> +
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
>  {
> @@ -1824,6 +1919,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.11.2
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Aug 16, 2012 at 03:10:50PM -0300, Eduardo Habkost wrote:
> On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > 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
> > ---
> >  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 37ba5ef..440e724 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -606,6 +606,101 @@ 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;
> > +    }
> 
> Some feature names are duplicated on feature_names and
> ext_feature_names. On AMD CPU models, we have to set both, on Intel
> models we need to set the bits only on cpuid_features.
> 
> Maybe it's better to:
> 
> 1) eliminate the duplication and set the names only on feature_name
>    array;
> 2) At the end of CPU initialization, set the features on
>    cpuid_ext2_features as copies of the corresponding cpuid_features
>    bits if CPU vendor == AMD (or, maybe, if some boolean
>    "ext2_features_aliases" flag is set, to make it not
>    vendor-dependent).

I implemented the above (after killing "cpudef" support), and pushed to:
https://github.com/ehabkost/qemu-hacks/tree/work/unduplicate-feature-names-v0.4-2012-08-17

I will submit it as an RFC soon.

I also made an experimental rebase of your series on top of that branch,
at:
https://github.com/ehabkost/qemu-hacks/tree/work/cpu-properties-igor-rebase-v3.4-2012-08-17
and also rebased my CPUID code work-in-progress, at:
https://github.com/ehabkost/qemu-hacks/tree/work/cpuid-refactor-v0.9-2012-08-15

> 
> 
> > +
> > +    if (value) {
> > +        *dst_features |= mask;
> > +    } else {
> > +        *dst_features &= ~mask;
> > +    }
> > +}
> > +
> > +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) {
> > +                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);
> 
> What happens when object_property_add() is called twice with the same
> feature name?
> 
> > +            }
> > +        }
> > +    }
> > +}
> > +
> >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
> >                                           const char *name, Error **errp)
> >  {
> > @@ -1824,6 +1919,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.11.2
> > 
> > 
> 
> -- 
> Eduardo
> 

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 16 Aug 2012 15:10:50 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > 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
> > ---
> >  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 37ba5ef..440e724 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -606,6 +606,101 @@ 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;
> > +    }
> 
> Some feature names are duplicated on feature_names and
> ext_feature_names. On AMD CPU models, we have to set both, on Intel
> models we need to set the bits only on cpuid_features.
> 
> Maybe it's better to:
> 
> 1) eliminate the duplication and set the names only on feature_name
>    array;
> 2) At the end of CPU initialization, set the features on
>    cpuid_ext2_features as copies of the corresponding cpuid_features
>    bits if CPU vendor == AMD (or, maybe, if some boolean
>    "ext2_features_aliases" flag is set, to make it not
>    vendor-dependent).

So far I was trying to keep original behavior and keep scope of this series
only on moving CPU features into properties. I'll rebase this series on top
of what you proposed in a week when I'm back from vacation.
 
> 
> > +
> > +    if (value) {
> > +        *dst_features |= mask;
> > +    } else {
> > +        *dst_features &= ~mask;
> > +    }
> > +}
> > +
> > +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) {
> > +                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);
> 
> What happens when object_property_add() is called twice with the same
> feature name?
Just extra item in list (pure waste). Thanks for pointing out.
I've fixed it (not yet pushed) by checking if property already exists before
adding it.

> 
> > +            }
> > +        }
> > +    }
> > +}
> > +
> >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
> >                                           const char *name, Error **errp)
> >  {
> > @@ -1824,6 +1919,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.11.2
> > 
> > 
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Mon, Aug 20, 2012 at 02:16:26PM +0200, Igor Mammedov wrote:
> On Thu, 16 Aug 2012 15:10:50 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > > 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
> > > ---
> > >  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 101 insertions(+)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 37ba5ef..440e724 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -606,6 +606,101 @@ 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;
> > > +    }
> > 
> > Some feature names are duplicated on feature_names and
> > ext_feature_names. On AMD CPU models, we have to set both, on Intel
> > models we need to set the bits only on cpuid_features.
> > 
> > Maybe it's better to:
> > 
> > 1) eliminate the duplication and set the names only on feature_name
> >    array;
> > 2) At the end of CPU initialization, set the features on
> >    cpuid_ext2_features as copies of the corresponding cpuid_features
> >    bits if CPU vendor == AMD (or, maybe, if some boolean
> >    "ext2_features_aliases" flag is set, to make it not
> >    vendor-dependent).
> 
> So far I was trying to keep original behavior and keep scope of this series
> only on moving CPU features into properties. I'll rebase this series on top
> of what you proposed in a week when I'm back from vacation.

Sure, keeping the behavior while changing the code to use properties
makes perfect sense. The only problem is that the current behavior is
incorrect and makes the property handling more complicated, so it makes
sense to fix it first.

>  
> > 
> > > +
> > > +    if (value) {
> > > +        *dst_features |= mask;
> > > +    } else {
> > > +        *dst_features &= ~mask;
> > > +    }
> > > +}
> > > +
> > > +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) {
> > > +                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);
> > 
> > What happens when object_property_add() is called twice with the same
> > feature name?
> Just extra item in list (pure waste). Thanks for pointing out.
> I've fixed it (not yet pushed) by checking if property already exists before
> adding it.
> 
> > 
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
> > >                                           const char *name, Error **errp)
> > >  {
> > > @@ -1824,6 +1919,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.11.2
> > > 
> > > 
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo