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

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

[Igor Mammedov]

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


Rebased on top of a9321a4d49d "x86: Implement SMEP and SMAP".

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

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

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

[Igor Mammedov]

it will allow to use property setters there later.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bb1e44e..e1ffa40 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1097,7 +1097,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;
@@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1282,6 +1284,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;
 }
 
@@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1419,6 +1426,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

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

diff --git a/target-i386/helper.c b/target-i386/helper.c
index c635667..1d39ba9 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1243,6 +1243,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;
@@ -1253,8 +1254,12 @@ X86CPU *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
 
-    x86_cpu_realize(OBJECT(cpu), NULL);
-
+    x86_cpu_realize(OBJECT(cpu), &error);
+    if (error) {
+        error_free(error);
+        object_delete(OBJECT(cpu));
+        return NULL;
+    }
     return cpu;
 }
 
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b7bdbd..a58b30d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -984,8 +984,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)
@@ -1242,9 +1251,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                     fprintf(stderr, "bad numerical value %s\n", val);
                     goto error;
                 }
-                if (numvalue < 0x80000000) {
-                    numvalue += 0x80000000;
-                }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
                 if (strlen(val) != 12) {
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5604f13..536dde5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1211,6 +1211,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;
@@ -2043,6 +2069,11 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+#if !defined(CONFIG_USER_ONLY)
+    object_property_add(obj, "hv_spinlocks", "int",
+                        x86_get_hv_spinlocks,
+                        x86_set_hv_spinlocks, NULL, NULL, NULL);
+#endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
     x86_register_cpuid_properties(obj, ext2_feature_name);
-- 
1.7.1

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 536dde5..0788a1e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1235,6 +1235,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)
@@ -2073,6 +2093,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "hv_spinlocks", "int",
                         x86_get_hv_spinlocks,
                         x86_set_hv_spinlocks, NULL, NULL, NULL);
+    object_property_add(obj, "hv_relaxed", "bool",
+                        x86_get_hv_relaxed,
+                        x86_set_hv_relaxed, NULL, NULL, NULL);
 #endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
-- 
1.7.1

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0788a1e..86f60b1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1255,6 +1255,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)
@@ -2096,6 +2116,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "hv_relaxed", "bool",
                         x86_get_hv_relaxed,
                         x86_set_hv_relaxed, NULL, NULL, NULL);
+    object_property_add(obj, "hv_vapic", "bool",
+                        x86_set_hv_vapic,
+                        x86_get_hv_vapic, NULL, NULL, NULL);
 #endif
     x86_register_cpuid_properties(obj, feature_name);
     x86_register_cpuid_properties(obj, ext_feature_name);
-- 
1.7.1

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 86f60b1..9bfa521 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -121,8 +121,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)
@@ -818,19 +818,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);
@@ -1277,6 +1278,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;
@@ -1492,10 +1530,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
-    if (check_cpuid) {
-        if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
-            goto error;
-    }
     if (x86_cpu_def->cpuid_7_0_ebx_features && x86_cpu_def->level < 7) {
         x86_cpu_def->level = 7;
     }
@@ -2056,6 +2090,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
 
+    if (check_cpuid && check_features_against_host(cpu)
+        && enforce_cpuid) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
@@ -2109,6 +2149,12 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    object_property_add(obj, "check", "bool",
+                        x86_cpuid_get_check,
+                        x86_cpuid_set_check, NULL, NULL, NULL);
+    object_property_add(obj, "enforce", "bool",
+                        x86_cpuid_get_enforce,
+                        x86_cpuid_set_enforce, NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
     object_property_add(obj, "hv_spinlocks", "int",
                         x86_get_hv_spinlocks,
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

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

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

[Qemu-devel] [PATCH 15/23] 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 7bb6caa..04497f1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1141,7 +1141,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)
@@ -1315,6 +1314,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;
@@ -1328,7 +1352,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);
@@ -2143,6 +2167,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 e4a7d5b..f99a735 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -788,7 +788,7 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
+    bool cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.1

[Qemu-devel] [PATCH 16/23] 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 04497f1..038b37e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1110,13 +1110,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;
 }
 
@@ -1127,7 +1127,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 f99a735..c8a5270 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -488,6 +488,8 @@
 #define CPUID_7_0_EBX_SMEP     (1 << 7)
 #define CPUID_7_0_EBX_SMAP     (1 << 20)
 
+#define CPUID_VENDOR_SZ      12
+
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
-- 
1.7.1

[Qemu-devel] [PATCH 17/23] target-i386: postpone cpuid_level update to realize time

[Igor Mammedov]

delay force set cpuid_level to 7 to realize time so property setters
for cpuid_7_0_ebx_features and "level" could be used in any order/time
between x86_cpu_initfn() and x86_cpu_realize().

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 038b37e..0e9a2f1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1554,9 +1554,7 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
-    if (x86_cpu_def->cpuid_7_0_ebx_features && x86_cpu_def->level < 7) {
-        x86_cpu_def->level = 7;
-    }
+
     g_free(s);
     return 0;
 
@@ -2120,6 +2118,10 @@ void x86_cpu_realize(Object *obj, Error **errp)
         return;
     }
 
+    if (env->cpuid_7_0_ebx_features && env->cpuid_level < 7) {
+        env->cpuid_level = 7;
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e9a2f1..cae5aed 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -244,7 +244,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;
@@ -309,9 +309,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,
@@ -328,9 +326,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,
@@ -374,9 +370,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,
@@ -475,9 +469,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,
@@ -509,9 +501,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Conroe",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -529,9 +519,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Penryn",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -550,9 +538,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Nehalem",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -571,9 +557,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Westmere",
         .level = 11,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 44,
         .stepping = 1,
@@ -593,9 +577,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "SandyBridge",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 42,
         .stepping = 1,
@@ -618,9 +600,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G1",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -642,9 +622,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G2",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -668,9 +646,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G3",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -696,9 +672,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G4",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 1,
         .stepping = 2,
@@ -745,13 +719,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);
@@ -776,9 +753,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 */
@@ -1343,15 +1318,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);
@@ -1385,7 +1353,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);
@@ -1486,18 +1453,7 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                if (strlen(val) != 12) {
-                    fprintf(stderr, "vendor string must be 12 chars long\n");
-                    goto error;
-                }
-                x86_cpu_def->vendor1 = 0;
-                x86_cpu_def->vendor2 = 0;
-                x86_cpu_def->vendor3 = 0;
-                for(i = 0; i < 4; i++) {
-                    x86_cpu_def->vendor1 |= ((uint8_t)val[i    ]) << (8 * i);
-                    x86_cpu_def->vendor2 |= ((uint8_t)val[i + 4]) << (8 * i);
-                    x86_cpu_def->vendor3 |= ((uint8_t)val[i + 8]) << (8 * i);
-                }
+                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
                 x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index c8a5270..7808141 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -493,14 +493,14 @@
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+#define CPUID_VENDOR_INTEL "GenuineIntel"
 
 #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
 #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
 #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
 
-#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_VIA   "CentaurHauls"
 
 #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

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

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9b3cffd..22a1ded 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -221,25 +221,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,
-                                    uint32_t *cpuid_7_0_ebx_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) &&
-        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
-                        cpuid_7_0_ebx_feature_name))
-            fprintf(stderr, "CPU feature %s not found\n", flagname);
-}
-
 typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
@@ -847,7 +828,9 @@ static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
        !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)) {
+       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name) &&
+       !is_feature_set(name, env->cpuid_7_0_ebx_features,
+                       cpuid_7_0_ebx_feature_name)) {
         value = false;
     }
 
