[v3,2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()

Modify all CPUs to call it from XXX_cpu_realizefn() function.

Remove all the cannot_destroy_with_object_finalize_yet as
unsafe references have been moved to cpu_exec_realizefn().
(tested with QOM command provided by commit 4c315c27)

for arm:

Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
to arm_cpu_realizefn() as setting of cpu_index is now done
in cpu_exec_realizefn().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 exec.c                      |  2 +-
 include/exec/exec-all.h     |  1 -
 include/qom/cpu.h           |  1 +
 target-alpha/cpu.c          | 15 +++++++--------
 target-arm/cpu.c            | 39 +++++++++++++++++----------------------
 target-cris/cpu.c           | 15 +++++++--------
 target-i386/cpu.c           | 11 +++++------
 target-lm32/cpu.c           | 15 +++++++--------
 target-m68k/cpu.c           | 15 +++++++--------
 target-microblaze/cpu.c     | 14 +++++++-------
 target-mips/cpu.c           | 15 +++++++--------
 target-moxie/cpu.c          | 15 +++++++--------
 target-openrisc/cpu.c       | 15 +++++++--------
 target-ppc/translate_init.c |  2 +-
 target-s390x/cpu.c          |  8 +-------
 target-sh4/cpu.c            | 15 +++++++--------
 target-sparc/cpu.c          | 18 +++++++++---------
 target-tilegx/cpu.c         | 15 +++++++--------
 target-tricore/cpu.c        | 15 +++++++--------
 target-unicore32/cpu.c      | 18 +++++++++---------
 target-xtensa/cpu.c         | 15 +++++++--------
 21 files changed, 128 insertions(+), 151 deletions(-)

On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> Modify all CPUs to call it from XXX_cpu_realizefn() function.
> 
> Remove all the cannot_destroy_with_object_finalize_yet as
> unsafe references have been moved to cpu_exec_realizefn().
> (tested with QOM command provided by commit 4c315c27)
> 
> for arm:
> 
> Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> to arm_cpu_realizefn() as setting of cpu_index is now done
> in cpu_exec_realizefn().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  exec.c                      |  2 +-
>  include/exec/exec-all.h     |  1 -
>  include/qom/cpu.h           |  1 +
>  target-alpha/cpu.c          | 15 +++++++--------
>  target-arm/cpu.c            | 39 +++++++++++++++++----------------------
>  target-cris/cpu.c           | 15 +++++++--------
>  target-i386/cpu.c           | 11 +++++------
>  target-lm32/cpu.c           | 15 +++++++--------
>  target-m68k/cpu.c           | 15 +++++++--------
>  target-microblaze/cpu.c     | 14 +++++++-------
>  target-mips/cpu.c           | 15 +++++++--------
>  target-moxie/cpu.c          | 15 +++++++--------
>  target-openrisc/cpu.c       | 15 +++++++--------
>  target-ppc/translate_init.c |  2 +-
>  target-s390x/cpu.c          |  8 +-------
>  target-sh4/cpu.c            | 15 +++++++--------
>  target-sparc/cpu.c          | 18 +++++++++---------
>  target-tilegx/cpu.c         | 15 +++++++--------
>  target-tricore/cpu.c        | 15 +++++++--------
>  target-unicore32/cpu.c      | 18 +++++++++---------
>  target-xtensa/cpu.c         | 15 +++++++--------
>  21 files changed, 128 insertions(+), 151 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d1e57c4..203eb52 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu)
>  #endif
>  }
>  
> -void cpu_exec_init(CPUState *cpu, Error **errp)
> +void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
>  
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 336a57c..9797d55 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>                                uint32_t flags,
>                                int cflags);
>  
> -void cpu_exec_init(CPUState *cpu, Error **errp);
>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index d7648a9..5520c6c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx);
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  void cpu_exec_initfn(CPUState *cpu);
> +void cpu_exec_realizefn(CPUState *cpu, Error **errp);
>  void cpu_exec_exit(CPUState *cpu);
>  
>  #ifdef CONFIG_SOFTMMU
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index 6d01d7f..30d77ce 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>  
> @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj)
>      CPUAlphaState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>      tlb_flush(cs, 1);
>  
>      alpha_translate_init();
> @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>      cc->disas_set_info = alpha_cpu_disas_set_info;
>  
>      cc->gdb_num_core_regs = 67;
> -
> -    /*
> -     * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo alpha_cpu_type_info = {
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..364a45d 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
>      static bool inited;
> -    uint32_t Aff1, Aff0;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>                                           g_free, g_free);
>  
> -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> -     * in later ARM ARM versions), or any of the higher affinity level fields,
> -     * so these bits always RAZ.
> -     */
> -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> -
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
> @@ -576,6 +565,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      ARMCPU *cpu = ARM_CPU(dev);
>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>      CPUARMState *env = &cpu->env;
> +    Error *local_err = NULL;
> +    uint32_t Aff1, Aff0;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
> @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          set_feature(env, ARM_FEATURE_THUMB_DSP);
>      }
>  
> +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> +     * in later ARM ARM versions), or any of the higher affinity level fields,
> +     * so these bits always RAZ.
> +     */
> +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> +
>      if (cpu->reset_hivecs) {
>              cpu->reset_sctlr |= (1 << 13);
>      }
> @@ -1533,17 +1539,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->debug_check_watchpoint = arm_debug_check_watchpoint;
>  
>      cc->disas_set_info = arm_disas_set_info;
> -
> -    /*
> -     * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     *
> -     * Once this is fixed, the devices that create ARM CPUs should be
> -     * updated not to set cannot_destroy_with_object_finalize_yet,
> -     * unless they still screw up something else.
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const ARMCPUInfo *info)
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index d680cfb..2e9ab97 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -142,6 +142,13 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      CRISCPUClass *ccc = CRIS_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -187,7 +194,6 @@ static void cris_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->pregs[PR_VR] = ccc->vr;
>  
> @@ -326,13 +332,6 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>  
>      cc->disas_set_info = cris_disas_set_info;
> -
> -    /*
> -     * Reason: cris_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo cris_cpu_type_info = {
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1c57fce..3476d46 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3158,7 +3158,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>              cpu->phys_bits = 32;
>          }
>      }
> -    cpu_exec_init(cs, &error_abort);
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      if (tcg_enabled()) {
>          tcg_x86_init();
> @@ -3537,11 +3541,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->cpu_exec_exit = x86_cpu_exec_exit;
>  
>      dc->cannot_instantiate_with_device_add_yet = false;
> -    /*
> -     * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
> -     * object in cpus -> dangling pointer after final object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo x86_cpu_type_info = {
> diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
> index a783d46..8d939a7 100644
> --- a/target-lm32/cpu.c
> +++ b/target-lm32/cpu.c
> @@ -144,6 +144,13 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      LM32CPUClass *lcc = LM32_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>  
> @@ -160,7 +167,6 @@ static void lm32_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->flags = 0;
>  
> @@ -285,13 +291,6 @@ static void lm32_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>      cc->debug_excp_handler = lm32_debug_excp_handler;
>      cc->disas_set_info = lm32_cpu_disas_set_info;
> -
> -    /*
> -     * Reason: lm32_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void lm32_register_cpu_type(const LM32CPUInfo *info)
> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
> index 116b784..17e4be2 100644
> --- a/target-m68k/cpu.c
> +++ b/target-m68k/cpu.c
> @@ -159,6 +159,13 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUState *cs = CPU(dev);
>      M68kCPU *cpu = M68K_CPU(dev);
>      M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      m68k_cpu_init_gdb(cpu);
>  
> @@ -176,7 +183,6 @@ static void m68k_cpu_initfn(Object *obj)
>      static bool inited;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = true;
> @@ -222,13 +228,6 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>      cc->gdb_core_xml_file = "cf-core.xml";
>  
>      dc->vmsd = &vmstate_m68k_cpu;
> -
> -    /*
> -     * Reason: m68k_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void register_cpu_type(const M68kCPUInfo *info)
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 8edc00a..389c7b6 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -138,6 +138,13 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUMBState *env = &cpu->env;
>      uint8_t version_code = 0;
>      int i = 0;
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>  
> @@ -199,7 +206,6 @@ static void mb_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
>  
> @@ -267,12 +273,6 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 32 + 5;
>  
>      cc->disas_set_info = mb_disas_set_info;
> -
> -    /*
> -     * Reason: mb_cpu_initfn() calls cpu_exec_init(), which saves the
> -     * object in cpus -> dangling pointer after final object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo mb_cpu_type_info = {
> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
> index 64ad112..65ca607 100644
> --- a/target-mips/cpu.c
> +++ b/target-mips/cpu.c
> @@ -124,6 +124,13 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -138,7 +145,6 @@ static void mips_cpu_initfn(Object *obj)
>      CPUMIPSState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          mips_tcg_init();
> @@ -177,13 +183,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>  
>      cc->gdb_num_core_regs = 73;
>      cc->gdb_stop_before_watchpoint = true;
> -
> -    /*
> -     * Reason: mips_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo mips_cpu_type_info = {
> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
> index 50a0899..b0be4a7 100644
> --- a/target-moxie/cpu.c
> +++ b/target-moxie/cpu.c
> @@ -61,6 +61,13 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      MoxieCPUClass *mcc = MOXIE_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
> @@ -75,7 +82,6 @@ static void moxie_cpu_initfn(Object *obj)
>      static int inited;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = 1;
> @@ -124,13 +130,6 @@ static void moxie_cpu_class_init(ObjectClass *oc, void *data)
>      cc->vmsd = &vmstate_moxie_cpu;
>  #endif
>      cc->disas_set_info = moxie_cpu_disas_set_info;
> -
> -    /*
> -     * Reason: moxie_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void moxielite_initfn(Object *obj)
> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
> index 155913f..698e87b 100644
> --- a/target-openrisc/cpu.c
> +++ b/target-openrisc/cpu.c
> @@ -81,6 +81,13 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      OpenRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
> @@ -95,7 +102,6 @@ static void openrisc_cpu_initfn(Object *obj)
>      static int inited;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>  
>  #ifndef CONFIG_USER_ONLY
>      cpu_openrisc_mmu_init(cpu);
> @@ -180,13 +186,6 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
>      dc->vmsd = &vmstate_openrisc_cpu;
>  #endif
>      cc->gdb_num_core_regs = 32 + 3;
> -
> -    /*
> -     * Reason: openrisc_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const OpenRISCCPUInfo *info)
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index b66b40b..40dae70 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9678,7 +9678,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    cpu_exec_init(cs, &local_err);
> +    cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 35ae2ce..9e2f239 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -207,7 +207,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> -    cpu_exec_init(cs, &err);
> +    cpu_exec_realizefn(cs, &err);
>      if (err != NULL) {
>          goto out;
>      }
> @@ -440,12 +440,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_core_xml_file = "s390x-core64.xml";
>      cc->gdb_arch_name = s390_gdb_arch_name;
>  
> -    /*
> -     * Reason: s390_cpu_realizefn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>      s390_cpu_model_class_register_props(oc);
>  }
>  
> diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
> index f589532..a38f6a6 100644
> --- a/target-sh4/cpu.c
> +++ b/target-sh4/cpu.c
> @@ -244,6 +244,13 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -258,7 +265,6 @@ static void superh_cpu_initfn(Object *obj)
>      CPUSH4State *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->movcal_backup_tail = &(env->movcal_backup);
>  
> @@ -303,13 +309,6 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 59;
>  
>      dc->vmsd = &vmstate_sh_cpu;
> -
> -    /*
> -     * Reason: superh_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo superh_cpu_type_info = {
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index 800a25a..4e07b92 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -792,7 +792,9 @@ static bool sparc_cpu_has_work(CPUState *cs)
>  
>  static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUState *cs = CPU(dev);
>      SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
>  #if defined(CONFIG_USER_ONLY)
>      SPARCCPU *cpu = SPARC_CPU(dev);
>      CPUSPARCState *env = &cpu->env;
> @@ -802,7 +804,13 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    qemu_init_vcpu(CPU(dev));
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    qemu_init_vcpu(cs);
>  
>      scc->parent_realize(dev, errp);
>  }
> @@ -814,7 +822,6 @@ static void sparc_cpu_initfn(Object *obj)
>      CPUSPARCState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          gen_intermediate_code_init(env);
> @@ -867,13 +874,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>  #else
>      cc->gdb_num_core_regs = 72;
>  #endif
> -
> -    /*
> -     * Reason: sparc_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo sparc_cpu_type_info = {
> diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
> index 7017cb6..454793f 100644
> --- a/target-tilegx/cpu.c
> +++ b/target-tilegx/cpu.c
> @@ -92,6 +92,13 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      TileGXCPUClass *tcc = TILEGX_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -107,7 +114,6 @@ static void tilegx_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !tcg_initialized) {
>          tcg_initialized = true;
> @@ -162,13 +168,6 @@ static void tilegx_cpu_class_init(ObjectClass *oc, void *data)
>      cc->set_pc = tilegx_cpu_set_pc;
>      cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault;
>      cc->gdb_num_core_regs = 0;
> -
> -    /*
> -     * Reason: tilegx_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo tilegx_cpu_type_info = {
> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
> index 35d4ee4..785b76b 100644
> --- a/target-tricore/cpu.c
> +++ b/target-tricore/cpu.c
> @@ -69,6 +69,13 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      TriCoreCPU *cpu = TRICORE_CPU(dev);
>      TriCoreCPUClass *tcc = TRICORE_CPU_GET_CLASS(dev);
>      CPUTriCoreState *env = &cpu->env;
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Some features automatically imply others */
>      if (tricore_feature(env, TRICORE_FEATURE_161)) {
> @@ -95,7 +102,6 @@ static void tricore_cpu_initfn(Object *obj)
>      CPUTriCoreState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          tricore_tcg_init();
> @@ -172,13 +178,6 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
>      cc->dump_state = tricore_cpu_dump_state;
>      cc->set_pc = tricore_cpu_set_pc;
>      cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
> -
> -    /*
> -     * Reason: tricore_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const TriCoreCPUInfo *info)
> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
> index e7a4984..c169972 100644
> --- a/target-unicore32/cpu.c
> +++ b/target-unicore32/cpu.c
> @@ -101,9 +101,17 @@ static const UniCore32CPUInfo uc32_cpus[] = {
>  
>  static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUState *cs = CPU(dev);
>      UniCore32CPUClass *ucc = UNICORE32_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
>  
> -    qemu_init_vcpu(CPU(dev));
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    qemu_init_vcpu(cs);
>  
>      ucc->parent_realize(dev, errp);
>  }
> @@ -116,7 +124,6 @@ static void uc32_cpu_initfn(Object *obj)
>      static bool inited;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>  #ifdef CONFIG_USER_ONLY
>      env->uncached_asr = ASR_MODE_USER;
> @@ -160,13 +167,6 @@ static void uc32_cpu_class_init(ObjectClass *oc, void *data)
>      cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
>  #endif
>      dc->vmsd = &vmstate_uc32_cpu;
> -
> -    /*
> -     * Reason: uc32_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void uc32_register_cpu_type(const UniCore32CPUInfo *info)
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 5ad08a2..e8e9f91 100644
> --- a/target-xtensa/cpu.c
> +++ b/target-xtensa/cpu.c
> @@ -99,6 +99,13 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>  
> @@ -117,7 +124,6 @@ static void xtensa_cpu_initfn(Object *obj)
>  
>      cs->env_ptr = env;
>      env->config = xcc->config;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !tcg_inited) {
>          tcg_inited = true;
> @@ -158,13 +164,6 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
>  #endif
>      cc->debug_excp_handler = xtensa_breakpoint_handler;
>      dc->vmsd = &vmstate_xtensa_cpu;
> -
> -    /*
> -     * Reason: xtensa_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo xtensa_cpu_type_info = {

On Sat, 15 Oct 2016 00:52:48 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Modify all CPUs to call it from XXX_cpu_realizefn() function.
> 
> Remove all the cannot_destroy_with_object_finalize_yet as
> unsafe references have been moved to cpu_exec_realizefn().
> (tested with QOM command provided by commit 4c315c27)
> 
> for arm:
> 
> Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> to arm_cpu_realizefn() as setting of cpu_index is now done
> in cpu_exec_realizefn().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
For target-i386 part:
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c                      |  2 +-
>  include/exec/exec-all.h     |  1 -
>  include/qom/cpu.h           |  1 +
>  target-alpha/cpu.c          | 15 +++++++--------
>  target-arm/cpu.c            | 39 +++++++++++++++++----------------------
>  target-cris/cpu.c           | 15 +++++++--------
>  target-i386/cpu.c           | 11 +++++------
>  target-lm32/cpu.c           | 15 +++++++--------
>  target-m68k/cpu.c           | 15 +++++++--------
>  target-microblaze/cpu.c     | 14 +++++++-------
>  target-mips/cpu.c           | 15 +++++++--------
>  target-moxie/cpu.c          | 15 +++++++--------
>  target-openrisc/cpu.c       | 15 +++++++--------
>  target-ppc/translate_init.c |  2 +-
>  target-s390x/cpu.c          |  8 +-------
>  target-sh4/cpu.c            | 15 +++++++--------
>  target-sparc/cpu.c          | 18 +++++++++---------
>  target-tilegx/cpu.c         | 15 +++++++--------
>  target-tricore/cpu.c        | 15 +++++++--------
>  target-unicore32/cpu.c      | 18 +++++++++---------
>  target-xtensa/cpu.c         | 15 +++++++--------
>  21 files changed, 128 insertions(+), 151 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d1e57c4..203eb52 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu)
>  #endif
>  }
>  
> -void cpu_exec_init(CPUState *cpu, Error **errp)
> +void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
>  
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 336a57c..9797d55 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>                                uint32_t flags,
>                                int cflags);
>  
> -void cpu_exec_init(CPUState *cpu, Error **errp);
>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index d7648a9..5520c6c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx);
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  void cpu_exec_initfn(CPUState *cpu);
> +void cpu_exec_realizefn(CPUState *cpu, Error **errp);
>  void cpu_exec_exit(CPUState *cpu);
>  
>  #ifdef CONFIG_SOFTMMU
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index 6d01d7f..30d77ce 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>  
> @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj)
>      CPUAlphaState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>      tlb_flush(cs, 1);
>  
>      alpha_translate_init();
> @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>      cc->disas_set_info = alpha_cpu_disas_set_info;
>  
>      cc->gdb_num_core_regs = 67;
> -
> -    /*
> -     * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo alpha_cpu_type_info = {
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..364a45d 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
>      static bool inited;
> -    uint32_t Aff1, Aff0;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>                                           g_free, g_free);
>  
> -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> -     * in later ARM ARM versions), or any of the higher affinity level fields,
> -     * so these bits always RAZ.
> -     */
> -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> -
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
> @@ -576,6 +565,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      ARMCPU *cpu = ARM_CPU(dev);
>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>      CPUARMState *env = &cpu->env;
> +    Error *local_err = NULL;
> +    uint32_t Aff1, Aff0;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
> @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          set_feature(env, ARM_FEATURE_THUMB_DSP);
>      }
>  
> +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> +     * in later ARM ARM versions), or any of the higher affinity level fields,
> +     * so these bits always RAZ.
> +     */
> +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> +
>      if (cpu->reset_hivecs) {
>              cpu->reset_sctlr |= (1 << 13);
>      }
> @@ -1533,17 +1539,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->debug_check_watchpoint = arm_debug_check_watchpoint;
>  
>      cc->disas_set_info = arm_disas_set_info;
> -
> -    /*
> -     * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     *
> -     * Once this is fixed, the devices that create ARM CPUs should be
> -     * updated not to set cannot_destroy_with_object_finalize_yet,
> -     * unless they still screw up something else.
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const ARMCPUInfo *info)
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index d680cfb..2e9ab97 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -142,6 +142,13 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      CRISCPUClass *ccc = CRIS_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -187,7 +194,6 @@ static void cris_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->pregs[PR_VR] = ccc->vr;
>  
> @@ -326,13 +332,6 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>  
>      cc->disas_set_info = cris_disas_set_info;
> -
> -    /*
> -     * Reason: cris_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo cris_cpu_type_info = {
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1c57fce..3476d46 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3158,7 +3158,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>              cpu->phys_bits = 32;
>          }
>      }
> -    cpu_exec_init(cs, &error_abort);
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      if (tcg_enabled()) {
>          tcg_x86_init();
> @@ -3537,11 +3541,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->cpu_exec_exit = x86_cpu_exec_exit;
>  
>      dc->cannot_instantiate_with_device_add_yet = false;
> -    /*
> -     * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
> -     * object in cpus -> dangling pointer after final object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo x86_cpu_type_info = {
> diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
> index a783d46..8d939a7 100644
> --- a/target-lm32/cpu.c
> +++ b/target-lm32/cpu.c
> @@ -144,6 +144,13 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      LM32CPUClass *lcc = LM32_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>  
> @@ -160,7 +167,6 @@ static void lm32_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->flags = 0;
>  
> @@ -285,13 +291,6 @@ static void lm32_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>      cc->debug_excp_handler = lm32_debug_excp_handler;
>      cc->disas_set_info = lm32_cpu_disas_set_info;
> -
> -    /*
> -     * Reason: lm32_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void lm32_register_cpu_type(const LM32CPUInfo *info)
> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
> index 116b784..17e4be2 100644
> --- a/target-m68k/cpu.c
> +++ b/target-m68k/cpu.c
> @@ -159,6 +159,13 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUState *cs = CPU(dev);
>      M68kCPU *cpu = M68K_CPU(dev);
>      M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      m68k_cpu_init_gdb(cpu);
>  
> @@ -176,7 +183,6 @@ static void m68k_cpu_initfn(Object *obj)
>      static bool inited;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = true;
> @@ -222,13 +228,6 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>      cc->gdb_core_xml_file = "cf-core.xml";
>  
>      dc->vmsd = &vmstate_m68k_cpu;
> -
> -    /*
> -     * Reason: m68k_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void register_cpu_type(const M68kCPUInfo *info)
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 8edc00a..389c7b6 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -138,6 +138,13 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUMBState *env = &cpu->env;
>      uint8_t version_code = 0;
>      int i = 0;
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>  
> @@ -199,7 +206,6 @@ static void mb_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
>  
> @@ -267,12 +273,6 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 32 + 5;
>  
>      cc->disas_set_info = mb_disas_set_info;
> -
> -    /*
> -     * Reason: mb_cpu_initfn() calls cpu_exec_init(), which saves the
> -     * object in cpus -> dangling pointer after final object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo mb_cpu_type_info = {
> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
> index 64ad112..65ca607 100644
> --- a/target-mips/cpu.c
> +++ b/target-mips/cpu.c
> @@ -124,6 +124,13 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -138,7 +145,6 @@ static void mips_cpu_initfn(Object *obj)
>      CPUMIPSState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          mips_tcg_init();
> @@ -177,13 +183,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>  
>      cc->gdb_num_core_regs = 73;
>      cc->gdb_stop_before_watchpoint = true;
> -
> -    /*
> -     * Reason: mips_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo mips_cpu_type_info = {
> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
> index 50a0899..b0be4a7 100644
> --- a/target-moxie/cpu.c
> +++ b/target-moxie/cpu.c
> @@ -61,6 +61,13 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      MoxieCPUClass *mcc = MOXIE_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
> @@ -75,7 +82,6 @@ static void moxie_cpu_initfn(Object *obj)
>      static int inited;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = 1;
> @@ -124,13 +130,6 @@ static void moxie_cpu_class_init(ObjectClass *oc, void *data)
>      cc->vmsd = &vmstate_moxie_cpu;
>  #endif
>      cc->disas_set_info = moxie_cpu_disas_set_info;
> -
> -    /*
> -     * Reason: moxie_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void moxielite_initfn(Object *obj)
> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
> index 155913f..698e87b 100644
> --- a/target-openrisc/cpu.c
> +++ b/target-openrisc/cpu.c
> @@ -81,6 +81,13 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      OpenRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
> @@ -95,7 +102,6 @@ static void openrisc_cpu_initfn(Object *obj)
>      static int inited;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>  
>  #ifndef CONFIG_USER_ONLY
>      cpu_openrisc_mmu_init(cpu);
> @@ -180,13 +186,6 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
>      dc->vmsd = &vmstate_openrisc_cpu;
>  #endif
>      cc->gdb_num_core_regs = 32 + 3;
> -
> -    /*
> -     * Reason: openrisc_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const OpenRISCCPUInfo *info)
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index b66b40b..40dae70 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9678,7 +9678,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    cpu_exec_init(cs, &local_err);
> +    cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 35ae2ce..9e2f239 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -207,7 +207,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> -    cpu_exec_init(cs, &err);
> +    cpu_exec_realizefn(cs, &err);
>      if (err != NULL) {
>          goto out;
>      }
> @@ -440,12 +440,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_core_xml_file = "s390x-core64.xml";
>      cc->gdb_arch_name = s390_gdb_arch_name;
>  
> -    /*
> -     * Reason: s390_cpu_realizefn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>      s390_cpu_model_class_register_props(oc);
>  }
>  
> diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
> index f589532..a38f6a6 100644
> --- a/target-sh4/cpu.c
> +++ b/target-sh4/cpu.c
> @@ -244,6 +244,13 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -258,7 +265,6 @@ static void superh_cpu_initfn(Object *obj)
>      CPUSH4State *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->movcal_backup_tail = &(env->movcal_backup);
>  
> @@ -303,13 +309,6 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 59;
>  
>      dc->vmsd = &vmstate_sh_cpu;
> -
> -    /*
> -     * Reason: superh_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo superh_cpu_type_info = {
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index 800a25a..4e07b92 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -792,7 +792,9 @@ static bool sparc_cpu_has_work(CPUState *cs)
>  
>  static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUState *cs = CPU(dev);
>      SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
>  #if defined(CONFIG_USER_ONLY)
>      SPARCCPU *cpu = SPARC_CPU(dev);
>      CPUSPARCState *env = &cpu->env;
> @@ -802,7 +804,13 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    qemu_init_vcpu(CPU(dev));
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    qemu_init_vcpu(cs);
>  
>      scc->parent_realize(dev, errp);
>  }
> @@ -814,7 +822,6 @@ static void sparc_cpu_initfn(Object *obj)
>      CPUSPARCState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          gen_intermediate_code_init(env);
> @@ -867,13 +874,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>  #else
>      cc->gdb_num_core_regs = 72;
>  #endif
> -
> -    /*
> -     * Reason: sparc_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo sparc_cpu_type_info = {
> diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
> index 7017cb6..454793f 100644
> --- a/target-tilegx/cpu.c
> +++ b/target-tilegx/cpu.c
> @@ -92,6 +92,13 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      TileGXCPUClass *tcc = TILEGX_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -107,7 +114,6 @@ static void tilegx_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !tcg_initialized) {
>          tcg_initialized = true;
> @@ -162,13 +168,6 @@ static void tilegx_cpu_class_init(ObjectClass *oc, void *data)
>      cc->set_pc = tilegx_cpu_set_pc;
>      cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault;
>      cc->gdb_num_core_regs = 0;
> -
> -    /*
> -     * Reason: tilegx_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo tilegx_cpu_type_info = {
> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
> index 35d4ee4..785b76b 100644
> --- a/target-tricore/cpu.c
> +++ b/target-tricore/cpu.c
> @@ -69,6 +69,13 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      TriCoreCPU *cpu = TRICORE_CPU(dev);
>      TriCoreCPUClass *tcc = TRICORE_CPU_GET_CLASS(dev);
>      CPUTriCoreState *env = &cpu->env;
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Some features automatically imply others */
>      if (tricore_feature(env, TRICORE_FEATURE_161)) {
> @@ -95,7 +102,6 @@ static void tricore_cpu_initfn(Object *obj)
>      CPUTriCoreState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          tricore_tcg_init();
> @@ -172,13 +178,6 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
>      cc->dump_state = tricore_cpu_dump_state;
>      cc->set_pc = tricore_cpu_set_pc;
>      cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
> -
> -    /*
> -     * Reason: tricore_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const TriCoreCPUInfo *info)
> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
> index e7a4984..c169972 100644
> --- a/target-unicore32/cpu.c
> +++ b/target-unicore32/cpu.c
> @@ -101,9 +101,17 @@ static const UniCore32CPUInfo uc32_cpus[] = {
>  
>  static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUState *cs = CPU(dev);
>      UniCore32CPUClass *ucc = UNICORE32_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
>  
> -    qemu_init_vcpu(CPU(dev));
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    qemu_init_vcpu(cs);
>  
>      ucc->parent_realize(dev, errp);
>  }
> @@ -116,7 +124,6 @@ static void uc32_cpu_initfn(Object *obj)
>      static bool inited;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>  #ifdef CONFIG_USER_ONLY
>      env->uncached_asr = ASR_MODE_USER;
> @@ -160,13 +167,6 @@ static void uc32_cpu_class_init(ObjectClass *oc, void *data)
>      cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
>  #endif
>      dc->vmsd = &vmstate_uc32_cpu;
> -
> -    /*
> -     * Reason: uc32_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void uc32_register_cpu_type(const UniCore32CPUInfo *info)
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 5ad08a2..e8e9f91 100644
> --- a/target-xtensa/cpu.c
> +++ b/target-xtensa/cpu.c
> @@ -99,6 +99,13 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>  
> @@ -117,7 +124,6 @@ static void xtensa_cpu_initfn(Object *obj)
>  
>      cs->env_ptr = env;
>      env->config = xcc->config;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !tcg_inited) {
>          tcg_inited = true;
> @@ -158,13 +164,6 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
>  #endif
>      cc->debug_excp_handler = xtensa_breakpoint_handler;
>      dc->vmsd = &vmstate_xtensa_cpu;
> -
> -    /*
> -     * Reason: xtensa_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo xtensa_cpu_type_info = {

On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> Modify all CPUs to call it from XXX_cpu_realizefn() function.
> 
> Remove all the cannot_destroy_with_object_finalize_yet as
> unsafe references have been moved to cpu_exec_realizefn().
> (tested with QOM command provided by commit 4c315c27)
> 
> for arm:
> 
> Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> to arm_cpu_realizefn() as setting of cpu_index is now done
> in cpu_exec_realizefn().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Subject line is misleading: it looks like a trivial function
rename, but there are functional changes in multiple
architectures.

Do you mind if subject is changed to "move cpu_exec_init() calls
to realize functions" when committing? That's the main purpose of
the patch, the rename is just a detail.

I liked to have the "move cpu_exec_realize() to realize function"
part split per architecture in v1/v2. But the rename would touch
all architectures anyway, and making this as a single patch would
help us avoid leaving some architecture behind.


> ---
>  exec.c                      |  2 +-
>  include/exec/exec-all.h     |  1 -
>  include/qom/cpu.h           |  1 +
>  target-alpha/cpu.c          | 15 +++++++--------
>  target-arm/cpu.c            | 39 +++++++++++++++++----------------------
>  target-cris/cpu.c           | 15 +++++++--------
>  target-i386/cpu.c           | 11 +++++------
>  target-lm32/cpu.c           | 15 +++++++--------
>  target-m68k/cpu.c           | 15 +++++++--------
>  target-microblaze/cpu.c     | 14 +++++++-------
>  target-mips/cpu.c           | 15 +++++++--------
>  target-moxie/cpu.c          | 15 +++++++--------
>  target-openrisc/cpu.c       | 15 +++++++--------
>  target-ppc/translate_init.c |  2 +-
>  target-s390x/cpu.c          |  8 +-------
>  target-sh4/cpu.c            | 15 +++++++--------
>  target-sparc/cpu.c          | 18 +++++++++---------
>  target-tilegx/cpu.c         | 15 +++++++--------
>  target-tricore/cpu.c        | 15 +++++++--------
>  target-unicore32/cpu.c      | 18 +++++++++---------
>  target-xtensa/cpu.c         | 15 +++++++--------
>  21 files changed, 128 insertions(+), 151 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d1e57c4..203eb52 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu)
>  #endif
>  }
>  
> -void cpu_exec_init(CPUState *cpu, Error **errp)
> +void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
>  
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 336a57c..9797d55 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>                                uint32_t flags,
>                                int cflags);
>  
> -void cpu_exec_init(CPUState *cpu, Error **errp);
>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index d7648a9..5520c6c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx);
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  void cpu_exec_initfn(CPUState *cpu);
> +void cpu_exec_realizefn(CPUState *cpu, Error **errp);
>  void cpu_exec_exit(CPUState *cpu);
>  
>  #ifdef CONFIG_SOFTMMU
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index 6d01d7f..30d77ce 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>  
> @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj)
>      CPUAlphaState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>      tlb_flush(cs, 1);
>  
>      alpha_translate_init();
> @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>      cc->disas_set_info = alpha_cpu_disas_set_info;
>  
>      cc->gdb_num_core_regs = 67;
> -
> -    /*
> -     * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo alpha_cpu_type_info = {
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..364a45d 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
>      static bool inited;
> -    uint32_t Aff1, Aff0;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>                                           g_free, g_free);
>  
> -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> -     * in later ARM ARM versions), or any of the higher affinity level fields,
> -     * so these bits always RAZ.
> -     */
> -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> -
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
> @@ -576,6 +565,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      ARMCPU *cpu = ARM_CPU(dev);
>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>      CPUARMState *env = &cpu->env;
> +    Error *local_err = NULL;
> +    uint32_t Aff1, Aff0;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
> @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          set_feature(env, ARM_FEATURE_THUMB_DSP);
>      }
>  
> +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> +     * in later ARM ARM versions), or any of the higher affinity level fields,
> +     * so these bits always RAZ.
> +     */
> +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> +
>      if (cpu->reset_hivecs) {
>              cpu->reset_sctlr |= (1 << 13);
>      }
> @@ -1533,17 +1539,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->debug_check_watchpoint = arm_debug_check_watchpoint;
>  
>      cc->disas_set_info = arm_disas_set_info;
> -
> -    /*
> -     * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     *
> -     * Once this is fixed, the devices that create ARM CPUs should be
> -     * updated not to set cannot_destroy_with_object_finalize_yet,
> -     * unless they still screw up something else.
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const ARMCPUInfo *info)
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index d680cfb..2e9ab97 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -142,6 +142,13 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      CRISCPUClass *ccc = CRIS_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -187,7 +194,6 @@ static void cris_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->pregs[PR_VR] = ccc->vr;
>  
> @@ -326,13 +332,6 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>  
>      cc->disas_set_info = cris_disas_set_info;
> -
> -    /*
> -     * Reason: cris_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo cris_cpu_type_info = {
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1c57fce..3476d46 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3158,7 +3158,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>              cpu->phys_bits = 32;
>          }
>      }
> -    cpu_exec_init(cs, &error_abort);
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      if (tcg_enabled()) {
>          tcg_x86_init();
> @@ -3537,11 +3541,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->cpu_exec_exit = x86_cpu_exec_exit;
>  
>      dc->cannot_instantiate_with_device_add_yet = false;
> -    /*
> -     * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
> -     * object in cpus -> dangling pointer after final object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo x86_cpu_type_info = {
> diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
> index a783d46..8d939a7 100644
> --- a/target-lm32/cpu.c
> +++ b/target-lm32/cpu.c
> @@ -144,6 +144,13 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      LM32CPUClass *lcc = LM32_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>  
> @@ -160,7 +167,6 @@ static void lm32_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->flags = 0;
>  
> @@ -285,13 +291,6 @@ static void lm32_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>      cc->debug_excp_handler = lm32_debug_excp_handler;
>      cc->disas_set_info = lm32_cpu_disas_set_info;
> -
> -    /*
> -     * Reason: lm32_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void lm32_register_cpu_type(const LM32CPUInfo *info)
> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
> index 116b784..17e4be2 100644
> --- a/target-m68k/cpu.c
> +++ b/target-m68k/cpu.c
> @@ -159,6 +159,13 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUState *cs = CPU(dev);
>      M68kCPU *cpu = M68K_CPU(dev);
>      M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      m68k_cpu_init_gdb(cpu);
>  
> @@ -176,7 +183,6 @@ static void m68k_cpu_initfn(Object *obj)
>      static bool inited;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = true;
> @@ -222,13 +228,6 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>      cc->gdb_core_xml_file = "cf-core.xml";
>  
>      dc->vmsd = &vmstate_m68k_cpu;
> -
> -    /*
> -     * Reason: m68k_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void register_cpu_type(const M68kCPUInfo *info)
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 8edc00a..389c7b6 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -138,6 +138,13 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUMBState *env = &cpu->env;
>      uint8_t version_code = 0;
>      int i = 0;
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>  
> @@ -199,7 +206,6 @@ static void mb_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
>  
> @@ -267,12 +273,6 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 32 + 5;
>  
>      cc->disas_set_info = mb_disas_set_info;
> -
> -    /*
> -     * Reason: mb_cpu_initfn() calls cpu_exec_init(), which saves the
> -     * object in cpus -> dangling pointer after final object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo mb_cpu_type_info = {
> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
> index 64ad112..65ca607 100644
> --- a/target-mips/cpu.c
> +++ b/target-mips/cpu.c
> @@ -124,6 +124,13 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -138,7 +145,6 @@ static void mips_cpu_initfn(Object *obj)
>      CPUMIPSState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          mips_tcg_init();
> @@ -177,13 +183,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>  
>      cc->gdb_num_core_regs = 73;
>      cc->gdb_stop_before_watchpoint = true;
> -
> -    /*
> -     * Reason: mips_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo mips_cpu_type_info = {
> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
> index 50a0899..b0be4a7 100644
> --- a/target-moxie/cpu.c
> +++ b/target-moxie/cpu.c
> @@ -61,6 +61,13 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      MoxieCPUClass *mcc = MOXIE_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
> @@ -75,7 +82,6 @@ static void moxie_cpu_initfn(Object *obj)
>      static int inited;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = 1;
> @@ -124,13 +130,6 @@ static void moxie_cpu_class_init(ObjectClass *oc, void *data)
>      cc->vmsd = &vmstate_moxie_cpu;
>  #endif
>      cc->disas_set_info = moxie_cpu_disas_set_info;
> -
> -    /*
> -     * Reason: moxie_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void moxielite_initfn(Object *obj)
> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
> index 155913f..698e87b 100644
> --- a/target-openrisc/cpu.c
> +++ b/target-openrisc/cpu.c
> @@ -81,6 +81,13 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      OpenRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
> @@ -95,7 +102,6 @@ static void openrisc_cpu_initfn(Object *obj)
>      static int inited;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>  
>  #ifndef CONFIG_USER_ONLY
>      cpu_openrisc_mmu_init(cpu);
> @@ -180,13 +186,6 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
>      dc->vmsd = &vmstate_openrisc_cpu;
>  #endif
>      cc->gdb_num_core_regs = 32 + 3;
> -
> -    /*
> -     * Reason: openrisc_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const OpenRISCCPUInfo *info)
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index b66b40b..40dae70 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9678,7 +9678,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    cpu_exec_init(cs, &local_err);
> +    cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 35ae2ce..9e2f239 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -207,7 +207,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> -    cpu_exec_init(cs, &err);
> +    cpu_exec_realizefn(cs, &err);
>      if (err != NULL) {
>          goto out;
>      }
> @@ -440,12 +440,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_core_xml_file = "s390x-core64.xml";
>      cc->gdb_arch_name = s390_gdb_arch_name;
>  
> -    /*
> -     * Reason: s390_cpu_realizefn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>      s390_cpu_model_class_register_props(oc);
>  }
>  
> diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
> index f589532..a38f6a6 100644
> --- a/target-sh4/cpu.c
> +++ b/target-sh4/cpu.c
> @@ -244,6 +244,13 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -258,7 +265,6 @@ static void superh_cpu_initfn(Object *obj)
>      CPUSH4State *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      env->movcal_backup_tail = &(env->movcal_backup);
>  
> @@ -303,13 +309,6 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 59;
>  
>      dc->vmsd = &vmstate_sh_cpu;
> -
> -    /*
> -     * Reason: superh_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo superh_cpu_type_info = {
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index 800a25a..4e07b92 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -792,7 +792,9 @@ static bool sparc_cpu_has_work(CPUState *cs)
>  
>  static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUState *cs = CPU(dev);
>      SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
>  #if defined(CONFIG_USER_ONLY)
>      SPARCCPU *cpu = SPARC_CPU(dev);
>      CPUSPARCState *env = &cpu->env;
> @@ -802,7 +804,13 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    qemu_init_vcpu(CPU(dev));
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    qemu_init_vcpu(cs);
>  
>      scc->parent_realize(dev, errp);
>  }
> @@ -814,7 +822,6 @@ static void sparc_cpu_initfn(Object *obj)
>      CPUSPARCState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          gen_intermediate_code_init(env);
> @@ -867,13 +874,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>  #else
>      cc->gdb_num_core_regs = 72;
>  #endif
> -
> -    /*
> -     * Reason: sparc_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo sparc_cpu_type_info = {
> diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
> index 7017cb6..454793f 100644
> --- a/target-tilegx/cpu.c
> +++ b/target-tilegx/cpu.c
> @@ -92,6 +92,13 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      TileGXCPUClass *tcc = TILEGX_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cpu_reset(cs);
>      qemu_init_vcpu(cs);
> @@ -107,7 +114,6 @@ static void tilegx_cpu_initfn(Object *obj)
>      static bool tcg_initialized;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !tcg_initialized) {
>          tcg_initialized = true;
> @@ -162,13 +168,6 @@ static void tilegx_cpu_class_init(ObjectClass *oc, void *data)
>      cc->set_pc = tilegx_cpu_set_pc;
>      cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault;
>      cc->gdb_num_core_regs = 0;
> -
> -    /*
> -     * Reason: tilegx_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo tilegx_cpu_type_info = {
> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
> index 35d4ee4..785b76b 100644
> --- a/target-tricore/cpu.c
> +++ b/target-tricore/cpu.c
> @@ -69,6 +69,13 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      TriCoreCPU *cpu = TRICORE_CPU(dev);
>      TriCoreCPUClass *tcc = TRICORE_CPU_GET_CLASS(dev);
>      CPUTriCoreState *env = &cpu->env;
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Some features automatically imply others */
>      if (tricore_feature(env, TRICORE_FEATURE_161)) {
> @@ -95,7 +102,6 @@ static void tricore_cpu_initfn(Object *obj)
>      CPUTriCoreState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled()) {
>          tricore_tcg_init();
> @@ -172,13 +178,6 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
>      cc->dump_state = tricore_cpu_dump_state;
>      cc->set_pc = tricore_cpu_set_pc;
>      cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
> -
> -    /*
> -     * Reason: tricore_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void cpu_register(const TriCoreCPUInfo *info)
> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
> index e7a4984..c169972 100644
> --- a/target-unicore32/cpu.c
> +++ b/target-unicore32/cpu.c
> @@ -101,9 +101,17 @@ static const UniCore32CPUInfo uc32_cpus[] = {
>  
>  static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUState *cs = CPU(dev);
>      UniCore32CPUClass *ucc = UNICORE32_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
>  
> -    qemu_init_vcpu(CPU(dev));
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    qemu_init_vcpu(cs);
>  
>      ucc->parent_realize(dev, errp);
>  }
> @@ -116,7 +124,6 @@ static void uc32_cpu_initfn(Object *obj)
>      static bool inited;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>  #ifdef CONFIG_USER_ONLY
>      env->uncached_asr = ASR_MODE_USER;
> @@ -160,13 +167,6 @@ static void uc32_cpu_class_init(ObjectClass *oc, void *data)
>      cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
>  #endif
>      dc->vmsd = &vmstate_uc32_cpu;
> -
> -    /*
> -     * Reason: uc32_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static void uc32_register_cpu_type(const UniCore32CPUInfo *info)
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 5ad08a2..e8e9f91 100644
> --- a/target-xtensa/cpu.c
> +++ b/target-xtensa/cpu.c
> @@ -99,6 +99,13 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
>      XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
> +    Error *local_err = NULL;
> +
> +    cpu_exec_realizefn(cs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>  
> @@ -117,7 +124,6 @@ static void xtensa_cpu_initfn(Object *obj)
>  
>      cs->env_ptr = env;
>      env->config = xcc->config;
> -    cpu_exec_init(cs, &error_abort);
>  
>      if (tcg_enabled() && !tcg_inited) {
>          tcg_inited = true;
> @@ -158,13 +164,6 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
>  #endif
>      cc->debug_excp_handler = xtensa_breakpoint_handler;
>      dc->vmsd = &vmstate_xtensa_cpu;
> -
> -    /*
> -     * Reason: xtensa_cpu_initfn() calls cpu_exec_init(), which saves
> -     * the object in cpus -> dangling pointer after final
> -     * object_unref().
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  static const TypeInfo xtensa_cpu_type_info = {
> -- 
> 2.7.4
>

On 17/10/2016 16:03, Eduardo Habkost wrote:
> On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
>> Modify all CPUs to call it from XXX_cpu_realizefn() function.
>>
>> Remove all the cannot_destroy_with_object_finalize_yet as
>> unsafe references have been moved to cpu_exec_realizefn().
>> (tested with QOM command provided by commit 4c315c27)
>>
>> for arm:
>>
>> Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
>> to arm_cpu_realizefn() as setting of cpu_index is now done
>> in cpu_exec_realizefn().
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Subject line is misleading: it looks like a trivial function
> rename, but there are functional changes in multiple
> architectures.
> 
> Do you mind if subject is changed to "move cpu_exec_init() calls
> to realize functions" when committing? That's the main purpose of
> the patch, the rename is just a detail.

I agree.

Thanks,
Laurent

> I liked to have the "move cpu_exec_realize() to realize function"
> part split per architecture in v1/v2. But the rename would touch
> all architectures anyway, and making this as a single patch would
> help us avoid leaving some architecture behind.
> 
> 
>> ---
>>  exec.c                      |  2 +-
>>  include/exec/exec-all.h     |  1 -
>>  include/qom/cpu.h           |  1 +
>>  target-alpha/cpu.c          | 15 +++++++--------
>>  target-arm/cpu.c            | 39 +++++++++++++++++----------------------
>>  target-cris/cpu.c           | 15 +++++++--------
>>  target-i386/cpu.c           | 11 +++++------
>>  target-lm32/cpu.c           | 15 +++++++--------
>>  target-m68k/cpu.c           | 15 +++++++--------
>>  target-microblaze/cpu.c     | 14 +++++++-------
>>  target-mips/cpu.c           | 15 +++++++--------
>>  target-moxie/cpu.c          | 15 +++++++--------
>>  target-openrisc/cpu.c       | 15 +++++++--------
>>  target-ppc/translate_init.c |  2 +-
>>  target-s390x/cpu.c          |  8 +-------
>>  target-sh4/cpu.c            | 15 +++++++--------
>>  target-sparc/cpu.c          | 18 +++++++++---------
>>  target-tilegx/cpu.c         | 15 +++++++--------
>>  target-tricore/cpu.c        | 15 +++++++--------
>>  target-unicore32/cpu.c      | 18 +++++++++---------
>>  target-xtensa/cpu.c         | 15 +++++++--------
>>  21 files changed, 128 insertions(+), 151 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index d1e57c4..203eb52 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu)
>>  #endif
>>  }
>>  
>> -void cpu_exec_init(CPUState *cpu, Error **errp)
>> +void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>>  {
>>      CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
>>  
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 336a57c..9797d55 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>                                uint32_t flags,
>>                                int cflags);
>>  
>> -void cpu_exec_init(CPUState *cpu, Error **errp);
>>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>>  
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index d7648a9..5520c6c 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx);
>>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>>      GCC_FMT_ATTR(2, 3);
>>  void cpu_exec_initfn(CPUState *cpu);
>> +void cpu_exec_realizefn(CPUState *cpu, Error **errp);
>>  void cpu_exec_exit(CPUState *cpu);
>>  
>>  #ifdef CONFIG_SOFTMMU
>> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
>> index 6d01d7f..30d77ce 100644
>> --- a/target-alpha/cpu.c
>> +++ b/target-alpha/cpu.c
>> @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      qemu_init_vcpu(cs);
>>  
>> @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj)
>>      CPUAlphaState *env = &cpu->env;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>      tlb_flush(cs, 1);
>>  
>>      alpha_translate_init();
>> @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->disas_set_info = alpha_cpu_disas_set_info;
>>  
>>      cc->gdb_num_core_regs = 67;
>> -
>> -    /*
>> -     * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo alpha_cpu_type_info = {
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 1b9540e..364a45d 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
>>      CPUState *cs = CPU(obj);
>>      ARMCPU *cpu = ARM_CPU(obj);
>>      static bool inited;
>> -    uint32_t Aff1, Aff0;
>>  
>>      cs->env_ptr = &cpu->env;
>> -    cpu_exec_init(cs, &error_abort);
>>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>>                                           g_free, g_free);
>>  
>> -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
>> -     * We don't support setting cluster ID ([16..23]) (known as Aff2
>> -     * in later ARM ARM versions), or any of the higher affinity level fields,
>> -     * so these bits always RAZ.
>> -     */
>> -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
>> -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
>> -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
>> -
>>  #ifndef CONFIG_USER_ONLY
>>      /* Our inbound IRQ and FIQ lines */
>>      if (kvm_enabled()) {
>> @@ -576,6 +565,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>      ARMCPU *cpu = ARM_CPU(dev);
>>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>>      CPUARMState *env = &cpu->env;
>> +    Error *local_err = NULL;
>> +    uint32_t Aff1, Aff0;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      /* Some features automatically imply others: */
>>      if (arm_feature(env, ARM_FEATURE_V8)) {
>> @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          set_feature(env, ARM_FEATURE_THUMB_DSP);
>>      }
>>  
>> +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
>> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
>> +     * in later ARM ARM versions), or any of the higher affinity level fields,
>> +     * so these bits always RAZ.
>> +     */
>> +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
>> +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
>> +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
>> +
>>      if (cpu->reset_hivecs) {
>>              cpu->reset_sctlr |= (1 << 13);
>>      }
>> @@ -1533,17 +1539,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->debug_check_watchpoint = arm_debug_check_watchpoint;
>>  
>>      cc->disas_set_info = arm_disas_set_info;
>> -
>> -    /*
>> -     * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     *
>> -     * Once this is fixed, the devices that create ARM CPUs should be
>> -     * updated not to set cannot_destroy_with_object_finalize_yet,
>> -     * unless they still screw up something else.
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void cpu_register(const ARMCPUInfo *info)
>> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
>> index d680cfb..2e9ab97 100644
>> --- a/target-cris/cpu.c
>> +++ b/target-cris/cpu.c
>> @@ -142,6 +142,13 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      CRISCPUClass *ccc = CRIS_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      cpu_reset(cs);
>>      qemu_init_vcpu(cs);
>> @@ -187,7 +194,6 @@ static void cris_cpu_initfn(Object *obj)
>>      static bool tcg_initialized;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      env->pregs[PR_VR] = ccc->vr;
>>  
>> @@ -326,13 +332,6 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_stop_before_watchpoint = true;
>>  
>>      cc->disas_set_info = cris_disas_set_info;
>> -
>> -    /*
>> -     * Reason: cris_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo cris_cpu_type_info = {
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 1c57fce..3476d46 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -3158,7 +3158,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>              cpu->phys_bits = 32;
>>          }
>>      }
>> -    cpu_exec_init(cs, &error_abort);
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      if (tcg_enabled()) {
>>          tcg_x86_init();
>> @@ -3537,11 +3541,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>>      cc->cpu_exec_exit = x86_cpu_exec_exit;
>>  
>>      dc->cannot_instantiate_with_device_add_yet = false;
>> -    /*
>> -     * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
>> -     * object in cpus -> dangling pointer after final object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo x86_cpu_type_info = {
>> diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
>> index a783d46..8d939a7 100644
>> --- a/target-lm32/cpu.c
>> +++ b/target-lm32/cpu.c
>> @@ -144,6 +144,13 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      LM32CPUClass *lcc = LM32_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      cpu_reset(cs);
>>  
>> @@ -160,7 +167,6 @@ static void lm32_cpu_initfn(Object *obj)
>>      static bool tcg_initialized;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      env->flags = 0;
>>  
>> @@ -285,13 +291,6 @@ static void lm32_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_stop_before_watchpoint = true;
>>      cc->debug_excp_handler = lm32_debug_excp_handler;
>>      cc->disas_set_info = lm32_cpu_disas_set_info;
>> -
>> -    /*
>> -     * Reason: lm32_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void lm32_register_cpu_type(const LM32CPUInfo *info)
>> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
>> index 116b784..17e4be2 100644
>> --- a/target-m68k/cpu.c
>> +++ b/target-m68k/cpu.c
>> @@ -159,6 +159,13 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>>      CPUState *cs = CPU(dev);
>>      M68kCPU *cpu = M68K_CPU(dev);
>>      M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      m68k_cpu_init_gdb(cpu);
>>  
>> @@ -176,7 +183,6 @@ static void m68k_cpu_initfn(Object *obj)
>>      static bool inited;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled() && !inited) {
>>          inited = true;
>> @@ -222,13 +228,6 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>>      cc->gdb_core_xml_file = "cf-core.xml";
>>  
>>      dc->vmsd = &vmstate_m68k_cpu;
>> -
>> -    /*
>> -     * Reason: m68k_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void register_cpu_type(const M68kCPUInfo *info)
>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>> index 8edc00a..389c7b6 100644
>> --- a/target-microblaze/cpu.c
>> +++ b/target-microblaze/cpu.c
>> @@ -138,6 +138,13 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>>      CPUMBState *env = &cpu->env;
>>      uint8_t version_code = 0;
>>      int i = 0;
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      qemu_init_vcpu(cs);
>>  
>> @@ -199,7 +206,6 @@ static void mb_cpu_initfn(Object *obj)
>>      static bool tcg_initialized;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
>>  
>> @@ -267,12 +273,6 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_num_core_regs = 32 + 5;
>>  
>>      cc->disas_set_info = mb_disas_set_info;
>> -
>> -    /*
>> -     * Reason: mb_cpu_initfn() calls cpu_exec_init(), which saves the
>> -     * object in cpus -> dangling pointer after final object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo mb_cpu_type_info = {
>> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
>> index 64ad112..65ca607 100644
>> --- a/target-mips/cpu.c
>> +++ b/target-mips/cpu.c
>> @@ -124,6 +124,13 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      cpu_reset(cs);
>>      qemu_init_vcpu(cs);
>> @@ -138,7 +145,6 @@ static void mips_cpu_initfn(Object *obj)
>>      CPUMIPSState *env = &cpu->env;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled()) {
>>          mips_tcg_init();
>> @@ -177,13 +183,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>>  
>>      cc->gdb_num_core_regs = 73;
>>      cc->gdb_stop_before_watchpoint = true;
>> -
>> -    /*
>> -     * Reason: mips_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo mips_cpu_type_info = {
>> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
>> index 50a0899..b0be4a7 100644
>> --- a/target-moxie/cpu.c
>> +++ b/target-moxie/cpu.c
>> @@ -61,6 +61,13 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      MoxieCPUClass *mcc = MOXIE_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      qemu_init_vcpu(cs);
>>      cpu_reset(cs);
>> @@ -75,7 +82,6 @@ static void moxie_cpu_initfn(Object *obj)
>>      static int inited;
>>  
>>      cs->env_ptr = &cpu->env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled() && !inited) {
>>          inited = 1;
>> @@ -124,13 +130,6 @@ static void moxie_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->vmsd = &vmstate_moxie_cpu;
>>  #endif
>>      cc->disas_set_info = moxie_cpu_disas_set_info;
>> -
>> -    /*
>> -     * Reason: moxie_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void moxielite_initfn(Object *obj)
>> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
>> index 155913f..698e87b 100644
>> --- a/target-openrisc/cpu.c
>> +++ b/target-openrisc/cpu.c
>> @@ -81,6 +81,13 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      OpenRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      qemu_init_vcpu(cs);
>>      cpu_reset(cs);
>> @@ -95,7 +102,6 @@ static void openrisc_cpu_initfn(Object *obj)
>>      static int inited;
>>  
>>      cs->env_ptr = &cpu->env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>  #ifndef CONFIG_USER_ONLY
>>      cpu_openrisc_mmu_init(cpu);
>> @@ -180,13 +186,6 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
>>      dc->vmsd = &vmstate_openrisc_cpu;
>>  #endif
>>      cc->gdb_num_core_regs = 32 + 3;
>> -
>> -    /*
>> -     * Reason: openrisc_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void cpu_register(const OpenRISCCPUInfo *info)
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index b66b40b..40dae70 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -9678,7 +9678,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>>      }
>>  #endif
>>  
>> -    cpu_exec_init(cs, &local_err);
>> +    cpu_exec_realizefn(cs, &local_err);
>>      if (local_err != NULL) {
>>          error_propagate(errp, local_err);
>>          return;
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 35ae2ce..9e2f239 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -207,7 +207,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>>          goto out;
>>      }
>>  
>> -    cpu_exec_init(cs, &err);
>> +    cpu_exec_realizefn(cs, &err);
>>      if (err != NULL) {
>>          goto out;
>>      }
>> @@ -440,12 +440,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_core_xml_file = "s390x-core64.xml";
>>      cc->gdb_arch_name = s390_gdb_arch_name;
>>  
>> -    /*
>> -     * Reason: s390_cpu_realizefn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>      s390_cpu_model_class_register_props(oc);
>>  }
>>  
>> diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
>> index f589532..a38f6a6 100644
>> --- a/target-sh4/cpu.c
>> +++ b/target-sh4/cpu.c
>> @@ -244,6 +244,13 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      cpu_reset(cs);
>>      qemu_init_vcpu(cs);
>> @@ -258,7 +265,6 @@ static void superh_cpu_initfn(Object *obj)
>>      CPUSH4State *env = &cpu->env;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      env->movcal_backup_tail = &(env->movcal_backup);
>>  
>> @@ -303,13 +309,6 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_num_core_regs = 59;
>>  
>>      dc->vmsd = &vmstate_sh_cpu;
>> -
>> -    /*
>> -     * Reason: superh_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo superh_cpu_type_info = {
>> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
>> index 800a25a..4e07b92 100644
>> --- a/target-sparc/cpu.c
>> +++ b/target-sparc/cpu.c
>> @@ -792,7 +792,9 @@ static bool sparc_cpu_has_work(CPUState *cs)
>>  
>>  static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>> +    CPUState *cs = CPU(dev);
>>      SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>>  #if defined(CONFIG_USER_ONLY)
>>      SPARCCPU *cpu = SPARC_CPU(dev);
>>      CPUSPARCState *env = &cpu->env;
>> @@ -802,7 +804,13 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>>      }
>>  #endif
>>  
>> -    qemu_init_vcpu(CPU(dev));
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    qemu_init_vcpu(cs);
>>  
>>      scc->parent_realize(dev, errp);
>>  }
>> @@ -814,7 +822,6 @@ static void sparc_cpu_initfn(Object *obj)
>>      CPUSPARCState *env = &cpu->env;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled()) {
>>          gen_intermediate_code_init(env);
>> @@ -867,13 +874,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>>  #else
>>      cc->gdb_num_core_regs = 72;
>>  #endif
>> -
>> -    /*
>> -     * Reason: sparc_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo sparc_cpu_type_info = {
>> diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
>> index 7017cb6..454793f 100644
>> --- a/target-tilegx/cpu.c
>> +++ b/target-tilegx/cpu.c
>> @@ -92,6 +92,13 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      TileGXCPUClass *tcc = TILEGX_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      cpu_reset(cs);
>>      qemu_init_vcpu(cs);
>> @@ -107,7 +114,6 @@ static void tilegx_cpu_initfn(Object *obj)
>>      static bool tcg_initialized;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled() && !tcg_initialized) {
>>          tcg_initialized = true;
>> @@ -162,13 +168,6 @@ static void tilegx_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->set_pc = tilegx_cpu_set_pc;
>>      cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault;
>>      cc->gdb_num_core_regs = 0;
>> -
>> -    /*
>> -     * Reason: tilegx_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo tilegx_cpu_type_info = {
>> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
>> index 35d4ee4..785b76b 100644
>> --- a/target-tricore/cpu.c
>> +++ b/target-tricore/cpu.c
>> @@ -69,6 +69,13 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>>      TriCoreCPU *cpu = TRICORE_CPU(dev);
>>      TriCoreCPUClass *tcc = TRICORE_CPU_GET_CLASS(dev);
>>      CPUTriCoreState *env = &cpu->env;
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      /* Some features automatically imply others */
>>      if (tricore_feature(env, TRICORE_FEATURE_161)) {
>> @@ -95,7 +102,6 @@ static void tricore_cpu_initfn(Object *obj)
>>      CPUTriCoreState *env = &cpu->env;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled()) {
>>          tricore_tcg_init();
>> @@ -172,13 +178,6 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
>>      cc->dump_state = tricore_cpu_dump_state;
>>      cc->set_pc = tricore_cpu_set_pc;
>>      cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
>> -
>> -    /*
>> -     * Reason: tricore_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void cpu_register(const TriCoreCPUInfo *info)
>> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
>> index e7a4984..c169972 100644
>> --- a/target-unicore32/cpu.c
>> +++ b/target-unicore32/cpu.c
>> @@ -101,9 +101,17 @@ static const UniCore32CPUInfo uc32_cpus[] = {
>>  
>>  static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>> +    CPUState *cs = CPU(dev);
>>      UniCore32CPUClass *ucc = UNICORE32_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>>  
>> -    qemu_init_vcpu(CPU(dev));
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    qemu_init_vcpu(cs);
>>  
>>      ucc->parent_realize(dev, errp);
>>  }
>> @@ -116,7 +124,6 @@ static void uc32_cpu_initfn(Object *obj)
>>      static bool inited;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>  #ifdef CONFIG_USER_ONLY
>>      env->uncached_asr = ASR_MODE_USER;
>> @@ -160,13 +167,6 @@ static void uc32_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
>>  #endif
>>      dc->vmsd = &vmstate_uc32_cpu;
>> -
>> -    /*
>> -     * Reason: uc32_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void uc32_register_cpu_type(const UniCore32CPUInfo *info)
>> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
>> index 5ad08a2..e8e9f91 100644
>> --- a/target-xtensa/cpu.c
>> +++ b/target-xtensa/cpu.c
>> @@ -99,6 +99,13 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>>  {
>>      CPUState *cs = CPU(dev);
>>      XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    cpu_exec_realizefn(cs, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>>  
>> @@ -117,7 +124,6 @@ static void xtensa_cpu_initfn(Object *obj)
>>  
>>      cs->env_ptr = env;
>>      env->config = xcc->config;
>> -    cpu_exec_init(cs, &error_abort);
>>  
>>      if (tcg_enabled() && !tcg_inited) {
>>          tcg_inited = true;
>> @@ -158,13 +164,6 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
>>  #endif
>>      cc->debug_excp_handler = xtensa_breakpoint_handler;
>>      dc->vmsd = &vmstate_xtensa_cpu;
>> -
>> -    /*
>> -     * Reason: xtensa_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static const TypeInfo xtensa_cpu_type_info = {
>> -- 
>> 2.7.4
>>
>

On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> Modify all CPUs to call it from XXX_cpu_realizefn() function.
> 
> Remove all the cannot_destroy_with_object_finalize_yet as
> unsafe references have been moved to cpu_exec_realizefn().
> (tested with QOM command provided by commit 4c315c27)
> 
> for arm:
> 
> Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> to arm_cpu_realizefn() as setting of cpu_index is now done
> in cpu_exec_realizefn().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
[...]
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..364a45d 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
>      static bool inited;
> -    uint32_t Aff1, Aff0;
>  
>      cs->env_ptr = &cpu->env;
> -    cpu_exec_init(cs, &error_abort);
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>                                           g_free, g_free);
>  
> -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> -     * in later ARM ARM versions), or any of the higher affinity level fields,
> -     * so these bits always RAZ.
> -     */
> -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> -
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
[...]
> @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          set_feature(env, ARM_FEATURE_THUMB_DSP);
>      }
>  
> +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> +     * in later ARM ARM versions), or any of the higher affinity level fields,
> +     * so these bits always RAZ.
> +     */
> +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> +

