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

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

[Igor Mammedov]

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

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

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

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

-- 
1.7.11.4

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

[Igor Mammedov]

it will allow to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
--
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 ac12139..a89bdc4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1241,6 +1242,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;
@@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1401,6 +1412,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.4

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a89bdc4..3f80069 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1415,6 +1415,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.4

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Andreas Färber <afaerber@suse.de>
---
 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.4

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3f80069..567ad69 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1400,17 +1400,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
     }
 
-    if (!kvm_enabled()) {
-        env->cpuid_features &= TCG_FEATURES;
-        env->cpuid_ext_features &= TCG_EXT_FEATURES;
-        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
-#ifdef TARGET_X86_64
-            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
-#endif
-            );
-        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
-        env->cpuid_svm_features &= TCG_SVM_FEATURES;
-    }
+
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
 
 out:
@@ -1881,6 +1871,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.4

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

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

[Qemu-devel] [PATCH 07/22] 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
v3:
  * check if property exists before adding it
  * rebased on top of  "i386: cpu: remove duplicate feature names"
          http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
    place ext2_feature_name for AMD case into setter, so that not to
    clutter x86_cpu_realize() with property specific code.

fix for convert cpuid features
---
 target-i386/cpu.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 114 insertions(+), 10 deletions(-)

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

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ae3bc9d..7d45c6c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1203,6 +1203,32 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+#if !defined(CONFIG_USER_ONLY)
+static void x86_get_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    int64_t value = hyperv_get_spinlock_retries();
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void x86_set_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    int64_t value;
+
+    visit_type_int(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (!value) {
+        error_set(errp, QERR_PROPERTY_VALUE_BAD, "", name, "0");
+        return;
+    }
+    hyperv_set_spinlock_retries(value);
+}
+#endif
+
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
@@ -2034,6 +2060,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.4

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7d45c6c..6331eab 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1227,6 +1227,26 @@ static void x86_set_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
     }
     hyperv_set_spinlock_retries(value);
 }
+
+static void x86_get_hv_relaxed(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value = hyperv_relaxed_timing_enabled();
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    hyperv_enable_relaxed_timing(value);
+}
 #endif
 
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
@@ -2064,6 +2084,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.4

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6331eab..3b802ea 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1247,6 +1247,26 @@ static void x86_set_hv_relaxed(Object *obj, Visitor *v, void *opaque,
     }
     hyperv_enable_relaxed_timing(value);
 }
+
+static void x86_get_hv_vapic(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value = hyperv_vapic_recommended();
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    hyperv_enable_vapic_recommended(value);
+}
 #endif
 
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
@@ -2087,6 +2107,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.4

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b802ea..7ff9645 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -114,8 +114,8 @@ typedef struct model_features_t {
     uint32_t cpuid;
     } model_features_t;
 
-int check_cpuid = 0;
-int enforce_cpuid = 0;
+bool check_cpuid;
+bool enforce_cpuid;
 
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
@@ -807,19 +807,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
  * their way to the guest.  Note: ft[].check_feat ideally should be
  * specified via a guest_def field to suppress report of extraneous flags.
  */
