Re: [Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 02:33:24PM +0100, Igor Mammedov wrote:
> On Mon, 10 Dec 2012 22:30:42 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >      }
> >  
> 

-- 
Eduardo