This will override any value set in the "mp-affinity" property,
The mp-affinity property can be set by the user in the
command-line, and it is also set by machvirt_init() in
hw/arm/virt.c.

Considering that each CPU is supposed to have a different value,
I doubt there are existing use cases for mp-affinity being set
directly by the user.

I suggest having a "cluster-size" property, instead of
"mp-affinity". This way the mp_affinity field can be calculated
on realize, based on the configured cluster-size.

>      if (cpu->reset_hivecs) {
>              cpu->reset_sctlr |= (1 << 13);
>      }
[...]

On Mon, 17 Oct 2016 17:20:22 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> > Modify all CPUs to call it from XXX_cpu_realizefn() function.
> > 
> > Remove all the cannot_destroy_with_object_finalize_yet as
> > unsafe references have been moved to cpu_exec_realizefn().
> > (tested with QOM command provided by commit 4c315c27)
> > 
> > for arm:
> > 
> > Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> > to arm_cpu_realizefn() as setting of cpu_index is now done
> > in cpu_exec_realizefn().
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>  
> [...]
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 1b9540e..364a45d 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
> >      CPUState *cs = CPU(obj);
> >      ARMCPU *cpu = ARM_CPU(obj);
> >      static bool inited;
> > -    uint32_t Aff1, Aff0;
> >  
> >      cs->env_ptr = &cpu->env;
> > -    cpu_exec_init(cs, &error_abort);
> >      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> >                                           g_free, g_free);
> >  
> > -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > -     * in later ARM ARM versions), or any of the higher affinity level fields,
> > -     * so these bits always RAZ.
> > -     */
> > -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > -
> >  #ifndef CONFIG_USER_ONLY
> >      /* Our inbound IRQ and FIQ lines */
> >      if (kvm_enabled()) {  
> [...]
> > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >          set_feature(env, ARM_FEATURE_THUMB_DSP);
> >      }
> >  
> > +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > +     * in later ARM ARM versions), or any of the higher affinity level fields,
> > +     * so these bits always RAZ.
> > +     */
> > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > +  
>
> This will override any value set in the "mp-affinity" property,
> The mp-affinity property can be set by the user in the
> command-line, and it is also set by machvirt_init() in
> hw/arm/virt.c.
> 
> Considering that each CPU is supposed to have a different value,
> I doubt there are existing use cases for mp-affinity being set
> directly by the user.
> 
> I suggest having a "cluster-size" property, instead of
> "mp-affinity". This way the mp_affinity field can be calculated
> on realize, based on the configured cluster-size.
CCing Drew as he knows more about MPIDR stuff more.

