[Qemu-devel] [RFC 00/20] target-i386: convert CPU features into properties

[Qemu-devel] [RFC 00/20] 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

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

 qapi/qapi-visit-core.c      |  11 +
 qapi/qapi-visit-core.h      |   2 +
 qapi/string-input-visitor.c |  22 ++
 target-i386/cpu.c           | 669 +++++++++++++++++++++++++++-----------------
 target-i386/cpu.h           |  14 +-
 target-i386/helper.c        |   9 +-
 6 files changed, 464 insertions(+), 263 deletions(-)

-- 
1.7.11.2

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 880cfea..ee25309 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -858,7 +858,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;
@@ -1003,6 +1004,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;
@@ -1026,6 +1032,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;
 }
 
@@ -1133,8 +1142,9 @@ 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;
@@ -1173,6 +1183,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;
-- 
1.7.11.2

[Qemu-devel] [RFC 02/20] 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 ee25309..17e98e1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1186,6 +1186,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
 out:
     if (error_is_set(&error)) {
+        fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
         return -1;
     }
-- 
1.7.11.2

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 17e98e1..d0dec63 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1171,17 +1171,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     env->cpuid_xlevel2 = def->xlevel2;
     object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
                             "tsc-frequency", &error);
-    if (!kvm_enabled()) {
-        env->cpuid_features &= TCG_FEATURES;
-        env->cpuid_ext_features &= TCG_EXT_FEATURES;
-        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
-#ifdef TARGET_X86_64
-            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
-#endif
-            );
-        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
-        env->cpuid_svm_features &= TCG_SVM_FEATURES;
-    }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
 
 out:
@@ -1745,6 +1734,19 @@ static void mce_init(X86CPU *cpu)
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+
+    if (!kvm_enabled()) {
+        env->cpuid_features &= TCG_FEATURES;
+        env->cpuid_ext_features &= TCG_EXT_FEATURES;
+        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
+#ifdef TARGET_X86_64
+            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
+#endif
+            );
+        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
+        env->cpuid_svm_features &= TCG_SVM_FEATURES;
+    }
 
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
-- 
1.7.11.2

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d0dec63..783c6f4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -858,6 +858,39 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
+{
+    CPUX86State *env = &cpu->env;
+
+    if (def->vendor1) {
+        env->cpuid_vendor1 = def->vendor1;
+        env->cpuid_vendor2 = def->vendor2;
+        env->cpuid_vendor3 = def->vendor3;
+    } else {
+        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
+        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
+        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
+    }
+    env->cpuid_vendor_override = def->vendor_override;
+    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
+    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
+    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
+    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
+    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
+    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
+                            "tsc-frequency", errp);
+    env->cpuid_features = def->features;
+    env->cpuid_ext_features = def->ext_features;
+    env->cpuid_ext2_features = def->ext2_features;
+    env->cpuid_ext3_features = def->ext3_features;
+    env->cpuid_kvm_features = def->kvm_features;
+    env->cpuid_svm_features = def->svm_features;
+    env->cpuid_ext4_features = def->ext4_features;
+    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
+    env->cpuid_xlevel2 = def->xlevel2;
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
@@ -1136,7 +1169,6 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
 
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
-    CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
 
@@ -1145,33 +1177,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     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;
-        env->cpuid_vendor3 = def->vendor3;
-    } else {
-        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
-        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
-        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
-    }
-    env->cpuid_vendor_override = def->vendor_override;
-    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
-    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
-    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
-    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
-    env->cpuid_features = def->features;
-    env->cpuid_ext_features = def->ext_features;
-    env->cpuid_ext2_features = def->ext2_features;
-    env->cpuid_ext3_features = def->ext3_features;
-    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
-    env->cpuid_kvm_features = def->kvm_features;
-    env->cpuid_svm_features = def->svm_features;
-    env->cpuid_ext4_features = def->ext4_features;
-    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
-    env->cpuid_xlevel2 = def->xlevel2;
-    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
-                            "tsc-frequency", &error);
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    cpudef_2_x86_cpu(cpu, def, &error);
 
 out:
     if (error_is_set(&error)) {
-- 
1.7.11.2

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@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 783c6f4..a47cc12 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -745,8 +745,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)
@@ -981,9 +990,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                     fprintf(stderr, "bad numerical value %s\n", val);
                     goto error;
                 }
-                if (numvalue < 0x80000000) {
-                    numvalue += 0x80000000;
-                }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
                 if (strlen(val) != 12) {
-- 
1.7.11.2

[Qemu-devel] [RFC 07/20] target-i386: convert cpuid features into properties

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a47cc12..4b22598 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -605,6 +605,103 @@ 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 mask;
+
+    for (mask = 1; mask; mask <<= 1) {
+        if (featureset[ffs(mask) - 1] &&
+            !altcmp(name, NULL, featureset[ffs(mask) - 1])) {
+            break;
+    }
+    }
+    if (featbitmap & mask) {
+        return true;
+    }
+    return false;
+}
+
+static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    bool value = true;
+
+    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
+       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
+       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
+       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
+       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
+       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
+        value = false;
+    }
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque,
+                                         const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    uint32_t mask = 0;
+    uint32_t *dst_features;
+    bool value;
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (lookup_feature(&mask, name, NULL, feature_name)) {
+        dst_features = &env->cpuid_features;
+    } else if (lookup_feature(&mask, name, NULL, ext_feature_name)) {
+        dst_features = &env->cpuid_ext_features;
+    } else if (lookup_feature(&mask, name, NULL, ext2_feature_name)) {
+        dst_features = &env->cpuid_ext2_features;
+    } else if (lookup_feature(&mask, name, NULL, ext3_feature_name)) {
+        dst_features = &env->cpuid_ext3_features;
+    } else if (lookup_feature(&mask, name, NULL, kvm_feature_name)) {
+        dst_features = &env->cpuid_kvm_features;
+    } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) {
+        dst_features = &env->cpuid_svm_features;
+    } else {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
+        return;
+    }
+
+    if (value) {
+        *dst_features |= mask;
+    } else {
+        *dst_features &= ~mask;
+    }
+}
+
+static void x86_register_cpuid_properties(Object *obj, const char **featureset)
+{
+    uint32_t mask;
+
+    for (mask = 1; mask; mask <<= 1) {
+        if (featureset[ffs(mask) - 1]) {
+            char *feature_name, *save_ptr;
+            char buf[32];
+            if (strlen(featureset[ffs(mask) - 1]) > sizeof(buf) - 1) {
+                abort();
+            }
+            pstrcpy(buf, sizeof(buf), featureset[ffs(mask) - 1]);
+            feature_name = strtok_r(buf, "|", &save_ptr);
+            while (feature_name) {
+                object_property_add(obj, feature_name, "bool",
+                                x86_cpuid_get_feature,
+                                x86_cpuid_set_feature, NULL, NULL, NULL);
+                feature_name = strtok_r(NULL, "|", &save_ptr);
+            }
+        }
+    }
+}
+
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
 {
@@ -1801,6 +1898,12 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    x86_register_cpuid_properties(obj, feature_name);
+    x86_register_cpuid_properties(obj, ext_feature_name);
+    x86_register_cpuid_properties(obj, ext2_feature_name);
+    x86_register_cpuid_properties(obj, ext3_feature_name);
+    x86_register_cpuid_properties(obj, kvm_feature_name);
+    x86_register_cpuid_properties(obj, svm_feature_name);
 
     env->cpuid_apic_id = env->cpu_index;
 
-- 
1.7.11.2

[Qemu-devel] [RFC 08/20] 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 4b22598..fb98bbd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -964,6 +964,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;
@@ -1898,6 +1924,11 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+#if !defined(CONFIG_USER_ONLY)
+    object_property_add(obj, "hv_spinlocks", "int",
+                        x86_get_hv_spinlocks,
+                        x86_set_hv_spinlocks, NULL, NULL, NULL);
+#endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
     x86_register_cpuid_properties(obj, ext2_feature_name);
-- 
1.7.11.2

[Qemu-devel] [RFC 09/20] 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 fb98bbd..f73309e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -988,6 +988,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)
@@ -1928,6 +1948,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "hv_spinlocks", "int",
                         x86_get_hv_spinlocks,
                         x86_set_hv_spinlocks, NULL, NULL, NULL);
+    object_property_add(obj, "hv_relaxed", "bool",
+                        x86_get_hv_relaxed,
+                        x86_set_hv_relaxed, NULL, NULL, NULL);
 #endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
-- 
1.7.11.2

[Qemu-devel] [RFC 10/20] 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 f73309e..7734613 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1008,6 +1008,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)
@@ -1951,6 +1971,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "hv_relaxed", "bool",
                         x86_get_hv_relaxed,
                         x86_set_hv_relaxed, NULL, NULL, NULL);
+    object_property_add(obj, "hv_vapic", "bool",
+                        x86_set_hv_vapic,
+                        x86_get_hv_vapic, NULL, NULL, NULL);
 #endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
-- 
1.7.11.2

[Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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 7734613..a154e89 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -106,8 +106,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)
@@ -579,19 +579,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);
@@ -1030,6 +1031,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;
@@ -1225,10 +1263,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;
 
@@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
 
