[Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[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.

Changes in v3
-------------
- Avoid indentation in non-error path (cosmetic change suggested by Eduardo)

v3: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02151.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01385.html
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                      | 57 ++++++++++++++++++++++++++++++++++++++++-----
 include/exec/exec-all.h     |  2 +-
 include/qom/cpu.h           |  1 +
 qom/cpu.c                   |  7 ++++++
 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 |  9 +++++--
 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 +-
 21 files changed, 83 insertions(+), 25 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 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. This is in preparation to change the CPU enumeration logic
in cpu_exec_init(). With the new enumeration logic, cpu_exec_init()
can fail if cpu_index values corresponding to max_cpus have already
been handed out.

Since all current callers of cpu_exec_init() are from instance_init,
use error_abort Error arugment to abort in case of an error.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 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 e19ab22..5cf821e 100644
--- a/exec.c
+++ b/exec.c
@@ -518,7 +518,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..e865ba7 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, &error_abort);
     tlb_flush(cs, 1);
 
     alpha_translate_init();
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 3ca3fa8..04a79bc 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, &error_abort);
     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..bb8e7ea 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, &error_abort);
 
     env->pregs[PR_VR] = ccc->vr;
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3305e09..5f3822f 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, &error_abort);
 
     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..da4fde1 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, &error_abort);
 
     env->flags = 0;
 
diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
index 4cfb725..ae3d765 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, &error_abort);
 
     if (tcg_enabled() && !inited) {
         inited = true;
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index 67e3182..eac4de0 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, &error_abort);
 
     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..1fd9f22 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, &error_abort);
 
     if (tcg_enabled()) {
         mips_tcg_init();
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 47b617f..415c65a 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, &error_abort);
 
     if (tcg_enabled() && !inited) {
         inited = 1;
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index 39bedc1..cd6c657 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, &error_abort);
 
 #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..52d95ce 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, &error_abort);
     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 d2f9836..b5225a1 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, &error_abort);
 #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..c988adf 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, &error_abort);
 
     env->movcal_backup_tail = &(env->movcal_backup);
 
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index a952097..4d1da7c 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, &error_abort);
 
     if (tcg_enabled()) {
         gen_intermediate_code_init(env);
diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
index 2ba0cf4..8505a45 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, &error_abort);
 
     if (tcg_enabled()) {
         tricore_tcg_init();
diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
index 5b32987..eda039c 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, &error_abort);
 
 #ifdef CONFIG_USER_ONLY
     env->uncached_asr = ASR_MODE_USER;
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index 2b75678..8d69d23 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, &error_abort);
 
     if (tcg_enabled() && !tcg_inited) {
         tcg_inited = true;
-- 
2.1.0

[Qemu-devel] [PATCH v3 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. Newly added
cpu_exec_exit() API handles the deallocation part and this routine is
called from generic CPU::instance_finalize().

Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
CONFIG_USER_ONLY continues to have the old enumeration logic.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c            | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 include/qom/cpu.h |  1 +
 qom/cpu.c         |  7 +++++++
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index 5cf821e..dd688b8 100644
--- a/exec.c
+++ b/exec.c
@@ -518,21 +518,66 @@ 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 -1;
+    }
+
+    bitmap_set(cpu_index_map, cpu, 1);
+    return cpu;
+}
+
+void cpu_exec_exit(CPUState *cpu)
+{
+    if (cpu->cpu_index == -1) {
+        /* cpu_index was never allocated by this @cpu or was already freed. */
+        return;
+    }
+
+    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
+    cpu->cpu_index = -1;
+}
+#else
+
+static int cpu_get_free_index(Error **errp)
+{
+    CPUState *some_cpu;
+    int cpu_index = 0;
+
+    CPU_FOREACH(some_cpu) {
+        cpu_index++;
+    }
+    return cpu_index;
+}
+
+void cpu_exec_exit(CPUState *cpu)
+{
+}
+#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;
+    Error *local_err = NULL;
 
 #if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
 #endif
