[Qemu-devel] [RFC v1 PATCH 0/3] cpus: Convert cpu_index into a bitmap

[Qemu-devel] [RFC v1 PATCH 0/3] cpus: Convert cpu_index into a bitmap

[Bharata B Rao]

This patch changes the way cpu_index is handed out to newly created
CPUs by tracking the allocted CPUs in a bitmap. More information and
the need for this patch is described in patch 2/3 of this series. These
generic changes are needed to support CPU hot plug/unplug on PowerPC.

cpu_index is allocated in cpu_exec_init() and freed (during CPU unplug) in
the newly added API cpu_exec_exit(). Currently all architectures call
cpu_exec_init() from instance_init and hence cpu_exec_exit() is called
from instance_finalize for all archs in this series.

Instead of adding instance_finalize to all archs, we could call
cpu_exec_exit() from the parent class (CPUClass.instance_finalize).
However archs like x86 and PowerPC want cpu_exec_init() to be moved to
realizefn from instance_init. Such movement will become easy with the
current approach. Eudardo's suggested alternative of making cpu_exec_exit()
idempotent

(http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg01241.html)

will also simplify this movement, but I think that would compilicate the
deallocation logic.

Another open question is about handling of holes correctly in the allocated
bitmap to support VM migration after CPU unplug. This was briefly discussed
here:

https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00560.html

Should cpu_exec_init() API support specifying of a particular cpu_index
in addition to returning the next available cpu_index by default ? I know
that QEMU cmdline semantics for CPU device add/delele haven't been defined
yet, but should we now make provision in cpu_exec_init() to allocate the
required cpu_index ?

Changes in v1
-------------
- Added Error argument to cpu_exec_init() so that it callers calling
  it from realizefn can collect errors.

v0: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg02950.html

Bharata B Rao (3):
  cpus: Add Error argument to cpu_exec_init()
  cpus: Convert cpu_index into a bitmap
  ppc: Move cpu_exec_init() call to realize function

 exec.c                      | 39 +++++++++++++++++++++++++++++++++++----
 include/exec/exec-all.h     |  2 +-
 include/qom/cpu.h           |  8 ++++++++
 target-alpha/cpu.c          |  8 +++++++-
 target-arm/cpu.c            |  3 ++-
 target-cris/cpu.c           |  8 +++++++-
 target-i386/cpu.c           |  8 +++++++-
 target-lm32/cpu.c           |  8 +++++++-
 target-m68k/cpu.c           |  8 +++++++-
 target-microblaze/cpu.c     |  8 +++++++-
 target-mips/cpu.c           |  8 +++++++-
 target-moxie/cpu.c          |  8 +++++++-
 target-openrisc/cpu.c       |  8 +++++++-
 target-ppc/translate_init.c | 15 +++++++++++++--
 target-s390x/cpu.c          |  3 ++-
 target-sh4/cpu.c            |  8 +++++++-
 target-sparc/cpu.c          |  3 ++-
 target-tricore/cpu.c        |  7 ++++++-
 target-unicore32/cpu.c      |  8 +++++++-
 target-xtensa/cpu.c         |  8 +++++++-
 20 files changed, 153 insertions(+), 23 deletions(-)

-- 
2.1.0

[Qemu-devel] [RFC v1 PATCH 1/3] cpus: Add Error argument to cpu_exec_init()

[Bharata B Rao]

Add an Error argument to cpu_exec_init() to let users collect the
error. Change all callers to currently pass NULL error argument. This change
is needed for the following reasons:

- A subsequent commit changes the CPU enumeration logic in cpu_exec_init()
  resulting in cpu_exec_init() to fail if cpu_index values corresponding
  to max_cpus have already been handed out.
- Archs like PowerPC and x86 want cpu_exec_init() to be called from realize
  rather than instance_init. With this change, those architectures
  that can move this call into realize function can do so in a phased
  manner.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 exec.c                      | 2 +-
 include/exec/exec-all.h     | 2 +-
 target-alpha/cpu.c          | 2 +-
 target-arm/cpu.c            | 2 +-
 target-cris/cpu.c           | 2 +-
 target-i386/cpu.c           | 2 +-
 target-lm32/cpu.c           | 2 +-
 target-m68k/cpu.c           | 2 +-
 target-microblaze/cpu.c     | 2 +-
 target-mips/cpu.c           | 2 +-
 target-moxie/cpu.c          | 2 +-
 target-openrisc/cpu.c       | 2 +-
 target-ppc/translate_init.c | 2 +-
 target-s390x/cpu.c          | 2 +-
 target-sh4/cpu.c            | 2 +-
 target-sparc/cpu.c          | 2 +-
 target-tricore/cpu.c        | 2 +-
 target-unicore32/cpu.c      | 2 +-
 target-xtensa/cpu.c         | 2 +-
 19 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/exec.c b/exec.c