-static int check_features_against_host(x86_def_t *guest_def)
+static int check_features_against_host(X86CPU *cpu)
 {
+    CPUX86State *env = &cpu->env;
     x86_def_t host_def;
     uint32_t mask;
     int rv, i;
     struct model_features_t ft[] = {
-        {&guest_def->features, &host_def.features,
+        {&env->cpuid_features, &host_def.features,
             ~0, feature_name, 0x00000000},
-        {&guest_def->ext_features, &host_def.ext_features,
+        {&env->cpuid_ext_features, &host_def.ext_features,
             ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
-        {&guest_def->ext2_features, &host_def.ext2_features,
+        {&env->cpuid_ext2_features, &host_def.ext2_features,
             ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
-        {&guest_def->ext3_features, &host_def.ext3_features,
+        {&env->cpuid_ext3_features, &host_def.ext3_features,
             ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
 
     cpu_x86_fill_host(&host_def);
@@ -1269,6 +1270,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque,
 }
 #endif
 
+static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    visit_type_bool(v, &check_cpuid, name, errp);
+}
+
+static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    check_cpuid = value;
+}
+
+static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    visit_type_bool(v, &enforce_cpuid, name, errp);
+}
+
+static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    enforce_cpuid = value;
+    object_property_set_bool(obj, value, "check", errp);
+}
+
 static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
@@ -1474,10 +1512,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;
 
@@ -2047,6 +2081,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;
@@ -2100,6 +2140,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.4

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Andreas Färber <afaerber@suse.de>
--
v2:
  * replaced _hz suffix for frequency visitor by _freq suffix
    suggested-by: Andreas Färber
  * fixed typo & extra space spotted-by: Andreas Färber
---
 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..47d2a84 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.4

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
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 7ff9645..1e10388 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1191,7 +1191,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     const int64_t max = 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.4

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

[Igor Mammedov]

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

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

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

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

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

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

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

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.

[ehabkost: rebase on top of my unduplicate-features branch]
[ehabkost: fix the new CPU models to use the string .vendor field, too,
 on the CPU model array]
[ehabkost: keep CPUID_VENDOR_AMD_[123] #defines, as they are used
 in the AMD CPU feature alias handling]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 92 +++++++++++++++----------------------------------------
 target-i386/cpu.h |  6 ++--
 2 files changed, 27 insertions(+), 71 deletions(-)

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

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

[Igor Mammedov]

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

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

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

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

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

[Igor Mammedov]

[ehabkost: rebase on top of latest qemu.git master, where default KVM
features are set differently

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2c9cd6b..be1be84 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -214,22 +214,6 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
     return found;
 }
 
-static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
-                                    uint32_t *ext_features,
-                                    uint32_t *ext2_features,
-                                    uint32_t *ext3_features,
-                                    uint32_t *kvm_features,
-                                    uint32_t *svm_features)
-{
-    if (!lookup_feature(features, flagname, NULL, feature_name) &&
-        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
-        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
-        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
-        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
-        !lookup_feature(svm_features, flagname, NULL, svm_feature_name))
-            fprintf(stderr, "CPU feature %s not found\n", flagname);
-}
-
 typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
@@ -1325,7 +1309,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_ext_features = def->ext_features;
     env->cpuid_ext2_features = def->ext2_features;
     env->cpuid_ext3_features = def->ext3_features;
-    env->cpuid_kvm_features = def->kvm_features;
     env->cpuid_svm_features = def->svm_features;
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
@@ -1384,22 +1367,34 @@ static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
     return;
 }
 
+/* Set features on X86CPU object based on a QDict */
+static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
+        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
+        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
+                              qdict_entry_key(ent), errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
     x86_def_t *def;
 
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
-    /* Features to be added*/
-    uint32_t plus_features = 0, plus_ext_features = 0;
-    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
-    /* Features to be removed */
-    uint32_t minus_features = 0, minus_ext_features = 0;
-    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
-    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
-    uint32_t numvalue;
+    CPUX86State *env = &cpu->env;
+    QDict *features = NULL;
+    char *name = NULL;
+
+    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
+    if (error_is_set(errp)) {
+        goto error;
+    }
 
     for (def = x86_defs; def; def = def->next)
         if (name && !strcmp(name, def->name))
@@ -1412,8 +1407,10 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
+    cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
+
 #if defined(CONFIG_KVM)
-    plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
+    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_NOP_IO_DELAY) | 
         (1 << KVM_FEATURE_MMU_OP) |
         (1 << KVM_FEATURE_CLOCKSOURCE2) |
@@ -1421,134 +1418,23 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
 #else
-    plus_kvm_features = 0;
+    env->cpuid_kvm_features = 0;
 #endif
 
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features, &plus_svm_features);
-
-    featurestr = strtok(NULL, ",");
+    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;
-        }
-
-        featurestr = strtok(NULL, ",");
+    cpu_x86_set_props(cpu, features, errp);
+    QDECREF(features);
+    if (error_is_set(errp)) {
+        goto error;
     }