> 
> >      if (cpu->reset_hivecs) {
> >              cpu->reset_sctlr |= (1 << 13);
> >      }  
> [...]
>

On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote:
> On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> > Modify all CPUs to call it from XXX_cpu_realizefn() function.
> > 
> > Remove all the cannot_destroy_with_object_finalize_yet as
> > unsafe references have been moved to cpu_exec_realizefn().
> > (tested with QOM command provided by commit 4c315c27)
> > 
> > for arm:
> > 
> > Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> > to arm_cpu_realizefn() as setting of cpu_index is now done
> > in cpu_exec_realizefn().
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> [...]
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 1b9540e..364a45d 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
> >      CPUState *cs = CPU(obj);
> >      ARMCPU *cpu = ARM_CPU(obj);
> >      static bool inited;
> > -    uint32_t Aff1, Aff0;
> >  
> >      cs->env_ptr = &cpu->env;
> > -    cpu_exec_init(cs, &error_abort);
> >      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> >                                           g_free, g_free);
> >  
> > -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > -     * in later ARM ARM versions), or any of the higher affinity level fields,
> > -     * so these bits always RAZ.
> > -     */
> > -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > -
> >  #ifndef CONFIG_USER_ONLY
> >      /* Our inbound IRQ and FIQ lines */
> >      if (kvm_enabled()) {
> [...]
> > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >          set_feature(env, ARM_FEATURE_THUMB_DSP);
> >      }
> >  
> > +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > +     * in later ARM ARM versions), or any of the higher affinity level fields,
> > +     * so these bits always RAZ.
> > +     */
> > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > +
> 
> This will override any value set in the "mp-affinity" property,
> The mp-affinity property can be set by the user in the
> command-line, and it is also set by machvirt_init() in
> hw/arm/virt.c.