index ae37b98..68b632b 100644
--- a/exec.c
+++ b/exec.c
@@ -519,7 +519,7 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 }
 #endif
 
-void cpu_exec_init(CPUArchState *env)
+void cpu_exec_init(CPUArchState *env, Error **errp)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index b58cd47..0a3a504 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -88,7 +88,7 @@ void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base, int flags,
                               int cflags);
-void cpu_exec_init(CPUArchState *env);
+void cpu_exec_init(CPUArchState *env, Error **errp);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index a98b7d8..0a0c21e 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -257,7 +257,7 @@ static void alpha_cpu_initfn(Object *obj)
     CPUAlphaState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
     tlb_flush(cs, 1);
 
     alpha_translate_init();
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 3ca3fa8..0b27c19 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -369,7 +369,7 @@ static void arm_cpu_initfn(Object *obj)
     static bool inited;
 
     cs->env_ptr = &cpu->env;
-    cpu_exec_init(&cpu->env);
+    cpu_exec_init(&cpu->env, NULL);
     cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
                                          g_free, g_free);
 
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 16cfba9..8b589ec 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -170,7 +170,7 @@ static void cris_cpu_initfn(Object *obj)
     static bool tcg_initialized;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     env->pregs[PR_VR] = ccc->vr;
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3305e09..72e8fa9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2850,7 +2850,7 @@ static void x86_cpu_initfn(Object *obj)
     static int inited;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
index f8081f5..89b6631 100644
--- a/target-lm32/cpu.c
+++ b/target-lm32/cpu.c
@@ -151,7 +151,7 @@ static void lm32_cpu_initfn(Object *obj)
     static bool tcg_initialized;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     env->flags = 0;
 
diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
index 4cfb725..6a41551 100644
--- a/target-m68k/cpu.c
+++ b/target-m68k/cpu.c
@@ -168,7 +168,7 @@ static void m68k_cpu_initfn(Object *obj)
     static bool inited;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     if (tcg_enabled() && !inited) {
         inited = true;
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index 67e3182..6b3732d 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -130,7 +130,7 @@ static void mb_cpu_initfn(Object *obj)
     static bool tcg_initialized;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
 
diff --git a/target-mips/cpu.c b/target-mips/cpu.c
index 958c999..5eea836 100644
--- a/target-mips/cpu.c
+++ b/target-mips/cpu.c
@@ -115,7 +115,7 @@ static void mips_cpu_initfn(Object *obj)
     CPUMIPSState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     if (tcg_enabled()) {
         mips_tcg_init();
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 47b617f..f815fb3 100644
--- a/target-moxie/cpu.c
+++ b/target-moxie/cpu.c
@@ -66,7 +66,7 @@ static void moxie_cpu_initfn(Object *obj)
     static int inited;
 
     cs->env_ptr = &cpu->env;
-    cpu_exec_init(&cpu->env);
+    cpu_exec_init(&cpu->env, NULL);
 
     if (tcg_enabled() && !inited) {
         inited = 1;
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index 39bedc1..87b2f80 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -92,7 +92,7 @@ static void openrisc_cpu_initfn(Object *obj)
     static int inited;
 
     cs->env_ptr = &cpu->env;
-    cpu_exec_init(&cpu->env);
+    cpu_exec_init(&cpu->env, NULL);
 
 #ifndef CONFIG_USER_ONLY
     cpu_openrisc_mmu_init(cpu);
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index d74f4f0..4665cf9 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9633,7 +9633,7 @@ static void ppc_cpu_initfn(Object *obj)
     CPUPPCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
     cpu->cpu_dt_id = cs->cpu_index;
 
     env->msr_mask = pcc->msr_mask;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index e0537fa..22499bb 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -187,7 +187,7 @@ static void s390_cpu_initfn(Object *obj)
 #endif
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
     qemu_get_timedate(&tm, 0);
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index d187a2b..ffb635e 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -247,7 +247,7 @@ static void superh_cpu_initfn(Object *obj)
     CPUSH4State *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     env->movcal_backup_tail = &(env->movcal_backup);
 
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index a952097..d857aae 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -802,7 +802,7 @@ static void sparc_cpu_initfn(Object *obj)
     CPUSPARCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     if (tcg_enabled()) {
         gen_intermediate_code_init(env);
diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
index 2ba0cf4..53b117b 100644
--- a/target-tricore/cpu.c
+++ b/target-tricore/cpu.c
@@ -88,7 +88,7 @@ static void tricore_cpu_initfn(Object *obj)
     CPUTriCoreState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     if (tcg_enabled()) {
         tricore_tcg_init();
diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
index 5b32987..d56d78a 100644
--- a/target-unicore32/cpu.c
+++ b/target-unicore32/cpu.c
@@ -111,7 +111,7 @@ static void uc32_cpu_initfn(Object *obj)
     static bool inited;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
 #ifdef CONFIG_USER_ONLY
     env->uncached_asr = ASR_MODE_USER;
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index 2b75678..87178e6 100644
--- a/target-xtensa/cpu.c
+++ b/target-xtensa/cpu.c
@@ -114,7 +114,7 @@ static void xtensa_cpu_initfn(Object *obj)
 
     cs->env_ptr = env;
     env->config = xcc->config;
-    cpu_exec_init(env);
+    cpu_exec_init(env, NULL);
 
     if (tcg_enabled() && !tcg_inited) {
         tcg_inited = true;
-- 
2.1.0

[Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Bharata B Rao]

Currently CPUState.cpu_index is monotonically increasing and a newly
created CPU always gets the next higher index. The next available
index is calculated by counting the existing number of CPUs. This is
fine as long as we only add CPUs, but there are architectures which
are starting to support CPU removal too. For an architecture like PowerPC
which derives its CPU identifier (device tree ID) from cpu_index, the
existing logic of generating cpu_index values causes problems.

With the currently proposed method of handling vCPU removal by parking
the vCPU fd in QEMU
(Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
generating cpu_index this way will not work for PowerPC.

This patch changes the way cpu_index is handed out by maintaining
a bit map of the CPUs that tracks both addition and removal of CPUs.

The CPU bitmap allocation logic is part of cpu_exec_init() which is
called by instance_init routines of various CPU targets. This patch
also adds corresponding instance_finalize routine if needed for these
CPU targets so that CPU can be marked free when it is removed.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c                      | 37 ++++++++++++++++++++++++++++++++++---
 include/qom/cpu.h           |  8 ++++++++
 target-alpha/cpu.c          |  6 ++++++
 target-arm/cpu.c            |  1 +
 target-cris/cpu.c           |  6 ++++++
 target-i386/cpu.c           |  6 ++++++
 target-lm32/cpu.c           |  6 ++++++
 target-m68k/cpu.c           |  6 ++++++
 target-microblaze/cpu.c     |  6 ++++++
 target-mips/cpu.c           |  6 ++++++
 target-moxie/cpu.c          |  6 ++++++
 target-openrisc/cpu.c       |  6 ++++++
 target-ppc/translate_init.c |  6 ++++++
 target-s390x/cpu.c          |  1 +
 target-sh4/cpu.c            |  6 ++++++
 target-sparc/cpu.c          |  1 +
 target-tricore/cpu.c        |  5 +++++
 target-unicore32/cpu.c      |  6 ++++++
 target-xtensa/cpu.c         |  6 ++++++
 19 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/exec.c b/exec.c
index 68b632b..2422948 100644
--- a/exec.c
+++ b/exec.c
@@ -519,21 +519,52 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 }
 #endif
 
+#ifndef CONFIG_USER_ONLY
+static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
+
+static int cpu_get_free_index(Error **errp)
+{
+    int cpu = find_first_zero_bit(cpu_index_map, max_cpus);
+
+    if (cpu == max_cpus) {
+        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
+                    max_cpus);
+        return max_cpus;
+    } else {
+        bitmap_set(cpu_index_map, cpu, 1);
+        return cpu;
+    }
+}
+
+void cpu_exec_exit(CPUState *cpu)
+{
+    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
+}
+#endif
+
 void cpu_exec_init(CPUArchState *env, Error **errp)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     CPUClass *cc = CPU_GET_CLASS(cpu);
-    CPUState *some_cpu;
     int cpu_index;
-
 #if defined(CONFIG_USER_ONLY)
+    CPUState *some_cpu;
+
     cpu_list_lock();
-#endif
     cpu_index = 0;
     CPU_FOREACH(some_cpu) {
         cpu_index++;
     }
     cpu->cpu_index = cpu_index;
+#else
+    Error *local_err = NULL;
+
+    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+#endif
     cpu->numa_node = 0;
     QTAILQ_INIT(&cpu->breakpoints);
     QTAILQ_INIT(&cpu->watchpoints);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..3611ce5 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -673,6 +673,14 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
 void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
+#ifndef CONFIG_USER_ONLY
+void cpu_exec_exit(CPUState *cpu);
+#else
+static inline void cpu_exec_exit(CPUState *cpu)
+{
+}
+#endif
+
 #ifdef CONFIG_SOFTMMU
 extern const struct VMStateDescription vmstate_cpu_common;
 #else
diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index 0a0c21e..259a04c 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -250,6 +250,11 @@ static const TypeInfo ev68_cpu_type_info = {
     .parent = TYPE("ev67"),
 };
 
+static void alpha_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void alpha_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -305,6 +310,7 @@ static const TypeInfo alpha_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(AlphaCPU),
     .instance_init = alpha_cpu_initfn,
+    .instance_finalize = alpha_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(AlphaCPUClass),
     .class_init = alpha_cpu_class_init,
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 0b27c19..ef04ae1 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -454,6 +454,7 @@ static void arm_cpu_finalizefn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
     g_hash_table_destroy(cpu->cp_regs);
+    cpu_exec_exit(CPU(obj));
 }
 
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 8b589ec..da39223 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -161,6 +161,11 @@ static void cris_cpu_set_irq(void *opaque, int irq, int level)
 }
 #endif
 
+static void cris_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void cris_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -299,6 +304,7 @@ static const TypeInfo cris_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(CRISCPU),
     .instance_init = cris_cpu_initfn,
+    .instance_finalize = cris_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(CRISCPUClass),
     .class_init = cris_cpu_class_init,
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 72e8fa9..7305cdc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2841,6 +2841,11 @@ out:
     }
 }
 
+static void x86_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -3010,6 +3015,7 @@ static const TypeInfo x86_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(X86CPU),
     .instance_init = x86_cpu_initfn,
+    .instance_finalize = x86_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
index 89b6631..d7bc973 100644
--- a/target-lm32/cpu.c
+++ b/target-lm32/cpu.c
@@ -143,6 +143,11 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
     lcc->parent_realize(dev, errp);
 }
 
