[Qemu-devel] [PATCH 00/20 v3] x86 CPU cleanup (wave 2)

[Qemu-devel] [PATCH 00/20 v3] x86 CPU cleanup (wave 2)

[Igor Mammedov]

This series is several cleanups, moved out from CPU properties series,
since they do not really depend on CPU properties re-factoring and could
simplify CPU subclasses work as well.

Series doesn't depend on cpu as device or any other series, and applies
to current master.

git tree for testing:
    https://github.com/imammedo/qemu/tree/x86_cpu_cleanup.wave2.v3

v3:
 - [07/20] sets error if cpu name is empty, restore return -1 on error
 - get rid of *vendor_override field in CPUX86State & co
 - mark xlevel < 0x80000000 as depricated
 - squash idef-ing kvm specific functions in [08/20]
 - expand comment of [12/20] and reorder it right before "set custom" patches

v2:
 - cleanup commit message  and style fixes in
     [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time
 - extracted more patches [07-20] from cpu properties series, that were
   more cleanups and code reorganizing than conversion to static properties.

Igor Mammedov (20):
  target-i386: filter out not TCG features if running without kvm at
    realize time
  target-i386: sanitize AMD's ext2_features at realize time
  target-i386: explicitly set vendor for each built-in cpudef
  target-i386: setting default 'vendor' is obsolete, remove it
  target-i386: move setting defaults out of cpu_x86_parse_featurestr()
  target-i386: move out CPU features initialization in separate func
  target-i386: cpu_x86_register() consolidate freeing resources
  target-i386: move kvm_check_features_against_host() check to realize
    time
  target-i386: add x86cpu_vendor_words2str()
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: remove vendor_override field from CPUX86State
  target-i386: prepare cpu_x86_parse_featurestr() to return a set of
    key,value property pairs
  target-i386: set custom 'vendor' without intermediate x86_def_t
  target-i386: print depricated warning if xlevel < 0x80000000
  target-i386: set custom 'xlevel' without intermediate x86_def_t
  target-i386: set custom 'level' without intermediate x86_def_t
  target-i386: set custom 'model-id' without intermediate x86_def_t
  target-i386: set custom 'stepping' without intermediate x86_def_t
  target-i386: set custom 'model' without intermediate x86_def_t
  target-i386: set custom 'family' without intermediate x86_def_t

 target-i386/cpu.c |  377 +++++++++++++++++++++++++----------------------------
 target-i386/cpu.h |    7 +-
 2 files changed, 178 insertions(+), 206 deletions(-)

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

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7be3ad8..63aae86 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1549,21 +1549,6 @@ 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;
-    } else {
-#ifdef CONFIG_KVM
-        filter_features_for_kvm(cpu);
-#endif
-    }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
@@ -2077,6 +2062,22 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_level = 7;
     }
 
+    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;
+    } else {
+#ifdef CONFIG_KVM
+        filter_features_for_kvm(cpu);
+#endif
+    }
+
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
-- 
1.7.1

[Qemu-devel] [PATCH 02/20] target-i386: sanitize AMD's ext2_features at realize time

[Igor Mammedov]

when CPU properties are implemented, ext2_features may change
between object_new(CPU) and cpu_realize_fn(). Sanitizing
ext2_features for AMD based CPU at realize() time will keep
current behavior after CPU features are converted to properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
  - style fix, make line shorter than 80 characters
---
 target-i386/cpu.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 63aae86..64b7637 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     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);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
@@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_level = 7;
     }
 
+    /* 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 |= (env->cpuid_features
+           &  CPUID_EXT2_AMD_ALIASES);
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
-- 
1.7.1

[Qemu-devel] [PATCH 03/20] target-i386: explicitly set vendor for each built-in cpudef

[Igor Mammedov]

it will help to get rid of setting default.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 64b7637..1497980 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -387,6 +387,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "core2duo",
         .level = 10,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 15,
         .stepping = 11,
@@ -431,6 +434,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu32",
         .level = 4,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -441,6 +447,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm32",
         .level = 5,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -455,6 +464,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "coreduo",
         .level = 10,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 14,
         .stepping = 8,
@@ -470,6 +482,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "486",
         .level = 1,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 4,
         .model = 0,
         .stepping = 0,
@@ -479,6 +494,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium",
         .level = 1,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 5,
         .model = 4,
         .stepping = 3,
@@ -488,6 +506,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium2",
         .level = 2,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 5,
         .stepping = 2,
@@ -497,6 +518,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium3",
         .level = 2,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 7,
         .stepping = 3,
@@ -522,6 +546,9 @@ static x86_def_t builtin_x86_defs[] = {
         .name = "n270",
         /* original is on level 10 */
         .level = 5,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 28,
         .stepping = 2,
-- 
1.7.1

[Qemu-devel] [PATCH 04/20] target-i386: setting default 'vendor' is obsolete, remove it

[Igor Mammedov]

since cpu_def config is not supported anymore and all remaining sources now
always set x86_def_t.vendor[123] fields remove setting default vendor to
simplify future re-factoring.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1497980..99fd3f3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,15 +1539,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-    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;
-    }
+    assert(def->vendor1);
+    env->cpuid_vendor1 = def->vendor1;
+    env->cpuid_vendor2 = def->vendor2;
+    env->cpuid_vendor3 = def->vendor3;
     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);
-- 
1.7.1

[Qemu-devel] [PATCH 05/20] target-i386: move setting defaults out of cpu_x86_parse_featurestr()

[Igor Mammedov]