-    cpu_index = 0;
-    CPU_FOREACH(some_cpu) {
-        cpu_index++;
+    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
     }
-    cpu->cpu_index = cpu_index;
     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..7db310e 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
 
 void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
+void cpu_exec_exit(CPUState *cpu);
 
 #ifdef CONFIG_SOFTMMU
 extern const struct VMStateDescription vmstate_cpu_common;
diff --git a/qom/cpu.c b/qom/cpu.c
index 108bfa2..061a0c3 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -312,9 +312,15 @@ static void cpu_common_initfn(Object *obj)
     CPUState *cpu = CPU(obj);
     CPUClass *cc = CPU_GET_CLASS(obj);
 
+    cpu->cpu_index = -1;
     cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
 }
 
+static void cpu_common_finalize(Object *obj)
+{
+    cpu_exec_exit(CPU(obj));
+}
+
 static int64_t cpu_common_get_arch_id(CPUState *cpu)
 {
     return cpu->cpu_index;
@@ -356,6 +362,7 @@ static const TypeInfo cpu_type_info = {
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(CPUState),
     .instance_init = cpu_common_initfn,
+    .instance_finalize = cpu_common_finalize,
     .abstract = true,
     .class_size = sizeof(CPUClass),
     .class_init = cpu_class_init,
-- 
2.1.0

[Qemu-devel] [PATCH v3 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 52d95ce..2b72f2d 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;
@@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
     CPUPPCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env, &error_abort);
-    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] [PATCH v3 3/3] ppc: Move cpu_exec_init() call to realize function

[Andreas Färber]

Am 21.05.2015 um 07:02 schrieb 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 52d95ce..2b72f2d 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;
> @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
>      CPUPPCState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(env, &error_abort);
> -    cpu->cpu_dt_id = cs->cpu_index;

Commit message talks about movements, but not about dropping cpu_dt_id
assignment - accidental or just missing an explanation?

Regards,
Andreas

>  
>      env->msr_mask = pcc->msr_mask;
>      env->mmu_model = pcc->mmu_model;
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

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

[Bharata B Rao]

On Thu, May 21, 2015 at 07:28:16AM +0200, Andreas Färber wrote:
> Am 21.05.2015 um 07:02 schrieb 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 52d95ce..2b72f2d 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;
> > @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
> >      CPUPPCState *env = &cpu->env;
> >  
> >      cs->env_ptr = env;
> > -    cpu_exec_init(env, &error_abort);
> > -    cpu->cpu_dt_id = cs->cpu_index;
> 
> Commit message talks about movements, but not about dropping cpu_dt_id
> assignment - accidental or just missing an explanation?

There is already code in realizefn of ppc cpu to deduce cpu_dt_id from
cpu_index, so this default assignment here isn't necessary.

Regards,
Bharata.

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

[Igor Mammedov]

On Thu, 21 May 2015 10:32:06 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Add an Error argument to cpu_exec_init() to let users collect the
> error. This is in preparation to change the CPU enumeration logic
> in cpu_exec_init(). With the new enumeration logic, cpu_exec_init()
> can fail if cpu_index values corresponding to max_cpus have already
> been handed out.
> 
> Since all current callers of cpu_exec_init() are from instance_init,
> use error_abort Error arugment to abort in case of an error.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  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 e19ab22..5cf821e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -518,7 +518,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..e865ba7 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, &error_abort);
>      tlb_flush(cs, 1);
>  
>      alpha_translate_init();
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 3ca3fa8..04a79bc 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, &error_abort);
>      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..bb8e7ea 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, &error_abort);
>  
>      env->pregs[PR_VR] = ccc->vr;
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3305e09..5f3822f 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, &error_abort);
>  
>      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..da4fde1 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, &error_abort);
>  
>      env->flags = 0;
>  
> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
> index 4cfb725..ae3d765 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, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = true;
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 67e3182..eac4de0 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, &error_abort);
>  
>      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..1fd9f22 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, &error_abort);
>  
>      if (tcg_enabled()) {
>          mips_tcg_init();
> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
> index 47b617f..415c65a 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, &error_abort);
>  
>      if (tcg_enabled() && !inited) {
>          inited = 1;
> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
> index 39bedc1..cd6c657 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, &error_abort);
>  
>  #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..52d95ce 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, &error_abort);
>      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 d2f9836..b5225a1 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, &error_abort);
>  #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..c988adf 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, &error_abort);
>  
>      env->movcal_backup_tail = &(env->movcal_backup);
>  
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index a952097..4d1da7c 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, &error_abort);
>  
>      if (tcg_enabled()) {
>          gen_intermediate_code_init(env);
> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
> index 2ba0cf4..8505a45 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, &error_abort);
>  
>      if (tcg_enabled()) {
>          tricore_tcg_init();
> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
> index 5b32987..eda039c 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, &error_abort);
>  
>  #ifdef CONFIG_USER_ONLY
>      env->uncached_asr = ASR_MODE_USER;
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 2b75678..8d69d23 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, &error_abort);
>  
>      if (tcg_enabled() && !tcg_inited) {
>          tcg_inited = true;

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

[Igor Mammedov]

On Thu, 21 May 2015 10:32:07 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> 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. Newly added
> cpu_exec_exit() API handles the deallocation part and this routine is
> called from generic CPU::instance_finalize().
> 
> Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
> CONFIG_USER_ONLY continues to have the old enumeration logic.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
minor nit below, otherwise:

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c            | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/qom/cpu.h |  1 +
>  qom/cpu.c         |  7 +++++++
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 5cf821e..dd688b8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -518,21 +518,66 @@ 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);
wrong indent

