[Qemu-devel] [RFC 00/19] i386 CPU code cleanup + CPU model classes

[Qemu-devel] [RFC 00/19] i386 CPU code cleanup + CPU model classes

[Eduardo Habkost]

Hi,

I started looking at how the current CPU models could be converted to QOM
classes, and ended up implementing this. It's completely experimental and
untested, and the final step involves a hack, that is necessary until we remove
support for the cpudef config section.

The first 7 patches are pure cleanups. Patches 8 to 17 does lots of code
movements to isolate the CPU model string parsing and CPU object creation
together, so they can be changed to use the right CPU class.

Patch 18 finally does the change to use a CPU class corresponding to the
requested CPU model. Patch 19 is an ugly hack to make it possible to test this
code, by now.

This is surely not something for 1.2 (maybe just some cleanups from the first
few patches). It's just a RFC to see what you think about this approach. It
conflicts with ongoing work in this area, too, but I can rebase as necessary.

The series depends on the "[PATCH 0/3] Move CPU model definitions to C" series I
submitted yesterday.

Note that there's still lots of room for improvement. The X86CPUDefinition
struct could be much cleaner and simpler, and cpu_x86_create() (and the
functions it calls) could be simplified and become more QOM-like, too. But I
believe these changes at least make the steps involved in the CPU object
creation more visible, and can facilitate further refactorings.


Eduardo Habkost (19):
  target-i386/cpu.c: coding style fixes
  x86_cpudef_setup: coding style change
  i386: x86_def_t: rename 'flags' field
  rename cpu_x86_find_by_name to x86_cpu_build_from_name
  cpu_x86_build_from_name: use strtok_r()
  i386: cpu: extract the full feature list before parsing it
  i386: cpu: extract parsing of feature strings to separate function
  i386: extract CPU model lookup to a separate function
  i386: reorder object setup on cpu_x86_init()
  move CPU object creation to cpu.c
  rename x86_def_t to X86CPUDefinition
  create struct X86CPUModelTableEntry
  move X86CPUDefinition to cpu-qom.h
  extract CPU object field initialization from cpu_x86_register()
  cpu_x86_create: move error handling to end of function
  kill cpu_x86_register()
  kill cpu_x86_build_from_name()
  register a class for each CPU model
  HACK: late CPU class initialization

 target-i386/cpu-qom.h |  22 ++++
 target-i386/cpu.c     | 356 ++++++++++++++++++++++++++++++++------------------
 target-i386/cpu.h     |   2 +-
 target-i386/helper.c  |  14 +-
 4 files changed, 256 insertions(+), 138 deletions(-)

-- 
1.7.11.2

[Qemu-devel] [RFC 01/19] target-i386/cpu.c: coding style fixes

[Eduardo Habkost]

Changes to make checkpatch.pl happy:
- Use "sizeof(...)" instead of "sizeof (...)";
- Spaces around ":";
- Use spaces instead of tabs;
- Braces on if statements.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 66 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3eccf1b..9414d75 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1094,9 +1094,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     uint32_t minus_kvm_features = 0, minus_svm_features = 0;
     uint32_t numvalue;
 