No functional change, needed for simplifying conversion to properties.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 99fd3f3..e534254 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1264,7 +1264,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     /* 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 = kvm_default_features, plus_svm_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;
@@ -1273,10 +1273,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     uint32_t minus_7_0_ebx_features = 0;
     uint32_t numvalue;
 
-    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 = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
@@ -1536,6 +1532,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto error;
     }
 
+    def->kvm_features |= kvm_default_features;
+    add_flagname_to_bitmaps("hypervisor", &def->features,
+                            &def->ext_features, &def->ext2_features,
+                            &def->ext3_features, &def->kvm_features,
+                            &def->svm_features, &def->cpuid_7_0_ebx_features);
+
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-- 
1.7.1

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

[Igor Mammedov]

No functional change, a simple code movement to simplify following refactoring.

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
 v3:
  - rebased on top of 1.3 & split cpu_x86_find_by_name()
  - AMD's ext2_features filtering is moved into separate patch
---
 target-i386/cpu.c |   53 ++++++++++++++++++++++++++++++-----------------------
 1 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e534254..3b9bbfe 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1235,6 +1235,34 @@ 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;
+
+    assert(def->vendor1);
+    env->cpuid_vendor1 = def->vendor1;
+    env->cpuid_vendor2 = def->vendor2;
+    env->cpuid_vendor3 = def->vendor3;
+    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);
+    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", errp);
+    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", errp);
+    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+}
+
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
     x86_def_t *def;
@@ -1513,7 +1541,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
 
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
-    CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
     char *name, *features;
@@ -1541,29 +1568,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-    assert(def->vendor1);
-    env->cpuid_vendor1 = def->vendor1;
-    env->cpuid_vendor2 = def->vendor2;
-    env->cpuid_vendor3 = def->vendor3;
-    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);
 
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    cpudef_2_x86_cpu(cpu, def, &error);
+
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-- 
1.7.1

[Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

freeing resources in one place would require setting 'error'
to not NULL, so add some more error reporting before jumping to
exit branch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
   - add missing 'return -1' on exit if error is not NULL,
           Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v3:
   - set error if cpu_model is empty
---
 target-i386/cpu.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..97cce89 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,13 +1550,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        goto error;
+        error_setg(&error, "Invalid/empty CPU model name");
+        goto out;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
     }
 
     def->kvm_features |= kvm_default_features;
@@ -1566,22 +1568,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
+out:
+    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-        goto error;
+        return -1;
     }
-
-    g_strfreev(model_pieces);
     return 0;
-error:
-    g_strfreev(model_pieces);
-    return -1;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.1

[Qemu-devel] [PATCH 08/20] target-i386: move kvm_check_features_against_host() check to realize time

[Igor Mammedov]

kvm_check_features_against_host() should be called when features can't be changed
and when features are convereted to properties it would be possible to change them
until realize time, so correct way is to call kvm_check_features_against_host() in
x86_cpu_realize()

 - calling kvm_check_features_against_host() makes sense only when qemu is compiled
   --enable-kvm, so move it inside CONFIG_KVM ifdef and compile it and
   other kvm specific functions only if CONFIG_KVM is defined to avoid *-user target
   build breakage.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v2:
   - squash in ifdef-ing kvm specific functions into this patch
---
 target-i386/cpu.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 97cce89..d93bf5a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -862,7 +862,6 @@ static int cpu_x86_fill_model_id(char *str)
     }
     return 0;
 }
-#endif
 
 /* Fill a x86_def_t struct with information about the host CPU, and
  * the CPU features supported by the host hardware + host kernel
@@ -871,7 +870,6 @@ static int cpu_x86_fill_model_id(char *str)
  */
 static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 {
-#ifdef CONFIG_KVM
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
@@ -930,7 +928,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
      * unsupported ones later.
      */
     x86_cpu_def->svm_features = -1;
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
@@ -954,19 +951,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
  *
  * This function may be called only if KVM is enabled.
  */
-static int kvm_check_features_against_host(x86_def_t *guest_def)
+static int kvm_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}};
 
     assert(kvm_enabled());
@@ -981,6 +979,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
                 }
     return rv;
 }
+#endif /* CONFIG_KVM */
 
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
@@ -1273,7 +1272,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
         }
     }
     if (kvm_enabled() && name && strcmp(name, "host") == 0) {
+#ifdef CONFIG_KVM
         kvm_cpu_fill_host(x86_cpu_def);
+#endif
     } else if (!def) {
         return -1;
     } else {
@@ -1428,10 +1429,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
-    if (check_cpuid && kvm_enabled()) {
-        if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
-            goto error;
-    }
     return 0;
 
 error:
@@ -2106,6 +2103,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     } else {
 #ifdef CONFIG_KVM
+        if (check_cpuid && kvm_check_features_against_host(cpu)
+            && enforce_cpuid) {
+            error_setg(errp, "Host's CPU doesn't support requested features");
+            return;
+        }
+
         filter_features_for_kvm(cpu);
 #endif
     }
-- 
1.7.1

[Qemu-devel] [PATCH 09/20] target-i386: add x86cpu_vendor_words2str()

[Igor Mammedov]

Make for() cycle reusable for the next patch

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d93bf5a..935bc42 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -847,6 +847,18 @@ static x86_def_t builtin_x86_defs[] = {
     },
 };
 
+static void x86cpu_vendor_words2str(char *dst, uint32_t ebx, uint32_t ecx,
+                                    uint32_t edx)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = ebx >> (8 * i);
+        dst[i + 4] = edx >> (8 * i);
+        dst[i + 8] = ecx >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 #ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
@@ -1130,15 +1142,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     char *value;
-    int i;
 
     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[CPUID_VENDOR_SZ] = '\0';
+    x86cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
+                            env->cpuid_vendor3);
     return value;
 }
 
-- 
1.7.1

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

[Igor Mammedov]

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

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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |  127 ++++++++++++----------------------------------------
 target-i386/cpu.h |    6 +-
 2 files changed, 33 insertions(+), 100 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 935bc42..6cce311 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -275,7 +275,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;
@@ -340,9 +340,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,
@@ -359,9 +357,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,
@@ -387,9 +383,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "core2duo",
         .level = 10,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 15,
         .stepping = 11,
@@ -408,9 +402,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,
@@ -434,9 +426,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu32",
         .level = 4,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -447,9 +437,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm32",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -464,9 +452,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "coreduo",
         .level = 10,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 14,
         .stepping = 8,
@@ -482,9 +468,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "486",
         .level = 1,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 4,
         .model = 0,
         .stepping = 0,
@@ -494,9 +478,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium",
         .level = 1,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 5,
         .model = 4,
         .stepping = 3,
@@ -506,9 +488,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium2",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 5,
         .stepping = 2,
@@ -518,9 +498,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium3",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 7,
         .stepping = 3,