I'm glad you noticed this Eduardo. We'd indeed lose changes made to
the MPIDRs for mach-virt and Raspberry Pi.

> 
> Considering that each CPU is supposed to have a different value,
> I doubt there are existing use cases for mp-affinity being set
> directly by the user.

Probably not. It was turned into a property in order for the gicv3
model to access it. I don't think that's necessary, so we can just
un-property it, accessing it directly from the structure instead.

> 
> I suggest having a "cluster-size" property, instead of
> "mp-affinity". This way the mp_affinity field can be calculated
> on realize, based on the configured cluster-size.

This won't work for Raspberry Pi's MPIDR adjustment. Anyway, IMO
realize should only set values when it knows nothing has been set.
If values have been set, it should only sanity check them. In this
case it's safe to simply check for uniqueness and zeros;

 1) The MPIDRs are all zero. As they're not all unique that's not
    valid. It's pretty safe to assume they just weren't set in this
    case though, so we can set the default values without complaining.
    If there was only one cpu, then MPIDR=0 is valid, but that's the
    default anyway.
 2) The MPIDRs are all unique, or there's a single cpu and its MPIDR
    is not zero. In this case we shouldn't touch them.
 3) The MPIDRs are not all zero and not all unique. This is a failed
    sanity check and should abort.

Thanks,
drew