@@ -880,6 +863,8 @@ static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque,
         dst_features = &env->cpuid_kvm_features;
     } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) {
         dst_features = &env->cpuid_svm_features;
+    } else if (lookup_feature(&mask, name, NULL, cpuid_7_0_ebx_feature_name)) {
+        dst_features = &env->cpuid_7_0_ebx_features;
     } else {
         error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
         return;
@@ -1333,7 +1318,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_features = def->cpuid_7_0_ebx_features;
@@ -1401,24 +1385,34 @@ static void compat_normalize_cpu_model(const char *cpu_model, char **cpu_name,
     return;
 }
 
+/* Set features on X86CPU object based on a QDict */
+static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
+        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
+        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
+                              qdict_entry_key(ent), errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
     x86_def_t *def;
 
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
-    /* Features to be added*/
-    uint32_t plus_features = 0, plus_ext_features = 0;
-    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
-    uint32_t plus_7_0_ebx_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 minus_7_0_ebx_features = 0;
-    uint32_t numvalue;
+    CPUX86State *env = &cpu->env;
+    QDict *features = NULL;
+    char *name = NULL;
+
+    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
+    if (error_is_set(errp)) {
+        goto error;
+    }
 
     for (def = x86_defs; def; def = def->next)
         if (name && !strcmp(name, def->name))
@@ -1431,8 +1425,10 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
+    cpudef_2_x86_cpu(cpu, x86_cpu_def, errp);
+
 #if defined(CONFIG_KVM)
-    plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
+    env->cpuid_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_NOP_IO_DELAY) | 
         (1 << KVM_FEATURE_MMU_OP) |
         (1 << KVM_FEATURE_CLOCKSOURCE2) |
@@ -1440,133 +1436,23 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
 #else
-    plus_kvm_features = 0;
+    env->cpuid_kvm_features = 0;
 #endif
 
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-            &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-            &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
-
-    featurestr = strtok(NULL, ",");
-
-    while (featurestr) {
-        char *val;
-        if (featurestr[0] == '+') {
-            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
-                            &plus_ext_features, &plus_ext2_features,
-                            &plus_ext3_features, &plus_kvm_features,
-                            &plus_svm_features, &plus_7_0_ebx_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, &minus_7_0_ebx_features);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
-            if (!strcmp(featurestr, "family")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff + 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->family = numvalue;
-            } else if (!strcmp(featurestr, "model")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->model = numvalue;
-            } else if (!strcmp(featurestr, "stepping")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->stepping = numvalue ;
-            } else if (!strcmp(featurestr, "level")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->level = numvalue;
-            } else if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->xlevel = numvalue;
-            } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
-            } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
-            } else if (!strcmp(featurestr, "tsc_freq")) {
-                int64_t tsc_freq;
-                char *err;
-
-                tsc_freq = strtosz_suffix_unit(val, &err,
-                                               STRTOSZ_DEFSUFFIX_B, 1000);
-                if (tsc_freq < 0 || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
-            } else if (!strcmp(featurestr, "hv_spinlocks")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                hyperv_set_spinlock_retries(numvalue);
-            } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
-                goto error;
-            }
-        } else if (!strcmp(featurestr, "check")) {
-            check_cpuid = 1;
-        } else if (!strcmp(featurestr, "enforce")) {
-            check_cpuid = enforce_cpuid = 1;
-        } else if (!strcmp(featurestr, "hv_relaxed")) {
-            hyperv_enable_relaxed_timing(true);
-        } else if (!strcmp(featurestr, "hv_vapic")) {
-            hyperv_enable_vapic_recommended(true);
-        } else {
-            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
-            goto error;
-        }
+    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
-        featurestr = strtok(NULL, ",");
+    cpu_x86_set_props(cpu, features, errp);
+    QDECREF(features);
+    if (error_is_set(errp)) {
+        goto error;
     }
-    x86_cpu_def->features |= plus_features;
-    x86_cpu_def->ext_features |= plus_ext_features;
-    x86_cpu_def->ext2_features |= plus_ext2_features;
-    x86_cpu_def->ext3_features |= plus_ext3_features;
-    x86_cpu_def->kvm_features |= plus_kvm_features;
-    x86_cpu_def->svm_features |= plus_svm_features;
-    x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_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;
-    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
-
-    g_free(s);
+
+    g_free(name);
     return 0;
 
 error:
-    g_free(s);
+    g_free(name);
+    QDECREF(features);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1659,8 +1545,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto out;
     }
 
-    cpudef_2_x86_cpu(cpu, def, &error);
-
 out:
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
@@ -2208,6 +2092,7 @@ static void x86_cpu_initfn(Object *obj)
     x86_register_cpuid_properties(obj, ext3_feature_name);
     x86_register_cpuid_properties(obj, kvm_feature_name);
     x86_register_cpuid_properties(obj, svm_feature_name);
+    x86_register_cpuid_properties(obj, cpuid_7_0_ebx_feature_name);
 
     env->cpuid_apic_id = env->cpu_index;
 
-- 
1.7.1

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

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

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

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

[Igor Mammedov]

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

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

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

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

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

[Eduardo Habkost]

On Tue, Oct 02, 2012 at 05:37:12PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v2:
>   * fix error of initializing from incorrect cpu model definition
>     use x86_cpu_def instead of def. spotted-by: Eduardo Habkost
>   * add missing env in cpu_x86_find_by_name()
>   * added cpu_x86_set_props() to make following code movement more
>     clean. suggested-by: Eduardo Habkost
>   * init name and feature to NULL, to avoid freeing uninitialized mem
> v3:
>   * rebase on top of "x86: Implement SMEP and SMAP"
> ---
>  target-i386/cpu.c |  199 +++++++++++-----------------------------------------
>  1 files changed, 42 insertions(+), 157 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9b3cffd..22a1ded 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -221,25 +221,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,
> -                                    uint32_t *cpuid_7_0_ebx_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) &&
> -        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
> -                        cpuid_7_0_ebx_feature_name))
> -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> -}
> -
>  typedef struct x86_def_t {
>      struct x86_def_t *next;
>      const char *name;
> @@ -847,7 +828,9 @@ static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
>         !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)) {
> +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name) &&
> +       !is_feature_set(name, env->cpuid_7_0_ebx_features,
> +                       cpuid_7_0_ebx_feature_name)) {

If you ever need to respin this series, it would be nice to do this at
patch 07/23 instead.

>          value = false;
>      }
>  
[...]
> @@ -2208,6 +2092,7 @@ static void x86_cpu_initfn(Object *obj)
>      x86_register_cpuid_properties(obj, ext3_feature_name);
>      x86_register_cpuid_properties(obj, kvm_feature_name);
>      x86_register_cpuid_properties(obj, svm_feature_name);
> +    x86_register_cpuid_properties(obj, cpuid_7_0_ebx_feature_name);

This could go to patch 07/23 as well.

It's not a problem to keep it here, however, because the new properties
don't get actually used until this patch is applied.

>  
>      env->cpuid_apic_id = env->cpu_index;
>  
> -- 
> 1.7.1
> 

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 2 Oct 2012 13:01:40 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 02, 2012 at 05:37:12PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > v2:
> >   * fix error of initializing from incorrect cpu model definition
> >     use x86_cpu_def instead of def. spotted-by: Eduardo Habkost
> >   * add missing env in cpu_x86_find_by_name()
> >   * added cpu_x86_set_props() to make following code movement more
> >     clean. suggested-by: Eduardo Habkost
> >   * init name and feature to NULL, to avoid freeing uninitialized mem
> > v3:
> >   * rebase on top of "x86: Implement SMEP and SMAP"
> > ---
> >  target-i386/cpu.c |  199
> > +++++++++++----------------------------------------- 1 files changed, 42
> > insertions(+), 157 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9b3cffd..22a1ded 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -221,25 +221,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,
> > -                                    uint32_t *cpuid_7_0_ebx_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)
> > &&
> > -        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
> > -                        cpuid_7_0_ebx_feature_name))
> > -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> > -}
> > -
> >  typedef struct x86_def_t {
> >      struct x86_def_t *next;
> >      const char *name;
> > @@ -847,7 +828,9 @@ static void x86_cpuid_get_feature(Object *obj,
> > Visitor *v, void *opaque, !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))
> > {
> > +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)
> > &&
> > +       !is_feature_set(name, env->cpuid_7_0_ebx_features,
> > +                       cpuid_7_0_ebx_feature_name)) {
> 
> If you ever need to respin this series, it would be nice to do this at
> patch 07/23 instead.
> 
> >          value = false;
> >      }
> >  
> [...]
> > @@ -2208,6 +2092,7 @@ static void x86_cpu_initfn(Object *obj)
> >      x86_register_cpuid_properties(obj, ext3_feature_name);
> >      x86_register_cpuid_properties(obj, kvm_feature_name);
> >      x86_register_cpuid_properties(obj, svm_feature_name);
> > +    x86_register_cpuid_properties(obj, cpuid_7_0_ebx_feature_name);
> 
> This could go to patch 07/23 as well.
Thanks,
I'll move it there and queue for next respin.