@@ -530,9 +508,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,
@@ -546,9 +522,7 @@ static x86_def_t builtin_x86_defs[] = {
         .name = "n270",
         /* original is on level 10 */
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 28,
         .stepping = 2,
@@ -567,9 +541,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,
@@ -587,9 +559,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,
@@ -608,9 +578,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,
@@ -629,9 +597,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,
@@ -651,9 +617,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,
@@ -676,9 +640,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Haswell",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 60,
         .stepping = 1,
@@ -706,9 +668,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,
@@ -730,9 +690,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,
@@ -756,9 +714,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,
@@ -784,9 +740,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,
@@ -816,9 +770,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G5",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 2,
         .stepping = 0,
@@ -888,10 +840,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     assert(kvm_enabled());
 
     x86_cpu_def->name = "host";
-    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_def->vendor1 = ebx;
-    x86_cpu_def->vendor2 = edx;
-    x86_cpu_def->vendor3 = ecx;
+    x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
 
     host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
@@ -919,9 +868,7 @@ static void kvm_cpu_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);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
@@ -1245,10 +1192,8 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
 
-    assert(def->vendor1);
-    env->cpuid_vendor1 = def->vendor1;
-    env->cpuid_vendor2 = def->vendor2;
-    env->cpuid_vendor3 = def->vendor3;
+    assert(def->vendor[0]);
+    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     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);
@@ -1295,7 +1240,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
  */
 static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
 {
-    unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
     uint32_t plus_features = 0, plus_ext_features = 0;
@@ -1369,18 +1313,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
                 }
                 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 386c4f6..fbbe730 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -515,14 +515,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 11/20] target-i386: remove vendor_override field from CPUX86State

[Igor Mammedov]

commit 8935499831312 makes cpuid return to guest host's vendor value
instead of built-in one by default if kvm_enabled() == true and allows
to override this behavior if 'vendor' is specified on -cpu command line.

But every time guest calls cpuid to get 'vendor' value, host's value is
read again and again in default case.

It complicates semantic of vendor property and makes it harder to use.

Instead of reading 'vendor' value from host every time cpuid[vendor] is
called, override 'vendor' value only once in cpu_x86_find_by_name(), when
built-in CPU model is found and if(kvm_enabled() == true).

It provides the same default semantic
 if (kvm_enabled() == true)  vendor = host's vendor
 else vendor = built-in vendor

and then later:
 if (custom vendor) vendor = custom vendor

'vendor' value is overridden when user provides it on -cpu command line,
and there isn't need in vendor_override field anymore, remove it.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6cce311..c223599 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -284,7 +284,6 @@ typedef struct x86_def_t {
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
-    int vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -865,7 +864,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
                 kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
-    x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
     if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
@@ -1117,7 +1115,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)
@@ -1194,7 +1191,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 
     assert(def->vendor[0]);
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
-    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);
@@ -1231,6 +1227,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
         return -1;
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
+        /* sysenter isn't supported on compatibility mode on AMD, syscall
+         * isn't supported in compatibility mode on Intel.
+         * Normally we advertise the actual cpu vendor, but you can override
+         * this using the 'vendor' property if you want to use KVM's
+         * sysenter/syscall emulation in compatibility mode and when doing
+         * cross vendor migration
+         */
+        if (kvm_enabled()) {
+            uint32_t  ebx = 0, ecx = 0, edx = 0;
+            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+            x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
+        }
     }
 
     return 0;
@@ -1314,7 +1322,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
                 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);
@@ -1563,16 +1570,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
     *ebx = env->cpuid_vendor1;
     *edx = env->cpuid_vendor2;
     *ecx = env->cpuid_vendor3;
-
-    /* sysenter isn't supported on compatibility mode on AMD, syscall
-     * isn't supported in compatibility mode on Intel.
-     * Normally we advertise the actual cpu vendor, but you can override
-     * this if you want to use KVM's sysenter/syscall emulation
-     * in compatibility mode and when doing cross vendor migration
-     */
-    if (kvm_enabled() && ! env->cpuid_vendor_override) {
-        host_cpuid(0, 0, NULL, ebx, ecx, edx);
-    }
 }
 
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fbbe730..a15a09e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -812,7 +812,6 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int 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 12/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov]

It prepares for converting "+feature,-feature,feature=foo,feature" into
a set of key,value property pairs that will be applied to CPU by
cpu_x86_set_props().

It separates legacy format parsing from property setting, later it could be
transformed into code that sets global properties for a given CPU type.

Each feature handled by cpu_x86_parse_featurestr() will be converted into
foo,val pair and a corresponding property setter by following patches.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c223599..3ead1f4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1244,9 +1244,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
     return 0;
 }
 
+/* Set features on X86CPU object based on a provide key,value list */
+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;
+        }
+    }
+}
+
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
+static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
+                                    QDict **props)
 {
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
@@ -1260,10 +1276,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     uint32_t minus_kvm_features = 0, minus_svm_features = 0;
     uint32_t minus_7_0_ebx_features = 0;
     uint32_t numvalue;
+    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
+    *props = qdict_new();
+    int j = 0;
 
-    featurestr = features ? strtok(features, ",") : NULL;
-
-    while (featurestr) {
+    while ((featurestr = feat_array[j++])) {
         char *val;
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, &plus_features,
@@ -1360,7 +1377,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
             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;
     x86_cpu_def->ext_features |= plus_ext_features;
@@ -1376,9 +1392,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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_strfreev(feat_array);
     return 0;
 
 error:
+    g_strfreev(feat_array);
     return -1;
 }
 
@@ -1486,6 +1504,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
+    QDict *props = NULL;
     Error *error = NULL;
     char *name, *features;
     gchar **model_pieces;
@@ -1511,14 +1530,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->ext3_features, &def->kvm_features,
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
-    if (cpu_x86_parse_featurestr(def, features) < 0) {
+    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
         error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
         goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
+    cpu_x86_set_props(cpu, props, &error);
 
 out:
+    QDECREF(props);
     g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
-- 
1.7.1

[Qemu-devel] [PATCH 13/20] target-i386: set custom 'vendor' without intermediate x86_def_t

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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 3ead1f4..084faeb 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1338,7 +1338,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
                         val);
-- 
1.7.1

[Qemu-devel] [PATCH 14/20] target-i386: print depricated warning if xlevel < 0x80000000

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 084faeb..0764015 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1334,6 +1334,8 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                     goto error;
                 }
                 if (numvalue < 0x80000000) {
+                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
+                            ", fixup will be depricated in future versions\n");
                     numvalue += 0x80000000;
                 }
                 x86_cpu_def->xlevel = numvalue;
-- 
1.7.1

[Qemu-devel] [PATCH 15/20] target-i386: set custom 'xlevel' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0764015..d8af528 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1338,7 +1338,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                             ", fixup will be depricated in future versions\n");
                     numvalue += 0x80000000;
                 }
