Patchwork [20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

login
register
mail settings
Submitter Eduardo Habkost
Date Oct. 24, 2012, 5:49 p.m.
Message ID <1351101001-14589-21-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/193907/
State New
Headers show

Comments

Eduardo Habkost - Oct. 24, 2012, 5:49 p.m.
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
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(-)
Igor Mammedov - Oct. 31, 2012, 4:32 p.m.
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;
>  }
>
Andreas Färber - Oct. 31, 2012, 4:43 p.m.
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
series (that now depend on qdev, which I find rather nasty to review),
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;
>>  }
>>  
>
Eduardo Habkost - Oct. 31, 2012, 5:01 p.m.
On Wed, Oct 31, 2012 at 05:32:33PM +0100, Igor Mammedov wrote:
> 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();

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 <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;
> >  }
> >  
>
Eduardo Habkost - Oct. 31, 2012, 5:10 p.m.
On Wed, Oct 31, 2012 at 05:43:48PM +0100, Andreas Färber 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
> series (that now depend on qdev, which I find rather nasty to review),
> but I have my hands quite full currently, no promises...

I always planned to send the CPU classes code after the CPU properties
are in, but maybe we can reverse the order.

I am not sure if we have time for that. What would be a reasonable
deadline to have a CPU classes series submitted, so it would be
reasonable/feasible to review it and get it into 1.3? Yesterday?  ;-)

> 
> 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;
> >>  }
> >>  
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Igor Mammedov - Nov. 1, 2012, 12:53 p.m.
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;
> >>  }
> >>  
> > 
> 
>

Patch

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