-    for (def = x86_defs; def; def = def->next)
-        if (name && !strcmp(name, def->name))
+    for (def = x86_defs; def; def = def->next) {
+        if (name && !strcmp(name, def->name)) {
             break;
+        }
+    }
+
     if (kvm_enabled() && name && strcmp(name, "host") == 0) {
         cpu_x86_fill_host(x86_cpu_def);
     } else if (!def) {
@@ -1237,8 +1240,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
     if (check_cpuid) {
-        if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
+        if (check_features_against_host(x86_cpu_def) && enforce_cpuid) {
             goto error;
+        }
     }
     g_free(s);
     return 0;
@@ -1294,45 +1298,45 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
 
     if (cpuid) {
         (*cpu_fprintf)(f, "Recognized CPUID flags:\n");
-        listflags(buf, sizeof (buf), (uint32_t)~0, feature_name, 1);
+        listflags(buf, sizeof(buf), (uint32_t)~0, feature_name, 1);
         (*cpu_fprintf)(f, "  f_edx: %s\n", buf);
-        listflags(buf, sizeof (buf), (uint32_t)~0, ext_feature_name, 1);
+        listflags(buf, sizeof(buf), (uint32_t)~0, ext_feature_name, 1);
         (*cpu_fprintf)(f, "  f_ecx: %s\n", buf);
-        listflags(buf, sizeof (buf), (uint32_t)~0, ext2_feature_name, 1);
+        listflags(buf, sizeof(buf), (uint32_t)~0, ext2_feature_name, 1);
         (*cpu_fprintf)(f, "  extf_edx: %s\n", buf);
-        listflags(buf, sizeof (buf), (uint32_t)~0, ext3_feature_name, 1);
+        listflags(buf, sizeof(buf), (uint32_t)~0, ext3_feature_name, 1);
         (*cpu_fprintf)(f, "  extf_ecx: %s\n", buf);
         return;
     }
     for (def = x86_defs; def; def = def->next) {
-        snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name);
+        snprintf(buf, sizeof(buf), def->flags ? "[%s]" : "%s", def->name);
         if (model || dump) {
             (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
         } else {
             (*cpu_fprintf)(f, "x86 %16s\n", buf);
         }
         if (dump) {
-            memcpy(buf, &def->vendor1, sizeof (def->vendor1));
-            memcpy(buf + 4, &def->vendor2, sizeof (def->vendor2));
-            memcpy(buf + 8, &def->vendor3, sizeof (def->vendor3));
+            memcpy(buf, &def->vendor1, sizeof(def->vendor1));
+            memcpy(buf + 4, &def->vendor2, sizeof(def->vendor2));
+            memcpy(buf + 8, &def->vendor3, sizeof(def->vendor3));
             buf[12] = '\0';
             (*cpu_fprintf)(f,
                 "  family %d model %d stepping %d level %d xlevel 0x%x"
                 " vendor \"%s\"\n",
                 def->family, def->model, def->stepping, def->level,
                 def->xlevel, buf);
-            listflags(buf, sizeof (buf), def->features, feature_name, 0);
+            listflags(buf, sizeof(buf), def->features, feature_name, 0);
             (*cpu_fprintf)(f, "  feature_edx %08x (%s)\n", def->features,
                 buf);
-            listflags(buf, sizeof (buf), def->ext_features, ext_feature_name,
+            listflags(buf, sizeof(buf), def->ext_features, ext_feature_name,
                 0);
             (*cpu_fprintf)(f, "  feature_ecx %08x (%s)\n", def->ext_features,
                 buf);
-            listflags(buf, sizeof (buf), def->ext2_features, ext2_feature_name,
+            listflags(buf, sizeof(buf), def->ext2_features, ext2_feature_name,
                 0);
             (*cpu_fprintf)(f, "  extfeature_edx %08x (%s)\n",
                 def->ext2_features, buf);
-            listflags(buf, sizeof (buf), def->ext3_features, ext3_feature_name,
+            listflags(buf, sizeof(buf), def->ext3_features, ext3_feature_name,
                 0);
             (*cpu_fprintf)(f, "  extfeature_ecx %08x (%s)\n",
                 def->ext3_features, buf);
@@ -1352,8 +1356,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
+    if (cpu_x86_find_by_name(def, cpu_model) < 0) {
         return -1;
+    }
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1407,8 +1412,9 @@ static void cpyid(const char *s, uint32_t *id)
     char *d = (char *)id;
     char i;
 
-    for (i = sizeof (*id); i--; )
+    for (i = sizeof(*id); i--; ) {
         *d++ = *s ? *s++ : '\0';
+    }
 }
 
 /* interpret radix and convert from string to arbitrary scalar,
@@ -1458,7 +1464,7 @@ static int cpudef_setfield(const char *name, const char *str, void *opaque)
         g_free((void *)def->name);
         def->name = g_strdup(str);
     } else if (!strcmp(name, "model_id")) {
-        strncpy(def->model_id, str, sizeof (def->model_id));
+        strncpy(def->model_id, str, sizeof(def->model_id));
     } else if (!strcmp(name, "level")) {
         setscalar(&def->level, str, &err)
     } else if (!strcmp(name, "vendor")) {
@@ -1496,7 +1502,7 @@ static int cpudef_setfield(const char *name, const char *str, void *opaque)
  */
 static int cpudef_register(QemuOpts *opts, void *opaque)
 {
-    x86_def_t *def = g_malloc0(sizeof (x86_def_t));
+    x86_def_t *def = g_malloc0(sizeof(x86_def_t));
 
     qemu_opt_foreach(opts, cpudef_setfield, def, 1);
     def->next = x86_defs;
@@ -1783,17 +1789,17 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         break;
     case 0x8000000A:
-	if (env->cpuid_ext3_features & CPUID_EXT3_SVM) {
-		*eax = 0x00000001; /* SVM Revision */
-		*ebx = 0x00000010; /* nr of ASIDs */
-		*ecx = 0;
-		*edx = env->cpuid_svm_features; /* optional features */
-	} else {
-		*eax = 0;
-		*ebx = 0;
-		*ecx = 0;
-		*edx = 0;
-	}
+        if (env->cpuid_ext3_features & CPUID_EXT3_SVM) {
+            *eax = 0x00000001; /* SVM Revision */
+            *ebx = 0x00000010; /* nr of ASIDs */
+            *ecx = 0;
+            *edx = env->cpuid_svm_features; /* optional features */
+        } else {
+            *eax = 0;
+            *ebx = 0;
+            *ecx = 0;
+            *edx = 0;
+        }
         break;
     case 0xC0000000:
         *eax = env->cpuid_xlevel2;
-- 
1.7.11.2

[Qemu-devel] [RFC 02/19] x86_cpudef_setup: coding style change

[Eduardo Habkost]

Make source code lines shorter.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9414d75..d546534 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1526,20 +1526,23 @@ void x86_cpudef_setup(void)
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
 
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
-        builtin_x86_defs[i].next = x86_defs;
-        builtin_x86_defs[i].flags = 1;
+        x86_def_t *def = &builtin_x86_defs[i];
+        def->next = x86_defs;
+        def->flags = 1;
 
         /* Look for specific "cpudef" models that */
         /* have the QEMU version in .model_id */
         for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
-            if (strcmp(model_with_versions[j], builtin_x86_defs[i].name) == 0) {
-                pstrcpy(builtin_x86_defs[i].model_id, sizeof(builtin_x86_defs[i].model_id), "QEMU Virtual CPU version ");
-                pstrcat(builtin_x86_defs[i].model_id, sizeof(builtin_x86_defs[i].model_id), qemu_get_version());
+            if (strcmp(model_with_versions[j], def->name) == 0) {
+                pstrcpy(def->model_id, sizeof(def->model_id),
+                        "QEMU Virtual CPU version ");
+                pstrcat(def->model_id, sizeof(def->model_id),
+                        qemu_get_version());
                 break;
             }
         }
 
-        x86_defs = &builtin_x86_defs[i];
+        x86_defs = def;
     }
 #if !defined(CONFIG_USER_ONLY)
     qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
-- 
1.7.11.2

[Qemu-devel] [RFC 03/19] i386: x86_def_t: rename 'flags' field

[Eduardo Habkost]

The field is being used for a single purpose: to indicate if the CPU
model is a builtin one. So, instead of an opaque and confusing 'flags'
field name, use a boolean 'is_builtin' field to indicate if the model is
a built-in one.

No behavior change, just a field rename.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d546534..1154a34 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -236,7 +236,7 @@ typedef struct x86_def_t {
     uint32_t xlevel;
     char model_id[48];
     int vendor_override;
-    uint32_t flags;
+    bool is_builtin;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -1309,7 +1309,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
         return;
     }
     for (def = x86_defs; def; def = def->next) {
-        snprintf(buf, sizeof(buf), def->flags ? "[%s]" : "%s", def->name);
+        snprintf(buf, sizeof(buf), def->is_builtin ? "[%s]" : "%s", def->name);
         if (model || dump) {
             (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
         } else {
@@ -1528,7 +1528,7 @@ void x86_cpudef_setup(void)
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
         x86_def_t *def = &builtin_x86_defs[i];
         def->next = x86_defs;
-        def->flags = 1;
+        def->is_builtin = true;
 
         /* Look for specific "cpudef" models that */
         /* have the QEMU version in .model_id */
-- 
1.7.11.2

[Qemu-devel] [RFC 04/19] rename cpu_x86_find_by_name to x86_cpu_build_from_name

[Eduardo Habkost]

The function does a lot more than just "finding" the CPU model, so name it
accordingly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1154a34..be04e4b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1077,7 +1077,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
+                                   const char *cpu_model)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1356,7 +1357,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0) {
+    if (cpu_x86_build_from_name(def, cpu_model) < 0) {
         return -1;
     }
     if (def->vendor1) {
-- 
1.7.11.2

[Qemu-devel] [RFC 05/19] cpu_x86_build_from_name: use strtok_r()

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index be04e4b..154a01f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1083,8 +1083,9 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
     unsigned int i;
     x86_def_t *def;
 
+    char *last;
     char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
+    char *featurestr, *name = strtok_r(s, ",", &last);
     /* Features to be added*/
     uint32_t plus_features = 0, plus_ext_features = 0;
     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
@@ -1115,7 +1116,7 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
         &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
         &plus_kvm_features, &plus_svm_features);
 
-    featurestr = strtok(NULL, ",");
+    featurestr = strtok_r(NULL, ",", &last);
 
     while (featurestr) {
         char *val;
@@ -1226,7 +1227,7 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
-        featurestr = strtok(NULL, ",");
+        featurestr = strtok_r(NULL, ",", &last);
     }
     x86_cpu_def->features |= plus_features;
     x86_cpu_def->ext_features |= plus_ext_features;
-- 
1.7.11.2

[Qemu-devel] [RFC 06/19] i386: cpu: extract the full feature list before parsing it

[Eduardo Habkost]

The parsing of the feature list string will be moved to a separate
function, so first extract the full feature list, then parse it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 154a01f..7af80a0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,6 +1086,8 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
     char *last;
     char *s = g_strdup(cpu_model);
     char *featurestr, *name = strtok_r(s, ",", &last);
+    char *featlist = strtok_r(NULL, "", &last);
+
     /* Features to be added*/
     uint32_t plus_features = 0, plus_ext_features = 0;
     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
@@ -1116,7 +1118,7 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
         &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
         &plus_kvm_features, &plus_svm_features);
 
-    featurestr = strtok_r(NULL, ",", &last);
+    featurestr = featlist ? strtok_r(featlist, ",", &last) : NULL;
 
     while (featurestr) {
         char *val;
-- 
1.7.11.2

[Qemu-devel] [RFC 07/19] i386: cpu: extract parsing of feature strings to separate function

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 62 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7af80a0..dd2dc45 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1077,17 +1077,14 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
-                                   const char *cpu_model)
+/* Extend a x86_def_t struct based on a +feature,-feature,feature=xyz string
+ *
+ * Returns 0 on success, -1 on error.
+ */
+static int cpu_x86_extend_features(x86_def_t *x86_cpu_def, char *featlist)
 {
     unsigned int i;
-    x86_def_t *def;
-
-    char *last;
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok_r(s, ",", &last);
-    char *featlist = strtok_r(NULL, "", &last);
-
+    char *last, *featurestr;
     /* Features to be added*/
     uint32_t plus_features = 0, plus_ext_features = 0;
     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
@@ -1098,20 +1095,6 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
     uint32_t minus_kvm_features = 0, minus_svm_features = 0;
     uint32_t numvalue;
 
-    for (def = x86_defs; def; def = def->next) {
-        if (name && !strcmp(name, def->name)) {
-            break;
-        }
-    }
-
-    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
-        cpu_x86_fill_host(x86_cpu_def);
-    } else if (!def) {
-        goto error;
-    } else {
-        memcpy(x86_cpu_def, def, sizeof(*def));
-    }
-
     plus_kvm_features = ~0; /* not supported bits will be filtered out later */
 
     add_flagname_to_bitmaps("hypervisor", &plus_features,
@@ -1243,6 +1226,39 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
     x86_cpu_def->ext3_features &= ~minus_ext3_features;
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
+    return 0;
+error:
+    return -1;
+}
+
+static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
+                                   const char *cpu_model)
+{
+    x86_def_t *def;
+
+    char *last;
+    char *s = g_strdup(cpu_model);
+    char *name = strtok_r(s, ",", &last);
+    char *featlist = strtok_r(NULL, "", &last);
+
+    for (def = x86_defs; def; def = def->next) {
+        if (name && !strcmp(name, def->name)) {
+            break;
+        }
+    }
+
+    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
+        cpu_x86_fill_host(x86_cpu_def);
+    } else if (!def) {
+        goto error;
+    } else {
+        memcpy(x86_cpu_def, def, sizeof(*def));
+    }
+
+    if (cpu_x86_extend_features(x86_cpu_def, featlist) < 0) {
+        goto error;
+    }
+
     if (check_cpuid) {
         if (check_features_against_host(x86_cpu_def) && enforce_cpuid) {
             goto error;
-- 
1.7.11.2

[Qemu-devel] [RFC 08/19] i386: extract CPU model lookup to a separate function

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index dd2dc45..e5be586 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1231,16 +1231,10 @@ error:
     return -1;
 }
 
-static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
-                                   const char *cpu_model)
+static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
     x86_def_t *def;
 
-    char *last;
-    char *s = g_strdup(cpu_model);
-    char *name = strtok_r(s, ",", &last);
-    char *featlist = strtok_r(NULL, "", &last);
-
     for (def = x86_defs; def; def = def->next) {
         if (name && !strcmp(name, def->name)) {
             break;
@@ -1254,6 +1248,22 @@ static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
+    return 0;
+error:
+    return -1;
+}
+
+static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
+                                   const char *cpu_model)
+{
+    char *last;
+    char *s = g_strdup(cpu_model);
+    char *name = strtok_r(s, ",", &last);
+    char *featlist = strtok_r(NULL, "", &last);
+
+    if (cpu_x86_find_by_name(x86_cpu_def, name) != 0) {
+        goto error;
+    }
 
     if (cpu_x86_extend_features(x86_cpu_def, featlist) < 0) {
         goto error;
-- 
1.7.11.2

[Qemu-devel] [RFC 09/19] i386: reorder object setup on cpu_x86_init()

[Eduardo Habkost]

The object creation will be moved to cpu.c, with all the rest of the CPU model
handling.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index b748d90..5a7eb56 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1157,10 +1157,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     CPUX86State *env;
     static int inited;
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_model;
-
     /* init various static tables used in TCG mode */
     if (tcg_enabled() && !inited) {
         inited = 1;
@@ -1170,6 +1166,10 @@ X86CPU *cpu_x86_init(const char *cpu_model)
             cpu_set_debug_excp_handler(breakpoint_handler);
 #endif
     }
+
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_model;
     if (cpu_x86_register(cpu, cpu_model) < 0) {
         object_delete(OBJECT(cpu));
         return NULL;
-- 
1.7.11.2

[Qemu-devel] [RFC 10/19] move CPU object creation to cpu.c

[Eduardo Habkost]

cpu.c handles the class registration and parsing of the CPU model string, so
moving it there will make it easier to refactor the CPU model string parsing
and CPU object creation.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c    | 17 +++++++++++++++++
 target-i386/cpu.h    |  1 +
 target-i386/helper.c | 14 +-------------
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e5be586..c96300e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1434,6 +1434,23 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     return 0;
 }
 
+X86CPU *cpu_x86_create(const char *cpu_model)
+{
+    X86CPU *cpu;
+    CPUX86State *env;
+
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_model;
+    if (cpu_x86_register(cpu, cpu_model) < 0) {
+        object_delete(OBJECT(cpu));
+        return NULL;
+    }
+
+    x86_cpu_realize(OBJECT(cpu), NULL);
+    return cpu;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 /* copy vendor id string to 32 bit register, nul pad as needed
  */
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index c81f7bf..436ff86 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -927,6 +927,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
+X86CPU *cpu_x86_create(const char *cpu_model);
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 5a7eb56..3c10ec1 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1153,8 +1153,6 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
 
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
-    X86CPU *cpu;
-    CPUX86State *env;
     static int inited;
 
     /* init various static tables used in TCG mode */
@@ -1167,17 +1165,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 #endif
     }
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_model;
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
-        object_delete(OBJECT(cpu));
-        return NULL;
-    }
-
-    x86_cpu_realize(OBJECT(cpu), NULL);
-
-    return cpu;
+    return cpu_x86_create(cpu_model);
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.11.2

[Qemu-devel] [RFC 11/19] rename x86_def_t to X86CPUDefinition

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c96300e..4f67ecc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -222,8 +222,8 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
             fprintf(stderr, "CPU feature %s not found\n", flagname);
 }
 
-typedef struct x86_def_t {
-    struct x86_def_t *next;
+typedef struct X86CPUDefinition {
+    struct X86CPUDefinition *next;
     const char *name;
     uint32_t level;
     uint32_t vendor1, vendor2, vendor3;
@@ -242,7 +242,7 @@ typedef struct x86_def_t {
     uint32_t xlevel2;
     /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
     uint32_t cpuid_7_0_ebx_features;
-} x86_def_t;
+} X86CPUDefinition;
 
 #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
@@ -284,11 +284,11 @@ typedef struct x86_def_t {
 
 /* maintains list of cpu model definitions
  */
-static x86_def_t *x86_defs = {NULL};
+static X86CPUDefinition *x86_defs = {NULL};
 
 /* built-in cpu model definitions (deprecated)
  */
-static x86_def_t builtin_x86_defs[] = {
+static X86CPUDefinition builtin_x86_defs[] = {
     {
         .name = "qemu64",
         .level = 4,
@@ -722,7 +722,7 @@ static int cpu_x86_fill_model_id(char *str)
     return 0;
 }
 
-static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
+static int cpu_x86_fill_host(X86CPUDefinition *x86_cpu_def)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
@@ -798,9 +798,9 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
  * their way to the guest.  Note: ft[].check_feat ideally should be
  * specified via a guest_def field to suppress report of extraneous flags.
  */
-static int check_features_against_host(x86_def_t *guest_def)
+static int check_features_against_host(X86CPUDefinition *guest_def)
 {
-    x86_def_t host_def;
+    X86CPUDefinition host_def;
     uint32_t mask;
     int rv, i;
     struct model_features_t ft[] = {
@@ -1077,11 +1077,12 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-/* Extend a x86_def_t struct based on a +feature,-feature,feature=xyz string
+/* Extend a X86CPUDefinition based on a +feature,-feature,feature=xyz string
  *
  * Returns 0 on success, -1 on error.
  */
-static int cpu_x86_extend_features(x86_def_t *x86_cpu_def, char *featlist)
+static int cpu_x86_extend_features(X86CPUDefinition *x86_cpu_def,
+                                   char *featlist)
 {
     unsigned int i;
     char *last, *featurestr;
@@ -1231,9 +1232,9 @@ error:
     return -1;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
+static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def, const char *name)
 {
-    x86_def_t *def;
+    X86CPUDefinition *def;
 
     for (def = x86_defs; def; def = def->next) {
         if (name && !strcmp(name, def->name)) {
@@ -1253,7 +1254,7 @@ error:
     return -1;
 }
 
-static int cpu_x86_build_from_name(x86_def_t *x86_cpu_def,
+static int cpu_x86_build_from_name(X86CPUDefinition *x86_cpu_def,
                                    const char *cpu_model)
 {
     char *last;
@@ -1315,7 +1316,7 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
 /* generate CPU information:
  * -?        list model names
  * -?model   list model names/IDs
- * -?dump    output all model (x86_def_t) data
+ * -?dump    output all model (X86CPUDefinition) data
  * -?cpuid   list all recognized cpuid flag names
  */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
@@ -1323,7 +1324,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
     unsigned char model = !strcmp("?model", optarg);
     unsigned char dump = !strcmp("?dump", optarg);
     unsigned char cpuid = !strcmp("?cpuid", optarg);
-    x86_def_t *def;
+    X86CPUDefinition *def;
     char buf[256];
 
     if (cpuid) {
@@ -1381,7 +1382,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     CPUX86State *env = &cpu->env;
-    x86_def_t def1, *def = &def1;
+    X86CPUDefinition def1, *def = &def1;
     Error *error = NULL;
 
     memset(def, 0, sizeof(*def));
@@ -1500,11 +1501,11 @@ static void setfeatures(uint32_t *pval, const char *str,
     }
 }
 
-/* map config file options to x86_def_t form
+/* map config file options to X86CPUDefinition form
  */
 static int cpudef_setfield(const char *name, const char *str, void *opaque)
 {
-    x86_def_t *def = opaque;
+    X86CPUDefinition *def = opaque;
     int err = 0;
 
     if (!strcmp(name, "name")) {
@@ -1545,11 +1546,11 @@ static int cpudef_setfield(const char *name, const char *str, void *opaque)
     return (0);
 }
 
-/* register config file entry as x86_def_t
+/* register config file entry as X86CPUDefinition
  */
 static int cpudef_register(QemuOpts *opts, void *opaque)
 {
-    x86_def_t *def = g_malloc0(sizeof(x86_def_t));
+    X86CPUDefinition *def = g_malloc0(sizeof(X86CPUDefinition));
 
     qemu_opt_foreach(opts, cpudef_setfield, def, 1);
     def->next = x86_defs;
@@ -1573,7 +1574,7 @@ void x86_cpudef_setup(void)
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
 
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
-        x86_def_t *def = &builtin_x86_defs[i];
+        X86CPUDefinition *def = &builtin_x86_defs[i];
         def->next = x86_defs;
         def->is_builtin = true;
 
-- 
1.7.11.2

[Qemu-devel] [RFC 12/19] create struct X86CPUModelTableEntry

[Eduardo Habkost]

X86CPUDefinition will be reused in other places, so remove the fields
that are specific to the CPU model name table ('name', 'is_builtin', and
'next') and put them into a separate struct.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 132 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 92 insertions(+), 40 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4f67ecc..3f6efdb 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -223,8 +223,6 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
 }
 
 typedef struct X86CPUDefinition {
-    struct X86CPUDefinition *next;
-    const char *name;
     uint32_t level;
     uint32_t vendor1, vendor2, vendor3;
     int family;
@@ -236,7 +234,6 @@ typedef struct X86CPUDefinition {
     uint32_t xlevel;
     char model_id[48];
     int vendor_override;
-    bool is_builtin;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -282,15 +279,24 @@ typedef struct X86CPUDefinition {
           CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
 #define TCG_SVM_FEATURES 0
 
+
+typedef struct X86CPUModelTableEntry {
+    const char *name;
+    struct X86CPUModelTableEntry *next;
+    bool is_builtin;
+    X86CPUDefinition cpudef;
+} X86CPUModelTableEntry;
+
 /* maintains list of cpu model definitions
  */
-static X86CPUDefinition *x86_defs = {NULL};
+static X86CPUModelTableEntry *x86_defs = {NULL};
 
 /* built-in cpu model definitions (deprecated)
  */
-static X86CPUDefinition builtin_x86_defs[] = {
+static X86CPUModelTableEntry builtin_x86_defs[] = {
     {
-        .name = "qemu64",
+      .name = "qemu64",
+      .cpudef = {
         .level = 4,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -307,9 +313,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
             CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
         .xlevel = 0x8000000A,
+      },
     },
     {
-        .name = "phenom",
+      .name = "phenom",
+      .cpudef = {
         .level = 5,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -335,9 +343,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV,
         .xlevel = 0x8000001A,
         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
+      },
     },
     {
-        .name = "core2duo",
+      .name = "core2duo",
+      .cpudef = {
         .level = 10,
         .family = 6,
         .model = 15,
@@ -353,9 +363,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x80000008,
         .model_id = "Intel(R) Core(TM)2 Duo CPU     T7700  @ 2.40GHz",
+      },
     },
     {
-        .name = "kvm64",
+      .name = "kvm64",
+      .cpudef = {
         .level = 5,
         .vendor1 = CPUID_VENDOR_INTEL_1,
         .vendor2 = CPUID_VENDOR_INTEL_2,
@@ -379,9 +391,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = 0,
         .xlevel = 0x80000008,
         .model_id = "Common KVM processor"
+      },
     },
     {
-        .name = "qemu32",
+      .name = "qemu32",
+      .cpudef = {
         .level = 4,
         .family = 6,
         .model = 3,
@@ -389,9 +403,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .features = PPRO_FEATURES,
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_POPCNT,
         .xlevel = 0x80000004,
+      },
     },
     {
-        .name = "kvm32",
+      .name = "kvm32",
+      .cpudef = {
         .level = 5,
         .family = 15,
         .model = 6,
@@ -403,9 +419,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = 0,
         .xlevel = 0x80000008,
         .model_id = "Common 32-bit KVM processor"
+      },
     },
     {
-        .name = "coreduo",
+      .name = "coreduo",
+      .cpudef = {
         .level = 10,
         .family = 6,
         .model = 14,
@@ -418,45 +436,55 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext2_features = CPUID_EXT2_NX,
         .xlevel = 0x80000008,
         .model_id = "Genuine Intel(R) CPU           T2600  @ 2.16GHz",
+      },
     },
     {
-        .name = "486",
+      .name = "486",
+      .cpudef = {
         .level = 1,
         .family = 4,
         .model = 0,
         .stepping = 0,
         .features = I486_FEATURES,
         .xlevel = 0,
+      },
     },
     {
-        .name = "pentium",
+      .name = "pentium",
+      .cpudef = {
         .level = 1,
         .family = 5,
         .model = 4,
         .stepping = 3,
         .features = PENTIUM_FEATURES,
         .xlevel = 0,
+      },
     },
     {
-        .name = "pentium2",
+      .name = "pentium2",
+      .cpudef = {
         .level = 2,
         .family = 6,
         .model = 5,
         .stepping = 2,
         .features = PENTIUM2_FEATURES,
         .xlevel = 0,
+      },
     },
     {
-        .name = "pentium3",
+      .name = "pentium3",
+      .cpudef = {
         .level = 2,
         .family = 6,
         .model = 7,
         .stepping = 3,
         .features = PENTIUM3_FEATURES,
         .xlevel = 0,
+      },
     },
     {
-        .name = "athlon",
+      .name = "athlon",
+      .cpudef = {
         .level = 2,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -467,9 +495,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR | CPUID_MCA,
         .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) | CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
         .xlevel = 0x80000008,
+      },
     },
     {
-        .name = "n270",
+      .name = "n270",
+      .cpudef = {
         /* original is on level 10 */
         .level = 5,
         .family = 6,
@@ -485,9 +515,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Intel(R) Atom(TM) CPU N270   @ 1.60GHz",
+      },
     },
     {
-        .name = "Conroe",
+      .name = "Conroe",
+      .cpudef = {
         .level = 2,
         .vendor1 = CPUID_VENDOR_INTEL_1,
         .vendor2 = CPUID_VENDOR_INTEL_2,
@@ -505,9 +537,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Intel Celeron_4x0 (Conroe/Merom Class Core 2)",
+      },
     },
     {
-        .name = "Penryn",
+      .name = "Penryn",
+      .cpudef = {
         .level = 2,
         .vendor1 = CPUID_VENDOR_INTEL_1,
         .vendor2 = CPUID_VENDOR_INTEL_2,
@@ -526,9 +560,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Intel Core 2 Duo P9xxx (Penryn Class Core 2)",
+      },
     },
     {
-        .name = "Nehalem",
+      .name = "Nehalem",
+      .cpudef = {
         .level = 2,
         .vendor1 = CPUID_VENDOR_INTEL_1,
         .vendor2 = CPUID_VENDOR_INTEL_2,
@@ -547,9 +583,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Intel Core i7 9xx (Nehalem Class Core i7)",
+      },
     },
     {
-        .name = "Westmere",
+      .name = "Westmere",
+      .cpudef = {
         .level = 11,
         .vendor1 = CPUID_VENDOR_INTEL_1,
         .vendor2 = CPUID_VENDOR_INTEL_2,
@@ -569,9 +607,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Westmere E56xx/L56xx/X56xx (Nehalem-C)",
+      },
     },
     {
-        .name = "SandyBridge",
+      .name = "SandyBridge",
+      .cpudef = {
         .level = 0xd,
         .vendor1 = CPUID_VENDOR_INTEL_1,
         .vendor2 = CPUID_VENDOR_INTEL_2,
@@ -594,9 +634,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000000A,
         .model_id = "Intel Xeon E312xx (Sandy Bridge)",
+      },
     },
     {
-        .name = "Opteron_G1",
+      .name = "Opteron_G1",
+      .cpudef = {
         .level = 5,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -618,9 +660,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
              CPUID_EXT2_TSC | CPUID_EXT2_PSE | CPUID_EXT2_DE | CPUID_EXT2_FPU,
         .xlevel = 0x80000008,
         .model_id = "AMD Opteron 240 (Gen 1 Class Opteron)",
+      },
     },
     {
-        .name = "Opteron_G2",
+      .name = "Opteron_G2",
+      .cpudef = {
         .level = 5,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -644,9 +688,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .ext3_features = CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
         .xlevel = 0x80000008,
         .model_id = "AMD Opteron 22xx (Gen 2 Class Opteron)",
+      },
     },
     {
-        .name = "Opteron_G3",
+      .name = "Opteron_G3",
+      .cpudef = {
         .level = 5,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -672,9 +718,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
              CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
         .xlevel = 0x80000008,
         .model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)",
+      },
     },
     {
-        .name = "Opteron_G4",
+      .name = "Opteron_G4",
+      .cpudef = {
         .level = 0xd,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
@@ -704,6 +752,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
              CPUID_EXT3_LAHF_LM,
         .xlevel = 0x8000001A,
         .model_id = "AMD Opteron 62xx class CPU",
+      },
     },
 };
 
@@ -722,11 +771,11 @@ static int cpu_x86_fill_model_id(char *str)
     return 0;
 }
 
+/* Fill X86CPUDefinition struct with host CPU features */
 static int cpu_x86_fill_host(X86CPUDefinition *x86_cpu_def)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
-    x86_cpu_def->name = "host";
     host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->level = eax;
     x86_cpu_def->vendor1 = ebx;
@@ -1234,7 +1283,7 @@ error:
 
 static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def, const char *name)
 {
-    X86CPUDefinition *def;
+    X86CPUModelTableEntry *def;
 
     for (def = x86_defs; def; def = def->next) {
         if (name && !strcmp(name, def->name)) {
@@ -1247,7 +1296,7 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def, const char *name)
     } else if (!def) {
         goto error;
     } else {
-        memcpy(x86_cpu_def, def, sizeof(*def));
+        memcpy(x86_cpu_def, &def->cpudef, sizeof(*def));
     }
     return 0;
 error:
@@ -1324,7 +1373,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
     unsigned char model = !strcmp("?model", optarg);
     unsigned char dump = !strcmp("?dump", optarg);
     unsigned char cpuid = !strcmp("?cpuid", optarg);
-    X86CPUDefinition *def;
+    X86CPUModelTableEntry *deft;
     char buf[256];
 
     if (cpuid) {
@@ -1339,8 +1388,10 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
         (*cpu_fprintf)(f, "  extf_ecx: %s\n", buf);
         return;
     }
-    for (def = x86_defs; def; def = def->next) {
-        snprintf(buf, sizeof(buf), def->is_builtin ? "[%s]" : "%s", def->name);
+    for (deft = x86_defs; deft; deft = deft->next) {
+        X86CPUDefinition *def = &deft->cpudef;
+        snprintf(buf, sizeof(buf), deft->is_builtin ? "[%s]" : "%s",
+                                   deft->name);
         if (model || dump) {
             (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
         } else {
@@ -1505,12 +1556,13 @@ static void setfeatures(uint32_t *pval, const char *str,
  */
 static int cpudef_setfield(const char *name, const char *str, void *opaque)
 {
-    X86CPUDefinition *def = opaque;
+    X86CPUModelTableEntry *deft = opaque;
+    X86CPUDefinition *def = &deft->cpudef;
     int err = 0;
 
     if (!strcmp(name, "name")) {
-        g_free((void *)def->name);
-        def->name = g_strdup(str);
+        g_free((void *)deft->name);
+        deft->name = g_strdup(str);
     } else if (!strcmp(name, "model_id")) {
         strncpy(def->model_id, str, sizeof(def->model_id));
     } else if (!strcmp(name, "level")) {
@@ -1550,7 +1602,7 @@ static int cpudef_setfield(const char *name, const char *str, void *opaque)
  */
 static int cpudef_register(QemuOpts *opts, void *opaque)
 {
-    X86CPUDefinition *def = g_malloc0(sizeof(X86CPUDefinition));
+    X86CPUModelTableEntry *def = g_malloc0(sizeof(X86CPUModelTableEntry));
 
     qemu_opt_foreach(opts, cpudef_setfield, def, 1);
     def->next = x86_defs;
@@ -1574,7 +1626,7 @@ void x86_cpudef_setup(void)
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
 
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
-        X86CPUDefinition *def = &builtin_x86_defs[i];
+        X86CPUModelTableEntry *def = &builtin_x86_defs[i];
         def->next = x86_defs;
         def->is_builtin = true;
 
@@ -1582,9 +1634,9 @@ void x86_cpudef_setup(void)
         /* have the QEMU version in .model_id */
         for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) {
             if (strcmp(model_with_versions[j], def->name) == 0) {
-                pstrcpy(def->model_id, sizeof(def->model_id),
+                pstrcpy(def->cpudef.model_id, sizeof(def->cpudef.model_id),
                         "QEMU Virtual CPU version ");
-                pstrcat(def->model_id, sizeof(def->model_id),
+                pstrcat(def->cpudef.model_id, sizeof(def->cpudef.model_id),
                         qemu_get_version());
                 break;
             }
-- 
1.7.11.2

[Qemu-devel] [RFC 13/19] move X86CPUDefinition to cpu-qom.h

[Eduardo Habkost]

From: Eduardo Habkost <ehabkost@thinair.local>

The struct will be used by X86CPUClass, too.

Signed-off-by: Eduardo Habkost <ehabkost@thinair.local>
---
 target-i386/cpu-qom.h | 20 ++++++++++++++++++++
 target-i386/cpu.c     | 19 -------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 5901140..03e2c3a 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -37,6 +37,26 @@
 #define X86_CPU_GET_CLASS(obj) \
     OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
 
+
+typedef struct X86CPUDefinition {
+    uint32_t level;
+    uint32_t vendor1, vendor2, vendor3;
+    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;
+    char model_id[48];
+    int vendor_override;
+    /* Store the results of Centaur's CPUID instructions */
+    uint32_t ext4_features;
+    uint32_t xlevel2;
+    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
+    uint32_t cpuid_7_0_ebx_features;
+} X86CPUDefinition;
+
 /**
  * X86CPUClass:
  * @parent_reset: The parent class' reset handler.
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3f6efdb..5efbe41 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -222,25 +222,6 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
             fprintf(stderr, "CPU feature %s not found\n", flagname);
 }
 
-typedef struct X86CPUDefinition {
-    uint32_t level;
-    uint32_t vendor1, vendor2, vendor3;
-    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;
-    char model_id[48];
-    int vendor_override;
-    /* Store the results of Centaur's CPUID instructions */
-    uint32_t ext4_features;
-    uint32_t xlevel2;
-    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
-    uint32_t cpuid_7_0_ebx_features;
-} X86CPUDefinition;
-
 #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
-- 
1.7.11.2

[Qemu-devel] [RFC 14/19] extract CPU object field initialization from cpu_x86_register()

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5efbe41..f30e621 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1411,17 +1411,12 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
     }
 }
 
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+/* Initialize CPU object from X86CPUDefinition struct */
+static int cpu_x86_init_from_def(X86CPU *cpu, X86CPUDefinition *def)
 {
     CPUX86State *env = &cpu->env;
-    X86CPUDefinition def1, *def = &def1;
     Error *error = NULL;
 
-    memset(def, 0, sizeof(*def));
-
-    if (cpu_x86_build_from_name(def, cpu_model) < 0) {
-        return -1;
-    }
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1464,6 +1459,24 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         error_free(error);
         return -1;
     }
+
+    return 0;
+}
+
+int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+{
+    X86CPUDefinition def1, *def = &def1;
+
+    memset(def, 0, sizeof(*def));
+
+    if (cpu_x86_build_from_name(def, cpu_model) < 0) {
+        return -1;
+    }
+
+    if (cpu_x86_init_from_def(cpu, def) < 0) {
+        return -1;
+    }
+
     return 0;
 }
 
-- 
1.7.11.2

[Qemu-devel] [RFC 15/19] cpu_x86_create: move error handling to end of function

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f30e621..7821331 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1489,12 +1489,15 @@ X86CPU *cpu_x86_create(const char *cpu_model)
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
     if (cpu_x86_register(cpu, cpu_model) < 0) {
-        object_delete(OBJECT(cpu));
-        return NULL;
+        goto error;
     }
 
     x86_cpu_realize(OBJECT(cpu), NULL);
     return cpu;
+
+error:
+    object_delete(OBJECT(cpu));
+    return NULL;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.11.2

[Qemu-devel] [RFC 16/19] kill cpu_x86_register()

[Eduardo Habkost]

We will need to reorder parts the CPU object creation code, so move
cpu_x86_register() code into the cpu_x86_create() function to make it possible.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 25 ++++++++-----------------
 target-i386/cpu.h |  1 -
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7821331..751cf90 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1463,32 +1463,23 @@ static int cpu_x86_init_from_def(X86CPU *cpu, X86CPUDefinition *def)
     return 0;
 }
 
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+X86CPU *cpu_x86_create(const char *cpu_model)
 {
+    X86CPU *cpu;
+    CPUX86State *env;
     X86CPUDefinition def1, *def = &def1;
 
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_model;
+
     memset(def, 0, sizeof(*def));
 
     if (cpu_x86_build_from_name(def, cpu_model) < 0) {
-        return -1;
+        goto error;
     }
 
     if (cpu_x86_init_from_def(cpu, def) < 0) {
-        return -1;
-    }
-
-    return 0;
-}
-
-X86CPU *cpu_x86_create(const char *cpu_model)
-{
-    X86CPU *cpu;
-    CPUX86State *env;
-
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_model;
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
         goto error;
     }
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 436ff86..c1c1194 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -928,7 +928,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
 X86CPU *cpu_x86_create(const char *cpu_model);
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
-- 
1.7.11.2

[Qemu-devel] [RFC 17/19] kill cpu_x86_build_from_name()

[Eduardo Habkost]

We will reorder some code in the CPU object initialization, so move all
the cpu_x86_build_from_name() code inside cpu_x86_create().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 48 ++++++++++++++++++------------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 751cf90..f401a16 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1284,35 +1284,6 @@ error:
     return -1;
 }
 
-static int cpu_x86_build_from_name(X86CPUDefinition *x86_cpu_def,
-                                   const char *cpu_model)
-{
-    char *last;
-    char *s = g_strdup(cpu_model);
-    char *name = strtok_r(s, ",", &last);
-    char *featlist = strtok_r(NULL, "", &last);
-
-    if (cpu_x86_find_by_name(x86_cpu_def, name) != 0) {
-        goto error;
-    }
-
-    if (cpu_x86_extend_features(x86_cpu_def, featlist) < 0) {
-        goto error;
-    }
-
-    if (check_cpuid) {
-        if (check_features_against_host(x86_cpu_def) && enforce_cpuid) {
-            goto error;
-        }
-    }
-    g_free(s);
-    return 0;
-
-error:
-    g_free(s);
-    return -1;
-}
-
 /* generate a composite string into buf of all cpuid names in featureset
  * selected by fbits.  indicate truncation at bufsize in the event of overflow.
  * if flags, suppress names undefined in featureset.
@@ -1468,6 +1439,10 @@ X86CPU *cpu_x86_create(const char *cpu_model)
     X86CPU *cpu;
     CPUX86State *env;
     X86CPUDefinition def1, *def = &def1;
+    char *last;
+    char *s = g_strdup(cpu_model);
+    char *name = strtok_r(s, ",", &last);
+    char *featlist = strtok_r(NULL, "", &last);
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
@@ -1475,18 +1450,31 @@ X86CPU *cpu_x86_create(const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_build_from_name(def, cpu_model) < 0) {
+    if (cpu_x86_find_by_name(def, name) != 0) {
         goto error;
     }
 
+    if (cpu_x86_extend_features(def, featlist) < 0) {
+        goto error;
+    }
+
+    if (check_cpuid) {
+        if (check_features_against_host(def) && enforce_cpuid) {
+            goto error;
+        }
+    }
+
     if (cpu_x86_init_from_def(cpu, def) < 0) {
         goto error;
     }
 
     x86_cpu_realize(OBJECT(cpu), NULL);
+
+    g_free(s);
     return cpu;
 
 error:
+    g_free(s);
     object_delete(OBJECT(cpu));
     return NULL;
 }
-- 
1.7.11.2

[Qemu-devel] [RFC 18/19] register a class for each CPU model

[Eduardo Habkost]

The trick here is to replace only the cpu_x86_find_by_name() logic and nothing
else. So, the only difference in relation to the previous code is that instead
of looking at the CPU model table on cpu_x86_create()/cpu_x86_find_by_name(),
we just use the right CPU class, that will already contain the CPU model
definition inside it.

I'm not sure what would be the best naming convention for the CPU classes. I'm
using "<arch>-cpu.<model>" (TYPE_X86_CPU is "<arch>-cpu").

Note: This patch won't work as-is, yet, because of initialization ordering
problems. The next patch will be a hack to make this work, by now.  Reordering
the initialization will be easier once we eliminate the support for cpudef
config sections.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |  2 ++
 target-i386/cpu.c     | 78 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 03e2c3a..20144ea 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -69,6 +69,8 @@ typedef struct X86CPUClass {
     /*< public >*/
 
     void (*parent_reset)(CPUState *cpu);
+
+    X86CPUDefinition cpudef;
 } X86CPUClass;
 
 /**
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f401a16..df4fb1f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1262,28 +1262,6 @@ error:
     return -1;
 }
 
-static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def, const char *name)
-{
-    X86CPUModelTableEntry *def;
-
-    for (def = x86_defs; def; def = def->next) {
-        if (name && !strcmp(name, def->name)) {
-            break;
-        }
-    }
-
-    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
-        cpu_x86_fill_host(x86_cpu_def);
-    } else if (!def) {
-        goto error;
-    } else {
-        memcpy(x86_cpu_def, &def->cpudef, sizeof(*def));
-    }
-    return 0;
-error:
-    return -1;
-}
-
 /* generate a composite string into buf of all cpuid names in featureset
  * selected by fbits.  indicate truncation at bufsize in the event of overflow.
  * if flags, suppress names undefined in featureset.
@@ -1434,6 +1412,12 @@ static int cpu_x86_init_from_def(X86CPU *cpu, X86CPUDefinition *def)
     return 0;
 }
 
+/* Build class name for specific CPU model */
+static char *build_cpu_class_name(const char *model)
+{
+    return g_strdup_printf("%s.%s", TYPE_X86_CPU, model);
+}
+
 X86CPU *cpu_x86_create(const char *cpu_model)
 {
     X86CPU *cpu;
@@ -1443,16 +1427,14 @@ X86CPU *cpu_x86_create(const char *cpu_model)
     char *s = g_strdup(cpu_model);
     char *name = strtok_r(s, ",", &last);
     char *featlist = strtok_r(NULL, "", &last);
+    char *class_name = build_cpu_class_name(name);
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    cpu = X86_CPU(object_new(class_name));
+    g_free(class_name);
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    memset(def, 0, sizeof(*def));
-
-    if (cpu_x86_find_by_name(def, name) != 0) {
-        goto error;
-    }
+    *def = X86_CPU_GET_CLASS(cpu)->cpudef;
 
     if (cpu_x86_extend_features(def, featlist) < 0) {
         goto error;
@@ -2090,19 +2072,55 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->reset = x86_cpu_reset;
 }
 
+static void x86_cpu_class_init(ObjectClass *oc, void *data)
+{
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    X86CPUDefinition *def = data;
+
+    xcc->cpudef = *def;
+}
+
 static const TypeInfo x86_cpu_type_info = {
     .name = TYPE_X86_CPU,
     .parent = TYPE_CPU,
     .instance_size = sizeof(X86CPU),
-    .instance_init = x86_cpu_initfn,
-    .abstract = false,
+    .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
 };
 
+static void x86_cpu_register_class(const char *name, X86CPUDefinition *def)
+{
+    char *class_name = build_cpu_class_name(name);
+    TypeInfo type = {
+        .name = class_name,
+        .parent = TYPE_X86_CPU,
+        .instance_size = sizeof(X86CPU),
+        .instance_init = x86_cpu_initfn,
+        .class_size = sizeof(X86CPUClass),
+        .class_init = x86_cpu_class_init,
+        .class_data = def,
+    };
+    type_register(&type);
+    g_free(class_name);
+}
+
 static void x86_cpu_register_types(void)
 {
+    X86CPUModelTableEntry *def;
+    X86CPUDefinition host_def;
+
+    /* Abstract CPUX86 class */
     type_register_static(&x86_cpu_type_info);
+
+    /* One class for each CPU model: */
+    for (def = x86_defs; def; def = def->next) {
+        x86_cpu_register_class(def->name, &def->cpudef);
+    }
+
+    /* -cpu host class */
+    cpu_x86_fill_host(&host_def);
+    x86_cpu_register_class("host", &host_def);
 }
 
 type_init(x86_cpu_register_types)
-- 
1.7.11.2

[Qemu-devel] [RFC 19/19] HACK: late CPU class initialization

[Eduardo Habkost]

This is just a hack to make the experimental CPU class code work without
removing the cpudef support.

After we remove the cpudef support, we can simply use type_init() again,
and register only the builtin CPU models as CPU classes.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index df4fb1f..8e978bd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1575,6 +1575,8 @@ void cpu_clear_apic_feature(CPUX86State *env)
 
 #endif /* !CONFIG_USER_ONLY */
 
+static void x86_cpu_register_types(void);
+
 /* register "cpudef" models defined in configuration file.  Here we first
  * preload any built-in definitions
  */
@@ -1605,6 +1607,8 @@ void x86_cpudef_setup(void)
 #if !defined(CONFIG_USER_ONLY)
     qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0);
 #endif
+
+    x86_cpu_register_types();
 }
 
 static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
@@ -2123,4 +2127,5 @@ static void x86_cpu_register_types(void)
     x86_cpu_register_class("host", &host_def);
 }
 
-type_init(x86_cpu_register_types)
+//HACK: the function is being called from x86_cpudef_setup()
+//type_init(x86_cpu_register_types)
-- 
1.7.11.2