[Qemu-devel] [PATCH qom-cpu 00/17] x86 CPU cleanup, part 3

[Qemu-devel] [PATCH qom-cpu 00/17] x86 CPU cleanup, part 3

[Igor Mammedov]

It's rebase of "PATCH 00/20 v4] x86 CPU cleanup (wave 2)" including
changes asked by Andreas on IRC. See below (v5) for detailed changes.

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.

Depends on "[PATCH qom-cpu 0/7] disable kvm_mmu + -cpu "enforce" fixes (v3)"
  http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00815.html
  which is applied to qom-cpu now

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

vendor related changes are tested with:
  https://github.com/imammedo/virt-test/tree/cpuid_features
  following command was used to run test:
   ./run -t kvm --qemu-bin=$QEMU --tests="qemu_cpu.qemu13 cpuid.custom_vendor"

v5:
 - dropped patch
   "[PATCH 06/20] target-i386: move out CPU features initialization in separate func"
   due to Andreas objection
 - fixed x86cpu to x86_cpu new function prefixes
 - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)" due conflicts
 - patches 1-4 from "[PATCH 00/20 v4] x86 CPU cleanup (wave 2)", already
   in master. So dropped from here
 - added patches 16-17 to deal with tsc_freq parsing without introducing
   new new visitor

v4:
 - rename [01/20] from:
     target-i386: filter out not TCG features if running without kvm at
       realize time
   to:
     target-i386: filter out unsupported features at realize time
 - make commit lines shorter for:
     target-i386: move kvm_check_features_against_host() check to realize
       time
 - restore removed by mistake host_cpuid() call in:
     target-i386: replace uint32_t vendor fields by vendor string in x86_def_t
 - fix spelling in:
     target-i386: print depricated warning if xlevel < 0x80000000
 - use qstring_append_int() for converting xlevel to string in:
     target-i386: set custom 'xlevel' without intermediate x86_def_t

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.



*** BLURB HERE ***

Igor Mammedov (17):
  target-i386: move setting defaults out of cpu_x86_parse_featurestr()
  target-i386: cpu_x86_register() consolidate freeing resources
  target-i386: move kvm_check_features_against_host() check to realize
    time
  target-i386: add x86_cpu_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 deprecated 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: set custom 'tsc-frequency' without intermediate
    x86_def_t
  target-i386: remove setting tsc-frequency from x86_def_t

 target-i386/cpu.c |  314 ++++++++++++++++++++++-------------------------------
 target-i386/cpu.h |    7 +-
 2 files changed, 131 insertions(+), 190 deletions(-)

[Qemu-devel] [PATCH 01/17] 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>
---
 v2:
  - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)"
---
 target-i386/cpu.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0a0a134..0f7a5eb 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1343,15 +1343,11 @@ 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 */
-    FeatureWordArray plus_features = {
-        [FEAT_KVM] = kvm_default_features,
-    };
+    FeatureWordArray plus_features = { 0 };
     /* Features to be removed */
     FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
 
-    add_flagname_to_bitmaps("hypervisor", plus_features);
-
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
@@ -1607,6 +1603,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto error;
     }
 
+    def->kvm_features |= kvm_default_features;
+    def->ext_features |= CPUID_EXT_HYPERVISOR;
+
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-- 
1.7.1

[Qemu-devel] [PATCH 02/17] 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
  v4:
   - rebased with "target-i386: move out CPU features
     initialization in separate func" dropped

  v3:
   - set error if cpu_model is empty

  v2:
   - add missing 'return -1' on exit if error is not NULL,
           Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0f7a5eb..ea68bc0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1594,20 +1594,23 @@ 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;
     def->ext_features |= CPUID_EXT_HYPERVISOR;
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
     assert(def->vendor1);
     env->cpuid_vendor1 = def->vendor1;
@@ -1632,17 +1635,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             "tsc-frequency", &error);
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &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 03/17] 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()

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v3:
   - call kvm_check_features_against_host() outside of #ifdef CONFIG_KVM
     to avoid adding ifdefs in it and around unavailable_host_feature()
   - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)"
 v2:
   - squash in ifdef-ing kvm specific functions into this patch
 v3:
   - fit commit message into 80 column limit, no change to content
---
 target-i386/cpu.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ea68bc0..c4ff761 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1022,27 +1022,28 @@ static int unavailable_host_feature(FeatureWordInfo *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,
             FEAT_1_EDX },
-        {&guest_def->ext_features, &host_def.ext_features,
+        {&env->cpuid_ext_features, &host_def.ext_features,
             FEAT_1_ECX },
-        {&guest_def->ext2_features, &host_def.ext2_features,
+        {&env->cpuid_ext2_features, &host_def.ext2_features,
             FEAT_8000_0001_EDX },
-        {&guest_def->ext3_features, &host_def.ext3_features,
+        {&env->cpuid_ext3_features, &host_def.ext3_features,
             FEAT_8000_0001_ECX },
-        {&guest_def->ext4_features, &host_def.ext4_features,
+        {&env->cpuid_ext4_features, &host_def.ext4_features,
             FEAT_C000_0001_EDX },
-        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
+        {&env->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
             FEAT_7_0_EBX },
-        {&guest_def->svm_features, &host_def.svm_features,
+        {&env->cpuid_svm_features, &host_def.svm_features,
             FEAT_SVM },
-        {&guest_def->kvm_features, &host_def.kvm_features,
+        {&env->cpuid_kvm_features, &host_def.kvm_features,
             FEAT_KVM },
     };
 
@@ -1471,10 +1472,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
     x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
-    if (check_cpuid && kvm_enabled()) {
-        if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
-            goto error;
-    }
     return 0;
 
 error:
@@ -2177,6 +2174,11 @@ void x86_cpu_realize(Object *obj, Error **errp)
 #ifdef CONFIG_KVM
         filter_features_for_kvm(cpu);
 #endif
+        if (check_cpuid && kvm_check_features_against_host(cpu)
+            && enforce_cpuid) {
+            error_setg(errp, "Host's CPU doesn't support requested features");
+            return;
+        }
     }
 
 #ifndef CONFIG_USER_ONLY
-- 
1.7.1

[Qemu-devel] [PATCH 04/17] target-i386: add x86_cpu_vendor_words2str()

[Igor Mammedov]

Make for() cycle reusable for the next patch

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v4:
   - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
        Suggested-By: Andreas Färber <afaerber@suse.de>
  v3:
   -replace e[bcd]x arguments naming with vendor[123]
   -fix/swap vendor2 and vendor3 order
        Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v2:
   -place x86cpu_vendor_words2str() a bit earlier, before feature
    arrays to avoid compile error when vendor property is converted
    static property.
---
 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 c4ff761..64cc88f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -45,6 +45,18 @@
 #include "hw/apic_internal.h"
 #endif
 
+static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                                    uint32_t vendor2, uint32_t vendor3)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = vendor1 >> (8 * i);
+        dst[i + 4] = vendor2 >> (8 * i);
+        dst[i + 8] = vendor3 >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1213,15 +1225,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';
+    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
+                            env->cpuid_vendor3);
     return value;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 05/17] 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>
---
v3:
 - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
   due to renaming of the last in previous patch
 - rebased with "target-i386: move out CPU features initialization
   in separate func" patch dropped
v2:
  - restore deleted host_cpuid() call in kvm_cpu_fill_host()
     Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |  126 +++++++++++++----------------------------------------
 target-i386/cpu.h |    6 +-
 2 files changed, 33 insertions(+), 99 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 64cc88f..0b4fa57 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -353,7 +353,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;
@@ -418,9 +418,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,
@@ -437,9 +435,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,
@@ -465,9 +461,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,
@@ -486,9 +480,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,
@@ -512,9 +504,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,
@@ -525,9 +515,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,
@@ -542,9 +530,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,
@@ -560,9 +546,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,
@@ -572,9 +556,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,
@@ -584,9 +566,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,
@@ -596,9 +576,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,
@@ -608,9 +586,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,
@@ -624,9 +600,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,
@@ -645,9 +619,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,
@@ -665,9 +637,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,
@@ -686,9 +656,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,
@@ -707,9 +675,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,
@@ -729,9 +695,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,
@@ -754,9 +718,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,
@@ -784,9 +746,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,
@@ -808,9 +768,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,
@@ -834,9 +792,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,
@@ -862,9 +818,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,
@@ -894,9 +848,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,
@@ -957,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     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;
+    x86_cpu_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);
@@ -987,9 +937,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) {
@@ -1348,7 +1296,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 */
     FeatureWordArray plus_features = { 0 };
@@ -1410,18 +1357,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),
@@ -1616,10 +1552,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
         goto out;
     }
-    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", &error);
     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);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e4a7c50..983aab1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #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 06/17] 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>
---
 v4:
   - rebased with "target-i386: move out CPU features initialization
     in separate func" dropped. So remove vendor_override in
     cpu_x86_register() instead.
   - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
     due to renaming of the last in previous patch
---
 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 0b4fa57..4250c77 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -362,7 +362,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;
@@ -934,7 +933,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)) {
@@ -1201,7 +1199,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)
@@ -1287,6 +1284,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);
+            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
+        }
     }
 
     return 0;
@@ -1358,7 +1367,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);
@@ -1554,7 +1562,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     }
     assert(def->vendor[0]);
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
-    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);
@@ -1626,16 +1633,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 983aab1..8bb5a58 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -829,7 +829,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 07/17] 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
x86_cpu_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 in following patches.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
    - rebased with "target-i386: move out CPU features initialization
      in separate func" patch dropped
    - renamed cpu_x86_set_props() to x86_cpu_set_props()
    - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)"
---
 target-i386/cpu.c |   34 ++++++++++++++++++++++++++++------
 1 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4250c77..c55f137 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1301,9 +1301,26 @@ 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 provided key,value list */
+static void x86_cpu_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));
+        /* TODO: switch to using global properties after subclasses are done */
+        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 */
@@ -1311,10 +1328,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     /* Features to be removed */
     FeatureWordArray minus_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);
@@ -1405,7 +1423,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[FEAT_1_EDX];
     x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
@@ -1423,9 +1440,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
     x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
+    g_strfreev(feat_array);
     return 0;
 
 error:
+    g_strfreev(feat_array);
     return -1;
 }
 
@@ -1534,6 +1553,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
+    QDict *props = NULL;
     Error *error = NULL;
     char *name, *features;
     gchar **model_pieces;
@@ -1556,7 +1576,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     def->kvm_features |= kvm_default_features;
     def->ext_features |= CPUID_EXT_HYPERVISOR;
 
-    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;
     }
@@ -1580,8 +1600,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             "tsc-frequency", &error);
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    x86_cpu_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 08/17] target-i386: set custom 'vendor' without intermediate x86_def_t

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@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 c55f137..485154d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1384,7 +1384,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 09/17] target-i386: print deprecated 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 485154d..9cf0ade 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1380,6 +1380,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 deprecated in future versions\n");
                     numvalue += 0x80000000;
                 }
                 x86_cpu_def->xlevel = numvalue;
-- 
1.7.1

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
  use qstring_append_int() for converting xlevel to string
   Suggested-By: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9cf0ade..b00c651 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1374,6 +1374,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 x86_cpu_def->level = numvalue;
             } else if (!strcmp(featurestr, "xlevel")) {
                 char *err;
+                QString *s;
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
                     fprintf(stderr, "bad numerical value %s\n", val);
@@ -1384,7 +1385,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                             ", fixup will be deprecated in future versions\n");
                     numvalue += 0x80000000;
                 }
-                x86_cpu_def->xlevel = numvalue;
+                s = qstring_new();
+                qstring_append_int(s, numvalue);
+                qdict_put(*props, featurestr, s);
             } 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 11/17] target-i386: set custom 'level' without intermediate x86_def_t

[Igor Mammedov]

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 b00c651..b84bde6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1365,13 +1365,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;
                 QString *s;
-- 
1.7.1

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

[Igor Mammedov]

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 b84bde6..635f126 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1385,8 +1385,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 13/17] target-i386: set custom 'stepping' without intermediate x86_def_t