+    if (check_cpuid && check_features_against_host(cpu)
+        && enforce_cpuid) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 #endif
@@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    object_property_add(obj, "check", "bool",
+                        x86_cpuid_get_check,
+                        x86_cpuid_set_check, NULL, NULL, NULL);
+    object_property_add(obj, "enforce", "bool",
+                        x86_cpuid_get_enforce,
+                        x86_cpuid_set_enforce, NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
     object_property_add(obj, "hv_spinlocks", "int",
                         x86_get_hv_spinlocks,
-- 
1.7.11.2

[Qemu-devel] [RFC 12/20] add visitor for parsing hz[KMG] input string

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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..322cfa6 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_hz(Visitor *v, int64_t *obj, const char *name, Error **errp)
+{
+    if (!error_is_set(errp)) {
+        if (v->type_hz) {
+            v->type_hz(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..29d3038 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_hz)(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_hz(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..32e3780 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_hz(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 representible 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_hz = parse_type_hz;
 
     v->string = str;
     return v;
-- 
1.7.11.2

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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 a154e89..c935c04 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -952,7 +952,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_hz(v, &value, name, errp);
     if (error_is_set(errp)) {
         return;
     }
-- 
1.7.11.2

[Qemu-devel] [RFC 14/20] 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 c935c04..d3ce849 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -894,7 +894,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)
@@ -1068,6 +1067,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;
@@ -1081,7 +1105,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);
@@ -1998,6 +2022,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_str(obj, "vendor",
                             x86_cpuid_get_vendor,
                             x86_cpuid_set_vendor, NULL);
+    object_property_add(obj, "vendor-override", "bool",
+                        x86_cpuid_get_vendor_override,
+                        x86_cpuid_set_vendor_override, NULL, NULL, NULL);
     object_property_add_str(obj, "model-id",
                             x86_cpuid_get_model_id,
                             x86_cpuid_set_model_id, NULL);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 60f9e97..979682a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -739,7 +739,7 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
+    bool cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.11.2

[Qemu-devel] [RFC 15/20] 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 d3ce849..368360f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -863,13 +863,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;
 }
 
@@ -880,7 +880,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
     CPUX86State *env = &cpu->env;
     int i;
 
-    if (strlen(value) != 12) {
+    if (strlen(value) != CPUID_VENDOR_SZ) {
         error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
                   "vendor", value);
         return;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 979682a..5c75704 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -440,6 +440,8 @@
 #define CPUID_SVM_PAUSEFILTER  (1 << 10)
 #define CPUID_SVM_PFTHRESHOLD  (1 << 12)
 
+#define CPUID_VENDOR_SZ      12
+
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
-- 
1.7.11.2

[Qemu-devel] [RFC 16/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

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

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

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 368360f..6f1b66e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -226,7 +226,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;
@@ -292,9 +292,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,
@@ -311,9 +309,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,
@@ -357,9 +353,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,
@@ -458,9 +452,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,
@@ -506,13 +498,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);
@@ -537,9 +532,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 */
@@ -1096,15 +1089,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);
@@ -1128,7 +1114,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);
@@ -1217,18 +1202,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),
@@ -1362,10 +1336,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
             (*cpu_fprintf)(f, "x86 %16s\n", buf);
         }
         if (dump) {
-            memcpy(buf, &def->vendor1, sizeof (def->vendor1));
-            memcpy(buf + 4, &def->vendor2, sizeof (def->vendor2));
-            memcpy(buf + 8, &def->vendor3, sizeof (def->vendor3));
-            buf[12] = '\0';
+            pstrcpy(buf, sizeof(def->vendor), def->vendor);
             (*cpu_fprintf)(f,
                 "  family %d model %d stepping %d level %d xlevel 0x%x"
                 " vendor \"%s\"\n",
@@ -1416,17 +1387,6 @@ out:
 }
 
 #if !defined(CONFIG_USER_ONLY)
-/* copy vendor id string to 32 bit register, nul pad as needed
- */
-static void cpyid(const char *s, uint32_t *id)
-{
-    char *d = (char *)id;
-    char i;
-
-    for (i = sizeof (*id); i--; )
-        *d++ = *s ? *s++ : '\0';
-}
-
 /* interpret radix and convert from string to arbitrary scalar,
  * otherwise flag failure
  */
@@ -1478,9 +1438,7 @@ static int cpudef_setfield(const char *name, const char *str, void *opaque)
     } else if (!strcmp(name, "level")) {
         setscalar(&def->level, str, &err)
     } else if (!strcmp(name, "vendor")) {
-        cpyid(&str[0], &def->vendor1);
-        cpyid(&str[4], &def->vendor2);
-        cpyid(&str[8], &def->vendor3);
+        pstrcpy(def->vendor, sizeof(def->vendor), str);
     } else if (!strcmp(name, "family")) {
         setscalar(&def->family, str, &err)
     } else if (!strcmp(name, "model")) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5c75704..5543496 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -445,14 +445,10 @@
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+#define CPUID_VENDOR_INTEL "GenuineIntel"
 
-#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
-#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
-#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
-
-#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
+#define CPUID_VENDOR_VIA   "CentaurHauls"
 
 #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
-- 
1.7.11.2

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

[Qemu-devel] [RFC 18/20] target-i386: use properties to set/unset user specified features on CPU

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7555b08..0174c4d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -206,22 +206,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;
@@ -1104,7 +1088,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;
@@ -1168,17 +1151,14 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 {
     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;
+    QDict *features;
+    const QDictEntry *ent;
+    char *name;
+
+    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
+    if (error_is_set(errp)) {
+        goto error;
+    }
 
     for (def = x86_defs; def; def = def->next)
         if (name && !strcmp(name, def->name))
@@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
-    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
-
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features, &plus_svm_features);
+    cpudef_2_x86_cpu(cpu, def, errp);
 
-    featurestr = strtok(NULL, ",");
+    /* not supported bits will be filtered out later */
+    env->cpuid_kvm_features = ~0;
 
-    while (featurestr) {
-        char *val;
-        if (featurestr[0] == '+') {
-            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
-                            &plus_ext_features, &plus_ext2_features,
-                            &plus_ext3_features, &plus_kvm_features,
-                            &plus_svm_features);
-        } else if (featurestr[0] == '-') {
-            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
-                            &minus_ext_features, &minus_ext2_features,
-                            &minus_ext3_features, &minus_kvm_features,
-                            &minus_svm_features);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
-            if (!strcmp(featurestr, "family")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff + 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->family = numvalue;
-            } else if (!strcmp(featurestr, "model")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->model = numvalue;
-            } else if (!strcmp(featurestr, "stepping")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->stepping = numvalue ;
-            } else if (!strcmp(featurestr, "level")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->level = numvalue;
-            } else if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->xlevel = numvalue;
-            } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
-            } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
-            } else if (!strcmp(featurestr, "tsc_freq")) {
-                int64_t tsc_freq;
-                char *err;
-
-                tsc_freq = strtosz_suffix_unit(val, &err,
-                                               STRTOSZ_DEFSUFFIX_B, 1000);
-                if (tsc_freq < 0 || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
-            } else if (!strcmp(featurestr, "hv_spinlocks")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                hyperv_set_spinlock_retries(numvalue);
-            } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
-                goto error;
-            }
-        } else if (!strcmp(featurestr, "check")) {
-            check_cpuid = 1;
-        } else if (!strcmp(featurestr, "enforce")) {
-            check_cpuid = enforce_cpuid = 1;
-        } else if (!strcmp(featurestr, "hv_relaxed")) {
-            hyperv_enable_relaxed_timing(true);
-        } else if (!strcmp(featurestr, "hv_vapic")) {
-            hyperv_enable_vapic_recommended(true);
-        } else {
-            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
-            goto error;
-        }
+    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
+    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)) {
             goto error;
         }
-
-        featurestr = strtok(NULL, ",");
     }
-    x86_cpu_def->features |= plus_features;
-    x86_cpu_def->ext_features |= plus_ext_features;
-    x86_cpu_def->ext2_features |= plus_ext2_features;
-    x86_cpu_def->ext3_features |= plus_ext3_features;
-    x86_cpu_def->kvm_features |= plus_kvm_features;
-    x86_cpu_def->svm_features |= plus_svm_features;
-    x86_cpu_def->features &= ~minus_features;
-    x86_cpu_def->ext_features &= ~minus_ext_features;
-    x86_cpu_def->ext2_features &= ~minus_ext2_features;
-    x86_cpu_def->ext3_features &= ~minus_ext3_features;
-    x86_cpu_def->kvm_features &= ~minus_kvm_features;
-    x86_cpu_def->svm_features &= ~minus_svm_features;
-    g_free(s);
+    QDECREF(features);
+
+    g_free(name);
     return 0;
 
 error:
-    g_free(s);
+    g_free(name);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
         goto out;
 
-    cpudef_2_x86_cpu(cpu, def, &error);
-
 out:
     if (error_is_set(&error)) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
-- 
1.7.11.2

[Qemu-devel] [RFC 19/20] 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 0174c4d..43601a3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1092,6 +1092,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
@@ -1176,8 +1178,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     /* not supported bits will be filtered out later */
     env->cpuid_kvm_features = ~0;
 
-    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
-
     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),
-- 
1.7.11.2

[Qemu-devel] [RFC 20/20] 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.

PS:
  Is kvm_features field necessary in cpudef, what the point
  if it's almost imediately overwritten to ~0? Could it be removed
  from cpudef?

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 43601a3..e266792 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1093,6 +1093,9 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
     env->cpuid_xlevel2 = def->xlevel2;
 
+    /* not supported bits will be filtered out later */
+    env->cpuid_kvm_features = ~0;
+
     object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 }
 
@@ -1175,9 +1178,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 
     cpudef_2_x86_cpu(cpu, def, errp);
 
-    /* not supported bits will be filtered out later */
-    env->cpuid_kvm_features = ~0;
-
     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),
-- 
1.7.11.2

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

[Igor Mammedov]

PS:
forgot to add git tree for testing:
https://github.com/imammedo/qemu/tree/x86-cpu-properties_RFC