+static void lm32_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void lm32_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -294,6 +299,7 @@ static const TypeInfo lm32_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(LM32CPU),
     .instance_init = lm32_cpu_initfn,
+    .instance_finalize = lm32_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(LM32CPUClass),
     .class_init = lm32_cpu_class_init,
diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
index 6a41551..c2fce97 100644
--- a/target-m68k/cpu.c
+++ b/target-m68k/cpu.c
@@ -160,6 +160,11 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
     mcc->parent_realize(dev, errp);
 }
 
+static void m68k_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void m68k_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -231,6 +236,7 @@ static const TypeInfo m68k_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(M68kCPU),
     .instance_init = m68k_cpu_initfn,
+    .instance_finalize = m68k_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(M68kCPUClass),
     .class_init = m68k_cpu_class_init,
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index 6b3732d..3aa3796 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -122,6 +122,11 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
     mcc->parent_realize(dev, errp);
 }
 
+static void mb_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void mb_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -190,6 +195,7 @@ static const TypeInfo mb_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(MicroBlazeCPU),
     .instance_init = mb_cpu_initfn,
+    .instance_finalize = mb_cpu_finalize,
     .class_size = sizeof(MicroBlazeCPUClass),
     .class_init = mb_cpu_class_init,
 };
diff --git a/target-mips/cpu.c b/target-mips/cpu.c
index 5eea836..0cadfff 100644
--- a/target-mips/cpu.c
+++ b/target-mips/cpu.c
@@ -108,6 +108,11 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     mcc->parent_realize(dev, errp);
 }
 
+static void mips_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void mips_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -160,6 +165,7 @@ static const TypeInfo mips_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(MIPSCPU),
     .instance_init = mips_cpu_initfn,
+    .instance_finalize = mips_cpu_finalize,
     .abstract = false,
     .class_size = sizeof(MIPSCPUClass),
     .class_init = mips_cpu_class_init,
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index f815fb3..25d5f30 100644
--- a/target-moxie/cpu.c
+++ b/target-moxie/cpu.c
@@ -59,6 +59,11 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
     mcc->parent_realize(dev, errp);
 }
 
+static void moxie_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void moxie_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -160,6 +165,7 @@ static const TypeInfo moxie_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(MoxieCPU),
     .instance_init = moxie_cpu_initfn,
+    .instance_finalize = moxie_cpu_finalize,
     .class_size = sizeof(MoxieCPUClass),
     .class_init = moxie_cpu_class_init,
 };
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index 87b2f80..f0c990f 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -85,6 +85,11 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
     occ->parent_realize(dev, errp);
 }
 
+static void openrisc_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void openrisc_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -198,6 +203,7 @@ static const TypeInfo openrisc_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(OpenRISCCPU),
     .instance_init = openrisc_cpu_initfn,
+    .instance_finalize = openrisc_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(OpenRISCCPUClass),
     .class_init = openrisc_cpu_class_init,
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 4665cf9..b915697 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9625,6 +9625,11 @@ static bool ppc_cpu_is_big_endian(CPUState *cs)
 }
 #endif
 
+static void ppc_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void ppc_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -9738,6 +9743,7 @@ static const TypeInfo ppc_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(PowerPCCPU),
     .instance_init = ppc_cpu_initfn,
