x86/cpu: identify uninitialized CPU data using BAD_APICID

[PATCH] x86/cpu: identify uninitialized CPU data using BAD_APICID

Posted by Roger Pau Monne 2 weeks, 5 days ago

Uninitialized cpu_data[] entries have the apicid field set to BAD_APICID,
not boot_cpu_data.apicid.  Fix the check in cpu_smpboot_free() to use the
correct condition.

Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/smpboot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 491cbbba33ae..ff05955bae40 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -961,8 +961,7 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
      * In that case the socket number cannot be relied upon, but the respective
      * socket_cpumask[] slot also wouldn't have been set.
      */
-    if ( c[cpu].apicid != boot_cpu_data.apicid &&
-         cpumask_empty(socket_cpumask[socket]) )
+    if ( c[cpu].apicid != BAD_APICID && cpumask_empty(socket_cpumask[socket]) )
     {
         xfree(socket_cpumask[socket]);
         socket_cpumask[socket] = NULL;
-- 
2.53.0

Re: [PATCH] x86/cpu: identify uninitialized CPU data using BAD_APICID

Posted by Jan Beulich 2 weeks, 1 day ago

On 30.04.2026 13:46, Roger Pau Monne wrote:
> Uninitialized cpu_data[] entries have the apicid field set to BAD_APICID,
> not boot_cpu_data.apicid.  Fix the check in cpu_smpboot_free() to use the
> correct condition.
> 
> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")

I think this isn't correct, and the issue here is me having overlooked a
dependency between that commit and the one introducing the line which
you change: 8c15d3d18725 ("x86/SMP: guard socket_cpumask[] access in
cpu_smpboot_free()"). Both changes were committed close together, but
they were entirely separate submissions (well over a year apart). Hence
as an individual patch that latter commit was correct, but on top of
7126b7f806d5 it would have needed adjustment. Since 7126b7f806d5 wasn't
backported (while 8c15d3d18725 was), the fix here also shouldn't be
backported to anything earlier than 4.21 (which indeed the Fixes: tag
better expresses).

Jan

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -961,8 +961,7 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>       * In that case the socket number cannot be relied upon, but the respective
>       * socket_cpumask[] slot also wouldn't have been set.
>       */
> -    if ( c[cpu].apicid != boot_cpu_data.apicid &&
> -         cpumask_empty(socket_cpumask[socket]) )
> +    if ( c[cpu].apicid != BAD_APICID && cpumask_empty(socket_cpumask[socket]) )
>      {
>          xfree(socket_cpumask[socket]);
>          socket_cpumask[socket] = NULL;

Re: [PATCH] x86/cpu: identify uninitialized CPU data using BAD_APICID

Posted by Roger Pau Monné 2 weeks, 1 day ago

On Mon, May 04, 2026 at 01:31:34PM +0200, Jan Beulich wrote:
> On 30.04.2026 13:46, Roger Pau Monne wrote:
> > Uninitialized cpu_data[] entries have the apicid field set to BAD_APICID,
> > not boot_cpu_data.apicid.  Fix the check in cpu_smpboot_free() to use the
> > correct condition.
> > 
> > Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
> 
> I think this isn't correct, and the issue here is me having overlooked a
> dependency between that commit and the one introducing the line which
> you change: 8c15d3d18725 ("x86/SMP: guard socket_cpumask[] access in
> cpu_smpboot_free()"). Both changes were committed close together, but
> they were entirely separate submissions (well over a year apart). Hence
> as an individual patch that latter commit was correct, but on top of
> 7126b7f806d5 it would have needed adjustment. Since 7126b7f806d5 wasn't
> backported (while 8c15d3d18725 was), the fix here also shouldn't be
> backported to anything earlier than 4.21 (which indeed the Fixes: tag
> better expresses).

I see, right, in the git history 8c15d3d18725 is indeed later than
7126b7f806d5, and hence 7126b7f806d5 was correct when introduced.  The
bug in cpu_smpboot_free() was there, but 7126b7f806d5 made it no
worse.  It was 8c15d3d18725 that would indeed need to be adjusted to
pick up the new initialization value.

Thanks, Roger.

Re: [PATCH] x86/cpu: identify uninitialized CPU data using BAD_APICID

Posted by Andrew Cooper 2 weeks, 5 days ago

On 30/04/2026 12:46 pm, Roger Pau Monne wrote:
> Uninitialized cpu_data[] entries have the apicid field set to BAD_APICID,
> not boot_cpu_data.apicid.  Fix the check in cpu_smpboot_free() to use the
> correct condition.
>
> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/smpboot.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 491cbbba33ae..ff05955bae40 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -961,8 +961,7 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>       * In that case the socket number cannot be relied upon, but the respective
>       * socket_cpumask[] slot also wouldn't have been set.
>       */
> -    if ( c[cpu].apicid != boot_cpu_data.apicid &&
> -         cpumask_empty(socket_cpumask[socket]) )
> +    if ( c[cpu].apicid != BAD_APICID && cpumask_empty(socket_cpumask[socket]) )
>      {
>          xfree(socket_cpumask[socket]);
>          socket_cpumask[socket] = NULL;

Yet more fallout from that change...

This is the singular use of apicid.  We should consider removing it, but
that's a change for later.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] x86/cpu: identify uninitialized CPU data using BAD_APICID

Posted by Teddy Astie 2 weeks, 5 days ago

Le 30/04/2026 à 13:47, Roger Pau Monne a écrit :
> Uninitialized cpu_data[] entries have the apicid field set to BAD_APICID,
> not boot_cpu_data.apicid.  Fix the check in cpu_smpboot_free() to use the
> correct condition.
> 
> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   xen/arch/x86/smpboot.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 491cbbba33ae..ff05955bae40 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -961,8 +961,7 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>        * In that case the socket number cannot be relied upon, but the respective
>        * socket_cpumask[] slot also wouldn't have been set.
>        */
> -    if ( c[cpu].apicid != boot_cpu_data.apicid &&
> -         cpumask_empty(socket_cpumask[socket]) )
> +    if ( c[cpu].apicid != BAD_APICID && cpumask_empty(socket_cpumask[socket]) )
>       {
>           xfree(socket_cpumask[socket]);
>           socket_cpumask[socket] = NULL;

Reviewed-by: Teddy Astie <teddy.astie@vates.tech>


-- 
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech