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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 31 Oct 2012 17:43:48 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 31.10.2012 17:32, schrieb Igor Mammedov:
> > On Wed, 24 Oct 2012 15:49:54 -0200
> > Eduardo Habkost <ehabkost@redhat.com> 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();
> > 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.
> 
> To my regret, CPU subclasses have moved to the end of your two queues. I
> was considering doing a proposal to fast-track that for symmetry with
> the other targets and gradually improve that with the pending properties
I could revive not qdev based version of cpu properties so we could move
forward if Anthony won't object (he argued in favor of qdev based one on IRC). 
> series (that now depend on qdev, which I find rather nasty to review),
Is there anything that could be done to improve review-ability of cpu-as-qdev
> but I have my hands quite full currently, no promises...
> 
> Andreas
> 
> > 
> >> 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 <ehabkost@redhat.com>
> >> ---
> >>  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;
> >>  }
> >>  
> > 
> 
>