[Igor Mammedov]

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 635f126..6cb9bed 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1357,13 +1357,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 14/17] target-i386: set custom 'model' without intermediate x86_def_t

[Igor Mammedov]

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 6cb9bed..51411aa 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1349,13 +1349,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 15/17] target-i386: set custom 'family' without intermediate x86_def_t

[Igor Mammedov]

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 51411aa..ec27cf6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1341,13 +1341,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

[Qemu-devel] [PATCH 16/17] target-i386: set custom 'tsc-frequency' without intermediate x86_def_t

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  make patch independent of visit_type_freq()
---
 target-i386/cpu.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ec27cf6..c3e1792 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1371,6 +1371,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
             } else if (!strcmp(featurestr, "tsc_freq")) {
                 int64_t tsc_freq;
                 char *err;
+                QString *s;
 
                 tsc_freq = strtosz_suffix_unit(val, &err,
                                                STRTOSZ_DEFSUFFIX_B, 1000);
@@ -1378,7 +1379,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                     fprintf(stderr, "bad numerical value %s\n", val);
                     goto error;
                 }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
+                s = qstring_new();
+                qstring_append_int(s, tsc_freq);
+                qdict_put(*props, "tsc-frequency", s);
             } else if (!strcmp(featurestr, "hv_spinlocks")) {
                 char *err;
                 numvalue = strtoul(val, &err, 0);
-- 
1.7.1

[Qemu-devel] [PATCH 17/17] target-i386: remove setting tsc-frequency from x86_def_t

[Igor Mammedov]

Setting tsc-frequency from x86_def_t is NOP because default tsc_khz
in x86_def_t is 0 and CPUX86State.tsc_khz is also initialized to 0
by default. So there is not need to set ovewrite tsc_khz with default
0 because field was already initialized to 0.

custom tsc-frequency setting is not affected due to it is being set
without using x86_def_t (previous patch)

Field tsc_khz in x86_def_t becomes unused with this patch, so drop it
as well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v2:
  - rebased with "target-i386: move out CPU features initialization
    in separate func" patch dropped
---
 target-i386/cpu.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c3e1792..af93673 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -357,7 +357,6 @@ typedef struct x86_def_t {
     int family;
     int model;
     int stepping;
-    int tsc_khz;
     uint32_t features, ext_features, ext2_features, ext3_features;
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
@@ -1579,8 +1578,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     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);
     x86_cpu_set_props(cpu, props, &error);
-- 
1.7.1

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:15AM +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>

> ---
>  v2:
>   - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)"
> ---
>  target-i386/cpu.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0a0a134..0f7a5eb 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1343,15 +1343,11 @@ 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 */
> -    FeatureWordArray plus_features = {
> -        [FEAT_KVM] = kvm_default_features,
> -    };
> +    FeatureWordArray plus_features = { 0 };
>      /* Features to be removed */
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
>  
> -    add_flagname_to_bitmaps("hypervisor", plus_features);
> -
>      featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
> @@ -1607,6 +1603,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto error;
>      }
>  
> +    def->kvm_features |= kvm_default_features;
> +    def->ext_features |= CPUID_EXT_HYPERVISOR;
> +
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -- 
> 1.7.1
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:16AM +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>

To confirm that this rebase looks good to me:

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

> ---
>   v4:
>    - rebased with "target-i386: move out CPU features
>      initialization in separate func" dropped
> 
>   v3:
>    - set error if cpu_model is empty
> 
>   v2:
>    - add missing 'return -1' on exit if error is not NULL,
>            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0f7a5eb..ea68bc0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1594,20 +1594,23 @@ 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;
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> -        goto error;
> +        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> +        goto out;
>      }
>      assert(def->vendor1);
>      env->cpuid_vendor1 = def->vendor1;
> @@ -1632,17 +1635,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              "tsc-frequency", &error);
>  
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &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 03/17] target-i386: move kvm_check_features_against_host() check to realize time

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:17AM +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()
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  v3:
>    - call kvm_check_features_against_host() outside of #ifdef CONFIG_KVM
>      to avoid adding ifdefs in it and around unavailable_host_feature()
>    - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)"
>  v2:
>    - squash in ifdef-ing kvm specific functions into this patch
>  v3:
>    - fit commit message into 80 column limit, no change to content
> ---
>  target-i386/cpu.c |   28 +++++++++++++++-------------
>  1 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ea68bc0..c4ff761 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1022,27 +1022,28 @@ static int unavailable_host_feature(FeatureWordInfo *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,
>              FEAT_1_EDX },
> -        {&guest_def->ext_features, &host_def.ext_features,
> +        {&env->cpuid_ext_features, &host_def.ext_features,
>              FEAT_1_ECX },
> -        {&guest_def->ext2_features, &host_def.ext2_features,
> +        {&env->cpuid_ext2_features, &host_def.ext2_features,
>              FEAT_8000_0001_EDX },
> -        {&guest_def->ext3_features, &host_def.ext3_features,
> +        {&env->cpuid_ext3_features, &host_def.ext3_features,
>              FEAT_8000_0001_ECX },
> -        {&guest_def->ext4_features, &host_def.ext4_features,
> +        {&env->cpuid_ext4_features, &host_def.ext4_features,
>              FEAT_C000_0001_EDX },
> -        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
> +        {&env->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
>              FEAT_7_0_EBX },
> -        {&guest_def->svm_features, &host_def.svm_features,
> +        {&env->cpuid_svm_features, &host_def.svm_features,
>              FEAT_SVM },
> -        {&guest_def->kvm_features, &host_def.kvm_features,
> +        {&env->cpuid_kvm_features, &host_def.kvm_features,
>              FEAT_KVM },
>      };
>  
> @@ -1471,10 +1472,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
>      x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
>      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> -    if (check_cpuid && kvm_enabled()) {
> -        if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
> -            goto error;
> -    }
>      return 0;
>  
>  error:
> @@ -2177,6 +2174,11 @@ void x86_cpu_realize(Object *obj, Error **errp)
>  #ifdef CONFIG_KVM
>          filter_features_for_kvm(cpu);
>  #endif
> +        if (check_cpuid && kvm_check_features_against_host(cpu)
> +            && enforce_cpuid) {
> +            error_setg(errp, "Host's CPU doesn't support requested features");
> +            return;
> +        }
>      }
>  
>  #ifndef CONFIG_USER_ONLY
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/17] target-i386: add x86_cpu_vendor_words2str()

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:18AM +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>

Very minor style comment below.

> ---
>   v4:
>    - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
>         Suggested-By: Andreas Färber <afaerber@suse.de>
>   v3:
>    -replace e[bcd]x arguments naming with vendor[123]
>    -fix/swap vendor2 and vendor3 order
>         Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
>   v2:
>    -place x86cpu_vendor_words2str() a bit earlier, before feature
>     arrays to avoid compile error when vendor property is converted
>     static property.
> ---
>  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 c4ff761..64cc88f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -45,6 +45,18 @@
>  #include "hw/apic_internal.h"
>  #endif
>  
> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> +                                    uint32_t vendor2, uint32_t vendor3)
> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = vendor1 >> (8 * i);
> +        dst[i + 4] = vendor2 >> (8 * i);
> +        dst[i + 8] = vendor3 >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}
> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -1213,15 +1225,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';
> +    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
> +                            env->cpuid_vendor3);

I would add one extra space, so the argument in the second line are
aligned with the first argument from the previous line.

>      return value;
>  }
>  
> -- 
> 1.7.1
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:19AM +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>
> ---
> v3:
>  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>    due to renaming of the last in previous patch
>  - rebased with "target-i386: move out CPU features initialization
>    in separate func" patch dropped
> v2:
>   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
>      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |  126 +++++++++++++----------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 33 insertions(+), 99 deletions(-)
> 

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

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:20AM +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
> 

How do you plan to make this work when the vendor string for each model
gets represented using a default defined at class_init-time[1]? When we do
that, we wouldn't be able to differentiate the built/predefined vendor
value (used for TCG only) and the user-defined vendor value (that would
be set in KVM mode too).

[1] AFAIU, class_init would never be able to check kvm_enabled(), by
design, because it may run before anything is configured (even the
choice to enable/disable KVM).

Maybe we can later add a "tcg-vendor" property, that would override the
vendor only if in TCG mode. Then the predefined CPU models could have a
"tcg-vendor" default set, and no "vendor" default set, to emulate the
current behavior.

> '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>
> ---
>  v4:
>    - rebased with "target-i386: move out CPU features initialization
>      in separate func" dropped. So remove vendor_override in
>      cpu_x86_register() instead.
>    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>      due to renaming of the last in previous patch
> ---
>  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 0b4fa57..4250c77 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -362,7 +362,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;
> @@ -934,7 +933,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)) {
> @@ -1201,7 +1199,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)
> @@ -1287,6 +1284,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);
> +            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> +        }
>      }
>  
>      return 0;
> @@ -1358,7 +1367,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);
> @@ -1554,7 +1562,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      }
>      assert(def->vendor[0]);
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> -    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);
> @@ -1626,16 +1633,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 983aab1..8bb5a58 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -829,7 +829,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

[Qemu-devel] [PATCH 04/17 v2] target-i386: add x86_cpu_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>
---
  v5:
   - style fix: added extra space to align arguments
        Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v4:
   - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
        Suggested-By: Andreas Färber <afaerber@suse.de>
  v3:
   -replace e[bcd]x arguments naming with vendor[123]
   -fix/swap vendor2 and vendor3 order
        Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v2:
   -place x86cpu_vendor_words2str() a bit earlier, before feature
    arrays to avoid compile error when vendor property is converted
    static property.
---
 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 333745b..ede44d2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -45,6 +45,18 @@
 #include "hw/apic_internal.h"
 #endif
 
+static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                                    uint32_t vendor2, uint32_t vendor3)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = vendor1 >> (8 * i);
+        dst[i + 4] = vendor2 >> (8 * i);
+        dst[i + 8] = vendor3 >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1213,15 +1225,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';
+    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
+                             env->cpuid_vendor3);
     return value;
 }
 
-- 
1.7.1

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

[Igor Mammedov]

On Fri, 11 Jan 2013 00:42:13 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jan 11, 2013 at 03:10:20AM +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
> > 
> 
> How do you plan to make this work when the vendor string for each model
> gets represented using a default defined at class_init-time[1]? When we do
it could be done by late update hook like you did for host cpu subclass or
forcing early kvm_init() and then do it at class_init-time. I still hope that
second option would work(but I haven't checked yet).

> that, we wouldn't be able to differentiate the built/predefined vendor
> value (used for TCG only) and the user-defined vendor value (that would
> be set in KVM mode too).
If we have subclasses with already correct vendor for a specific mode then we
needn't to know if it is built-in vendor or user-defined.

> 
> [1] AFAIU, class_init would never be able to check kvm_enabled(), by
> design, because it may run before anything is configured (even the
> choice to enable/disable KVM).
> 
> Maybe we can later add a "tcg-vendor" property, that would override the
> vendor only if in TCG mode. Then the predefined CPU models could have a
> "tcg-vendor" default set, and no "vendor" default set, to emulate the
> current behavior.
> 
> > '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>
> > ---
> >  v4:
> >    - rebased with "target-i386: move out CPU features initialization
> >      in separate func" dropped. So remove vendor_override in
> >      cpu_x86_register() instead.
> >    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >      due to renaming of the last in previous patch
> > ---
> >  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 0b4fa57..4250c77 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -362,7 +362,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;
> > @@ -934,7 +933,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)) {
> > @@ -1201,7 +1199,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)
> > @@ -1287,6 +1284,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);
> > +            x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> > +        }
> >      }
> >  
> >      return 0;
> > @@ -1358,7 +1367,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);
> > @@ -1554,7 +1562,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      }
> >      assert(def->vendor[0]);
> >      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> > -    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);
> > @@ -1626,16 +1633,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 983aab1..8bb5a58 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -829,7 +829,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


