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?]

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;
 }
 
 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;
 }
-- 
2.20.1

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?]
>
> 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

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
(ex: would be apic emulation (but its not affected by this patch since x86 control
cpu_index assignment))


> >
> > 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;
> >  }  
> 
> 

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

On Thu, 21 May 2020 18:10:40 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> 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?
I'd think so, and those do not (should not) use cpu_get_free_index(), so

Acked-by: Igor Mammedow <imammedo@redhat.com>

> 
> > (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;
> >> >  }    
> >> 
> >>   
> 
>