[Qemu-devel] [PATCH v4 0/3] Split cpu_exec_init() into an init and a realize part

[Qemu-devel] [PATCH v4 0/3] Split cpu_exec_init() into an init and a realize part

[Laurent Vivier]

Since commit 42ecaba ("target-i386: Call cpu_exec_init() on realize"),
, commit 6dd0f83 ("target-ppc: Move cpu_exec_init() call to realize function"),
and commit c6644fc ("s390x/cpu: Get rid of side effects when creating a vcpu"),
cpu_exec_init() has been moved to realize function for some architectures
to implement CPU htoplug. This allows any failures from cpu_exec_init() to be
handled appropriately.

This series tries to do the same work for all the other CPUs.

But as the ARM Virtual Machine ("virt") needs the "memory" property of the CPU
in the machine init function (the "memory" property is created in
cpu_exec_init() we want to move to the realize part), split cpu_exec_init() in
two parts: a realize part (cpu_exec_realizefn(), adding the CPU in the
environment) and an init part (cpu_exec_initfn(), initializing the CPU, like
adding the "memory" property). To mirror the realize part, add an unrealize
part, and remove the cpu_exec_exit() call from the finalize part.

This also allows to remove all the "cannot_destroy_with_object_finalize_yet"
properties from the CPU device class.

v4:
- arm: update mp_affinity in realize only if it s not set by the user
- i386 and ppc: add an parent_unrealize to call the Device unrealize
  because it is overwritten by the CPU unrealize
- update commit messages

v3:
- reorganize the series in 3 patches:
  1- split cpu_exec_init() and call the init part from
     cpu_common_initfn()
  2- move all the cpu_exec_realize() calls to the
     XXX_cpu_realizefn() functions
  3- create a cpu_common_unrealizefn() function
      and call the cpu_exec_unrealize() function from here,
      except for ppc64 and i386 that have their own XX_cpu_unrealizefn()
      functions
- rename cpu_exec_init(), cpu_exec_realize(), cpu_exec_unrealize()
  to cpu_exec_initfn(), cpu_exec_realizefn() and cpu_exec_unrealizefn(),
- move cpu_exec_unrealizefn() to the device class unrealize
  and declare it in qom/cpu.h instead of exec/exec-all.h

v2:
- rename cpu_exec_exit() as cpu_exec_unrealize(),
  as it un-does what cpu_exec_realize() does,
- remove cpu_exec_exit() from ppc_cpu_unrealizefn() as
  it is called from cpu_common_finalize(),
- add a patch to move all cpu_exec_init() from
  all XX_cpu_initfn() to cpu_common_initfn(),
- arm: move setting of cpu->mp_affinity to
  arm_cpu_realizefn() as the cpu_index is now set in
  cpu_exec_realizefn().
- update some commit messages

Laurent Vivier (3):
  exec: split cpu_exec_init()
  exec: move cpu_exec_init() calls to realize functions
  exec: call cpu_exec_exit() from a CPU unrealize common function

 exec.c                      | 12 +++++++-----
 include/exec/exec-all.h     |  1 -
 include/qom/cpu.h           |  4 +++-
 qom/cpu.c                   | 10 +++++++++-
 target-alpha/cpu.c          | 15 +++++++--------
 target-arm/cpu.c            | 45 ++++++++++++++++++++++-----------------------
 target-cris/cpu.c           | 15 +++++++--------
 target-i386/cpu-qom.h       |  1 +
 target-i386/cpu.c           | 20 ++++++++++++++------
 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/cpu-qom.h        |  1 +
 target-ppc/translate_init.c | 11 +++++++++--
 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 +++++++--------
 24 files changed, 169 insertions(+), 159 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/3] exec: split cpu_exec_init()

[Laurent Vivier]

Put in cpu_exec_initfn() what initializes the CPU,
and leave in cpu_exec_init() what adds it to the environment.

As cpu_exec_initfn() is called by all XX_cpu_initfn(), call it
directly in cpu_common_initfn().
cpu_exec_init() is now a realize function, it will be renamed
to cpu_exec_realizefn() and moved to the XX_cpu_realizefn()
function in a following patch.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c            | 10 ++++++----
 include/qom/cpu.h |  1 +
 qom/cpu.c         |  2 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 374c364..d1e57c4 100644
--- a/exec.c
+++ b/exec.c
@@ -610,11 +610,8 @@ void cpu_exec_exit(CPUState *cpu)
     }
 }
 
-void cpu_exec_init(CPUState *cpu, Error **errp)
+void cpu_exec_initfn(CPUState *cpu)
 {
-    CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
-    Error *local_err ATTRIBUTE_UNUSED = NULL;
-
     cpu->as = NULL;
     cpu->num_ases = 0;
 
@@ -635,6 +632,11 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     cpu->memory = system_memory;
     object_ref(OBJECT(cpu->memory));
 #endif
+}
+
+void cpu_exec_init(CPUState *cpu, Error **errp)
+{
+    CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
 
     cpu_list_add(cpu);
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 6d481a1..d7648a9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -946,6 +946,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_exit(CPUState *cpu);
 
 #ifdef CONFIG_SOFTMMU
diff --git a/qom/cpu.c b/qom/cpu.c
index c40f774..85f1132 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -362,6 +362,8 @@ static void cpu_common_initfn(Object *obj)
     QTAILQ_INIT(&cpu->watchpoints);
 
     cpu->trace_dstate = bitmap_new(trace_get_vcpu_event_count());
+
+    cpu_exec_initfn(cpu);
 }
 
 static void cpu_common_finalize(Object *obj)
-- 
2.7.4

[Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions

[Laurent Vivier]

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(). To avoid to overwrite an user defined
value, we set it to an invalid value by default, and update
it in realize function only if the value is still invalid.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
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            | 45 ++++++++++++++++++++++-----------------------
 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, 133 insertions(+), 152 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..f0d2074 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -33,6 +33,8 @@
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 
+#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
+
 static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -441,22 +443,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 +567,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 +630,17 @@ 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.
+     */
+    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
+        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);
     }
@@ -1461,7 +1471,7 @@ static Property arm_cpu_properties[] = {
     DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
     DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
     DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
-    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
+    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, MP_AFFINITY_INVALID),
     DEFINE_PROP_END_OF_LIST()
 };
 
@@ -1533,17 +1543,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

Re: [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions

[Andrew Jones]

On Tue, Oct 18, 2016 at 09:22:52PM +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(). To avoid to overwrite an user defined
> value, we set it to an invalid value by default, and update
> it in realize function only if the value is still invalid.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
[...]
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..f0d2074 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -33,6 +33,8 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  
> +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)

I would have defined this next to ARM64_AFFINITY_MASK in
target-arm/cpu-qom.h

> +
>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -441,22 +443,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 +567,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 +630,17 @@ 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.
> +     */
> +    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
> +        Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> +        Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;

I think ARM_CPUS_PER_CLUSTER should be renamed to
ARM_DEFAULT_CPUS_PER_CLUSTER and either moved from where
it's currently defined (above arm_cpu_initfn) to just above
arm_cpu_realizefn, or to the same place as ARM64_AFFINITY_MASK
and MP_AFFINITY_INVALID.

Aff0 and Aff1 could be declared in this scope, as they're only
used here.

> +        cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> +    }
> +
>      if (cpu->reset_hivecs) {
>              cpu->reset_sctlr |= (1 << 13);
>      }
> @@ -1461,7 +1471,7 @@ static Property arm_cpu_properties[] = {
>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
> -    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
> +    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, MP_AFFINITY_INVALID),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> @@ -1533,17 +1543,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)
[...]

Otherwise looks good to me.

Thanks,
drew

