| Message ID | 1358666571-1737-4-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Sun, Jan 20, 2013 at 08:22:26AM +0100, Andreas Färber wrote: > Turn arm_cpu_realize() into a QOM realize function, no longer called > via cpu.h prototype. To maintain the semantics of cpu_init(), set > realized = true explicitly in cpu_arm_init(). > > Move GDB coprocessor registration, CPU reset and vCPU initialization > into the realizefn. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > target-arm/cpu-qom.h | 3 ++- > target-arm/cpu.c | 21 ++++++++++++++------- > target-arm/cpu.h | 1 + > target-arm/helper.c | 14 ++++++++++---- > 4 Dateien geändert, 27 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-) > > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h > index 0f455c4..aff7bf3 100644 > --- a/target-arm/cpu-qom.h > +++ b/target-arm/cpu-qom.h > @@ -33,6 +33,7 @@ > > /** > * ARMCPUClass: > + * @parent_realize: The parent class' realize handler. > * @parent_reset: The parent class' reset handler. > * > * An ARM CPU model. > @@ -42,6 +43,7 @@ typedef struct ARMCPUClass { > CPUClass parent_class; > /*< public >*/ > > + DeviceRealize parent_realize; > void (*parent_reset)(CPUState *cpu); > } ARMCPUClass; > > @@ -107,7 +109,6 @@ static inline ARMCPU *arm_env_get_cpu(CPUARMState *env) > > #define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e)) > > -void arm_cpu_realize(ARMCPU *cpu); > void register_cp_regs_for_features(ARMCPU *cpu); > > #endif > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 07588a1..19d5ae4 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -147,15 +147,12 @@ static void arm_cpu_finalizefn(Object *obj) > g_hash_table_destroy(cpu->cp_regs); > } > > -void arm_cpu_realize(ARMCPU *cpu) > +static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > { > - /* This function is called by cpu_arm_init() because it > - * needs to do common actions based on feature bits, etc > - * that have been set by the subclass init functions. > - * When we have QOM realize support it should become > - * a true realize function instead. > - */ > + ARMCPU *cpu = ARM_CPU(dev); > + ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev); > CPUARMState *env = &cpu->env; > + > /* Some features automatically imply others: */ > if (arm_feature(env, ARM_FEATURE_V7)) { > set_feature(env, ARM_FEATURE_VAPA); > @@ -197,6 +194,12 @@ void arm_cpu_realize(ARMCPU *cpu) > } > > register_cp_regs_for_features(cpu); > + arm_cpu_register_gdb_regs_for_features(cpu); > + > + cpu_reset(CPU(cpu)); > + qemu_init_vcpu(env); > + > + acc->parent_realize(dev, errp); > } > > /* CPU models */ > @@ -763,6 +766,10 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) > { > ARMCPUClass *acc = ARM_CPU_CLASS(oc); > CPUClass *cc = CPU_CLASS(acc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + acc->parent_realize = dc->realize; > + dc->realize = arm_cpu_realizefn; > > acc->parent_reset = cc->reset; > cc->reset = arm_cpu_reset; > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index ffddfcb..2902ba5 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -234,6 +234,7 @@ typedef struct CPUARMState { > > ARMCPU *cpu_arm_init(const char *cpu_model); > void arm_translate_init(void); > +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu); > int cpu_arm_exec(CPUARMState *s); > void do_interrupt(CPUARMState *); > void switch_mode(CPUARMState *, int); > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 37c34a1..f412143 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -1270,14 +1270,22 @@ ARMCPU *cpu_arm_init(const char *cpu_model) > cpu = ARM_CPU(object_new(cpu_model)); > env = &cpu->env; > env->cpu_model_str = cpu_model; > - arm_cpu_realize(cpu); > + > + /* TODO this should be set centrally, once possible */ > + object_property_set_bool(OBJECT(cpu), true, "realized", NULL); > > if (tcg_enabled() && !inited) { > inited = 1; > arm_translate_init(); > } My first question I had when I saw this was: are you really sure it is safe to call cpu_reset() and qemu_init_vcpu() before arm_translate_init()? But I see that you change this on commit 092028dbf1. So now I have the opposite question: are you really sure it is safe to call arm_translate_init() before arm_cpu_realizefn()? > > - cpu_reset(CPU(cpu)); > + return cpu; > +} > + > +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) > +{ > + CPUARMState *env = &cpu->env; > + > if (arm_feature(env, ARM_FEATURE_NEON)) { > gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg, > 51, "arm-neon.xml", 0); > @@ -1288,8 +1296,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model) > gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg, > 19, "arm-vfp.xml", 0); > } > - qemu_init_vcpu(env); > - return cpu; > } > > /* Sort alphabetically by type name, except for "any". */ > -- > 1.7.10.4 > >
Am 07.02.2013 17:31, schrieb Eduardo Habkost: > On Sun, Jan 20, 2013 at 08:22:26AM +0100, Andreas Färber wrote: >> Turn arm_cpu_realize() into a QOM realize function, no longer called >> via cpu.h prototype. To maintain the semantics of cpu_init(), set >> realized = true explicitly in cpu_arm_init(). >> >> Move GDB coprocessor registration, CPU reset and vCPU initialization >> into the realizefn. >> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> target-arm/cpu-qom.h | 3 ++- >> target-arm/cpu.c | 21 ++++++++++++++------- >> target-arm/cpu.h | 1 + >> target-arm/helper.c | 14 ++++++++++---- >> 4 Dateien geändert, 27 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-) >> >> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h >> index 0f455c4..aff7bf3 100644 >> --- a/target-arm/cpu-qom.h >> +++ b/target-arm/cpu-qom.h >> @@ -33,6 +33,7 @@ >> >> /** >> * ARMCPUClass: >> + * @parent_realize: The parent class' realize handler. >> * @parent_reset: The parent class' reset handler. >> * >> * An ARM CPU model. >> @@ -42,6 +43,7 @@ typedef struct ARMCPUClass { >> CPUClass parent_class; >> /*< public >*/ >> >> + DeviceRealize parent_realize; >> void (*parent_reset)(CPUState *cpu); >> } ARMCPUClass; >> >> @@ -107,7 +109,6 @@ static inline ARMCPU *arm_env_get_cpu(CPUARMState *env) >> >> #define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e)) >> >> -void arm_cpu_realize(ARMCPU *cpu); >> void register_cp_regs_for_features(ARMCPU *cpu); >> >> #endif >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index 07588a1..19d5ae4 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -147,15 +147,12 @@ static void arm_cpu_finalizefn(Object *obj) >> g_hash_table_destroy(cpu->cp_regs); >> } >> >> -void arm_cpu_realize(ARMCPU *cpu) >> +static void arm_cpu_realizefn(DeviceState *dev, Error **errp) >> { >> - /* This function is called by cpu_arm_init() because it >> - * needs to do common actions based on feature bits, etc >> - * that have been set by the subclass init functions. >> - * When we have QOM realize support it should become >> - * a true realize function instead. >> - */ >> + ARMCPU *cpu = ARM_CPU(dev); >> + ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev); >> CPUARMState *env = &cpu->env; >> + >> /* Some features automatically imply others: */ >> if (arm_feature(env, ARM_FEATURE_V7)) { >> set_feature(env, ARM_FEATURE_VAPA); >> @@ -197,6 +194,12 @@ void arm_cpu_realize(ARMCPU *cpu) >> } >> >> register_cp_regs_for_features(cpu); >> + arm_cpu_register_gdb_regs_for_features(cpu); >> + >> + cpu_reset(CPU(cpu)); >> + qemu_init_vcpu(env); >> + >> + acc->parent_realize(dev, errp); >> } >> >> /* CPU models */ >> @@ -763,6 +766,10 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) >> { >> ARMCPUClass *acc = ARM_CPU_CLASS(oc); >> CPUClass *cc = CPU_CLASS(acc); >> + DeviceClass *dc = DEVICE_CLASS(oc); >> + >> + acc->parent_realize = dc->realize; >> + dc->realize = arm_cpu_realizefn; >> >> acc->parent_reset = cc->reset; >> cc->reset = arm_cpu_reset; >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index ffddfcb..2902ba5 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -234,6 +234,7 @@ typedef struct CPUARMState { >> >> ARMCPU *cpu_arm_init(const char *cpu_model); >> void arm_translate_init(void); >> +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu); >> int cpu_arm_exec(CPUARMState *s); >> void do_interrupt(CPUARMState *); >> void switch_mode(CPUARMState *, int); >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 37c34a1..f412143 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -1270,14 +1270,22 @@ ARMCPU *cpu_arm_init(const char *cpu_model) >> cpu = ARM_CPU(object_new(cpu_model)); >> env = &cpu->env; >> env->cpu_model_str = cpu_model; >> - arm_cpu_realize(cpu); >> + >> + /* TODO this should be set centrally, once possible */ >> + object_property_set_bool(OBJECT(cpu), true, "realized", NULL); >> >> if (tcg_enabled() && !inited) { >> inited = 1; >> arm_translate_init(); >> } > > My first question I had when I saw this was: are you really sure it is > safe to call cpu_reset() and qemu_init_vcpu() before > arm_translate_init()? > > But I see that you change this on commit 092028dbf1. So now I have the > opposite question: are you really sure it is safe to call > arm_translate_init() before arm_cpu_realizefn()? The answer to either is yes. TCG initialization functions themselves (after this series latest) don't depend on CPU state and only calculate static field offsets (in one or two cases depending on CPU versions that I have inlined as part of the series). ARM's doesn't. If you spot any remaining exception to that rule, please shout. Andreas > >> >> - cpu_reset(CPU(cpu)); >> + return cpu; >> +} >> + >> +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) >> +{ >> + CPUARMState *env = &cpu->env; >> + >> if (arm_feature(env, ARM_FEATURE_NEON)) { >> gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg, >> 51, "arm-neon.xml", 0); >> @@ -1288,8 +1296,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model) >> gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg, >> 19, "arm-vfp.xml", 0); >> } >> - qemu_init_vcpu(env); >> - return cpu; >> } >> >> /* Sort alphabetically by type name, except for "any". */ >> -- >> 1.7.10.4 >> >> >
On 7 February 2013 18:44, Andreas Färber <afaerber@suse.de> wrote: > Am 07.02.2013 17:31, schrieb Eduardo Habkost: >> My first question I had when I saw this was: are you really sure it is >> safe to call cpu_reset() and qemu_init_vcpu() before >> arm_translate_init()? >> >> But I see that you change this on commit 092028dbf1. So now I have the >> opposite question: are you really sure it is safe to call >> arm_translate_init() before arm_cpu_realizefn()? > > The answer to either is yes. TCG initialization functions themselves > (after this series latest) don't depend on CPU state and only calculate > static field offsets (in one or two cases depending on CPU versions that > I have inlined as part of the series). ARM's doesn't. If you spot any > remaining exception to that rule, please shout. An idle thought: I wonder if we could rearrange things so that the target-specific TCG init is called via tcg_init(), to parallel the way that target-specific KVM init is called via kvm_init()? That would avoid the slight oddity that cpu_arm_init() &c have tcg_enabled()-specific code but not kvm_enabled()-specific or other accelerator-specific init. I'm not sure it would be worth the effort to make that change, though. -- PMM
Am 07.02.2013 19:56, schrieb Peter Maydell: > On 7 February 2013 18:44, Andreas Färber <afaerber@suse.de> wrote: >> Am 07.02.2013 17:31, schrieb Eduardo Habkost: >>> My first question I had when I saw this was: are you really sure it is >>> safe to call cpu_reset() and qemu_init_vcpu() before >>> arm_translate_init()? >>> >>> But I see that you change this on commit 092028dbf1. So now I have the >>> opposite question: are you really sure it is safe to call >>> arm_translate_init() before arm_cpu_realizefn()? >> >> The answer to either is yes. TCG initialization functions themselves >> (after this series latest) don't depend on CPU state and only calculate >> static field offsets (in one or two cases depending on CPU versions that >> I have inlined as part of the series). ARM's doesn't. If you spot any >> remaining exception to that rule, please shout. > > An idle thought: I wonder if we could rearrange things so that > the target-specific TCG init is called via tcg_init(), to > parallel the way that target-specific KVM init is called > via kvm_init()? That would avoid the slight oddity that > cpu_arm_init() &c have tcg_enabled()-specific code but not > kvm_enabled()-specific or other accelerator-specific init. > > I'm not sure it would be worth the effort to make that change, > though. I think that would be counter-productive. KVM is a host technology, it depends on whether you're running the right target arch and whether it's available/usable on your host. Since you're running on a single host, there's only one KVM init to call. The reason we made it conditional to tcg_enabled() is QTest, which doesn't need it. Nor will KVM on ARM. For TCG I'm hoping to enable multiple targets coexisting at some point, so consolidating things into *CPUClass and instance_init seems better. There's no ugly #ifdef'ery involved for TCG. I am still thinking about how to restructure TCG code - for one whether I can make TCG helpers optionally use CPUState at some point and for another how to consolidate the various global variables. We'd need only one cpu_env, I think - might go into CPUClass or as global into qom/cpu.c then. Cheers, Andreas
On 8 February 2013 17:31, Andreas Färber <afaerber@suse.de> wrote: > Am 07.02.2013 19:56, schrieb Peter Maydell: >> An idle thought: I wonder if we could rearrange things so that >> the target-specific TCG init is called via tcg_init(), to >> parallel the way that target-specific KVM init is called >> via kvm_init()? > I think that would be counter-productive. KVM is a host technology, it > depends on whether you're running the right target arch and whether it's > available/usable on your host. Since you're running on a single host, > there's only one KVM init to call. > > The reason we made it conditional to tcg_enabled() is QTest, which > doesn't need it. Nor will KVM on ARM. > > For TCG I'm hoping to enable multiple targets coexisting at some point, > so consolidating things into *CPUClass and instance_init seems better. > There's no ugly #ifdef'ery involved for TCG. That would suggest that some of the KVM init should also go via the CPU object; consider a system with one KVM-accelerated core and one TCG core of a different architecture. Anyway, it was just an idle thought, as I said. -- PMM
Am 08.02.2013 18:35, schrieb Peter Maydell: > On 8 February 2013 17:31, Andreas Färber <afaerber@suse.de> wrote: >> Am 07.02.2013 19:56, schrieb Peter Maydell: >>> An idle thought: I wonder if we could rearrange things so that >>> the target-specific TCG init is called via tcg_init(), to >>> parallel the way that target-specific KVM init is called >>> via kvm_init()? > >> I think that would be counter-productive. KVM is a host technology, it >> depends on whether you're running the right target arch and whether it's >> available/usable on your host. Since you're running on a single host, >> there's only one KVM init to call. >> >> The reason we made it conditional to tcg_enabled() is QTest, which >> doesn't need it. Nor will KVM on ARM. >> >> For TCG I'm hoping to enable multiple targets coexisting at some point, >> so consolidating things into *CPUClass and instance_init seems better. >> There's no ugly #ifdef'ery involved for TCG. > > That would suggest that some of the KVM init should also go > via the CPU object; consider a system with one KVM-accelerated > core and one TCG core of a different architecture. Yes, I had considered such a mixed scenario but pushed it aside for now because most kvm_enabled() and tcg_enabled() checks would need to get refactored then, which would be quite invasive for a not yet supported and hard to verify constellation. If someone with a use case wanted to send patches I would certainly help with review. ;-) For the current x86 subclasses discussion this thought would mean that we should try to keep KVM/no-KVM decisions to object instances. Andreas
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index 0f455c4..aff7bf3 100644 --- a/target-arm/cpu-qom.h +++ b/target-arm/cpu-qom.h @@ -33,6 +33,7 @@ /** * ARMCPUClass: + * @parent_realize: The parent class' realize handler. * @parent_reset: The parent class' reset handler. * * An ARM CPU model. @@ -42,6 +43,7 @@ typedef struct ARMCPUClass { CPUClass parent_class; /*< public >*/ + DeviceRealize parent_realize; void (*parent_reset)(CPUState *cpu); } ARMCPUClass; @@ -107,7 +109,6 @@ static inline ARMCPU *arm_env_get_cpu(CPUARMState *env) #define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e)) -void arm_cpu_realize(ARMCPU *cpu); void register_cp_regs_for_features(ARMCPU *cpu); #endif diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 07588a1..19d5ae4 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -147,15 +147,12 @@ static void arm_cpu_finalizefn(Object *obj) g_hash_table_destroy(cpu->cp_regs); } -void arm_cpu_realize(ARMCPU *cpu) +static void arm_cpu_realizefn(DeviceState *dev, Error **errp) { - /* This function is called by cpu_arm_init() because it - * needs to do common actions based on feature bits, etc - * that have been set by the subclass init functions. - * When we have QOM realize support it should become - * a true realize function instead. - */ + ARMCPU *cpu = ARM_CPU(dev); + ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev); CPUARMState *env = &cpu->env; + /* Some features automatically imply others: */ if (arm_feature(env, ARM_FEATURE_V7)) { set_feature(env, ARM_FEATURE_VAPA); @@ -197,6 +194,12 @@ void arm_cpu_realize(ARMCPU *cpu) } register_cp_regs_for_features(cpu); + arm_cpu_register_gdb_regs_for_features(cpu); + + cpu_reset(CPU(cpu)); + qemu_init_vcpu(env); + + acc->parent_realize(dev, errp); } /* CPU models */ @@ -763,6 +766,10 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) { ARMCPUClass *acc = ARM_CPU_CLASS(oc); CPUClass *cc = CPU_CLASS(acc); + DeviceClass *dc = DEVICE_CLASS(oc); + + acc->parent_realize = dc->realize; + dc->realize = arm_cpu_realizefn; acc->parent_reset = cc->reset; cc->reset = arm_cpu_reset; diff --git a/target-arm/cpu.h b/target-arm/cpu.h index ffddfcb..2902ba5 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -234,6 +234,7 @@ typedef struct CPUARMState { ARMCPU *cpu_arm_init(const char *cpu_model); void arm_translate_init(void); +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu); int cpu_arm_exec(CPUARMState *s); void do_interrupt(CPUARMState *); void switch_mode(CPUARMState *, int); diff --git a/target-arm/helper.c b/target-arm/helper.c index 37c34a1..f412143 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -1270,14 +1270,22 @@ ARMCPU *cpu_arm_init(const char *cpu_model) cpu = ARM_CPU(object_new(cpu_model)); env = &cpu->env; env->cpu_model_str = cpu_model; - arm_cpu_realize(cpu); + + /* TODO this should be set centrally, once possible */ + object_property_set_bool(OBJECT(cpu), true, "realized", NULL); if (tcg_enabled() && !inited) { inited = 1; arm_translate_init(); } - cpu_reset(CPU(cpu)); + return cpu; +} + +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) +{ + CPUARMState *env = &cpu->env; + if (arm_feature(env, ARM_FEATURE_NEON)) { gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg, 51, "arm-neon.xml", 0); @@ -1288,8 +1296,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model) gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg, 19, "arm-vfp.xml", 0); } - qemu_init_vcpu(env); - return cpu; } /* Sort alphabetically by type name, except for "any". */
Turn arm_cpu_realize() into a QOM realize function, no longer called via cpu.h prototype. To maintain the semantics of cpu_init(), set realized = true explicitly in cpu_arm_init(). Move GDB coprocessor registration, CPU reset and vCPU initialization into the realizefn. Signed-off-by: Andreas Färber <afaerber@suse.de> --- target-arm/cpu-qom.h | 3 ++- target-arm/cpu.c | 21 ++++++++++++++------- target-arm/cpu.h | 1 + target-arm/helper.c | 14 ++++++++++---- 4 Dateien geändert, 27 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)