> +        return -1;
> +    }
> +
> +    bitmap_set(cpu_index_map, cpu, 1);
> +    return cpu;
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +    if (cpu->cpu_index == -1) {
> +        /* cpu_index was never allocated by this @cpu or was already freed. */
> +        return;
> +    }
> +
> +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> +    cpu->cpu_index = -1;
> +}
> +#else
> +
> +static int cpu_get_free_index(Error **errp)
> +{
> +    CPUState *some_cpu;
> +    int cpu_index = 0;
> +
> +    CPU_FOREACH(some_cpu) {
> +        cpu_index++;
> +    }
> +    return cpu_index;
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +}
> +#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;
> +    Error *local_err = NULL;
>  
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> -    cpu_index = 0;
> -    CPU_FOREACH(some_cpu) {
> -        cpu_index++;
> +    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
>      }
> -    cpu->cpu_index = cpu_index;
>      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..7db310e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
>  
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
> +void cpu_exec_exit(CPUState *cpu);
>  
>  #ifdef CONFIG_SOFTMMU
>  extern const struct VMStateDescription vmstate_cpu_common;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 108bfa2..061a0c3 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -312,9 +312,15 @@ static void cpu_common_initfn(Object *obj)
>      CPUState *cpu = CPU(obj);
>      CPUClass *cc = CPU_GET_CLASS(obj);
>  
> +    cpu->cpu_index = -1;
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>  }
>  
> +static void cpu_common_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static int64_t cpu_common_get_arch_id(CPUState *cpu)
>  {
>      return cpu->cpu_index;
> @@ -356,6 +362,7 @@ static const TypeInfo cpu_type_info = {
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(CPUState),
>      .instance_init = cpu_common_initfn,
> +    .instance_finalize = cpu_common_finalize,
>      .abstract = true,
>      .class_size = sizeof(CPUClass),
>      .class_init = cpu_class_init,

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

[Igor Mammedov]

On Thu, 21 May 2015 10:32:08 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> 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 52d95ce..2b72f2d 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;
> @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
>      CPUPPCState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(env, &error_abort);
> -    cpu->cpu_dt_id = cs->cpu_index;
could you switch ppc to use CPUClass->get_arch_id()
it defaults to cpu_index if arch doesn't overload it.

there were patches on list wrt migration to use
get_arch_id() instead of cpu_index as migration block id.

>  
>      env->msr_mask = pcc->msr_mask;
>      env->mmu_model = pcc->mmu_model;

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

[Bharata B Rao]

On Thu, May 21, 2015 at 11:05:19AM +0200, Igor Mammedov wrote:
> On Thu, 21 May 2015 10:32:08 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > 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 52d95ce..2b72f2d 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;
> > @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
> >      CPUPPCState *env = &cpu->env;
> >  
> >      cs->env_ptr = env;
> > -    cpu_exec_init(env, &error_abort);
> > -    cpu->cpu_dt_id = cs->cpu_index;
> could you switch ppc to use CPUClass->get_arch_id()
> it defaults to cpu_index if arch doesn't overload it.

Just to be sure, your suggestion to implement ->get_arch_id() for
ppc has nothing to do with this patch right ?

> 
> there were patches on list wrt migration to use
> get_arch_id() instead of cpu_index as migration block id.

Yes, saw them. AFAICS, we could have ->get_arch_id returning
cpu_dt_id like how x86 returns apic_id when Zhu's patches make it
upstream.

Regards,
Bharata.

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

[Igor Mammedov]

On Thu, 21 May 2015 15:48:15 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Thu, May 21, 2015 at 11:05:19AM +0200, Igor Mammedov wrote:
> > On Thu, 21 May 2015 10:32:08 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > 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 52d95ce..2b72f2d 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;
> > > @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > >      cs->env_ptr = env;
> > > -    cpu_exec_init(env, &error_abort);
> > > -    cpu->cpu_dt_id = cs->cpu_index;
> > could you switch ppc to use CPUClass->get_arch_id()
> > it defaults to cpu_index if arch doesn't overload it.
> 
> Just to be sure, your suggestion to implement ->get_arch_id() for
> ppc has nothing to do with this patch right ?
yep

> 
> > 
> > there were patches on list wrt migration to use
> > get_arch_id() instead of cpu_index as migration block id.
> 
> Yes, saw them. AFAICS, we could have ->get_arch_id returning
> cpu_dt_id like how x86 returns apic_id when Zhu's patches make it
> upstream.
> 
> Regards,
> Bharata.
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

On Thu, May 21, 2015 at 10:32:06AM +0530, Bharata B Rao wrote:
> Add an Error argument to cpu_exec_init() to let users collect the
> error. This is in preparation to change the CPU enumeration logic
> in cpu_exec_init(). With the new enumeration logic, cpu_exec_init()
> can fail if cpu_index values corresponding to max_cpus have already
> been handed out.
> 
> Since all current callers of cpu_exec_init() are from instance_init,
> use error_abort Error arugment to abort in case of an error.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]

On Thu, May 21, 2015 at 10:32:07AM +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. Newly added
> cpu_exec_exit() API handles the deallocation part and this routine is
> called from generic CPU::instance_finalize().
> 
> Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
> CONFIG_USER_ONLY continues to have the old enumeration logic.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Bharata B Rao]

