[LinuxBIOS] PATCH: nested spinlock hang when initializing x86 sibling CPUs

[Steven J. Magnani]

This patch resolves the issue identified in
http://openbios.org/pipermail/linuxbios/2005-July/011903.html.

Changes:
* Remove spinlock bracketing of cpu_initialize() call
* Drop SERIAL_CPU_INIT option
* (Unrelated) Attempt more descriptive comments for various config
options

PATCH:

--- src/cpu/x86/lapic/lapic_cpu_init.c.orig	2005-09-09
09:29:25.156250000 -0500
+++ src/cpu/x86/lapic/lapic_cpu_init.c	2005-09-09 09:31:59.109375000
-0500
@@ -230,17 +230,16 @@
 void secondary_cpu_init(void)
 {
    atomic_inc(&active_cpus);
-#if SERIAL_CPU_INIT == 1
-  #if CONFIG_MAX_CPUS>2
-	spin_lock(&start_cpu_lock);
-  #endif
-#endif
+
+	// NOTE: The call to cpu_initialize() used to be bracketed by 
+	//       calls to lock and unlock the start_cpu_lock spin lock.
+	//       These were removed to resolve a hang due to nested spin
locking
+	//       when secondary CPUs go to initialize their siblings.
+	//       See
http://openbios.org/pipermail/linuxbios/2005-July/011903.html.
+	//       It's possible that some other form of concurrency
control 
+	//       is needed at this level.
    cpu_initialize();
-#if SERIAL_CPU_INIT == 1
-  #if CONFIG_MAX_CPUS>2
-	spin_unlock(&start_cpu_lock);
-  #endif
-#endif
+
    atomic_dec(&active_cpus);
    stop_this_cpu();
 }
@@ -265,11 +264,9 @@
    		printk_err("CPU  %u would not start!\n",
    			cpu->path.u.apic.apic_id);
    	}
-#if SERIAL_CPU_INIT == 1
   #if CONFIG_MAX_CPUS>2
    	udelay(10);
   #endif
-#endif
    }

/* Now loop until the other cpus have finished initializing */
--- src/config/Options.lb.orig	2005-09-09 09:36:27.562500000 -0500
+++ src/config/Options.lb	2005-09-09 09:35:17.125000000 -0500
@@ -188,37 +188,37 @@
    default 65536
    format "0x%x"
    export used
-	comment "Default fallback image size"
+	comment "ROM_SECTION_SIZE to use for the fallback build."
 end
 define ROM_SIZE
    default none
    format "0x%x"
    export used
-	comment "Size of your ROM"
+	comment "Total number of bytes allocated for normal and fallback
LinuxBIOS images and payloads. Note that the fallback image goes at the
end of the ROM, and the normal image at the beginning."
 end
 define ROM_IMAGE_SIZE
    default 65535
    format "0x%x"
    export always
-	comment "Default image size"
+	comment "Maximum number of bytes allowed for a LinuxBIOS image.
Does not include the payload."
 end
 define ROM_SECTION_SIZE
    default {FALLBACK_SIZE}
    format "0x%x"
    export used
-	comment "Default rom section size"
+	comment "Default rom section size. Normally, this is calculated
in mainboard Config.lb and varies between the normal and fallback
builds."
 end
 define ROM_SECTION_OFFSET
    default {ROM_SIZE - FALLBACK_SIZE}
    format "0x%x"
    export used
-	comment "Default rom section offset"
+	comment "Number of bytes from the beginning of the ROM to the
start of the section containing this build (normal or fallback).
Normally, this is calculated in mainboard Config.lb."
 end
 define PAYLOAD_SIZE
    default {ROM_SECTION_SIZE - ROM_IMAGE_SIZE}
    format "0x%x"
    export always
-	comment "Default payload size"
+	comment "Maximum number of bytes allowed for a payload.
Normally, this is calculated as above."
 end
 define _ROMBASE
    default {PAYLOAD_SIZE}
@@ -479,7 +479,7 @@
    export used
    comment "System clock frequency in MHz"
 end
-
+
 ###############################################
 # SMP options
 ###############################################
@@ -509,11 +509,6 @@
    export used
    comment "Define to build an MP table"
 end
-define SERIAL_CPU_INIT
-        default 1
-        export always
-        comment "Serialize CPU init"
-end

###############################################
 # Boot options
@@ -533,7 +528,7 @@
    default {0xffffffff - ROM_SIZE + ROM_SECTION_OFFSET + 1}
    format "0x%x"
    export always
-	comment "ROM stream start location"
+	comment "Memory address of this (normal or fallback) build's
payload in ROM. Normally, this is calculated as above."
 end
 define CONFIG_FS_STREAM
    default 0