> 
> >      if (cpu->reset_hivecs) {
> >              cpu->reset_sctlr |= (1 << 13);
> >      }
> [...]
> 
> -- 
> Eduardo
>

On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote:
> On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote:
> > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> > > Modify all CPUs to call it from XXX_cpu_realizefn() function.
> > > 
> > > Remove all the cannot_destroy_with_object_finalize_yet as
> > > unsafe references have been moved to cpu_exec_realizefn().
> > > (tested with QOM command provided by commit 4c315c27)
> > > 
> > > for arm:
> > > 
> > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> > > to arm_cpu_realizefn() as setting of cpu_index is now done
> > > in cpu_exec_realizefn().
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > [...]
> > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > index 1b9540e..364a45d 100644
> > > --- a/target-arm/cpu.c
> > > +++ b/target-arm/cpu.c
> > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
> > >      CPUState *cs = CPU(obj);
> > >      ARMCPU *cpu = ARM_CPU(obj);
> > >      static bool inited;
> > > -    uint32_t Aff1, Aff0;
> > >  
> > >      cs->env_ptr = &cpu->env;
> > > -    cpu_exec_init(cs, &error_abort);
> > >      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> > >                                           g_free, g_free);
> > >  
> > > -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > -     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > -     * so these bits always RAZ.
> > > -     */
> > > -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > -
> > >  #ifndef CONFIG_USER_ONLY
> > >      /* Our inbound IRQ and FIQ lines */
> > >      if (kvm_enabled()) {
> > [...]
> > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> > >          set_feature(env, ARM_FEATURE_THUMB_DSP);
> > >      }
> > >  
> > > +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > +     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > +     * so these bits always RAZ.
> > > +     */
> > > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > +
> > 
> > This will override any value set in the "mp-affinity" property,
> > The mp-affinity property can be set by the user in the
> > command-line, and it is also set by machvirt_init() in
> > hw/arm/virt.c.
> 
> I'm glad you noticed this Eduardo. We'd indeed lose changes made to
> the MPIDRs for mach-virt and Raspberry Pi.
> 
> > 
> > Considering that each CPU is supposed to have a different value,
> > I doubt there are existing use cases for mp-affinity being set
> > directly by the user.
> 
> Probably not. It was turned into a property in order for the gicv3
> model to access it. I don't think that's necessary, so we can just
> un-property it, accessing it directly from the structure instead.
> 
> > 
> > I suggest having a "cluster-size" property, instead of
> > "mp-affinity". This way the mp_affinity field can be calculated
> > on realize, based on the configured cluster-size.
> 
> This won't work for Raspberry Pi's MPIDR adjustment.