All the comments have been addressed and the series has been reviewed
by David, Eduardo and Igor. Can this series be taken in now ?

Regards,
Bharata.

On Thu, May 21, 2015 at 10:32:05AM +0530, Bharata B Rao wrote:
> 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.
> 
> Changes in v3
> -------------
> - Avoid indentation in non-error path (cosmetic change suggested by Eduardo)
> 
> v3: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02151.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01385.html
> 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                      | 57 ++++++++++++++++++++++++++++++++++++++++-----
>  include/exec/exec-all.h     |  2 +-
>  include/qom/cpu.h           |  1 +
>  qom/cpu.c                   |  7 ++++++
>  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 |  9 +++++--
>  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 +-
>  21 files changed, 83 insertions(+), 25 deletions(-)
> 
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Peter Crosthwaite]

On Thu, May 28, 2015 at 7:27 PM, Bharata B Rao
<bharata@linux.vnet.ibm.com> wrote:
> All the comments have been addressed and the series has been reviewed
> by David, Eduardo and Igor. Can this series be taken in now ?
>

Andreas' comment on P3 looks unaddressed. I think it can be handled by
just putting that one sentance explanation you gave in commit message,
or if its far enough out of scope just drop the change.

I think Igor's comment was an out of scope suggestion in the end so
nothing needed there?

