Re: [Qemu-ppc] [PATCH v2 3/6] exec: set cpu_index only if it's not been explictly set

[Eduardo Habkost]

On Mon, Jul 25, 2016 at 11:59:21AM +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/delr, which
> will be altered by follow up patches to set stable
> cpu_index manually.

So, cpu_get_free_index() behavior is exactly the same because
cpu-remove is either unsupported, or only supported for the last
CPU. But I worry that this will easily break if anybody starts
implementing CPU removal in other machines without setting
cpu_index explicitly in the board code. Then we can make
cpu_get_free_index() generate a duplicate cpu_index.

I wonder if there any way we can add an assert() somewhere to
ensure no machine will ever allow CPU removal while not
initializing cpu_index explicitly.

(This shouldn't hold this patch, it's just a suggestion for a
possible follow-up patch).

Additional comment:

> 
> Signed-off-by: Igor Mammedov <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
[...]
> -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;
> -    }

We are now relying on the rest of the QEMU code to make sure
cpu_index will be always < MAX_CPUMASK_BITS. In this case, I
suggest we add an assert() below:

[...]
> -    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);
>      }

Here:
  assert(cpu->cpu_index <= MAX_CPUMASK_BITS)


Both comments can be addressed in a follow-up patch, so:

Reviewed-by: Eduardo Habkost <address@hidden>

-- 
Eduardo