Igor Mammedov <imamm...@redhat.com> writes:
> On Thu, 14 May 2020 17:27:53 +0100
> Alex Bennée <alex.ben...@linaro.org> wrote:
>
>> a
>> Alex Bennée <alex.ben...@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.ben...@linaro.org>
>> > Cc: Nikolay Igotti <igo...@gmail.com>
>> > Cc: Paolo Bonzini <pbonz...@redhat.com>
>> > Cc: Igor Mammedov <imamm...@redhat.com>
>> > Cc: Eduardo Habkost <ehabk...@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
