Re: [Qemu-devel] [PULL v2 05/22] cpu: Convert cpu_index into a bitmap

[Bharata B Rao]

On Wed, Jul 08, 2015 at 09:07:30PM +0200, Andreas Färber wrote:
> Am 07.07.2015 um 19:16 schrieb Andreas Färber:
> > From: Bharata B Rao <address@hidden>
> > 
> > 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 <address@hidden>
> > Reviewed-by: Eduardo Habkost <address@hidden>
> > Reviewed-by: Igor Mammedov <address@hidden>
> > Reviewed-by: David Gibson <address@hidden>
> > Reviewed-by: Peter Crosthwaite <address@hidden>
> > Acked-by: Paolo Bonzini <address@hidden>
> > Signed-off-by: Peter Crosthwaite <address@hidden>
> > [AF: max_cpus -> MAX_CPUMASK_BITS]
> > Signed-off-by: Andreas Färber <address@hidden>
> > ---
> >  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 8abac69..02602b6 100644
> > --- a/exec.c
> > +++ b/exec.c
> [...]
> > @@ -542,11 +587,11 @@ void cpu_exec_init(CPUArchState *env, Error **errp)
> >  #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);
> 
> This is lacking a matching cpu_list_unlock() in the CONFIG_USER_ONLY case:
> 
> diff --git a/exec.c b/exec.c
> index ee5bf7c..d817e5f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -590,6 +590,9 @@ void cpu_exec_init(CPUArchState *env, Error **errp)
>      cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> +#if defined(CONFIG_USER_ONLY)
> +        cpu_list_unlock();
> +#endif
>          return;
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
> 

Sorry about this breakage, I should have been more careful.

If this doesn't involve additional effort, feel free to drop these 3 patches,
I will rework them in the next cycle.

Regards,
Bharata.