Where's that code?

> Anyway, IMO realize should only set values when it knows
> nothing has been set.

Realize could also initialize some (private) fields using other
(public) fields as input. Instead of making machine code
calculate mp_affinity, machine code could just provide input for
realize to calculate the right values.

But maybe my cluster-size suggestion won't work anyway because of
other cases where mp_affinity is set (I didn't check all code
that sets/gets mp_affinity).

> If values have been set, it should only sanity check them. In this
> case it's safe to simply check for uniqueness and zeros;
> 
>  1) The MPIDRs are all zero. As they're not all unique that's not
>     valid. It's pretty safe to assume they just weren't set in this
>     case though, so we can set the default values without complaining.
>     If there was only one cpu, then MPIDR=0 is valid, but that's the
>     default anyway.

Implementing this check in arm_cpu_realizefn() would be
difficult, as the CPU realize method don't have access to the
other CPUs. But maybe we can use some other default value to
indicate that the field was never set? UINT64_MAX?

>  2) The MPIDRs are all unique, or there's a single cpu and its MPIDR
>     is not zero. In this case we shouldn't touch them.
>  3) The MPIDRs are not all zero and not all unique. This is a failed
>     sanity check and should abort.

A sanity check would be useful, but I don't know where exactly it
could be implemented. Is there any post-machine-init hook you
could use?

> 
> Thanks,
> drew
> 
> > 
> > >      if (cpu->reset_hivecs) {
> > >              cpu->reset_sctlr |= (1 << 13);
> > >      }
> > [...]
> > 
> > -- 
> > Eduardo
> >

On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote:
> On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote:
> > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote:
> > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> > > > Modify all CPUs to call it from XXX_cpu_realizefn() function.
> > > > 
> > > > Remove all the cannot_destroy_with_object_finalize_yet as
> > > > unsafe references have been moved to cpu_exec_realizefn().
> > > > (tested with QOM command provided by commit 4c315c27)
> > > > 
> > > > for arm:
> > > > 
> > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> > > > to arm_cpu_realizefn() as setting of cpu_index is now done
> > > > in cpu_exec_realizefn().
> > > > 
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > [...]
> > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > > index 1b9540e..364a45d 100644
> > > > --- a/target-arm/cpu.c
> > > > +++ b/target-arm/cpu.c
> > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
> > > >      CPUState *cs = CPU(obj);
> > > >      ARMCPU *cpu = ARM_CPU(obj);
> > > >      static bool inited;
> > > > -    uint32_t Aff1, Aff0;
> > > >  
> > > >      cs->env_ptr = &cpu->env;
> > > > -    cpu_exec_init(cs, &error_abort);
> > > >      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> > > >                                           g_free, g_free);
> > > >  
> > > > -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > > -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > > -     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > > -     * so these bits always RAZ.
> > > > -     */
> > > > -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > -
> > > >  #ifndef CONFIG_USER_ONLY
> > > >      /* Our inbound IRQ and FIQ lines */
> > > >      if (kvm_enabled()) {
> > > [...]
> > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> > > >          set_feature(env, ARM_FEATURE_THUMB_DSP);
> > > >      }
> > > >  
> > > > +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > > +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > > +     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > > +     * so these bits always RAZ.
> > > > +     */
> > > > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > +
> > > 
> > > This will override any value set in the "mp-affinity" property,
> > > The mp-affinity property can be set by the user in the
> > > command-line, and it is also set by machvirt_init() in
> > > hw/arm/virt.c.
> > 
> > I'm glad you noticed this Eduardo. We'd indeed lose changes made to
> > the MPIDRs for mach-virt and Raspberry Pi.
> > 
> > > 
> > > Considering that each CPU is supposed to have a different value,
> > > I doubt there are existing use cases for mp-affinity being set
> > > directly by the user.
> > 
> > Probably not. It was turned into a property in order for the gicv3
> > model to access it. I don't think that's necessary, so we can just
> > un-property it, accessing it directly from the structure instead.
> > 
> > > 
> > > I suggest having a "cluster-size" property, instead of
> > > "mp-affinity". This way the mp_affinity field can be calculated
> > > on realize, based on the configured cluster-size.
> > 
> > This won't work for Raspberry Pi's MPIDR adjustment.
> 
> Where's that code?

hw/arm/bcm2836.c:109

> 
> > Anyway, IMO realize should only set values when it knows
> > nothing has been set.
> 
> Realize could also initialize some (private) fields using other
> (public) fields as input. Instead of making machine code
> calculate mp_affinity, machine code could just provide input for
> realize to calculate the right values.
> 
> But maybe my cluster-size suggestion won't work anyway because of
> other cases where mp_affinity is set (I didn't check all code
> that sets/gets mp_affinity).

Anyway, cluster-size is machine state, not cpu state.

> 
> > If values have been set, it should only sanity check them. In this
> > case it's safe to simply check for uniqueness and zeros;
> > 
> >  1) The MPIDRs are all zero. As they're not all unique that's not
> >     valid. It's pretty safe to assume they just weren't set in this
> >     case though, so we can set the default values without complaining.
> >     If there was only one cpu, then MPIDR=0 is valid, but that's the
> >     default anyway.
> 
> Implementing this check in arm_cpu_realizefn() would be
> difficult, as the CPU realize method don't have access to the
> other CPUs. But maybe we can use some other default value to
> indicate that the field was never set? UINT64_MAX?

I considered that, but if we un-property ARMCPU->mp_affinity, then where
can it be initialized to "MP_AFFINITY_INVALID", which would be ff000000?

> 
> >  2) The MPIDRs are all unique, or there's a single cpu and its MPIDR
> >     is not zero. In this case we shouldn't touch them.
> >  3) The MPIDRs are not all zero and not all unique. This is a failed
> >     sanity check and should abort.
> 
> A sanity check would be useful, but I don't know where exactly it
> could be implemented. Is there any post-machine-init hook you could use?

We do have virt_guest_info_machine_done for mach-virt, but it'd be
sort of ugly to put it there...

This is one of those things where it's true that the set of all cpu
MPIDRs is machine state, and thus should be checked by the machine for
uniqueness. But, the purpose of checking is to ensure each CPU instance
is sane, a CPU internal motivation. Doing the check at the machine level
is ugly. Doing the check at the CPU level is either not possible or
breaks the object model. Sigh... maybe we should just forget it.


OK, to proceed with this patch, since mp-affinity *is* currently a
property, we can just change its default value to ff000000. Then,
when moving the calculation to realize we just need to ensure the
current value is ff000000 before overwriting it.

Another comment on this patch; please rename the definition
ARM_CPUS_PER_CLUSTER to ARM_DEFAULT_CPUS_PER_CLUSTER, and move it
down to just above arm_cpu_realizefn.

Thanks,
drew

On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote:
> On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote:
> > > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote:
> > > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> > > > > Modify all CPUs to call it from XXX_cpu_realizefn() function.
> > > > > 
> > > > > Remove all the cannot_destroy_with_object_finalize_yet as
> > > > > unsafe references have been moved to cpu_exec_realizefn().
> > > > > (tested with QOM command provided by commit 4c315c27)
> > > > > 
> > > > > for arm:
> > > > > 
> > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> > > > > to arm_cpu_realizefn() as setting of cpu_index is now done
> > > > > in cpu_exec_realizefn().
> > > > > 
> > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > [...]
> > > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > > > index 1b9540e..364a45d 100644
> > > > > --- a/target-arm/cpu.c
> > > > > +++ b/target-arm/cpu.c
> > > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
> > > > >      CPUState *cs = CPU(obj);
> > > > >      ARMCPU *cpu = ARM_CPU(obj);
> > > > >      static bool inited;
> > > > > -    uint32_t Aff1, Aff0;
> > > > >  
> > > > >      cs->env_ptr = &cpu->env;
> > > > > -    cpu_exec_init(cs, &error_abort);
> > > > >      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> > > > >                                           g_free, g_free);
> > > > >  
> > > > > -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > > > -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > > > -     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > > > -     * so these bits always RAZ.
> > > > > -     */
> > > > > -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > > -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > > -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > > -
> > > > >  #ifndef CONFIG_USER_ONLY
> > > > >      /* Our inbound IRQ and FIQ lines */
> > > > >      if (kvm_enabled()) {
> > > > [...]
> > > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> > > > >          set_feature(env, ARM_FEATURE_THUMB_DSP);
> > > > >      }
> > > > >  
> > > > > +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > > > +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > > > +     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > > > +     * so these bits always RAZ.
> > > > > +     */
> > > > > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > > +
> > > > 
> > > > This will override any value set in the "mp-affinity" property,
> > > > The mp-affinity property can be set by the user in the
> > > > command-line, and it is also set by machvirt_init() in
> > > > hw/arm/virt.c.
> > > 
> > > I'm glad you noticed this Eduardo. We'd indeed lose changes made to
> > > the MPIDRs for mach-virt and Raspberry Pi.
> > > 
> > > > 
> > > > Considering that each CPU is supposed to have a different value,
> > > > I doubt there are existing use cases for mp-affinity being set
> > > > directly by the user.
> > > 
> > > Probably not. It was turned into a property in order for the gicv3
> > > model to access it. I don't think that's necessary, so we can just
> > > un-property it, accessing it directly from the structure instead.
> > > 
> > > > 
> > > > I suggest having a "cluster-size" property, instead of
> > > > "mp-affinity". This way the mp_affinity field can be calculated
> > > > on realize, based on the configured cluster-size.
> > > 
> > > This won't work for Raspberry Pi's MPIDR adjustment.
> > 
> > Where's that code?
> 
> hw/arm/bcm2836.c:109

Thanks!

> 
> > 
> > > Anyway, IMO realize should only set values when it knows
> > > nothing has been set.
> > 
> > Realize could also initialize some (private) fields using other
> > (public) fields as input. Instead of making machine code
> > calculate mp_affinity, machine code could just provide input for
> > realize to calculate the right values.
> > 
> > But maybe my cluster-size suggestion won't work anyway because of
> > other cases where mp_affinity is set (I didn't check all code
> > that sets/gets mp_affinity).
> 
> Anyway, cluster-size is machine state, not cpu state.

Right: if in this case the realize function can query
current_machine for input instead of requiring a new
field/property to be used as input, that's even better. But
sometimes we can't easily avoid adding fields/properties that
aren't device state, to be used as input to realize (e.g.
nr_cores/nr_threads).

(This is just theoretical discussion, anyway, as just using
cluster-size as input probably doesn't solve Raspberry Pi's
problem?)