Regards,
Peter

P.S. I am not the maintainer but I need to rebase on you for one of my
patch sets so I'd like to help see this though!

> Regards,
> Bharata.
>
> On Thu, May 21, 2015 at 10:32:05AM +0530, Bharata B Rao wrote:
>> 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.
>>
>> Changes in v3
>> -------------
>> - Avoid indentation in non-error path (cosmetic change suggested by Eduardo)
>>
>> v3: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02151.html
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01385.html
>> 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                      | 57 ++++++++++++++++++++++++++++++++++++++++-----
>>  include/exec/exec-all.h     |  2 +-
>>  include/qom/cpu.h           |  1 +
>>  qom/cpu.c                   |  7 ++++++
>>  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 |  9 +++++--
>>  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 +-
>>  21 files changed, 83 insertions(+), 25 deletions(-)
>>
>> --
>> 2.1.0
>
>

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

[Peter Crosthwaite]

On Wed, May 20, 2015 at 10:02 PM, Bharata B Rao
<bharata@linux.vnet.ibm.com> wrote:
> Add an Error argument to cpu_exec_init() to let users collect the
> error. This is in preparation to change the CPU enumeration logic
> in cpu_exec_init(). With the new enumeration logic, cpu_exec_init()
> can fail if cpu_index values corresponding to max_cpus have already
> been handed out.
>
> Since all current callers of cpu_exec_init() are from instance_init,
> use error_abort Error arugment to abort in case of an error.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  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 e19ab22..5cf821e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -518,7 +518,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..e865ba7 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, &error_abort);
>      tlb_flush(cs, 1);
>
>      alpha_translate_init();
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 3ca3fa8..04a79bc 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, &error_abort);
>      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..bb8e7ea 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, &error_abort);
>
>      env->pregs[PR_VR] = ccc->vr;
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3305e09..5f3822f 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, &error_abort);
>
>      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..da4fde1 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, &error_abort);
>
>      env->flags = 0;
>
> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
> index 4cfb725..ae3d765 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, &error_abort);
>
>      if (tcg_enabled() && !inited) {
>          inited = true;
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 67e3182..eac4de0 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, &error_abort);
>
>      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..1fd9f22 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, &error_abort);
>
>      if (tcg_enabled()) {
>          mips_tcg_init();
> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
> index 47b617f..415c65a 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, &error_abort);
>
>      if (tcg_enabled() && !inited) {
>          inited = 1;
> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
> index 39bedc1..cd6c657 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, &error_abort);
>
>  #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..52d95ce 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, &error_abort);
>      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 d2f9836..b5225a1 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, &error_abort);
>  #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..c988adf 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, &error_abort);
>
>      env->movcal_backup_tail = &(env->movcal_backup);
>
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index a952097..4d1da7c 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, &error_abort);
>
>      if (tcg_enabled()) {
>          gen_intermediate_code_init(env);
> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
> index 2ba0cf4..8505a45 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, &error_abort);
>
>      if (tcg_enabled()) {
>          tricore_tcg_init();
> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
> index 5b32987..eda039c 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, &error_abort);
>
>  #ifdef CONFIG_USER_ONLY
>      env->uncached_asr = ASR_MODE_USER;
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 2b75678..8d69d23 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, &error_abort);
>
>      if (tcg_enabled() && !tcg_inited) {
>          tcg_inited = true;
> --
> 2.1.0
>
>

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

[Peter Crosthwaite]

On Wed, May 20, 2015 at 10:02 PM, Bharata B Rao
<bharata@linux.vnet.ibm.com> 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. Newly added
> cpu_exec_exit() API handles the deallocation part and this routine is
> called from generic CPU::instance_finalize().
>
> Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
> CONFIG_USER_ONLY continues to have the old enumeration logic.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  exec.c            | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/qom/cpu.h |  1 +
>  qom/cpu.c         |  7 +++++++
>  3 files changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 5cf821e..dd688b8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -518,21 +518,66 @@ 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 -1;
> +    }
> +
> +    bitmap_set(cpu_index_map, cpu, 1);
> +    return cpu;
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +    if (cpu->cpu_index == -1) {
> +        /* cpu_index was never allocated by this @cpu or was already freed. */
> +        return;
> +    }
> +
> +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> +    cpu->cpu_index = -1;
> +}
> +#else
> +
> +static int cpu_get_free_index(Error **errp)
> +{
> +    CPUState *some_cpu;
> +    int cpu_index = 0;
> +
> +    CPU_FOREACH(some_cpu) {
> +        cpu_index++;
> +    }
> +    return cpu_index;
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +}
> +#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;
> +    Error *local_err = NULL;
>
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> -    cpu_index = 0;
> -    CPU_FOREACH(some_cpu) {
> -        cpu_index++;
> +    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
>      }
> -    cpu->cpu_index = cpu_index;
>      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..7db310e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
>
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
> +void cpu_exec_exit(CPUState *cpu);
>
>  #ifdef CONFIG_SOFTMMU
>  extern const struct VMStateDescription vmstate_cpu_common;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 108bfa2..061a0c3 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -312,9 +312,15 @@ static void cpu_common_initfn(Object *obj)
>      CPUState *cpu = CPU(obj);
>      CPUClass *cc = CPU_GET_CLASS(obj);
>
> +    cpu->cpu_index = -1;
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>  }
>
> +static void cpu_common_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static int64_t cpu_common_get_arch_id(CPUState *cpu)
>  {
>      return cpu->cpu_index;
> @@ -356,6 +362,7 @@ static const TypeInfo cpu_type_info = {
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(CPUState),
>      .instance_init = cpu_common_initfn,
> +    .instance_finalize = cpu_common_finalize,
>      .abstract = true,
>      .class_size = sizeof(CPUClass),
>      .class_init = cpu_class_init,
> --
> 2.1.0
>
>

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

[Peter Crosthwaite]

On Wed, May 20, 2015 at 10:02 PM, Bharata B Rao
<bharata@linux.vnet.ibm.com> wrote:
> 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 52d95ce..2b72f2d 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;
> @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
>      CPUPPCState *env = &cpu->env;
>
>      cs->env_ptr = env;
> -    cpu_exec_init(env, &error_abort);

> -    cpu->cpu_dt_id = cs->cpu_index;

With droppage of this line, or update to commit msg:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

>
>      env->msr_mask = pcc->msr_mask;
>      env->mmu_model = pcc->mmu_model;
> --
> 2.1.0
>
>

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

[Bharata B Rao]

On Thu, May 28, 2015 at 10:17:25PM -0700, Peter Crosthwaite wrote:
> On Wed, May 20, 2015 at 10:02 PM, Bharata B Rao
> <bharata@linux.vnet.ibm.com> wrote:
> > 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 52d95ce..2b72f2d 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;
> > @@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
> >      CPUPPCState *env = &cpu->env;
> >
> >      cs->env_ptr = env;
> > -    cpu_exec_init(env, &error_abort);
> 
> > -    cpu->cpu_dt_id = cs->cpu_index;
> 
> With droppage of this line, or update to commit msg:
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

This patch with updated patch description is below:


>From 63d355f6b796858cb99758f6f91043826cba7617 Mon Sep 17 00:00:00 2001
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
Date: Thu, 21 May 2015 10:01:17 +0530
Subject: [PATCH v3 3/3] ppc: Move cpu_exec_init() call to realize function

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.

cpu_dt_id assignment from instance_init is no longer needed since correct
assignment for cpu_dt_id is already present in realizefn.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 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 52d95ce..2b72f2d 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;
@@ -9633,8 +9640,6 @@ static void ppc_cpu_initfn(Object *obj)
     CPUPPCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env, &error_abort);
-    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] [PATCH v3 0/3] Bitmap based CPU enumeration