-                x86_cpu_def->xlevel = numvalue;
+                val = g_strdup_printf("%u", numvalue);
+                qdict_put(*props, featurestr, qstring_from_str(val));
+                g_free(val);
             } else if (!strcmp(featurestr, "vendor")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "model_id")) {
-- 
1.7.1

[Qemu-devel] [PATCH 16/20] target-i386: set custom 'level' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d8af528..1cb2f08 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1319,13 +1319,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 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;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "xlevel")) {
                 char *err;
                 numvalue = strtoul(val, &err, 0);
-- 
1.7.1

[Qemu-devel] [PATCH 17/20] target-i386: set custom 'model-id' without intermediate x86_def_t

[Igor Mammedov]

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 1cb2f08..aab5f6f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1338,8 +1338,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
             } else if (!strcmp(featurestr, "vendor")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
+                qdict_put(*props, "model-id", qstring_from_str(val));
             } else if (!strcmp(featurestr, "tsc_freq")) {
                 int64_t tsc_freq;
                 char *err;
-- 
1.7.1

[Qemu-devel] [PATCH 18/20] target-i386: set custom 'stepping' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index aab5f6f..3ee4c65 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1311,13 +1311,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 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 ;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "level")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "xlevel")) {
-- 
1.7.1

[Qemu-devel] [PATCH 19/20] target-i386: set custom 'model' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3ee4c65..8c65012 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1303,13 +1303,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 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;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "stepping")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "level")) {
-- 
1.7.1

[Qemu-devel] [PATCH 20/20] target-i386: set custom 'family' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8c65012..fd53b41 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1295,13 +1295,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "model")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "stepping")) {
-- 
1.7.1

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

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:17PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Patch is OK, but Subject line needs to be fixed. You are moving the
filtering for both KVM and non-KVM cases, not just the TCG filtering.

> ---
>  target-i386/cpu.c |   31 ++++++++++++++++---------------
>  1 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7be3ad8..63aae86 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1549,21 +1549,6 @@ 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;
> -    } else {
> -#ifdef CONFIG_KVM
> -        filter_features_for_kvm(cpu);
> -#endif
> -    }
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> @@ -2077,6 +2062,22 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_level = 7;
>      }
>  
> +    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;
> +    } else {
> +#ifdef CONFIG_KVM
> +        filter_features_for_kvm(cpu);
> +#endif
> +    }
> +
>  #ifndef CONFIG_USER_ONLY
>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>  
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 02/20] target-i386: sanitize AMD's ext2_features at realize time

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:18PM +0100, Igor Mammedov wrote:
> when CPU properties are implemented, ext2_features may change
> between object_new(CPU) and cpu_realize_fn(). Sanitizing
> ext2_features for AMD based CPU at realize() time will keep
> current behavior after CPU features are converted to properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission is OK to me:

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


> ---
> v2:
>   - style fix, make line shorter than 80 characters
> ---
>  target-i386/cpu.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 63aae86..64b7637 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      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);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_level = 7;
>      }
>  
> +    /* 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 |= (env->cpuid_features
> +           &  CPUID_EXT2_AMD_ALIASES);
> +    }
> +
>      if (!kvm_enabled()) {
>          env->cpuid_features &= TCG_FEATURES;
>          env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/20] target-i386: explicitly set vendor for each built-in cpudef

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:19PM +0100, Igor Mammedov wrote:
> it will help to get rid of setting default.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission is OK to me:
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  target-i386/cpu.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 64b7637..1497980 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -387,6 +387,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "core2duo",
>          .level = 10,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 15,
>          .stepping = 11,
> @@ -431,6 +434,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "qemu32",
>          .level = 4,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 3,
>          .stepping = 3,
> @@ -441,6 +447,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "kvm32",
>          .level = 5,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -455,6 +464,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "coreduo",
>          .level = 10,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 14,
>          .stepping = 8,
> @@ -470,6 +482,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "486",
>          .level = 1,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 4,
>          .model = 0,
>          .stepping = 0,
> @@ -479,6 +494,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium",
>          .level = 1,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 5,
>          .model = 4,
>          .stepping = 3,
> @@ -488,6 +506,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium2",
>          .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 5,
>          .stepping = 2,
> @@ -497,6 +518,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium3",
>          .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 7,
>          .stepping = 3,
> @@ -522,6 +546,9 @@ static x86_def_t builtin_x86_defs[] = {
>          .name = "n270",
>          /* original is on level 10 */
>          .level = 5,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 28,
>          .stepping = 2,
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/20] target-i386: setting default 'vendor' is obsolete, remove it

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:20PM +0100, Igor Mammedov wrote:
> since cpu_def config is not supported anymore and all remaining sources now
> always set x86_def_t.vendor[123] fields remove setting default vendor to
> simplify future re-factoring.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission is OK to me:

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


> ---
>  target-i386/cpu.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1497980..99fd3f3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1539,15 +1539,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -    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;
> -    }
> +    assert(def->vendor1);
> +    env->cpuid_vendor1 = def->vendor1;
> +    env->cpuid_vendor2 = def->vendor2;
> +    env->cpuid_vendor3 = def->vendor3;
>      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);
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 05/20] target-i386: move setting defaults out of cpu_x86_parse_featurestr()

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:21PM +0100, Igor Mammedov wrote:
> No functional change, needed for simplifying conversion to properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission is OK to me:

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