-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 04:09:45AM +0100, Igor Mammedov wrote:
> On Fri, 11 Jan 2013 00:42:13 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jan 11, 2013 at 03:10:20AM +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
> > > 
> > 
> > How do you plan to make this work when the vendor string for each model
> > gets represented using a default defined at class_init-time[1]? When we do
> it could be done by late update hook like you did for host cpu subclass or
> forcing early kvm_init() and then do it at class_init-time. I still hope that
> second option would work(but I haven't checked yet).

I believe that it is by design that class_init runs before kvm={on,off}
configuration is handled, and we won't be able to change it. We'll need
to clarify that.

And I don't believe we will be allowed to change the class defaults
after class_init already ran. That may be a problem for -cpu "host" as
well. Maybe in the case of -cpu "host" we will need to translate the
behavior to a "enable-all-features-supported-by-the-host=true" property
that would be handled by instance_init and override all f-* flags, or
making the feature flags tristate (on/off/host), and setting
"f-feature=host" for all features.

> 
> > that, we wouldn't be able to differentiate the built/predefined vendor
> > value (used for TCG only) and the user-defined vendor value (that would
> > be set in KVM mode too).
> If we have subclasses with already correct vendor for a specific mode then we
> needn't to know if it is built-in vendor or user-defined.

True, but I still don't see an obvious way to make sure the class has
the already correct vendor for a specific mode.

That's why having separate "vendor"/"tcg-vendor" (and maybe even
"kvm-vendor") properties sounded like a feasible solution.

Anyway, this shouldn't hold this specific patch, and we will be forced
to discuss that when we have a patch that adds a "vendor" static
property, so:

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

> 
> > 
> > [1] AFAIU, class_init would never be able to check kvm_enabled(), by
> > design, because it may run before anything is configured (even the
> > choice to enable/disable KVM).
> > 
> > Maybe we can later add a "tcg-vendor" property, that would override the
> > vendor only if in TCG mode. Then the predefined CPU models could have a
> > "tcg-vendor" default set, and no "vendor" default set, to emulate the
> > current behavior.
> > 
> > > '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>
> > > ---
> > >  v4:
> > >    - rebased with "target-i386: move out CPU features initialization
> > >      in separate func" dropped. So remove vendor_override in
> > >      cpu_x86_register() instead.
> > >    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> > >      due to renaming of the last in previous patch
> > > ---
> > >  target-i386/cpu.c |   27 ++++++++++++---------------
> > >  target-i386/cpu.h |    1 -
> > >  2 files changed, 12 insertions(+), 16 deletions(-)
> > > 
[...]

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:20AM +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>

In case my previous reviewed-by line (buried in the discussion about
property defaults) was missed:

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

> ---
>  v4:
>    - rebased with "target-i386: move out CPU features initialization
>      in separate func" dropped. So remove vendor_override in
>      cpu_x86_register() instead.
>    - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>      due to renaming of the last in previous patch
> ---
>  target-i386/cpu.c |   27 ++++++++++++---------------
>  target-i386/cpu.h |    1 -
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 
[...]

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:21AM +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
> x86_cpu_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 in following patches.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>   v2:
>     - rebased with "target-i386: move out CPU features initialization
>       in separate func" patch dropped
>     - renamed cpu_x86_set_props() to x86_cpu_set_props()
>     - rebased on top of "disable kvm_mmu + -cpu "enforce" fixes (v3)"
> ---
>  target-i386/cpu.c |   34 ++++++++++++++++++++++++++++------
>  1 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4250c77..c55f137 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1301,9 +1301,26 @@ 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 provided key,value list */
> +static void x86_cpu_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));
> +        /* TODO: switch to using global properties after subclasses are done */
> +        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 */
> @@ -1311,10 +1328,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      /* Features to be removed */
>      FeatureWordArray minus_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);
> @@ -1405,7 +1423,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[FEAT_1_EDX];
>      x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
> @@ -1423,9 +1440,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
>      x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
>      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> +    g_strfreev(feat_array);
>      return 0;
>  
>  error:
> +    g_strfreev(feat_array);
>      return -1;
>  }
>  
> @@ -1534,6 +1553,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
>      CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
> +    QDict *props = NULL;
>      Error *error = NULL;
>      char *name, *features;
>      gchar **model_pieces;
> @@ -1556,7 +1576,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      def->kvm_features |= kvm_default_features;
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
> -    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;
>      }
> @@ -1580,8 +1600,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              "tsc-frequency", &error);
>  
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +    x86_cpu_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 09/17] target-i386: print deprecated warning if xlevel < 0x80000000

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:23AM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@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 485154d..9cf0ade 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1380,6 +1380,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 deprecated in future versions\n");
>                      numvalue += 0x80000000;
>                  }
>                  x86_cpu_def->xlevel = numvalue;
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 16/17] target-i386: set custom 'tsc-frequency' without intermediate x86_def_t

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:30AM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

Will this be converted to a simple qdict_put() of the raw string value,
to be parsed using visit_type_freq() (or something equivalent), in the
future? It would be interesting to keep the ability to use
"tsc-frequency=1GHz" on device_add/-device.


> ---
> v2:
>   make patch independent of visit_type_freq()
> ---
>  target-i386/cpu.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ec27cf6..c3e1792 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1371,6 +1371,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>              } else if (!strcmp(featurestr, "tsc_freq")) {
>                  int64_t tsc_freq;
>                  char *err;
> +                QString *s;
>  
>                  tsc_freq = strtosz_suffix_unit(val, &err,
>                                                 STRTOSZ_DEFSUFFIX_B, 1000);
> @@ -1378,7 +1379,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                      fprintf(stderr, "bad numerical value %s\n", val);
>                      goto error;
>                  }
> -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> +                s = qstring_new();
> +                qstring_append_int(s, tsc_freq);
> +                qdict_put(*props, "tsc-frequency", s);
>              } else if (!strcmp(featurestr, "hv_spinlocks")) {
>                  char *err;
>                  numvalue = strtoul(val, &err, 0);
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 17/17] target-i386: remove setting tsc-frequency from x86_def_t

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 03:10:31AM +0100, Igor Mammedov wrote:
> Setting tsc-frequency from x86_def_t is NOP because default tsc_khz
> in x86_def_t is 0 and CPUX86State.tsc_khz is also initialized to 0
> by default. So there is not need to set ovewrite tsc_khz with default
> 0 because field was already initialized to 0.
> 
> custom tsc-frequency setting is not affected due to it is being set
> without using x86_def_t (previous patch)
> 
> Field tsc_khz in x86_def_t becomes unused with this patch, so drop it
> as well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  v2:
>   - rebased with "target-i386: move out CPU features initialization
>     in separate func" patch dropped
> ---
>  target-i386/cpu.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c3e1792..af93673 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -357,7 +357,6 @@ typedef struct x86_def_t {
>      int family;
>      int model;
>      int stepping;
> -    int tsc_khz;
>      uint32_t features, ext_features, ext2_features, ext3_features;
>      uint32_t kvm_features, svm_features;
>      uint32_t xlevel;
> @@ -1579,8 +1578,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      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);
>      x86_cpu_set_props(cpu, props, &error);
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 16/17] target-i386: set custom 'tsc-frequency' without intermediate x86_def_t

[Igor Mammedov]

On Mon, 14 Jan 2013 13:20:44 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jan 11, 2013 at 03:10:30AM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Will this be converted to a simple qdict_put() of the raw string value,
> to be parsed using visit_type_freq() (or something equivalent), in the
> future? It would be interesting to keep the ability to use
> "tsc-frequency=1GHz" on device_add/-device.
Anthony was opposed to idea of special visitor. 
This patch allow us to move forward and in easily convert to visitor
later if there will be agreement to adding new visitor.

On the other hand we could leave it as it done here and enforce users to
provide numeric value for "tsc-frequency" in via  device_add interface
while maintaining compatibility for legacy 'tsc_freq'.

>
> 
> > ---
> > v2:
> >   make patch independent of visit_type_freq()
> > ---
> >  target-i386/cpu.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ec27cf6..c3e1792 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1371,6 +1371,7 @@ static int cpu_x86_parse_featurestr(x86_def_t
> > *x86_cpu_def, char *features, } else if (!strcmp(featurestr, "tsc_freq"))
> > { int64_t tsc_freq;
> >                  char *err;
> > +                QString *s;
> >  
> >                  tsc_freq = strtosz_suffix_unit(val, &err,
> >                                                 STRTOSZ_DEFSUFFIX_B,
> > 1000); @@ -1378,7 +1379,9 @@ static int
> > cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> > fprintf(stderr, "bad numerical value %s\n", val); goto error;
> >                  }
> > -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > +                s = qstring_new();
> > +                qstring_append_int(s, tsc_freq);
> > +                qdict_put(*props, "tsc-frequency", s);
> >              } else if (!strcmp(featurestr, "hv_spinlocks")) {
> >                  char *err;
> >                  numvalue = strtoul(val, &err, 0);
> > -- 
> > 1.7.1
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 16/17] target-i386: set custom 'tsc-frequency' without intermediate x86_def_t

[Eduardo Habkost]

On Mon, Jan 14, 2013 at 04:49:23PM +0100, Igor Mammedov wrote:
> On Mon, 14 Jan 2013 13:20:44 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jan 11, 2013 at 03:10:30AM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > Will this be converted to a simple qdict_put() of the raw string value,
> > to be parsed using visit_type_freq() (or something equivalent), in the
> > future? It would be interesting to keep the ability to use
> > "tsc-frequency=1GHz" on device_add/-device.
> Anthony was opposed to idea of special visitor. 
> This patch allow us to move forward and in easily convert to visitor
> later if there will be agreement to adding new visitor.
> 
> On the other hand we could leave it as it done here and enforce users to
> provide numeric value for "tsc-frequency" in via  device_add interface
> while maintaining compatibility for legacy 'tsc_freq'.

I am starting to think that just using an integer without any special
parsing for "tsc-frequencey" on device_add/-device would be much
simpler.

(so ignore where I said "it would be interesting to keep the ability"
above).

> 
> >
> > 
> > > ---
> > > v2:
> > >   make patch independent of visit_type_freq()
> > > ---
> > >  target-i386/cpu.c |    5 ++++-
> > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index ec27cf6..c3e1792 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1371,6 +1371,7 @@ static int cpu_x86_parse_featurestr(x86_def_t
> > > *x86_cpu_def, char *features, } else if (!strcmp(featurestr, "tsc_freq"))
> > > { int64_t tsc_freq;
> > >                  char *err;
> > > +                QString *s;
> > >  
> > >                  tsc_freq = strtosz_suffix_unit(val, &err,
> > >                                                 STRTOSZ_DEFSUFFIX_B,
> > > 1000); @@ -1378,7 +1379,9 @@ static int
> > > cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> > > fprintf(stderr, "bad numerical value %s\n", val); goto error;
> > >                  }
> > > -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > > +                s = qstring_new();
> > > +                qstring_append_int(s, tsc_freq);
> > > +                qdict_put(*props, "tsc-frequency", s);
> > >              } else if (!strcmp(featurestr, "hv_spinlocks")) {
> > >                  char *err;
> > >                  numvalue = strtoul(val, &err, 0);
> > > -- 
> > > 1.7.1
> > > 
> > > 
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/17] target-i386: add x86_cpu_vendor_words2str()

[Andreas Färber]

Am 11.01.2013 03:28, schrieb Eduardo Habkost:
> On Fri, Jan 11, 2013 at 03:10:18AM +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>
> 
> Very minor style comment below.
> 
>> ---
>>   v4:
>>    - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
>>         Suggested-By: Andreas Färber <afaerber@suse.de>
>>   v3:
>>    -replace e[bcd]x arguments naming with vendor[123]
>>    -fix/swap vendor2 and vendor3 order
>>         Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
>>   v2:
>>    -place x86cpu_vendor_words2str() a bit earlier, before feature
>>     arrays to avoid compile error when vendor property is converted
>>     static property.
>> ---
>>  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 c4ff761..64cc88f 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -45,6 +45,18 @@
>>  #include "hw/apic_internal.h"
>>  #endif
>>  
>> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>> +                                    uint32_t vendor2, uint32_t vendor3)