----- Original Message -----
> From: "Igor Mammedov" <imammedo@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, gleb@redhat.com, "jan kiszka" <jan.kiszka@siemens.com>,
> mtosatti@redhat.com, mdroth@linux.vnet.ibm.com, blauwirbel@gmail.com, avi@redhat.com, pbonzini@redhat.com,
> akong@redhat.com, lersek@redhat.com, afaerber@suse.de, ehabkost@redhat.com
> Sent: Friday, August 10, 2012 1:22:16 PM
> Subject: [Qemu-devel] [RFC 00/20] target-i386: convert CPU features into	properties
> 
> build and run tested in FC17 host with x86_64-linux-user,
> x86_64-softmmu
> targets
> 
> Igor Mammedov (20):
>   target-i386: return Error from cpu_x86_find_by_name()
>   target-i386: cpu_x86_register(): report error from property setter
>   target-i386: if x86_cpu_realize() failed report error and do
>   cleanup
>   target-i386: filter out not TCG features if running without kvm at
>     realize time
>   target-i386: move out CPU features initialization in separate func
>   target-i386: xlevel should be more than 0x80000000, move fixup into
>     setter
>   target-i386: convert cpuid features into properties
>   target-i386: convert 'hv_spinlocks' feature into property
>   target-i386: convert 'hv_relaxed' feature into property
>   target-i386: convert 'hv_vapic' feature into property
>   target-i386: convert 'check' and 'enforce' features into properties
>   add visitor for parsing hz[KMG] input string
>   target-i386: use visit_type_hz to parse tsc_freq property value
>   target-i386: introduce vendor-override property
>   target-i386: use define for cpuid vendor string size
>   target-i386: replace uint32_t vendor fields by vendor string in
>     x86_def_t
>   target-i386: parse cpu_model string into set of stringified
>     properties
>   target-i386: use properties to set/unset user specified features on
>     CPU
>   target-i386: move init of "hypervisor" feature into CPU initializer
>     from cpudef
>   target-i386: move default init of cpuid_kvm_features bitmap into
>   CPU
>     initializer from cpudef
> 
>  qapi/qapi-visit-core.c      |  11 +
>  qapi/qapi-visit-core.h      |   2 +
>  qapi/string-input-visitor.c |  22 ++
>  target-i386/cpu.c           | 669
>  +++++++++++++++++++++++++++-----------------
>  target-i386/cpu.h           |  14 +-
>  target-i386/helper.c        |   9 +-
>  6 files changed, 464 insertions(+), 263 deletions(-)
> 
> --
> 1.7.11.2
> 
> 
> 

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

[Andreas Färber]

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

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

/-F

-- 
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] [RFC 12/20] add visitor for parsing hz[KMG] input string

[Andreas Färber]

Am 10.08.2012 13:22, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  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..322cfa6 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_hz(Visitor *v, int64_t *obj, const char *name, Error **errp)
> +{
> +    if (!error_is_set(errp)) {
> +        if (v->type_hz) {
> +            v->type_hz(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..29d3038 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_hz)(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_hz(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..32e3780 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_hz(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 representible as a non-negative int64");

Thunderbird suggests "representable".

> +        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_hz = parse_type_hz;
>  
>      v->string = str;
>      return v;

I would prefer to stay physically exact and do s/hz/freq/g. ;)

Seems like a good idea to have a specialized visitor, didn't think of
that. Quite possibly there's use cases beyond tsc_freq that we might
want to switch as follow-ups.

Regards,
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] [RFC 12/20] add visitor for parsing hz[KMG] input string

[Igor Mammedov]

----- Original Message -----
> From: "Andreas Färber" <afaerber@suse.de>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: qemu-devel@nongnu.org, gleb@redhat.com, lersek@redhat.com, pbonzini@redhat.com, stefanha@linux.vnet.ibm.com,
> mdroth@linux.vnet.ibm.com, akong@redhat.com, ehabkost@redhat.com, aliguori@us.ibm.com, mtosatti@redhat.com, "jan
> kiszka" <jan.kiszka@siemens.com>, blauwirbel@gmail.com, avi@redhat.com
> Sent: Friday, August 10, 2012 1:57:42 PM
> Subject: Re: [RFC 12/20] add visitor for parsing hz[KMG] input string
> 
> Am 10.08.2012 13:22, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  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..322cfa6 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_hz(Visitor *v, int64_t *obj, const char *name,
> > Error **errp)
> > +{
> > +    if (!error_is_set(errp)) {
> > +        if (v->type_hz) {
> > +            v->type_hz(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..29d3038 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_hz)(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_hz(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..32e3780 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_hz(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 representible as a non-negative int64");
> 
> Thunderbird suggests "representable".
copy paste error, I'll fix source and here.

> 
> > +        return;
> > +    }
> > +
> > +    *obj = val;
> > +}
> > +
> >  Visitor *string_input_get_visitor(StringInputVisitor *v)
> >  {
> >      return &v->visitor;
> > @@ -132,6 +153,7 @@ StringInputVisitor
> > *string_input_visitor_new(const char *str)
> >      v->visitor.type_str = parse_type_str;
> >      v->visitor.type_number = parse_type_number;
> >      v->visitor.start_optional = parse_start_optional;
> > +    v->visitor.type_hz = parse_type_hz;
> >  
> >      v->string = str;
> >      return v;
> 
> I would prefer to stay physically exact and do s/hz/freq/g. ;)
I'll do it next respin.

> 
> Seems like a good idea to have a specialized visitor, didn't think of
> that. Quite possibly there's use cases beyond tsc_freq that we might
> want to switch as follow-ups.
> 
> Regards,
> Andreas
> 
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG
> Nürnberg
> 
Thanks,
  Igor

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

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:20PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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


> ---
>  target-i386/cpu.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 17e98e1..d0dec63 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1171,17 +1171,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      env->cpuid_xlevel2 = def->xlevel2;
>      object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
>                              "tsc-frequency", &error);
> -    if (!kvm_enabled()) {
> -        env->cpuid_features &= TCG_FEATURES;
> -        env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
> -#ifdef TARGET_X86_64
> -            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
> -#endif
> -            );
> -        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
> -        env->cpuid_svm_features &= TCG_SVM_FEATURES;
> -    }
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>  
>  out:
> @@ -1745,6 +1734,19 @@ static void mce_init(X86CPU *cpu)
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +
> +    if (!kvm_enabled()) {
> +        env->cpuid_features &= TCG_FEATURES;
> +        env->cpuid_ext_features &= TCG_EXT_FEATURES;
> +        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
> +#ifdef TARGET_X86_64
> +            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
> +#endif
> +            );
> +        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
> +        env->cpuid_svm_features &= TCG_SVM_FEATURES;
> +    }
>  
>  #ifndef CONFIG_USER_ONLY
>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> -- 
> 1.7.11.2
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:21PM +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>


> ---
>  target-i386/cpu.c | 62 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d0dec63..783c6f4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -858,6 +858,39 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> +static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> +{
> +    CPUX86State *env = &cpu->env;
> +
> +    if (def->vendor1) {
> +        env->cpuid_vendor1 = def->vendor1;
> +        env->cpuid_vendor2 = def->vendor2;
> +        env->cpuid_vendor3 = def->vendor3;
> +    } else {
> +        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
> +        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
> +        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
> +    }
> +    env->cpuid_vendor_override = def->vendor_override;
> +    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
> +    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
> +    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
> +    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> +                            "tsc-frequency", errp);
> +    env->cpuid_features = def->features;
> +    env->cpuid_ext_features = def->ext_features;
> +    env->cpuid_ext2_features = def->ext2_features;
> +    env->cpuid_ext3_features = def->ext3_features;
> +    env->cpuid_kvm_features = def->kvm_features;
> +    env->cpuid_svm_features = def->svm_features;
> +    env->cpuid_ext4_features = def->ext4_features;
> +    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> +    env->cpuid_xlevel2 = def->xlevel2;
> +}
> +
>  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>                                  const char *cpu_model, Error **errp)
>  {
> @@ -1136,7 +1169,6 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
>  
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
> -    CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>  
> @@ -1145,33 +1177,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      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;
> -        env->cpuid_vendor3 = def->vendor3;
> -    } else {
> -        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
> -        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
> -        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
> -    }
> -    env->cpuid_vendor_override = def->vendor_override;
> -    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> -    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> -    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> -    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
> -    env->cpuid_features = def->features;
> -    env->cpuid_ext_features = def->ext_features;
> -    env->cpuid_ext2_features = def->ext2_features;
> -    env->cpuid_ext3_features = def->ext3_features;
> -    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
> -    env->cpuid_kvm_features = def->kvm_features;
> -    env->cpuid_svm_features = def->svm_features;
> -    env->cpuid_ext4_features = def->ext4_features;
> -    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> -    env->cpuid_xlevel2 = def->xlevel2;
> -    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> -                            "tsc-frequency", &error);
> -    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +    cpudef_2_x86_cpu(cpu, def, &error);
>  
>  out:
>      if (error_is_set(&error)) {
> -- 
> 1.7.11.2
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:22PM +0200, Igor Mammedov wrote:
> 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 783c6f4..a47cc12 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -745,8 +745,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)
> @@ -981,9 +990,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>                      fprintf(stderr, "bad numerical value %s\n", val);
>                      goto error;
>                  }
> -                if (numvalue < 0x80000000) {
> -                    numvalue += 0x80000000;
> -                }
>                  x86_cpu_def->xlevel = numvalue;
>              } else if (!strcmp(featurestr, "vendor")) {
>                  if (strlen(val) != 12) {
> -- 
> 1.7.11.2
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:23PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a47cc12..4b22598 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -605,6 +605,103 @@ 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 mask;
> +
> +    for (mask = 1; mask; mask <<= 1) {
> +        if (featureset[ffs(mask) - 1] &&
> +            !altcmp(name, NULL, featureset[ffs(mask) - 1])) {
> +            break;
> +    }

Wrong indentation.

> +    }
> +    if (featbitmap & mask) {
> +        return true;
> +    }
> +    return false;

Isn't it simpler to write this as:

    int bit;

    for (bit = 0; bit < 32; bit++) {
        if (featureset[bit] && !altcmp(name, NULL, featureset[bit])) {
            if (featbitmap & (1 << bit)) {
                return true;
        }
    }
    return false;

?

> +}
> +
> +static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
> +                                         const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    bool value = true;
> +
> +    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
> +       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
> +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
> +        value = false;
> +    }
> +
> +    visit_type_bool(v, &value, name, errp);
> +}
> +
> +static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque,
> +                                         const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    uint32_t mask = 0;
> +    uint32_t *dst_features;
> +    bool value;
> +
> +    visit_type_bool(v, &value, name, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +
> +    if (lookup_feature(&mask, name, NULL, feature_name)) {
> +        dst_features = &env->cpuid_features;
> +    } else if (lookup_feature(&mask, name, NULL, ext_feature_name)) {
> +        dst_features = &env->cpuid_ext_features;
> +    } else if (lookup_feature(&mask, name, NULL, ext2_feature_name)) {
> +        dst_features = &env->cpuid_ext2_features;
> +    } else if (lookup_feature(&mask, name, NULL, ext3_feature_name)) {
> +        dst_features = &env->cpuid_ext3_features;
> +    } else if (lookup_feature(&mask, name, NULL, kvm_feature_name)) {
> +        dst_features = &env->cpuid_kvm_features;
> +    } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) {
> +        dst_features = &env->cpuid_svm_features;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
> +        return;
> +    }
> +
> +    if (value) {
> +        *dst_features |= mask;
> +    } else {
> +        *dst_features &= ~mask;
> +    }
> +}
> +
> +static void x86_register_cpuid_properties(Object *obj, const char **featureset)
> +{
> +    uint32_t mask;
> +
> +    for (mask = 1; mask; mask <<= 1) {
> +        if (featureset[ffs(mask) - 1]) {
> +            char *feature_name, *save_ptr;
> +            char buf[32];
> +            if (strlen(featureset[ffs(mask) - 1]) > sizeof(buf) - 1) {
> +                abort();
> +            }
> +            pstrcpy(buf, sizeof(buf), featureset[ffs(mask) - 1]);
> +            feature_name = strtok_r(buf, "|", &save_ptr);
> +            while (feature_name) {
> +                object_property_add(obj, feature_name, "bool",
> +                                x86_cpuid_get_feature,
> +                                x86_cpuid_set_feature, NULL, NULL, NULL);
> +                feature_name = strtok_r(NULL, "|", &save_ptr);
> +            }
> +        }
> +    }

Same case as above: why the mask/ffs tricks when you could just use
"for (bit = 0; bit < 32; bit++)" and use "bit" instead of
"ffs(mask) - 1"?

> +}
> +
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
>  {
> @@ -1801,6 +1898,12 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    x86_register_cpuid_properties(obj, feature_name);
> +    x86_register_cpuid_properties(obj, ext_feature_name);
> +    x86_register_cpuid_properties(obj, ext2_feature_name);
> +    x86_register_cpuid_properties(obj, ext3_feature_name);
> +    x86_register_cpuid_properties(obj, kvm_feature_name);
> +    x86_register_cpuid_properties(obj, svm_feature_name);
>  
>      env->cpuid_apic_id = env->cpu_index;
>  
> -- 
> 1.7.11.2
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  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 7734613..a154e89 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -106,8 +106,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)
> @@ -579,19 +579,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);
> @@ -1030,6 +1031,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;
> @@ -1225,10 +1263,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;
>  
> @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_svm_features &= TCG_SVM_FEATURES;
>      }
>  
> +    if (check_cpuid && check_features_against_host(cpu)
> +        && enforce_cpuid) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +

I just noticed that you changed behavior on patch 04/20 and now restore
the behavior in this patch:

- Before patch 04/20, the feature check was being done after the
  features were filtered according to the TCG support (meaning a feature
  not supported by TCG will not trigger enforce/check errors).
- After patch 04/20, the check was being done _before_ the features were
  filtered according to TCG support (meaning a feature not supported by
  TCG would trigger enforce/check errors).
- With this patch, the old behavior is restored.

I'm not sure which behavior is better. But we surely shouldn't silently
move back and forth between those two modes.

IMO, checking _before_ the TCG filtering is better, as it's more
predictable. it means having lots of warnings in case too-new CPU models
are chosen in TCG model, but that's exactly the point.


>  #ifndef CONFIG_USER_ONLY
>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>  #endif
> @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    object_property_add(obj, "check", "bool",
> +                        x86_cpuid_get_check,
> +                        x86_cpuid_set_check, NULL, NULL, NULL);
> +    object_property_add(obj, "enforce", "bool",
> +                        x86_cpuid_get_enforce,
> +                        x86_cpuid_set_enforce, NULL, NULL, NULL);
>  #if !defined(CONFIG_USER_ONLY)
>      object_property_add(obj, "hv_spinlocks", "int",
>                          x86_get_hv_spinlocks,
> -- 
> 1.7.11.2
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 20/20] target-i386: move default init of cpuid_kvm_features bitmap into CPU initializer from cpudef

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:36PM +0200, Igor Mammedov wrote:
> Moving it inside CPU initializer from cpudef will help to split
> cpu_x86_find_by_name() into default init and user settable properties.
> 
> PS:
>   Is kvm_features field necessary in cpudef, what the point
>   if it's almost imediately overwritten to ~0? Could it be removed
>   from cpudef?

We could probably drop it, but: maybe better than dropping it is to
initialize it properly with all the currently-supported KVM feature
flags, instead of initializing it to ~0 and then filter the bits later.
Otherwise "-cpu enforce" would not be able to check the KVM feature bits
properly.


> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 43601a3..e266792 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1093,6 +1093,9 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>      env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
>      env->cpuid_xlevel2 = def->xlevel2;
>  
> +    /* not supported bits will be filtered out later */
> +    env->cpuid_kvm_features = ~0;
> +
>      object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
>  }
>  
> @@ -1175,9 +1178,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>  
>      cpudef_2_x86_cpu(cpu, def, errp);
>  
> -    /* not supported bits will be filtered out later */
> -    env->cpuid_kvm_features = ~0;
> -
>      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),
> -- 
> 1.7.11.2
> 

-- 
Eduardo

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

[Blue Swirl]

On Fri, Aug 10, 2012 at 11:22 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> it will allow to use property setters there later.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 880cfea..ee25309 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -858,7 +858,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;
> @@ -1003,6 +1004,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;
> @@ -1026,6 +1032,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;
>  }
>
> @@ -1133,8 +1142,9 @@ 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)

Please add braces.

> +        goto out;
> +
>      if (def->vendor1) {
>          env->cpuid_vendor1 = def->vendor1;
>          env->cpuid_vendor2 = def->vendor2;
> @@ -1173,6 +1183,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          env->cpuid_svm_features &= TCG_SVM_FEATURES;
>      }
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +
> +out:
>      if (error_is_set(&error)) {
>          error_free(error);
>          return -1;
> --
> 1.7.11.2
>

Re: [Qemu-devel] [RFC 18/20] target-i386: use properties to set/unset user specified features on CPU

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 167 +++++++-----------------------------------------------
>  1 file changed, 20 insertions(+), 147 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7555b08..0174c4d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -206,22 +206,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;
> @@ -1104,7 +1088,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;
> @@ -1168,17 +1151,14 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>  {
>      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;
> +    QDict *features;
> +    const QDictEntry *ent;
> +    char *name;
> +
> +    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
> +    if (error_is_set(errp)) {
> +        goto error;
> +    }
>  
>      for (def = x86_defs; def; def = def->next)
>          if (name && !strcmp(name, def->name))
> @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>          memcpy(x86_cpu_def, def, sizeof(*def));
>      }
>  
> -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> -
> -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -        &plus_kvm_features, &plus_svm_features);
> +    cpudef_2_x86_cpu(cpu, def, errp);

It looks like you have to use 'x86_cpu_def' instead of 'def', here, as
'def' is the variable for the x86_defs name-lookup loop, only,
'x86_cpu_def' is the actual CPU definition.