-    x86_cpu_def->features |= plus_features;
-    x86_cpu_def->ext_features |= plus_ext_features;
-    x86_cpu_def->ext2_features |= plus_ext2_features;
-    x86_cpu_def->ext3_features |= plus_ext3_features;
-    x86_cpu_def->kvm_features |= plus_kvm_features;
-    x86_cpu_def->svm_features |= plus_svm_features;
-    x86_cpu_def->features &= ~minus_features;
-    x86_cpu_def->ext_features &= ~minus_ext_features;
-    x86_cpu_def->ext2_features &= ~minus_ext2_features;
-    x86_cpu_def->ext3_features &= ~minus_ext3_features;
-    x86_cpu_def->kvm_features &= ~minus_kvm_features;
-    x86_cpu_def->svm_features &= ~minus_svm_features;
-    g_free(s);
+
+    g_free(name);
     return 0;
 
 error:
-    g_free(s);
+    g_free(name);
+    QDECREF(features);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1641,8 +1527,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.4

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

[Igor Mammedov]

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

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index be1be84..fe67823 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1313,6 +1313,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
@@ -1421,8 +1423,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     env->cpuid_kvm_features = 0;
 #endif
 
-    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
-
     cpu_x86_set_props(cpu, features, errp);
     QDECREF(features);
     if (error_is_set(errp)) {
-- 
1.7.11.4

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

[Igor Mammedov]

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

[ehabkost: rebase on top of latest qemu.git master, where the bitmap
initialization is now different]
[imammedo: fix whitespace

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index fe67823..6063904 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1314,6 +1314,18 @@ 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;
 
+#if defined(CONFIG_KVM)
+    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
+        (1 << KVM_FEATURE_NOP_IO_DELAY) |
+        (1 << KVM_FEATURE_MMU_OP) |
+        (1 << KVM_FEATURE_CLOCKSOURCE2) |
+        (1 << KVM_FEATURE_ASYNC_PF) |
+        (1 << KVM_FEATURE_STEAL_TIME) |
+        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
+#else
+    env->cpuid_kvm_features = 0;
+#endif
+
     object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 }
 
@@ -1389,7 +1401,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 {
     x86_def_t *def;
 
-    CPUX86State *env = &cpu->env;
     QDict *features = NULL;
     char *name = NULL;
 
@@ -1411,18 +1422,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 
     cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
 
-#if defined(CONFIG_KVM)
-    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
-        (1 << KVM_FEATURE_NOP_IO_DELAY) | 
-        (1 << KVM_FEATURE_MMU_OP) |
-        (1 << KVM_FEATURE_CLOCKSOURCE2) |
-        (1 << KVM_FEATURE_ASYNC_PF) | 
-        (1 << KVM_FEATURE_STEAL_TIME) |
-        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-#else
-    env->cpuid_kvm_features = 0;
-#endif
-
     cpu_x86_set_props(cpu, features, errp);
     QDECREF(features);
     if (error_is_set(errp)) {
-- 
1.7.11.4

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

[Igor Mammedov]

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

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

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

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

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

[Igor Mammedov]

On Fri,  7 Sep 2012 22:54:49 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> git tree for testing:
>   https://github.com/imammedo/qemu/tree/x86-cpu-properties.v2
> 
here are series it depends on:

[Qemu-devel] [PATCH 0/7] x86 CPU patches that didn't get into 1.2
http://www.mail-archive.com/qemu-devel@nongnu.org/msg129240.html

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

-- 
Regards,
  Igor

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

[Don Slutz]

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

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

And the change:

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

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

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

fixes this for me.

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

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

[Don Slutz]

On 09/07/12 16:55, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Acked-by: Andreas Färber <afaerber@suse.de>
> --
> v2:
>    * replaced _hz suffix for frequency visitor by _freq suffix
>      suggested-by: Andreas Färber
>    * fixed typo & extra space spotted-by: Andreas Färber
> ---
>   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..47d2a84 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;
I get:

cc1: warnings being treated as errors
qapi/string-input-visitor.c: In function 'parse_type_freq':
qapi/string-input-visitor.c:118: error: 'val' may be used uninitialized 
in this function
make: *** [qapi/string-input-visitor.o] Error 1
make: *** Waiting for unfinished jobs....

Which the change:


diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 47d2a84..74fe395 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -115,7 +115,7 @@ static void parse_type_freq(Visitor *v, int64_t 
*obj, const char *name,
  {
      StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
      char *endp = (char *) siv->string;
-    long long val;
+    long long val = 0;

      errno = 0;
      if (siv->string) {

Fixes it for me.

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

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

[Igor Mammedov]

On Fri, 7 Sep 2012 18:12:00 -0400
Don Slutz <Don@cloudswitch.com> wrote:

> On 09/07/12 16:55, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Acked-by: Andreas Färber <afaerber@suse.de>
> > --
> > v2:
> >    * replaced _hz suffix for frequency visitor by _freq suffix
> >      suggested-by: Andreas Färber
> >    * fixed typo & extra space spotted-by: Andreas Färber
> > ---
> >   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..47d2a84 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;
> I get:
> 
> cc1: warnings being treated as errors
> qapi/string-input-visitor.c: In function 'parse_type_freq':
> qapi/string-input-visitor.c:118: error: 'val' may be used uninitialized 
> in this function
> make: *** [qapi/string-input-visitor.o] Error 1
> make: *** Waiting for unfinished jobs....

FC17 with default configure settings doesn't complain.
And I really do not see how it could be.

> Which the change:
> 
> 
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 47d2a84..74fe395 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -115,7 +115,7 @@ static void parse_type_freq(Visitor *v, int64_t 
> *obj, const char *name,
>   {
>       StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
>       char *endp = (char *) siv->string;
> -    long long val;
> +    long long val = 0;
>       errno = 0;
>       if (siv->string) {
> 
> Fixes it for me.
> 
> > +
> > +    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;
>    -Don Slutz


-- 
Regards,
  Igor

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

[Don Slutz]

On 09/07/12 18:47, Igor Mammedov wrote:
> On Fri, 7 Sep 2012 18:12:00 -0400
> Don Slutz <Don@cloudswitch.com> wrote:
>
>> On 09/07/12 16:55, Igor Mammedov wrote:
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> Acked-by: Andreas Färber <afaerber@suse.de>
>>> --
>>> v2:
>>>     * replaced _hz suffix for frequency visitor by _freq suffix
>>>       suggested-by: Andreas Färber
>>>     * fixed typo & extra space spotted-by: Andreas Färber
>>> ---
>>>    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..47d2a84 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;
>> I get:
>>
>> cc1: warnings being treated as errors
>> qapi/string-input-visitor.c: In function 'parse_type_freq':
>> qapi/string-input-visitor.c:118: error: 'val' may be used uninitialized
>> in this function
>> make: *** [qapi/string-input-visitor.o] Error 1
>> make: *** Waiting for unfinished jobs....
> FC17 with default configure settings doesn't complain.
> And I really do not see how it could be.
>
>> Which the change:
>>
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index 47d2a84..74fe395 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -115,7 +115,7 @@ static void parse_type_freq(Visitor *v, int64_t
>> *obj, const char *name,
>>    {
>>        StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
>>        char *endp = (char *) siv->string;
>> -    long long val;
>> +    long long val = 0;
>>        errno = 0;
>>        if (siv->string) {
>>
>> Fixes it for me.
>>
>>> +
>>> +    errno = 0;
>>> +    if (siv->string) {
>>> +        val = strtosz_suffix_unit(siv->string, &endp,
>>> +                             STRTOSZ_DEFSUFFIX_B, 1000);
>>> +    }
>>> +    if (!siv->string || val == -1 || *endp) {
I am using CentOS 6.3 so a different compiler.  This is the line that 
has the issue.

If !siv->string is true the 1st if does not set val. val is then checked 
for -1.

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

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

[Don Slutz]

Don Slutz wrote:
> On 09/07/12 18:47, Igor Mammedov wrote:
>> On Fri, 7 Sep 2012 18:12:00 -0400
>> Don Slutz <Don@cloudswitch.com> wrote:
>>
>>> On 09/07/12 16:55, Igor Mammedov wrote:
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> Acked-by: Andreas Färber <afaerber@suse.de>
>>>> -- 
>>>> v2:
>>>>     * replaced _hz suffix for frequency visitor by _freq suffix
>>>>       suggested-by: Andreas Färber
>>>>     * fixed typo & extra space spotted-by: Andreas Färber
>>>> ---
>>>>    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..47d2a84 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;
>>> I get:
>>>
>>> cc1: warnings being treated as errors
>>> qapi/string-input-visitor.c: In function 'parse_type_freq':
>>> qapi/string-input-visitor.c:118: error: 'val' may be used uninitialized
>>> in this function
>>> make: *** [qapi/string-input-visitor.o] Error 1
>>> make: *** Waiting for unfinished jobs....
>> FC17 with default configure settings doesn't complain.
>> And I really do not see how it could be.
>>
>>> Which the change:
>>>
>>>
>>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>>> index 47d2a84..74fe395 100644
>>> --- a/qapi/string-input-visitor.c
>>> +++ b/qapi/string-input-visitor.c
>>> @@ -115,7 +115,7 @@ static void parse_type_freq(Visitor *v, int64_t
>>> *obj, const char *name,
>>>    {
>>>        StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, 
>>> visitor, v);
>>>        char *endp = (char *) siv->string;
>>> -    long long val;
>>> +    long long val = 0;
>>>        errno = 0;
>>>        if (siv->string) {
>>>
>>> Fixes it for me.
>>>
>>>> +
>>>> +    errno = 0;
>>>> +    if (siv->string) {
>>>> +        val = strtosz_suffix_unit(siv->string, &endp,
>>>> +                             STRTOSZ_DEFSUFFIX_B, 1000);
>>>> +    }
>>>> +    if (!siv->string || val == -1 || *endp) {
> I am using CentOS 6.3 so a different compiler.  This is the line that 
> has the issue.
>
> If !siv->string is true the 1st if does not set val. val is then 
> checked for -1.
Opps, This is not correct.   I was going too fast.  After more thought, 
I will agree that C says that val will not be used un-initialized.  So 
it looks to me like a compiler bug.  Since the warning says "val' may be 
used uninitialized.." gcc is "not" reporting a real coding error.

This all said, I think the extra init of val (to 0 or -1) is better then 
requiring a compiler upgrade.
>
>>>> +        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;
>>>     -Don Slutz
>>
>   -Don Slutz
>
  -Don Slutz

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

[Andreas Färber]

Am 07.09.2012 22:55, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Thanks,

Reviewed-by: Andreas Färber <afaerber@suse.de>

> v2:
>   * use visit_type_freq() which replaced visit_type_hz()

Change Logs are usually requested to go in the cover letter or below ---
so it does not go into git history.

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 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Don Slutz]

On 09/07/12 16:54, Igor Mammedov wrote:
> 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 ac12139..a89bdc4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>       cpu->env.tsc_khz = value / 1000;
>   }
>   
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> +                                const char *cpu_model, Error **errp)
>   {
>       unsigned int i;
>       x86_def_t *def;
> @@ -1241,6 +1242,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;
> @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>   
>   error:
>       g_free(s);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +    }
>       return -1;
>   }
>   
> @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>   
>       memset(def, 0, sizeof(*def));
>   
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>       if (def->vendor1) {
>           env->cpuid_vendor1 = def->vendor1;
>           env->cpuid_vendor2 = def->vendor2;
> @@ -1401,6 +1412,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;
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

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

[Eduardo Habkost]

On Fri, Sep 07, 2012 at 10:54:50PM +0200, Igor Mammedov wrote:
> 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 ac12139..a89bdc4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> +                                const char *cpu_model, Error **errp)
>  {
>      unsigned int i;
>      x86_def_t *def;
> @@ -1241,6 +1242,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;
> +        }
> +

Why add this check, if you never use errp on any code above that line
(and eventually all the code above gets removed by patch 19)?

But is harmless (as error is never set anyway), and this is code that
simply gets removed later in this series. So:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>          featurestr = strtok(NULL, ",");
>      }
>      x86_cpu_def->features |= plus_features;
> @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>  
>  error:
>      g_free(s);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +    }
>      return -1;
>  }
>  
> @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      memset(def, 0, sizeof(*def));
>  
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>      if (def->vendor1) {
>          env->cpuid_vendor1 = def->vendor1;
>          env->cpuid_vendor2 = def->vendor2;
> @@ -1401,6 +1412,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.4
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Sep 07, 2012 at 10:54:51PM +0200, Igor Mammedov wrote:
> 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 a89bdc4..3f80069 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1415,6 +1415,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>  out:
>      if (error_is_set(&error)) {

Isn't "if (error_is_set(&error)) better written as "if (error)"?

There are lots of places where "error_is_set(&error)" is used, but I saw
other QEMU code using "if (error)" before, so I don't know what's the
recommended style.

Anyway, the point of this patch is to add the error message, not
cleaning up the existing code. So:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> +        fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
>          return -1;
>      }
> -- 
> 1.7.11.4
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Sep 07, 2012 at 10:54:52PM +0200, Igor Mammedov wrote:
> 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)) {

Can't this be just written as "if (error)"?

(Same question as in the previous patch, but as this is new code I would
like to clarify what's the recommended style, before ACKing it)


> +        error_free(error);
> +        object_delete(OBJECT(cpu));
> +        return NULL;
> +    }
>      return cpu;
>  }
>  
> -- 
> 1.7.11.4
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Sep 07, 2012 at 10:54:53PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

(The Reviewed-by line is present above already, but I am just confirming
that this refresh looks OK to me)

> ---
>  target-i386/cpu.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3f80069..567ad69 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1400,17 +1400,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
>      }
>  
> -    if (!kvm_enabled()) {
> -        env->cpuid_features &= TCG_FEATURES;
> -        env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
> -#ifdef TARGET_X86_64
> -            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
> -#endif
> -            );
> -        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
> -        env->cpuid_svm_features &= TCG_SVM_FEATURES;
> -    }
> +
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>  
>  out:
> @@ -1881,6 +1871,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.4
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Sep 07, 2012 at 10:54:54PM +0200, Igor Mammedov wrote:
> 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>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

(Just confirming that the patch refresh looks OK to me)


> --
> v2:
>    - rebased on top of  "i386: cpu: remove duplicate feature names"
>       http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
> ---
>  target-i386/cpu.c | 84 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 44 insertions(+), 40 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 567ad69..ff8c78e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,6 +1086,49 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> +static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> +{
> +    CPUX86State *env = &cpu->env;
> +
> +    if (def->vendor1) {
> +        env->cpuid_vendor1 = def->vendor1;
> +        env->cpuid_vendor2 = def->vendor2;
> +        env->cpuid_vendor3 = def->vendor3;
> +    } else {
> +        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
> +        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
> +        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
> +    }
> +    env->cpuid_vendor_override = def->vendor_override;
> +    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
> +    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
> +    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
> +    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> +                            "tsc-frequency", errp);
> +    env->cpuid_features = def->features;
> +    env->cpuid_ext_features = def->ext_features;
> +    env->cpuid_ext2_features = def->ext2_features;
> +    env->cpuid_ext3_features = def->ext3_features;
> +    env->cpuid_kvm_features = def->kvm_features;
> +    env->cpuid_svm_features = def->svm_features;
> +    env->cpuid_ext4_features = def->ext4_features;
> +    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> +    env->cpuid_xlevel2 = def->xlevel2;
> +
> +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> +     * CPUID[1].EDX.
> +     */
> +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> +            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> +            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> +        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> +        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
> +    }
> +}
> +
>  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>                                  const char *cpu_model, Error **errp)
>  {
> @@ -1353,7 +1396,6 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
> -    CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>  
> @@ -1363,45 +1405,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto out;
>      }
>  
> -    if (def->vendor1) {
> -        env->cpuid_vendor1 = def->vendor1;
> -        env->cpuid_vendor2 = def->vendor2;
> -        env->cpuid_vendor3 = def->vendor3;
> -    } else {
> -        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
> -        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
> -        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
> -    }
> -    env->cpuid_vendor_override = def->vendor_override;
> -    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> -    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> -    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> -    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
> -    env->cpuid_features = def->features;
> -    env->cpuid_ext_features = def->ext_features;
> -    env->cpuid_ext2_features = def->ext2_features;
> -    env->cpuid_ext3_features = def->ext3_features;
> -    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
> -    env->cpuid_kvm_features = def->kvm_features;
> -    env->cpuid_svm_features = def->svm_features;
> -    env->cpuid_ext4_features = def->ext4_features;
> -    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> -    env->cpuid_xlevel2 = def->xlevel2;
> -    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> -                            "tsc-frequency", &error);
> -
> -    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> -     * CPUID[1].EDX.
> -     */
> -    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> -            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> -            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> -        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> -        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
> -    }
> -
> -
> -    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +    cpudef_2_x86_cpu(cpu, def, &error);
>  
>  out:
>      if (error_is_set(&error)) {
> -- 
> 1.7.11.4
> 
> 

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 13 Sep 2012 11:38:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Sep 07, 2012 at 10:54:50PM +0200, Igor Mammedov wrote:
> > 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 ac12139..a89bdc4 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      cpu->env.tsc_khz = value / 1000;
> >  }
> >  
> > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> > +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> > +                                const char *cpu_model, Error **errp)
> >  {
> >      unsigned int i;
> >      x86_def_t *def;
> > @@ -1241,6 +1242,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;
> > +        }
> > +
> 
> Why add this check, if you never use errp on any code above that line
> (and eventually all the code above gets removed by patch 19)?
It looks like remnants from initial version where each converted feature had
setter in this loop. 
I'll remove it for next respin.