Re: [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions

[Laurent Vivier]

On 19/10/2016 10:13, Andrew Jones wrote:
> On Tue, Oct 18, 2016 at 09:22:52PM +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(). To avoid to overwrite an user defined
>> value, we set it to an invalid value by default, and update
>> it in realize function only if the value is still invalid.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> ---
> [...]
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 1b9540e..f0d2074 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -33,6 +33,8 @@
>>  #include "sysemu/kvm.h"
>>  #include "kvm_arm.h"
>>  
>> +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
> 
> I would have defined this next to ARM64_AFFINITY_MASK in
> target-arm/cpu-qom.h

It was my first idea, but all macros in cpu-qom.h start by ARM_ or
ARM64_, this one looks like a local macro (the name is too generic), so
I put it in cpu.c.

>> +
>>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>>  {
>>      ARMCPU *cpu = ARM_CPU(cs);
>> @@ -441,22 +443,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 +567,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 +630,17 @@ 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.
>> +     */
>> +    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
>> +        Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
>> +        Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> 
> I think ARM_CPUS_PER_CLUSTER should be renamed to
> ARM_DEFAULT_CPUS_PER_CLUSTER and either moved from where
> it's currently defined (above arm_cpu_initfn) to just above
> arm_cpu_realizefn, or to the same place as ARM64_AFFINITY_MASK
> and MP_AFFINITY_INVALID.
> 
> Aff0 and Aff1 could be declared in this scope, as they're only
> used here.

The goal was to have as less diff as possible with my previous patch, so
I didn't move the declaration. This will not change the generated code.
> 
>> +        cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
>> +    }
>> +
>>      if (cpu->reset_hivecs) {
>>              cpu->reset_sctlr |= (1 << 13);
>>      }
>> @@ -1461,7 +1471,7 @@ static Property arm_cpu_properties[] = {
>>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>> -    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
>> +    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, MP_AFFINITY_INVALID),
>>      DEFINE_PROP_END_OF_LIST()
>>  };
>>  
>> @@ -1533,17 +1543,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)
> [...]
> 
> Otherwise looks good to me.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions

[Andrew Jones]

On Wed, Oct 19, 2016 at 10:30:52AM +0200, Laurent Vivier wrote:
> 
> 
> On 19/10/2016 10:13, Andrew Jones wrote:
> > On Tue, Oct 18, 2016 at 09:22:52PM +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(). To avoid to overwrite an user defined
> >> value, we set it to an invalid value by default, and update
> >> it in realize function only if the value is still invalid.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> > [...]
> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> >> index 1b9540e..f0d2074 100644
> >> --- a/target-arm/cpu.c
> >> +++ b/target-arm/cpu.c
> >> @@ -33,6 +33,8 @@
> >>  #include "sysemu/kvm.h"
> >>  #include "kvm_arm.h"
> >>  
> >> +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
> > 
> > I would have defined this next to ARM64_AFFINITY_MASK in
> > target-arm/cpu-qom.h
> 
> It was my first idea, but all macros in cpu-qom.h start by ARM_ or
> ARM64_, this one looks like a local macro (the name is too generic), so
> I put it in cpu.c.

Feel free to prefix it with ARM64_ :-) How about
ARM64_AFFINITY_INVALID