[Bharata B Rao]

On Thu, May 28, 2015 at 09:59:38PM -0700, Peter Crosthwaite wrote:
> On Thu, May 28, 2015 at 7:27 PM, Bharata B Rao
> <bharata@linux.vnet.ibm.com> wrote:
> > All the comments have been addressed and the series has been reviewed
> > by David, Eduardo and Igor. Can this series be taken in now ?
> >
> 
> Andreas' comment on P3 looks unaddressed. I think it can be handled by
> just putting that one sentance explanation you gave in commit message,
> or if its far enough out of scope just drop the change.
> 
> I think Igor's comment was an out of scope suggestion in the end so
> nothing needed there?
> 
> Regards,
> Peter
> 
> P.S. I am not the maintainer but I need to rebase on you for one of my
> patch sets so I'd like to help see this though!

Should I be rebasing against latest master or anyone else's tree to make
it easier for inclusion ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Peter Crosthwaite]

On Wed, Jun 3, 2015 at 8:08 PM, Bharata B Rao
<bharata@linux.vnet.ibm.com> wrote:
> On Thu, May 28, 2015 at 09:59:38PM -0700, Peter Crosthwaite wrote:
>> On Thu, May 28, 2015 at 7:27 PM, Bharata B Rao
>> <bharata@linux.vnet.ibm.com> wrote:
>> > All the comments have been addressed and the series has been reviewed
>> > by David, Eduardo and Igor. Can this series be taken in now ?
>> >
>>
>> Andreas' comment on P3 looks unaddressed. I think it can be handled by
>> just putting that one sentance explanation you gave in commit message,
>> or if its far enough out of scope just drop the change.
>>
>> I think Igor's comment was an out of scope suggestion in the end so
>> nothing needed there?
>>
>> Regards,
>> Peter
>>
>> P.S. I am not the maintainer but I need to rebase on you for one of my
>> patch sets so I'd like to help see this though!
>
> Should I be rebasing against latest master or anyone else's tree to make
> it easier for inclusion ?
>

