Re: [Qemu-devel] [RFC PATCH v3 14/24] cpus: Convert cpu_index into a bitmap

[David Gibson]

On Fri, Apr 24, 2015 at 12:17:36PM +0530, Bharata B Rao wrote:
> Currently CPUState.cpu_index is monotonically increasing and a newly
> created CPU always gets the next higher index. The next available
> index is calculated by counting the existing number of CPUs. This is
> fine as long as we only add CPUs, but there are architectures which
> are starting to support CPU removal too. For an architecture like PowerPC
> which derives its CPU identifier (device tree ID) from cpu_index, the
> existing logic of generating cpu_index values causes problems.
> 
> With the currently proposed method of handling vCPU removal by parking
> the vCPU fd in QEMU
> (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
> generating cpu_index this way will not work for PowerPC.
> 
> This patch changes the way cpu_index is handed out by maintaining
> a bit map of the CPUs that tracks both addition and removal of CPUs.
> 
> The CPU bitmap allocation logic is part of cpu_exec_init() which is
> called by instance_init routines of various CPU targets. This patch
> also adds corresponding instance_finalize routine if needed for these
> CPU targets so that CPU can be marked free when it is removed.
> 
> Signed-off-by: Bharata B Rao <address@hidden>

Looks good in concept, though there are a couple of implementation
nits noted below.

I thin kit might be worth posting this patch and the previous one
separately from your spapr hotplug series.  They're generic patches
which can't go through my tree, and they also look sound to me
regardless of how the remaining details of cpu hotplug work out.

> ---
>  exec.c                      | 37 ++++++++++++++++++++++++++++++++++---
>  include/qom/cpu.h           |  8 ++++++++
>  target-alpha/cpu.c          |  6 ++++++
>  target-arm/cpu.c            |  1 +
>  target-cris/cpu.c           |  6 ++++++
>  target-i386/cpu.c           |  6 ++++++
>  target-lm32/cpu.c           |  6 ++++++
>  target-m68k/cpu.c           |  6 ++++++
>  target-microblaze/cpu.c     |  6 ++++++
>  target-mips/cpu.c           |  6 ++++++
>  target-moxie/cpu.c          |  6 ++++++
>  target-openrisc/cpu.c       |  6 ++++++
>  target-ppc/translate_init.c |  6 ++++++
>  target-s390x/cpu.c          |  1 +
>  target-sh4/cpu.c            |  6 ++++++
>  target-sparc/cpu.c          |  1 +
>  target-tricore/cpu.c        |  5 +++++
>  target-unicore32/cpu.c      |  6 ++++++
>  target-xtensa/cpu.c         |  6 ++++++
>  19 files changed, 128 insertions(+), 3 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index e1ff6b0..9bbab02 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -527,21 +527,52 @@ void tcg_cpu_address_space_init(CPUState *cpu, 
> AddressSpace *as)
>  }
>  #endif
>  
> +#ifndef CONFIG_USER_ONLY

Having different methods of handling the cpu infox for USER_ONLY and
softmmu mode seems a bit ugly.  There's no need for the bitmap in user
only mode, but there's no harm to it either.