>  
> -    featurestr = strtok(NULL, ",");
> +    /* not supported bits will be filtered out later */
> +    env->cpuid_kvm_features = ~0;
>  
> -    while (featurestr) {
> -        char *val;
> -        if (featurestr[0] == '+') {
> -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> -                            &plus_ext_features, &plus_ext2_features,
> -                            &plus_ext3_features, &plus_kvm_features,
> -                            &plus_svm_features);
> -        } else if (featurestr[0] == '-') {
> -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> -                            &minus_ext_features, &minus_ext2_features,
> -                            &minus_ext3_features, &minus_kvm_features,
> -                            &minus_svm_features);
> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> -            if (!strcmp(featurestr, "family")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff + 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->family = numvalue;
> -            } else if (!strcmp(featurestr, "model")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->model = numvalue;
> -            } else if (!strcmp(featurestr, "stepping")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->stepping = numvalue ;
> -            } else if (!strcmp(featurestr, "level")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->level = numvalue;
> -            } else if (!strcmp(featurestr, "xlevel")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->xlevel = numvalue;
> -            } else if (!strcmp(featurestr, "vendor")) {
> -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> -                x86_cpu_def->vendor_override = 1;
> -            } else if (!strcmp(featurestr, "model_id")) {
> -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> -                        val);
> -            } else if (!strcmp(featurestr, "tsc_freq")) {
> -                int64_t tsc_freq;
> -                char *err;
> -
> -                tsc_freq = strtosz_suffix_unit(val, &err,
> -                                               STRTOSZ_DEFSUFFIX_B, 1000);
> -                if (tsc_freq < 0 || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> -            } else if (!strcmp(featurestr, "hv_spinlocks")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                hyperv_set_spinlock_retries(numvalue);
> -            } else {
> -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> -                goto error;
> -            }
> -        } else if (!strcmp(featurestr, "check")) {
> -            check_cpuid = 1;
> -        } else if (!strcmp(featurestr, "enforce")) {
> -            check_cpuid = enforce_cpuid = 1;
> -        } else if (!strcmp(featurestr, "hv_relaxed")) {
> -            hyperv_enable_relaxed_timing(true);
> -        } else if (!strcmp(featurestr, "hv_vapic")) {
> -            hyperv_enable_vapic_recommended(true);
> -        } else {
> -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> -            goto error;
> -        }
> +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
>  
> +    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)) {
>              goto error;
>          }
> -
> -        featurestr = strtok(NULL, ",");
>      }
> -    x86_cpu_def->features |= plus_features;
> -    x86_cpu_def->ext_features |= plus_ext_features;
> -    x86_cpu_def->ext2_features |= plus_ext2_features;
> -    x86_cpu_def->ext3_features |= plus_ext3_features;
> -    x86_cpu_def->kvm_features |= plus_kvm_features;
> -    x86_cpu_def->svm_features |= plus_svm_features;
> -    x86_cpu_def->features &= ~minus_features;
> -    x86_cpu_def->ext_features &= ~minus_ext_features;
> -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> -    x86_cpu_def->svm_features &= ~minus_svm_features;
> -    g_free(s);
> +    QDECREF(features);
> +
> +    g_free(name);
>      return 0;
>  
>  error:
> -    g_free(s);
> +    g_free(name);
>      if (!error_is_set(errp)) {
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
>      }
> @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
>          goto out;
>  
> -    cpudef_2_x86_cpu(cpu, def, &error);
> -

You could move the 'def' and 'def1' variables inside
cpu_x86_find_by_name(), now, as they are now only used inside that
function.

>  out:
>      if (error_is_set(&error)) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> -- 
> 1.7.11.2
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Igor Mammedov]

On Fri, 10 Aug 2012 12:09:04 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  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 7734613..a154e89 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -106,8 +106,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)
> > @@ -579,19 +579,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);
> > @@ -1030,6 +1031,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;
> > @@ -1225,10 +1263,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;
> >  
> > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >          env->cpuid_svm_features &= TCG_SVM_FEATURES;
> >      }
> >  
> > +    if (check_cpuid && check_features_against_host(cpu)
> > +        && enforce_cpuid) {
> > +        error_set(errp, QERR_PERMISSION_DENIED);
> > +        return;
> > +    }
> > +
> 
> I just noticed that you changed behavior on patch 04/20 and now restore
> the behavior in this patch:
> 
> - Before patch 04/20, the feature check was being done after the
>   features were filtered according to the TCG support (meaning a feature
>   not supported by TCG will not trigger enforce/check errors).
before 04/20 check is done before TCG features filtering in
cpu_x86_find_by_name() and then later in cpu_x86_register() features are
TCG filtered.

> - After patch 04/20, the check was being done _before_ the features were
>   filtered according to TCG support (meaning a feature not supported by
>   TCG would trigger enforce/check errors).
> - With this patch, the old behavior is restored.
after 04/20, it is the same as before, i.e realize is called after
cpu_x86_register().

it's by mistake that in this patch I've put check after TCG filtering, I'll
fix it and do check before it.
> 
> I'm not sure which behavior is better. But we surely shouldn't silently
> move back and forth between those two modes.
> 
> IMO, checking _before_ the TCG filtering is better, as it's more
> predictable. it means having lots of warnings in case too-new CPU models
> are chosen in TCG model, but that's exactly the point.
> 
> 
> >  #ifndef CONFIG_USER_ONLY
> >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> >  #endif
> > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj)
> >      object_property_add(obj, "tsc-frequency", "int",
> >                          x86_cpuid_get_tsc_freq,
> >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > +    object_property_add(obj, "check", "bool",
> > +                        x86_cpuid_get_check,
> > +                        x86_cpuid_set_check, NULL, NULL, NULL);
> > +    object_property_add(obj, "enforce", "bool",
> > +                        x86_cpuid_get_enforce,
> > +                        x86_cpuid_set_enforce, NULL, NULL, NULL);
> >  #if !defined(CONFIG_USER_ONLY)
> >      object_property_add(obj, "hv_spinlocks", "int",
> >                          x86_get_hv_spinlocks,
> > -- 
> > 1.7.11.2
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 18/20] target-i386: use properties to set/unset user specified features on CPU

[Igor Mammedov]

On Mon, 13 Aug 2012 17:48:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 167 +++++++-----------------------------------------------
> >  1 file changed, 20 insertions(+), 147 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7555b08..0174c4d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -206,22 +206,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;
> > @@ -1104,7 +1088,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;
> > @@ -1168,17 +1151,14 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >  {
> >      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;
> > +    QDict *features;
> > +    const QDictEntry *ent;
> > +    char *name;
> > +
> > +    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
> > +    if (error_is_set(errp)) {
> > +        goto error;
> > +    }
> >  
> >      for (def = x86_defs; def; def = def->next)
> >          if (name && !strcmp(name, def->name))
> > @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >          memcpy(x86_cpu_def, def, sizeof(*def));
> >      }
> >  
> > -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> > -
> > -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> > -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> > -        &plus_kvm_features, &plus_svm_features);
> > +    cpudef_2_x86_cpu(cpu, def, errp);
> 
> It looks like you have to use 'x86_cpu_def' instead of 'def', here, as
> 'def' is the variable for the x86_defs name-lookup loop, only,
> 'x86_cpu_def' is the actual CPU definition.
yep, I'll fix it.

> 
> 
> >  
> > -    featurestr = strtok(NULL, ",");
> > +    /* not supported bits will be filtered out later */
> > +    env->cpuid_kvm_features = ~0;
> >  
> > -    while (featurestr) {
> > -        char *val;
> > -        if (featurestr[0] == '+') {
> > -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > -                            &plus_ext_features, &plus_ext2_features,
> > -                            &plus_ext3_features, &plus_kvm_features,
> > -                            &plus_svm_features);
> > -        } else if (featurestr[0] == '-') {
> > -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> > -                            &minus_ext_features, &minus_ext2_features,
> > -                            &minus_ext3_features, &minus_kvm_features,
> > -                            &minus_svm_features);
> > -        } else if ((val = strchr(featurestr, '='))) {
> > -            *val = 0; val++;
> > -            if (!strcmp(featurestr, "family")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xff + 0xf) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->family = numvalue;
> > -            } else if (!strcmp(featurestr, "model")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xff) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->model = numvalue;
> > -            } else if (!strcmp(featurestr, "stepping")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xf) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->stepping = numvalue ;
> > -            } else if (!strcmp(featurestr, "level")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->level = numvalue;
> > -            } else if (!strcmp(featurestr, "xlevel")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->xlevel = numvalue;
> > -            } else if (!strcmp(featurestr, "vendor")) {
> > -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> > -                x86_cpu_def->vendor_override = 1;
> > -            } else if (!strcmp(featurestr, "model_id")) {
> > -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> > -                        val);
> > -            } else if (!strcmp(featurestr, "tsc_freq")) {
> > -                int64_t tsc_freq;
> > -                char *err;
> > -
> > -                tsc_freq = strtosz_suffix_unit(val, &err,
> > -                                               STRTOSZ_DEFSUFFIX_B, 1000);
> > -                if (tsc_freq < 0 || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > -            } else if (!strcmp(featurestr, "hv_spinlocks")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                hyperv_set_spinlock_retries(numvalue);
> > -            } else {
> > -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> > -                goto error;
> > -            }
> > -        } else if (!strcmp(featurestr, "check")) {
> > -            check_cpuid = 1;
> > -        } else if (!strcmp(featurestr, "enforce")) {
> > -            check_cpuid = enforce_cpuid = 1;
> > -        } else if (!strcmp(featurestr, "hv_relaxed")) {
> > -            hyperv_enable_relaxed_timing(true);
> > -        } else if (!strcmp(featurestr, "hv_vapic")) {
> > -            hyperv_enable_vapic_recommended(true);
> > -        } else {
> > -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> > -            goto error;
> > -        }
> > +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> >  
> > +    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)) {
> >              goto error;
> >          }
> > -
> > -        featurestr = strtok(NULL, ",");
> >      }
> > -    x86_cpu_def->features |= plus_features;
> > -    x86_cpu_def->ext_features |= plus_ext_features;
> > -    x86_cpu_def->ext2_features |= plus_ext2_features;
> > -    x86_cpu_def->ext3_features |= plus_ext3_features;
> > -    x86_cpu_def->kvm_features |= plus_kvm_features;
> > -    x86_cpu_def->svm_features |= plus_svm_features;
> > -    x86_cpu_def->features &= ~minus_features;
> > -    x86_cpu_def->ext_features &= ~minus_ext_features;
> > -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> > -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> > -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> > -    x86_cpu_def->svm_features &= ~minus_svm_features;
> > -    g_free(s);
> > +    QDECREF(features);
> > +
> > +    g_free(name);
> >      return 0;
> >  
> >  error:
> > -    g_free(s);
> > +    g_free(name);
> >      if (!error_is_set(errp)) {
> >          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> >      }
> > @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
> >          goto out;
> >  
> > -    cpudef_2_x86_cpu(cpu, def, &error);
> > -

