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

[Qemu-devel] [PATCH v2 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.

An 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 v2
-------------
Following changes to address Eduardo's comments:

- Call cpu_exec_init() from generic CPU::instance_finalize() instead
  of touching all archs.
- Initialize cpu_index to -1 from generic CPU::instance_init().
- Callers of cpu_exec_init() now pass error_abort argument instead of NULL.
- Consolidate the older CPU enumeration code for CONFIG_USER_ONLY under
  cpu_get_free_index().

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 v2 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>
---
 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 v2 1/3] cpus: Add Error argument to cpu_exec_init()

[Eduardo Habkost]

On Tue, May 12, 2015 at 11:06:24AM +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>

-- 
Eduardo

[Qemu-devel] [PATCH v2 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>
---
 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..c8c4e53 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;
+    } else {
+        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 v2 2/3] cpus: Convert cpu_index into a bitmap

[Eduardo Habkost]

On Tue, May 12, 2015 at 11:06:25AM +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>

Looks good to me, minor comments below:

> ---
>  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..c8c4e53 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;
> +    } else {
> +        bitmap_set(cpu_index_map, cpu, 1);
> +        return cpu;
> +    }

I prefer the following style, to avoid indenting the non-error path:

     if (error_condition) {
        error_setg(...);
        return;
     }

     bitmap_set(...)
     return cpu;

(Which is exactly the style you used in cpu_exec_exit() below, by the
way).

> +}
> +
> +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);

Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same
header file?

>
>  #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
> 

-- 
Eduardo

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

[Bharata B Rao]

On Mon, May 18, 2015 at 03:36:54PM -0300, Eduardo Habkost wrote:
> On Tue, May 12, 2015 at 11:06:25AM +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>
> 
> Looks good to me, minor comments below:
> 
> > ---
> >  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..c8c4e53 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;
> > +    } else {
> > +        bitmap_set(cpu_index_map, cpu, 1);
> > +        return cpu;
> > +    }
> 
> I prefer the following style, to avoid indenting the non-error path:
> 
>      if (error_condition) {
>         error_setg(...);
>         return;
>      }
> 
>      bitmap_set(...)
>      return cpu;
> 
> (Which is exactly the style you used in cpu_exec_exit() below, by the
> way).

Sure, I can change this in next post.

> 
> > +}
> > +
> > +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);
> 
> Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same
> header file?

Currently cpu_exec_init() is in exec-all.h.

1. If I put cpu_exec_exit() also there, qom/cpu.c doesn't like it since
many definitions (like ram_addr_t etc) aren't known in qom/cpu.c.

2. I can't move cpu_exec_init() declaration to qom/cpu.h since it results
in the use of poisoned definition of CPUArchState from qom/cpu.c

Hence I separated them into different hearder files.

Regards,
Bharata.

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

[Eduardo Habkost]

On Tue, May 19, 2015 at 11:49:36AM +0530, Bharata B Rao wrote:
> On Mon, May 18, 2015 at 03:36:54PM -0300, Eduardo Habkost wrote:
> > On Tue, May 12, 2015 at 11:06:25AM +0530, Bharata B Rao wrote:
[...]
> > >  void cpu_exec_init(CPUArchState *env, Error **errp)
[...]
> > > 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);
> > 
> > Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same
> > header file?
> 
> Currently cpu_exec_init() is in exec-all.h.
> 
> 1. If I put cpu_exec_exit() also there, qom/cpu.c doesn't like it since
> many definitions (like ram_addr_t etc) aren't known in qom/cpu.c.
> 
> 2. I can't move cpu_exec_init() declaration to qom/cpu.h since it results
> in the use of poisoned definition of CPUArchState from qom/cpu.c

OK, so we can change cpu_exec_init() to use CPUState in a follow-up
patch later.

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

-- 
Eduardo

[Qemu-devel] [PATCH v2 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 v2 0/3] Bitmap based CPU enumeration

[Eduardo Habkost]

On Tue, May 12, 2015 at 11:06:23AM +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.

What about the existing vmstate and savevm calls on cpu_exec_init()?
Won't QEMU crash if you destroy the CPU without unregistering the
vmstate and savevm handlers?

> 
> An 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 ?

