Re: [Qemu-devel] [PATCH 3/8] exec: set cpu_index only if it's been explictly set

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Jul 21, 2016 at 05:54:34PM +0200, Igor Mammedov wrote:
> it keeps the legacy behavior for all users that doesn't care
> about stable cpu_index value, but would allow boards that
> would support device_add/device_del to set stable cpu_index
> that won't depend on order in which cpus are created/destroyed.
> 
> While at that simplify cpu_get_free_index() as cpu_index
> generated by USER_ONLY and softmmu variants is the same
> since none of the users support cpu-remove so far except
> of not yet released spapr/x86 device_add/del. which
> will be altered in follow up patches to provide stable
> cpu_index.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

It looks like the 1-line description needs a "not" in it somewhere,
but otherwise:

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

> ---
>  include/qom/cpu.h |  2 ++
>  exec.c            | 44 ++++++--------------------------------------
>  qom/cpu.c         |  2 +-
>  3 files changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index cbcd64c..ce0c406 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -883,4 +883,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
>      .offset = 0,                                                            \
>  }
>  
> +#define UNASSIGNED_CPU_INDEX -1
> +
>  #endif
> diff --git a/exec.c b/exec.c
> index 8c5da32..8e8416b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -598,30 +598,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>  }
>  #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_CPUMASK_BITS);
> -
> -    if (cpu >= MAX_CPUMASK_BITS) {
> -        error_setg(errp, "Trying to use more CPUs than max of %d",
> -                   MAX_CPUMASK_BITS);
> -        return -1;
> -    }
> -
> -    bitmap_set(cpu_index_map, cpu, 1);
> -    return cpu;
> -}
> -
> -static void cpu_release_index(CPUState *cpu)
> -{
> -    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> -}
> -#else
> -
> -static int cpu_get_free_index(Error **errp)
> +static int cpu_get_free_index(void)
>  {
>      CPUState *some_cpu;
>      int cpu_index = 0;
> @@ -632,12 +609,6 @@ static int cpu_get_free_index(Error **errp)
>      return cpu_index;
>  }
>  
> -static void cpu_release_index(CPUState *cpu)
> -{
> -    return;
> -}
> -#endif
> -
>  void cpu_exec_exit(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> @@ -650,8 +621,7 @@ void cpu_exec_exit(CPUState *cpu)
>      }
>  
>      QTAILQ_REMOVE(&cpus, cpu, node);
> -    cpu_release_index(cpu);
> -    cpu->cpu_index = -1;
> +    cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>      cpu_list_unlock();
>  
>      if (cc->vmsd != NULL) {
> @@ -665,7 +635,7 @@ void cpu_exec_exit(CPUState *cpu)
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
> -    Error *local_err = NULL;
> +    Error *local_err ATTRIBUTE_UNUSED = NULL;
>  
>      cpu->as = NULL;
>      cpu->num_ases = 0;
> @@ -689,11 +659,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>  #endif
>  
>      cpu_list_lock();
> -    cpu->cpu_index = cpu_get_free_index(&local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        cpu_list_unlock();
> -        return;
> +    if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
> +        cpu->cpu_index = cpu_get_free_index();
> +        assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>      cpu_list_unlock();
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 42b5631..2553247 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -340,7 +340,7 @@ static void cpu_common_initfn(Object *obj)
>      CPUState *cpu = CPU(obj);
>      CPUClass *cc = CPU_GET_CLASS(obj);
>  
> -    cpu->cpu_index = -1;
> +    cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>      qemu_mutex_init(&cpu->work_mutex);
>      QTAILQ_INIT(&cpu->breakpoints);

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]