> ---
>  target-i386/cpu.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 99fd3f3..e534254 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1264,7 +1264,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      /* 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 = kvm_default_features, plus_svm_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;
> @@ -1273,10 +1273,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      uint32_t minus_7_0_ebx_features = 0;
>      uint32_t numvalue;
>  
> -    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 = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
> @@ -1536,6 +1532,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto error;
>      }
>  
> +    def->kvm_features |= kvm_default_features;
> +    add_flagname_to_bitmaps("hypervisor", &def->features,
> +                            &def->ext_features, &def->ext2_features,
> +                            &def->ext3_features, &def->kvm_features,
> +                            &def->svm_features, &def->cpuid_7_0_ebx_features);
> +
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -- 
> 1.7.1
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:22PM +0100, Igor Mammedov wrote:
> No functional change, a simple code movement to simplify following refactoring.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission is OK to me:

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
>  v3:
>   - rebased on top of 1.3 & split cpu_x86_find_by_name()
>   - AMD's ext2_features filtering is moved into separate patch
> ---
>  target-i386/cpu.c |   53 ++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e534254..3b9bbfe 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1235,6 +1235,34 @@ 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;
> +
> +    assert(def->vendor1);
> +    env->cpuid_vendor1 = def->vendor1;
> +    env->cpuid_vendor2 = def->vendor2;
> +    env->cpuid_vendor3 = def->vendor3;
> +    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);
> +    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", errp);
> +    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", errp);
> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
> +}
> +
>  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
>      x86_def_t *def;
> @@ -1513,7 +1541,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
> -    CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>      char *name, *features;
> @@ -1541,29 +1568,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -    assert(def->vendor1);
> -    env->cpuid_vendor1 = def->vendor1;
> -    env->cpuid_vendor2 = def->vendor2;
> -    env->cpuid_vendor3 = def->vendor3;
> -    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);
>  
> -    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +    cpudef_2_x86_cpu(cpu, def, &error);
> +
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:23PM +0100, Igor Mammedov wrote:
> freeing resources in one place would require setting 'error'
> to not NULL, so add some more error reporting before jumping to
> exit branch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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


> ---
>   v2:
>    - add missing 'return -1' on exit if error is not NULL,
>            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
>   v3:
>    - set error if cpu_model is empty
> ---
>  target-i386/cpu.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3b9bbfe..97cce89 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1550,13 +1550,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> -        goto error;
> +        error_setg(&error, "Invalid/empty CPU model name");
> +        goto out;
>      }
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
>      if (cpu_x86_find_by_name(def, name) < 0) {
> -        goto error;
> +        error_setg(&error, "Unable to find CPU definition: %s", name);
> +        goto out;
>      }
>  
>      def->kvm_features |= kvm_default_features;
> @@ -1566,22 +1568,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> -        goto error;
> +        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> +        goto out;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
>  
> +out:
> +    g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -        goto error;
> +        return -1;
>      }
> -
> -    g_strfreev(model_pieces);
>      return 0;
> -error:
> -    g_strfreev(model_pieces);
> -    return -1;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/20] target-i386: move kvm_check_features_against_host() check to realize time

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:24PM +0100, Igor Mammedov wrote:
> kvm_check_features_against_host() should be called when features can't be changed
> and when features are convereted to properties it would be possible to change them
> until realize time, so correct way is to call kvm_check_features_against_host() in
> x86_cpu_realize()
> 
>  - calling kvm_check_features_against_host() makes sense only when qemu is compiled
>    --enable-kvm, so move it inside CONFIG_KVM ifdef and compile it and
>    other kvm specific functions only if CONFIG_KVM is defined to avoid *-user target
>    build breakage.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Commit message has more than 80 columns.

But considering that the code changes look OK:

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

We can probably #ifdef-out the whole host_cpuid() function as well. But
that can be done in a separate patch, as it requires more testing to
make sure it won't cause unexpected build errors on some configurations
(as the function is not static).


> ---
>  v2:
>    - squash in ifdef-ing kvm specific functions into this patch
> ---
>  target-i386/cpu.c |   27 +++++++++++++++------------
>  1 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 97cce89..d93bf5a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -862,7 +862,6 @@ static int cpu_x86_fill_model_id(char *str)
>      }
>      return 0;
>  }
> -#endif
>  
>  /* Fill a x86_def_t struct with information about the host CPU, and
>   * the CPU features supported by the host hardware + host kernel
> @@ -871,7 +870,6 @@ static int cpu_x86_fill_model_id(char *str)
>   */
>  static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>  {
> -#ifdef CONFIG_KVM
>      KVMState *s = kvm_state;
>      uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>  
> @@ -930,7 +928,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>       * unsupported ones later.
>       */
>      x86_cpu_def->svm_features = -1;
> -#endif /* CONFIG_KVM */
>  }
>  
>  static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
> @@ -954,19 +951,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
>   *
>   * This function may be called only if KVM is enabled.
>   */
> -static int kvm_check_features_against_host(x86_def_t *guest_def)
> +static int kvm_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}};
>  
>      assert(kvm_enabled());
> @@ -981,6 +979,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
>                  }
>      return rv;
>  }
> +#endif /* CONFIG_KVM */
>  
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
> @@ -1273,7 +1272,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>          }
>      }
>      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> +#ifdef CONFIG_KVM
>          kvm_cpu_fill_host(x86_cpu_def);
> +#endif
>      } else if (!def) {
>          return -1;
>      } else {
> @@ -1428,10 +1429,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
> -    if (check_cpuid && kvm_enabled()) {
> -        if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
> -            goto error;
> -    }
>      return 0;
>  
>  error:
> @@ -2106,6 +2103,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_svm_features &= TCG_SVM_FEATURES;
>      } else {
>  #ifdef CONFIG_KVM
> +        if (check_cpuid && kvm_check_features_against_host(cpu)
> +            && enforce_cpuid) {
> +            error_setg(errp, "Host's CPU doesn't support requested features");
> +            return;
> +        }
> +
>          filter_features_for_kvm(cpu);
>  #endif
>      }
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 09/20] target-i386: add x86cpu_vendor_words2str()

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:25PM +0100, Igor Mammedov wrote:
> Make for() cycle reusable for the next patch
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission looks OK to me:

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

> ---
>  target-i386/cpu.c |   21 ++++++++++++++-------
>  1 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d93bf5a..935bc42 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -847,6 +847,18 @@ static x86_def_t builtin_x86_defs[] = {
>      },
>  };
>  
> +static void x86cpu_vendor_words2str(char *dst, uint32_t ebx, uint32_t ecx,
> +                                    uint32_t edx)
> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = ebx >> (8 * i);
> +        dst[i + 4] = edx >> (8 * i);
> +        dst[i + 8] = ecx >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}
> +
>  #ifdef CONFIG_KVM
>  static int cpu_x86_fill_model_id(char *str)
>  {
> @@ -1130,15 +1142,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
>      char *value;
> -    int i;
>  
>      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[CPUID_VENDOR_SZ] = '\0';
> +    x86cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
> +                            env->cpuid_vendor3);
>      return value;
>  }
>  
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:26PM +0100, Igor Mammedov wrote:
> 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>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this specific submission looks OK to me:

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