> 
> > 
> > > If values have been set, it should only sanity check them. In this
> > > case it's safe to simply check for uniqueness and zeros;
> > > 
> > >  1) The MPIDRs are all zero. As they're not all unique that's not
> > >     valid. It's pretty safe to assume they just weren't set in this
> > >     case though, so we can set the default values without complaining.
> > >     If there was only one cpu, then MPIDR=0 is valid, but that's the
> > >     default anyway.
> > 
> > Implementing this check in arm_cpu_realizefn() would be
> > difficult, as the CPU realize method don't have access to the
> > other CPUs. But maybe we can use some other default value to
> > indicate that the field was never set? UINT64_MAX?
> 
> I considered that, but if we un-property ARMCPU->mp_affinity, then where
> can it be initialized to "MP_AFFINITY_INVALID", which would be ff000000?

Property defaults are normally initialized on instance_init.

> 
> > 
> > >  2) The MPIDRs are all unique, or there's a single cpu and its MPIDR
> > >     is not zero. In this case we shouldn't touch them.
> > >  3) The MPIDRs are not all zero and not all unique. This is a failed
> > >     sanity check and should abort.
> > 
> > A sanity check would be useful, but I don't know where exactly it
> > could be implemented. Is there any post-machine-init hook you could use?
> 
> We do have virt_guest_info_machine_done for mach-virt, but it'd be
> sort of ugly to put it there...
> 
> This is one of those things where it's true that the set of all cpu
> MPIDRs is machine state, and thus should be checked by the machine for
> uniqueness. But, the purpose of checking is to ensure each CPU instance
> is sane, a CPU internal motivation. Doing the check at the machine level
> is ugly. Doing the check at the CPU level is either not possible or
> breaks the object model. Sigh... maybe we should just forget it.
> 
> 
> OK, to proceed with this patch, since mp-affinity *is* currently a
> property, we can just change its default value to ff000000. Then,
> when moving the calculation to realize we just need to ensure the
> current value is ff000000 before overwriting it.

Sounds good to me.

> [...]

On Tue, Oct 18, 2016 at 01:22:07PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote:
> > On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote:
> > > On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote:
> > > > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote:
> > > > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote:
> > > > > > Modify all CPUs to call it from XXX_cpu_realizefn() function.
> > > > > > 
> > > > > > Remove all the cannot_destroy_with_object_finalize_yet as
> > > > > > unsafe references have been moved to cpu_exec_realizefn().
> > > > > > (tested with QOM command provided by commit 4c315c27)
> > > > > > 
> > > > > > for arm:
> > > > > > 
> > > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
> > > > > > to arm_cpu_realizefn() as setting of cpu_index is now done
> > > > > > in cpu_exec_realizefn().
> > > > > > 
> > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > > [...]
> > > > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > > > > index 1b9540e..364a45d 100644
> > > > > > --- a/target-arm/cpu.c
> > > > > > +++ b/target-arm/cpu.c
> > > > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj)
> > > > > >      CPUState *cs = CPU(obj);
> > > > > >      ARMCPU *cpu = ARM_CPU(obj);
> > > > > >      static bool inited;
> > > > > > -    uint32_t Aff1, Aff0;
> > > > > >  
> > > > > >      cs->env_ptr = &cpu->env;
> > > > > > -    cpu_exec_init(cs, &error_abort);
> > > > > >      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> > > > > >                                           g_free, g_free);
> > > > > >  
> > > > > > -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > > > > -     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > > > > -     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > > > > -     * so these bits always RAZ.
> > > > > > -     */
> > > > > > -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > > > -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > > > -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > > > -
> > > > > >  #ifndef CONFIG_USER_ONLY
> > > > > >      /* Our inbound IRQ and FIQ lines */
> > > > > >      if (kvm_enabled()) {
> > > > > [...]
> > > > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> > > > > >          set_feature(env, ARM_FEATURE_THUMB_DSP);
> > > > > >      }
> > > > > >  
> > > > > > +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
> > > > > > +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> > > > > > +     * in later ARM ARM versions), or any of the higher affinity level fields,
> > > > > > +     * so these bits always RAZ.
> > > > > > +     */
> > > > > > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > > > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > > > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > > > +
> > > > > 
> > > > > This will override any value set in the "mp-affinity" property,
> > > > > The mp-affinity property can be set by the user in the
> > > > > command-line, and it is also set by machvirt_init() in
> > > > > hw/arm/virt.c.
> > > > 
> > > > I'm glad you noticed this Eduardo. We'd indeed lose changes made to
> > > > the MPIDRs for mach-virt and Raspberry Pi.
> > > > 
> > > > > 
> > > > > Considering that each CPU is supposed to have a different value,
> > > > > I doubt there are existing use cases for mp-affinity being set
> > > > > directly by the user.
> > > > 
> > > > Probably not. It was turned into a property in order for the gicv3
> > > > model to access it. I don't think that's necessary, so we can just
> > > > un-property it, accessing it directly from the structure instead.
> > > > 
> > > > > 
> > > > > I suggest having a "cluster-size" property, instead of
> > > > > "mp-affinity". This way the mp_affinity field can be calculated
> > > > > on realize, based on the configured cluster-size.
> > > > 
> > > > This won't work for Raspberry Pi's MPIDR adjustment.
> > > 
> > > Where's that code?
> > 
> > hw/arm/bcm2836.c:109
> 
> Thanks!
> 
> > 
> > > 
> > > > Anyway, IMO realize should only set values when it knows
> > > > nothing has been set.
> > > 
> > > Realize could also initialize some (private) fields using other
> > > (public) fields as input. Instead of making machine code
> > > calculate mp_affinity, machine code could just provide input for
> > > realize to calculate the right values.
> > > 
> > > But maybe my cluster-size suggestion won't work anyway because of
> > > other cases where mp_affinity is set (I didn't check all code
> > > that sets/gets mp_affinity).
> > 
> > Anyway, cluster-size is machine state, not cpu state.
> 
> Right: if in this case the realize function can query
> current_machine for input instead of requiring a new
> field/property to be used as input, that's even better. But
> sometimes we can't easily avoid adding fields/properties that
> aren't device state, to be used as input to realize (e.g.
> nr_cores/nr_threads).
> 
> (This is just theoretical discussion, anyway, as just using
> cluster-size as input probably doesn't solve Raspberry Pi's
> problem?)

Agreed it's theoretical, but still interesting. I'm always unsure
what good practice with the object model is. I'll note that it's
accepted to reach into machine state through current_machine from
the machine's devices. I'll need to apply that when I get around
to respinning the 'smp rework' series.

> 
> > 
> > > 
> > > > If values have been set, it should only sanity check them. In this
> > > > case it's safe to simply check for uniqueness and zeros;
> > > > 
> > > >  1) The MPIDRs are all zero. As they're not all unique that's not
> > > >     valid. It's pretty safe to assume they just weren't set in this
> > > >     case though, so we can set the default values without complaining.
> > > >     If there was only one cpu, then MPIDR=0 is valid, but that's the
> > > >     default anyway.
> > > 
> > > Implementing this check in arm_cpu_realizefn() would be
> > > difficult, as the CPU realize method don't have access to the
> > > other CPUs. But maybe we can use some other default value to
> > > indicate that the field was never set? UINT64_MAX?
> > 
> > I considered that, but if we un-property ARMCPU->mp_affinity, then where
> > can it be initialized to "MP_AFFINITY_INVALID", which would be ff000000?
> 
> Property defaults are normally initialized on instance_init.

OK, so after this series goes through we can un-property mp_affinity and
add an mp_affinity initialization back to arm_cpu_initfn, but one that
only sets the initial invalid value.

> 
> > 
> > > 
> > > >  2) The MPIDRs are all unique, or there's a single cpu and its MPIDR
> > > >     is not zero. In this case we shouldn't touch them.
> > > >  3) The MPIDRs are not all zero and not all unique. This is a failed
> > > >     sanity check and should abort.
> > > 
> > > A sanity check would be useful, but I don't know where exactly it
> > > could be implemented. Is there any post-machine-init hook you could use?
> > 
> > We do have virt_guest_info_machine_done for mach-virt, but it'd be
> > sort of ugly to put it there...
> > 
> > This is one of those things where it's true that the set of all cpu
> > MPIDRs is machine state, and thus should be checked by the machine for
> > uniqueness. But, the purpose of checking is to ensure each CPU instance
> > is sane, a CPU internal motivation. Doing the check at the machine level
> > is ugly. Doing the check at the CPU level is either not possible or
> > breaks the object model. Sigh... maybe we should just forget it.
> > 
> > 
> > OK, to proceed with this patch, since mp-affinity *is* currently a
> > property, we can just change its default value to ff000000. Then,
> > when moving the calculation to realize we just need to ensure the
> > current value is ff000000 before overwriting it.
> 
> Sounds good to me.

Good. Laurent, to be clear, MP_AFFINITY_INVALID doesn't currently exist,
and it should be defined as ~ARM64_AFFINITY_MASK

Thanks,
drew

On 18/10/2016 18:22, Andrew Jones wrote:
> On Tue, Oct 18, 2016 at 01:22:07PM -0200, Eduardo Habkost wrote:
>> On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote:
...
>>> OK, to proceed with this patch, since mp-affinity *is* currently a
>>> property, we can just change its default value to ff000000. Then,
>>> when moving the calculation to realize we just need to ensure the
>>> current value is ff000000 before overwriting it.
>>
>> Sounds good to me.
> 
> Good. Laurent, to be clear, MP_AFFINITY_INVALID doesn't currently exist,
> and it should be defined as ~ARM64_AFFINITY_MASK

OK, thank you for all your comments.

I'm going to update my series with this change.

Thanks,
Laurent

On 18 October 2016 at 17:22, Andrew Jones <drjones@redhat.com> wrote:
> I'll note that it's
> accepted to reach into machine state through current_machine from
> the machine's devices.

That doesn't sound like a great idea to me -- where do we
do it? A quick grep for uses of current_machine in hw/ shows only
hw/ppc/spapr_rtas.c, which is a bit of a weirdo thing because
it's the implementation of a bunch of hypercalls, not an actual
device.

We have lots of code currently in devices that looks at bits
of overall state that it should ideally not (but which it can
because we have a lot of global variables). In an ideal world
we'd clean these up so that the devices were initialized with
suitable properties instead to tell them about this global state.

>> > I considered that, but if we un-property ARMCPU->mp_affinity, then where
>> > can it be initialized to "MP_AFFINITY_INVALID", which would be ff000000?
>>
>> Property defaults are normally initialized on instance_init.
>
> OK, so after this series goes through we can un-property mp_affinity and
> add an mp_affinity initialization back to arm_cpu_initfn, but one that
> only sets the initial invalid value.

Note that mp_affinity is a bit awkward because for KVM we can't
get the right values out of the kernel until quite late (when
kvm_init_vcpu() is called).

Why do you want to un-property mp_affinity? Eventually it would
be nice for the machine model to be able to use it to set up
a specific NUMA configuration.

thanks
-- PMM

On Tue, Oct 18, 2016 at 06:07:49PM +0100, Peter Maydell wrote:
> On 18 October 2016 at 17:22, Andrew Jones <drjones@redhat.com> wrote:
> > I'll note that it's
> > accepted to reach into machine state through current_machine from
> > the machine's devices.
> 
> That doesn't sound like a great idea to me -- where do we
> do it? A quick grep for uses of current_machine in hw/ shows only
> hw/ppc/spapr_rtas.c, which is a bit of a weirdo thing because
> it's the implementation of a bunch of hypercalls, not an actual
> device.
> 
> We have lots of code currently in devices that looks at bits
> of overall state that it should ideally not (but which it can
> because we have a lot of global variables). In an ideal world
> we'd clean these up so that the devices were initialized with
> suitable properties instead to tell them about this global state.
> 
> >> > I considered that, but if we un-property ARMCPU->mp_affinity, then where
> >> > can it be initialized to "MP_AFFINITY_INVALID", which would be ff000000?
> >>
> >> Property defaults are normally initialized on instance_init.
> >
> > OK, so after this series goes through we can un-property mp_affinity and
> > add an mp_affinity initialization back to arm_cpu_initfn, but one that
> > only sets the initial invalid value.
> 
> Note that mp_affinity is a bit awkward because for KVM we can't
> get the right values out of the kernel until quite late (when
> kvm_init_vcpu() is called).
> 
> Why do you want to un-property mp_affinity? Eventually it would
> be nice for the machine model to be able to use it to set up
> a specific NUMA configuration.

I thought about that, but I think we'll want to specify machine
properties; nr_sockets, nr_cores, nr_threads and use the -device
command line for the cpu to specify which socket, which core,
which thread it is. This would be consistent with other architectures
and easily map to the MPIDR & cpu topology hardware descriptions.
Anyway, atm, I don't know of any reason to have the property user-
settable, so it seems safest to keep it hidden until we decide.

Thanks,
drew

On 18 October 2016 at 18:57, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Oct 18, 2016 at 06:07:49PM +0100, Peter Maydell wrote:
>> Why do you want to un-property mp_affinity? Eventually it would
>> be nice for the machine model to be able to use it to set up
>> a specific NUMA configuration.
>
> I thought about that, but I think we'll want to specify machine
> properties; nr_sockets, nr_cores, nr_threads and use the -device
> command line for the cpu to specify which socket, which core,
> which thread it is. This would be consistent with other architectures
> and easily map to the MPIDR & cpu topology hardware descriptions.

I was thinking more about "modelling board X, which we know
always has 2xA53 and 4xA57 with these MPIDRs".

We actually have a concrete instance in the tree at the moment:
the raspberry pi 2. Specifically hw/arm/bcm2836.c sets the
mp_affinity for each cpu to 0xF00 | n (where n is the CPUID).
Currently it's doing that by reaching in and messing with
the mp_affinity field directly, but really it ought to be
doing it by setting a property on the CPU, and what it
wants isn't somethnig that can be expressed with a simple
nr_sockets/nr_cores/etc scheme.

> Anyway, atm, I don't know of any reason to have the property user-
> settable, so it seems safest to keep it hidden until we decide.

I agree that it doesn't make sense to let the user mess with it,
but it should be available for the board code to read and write.

thanks
-- PMM