> 
> It's not a problem to keep it here, however, because the new properties
> don't get actually used until this patch is applied.
> 
> >  
> >      env->cpuid_apic_id = env->cpu_index;
> >  
> > -- 
> > 1.7.1
> > 
> 

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

[Eduardo Habkost]

(Now replying on the right thread, to keep the discussion in the right
place. I don't know how I ended up replying to a pre-historic version of
the patch, sorry.)

On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
[...]
> @@ -1938,6 +2043,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

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

[Eduardo Habkost]

On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> (Now replying on the right thread, to keep the discussion in the right
> place. I don't know how I ended up replying to a pre-historic version of
> the patch, sorry.)
> 
> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> [...]
> > @@ -1938,6 +2043,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?

Replying to myself: it wouldn't make sense to call it on qdev_init() as
it would overwrite properties set between the calls to object_new() and
qdev_init().

But I still don't see what would be the proper way to solve this. I
don't see any mechanism that would allow us to ensure the
object_property_add() calls are made before qdev_prop_set_globals() is
called.

-- 
Eduardo

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

[Paolo Bonzini]

Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
>> (Now replying on the right thread, to keep the discussion in the right
>> place. I don't know how I ended up replying to a pre-historic version of
>> the patch, sorry.)
>>
>> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
>> [...]
>>> @@ -1938,6 +2043,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?

Properties should be registered (for all objects, not just CPUs) in the
instance_init function.  This is device_initfn.

I would add an instance_postinit function that is called at the end of
object_initialize_with_type, that is after instance_init, and in the
opposite order (i.e. from the leaf to the root).

Paolo

> Replying to myself: it wouldn't make sense to call it on qdev_init() as
> it would overwrite properties set between the calls to object_new() and
> qdev_init().
> 
> But I still don't see what would be the proper way to solve this. I
> don't see any mechanism that would allow us to ensure the
> object_property_add() calls are made before qdev_prop_set_globals() is
> called.
> 

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

[Igor Mammedov]

On Wed, 03 Oct 2012 17:20:46 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> >> (Now replying on the right thread, to keep the discussion in the right
> >> place. I don't know how I ended up replying to a pre-historic version of
> >> the patch, sorry.)
> >>
> >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> >> [...]
> >>> @@ -1938,6 +2043,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?
> 
> Properties should be registered (for all objects, not just CPUs) in the
> instance_init function.  This is device_initfn.
> 
> I would add an instance_postinit function that is called at the end of
> object_initialize_with_type, that is after instance_init, and in the
> opposite order (i.e. from the leaf to the root).

You've meant something like that?

diff --git a/hw/qdev.c b/hw/qdev.c
index b5a52ac..4eb5f44 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -682,12 +682,17 @@ static void device_initfn(Object *obj)
         }
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
-    qdev_prop_set_globals(dev);
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL);
 }
 
+static void device_postinitfn(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    qdev_prop_set_globals(dev);
+}
+
 /* Unlink device from bus and free the structure.  */
 static void device_finalize(Object *obj)
 {
@@ -750,6 +755,7 @@ static TypeInfo device_type_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(DeviceState),
     .instance_init = device_initfn,
+    .instance_postinit = device_postinitfn,
     .instance_finalize = device_finalize,
     .class_base_init = device_class_base_init,
     .abstract = true,
diff --git a/include/qemu/object.h b/include/qemu/object.h
index cc75fee..dfb5d8a 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -311,6 +311,7 @@ struct TypeInfo
 
     size_t instance_size;
     void (*instance_init)(Object *obj);
+    void (*instance_postinit)(Object *obj);
     void (*instance_finalize)(Object *obj);
 
     bool abstract;
diff --git a/qom/object.c b/qom/object.c
index e3e9242..979b410 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -49,6 +49,7 @@ struct TypeImpl
     void *class_data;
 
     void (*instance_init)(Object *obj);
+    void (*instance_postinit)(Object *obj);
     void (*instance_finalize)(Object *obj);
 
     bool abstract;
@@ -295,6 +296,17 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
+static void object_postinit_with_type(Object *obj, TypeImpl *ti)
+{
+    if (ti->instance_postinit) {
+        ti->instance_postinit(obj);
+    }
+
+    if (type_has_parent(ti)) {
+        object_postinit_with_type(obj, type_get_parent(ti));
+    }
+}
+
 void object_initialize_with_type(void *data, TypeImpl *type)
 {
     Object *obj = data;
@@ -309,6 +321,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     obj->class = type->class;
     QTAILQ_INIT(&obj->properties);
     object_init_with_type(obj, type);
+    object_postinit_with_type(obj, type);
 }
 
 void object_initialize(void *data, const char *typename)

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

[Eduardo Habkost]

On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> On Wed, 03 Oct 2012 17:20:46 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> > >> (Now replying on the right thread, to keep the discussion in the right
> > >> place. I don't know how I ended up replying to a pre-historic version of
> > >> the patch, sorry.)
> > >>
> > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> > >> [...]
> > >>> @@ -1938,6 +2043,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?
> > 
> > Properties should be registered (for all objects, not just CPUs) in the
> > instance_init function.  This is device_initfn.
> > 
> > I would add an instance_postinit function that is called at the end of
> > object_initialize_with_type, that is after instance_init, and in the
> > opposite order (i.e. from the leaf to the root).
> 
> You've meant something like that?
> 

That's almost exactly the same code I wrote here. :-)

The only difference is that I added post_init to the struct Object
documentation comments, and added a unit test. The unit test required
the qdev-core/qdev split, so we could compile it without bringing too
many dependencies. I will submit it soon.


> diff --git a/hw/qdev.c b/hw/qdev.c
> index b5a52ac..4eb5f44 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -682,12 +682,17 @@ static void device_initfn(Object *obj)
>          }
>          class = object_class_get_parent(class);
>      } while (class != object_class_by_name(TYPE_DEVICE));
> -    qdev_prop_set_globals(dev);
>  
>      object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
>                               (Object **)&dev->parent_bus, NULL);
>  }
>  
> +static void device_postinitfn(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    qdev_prop_set_globals(dev);
> +}
> +
>  /* Unlink device from bus and free the structure.  */
>  static void device_finalize(Object *obj)
>  {
> @@ -750,6 +755,7 @@ static TypeInfo device_type_info = {
>      .parent = TYPE_OBJECT,
>      .instance_size = sizeof(DeviceState),
>      .instance_init = device_initfn,
> +    .instance_postinit = device_postinitfn,
>      .instance_finalize = device_finalize,
>      .class_base_init = device_class_base_init,
>      .abstract = true,
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index cc75fee..dfb5d8a 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -311,6 +311,7 @@ struct TypeInfo
>  
>      size_t instance_size;
>      void (*instance_init)(Object *obj);
> +    void (*instance_postinit)(Object *obj);
>      void (*instance_finalize)(Object *obj);
>  
>      bool abstract;
> diff --git a/qom/object.c b/qom/object.c
> index e3e9242..979b410 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -49,6 +49,7 @@ struct TypeImpl
>      void *class_data;
>  
>      void (*instance_init)(Object *obj);
> +    void (*instance_postinit)(Object *obj);
>      void (*instance_finalize)(Object *obj);
>  
>      bool abstract;
> @@ -295,6 +296,17 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
>      }
>  }
>  
> +static void object_postinit_with_type(Object *obj, TypeImpl *ti)
> +{
> +    if (ti->instance_postinit) {
> +        ti->instance_postinit(obj);
> +    }
> +
> +    if (type_has_parent(ti)) {
> +        object_postinit_with_type(obj, type_get_parent(ti));
> +    }
> +}
> +
>  void object_initialize_with_type(void *data, TypeImpl *type)
>  {
>      Object *obj = data;
> @@ -309,6 +321,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
>      obj->class = type->class;
>      QTAILQ_INIT(&obj->properties);
>      object_init_with_type(obj, type);
> +    object_postinit_with_type(obj, type);
>  }
>  
>  void object_initialize(void *data, const char *typename)