> ---
>  target-i386/cpu.c |  127 ++++++++++++----------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 33 insertions(+), 100 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 935bc42..6cce311 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -275,7 +275,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;
> @@ -340,9 +340,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,
> @@ -359,9 +357,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,
> @@ -387,9 +383,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "core2duo",
>          .level = 10,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 15,
>          .stepping = 11,
> @@ -408,9 +402,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,
> @@ -434,9 +426,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "qemu32",
>          .level = 4,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 3,
>          .stepping = 3,
> @@ -447,9 +437,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "kvm32",
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -464,9 +452,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "coreduo",
>          .level = 10,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 14,
>          .stepping = 8,
> @@ -482,9 +468,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "486",
>          .level = 1,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 4,
>          .model = 0,
>          .stepping = 0,
> @@ -494,9 +478,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium",
>          .level = 1,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 5,
>          .model = 4,
>          .stepping = 3,
> @@ -506,9 +488,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium2",
>          .level = 2,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 5,
>          .stepping = 2,
> @@ -518,9 +498,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium3",
>          .level = 2,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 7,
>          .stepping = 3,
> @@ -530,9 +508,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,
> @@ -546,9 +522,7 @@ static x86_def_t builtin_x86_defs[] = {
>          .name = "n270",
>          /* original is on level 10 */
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 28,
>          .stepping = 2,
> @@ -567,9 +541,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,
> @@ -587,9 +559,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,
> @@ -608,9 +578,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,
> @@ -629,9 +597,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,
> @@ -651,9 +617,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,
> @@ -676,9 +640,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Haswell",
>          .level = 0xd,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 60,
>          .stepping = 1,
> @@ -706,9 +668,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,
> @@ -730,9 +690,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,
> @@ -756,9 +714,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,
> @@ -784,9 +740,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,
> @@ -816,9 +770,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Opteron_G5",
>          .level = 0xd,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 21,
>          .model = 2,
>          .stepping = 0,
> @@ -888,10 +840,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      assert(kvm_enabled());
>  
>      x86_cpu_def->name = "host";
> -    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> -    x86_cpu_def->vendor1 = ebx;
> -    x86_cpu_def->vendor2 = edx;
> -    x86_cpu_def->vendor3 = ecx;
> +    x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
>  
>      host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
>      x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> @@ -919,9 +868,7 @@ static void kvm_cpu_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);
>          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>          if (eax >= 0xC0000001) {
> @@ -1245,10 +1192,8 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>  {
>      CPUX86State *env = &cpu->env;
>  
> -    assert(def->vendor1);
> -    env->cpuid_vendor1 = def->vendor1;
> -    env->cpuid_vendor2 = def->vendor2;
> -    env->cpuid_vendor3 = def->vendor3;
> +    assert(def->vendor[0]);
> +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
>      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);
> @@ -1295,7 +1240,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>   */
>  static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>  {
> -    unsigned int i;
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
>      uint32_t plus_features = 0, plus_ext_features = 0;
> @@ -1369,18 +1313,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>                  }
>                  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 386c4f6..fbbe730 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -515,14 +515,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
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 11/20] target-i386: remove vendor_override field from CPUX86State

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:27PM +0100, Igor Mammedov wrote:
> commit 8935499831312 makes cpuid return to guest host's vendor value
> instead of built-in one by default if kvm_enabled() == true and allows
> to override this behavior if 'vendor' is specified on -cpu command line.
> 
> But every time guest calls cpuid to get 'vendor' value, host's value is
> read again and again in default case.
> 
> It complicates semantic of vendor property and makes it harder to use.
> 
> Instead of reading 'vendor' value from host every time cpuid[vendor] is
> called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> built-in CPU model is found and if(kvm_enabled() == true).
> 
> It provides the same default semantic
>  if (kvm_enabled() == true)  vendor = host's vendor
>  else vendor = built-in vendor
> 
> and then later:
>  if (custom vendor) vendor = custom vendor
> 
> 'vendor' value is overridden when user provides it on -cpu command line,
> and there isn't need in vendor_override field anymore, remove it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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


> ---
>  target-i386/cpu.c |   27 ++++++++++++---------------
>  target-i386/cpu.h |    1 -
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 6cce311..c223599 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -284,7 +284,6 @@ typedef struct x86_def_t {
>      uint32_t kvm_features, svm_features;
>      uint32_t xlevel;
>      char model_id[48];
> -    int vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;
> @@ -865,7 +864,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>                  kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
>  
>      cpu_x86_fill_model_id(x86_cpu_def->model_id);
> -    x86_cpu_def->vendor_override = 0;
>  
>      /* Call Centaur's CPUID instruction. */
>      if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> @@ -1117,7 +1115,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)
> @@ -1194,7 +1191,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>  
>      assert(def->vendor[0]);
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
> -    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);
> @@ -1231,6 +1227,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>          return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));
> +        /* sysenter isn't supported on compatibility mode on AMD, syscall
> +         * isn't supported in compatibility mode on Intel.
> +         * Normally we advertise the actual cpu vendor, but you can override
> +         * this using the 'vendor' property if you want to use KVM's
> +         * sysenter/syscall emulation in compatibility mode and when doing
> +         * cross vendor migration
> +         */
> +        if (kvm_enabled()) {
> +            uint32_t  ebx = 0, ecx = 0, edx = 0;
> +            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> +            x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> +        }
>      }
>  
>      return 0;
> @@ -1314,7 +1322,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>                  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);
> @@ -1563,16 +1570,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>      *ebx = env->cpuid_vendor1;
>      *edx = env->cpuid_vendor2;
>      *ecx = env->cpuid_vendor3;
> -
> -    /* sysenter isn't supported on compatibility mode on AMD, syscall
> -     * isn't supported in compatibility mode on Intel.
> -     * Normally we advertise the actual cpu vendor, but you can override
> -     * this if you want to use KVM's sysenter/syscall emulation
> -     * in compatibility mode and when doing cross vendor migration
> -     */
> -    if (kvm_enabled() && ! env->cpuid_vendor_override) {
> -        host_cpuid(0, 0, NULL, ebx, ecx, edx);
> -    }
>  }
>  
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index fbbe730..a15a09e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -812,7 +812,6 @@ typedef struct CPUX86State {
>      uint32_t cpuid_ext2_features;
>      uint32_t cpuid_ext3_features;
>      uint32_t cpuid_apic_id;
> -    int cpuid_vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t cpuid_xlevel2;
>      uint32_t cpuid_ext4_features;
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 14/20] target-i386: print depricated warning if xlevel < 0x80000000

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:30PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 084faeb..0764015 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1334,6 +1334,8 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                      goto error;
>                  }
>                  if (numvalue < 0x80000000) {
> +                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
> +                            ", fixup will be depricated in future versions\n");