Same nit here.

>> +{
>> +    int i;
>> +    for (i = 0; i < 4; i++) {
>> +        dst[i] = vendor1 >> (8 * i);
>> +        dst[i + 4] = vendor2 >> (8 * i);
>> +        dst[i + 8] = vendor3 >> (8 * i);
>> +    }
>> +    dst[CPUID_VENDOR_SZ] = '\0';
>> +}
>> +
>>  /* feature flags taken from "Intel Processor Identification and the CPUID
>>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>>   * between feature naming conventions, aliases may be added.
>> @@ -1213,15 +1225,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';
>> +    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
>> +                            env->cpuid_vendor3);
> 
> I would add one extra space, so the argument in the second line are
> aligned with the first argument from the previous line.

I can edit both in myself, but I won't manage to go through the whole
patchset(s) til tomorrow. 1-4 look fine so far.

Andreas

> 
>>      return value;
>>  }
>>  
>> -- 
>> 1.7.1
>>
> 


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

Re: [Qemu-devel] [PATCH 04/17] target-i386: add x86_cpu_vendor_words2str()

[Igor Mammedov]

On Mon, 14 Jan 2013 18:14:27 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 11.01.2013 03:28, schrieb Eduardo Habkost:
> > On Fri, Jan 11, 2013 at 03:10:18AM +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>
> > 
> > Very minor style comment below.
> > 
> >> ---
> >>   v4:
> >>    - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
> >>         Suggested-By: Andreas Färber <afaerber@suse.de>
> >>   v3:
> >>    -replace e[bcd]x arguments naming with vendor[123]
> >>    -fix/swap vendor2 and vendor3 order
> >>         Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> >>   v2:
> >>    -place x86cpu_vendor_words2str() a bit earlier, before feature
> >>     arrays to avoid compile error when vendor property is converted
> >>     static property.
> >> ---
> >>  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 c4ff761..64cc88f 100644
> >> --- a/target-i386/cpu.c
> >> +++ b/target-i386/cpu.c
> >> @@ -45,6 +45,18 @@
> >>  #include "hw/apic_internal.h"
> >>  #endif
> >>  
> >> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> >> +                                    uint32_t vendor2, uint32_t vendor3)
> 
> Same nit here.
I'll resubmit fixed patch. Thanks!

> 
> >> +{
> >> +    int i;
> >> +    for (i = 0; i < 4; i++) {
> >> +        dst[i] = vendor1 >> (8 * i);
> >> +        dst[i + 4] = vendor2 >> (8 * i);
> >> +        dst[i + 8] = vendor3 >> (8 * i);
> >> +    }
> >> +    dst[CPUID_VENDOR_SZ] = '\0';
> >> +}
> >> +
> >>  /* feature flags taken from "Intel Processor Identification and the CPUID
> >>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
> >>   * between feature naming conventions, aliases may be added.
> >> @@ -1213,15 +1225,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';
> >> +    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
> >> +                            env->cpuid_vendor3);
> > 
> > I would add one extra space, so the argument in the second line are
> > aligned with the first argument from the previous line.
it was fixed in "[PATCH 04/17 v2] target-i386: add x86_cpu_vendor_words2str()"
follow-up to original patch.
> 
> I can edit both in myself, but I won't manage to go through the whole
> patchset(s) til tomorrow. 1-4 look fine so far.
No need to edit. I'll re-submit fixed version in a moment.

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


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 04/17 v3] target-i386: add x86_cpu_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>
---
  v5:
   - style fix: added extra space to align arguments
        Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
        Spotted-By: Andreas Färber <afaerber@suse.de>
  v4:
   - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
        Suggested-By: Andreas Färber <afaerber@suse.de>
  v3:
   -replace e[bcd]x arguments naming with vendor[123]
   -fix/swap vendor2 and vendor3 order
        Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v2:
   -place x86cpu_vendor_words2str() a bit earlier, before feature
    arrays to avoid compile error when vendor property is converted
    static property.
---
 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 333745b..e9aebda 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -45,6 +45,18 @@
 #include "hw/apic_internal.h"
 #endif
 
+static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                                     uint32_t vendor2, uint32_t vendor3)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = vendor1 >> (8 * i);
+        dst[i + 4] = vendor2 >> (8 * i);
+        dst[i + 8] = vendor3 >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1213,15 +1225,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';
+    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
+                             env->cpuid_vendor3);
     return value;
 }
 
-- 
1.7.1

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

[Andreas Färber]

Am 11.01.2013 03:10, schrieb 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>

I have doubts about this patch as-is, maybe unfounded. It happens to
work for the three vendors we have in-tree. But three numeric words
would be well capable of respesenting a NUL char whereas the QOM
property API has no concept of a string length != strlen(value) IIUC (no
way to specify on setting and using g_strdup() internally).

Is there any validity guarantee documented in some Intel spec?

Another disadvantage I see is that each X86CPU subclass will have to use
pstrcpy() to initialize the value - or we would have to go with three
vendor base classes to hide this annoyance from each model. It still
"happens" to look nice here due to C99 x86_def_t initialization.

> ---
> v3:
>  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>    due to renaming of the last in previous patch
>  - rebased with "target-i386: move out CPU features initialization
>    in separate func" patch dropped
> v2:
>   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
>      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |  126 +++++++++++++----------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 33 insertions(+), 99 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 64cc88f..0b4fa57 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -353,7 +353,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;
[...]
> @@ -957,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>  
>      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;
> +    x86_cpu_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);
> @@ -987,9 +937,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)) {

I'd be more comfortable doing such comparisons with a length or, even
better, using a static inline helper doing so: In this case
("CentaurHauls") it happens to work but I'm worried about setting a bad
example that gets copied-and-pasted.

>          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>          if (eax >= 0xC0000001) {
> @@ -1348,7 +1296,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 */
>      FeatureWordArray plus_features = { 0 };
> @@ -1410,18 +1357,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),

This one is fine by comparison.

> @@ -1616,10 +1552,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
>          goto out;
>      }
> -    assert(def->vendor1);
> -    env->cpuid_vendor1 = def->vendor1;
> -    env->cpuid_vendor2 = def->vendor2;
> -    env->cpuid_vendor3 = def->vendor3;
> +    assert(def->vendor[0]);

Instead of asserting on an empty string here, we should set the Error*
inside the property setter.

> +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);

The QOM API does not allow to specify the length here.

That property is not added in this patch, so it was probably added by
myself for the command line handling, where we could expect a sane
input. Inside x86_def_t or X86CPUClass anything could be coded though,
in theory. Admittedly the property output would already be broken then.

Regards,
Andreas

>      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);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index e4a7c50..983aab1 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #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 */

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

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

[Eduardo Habkost]

On Mon, Jan 14, 2013 at 08:32:12PM +0100, Andreas Färber wrote:
> Am 11.01.2013 03:10, schrieb 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>
> 
> I have doubts about this patch as-is, maybe unfounded. It happens to
> work for the three vendors we have in-tree. But three numeric words
> would be well capable of respesenting a NUL char whereas the QOM
> property API has no concept of a string length != strlen(value) IIUC (no
> way to specify on setting and using g_strdup() internally).

Internally, we can easily represent the values with embeded NULs using
the char array, and I believe the char array is clearer easier to
manipulate than the three integers.

Externally, a vendor with NUL characters would be a problem for the
"vendor=foo" interface. But this is an existing problem with the
existing command-line interface, this patch doesn't change that
interface at all.

We could add numeric vendor{1,2,3} properties for people who really want
to set the vendor IDs bit-by-bit. But I suspect nobody would ever user
that interface at all.


> 
> Is there any validity guarantee documented in some Intel spec?

Yes: the Intel spec says all their CPUs have vendor=GenuineIntel. If you
see any other value, their documentation don't apply at all[1]. :-)

We can't even guarantee that QEMU is emulating the CPU correctly if an
unknown string is used, because the CPU won't be covered by any known
documentation. We allow arbitrary vendor strings just because it would
be extra code to care about, but I don't think we should document
anything except GenuineIntel/AuthenticAMD/CentaurHauls as supported.


> 
> Another disadvantage I see is that each X86CPU subclass will have to use
> pstrcpy() to initialize the value - or we would have to go with three
> vendor base classes to hide this annoyance from each model. It still
> "happens" to look nice here due to C99 x86_def_t initialization.

Actually, each subclass would just set a default value to the "vendor"
property, eventually. The pstrcpy()-based initialization would be just
temporary. (On either case, maybe having vendor-specific base classes
would be a good thing).

----
References:

[1] Intel Application Note 485: Intel® Processor Identification and the
CPUID Instruction, section 5.1.1 - "Vendor-ID and Largest Standard
Function (Function 0)":

   If the “GenuineIntel” string is not returned after execution of the
   CPUID instruction, do not rely upon the information described in this
   document to interpret the information returned by the CPUID
   instruction.

AMD docs say something similar:

AMD64 Architecture Programmer’s Manual Volume 3
CPUID Instruction

   Standard function 0 and extended function 8000_0000h both load a
   12-character string into the EBX, EDX, and ECX registers identifying the
   processor vendor. For AMD processors, the string is AuthenticAMD. This
   string informs software that it should follow the AMD CPUID definition
   for subsequent CPUID function calls. If the function returns another
   vendor’s string, software must use that vendor’s CPUID definition when
   interpreting the results of subsequent CPUID function calls.


> 
> > ---
> > v3:
> >  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >    due to renaming of the last in previous patch
> >  - rebased with "target-i386: move out CPU features initialization
> >    in separate func" patch dropped
> > v2:
> >   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
> >      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c |  126 +++++++++++++----------------------------------------
> >  target-i386/cpu.h |    6 +-
> >  2 files changed, 33 insertions(+), 99 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 64cc88f..0b4fa57 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -353,7 +353,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;
> [...]
> > @@ -957,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
> >  
> >      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;
> > +    x86_cpu_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);
> > @@ -987,9 +937,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)) {
> 
> I'd be more comfortable doing such comparisons with a length or, even
> better, using a static inline helper doing so: In this case
> ("CentaurHauls") it happens to work but I'm worried about setting a bad
> example that gets copied-and-pasted.
> 
> >          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> >          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> >          if (eax >= 0xC0000001) {
> > @@ -1348,7 +1296,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 */
> >      FeatureWordArray plus_features = { 0 };
> > @@ -1410,18 +1357,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),
> 
> This one is fine by comparison.
> 
> > @@ -1616,10 +1552,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> >          goto out;
> >      }
> > -    assert(def->vendor1);
> > -    env->cpuid_vendor1 = def->vendor1;
> > -    env->cpuid_vendor2 = def->vendor2;
> > -    env->cpuid_vendor3 = def->vendor3;
> > +    assert(def->vendor[0]);
> 
> Instead of asserting on an empty string here, we should set the Error*
> inside the property setter.
> 
> > +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> 
> The QOM API does not allow to specify the length here.
> 
> That property is not added in this patch, so it was probably added by
> myself for the command line handling, where we could expect a sane
> input. Inside x86_def_t or X86CPUClass anything could be coded though,
> in theory. Admittedly the property output would already be broken then.
> 
> Regards,
> Andreas
> 
> >      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);
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index e4a7c50..983aab1 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> >  #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 */
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

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

[Igor Mammedov]

On Mon, 14 Jan 2013 20:32:12 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 11.01.2013 03:10, schrieb 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>
> 
> I have doubts about this patch as-is, maybe unfounded. It happens to
> work for the three vendors we have in-tree. But three numeric words
> would be well capable of respesenting a NUL char whereas the QOM
> property API has no concept of a string length != strlen(value) IIUC (no
> way to specify on setting and using g_strdup() internally).
> 
> Is there any validity guarantee documented in some Intel spec?

AMD's spec http://support.amd.com/us/Embedded_TechDocs/25481.pdf
just states that it's "AuthenticAMD" packed in ebx,edx,exc registers.