I don't know about anyone elses tree, but there is an edit to last
patch so a fresh complete v4 rebased is probably going to make life
easy for whoever.

I have CCd Paolo who owns exec.c according to MAINTAINERS.

Regards,
Peter

> Regards,
> Bharata.
>
>

Re: [Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Paolo Bonzini]

On 04/06/2015 07:44, Peter Crosthwaite wrote:
> On Wed, Jun 3, 2015 at 8:08 PM, Bharata B Rao
> <bharata@linux.vnet.ibm.com> wrote:
>> On Thu, May 28, 2015 at 09:59:38PM -0700, Peter Crosthwaite wrote:
>>> On Thu, May 28, 2015 at 7:27 PM, Bharata B Rao
>>> <bharata@linux.vnet.ibm.com> wrote:
>>>> All the comments have been addressed and the series has been reviewed
>>>> by David, Eduardo and Igor. Can this series be taken in now ?
>>>>
>>>
>>> Andreas' comment on P3 looks unaddressed. I think it can be handled by
>>> just putting that one sentance explanation you gave in commit message,
>>> or if its far enough out of scope just drop the change.
>>>
>>> I think Igor's comment was an out of scope suggestion in the end so
>>> nothing needed there?
>>>
>>> Regards,
>>> Peter
>>>
>>> P.S. I am not the maintainer but I need to rebase on you for one of my
>>> patch sets so I'd like to help see this though!
>>
>> Should I be rebasing against latest master or anyone else's tree to make
>> it easier for inclusion ?
>>
> 
> I don't know about anyone elses tree, but there is an edit to last
> patch so a fresh complete v4 rebased is probably going to make life
> easy for whoever.
> 
> I have CCd Paolo who owns exec.c according to MAINTAINERS.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I wouldn't mind separating the "CPU" parts of exec.c and moving them
under Andreas and Eduardo's mantainership.  In fact, Peter, in your
patch to move stuff from cpu-exec.c to cpus.c, perhaps you can use
qom/cpu.c instead?  Then qom/cpu.c can also be the place where we can
move the CPU parts of exec.c.