Did you mean: "deprecated"?  :-)

>                      numvalue += 0x80000000;
>                  }
>                  x86_cpu_def->xlevel = numvalue;
> -- 
> 1.7.1
> 

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 27 Dec 2012 16:41:47 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Dec 27, 2012 at 03:59:17PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Patch is OK, but Subject line needs to be fixed. You are moving the
> filtering for both KVM and non-KVM cases, not just the TCG filtering.
would be "target-i386: filter out features at realize time" acceptable?

> 
> > ---
> >  target-i386/cpu.c |   31 ++++++++++++++++---------------
> >  1 files changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7be3ad8..63aae86 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1549,21 +1549,6 @@ 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;
> > -    } else {
> > -#ifdef CONFIG_KVM
> > -        filter_features_for_kvm(cpu);
> > -#endif
> > -    }
> >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> >      if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > @@ -2077,6 +2062,22 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >          env->cpuid_level = 7;
> >      }
> >  
> > +    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;
> > +    } else {
> > +#ifdef CONFIG_KVM
> > +        filter_features_for_kvm(cpu);
> > +#endif
> > +    }
> > +
> >  #ifndef CONFIG_USER_ONLY
> >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> >  
> > -- 
> > 1.7.1
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 12/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:28PM +0100, Igor Mammedov wrote:
> It prepares for converting "+feature,-feature,feature=foo,feature" into
> a set of key,value property pairs that will be applied to CPU by
> cpu_x86_set_props().
> 
> It separates legacy format parsing from property setting, later it could be
> transformed into code that sets global properties for a given CPU type.
> 
> Each feature handled by cpu_x86_parse_featurestr() will be converted into
> foo,val pair and a corresponding property setter by following patches.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

As now I was convinced that the options will eventually become global
properties, and as global properties are all based on key/value string
pairs:

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


> ---
>  target-i386/cpu.c |   33 +++++++++++++++++++++++++++------
>  1 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c223599..3ead1f4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1244,9 +1244,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>      return 0;
>  }
>  
> +/* Set features on X86CPU object based on a provide key,value list */
> +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;
> +        }
> +    }
> +}
> +
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> +                                    QDict **props)
>  {
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
> @@ -1260,10 +1276,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      uint32_t minus_kvm_features = 0, minus_svm_features = 0;
>      uint32_t minus_7_0_ebx_features = 0;
>      uint32_t numvalue;
> +    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
> +    *props = qdict_new();
> +    int j = 0;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> -
> -    while (featurestr) {
> +    while ((featurestr = feat_array[j++])) {
>          char *val;
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> @@ -1360,7 +1377,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>              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;
>      x86_cpu_def->ext_features |= plus_ext_features;
> @@ -1376,9 +1392,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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_strfreev(feat_array);
>      return 0;
>  
>  error:
> +    g_strfreev(feat_array);
>      return -1;
>  }
>  
> @@ -1486,6 +1504,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
>      x86_def_t def1, *def = &def1;
> +    QDict *props = NULL;
>      Error *error = NULL;
>      char *name, *features;
>      gchar **model_pieces;
> @@ -1511,14 +1530,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              &def->ext3_features, &def->kvm_features,
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
> -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> +    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
>          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
>          goto out;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
> +    cpu_x86_set_props(cpu, props, &error);
>  
>  out:
> +    QDECREF(props);
>      g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 14/20] target-i386: print depricated warning if xlevel < 0x80000000

[Igor Mammedov]

On Thu, 27 Dec 2012 17:09:16 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Dec 27, 2012 at 03:59:30PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 084faeb..0764015 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1334,6 +1334,8 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> >                      goto error;
> >                  }
> >                  if (numvalue < 0x80000000) {
> > +                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
> > +                            ", fixup will be depricated in future versions\n");
> 
> Did you mean: "deprecated"?  :-)
Yep, the same mistake in patch subj. I'll fix it.

Thanks!
> 
> >                      numvalue += 0x80000000;
> >                  }
> >                  x86_cpu_def->xlevel = numvalue;
> > -- 
> > 1.7.1
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 13/20] target-i386: set custom 'vendor' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:29PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

I would write it as
  qdict_put(*props, "vendor", qstring_from_str(val))
instead. It would make the property-setting code more greppable (and
easier to read IMO).

On the other hand, using featurestr as the key makes sense if the
trivial qdict_put() calls like this one are eventually going to be
unified into a single qdict_put(*props, featurestr, qstring_from_str(val))
generic fallback.

> ---
>  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 3ead1f4..084faeb 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1338,7 +1338,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  }
>                  x86_cpu_def->xlevel = numvalue;
>              } else if (!strcmp(featurestr, "vendor")) {
> -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> +                qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "model_id")) {
>                  pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
>                          val);
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 15/20] target-i386: set custom 'xlevel' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:31PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0764015..d8af528 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1338,7 +1338,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                              ", fixup will be depricated in future versions\n");
>                      numvalue += 0x80000000;
>                  }
> -                x86_cpu_def->xlevel = numvalue;
> +                val = g_strdup_printf("%u", numvalue);
> +                qdict_put(*props, featurestr, qstring_from_str(val));
> +                g_free(val);

I would write that as:

  QString *s = qstring_new();
  qstring_append_int(s, numvalue);
  qdict_put(*props, featurestr, s);

So there would be no need for manual memory handling.

>              } else if (!strcmp(featurestr, "vendor")) {
>                  qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "model_id")) {
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 16/20] target-i386: set custom 'level' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:32PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

Now I am starting to see why you are using featurestr in the qdict_put()
calls.  :-)