> 
> You could move the 'def' and 'def1' variables inside
> cpu_x86_find_by_name(), now, as they are now only used inside that
> function.

it's better stay here, I'll make another patch that will leave only def lookup
in cpu_x86_find_by_name() and move the rest in cpu_x86_register().
with subclasses cpu_x86_find_by_name() should go away and cpu_x86_register()
could be disbanded into pieces as it was shown in [NOT RFC] patch I've posted
recently.

and if you do not object I'd take your "create cpu_x86_set_props() function"
patch from
https://github.com/ehabkost/qemu-hacks/commit/4d734b316625d0b689031460328ce6e531be5b90

> 
> >  out:
> >      if (error_is_set(&error)) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > -- 
> > 1.7.11.2
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Eduardo Habkost]

On Tue, Aug 14, 2012 at 11:18:47PM +0200, Igor Mammedov wrote:
> On Fri, 10 Aug 2012 12:09:04 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  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 7734613..a154e89 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -106,8 +106,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)
> > > @@ -579,19 +579,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);
> > > @@ -1030,6 +1031,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;
> > > @@ -1225,10 +1263,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;
> > >  
> > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
> > >          env->cpuid_svm_features &= TCG_SVM_FEATURES;
> > >      }
> > >  
> > > +    if (check_cpuid && check_features_against_host(cpu)
> > > +        && enforce_cpuid) {
> > > +        error_set(errp, QERR_PERMISSION_DENIED);
> > > +        return;
> > > +    }
> > > +
> > 
> > I just noticed that you changed behavior on patch 04/20 and now restore
> > the behavior in this patch:
> > 
> > - Before patch 04/20, the feature check was being done after the
> >   features were filtered according to the TCG support (meaning a feature
> >   not supported by TCG will not trigger enforce/check errors).
> before 04/20 check is done before TCG features filtering in
> cpu_x86_find_by_name() and then later in cpu_x86_register() features are
> TCG filtered.

Right, my mistake. The check was done on cpu_x86_find_by_name() (before
the filtering), but for some reason I was thinking it was done much
later. Nevermind.

> 
> > - After patch 04/20, the check was being done _before_ the features were
> >   filtered according to TCG support (meaning a feature not supported by
> >   TCG would trigger enforce/check errors).
> > - With this patch, the old behavior is restored.
> after 04/20, it is the same as before, i.e realize is called after
> cpu_x86_register().
> 
> it's by mistake that in this patch I've put check after TCG filtering, I'll
> fix it and do check before it.

Good. I think it's a good idea to make the check before the filtering
(and eventually, instead of checking the host CPU features directly, the
check on TCG mode should be based on TCG_FEATURES, and the check on KVM
mode should be based on GET_SUPPORTED_CPUID, but that's another issue).

> > 
> > I'm not sure which behavior is better. But we surely shouldn't silently
> > move back and forth between those two modes.
> > 
> > IMO, checking _before_ the TCG filtering is better, as it's more
> > predictable. it means having lots of warnings in case too-new CPU models
> > are chosen in TCG model, but that's exactly the point.
> > 
> > 
> > >  #ifndef CONFIG_USER_ONLY
> > >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> > >  #endif
> > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj)
> > >      object_property_add(obj, "tsc-frequency", "int",
> > >                          x86_cpuid_get_tsc_freq,
> > >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > +    object_property_add(obj, "check", "bool",
> > > +                        x86_cpuid_get_check,
> > > +                        x86_cpuid_set_check, NULL, NULL, NULL);
> > > +    object_property_add(obj, "enforce", "bool",
> > > +                        x86_cpuid_get_enforce,
> > > +                        x86_cpuid_set_enforce, NULL, NULL, NULL);
> > >  #if !defined(CONFIG_USER_ONLY)
> > >      object_property_add(obj, "hv_spinlocks", "int",
> > >                          x86_get_hv_spinlocks,
> > > -- 
> > > 1.7.11.2
> > > 
> > 
> > -- 
> > Eduardo
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Igor Mammedov]

On Wed, 15 Aug 2012 08:39:54 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Aug 14, 2012 at 11:18:47PM +0200, Igor Mammedov wrote:
> > On Fri, 10 Aug 2012 12:09:04 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote:
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  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 7734613..a154e89 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -106,8 +106,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)
> > > > @@ -579,19 +579,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);
> > > > @@ -1030,6 +1031,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;
> > > > @@ -1225,10 +1263,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;
> > > >  
> > > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
> > > >          env->cpuid_svm_features &= TCG_SVM_FEATURES;
> > > >      }
> > > >  
> > > > +    if (check_cpuid && check_features_against_host(cpu)
> > > > +        && enforce_cpuid) {
> > > > +        error_set(errp, QERR_PERMISSION_DENIED);
> > > > +        return;
> > > > +    }
> > > > +
> > > 
> > > I just noticed that you changed behavior on patch 04/20 and now restore
> > > the behavior in this patch:
> > > 
> > > - Before patch 04/20, the feature check was being done after the
> > >   features were filtered according to the TCG support (meaning a feature
> > >   not supported by TCG will not trigger enforce/check errors).
> > before 04/20 check is done before TCG features filtering in
> > cpu_x86_find_by_name() and then later in cpu_x86_register() features are
> > TCG filtered.
> 
> Right, my mistake. The check was done on cpu_x86_find_by_name() (before
> the filtering), but for some reason I was thinking it was done much
> later. Nevermind.
> 
> > 
> > > - After patch 04/20, the check was being done _before_ the features were
> > >   filtered according to TCG support (meaning a feature not supported by
> > >   TCG would trigger enforce/check errors).
> > > - With this patch, the old behavior is restored.
> > after 04/20, it is the same as before, i.e realize is called after
> > cpu_x86_register().
> > 
> > it's by mistake that in this patch I've put check after TCG filtering, I'll
> > fix it and do check before it.
> 
> Good. I think it's a good idea to make the check before the filtering
> (and eventually, instead of checking the host CPU features directly, the
> check on TCG mode should be based on TCG_FEATURES, and the check on KVM
> mode should be based on GET_SUPPORTED_CPUID, but that's another issue).
If you are not opposed to improving checks later, I'd better re-post this
series with original behavior. Improving checks is kind of orthogonal to
moving features into properties. 

> 
> > > 
> > > I'm not sure which behavior is better. But we surely shouldn't silently
> > > move back and forth between those two modes.
> > > 
> > > IMO, checking _before_ the TCG filtering is better, as it's more
> > > predictable. it means having lots of warnings in case too-new CPU models
> > > are chosen in TCG model, but that's exactly the point.
> > > 
> > > 
> > > >  #ifndef CONFIG_USER_ONLY
> > > >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> > > >  #endif
> > > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj)
> > > >      object_property_add(obj, "tsc-frequency", "int",
> > > >                          x86_cpuid_get_tsc_freq,
> > > >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > > +    object_property_add(obj, "check", "bool",
> > > > +                        x86_cpuid_get_check,
> > > > +                        x86_cpuid_set_check, NULL, NULL, NULL);
> > > > +    object_property_add(obj, "enforce", "bool",
> > > > +                        x86_cpuid_get_enforce,
> > > > +                        x86_cpuid_set_enforce, NULL, NULL, NULL);
> > > >  #if !defined(CONFIG_USER_ONLY)
> > > >      object_property_add(obj, "hv_spinlocks", "int",
> > > >                          x86_get_hv_spinlocks,
> > > > -- 
> > > > 1.7.11.2
> > > > 
> > > 
> > > -- 
> > > Eduardo
> > 
> > 
> > -- 
> > Regards,
> >   Igor
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Eduardo Habkost]

On Wed, Aug 15, 2012 at 02:11:43PM +0200, Igor Mammedov wrote:
> On Wed, 15 Aug 2012 08:39:54 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Aug 14, 2012 at 11:18:47PM +0200, Igor Mammedov wrote:
> > > On Fri, 10 Aug 2012 12:09:04 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote:
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > >  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 7734613..a154e89 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -106,8 +106,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)
> > > > > @@ -579,19 +579,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);
> > > > > @@ -1030,6 +1031,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;
> > > > > @@ -1225,10 +1263,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;
> > > > >  
> > > > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
> > > > >          env->cpuid_svm_features &= TCG_SVM_FEATURES;
> > > > >      }
> > > > >  
> > > > > +    if (check_cpuid && check_features_against_host(cpu)
> > > > > +        && enforce_cpuid) {
> > > > > +        error_set(errp, QERR_PERMISSION_DENIED);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > 
> > > > I just noticed that you changed behavior on patch 04/20 and now restore
> > > > the behavior in this patch:
> > > > 
> > > > - Before patch 04/20, the feature check was being done after the
> > > >   features were filtered according to the TCG support (meaning a feature
> > > >   not supported by TCG will not trigger enforce/check errors).
> > > before 04/20 check is done before TCG features filtering in
> > > cpu_x86_find_by_name() and then later in cpu_x86_register() features are
> > > TCG filtered.
> > 
> > Right, my mistake. The check was done on cpu_x86_find_by_name() (before
> > the filtering), but for some reason I was thinking it was done much
> > later. Nevermind.
> > 
> > > 
> > > > - After patch 04/20, the check was being done _before_ the features were
> > > >   filtered according to TCG support (meaning a feature not supported by
> > > >   TCG would trigger enforce/check errors).
> > > > - With this patch, the old behavior is restored.
> > > after 04/20, it is the same as before, i.e realize is called after
> > > cpu_x86_register().
> > > 
> > > it's by mistake that in this patch I've put check after TCG filtering, I'll
> > > fix it and do check before it.
> > 
> > Good. I think it's a good idea to make the check before the filtering
> > (and eventually, instead of checking the host CPU features directly, the
> > check on TCG mode should be based on TCG_FEATURES, and the check on KVM
> > mode should be based on GET_SUPPORTED_CPUID, but that's another issue).
> If you are not opposed to improving checks later, I'd better re-post this
> series with original behavior. Improving checks is kind of orthogonal to
> moving features into properties. 

Absolutely. It's completely orthogonal, I just mentioned it because it's
related to the discussion about the expected '-cpu check' behavior, and
to let you know it's on my plans.

It's also better to first clean up that code and make it sane (like you
are doing), before introducing non-trivial behavior changes.

> 
> > 
> > > > 
> > > > I'm not sure which behavior is better. But we surely shouldn't silently
> > > > move back and forth between those two modes.
> > > > 
> > > > IMO, checking _before_ the TCG filtering is better, as it's more
> > > > predictable. it means having lots of warnings in case too-new CPU models
> > > > are chosen in TCG model, but that's exactly the point.
> > > > 
> > > > 
> > > > >  #ifndef CONFIG_USER_ONLY
> > > > >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> > > > >  #endif
> > > > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj)
> > > > >      object_property_add(obj, "tsc-frequency", "int",
> > > > >                          x86_cpuid_get_tsc_freq,
> > > > >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > > > +    object_property_add(obj, "check", "bool",
> > > > > +                        x86_cpuid_get_check,
> > > > > +                        x86_cpuid_set_check, NULL, NULL, NULL);
> > > > > +    object_property_add(obj, "enforce", "bool",
> > > > > +                        x86_cpuid_get_enforce,
> > > > > +                        x86_cpuid_set_enforce, NULL, NULL, NULL);
> > > > >  #if !defined(CONFIG_USER_ONLY)
> > > > >      object_property_add(obj, "hv_spinlocks", "int",
> > > > >                          x86_get_hv_spinlocks,
> > > > > -- 
> > > > > 1.7.11.2
> > > > > 
> > > > 
> > > > -- 
> > > > Eduardo
> > > 
> > > 
> > > -- 
> > > Regards,
> > >   Igor
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [RFC 20/20] target-i386: move default init of cpuid_kvm_features bitmap into CPU initializer from cpudef

[Igor Mammedov]

On Fri, 10 Aug 2012 12:24:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Aug 10, 2012 at 01:22:36PM +0200, Igor Mammedov wrote:
> > Moving it inside CPU initializer from cpudef will help to split
> > cpu_x86_find_by_name() into default init and user settable properties.
> > 
> > PS:
> >   Is kvm_features field necessary in cpudef, what the point
> >   if it's almost imediately overwritten to ~0? Could it be removed
> >   from cpudef?
> 
> We could probably drop it, but: maybe better than dropping it is to
> initialize it properly with all the currently-supported KVM feature
> flags, instead of initializing it to ~0 and then filter the bits later.
> Otherwise "-cpu enforce" would not be able to check the KVM feature bits
> properly.
Looking at current core, It should be doable, lets postpone it to a separate
series.
For now lets keep it initialized to ~0 and plan to improve it later.

> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 43601a3..e266792 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1093,6 +1093,9 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> >      env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> >      env->cpuid_xlevel2 = def->xlevel2;
> >  
> > +    /* not supported bits will be filtered out later */
> > +    env->cpuid_kvm_features = ~0;
> > +
> >      object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> >  }
> >  
> > @@ -1175,9 +1178,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >  
> >      cpudef_2_x86_cpu(cpu, def, errp);
> >  
> > -    /* not supported bits will be filtered out later */
> > -    env->cpuid_kvm_features = ~0;
> > -
> >      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),
> > -- 
> > 1.7.11.2
> > 
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 20/20] target-i386: move default init of cpuid_kvm_features bitmap into CPU initializer from cpudef