> 
> >> +
> >>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
> >>  {
> >>      ARMCPU *cpu = ARM_CPU(cs);
> >> @@ -441,22 +443,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 +567,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 +630,17 @@ 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.
> >> +     */
> >> +    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
> >> +        Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> >> +        Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > 
> > I think ARM_CPUS_PER_CLUSTER should be renamed to
> > ARM_DEFAULT_CPUS_PER_CLUSTER and either moved from where
> > it's currently defined (above arm_cpu_initfn) to just above
> > arm_cpu_realizefn, or to the same place as ARM64_AFFINITY_MASK
> > and MP_AFFINITY_INVALID.
> > 
> > Aff0 and Aff1 could be declared in this scope, as they're only
> > used here.
> 
> The goal was to have as less diff as possible with my previous patch, so
> I didn't move the declaration. This will not change the generated code.

I don't care much, but it's not just about generated code. It's also
about reviewers being able to dismiss those variables when they get
to the end of the scope (avoids too much brain clutter)

> > 
> >> +        cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> >> +    }
> >> +
> >>      if (cpu->reset_hivecs) {
> >>              cpu->reset_sctlr |= (1 << 13);
> >>      }
> >> @@ -1461,7 +1471,7 @@ static Property arm_cpu_properties[] = {
> >>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
> >>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
> >>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
> >> -    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
> >> +    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, MP_AFFINITY_INVALID),
> >>      DEFINE_PROP_END_OF_LIST()
> >>  };
> >>  
> >> @@ -1533,17 +1543,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)
> > [...]
> > 
> > Otherwise looks good to me.
> 
> Thanks,
> Laurent
> 

Re: [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions

[Laurent Vivier]

On 19/10/2016 15:46, Andrew Jones wrote:
> On Wed, Oct 19, 2016 at 10:30:52AM +0200, Laurent Vivier wrote:
>>
>>
>> On 19/10/2016 10:13, Andrew Jones wrote:
>>> On Tue, Oct 18, 2016 at 09:22:52PM +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(). To avoid to overwrite an user defined
>>>> value, we set it to an invalid value by default, and update
>>>> it in realize function only if the value is still invalid.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>> [...]
>>>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>>>> index 1b9540e..f0d2074 100644
>>>> --- a/target-arm/cpu.c
>>>> +++ b/target-arm/cpu.c
>>>> @@ -33,6 +33,8 @@
>>>>  #include "sysemu/kvm.h"
>>>>  #include "kvm_arm.h"
>>>>  
>>>> +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
>>>
>>> I would have defined this next to ARM64_AFFINITY_MASK in
>>> target-arm/cpu-qom.h
>>
>> It was my first idea, but all macros in cpu-qom.h start by ARM_ or
>> ARM64_, this one looks like a local macro (the name is too generic), so
>> I put it in cpu.c.
> 
> Feel free to prefix it with ARM64_ :-) How about
> ARM64_AFFINITY_INVALID
> 
>>
>>>> +
>>>>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>>>>  {
>>>>      ARMCPU *cpu = ARM_CPU(cs);
>>>> @@ -441,22 +443,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 +567,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 +630,17 @@ 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.
>>>> +     */
>>>> +    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
>>>> +        Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
>>>> +        Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
>>>
>>> I think ARM_CPUS_PER_CLUSTER should be renamed to
>>> ARM_DEFAULT_CPUS_PER_CLUSTER and either moved from where
>>> it's currently defined (above arm_cpu_initfn) to just above
>>> arm_cpu_realizefn, or to the same place as ARM64_AFFINITY_MASK
>>> and MP_AFFINITY_INVALID.
>>>
>>> Aff0 and Aff1 could be declared in this scope, as they're only
>>> used here.
>>
>> The goal was to have as less diff as possible with my previous patch, so
>> I didn't move the declaration. This will not change the generated code.
> 
> I don't care much, but it's not just about generated code. It's also
> about reviewers being able to dismiss those variables when they get
> to the end of the scope (avoids too much brain clutter)

OK, I will modify the patch as you say.

Thanks,
Laurent

[Qemu-devel] [PATCH v4 3/3] exec: call cpu_exec_exit() from a CPU unrealize common function

[Laurent Vivier]

As cpu_exec_exit() mirrors the cpu_exec_realizefn(),
rename it as cpu_exec_unrealizefn().

Create and register a cpu_common_unrealizefn() function for
the CPU device class and call cpu_exec_unrealizefn() from
this function.

Remove cpu_exec_exit() from cpu_common_finalize()
(which mirrors init, not realize), and as x86_cpu_unrealizefn()
and ppc_cpu_unrealizefn() overwrite the device class unrealize function,
add a call to a parent_unrealize pointer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 exec.c                      | 2 +-
 include/qom/cpu.h           | 2 +-
 qom/cpu.c                   | 8 +++++++-
 target-i386/cpu-qom.h       | 1 +
 target-i386/cpu.c           | 9 +++++++++
 target-ppc/cpu-qom.h        | 1 +
 target-ppc/translate_init.c | 9 ++++++++-
 7 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 203eb52..3cd25db 100644
--- a/exec.c
+++ b/exec.c
@@ -596,7 +596,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 }
 #endif
 