-- 
Eduardo

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

[Igor Mammedov]

On Wed, 3 Oct 2012 13:54:34 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > On Wed, 03 Oct 2012 17:20:46 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> > > >> (Now replying on the right thread, to keep the discussion in the
> > > >> right place. I don't know how I ended up replying to a pre-historic
> > > >> version of the patch, sorry.)
> > > >>
> > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> > > >> [...]
> > > >>> @@ -1938,6 +2043,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?
> > > 
> > > Properties should be registered (for all objects, not just CPUs) in the
> > > instance_init function.  This is device_initfn.
> > > 
> > > I would add an instance_postinit function that is called at the end of
> > > object_initialize_with_type, that is after instance_init, and in the
> > > opposite order (i.e. from the leaf to the root).
> > 
> > You've meant something like that?
> > 
> 
> That's almost exactly the same code I wrote here. :-)
> 
> The only difference is that I added post_init to the struct Object
> documentation comments, and added a unit test. The unit test required
> the qdev-core/qdev split, so we could compile it without bringing too
> many dependencies. I will submit it soon.
> 
After irc discussion, Anthony suggested to use static properties instead of
dynamic ones that we use now. 

But  qdev_prop_set_globals() in device_initfn() is still causes problems even
with static properties.

For x86 CPU classes we were going dynamically generate CPU classes and store
pointer to appropriate cpudef from builtin_x86_defs in class field for each
CPU class and then init default feature words values from this field int
x86_cpu_initfn().

However with qdev_prop_set_globals() in device_initfn() that is called before
x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will
overwrite whatever was set by qdev_prop_set_globals().

IMHO from general POV it's not correct to set properties before object is
completely created.
But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so could
we move it from device_initfn() to qdev_init() or some other place then?

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

[Paolo Bonzini]

Il 04/10/2012 08:53, Igor Mammedov ha scritto:
> IMHO from general POV it's not correct to set properties before object is
> completely created.
> But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so could
> we move it from device_initfn() to qdev_init() or some other place then?

Yes, moving it qdev_init would work.  I don't like it particularly but
hey...

Paolo

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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> On Wed, 3 Oct 2012 13:54:34 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > 
> > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> > > > >> (Now replying on the right thread, to keep the discussion in the
> > > > >> right place. I don't know how I ended up replying to a pre-historic
> > > > >> version of the patch, sorry.)
> > > > >>
> > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> > > > >> [...]
> > > > >>> @@ -1938,6 +2043,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?
> > > > 
> > > > Properties should be registered (for all objects, not just CPUs) in the
> > > > instance_init function.  This is device_initfn.
> > > > 
> > > > I would add an instance_postinit function that is called at the end of
> > > > object_initialize_with_type, that is after instance_init, and in the
> > > > opposite order (i.e. from the leaf to the root).
> > > 
> > > You've meant something like that?
> > > 
> > 
> > That's almost exactly the same code I wrote here. :-)
> > 
> > The only difference is that I added post_init to the struct Object
> > documentation comments, and added a unit test. The unit test required
> > the qdev-core/qdev split, so we could compile it without bringing too
> > many dependencies. I will submit it soon.
> > 
> After irc discussion, Anthony suggested to use static properties instead of
> dynamic ones that we use now. 
> 
> But  qdev_prop_set_globals() in device_initfn() is still causes problems even
> with static properties.
> 
> For x86 CPU classes we were going dynamically generate CPU classes and store
> pointer to appropriate cpudef from builtin_x86_defs in class field for each
> CPU class and then init default feature words values from this field int
> x86_cpu_initfn().
> 
> However with qdev_prop_set_globals() in device_initfn() that is called before
> x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will
> overwrite whatever was set by qdev_prop_set_globals().

We can set the default values on class_init, instead. The class_init
function for each CPU model can get the x86_def_t struct as the data
pointer.

I still think that the interface to build the DeviceClass.props array on
class_init is really painful to use, but it's still doable.

> 
> IMHO from general POV it's not correct to set properties before object is
> completely created.

If I understood it correctly, the point of all this is to allow
properties (and their defaults) to be introspected by just looking at
the class, without having to create any object.

IMO the problem is that we don't have any decent mechanism to implement
machine-type compatibility behavior that doesn't involve having to deal
with the strict rules of global properties. All this work is blocking us
from implementing many necessary bug fixes.

It's OK if the long-term goal is to move the code to this generic,
flexible, fully-instropectable, complex framework, but we need to have a
way to implement bug fixes without waiting for all this work to be
finished.


> But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so could
> we move it from device_initfn() to qdev_init() or some other place then?

-- 
Eduardo

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

[Andreas Färber]

Am 04.10.2012 14:43, schrieb Eduardo Habkost:
> On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
>> For x86 CPU classes we were going dynamically generate CPU classes and store
>> pointer to appropriate cpudef from builtin_x86_defs in class field for each
>> CPU class and then init default feature words values from this field int
>> x86_cpu_initfn().
>>
>> However with qdev_prop_set_globals() in device_initfn() that is called before
>> x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will
>> overwrite whatever was set by qdev_prop_set_globals().
> 
> We can set the default values on class_init, instead. The class_init
> function for each CPU model can get the x86_def_t struct as the data
> pointer.

Let's avoid going backwards here, the plan was to have imperative
initfns, so x86_def_t would go away, no?

I'm catching up my mail on multiple fronts and will continue review,
IIUC Blue already applied the CPU feature deduplification series so
according to your roadmap this series is next.

>> IMHO from general POV it's not correct to set properties before object is
>> completely created.
> 
> If I understood it correctly, the point of all this is to allow
> properties (and their defaults) to be introspected by just looking at
> the class, without having to create any object.

It would be news to me that that was Anthony's plan... Static properties
are being assigned to the class but populated at instantiation time. We
do not have class properties as such.

Andreas

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

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

[Igor Mammedov]

On Thu, 4 Oct 2012 09:43:41 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > On Wed, 3 Oct 2012 13:54:34 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > 
> > > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> > > > > >> (Now replying on the right thread, to keep the discussion in the
> > > > > >> right place. I don't know how I ended up replying to a
> > > > > >> pre-historic version of the patch, sorry.)
> > > > > >>
> > > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> > > > > >> [...]
> > > > > >>> @@ -1938,6 +2043,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?
> > > > > 
> > > > > Properties should be registered (for all objects, not just CPUs) in
> > > > > the instance_init function.  This is device_initfn.
> > > > > 
> > > > > I would add an instance_postinit function that is called at the end
> > > > > of object_initialize_with_type, that is after instance_init, and in
> > > > > the opposite order (i.e. from the leaf to the root).
> > > > 
> > > > You've meant something like that?
> > > > 
> > > 
> > > That's almost exactly the same code I wrote here. :-)
> > > 
> > > The only difference is that I added post_init to the struct Object
> > > documentation comments, and added a unit test. The unit test required
> > > the qdev-core/qdev split, so we could compile it without bringing too
> > > many dependencies. I will submit it soon.
> > > 
> > After irc discussion, Anthony suggested to use static properties instead
> > of dynamic ones that we use now. 
> > 
> > But  qdev_prop_set_globals() in device_initfn() is still causes problems
> > even with static properties.
> > 
> > For x86 CPU classes we were going dynamically generate CPU classes and
> > store pointer to appropriate cpudef from builtin_x86_defs in class field
> > for each CPU class and then init default feature words values from this
> > field int x86_cpu_initfn().
> > 
> > However with qdev_prop_set_globals() in device_initfn() that is called
> > before x86_cpu_initfn() it won't work because defaults in
> > x86_cpu_initfn() will overwrite whatever was set by
> > qdev_prop_set_globals().
> 
> We can set the default values on class_init, instead. The class_init
> function for each CPU model can get the x86_def_t struct as the data
> pointer.
> 
> I still think that the interface to build the DeviceClass.props array on
> class_init is really painful to use, but it's still doable.

