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, 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.

> 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;
>  }
>