-void cpu_exec_exit(CPUState *cpu)
+void cpu_exec_unrealizefn(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 5520c6c..633c3fc 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -948,7 +948,7 @@ 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);
+void cpu_exec_unrealizefn(CPUState *cpu);
 
 #ifdef CONFIG_SOFTMMU
 extern const struct VMStateDescription vmstate_cpu_common;
diff --git a/qom/cpu.c b/qom/cpu.c
index 85f1132..03d9190 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -345,6 +345,12 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
     trace_init_vcpu(cpu);
 }
 
+static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
+{
+    CPUState *cpu = CPU(dev);
+    cpu_exec_unrealizefn(cpu);
+}
+
 static void cpu_common_initfn(Object *obj)
 {
     CPUState *cpu = CPU(obj);
@@ -369,7 +375,6 @@ static void cpu_common_initfn(Object *obj)
 static void cpu_common_finalize(Object *obj)
 {
     CPUState *cpu = CPU(obj);
-    cpu_exec_exit(cpu);
     g_free(cpu->trace_dstate);
 }
 
@@ -403,6 +408,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
     dc->realize = cpu_common_realizefn;
+    dc->unrealize = cpu_common_unrealizefn;
     /*
      * Reason: CPUs still need special care by board code: wiring up
      * IRQs, adding reset handlers, halting non-first CPUs, ...
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 5dde658..07030d2 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -64,6 +64,7 @@ typedef struct X86CPUClass {
     bool kvm_required;
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3476d46..95ce067 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3243,6 +3243,8 @@ out:
 static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
 {
     X86CPU *cpu = X86_CPU(dev);
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
+    Error *local_err = NULL;
 
 #ifndef CONFIG_USER_ONLY
     cpu_remove_sync(CPU(dev));
@@ -3253,6 +3255,12 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
         object_unparent(OBJECT(cpu->apic_state));
         cpu->apic_state = NULL;
     }
+
+    xcc->parent_unrealize(dev, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 typedef struct BitProperty {
@@ -3502,6 +3510,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     xcc->parent_realize = dc->realize;
+    xcc->parent_unrealize = dc->unrealize;
     dc->realize = x86_cpu_realizefn;
     dc->unrealize = x86_cpu_unrealizefn;
     dc->props = x86_cpu_properties;
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 713deef..d46c31a 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -174,6 +174,7 @@ typedef struct PowerPCCPUClass {
     /*< public >*/
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     void (*parent_reset)(CPUState *cpu);
 
     uint32_t pvr;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 40dae70..208fa1e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9906,11 +9906,17 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
 static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(dev);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
+    Error *local_err = NULL;
     opc_handler_t **table, **table_2;
     int i, j, k;
 
-    cpu_exec_exit(CPU(dev));
+    pcc->parent_unrealize(dev, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
         if (env->opcodes[i] == &invalid_handler) {
@@ -10521,6 +10527,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     pcc->parent_realize = dc->realize;
+    pcc->parent_unrealize = dc->unrealize;
     pcc->pvr_match = ppc_pvr_match_default;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
     dc->realize = ppc_cpu_realizefn;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 0/3] Split cpu_exec_init() into an init and a realize part

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 09:22:50PM +0200, Laurent Vivier wrote:
> Since commit 42ecaba ("target-i386: Call cpu_exec_init() on realize"),
> , commit 6dd0f83 ("target-ppc: Move cpu_exec_init() call to realize function"),
> and commit c6644fc ("s390x/cpu: Get rid of side effects when creating a vcpu"),
> cpu_exec_init() has been moved to realize function for some architectures
> to implement CPU htoplug. This allows any failures from cpu_exec_init() to be
> handled appropriately.
> 
> This series tries to do the same work for all the other CPUs.
> 
> But as the ARM Virtual Machine ("virt") needs the "memory" property of the CPU
> in the machine init function (the "memory" property is created in
> cpu_exec_init() we want to move to the realize part), split cpu_exec_init() in
> two parts: a realize part (cpu_exec_realizefn(), adding the CPU in the
> environment) and an init part (cpu_exec_initfn(), initializing the CPU, like
> adding the "memory" property). To mirror the realize part, add an unrealize
> part, and remove the cpu_exec_exit() call from the finalize part.
> 
> This also allows to remove all the "cannot_destroy_with_object_finalize_yet"
> properties from the CPU device class.
> 
> v4:
> - arm: update mp_affinity in realize only if it s not set by the user
> - i386 and ppc: add an parent_unrealize to call the Device unrealize
>   because it is overwritten by the CPU unrealize
> - update commit messages

Series:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But I will wait for Drew or other ARM people to review the ARM
hunks on patch 2/3 before applying.

v3 -> v4 diff, for reference:

  diff --git a/target-arm/cpu.c b/target-arm/cpu.c
  index 364a45d..f0d2074 100644
  --- a/target-arm/cpu.c
  +++ b/target-arm/cpu.c
  @@ -33,6 +33,8 @@
   #include "sysemu/kvm.h"
   #include "kvm_arm.h"
   
  +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
  +
   static void arm_cpu_set_pc(CPUState *cs, vaddr value)
   {
       ARMCPU *cpu = ARM_CPU(cs);
  @@ -633,9 +635,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
        * 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->mp_affinity == MP_AFFINITY_INVALID) {
  +        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);
  @@ -1467,7 +1471,7 @@ static Property arm_cpu_properties[] = {
       DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
       DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
       DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
  -    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
  +    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, MP_AFFINITY_INVALID),
       DEFINE_PROP_END_OF_LIST()
   };
   
  diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
  index 5dde658..07030d2 100644
  --- a/target-i386/cpu-qom.h
  +++ b/target-i386/cpu-qom.h
  @@ -64,6 +64,7 @@ typedef struct X86CPUClass {
       bool kvm_required;
   
       DeviceRealize parent_realize;
  +    DeviceUnrealize parent_unrealize;
       void (*parent_reset)(CPUState *cpu);
   } X86CPUClass;
   
  diff --git a/target-i386/cpu.c b/target-i386/cpu.c
  index 399a3e4..95ce067 100644
  --- a/target-i386/cpu.c
  +++ b/target-i386/cpu.c
  @@ -3243,6 +3243,8 @@ out:
   static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
   {
       X86CPU *cpu = X86_CPU(dev);
  +    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
  +    Error *local_err = NULL;
   
   #ifndef CONFIG_USER_ONLY
       cpu_remove_sync(CPU(dev));
  @@ -3254,7 +3256,11 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
           cpu->apic_state = NULL;
       }
   
  -    cpu_exec_unrealizefn(CPU(dev));
  +    xcc->parent_unrealize(dev, &local_err);
  +    if (local_err != NULL) {
  +        error_propagate(errp, local_err);
  +        return;
  +    }
   }
   
   typedef struct BitProperty {
  @@ -3504,6 +3510,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
       DeviceClass *dc = DEVICE_CLASS(oc);
   
       xcc->parent_realize = dc->realize;
  +    xcc->parent_unrealize = dc->unrealize;
       dc->realize = x86_cpu_realizefn;
       dc->unrealize = x86_cpu_unrealizefn;
       dc->props = x86_cpu_properties;
  diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
  index 713deef..d46c31a 100644
  --- a/target-ppc/cpu-qom.h
  +++ b/target-ppc/cpu-qom.h
  @@ -174,6 +174,7 @@ typedef struct PowerPCCPUClass {
       /*< public >*/
   
       DeviceRealize parent_realize;
  +    DeviceUnrealize parent_unrealize;
       void (*parent_reset)(CPUState *cpu);
   
       uint32_t pvr;
  diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
  index 2de6a06..208fa1e 100644
  --- a/target-ppc/translate_init.c
  +++ b/target-ppc/translate_init.c
  @@ -9906,11 +9906,17 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
   static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
   {
       PowerPCCPU *cpu = POWERPC_CPU(dev);
  +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
       CPUPPCState *env = &cpu->env;
  +    Error *local_err = NULL;
       opc_handler_t **table, **table_2;
       int i, j, k;
   
  -    cpu_exec_unrealizefn(CPU(dev));
  +    pcc->parent_unrealize(dev, &local_err);
  +    if (local_err != NULL) {
  +        error_propagate(errp, local_err);
  +        return;
  +    }
   
       for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
           if (env->opcodes[i] == &invalid_handler) {
  @@ -10521,6 +10527,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
       DeviceClass *dc = DEVICE_CLASS(oc);
   
       pcc->parent_realize = dc->realize;
  +    pcc->parent_unrealize = dc->unrealize;
       pcc->pvr_match = ppc_pvr_match_default;
       pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
       dc->realize = ppc_cpu_realizefn;

> 
> v3:
> - reorganize the series in 3 patches:
>   1- split cpu_exec_init() and call the init part from
>      cpu_common_initfn()
>   2- move all the cpu_exec_realize() calls to the
>      XXX_cpu_realizefn() functions
>   3- create a cpu_common_unrealizefn() function
>       and call the cpu_exec_unrealize() function from here,
>       except for ppc64 and i386 that have their own XX_cpu_unrealizefn()
>       functions
> - rename cpu_exec_init(), cpu_exec_realize(), cpu_exec_unrealize()
>   to cpu_exec_initfn(), cpu_exec_realizefn() and cpu_exec_unrealizefn(),
> - move cpu_exec_unrealizefn() to the device class unrealize
>   and declare it in qom/cpu.h instead of exec/exec-all.h
> 
> v2:
> - rename cpu_exec_exit() as cpu_exec_unrealize(),
>   as it un-does what cpu_exec_realize() does,
> - remove cpu_exec_exit() from ppc_cpu_unrealizefn() as
>   it is called from cpu_common_finalize(),
> - add a patch to move all cpu_exec_init() from
>   all XX_cpu_initfn() to cpu_common_initfn(),
> - arm: move setting of cpu->mp_affinity to
>   arm_cpu_realizefn() as the cpu_index is now set in
>   cpu_exec_realizefn().
> - update some commit messages
> 
> Laurent Vivier (3):
>   exec: split cpu_exec_init()
>   exec: move cpu_exec_init() calls to realize functions
>   exec: call cpu_exec_exit() from a CPU unrealize common function
> 
>  exec.c                      | 12 +++++++-----
>  include/exec/exec-all.h     |  1 -
>  include/qom/cpu.h           |  4 +++-
>  qom/cpu.c                   | 10 +++++++++-
>  target-alpha/cpu.c          | 15 +++++++--------
>  target-arm/cpu.c            | 45 ++++++++++++++++++++++-----------------------
>  target-cris/cpu.c           | 15 +++++++--------
>  target-i386/cpu-qom.h       |  1 +
>  target-i386/cpu.c           | 20 ++++++++++++++------
>  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/cpu-qom.h        |  1 +
>  target-ppc/translate_init.c | 11 +++++++++--
>  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 +++++++--------
>  24 files changed, 169 insertions(+), 159 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
Eduardo