> 
> But is harmless (as error is never set anyway), and this is code that
> simply gets removed later in this series. So:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> >          featurestr = strtok(NULL, ",");
> >      }
> >      x86_cpu_def->features |= plus_features;
> > @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> >  
> >  error:
> >      g_free(s);
> > +    if (!error_is_set(errp)) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> > +    }
> >      return -1;
> >  }
> >  
> > @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >  
> >      memset(def, 0, sizeof(*def));
> >  
> > -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> > -        return -1;
> > +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> > +        goto out;
> > +    }
> > +
> >      if (def->vendor1) {
> >          env->cpuid_vendor1 = def->vendor1;
> >          env->cpuid_vendor2 = def->vendor2;
> > @@ -1401,6 +1412,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.4
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

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

[Igor Mammedov]

On Thu, 13 Sep 2012 11:40:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Sep 07, 2012 at 10:54:51PM +0200, Igor Mammedov wrote:
> > 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 a89bdc4..3f80069 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1415,6 +1415,7 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) 
> >  out:
> >      if (error_is_set(&error)) {
> 
> Isn't "if (error_is_set(&error)) better written as "if (error)"?
> 
> There are lots of places where "error_is_set(&error)" is used, but I saw
> other QEMU code using "if (error)" before, so I don't know what's the
> recommended style.
If there aren't any objections, I'll change it to suggested "if (error)"
style, for the next respin.

> 
> Anyway, the point of this patch is to add the error message, not
> cleaning up the existing code. So:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> > +        fprintf(stderr, "%s\n", error_get_pretty(error));
> >          error_free(error);
> >          return -1;
> >      }
> > -- 
> > 1.7.11.4
> > 
> > 
> 

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

[Igor Mammedov]

On Mon, 10 Sep 2012 15:20:52 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 07.09.2012 22:55, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Thanks,
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> > v2:
> >   * use visit_type_freq() which replaced visit_type_hz()
> 
> Change Logs are usually requested to go in the cover letter or below ---
> so it does not go into git history.
Thanks,

I'll move it below ---

> 
> Andreas
> 

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

[Igor Mammedov]

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

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