> ---
>  target-i386/cpu.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d8af528..1cb2f08 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1319,13 +1319,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  }
>                  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;
> +                qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "xlevel")) {
>                  char *err;
>                  numvalue = strtoul(val, &err, 0);
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 17/20] target-i386: set custom 'model-id' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:33PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@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 1cb2f08..aab5f6f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1338,8 +1338,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>              } else if (!strcmp(featurestr, "vendor")) {
>                  qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "model_id")) {
> -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> -                        val);
> +                qdict_put(*props, "model-id", qstring_from_str(val));
>              } else if (!strcmp(featurestr, "tsc_freq")) {
>                  int64_t tsc_freq;
>                  char *err;
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 18/20] target-i386: set custom 'stepping' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:34PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  target-i386/cpu.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index aab5f6f..3ee4c65 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1311,13 +1311,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  }
>                  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 ;
> +                qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "level")) {
>                  qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "xlevel")) {
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 19/20] target-i386: set custom 'model' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:35PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  target-i386/cpu.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3ee4c65..8c65012 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1303,13 +1303,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  }
>                  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;
> +                qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "stepping")) {
>                  qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "level")) {
> -- 
> 1.7.1
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 08:15:18PM +0100, Igor Mammedov wrote:
> On Thu, 27 Dec 2012 16:41:47 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Dec 27, 2012 at 03:59:17PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > Patch is OK, but Subject line needs to be fixed. You are moving the
> > filtering for both KVM and non-KVM cases, not just the TCG filtering.
> would be "target-i386: filter out features at realize time" acceptable?

That's OK to me. Maybe "target-i386: filter out unsupported features at
realize time" to explain what kind of filtering it is.

> 
> > 
> > > ---
> > >  target-i386/cpu.c |   31 ++++++++++++++++---------------
> > >  1 files changed, 16 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 7be3ad8..63aae86 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1549,21 +1549,6 @@ 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;
> > > -    } else {
> > > -#ifdef CONFIG_KVM
> > > -        filter_features_for_kvm(cpu);
> > > -#endif
> > > -    }
> > >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> > >      if (error) {
> > >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > > @@ -2077,6 +2062,22 @@ void x86_cpu_realize(Object *obj, Error **errp)
> > >          env->cpuid_level = 7;
> > >      }
> > >  
> > > +    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;
> > > +    } else {
> > > +#ifdef CONFIG_KVM
> > > +        filter_features_for_kvm(cpu);
> > > +#endif
> > > +    }
> > > +
> > >  #ifndef CONFIG_USER_ONLY
> > >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> > >  
> > > -- 
> > > 1.7.1
> > > 
> > 
> > -- 
> > Eduardo
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [PATCH 20/20] target-i386: set custom 'family' without intermediate x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:36PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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


> ---
>  target-i386/cpu.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8c65012..fd53b41 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1295,13 +1295,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
> +                qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "model")) {
>                  qdict_put(*props, featurestr, qstring_from_str(val));
>              } else if (!strcmp(featurestr, "stepping")) {
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:26PM +0100, Igor Mammedov wrote:
[...]
>      assert(kvm_enabled());
>  
>      x86_cpu_def->name = "host";
> -    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> -    x86_cpu_def->vendor1 = ebx;
> -    x86_cpu_def->vendor2 = edx;
> -    x86_cpu_def->vendor3 = ecx;
> +    x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);

Oops: you removed the host_cpuid() call by mistake, so now we're filling
the vendor string with zeroes.

Reviewed-by withdrawn.

-- 
Eduardo

[Qemu-devel] [PATCH 11/20] target-i386: remove vendor_override field from CPUX86State

[Igor Mammedov]

commit 8935499831312 makes cpuid return to guest host's vendor value
instead of built-in one by default if kvm_enabled() == true and allows
to override this behavior if 'vendor' is specified on -cpu command line.

But every time guest calls cpuid to get 'vendor' value, host's value is
read again and again in default case.

It complicates semantic of vendor property and makes it harder to use.

Instead of reading 'vendor' value from host every time cpuid[vendor] is
called, override 'vendor' value only once in cpu_x86_find_by_name(), when
built-in CPU model is found and if(kvm_enabled() == true).

It provides the same default semantic
 if (kvm_enabled() == true)  vendor = host's vendor
 else vendor = built-in vendor

and then later:
 if (custom vendor) vendor = custom vendor

'vendor' value is overridden when user provides it on -cpu command line,
and there isn't need in vendor_override field anymore, remove it.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4c483a7..0a39632 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -284,7 +284,6 @@ typedef struct x86_def_t {
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
-    int vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -866,7 +865,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
                 kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
-    x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
     if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
@@ -1118,7 +1116,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)
@@ -1195,7 +1192,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 
     assert(def->vendor[0]);
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
-    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);
@@ -1232,6 +1228,18 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
         return -1;
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
+        /* sysenter isn't supported on compatibility mode on AMD, syscall
+         * isn't supported in compatibility mode on Intel.
+         * Normally we advertise the actual cpu vendor, but you can override
+         * this using the 'vendor' property if you want to use KVM's
+         * sysenter/syscall emulation in compatibility mode and when doing
+         * cross vendor migration
+         */
+        if (kvm_enabled()) {
+            uint32_t  ebx = 0, ecx = 0, edx = 0;
+            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+            x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
+        }
     }
 
     return 0;
@@ -1315,7 +1323,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
                 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);
@@ -1564,16 +1571,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
     *ebx = env->cpuid_vendor1;
     *edx = env->cpuid_vendor2;
     *ecx = env->cpuid_vendor3;
-
-    /* sysenter isn't supported on compatibility mode on AMD, syscall
-     * isn't supported in compatibility mode on Intel.
-     * Normally we advertise the actual cpu vendor, but you can override
-     * this if you want to use KVM's sysenter/syscall emulation
-     * in compatibility mode and when doing cross vendor migration
-     */
-    if (kvm_enabled() && ! env->cpuid_vendor_override) {
-        host_cpuid(0, 0, NULL, ebx, ecx, edx);
-    }
 }
 
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fbbe730..a15a09e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -812,7 +812,6 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 10/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

On Fri, 28 Dec 2012 15:39:19 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Dec 27, 2012 at 03:59:26PM +0100, Igor Mammedov wrote:
> [...]
> >      assert(kvm_enabled());
> >  
> >      x86_cpu_def->name = "host";
> > -    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> > -    x86_cpu_def->vendor1 = ebx;
> > -    x86_cpu_def->vendor2 = edx;
> > -    x86_cpu_def->vendor3 = ecx;
> > +    x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> 
> Oops: you removed the host_cpuid() call by mistake, so now we're filling
> the vendor string with zeroes.
fixed in v4 series

Thanks!

> 
> Reviewed-by withdrawn.
> 
> -- 
> Eduardo


-- 
Regards,
  Igor