diff mbox series

x86/smp: Validate APIC ID before parking CPU in INIT

Message ID 20230719051302.15801-1-vasant.hegde@amd.com
State New
Headers
Series x86/smp: Validate APIC ID before parking CPU in INIT |

Commit Message

Vasant Hegde July 19, 2023, 5:13 a.m. UTC 
  Below commit is causing kexec to hang in certain scenarios with >255 CPUs.

Reproduce steps:
  - We are using 2 socket system with 384 CPUs
  - Booting first kernel with kernel command line intremap=off
    This disabled x2apic in kernel and booted with apic mode
  - During kexec it tries to send INIT to all CPUs except boot CPU
    If APIC ID is 0x100 (like in our case) then it will send CPU0
    to INIT mode and system hangs (in APIC mode DEST field is 8bit)

Fix this issue by adding apic->apic_id_valid() check before sending
INIT sequence.

Fixes: 45e34c8af58f ("x86/smp: Put CPUs into INIT on shutdown if possible")
Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 arch/x86/kernel/smpboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vasant Hegde Aug. 3, 2023, 4:28 p.m. UTC | #1 
Hi

Did you get a chance to look into this patch?


-Vasant

On 7/19/2023 10:43 AM, Vasant Hegde wrote:
> Below commit is causing kexec to hang in certain scenarios with >255 CPUs.
> 
> Reproduce steps:
>   - We are using 2 socket system with 384 CPUs
>   - Booting first kernel with kernel command line intremap=off
>     This disabled x2apic in kernel and booted with apic mode
>   - During kexec it tries to send INIT to all CPUs except boot CPU
>     If APIC ID is 0x100 (like in our case) then it will send CPU0
>     to INIT mode and system hangs (in APIC mode DEST field is 8bit)
> 
> Fix this issue by adding apic->apic_id_valid() check before sending
> INIT sequence.
> 
> Fixes: 45e34c8af58f ("x86/smp: Put CPUs into INIT on shutdown if possible")
> Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  arch/x86/kernel/smpboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index e1aa2cd7734b..e5ca0689c4dd 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1360,7 +1360,7 @@ bool smp_park_other_cpus_in_init(void)
>  		if (cpu == this_cpu)
>  			continue;
>  		apicid = apic->cpu_present_to_apicid(cpu);
> -		if (apicid == BAD_APICID)
> +		if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
>  			continue;
>  		send_init_sequence(apicid);
>  	}
Thomas Gleixner Aug. 9, 2023, 6:42 p.m. UTC | #2 
On Wed, Jul 19 2023 at 05:13, Vasant Hegde wrote:
> Below commit is causing kexec to hang in certain scenarios with >255 CPUs.
>
> Reproduce steps:
>   - We are using 2 socket system with 384 CPUs
>   - Booting first kernel with kernel command line intremap=off
>     This disabled x2apic in kernel and booted with apic mode
>   - During kexec it tries to send INIT to all CPUs except boot CPU
>     If APIC ID is 0x100 (like in our case) then it will send CPU0
>     to INIT mode and system hangs (in APIC mode DEST field is 8bit)

It took me a while to decode the above.

> Fix this issue by adding apic->apic_id_valid() check before sending
> INIT sequence.

Sigh, yes.

> Fixes: 45e34c8af58f ("x86/smp: Put CPUs into INIT on shutdown if possible")
> Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  arch/x86/kernel/smpboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index e1aa2cd7734b..e5ca0689c4dd 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1360,7 +1360,7 @@ bool smp_park_other_cpus_in_init(void)
>  		if (cpu == this_cpu)
>  			continue;
>  		apicid = apic->cpu_present_to_apicid(cpu);
> -		if (apicid == BAD_APICID)
> +		if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
>  			continue;
>  		send_init_sequence(apicid);
>  	}
Thomas Gleixner Aug. 9, 2023, 6:52 p.m. UTC | #3 
On Wed, Aug 09 2023 at 20:42, Thomas Gleixner wrote:
> On Wed, Jul 19 2023 at 05:13, Vasant Hegde wrote:
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index e1aa2cd7734b..e5ca0689c4dd 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1360,7 +1360,7 @@ bool smp_park_other_cpus_in_init(void)
>>  		if (cpu == this_cpu)
>>  			continue;
>>  		apicid = apic->cpu_present_to_apicid(cpu);
>> -		if (apicid == BAD_APICID)
>> +		if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
>>  			continue;
>>  		send_init_sequence(apicid);
>>  	}

I think this papers over the underlying problem that this sends INIT to
an APIC which was never booted. The below is curing the root cause.

Thanks,

        tglx
---
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1356,7 +1356,7 @@ bool smp_park_other_cpus_in_init(void)
 	if (this_cpu)
 		return false;
 
-	for_each_present_cpu(cpu) {
+	for_each_cpu_and(cpu, &cpus_booted_once_mask, cpu_present_mask) {
 		if (cpu == this_cpu)
 			continue;
 		apicid = apic->cpu_present_to_apicid(cpu);
Vasant Hegde Aug. 10, 2023, 11:26 a.m. UTC | #4 
Hi,


On 8/10/2023 12:22 AM, Thomas Gleixner wrote:
> On Wed, Aug 09 2023 at 20:42, Thomas Gleixner wrote:
>> On Wed, Jul 19 2023 at 05:13, Vasant Hegde wrote:
>>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>>> index e1aa2cd7734b..e5ca0689c4dd 100644
>>> --- a/arch/x86/kernel/smpboot.c
>>> +++ b/arch/x86/kernel/smpboot.c
>>> @@ -1360,7 +1360,7 @@ bool smp_park_other_cpus_in_init(void)
>>>  		if (cpu == this_cpu)
>>>  			continue;
>>>  		apicid = apic->cpu_present_to_apicid(cpu);
>>> -		if (apicid == BAD_APICID)
>>> +		if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
>>>  			continue;
>>>  		send_init_sequence(apicid);
>>>  	}
> 
> I think this papers over the underlying problem that this sends INIT to
> an APIC which was never booted. The below is curing the root cause.

I have tested below patch and it fixes the issue. Thanks

Tested-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

> 
> Thanks,
> 
>         tglx
> ---
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1356,7 +1356,7 @@ bool smp_park_other_cpus_in_init(void)
>  	if (this_cpu)
>  		return false;
>  
> -	for_each_present_cpu(cpu) {
> +	for_each_cpu_and(cpu, &cpus_booted_once_mask, cpu_present_mask) {
>  		if (cpu == this_cpu)
>  			continue;
>  		apicid = apic->cpu_present_to_apicid(cpu);
diff mbox series

Patch

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index e1aa2cd7734b..e5ca0689c4dd 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1360,7 +1360,7 @@  bool smp_park_other_cpus_in_init(void)
 		if (cpu == this_cpu)
 			continue;
 		apicid = apic->cpu_present_to_apicid(cpu);
-		if (apicid == BAD_APICID)
+		if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
 			continue;
 		send_init_sequence(apicid);
 	}