Paolo

Re: [Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Peter Crosthwaite]

On Thu, Jun 4, 2015 at 1:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 04/06/2015 07:44, Peter Crosthwaite wrote:
>> On Wed, Jun 3, 2015 at 8:08 PM, Bharata B Rao
>> <bharata@linux.vnet.ibm.com> wrote:
>>> On Thu, May 28, 2015 at 09:59:38PM -0700, Peter Crosthwaite wrote:
>>>> On Thu, May 28, 2015 at 7:27 PM, Bharata B Rao
>>>> <bharata@linux.vnet.ibm.com> wrote:
>>>>> All the comments have been addressed and the series has been reviewed
>>>>> by David, Eduardo and Igor. Can this series be taken in now ?
>>>>>
>>>>
>>>> Andreas' comment on P3 looks unaddressed. I think it can be handled by
>>>> just putting that one sentance explanation you gave in commit message,
>>>> or if its far enough out of scope just drop the change.
>>>>
>>>> I think Igor's comment was an out of scope suggestion in the end so
>>>> nothing needed there?
>>>>
>>>> Regards,
>>>> Peter
>>>>
>>>> P.S. I am not the maintainer but I need to rebase on you for one of my
>>>> patch sets so I'd like to help see this though!
>>>
>>> Should I be rebasing against latest master or anyone else's tree to make
>>> it easier for inclusion ?
>>>
>>
>> I don't know about anyone elses tree, but there is an edit to last
>> patch so a fresh complete v4 rebased is probably going to make life
>> easy for whoever.
>>
>> I have CCd Paolo who owns exec.c according to MAINTAINERS.
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> I wouldn't mind separating the "CPU" parts of exec.c and moving them
> under Andreas and Eduardo's mantainership.  In fact, Peter, in your
> patch to move stuff from cpu-exec.c to cpus.c, perhaps you can use
> qom/cpu.c instead?  Then qom/cpu.c can also be the place where we can
> move the CPU parts of exec.c.
>

So that relocated code uses conditional compile based on
CONFIG_SOFTMMU. Is that def accessible from common-obj-y code which
qom/cpu.c is?

My choice of cpus.c was based on the fact that it was obj-y.

I assume this is all follow up work out of scope of Bharata's code. Do
you have a queue I can rebase my conflicting ENV_GET_CPU work on?

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PATCH v3 0/3] Bitmap based CPU enumeration

[Paolo Bonzini]

On 04/06/2015 10:39, Peter Crosthwaite wrote:
>> > I wouldn't mind separating the "CPU" parts of exec.c and moving them
>> > under Andreas and Eduardo's mantainership.  In fact, Peter, in your
>> > patch to move stuff from cpu-exec.c to cpus.c, perhaps you can use
>> > qom/cpu.c instead?  Then qom/cpu.c can also be the place where we can
>> > move the CPU parts of exec.c.
>> >
> So that relocated code uses conditional compile based on
> CONFIG_SOFTMMU. Is that def accessible from common-obj-y code which
> qom/cpu.c is?
> 
> My choice of cpus.c was based on the fact that it was obj-y.

Hmm, right---qom/cpu.c is indeed common-obj-y, so it has to be a new
file.  cpu-exec.c is taken, so I guess I'll move the memory parts of
exec.c out to exec-memory.c and leave exec.c for the obj-y part of CPU
object handling.  We have:

- cpu-exec.c: TCG only, arch-obj-y

- cpus.c: thread management, obj-y

- exec.c: CPU object management, obj-y

- qom/cpu.c: CPU object management, common-obj-y

And you can move the stuff from cpu-exec.c to exec.c in your patches.

> I assume this is all follow up work out of scope of Bharata's code. Do
> you have a queue I can rebase my conflicting ENV_GET_CPU work on?

No, I don't, because I'm not going to be the one who merge these
patches.  Sorry.

Paolo