Re: [PATCH v1 5/8] cpus-common: ensure auto-assigned cpu_indexes don't clash

["Alex Bennée" <alex.bennee@linaro.org>]

Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 14 May 2020 17:27:53 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
>
>> a
>> Alex Bennée <alex.bennee@linaro.org> writes:
>> 
>> > Basing the cpu_index on the number of currently allocated vCPUs fails
>> > when vCPUs aren't removed in a LIFO manner. This is especially true
>> > when we are allocating a cpu_index for each guest thread in
>> > linux-user where there is no ordering constraint on their allocation
>> > and de-allocation.
>> >
>> > [I've dropped the assert which is there to guard against out-of-order
>> > removal as this should probably be caught higher up the stack. Maybe
>> > we could just ifdef CONFIG_SOFTTMU it?]
>
> for machines where we care about cross version migration (arm/virt,s390,x86,spapr),
> we do manual cpu_index assignment on keep control on its stability
> So orderining probably shouldn't matter for other softmmu boards,
> but what I'd watch for is arrays within devices where cpu_index is
> used as index

With the updated version for softmmu you should get the same indexes as
before from startup. It only gets complicated if CPU hotplug is a thing
which I think is only the case for machines that also support migration?

> (ex: would be apic emulation (but its not affected by this patch since x86 control
> cpu_index assignment))

I've just noticed that the gdbstub uses the maximum cpu_index at startup
to size it's array in CONFIG_USER which is obviously wrong but it was
wrong before so I guess that's another bug to look into on my part :-/

>
>
>> >
>> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> > Cc: Nikolay Igotti <igotti@gmail.com>
>> > Cc: Paolo Bonzini <pbonzini@redhat.com>
>> > Cc: Igor Mammedov <imammedo@redhat.com>
>> > Cc: Eduardo Habkost <ehabkost@redhat.com>
>> > ---
>> >  cpus-common.c | 9 ++++-----
>> >  1 file changed, 4 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/cpus-common.c b/cpus-common.c
>> > index 55d5df89237..5a7d2f6132b 100644
>> > --- a/cpus-common.c
>> > +++ b/cpus-common.c
>> > @@ -61,13 +61,14 @@ static bool cpu_index_auto_assigned;
>> >  static int cpu_get_free_index(void)
>> >  {
>> >      CPUState *some_cpu;
>> > -    int cpu_index = 0;
>> > +    int max_cpu_index = 0;
>> >  
>> >      cpu_index_auto_assigned = true;
>> >      CPU_FOREACH(some_cpu) {
>> > -        cpu_index++;
>> > +        max_cpu_index = MAX(some_cpu->cpu_index, max_cpu_index);
>> >      }
>> > -    return cpu_index;
>> > +    max_cpu_index++;
>> > +    return max_cpu_index;
>> >  }  
>> 
>> OK some ending up with cpu_index = 1 threw off devices that would do
>> qemu_get_cpu(0) so I've tweaked the algorithm to:
>> 
>>   static int cpu_get_free_index(void)
>>   {
>>       CPUState *some_cpu;
>>       int max_cpu_index = 0;
>> 
>>       cpu_index_auto_assigned = true;
>>       CPU_FOREACH(some_cpu) {
>>           if (some_cpu->cpu_index >= max_cpu_index) {
>>               max_cpu_index = some_cpu->cpu_index + 1;
>>           }
>>       }
>>       return max_cpu_index;
>>   }
>> 
>> >  
>> >  void cpu_list_add(CPUState *cpu)
>> > @@ -90,8 +91,6 @@ void cpu_list_remove(CPUState *cpu)
>> >          return;
>> >      }
>> >  
>> > -    assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus)));
>> > -
>> >      QTAILQ_REMOVE_RCU(&cpus, cpu, node);
>> >      cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>> >  }  
>> 
>> 


-- 
Alex Bennée