You mean dynamic building of DeviceClass.props arrays for each CPU sub-class?

> 
> > 
> > IMHO from general POV it's not correct to set properties before object is
> > completely created.
> 
> If I understood it correctly, the point of all this is to allow
> properties (and their defaults) to be introspected by just looking at
> the class, without having to create any object.
> 
> IMO the problem is that we don't have any decent mechanism to implement
> machine-type compatibility behavior that doesn't involve having to deal
> with the strict rules of global properties. All this work is blocking us
> from implementing many necessary bug fixes.
> 
> It's OK if the long-term goal is to move the code to this generic,
> flexible, fully-instropectable, complex framework, but we need to have a
> way to implement bug fixes without waiting for all this work to be
> finished.
> 
> 
> > But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so
> > could we move it from device_initfn() to qdev_init() or some other place
> > then?
> 

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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 02:57:29PM +0200, Andreas Färber wrote:
> Am 04.10.2012 14:43, schrieb Eduardo Habkost:
> > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> >> For x86 CPU classes we were going dynamically generate CPU classes and store
> >> pointer to appropriate cpudef from builtin_x86_defs in class field for each
> >> CPU class and then init default feature words values from this field int
> >> x86_cpu_initfn().
> >>
> >> However with qdev_prop_set_globals() in device_initfn() that is called before
> >> x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will
> >> overwrite whatever was set by qdev_prop_set_globals().
> > 
> > We can set the default values on class_init, instead. The class_init
> > function for each CPU model can get the x86_def_t struct as the data
> > pointer.
> 
> Let's avoid going backwards here, the plan was to have imperative
> initfns, so x86_def_t would go away, no?

It was never my plan, and I still don't see why we would want to do
that.

> 
> I'm catching up my mail on multiple fronts and will continue review,
> IIUC Blue already applied the CPU feature deduplification series so
> according to your roadmap this series is next.

It would be the next series on the queue, yes, but now Anthony wants the
CPU properties to be all static properties (set on the DeviceClass.props
array). (We were discussing this yesterday on #qemu.)

I suggested including this series anyway, and then move the properties
to be static later (after converting CPU to DeviceState), but Anthony
didn't like the idea.


> 
> >> IMHO from general POV it's not correct to set properties before object is
> >> completely created.
> > 
> > If I understood it correctly, the point of all this is to allow
> > properties (and their defaults) to be introspected by just looking at
> > the class, without having to create any object.
> 
> It would be news to me that that was Anthony's plan... Static properties
> are being assigned to the class but populated at instantiation time. We
> do not have class properties as such.

That was the explanation (if I understood it correctly) for why we won't
global properties can't be used to set "dynamic" properties (properties
registered and set on class->instance_init()).

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote:
> On Thu, 4 Oct 2012 09:43:41 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > > On Wed, 3 Oct 2012 13:54:34 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > 
> > > > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> > > > > > >> (Now replying on the right thread, to keep the discussion in the
> > > > > > >> right place. I don't know how I ended up replying to a
> > > > > > >> pre-historic version of the patch, sorry.)
> > > > > > >>
> > > > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> > > > > > >> [...]
> > > > > > >>> @@ -1938,6 +2043,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?
> > > > > > 
> > > > > > Properties should be registered (for all objects, not just CPUs) in
> > > > > > the instance_init function.  This is device_initfn.
> > > > > > 
> > > > > > I would add an instance_postinit function that is called at the end
> > > > > > of object_initialize_with_type, that is after instance_init, and in
> > > > > > the opposite order (i.e. from the leaf to the root).
> > > > > 
> > > > > You've meant something like that?
> > > > > 
> > > > 
> > > > That's almost exactly the same code I wrote here. :-)
> > > > 
> > > > The only difference is that I added post_init to the struct Object
> > > > documentation comments, and added a unit test. The unit test required
> > > > the qdev-core/qdev split, so we could compile it without bringing too
> > > > many dependencies. I will submit it soon.
> > > > 
> > > After irc discussion, Anthony suggested to use static properties instead
> > > of dynamic ones that we use now. 
> > > 
> > > But  qdev_prop_set_globals() in device_initfn() is still causes problems
> > > even with static properties.
> > > 
> > > For x86 CPU classes we were going dynamically generate CPU classes and
> > > store pointer to appropriate cpudef from builtin_x86_defs in class field
> > > for each CPU class and then init default feature words values from this
> > > field int x86_cpu_initfn().
> > > 
> > > However with qdev_prop_set_globals() in device_initfn() that is called
> > > before x86_cpu_initfn() it won't work because defaults in
> > > x86_cpu_initfn() will overwrite whatever was set by
> > > qdev_prop_set_globals().
> > 
> > We can set the default values on class_init, instead. The class_init
> > function for each CPU model can get the x86_def_t struct as the data
> > pointer.
> > 
> > I still think that the interface to build the DeviceClass.props array on
> > class_init is really painful to use, but it's still doable.
> 
> You mean dynamic building of DeviceClass.props arrays for each CPU sub-class?

That's the only solution I see if we want to make all the CPU properties
static, yes.

I'm still not convinced we really need to do that, though. Maybe we can
make static only the ones we really need to be able to implement
machine-type-compatibility global properties?

Machine-type compatibility global properties were the initial reason for
the static-properties requirement. We don't really need to allow _all_
CPU features to be controlled by global properties, only the ones we
need for machine-type compatibility.

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 04 Oct 2012 14:57:29 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 04.10.2012 14:43, schrieb Eduardo Habkost:
> > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> >> For x86 CPU classes we were going dynamically generate CPU classes and
> >> store pointer to appropriate cpudef from builtin_x86_defs in class field
> >> for each CPU class and then init default feature words values from this
> >> field int x86_cpu_initfn().
> >>
> >> However with qdev_prop_set_globals() in device_initfn() that is called
> >> before x86_cpu_initfn() it won't work because defaults in
> >> x86_cpu_initfn() will overwrite whatever was set by
> >> qdev_prop_set_globals().
> > 
> > We can set the default values on class_init, instead. The class_init
> > function for each CPU model can get the x86_def_t struct as the data
> > pointer.
> 
> Let's avoid going backwards here, the plan was to have imperative
> initfns, so x86_def_t would go away, no?
> 
> I'm catching up my mail on multiple fronts and will continue review,
> IIUC Blue already applied the CPU feature deduplification series so
> according to your roadmap this series is next.
> 
I guess no, it's now cpu-as-device that should be next on a queue, since I'm
rewriting this series to use static properties instead so CPU must be a
device.

Though work would be easier if static props we written&applied on top of
this, it's not like we need working for CPU qdev_prop_set_globals() after this
series.

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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 03:10:53PM +0200, Igor Mammedov wrote:
> On Thu, 04 Oct 2012 14:57:29 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
> > Am 04.10.2012 14:43, schrieb Eduardo Habkost:
> > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > >> For x86 CPU classes we were going dynamically generate CPU classes and
> > >> store pointer to appropriate cpudef from builtin_x86_defs in class field
> > >> for each CPU class and then init default feature words values from this
> > >> field int x86_cpu_initfn().
> > >>
> > >> However with qdev_prop_set_globals() in device_initfn() that is called
> > >> before x86_cpu_initfn() it won't work because defaults in
> > >> x86_cpu_initfn() will overwrite whatever was set by
> > >> qdev_prop_set_globals().
> > > 
> > > We can set the default values on class_init, instead. The class_init
> > > function for each CPU model can get the x86_def_t struct as the data
> > > pointer.
> > 
> > Let's avoid going backwards here, the plan was to have imperative
> > initfns, so x86_def_t would go away, no?
> > 
> > I'm catching up my mail on multiple fronts and will continue review,
> > IIUC Blue already applied the CPU feature deduplification series so
> > according to your roadmap this series is next.
> > 
> I guess no, it's now cpu-as-device that should be next on a queue, since I'm
> rewriting this series to use static properties instead so CPU must be a
> device.