Intel's CPUID spec http://www.intel.com/Assets/PDF/appnote/241618.pdf
says:
"
5.1.1 ...
These registers contain the ASCII string: GenuineIntel
...
"

It's not spec but a list of known vendor values here
http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID
suggests that they all are 12 ASCII characters long, padded where necessary
with space.

I guess we can safely assume that it's 12 characters long ASCII string.

> 
> Another disadvantage I see is that each X86CPU subclass will have to use
> pstrcpy() to initialize the value - or we would have to go with three
> vendor base classes to hide this annoyance from each model. It still
> "happens" to look nice here due to C99 x86_def_t initialization.
If we have static properties before sub-classes and add field defval_str to
struct Property with associated handling [1] in qdev_property_add_static(),
then when vendor is converted to static property, we could keep C99
initialization in class_init's [2].

For example look at hack of cpu sub-classes that converts only qemu64 cpu
model using static properties for setting defaults:
Branch https://github.com/imammedo/qemu/commits/x86-cpu-classes.Jan142013

*1) qdev: extend DEFINE_GENERIC_PROP() to support default values
        https://github.com/imammedo/qemu/commit/e9fd18f308baffe0bde70a6710afa4ec41533a66

*2) target-i386: add helpers to change default values of static properties before object is created
        https://github.com/imammedo/qemu/commit/8b3080e53843a5fe63ef3e9733c8e98cb68758f2
    target-i386: declare subclass for qemu64 cpu model
        https://github.com/imammedo/qemu/commit/a48e252a2800bf8dd56320e68e4f9517d0a25e5c

if sub-classes would go before static properties then we could use pstrcpy() in
every sub-class temporally and then replace it with setting default property
value. Not sure if three vendor base classes would are worth effort to
eliminate one-liner from class_init()-s.

> 
> > ---
> > v3:
> >  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >    due to renaming of the last in previous patch
> >  - rebased with "target-i386: move out CPU features initialization
> >    in separate func" patch dropped
> > v2:
> >   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
> >      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c |  126 +++++++++++++----------------------------------------
> >  target-i386/cpu.h |    6 +-
> >  2 files changed, 33 insertions(+), 99 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 64cc88f..0b4fa57 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -353,7 +353,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;
> [...]
> > @@ -957,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
> >  
> >      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;
> > +    x86_cpu_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);
> > @@ -987,9 +937,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)) {
> 
> I'd be more comfortable doing such comparisons with a length or, even
> better, using a static inline helper doing so: In this case
> ("CentaurHauls") it happens to work but I'm worried about setting a bad
> example that gets copied-and-pasted.
x86_cpu_vendor_words2str() guaranties that x86_cpu_def->vendor is nill
terminated string, would you prefer:

 if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA, sizeof(x86_cpu_def->vendor))) 

instead?

> 
> >          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> >          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> >          if (eax >= 0xC0000001) {
> > @@ -1348,7 +1296,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 */
> >      FeatureWordArray plus_features = { 0 };
> > @@ -1410,18 +1357,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),
> 
> This one is fine by comparison.
> 
> > @@ -1616,10 +1552,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> >          goto out;
> >      }
> > -    assert(def->vendor1);
> > -    env->cpuid_vendor1 = def->vendor1;
> > -    env->cpuid_vendor2 = def->vendor2;
> > -    env->cpuid_vendor3 = def->vendor3;
> > +    assert(def->vendor[0]);
> 
> Instead of asserting on an empty string here, we should set the Error*
> inside the property setter.
x86_cpuid_set_vendor() you've added some time ago, already checks for value
to be 12 characters exactly. If it is not, it sets *Error
Reason I've kept assert here is that after patch vendor in this place would
represent built-in vendor value, so it would be easier to detect invalid
default value by aborting here (it wouldn't be valid for cpu sub-classes
though).
But this check is really redundant, would you like it to be dropped? 

> > +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> 
> The QOM API does not allow to specify the length here.
> 
> That property is not added in this patch, so it was probably added by
> myself for the command line handling, where we could expect a sane
> input. Inside x86_def_t or X86CPUClass anything could be coded though,
> in theory. Admittedly the property output would already be broken then.
Perhaps we could check in x86_cpuid_set_vendor() that value is ASCII string,
but it is unrelated to this patch and I'd put that in separate patch anyway.
 
> 
> Regards,
> Andreas
> 
[...]
> >  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH qom-cpu 00/17] x86 CPU cleanup, part 3

[Andreas Färber]

Am 11.01.2013 03:10, schrieb Igor Mammedov:
> Igor Mammedov (17):
>   target-i386: move setting defaults out of cpu_x86_parse_featurestr()
>   target-i386: cpu_x86_register() consolidate freeing resources
>   target-i386: move kvm_check_features_against_host() check to realize
>     time

Thanks, I've applied 1-3 to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

I really like patch 3, which moves more logic into our realizefn! When
the QOM realize series gets applied, I have a queue prepared already to
change today's signatures and to hook up to the infrastructure and to
make this uniform across targets.

>   target-i386: add x86_cpu_vendor_words2str()

Patch 4 v3 looks okay too, but it mentions "next patch" and doesn't
provide any value on its own:

>   target-i386: replace uint32_t vendor fields by vendor string in
>     x86_def_t

I raised some (non-)issues on patch 5 that I would like to give others a
chance to review rather than taking the lonely decision of putting this
in a PULL tonight; a new idea there would be a union { uint32_t
words[3]; char str[12]; } to keep both options open while avoiding the
string -> 3x uint32_t -> string mess that patch 5 rightfully attempts to
clean up. Once consensus is reached and it is still needed, I would
prefer to squash patch 4 as justification, being a rather trivial code
movement.

>   target-i386: remove vendor_override field from CPUX86State
>   target-i386: prepare cpu_x86_parse_featurestr() to return a set of
>     key,value property pairs

Didn't get around to fully reviewing the remainder of the series yet.

However I am still unclear and skeptic towards this patch using a QDict
to set properties on the CPU. I was told this was for -feat and +feat
backwards compatibility but the properties it is being used for are not
features but regular properties that I would like to always set to get
errors from each property rather than papering over, e.g.,
"...,tsc_freq=invalid,tsc_freq=1G,..." (IIUC). By operating on the
X86CPU, the intermediate x86_def_t can be spared too, as shown in my
subclasses patch. Maybe I'm overlooking something, needs calm review
from my side and some trial-and-error.

As a reminder, I have been pushing for converting to subclasses early
because that is a prerequisite for doing some CPUClass-level changes
across targets to clean up the way CPUs get created wrt cpu_init() and
cpu_copy(). Feature properties are, as far as I have seen and heard, an
x86-only project that could thus well be done on top of that common
infrastructure IMO.

Regards,
Andreas

>   target-i386: set custom 'vendor' without intermediate x86_def_t
>   target-i386: print deprecated 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: set custom 'tsc-frequency' without intermediate
>     x86_def_t
>   target-i386: remove setting tsc-frequency from x86_def_t
> 
>  target-i386/cpu.c |  314 ++++++++++++++++++++++-------------------------------
>  target-i386/cpu.h |    7 +-
>  2 files changed, 131 insertions(+), 190 deletions(-)

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

Re: [Qemu-devel] [PATCH qom-cpu 00/17] x86 CPU cleanup, part 3

[Igor Mammedov]

On Tue, 15 Jan 2013 05:16:52 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 11.01.2013 03:10, schrieb Igor Mammedov:
> > Igor Mammedov (17):
> >   target-i386: move setting defaults out of cpu_x86_parse_featurestr()
> >   target-i386: cpu_x86_register() consolidate freeing resources
> >   target-i386: move kvm_check_features_against_host() check to realize
> >     time
> 
> Thanks, I've applied 1-3 to qom-cpu:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu
> 
> I really like patch 3, which moves more logic into our realizefn! When
> the QOM realize series gets applied, I have a queue prepared already to
> change today's signatures and to hook up to the infrastructure and to
> make this uniform across targets.
> 
> >   target-i386: add x86_cpu_vendor_words2str()
> 
> Patch 4 v3 looks okay too, but it mentions "next patch" and doesn't
> provide any value on its own:
> 
> >   target-i386: replace uint32_t vendor fields by vendor string in
> >     x86_def_t
> 
> I raised some (non-)issues on patch 5 that I would like to give others a
> chance to review rather than taking the lonely decision of putting this
> in a PULL tonight; a new idea there would be a union { uint32_t
> words[3]; char str[12]; } to keep both options open while avoiding the
> string -> 3x uint32_t -> string mess that patch 5 rightfully attempts to
> clean up. Once consensus is reached and it is still needed, I would
> prefer to squash patch 4 as justification, being a rather trivial code
> movement.
I'll try it.

> 
> >   target-i386: remove vendor_override field from CPUX86State
> >   target-i386: prepare cpu_x86_parse_featurestr() to return a set of
> >     key,value property pairs
> 
> Didn't get around to fully reviewing the remainder of the series yet.
Remainder of series is gradual conversion of currently existing properties
to foo,val pairs that set later by x86_cpu_set_props(). And following
static properties series will convert non property-fied features into
properties following the same pattern.

> 
> However I am still unclear and skeptic towards this patch using a QDict
> to set properties on the CPU. I was told this was for -feat and +feat
> backwards compatibility but the properties it is being used for are not
> features but regular properties that I would like to always set to get
> errors from each property rather than papering over, e.g.,
> "...,tsc_freq=invalid,tsc_freq=1G,..." (IIUC).
-feat and +feat is the only reason[1] to use dictionary as temporary storage
foo=val values, It could be hidden inside of cpu_x86_parse_featurestr()
and QList could be returned instead of it. That would achieve processing
of tsc_freq=invalid,tsc_freq=1G in sequence when setting properties and
failing on the first invalid tsc_freq or another similar case.

> By operating on the
> X86CPU, the intermediate x86_def_t can be spared too, as shown in my
> subclasses patch. Maybe I'm overlooking something, needs calm review
> from my side and some trial-and-error.

But main point of patch 7 is to separate parsing logic of command line
features (some of them legacy naming) from setting properties on actual
CPU instance.
Later when sub-classes are implemented and all features are converted into
static properties, x86_cpu_set_props() would be merged into
cpu_x86_parse_featurestr() and set global properties instead of operating
on CPU instance directly. After that cpu_x86_parse_featurestr() could be
called only once to set global properties for a given CPU type instead of
parsing the same featurestr for every CPU.

I could merge x86_cpu_set_props() into cpu_x86_parse_featurestr() and pass
cpu instance inside it temporally but I'd insist on first normalizing
featurestr into list of property pairs and then applying them to CPU.
It makes conversion to global properties trivial.

> 
> As a reminder, I have been pushing for converting to subclasses early
> because that is a prerequisite for doing some CPUClass-level changes
> across targets to clean up the way CPUs get created wrt cpu_init() and
> cpu_copy(). Feature properties are, as far as I have seen and heard, an
> x86-only project that could thus well be done on top of that common
> infrastructure IMO.
properties is x86-only project for now, but they make conversion to
sub-classes simpler and actually make x86 sub-classes meaningful.

We could push hard for properties and sub-classes to make in 1.4.


> 
> Regards,
> Andreas
> 
> >   target-i386: set custom 'vendor' without intermediate x86_def_t
> >   target-i386: print deprecated 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: set custom 'tsc-frequency' without intermediate
> >     x86_def_t
> >   target-i386: remove setting tsc-frequency from x86_def_t
> > 
> >  target-i386/cpu.c |  314 ++++++++++++++++++++++-------------------------------
> >  target-i386/cpu.h |    7 +-
> >  2 files changed, 131 insertions(+), 190 deletions(-)
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Mon, Jan 14, 2013 at 10:52:52PM +0100, Igor Mammedov wrote:
[...]
> > > @@ -987,9 +937,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)) {
> > 
> > I'd be more comfortable doing such comparisons with a length or, even
> > better, using a static inline helper doing so: In this case
> > ("CentaurHauls") it happens to work but I'm worried about setting a bad
> > example that gets copied-and-pasted.
> x86_cpu_vendor_words2str() guaranties that x86_cpu_def->vendor is nill
> terminated string, would you prefer:
> 
>  if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA, sizeof(x86_cpu_def->vendor))) 
> 
> instead?