I don't believe we need it, and instead we should make cpu_index
irrelevant. cpu_index is just an arbitrary ID assigned to the CPU, and
any interface that depends on it for something needs to use clearer and
more meaningful parameters as input (such as socket/core/thread
information), instead of cpu_index.

For example, on X86 the APIC ID depends on the CPU socket/core/thread
IDs, and today we just use the cpu_index to calculate it. In the future,
we need to let users choose (directly or indirectly) the specific
socket/core/thread IDs for the CPU, so the APIC ID can be calculated
without requiring cpu_index.

Probably the same applies to cpu_dt_id on PPC: you need to be able to
calculate cpu_dt_id without cpu_index (using core and thread IDs as
input, I guess?).


> 
> Changes in v2
> -------------
> Following changes to address Eduardo's comments:
> 
> - Call cpu_exec_init() from generic CPU::instance_finalize() instead
>   of touching all archs.
> - Initialize cpu_index to -1 from generic CPU::instance_init().
> - Callers of cpu_exec_init() now pass error_abort argument instead of NULL.
> - Consolidate the older CPU enumeration code for CONFIG_USER_ONLY under
>   cpu_get_free_index().
> 
> 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
> 

-- 
Eduardo

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

[Bharata B Rao]

On Mon, May 18, 2015 at 04:09:49PM -0300, Eduardo Habkost wrote:
> On Tue, May 12, 2015 at 11:06:23AM +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.
> 
> What about the existing vmstate and savevm calls on cpu_exec_init()?
> Won't QEMU crash if you destroy the CPU without unregistering the
> vmstate and savevm handlers?

There was a patch from Zhu to move the vmstate registration
code into cpu_common_realizefn

http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01550.html

Also there was also a patch to do unregistration from target CPU's
unrealizefn for x86.

https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02599.html

On PowerPC Currently I do unregistration in the ppc CPU's unrealizefn.

But irrespective of how the above patches evolve, does it make
sense to have unregistration part done in cpu_exec_exit() now as
part of this patch series ?

> 
> > 
> > An 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 ?
> 
> I don't believe we need it, and instead we should make cpu_index
> irrelevant. cpu_index is just an arbitrary ID assigned to the CPU, and
> any interface that depends on it for something needs to use clearer and
> more meaningful parameters as input (such as socket/core/thread
> information), instead of cpu_index.
> 
> For example, on X86 the APIC ID depends on the CPU socket/core/thread
> IDs, and today we just use the cpu_index to calculate it. In the future,
> we need to let users choose (directly or indirectly) the specific
> socket/core/thread IDs for the CPU, so the APIC ID can be calculated
> without requiring cpu_index.
> 
> Probably the same applies to cpu_dt_id on PPC: you need to be able to
> calculate cpu_dt_id without cpu_index (using core and thread IDs as
> input, I guess?).

I see your point and need more thinking to see how to disassociate
cpu_dt_id from cpu_index on PowerPC.

Regards,
Bharata.

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

[Eduardo Habkost]

On Tue, May 19, 2015 at 10:46:03AM +0530, Bharata B Rao wrote:
> On Mon, May 18, 2015 at 04:09:49PM -0300, Eduardo Habkost wrote:
> > On Tue, May 12, 2015 at 11:06:23AM +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.
> > 
> > What about the existing vmstate and savevm calls on cpu_exec_init()?
> > Won't QEMU crash if you destroy the CPU without unregistering the
> > vmstate and savevm handlers?
> 
> There was a patch from Zhu to move the vmstate registration
> code into cpu_common_realizefn
> 
> http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01550.html
> 
> Also there was also a patch to do unregistration from target CPU's
> unrealizefn for x86.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02599.html
> 
> On PowerPC Currently I do unregistration in the ppc CPU's unrealizefn.
> 
> But irrespective of how the above patches evolve, does it make
> sense to have unregistration part done in cpu_exec_exit() now as
> part of this patch series ?

It would make sense, but there's no need to do it in this series if we
are already in the process of moving the register/unregister calls
elsewhere.

(But wherever you do the registration calls, you shouldn't need to do
unregistration in arch-specific code if registration is done on generic
code.)

-- 
Eduardo