Whatever we decide on this thread about CPU properties, cpu-as-device
seems to be the obvious candidate for "next-in-the-queue", or at least
it can be written/discussed in parallel. I plan to submit a new RFC
soon.

> 
> Though work would be easier if static props we written&applied on top of
> this, it's not like we need working for CPU qdev_prop_set_globals() after this
> series.
> 
> 
> 

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 4 Oct 2012 10:10:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote:
> > On Thu, 4 Oct 2012 09:43:41 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > > > On Wed, 3 Oct 2012 13:54:34 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > > > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > > > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > > 
> > > > > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost
> > > > > > > > wrote:
> > > > > > > >> (Now replying on the right thread, to keep the discussion in
> > > > > > > >> the right place. I don't know how I ended up replying to a
> > > > > > > >> pre-historic version of the patch, sorry.)
> > > > > > > >>
> > > > > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov
> > > > > > > >> wrote: [...]
> > > > > > > >>> @@ -1938,6 +2043,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?
> > > > > > > 
> > > > > > > Properties should be registered (for all objects, not just
> > > > > > > CPUs) in the instance_init function.  This is device_initfn.
> > > > > > > 
> > > > > > > I would add an instance_postinit function that is called at the
> > > > > > > end of object_initialize_with_type, that is after
> > > > > > > instance_init, and in the opposite order (i.e. from the leaf to
> > > > > > > the root).
> > > > > > 
> > > > > > You've meant something like that?
> > > > > > 
> > > > > 
> > > > > That's almost exactly the same code I wrote here. :-)
> > > > > 
> > > > > The only difference is that I added post_init to the struct Object
> > > > > documentation comments, and added a unit test. The unit test
> > > > > required the qdev-core/qdev split, so we could compile it without
> > > > > bringing too many dependencies. I will submit it soon.
> > > > > 
> > > > After irc discussion, Anthony suggested to use static properties
> > > > instead of dynamic ones that we use now. 
> > > > 
> > > > But  qdev_prop_set_globals() in device_initfn() is still causes
> > > > problems even with static properties.
> > > > 
> > > > For x86 CPU classes we were going dynamically generate CPU classes and
> > > > store pointer to appropriate cpudef from builtin_x86_defs in class
> > > > field for each CPU class and then init default feature words values
> > > > from this field int x86_cpu_initfn().
> > > > 
> > > > However with qdev_prop_set_globals() in device_initfn() that is called
> > > > before x86_cpu_initfn() it won't work because defaults in
> > > > x86_cpu_initfn() will overwrite whatever was set by
> > > > qdev_prop_set_globals().
> > > 
> > > We can set the default values on class_init, instead. The class_init
> > > function for each CPU model can get the x86_def_t struct as the data
> > > pointer.
> > > 
> > > I still think that the interface to build the DeviceClass.props array on
> > > class_init is really painful to use, but it's still doable.
> > 
> > You mean dynamic building of DeviceClass.props arrays for each CPU
> > sub-class?
> 
> That's the only solution I see if we want to make all the CPU properties
> static, yes.

Well I could generate compile time arrays for every built-in cpu model
and we can remove then x86_def_t struct & builtins altogether.
Only 'host' would be left for dynamic generation then.

> 
> I'm still not convinced we really need to do that, though. Maybe we can
> make static only the ones we really need to be able to implement
> machine-type-compatibility global properties?
> 
> Machine-type compatibility global properties were the initial reason for
> the static-properties requirement. We don't really need to allow _all_
> CPU features to be controlled by global properties, only the ones we
> need for machine-type compatibility.
> 

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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 03:25:59PM +0200, Igor Mammedov wrote:
> On Thu, 4 Oct 2012 10:10:27 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote:
> > > On Thu, 4 Oct 2012 09:43:41 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > > > > On Wed, 3 Oct 2012 13:54:34 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > 
> > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > > > > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > > > > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > > > 
> > > > > > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost
> > > > > > > > > wrote:
> > > > > > > > >> (Now replying on the right thread, to keep the discussion in
> > > > > > > > >> the right place. I don't know how I ended up replying to a
> > > > > > > > >> pre-historic version of the patch, sorry.)
> > > > > > > > >>
> > > > > > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov
> > > > > > > > >> wrote: [...]
> > > > > > > > >>> @@ -1938,6 +2043,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?
> > > > > > > > 
> > > > > > > > Properties should be registered (for all objects, not just
> > > > > > > > CPUs) in the instance_init function.  This is device_initfn.
> > > > > > > > 
> > > > > > > > I would add an instance_postinit function that is called at the
> > > > > > > > end of object_initialize_with_type, that is after
> > > > > > > > instance_init, and in the opposite order (i.e. from the leaf to
> > > > > > > > the root).
> > > > > > > 
> > > > > > > You've meant something like that?
> > > > > > > 
> > > > > > 
> > > > > > That's almost exactly the same code I wrote here. :-)
> > > > > > 
> > > > > > The only difference is that I added post_init to the struct Object
> > > > > > documentation comments, and added a unit test. The unit test
> > > > > > required the qdev-core/qdev split, so we could compile it without
> > > > > > bringing too many dependencies. I will submit it soon.
> > > > > > 
> > > > > After irc discussion, Anthony suggested to use static properties
> > > > > instead of dynamic ones that we use now. 
> > > > > 
> > > > > But  qdev_prop_set_globals() in device_initfn() is still causes
> > > > > problems even with static properties.
> > > > > 
> > > > > For x86 CPU classes we were going dynamically generate CPU classes and
> > > > > store pointer to appropriate cpudef from builtin_x86_defs in class
> > > > > field for each CPU class and then init default feature words values
> > > > > from this field int x86_cpu_initfn().
> > > > > 
> > > > > However with qdev_prop_set_globals() in device_initfn() that is called
> > > > > before x86_cpu_initfn() it won't work because defaults in
> > > > > x86_cpu_initfn() will overwrite whatever was set by
> > > > > qdev_prop_set_globals().
> > > > 
> > > > We can set the default values on class_init, instead. The class_init
> > > > function for each CPU model can get the x86_def_t struct as the data
> > > > pointer.
> > > > 
> > > > I still think that the interface to build the DeviceClass.props array on
> > > > class_init is really painful to use, but it's still doable.
> > > 
> > > You mean dynamic building of DeviceClass.props arrays for each CPU
> > > sub-class?
> > 
> > That's the only solution I see if we want to make all the CPU properties
> > static, yes.
> 
> Well I could generate compile time arrays for every built-in cpu model
> and we can remove then x86_def_t struct & builtins altogether.
> Only 'host' would be left for dynamic generation then.

You mean duplicating the property list in the code? Then the
feature-name -> CPUID-bit mapping information would be duplicated on all
those arrays, and adding support to a new CPU feature would require
adding entries to all the arrays.