I believe Andreas is worrying in case there are NUL characters in the
middle of the vendor info. e.g., this wouldn't work:

  #define CPUID_VENDOR_FOOBAR "foo\0bar\0baz\0".
  [...]
  if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_FOOBAR, sizeof(x86_cpu_def->vendor))) 
      [...]

It's true that CPU vendors can do whatever they want and return anything
on the CPUID vendor leaf, but I believe this is never going to happen
because vendors know it would be a bad idea to do that. And if it
happens one day, we can easily add additional vendor1/vendor2/vendor3
properties to allow low-level setting of the vendor ID bytes.

[...]
> > > @@ -1616,10 +1552,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> > >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> > >          goto out;
> > >      }
> > > -    assert(def->vendor1);
> > > -    env->cpuid_vendor1 = def->vendor1;
> > > -    env->cpuid_vendor2 = def->vendor2;
> > > -    env->cpuid_vendor3 = def->vendor3;
> > > +    assert(def->vendor[0]);
> > 
> > Instead of asserting on an empty string here, we should set the Error*
> > inside the property setter.
> x86_cpuid_set_vendor() you've added some time ago, already checks for value
> to be 12 characters exactly. If it is not, it sets *Error
> Reason I've kept assert here is that after patch vendor in this place would
> represent built-in vendor value, so it would be easier to detect invalid
> default value by aborting here (it wouldn't be valid for cpu sub-classes
> though).
> But this check is really redundant, would you like it to be dropped? 

Well, every assert is _supposed_ to be redundant, right? I mean: we only
add assert()s to the code when we are already confident that other parts
of the code make sure the assert is never going to fail. I would like to
keep it.

-- 
Eduardo

[Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State

[Igor Mammedov]

Tested on x86 host yet, I don't have bigendian host avilable right now.

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.

Extra:
  - replace cpuid_vendor[1.2.3] words in CPUX86State with union
    to simplify vendor property setter and pack/unpack procedures
  - add x86_cpu_vendor_words2str() to make for() cycle reusable
  - convert words in cpuid_vendor union to little-endian when
    returning them to guest in cpuid instruction emulation, since
    they are not packed manualy anymore

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
 - use union for cpuid_vendor to simplify convertions string<=>registers
v3:
 - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
   due to renaming of the last in previous patch
 - rebased with "target-i386: move out CPU features initialization
   in separate func" patch dropped
v2:
  - restore deleted host_cpuid() call in kvm_cpu_fill_host()
     Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c       |  180 ++++++++++++++---------------------------------
 target-i386/cpu.h       |   17 +++--
 target-i386/translate.c |    4 +-
 3 files changed, 67 insertions(+), 134 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 333745b..882da50 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -45,6 +45,18 @@
 #include "hw/apic_internal.h"
 #endif
 
+static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                                     uint32_t vendor2, uint32_t vendor3)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = vendor1 >> (8 * i);
+        dst[i + 4] = vendor2 >> (8 * i);
+        dst[i + 8] = vendor3 >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -341,7 +353,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;
@@ -406,9 +418,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,
@@ -425,9 +435,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,
@@ -453,9 +461,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,
@@ -474,9 +480,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,
@@ -500,9 +504,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,
@@ -513,9 +515,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,
@@ -530,9 +530,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,
@@ -548,9 +546,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,
@@ -560,9 +556,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,
@@ -572,9 +566,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,
@@ -584,9 +576,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,
@@ -596,9 +586,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,
@@ -612,9 +600,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,
@@ -633,9 +619,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,
@@ -653,9 +637,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,
@@ -674,9 +656,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,
@@ -695,9 +675,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,
@@ -717,9 +695,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,
@@ -742,9 +718,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,
@@ -772,9 +746,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,
@@ -796,9 +768,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,
@@ -822,9 +792,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,
@@ -850,9 +818,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,
@@ -882,9 +848,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,
@@ -945,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     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;
+    x86_cpu_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);
@@ -975,9 +937,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) {
@@ -1213,15 +1173,9 @@ 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';
+    pstrcpy(value, CPUID_VENDOR_SZ + 1, env->cpuid_vendor.str);
     return value;
 }
 
@@ -1230,7 +1184,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    int i;
 
     if (strlen(value) != CPUID_VENDOR_SZ) {
         error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
@@ -1238,14 +1191,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
         return;
     }
 
-    env->cpuid_vendor1 = 0;
-    env->cpuid_vendor2 = 0;
-    env->cpuid_vendor3 = 0;
-    for (i = 0; i < 4; i++) {
-        env->cpuid_vendor1 |= ((uint8_t)value[i    ]) << (8 * i);
-        env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
-        env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
-    }
+    pstrcpy(env->cpuid_vendor.str, sizeof(env->cpuid_vendor.str), value);
     env->cpuid_vendor_override = 1;
 }
 
@@ -1341,7 +1287,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 */
     FeatureWordArray plus_features = { 0 };
@@ -1403,18 +1348,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),
@@ -1609,10 +1543,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
         goto out;
     }
-    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", &error);
     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);
@@ -1682,9 +1614,9 @@ void x86_cpudef_setup(void)
 static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
                              uint32_t *ecx, uint32_t *edx)
 {
-    *ebx = env->cpuid_vendor1;
-    *edx = env->cpuid_vendor2;
-    *ecx = env->cpuid_vendor3;
+    cpu_to_le32wu(ebx, env->cpuid_vendor.regs.ebx);
+    cpu_to_le32wu(edx, env->cpuid_vendor.regs.edx);
+    cpu_to_le32wu(ecx, env->cpuid_vendor.regs.ecx);
 
     /* sysenter isn't supported on compatibility mode on AMD, syscall
      * isn't supported in compatibility mode on Intel.
@@ -1862,9 +1794,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 0x80000000:
         *eax = env->cpuid_xlevel;
-        *ebx = env->cpuid_vendor1;
-        *edx = env->cpuid_vendor2;
-        *ecx = env->cpuid_vendor3;
+        *ebx = env->cpuid_vendor.regs.ebx;
+        *edx = env->cpuid_vendor.regs.edx;
+        *ecx = env->cpuid_vendor.regs.ecx;
         break;
     case 0x80000001:
         *eax = env->cpuid_version;
@@ -1877,11 +1809,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
          * So dont set it here for Intel to make Linux guests happy.
          */
         if (cs->nr_cores * cs->nr_threads > 1) {
-            uint32_t tebx, tecx, tedx;
-            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
-            if (tebx != CPUID_VENDOR_INTEL_1 ||
-                tedx != CPUID_VENDOR_INTEL_2 ||
-                tecx != CPUID_VENDOR_INTEL_3) {
+            if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_INTEL,
+                         sizeof(env->cpuid_vendor.str))) {
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
@@ -2152,9 +2081,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
     /* 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) {
+    if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_AMD,
+                 sizeof(env->cpuid_vendor.str))) {
         env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
         env->cpuid_ext2_features |= (env->cpuid_features
            & CPUID_EXT2_AMD_ALIASES);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e4a7c50..09a3b18 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #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 */
@@ -818,9 +818,14 @@ typedef struct CPUX86State {
 
     /* processor features (e.g. for CPUID insn) */
     uint32_t cpuid_level;
-    uint32_t cpuid_vendor1;
-    uint32_t cpuid_vendor2;
-    uint32_t cpuid_vendor3;
+    union {
+        struct __attribute__((packed)) {
+            uint32_t ebx;
+            uint32_t edx;
+            uint32_t ecx;
+        } regs;
+        char str[CPUID_VENDOR_SZ + 1];
+    } cpuid_vendor;
     uint32_t cpuid_version;
     uint32_t cpuid_features;
     uint32_t cpuid_ext_features;
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 32d21f5..985080b 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7028,7 +7028,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         break;
     case 0x134: /* sysenter */
         /* For Intel SYSENTER is valid on 64-bit */
-        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
+        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)
             goto illegal_op;
         if (!s->pe) {
             gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
@@ -7041,7 +7041,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         break;
     case 0x135: /* sysexit */
         /* For Intel SYSEXIT is valid on 64-bit */
-        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
+        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)
             goto illegal_op;
         if (!s->pe) {
             gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State

[Eduardo Habkost]

On Tue, Jan 15, 2013 at 01:06:28PM +0100, Igor Mammedov wrote:
> Tested on x86 host yet, I don't have bigendian host avilable right now.
> 
> 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.
> 
> Extra:
>   - replace cpuid_vendor[1.2.3] words in CPUX86State with union
>     to simplify vendor property setter and pack/unpack procedures
>   - add x86_cpu_vendor_words2str() to make for() cycle reusable
>   - convert words in cpuid_vendor union to little-endian when
>     returning them to guest in cpuid instruction emulation, since
>     they are not packed manualy anymore

What about all other cases where CPUID_VENDOR_*_[123] constants are
used?

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>  - use union for cpuid_vendor to simplify convertions string<=>registers
> v3:
>  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>    due to renaming of the last in previous patch
>  - rebased with "target-i386: move out CPU features initialization
>    in separate func" patch dropped
> v2:
>   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
>      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c       |  180 ++++++++++++++---------------------------------
>  target-i386/cpu.h       |   17 +++--
>  target-i386/translate.c |    4 +-
>  3 files changed, 67 insertions(+), 134 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 333745b..882da50 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -45,6 +45,18 @@
>  #include "hw/apic_internal.h"
>  #endif
>  
> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> +                                     uint32_t vendor2, uint32_t vendor3)
> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = vendor1 >> (8 * i);
> +        dst[i + 4] = vendor2 >> (8 * i);
> +        dst[i + 8] = vendor3 >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}

Why this function still exists, if we have the union?

Anyway: even with the union, we can't avoid having explicit conversion
code because of endianness. It doesn't matter which option we choose:

- if we store vendor[123], we need to convert to string on the vendor
  property getter/setter;
- if we store vendor string, we need to convert to register values on
  cpu_x86_cpuid();
- if we store it in an union we need endianness conversion code (inline
  or in a function) every time we read vendor[123].

That said, I don't see the point of the union.

> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -341,7 +353,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;
> @@ -406,9 +418,7 @@ static x86_def_t builtin_x86_defs[] = {
[...]
> @@ -882,9 +848,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,
> @@ -945,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>  
>      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;
> +    x86_cpu_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);
> @@ -975,9 +937,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) {
> @@ -1213,15 +1173,9 @@ 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';
> +    pstrcpy(value, CPUID_VENDOR_SZ + 1, env->cpuid_vendor.str);
>      return value;
>  }
>  
> @@ -1230,7 +1184,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    int i;
>  
>      if (strlen(value) != CPUID_VENDOR_SZ) {
>          error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
> @@ -1238,14 +1191,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>          return;
>      }
>  
> -    env->cpuid_vendor1 = 0;
> -    env->cpuid_vendor2 = 0;
> -    env->cpuid_vendor3 = 0;
> -    for (i = 0; i < 4; i++) {
> -        env->cpuid_vendor1 |= ((uint8_t)value[i    ]) << (8 * i);
> -        env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> -        env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> -    }
> +    pstrcpy(env->cpuid_vendor.str, sizeof(env->cpuid_vendor.str), value);
>      env->cpuid_vendor_override = 1;
>  }
>  
> @@ -1341,7 +1287,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 */
>      FeatureWordArray plus_features = { 0 };
> @@ -1403,18 +1348,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),
> @@ -1609,10 +1543,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
>          goto out;
>      }
> -    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", &error);
>      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);
> @@ -1682,9 +1614,9 @@ void x86_cpudef_setup(void)
>  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>                               uint32_t *ecx, uint32_t *edx)
>  {
> -    *ebx = env->cpuid_vendor1;
> -    *edx = env->cpuid_vendor2;
> -    *ecx = env->cpuid_vendor3;
> +    cpu_to_le32wu(ebx, env->cpuid_vendor.regs.ebx);
> +    cpu_to_le32wu(edx, env->cpuid_vendor.regs.edx);
> +    cpu_to_le32wu(ecx, env->cpuid_vendor.regs.ecx);
>  
>      /* sysenter isn't supported on compatibility mode on AMD, syscall
>       * isn't supported in compatibility mode on Intel.
> @@ -1862,9 +1794,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x80000000:
>          *eax = env->cpuid_xlevel;
> -        *ebx = env->cpuid_vendor1;
> -        *edx = env->cpuid_vendor2;
> -        *ecx = env->cpuid_vendor3;
> +        *ebx = env->cpuid_vendor.regs.ebx;
> +        *edx = env->cpuid_vendor.regs.edx;
> +        *ecx = env->cpuid_vendor.regs.ecx;

Don't we need endianness conversion here as well?

>          break;
>      case 0x80000001:
>          *eax = env->cpuid_version;
> @@ -1877,11 +1809,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           * So dont set it here for Intel to make Linux guests happy.
>           */
>          if (cs->nr_cores * cs->nr_threads > 1) {
> -            uint32_t tebx, tecx, tedx;
> -            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
> -            if (tebx != CPUID_VENDOR_INTEL_1 ||
> -                tedx != CPUID_VENDOR_INTEL_2 ||
> -                tecx != CPUID_VENDOR_INTEL_3) {
> +            if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_INTEL,
> +                         sizeof(env->cpuid_vendor.str))) {
>                  *ecx |= 1 << 1;    /* CmpLegacy bit */
>              }
>          }
> @@ -2152,9 +2081,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
>      /* 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) {
> +    if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_AMD,
> +                 sizeof(env->cpuid_vendor.str))) {
>          env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
>          env->cpuid_ext2_features |= (env->cpuid_features
>             & CPUID_EXT2_AMD_ALIASES);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index e4a7c50..09a3b18 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #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 */
> @@ -818,9 +818,14 @@ typedef struct CPUX86State {
>  
>      /* processor features (e.g. for CPUID insn) */
>      uint32_t cpuid_level;
> -    uint32_t cpuid_vendor1;
> -    uint32_t cpuid_vendor2;
> -    uint32_t cpuid_vendor3;
> +    union {
> +        struct __attribute__((packed)) {
> +            uint32_t ebx;
> +            uint32_t edx;
> +            uint32_t ecx;
> +        } regs;
> +        char str[CPUID_VENDOR_SZ + 1];
> +    } cpuid_vendor;
>      uint32_t cpuid_version;
>      uint32_t cpuid_features;
>      uint32_t cpuid_ext_features;
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 32d21f5..985080b 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7028,7 +7028,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>          break;
>      case 0x134: /* sysenter */
>          /* For Intel SYSENTER is valid on 64-bit */
> -        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
> +        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)

