On Wed, 31 Oct 2012 17:43:48 +0100
Andreas Färber <afaer...@suse.de> wrote:
> Am 31.10.2012 17:32, schrieb Igor Mammedov:
> > On Wed, 24 Oct 2012 15:49:54 -0200
> > Eduardo Habkost <ehabk...@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 <ehabk...@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;
> >> }
> >>
> >
>
>