+    .instance_finalize = ppc_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(PowerPCCPUClass),
     .class_init = ppc_cpu_class_init,
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 22499bb..1370d4a 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -214,6 +214,7 @@ static void s390_cpu_finalize(Object *obj)
 
     qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
+    cpu_exec_exit(CPU(obj));
 }
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index ffb635e..65f44c7 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -240,6 +240,11 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
     scc->parent_realize(dev, errp);
 }
 
+static void superh_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void superh_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -296,6 +301,7 @@ static const TypeInfo superh_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(SuperHCPU),
     .instance_init = superh_cpu_initfn,
+    .instance_finalize = superh_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(SuperHCPUClass),
     .class_init = superh_cpu_class_init,
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index d857aae..ac7091a 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -815,6 +815,7 @@ static void sparc_cpu_uninitfn(Object *obj)
     CPUSPARCState *env = &cpu->env;
 
     g_free(env->def);
+    cpu_exec_exit(CPU(obj));
 }
 
 static void sparc_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
index 53b117b..e871dc4 100644
--- a/target-tricore/cpu.c
+++ b/target-tricore/cpu.c
@@ -80,6 +80,10 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
     tcc->parent_realize(dev, errp);
 }
 
+static void tricore_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
 
 static void tricore_cpu_initfn(Object *obj)
 {
@@ -180,6 +184,7 @@ static const TypeInfo tricore_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(TriCoreCPU),
     .instance_init = tricore_cpu_initfn,
+    .instance_finalize = tricore_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(TriCoreCPUClass),
     .class_init = tricore_cpu_class_init,
diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
index d56d78a..64af9f8 100644
--- a/target-unicore32/cpu.c
+++ b/target-unicore32/cpu.c
@@ -103,6 +103,11 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
     ucc->parent_realize(dev, errp);
 }
 
+static void uc32_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void uc32_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -174,6 +179,7 @@ static const TypeInfo uc32_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(UniCore32CPU),
     .instance_init = uc32_cpu_initfn,
+    .instance_finalize = uc32_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(UniCore32CPUClass),
     .class_init = uc32_cpu_class_init,
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index 87178e6..e9d2a1e 100644
--- a/target-xtensa/cpu.c
+++ b/target-xtensa/cpu.c
@@ -104,6 +104,11 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
     xcc->parent_realize(dev, errp);
 }
 
+static void xtensa_cpu_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static void xtensa_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -162,6 +167,7 @@ static const TypeInfo xtensa_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(XtensaCPU),
     .instance_init = xtensa_cpu_initfn,
+    .instance_finalize = xtensa_cpu_finalize,
     .abstract = true,
     .class_size = sizeof(XtensaCPUClass),
     .class_init = xtensa_cpu_class_init,
-- 
2.1.0

[Qemu-devel] [RFC v1 PATCH 3/3] ppc: Move cpu_exec_init() call to realize function

[Bharata B Rao]