This needs endianness conversion as well.

>              goto illegal_op;
>          if (!s->pe) {
>              gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> @@ -7041,7 +7041,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>          break;
>      case 0x135: /* sysexit */
>          /* For Intel SYSEXIT is valid on 64-bit */
> -        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
> +        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)

Ditto.

>              goto illegal_op;
>          if (!s->pe) {
>              gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State

[Igor Mammedov]

On Tue, 15 Jan 2013 15:51:18 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jan 15, 2013 at 01:06:28PM +0100, Igor Mammedov wrote:
[..]
> >   - replace cpuid_vendor[1.2.3] words in CPUX86State with union
> >     to simplify vendor property setter and pack/unpack procedures
> >   - add x86_cpu_vendor_words2str() to make for() cycle reusable
> >   - convert words in cpuid_vendor union to little-endian when
> >     returning them to guest in cpuid instruction emulation, since
> >     they are not packed manualy anymore
> 
> What about all other cases where CPUID_VENDOR_*_[123] constants are
> used?
NACK patch, it is probably horribly broken on big-endian acrh.

> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> >  - use union for cpuid_vendor to simplify convertions string<=>registers
> > v3:
> >  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >    due to renaming of the last in previous patch
> >  - rebased with "target-i386: move out CPU features initialization
> >    in separate func" patch dropped
> > v2:
> >   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
> >      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c       |  180 ++++++++++++++---------------------------------
> >  target-i386/cpu.h       |   17 +++--
> >  target-i386/translate.c |    4 +-
> >  3 files changed, 67 insertions(+), 134 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..882da50 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -45,6 +45,18 @@
> >  #include "hw/apic_internal.h"
> >  #endif
> >  
> > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > +                                     uint32_t vendor2, uint32_t vendor3)
> > +{
> > +    int i;
> > +    for (i = 0; i < 4; i++) {
> > +        dst[i] = vendor1 >> (8 * i);
> > +        dst[i + 4] = vendor2 >> (8 * i);
> > +        dst[i + 8] = vendor3 >> (8 * i);
> > +    }
> > +    dst[CPUID_VENDOR_SZ] = '\0';
> > +}
> 
> Why this function still exists, if we have the union?
to copy words from host_cpuid(0, 0, NULL, &ebx, &ecx, &edx); to
x86_cpu_def->vendor when vendor_override is removed.
but never mind.

> 
> Anyway: even with the union, we can't avoid having explicit conversion
> code because of endianness. It doesn't matter which option we choose:
> 
> - if we store vendor[123], we need to convert to string on the vendor
>   property getter/setter;
> - if we store vendor string, we need to convert to register values on
>   cpu_x86_cpuid();
> - if we store it in an union we need endianness conversion code (inline
>   or in a function) every time we read vendor[123].
> 
> That said, I don't see the point of the union.
Agreed, union patch gets more intrusive as we would need to care about
endiannes in every place words are consumed.

Original patch looks much less intrusive and touches only words=>string
conversion. +1 in favor of original patch.

> 
[...]
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 04/17 v3] target-i386: add x86_cpu_vendor_words2str()

[li guang]

Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.com>

在 2013-01-14一的 19:33 +0100，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>
> ---
>   v5:
>    - style fix: added extra space to align arguments
>         Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
>         Spotted-By: Andreas Färber <afaerber@suse.de>
>   v4:
>    - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str()
>         Suggested-By: Andreas Färber <afaerber@suse.de>
>   v3:
>    -replace e[bcd]x arguments naming with vendor[123]
>    -fix/swap vendor2 and vendor3 order
>         Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
>   v2:
>    -place x86cpu_vendor_words2str() a bit earlier, before feature
>     arrays to avoid compile error when vendor property is converted
>     static property.
> ---
>  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 333745b..e9aebda 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -45,6 +45,18 @@
>  #include "hw/apic_internal.h"
>  #endif
>  
> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> +                                     uint32_t vendor2, uint32_t vendor3)
> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = vendor1 >> (8 * i);
> +        dst[i + 4] = vendor2 >> (8 * i);
> +        dst[i + 8] = vendor3 >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}
> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -1213,15 +1225,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';
> +    x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
> +                             env->cpuid_vendor3);
>      return value;
>  }
>  

-- 
regards!
li guang

Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State

[li guang]

在 2013-01-15二的 13:06 +0100，Igor Mammedov写道：
> Tested on x86 host yet, I don't have bigendian host avilable right now.
> 
> 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.
> 
> Extra:
>   - replace cpuid_vendor[1.2.3] words in CPUX86State with union
>     to simplify vendor property setter and pack/unpack procedures
>   - add x86_cpu_vendor_words2str() to make for() cycle reusable
>   - convert words in cpuid_vendor union to little-endian when
>     returning them to guest in cpuid instruction emulation, since
>     they are not packed manualy anymore
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>  - use union for cpuid_vendor to simplify convertions string<=>registers
> v3:
>  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
>    due to renaming of the last in previous patch
>  - rebased with "target-i386: move out CPU features initialization
>    in separate func" patch dropped
> v2:
>   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
>      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c       |  180 ++++++++++++++---------------------------------
>  target-i386/cpu.h       |   17 +++--
>  target-i386/translate.c |    4 +-
>  3 files changed, 67 insertions(+), 134 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 333745b..882da50 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -45,6 +45,18 @@
>  #include "hw/apic_internal.h"
>  #endif
>  
> +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> +                                     uint32_t vendor2, uint32_t vendor3)
> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = vendor1 >> (8 * i);
> +        dst[i + 4] = vendor2 >> (8 * i);
> +        dst[i + 8] = vendor3 >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}
> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -341,7 +353,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;
> @@ -406,9 +418,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,
> @@ -425,9 +435,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,
> @@ -453,9 +461,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,
> @@ -474,9 +480,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,
> @@ -500,9 +504,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,
> @@ -513,9 +515,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,
> @@ -530,9 +530,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,
> @@ -548,9 +546,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,
> @@ -560,9 +556,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,
> @@ -572,9 +566,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,
> @@ -584,9 +576,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,
> @@ -596,9 +586,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,
> @@ -612,9 +600,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,
> @@ -633,9 +619,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,
> @@ -653,9 +637,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,
> @@ -674,9 +656,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,
> @@ -695,9 +675,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,
> @@ -717,9 +695,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,
> @@ -742,9 +718,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,
> @@ -772,9 +746,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,
> @@ -796,9 +768,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,
> @@ -822,9 +792,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,
> @@ -850,9 +818,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,
> @@ -882,9 +848,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,
> @@ -945,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>  
>      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;
> +    x86_cpu_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);
> @@ -975,9 +937,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) {
> @@ -1213,15 +1173,9 @@ 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';
> +    pstrcpy(value, CPUID_VENDOR_SZ + 1, env->cpuid_vendor.str);
>      return value;
>  }
>  
> @@ -1230,7 +1184,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    int i;
>  
>      if (strlen(value) != CPUID_VENDOR_SZ) {
>          error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
> @@ -1238,14 +1191,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>          return;
>      }
>  
> -    env->cpuid_vendor1 = 0;
> -    env->cpuid_vendor2 = 0;
> -    env->cpuid_vendor3 = 0;
> -    for (i = 0; i < 4; i++) {
> -        env->cpuid_vendor1 |= ((uint8_t)value[i    ]) << (8 * i);
> -        env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> -        env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> -    }
> +    pstrcpy(env->cpuid_vendor.str, sizeof(env->cpuid_vendor.str), value);
>      env->cpuid_vendor_override = 1;
>  }
>  
> @@ -1341,7 +1287,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 */
>      FeatureWordArray plus_features = { 0 };
> @@ -1403,18 +1348,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),
> @@ -1609,10 +1543,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
>          goto out;
>      }
> -    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", &error);
>      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);
> @@ -1682,9 +1614,9 @@ void x86_cpudef_setup(void)
>  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>                               uint32_t *ecx, uint32_t *edx)
>  {
> -    *ebx = env->cpuid_vendor1;
> -    *edx = env->cpuid_vendor2;
> -    *ecx = env->cpuid_vendor3;
> +    cpu_to_le32wu(ebx, env->cpuid_vendor.regs.ebx);
> +    cpu_to_le32wu(edx, env->cpuid_vendor.regs.edx);
> +    cpu_to_le32wu(ecx, env->cpuid_vendor.regs.ecx);
>  
>      /* sysenter isn't supported on compatibility mode on AMD, syscall
>       * isn't supported in compatibility mode on Intel.
> @@ -1862,9 +1794,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x80000000:
>          *eax = env->cpuid_xlevel;
> -        *ebx = env->cpuid_vendor1;
> -        *edx = env->cpuid_vendor2;
> -        *ecx = env->cpuid_vendor3;
> +        *ebx = env->cpuid_vendor.regs.ebx;
> +        *edx = env->cpuid_vendor.regs.edx;
> +        *ecx = env->cpuid_vendor.regs.ecx;
>          break;
>      case 0x80000001:
>          *eax = env->cpuid_version;
> @@ -1877,11 +1809,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           * So dont set it here for Intel to make Linux guests happy.
>           */
>          if (cs->nr_cores * cs->nr_threads > 1) {
> -            uint32_t tebx, tecx, tedx;
> -            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
> -            if (tebx != CPUID_VENDOR_INTEL_1 ||
> -                tedx != CPUID_VENDOR_INTEL_2 ||
> -                tecx != CPUID_VENDOR_INTEL_3) {
> +            if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_INTEL,
> +                         sizeof(env->cpuid_vendor.str))) {
>                  *ecx |= 1 << 1;    /* CmpLegacy bit */
>              }
>          }
> @@ -2152,9 +2081,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
>      /* 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) {
> +    if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_AMD,
> +                 sizeof(env->cpuid_vendor.str))) {
>          env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
>          env->cpuid_ext2_features |= (env->cpuid_features
>             & CPUID_EXT2_AMD_ALIASES);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index e4a7c50..09a3b18 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #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"

why not also remove these 2 VENDOR_{INTEL,AMD}_{1,2,3} ?

>  
> -#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 */
> @@ -818,9 +818,14 @@ typedef struct CPUX86State {
>  
>      /* processor features (e.g. for CPUID insn) */
>      uint32_t cpuid_level;
> -    uint32_t cpuid_vendor1;
> -    uint32_t cpuid_vendor2;
> -    uint32_t cpuid_vendor3;
> +    union {
> +        struct __attribute__((packed)) {
> +            uint32_t ebx;
> +            uint32_t edx;
> +            uint32_t ecx;
> +        } regs;
> +        char str[CPUID_VENDOR_SZ + 1];
> +    } cpuid_vendor;
>      uint32_t cpuid_version;
>      uint32_t cpuid_features;
>      uint32_t cpuid_ext_features;
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 32d21f5..985080b 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7028,7 +7028,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>          break;
>      case 0x134: /* sysenter */
>          /* For Intel SYSENTER is valid on 64-bit */
> -        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
> +        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)
>              goto illegal_op;
>          if (!s->pe) {
>              gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> @@ -7041,7 +7041,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>          break;
>      case 0x135: /* sysexit */
>          /* For Intel SYSEXIT is valid on 64-bit */
> -        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
> +        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)
>              goto illegal_op;
>          if (!s->pe) {
>              gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);