> +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) {

Might be safer to have cpu >= max_cpus here, just in case something
changes.

> +        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
> +                    max_cpus);
> +        return max_cpus;
> +    } else {
> +        bitmap_set(cpu_index_map, cpu, 1);
> +        return cpu;
> +    }
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> +}
> +#endif
> +
>  void cpu_exec_init(CPUArchState *env, Error **errp)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -    CPUState *some_cpu;
>      int cpu_index;
> -
>  #if defined(CONFIG_USER_ONLY)
> +    CPUState *some_cpu;
> +
>      cpu_list_lock();
> -#endif
>      cpu_index = 0;
>      CPU_FOREACH(some_cpu) {
>          cpu_index++;
>      }
>      cpu->cpu_index = cpu_index;
> +#else
> +    Error *local_err = NULL;
> +
> +    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +#endif
>      cpu->numa_node = 0;
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 48fd6fb..5241cf4 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -659,6 +659,14 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  
> +#ifndef CONFIG_USER_ONLY
> +void cpu_exec_exit(CPUState *cpu);
> +#else
> +static inline void cpu_exec_exit(CPUState *cpu)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_SOFTMMU
>  extern const struct VMStateDescription vmstate_cpu_common;
>  #else
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index 0a0c21e..259a04c 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -250,6 +250,11 @@ static const TypeInfo ev68_cpu_type_info = {
>      .parent = TYPE("ev67"),
>  };
>  
> +static void alpha_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void alpha_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -305,6 +310,7 @@ static const TypeInfo alpha_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(AlphaCPU),
>      .instance_init = alpha_cpu_initfn,
> +    .instance_finalize = alpha_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(AlphaCPUClass),
>      .class_init = alpha_cpu_class_init,
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 86edaab..8d824d3 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -454,6 +454,7 @@ static void arm_cpu_finalizefn(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      g_hash_table_destroy(cpu->cp_regs);
> +    cpu_exec_exit(CPU(obj));
>  }
>  
>  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index 8b589ec..da39223 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -161,6 +161,11 @@ static void cris_cpu_set_irq(void *opaque, int irq, int 
> level)
>  }
>  #endif
>  
> +static void cris_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void cris_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -299,6 +304,7 @@ static const TypeInfo cris_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(CRISCPU),
>      .instance_init = cris_cpu_initfn,
> +    .instance_finalize = cris_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(CRISCPUClass),
>      .class_init = cris_cpu_class_init,
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index daccf4f..ca2b93e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2877,6 +2877,11 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int 
> cpu_index)
>      }
>  }
>  
> +static void x86_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void x86_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -3046,6 +3051,7 @@ static const TypeInfo x86_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(X86CPU),
>      .instance_init = x86_cpu_initfn,
> +    .instance_finalize = x86_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(X86CPUClass),
>      .class_init = x86_cpu_common_class_init,
> diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
> index 89b6631..d7bc973 100644
> --- a/target-lm32/cpu.c
> +++ b/target-lm32/cpu.c
> @@ -143,6 +143,11 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      lcc->parent_realize(dev, errp);
>  }
>  
> +static void lm32_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void lm32_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -294,6 +299,7 @@ static const TypeInfo lm32_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(LM32CPU),
>      .instance_init = lm32_cpu_initfn,
> +    .instance_finalize = lm32_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(LM32CPUClass),
>      .class_init = lm32_cpu_class_init,
> diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
> index 6a41551..c2fce97 100644
> --- a/target-m68k/cpu.c
> +++ b/target-m68k/cpu.c
> @@ -160,6 +160,11 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      mcc->parent_realize(dev, errp);
>  }
>  
> +static void m68k_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void m68k_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -231,6 +236,7 @@ static const TypeInfo m68k_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(M68kCPU),
>      .instance_init = m68k_cpu_initfn,
> +    .instance_finalize = m68k_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(M68kCPUClass),
>      .class_init = m68k_cpu_class_init,
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 6b3732d..3aa3796 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -122,6 +122,11 @@ static void mb_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      mcc->parent_realize(dev, errp);
>  }
>  
> +static void mb_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void mb_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -190,6 +195,7 @@ static const TypeInfo mb_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(MicroBlazeCPU),
>      .instance_init = mb_cpu_initfn,
> +    .instance_finalize = mb_cpu_finalize,
>      .class_size = sizeof(MicroBlazeCPUClass),
>      .class_init = mb_cpu_class_init,
>  };
> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
> index 02f1d32..2150999 100644
> --- a/target-mips/cpu.c
> +++ b/target-mips/cpu.c
> @@ -108,6 +108,11 @@ static void mips_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      mcc->parent_realize(dev, errp);
>  }
>  
> +static void mips_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void mips_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -159,6 +164,7 @@ static const TypeInfo mips_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(MIPSCPU),
>      .instance_init = mips_cpu_initfn,
> +    .instance_finalize = mips_cpu_finalize,
>      .abstract = false,
>      .class_size = sizeof(MIPSCPUClass),
>      .class_init = mips_cpu_class_init,
> diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
> index f815fb3..25d5f30 100644
> --- a/target-moxie/cpu.c
> +++ b/target-moxie/cpu.c
> @@ -59,6 +59,11 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      mcc->parent_realize(dev, errp);
>  }
>  
> +static void moxie_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void moxie_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -160,6 +165,7 @@ static const TypeInfo moxie_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(MoxieCPU),
>      .instance_init = moxie_cpu_initfn,
> +    .instance_finalize = moxie_cpu_finalize,
>      .class_size = sizeof(MoxieCPUClass),
>      .class_init = moxie_cpu_class_init,
>  };
> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
> index 87b2f80..f0c990f 100644
> --- a/target-openrisc/cpu.c
> +++ b/target-openrisc/cpu.c
> @@ -85,6 +85,11 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      occ->parent_realize(dev, errp);
>  }
>  
> +static void openrisc_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void openrisc_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -198,6 +203,7 @@ static const TypeInfo openrisc_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(OpenRISCCPU),
>      .instance_init = openrisc_cpu_initfn,
> +    .instance_finalize = openrisc_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(OpenRISCCPUClass),
>      .class_init = openrisc_cpu_class_init,
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 9f4f172..a553560 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9671,6 +9671,11 @@ static bool ppc_cpu_is_big_endian(CPUState *cs)
>  }
>  #endif
>  
> +static void ppc_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void ppc_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -9784,6 +9789,7 @@ static const TypeInfo ppc_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(PowerPCCPU),
>      .instance_init = ppc_cpu_initfn,
> +    .instance_finalize = ppc_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(PowerPCCPUClass),
>      .class_init = ppc_cpu_class_init,
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 28717bd..198e57b 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -212,6 +212,7 @@ static void s390_cpu_finalize(Object *obj)
>  
>      qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu);
>  #endif
> +    cpu_exec_exit(CPU(obj));
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
> index ffb635e..65f44c7 100644
> --- a/target-sh4/cpu.c
> +++ b/target-sh4/cpu.c
> @@ -240,6 +240,11 @@ static void superh_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      scc->parent_realize(dev, errp);
>  }
>  
> +static void superh_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void superh_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -296,6 +301,7 @@ static const TypeInfo superh_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(SuperHCPU),
>      .instance_init = superh_cpu_initfn,
> +    .instance_finalize = superh_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(SuperHCPUClass),
>      .class_init = superh_cpu_class_init,
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index d857aae..ac7091a 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -815,6 +815,7 @@ static void sparc_cpu_uninitfn(Object *obj)
>      CPUSPARCState *env = &cpu->env;
>  
>      g_free(env->def);
> +    cpu_exec_exit(CPU(obj));
>  }
>  
>  static void sparc_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
> index 53b117b..e871dc4 100644
> --- a/target-tricore/cpu.c
> +++ b/target-tricore/cpu.c
> @@ -80,6 +80,10 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      tcc->parent_realize(dev, errp);
>  }
>  
> +static void tricore_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
>  
>  static void tricore_cpu_initfn(Object *obj)
>  {
> @@ -180,6 +184,7 @@ static const TypeInfo tricore_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(TriCoreCPU),
>      .instance_init = tricore_cpu_initfn,
> +    .instance_finalize = tricore_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(TriCoreCPUClass),
>      .class_init = tricore_cpu_class_init,
> diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
> index d56d78a..64af9f8 100644
> --- a/target-unicore32/cpu.c
> +++ b/target-unicore32/cpu.c
> @@ -103,6 +103,11 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      ucc->parent_realize(dev, errp);
>  }
>  
> +static void uc32_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void uc32_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -174,6 +179,7 @@ static const TypeInfo uc32_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(UniCore32CPU),
>      .instance_init = uc32_cpu_initfn,
> +    .instance_finalize = uc32_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(UniCore32CPUClass),
>      .class_init = uc32_cpu_class_init,
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index dd23d32..565a946 100644
> --- a/target-xtensa/cpu.c
> +++ b/target-xtensa/cpu.c
> @@ -104,6 +104,11 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      xcc->parent_realize(dev, errp);
>  }
>  
> +static void xtensa_cpu_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static void xtensa_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -161,6 +166,7 @@ static const TypeInfo xtensa_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(XtensaCPU),
>      .instance_init = xtensa_cpu_initfn,
> +    .instance_finalize = xtensa_cpu_finalize,
>      .abstract = true,
>      .class_size = sizeof(XtensaCPUClass),
>      .class_init = xtensa_cpu_class_init,

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

pgpmniXBzfSj8.pgp
Description: PGP signature