[Eduardo Habkost]

On Wed, Aug 15, 2012 at 02:23:46PM +0200, Igor Mammedov wrote:
> On Fri, 10 Aug 2012 12:24:48 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Aug 10, 2012 at 01:22:36PM +0200, Igor Mammedov wrote:
> > > Moving it inside CPU initializer from cpudef will help to split
> > > cpu_x86_find_by_name() into default init and user settable properties.
> > > 
> > > PS:
> > >   Is kvm_features field necessary in cpudef, what the point
> > >   if it's almost imediately overwritten to ~0? Could it be removed
> > >   from cpudef?
> > 
> > We could probably drop it, but: maybe better than dropping it is to
> > initialize it properly with all the currently-supported KVM feature
> > flags, instead of initializing it to ~0 and then filter the bits later.
> > Otherwise "-cpu enforce" would not be able to check the KVM feature bits
> > properly.
> Looking at current core, It should be doable, lets postpone it to a separate
> series.
> For now lets keep it initialized to ~0 and plan to improve it later.

Agreed. Let's move one step at a time. The good thing is that the
cleanup is making those weird exceptions in the code easier to spot.

> 
> > 
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 43601a3..e266792 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1093,6 +1093,9 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> > >      env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> > >      env->cpuid_xlevel2 = def->xlevel2;
> > >  
> > > +    /* not supported bits will be filtered out later */
> > > +    env->cpuid_kvm_features = ~0;
> > > +
> > >      object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> > >  }
> > >  
> > > @@ -1175,9 +1178,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> > >  
> > >      cpudef_2_x86_cpu(cpu, def, errp);
> > >  
> > > -    /* not supported bits will be filtered out later */
> > > -    env->cpuid_kvm_features = ~0;
> > > -
> > >      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),
> > > -- 
> > > 1.7.11.2
> > > 
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [RFC 18/20] target-i386: use properties to set/unset user specified features on CPU

[Eduardo Habkost]

Another issue:

On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 167 +++++++-----------------------------------------------
>  1 file changed, 20 insertions(+), 147 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7555b08..0174c4d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
[...]
> @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>          memcpy(x86_cpu_def, def, sizeof(*def));
>      }
>  
> -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> -
> -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -        &plus_kvm_features, &plus_svm_features);
> +    cpudef_2_x86_cpu(cpu, def, errp);
>  
> -    featurestr = strtok(NULL, ",");
> +    /* not supported bits will be filtered out later */
> +    env->cpuid_kvm_features = ~0;

This doesn't compile. The 'env' variable doesn't exist inside
cpu_x86_find_by_name().

>  
> -    while (featurestr) {
> -        char *val;
> -        if (featurestr[0] == '+') {
> -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> -                            &plus_ext_features, &plus_ext2_features,
> -                            &plus_ext3_features, &plus_kvm_features,
> -                            &plus_svm_features);
> -        } else if (featurestr[0] == '-') {
> -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> -                            &minus_ext_features, &minus_ext2_features,
> -                            &minus_ext3_features, &minus_kvm_features,
> -                            &minus_svm_features);
> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> -            if (!strcmp(featurestr, "family")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff + 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->family = numvalue;
> -            } else if (!strcmp(featurestr, "model")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->model = numvalue;
> -            } else if (!strcmp(featurestr, "stepping")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->stepping = numvalue ;
> -            } else if (!strcmp(featurestr, "level")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->level = numvalue;
> -            } else if (!strcmp(featurestr, "xlevel")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->xlevel = numvalue;
> -            } else if (!strcmp(featurestr, "vendor")) {
> -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> -                x86_cpu_def->vendor_override = 1;
> -            } else if (!strcmp(featurestr, "model_id")) {
> -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> -                        val);
> -            } else if (!strcmp(featurestr, "tsc_freq")) {
> -                int64_t tsc_freq;
> -                char *err;
> -
> -                tsc_freq = strtosz_suffix_unit(val, &err,
> -                                               STRTOSZ_DEFSUFFIX_B, 1000);
> -                if (tsc_freq < 0 || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> -            } else if (!strcmp(featurestr, "hv_spinlocks")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                hyperv_set_spinlock_retries(numvalue);
> -            } else {
> -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> -                goto error;
> -            }
> -        } else if (!strcmp(featurestr, "check")) {
> -            check_cpuid = 1;
> -        } else if (!strcmp(featurestr, "enforce")) {
> -            check_cpuid = enforce_cpuid = 1;
> -        } else if (!strcmp(featurestr, "hv_relaxed")) {
> -            hyperv_enable_relaxed_timing(true);
> -        } else if (!strcmp(featurestr, "hv_vapic")) {
> -            hyperv_enable_vapic_recommended(true);
> -        } else {
> -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> -            goto error;
> -        }
> +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
>  
> +    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)) {
>              goto error;
>          }
> -
> -        featurestr = strtok(NULL, ",");
>      }
> -    x86_cpu_def->features |= plus_features;
> -    x86_cpu_def->ext_features |= plus_ext_features;
> -    x86_cpu_def->ext2_features |= plus_ext2_features;
> -    x86_cpu_def->ext3_features |= plus_ext3_features;
> -    x86_cpu_def->kvm_features |= plus_kvm_features;
> -    x86_cpu_def->svm_features |= plus_svm_features;
> -    x86_cpu_def->features &= ~minus_features;
> -    x86_cpu_def->ext_features &= ~minus_ext_features;
> -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> -    x86_cpu_def->svm_features &= ~minus_svm_features;
> -    g_free(s);
> +    QDECREF(features);
> +
> +    g_free(name);
>      return 0;
>  
>  error:
> -    g_free(s);
> +    g_free(name);
>      if (!error_is_set(errp)) {
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
>      }
> @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
>          goto out;
>  
> -    cpudef_2_x86_cpu(cpu, def, &error);
> -
>  out:
>      if (error_is_set(&error)) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> -- 
> 1.7.11.2
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 18/20] target-i386: use properties to set/unset user specified features on CPU