--- src/mainboard/tyan/s2735/Options.lb.orig	2005-09-08
13:35:22.453125000 -0500
+++ src/mainboard/tyan/s2735/Options.lb	2005-09-09 09:21:21.531250000
-0500
@@ -8,7 +8,6 @@
 uses CONFIG_MAX_CPUS
 uses CONFIG_MAX_PHYSICAL_CPUS
 uses CONFIG_LOGICAL_CPUS
-uses SERIAL_CPU_INIT
 uses CONFIG_IOAPIC
 uses CONFIG_SMP
 uses FALLBACK_SIZE
@@ -127,8 +126,6 @@
 default CONFIG_MAX_PHYSICAL_CPUS=2
 default CONFIG_LOGICAL_CPUS=1

-default SERIAL_CPU_INIT=0
-
 #BTEXT Console
 #default CONFIG_CONSOLE_BTEXT=1

------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

#include <standard.disclaimer>

[Richard Smith]

On 9/9/05, Steven J. Magnani  wrote:
> This patch resolves the issue identified in
> http://openbios.org/pipermail/linuxbios/2005-July/011903.html.

Does LinuxBIOS need a bug tracker?

That might make references like this a little easier rather than
haveing to prowl through the mailing archive for the right thread.

[yhlu]

You need to keep SERIAL_CPU_INIT.

reason is for AMD K8 cpu_init regarding init_ecc there maybe some conflict 
on console.

You can disable it by set it to 0.

YH

On 9/9/05, Steven J. Magnani  wrote:
> This patch resolves the issue identified in
> http://openbios.org/pipermail/linuxbios/2005-July/011903.html.
>
> Changes:
> Remove spinlock bracketing of cpu_initialize() call
> Drop SERIAL_CPU_INIT option
> (Unrelated) Attempt more descriptive comments for various config
>
> options
>
> PATCH:
>
> --- src/cpu/x86/lapic/lapic_cpu_init.c.orig 2005-09-09
> 09:29:25.156250000 -0500
> +++ src/cpu/x86/lapic/lapic_cpu_init.c 2005-09-09 09:31:59.109375000
> -0500
> @@ -230,17 +230,16 @@
> void secondary_cpu_init(void)
> {
> atomic_inc(&active_cpus);
> -#if SERIAL_CPU_INIT == 1
> #if CONFIG_MAX_CPUS>2
> spin_lock(&start_cpu_lock);
> #endif
>
> -#endif
> // NOTE: The call to cpu_initialize() used to be bracketed by
> // calls to lock and unlock the start_cpu_lock spin lock.
> // These were removed to resolve a hang due to nested spin
>
> locking
> // when secondary CPUs go to initialize their siblings.
> // See
>
> http://openbios.org/pipermail/linuxbios/2005-July/011903.html.
> // It's possible that some other form of concurrency
>
> control
> // is needed at this level.
>
> cpu_initialize();
> -#if SERIAL_CPU_INIT == 1
> #if CONFIG_MAX_CPUS>2
> spin_unlock(&start_cpu_lock);
> #endif
>
> -#endif
>
> atomic_dec(&active_cpus);
> stop_this_cpu();
> }
> @@ -265,11 +264,9 @@
> printk_err("CPU %u would not start!\n",
> cpu->path.u.apic.apic_id);
> }
> -#if SERIAL_CPU_INIT == 1
> #if CONFIG_MAX_CPUS>2
> udelay(10);
> #endif
> -#endif
> }
>
> /* Now loop until the other cpus have finished initializing */
> --- src/config/Options.lb.orig 2005-09-09 09:36:27.562500000 -0500
> +++ src/config/Options.lb 2005-09-09 09:35:17.125000000 -0500
> @@ -188,37 +188,37 @@
> default 65536
> format "0x%x"
> export used
> comment "Default fallback image size"
> comment "ROM_SECTION_SIZE to use for the fallback build."
>
> end
> define ROM_SIZE
> default none
> format "0x%x"
> export used
> comment "Size of your ROM"
> comment "Total number of bytes allocated for normal and fallback
>
> LinuxBIOS images and payloads. Note that the fallback image goes at the
> end of the ROM, and the normal image at the beginning."
> end
> define ROM_IMAGE_SIZE
> default 65535
> format "0x%x"
> export always
> comment "Default image size"
> comment "Maximum number of bytes allowed for a LinuxBIOS image.
>
> Does not include the payload."
> end
> define ROM_SECTION_SIZE
> default {FALLBACK_SIZE}
> format "0x%x"
> export used
> comment "Default rom section size"
> comment "Default rom section size. Normally, this is calculated
>
> in mainboard Config.lb http://Config.lb and varies between the normal 
> and fallback
> builds."
> end
> define ROM_SECTION_OFFSET
> default {ROM_SIZE - FALLBACK_SIZE}
> format "0x%x"
> export used
> comment "Default rom section offset"
> comment "Number of bytes from the beginning of the ROM to the
>
> start of the section containing this build (normal or fallback).
> Normally, this is calculated in mainboard Config.lb http://Config.lb."
> end
> define PAYLOAD_SIZE
> default {ROM_SECTION_SIZE - ROM_IMAGE_SIZE}
> format "0x%x"
> export always
> comment "Default payload size"
> comment "Maximum number of bytes allowed for a payload.
>
> Normally, this is calculated as above."
> end
> define _ROMBASE
> default {PAYLOAD_SIZE}
> @@ -479,7 +479,7 @@
> export used
> comment "System clock frequency in MHz"
> end
>
> ###############################################
> # SMP options
> ###############################################
> @@ -509,11 +509,6 @@
> export used
> comment "Define to build an MP table"
> end
> -define SERIAL_CPU_INIT
> default 1
> export always
> comment "Serialize CPU init"
>
> -end
>
> ###############################################
> # Boot options
> @@ -533,7 +528,7 @@
> default {0xffffffff - ROM_SIZE + ROM_SECTION_OFFSET + 1}
> format "0x%x"
> export always
> comment "ROM stream start location"
> comment "Memory address of this (normal or fallback) build's
>
> payload in ROM. Normally, this is calculated as above."
> end
> define CONFIG_FS_STREAM
> default 0
>
> --- src/mainboard/tyan/s2735/Options.lb.orig 2005-09-08
> 13:35:22.453125000 -0500
> +++ src/mainboard/tyan/s2735/Options.lb 2005-09-09 09:21:21.531250000
> -0500
> @@ -8,7 +8,6 @@
> uses CONFIG_MAX_CPUS
> uses CONFIG_MAX_PHYSICAL_CPUS
> uses CONFIG_LOGICAL_CPUS
> -uses SERIAL_CPU_INIT
> uses CONFIG_IOAPIC
> uses CONFIG_SMP
> uses FALLBACK_SIZE
> @@ -127,8 +126,6 @@
> default CONFIG_MAX_PHYSICAL_CPUS=2
> default CONFIG_LOGICAL_CPUS=1
>
> -default SERIAL_CPU_INIT=0
>
> #BTEXT Console
> #default CONFIG_CONSOLE_BTEXT=1
>
> Steven J. Magnani "I claim this network for MARS!
> www.digidescorp.com http://www.digidescorp.com Earthling, return my 
> space modulator!"
>
> #include <standard.disclaimer>