> 
> > 
> > I'm still not convinced we really need to do that, though. Maybe we can
> > make static only the ones we really need to be able to implement
> > machine-type-compatibility global properties?
> > 
> > Machine-type compatibility global properties were the initial reason for
> > the static-properties requirement. We don't really need to allow _all_
> > CPU features to be controlled by global properties, only the ones we
> > need for machine-type compatibility.
> > 
> 

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 4 Oct 2012 10:33:30 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 04, 2012 at 03:25:59PM +0200, Igor Mammedov wrote:
> > On Thu, 4 Oct 2012 10:10:27 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote:
> > > > On Thu, 4 Oct 2012 09:43:41 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > > > > > On Wed, 3 Oct 2012 13:54:34 -0300
> > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > 
> > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > > > > > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > > > > > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > > > > 
> > > > > > > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > > > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost
> > > > > > > > > > wrote:
> > > > > > > > > >> (Now replying on the right thread, to keep the
> > > > > > > > > >> discussion in the right place. I don't know how I ended
> > > > > > > > > >> up replying to a pre-historic version of the patch,
> > > > > > > > > >> sorry.)
> > > > > > > > > >>
> > > > > > > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov
> > > > > > > > > >> wrote: [...]
> > > > > > > > > >>> @@ -1938,6 +2043,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?
> > > > > > > > > 
> > > > > > > > > Properties should be registered (for all objects, not just
> > > > > > > > > CPUs) in the instance_init function.  This is device_initfn.
> > > > > > > > > 
> > > > > > > > > I would add an instance_postinit function that is called at
> > > > > > > > > the end of object_initialize_with_type, that is after
> > > > > > > > > instance_init, and in the opposite order (i.e. from the
> > > > > > > > > leaf to the root).
> > > > > > > > 
> > > > > > > > You've meant something like that?
> > > > > > > > 
> > > > > > > 
> > > > > > > That's almost exactly the same code I wrote here. :-)
> > > > > > > 
> > > > > > > The only difference is that I added post_init to the struct
> > > > > > > Object documentation comments, and added a unit test. The unit
> > > > > > > test required the qdev-core/qdev split, so we could compile it
> > > > > > > without bringing too many dependencies. I will submit it soon.
> > > > > > > 
> > > > > > After irc discussion, Anthony suggested to use static properties
> > > > > > instead of dynamic ones that we use now. 
> > > > > > 
> > > > > > But  qdev_prop_set_globals() in device_initfn() is still causes
> > > > > > problems even with static properties.
> > > > > > 
> > > > > > For x86 CPU classes we were going dynamically generate CPU
> > > > > > classes and store pointer to appropriate cpudef from
> > > > > > builtin_x86_defs in class field for each CPU class and then init
> > > > > > default feature words values from this field int x86_cpu_initfn().
> > > > > > 
> > > > > > However with qdev_prop_set_globals() in device_initfn() that is
> > > > > > called before x86_cpu_initfn() it won't work because defaults in
> > > > > > x86_cpu_initfn() will overwrite whatever was set by
> > > > > > qdev_prop_set_globals().
> > > > > 
> > > > > We can set the default values on class_init, instead. The class_init
> > > > > function for each CPU model can get the x86_def_t struct as the data
> > > > > pointer.
> > > > > 
> > > > > I still think that the interface to build the DeviceClass.props
> > > > > array on class_init is really painful to use, but it's still doable.
> > > > 
> > > > You mean dynamic building of DeviceClass.props arrays for each CPU
> > > > sub-class?
> > > 
> > > That's the only solution I see if we want to make all the CPU properties
> > > static, yes.
> > 
> > Well I could generate compile time arrays for every built-in cpu model
> > and we can remove then x86_def_t struct & builtins altogether.
> > Only 'host' would be left for dynamic generation then.
> 
> You mean duplicating the property list in the code? Then the
> feature-name -> CPUID-bit mapping information would be duplicated on all
> those arrays, and adding support to a new CPU feature would require
> adding entries to all the arrays.
Not to all arrays, but only to ones which cpumodel-s support specific feature.

Here is possible ups&downs this:
 + full introspection, including default cpu features values.
 -/+ it would be possible to represent cpumodel more faithfully, i.e. include
   only features that specific cpu supports. (so no AVX=on in 486 model), not
   sure if it is plus but it would more like real hw.
 - a lot of lines of code , but it could be dealt with extra macros, so
   resulting arrays could look like built-in now (if we add feature there we
   anyway should replicate it other relevant builtins).
 + maybe generating only "host"'s DeviceClass.props could be simplier.

And then for builtin models I could make a series static CPU classes that
would use this arrays.
 

> 
> > 
> > > 
> > > I'm still not convinced we really need to do that, though. Maybe we can
> > > make static only the ones we really need to be able to implement
> > > machine-type-compatibility global properties?
> > > 
> > > Machine-type compatibility global properties were the initial reason for
> > > the static-properties requirement. We don't really need to allow _all_
> > > CPU features to be controlled by global properties, only the ones we
> > > need for machine-type compatibility.
> > > 
> > 
> 

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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 03:50:34PM +0200, Igor Mammedov wrote:
> On Thu, 4 Oct 2012 10:33:30 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 04, 2012 at 03:25:59PM +0200, Igor Mammedov wrote:
> > > On Thu, 4 Oct 2012 10:10:27 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote:
> > > > > On Thu, 4 Oct 2012 09:43:41 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > 
> > > > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote:
> > > > > > > On Wed, 3 Oct 2012 13:54:34 -0300
> > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > > 
> > > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> > > > > > > > > On Wed, 03 Oct 2012 17:20:46 +0200
> > > > > > > > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > > > > > 
> > > > > > > > > > Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > > > > > > > > > > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost
> > > > > > > > > > > wrote:
> > > > > > > > > > >> (Now replying on the right thread, to keep the
> > > > > > > > > > >> discussion in the right place. I don't know how I ended
> > > > > > > > > > >> up replying to a pre-historic version of the patch,
> > > > > > > > > > >> sorry.)
> > > > > > > > > > >>
> > > > > > > > > > >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov
> > > > > > > > > > >> wrote: [...]
> > > > > > > > > > >>> @@ -1938,6 +2043,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?
> > > > > > > > > > 
> > > > > > > > > > Properties should be registered (for all objects, not just
> > > > > > > > > > CPUs) in the instance_init function.  This is device_initfn.
> > > > > > > > > > 
> > > > > > > > > > I would add an instance_postinit function that is called at
> > > > > > > > > > the end of object_initialize_with_type, that is after
> > > > > > > > > > instance_init, and in the opposite order (i.e. from the
> > > > > > > > > > leaf to the root).
> > > > > > > > > 
> > > > > > > > > You've meant something like that?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > That's almost exactly the same code I wrote here. :-)
> > > > > > > > 
> > > > > > > > The only difference is that I added post_init to the struct
> > > > > > > > Object documentation comments, and added a unit test. The unit
> > > > > > > > test required the qdev-core/qdev split, so we could compile it
> > > > > > > > without bringing too many dependencies. I will submit it soon.
> > > > > > > > 
> > > > > > > After irc discussion, Anthony suggested to use static properties
> > > > > > > instead of dynamic ones that we use now. 
> > > > > > > 
> > > > > > > But  qdev_prop_set_globals() in device_initfn() is still causes
> > > > > > > problems even with static properties.
> > > > > > > 
> > > > > > > For x86 CPU classes we were going dynamically generate CPU
> > > > > > > classes and store pointer to appropriate cpudef from
> > > > > > > builtin_x86_defs in class field for each CPU class and then init
> > > > > > > default feature words values from this field int x86_cpu_initfn().
> > > > > > > 
> > > > > > > However with qdev_prop_set_globals() in device_initfn() that is
> > > > > > > called before x86_cpu_initfn() it won't work because defaults in
> > > > > > > x86_cpu_initfn() will overwrite whatever was set by
> > > > > > > qdev_prop_set_globals().
> > > > > > 
> > > > > > We can set the default values on class_init, instead. The class_init
> > > > > > function for each CPU model can get the x86_def_t struct as the data
> > > > > > pointer.
> > > > > > 
> > > > > > I still think that the interface to build the DeviceClass.props
> > > > > > array on class_init is really painful to use, but it's still doable.
> > > > > 
> > > > > You mean dynamic building of DeviceClass.props arrays for each CPU
> > > > > sub-class?
> > > > 
> > > > That's the only solution I see if we want to make all the CPU properties
> > > > static, yes.
> > > 
> > > Well I could generate compile time arrays for every built-in cpu model
> > > and we can remove then x86_def_t struct & builtins altogether.
> > > Only 'host' would be left for dynamic generation then.
> > 
> > You mean duplicating the property list in the code? Then the
> > feature-name -> CPUID-bit mapping information would be duplicated on all
> > those arrays, and adding support to a new CPU feature would require
> > adding entries to all the arrays.
> Not to all arrays, but only to ones which cpumodel-s support specific feature.
> 
> Here is possible ups&downs this:
>  + full introspection, including default cpu features values.
>  -/+ it would be possible to represent cpumodel more faithfully, i.e. include
>    only features that specific cpu supports. (so no AVX=on in 486 model), not
>    sure if it is plus but it would more like real hw.

