[Qemu-devel] [PATCH 00/17] target-i386: CPU init cleanup for CPU classes/properties

[Qemu-devel] [PATCH 00/17] target-i386: CPU init cleanup for CPU classes/properties

[Eduardo Habkost]

Hi,

One of the problems when coordinating with the recent x86 CPU work (CPU
properties, APIC ID topology fixes) is to have a sane x86 CPU initialization
code to be used as base.

To make things worse, the cleanups we're doing are buried inside series that do
more intrusive work. This is an attempt to establish a saner base for further
work.

Except for additional error messages when initialization fails, this series
should not introduce any behavior change, but just clean up cpu_x86_init() to
look like this:

    name, features = g_strsplit(cpu_string, ",", 2);
    cpu = X86_CPU(object_new(TYPE_X86_CPU));
    cpu_x86_find_cpudef(name, &def, &error)
    cpu_x86_parse_featurestr(&def, features, &error)
    cpudef_2_x86_cpu(cpu, &def, &error)
    x86_cpu_realize(OBJECT(cpu), &error);

Basically, this isolates the parts of the code that do:
 - Splitting cpu_model string into CPU model and feature string;
 - CPU object creation;
 - CPU model lookup;
 - CPU feature string parsing.

After this, I will send an additional patch to introduce CPU classes on
target-i386, based on this series.

Eduardo Habkost (16):
  target-i386/cpu.c: coding style fix
  target-i386: move cpu_x86_init() to cpu.c
  target-i386: cpu: rename x86_def_t to X86CPUDefinition
  target-i386: x86_cpudef_setup(): cosmetic change on comment
  target-i386: cpu_x86_init(): move error handling to end of function
  target-i386: cpu_x86_init(): print error message in case of error
  target-i386: cpu_x86_register(): report errors using Error parameter
  target-i386: cpu_x86_register(): reorder CPU property setting
  target-i386: kill cpu_x86_register()
  target-i386: return Error from cpu_x86_find_by_name()
  target-i386: cpu_x86_find_by_name(): split CPU model and feature
    string first
  target-i386: cpu: create cpu_x86_find_cpudef() function
  target-i386: cpu_x86_init(): rename cpu_model to cpu_string
  target-i386: cpu_x86_init(): eliminate extra 'def1' variable
  target-i386: cpu: separate cpudef lookup from feature string parsing
  target-i386: cpu_x86_init(): reorder split of CPU string and creation
    of CPU object

Igor Mammedov (1):
  target-i386: move out CPU features initialization to separate func

 target-i386/cpu.c    | 175 ++++++++++++++++++++++++++++++++++-----------------
 target-i386/cpu.h    |   1 -
 target-i386/helper.c |  24 -------
 3 files changed, 118 insertions(+), 82 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 01/17] target-i386/cpu.c: coding style fix

[Eduardo Habkost]

Use spaces instead of tabs on cpu_x86_cpuid().

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e1db639..fa8b5bd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1773,17 +1773,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.7

[Qemu-devel] [PATCH 02/17] target-i386: move cpu_x86_init() to cpu.c

[Eduardo Habkost]

Eventually all of the CPU init code will probably become just a simple
object_new() call, with some arch-independent function that handles the
CPU model string parsing. But right now we need to reorder and split
many of the steps invoved in the CPU model string parsing and CPU object
creation, and it will be easier to do that inside cpu.c, by now.

Also, make cpu_x86_register() static, as now it is only used inside
cpu.c.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index fa8b5bd..b50ca8c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1423,7 +1423,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
@@ -1494,6 +1494,30 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     return 0;
 }
 
+X86CPU *cpu_x86_init(const char *cpu_model)
+{
+    X86CPU *cpu;
+    CPUX86State *env;
+    Error *error = NULL;
+
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    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), &error);
+    if (error) {
+        error_free(error);
+        object_delete(OBJECT(cpu));
+        return NULL;
+    }
+    return cpu;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 
 void cpu_clear_apic_feature(CPUX86State *env)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index cdc59dc..4d5510e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -956,7 +956,6 @@ 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);