[Steven J. Magnani]

Based on the mainboard config files I see, it looks like the only
purpose of SERIAL_CPU_INIT is to work around the nested spinlock issue
with Intel hyperthreaded CPUs. I don't think this is the best solution
to the problem. To me, 'SERIAL_CPU_INIT' implies a free choice - I can
choose whether my CPUs are initialized serially or concurrently, and
either choice is acceptable. But that's not the way it works; making the
wrong choice on Intel systems causes the system to hang.

I think a better solution will do away with SERIAL_CPU_INIT, but might
keep the spinlock bracket around cpu_initialize(). The question is how
to reconcile the differences between AMD and Intel SMP startup. Can you
provide any suggestions from the AMD viewpoint?

Steve

-----Original Message-----
From: yhlu [mailto:[e-mail]] 
Sent: Friday, September 09, 2005 10:01 AM
To: Steven J. Magnani
Cc: [e-mail]
Subject: Re: [LinuxBIOS] PATCH: nested spinlock hang when initializing
x86 sibling CPUs

You need to keep SERIAL_CPU_INIT.

reason is for AMD K8 cpu_init regarding init_ecc there maybe some
conflict on console.

You can disable it by set it to 0.

YH

On 9/9/05, Steven J. Magnani  wrote:

This patch resolves the issue identified in
http://openbios.org/pipermail/linuxbios/2005-July/011903.html.

Changes:
* Remove spinlock bracketing of cpu_initialize() call 
* Drop SERIAL_CPU_INIT option
* (Unrelated) Attempt more descriptive comments for various config
options

PATCH:

--- src/cpu/x86/lapic/lapic_cpu_init.c.orig     2005-09-09
09:29:25.156250000 -0500 
+++ src/cpu/x86/lapic/lapic_cpu_init.c  2005-09-09 09:31:59.109375000
-0500
@@ -230,17 +230,16 @@
void secondary_cpu_init(void)
{
        atomic_inc(&active_cpus);
-#if SERIAL_CPU_INIT == 1
-  #if CONFIG_MAX_CPUS>2
-       spin_lock(&start_cpu_lock);
-  #endif
-#endif
+
+       // NOTE: The call to cpu_initialize() used to be bracketed by
+       //       calls to lock and unlock the start_cpu_lock spin lock.
+       //       These were removed to resolve a hang due to nested spin
locking
+       //       when secondary CPUs go to initialize their siblings.
+       //       See
http://openbios.org/pipermail/linuxbios/2005-July/011903.html.
+       //       It's possible that some other form of concurrency 
control
+       //       is needed at this level.
        cpu_initialize();
-#if SERIAL_CPU_INIT == 1
-  #if CONFIG_MAX_CPUS>2
-       spin_unlock(&start_cpu_lock);
-  #endif
-#endif
+
        atomic_dec(&active_cpus);
        stop_this_cpu();
}
@@ -265,11 +264,9 @@
                        printk_err("CPU  %u would not start!\n",
                                cpu->path.u.apic.apic_id);
                }
-#if SERIAL_CPU_INIT == 1
   #if CONFIG_MAX_CPUS>2
                udelay(10);
   #endif
-#endif 
        }

/* Now loop until the other cpus have finished initializing */
--- src/config/Options.lb.orig  2005-09-09 09:36:27.562500000 -0500
+++ src/config/Options.lb       2005-09-09 09:35:17.125000000 -0500
@@ -188,37 +188,37 @@
        default 65536
        format "0x%x"
        export used
-       comment "Default fallback image size"
+       comment "ROM_SECTION_SIZE to use for the fallback build." 
end
define ROM_SIZE
        default none
        format "0x%x"
        export used
-       comment "Size of your ROM"
+       comment "Total number of bytes allocated for normal and fallback

LinuxBIOS images and payloads. Note that the fallback image goes at the
end of the ROM, and the normal image at the beginning."
end
define ROM_IMAGE_SIZE
        default 65535
        format "0x%x" 
        export always
-       comment "Default image size"
+       comment "Maximum number of bytes allowed for a LinuxBIOS image.
Does not include the payload."
end
define ROM_SECTION_SIZE 
        default {FALLBACK_SIZE}
        format "0x%x"
        export used
-       comment "Default rom section size"
+       comment "Default rom section size. Normally, this is calculated 
in mainboard Config.lb and varies between the normal and fallback
builds."
end
define ROM_SECTION_OFFSET
        default {ROM_SIZE - FALLBACK_SIZE}
        format "0x%x" 
        export used
-       comment "Default rom section offset"
+       comment "Number of bytes from the beginning of the ROM to the
start of the section containing this build (normal or fallback). 
Normally, this is calculated in mainboard Config.lb."
end
define PAYLOAD_SIZE
        default {ROM_SECTION_SIZE - ROM_IMAGE_SIZE}
        format "0x%x"
        export always 
-       comment "Default payload size"
+       comment "Maximum number of bytes allowed for a payload.
Normally, this is calculated as above."
end
define _ROMBASE
        default {PAYLOAD_SIZE} 
@@ -479,7 +479,7 @@
        export used
        comment "System clock frequency in MHz"
end
-
+
###############################################
# SMP options
############################################### 
@@ -509,11 +509,6 @@
        export used
        comment "Define to build an MP table"
end
-define SERIAL_CPU_INIT
-        default 1
-        export always
-        comment "Serialize CPU init" 
-end

###############################################
# Boot options
@@ -533,7 +528,7 @@
        default {0xffffffff - ROM_SIZE + ROM_SECTION_OFFSET + 1}
        format "0x%x"
        export always 
-       comment "ROM stream start location"
+       comment "Memory address of this (normal or fallback) build's
payload in ROM. Normally, this is calculated as above."
end
define CONFIG_FS_STREAM 
        default 0

--- src/mainboard/tyan/s2735/Options.lb.orig    2005-09-08
13:35:22.453125000 -0500
+++ src/mainboard/tyan/s2735/Options.lb 2005-09-09 09:21:21.531250000
-0500
@@ -8,7 +8,6 @@
uses CONFIG_MAX_CPUS
uses CONFIG_MAX_PHYSICAL_CPUS
uses CONFIG_LOGICAL_CPUS
-uses SERIAL_CPU_INIT
uses CONFIG_IOAPIC
uses CONFIG_SMP
uses FALLBACK_SIZE
@@ -127,8 +126,6 @@
default CONFIG_MAX_PHYSICAL_CPUS=2 
default CONFIG_LOGICAL_CPUS=1

-default SERIAL_CPU_INIT=0
-
#BTEXT Console
#default CONFIG_CONSOLE_BTEXT=1

------------------------------------------------------------------------
Steven J. Magnani               "I claim this network for MARS!
www.digidescorp.com              Earthling, return my space modulator!"

#include <standard.disclaimer>