It looks like a downside, as it would break configurations that could
have worked previously.

I mean: a 486 with AVX may not exist, but that doesn't mean people don't
have existing (and working) VMs in their systems with those settings,
and they would break on a QEMU upgrade.


>  - a lot of lines of code , but it could be dealt with extra macros, so
>    resulting arrays could look like built-in now (if we add feature there we
>    anyway should replicate it other relevant builtins).

The main problem I see is that the macros that define the property have
to define its default value, too.

For example: Westmere supports the tsc-deadline flag, but it
default=false, and SandyBride would support the same flag, but with
default=true. That means that somewhere in the code we will have a line
saying:
 DEFINE_PROP_BIT("tsc-deadline", CPUX86State, ext_features, 24, false)
and somewhere else, we will have a line saying:
 DEFINE_PROP_BIT("tsc-deadline", CPUX86State, ext_features, 24, true)

I don't see how we could let different classes have different defaults
without duplicating the list of properties, even with help of macros.

>  + maybe generating only "host"'s DeviceClass.props could be simplier.

True. But we can still make the "host" class simpler, even if we don't
do any of the above. (It could use a different instance init function,
for example).


Anyway, I don't see why so much effort to generate those property lists
at compile time. The list is set on DeviceClass.props at runtime (at
class_init) so we can generate the property list at runtime inside
class_init, already.


> 
> And then for builtin models I could make a series static CPU classes that
> would use this arrays.
>  
> 
> > 
> > > 
> > > > 
> > > > I'm still not convinced we really need to do that, though. Maybe we can
> > > > make static only the ones we really need to be able to implement
> > > > machine-type-compatibility global properties?
> > > > 
> > > > Machine-type compatibility global properties were the initial reason for
> > > > the static-properties requirement. We don't really need to allow _all_
> > > > CPU features to be controlled by global properties, only the ones we
> > > > need for machine-type compatibility.
> > > > 
> > > 
> > 
> 

-- 
Eduardo

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

[Andreas Färber]

Am 02.10.2012 17:36, schrieb Igor Mammedov:
> it will allow to use property setters there later.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Don Slutz <Don@CloudSwitch.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> --
> v2:
>     - style change, add braces (reqested by Blue Swirl)
>     - removed unused error_is_set(errp) in properties set loop
> ---
>  target-i386/cpu.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index bb1e44e..e1ffa40 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1097,7 +1097,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;

> @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
>              goto error;
>          }
> +
>          featurestr = strtok(NULL, ",");
>      }
>      x86_cpu_def->features |= plus_features;

Disconnected whitespace change.

> @@ -1282,6 +1284,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);

Note to self: error_set() checks for errp being NULL, so the use of
!error_is_set() logic seems fine.

QERR_ alarm - acceptable use? Or better use error_setg() or something?

Otherwise looks okay to me, I could drop the whitespace hunk myself.

Andreas

> +    }
>      return -1;
>  }
>  
> @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      memset(def, 0, sizeof(*def));
>  
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>      if (def->vendor1) {
>          env->cpuid_vendor1 = def->vendor1;
>          env->cpuid_vendor2 = def->vendor2;
> @@ -1419,6 +1426,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;
> 


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

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

[Andreas Färber]

Am 02.10.2012 17:36, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks, queued on qom-cpu branch:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

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

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

[Andreas Färber]

Am 02.10.2012 17:36, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Acked-by: Andreas Färber <afaerber@suse.de>

Thanks, queued on qom-cpu branch:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

This is a prerequisite for the APIC initialization error reporting.

Andreas

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

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

[Igor Mammedov]

On Wed, 10 Oct 2012 16:05:10 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 02.10.2012 17:36, schrieb Igor Mammedov:
> > it will allow to use property setters there later.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Don Slutz <Don@CloudSwitch.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > --
> > v2:
> >     - style change, add braces (reqested by Blue Swirl)
> >     - removed unused error_is_set(errp) in properties set loop
> > ---
> >  target-i386/cpu.c |   15 ++++++++++++---
> >  1 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index bb1e44e..e1ffa40 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1097,7 +1097,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;
> 
> > @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *cpu_model) fprintf(stderr, "feature string `%s'
> > not in format (+feature|-feature|feature=xyz)\n", featurestr); goto error;
> >          }
> > +
> >          featurestr = strtok(NULL, ",");
> >      }
> >      x86_cpu_def->features |= plus_features;
> 
> Disconnected whitespace change.
> 
> > @@ -1282,6 +1284,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);
> 
> Note to self: error_set() checks for errp being NULL, so the use of
> !error_is_set() logic seems fine.
> 
> QERR_ alarm - acceptable use? Or better use error_setg() or something?
> 
> Otherwise looks okay to me, I could drop the whitespace hunk myself.
I'll fix it for the next re-spin of this series and do whatever is decided
with QERR_...

Thanks!

> 
> Andreas
> 
> > +    }
> >      return -1;
> >  }
> >  
> > @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) 
> >      memset(def, 0, sizeof(*def));
> >  
> > -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> > -        return -1;
> > +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> > +        goto out;
> > +    }
> > +
> >      if (def->vendor1) {
> >          env->cpuid_vendor1 = def->vendor1;
> >          env->cpuid_vendor2 = def->vendor2;
> > @@ -1419,6 +1426,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;
> > 
> 
> 

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

[Luiz Capitulino]

On Wed, 10 Oct 2012 16:38:10 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 10 Oct 2012 16:05:10 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
> > Am 02.10.2012 17:36, schrieb Igor Mammedov:
> > > it will allow to use property setters there later.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Don Slutz <Don@CloudSwitch.com>
> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > --
> > > v2:
> > >     - style change, add braces (reqested by Blue Swirl)
> > >     - removed unused error_is_set(errp) in properties set loop
> > > ---
> > >  target-i386/cpu.c |   15 ++++++++++++---
> > >  1 files changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index bb1e44e..e1ffa40 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1097,7 +1097,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;
> > 
> > > @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t
> > > *x86_cpu_def, const char *cpu_model) fprintf(stderr, "feature string `%s'
> > > not in format (+feature|-feature|feature=xyz)\n", featurestr); goto error;
> > >          }
> > > +
> > >          featurestr = strtok(NULL, ",");
> > >      }
> > >      x86_cpu_def->features |= plus_features;
> > 
> > Disconnected whitespace change.
> > 
> > > @@ -1282,6 +1284,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);
> > 
> > Note to self: error_set() checks for errp being NULL, so the use of
> > !error_is_set() logic seems fine.
> > 
> > QERR_ alarm - acceptable use? Or better use error_setg() or something?
> > 
> > Otherwise looks okay to me, I could drop the whitespace hunk myself.
> I'll fix it for the next re-spin of this series and do whatever is decided
> with QERR_...

It's simpler than you can imagine, you can just do:

error_setg(errp, "invalid parameter combination: %s", params);

> 
> Thanks!
> 
> > 
> > Andreas
> > 
> > > +    }
> > >      return -1;
> > >  }
> > >  
> > > @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char
> > > *cpu_model) 
> > >      memset(def, 0, sizeof(*def));
> > >  
> > > -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> > > -        return -1;
> > > +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> > > +        goto out;
> > > +    }
> > > +
> > >      if (def->vendor1) {
> > >          env->cpuid_vendor1 = def->vendor1;
> > >          env->cpuid_vendor2 = def->vendor2;
> > > @@ -1419,6 +1426,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;
> > > 
> > 
> > 
>