| Message ID | 1355185843-19682-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 10, 2012 at 10:30:42PM -0200, Eduardo Habkost wrote: > As we will need to create the CPU object after splitting the CPU model > string (because we're going to use different subclasses for each CPU > model), move the CPU object creation to cpu_x86_register(), and at the > same time rename cpu_x86_register() to cpu_x86_create(). > > This will also simplify the CPU creation code to a trivial > cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for > code that have to set additional properties before cpu_x86_realize() is > called (e.g. the PC CPU initialization code, that needs to set APIC IDs > depending on the CPU cores/threads topology). > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Igor just sent "[PATCH 0/6] x86 CPU cleanup (wave 2)", that conflicts a little with this series. As the series he sent is larger and some of the patches got some reviews and comments previously, I will rebase this on top of his series and resubmit. > --- > target-i386/cpu.c | 17 +++++++++++++---- > target-i386/cpu.h | 2 +- > target-i386/helper.c | 9 ++------- > 3 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 7be3ad8..044e2d9 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu) > } > #endif > > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > +/* Create and initialize a X86CPU object, based on the full CPU model string > + * (that may include "+feature,-feature,feature=xxx" feature strings) > + */ > +X86CPU *cpu_x86_create(const char *cpu_model) > { > - CPUX86State *env = &cpu->env; > + X86CPU *cpu; > + CPUX86State *env; > x86_def_t def1, *def = &def1; > Error *error = NULL; > char *name, *features; > gchar **model_pieces; > > + cpu = X86_CPU(object_new(TYPE_X86_CPU)); > + env = &cpu->env; > + env->cpu_model_str = cpu_model; > + > memset(def, 0, sizeof(*def)); > > model_pieces = g_strsplit(cpu_model, ",", 2); > @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > } > > g_strfreev(model_pieces); > - return 0; > + return cpu; > error: > + object_delete(OBJECT(cpu)); > g_strfreev(model_pieces); > - return -1; > + return NULL; > } > > #if !defined(CONFIG_USER_ONLY) > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 386c4f6..3ebaae9 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -980,7 +980,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); > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model); > +X86CPU *cpu_x86_create(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..23af4a8 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, > 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)); > + cpu = cpu_x86_create(cpu_model); > + if (!cpu) { > return NULL; > } > > -- > 1.7.11.7 > >
On Mon, 10 Dec 2012 22:30:42 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > As we will need to create the CPU object after splitting the CPU model > string (because we're going to use different subclasses for each CPU > model), move the CPU object creation to cpu_x86_register(), and at the > same time rename cpu_x86_register() to cpu_x86_create(). perhaps it would be better to move cpu_x86_init() inside cpu.c and embed cpu_x86_register() in it, as you have done in one of yours series, and avoid creating intermediate cpu_x86_create() when the end result of cpu subclasses & properties should become a simple cpu_x86_init(): 1. lookup class name, 2. get cpu instance , 3. set properties, 4. realize() > > This will also simplify the CPU creation code to a trivial > cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for > code that have to set additional properties before cpu_x86_realize() is > called (e.g. the PC CPU initialization code, that needs to set APIC IDs > depending on the CPU cores/threads topology). > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 17 +++++++++++++---- > target-i386/cpu.h | 2 +- > target-i386/helper.c | 9 ++------- > 3 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 7be3ad8..044e2d9 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu) > } > #endif > > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > +/* Create and initialize a X86CPU object, based on the full CPU model > string > + * (that may include "+feature,-feature,feature=xxx" feature strings) > + */ > +X86CPU *cpu_x86_create(const char *cpu_model) > { > - CPUX86State *env = &cpu->env; > + X86CPU *cpu; > + CPUX86State *env; > x86_def_t def1, *def = &def1; > Error *error = NULL; > char *name, *features; > gchar **model_pieces; > > + cpu = X86_CPU(object_new(TYPE_X86_CPU)); > + env = &cpu->env; > + env->cpu_model_str = cpu_model; > + > memset(def, 0, sizeof(*def)); > > model_pieces = g_strsplit(cpu_model, ",", 2); > @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char > *cpu_model) } > > g_strfreev(model_pieces); > - return 0; > + return cpu; > error: > + object_delete(OBJECT(cpu)); > g_strfreev(model_pieces); > - return -1; > + return NULL; > } > > #if !defined(CONFIG_USER_ONLY) > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 386c4f6..3ebaae9 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -980,7 +980,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); > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model); > +X86CPU *cpu_x86_create(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..23af4a8 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, > unsigned int selector, 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)); > + cpu = cpu_x86_create(cpu_model); > + if (!cpu) { > return NULL; > } >
On Tue, Dec 11, 2012 at 02:33:24PM +0100, Igor Mammedov wrote: > On Mon, 10 Dec 2012 22:30:42 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > As we will need to create the CPU object after splitting the CPU model > > string (because we're going to use different subclasses for each CPU > > model), move the CPU object creation to cpu_x86_register(), and at the > > same time rename cpu_x86_register() to cpu_x86_create(). > perhaps it would be better to move cpu_x86_init() inside cpu.c and embed > cpu_x86_register() in it, as you have done in one of yours series, and avoid > creating intermediate cpu_x86_create() when the end result of cpu subclasses > & properties should become a simple cpu_x86_init(): > 1. lookup class name, 2. get cpu instance , 3. set properties, 4. realize() I don't think cpu_x86_create() is an intermediate code, it is becoming what will be the final CPU creation code. cpu_x86_create() is now doing all steps you suggest above above, except for step 4 (realize). But leaving the realize() call outside of cpu.c is actually a feature, because some code will need to do additional initialization steps between steps 3 and 4 (so that code will become a simple create()+[additional steps]+realize() function). I believe that moving the object creation to cpu.c only (instead of the whole cpu_x86_init() function) ended up being much simpler than the original approach I used (that was moving the entire cpu_x86_init() function to cpu.c). cpu_x86_init() is now just a create()+realize() helper, and nothing else, and I believe we won't need to touch it anymore except at some point in the future when (hopefully) we will kill it and replace with a generic target-independent function. > > > > > This will also simplify the CPU creation code to a trivial > > cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for > > code that have to set additional properties before cpu_x86_realize() is > > called (e.g. the PC CPU initialization code, that needs to set APIC IDs > > depending on the CPU cores/threads topology). > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 17 +++++++++++++---- > > target-i386/cpu.h | 2 +- > > target-i386/helper.c | 9 ++------- > > 3 files changed, 16 insertions(+), 12 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 7be3ad8..044e2d9 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu) > > } > > #endif > > > > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > +/* Create and initialize a X86CPU object, based on the full CPU model > > string > > + * (that may include "+feature,-feature,feature=xxx" feature strings) > > + */ > > +X86CPU *cpu_x86_create(const char *cpu_model) > > { > > - CPUX86State *env = &cpu->env; > > + X86CPU *cpu; > > + CPUX86State *env; > > x86_def_t def1, *def = &def1; > > Error *error = NULL; > > char *name, *features; > > gchar **model_pieces; > > > > + cpu = X86_CPU(object_new(TYPE_X86_CPU)); > > + env = &cpu->env; > > + env->cpu_model_str = cpu_model; > > + > > memset(def, 0, sizeof(*def)); > > > > model_pieces = g_strsplit(cpu_model, ",", 2); > > @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char > > *cpu_model) } > > > > g_strfreev(model_pieces); > > - return 0; > > + return cpu; > > error: > > + object_delete(OBJECT(cpu)); > > g_strfreev(model_pieces); > > - return -1; > > + return NULL; > > } > > > > #if !defined(CONFIG_USER_ONLY) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index 386c4f6..3ebaae9 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -980,7 +980,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); > > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model); > > +X86CPU *cpu_x86_create(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..23af4a8 100644 > > --- a/target-i386/helper.c > > +++ b/target-i386/helper.c > > @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, > > unsigned int selector, 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)); > > + cpu = cpu_x86_create(cpu_model); > > + if (!cpu) { > > return NULL; > > } > > >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 7be3ad8..044e2d9 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu) } #endif -int cpu_x86_register(X86CPU *cpu, const char *cpu_model) +/* Create and initialize a X86CPU object, based on the full CPU model string + * (that may include "+feature,-feature,feature=xxx" feature strings) + */ +X86CPU *cpu_x86_create(const char *cpu_model) { - CPUX86State *env = &cpu->env; + X86CPU *cpu; + CPUX86State *env; x86_def_t def1, *def = &def1; Error *error = NULL; char *name, *features; gchar **model_pieces; + cpu = X86_CPU(object_new(TYPE_X86_CPU)); + env = &cpu->env; + env->cpu_model_str = cpu_model; + memset(def, 0, sizeof(*def)); model_pieces = g_strsplit(cpu_model, ",", 2); @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) } g_strfreev(model_pieces); - return 0; + return cpu; error: + object_delete(OBJECT(cpu)); g_strfreev(model_pieces); - return -1; + return NULL; } #if !defined(CONFIG_USER_ONLY) diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 386c4f6..3ebaae9 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -980,7 +980,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); -int cpu_x86_register(X86CPU *cpu, const char *cpu_model); +X86CPU *cpu_x86_create(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..23af4a8 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, 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)); + cpu = cpu_x86_create(cpu_model); + if (!cpu) { return NULL; }
As we will need to create the CPU object after splitting the CPU model string (because we're going to use different subclasses for each CPU model), move the CPU object creation to cpu_x86_register(), and at the same time rename cpu_x86_register() to cpu_x86_create(). This will also simplify the CPU creation code to a trivial cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for code that have to set additional properties before cpu_x86_realize() is called (e.g. the PC CPU initialization code, that needs to set APIC IDs depending on the CPU cores/threads topology). Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 17 +++++++++++++---- target-i386/cpu.h | 2 +- target-i386/helper.c | 9 ++------- 3 files changed, 16 insertions(+), 12 deletions(-)