-- 
regards!
li guang

Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State

[Igor Mammedov]

On Wed, 16 Jan 2013 15:07:05 +0800
li guang <lig.fnst@cn.fujitsu.com> wrote:

> 在 2013-01-15二的 13:06 +0100，Igor Mammedov写道：
> > Tested on x86 host yet, I don't have bigendian host avilable right now.
> > 
> > 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.
> > 
> > Extra:
> >   - replace cpuid_vendor[1.2.3] words in CPUX86State with union
> >     to simplify vendor property setter and pack/unpack procedures
> >   - add x86_cpu_vendor_words2str() to make for() cycle reusable
> >   - convert words in cpuid_vendor union to little-endian when
> >     returning them to guest in cpuid instruction emulation, since
> >     they are not packed manualy anymore
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> >  - use union for cpuid_vendor to simplify convertions string<=>registers
> > v3:
> >  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >    due to renaming of the last in previous patch
> >  - rebased with "target-i386: move out CPU features initialization
> >    in separate func" patch dropped
> > v2:
> >   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
> >      Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c       |  180 ++++++++++++++---------------------------------
> >  target-i386/cpu.h       |   17 +++--
> >  target-i386/translate.c |    4 +-
> >  3 files changed, 67 insertions(+), 134 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..882da50 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -45,6 +45,18 @@
> >  #include "hw/apic_internal.h"
> >  #endif
> >  
> > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > +                                     uint32_t vendor2, uint32_t vendor3)
> > +{
> > +    int i;
> > +    for (i = 0; i < 4; i++) {
> > +        dst[i] = vendor1 >> (8 * i);
> > +        dst[i + 4] = vendor2 >> (8 * i);
> > +        dst[i + 8] = vendor3 >> (8 * i);
> > +    }
> > +    dst[CPUID_VENDOR_SZ] = '\0';
> > +}
> > +
> >  /* feature flags taken from "Intel Processor Identification and the CPUID
> >   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
> >   * between feature naming conventions, aliases may be added.
> > @@ -341,7 +353,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;
> > @@ -406,9 +418,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,
> > @@ -425,9 +435,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,
> > @@ -453,9 +461,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,
> > @@ -474,9 +480,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,
> > @@ -500,9 +504,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,
> > @@ -513,9 +515,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,
> > @@ -530,9 +530,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,
> > @@ -548,9 +546,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,
> > @@ -560,9 +556,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,
> > @@ -572,9 +566,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,
> > @@ -584,9 +576,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,
> > @@ -596,9 +586,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,
> > @@ -612,9 +600,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,
> > @@ -633,9 +619,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,
> > @@ -653,9 +637,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,
> > @@ -674,9 +656,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,
> > @@ -695,9 +675,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,
> > @@ -717,9 +695,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,
> > @@ -742,9 +718,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,
> > @@ -772,9 +746,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,
> > @@ -796,9 +768,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,
> > @@ -822,9 +792,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,
> > @@ -850,9 +818,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,
> > @@ -882,9 +848,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,
> > @@ -945,9 +909,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
> >  
> >      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;
> > +    x86_cpu_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);
> > @@ -975,9 +937,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) {
> > @@ -1213,15 +1173,9 @@ 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';
> > +    pstrcpy(value, CPUID_VENDOR_SZ + 1, env->cpuid_vendor.str);
> >      return value;
> >  }
> >  
> > @@ -1230,7 +1184,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> >  {
> >      X86CPU *cpu = X86_CPU(obj);
> >      CPUX86State *env = &cpu->env;
> > -    int i;
> >  
> >      if (strlen(value) != CPUID_VENDOR_SZ) {
> >          error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
> > @@ -1238,14 +1191,7 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> >          return;
> >      }
> >  
> > -    env->cpuid_vendor1 = 0;
> > -    env->cpuid_vendor2 = 0;
> > -    env->cpuid_vendor3 = 0;
> > -    for (i = 0; i < 4; i++) {
> > -        env->cpuid_vendor1 |= ((uint8_t)value[i    ]) << (8 * i);
> > -        env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> > -        env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> > -    }
> > +    pstrcpy(env->cpuid_vendor.str, sizeof(env->cpuid_vendor.str), value);
> >      env->cpuid_vendor_override = 1;
> >  }
> >  
> > @@ -1341,7 +1287,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 */
> >      FeatureWordArray plus_features = { 0 };
> > @@ -1403,18 +1348,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),
> > @@ -1609,10 +1543,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> >          goto out;
> >      }
> > -    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", &error);
> >      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);
> > @@ -1682,9 +1614,9 @@ void x86_cpudef_setup(void)
> >  static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
> >                               uint32_t *ecx, uint32_t *edx)
> >  {
> > -    *ebx = env->cpuid_vendor1;
> > -    *edx = env->cpuid_vendor2;
> > -    *ecx = env->cpuid_vendor3;
> > +    cpu_to_le32wu(ebx, env->cpuid_vendor.regs.ebx);
> > +    cpu_to_le32wu(edx, env->cpuid_vendor.regs.edx);
> > +    cpu_to_le32wu(ecx, env->cpuid_vendor.regs.ecx);
> >  
> >      /* sysenter isn't supported on compatibility mode on AMD, syscall
> >       * isn't supported in compatibility mode on Intel.
> > @@ -1862,9 +1794,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >          break;
> >      case 0x80000000:
> >          *eax = env->cpuid_xlevel;
> > -        *ebx = env->cpuid_vendor1;
> > -        *edx = env->cpuid_vendor2;
> > -        *ecx = env->cpuid_vendor3;
> > +        *ebx = env->cpuid_vendor.regs.ebx;
> > +        *edx = env->cpuid_vendor.regs.edx;
> > +        *ecx = env->cpuid_vendor.regs.ecx;
> >          break;
> >      case 0x80000001:
> >          *eax = env->cpuid_version;
> > @@ -1877,11 +1809,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >           * So dont set it here for Intel to make Linux guests happy.
> >           */
> >          if (cs->nr_cores * cs->nr_threads > 1) {
> > -            uint32_t tebx, tecx, tedx;
> > -            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
> > -            if (tebx != CPUID_VENDOR_INTEL_1 ||
> > -                tedx != CPUID_VENDOR_INTEL_2 ||
> > -                tecx != CPUID_VENDOR_INTEL_3) {
> > +            if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_INTEL,
> > +                         sizeof(env->cpuid_vendor.str))) {
> >                  *ecx |= 1 << 1;    /* CmpLegacy bit */
> >              }
> >          }
> > @@ -2152,9 +2081,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >      /* 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) {
> > +    if (!strncmp(env->cpuid_vendor.str, CPUID_VENDOR_AMD,
> > +                 sizeof(env->cpuid_vendor.str))) {
> >          env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> >          env->cpuid_ext2_features |= (env->cpuid_features
> >             & CPUID_EXT2_AMD_ALIASES);
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index e4a7c50..09a3b18 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -531,14 +531,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> >  #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"
> 
> why not also remove these 2 VENDOR_{INTEL,AMD}_{1,2,3} ?
Indeed with this patch VENDOR_AMD_{1,2,3} unused, and could be deleted,
but CPUID_VENDOR_INTEL_{1,2,3} is used elsewhere.
This said, this patch looks more intrusive, could you consider to review
original patch:

"[PATCH 05/17] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t"

instead in this series, pls?

> 
> >  
> > -#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 */
> > @@ -818,9 +818,14 @@ typedef struct CPUX86State {
> >  
> >      /* processor features (e.g. for CPUID insn) */
> >      uint32_t cpuid_level;
> > -    uint32_t cpuid_vendor1;
> > -    uint32_t cpuid_vendor2;
> > -    uint32_t cpuid_vendor3;
> > +    union {
> > +        struct __attribute__((packed)) {
> > +            uint32_t ebx;
> > +            uint32_t edx;
> > +            uint32_t ecx;
> > +        } regs;
> > +        char str[CPUID_VENDOR_SZ + 1];
> > +    } cpuid_vendor;
> >      uint32_t cpuid_version;
> >      uint32_t cpuid_features;
> >      uint32_t cpuid_ext_features;
> > diff --git a/target-i386/translate.c b/target-i386/translate.c
> > index 32d21f5..985080b 100644
> > --- a/target-i386/translate.c
> > +++ b/target-i386/translate.c
> > @@ -7028,7 +7028,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
> >          break;
> >      case 0x134: /* sysenter */
> >          /* For Intel SYSENTER is valid on 64-bit */
> > -        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
> > +        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)
> >              goto illegal_op;
> >          if (!s->pe) {
> >              gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> > @@ -7041,7 +7041,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
> >          break;
> >      case 0x135: /* sysexit */
> >          /* For Intel SYSEXIT is valid on 64-bit */
> > -        if (CODE64(s) && env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1)
> > +        if (CODE64(s) && env->cpuid_vendor.regs.ebx != CPUID_VENDOR_INTEL_1)
> >              goto illegal_op;
> >          if (!s->pe) {
> >              gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> 
> -- 
> regards!
> li guang
> 


-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 12:25:50AM -0200, Eduardo Habkost wrote:
> On Fri, Jan 11, 2013 at 03:10:17AM +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()
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Oops, I missed a problem on this patch:

> 
[...]
> > @@ -2177,6 +2174,11 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >  #ifdef CONFIG_KVM
> >          filter_features_for_kvm(cpu);
> >  #endif
> > +        if (check_cpuid && kvm_check_features_against_host(cpu)
> > +            && enforce_cpuid) {
> > +            error_setg(errp, "Host's CPU doesn't support requested features");
> > +            return;
> > +        }

Checking kvm_check_features_against_host() after
filter_features_for_kvm() is useless, as filter_features_for_kvm() will
have filtered out everything that is not supported by the host, already.
This patch broke "-cpu enforce" on v1.4.0.

I will send a fix soon.

-- 
Eduardo