Re: [PATCH v9 0/8] Parallel CPU bringup for x86_64

From: David Woodhouse <dwmw2@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>,
	Oleksandr Natalenko <oleksandr@natalenko.name>
Cc: Kim Phillips <kim.phillips@amd.com>,
	Usama Arif <usama.arif@bytedance.com>,
	arjan@linux.intel.com, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, hpa@zytor.com, x86@kernel.org,
	pbonzini@redhat.com, paulmck@kernel.org,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	rcu@vger.kernel.org, mimoja@mimoja.de, hewenliang4@huawei.com,
	thomas.lendacky@amd.com, seanjc@google.com,
	pmenzel@molgen.mpg.de, fam.zheng@bytedance.com,
	punit.agrawal@bytedance.com, simon.evans@bytedance.com,
	liangma@liangbit.com, "Limonciello,
	Mario" <Mario.Limonciello@amd.com>,
	Piotr Gorski <piotrgorski@cachyos.org>
Subject: Re: [PATCH v9 0/8] Parallel CPU bringup for x86_64
Date: Tue, 21 Feb 2023 23:18:40 +0000	[thread overview]
Message-ID: <aac036a17b1bcbabe8ee5a7c69fb2dfbc546d06e.camel@infradead.org> (raw)
In-Reply-To: <87356yofw3.ffs@tglx>

[-- Attachment #1: Type: text/plain, Size: 4830 bytes --]

On Tue, 2023-02-21 at 22:41 +0100, Thomas Gleixner wrote:
> 
> @@ -57,6 +58,7 @@ asmlinkage acpi_status __visible x86_acp
>   */
>  int x86_acpi_suspend_lowlevel(void)
>  {
> +       unsigned int __maybe_unused saved_smpboot_ctrl;
>         struct wakeup_header *header =
>                 (struct wakeup_header *) __va(real_mode_header->wakeup_header);
>  
> @@ -115,7 +117,8 @@ int x86_acpi_suspend_lowlevel(void)
>         early_gdt_descr.address =
>                         (unsigned long)get_cpu_gdt_rw(smp_processor_id());
>         initial_gs = per_cpu_offset(smp_processor_id());
> -       smpboot_control = 0;
> +       /* Force the startup into boot mode */
> +       saved_smpboot_ctrl = xchg(&smpboot_control, 0);
>  #endif
>         initial_code = (unsigned long)wakeup_long64;
>         saved_magic = 0x123456789abcdef0L;
> @@ -128,6 +131,9 @@ int x86_acpi_suspend_lowlevel(void)
>         pause_graph_tracing();
>         do_suspend_lowlevel();
>         unpause_graph_tracing();
> +
> +       if (IS_ENABLED(CONFIG_64BIT) && IS_ENABLED(CONFIG_SMP))
> +               smpboot_control = saved_smpboot_ctrl;
>         return 0;
>  }
>  

But wait, why is this giving it a dedicated temp_stack anyway? Why
can't it use that CPU's idle thread stack like we usually do? I already
made idle_thread_get() accessible from here. So we could do this...

@@ -111,14 +112,16 @@ int x86_acpi_suspend_lowlevel(void)
        saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-       initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
-       early_gdt_descr.address =
-                       (unsigned long)get_cpu_gdt_rw(smp_processor_id());
-       initial_gs = per_cpu_offset(smp_processor_id());
-       smpboot_control = 0;
+       if (!(smpboot_control & STARTUP_PARALLEL_MASK)) {
+               unsigned int cpu = smp_processor_id();
+               initial_stack = (unsigned long)idle_thread_get(cpu)->thread.sp;
+               early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
+               initial_gs = per_cpu_offset(cpu);
+               smpboot_control = 0;
+       }
 #endif
        initial_code = (unsigned long)wakeup_long64;


But that's a whole bunch of pointless, because it can be even further
simplified to just let the find its own crap like the secondaries do,
except in the 'OMG CPUID won't tell me' case where it has to be told:

So how about we just do something more like this. I'd *quite* like to
put the actual handling of smpboot_control into a function we call in
smpboot.c. and that whole x86_acpi_suspend_lowlevel() function wants
all its horrid 64bit/smp ifdefs fixed up (and is there any reason
there's a generic part saving CR0 and IA32_MISC_ENABLE right in the
middle of some !CONFIG_64BIT parts? I don't see ordering constraints
there). But this should work, I think:

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 33c0d5fd8af6..72b9375fec7c 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -208,4 +208,6 @@ extern unsigned int smpboot_control;
 #define STARTUP_APICID_CPUID_0B	0x40000000
 #define STARTUP_APICID_CPUID_01	0x20000000
 
+#define STARTUP_PARALLEL_MASK	0x60000000
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 06adf340a0f1..a1343a900caf 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -16,17 +16,14 @@
 #include <asm/cacheflush.h>
 #include <asm/realmode.h>
 #include <asm/hypervisor.h>
-
+#include <asm/smp.h>
+#include <linux/smpboot.h>
 #include <linux/ftrace.h>
 #include "../../realmode/rm/wakeup.h"
 #include "sleep.h"
 
 unsigned long acpi_realmode_flags;
 
-#if defined(CONFIG_SMP) && defined(CONFIG_64BIT)
-static char temp_stack[4096];
-#endif
-
 /**
  * acpi_get_wakeup_address - provide physical address for S3 wakeup
  *
@@ -111,14 +108,11 @@ int x86_acpi_suspend_lowlevel(void)
 	saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
-	early_gdt_descr.address =
-			(unsigned long)get_cpu_gdt_rw(smp_processor_id());
-	initial_gs = per_cpu_offset(smp_processor_id());
-	smpboot_control = 0;
+	if (!(smpboot_control & STARTUP_PARALLEL_MASK))
+		smpboot_control = STARTUP_SECONDARY | cpu_physical_id(smp_processor_id());
 #endif
 	initial_code = (unsigned long)wakeup_long64;
-       saved_magic = 0x123456789abcdef0L;
+	saved_magic = 0x123456789abcdef0L;
 #endif /* CONFIG_64BIT */
 
 	/*



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
