Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Eduardo Habkost]

On Wed, Oct 31, 2012 at 05:32:33PM +0100, Igor Mammedov wrote:
> On Wed, 24 Oct 2012 15:49:54 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > The PC code will need to run additional steps when initializing the CPU
> > object, before x86_cpu_realize(). So, make cpu_x86_init() not call
> Killing cpu_x86_init() altogether will make future re-factoring even easier.
> For present its code could be duplicated in cpu_init() and pc.c,
> 
> and with cpu subclasses cpu_init () would be reduced to
>   cpu = object_new(X86CPU.QEMUxx);
>   cpu.realize();

I suspect *-user supports +feature,-feature on the CPU model string as
well. So both cases the cpu_model compat string parsing/property-setting
and the CPU class lookup would be necessary. So cpu_init() wouldn't look
so simple as above.


> and pc_cpus_init()
>   cpu = object_new(X86CPU.QEMUxx);
>   make cpu a child of /machine ();
>   apply custom properties ();
>   cpu.realize();
> 
> I don't see any benefits in keeping cpu_x86_init() around and if we start
> touching it then just lets get rid of it in one step.

I believe the sequence that creates the CPU object will look like this
on *-user:

  compat_normalize_cpu_model(cpu_model, &class_name, &features);
  class = type_get_by_name(class_name);
  cpu = object_new(class);
  cpu_x86_set_props(cpu, features);
  cpu.realize();

And on PC it will look like:
  
  compat_normalize_cpu_model(cpu_model, &class_name, &features);
  class = type_get_by_name(class_name);
  cpu = object_new(class);
  cpu_x86_set_props(cpu, features);
  cpu_init_steps_sepcific_for_pc(cpu); /* APIC ID, make a child of
                                        * /machine, whatever else.
                                        */
  cpu.realize();

If cpu_init() was going to be just object_new() + cpu.realize(), I
wouldn't mind duplicating the code. But I don't see a reason to
duplicate code and not use a common function for the "cpu_model parsing
+ class lookup + object_new() + compat props setting" steps. I think
cpu_x86_init() can be that common function.


> 
> > x86_cpu_realize(), and add two x86_cpu_realize() calls:
> > 
> > - One on cpu_init(), that is called only by *-user
> > - One on pc_cpu_init(), that will include the more advanced PC CPU
> >   initialization steps
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  hw/pc.c              | 12 +++++++++++-
> >  target-i386/cpu.h    | 14 ++++++++++++++
> >  target-i386/helper.c | 11 ++++-------
> >  3 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 85eab04..c209d3d 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> > level) 
> >  static void pc_cpu_init(PCInitArgs *args, int cpu_index)
> >  {
> > -    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
> > +    Error *err = NULL;
> > +    X86CPU *cpu;
> > +
> > +    cpu = cpu_x86_init(args->qemu_args->cpu_model);
> > +    if (!cpu) {
> >          fprintf(stderr, "Unable to find x86 CPU definition\n");
> >          exit(1);
> >      }
> > +
> > +    x86_cpu_realize(OBJECT(cpu), &err);
> > +    if (err) {
> > +        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> > +        exit(1);
> > +    }
> >  }
> >  
> >  void pc_cpus_init(PCInitArgs *args)
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 871c270..6853b17 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -21,6 +21,7 @@
> >  
> >  #include "config.h"
> >  #include "qemu-common.h"
> > +#include "qemu-error.h"
> >  
> >  #ifdef TARGET_X86_64
> >  #define TARGET_LONG_BITS 64
> > @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
> >  #define TARGET_VIRT_ADDR_SPACE_BITS 32
> >  #endif
> >  
> > +/* Helper for simple CPU initialization (for target-independent code)
> > + *
> > + * Note that the PC code doesn't use this function, as it does additional
> > + * initialization steps between cpu_x86_init() and cpu_x86_realize() is
> > called.
> > + */
> >  static inline CPUX86State *cpu_init(const char *cpu_model)
> >  {
> > +    Error *err = NULL;
> >      X86CPU *cpu = cpu_x86_init(cpu_model);
> >      if (cpu == NULL) {
> >          return NULL;
> >      }
> > +
> > +    x86_cpu_realize(OBJECT(cpu), &err);
> > +    if (err) {
> > +        error_report("cpu_init: %s\n", error_get_pretty(err));
> > +        return NULL;
> > +    }
> > +
> >      return &cpu->env;
> >  }
> >  
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index 1e5f61f..87a9221 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> > unsigned int selector, return 1;
> >  }
> >  
> > +/* Initialize X86CPU object
> > + *
> > + * Callers must eventually call x86_cpu_realize(), to finish
> > initialization.
> > + */
> >  X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> >      X86CPU *cpu;
> >      CPUX86State *env;
> > -    Error *err = NULL;
> >  
> >      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >      env = &cpu->env;
> > @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >          return NULL;
> >      }
> >  
> > -    x86_cpu_realize(OBJECT(cpu), &err);
> > -    if (err) {
> > -        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
> > -        return NULL;
> > -    }
> > -
> >      return cpu;
> >  }
> >  
> 

-- 
Eduardo