[Igor Mammedov]

On Wed, 15 Aug 2012 09:37:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Another issue:
> 
> On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 167 +++++++-----------------------------------------------
> >  1 file changed, 20 insertions(+), 147 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7555b08..0174c4d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> [...]
> > @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >          memcpy(x86_cpu_def, def, sizeof(*def));
> >      }
> >  
> > -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> > -
> > -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> > -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> > -        &plus_kvm_features, &plus_svm_features);
> > +    cpudef_2_x86_cpu(cpu, def, errp);
> >  
> > -    featurestr = strtok(NULL, ",");
> > +    /* not supported bits will be filtered out later */
> > +    env->cpuid_kvm_features = ~0;
> 
> This doesn't compile. The 'env' variable doesn't exist inside
> cpu_x86_find_by_name().
Thanks, it's fixed now.
> 
> >  
> > -    while (featurestr) {
> > -        char *val;
> > -        if (featurestr[0] == '+') {
> > -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > -                            &plus_ext_features, &plus_ext2_features,
> > -                            &plus_ext3_features, &plus_kvm_features,
> > -                            &plus_svm_features);
> > -        } else if (featurestr[0] == '-') {
> > -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> > -                            &minus_ext_features, &minus_ext2_features,
> > -                            &minus_ext3_features, &minus_kvm_features,
> > -                            &minus_svm_features);
> > -        } else if ((val = strchr(featurestr, '='))) {
> > -            *val = 0; val++;
> > -            if (!strcmp(featurestr, "family")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xff + 0xf) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->family = numvalue;
> > -            } else if (!strcmp(featurestr, "model")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xff) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->model = numvalue;
> > -            } else if (!strcmp(featurestr, "stepping")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xf) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->stepping = numvalue ;
> > -            } else if (!strcmp(featurestr, "level")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->level = numvalue;
> > -            } else if (!strcmp(featurestr, "xlevel")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->xlevel = numvalue;
> > -            } else if (!strcmp(featurestr, "vendor")) {
> > -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> > -                x86_cpu_def->vendor_override = 1;
> > -            } else if (!strcmp(featurestr, "model_id")) {
> > -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> > -                        val);
> > -            } else if (!strcmp(featurestr, "tsc_freq")) {
> > -                int64_t tsc_freq;
> > -                char *err;
> > -
> > -                tsc_freq = strtosz_suffix_unit(val, &err,
> > -                                               STRTOSZ_DEFSUFFIX_B, 1000);
> > -                if (tsc_freq < 0 || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > -            } else if (!strcmp(featurestr, "hv_spinlocks")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                hyperv_set_spinlock_retries(numvalue);
> > -            } else {
> > -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> > -                goto error;
> > -            }
> > -        } else if (!strcmp(featurestr, "check")) {
> > -            check_cpuid = 1;
> > -        } else if (!strcmp(featurestr, "enforce")) {
> > -            check_cpuid = enforce_cpuid = 1;
> > -        } else if (!strcmp(featurestr, "hv_relaxed")) {
> > -            hyperv_enable_relaxed_timing(true);
> > -        } else if (!strcmp(featurestr, "hv_vapic")) {
> > -            hyperv_enable_vapic_recommended(true);
> > -        } else {
> > -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> > -            goto error;
> > -        }
> > +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> >  
> > +    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)) {
> >              goto error;
> >          }
> > -
> > -        featurestr = strtok(NULL, ",");
> >      }
> > -    x86_cpu_def->features |= plus_features;
> > -    x86_cpu_def->ext_features |= plus_ext_features;
> > -    x86_cpu_def->ext2_features |= plus_ext2_features;
> > -    x86_cpu_def->ext3_features |= plus_ext3_features;
> > -    x86_cpu_def->kvm_features |= plus_kvm_features;
> > -    x86_cpu_def->svm_features |= plus_svm_features;
> > -    x86_cpu_def->features &= ~minus_features;
> > -    x86_cpu_def->ext_features &= ~minus_ext_features;
> > -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> > -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> > -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> > -    x86_cpu_def->svm_features &= ~minus_svm_features;
> > -    g_free(s);
> > +    QDECREF(features);
> > +
> > +    g_free(name);
> >      return 0;
> >  
> >  error:
> > -    g_free(s);
> > +    g_free(name);
> >      if (!error_is_set(errp)) {
> >          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> >      }
> > @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
> >          goto out;
> >  
> > -    cpudef_2_x86_cpu(cpu, def, &error);
> > -
> >  out:
> >      if (error_is_set(&error)) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > -- 
> > 1.7.11.2
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 15/20] target-i386: use define for cpuid vendor string size

[Pandarathil, Vijaymohan R]

Since you are introducing #define CPUID_VENDOR_SZ, you may as well want to introduce

#define CPUID_VENDOR_OFF_1  0
#define CPUID_VENDOR_OFF_2  4
#define CPUID_VENDOR_OFF_3  8

and use them.

Regards

Vijay


-----Original Message-----
From: qemu-devel-bounces+vijaymohan.pandarathil=hp.com@nongnu.org [mailto:qemu-devel-bounces+vijaymohan.pandarathil=hp.com@nongnu.org] On Behalf Of Igor Mammedov
Sent: Friday, August 10, 2012 4:23 AM
To: qemu-devel@nongnu.org
Cc: aliguori@us.ibm.com; stefanha@linux.vnet.ibm.com; gleb@redhat.com; jan.kiszka@siemens.com; mtosatti@redhat.com; mdroth@linux.vnet.ibm.com; blauwirbel@gmail.com; avi@redhat.com; pbonzini@redhat.com; akong@redhat.com; lersek@redhat.com; afaerber@suse.de; ehabkost@redhat.com
Subject: [Qemu-devel] [RFC 15/20] target-i386: use define for cpuid vendor string size

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 d3ce849..368360f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -863,13 +863,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;
 }
 
@@ -880,7 +880,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
     CPUX86State *env = &cpu->env;
     int i;
 
-    if (strlen(value) != 12) {
+    if (strlen(value) != CPUID_VENDOR_SZ) {
         error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
                   "vendor", value);
         return;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 979682a..5c75704 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -440,6 +440,8 @@
 #define CPUID_SVM_PAUSEFILTER  (1 << 10)
 #define CPUID_SVM_PFTHRESHOLD  (1 << 12)
 
+#define CPUID_VENDOR_SZ      12
+
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
-- 
1.7.11.2

Re: [Qemu-devel] [RFC 15/20] target-i386: use define for cpuid vendor string size

[Eduardo Habkost]

On Wed, Aug 15, 2012 at 03:52:17PM +0000, Pandarathil, Vijaymohan R wrote:
> Since you are introducing #define CPUID_VENDOR_SZ, you may as well want to introduce
> 
> #define CPUID_VENDOR_OFF_1  0
> #define CPUID_VENDOR_OFF_2  4
> #define CPUID_VENDOR_OFF_3  8
> 
> and use them.
> 

The vendor1/vendor2/vendor3 fields are eliminated on patch 16/20, so I
don't see the need for that.

> Regards
> 
> Vijay
> 
> 
> -----Original Message-----
> From: qemu-devel-bounces+vijaymohan.pandarathil=hp.com@nongnu.org [mailto:qemu-devel-bounces+vijaymohan.pandarathil=hp.com@nongnu.org] On Behalf Of Igor Mammedov
> Sent: Friday, August 10, 2012 4:23 AM
> To: qemu-devel@nongnu.org
> Cc: aliguori@us.ibm.com; stefanha@linux.vnet.ibm.com; gleb@redhat.com; jan.kiszka@siemens.com; mtosatti@redhat.com; mdroth@linux.vnet.ibm.com; blauwirbel@gmail.com; avi@redhat.com; pbonzini@redhat.com; akong@redhat.com; lersek@redhat.com; afaerber@suse.de; ehabkost@redhat.com
> Subject: [Qemu-devel] [RFC 15/20] target-i386: use define for cpuid vendor string size
> 
> 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 d3ce849..368360f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -863,13 +863,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;
>  }
>  
> @@ -880,7 +880,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>      CPUX86State *env = &cpu->env;
>      int i;
>  
> -    if (strlen(value) != 12) {
> +    if (strlen(value) != CPUID_VENDOR_SZ) {
>          error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
>                    "vendor", value);
>          return;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 979682a..5c75704 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -440,6 +440,8 @@
>  #define CPUID_SVM_PAUSEFILTER  (1 << 10)
>  #define CPUID_SVM_PFTHRESHOLD  (1 << 12)
>  
> +#define CPUID_VENDOR_SZ      12
> +
>  #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
>  #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
>  #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> -- 
> 1.7.11.2
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Aug 10, 2012 at 01:22:23PM +0200, Igor Mammedov wrote:
[...]
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
>  {
> @@ -1801,6 +1898,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);

Stupid question about qdev:

- qdev_prop_set_globals() is called from device_initfn()
- device_initfn() is called before the child class instance_init()
  function (x86_cpu_initfn())
- So, qdev_prop_set_globals() gets called before the CPU class
  properties are registered.

So this would defeat the whole point of all the work we're doing, that
is to allow compatibility bits to be set as machine-type global
properties. But I don't know what's the right solution here.

Should the qdev_prop_set_globals() call be moved to qdev_init() instead?
Should the CPU properties be registered somewhere else?

-- 
Eduardo