Move cpu_exec_init() call from instance_init to realize. This allows
any failures from cpu_exec_init() to be handled appropriately.
Also add corresponding cpu_exec_exit() call from unrealize.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/translate_init.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index b915697..47fe3d6 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8928,6 +8928,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    cpu_exec_init(&cpu->env, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
     cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
         + (cs->cpu_index % smp_threads);
 #endif
@@ -9141,6 +9146,8 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
     opc_handler_t **table;
     int i, j;
 
+    cpu_exec_exit(CPU(dev));
+
     for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
         if (env->opcodes[i] == &invalid_handler) {
             continue;
@@ -9638,8 +9645,6 @@ static void ppc_cpu_initfn(Object *obj)
     CPUPPCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env, NULL);
-    cpu->cpu_dt_id = cs->cpu_index;
 
     env->msr_mask = pcc->msr_mask;
     env->mmu_model = pcc->mmu_model;
-- 
2.1.0

Re: [Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Eduardo Habkost]

On Fri, May 08, 2015 at 03:21:35PM +0530, Bharata B Rao wrote:
> Currently CPUState.cpu_index is monotonically increasing and a newly
> created CPU always gets the next higher index. The next available
> index is calculated by counting the existing number of CPUs. This is
> fine as long as we only add CPUs, but there are architectures which
> are starting to support CPU removal too. For an architecture like PowerPC
> which derives its CPU identifier (device tree ID) from cpu_index, the
> existing logic of generating cpu_index values causes problems.
> 
> With the currently proposed method of handling vCPU removal by parking
> the vCPU fd in QEMU
> (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
> generating cpu_index this way will not work for PowerPC.
> 
> This patch changes the way cpu_index is handed out by maintaining
> a bit map of the CPUs that tracks both addition and removal of CPUs.
> 
> The CPU bitmap allocation logic is part of cpu_exec_init() which is
> called by instance_init routines of various CPU targets. This patch
> also adds corresponding instance_finalize routine if needed for these
> CPU targets so that CPU can be marked free when it is removed.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c                      | 37 ++++++++++++++++++++++++++++++++++---
>  include/qom/cpu.h           |  8 ++++++++
>  target-alpha/cpu.c          |  6 ++++++
>  target-arm/cpu.c            |  1 +
>  target-cris/cpu.c           |  6 ++++++
>  target-i386/cpu.c           |  6 ++++++
>  target-lm32/cpu.c           |  6 ++++++
>  target-m68k/cpu.c           |  6 ++++++
>  target-microblaze/cpu.c     |  6 ++++++
>  target-mips/cpu.c           |  6 ++++++
>  target-moxie/cpu.c          |  6 ++++++
>  target-openrisc/cpu.c       |  6 ++++++
>  target-ppc/translate_init.c |  6 ++++++
>  target-s390x/cpu.c          |  1 +
>  target-sh4/cpu.c            |  6 ++++++
>  target-sparc/cpu.c          |  1 +
>  target-tricore/cpu.c        |  5 +++++
>  target-unicore32/cpu.c      |  6 ++++++
>  target-xtensa/cpu.c         |  6 ++++++
>  19 files changed, 128 insertions(+), 3 deletions(-)

Why not simply call cpu_exec_exit() on generic CPU::instance_finalize,
to avoid forcing every architecture to call it manually? Calling
cpu_exec_exit() twice would be harmless, anyway.

(It would just need an additional check to make sure the bit will be
cleared only if cpu_exec_init() was really called and cpu_index was
properly set.)

-- 
Eduardo

Re: [Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Eduardo Habkost]

On Fri, May 08, 2015 at 03:21:35PM +0530, Bharata B Rao wrote:
[...]
>  void cpu_exec_init(CPUArchState *env, Error **errp)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -    CPUState *some_cpu;
>      int cpu_index;
> -
>  #if defined(CONFIG_USER_ONLY)
> +    CPUState *some_cpu;
> +
>      cpu_list_lock();
> -#endif
>      cpu_index = 0;
>      CPU_FOREACH(some_cpu) {
>          cpu_index++;
>      }
>      cpu->cpu_index = cpu_index;

Why not use the bitmap on CONFIG_USER too?

> +#else
> +    Error *local_err = NULL;
> +
> +    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +#endif
>      cpu->numa_node = 0;
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
[...]

-- 
Eduardo

Re: [Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Bharata B Rao]

On Fri, May 08, 2015 at 11:55:00AM -0300, Eduardo Habkost wrote:
> On Fri, May 08, 2015 at 03:21:35PM +0530, Bharata B Rao wrote:
> > Currently CPUState.cpu_index is monotonically increasing and a newly
> > created CPU always gets the next higher index. The next available
> > index is calculated by counting the existing number of CPUs. This is
> > fine as long as we only add CPUs, but there are architectures which
> > are starting to support CPU removal too. For an architecture like PowerPC
> > which derives its CPU identifier (device tree ID) from cpu_index, the
> > existing logic of generating cpu_index values causes problems.
> > 
> > With the currently proposed method of handling vCPU removal by parking
> > the vCPU fd in QEMU
> > (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
> > generating cpu_index this way will not work for PowerPC.
> > 
> > This patch changes the way cpu_index is handed out by maintaining
> > a bit map of the CPUs that tracks both addition and removal of CPUs.
> > 
> > The CPU bitmap allocation logic is part of cpu_exec_init() which is
> > called by instance_init routines of various CPU targets. This patch
> > also adds corresponding instance_finalize routine if needed for these
> > CPU targets so that CPU can be marked free when it is removed.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c                      | 37 ++++++++++++++++++++++++++++++++++---
> >  include/qom/cpu.h           |  8 ++++++++
> >  target-alpha/cpu.c          |  6 ++++++
> >  target-arm/cpu.c            |  1 +
> >  target-cris/cpu.c           |  6 ++++++
> >  target-i386/cpu.c           |  6 ++++++
> >  target-lm32/cpu.c           |  6 ++++++
> >  target-m68k/cpu.c           |  6 ++++++
> >  target-microblaze/cpu.c     |  6 ++++++
> >  target-mips/cpu.c           |  6 ++++++
> >  target-moxie/cpu.c          |  6 ++++++
> >  target-openrisc/cpu.c       |  6 ++++++
> >  target-ppc/translate_init.c |  6 ++++++
> >  target-s390x/cpu.c          |  1 +
> >  target-sh4/cpu.c            |  6 ++++++
> >  target-sparc/cpu.c          |  1 +
> >  target-tricore/cpu.c        |  5 +++++
> >  target-unicore32/cpu.c      |  6 ++++++
> >  target-xtensa/cpu.c         |  6 ++++++
> >  19 files changed, 128 insertions(+), 3 deletions(-)
> 
> Why not simply call cpu_exec_exit() on generic CPU::instance_finalize,
> to avoid forcing every architecture to call it manually? Calling
> cpu_exec_exit() twice would be harmless, anyway.

Yes cpu_exec_exit() can be called from generic CPU::instance_finalize and
it does appear harmless calling it twice but,

Can there be a situation where cpu_index freed from the first cpu_exec_exit()
call from ->unrealize() be allocated (to a different caller) again before
the 2nd call for the same CPU from CPU::instance_finalize ? If yes,
cpu_exec_exit() needs to be more intelligent than what it is currently is.

> 
> (It would just need an additional check to make sure the bit will be
> cleared only if cpu_exec_init() was really called and cpu_index was
> properly set.)

If the situation I describe above can indeed happen, then cpu_exec_exit()
needs to maintain state to safely fail the double free for the same CPU
from the same caller. I think touching all archs and adding instance_finalize
would be much more simpler, cleaner and correct. When archs want to move
cpu_exec_init() and cpu_exec_exit() to realize/unlrealize, they can do
so.

Regards,
Bharata.

Re: [Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Bharata B Rao]

On Fri, May 08, 2015 at 11:57:40AM -0300, Eduardo Habkost wrote:
> On Fri, May 08, 2015 at 03:21:35PM +0530, Bharata B Rao wrote:
> [...]
> >  void cpu_exec_init(CPUArchState *env, Error **errp)
> >  {
> >      CPUState *cpu = ENV_GET_CPU(env);
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > -    CPUState *some_cpu;
> >      int cpu_index;
> > -
> >  #if defined(CONFIG_USER_ONLY)
> > +    CPUState *some_cpu;
> > +
> >      cpu_list_lock();
> > -#endif
> >      cpu_index = 0;
> >      CPU_FOREACH(some_cpu) {
> >          cpu_index++;
> >      }
> >      cpu->cpu_index = cpu_index;
> 
> Why not use the bitmap on CONFIG_USER too?

I was doing like that in v1 and it required me to cook up a max_cpus value
for CONFIG_USER case to define the bitmap.

Andreas pointed out that it is better not to touch the allocation
logic for CONFIG_USER.

https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03571.html

Regards,
Bharata.

Re: [Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Eduardo Habkost]

On Mon, May 11, 2015 at 09:03:57AM +0530, Bharata B Rao wrote:
> On Fri, May 08, 2015 at 11:55:00AM -0300, Eduardo Habkost wrote:
> > On Fri, May 08, 2015 at 03:21:35PM +0530, Bharata B Rao wrote:
> > > Currently CPUState.cpu_index is monotonically increasing and a newly
> > > created CPU always gets the next higher index. The next available
> > > index is calculated by counting the existing number of CPUs. This is
> > > fine as long as we only add CPUs, but there are architectures which
> > > are starting to support CPU removal too. For an architecture like PowerPC
> > > which derives its CPU identifier (device tree ID) from cpu_index, the
> > > existing logic of generating cpu_index values causes problems.
> > > 
> > > With the currently proposed method of handling vCPU removal by parking
> > > the vCPU fd in QEMU
> > > (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
> > > generating cpu_index this way will not work for PowerPC.
> > > 
> > > This patch changes the way cpu_index is handed out by maintaining
> > > a bit map of the CPUs that tracks both addition and removal of CPUs.
> > > 
> > > The CPU bitmap allocation logic is part of cpu_exec_init() which is
> > > called by instance_init routines of various CPU targets. This patch
> > > also adds corresponding instance_finalize routine if needed for these
> > > CPU targets so that CPU can be marked free when it is removed.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  exec.c                      | 37 ++++++++++++++++++++++++++++++++++---
> > >  include/qom/cpu.h           |  8 ++++++++
> > >  target-alpha/cpu.c          |  6 ++++++
> > >  target-arm/cpu.c            |  1 +
> > >  target-cris/cpu.c           |  6 ++++++
> > >  target-i386/cpu.c           |  6 ++++++
> > >  target-lm32/cpu.c           |  6 ++++++
> > >  target-m68k/cpu.c           |  6 ++++++
> > >  target-microblaze/cpu.c     |  6 ++++++
> > >  target-mips/cpu.c           |  6 ++++++
> > >  target-moxie/cpu.c          |  6 ++++++
> > >  target-openrisc/cpu.c       |  6 ++++++
> > >  target-ppc/translate_init.c |  6 ++++++
> > >  target-s390x/cpu.c          |  1 +
> > >  target-sh4/cpu.c            |  6 ++++++
> > >  target-sparc/cpu.c          |  1 +
> > >  target-tricore/cpu.c        |  5 +++++
> > >  target-unicore32/cpu.c      |  6 ++++++
> > >  target-xtensa/cpu.c         |  6 ++++++
> > >  19 files changed, 128 insertions(+), 3 deletions(-)
> > 
> > Why not simply call cpu_exec_exit() on generic CPU::instance_finalize,
> > to avoid forcing every architecture to call it manually? Calling
> > cpu_exec_exit() twice would be harmless, anyway.
> 
> Yes cpu_exec_exit() can be called from generic CPU::instance_finalize and
> it does appear harmless calling it twice but,
> 
> Can there be a situation where cpu_index freed from the first cpu_exec_exit()
> call from ->unrealize() be allocated (to a different caller) again before
> the 2nd call for the same CPU from CPU::instance_finalize ? If yes,
> cpu_exec_exit() needs to be more intelligent than what it is currently is.

Yes, it would need to be a bit more intelligent, but it could be done
very easily by using cpu_index=-1 to indicate that cpu_exec_init() was
never called, or that cpu_exec_exit() was already called.

> 
> > 
> > (It would just need an additional check to make sure the bit will be
> > cleared only if cpu_exec_init() was really called and cpu_index was
> > properly set.)
> 
> If the situation I describe above can indeed happen, then cpu_exec_exit()
> needs to maintain state to safely fail the double free for the same CPU
> from the same caller. I think touching all archs and adding instance_finalize
> would be much more simpler, cleaner and correct. When archs want to move
> cpu_exec_init() and cpu_exec_exit() to realize/unlrealize, they can do
> so.

Sorry, but I don't see how having to duplicate the same 6 lines of code
over 17 different files is simpler and cleaner, if you can do the same
with just 4 extra lines on exec.c.

-- 
Eduardo

Re: [Qemu-devel] [RFC v1 PATCH 2/3] cpus: Convert cpu_index into a bitmap

[Eduardo Habkost]

On Mon, May 11, 2015 at 09:07:35AM +0530, Bharata B Rao wrote:
> On Fri, May 08, 2015 at 11:57:40AM -0300, Eduardo Habkost wrote:
> > On Fri, May 08, 2015 at 03:21:35PM +0530, Bharata B Rao wrote:
> > [...]
> > >  void cpu_exec_init(CPUArchState *env, Error **errp)
> > >  {
> > >      CPUState *cpu = ENV_GET_CPU(env);
> > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > -    CPUState *some_cpu;
> > >      int cpu_index;
> > > -
> > >  #if defined(CONFIG_USER_ONLY)
> > > +    CPUState *some_cpu;
> > > +
> > >      cpu_list_lock();
> > > -#endif
> > >      cpu_index = 0;
> > >      CPU_FOREACH(some_cpu) {
> > >          cpu_index++;
> > >      }
> > >      cpu->cpu_index = cpu_index;
> > 
> > Why not use the bitmap on CONFIG_USER too?
> 
> I was doing like that in v1 and it required me to cook up a max_cpus value
> for CONFIG_USER case to define the bitmap.
> 
> Andreas pointed out that it is better not to touch the allocation
> logic for CONFIG_USER.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03571.html

If you want to keep the existing CONFIG_USER logic (even if
temporarily), can we at least provide a cpu_get_free_index() function
for CONFIG_USER, to make cpu_exec_init() cleaner?

-- 
Eduardo

Re: [Qemu-devel] [RFC v1 PATCH 1/3] cpus: Add Error argument to cpu_exec_init()

[Eduardo Habkost]

On Fri, May 08, 2015 at 03:21:34PM +0530, Bharata B Rao wrote:
> Add an Error argument to cpu_exec_init() to let users collect the
> error. Change all callers to currently pass NULL error argument. This change
> is needed for the following reasons:
> 
> - A subsequent commit changes the CPU enumeration logic in cpu_exec_init()
>   resulting in cpu_exec_init() to fail if cpu_index values corresponding
>   to max_cpus have already been handed out.
> - Archs like PowerPC and x86 want cpu_exec_init() to be called from realize
>   rather than instance_init. With this change, those architectures
>   that can move this call into realize function can do so in a phased
>   manner.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
[...]
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index a98b7d8..0a0c21e 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -257,7 +257,7 @@ static void alpha_cpu_initfn(Object *obj)
>      CPUAlphaState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(env);
> +    cpu_exec_init(env, NULL);

If cpu_exec_init() fails, we don't want to silently ignore the error. If
you can't report the error back to the caller, you can use &error_abort
to make QEMU abort on errors.

-- 
Eduardo