-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);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf206cf..47b53ed 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1240,30 +1240,6 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
     return 1;
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
-{
-    X86CPU *cpu;
-    CPUX86State *env;
-    Error *error = NULL;
-
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    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), &error);
-    if (error) {
-        error_free(error);
-        object_delete(OBJECT(cpu));
-        return NULL;
-    }
-    return cpu;
-}
-
 #if !defined(CONFIG_USER_ONLY)
 void do_cpu_init(X86CPU *cpu)
 {
-- 
1.7.11.7

[Qemu-devel] [PATCH 03/17] target-i386: cpu: rename x86_def_t to X86CPUDefinition

[Eduardo Habkost]

Change to match QEMU coding style.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b50ca8c..2dfcc9c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -271,8 +271,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;
@@ -290,7 +290,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 | \
@@ -330,13 +330,14 @@ typedef struct x86_def_t {
 #define TCG_SVM_FEATURES 0
 #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
 
+
 /* 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,
@@ -775,12 +776,12 @@ static int cpu_x86_fill_model_id(char *str)
 }
 #endif
 
-/* Fill a x86_def_t struct with information about the host CPU, and
+/* Fill a X86CPUDefinition struct with information about the host CPU, and
  * the CPU features supported by the host hardware + host kernel
  *
  * This function may be called only if KVM is enabled.
  */
-static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
+static void kvm_cpu_fill_host(X86CPUDefinition *x86_cpu_def)
 {
 #ifdef CONFIG_KVM
     KVMState *s = kvm_state;
@@ -788,7 +789,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     assert(kvm_enabled());
 
-    x86_cpu_def->name = "host";
     host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->vendor1 = ebx;
     x86_cpu_def->vendor2 = edx;
@@ -865,9 +865,9 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
  *
  * This function may be called only if KVM is enabled.
  */
-static int kvm_check_features_against_host(x86_def_t *guest_def)
+static int kvm_check_features_against_host(X86CPUDefinition *guest_def)
 {
-    x86_def_t host_def;
+    X86CPUDefinition host_def;
     uint32_t mask;
     int rv, i;
     struct model_features_t ft[] = {
@@ -1146,10 +1146,11 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
+                                const char *cpu_model)
 {
     unsigned int i;
-    x86_def_t *def;
+    X86CPUDefinition *def;
 
     char *s = g_strdup(cpu_model);
     char *featurestr, *name = strtok(s, ",");
@@ -1355,7 +1356,7 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
 /* generate CPU information. */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    x86_def_t *def;
+    X86CPUDefinition *def;
     char buf[256];
 
     for (def = x86_defs; def; def = def->next) {
@@ -1379,7 +1380,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
-    x86_def_t *def;
+    X86CPUDefinition *def;
 
     for (def = x86_defs; def; def = def->next) {
         CpuDefinitionInfoList *entry;
@@ -1426,7 +1427,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 static 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));
@@ -1535,7 +1536,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;
 
         /* Look for specific "cpudef" models that */
-- 
1.7.11.7

[Qemu-devel] [PATCH 04/17] target-i386: x86_cpudef_setup(): cosmetic change on comment

[Eduardo Habkost]

There are no "cpudef" models, all of them are builtin, now.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2dfcc9c..73b0fa1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,8 +1539,7 @@ void x86_cpudef_setup(void)
         X86CPUDefinition *def = &builtin_x86_defs[i];
         def->next = x86_defs;
 
-        /* Look for specific "cpudef" models that */
-        /* have the QEMU version in .model_id */
+        /* Look for specific 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], def->name) == 0) {
                 pstrcpy(def->model_id, sizeof(def->model_id),
-- 
1.7.11.7

[Qemu-devel] [PATCH 05/17] target-i386: cpu_x86_init(): move error handling to end of function

[Eduardo Habkost]

Doing error handling on a single place will make it easier to make sure
memory is freed, and that error information is properly printed or
returned to the caller.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 73b0fa1..69f1204 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1506,17 +1506,20 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     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), &error);
     if (error) {
-        error_free(error);
-        object_delete(OBJECT(cpu));
-        return NULL;
+        goto error;
     }
     return cpu;
+error:
+    object_delete(OBJECT(cpu));
+    if (error) {
+        error_free(error);
+    }
+    return NULL;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.11.7

[Qemu-devel] [PATCH 06/17] target-i386: cpu_x86_init(): print error message in case of error

[Eduardo Habkost]

Error information is being ignored and never returned to the caller.

While we don't change cpu_x86_init() to not return error information, print
error message inside cpu_x86_init() in case of error.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Use error_report()
 - Add "cpu_x86_init:" prefix to error message
---
 target-i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 69f1204..2907fb0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1517,6 +1517,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 error:
     object_delete(OBJECT(cpu));
     if (error) {
+        error_report("cpu_x86_init: %s", error_get_pretty(error));
         error_free(error);
     }
     return NULL;
-- 
1.7.11.7

[Qemu-devel] [PATCH 07/17] target-i386: cpu_x86_register(): report errors using Error parameter

[Eduardo Habkost]

Do it using a local Error variable and error_propagate(), so we don't
miss any error reported by the property setters in case errp is NULL.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2907fb0..9334e0c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1424,7 +1424,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
 {
     CPUX86State *env = &cpu->env;
     X86CPUDefinition def1, *def = &def1;
@@ -1488,8 +1488,7 @@ static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error) {
-        fprintf(stderr, "%s\n", error_get_pretty(error));
-        error_free(error);
+        error_propagate(errp, error);
         return -1;
     }
     return 0;
@@ -1505,7 +1504,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
+    if (cpu_x86_register(cpu, cpu_model, &error) < 0) {
         goto error;
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 08/17] target-i386: cpu_x86_register(): reorder CPU property setting

[Eduardo Habkost]

Trivial code movement, before moving the code to another function.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9334e0c..7f4e8f0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1448,18 +1448,19 @@ static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
     object_property_set_int(OBJECT(cpu), def->family, "family", &error);
     object_property_set_int(OBJECT(cpu), def->model, "model", &error);
     object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
+    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
+    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);
     env->cpuid_features = def->features;
     env->cpuid_ext_features = def->ext_features;
     env->cpuid_ext2_features = def->ext2_features;
     env->cpuid_ext3_features = def->ext3_features;
-    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
     env->cpuid_kvm_features = def->kvm_features;
     env->cpuid_svm_features = def->svm_features;
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
     env->cpuid_xlevel2 = def->xlevel2;
-    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
-                            "tsc-frequency", &error);
 
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
      * CPUID[1].EDX.
@@ -1486,7 +1487,6 @@ static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
         filter_features_for_kvm(cpu);
 #endif
     }
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error) {
         error_propagate(errp, error);
         return -1;
-- 
1.7.11.7

[Qemu-devel] [PATCH 09/17] target-i386: move out CPU features initialization to separate func

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

Later it will be used in cpu_x86_init() to init CPU from found cpudef.

This is will make it easier to reorder and clean up the cpu_x86_init()
code later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[ehabkost: added error reporting to function]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
   - rebased on top of  "i386: cpu: remove duplicate feature names"
      http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
v3 (ehabkost):
 - Rebased on top of CPU model classes work in progress
 - Added error reporting to new function
 - Changed commit message
---
 target-i386/cpu.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7f4e8f0..cef120e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1424,16 +1424,11 @@ static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
+static int cpudef_2_x86_cpu(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 {
     CPUX86State *env = &cpu->env;
-    X86CPUDefinition def1, *def = &def1;
     Error *error = NULL;
 
-    memset(def, 0, sizeof(*def));
-
-    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;
@@ -1494,6 +1489,19 @@ static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
     return 0;
 }
 
+static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
+{
+    X86CPUDefinition def1, *def = &def1;
+
+    memset(def, 0, sizeof(*def));
+
+    if (cpu_x86_find_by_name(def, cpu_model) < 0)
+        return -1;
+    if (cpudef_2_x86_cpu(cpu, def, errp) < 0)
+        return -1;
+    return 0;
+}
+
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
-- 
1.7.11.7

[Qemu-devel] [PATCH 10/17] target-i386: kill cpu_x86_register()

[Eduardo Habkost]

Move the cpu_x86_register() code inside cpu_x86_init(), as the initialization
steps are going to be reordered.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cef120e..3f04054 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1489,30 +1489,24 @@ static int cpudef_2_x86_cpu(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     return 0;
 }
 
-static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
-{
-    X86CPUDefinition def1, *def = &def1;
-
-    memset(def, 0, sizeof(*def));
-
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
-    if (cpudef_2_x86_cpu(cpu, def, errp) < 0)
-        return -1;
-    return 0;
-}
-
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
     CPUX86State *env;
     Error *error = NULL;
+    X86CPUDefinition def1, *def = &def1;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    if (cpu_x86_register(cpu, cpu_model, &error) < 0) {
+    memset(def, 0, sizeof(*def));
+
+    if (cpu_x86_find_by_name(def, cpu_model) < 0) {
+        goto error;
+    }
+
+    if (cpudef_2_x86_cpu(cpu, def, &error) < 0) {
         goto error;
     }
 
-- 
1.7.11.7

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

[Eduardo Habkost]

It will allow us to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[ehabkost: rebased on top of CPU classes work in progress]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
    - style change, add braces (requested by Blue Swirl)
    - removed unused error_is_set(errp) in properties set loop
v3:
    - removed unnecessary whitespace change

v4 (ehabkost):
    - rebased on top of CPU classes work in progress
    - reworded commit message
---
 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 3f04054..214a292 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1147,7 +1147,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
 }
 
 static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
-                                const char *cpu_model)
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     X86CPUDefinition *def;
@@ -1320,6 +1320,9 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1502,7 +1505,7 @@ X86CPU *cpu_x86_init(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, &error) < 0) {
         goto error;
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 12/17] target-i386: cpu_x86_find_by_name(): split CPU model and feature string first

[Eduardo Habkost]

Instead of using strtok() for the whole cpu_model string, first split it
into the CPU model name and the full feature string, then parse the
feature string into pieces.

When using CPU model classes, those two pieces of information will be
used at different moments (CPU model name will be used to find CPU
class, feature string will be used after CPU object was created), so
making the split in two steps will make it easier to refactor the code
later.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 214a292..3a85989 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1151,9 +1151,10 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
 {
     unsigned int i;
     X86CPUDefinition *def;
-
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
+    char *name; /* CPU model name */
+    char *features; /* Full feature "key=value,..." string */
+    char *featurestr; /* Single 'key=value" string being parsed */
+    gchar **model_pieces; /* array after split of name,features */
     /* Features to be added*/
     uint32_t plus_features = 0, plus_ext_features = 0;
     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
@@ -1166,6 +1167,14 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
     uint32_t minus_7_0_ebx_features = 0;
     uint32_t numvalue;
 
+    model_pieces = g_strsplit(cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        goto error;
+    }
+
+    name = model_pieces[0];
+    features = model_pieces[1];
+
     for (def = x86_defs; def; def = def->next)
         if (name && !strcmp(name, def->name))
             break;
@@ -1181,7 +1190,7 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
             &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
             &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
 
-    featurestr = strtok(NULL, ",");
+    featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
         char *val;
@@ -1315,11 +1324,11 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
     if (x86_cpu_def->cpuid_7_0_ebx_features && x86_cpu_def->level < 7) {
         x86_cpu_def->level = 7;
     }
-    g_free(s);
+    g_strfreev(model_pieces);
     return 0;
 
 error:
-    g_free(s);
+    g_strfreev(model_pieces);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
-- 
1.7.11.7

[Qemu-devel] [PATCH 13/17] target-i386: cpu: create cpu_x86_find_cpudef() function

[Eduardo Habkost]

Move the code that looks for a given CPU model to a separate function.

This will make it easier to separate the cpudef lookup code and the
feature string parsing code, later.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3a85989..c2594bd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1146,6 +1146,33 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+/* Find a CPU model definition and put the result on a X86CPUDefinition struct
+ */
+static int cpu_x86_find_cpudef(const char *name,
+                               X86CPUDefinition *result,
+                               Error **errp)
+{
+    X86CPUDefinition *def;
+
+    for (def = x86_defs; def; def = def->next)
+        if (name && !strcmp(name, def->name))
+            break;
+    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
+        kvm_cpu_fill_host(result);
+    } else if (!def) {
+        goto error;
+    } else {
+        memcpy(result, def, sizeof(*def));
+    }
+    return 0;
+
+error:
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
+    return -1;
+}
+
 static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
@@ -1175,15 +1202,8 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
     name = model_pieces[0];
     features = model_pieces[1];
 
-    for (def = x86_defs; def; def = def->next)
-        if (name && !strcmp(name, def->name))
-            break;
-    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
-        kvm_cpu_fill_host(x86_cpu_def);
-    } else if (!def) {
+    if (cpu_x86_find_cpudef(name, x86_cpu_def, errp) < 0) {
         goto error;
-    } else {
-        memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
     add_flagname_to_bitmaps("hypervisor", &plus_features,
-- 
1.7.11.7

[Qemu-devel] [PATCH 14/17] target-i386: cpu_x86_init(): rename cpu_model to cpu_string

[Eduardo Habkost]

Rename the variable, to avoid confusion between the actual CPU model
name and the -cpu string argument (that may contain additional
parameters).

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 c2594bd..57acf3a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1521,7 +1521,7 @@ static int cpudef_2_x86_cpu(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     return 0;
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
+X86CPU *cpu_x86_init(const char *cpu_string)
 {
     X86CPU *cpu;
     CPUX86State *env;
@@ -1530,11 +1530,11 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
-    env->cpu_model_str = cpu_model;
+    env->cpu_model_str = cpu_string;
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model, &error) < 0) {
+    if (cpu_x86_find_by_name(def, cpu_string, &error) < 0) {
         goto error;
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 15/17] target-i386: cpu_x86_init(): eliminate extra 'def1' variable

[Eduardo Habkost]

Just use '&def' where a pointer to the under-construction
X86CPUDefinition struct is being used.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 57acf3a..9b8e480 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1526,19 +1526,19 @@ X86CPU *cpu_x86_init(const char *cpu_string)
     X86CPU *cpu;
     CPUX86State *env;
     Error *error = NULL;
-    X86CPUDefinition def1, *def = &def1;
+    X86CPUDefinition def;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
     env->cpu_model_str = cpu_string;
 
-    memset(def, 0, sizeof(*def));
+    memset(&def, 0, sizeof(def));
 
-    if (cpu_x86_find_by_name(def, cpu_string, &error) < 0) {
+    if (cpu_x86_find_by_name(&def, cpu_string, &error) < 0) {
         goto error;
     }
 
-    if (cpudef_2_x86_cpu(cpu, def, &error) < 0) {
+    if (cpudef_2_x86_cpu(cpu, &def, &error) < 0) {
         goto error;
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 16/17] target-i386: cpu: separate cpudef lookup from feature string parsing

[Eduardo Habkost]

- Move the CPU string split and cpudef lookup to cpu_x86_init();
- Rename cpu_x86_find_by_name() to cpu_x86_parse_feature_string(),
  and make it just get the feature string as input.

This will allow us to use the CPU model name for the CPU class lookup,
inside cpu_x86_init().

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9b8e480..e47ec5d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1173,15 +1173,11 @@ error:
     return -1;
 }
 
-static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
-                                const char *cpu_model, Error **errp)
+static int cpu_x86_parse_featurestr(X86CPUDefinition *x86_cpu_def,
+                                    char *features, Error **errp)
 {
     unsigned int i;
-    X86CPUDefinition *def;
-    char *name; /* CPU model name */
-    char *features; /* Full feature "key=value,..." string */
     char *featurestr; /* Single 'key=value" string being parsed */
-    gchar **model_pieces; /* array after split of name,features */
     /* Features to be added*/
     uint32_t plus_features = 0, plus_ext_features = 0;
     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
@@ -1194,18 +1190,6 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
     uint32_t minus_7_0_ebx_features = 0;
     uint32_t numvalue;
 
-    model_pieces = g_strsplit(cpu_model, ",", 2);
-    if (!model_pieces[0]) {
-        goto error;
-    }
-
-    name = model_pieces[0];
-    features = model_pieces[1];
-
-    if (cpu_x86_find_cpudef(name, x86_cpu_def, errp) < 0) {
-        goto error;
-    }
-
     add_flagname_to_bitmaps("hypervisor", &plus_features,
             &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
             &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
@@ -1344,11 +1328,9 @@ static int cpu_x86_find_by_name(X86CPUDefinition *x86_cpu_def,
     if (x86_cpu_def->cpuid_7_0_ebx_features && x86_cpu_def->level < 7) {
         x86_cpu_def->level = 7;
     }
-    g_strfreev(model_pieces);
     return 0;
 
 error:
-    g_strfreev(model_pieces);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1527,6 +1509,8 @@ X86CPU *cpu_x86_init(const char *cpu_string)
     CPUX86State *env;
     Error *error = NULL;
     X86CPUDefinition def;
+    char *name, *features;
+    gchar **model_pieces;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
@@ -1534,7 +1518,18 @@ X86CPU *cpu_x86_init(const char *cpu_string)
 
     memset(&def, 0, sizeof(def));
 
-    if (cpu_x86_find_by_name(&def, cpu_string, &error) < 0) {
+    model_pieces = g_strsplit(cpu_string, ",", 2);
+    if (!model_pieces[0]) {
+        goto error;
+    }
+    name = model_pieces[0];
+    features = model_pieces[1];
+
+    if (cpu_x86_find_cpudef(name, &def, &error) < 0) {
+        goto error;
+    }
+
+    if (cpu_x86_parse_featurestr(&def, features, &error) < 0) {
         goto error;
     }
 
@@ -1546,8 +1541,11 @@ X86CPU *cpu_x86_init(const char *cpu_string)
     if (error) {
         goto error;
     }
+
+    g_strfreev(model_pieces);
     return cpu;
 error:
+    g_strfreev(model_pieces);
     object_delete(OBJECT(cpu));
     if (error) {
         error_report("cpu_x86_init: %s", error_get_pretty(error));
-- 
1.7.11.7

[Qemu-devel] [PATCH 17/17] target-i386: cpu_x86_init(): reorder split of CPU string and creation of CPU object

[Eduardo Habkost]

A step towards making the creation of CPU objects use the CPU model name
as class name.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e47ec5d..5f2ce7d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1505,19 +1505,13 @@ static int cpudef_2_x86_cpu(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
 X86CPU *cpu_x86_init(const char *cpu_string)
 {
-    X86CPU *cpu;
+    X86CPU *cpu = NULL;
     CPUX86State *env;
     Error *error = NULL;
     X86CPUDefinition def;
     char *name, *features;
     gchar **model_pieces;
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_string;
-
-    memset(&def, 0, sizeof(def));
-
     model_pieces = g_strsplit(cpu_string, ",", 2);
     if (!model_pieces[0]) {
         goto error;
@@ -1525,6 +1519,12 @@ X86CPU *cpu_x86_init(const char *cpu_string)
     name = model_pieces[0];
     features = model_pieces[1];
 
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_string;
+
+    memset(&def, 0, sizeof(def));
+
     if (cpu_x86_find_cpudef(name, &def, &error) < 0) {
         goto error;
     }
@@ -1546,7 +1546,9 @@ X86CPU *cpu_x86_init(const char *cpu_string)
     return cpu;
 error:
     g_strfreev(model_pieces);
-    object_delete(OBJECT(cpu));
+    if (cpu) {
+        object_delete(OBJECT(cpu));
+    }
     if (error) {
         error_report("cpu_x86_init: %s", error_get_pretty(error));
         error_free(error);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 09/17] target-i386: move out CPU features initialization to separate func

[Blue Swirl]

On Mon, Nov 12, 2012 at 9:38 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> From: Igor Mammedov <imammedo@redhat.com>
>
> Later it will be used in cpu_x86_init() to init CPU from found cpudef.
>
> This is will make it easier to reorder and clean up the cpu_x86_init()
> code later.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> [ehabkost: added error reporting to function]
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v2:
>    - rebased on top of  "i386: cpu: remove duplicate feature names"
>       http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
> v3 (ehabkost):
>  - Rebased on top of CPU model classes work in progress
>  - Added error reporting to new function
>  - Changed commit message
> ---
>  target-i386/cpu.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7f4e8f0..cef120e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1424,16 +1424,11 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  }
>  #endif
>
> -static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
> +static int cpudef_2_x86_cpu(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>  {
>      CPUX86State *env = &cpu->env;
> -    X86CPUDefinition def1, *def = &def1;
>      Error *error = NULL;
>
> -    memset(def, 0, sizeof(*def));
> -
> -    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;
> @@ -1494,6 +1489,19 @@ static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
>      return 0;
>  }
>
> +static int cpu_x86_register(X86CPU *cpu, const char *cpu_model, Error **errp)
> +{
> +    X86CPUDefinition def1, *def = &def1;
> +
> +    memset(def, 0, sizeof(*def));
> +
> +    if (cpu_x86_find_by_name(def, cpu_model) < 0)

Please add braces, also below.

> +        return -1;
> +    if (cpudef_2_x86_cpu(cpu, def, errp) < 0)
> +        return -1;
> +    return 0;
> +}
> +
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
> --
> 1.7.11.7
>
>

Re: [Qemu-devel] [PATCH 02/17] target-i386: move cpu_x86_init() to cpu.c

[li guang]

在 2012-11-12一的 19:38 -0200，Eduardo Habkost写道：
> Eventually all of the CPU init code will probably become just a simple
> object_new() call, with some arch-independent function that handles the
> CPU model string parsing. But right now we need to reorder and split
> many of the steps invoved in the CPU model string parsing and CPU object
> creation, and it will be easier to do that inside cpu.c, by now.
> 
> Also, make cpu_x86_register() static, as now it is only used inside
> cpu.c.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c    | 26 +++++++++++++++++++++++++-
>  target-i386/cpu.h    |  1 -
>  target-i386/helper.c | 24 ------------------------
>  3 files changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index fa8b5bd..b50ca8c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1423,7 +1423,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  }
>  #endif
>  
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> +static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
>      CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
> @@ -1494,6 +1494,30 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      return 0;
>  }
>  
> +X86CPU *cpu_x86_init(const char *cpu_model)
> +{
> +    X86CPU *cpu;
> +    CPUX86State *env;
> +    Error *error = NULL;

seems env unused anymore

> +
> +    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), &error);
> +    if (error) {
> +        error_free(error);
> +        object_delete(OBJECT(cpu));
> +        return NULL;
> +    }
> +    return cpu;
> +}
> +
>  #if !defined(CONFIG_USER_ONLY)
>  
>  void cpu_clear_apic_feature(CPUX86State *env)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index cdc59dc..4d5510e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -956,7 +956,6 @@ 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);
> -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);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf206cf..47b53ed 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1240,30 +1240,6 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>      return 1;
>  }
>  
> -X86CPU *cpu_x86_init(const char *cpu_model)
> -{
> -    X86CPU *cpu;
> -    CPUX86State *env;
> -    Error *error = NULL;
> -
> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> -    env = &cpu->env;
> -    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), &error);
> -    if (error) {
> -        error_free(error);
> -        object_delete(OBJECT(cpu));
> -        return NULL;
> -    }
> -    return cpu;
> -}
> -
>  #if !defined(CONFIG_USER_ONLY)
>  void do_cpu_init(X86CPU *cpu)
>  {

-- 
regards!
li guang                  
linux kernel team at FNST, china

thinking with brain but heart
living with heart but brain

Re: [Qemu-devel] [PATCH 05/17] target-i386: cpu_x86_init(): move error handling to end of function

[li guang]

在 2012-11-12一的 19:38 -0200，Eduardo Habkost写道：
> Doing error handling on a single place will make it easier to make sure
> memory is freed, and that error information is properly printed or
> returned to the caller.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 73b0fa1..69f1204 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1506,17 +1506,20 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      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), &error);
>      if (error) {
> -        error_free(error);
> -        object_delete(OBJECT(cpu));
> -        return NULL;
> +        goto error;
>      }
>      return cpu;
> +error:

seems we should not use lable 'error' as it's 
the same name with variable 'error'

hmm, maybe I should send a patch to clarify.
 

> +    object_delete(OBJECT(cpu));
> +    if (error) {
> +        error_free(error);
> +    }
> +    return NULL;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)

-- 
regards!
li guang                  
linux kernel team at FNST, china

thinking with brain but heart
living with heart but brain

Re: [Qemu-devel] [PATCH 05/17] target-i386: cpu_x86_init(): move error handling to end of function

[Eduardo Habkost]

On Wed, Nov 28, 2012 at 02:08:01PM +0800, li guang wrote:
> 在 2012-11-12一的 19:38 -0200，Eduardo Habkost写道：
> > Doing error handling on a single place will make it easier to make sure
> > memory is freed, and that error information is properly printed or
> > returned to the caller.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 73b0fa1..69f1204 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1506,17 +1506,20 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >      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), &error);
> >      if (error) {
> > -        error_free(error);
> > -        object_delete(OBJECT(cpu));
> > -        return NULL;
> > +        goto error;
> >      }
> >      return cpu;
> > +error:
> 
> seems we should not use lable 'error' as it's 
> the same name with variable 'error'

Do you know any compiler that complains about that? I don't find this to
be a problem at all.

> 
> hmm, maybe I should send a patch to clarify.
>  
> 
> > +    object_delete(OBJECT(cpu));
> > +    if (error) {
> > +        error_free(error);
> > +    }
> > +    return NULL;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> 
> -- 
> regards!
> li guang                  
> linux kernel team at FNST, china
> 
> thinking with brain but heart
> living with heart but brain
> 

-- 
Eduardo