Re: [Qemu-devel] [RFC qom-cpu v2 03/28] target-arm: Update ARMCPU to QOM realizefn

[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 <address@hidden>
> ---
>  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
> 
> 

-- 
Eduardo