On Tue, Oct 18, 2016 at 07:12:51PM +0100, Peter Maydell wrote:
> On 18 October 2016 at 18:57, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Oct 18, 2016 at 06:07:49PM +0100, Peter Maydell wrote:
> >> Why do you want to un-property mp_affinity? Eventually it would
> >> be nice for the machine model to be able to use it to set up
> >> a specific NUMA configuration.
> >
> > I thought about that, but I think we'll want to specify machine
> > properties; nr_sockets, nr_cores, nr_threads and use the -device
> > command line for the cpu to specify which socket, which core,
> > which thread it is. This would be consistent with other architectures
> > and easily map to the MPIDR & cpu topology hardware descriptions.
> 
> I was thinking more about "modelling board X, which we know
> always has 2xA53 and 4xA57 with these MPIDRs".
> 
> We actually have a concrete instance in the tree at the moment:
> the raspberry pi 2. Specifically hw/arm/bcm2836.c sets the
> mp_affinity for each cpu to 0xF00 | n (where n is the CPUID).
> Currently it's doing that by reaching in and messing with
> the mp_affinity field directly, but really it ought to be
> doing it by setting a property on the CPU, and what it
> wants isn't somethnig that can be expressed with a simple
> nr_sockets/nr_cores/etc scheme.

I am confused now. I thought QOM properties were meant for
user-visible and/or user-configurable data. I assumed that if
it's only meant to be used by C code inside QEMU, C functions
and/or C struct fields were the way to go.

See a previous thread where this was discussed:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg387169.html

(Subject: "QOM: best way for parents to pass information to
children?")

> 
> > Anyway, atm, I don't know of any reason to have the property user-
> > settable, so it seems safest to keep it hidden until we decide.
> 
> I agree that it doesn't make sense to let the user mess with it,
> but it should be available for the board code to read and write.

If it doesn't make sense to let the user mess with it, why would
we make it a QOM property?

On 18 October 2016 at 19:45, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 18, 2016 at 07:12:51PM +0100, Peter Maydell wrote:
>> We actually have a concrete instance in the tree at the moment:
>> the raspberry pi 2. Specifically hw/arm/bcm2836.c sets the
>> mp_affinity for each cpu to 0xF00 | n (where n is the CPUID).
>> Currently it's doing that by reaching in and messing with
>> the mp_affinity field directly, but really it ought to be
>> doing it by setting a property on the CPU, and what it
>> wants isn't somethnig that can be expressed with a simple
>> nr_sockets/nr_cores/etc scheme.
>
> I am confused now. I thought QOM properties were meant for
> user-visible and/or user-configurable data. I assumed that if
> it's only meant to be used by C code inside QEMU, C functions
> and/or C struct fields were the way to go.

Lots of stuff in a device's C struct is strictly internal
and not to be messed with. I thought that QOM properties
were essentially how a device defined its public (and
typically settable-only-once) config knobs. QOM properties
shouldn't be user-facing (read: stable, required to be
backwards-compatible) interface in general because
we don't really want to tie ourselves down that much.
In fact almost all our QOM objects are not usefully
user-facing at all.

thanks
-- PMM

On Tue, Oct 18, 2016 at 09:30:01PM +0100, Peter Maydell wrote:
> On 18 October 2016 at 19:45, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Oct 18, 2016 at 07:12:51PM +0100, Peter Maydell wrote:
> >> We actually have a concrete instance in the tree at the moment:
> >> the raspberry pi 2. Specifically hw/arm/bcm2836.c sets the
> >> mp_affinity for each cpu to 0xF00 | n (where n is the CPUID).
> >> Currently it's doing that by reaching in and messing with
> >> the mp_affinity field directly, but really it ought to be
> >> doing it by setting a property on the CPU, and what it
> >> wants isn't somethnig that can be expressed with a simple
> >> nr_sockets/nr_cores/etc scheme.
> >
> > I am confused now. I thought QOM properties were meant for
> > user-visible and/or user-configurable data. I assumed that if
> > it's only meant to be used by C code inside QEMU, C functions
> > and/or C struct fields were the way to go.
> 
> Lots of stuff in a device's C struct is strictly internal
> and not to be messed with. I thought that QOM properties
> were essentially how a device defined its public (and
> typically settable-only-once) config knobs. QOM properties
> shouldn't be user-facing (read: stable, required to be
> backwards-compatible) interface in general because
> we don't really want to tie ourselves down that much.
> In fact almost all our QOM objects are not usefully
> user-facing at all.

This interpretation surprises me, because it is the opposite of
what I have seen us doing. Most of our QOM objects and properties
are user-visible and user-configurable using -global, -device,
-object, or qom-set (and probably other QMP commands).

Some QOM properties are internal and we have been using the "x-"
prefix to indicate that, but most of them do _not_ have the "x-"
prefix.

On 18 October 2016 at 21:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 18, 2016 at 09:30:01PM +0100, Peter Maydell wrote:
>> Lots of stuff in a device's C struct is strictly internal
>> and not to be messed with. I thought that QOM properties
>> were essentially how a device defined its public (and
>> typically settable-only-once) config knobs. QOM properties
>> shouldn't be user-facing (read: stable, required to be
>> backwards-compatible) interface in general because
>> we don't really want to tie ourselves down that much.
>> In fact almost all our QOM objects are not usefully
>> user-facing at all.
>
> This interpretation surprises me, because it is the opposite of
> what I have seen us doing. Most of our QOM objects and properties
> are user-visible and user-configurable using -global, -device,
> -object, or qom-set (and probably other QMP commands).

Most of the devices I deal with are not and never will
be sensibly usable with -device. Exposing wiring up
of IRQ and GPIO lines or MMIO regions to the user is
never going to make sense. For x86 most devices are
probably pluggable (and usable with -device), but over
the whole source tree I think the embedded-style device
is in the majority. They're all still worth QOMifying
and having properties for the things board code wants
to modify, though.

thanks
-- PMM

On Tue, Oct 18, 2016 at 10:08:21PM +0100, Peter Maydell wrote:
> On 18 October 2016 at 21:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Oct 18, 2016 at 09:30:01PM +0100, Peter Maydell wrote:
> >> Lots of stuff in a device's C struct is strictly internal
> >> and not to be messed with. I thought that QOM properties
> >> were essentially how a device defined its public (and
> >> typically settable-only-once) config knobs. QOM properties
> >> shouldn't be user-facing (read: stable, required to be
> >> backwards-compatible) interface in general because
> >> we don't really want to tie ourselves down that much.
> >> In fact almost all our QOM objects are not usefully
> >> user-facing at all.
> >
> > This interpretation surprises me, because it is the opposite of
> > what I have seen us doing. Most of our QOM objects and properties
> > are user-visible and user-configurable using -global, -device,
> > -object, or qom-set (and probably other QMP commands).
> 
> Most of the devices I deal with are not and never will
> be sensibly usable with -device. Exposing wiring up
> of IRQ and GPIO lines or MMIO regions to the user is
> never going to make sense. For x86 most devices are
> probably pluggable (and usable with -device), but over
> the whole source tree I think the embedded-style device
> is in the majority. They're all still worth QOMifying
> and having properties for the things board code wants
> to modify, though.

Even if they are not usable with -device, all properties are
configurable using -global. There's no mechanism to avoid letting
the user configure properties for devices. Is this really OK?

If internal-only usage is also an intended use case for QOM
properties, fine. But I believe we need to communicate this more
clearly, based on the previous thread (subject: "QOM: best way
for parents to pass information to children?").

BTW, if most devices aren't supposed to be used with -device,
possibly many of them don't have
cannot_instantiate_with_device_add_yet set properly. On the 2.6.2
binaries in Fedora 24, I see:

* 1671 device types (including CPU types)
* 1011 CPU device types
* 1076 no-user device types (including CPU types)
* 660 non-CPU device types
* 65 no-user non-CPU device types (10% of them)
* 860 type_register_*() lines in the code
  (this includes non-TYPE_DEVICE types, though)
* 56 cannot_instantiate_with_device_add_yet=true lines in the code

Commands I used to get the numbers above:

  $ for f in /usr/bin/qemu-system-*;do \
     (echo 'info qdm';echo quit;) | $f -machine none -nodefaults -monitor stdio -nographic 2>&1 \
    done | grep ^name | sort -u > /tmp/devs
  $ wc -l /tmp/devs
  1671 /tmp/devs
  $ grep no-user /tmp/devs | wc -l
  1076
  $ grep -- '-cpu"' /tmp/devs | wc -l
  1011
  $ grep -v -- '-cpu"' /tmp/devs | wc -l
  660
  $ grep -v -- '-cpu"' /tmp/devs | grep no-user | wc -l
  65
  $

On 19 October 2016 at 12:11, Eduardo Habkost <ehabkost@redhat.com> wrote:
> BTW, if most devices aren't supposed to be used with -device,
> possibly many of them don't have
> cannot_instantiate_with_device_add_yet set properly.

They used to be covered by hw/core/sysbus.c setting
it for them. In commit 33cd52b5d7b9adf that was removed,
because under certain specialized circumstances it
is now possible to dynamically add a sysbus device.
If the machine model doesn't set mc->has_dynamic_sysbus
they'll error out via a different route. If the machine
model does set that option then it'll probably just do
something unhelpful unless the user knew what they were
doing.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 18 October 2016 at 21:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Tue, Oct 18, 2016 at 09:30:01PM +0100, Peter Maydell wrote:
>>> Lots of stuff in a device's C struct is strictly internal
>>> and not to be messed with. I thought that QOM properties
>>> were essentially how a device defined its public (and
>>> typically settable-only-once) config knobs. QOM properties
>>> shouldn't be user-facing (read: stable, required to be
>>> backwards-compatible) interface in general because
>>> we don't really want to tie ourselves down that much.
>>> In fact almost all our QOM objects are not usefully
>>> user-facing at all.
>>
>> This interpretation surprises me, because it is the opposite of
>> what I have seen us doing. Most of our QOM objects and properties
>> are user-visible and user-configurable using -global, -device,
>> -object, or qom-set (and probably other QMP commands).
>
> Most of the devices I deal with are not and never will
> be sensibly usable with -device. Exposing wiring up
> of IRQ and GPIO lines or MMIO regions to the user is
> never going to make sense. For x86 most devices are
> probably pluggable (and usable with -device), but over
> the whole source tree I think the embedded-style device
> is in the majority. They're all still worth QOMifying
> and having properties for the things board code wants
> to modify, though.

"Device not pluggable" does not imply "device has no configuration knobs
a user may legitimately want to mess with".  Plenty of onboard devices
have such knobs.

Right now, users configure these mostly via board-agnostic options like
-serial.  Anything that doesn't fit the mold can't be configured that
way.

However, A fully mature QOM as I envisage it would provide users access
to QOM properties for onboard devices, too.  Not with -device,
obviously, but with qom-set and similar, as Eduardo said.  If any of
these properties are not for users, they should be marked as such.  Just
like for pluggable devices.

Perhaps non-pluggable devices tend to have more "not for users" QOM
properties than pluggable ones, I don't know.  But that would be a
*quantitative* difference, not a *qualitative* one.

On 21 October 2016 at 19:26, Markus Armbruster <armbru@redhat.com> wrote:
> "Device not pluggable" does not imply "device has no configuration knobs
> a user may legitimately want to mess with".  Plenty of onboard devices
> have such knobs.
>
> Right now, users configure these mostly via board-agnostic options like
> -serial.  Anything that doesn't fit the mold can't be configured that
> way.
>
> However, A fully mature QOM as I envisage it would provide users access
> to QOM properties for onboard devices, too.  Not with -device,
> obviously, but with qom-set and similar, as Eduardo said.  If any of
> these properties are not for users, they should be marked as such.  Just
> like for pluggable devices.

Yes, you're right about this. (What's the command line
equivalent of qom-set? We have -global but that's a bit
awkward if I'm remembering its syntax correctly.)

> Perhaps non-pluggable devices tend to have more "not for users" QOM
> properties than pluggable ones, I don't know.  But that would be a
> *quantitative* difference, not a *qualitative* one.

I agree that it's not really qualitative, but a pluggable
device's properties are all by definition for the user
to set (since the user is the only one who can set them).
In a pre-plugged device, although there may be a lot of
properties, some of them won't be usefully changeable
in the context of this device in this board (ie if you
mess with them you'll just break things). We don't have
any way for the board to say "this stuff isn't for the
user", I think.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 21 October 2016 at 19:26, Markus Armbruster <armbru@redhat.com> wrote:
>> "Device not pluggable" does not imply "device has no configuration knobs
>> a user may legitimately want to mess with".  Plenty of onboard devices
>> have such knobs.
>>
>> Right now, users configure these mostly via board-agnostic options like
>> -serial.  Anything that doesn't fit the mold can't be configured that
>> way.
>>
>> However, A fully mature QOM as I envisage it would provide users access
>> to QOM properties for onboard devices, too.  Not with -device,
>> obviously, but with qom-set and similar, as Eduardo said.  If any of
>> these properties are not for users, they should be marked as such.  Just
>> like for pluggable devices.
>
> Yes, you're right about this. (What's the command line
> equivalent of qom-set?

Command line isn't implemented.

>                        We have -global but that's a bit
> awkward if I'm remembering its syntax correctly.)

-global changes a device property default value.  You can abuse it to
set a specific device's property only when there's at most one instance.

>> Perhaps non-pluggable devices tend to have more "not for users" QOM
>> properties than pluggable ones, I don't know.  But that would be a
>> *quantitative* difference, not a *qualitative* one.
>
> I agree that it's not really qualitative, but a pluggable
> device's properties are all by definition for the user
> to set (since the user is the only one who can set them).
> In a pre-plugged device, although there may be a lot of
> properties, some of them won't be usefully changeable
> in the context of this device in this board (ie if you
> mess with them you'll just break things). We don't have
> any way for the board to say "this stuff isn't for the
> user", I think.

The closest we have now is the x-prefix convention.

Perhaps we could use a flag to mark certain properties as "not
user-settable", similar to how we mark devices as "not pluggable"
(really: can be created only by board code, not the user).