| Message ID | 1355336183-18858-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 12 Dec 2012 16:16:22 -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(). > > 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 | 16 +++++++++++++--- > target-i386/cpu.h | 2 +- > target-i386/helper.c | 9 ++------- > 3 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 3b9bbfe..b242bf1 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1539,13 +1539,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) feature format of cpu_model string misses just 'feature' > + */ > +X86CPU *cpu_x86_create(const char *cpu_model) > { > + 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)); Could we put this after cpu_x86_parse_featurestr(), it's really not needed before it now and eventually we would like to move it there anyway. > + env = &cpu->env; > + env->cpu_model_str = cpu_model; > + > memset(def, 0, sizeof(*def)); > > model_pieces = g_strsplit(cpu_model, ",", 2); > @@ -1578,10 +1587,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 Fri, Dec 14, 2012 at 11:26:55AM +0100, Igor Mammedov wrote: > On Wed, 12 Dec 2012 16:16:22 -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(). > > > > 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 | 16 +++++++++++++--- > > target-i386/cpu.h | 2 +- > > target-i386/helper.c | 9 ++------- > > 3 files changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 3b9bbfe..b242bf1 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1539,13 +1539,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) > feature format of cpu_model string misses just 'feature' Thanks. I'll change it. > > > + */ > > +X86CPU *cpu_x86_create(const char *cpu_model) > > { > > + 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)); > Could we put this after cpu_x86_parse_featurestr(), it's really not needed > before it now and eventually we would like to move it there anyway. I believe we want to call cpu_x86_parse_featurestr() _after_ the CPU object is created, don't we? Because one day the feature string is going be used to set properties in the CPU object. But we can move object_new() after g_strsplit() (because evantually the CPU object creation is going to use the first part of cpu_model to lookup the class name). > > > + env = &cpu->env; > > + env->cpu_model_str = cpu_model; > > + > > memset(def, 0, sizeof(*def)); > > > > model_pieces = g_strsplit(cpu_model, ",", 2); > > @@ -1578,10 +1587,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 > > > > > -- > Regards, > Igor
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 3b9bbfe..b242bf1 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1539,13 +1539,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) { + 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); @@ -1578,10 +1587,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 | 16 +++++++++++++--- target-i386/cpu.h | 2 +- target-i386/helper.c | 9 ++------- 3 files changed, 16 insertions(+), 11 deletions(-)