[RFC,v2,14/23] cpus: Convert cpu_index into a bitmap

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.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c            | 37 ++++++++++++++++++++++++++++++++++---
 include/qom/cpu.h |  8 ++++++++
 2 files changed, 42 insertions(+), 3 deletions(-)

On Mon, Mar 23, 2015 at 07:05:55PM +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.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c            | 37 ++++++++++++++++++++++++++++++++++---
>  include/qom/cpu.h |  8 ++++++++
>  2 files changed, 42 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
> +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> +
> +static int cpu_get_free_index(Error **errp)
> +{
> +    int cpu = find_first_zero_bit(cpu_index_map, max_cpus);
> +
> +    if (cpu == max_cpus) {
> +        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
> +                    max_cpus);
> +        return max_cpus;
> +    } else {
> +        bitmap_set(cpu_index_map, cpu, 1);
> +        return cpu;
> +    }
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> +}

AFAICT, this function is never called, which seems like a bug.

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

On Wed, Mar 25, 2015 at 02:23:29PM +1100, David Gibson wrote:
> On Mon, Mar 23, 2015 at 07:05:55PM +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.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c            | 37 ++++++++++++++++++++++++++++++++++---
> >  include/qom/cpu.h |  8 ++++++++
> >  2 files changed, 42 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
> > +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > +
> > +static int cpu_get_free_index(Error **errp)
> > +{
> > +    int cpu = find_first_zero_bit(cpu_index_map, max_cpus);
> > +
> > +    if (cpu == max_cpus) {
> > +        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
> > +                    max_cpus);
> > +        return max_cpus;
> > +    } else {
> > +        bitmap_set(cpu_index_map, cpu, 1);
> > +        return cpu;
> > +    }
> > +}
> > +
> > +void cpu_exec_exit(CPUState *cpu)
> > +{
> > +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > +}
> 
> AFAICT, this function is never called, which seems like a bug.

It is called in subsequent patch. If you are suggesting that a function
shouldn't be defined in a patch where it not used, I can move this
down to the other patch.

Regards,
Bharata.