[PATCH v12 00/11] Parallel CPU bringup for x86_64

[PATCH v12 00/11] Parallel CPU bringup for x86_64

[Usama Arif]

The main code change over v11 is the build error fix by Brian Gerst and
acquiring tr_lock in trampoline_64.S whenever the stack is setup.

The git history is also rewritten to move the commits that removed
initial_stack, early_gdt_descr and initial_gs earlier in the patchset.

Thanks,
Usama

Changes across versions:
v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
    in preparation for more parallelisation.
v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
    avoid scribbling on initial_gs in common_cpu_up(), and to allow all
    24 bits of the physical X2APIC ID to be used. That patch still needs
    a Signed-off-by from its original author, who once claimed not to
    remember writing it at all. But now we've fixed it, hopefully he'll
    admit it now :)
v5: rebase to v6.1 and remeasure performance, disable parallel bringup
    for AMD CPUs.
v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
    reused timer calibration for secondary CPUs.
v7: [David Woodhouse] iterate over all possible CPUs to find any existing
    cluster mask in alloc_clustermask. (patch 1/9)
    Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
    0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
    Included sanity checks for APIC id from 0x0B. (patch 6/9)
    Removed patch for reusing timer calibration for secondary CPUs.
    commit message and code improvements.
v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
    early_gdt_descr.
    Drop trampoline lock and bail if APIC ID not found in find_cpunr.
    Code comments improved and debug prints added.
v9: Drop patch to avoid repeated saves of MTRR at boot time.
    rebased and retested at v6.2-rc8.
    added kernel doc for no_parallel_bringup and made do_parallel_bringup
    __ro_after_init.
v10: Fixed suspend/resume not working with parallel smpboot.
     rebased and retested to 6.2.
     fixed checkpatch errors.
v11: Added patches from Brian Gerst to remove the global variables initial_gs,
     initial_stack, and early_gdt_descr from the 64-bit boot code
     (https://lore.kernel.org/all/20230222221301.245890-1-brgerst@gmail.com/).
v12: Fixed compilation errors, acquire tr_lock for every stack setup in
     trampoline_64.S.
     Rearranged commits for a cleaner git history.

Brian Gerst (3):
  x86/smpboot: Remove initial_stack on 64-bit
  x86/smpboot: Remove early_gdt_descr on 64-bit
  x86/smpboot: Remove initial_gs

David Woodhouse (8):
  x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
  cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
  cpu/hotplug: Add dynamic parallel bringup states before
    CPUHP_BRINGUP_CPU
  x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
  x86/smpboot: Split up native_cpu_up into separate phases and document
    them
  x86/smpboot: Support parallel startup of secondary CPUs
  x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
  x86/smpboot: Serialize topology updates for secondary bringup

 .../admin-guide/kernel-parameters.txt         |   3 +
 arch/x86/include/asm/processor.h              |   6 +-
 arch/x86/include/asm/realmode.h               |   4 +-
 arch/x86/include/asm/smp.h                    |  15 +-
 arch/x86/include/asm/topology.h               |   2 -
 arch/x86/kernel/acpi/sleep.c                  |  15 +-
 arch/x86/kernel/apic/apic.c                   |   2 +-
 arch/x86/kernel/apic/x2apic_cluster.c         | 126 ++++---
 arch/x86/kernel/asm-offsets.c                 |   1 +
 arch/x86/kernel/cpu/common.c                  |   6 +-
 arch/x86/kernel/head_64.S                     | 129 +++++--
 arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
 arch/x86/realmode/init.c                      |   3 +
 arch/x86/realmode/rm/trampoline_64.S          |  27 +-
 arch/x86/xen/smp_pv.c                         |   4 +-
 arch/x86/xen/xen-head.S                       |   2 +-
 include/linux/cpuhotplug.h                    |   2 +
 include/linux/smpboot.h                       |   7 +
 kernel/cpu.c                                  |  31 +-
 kernel/smpboot.h                              |   2 -
 20 files changed, 537 insertions(+), 200 deletions(-)

-- 
2.25.1

[PATCH v12 01/11] x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

Each of the sibling CPUs in a cluster uses the same clustermask. The first
CPU in a cluster will need a new clustermask allocated, while subsequent
siblings will use the same clustermask as the first.

However, the CPU being brought up cannot yet perform memory allocations
at the point that this occurs in init_x2apic_ldr().

So at present, the alloc_clustermask() function allocates a clustermask
just in case it's needed, storing it in the global cluster_hotplug_mask.
A CPU which is the first sibling of a cluster will "take" it from there
and set cluster_hotplug_mask to NULL, in order for alloc_clustermask()
to allocate a new one before bringing up the next CPU.

To facilitate parallel bringup of CPUs in future, switch to a model
where alloc_clustermask() prepopulates the clustermask in the per_cpu
data for each present CPU in the cluster in advance. All that the CPU
needs to do for itself in init_x2apic_ldr() is set its own bit in that
mask.

The 'node' and 'clusterid' members of struct cluster_mask are thus
redundant, and it can become a simple struct cpumask instead.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 arch/x86/kernel/apic/x2apic_cluster.c | 126 +++++++++++++++++---------
 1 file changed, 82 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index e696e22d0531..b2b2b7f3e03f 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -9,11 +9,7 @@
 
 #include "local.h"
 
-struct cluster_mask {
-	unsigned int	clusterid;
-	int		node;
-	struct cpumask	mask;
-};
+#define apic_cluster(apicid) ((apicid) >> 4)
 
 /*
  * __x2apic_send_IPI_mask() possibly needs to read
@@ -23,8 +19,7 @@ struct cluster_mask {
 static u32 *x86_cpu_to_logical_apicid __read_mostly;
 
 static DEFINE_PER_CPU(cpumask_var_t, ipi_mask);
-static DEFINE_PER_CPU_READ_MOSTLY(struct cluster_mask *, cluster_masks);
-static struct cluster_mask *cluster_hotplug_mask;
+static DEFINE_PER_CPU_READ_MOSTLY(struct cpumask *, cluster_masks);
 
 static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
@@ -60,10 +55,10 @@ __x2apic_send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
 
 	/* Collapse cpus in a cluster so a single IPI per cluster is sent */
 	for_each_cpu(cpu, tmpmsk) {
-		struct cluster_mask *cmsk = per_cpu(cluster_masks, cpu);
+		struct cpumask *cmsk = per_cpu(cluster_masks, cpu);
 
 		dest = 0;
-		for_each_cpu_and(clustercpu, tmpmsk, &cmsk->mask)
+		for_each_cpu_and(clustercpu, tmpmsk, cmsk)
 			dest |= x86_cpu_to_logical_apicid[clustercpu];
 
 		if (!dest)
@@ -71,7 +66,7 @@ __x2apic_send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
 
 		__x2apic_send_IPI_dest(dest, vector, APIC_DEST_LOGICAL);
 		/* Remove cluster CPUs from tmpmask */
-		cpumask_andnot(tmpmsk, tmpmsk, &cmsk->mask);
+		cpumask_andnot(tmpmsk, tmpmsk, cmsk);
 	}
 
 	local_irq_restore(flags);
@@ -105,55 +100,98 @@ static u32 x2apic_calc_apicid(unsigned int cpu)
 
 static void init_x2apic_ldr(void)
 {
-	struct cluster_mask *cmsk = this_cpu_read(cluster_masks);
-	u32 cluster, apicid = apic_read(APIC_LDR);
-	unsigned int cpu;
+	struct cpumask *cmsk = this_cpu_read(cluster_masks);
 
-	x86_cpu_to_logical_apicid[smp_processor_id()] = apicid;
+	BUG_ON(!cmsk);
 
-	if (cmsk)
-		goto update;
-
-	cluster = apicid >> 16;
-	for_each_online_cpu(cpu) {
-		cmsk = per_cpu(cluster_masks, cpu);
-		/* Matching cluster found. Link and update it. */
-		if (cmsk && cmsk->clusterid == cluster)
-			goto update;
+	cpumask_set_cpu(smp_processor_id(), cmsk);
+}
+
+/*
+ * As an optimisation during boot, set the cluster_mask for all present
+ * CPUs at once, to prevent each of them having to iterate over the others
+ * to find the existing cluster_mask.
+ */
+static void prefill_clustermask(struct cpumask *cmsk, unsigned int cpu, u32 cluster)
+{
+	int cpu_i;
+
+	for_each_present_cpu(cpu_i) {
+		struct cpumask **cpu_cmsk = &per_cpu(cluster_masks, cpu_i);
+		u32 apicid = apic->cpu_present_to_apicid(cpu_i);
+
+		if (apicid == BAD_APICID || cpu_i == cpu || apic_cluster(apicid) != cluster)
+			continue;
+
+		if (WARN_ON_ONCE(*cpu_cmsk == cmsk))
+			continue;
+
+		BUG_ON(*cpu_cmsk);
+		*cpu_cmsk = cmsk;
 	}
-	cmsk = cluster_hotplug_mask;
-	cmsk->clusterid = cluster;
-	cluster_hotplug_mask = NULL;
-update:
-	this_cpu_write(cluster_masks, cmsk);
-	cpumask_set_cpu(smp_processor_id(), &cmsk->mask);
 }
 
-static int alloc_clustermask(unsigned int cpu, int node)
+static int alloc_clustermask(unsigned int cpu, u32 cluster, int node)
 {
+	struct cpumask *cmsk = NULL;
+	unsigned int cpu_i;
+
+	/*
+	 * At boot time, the CPU present mask is stable. The cluster mask is
+	 * allocated for the first CPU in the cluster and propagated to all
+	 * present siblings in the cluster. If the cluster mask is already set
+	 * on entry to this function for a given CPU, there is nothing to do.
+	 */
 	if (per_cpu(cluster_masks, cpu))
 		return 0;
+
+	if (system_state < SYSTEM_RUNNING)
+		goto alloc;
+
 	/*
-	 * If a hotplug spare mask exists, check whether it's on the right
-	 * node. If not, free it and allocate a new one.
+	 * On post boot hotplug for a CPU which was not present at boot time,
+	 * iterate over all possible CPUs (even those which are not present
+	 * any more) to find any existing cluster mask.
 	 */
-	if (cluster_hotplug_mask) {
-		if (cluster_hotplug_mask->node == node)
-			return 0;
-		kfree(cluster_hotplug_mask);
+	for_each_possible_cpu(cpu_i) {
+		u32 apicid = apic->cpu_present_to_apicid(cpu_i);
+
+		if (apicid != BAD_APICID && apic_cluster(apicid) == cluster) {
+			cmsk = per_cpu(cluster_masks, cpu_i);
+			/*
+			 * If the cluster is already initialized, just store
+			 * the mask and return. There's no need to propagate.
+			 */
+			if (cmsk) {
+				per_cpu(cluster_masks, cpu) = cmsk;
+				return 0;
+			}
+		}
 	}
-
-	cluster_hotplug_mask = kzalloc_node(sizeof(*cluster_hotplug_mask),
-					    GFP_KERNEL, node);
-	if (!cluster_hotplug_mask)
+	/*
+	 * No CPU in the cluster has ever been initialized, so fall through to
+	 * the boot time code which will also populate the cluster mask for any
+	 * other CPU in the cluster which is (now) present.
+	 */
+alloc:
+	cmsk = kzalloc_node(sizeof(*cmsk), GFP_KERNEL, node);
+	if (!cmsk)
 		return -ENOMEM;
-	cluster_hotplug_mask->node = node;
+	per_cpu(cluster_masks, cpu) = cmsk;
+	prefill_clustermask(cmsk, cpu, cluster);
+
 	return 0;
 }
 
 static int x2apic_prepare_cpu(unsigned int cpu)
 {
-	if (alloc_clustermask(cpu, cpu_to_node(cpu)) < 0)
+	u32 phys_apicid = apic->cpu_present_to_apicid(cpu);
+	u32 cluster = apic_cluster(phys_apicid);
+	u32 logical_apicid = (cluster << 16) | (1 << (phys_apicid & 0xf));
+
+	x86_cpu_to_logical_apicid[cpu] = logical_apicid;
+
+	if (alloc_clustermask(cpu, cluster, cpu_to_node(cpu)) < 0)
 		return -ENOMEM;
 	if (!zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL))
 		return -ENOMEM;
@@ -162,10 +200,10 @@ static int x2apic_prepare_cpu(unsigned int cpu)
 
 static int x2apic_dead_cpu(unsigned int dead_cpu)
 {
-	struct cluster_mask *cmsk = per_cpu(cluster_masks, dead_cpu);
+	struct cpumask *cmsk = per_cpu(cluster_masks, dead_cpu);
 
 	if (cmsk)
-		cpumask_clear_cpu(dead_cpu, &cmsk->mask);
+		cpumask_clear_cpu(dead_cpu, cmsk);
 	free_cpumask_var(per_cpu(ipi_mask, dead_cpu));
 	return 0;
 }
-- 
2.25.1

[PATCH v12 02/11] cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

Instead of relying purely on the special-case wrapper in bringup_cpu()
to pass the idle thread to __cpu_up(), expose idle_thread_get() so that
the architecture code can obtain it directly when necessary.

This will be useful when the existing __cpu_up() is split into multiple
phases, only *one* of which will actually need the idle thread.

If the architecture code is to register its new pre-bringup states with
the cpuhp core, having a special-case wrapper to pass extra arguments is
non-trivial and it's easier just to let the arch register its function
pointer to be invoked with the standard API.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 include/linux/smpboot.h | 7 +++++++
 kernel/smpboot.h        | 2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index 9d1bc65d226c..3862addcaa34 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -5,6 +5,13 @@
 #include <linux/types.h>
 
 struct task_struct;
+
+#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
+struct task_struct *idle_thread_get(unsigned int cpu);
+#else
+static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
+#endif
+
 /* Cookie handed to the thread_fn*/
 struct smpboot_thread_data;
 
diff --git a/kernel/smpboot.h b/kernel/smpboot.h
index 34dd3d7ba40b..60c609318ad6 100644
--- a/kernel/smpboot.h
+++ b/kernel/smpboot.h
@@ -5,11 +5,9 @@
 struct task_struct;
 
 #ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
-struct task_struct *idle_thread_get(unsigned int cpu);
 void idle_thread_set_boot_cpu(void);
 void idle_threads_init(void);
 #else
-static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
 static inline void idle_thread_set_boot_cpu(void) { }
 static inline void idle_threads_init(void) { }
 #endif
-- 
2.25.1

[PATCH v12 03/11] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

There is often significant latency in the early stages of CPU bringup,
and time is wasted by waking each CPU (e.g. with SIPI/INIT/INIT on x86)
and then waiting for it to respond before moving on to the next.

Allow a platform to register a set of pre-bringup CPUHP states to which
each CPU can be stepped in parallel, thus absorbing some of that latency.

There is a subtlety here: even with an empty CPUHP_BP_PARALLEL_DYN step,
this means that *all* CPUs are brought through the prepare states and to
CPUHP_BP_PREPARE_DYN before any of them are taken to CPUHP_BRINGUP_CPU
and then are allowed to run for themselves to CPUHP_ONLINE.

So any combination of prepare/start calls which depend on A-B ordering
for each CPU in turn, such as the X2APIC code which used to allocate a
cluster mask 'just in case' and store it in a global variable in the
prep stage, then potentially consume that preallocated structure from
the AP and set the global pointer to NULL to be reallocated in
CPUHP_X2APIC_PREPARE for the next CPU... would explode horribly.

Any platform enabling the CPUHP_BP_PARALLEL_DYN steps must be reviewed
and tested to ensure that such issues do not exist, and the existing
behaviour of bringing CPUs to CPUHP_BP_PREPARE_DYN and then immediately
to CPUHP_BRINGUP_CPU and CPUHP_ONLINE only one at a time does not change
unless such a state is registered.

Note that the new parallel stages do *not* yet bring each AP to the
CPUHP_BRINGUP_CPU state at the same time, only to the new states which
exist before it. The final loop in bringup_nonboot_cpus() is untouched,
bringing each AP in turn from the final PARALLEL_DYN state (or all the
way from CPUHP_OFFLINE) to CPUHP_BRINGUP_CPU and then waiting for that
AP to do its own processing and reach CPUHP_ONLINE before releasing the
next.

Parallelising that part by bringing them all to CPUHP_BRINGUP_CPU
and then waiting for them all is an exercise for the future.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 include/linux/cpuhotplug.h |  2 ++
 kernel/cpu.c               | 31 +++++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 6c6859bfc454..e5a73ae6ccc0 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -133,6 +133,8 @@ enum cpuhp_state {
 	CPUHP_MIPS_SOC_PREPARE,
 	CPUHP_BP_PREPARE_DYN,
 	CPUHP_BP_PREPARE_DYN_END		= CPUHP_BP_PREPARE_DYN + 20,
+	CPUHP_BP_PARALLEL_DYN,
+	CPUHP_BP_PARALLEL_DYN_END		= CPUHP_BP_PARALLEL_DYN + 4,
 	CPUHP_BRINGUP_CPU,
 
 	/*
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6c0a92ca6bb5..fffb0da61ccc 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1504,8 +1504,30 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
 
 void bringup_nonboot_cpus(unsigned int setup_max_cpus)
 {
+	unsigned int n = setup_max_cpus - num_online_cpus();
 	unsigned int cpu;
 
+	/*
+	 * An architecture may have registered parallel pre-bringup states to
+	 * which each CPU may be brought in parallel. For each such state,
+	 * bring N CPUs to it in turn before the final round of bringing them
+	 * online.
+	 */
+	if (n > 0) {
+		enum cpuhp_state st = CPUHP_BP_PARALLEL_DYN;
+
+		while (st <= CPUHP_BP_PARALLEL_DYN_END && cpuhp_hp_states[st].name) {
+			int i = n;
+
+			for_each_present_cpu(cpu) {
+				cpu_up(cpu, st);
+				if (!--i)
+					break;
+			}
+			st++;
+		}
+	}
+
 	for_each_present_cpu(cpu) {
 		if (num_online_cpus() >= setup_max_cpus)
 			break;
@@ -1882,6 +1904,10 @@ static int cpuhp_reserve_state(enum cpuhp_state state)
 		step = cpuhp_hp_states + CPUHP_BP_PREPARE_DYN;
 		end = CPUHP_BP_PREPARE_DYN_END;
 		break;
+	case CPUHP_BP_PARALLEL_DYN:
+		step = cpuhp_hp_states + CPUHP_BP_PARALLEL_DYN;
+		end = CPUHP_BP_PARALLEL_DYN_END;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -1906,14 +1932,15 @@ static int cpuhp_store_callbacks(enum cpuhp_state state, const char *name,
 	/*
 	 * If name is NULL, then the state gets removed.
 	 *
-	 * CPUHP_AP_ONLINE_DYN and CPUHP_BP_PREPARE_DYN are handed out on
+	 * CPUHP_AP_ONLINE_DYN and CPUHP_BP_P*_DYN are handed out on
 	 * the first allocation from these dynamic ranges, so the removal
 	 * would trigger a new allocation and clear the wrong (already
 	 * empty) state, leaving the callbacks of the to be cleared state
 	 * dangling, which causes wreckage on the next hotplug operation.
 	 */
 	if (name && (state == CPUHP_AP_ONLINE_DYN ||
-		     state == CPUHP_BP_PREPARE_DYN)) {
+		     state == CPUHP_BP_PREPARE_DYN ||
+		     state == CPUHP_BP_PARALLEL_DYN)) {
 		ret = cpuhp_reserve_state(state);
 		if (ret < 0)
 			return ret;
-- 
2.25.1

[PATCH v12 04/11] x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

When bringing up a secondary CPU from do_boot_cpu(), the warm reset flag
is set in CMOS and the starting IP for the trampoline written inside the
BDA at 0x467. Once the CPU is running, the CMOS flag is unset and the
value in the BDA cleared.

To allow for parallel bringup of CPUs, add a reference count to track the
number of CPUs currently bring brought up, and clear the state only when
the count reaches zero.

Since the RTC spinlock is required to write to the CMOS, it can be used
for mutual exclusion on the refcount too.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 arch/x86/kernel/smpboot.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 55cad72715d9..3a793772a2aa 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -121,17 +121,20 @@ int arch_update_cpu_topology(void)
 	return retval;
 }
 
+
+static unsigned int smpboot_warm_reset_vector_count;
+
 static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0xa, 0xf);
+	if (!smpboot_warm_reset_vector_count++) {
+		CMOS_WRITE(0xa, 0xf);
+		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) = start_eip >> 4;
+		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = start_eip & 0xf;
+	}
 	spin_unlock_irqrestore(&rtc_lock, flags);
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
-							start_eip >> 4;
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
-							start_eip & 0xf;
 }
 
 static inline void smpboot_restore_warm_reset_vector(void)
@@ -143,10 +146,12 @@ static inline void smpboot_restore_warm_reset_vector(void)
 	 * to default values.
 	 */
 	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0, 0xf);
+	if (!--smpboot_warm_reset_vector_count) {
+		CMOS_WRITE(0, 0xf);
+		*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+	}
 	spin_unlock_irqrestore(&rtc_lock, flags);
 
-	*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
 }
 
 /*
-- 
2.25.1

[PATCH v12 05/11] x86/smpboot: Split up native_cpu_up into separate phases and document them

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

There are four logical parts to what native_cpu_up() does on the BSP (or
on the controlling CPU for a later hotplug):

 1) Wake the AP by sending the INIT/SIPI/SIPI sequence.

 2) Wait for the AP to make it as far as wait_for_master_cpu() which
    sets that CPU's bit in cpu_initialized_mask, then sets the bit in
    cpu_callout_mask to let the AP proceed through cpu_init().

 3) Wait for the AP to finish cpu_init() and get as far as the
    smp_callin() call, which sets that CPU's bit in cpu_callin_mask.

 4) Perform the TSC synchronization and wait for the AP to actually
    mark itself online in cpu_online_mask.

In preparation to allow these phases to operate in parallel on multiple
APs, split them out into separate functions and document the interactions
a little more clearly in both the BSP and AP code paths.

No functional change intended.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 arch/x86/kernel/smpboot.c | 181 ++++++++++++++++++++++++++------------
 1 file changed, 127 insertions(+), 54 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3a793772a2aa..b18c1385e181 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -204,6 +204,10 @@ static void smp_callin(void)
 
 	wmb();
 
+	/*
+	 * This runs the AP through all the cpuhp states to its target
+	 * state (CPUHP_ONLINE in the case of serial bringup).
+	 */
 	notify_cpu_starting(cpuid);
 
 	/*
@@ -231,17 +235,32 @@ static void notrace start_secondary(void *unused)
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
 #endif
+	/*
+	 * Sync point with do_wait_cpu_initialized(). Before proceeding through
+	 * cpu_init(), the AP will call wait_for_master_cpu() which sets its
+	 * own bit in cpu_initialized_mask and then waits for the BSP to set
+	 * its bit in cpu_callout_mask to release it.
+	 */
 	cpu_init_secondary();
 	rcu_cpu_starting(raw_smp_processor_id());
 	x86_cpuinit.early_percpu_clock_init();
+
+	/*
+	 * Sync point with do_wait_cpu_callin(). The AP doesn't wait here
+	 * but just sets the bit to let the controlling CPU (BSP) know that
+	 * it's got this far.
+	 */
 	smp_callin();
 
 	enable_start_cpu0 = 0;
 
 	/* otherwise gcc will move up smp_processor_id before the cpu_init */
 	barrier();
+
 	/*
-	 * Check TSC synchronization with the boot CPU:
+	 * Check TSC synchronization with the boot CPU (or whichever CPU
+	 * is controlling the bringup). It will do its part of this from
+	 * do_wait_cpu_online(), making it an implicit sync point.
 	 */
 	check_tsc_sync_target();
 
@@ -254,6 +273,7 @@ static void notrace start_secondary(void *unused)
 	 * half valid vector space.
 	 */
 	lock_vector_lock();
+	/* Sync point with do_wait_cpu_online() */
 	set_cpu_online(smp_processor_id(), true);
 	lapic_online();
 	unlock_vector_lock();
@@ -1083,7 +1103,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	unsigned long start_ip = real_mode_header->trampoline_start;
 
 	unsigned long boot_error = 0;
-	unsigned long timeout;
 
 #ifdef CONFIG_X86_64
 	/* If 64-bit wakeup method exists, use the 64-bit mode trampoline IP */
@@ -1144,55 +1163,94 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
 						     cpu0_nmi_registered);
 
-	if (!boot_error) {
-		/*
-		 * Wait 10s total for first sign of life from AP
-		 */
-		boot_error = -1;
-		timeout = jiffies + 10*HZ;
-		while (time_before(jiffies, timeout)) {
-			if (cpumask_test_cpu(cpu, cpu_initialized_mask)) {
-				/*
-				 * Tell AP to proceed with initialization
-				 */
-				cpumask_set_cpu(cpu, cpu_callout_mask);
-				boot_error = 0;
-				break;
-			}
-			schedule();
-		}
-	}
+	return boot_error;
+}
 
-	if (!boot_error) {
-		/*
-		 * Wait till AP completes initial initialization
-		 */
-		while (!cpumask_test_cpu(cpu, cpu_callin_mask)) {
-			/*
-			 * Allow other tasks to run while we wait for the
-			 * AP to come online. This also gives a chance
-			 * for the MTRR work(triggered by the AP coming online)
-			 * to be completed in the stop machine context.
-			 */
-			schedule();
-		}
+static int do_wait_cpu_cpumask(unsigned int cpu, const struct cpumask *mask)
+{
+	unsigned long timeout;
+
+	/*
+	 * Wait up to 10s for the CPU to report in.
+	 */
+	timeout = jiffies + 10*HZ;
+	while (time_before(jiffies, timeout)) {
+		if (cpumask_test_cpu(cpu, mask))
+			return 0;
+
+		schedule();
 	}
+	return -1;
+}
 
-	if (x86_platform.legacy.warm_reset) {
-		/*
-		 * Cleanup possible dangling ends...
-		 */
-		smpboot_restore_warm_reset_vector();
+/*
+ * Bringup step two: Wait for the target AP to reach cpu_init_secondary()
+ * and thus wait_for_master_cpu(), then set cpu_callout_mask to allow it
+ * to proceed.  The AP will then proceed past setting its 'callin' bit
+ * and end up waiting in check_tsc_sync_target() until we reach
+ * do_wait_cpu_online() to tend to it.
+ */
+static int do_wait_cpu_initialized(unsigned int cpu)
+{
+	/*
+	 * Wait for first sign of life from AP.
+	 */
+	if (do_wait_cpu_cpumask(cpu, cpu_initialized_mask))
+		return -1;
+
+	cpumask_set_cpu(cpu, cpu_callout_mask);
+	return 0;
+}
+
+/*
+ * Bringup step three: Wait for the target AP to reach smp_callin().
+ * The AP is not waiting for us here so we don't need to parallelise
+ * this step. Not entirely clear why we care about this, since we just
+ * proceed directly to TSC synchronization which is the next sync
+ * point with the AP anyway.
+ */
+static int do_wait_cpu_callin(unsigned int cpu)
+{
+	/*
+	 * Wait till AP completes initial initialization.
+	 */
+	return do_wait_cpu_cpumask(cpu, cpu_callin_mask);
+}
+
+/*
+ * Bringup step four: Synchronize the TSC and wait for the target AP
+ * to reach set_cpu_online() in start_secondary().
+ */
+static int do_wait_cpu_online(unsigned int cpu)
+{
+	unsigned long flags;
+
+	/*
+	 * Check TSC synchronization with the AP (keep irqs disabled
+	 * while doing so):
+	 */
+	local_irq_save(flags);
+	check_tsc_sync_source(cpu);
+	local_irq_restore(flags);
+
+	/*
+	 * Wait for the AP to mark itself online. Not entirely
+	 * clear why we care, since the generic cpuhp code will
+	 * wait for it to each CPUHP_AP_ONLINE_IDLE before going
+	 * ahead with the rest of the bringup anyway.
+	 */
+	while (!cpu_online(cpu)) {
+		cpu_relax();
+		touch_nmi_watchdog();
 	}
 
-	return boot_error;
+	return 0;
 }
 
-int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
+static int do_cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
 	int apicid = apic->cpu_present_to_apicid(cpu);
 	int cpu0_nmi_registered = 0;
-	unsigned long flags;
 	int err, ret = 0;
 
 	lockdep_assert_irqs_enabled();
@@ -1239,19 +1297,6 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 		goto unreg_nmi;
 	}
 
-	/*
-	 * Check TSC synchronization with the AP (keep irqs disabled
-	 * while doing so):
-	 */
-	local_irq_save(flags);
-	check_tsc_sync_source(cpu);
-	local_irq_restore(flags);
-
-	while (!cpu_online(cpu)) {
-		cpu_relax();
-		touch_nmi_watchdog();
-	}
-
 unreg_nmi:
 	/*
 	 * Clean up the nmi handler. Do this after the callin and callout sync
@@ -1263,6 +1308,34 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	return ret;
 }
 
+int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
+{
+	int ret;
+
+	ret = do_cpu_up(cpu, tidle);
+	if (ret)
+		return ret;
+
+	ret = do_wait_cpu_initialized(cpu);
+	if (ret)
+		return ret;
+
+	ret = do_wait_cpu_callin(cpu);
+	if (ret)
+		return ret;
+
+	ret = do_wait_cpu_online(cpu);
+
+	if (x86_platform.legacy.warm_reset) {
+		/*
+		 * Cleanup possible dangling ends...
+		 */
+		smpboot_restore_warm_reset_vector();
+	}
+
+	return ret;
+}
+
 /**
  * arch_disable_smp_support() - disables SMP support for x86 at runtime
  */
-- 
2.25.1

[PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[Usama Arif]

From: Brian Gerst <brgerst@gmail.com>

Eliminating global variables from the CPU startup path in order to simplify
it and facilitate parallel startup.

Remove initial_stack, and load RSP from current_task->thread.sp instead.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 arch/x86/include/asm/processor.h |  6 ++++-
 arch/x86/include/asm/smp.h       |  5 +++-
 arch/x86/kernel/acpi/sleep.c     |  5 ++--
 arch/x86/kernel/asm-offsets.c    |  1 +
 arch/x86/kernel/head_64.S        | 43 +++++++++++++++++++++-----------
 arch/x86/kernel/smpboot.c        |  7 +++++-
 arch/x86/xen/xen-head.S          |  2 +-
 7 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..bdde7316e75b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -648,7 +648,11 @@ static inline void spin_lock_prefetch(const void *x)
 #define KSTK_ESP(task)		(task_pt_regs(task)->sp)
 
 #else
-#define INIT_THREAD { }
+extern unsigned long __end_init_task[];
+
+#define INIT_THREAD {							    \
+	.sp	= (unsigned long)&__end_init_task - sizeof(struct pt_regs), \
+}
 
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index b4dbb20dab1a..bf2c51df9e0b 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,5 +199,8 @@ extern void nmi_selftest(void);
 #define nmi_selftest() do { } while (0)
 #endif
 
-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3b7f4cdbf2e0..ab6e29b32c04 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -111,13 +111,14 @@ int x86_acpi_suspend_lowlevel(void)
 	saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
+	current->thread.sp = (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 = smp_processor_id();
 #endif
 	initial_code = (unsigned long)wakeup_long64;
-       saved_magic = 0x123456789abcdef0L;
+	saved_magic = 0x123456789abcdef0L;
 #endif /* CONFIG_64BIT */
 
 	/*
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 82c783da16a8..797ae1a15c91 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -108,6 +108,7 @@ static void __used common(void)
 	OFFSET(TSS_sp1, tss_struct, x86_tss.sp1);
 	OFFSET(TSS_sp2, tss_struct, x86_tss.sp2);
 	OFFSET(X86_top_of_stack, pcpu_hot, top_of_stack);
+	OFFSET(X86_current_task, pcpu_hot, current_task);
 #ifdef CONFIG_CALL_DEPTH_TRACKING
 	OFFSET(X86_call_depth, pcpu_hot, call_depth);
 #endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 222efd4a09bc..5a2417d788d1 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -61,8 +61,8 @@ SYM_CODE_START_NOALIGN(startup_64)
 	 * tables and then reload them.
 	 */
 
-	/* Set up the stack for verify_cpu(), similar to initial_stack below */
-	leaq	(__end_init_task - FRAME_SIZE)(%rip), %rsp
+	/* Set up the stack for verify_cpu() */
+	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
 	leaq	_text(%rip), %rdi
 
@@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	UNWIND_HINT_EMPTY
 	ANNOTATE_NOENDBR // above
 
+#ifdef CONFIG_SMP
+	movl	smpboot_control(%rip), %ecx
+
+	/* Get the per cpu offset for the given CPU# which is in ECX */
+	movq	__per_cpu_offset(,%rcx,8), %rdx
+#else
+	xorl	%edx, %edx
+#endif /* CONFIG_SMP */
+
+	/*
+	 * Setup a boot time stack - Any secondary CPU will have lost its stack
+	 * by now because the cr3-switch above unmaps the real-mode stack.
+	 *
+	 * RDX contains the per-cpu offset
+	 */
+	movq	pcpu_hot + X86_current_task(%rdx), %rax
+	movq	TASK_threadsp(%rax), %rsp
+
 	/*
 	 * We must switch to a new descriptor in kernel space for the GDT
 	 * because soon the kernel won't have access anymore to the userspace
@@ -275,12 +293,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	movl	initial_gs+4(%rip),%edx
 	wrmsr
 
-	/*
-	 * Setup a boot time stack - Any secondary CPU will have lost its stack
-	 * by now because the cr3-switch above unmaps the real-mode stack
-	 */
-	movq initial_stack(%rip), %rsp
-
 	/* Setup and Load IDT */
 	pushq	%rsi
 	call	early_setup_idt
@@ -372,7 +384,11 @@ SYM_CODE_END(secondary_startup_64)
 SYM_CODE_START(start_cpu0)
 	ANNOTATE_NOENDBR
 	UNWIND_HINT_EMPTY
-	movq	initial_stack(%rip), %rsp
+
+	/* Find the idle task stack */
+	movq	PER_CPU_VAR(pcpu_hot) + X86_current_task, %rcx
+	movq	TASK_threadsp(%rcx), %rsp
+
 	jmp	.Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif
@@ -420,12 +436,6 @@ SYM_DATA(initial_gs,	.quad INIT_PER_CPU_VAR(fixed_percpu_data))
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
 #endif
-
-/*
- * The FRAME_SIZE gap is a convention which helps the in-kernel unwinder
- * reliably detect the end of the stack.
- */
-SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
 	__FINITDATA
 
 	__INIT
@@ -660,6 +670,9 @@ SYM_DATA_END(level1_fixmap_pgt)
 SYM_DATA(early_gdt_descr,		.word GDT_ENTRIES*8-1)
 SYM_DATA_LOCAL(early_gdt_descr_base,	.quad INIT_PER_CPU_VAR(gdt_page))
 
+	.align 16
+SYM_DATA(smpboot_control,		.long 0)
+
 	.align 16
 /* This must match the first entry in level2_kernel_pgt */
 SYM_DATA(phys_base, .quad 0x0)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b18c1385e181..62e3bf37f0b8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);
 	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 	initial_code = (unsigned long)start_secondary;
-	initial_stack  = idle->thread.sp;
+
+	if (IS_ENABLED(CONFIG_X86_32)) {
+		initial_stack  = idle->thread.sp;
+	} else {
+		smpboot_control = cpu;
+	}
 
 	/* Enable the espfix hack for this CPU */
 	init_espfix_ap(cpu);
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ffaa62167f6e..6bd391476656 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -49,7 +49,7 @@ SYM_CODE_START(startup_xen)
 	ANNOTATE_NOENDBR
 	cld
 
-	mov initial_stack(%rip), %rsp
+	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
 	/* Set up %gs.
 	 *
-- 
2.25.1

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[Thomas Gleixner]

On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> From: Brian Gerst <brgerst@gmail.com>
>
> Eliminating global variables from the CPU startup path in order to simplify
> it and facilitate parallel startup.

As this patch is now part of the parallel boot series and actually
introduces smpboot_control, the above is neither accurate nor useful.

Folks, really.

> Remove initial_stack, and load RSP from current_task->thread.sp instead.

  
>  #ifdef CONFIG_SMP
> -	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
> +	current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);

This lacks a comment about the temporary (ab)use of current->thread.sp

>  	early_gdt_descr.address =
>  			(unsigned long)get_cpu_gdt_rw(smp_processor_id());
>  	initial_gs = per_cpu_offset(smp_processor_id());
> +	smpboot_control = smp_processor_id();
>  #endif

> @@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>  	UNWIND_HINT_EMPTY
>  	ANNOTATE_NOENDBR // above
>  
> +#ifdef CONFIG_SMP
> +	movl	smpboot_control(%rip), %ecx
> +
> +	/* Get the per cpu offset for the given CPU# which is in ECX */
> +	movq	__per_cpu_offset(,%rcx,8), %rdx
> +#else
> +	xorl	%edx, %edx
> +#endif /* CONFIG_SMP */

Sigh, we should finally make CONFIG_SMP def_bool y ...

> +	/*
> +	 * Setup a boot time stack - Any secondary CPU will have lost its stack
> +	 * by now because the cr3-switch above unmaps the real-mode stack.
> +	 *
> +	 * RDX contains the per-cpu offset
> +	 */
> +	movq	pcpu_hot + X86_current_task(%rdx), %rax
> +	movq	TASK_threadsp(%rax), %rsp
> +
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index b18c1385e181..62e3bf37f0b8 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>  	idle->thread.sp = (unsigned long)task_pt_regs(idle);
>  	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
>  	initial_code = (unsigned long)start_secondary;
> -	initial_stack  = idle->thread.sp;
> +
> +	if (IS_ENABLED(CONFIG_X86_32)) {
> +		initial_stack  = idle->thread.sp;
> +	} else {
> +		smpboot_control = cpu;
> +	}

Please remove the pointless brackets.
  
Thanks,

        tglx

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[David Woodhouse]

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

On Tue, 2023-02-28 at 17:13 +0100, Thomas Gleixner wrote:
> On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> > From: Brian Gerst <brgerst@gmail.com>
> > 
> > Eliminating global variables from the CPU startup path in order to simplify
> > it and facilitate parallel startup.
> 
> As this patch is now part of the parallel boot series and actually
> introduces smpboot_control, the above is neither accurate nor useful.

I neglected to mention adding smpboot_control, but the above *is*
accurate. This patch, and the next two, are eliminating global
variables to simplify the startup path and make it possible to run it
in parallel.

Now it's slightly harder to phrase that without the Verboteneworte
'this patch', I'll grant you. But it's trying to explain *why* we're
eliminating those global variables.

I'll try again.

> Folks, really.

Also, while those two lines *happen* to be my addition to Brian's
commit message, I don't know if you knew that. Speak to me how you
like; you know I'll still love you. But be nicer to Brian and Usama.

> > Remove initial_stack, and load RSP from current_task->thread.sp instead.
> 
>   
> >  #ifdef CONFIG_SMP
> > -       initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
> > +       current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
> 
> This lacks a comment about the temporary (ab)use of current->thread.sp

Ack.

> >         early_gdt_descr.address =
> >                         (unsigned long)get_cpu_gdt_rw(smp_processor_id());
> >         initial_gs = per_cpu_offset(smp_processor_id());
> > +       smpboot_control = smp_processor_id();
> >  #endif
> 
> > @@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> >         UNWIND_HINT_EMPTY
> >         ANNOTATE_NOENDBR // above
> >  
> > +#ifdef CONFIG_SMP
> > +       movl    smpboot_control(%rip), %ecx
> > +
> > +       /* Get the per cpu offset for the given CPU# which is in ECX */
> > +       movq    __per_cpu_offset(,%rcx,8), %rdx
> > +#else
> > +       xorl    %edx, %edx
> > +#endif /* CONFIG_SMP */
> 
> Sigh, we should finally make CONFIG_SMP def_bool y ...

Not today :)

> > +       /*
> > +        * Setup a boot time stack - Any secondary CPU will have lost its stack
> > +        * by now because the cr3-switch above unmaps the real-mode stack.
> > +        *
> > +        * RDX contains the per-cpu offset
> > +        */
> > +       movq    pcpu_hot + X86_current_task(%rdx), %rax
> > +       movq    TASK_threadsp(%rax), %rsp
> > +
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index b18c1385e181..62e3bf37f0b8 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> >         idle->thread.sp = (unsigned long)task_pt_regs(idle);
> >         early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> >         initial_code = (unsigned long)start_secondary;
> > -       initial_stack  = idle->thread.sp;
> > +
> > +       if (IS_ENABLED(CONFIG_X86_32)) {
> > +               initial_stack  = idle->thread.sp;
> > +       } else {
> > +               smpboot_control = cpu;
> > +       }
> 
> Please remove the pointless brackets.

I pondered that, but they only get added back again in the next patch.
It just seemed like adding pointless churn.

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

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[Thomas Gleixner]

On Tue, Feb 28 2023 at 16:25, David Woodhouse wrote:
> On Tue, 2023-02-28 at 17:13 +0100, Thomas Gleixner wrote:
>> Folks, really.
>
> Also, while those two lines *happen* to be my addition to Brian's
> commit message, I don't know if you knew that.

I knew because I read Brians patches _and_ I know your quick changelog
style by heart.

> Speak to me how you like; you know I'll still love you. But be nicer
> to Brian and Usama.

Hmm. I was not aware that 'Folks, really.' qualifies as not nice
nowadays.

>> > +#endif /* CONFIG_SMP */
>> 
>> Sigh, we should finally make CONFIG_SMP def_bool y ...
>
> Not today :)

Right, but it's overdue nevertheless to adjust with reality :)

>> > +       if (IS_ENABLED(CONFIG_X86_32)) {
>> > +               initial_stack  = idle->thread.sp;
>> > +       } else {
>> > +               smpboot_control = cpu;
>> > +       }
>> 
>> Please remove the pointless brackets.
>
> I pondered that, but they only get added back again in the next patch.
> It just seemed like adding pointless churn.

Fair enough.

Thanks,

        tglx

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[David Woodhouse]

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

On Tue, 2023-02-28 at 17:13 +0100, Thomas Gleixner wrote:
> On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> > From: Brian Gerst <brgerst@gmail.com>
> > 
> > Eliminating global variables from the CPU startup path in order to
> > simplify
> > it and facilitate parallel startup.
> 
> As this patch is now part of the parallel boot series and actually
> introduces smpboot_control, the above is neither accurate nor useful.

Better commit message, add a comment where we abuse current->thread.sp
in the sleep path. Didn't remove the {} which would be added back in
the very next patch. Pushed to my tree for Usama's next round.


From b5b9a2ab146cfd840f115b0455930767b6f6fcea Mon Sep 17 00:00:00 2001
From: Brian Gerst <brgerst@gmail.com>
Date: Fri, 24 Feb 2023 10:42:31 -0500
Subject: [PATCH 06/11] x86/smpboot: Remove initial_stack on 64-bit

In order to facilitate parallel startup, start to eliminate global variables
from the CPU startup path.

However, we start by introducing one more: smpboot_control. For now this
merely holds the CPU# of the CPU which is coming up. That CPU can then
find its own per-cpu data, and everything else it needs can be found from
there, allowing the other global variables to be removed.

First to be removed is initial_stack. Each CPU can load %rsp from its
current_task->thread.sp instead. That is already set up with the correct
idle thread for APs. Set up the .sp field in INIT_THREAD on x86 so that
the BSP also finds a suitable stack pointer in the static per-cpu data
when coming up on first boot.

On resume from S3, the CPU needs a temporary stack because its idle task
is already active. Instead of setting initial_stack, the sleep code can
simply set its own current->thread.sp to point to the temporary stack.
The true stack pointer will get restored with the rest of the CPU
context in do_suspend_lowlevel().

Signed-off-by: Brian Gerst <brgerst@gmail.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 arch/x86/include/asm/processor.h |  6 ++++-
 arch/x86/include/asm/smp.h       |  5 +++-
 arch/x86/kernel/acpi/sleep.c     | 17 +++++++++++--
 arch/x86/kernel/asm-offsets.c    |  1 +
 arch/x86/kernel/head_64.S        | 43 +++++++++++++++++++++-----------
 arch/x86/kernel/smpboot.c        |  7 +++++-
 arch/x86/xen/xen-head.S          |  2 +-
 7 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..bdde7316e75b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -648,7 +648,11 @@ static inline void spin_lock_prefetch(const void *x)
 #define KSTK_ESP(task)		(task_pt_regs(task)->sp)
 
 #else
-#define INIT_THREAD { }
+extern unsigned long __end_init_task[];
+
+#define INIT_THREAD {							    \
+	.sp	= (unsigned long)&__end_init_task - sizeof(struct pt_regs), \
+}
 
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index b4dbb20dab1a..bf2c51df9e0b 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,5 +199,8 @@ extern void nmi_selftest(void);
 #define nmi_selftest() do { } while (0)
 #endif
 
-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3b7f4cdbf2e0..23e44416dc04 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -111,13 +111,26 @@ int x86_acpi_suspend_lowlevel(void)
 	saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
+	/*
+	 * As each CPU starts up, it will find its own stack pointer
+	 * from its current_task->thread.sp. Typically that will be
+	 * the idle thread for a newly-started AP, or even the boot
+	 * CPU which will find it set to &init_task in the static
+	 * per-cpu data.
+	 *
+	 * Make the resuming CPU use the temporary stack at startup
+	 * by setting current->thread.sp to point to that. The true
+	 * %rsp will be restored with the rest of the CPU context,
+	 * by do_suspend_lowlevel().
+	 */
+	current->thread.sp = (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 = smp_processor_id();
 #endif
 	initial_code = (unsigned long)wakeup_long64;
-       saved_magic = 0x123456789abcdef0L;
+	saved_magic = 0x123456789abcdef0L;
 #endif /* CONFIG_64BIT */
 
 	/*
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 82c783da16a8..797ae1a15c91 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -108,6 +108,7 @@ static void __used common(void)
 	OFFSET(TSS_sp1, tss_struct, x86_tss.sp1);
 	OFFSET(TSS_sp2, tss_struct, x86_tss.sp2);
 	OFFSET(X86_top_of_stack, pcpu_hot, top_of_stack);
+	OFFSET(X86_current_task, pcpu_hot, current_task);
 #ifdef CONFIG_CALL_DEPTH_TRACKING
 	OFFSET(X86_call_depth, pcpu_hot, call_depth);
 #endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 222efd4a09bc..5a2417d788d1 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -61,8 +61,8 @@ SYM_CODE_START_NOALIGN(startup_64)
 	 * tables and then reload them.
 	 */
 
-	/* Set up the stack for verify_cpu(), similar to initial_stack below */
-	leaq	(__end_init_task - FRAME_SIZE)(%rip), %rsp
+	/* Set up the stack for verify_cpu() */
+	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
 	leaq	_text(%rip), %rdi
 
@@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	UNWIND_HINT_EMPTY
 	ANNOTATE_NOENDBR // above
 
+#ifdef CONFIG_SMP
+	movl	smpboot_control(%rip), %ecx
+
+	/* Get the per cpu offset for the given CPU# which is in ECX */
+	movq	__per_cpu_offset(,%rcx,8), %rdx
+#else
+	xorl	%edx, %edx
+#endif /* CONFIG_SMP */
+
+	/*
+	 * Setup a boot time stack - Any secondary CPU will have lost its stack
+	 * by now because the cr3-switch above unmaps the real-mode stack.
+	 *
+	 * RDX contains the per-cpu offset
+	 */
+	movq	pcpu_hot + X86_current_task(%rdx), %rax
+	movq	TASK_threadsp(%rax), %rsp
+
 	/*
 	 * We must switch to a new descriptor in kernel space for the GDT
 	 * because soon the kernel won't have access anymore to the userspace
@@ -275,12 +293,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	movl	initial_gs+4(%rip),%edx
 	wrmsr
 
-	/*
-	 * Setup a boot time stack - Any secondary CPU will have lost its stack
-	 * by now because the cr3-switch above unmaps the real-mode stack
-	 */
-	movq initial_stack(%rip), %rsp
-
 	/* Setup and Load IDT */
 	pushq	%rsi
 	call	early_setup_idt
@@ -372,7 +384,11 @@ SYM_CODE_END(secondary_startup_64)
 SYM_CODE_START(start_cpu0)
 	ANNOTATE_NOENDBR
 	UNWIND_HINT_EMPTY
-	movq	initial_stack(%rip), %rsp
+
+	/* Find the idle task stack */
+	movq	PER_CPU_VAR(pcpu_hot) + X86_current_task, %rcx
+	movq	TASK_threadsp(%rcx), %rsp
+
 	jmp	.Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif
@@ -420,12 +436,6 @@ SYM_DATA(initial_gs,	.quad INIT_PER_CPU_VAR(fixed_percpu_data))
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
 #endif
-
-/*
- * The FRAME_SIZE gap is a convention which helps the in-kernel unwinder
- * reliably detect the end of the stack.
- */
-SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
 	__FINITDATA
 
 	__INIT
@@ -660,6 +670,9 @@ SYM_DATA_END(level1_fixmap_pgt)
 SYM_DATA(early_gdt_descr,		.word GDT_ENTRIES*8-1)
 SYM_DATA_LOCAL(early_gdt_descr_base,	.quad INIT_PER_CPU_VAR(gdt_page))
 
+	.align 16
+SYM_DATA(smpboot_control,		.long 0)
+
 	.align 16
 /* This must match the first entry in level2_kernel_pgt */
 SYM_DATA(phys_base, .quad 0x0)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b18c1385e181..62e3bf37f0b8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);
 	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 	initial_code = (unsigned long)start_secondary;
-	initial_stack  = idle->thread.sp;
+
+	if (IS_ENABLED(CONFIG_X86_32)) {
+		initial_stack  = idle->thread.sp;
+	} else {
+		smpboot_control = cpu;
+	}
 
 	/* Enable the espfix hack for this CPU */
 	init_espfix_ap(cpu);
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ffaa62167f6e..6bd391476656 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -49,7 +49,7 @@ SYM_CODE_START(startup_xen)
 	ANNOTATE_NOENDBR
 	cld
 
-	mov initial_stack(%rip), %rsp
+	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
 	/* Set up %gs.
 	 *
-- 
2.39.0



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

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[Thomas Gleixner]

On Tue, Feb 28 2023 at 17:09, David Woodhouse wrote:
> On Tue, 2023-02-28 at 17:13 +0100, Thomas Gleixner wrote:
>> As this patch is now part of the parallel boot series and actually
>> introduces smpboot_control, the above is neither accurate nor useful.
>
> Better commit message, add a comment where we abuse current->thread.sp
> in the sleep path. Didn't remove the {} which would be added back in
> the very next patch. Pushed to my tree for Usama's next round.

Ok.

> However, we start by introducing one more: smpboot_control. For now this

s/we// :)

> merely holds the CPU# of the CPU which is coming up. That CPU can then
> find its own per-cpu data, and everything else it needs can be found from
> there, allowing the other global variables to be removed.
>
> First to be removed is initial_stack. Each CPU can load %rsp from its
> current_task->thread.sp instead. That is already set up with the correct
> idle thread for APs. Set up the .sp field in INIT_THREAD on x86 so that
> the BSP also finds a suitable stack pointer in the static per-cpu data
> when coming up on first boot.
>
> On resume from S3, the CPU needs a temporary stack because its idle task
> is already active. Instead of setting initial_stack, the sleep code can
> simply set its own current->thread.sp to point to the temporary stack.
> The true stack pointer will get restored with the rest of the CPU
> context in do_suspend_lowlevel().

Thanks for writing this up!

> +	/*
> +	 * As each CPU starts up, it will find its own stack pointer
> +	 * from its current_task->thread.sp. Typically that will be
> +	 * the idle thread for a newly-started AP, or even the boot
> +	 * CPU which will find it set to &init_task in the static
> +	 * per-cpu data.
> +	 *
> +	 * Make the resuming CPU use the temporary stack at startup
> +	 * by setting current->thread.sp to point to that. The true
> +	 * %rsp will be restored with the rest of the CPU context,
> +	 * by do_suspend_lowlevel().

Right, but what restores current->thread.sp? thread.sp is used by
unwinders...

Thanks,

        tglx

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[David Woodhouse]

On 28 February 2023 20:17:19 GMT, Thomas Gleixner <tglx@linutronix.de> wrote:
>On Tue, Feb 28 2023 at 17:09, David Woodhouse wrote:
>> On Tue, 2023-02-28 at 17:13 +0100, Thomas Gleixner wrote:
>>> As this patch is now part of the parallel boot series and actually
>>> introduces smpboot_control, the above is neither accurate nor useful.
>>
>> Better commit message, add a comment where we abuse current->thread.sp
>> in the sleep path. Didn't remove the {} which would be added back in
>> the very next patch. Pushed to my tree for Usama's next round.
>
>Ok.
>
>> However, we start by introducing one more: smpboot_control. For now this
>
>s/we// :)

Yeah, actually spotted that one as I hit send and it's different in the git tree already.


>> merely holds the CPU# of the CPU which is coming up. That CPU can then
>> find its own per-cpu data, and everything else it needs can be found from
>> there, allowing the other global variables to be removed.
>>
>> First to be removed is initial_stack. Each CPU can load %rsp from its
>> current_task->thread.sp instead. That is already set up with the correct
>> idle thread for APs. Set up the .sp field in INIT_THREAD on x86 so that
>> the BSP also finds a suitable stack pointer in the static per-cpu data
>> when coming up on first boot.
>>
>> On resume from S3, the CPU needs a temporary stack because its idle task
>> is already active. Instead of setting initial_stack, the sleep code can
>> simply set its own current->thread.sp to point to the temporary stack.
>> The true stack pointer will get restored with the rest of the CPU
>> context in do_suspend_lowlevel().
>
>Thanks for writing this up!
>
>> +	/*
>> +	 * As each CPU starts up, it will find its own stack pointer
>> +	 * from its current_task->thread.sp. Typically that will be
>> +	 * the idle thread for a newly-started AP, or even the boot
>> +	 * CPU which will find it set to &init_task in the static
>> +	 * per-cpu data.
>> +	 *
>> +	 * Make the resuming CPU use the temporary stack at startup
>> +	 * by setting current->thread.sp to point to that. The true
>> +	 * %rsp will be restored with the rest of the CPU context,
>> +	 * by do_suspend_lowlevel().
>
>Right, but what restores current->thread.sp? thread.sp is used by
>unwinders...

Unwinding a thread that is actually *on* the CPU? By the time it's taken off, won't ->thread.sp have been written out again? I figured it was just a dead variable while the actual %rsp was in use?

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[Thomas Gleixner]

On Tue, Feb 28 2023 at 20:43, David Woodhouse wrote:
> On 28 February 2023 20:17:19 GMT, Thomas Gleixner <tglx@linutronix.de> wrote:
>>> +	 * Make the resuming CPU use the temporary stack at startup
>>> +	 * by setting current->thread.sp to point to that. The true
>>> +	 * %rsp will be restored with the rest of the CPU context,
>>> +	 * by do_suspend_lowlevel().
>>
>>Right, but what restores current->thread.sp? thread.sp is used by
>>unwinders...
>
> Unwinding a thread that is actually *on* the CPU?

No.

> By the time it's taken off, won't ->thread.sp have been written out
> again? I figured it was just a dead variable while the actual %rsp was
> in use?

Yes. It's not used when the thread is on the CPU. And you are right,
it's saved and restored in switch_to(). Can you please add a comment to
that effect?

Thanks,

        tglx

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[David Woodhouse]

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

On Wed, 2023-03-01 at 11:18 +0100, Thomas Gleixner wrote:
> 
> > By the time it's taken off, won't ->thread.sp have been written out
> > again? I figured it was just a dead variable while the actual %rsp was
> > in use?
> 
> Yes. It's not used when the thread is on the CPU. And you are right,
> it's saved and restored in switch_to(). Can you please add a comment to
> that effect?

Pushed to my v12bis branch:

From ac86ad32434d4661db0bef6791b7953d369585e2 Mon Sep 17 00:00:00 2001
From: Brian Gerst <brgerst@gmail.com>
Date: Fri, 24 Feb 2023 10:42:31 -0500
Subject: [PATCH 06/11] x86/smpboot: Remove initial_stack on 64-bit

In order to facilitate parallel startup, start to eliminate some of the
global variables passing information to CPUs in the startup path.

However, start by introducing one more: smpboot_control. For now this
merely holds the CPU# of the CPU which is coming up. Each CPU can then
find its own per-cpu data, and everything else it needs can be found
from there, allowing the other global variables to be removed.

First to be removed is initial_stack. Each CPU can load %rsp from its
current_task->thread.sp instead. That is already set up with the correct
idle thread for APs. Set up the .sp field in INIT_THREAD on x86 so that
the BSP also finds a suitable stack pointer in the static per-cpu data
when coming up on first boot.

On resume from S3, the CPU needs a temporary stack because its idle task
is already active. Instead of setting initial_stack, the sleep code can
simply set its own current->thread.sp to point to the temporary stack.
Nobody else cares about ->thread.sp for a thread which is currently on
a CPU, because the true value is actually in the %rsp register. Which
is restored with the rest of the CPU context in do_suspend_lowlevel().

Signed-off-by: Brian Gerst <brgerst@gmail.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 arch/x86/include/asm/processor.h |  6 ++++-
 arch/x86/include/asm/smp.h       |  5 +++-
 arch/x86/kernel/acpi/sleep.c     | 20 +++++++++++++--
 arch/x86/kernel/asm-offsets.c    |  1 +
 arch/x86/kernel/head_64.S        | 43 +++++++++++++++++++++-----------
 arch/x86/kernel/smpboot.c        |  7 +++++-
 arch/x86/xen/xen-head.S          |  2 +-
 7 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..bdde7316e75b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -648,7 +648,11 @@ static inline void spin_lock_prefetch(const void *x)
 #define KSTK_ESP(task)		(task_pt_regs(task)->sp)
 
 #else
-#define INIT_THREAD { }
+extern unsigned long __end_init_task[];
+
+#define INIT_THREAD {							    \
+	.sp	= (unsigned long)&__end_init_task - sizeof(struct pt_regs), \
+}
 
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index b4dbb20dab1a..bf2c51df9e0b 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,5 +199,8 @@ extern void nmi_selftest(void);
 #define nmi_selftest() do { } while (0)
 #endif
 
-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3b7f4cdbf2e0..1b4c43d0819a 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -111,13 +111,29 @@ int x86_acpi_suspend_lowlevel(void)
 	saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
+	/*
+	 * As each CPU starts up, it will find its own stack pointer
+	 * from its current_task->thread.sp. Typically that will be
+	 * the idle thread for a newly-started AP, or even the boot
+	 * CPU which will find it set to &init_task in the static
+	 * per-cpu data.
+	 *
+	 * Make the resuming CPU use the temporary stack at startup
+	 * by setting current->thread.sp to point to that. The true
+	 * %rsp will be restored with the rest of the CPU context,
+	 * by do_suspend_lowlevel(). And unwinders don't care about
+	 * the abuse of ->thread.sp because it's a dead variable
+	 * while the thread is running on the CPU anyway; the true
+	 * value is in the actual %rsp register.
+	 */
+	current->thread.sp = (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 = smp_processor_id();
 #endif
 	initial_code = (unsigned long)wakeup_long64;
-       saved_magic = 0x123456789abcdef0L;
+	saved_magic = 0x123456789abcdef0L;
 #endif /* CONFIG_64BIT */
 
 	/*
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 82c783da16a8..797ae1a15c91 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -108,6 +108,7 @@ static void __used common(void)
 	OFFSET(TSS_sp1, tss_struct, x86_tss.sp1);
 	OFFSET(TSS_sp2, tss_struct, x86_tss.sp2);
 	OFFSET(X86_top_of_stack, pcpu_hot, top_of_stack);
+	OFFSET(X86_current_task, pcpu_hot, current_task);
 #ifdef CONFIG_CALL_DEPTH_TRACKING
 	OFFSET(X86_call_depth, pcpu_hot, call_depth);
 #endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 222efd4a09bc..0b34f6a9221b 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -61,8 +61,8 @@ SYM_CODE_START_NOALIGN(startup_64)
 	 * tables and then reload them.
 	 */
 
-	/* Set up the stack for verify_cpu(), similar to initial_stack below */
-	leaq	(__end_init_task - FRAME_SIZE)(%rip), %rsp
+	/* Set up the stack for verify_cpu() */
+	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
 	leaq	_text(%rip), %rdi
 
@@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	UNWIND_HINT_EMPTY
 	ANNOTATE_NOENDBR // above
 
+#ifdef CONFIG_SMP
+	movl	smpboot_control(%rip), %ecx
+
+	/* Get the per cpu offset for the given CPU# which is in ECX */
+	movq	__per_cpu_offset(,%rcx,8), %rdx
+#else
+	xorq	%edx, %edx /* zero-extended to clear all of RDX */
+#endif /* CONFIG_SMP */
+
+	/*
+	 * Setup a boot time stack - Any secondary CPU will have lost its stack
+	 * by now because the cr3-switch above unmaps the real-mode stack.
+	 *
+	 * RDX contains the per-cpu offset
+	 */
+	movq	pcpu_hot + X86_current_task(%rdx), %rax
+	movq	TASK_threadsp(%rax), %rsp
+
 	/*
 	 * We must switch to a new descriptor in kernel space for the GDT
 	 * because soon the kernel won't have access anymore to the userspace
@@ -275,12 +293,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	movl	initial_gs+4(%rip),%edx
 	wrmsr
 
-	/*
-	 * Setup a boot time stack - Any secondary CPU will have lost its stack
-	 * by now because the cr3-switch above unmaps the real-mode stack
-	 */
-	movq initial_stack(%rip), %rsp
-
 	/* Setup and Load IDT */
 	pushq	%rsi
 	call	early_setup_idt
@@ -372,7 +384,11 @@ SYM_CODE_END(secondary_startup_64)
 SYM_CODE_START(start_cpu0)
 	ANNOTATE_NOENDBR
 	UNWIND_HINT_EMPTY
-	movq	initial_stack(%rip), %rsp
+
+	/* Find the idle task stack */
+	movq	PER_CPU_VAR(pcpu_hot) + X86_current_task, %rcx
+	movq	TASK_threadsp(%rcx), %rsp
+
 	jmp	.Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif
@@ -420,12 +436,6 @@ SYM_DATA(initial_gs,	.quad INIT_PER_CPU_VAR(fixed_percpu_data))
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
 #endif
-
-/*
- * The FRAME_SIZE gap is a convention which helps the in-kernel unwinder
- * reliably detect the end of the stack.
- */
-SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
 	__FINITDATA
 
 	__INIT
@@ -660,6 +670,9 @@ SYM_DATA_END(level1_fixmap_pgt)
 SYM_DATA(early_gdt_descr,		.word GDT_ENTRIES*8-1)
 SYM_DATA_LOCAL(early_gdt_descr_base,	.quad INIT_PER_CPU_VAR(gdt_page))
 
+	.align 16
+SYM_DATA(smpboot_control,		.long 0)
+
 	.align 16
 /* This must match the first entry in level2_kernel_pgt */
 SYM_DATA(phys_base, .quad 0x0)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b18c1385e181..62e3bf37f0b8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);
 	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 	initial_code = (unsigned long)start_secondary;
-	initial_stack  = idle->thread.sp;
+
+	if (IS_ENABLED(CONFIG_X86_32)) {
+		initial_stack  = idle->thread.sp;
+	} else {
+		smpboot_control = cpu;
+	}
 
 	/* Enable the espfix hack for this CPU */
 	init_espfix_ap(cpu);
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ffaa62167f6e..6bd391476656 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -49,7 +49,7 @@ SYM_CODE_START(startup_xen)
 	ANNOTATE_NOENDBR
 	cld
 
-	mov initial_stack(%rip), %rsp
+	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
 	/* Set up %gs.
 	 *
-- 
2.39.0



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

Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

[Brian Gerst]

On Tue, Feb 28, 2023 at 11:13 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> > From: Brian Gerst <brgerst@gmail.com>
> >
> > Eliminating global variables from the CPU startup path in order to simplify
> > it and facilitate parallel startup.
>
> As this patch is now part of the parallel boot series and actually
> introduces smpboot_control, the above is neither accurate nor useful.
>
> Folks, really.
>
> > Remove initial_stack, and load RSP from current_task->thread.sp instead.
>
>
> >  #ifdef CONFIG_SMP
> > -     initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
> > +     current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
>
> This lacks a comment about the temporary (ab)use of current->thread.sp

task->thread.sp represents the saved stack pointer of a sleeping or
newly forked task (see __switch_to_asm()), and the boot cpu's idle
task is effectively going to sleep.  Using a temporary stack for
resume is kind of a hack to workaround the fact that the idle task
stack is in use already, but improving that is out of scope for this
series.

--
Brian Gerst

[PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Usama Arif]

From: Brian Gerst <brgerst@gmail.com>

Build the GDT descriptor on the stack instead.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/acpi/sleep.c |  2 --
 arch/x86/kernel/head_64.S    | 11 ++++++-----
 arch/x86/kernel/smpboot.c    |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index ab6e29b32c04..236f2423454d 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -112,8 +112,6 @@ int x86_acpi_suspend_lowlevel(void)
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
 	current->thread.sp = (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 = smp_processor_id();
 #endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5a2417d788d1..0ccca297e90e 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -265,7 +265,12 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	 * addresses where we're currently running on. We have to do that here
 	 * because in 32bit we couldn't load a 64bit linear address.
 	 */
-	lgdt	early_gdt_descr(%rip)
+	subq	$16, %rsp
+	movw	$(GDT_SIZE-1), (%rsp)
+	leaq	gdt_page(%rdx), %rax
+	movq	%rax, 2(%rsp)
+	lgdt	(%rsp)
+	addq	$16, %rsp
 
 	/* set up data segments */
 	xorl %eax,%eax
@@ -667,10 +672,6 @@ SYM_DATA_END(level1_fixmap_pgt)
 	.data
 	.align 16
 
-SYM_DATA(early_gdt_descr,		.word GDT_ENTRIES*8-1)
-SYM_DATA_LOCAL(early_gdt_descr_base,	.quad INIT_PER_CPU_VAR(gdt_page))
-
-	.align 16
 SYM_DATA(smpboot_control,		.long 0)
 
 	.align 16
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 62e3bf37f0b8..a22460a07cf8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1110,10 +1110,10 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		start_ip = real_mode_header->trampoline_start64;
 #endif
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);
-	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 	initial_code = (unsigned long)start_secondary;
 
 	if (IS_ENABLED(CONFIG_X86_32)) {
+		early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 		initial_stack  = idle->thread.sp;
 	} else {
 		smpboot_control = cpu;
-- 
2.25.1

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Thomas Gleixner]

On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> @@ -265,7 +265,12 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>  	 * addresses where we're currently running on. We have to do that here
>  	 * because in 32bit we couldn't load a 64bit linear address.
>  	 */
> -	lgdt	early_gdt_descr(%rip)
> +	subq	$16, %rsp
> +	movw	$(GDT_SIZE-1), (%rsp)
> +	leaq	gdt_page(%rdx), %rax

Even on !SMP gdt_page is in the 0...__per_cpu_end range. Which means
that on !SMP this results in:

      leaq    0xb000(%rdx),%rax

and RDX is 0. That's not really a valid GDT pointer, right?

> +	movq	%rax, 2(%rsp)
> +	lgdt	(%rsp)

and obviously that's equally broken for the task stack part:

>	movq	pcpu_hot + X86_current_task(%rdx), %rax

This needs:

--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -239,7 +239,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_
 	/* Get the per cpu offset for the given CPU# which is in ECX */
 	movq	__per_cpu_offset(,%rcx,8), %rdx
 #else
-	xorl	%edx, %edx
+	leaq	INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
 #endif /* CONFIG_SMP */
 
 	/*

in the initial_stack patch, which then allows to remove this hunk in the
initial_gs patch:

@@ -286,9 +286,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_
 	 * the per cpu areas are set up.
 	 */
 	movl	$MSR_GS_BASE,%ecx
-#ifndef CONFIG_SMP
-	leaq	INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
-#endif
 	movl	%edx, %eax
 	shrq	$32, %rdx
 	wrmsr

Maybe we should enforce CONFIG_SMP=y first :)

Thanks,

        tglx

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Arjan van de Ven]

> 
> Maybe we should enforce CONFIG_SMP=y first :)
> 
> Thanks,

for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
maybe even for 32 bit if it just makes the code simpler I suppose

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Josh Triplett]

On Tue, Feb 28, 2023 at 01:02:33PM -0800, Arjan van de Ven wrote:
  Thomas Gleixner wrote:
> > 
> > Maybe we should enforce CONFIG_SMP=y first :)
> > 
> > Thanks,
> 
> for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
> maybe even for 32 bit if it just makes the code simpler I suppose

As one of the folks keeping an eye on tinyconfig and kernel size, I
actually think we *should* make this change and rip out !CONFIG_SMP,
albeit carefully.

In particular, I would propose that we rip out !CONFIG_SMP, *but* we
allow building with CONFIG_NR_CPUS=1. (And we could make sure in that
case that the compiler can recognize that at compile time and optimize
accordingly, so that it might provide some of the UP optimizations for
us.)

Then, any *optimizations* for the "will only have one CPU, ever" case
can move to CONFIG_NR_CPUS=1 rather than !CONFIG_SMP. I think many of
those optimizations may be worth keeping for small embedded systems, or
for cases like Linux-as-bootloader or similar.

The difference here would be that code written for !CONFIG_SMP today
needs to account for the UP case for *correctness*, whereas code written
for CONFIG_SMP can *optionally* consider CONFIG_NR_CPUS=1 for
*performance*.

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Paul E. McKenney]

On Wed, Mar 01, 2023 at 12:32:26PM -0800, Josh Triplett wrote:
> On Tue, Feb 28, 2023 at 01:02:33PM -0800, Arjan van de Ven wrote:
>   Thomas Gleixner wrote:
> > > 
> > > Maybe we should enforce CONFIG_SMP=y first :)
> > > 
> > > Thanks,
> > 
> > for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
> > maybe even for 32 bit if it just makes the code simpler I suppose
> 
> As one of the folks keeping an eye on tinyconfig and kernel size, I
> actually think we *should* make this change and rip out !CONFIG_SMP,
> albeit carefully.
> 
> In particular, I would propose that we rip out !CONFIG_SMP, *but* we
> allow building with CONFIG_NR_CPUS=1. (And we could make sure in that
> case that the compiler can recognize that at compile time and optimize
> accordingly, so that it might provide some of the UP optimizations for
> us.)
> 
> Then, any *optimizations* for the "will only have one CPU, ever" case
> can move to CONFIG_NR_CPUS=1 rather than !CONFIG_SMP. I think many of
> those optimizations may be worth keeping for small embedded systems, or
> for cases like Linux-as-bootloader or similar.
> 
> The difference here would be that code written for !CONFIG_SMP today
> needs to account for the UP case for *correctness*, whereas code written
> for CONFIG_SMP can *optionally* consider CONFIG_NR_CPUS=1 for
> *performance*.

It certainly would not make much sense to keep Tiny RCU and Tiny SRCU
around if there was no CONFIG_SMP=n.

It should be interesting seeing what comes up out of the IoT space.  ;-)

							Thanx, Paul

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Josh Triplett]

On Wed, Mar 01, 2023 at 02:16:32PM -0800, Paul E. McKenney wrote:
> On Wed, Mar 01, 2023 at 12:32:26PM -0800, Josh Triplett wrote:
> > On Tue, Feb 28, 2023 at 01:02:33PM -0800, Arjan van de Ven wrote:
> >   Thomas Gleixner wrote:
> > > > 
> > > > Maybe we should enforce CONFIG_SMP=y first :)
> > > > 
> > > > Thanks,
> > > 
> > > for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
> > > maybe even for 32 bit if it just makes the code simpler I suppose
> > 
> > As one of the folks keeping an eye on tinyconfig and kernel size, I
> > actually think we *should* make this change and rip out !CONFIG_SMP,
> > albeit carefully.
> > 
> > In particular, I would propose that we rip out !CONFIG_SMP, *but* we
> > allow building with CONFIG_NR_CPUS=1. (And we could make sure in that
> > case that the compiler can recognize that at compile time and optimize
> > accordingly, so that it might provide some of the UP optimizations for
> > us.)
> > 
> > Then, any *optimizations* for the "will only have one CPU, ever" case
> > can move to CONFIG_NR_CPUS=1 rather than !CONFIG_SMP. I think many of
> > those optimizations may be worth keeping for small embedded systems, or
> > for cases like Linux-as-bootloader or similar.
> > 
> > The difference here would be that code written for !CONFIG_SMP today
> > needs to account for the UP case for *correctness*, whereas code written
> > for CONFIG_SMP can *optionally* consider CONFIG_NR_CPUS=1 for
> > *performance*.
> 
> It certainly would not make much sense to keep Tiny RCU and Tiny SRCU
> around if there was no CONFIG_SMP=n.

On the contrary, I think it's entirely appropriate to keep them for
CONFIG_NR_CPUS=1; that's exactly the kind of simple optimization that
seems well worth having. (Ideal optimization: "very very simple for UP,
complex for SMP"; non-ideal optimization: "complex for SMP, differently
complex for UP".)

- Josh

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Paul E. McKenney]

On Wed, Mar 01, 2023 at 02:25:36PM -0800, Josh Triplett wrote:
> On Wed, Mar 01, 2023 at 02:16:32PM -0800, Paul E. McKenney wrote:
> > On Wed, Mar 01, 2023 at 12:32:26PM -0800, Josh Triplett wrote:
> > > On Tue, Feb 28, 2023 at 01:02:33PM -0800, Arjan van de Ven wrote:
> > >   Thomas Gleixner wrote:
> > > > > 
> > > > > Maybe we should enforce CONFIG_SMP=y first :)
> > > > > 
> > > > > Thanks,
> > > > 
> > > > for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
> > > > maybe even for 32 bit if it just makes the code simpler I suppose
> > > 
> > > As one of the folks keeping an eye on tinyconfig and kernel size, I
> > > actually think we *should* make this change and rip out !CONFIG_SMP,
> > > albeit carefully.
> > > 
> > > In particular, I would propose that we rip out !CONFIG_SMP, *but* we
> > > allow building with CONFIG_NR_CPUS=1. (And we could make sure in that
> > > case that the compiler can recognize that at compile time and optimize
> > > accordingly, so that it might provide some of the UP optimizations for
> > > us.)
> > > 
> > > Then, any *optimizations* for the "will only have one CPU, ever" case
> > > can move to CONFIG_NR_CPUS=1 rather than !CONFIG_SMP. I think many of
> > > those optimizations may be worth keeping for small embedded systems, or
> > > for cases like Linux-as-bootloader or similar.
> > > 
> > > The difference here would be that code written for !CONFIG_SMP today
> > > needs to account for the UP case for *correctness*, whereas code written
> > > for CONFIG_SMP can *optionally* consider CONFIG_NR_CPUS=1 for
> > > *performance*.
> > 
> > It certainly would not make much sense to keep Tiny RCU and Tiny SRCU
> > around if there was no CONFIG_SMP=n.
> 
> On the contrary, I think it's entirely appropriate to keep them for
> CONFIG_NR_CPUS=1; that's exactly the kind of simple optimization that
> seems well worth having. (Ideal optimization: "very very simple for UP,
> complex for SMP"; non-ideal optimization: "complex for SMP, differently
> complex for UP".)

Fair enough, but how does removing CONFIG_SMP help with that?  Given that
it is not all that hard to work around the lack of CONFIG_SMP for Tiny
RCU and Tiny SRCU, then it cannot be all that hard to work around that
lack for the use cases that you are trying to get rid of, right?

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
index 9071182b1284b..7487bee3d4341 100644
--- a/kernel/rcu/Kconfig
+++ b/kernel/rcu/Kconfig
@@ -7,7 +7,7 @@ menu "RCU Subsystem"
 
 config TREE_RCU
 	bool
-	default y if SMP
+	default y if CONFIG_NR_CPUS = 1
 	# Dynticks-idle tracking
 	select CONTEXT_TRACKING_IDLE
 	help
@@ -31,7 +31,7 @@ config PREEMPT_RCU
 
 config TINY_RCU
 	bool
-	default y if !PREEMPTION && !SMP
+	default y if !PREEMPTION && CONFIG_NR_CPUS != 1
 	help
 	  This option selects the RCU implementation that is
 	  designed for UP systems from which real-time response

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Randy Dunlap]

On 3/1/23 16:28, Paul E. McKenney wrote:
> On Wed, Mar 01, 2023 at 02:25:36PM -0800, Josh Triplett wrote:
>> On Wed, Mar 01, 2023 at 02:16:32PM -0800, Paul E. McKenney wrote:
>>> On Wed, Mar 01, 2023 at 12:32:26PM -0800, Josh Triplett wrote:
>>>> On Tue, Feb 28, 2023 at 01:02:33PM -0800, Arjan van de Ven wrote:
>>>>   Thomas Gleixner wrote:
>>>>>>
>>>>>> Maybe we should enforce CONFIG_SMP=y first :)
>>>>>>
>>>>>> Thanks,
>>>>>
>>>>> for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
>>>>> maybe even for 32 bit if it just makes the code simpler I suppose
>>>>
>>>> As one of the folks keeping an eye on tinyconfig and kernel size, I
>>>> actually think we *should* make this change and rip out !CONFIG_SMP,
>>>> albeit carefully.
>>>>
>>>> In particular, I would propose that we rip out !CONFIG_SMP, *but* we
>>>> allow building with CONFIG_NR_CPUS=1. (And we could make sure in that
>>>> case that the compiler can recognize that at compile time and optimize
>>>> accordingly, so that it might provide some of the UP optimizations for
>>>> us.)
>>>>
>>>> Then, any *optimizations* for the "will only have one CPU, ever" case
>>>> can move to CONFIG_NR_CPUS=1 rather than !CONFIG_SMP. I think many of
>>>> those optimizations may be worth keeping for small embedded systems, or
>>>> for cases like Linux-as-bootloader or similar.
>>>>
>>>> The difference here would be that code written for !CONFIG_SMP today
>>>> needs to account for the UP case for *correctness*, whereas code written
>>>> for CONFIG_SMP can *optionally* consider CONFIG_NR_CPUS=1 for
>>>> *performance*.
>>>
>>> It certainly would not make much sense to keep Tiny RCU and Tiny SRCU
>>> around if there was no CONFIG_SMP=n.
>>
>> On the contrary, I think it's entirely appropriate to keep them for
>> CONFIG_NR_CPUS=1; that's exactly the kind of simple optimization that
>> seems well worth having. (Ideal optimization: "very very simple for UP,
>> complex for SMP"; non-ideal optimization: "complex for SMP, differently
>> complex for UP".)
> 
> Fair enough, but how does removing CONFIG_SMP help with that?  Given that
> it is not all that hard to work around the lack of CONFIG_SMP for Tiny
> RCU and Tiny SRCU, then it cannot be all that hard to work around that
> lack for the use cases that you are trying to get rid of, right?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> index 9071182b1284b..7487bee3d4341 100644
> --- a/kernel/rcu/Kconfig
> +++ b/kernel/rcu/Kconfig
> @@ -7,7 +7,7 @@ menu "RCU Subsystem"
>  
>  config TREE_RCU
>  	bool
> -	default y if SMP
> +	default y if CONFIG_NR_CPUS = 1
>  	# Dynticks-idle tracking
>  	select CONTEXT_TRACKING_IDLE
>  	help
> @@ -31,7 +31,7 @@ config PREEMPT_RCU
>  
>  config TINY_RCU
>  	bool
> -	default y if !PREEMPTION && !SMP
> +	default y if !PREEMPTION && CONFIG_NR_CPUS != 1
>  	help
>  	  This option selects the RCU implementation that is
>  	  designed for UP systems from which real-time response

but drop the CONFIG_ prefixes...

-- 
~Randy

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Paul E. McKenney]

On Wed, Mar 01, 2023 at 05:05:39PM -0800, Randy Dunlap wrote:
> 
> 
> On 3/1/23 16:28, Paul E. McKenney wrote:
> > On Wed, Mar 01, 2023 at 02:25:36PM -0800, Josh Triplett wrote:
> >> On Wed, Mar 01, 2023 at 02:16:32PM -0800, Paul E. McKenney wrote:
> >>> On Wed, Mar 01, 2023 at 12:32:26PM -0800, Josh Triplett wrote:
> >>>> On Tue, Feb 28, 2023 at 01:02:33PM -0800, Arjan van de Ven wrote:
> >>>>   Thomas Gleixner wrote:
> >>>>>>
> >>>>>> Maybe we should enforce CONFIG_SMP=y first :)
> >>>>>>
> >>>>>> Thanks,
> >>>>>
> >>>>> for 64 bit I can see the point of removing the !SMP case entirely from arch/x86 .
> >>>>> maybe even for 32 bit if it just makes the code simpler I suppose
> >>>>
> >>>> As one of the folks keeping an eye on tinyconfig and kernel size, I
> >>>> actually think we *should* make this change and rip out !CONFIG_SMP,
> >>>> albeit carefully.
> >>>>
> >>>> In particular, I would propose that we rip out !CONFIG_SMP, *but* we
> >>>> allow building with CONFIG_NR_CPUS=1. (And we could make sure in that
> >>>> case that the compiler can recognize that at compile time and optimize
> >>>> accordingly, so that it might provide some of the UP optimizations for
> >>>> us.)
> >>>>
> >>>> Then, any *optimizations* for the "will only have one CPU, ever" case
> >>>> can move to CONFIG_NR_CPUS=1 rather than !CONFIG_SMP. I think many of
> >>>> those optimizations may be worth keeping for small embedded systems, or
> >>>> for cases like Linux-as-bootloader or similar.
> >>>>
> >>>> The difference here would be that code written for !CONFIG_SMP today
> >>>> needs to account for the UP case for *correctness*, whereas code written
> >>>> for CONFIG_SMP can *optionally* consider CONFIG_NR_CPUS=1 for
> >>>> *performance*.
> >>>
> >>> It certainly would not make much sense to keep Tiny RCU and Tiny SRCU
> >>> around if there was no CONFIG_SMP=n.
> >>
> >> On the contrary, I think it's entirely appropriate to keep them for
> >> CONFIG_NR_CPUS=1; that's exactly the kind of simple optimization that
> >> seems well worth having. (Ideal optimization: "very very simple for UP,
> >> complex for SMP"; non-ideal optimization: "complex for SMP, differently
> >> complex for UP".)
> > 
> > Fair enough, but how does removing CONFIG_SMP help with that?  Given that
> > it is not all that hard to work around the lack of CONFIG_SMP for Tiny
> > RCU and Tiny SRCU, then it cannot be all that hard to work around that
> > lack for the use cases that you are trying to get rid of, right?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> > index 9071182b1284b..7487bee3d4341 100644
> > --- a/kernel/rcu/Kconfig
> > +++ b/kernel/rcu/Kconfig
> > @@ -7,7 +7,7 @@ menu "RCU Subsystem"
> >  
> >  config TREE_RCU
> >  	bool
> > -	default y if SMP
> > +	default y if CONFIG_NR_CPUS = 1
> >  	# Dynticks-idle tracking
> >  	select CONTEXT_TRACKING_IDLE
> >  	help
> > @@ -31,7 +31,7 @@ config PREEMPT_RCU
> >  
> >  config TINY_RCU
> >  	bool
> > -	default y if !PREEMPTION && !SMP
> > +	default y if !PREEMPTION && CONFIG_NR_CPUS != 1
> >  	help
> >  	  This option selects the RCU implementation that is
> >  	  designed for UP systems from which real-time response
> 
> but drop the CONFIG_ prefixes...

Indeed.  What I don't understand is how the above passed some light
rcutorture testing.  And the comparisons are backwards as well.
Perhaps this time two bugs make working code?

How about the following?

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
index 9071182b1284b..a2ba97b949498 100644
--- a/kernel/rcu/Kconfig
+++ b/kernel/rcu/Kconfig
@@ -7,7 +7,7 @@ menu "RCU Subsystem"
 
 config TREE_RCU
 	bool
-	default y if SMP
+	default y if NR_CPUS != 1
 	# Dynticks-idle tracking
 	select CONTEXT_TRACKING_IDLE
 	help
@@ -31,7 +31,7 @@ config PREEMPT_RCU
 
 config TINY_RCU
 	bool
-	default y if !PREEMPTION && !SMP
+	default y if !PREEMPTION && NR_CPUS = 1
 	help
 	  This option selects the RCU implementation that is
 	  designed for UP systems from which real-time response

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[David Woodhouse]

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

On Tue, 2023-02-28 at 22:01 +0100, Thomas Gleixner wrote:
> 
> This needs:
> 
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -239,7 +239,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_
>         /* Get the per cpu offset for the given CPU# which is in ECX */
>         movq    __per_cpu_offset(,%rcx,8), %rdx
>  #else
> -       xorl    %edx, %edx
> +       leaq    INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
>  #endif /* CONFIG_SMP */
>  
>         /*
> 

So yeah, I'd frowned at the zero %edx there for a while (but evidently
not long enough to realise that even if it's right it should be %rdx).

But then figured it was an *offset*, that __per_cpu_offset[0] would be
zero too at least on first boot and probably forever, did a quick grep
for pcpu_hot and figured my brain hurt and Brian probably knew what he
was doing.

Empirically, yours doesn't boot and Brian's does. If I just fix it to
zero all of %rdx and then run (just up to the initial_gs patch) with
qemu -d in_asm ...

 grep gdt_page System.map 
ffffffff05d7a000 A init_per_cpu__gdt_page
ffffffff825cfeb0 r __ksymtab_gdt_page
ffffffff82811000 D gdt_page


0x060000a9:  48 c7 c0 b2 00 00 a2     movq     $-0x5dffff4e, %rax
0x060000b0:  ff e0                    jmpq     *%rax

----------------
IN: 
0xffffffffa20000b2:  48 31 d2                 xorq     %rdx, %rdx
0xffffffffa20000b5:  48 8b 82 c0 74 d5 a3     movq     -0x5c2a8b40(%rdx), %rax
0xffffffffa20000bc:  48 8b a0 58 14 00 00     movq     0x1458(%rax), %rsp
0xffffffffa20000c3:  48 83 ec 10              subq     $0x10, %rsp
0xffffffffa20000c7:  66 c7 04 24 7f 00        movw     $0x7f, (%rsp)
0xffffffffa20000cd:  48 8d 82 00 10 81 a3     leaq     -0x5c7ef000(%rdx), %rax
0xffffffffa20000d4:  48 89 44 24 02           movq     %rax, 2(%rsp)
0xffffffffa20000d9:  0f 01 14 24              lgdtq    (%rsp)
0xffffffffa20000dd:  48 83 c4 10              addq     $0x10, %rsp
0xffffffffa20000e1:  31 c0                    xorl     %eax, %eax
0xffffffffa20000e3:  8e d8                    movl     %eax, %ds

I cannot work out where the value -0x5c7ef000 comes from, but it
doesn't seem to be the 0xb000 you claimed, and my brain is hurting
again...


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

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[David Woodhouse]

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

On Tue, 2023-02-28 at 21:57 +0000, David Woodhouse wrote:
> 
> ----------------
> IN: 
> 0xffffffffa20000b2:  48 31 d2                 xorq     %rdx, %rdx
> 0xffffffffa20000b5:  48 8b 82 c0 74 d5 a3     movq     -0x5c2a8b40(%rdx), %rax
> 0xffffffffa20000bc:  48 8b a0 58 14 00 00     movq     0x1458(%rax), %rsp
> 0xffffffffa20000c3:  48 83 ec 10              subq     $0x10, %rsp
> 0xffffffffa20000c7:  66 c7 04 24 7f 00        movw     $0x7f, (%rsp)
> 0xffffffffa20000cd:  48 8d 82 00 10 81 a3     leaq     -0x5c7ef000(%rdx), %rax
> 0xffffffffa20000d4:  48 89 44 24 02           movq     %rax, 2(%rsp)
> 0xffffffffa20000d9:  0f 01 14 24              lgdtq    (%rsp)
> 0xffffffffa20000dd:  48 83 c4 10              addq     $0x10, %rsp
> 0xffffffffa20000e1:  31 c0                    xorl     %eax, %eax
> 0xffffffffa20000e3:  8e d8                    movl     %eax, %ds
> 
> I cannot work out where the value -0x5c7ef000 comes from, but it
> doesn't seem to be the 0xb000 you claimed, and my brain is hurting
> again...

Turning off CONFIG_RANDOMIZE_BASE (or just looking at the vmlinux
disassembly instead as Brian did) helps to resolve that FWIW.

I've changed it to zero all of %rdx and pushed it back to the v12bis
branch.



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

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Brian Gerst]

On Tue, Feb 28, 2023 at 5:41 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Tue, 2023-02-28 at 21:57 +0000, David Woodhouse wrote:
> >
> > ----------------
> > IN:
> > 0xffffffffa20000b2:  48 31 d2                 xorq     %rdx, %rdx
> > 0xffffffffa20000b5:  48 8b 82 c0 74 d5 a3     movq     -0x5c2a8b40(%rdx), %rax
> > 0xffffffffa20000bc:  48 8b a0 58 14 00 00     movq     0x1458(%rax), %rsp
> > 0xffffffffa20000c3:  48 83 ec 10              subq     $0x10, %rsp
> > 0xffffffffa20000c7:  66 c7 04 24 7f 00        movw     $0x7f, (%rsp)
> > 0xffffffffa20000cd:  48 8d 82 00 10 81 a3     leaq     -0x5c7ef000(%rdx), %rax
> > 0xffffffffa20000d4:  48 89 44 24 02           movq     %rax, 2(%rsp)
> > 0xffffffffa20000d9:  0f 01 14 24              lgdtq    (%rsp)
> > 0xffffffffa20000dd:  48 83 c4 10              addq     $0x10, %rsp
> > 0xffffffffa20000e1:  31 c0                    xorl     %eax, %eax
> > 0xffffffffa20000e3:  8e d8                    movl     %eax, %ds
> >
> > I cannot work out where the value -0x5c7ef000 comes from, but it
> > doesn't seem to be the 0xb000 you claimed, and my brain is hurting
> > again...
>
> Turning off CONFIG_RANDOMIZE_BASE (or just looking at the vmlinux
> disassembly instead as Brian did) helps to resolve that FWIW.
>
> I've changed it to zero all of %rdx and pushed it back to the v12bis
> branch.

xorl %edx, %edx is preferred, as a 32-bit operation zero-extends to
the full 64-bit register.  Using xorq to clear any of the lower 8
registers adds an unnecessary REX prefix.  Just one of many quirks of
the x86 instruction set...

--
Brian Gerst

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[David Woodhouse]

On 28 February 2023 22:48:42 GMT, Brian Gerst <brgerst@gmail.com> wrote:
>On Tue, Feb 28, 2023 at 5:41 PM David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> On Tue, 2023-02-28 at 21:57 +0000, David Woodhouse wrote:
>> >
>> > ----------------
>> > IN:
>> > 0xffffffffa20000b2:  48 31 d2                 xorq     %rdx, %rdx
>> > 0xffffffffa20000b5:  48 8b 82 c0 74 d5 a3     movq     -0x5c2a8b40(%rdx), %rax
>> > 0xffffffffa20000bc:  48 8b a0 58 14 00 00     movq     0x1458(%rax), %rsp
>> > 0xffffffffa20000c3:  48 83 ec 10              subq     $0x10, %rsp
>> > 0xffffffffa20000c7:  66 c7 04 24 7f 00        movw     $0x7f, (%rsp)
>> > 0xffffffffa20000cd:  48 8d 82 00 10 81 a3     leaq     -0x5c7ef000(%rdx), %rax
>> > 0xffffffffa20000d4:  48 89 44 24 02           movq     %rax, 2(%rsp)
>> > 0xffffffffa20000d9:  0f 01 14 24              lgdtq    (%rsp)
>> > 0xffffffffa20000dd:  48 83 c4 10              addq     $0x10, %rsp
>> > 0xffffffffa20000e1:  31 c0                    xorl     %eax, %eax
>> > 0xffffffffa20000e3:  8e d8                    movl     %eax, %ds
>> >
>> > I cannot work out where the value -0x5c7ef000 comes from, but it
>> > doesn't seem to be the 0xb000 you claimed, and my brain is hurting
>> > again...
>>
>> Turning off CONFIG_RANDOMIZE_BASE (or just looking at the vmlinux
>> disassembly instead as Brian did) helps to resolve that FWIW.
>>
>> I've changed it to zero all of %rdx and pushed it back to the v12bis
>> branch.
>
>xorl %edx, %edx is preferred, as a 32-bit operation zero-extends to
>the full 64-bit register.  Using xorq to clear any of the lower 8
>registers adds an unnecessary REX prefix.  Just one of many quirks of
>the x86 instruction set...

Ewww. Couldn't the assembler choose to omit the REX prefix then? It does more tricksy things than that already.

I almost prefer having the prefix but (in the morning) if you prefer I can put it back as it was with a comment about the zero-extension.

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[H. Peter Anvin]

On February 28, 2023 3:42:55 PM PST, David Woodhouse <dwmw2@infradead.org> wrote:
>
>
>On 28 February 2023 22:48:42 GMT, Brian Gerst <brgerst@gmail.com> wrote:
>>On Tue, Feb 28, 2023 at 5:41 PM David Woodhouse <dwmw2@infradead.org> wrote:
>>>
>>> On Tue, 2023-02-28 at 21:57 +0000, David Woodhouse wrote:
>>> >
>>> > ----------------
>>> > IN:
>>> > 0xffffffffa20000b2:  48 31 d2                 xorq     %rdx, %rdx
>>> > 0xffffffffa20000b5:  48 8b 82 c0 74 d5 a3     movq     -0x5c2a8b40(%rdx), %rax
>>> > 0xffffffffa20000bc:  48 8b a0 58 14 00 00     movq     0x1458(%rax), %rsp
>>> > 0xffffffffa20000c3:  48 83 ec 10              subq     $0x10, %rsp
>>> > 0xffffffffa20000c7:  66 c7 04 24 7f 00        movw     $0x7f, (%rsp)
>>> > 0xffffffffa20000cd:  48 8d 82 00 10 81 a3     leaq     -0x5c7ef000(%rdx), %rax
>>> > 0xffffffffa20000d4:  48 89 44 24 02           movq     %rax, 2(%rsp)
>>> > 0xffffffffa20000d9:  0f 01 14 24              lgdtq    (%rsp)
>>> > 0xffffffffa20000dd:  48 83 c4 10              addq     $0x10, %rsp
>>> > 0xffffffffa20000e1:  31 c0                    xorl     %eax, %eax
>>> > 0xffffffffa20000e3:  8e d8                    movl     %eax, %ds
>>> >
>>> > I cannot work out where the value -0x5c7ef000 comes from, but it
>>> > doesn't seem to be the 0xb000 you claimed, and my brain is hurting
>>> > again...
>>>
>>> Turning off CONFIG_RANDOMIZE_BASE (or just looking at the vmlinux
>>> disassembly instead as Brian did) helps to resolve that FWIW.
>>>
>>> I've changed it to zero all of %rdx and pushed it back to the v12bis
>>> branch.
>>
>>xorl %edx, %edx is preferred, as a 32-bit operation zero-extends to
>>the full 64-bit register.  Using xorq to clear any of the lower 8
>>registers adds an unnecessary REX prefix.  Just one of many quirks of
>>the x86 instruction set...
>
>Ewww. Couldn't the assembler choose to omit the REX prefix then? It does more tricksy things than that already.
>
>I almost prefer having the prefix but (in the morning) if you prefer I can put it back as it was with a comment about the zero-extension.
>

Like it or not, that's how the assembler currently works.

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[David Woodhouse]

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

On Tue, 2023-02-28 at 16:02 -0800, H. Peter Anvin wrote:
> 
> > Ewww. Couldn't the assembler choose to omit the REX prefix then? It
> > does more tricksy things than that already.
> > 
> > I almost prefer having the prefix but (in the morning) if you
> > prefer I can put it back as it was with a comment about the zero-
> > extension.
> > 
> 
> Like it or not, that's how the assembler currently works.

Last time someone told me "you can't tell the assembler what you
actually mean; you have to tell it something different" I threw my toys
out of the pram a little bit and implemented all of the 16-bit '-m16'
support in LLVM/clang.

On Tue, 2023-02-28 at 19:03 -0500, Brian Gerst wrote:
> 
> commit a7bea8308933aaeea76dad7d42a6e51000417626
> Author: Jan Beulich <JBeulich@suse.com>
> Date:   Mon Jul 2 04:31:54 2018 -0600
> 
>     x86/asm/64: Use 32-bit XOR to zero registers
> 
>     Some Intel CPUs don't recognize 64-bit XORs as zeroing idioms. Zeroing
>     idioms don't require execution bandwidth, as they're being taken care
>     of in the frontend (through register renaming). Use 32-bit XORs instead.
> 
> Not that speed is important here, but it's good to be consistent
> across the whole kernel.  Someone will eventually come by and fix it
> up anyways, as there have been a few of these patches already in the
> git history.

So there literally is a better encoding of this mnemonic which isn't
just a byte shorter, but is also faster to execute. And one *hopes*
that the compiler knows about it... yep... so why in $DEITY's name
can't I type what I mean into an asm file? Isn't asm intricate enough
already?

Meh, life's too short. Not shaving that particular yak today. I've put
the git tree back to using %edx, with a comment that it zero-extends.


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

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Brian Gerst]

On Tue, Feb 28, 2023 at 6:43 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
>
>
> On 28 February 2023 22:48:42 GMT, Brian Gerst <brgerst@gmail.com> wrote:
> >On Tue, Feb 28, 2023 at 5:41 PM David Woodhouse <dwmw2@infradead.org> wrote:
> >>
> >> On Tue, 2023-02-28 at 21:57 +0000, David Woodhouse wrote:
> >> >
> >> > ----------------
> >> > IN:
> >> > 0xffffffffa20000b2:  48 31 d2                 xorq     %rdx, %rdx
> >> > 0xffffffffa20000b5:  48 8b 82 c0 74 d5 a3     movq     -0x5c2a8b40(%rdx), %rax
> >> > 0xffffffffa20000bc:  48 8b a0 58 14 00 00     movq     0x1458(%rax), %rsp
> >> > 0xffffffffa20000c3:  48 83 ec 10              subq     $0x10, %rsp
> >> > 0xffffffffa20000c7:  66 c7 04 24 7f 00        movw     $0x7f, (%rsp)
> >> > 0xffffffffa20000cd:  48 8d 82 00 10 81 a3     leaq     -0x5c7ef000(%rdx), %rax
> >> > 0xffffffffa20000d4:  48 89 44 24 02           movq     %rax, 2(%rsp)
> >> > 0xffffffffa20000d9:  0f 01 14 24              lgdtq    (%rsp)
> >> > 0xffffffffa20000dd:  48 83 c4 10              addq     $0x10, %rsp
> >> > 0xffffffffa20000e1:  31 c0                    xorl     %eax, %eax
> >> > 0xffffffffa20000e3:  8e d8                    movl     %eax, %ds
> >> >
> >> > I cannot work out where the value -0x5c7ef000 comes from, but it
> >> > doesn't seem to be the 0xb000 you claimed, and my brain is hurting
> >> > again...
> >>
> >> Turning off CONFIG_RANDOMIZE_BASE (or just looking at the vmlinux
> >> disassembly instead as Brian did) helps to resolve that FWIW.
> >>
> >> I've changed it to zero all of %rdx and pushed it back to the v12bis
> >> branch.
> >
> >xorl %edx, %edx is preferred, as a 32-bit operation zero-extends to
> >the full 64-bit register.  Using xorq to clear any of the lower 8
> >registers adds an unnecessary REX prefix.  Just one of many quirks of
> >the x86 instruction set...
>
> Ewww. Couldn't the assembler choose to omit the REX prefix then? It does more tricksy things than that already.
>
> I almost prefer having the prefix but (in the morning) if you prefer I can put it back as it was with a comment about the zero-extension.

commit a7bea8308933aaeea76dad7d42a6e51000417626
Author: Jan Beulich <JBeulich@suse.com>
Date:   Mon Jul 2 04:31:54 2018 -0600

    x86/asm/64: Use 32-bit XOR to zero registers

    Some Intel CPUs don't recognize 64-bit XORs as zeroing idioms. Zeroing
    idioms don't require execution bandwidth, as they're being taken care
    of in the frontend (through register renaming). Use 32-bit XORs instead.

Not that speed is important here, but it's good to be consistent
across the whole kernel.  Someone will eventually come by and fix it
up anyways, as there have been a few of these patches already in the
git history.

--
Brian Gerst

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Thomas Gleixner]

On Tue, Feb 28 2023 at 21:57, David Woodhouse wrote:
> On Tue, 2023-02-28 at 22:01 +0100, Thomas Gleixner wrote:
> Empirically, yours doesn't boot and Brian's does.

Correct. It was just my sleep deprived brain which got lost in assembly
and #ifdef maze.

On UP per_cpu variables just become regular variables and gdt_page(%rdx)
with %rdx = 0 will just resolve that address.

Sorry for the noise.

Thanks,

        tglx

Re: [PATCH v12 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

[Brian Gerst]

On Tue, Feb 28, 2023 at 4:01 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> > @@ -265,7 +265,12 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> >        * addresses where we're currently running on. We have to do that here
> >        * because in 32bit we couldn't load a 64bit linear address.
> >        */
> > -     lgdt    early_gdt_descr(%rip)
> > +     subq    $16, %rsp
> > +     movw    $(GDT_SIZE-1), (%rsp)
> > +     leaq    gdt_page(%rdx), %rax
>
> Even on !SMP gdt_page is in the 0...__per_cpu_end range. Which means
> that on !SMP this results in:
>
>       leaq    0xb000(%rdx),%rax
>
> and RDX is 0. That's not really a valid GDT pointer, right?

No.  On !SMP per-cpu variables are normal variables in the .data
section.  They are not gathered together in the per-cpu section and
are not accessed with the GS prefix.

ffffffff810000c9:       48 8d 82 00 10 81 82    lea    0x82811000(%rdx),%rax
                        ffffffff810000cc: R_X86_64_32S  gdt_page

ffffffff82811000 D gdt_page

So RDX=0 is correct.

> > +     movq    %rax, 2(%rsp)
> > +     lgdt    (%rsp)
>
> and obviously that's equally broken for the task stack part:
>
> >       movq    pcpu_hot + X86_current_task(%rdx), %rax

Same as gdt_page:

ffffffff810000b1:       48 8b 82 00 88 a8 82    mov    0x82a88800(%rdx),%rax
                        ffffffff810000b4: R_X86_64_32S  pcpu_hot

ffffffff82a88800 D pcpu_hot

> This needs:
>
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -239,7 +239,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_
>         /* Get the per cpu offset for the given CPU# which is in ECX */
>         movq    __per_cpu_offset(,%rcx,8), %rdx
>  #else
> -       xorl    %edx, %edx
> +       leaq    INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
>  #endif /* CONFIG_SMP */
>
>         /*
>
> in the initial_stack patch, which then allows to remove this hunk in the
> initial_gs patch:
>
> @@ -286,9 +286,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_
>          * the per cpu areas are set up.
>          */
>         movl    $MSR_GS_BASE,%ecx
> -#ifndef CONFIG_SMP
> -       leaq    INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
> -#endif

On !SMP the only thing GSBASE is used for is the stack protector
canary, which is in fixed_percpu_data.  There is no per-cpu section.

FWIW, I posted a patch set a while back that switched x86-64 to use
the options added to newer compilers controlling where the canary is
located, allowing it to become a standard per-cpu variable and
removing the need to force the per-cpu section to be zero-based.
However it was not accepted at that time, due to removing support for
stack protector on older compilers (GCC < 8.1).

>         movl    %edx, %eax
>         shrq    $32, %rdx
>         wrmsr
>
> Maybe we should enforce CONFIG_SMP=y first :)

Makes sense, only the earliest generations of x86-64 processors have a
single core/thread, and an SMP kernel can still run on them.

--
Brian Gerst

[PATCH v12 08/11] x86/smpboot: Remove initial_gs

[Usama Arif]

From: Brian Gerst <brgerst@gmail.com>

Use the percpu offset directly to set GSBASE.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/include/asm/realmode.h |  1 -
 arch/x86/kernel/acpi/sleep.c    |  1 -
 arch/x86/kernel/head_64.S       | 22 ++++++++--------------
 arch/x86/kernel/smpboot.c       |  2 --
 4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index a336feef0af1..f6a1737c77be 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -59,7 +59,6 @@ extern struct real_mode_header *real_mode_header;
 extern unsigned char real_mode_blob_end[];
 
 extern unsigned long initial_code;
-extern unsigned long initial_gs;
 extern unsigned long initial_stack;
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 extern unsigned long initial_vc_handler;
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 236f2423454d..9d2d88424c77 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -112,7 +112,6 @@ int x86_acpi_suspend_lowlevel(void)
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
 	current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
-	initial_gs = per_cpu_offset(smp_processor_id());
 	smpboot_control = smp_processor_id();
 #endif
 	initial_code = (unsigned long)wakeup_long64;
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 0ccca297e90e..069191e33490 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -66,18 +66,10 @@ SYM_CODE_START_NOALIGN(startup_64)
 
 	leaq	_text(%rip), %rdi
 
-	/*
-	 * initial_gs points to initial fixed_percpu_data struct with storage for
-	 * the stack protector canary. Global pointer fixups are needed at this
-	 * stage, so apply them as is done in fixup_pointer(), and initialize %gs
-	 * such that the canary can be accessed at %gs:40 for subsequent C calls.
-	 */
+	/* Setup GSBASE to allow stack canary access for C code */
 	movl	$MSR_GS_BASE, %ecx
-	movq	initial_gs(%rip), %rax
-	movq	$_text, %rdx
-	subq	%rdx, %rax
-	addq	%rdi, %rax
-	movq	%rax, %rdx
+	leaq	INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
+	movl	%edx, %eax
 	shrq	$32,  %rdx
 	wrmsr
 
@@ -294,8 +286,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	 * the per cpu areas are set up.
 	 */
 	movl	$MSR_GS_BASE,%ecx
-	movl	initial_gs(%rip),%eax
-	movl	initial_gs+4(%rip),%edx
+#ifndef CONFIG_SMP
+	leaq	INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
+#endif
+	movl	%edx, %eax
+	shrq	$32, %rdx
 	wrmsr
 
 	/* Setup and Load IDT */
@@ -437,7 +432,6 @@ SYM_CODE_END(vc_boot_ghcb)
 	__REFDATA
 	.balign	8
 SYM_DATA(initial_code,	.quad x86_64_start_kernel)
-SYM_DATA(initial_gs,	.quad INIT_PER_CPU_VAR(fixed_percpu_data))
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
 #endif
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index a22460a07cf8..b04520085582 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1084,8 +1084,6 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
 #ifdef CONFIG_X86_32
 	/* Stack for startup_32 can be just as for start_secondary onwards */
 	per_cpu(pcpu_hot.top_of_stack, cpu) = task_top_of_stack(idle);
-#else
-	initial_gs = per_cpu_offset(cpu);
 #endif
 	return 0;
 }
-- 
2.25.1

[PATCH v12 09/11] x86/smpboot: Support parallel startup of secondary CPUs

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

Rework the real-mode startup code to allow for APs to be brought up in
parallel. This is in two parts:

1. Introduce a bit-spinlock to prevent them from all using the real
   mode stack at the same time.

2. Avoid needing to use the global smpboot_control variable to pass
   each AP its CPU#.

To achieve the latter, export the cpuid_to_apicid[] array so that each
AP can find its own CPU# by searching therein based on its APIC ID.

Introduce flags in the top bits of smpboot_control which indicate methods
by which an AP should find its CPU#. For a serialized bringup, the CPU#
is explicitly passed in the low bits of smpboot_control as before. For
parallel mode there are flags directing the AP to find its APIC ID in
CPUID leaf 0x0b (for X2APIC mode) or CPUID leaf 0x01 where 8 bits are
sufficient, then perform the cpuid_to_apicid[] lookup with that.

Parallel startup may be disabled by a command line option, and also if:
 • AMD SEV-ES is in use, since the AP may not use CPUID that early.
 • X2APIC is enabled, but CPUID leaf 0xb is not present and correect.
 • X2APIC is not enabled but not even CPUID leaf 0x01 exists.

Aside from the fact that APs will now look up their CPU# via the
newly-exported cpuid_to_apicid[] table, there is no behavioural change
intended yet, since new parallel CPUHP states have not — yet — been
added.

[ tglx: Initial proof of concept patch with bitlock and APIC ID lookup ]
[ dwmw2: Rework and testing, commit message, CPUID 0x1 and CPU0 support ]
[ seanc: Fix stray override of initial_gs in common_cpu_up() ]
[ Oleksandr Natalenko: reported suspend/resume issue fixed in
  x86_acpi_suspend_lowlevel ]
Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
Co-developed-by: Brian Gerst <brgerst@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Brian Gerst <brgerst@gmail.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 .../admin-guide/kernel-parameters.txt         |  3 +
 arch/x86/include/asm/realmode.h               |  3 +
 arch/x86/include/asm/smp.h                    |  6 ++
 arch/x86/kernel/acpi/sleep.c                  |  9 ++-
 arch/x86/kernel/apic/apic.c                   |  2 +-
 arch/x86/kernel/head_64.S                     | 63 ++++++++++++++++++-
 arch/x86/kernel/smpboot.c                     | 53 +++++++++++++++-
 arch/x86/realmode/init.c                      |  3 +
 arch/x86/realmode/rm/trampoline_64.S          | 27 ++++++--
 9 files changed, 159 insertions(+), 10 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6cfa6e3996cf..ee099b8aac6d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3819,6 +3819,9 @@
 
 	nomodule	Disable module load
 
+	no_parallel_bringup
+			[X86,SMP] Disable parallel brinugp of secondary cores.
+
 	nopat		[X86] Disable PAT (page attribute table extension of
 			pagetables) support.
 
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index f6a1737c77be..87e5482acd0d 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -52,6 +52,7 @@ struct trampoline_header {
 	u64 efer;
 	u32 cr4;
 	u32 flags;
+	u32 lock;
 #endif
 };
 
@@ -64,6 +65,8 @@ extern unsigned long initial_stack;
 extern unsigned long initial_vc_handler;
 #endif
 
+extern u32 *trampoline_lock;
+
 extern unsigned char real_mode_blob[];
 extern unsigned char real_mode_relocs[];
 
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index bf2c51df9e0b..1cf4f1e57570 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -203,4 +203,10 @@ extern unsigned int smpboot_control;
 
 #endif /* !__ASSEMBLY__ */
 
+/* Control bits for startup_64 */
+#define STARTUP_APICID_CPUID_0B	0x80000000
+#define STARTUP_APICID_CPUID_01	0x40000000
+
+#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B)
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 9d2d88424c77..5dcf5ca15383 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -16,6 +16,7 @@
 #include <asm/cacheflush.h>
 #include <asm/realmode.h>
 #include <asm/hypervisor.h>
+#include <asm/smp.h>
 
 #include <linux/ftrace.h>
 #include "../../realmode/rm/wakeup.h"
@@ -112,7 +113,13 @@ int x86_acpi_suspend_lowlevel(void)
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
 	current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
-	smpboot_control = smp_processor_id();
+	/*
+	 * Ensure the CPU knows which one it is when it comes back, if
+	 * it isn't in parallel mode and expected to work that out for
+	 * itself.
+	 */
+	if (!(smpboot_control & STARTUP_PARALLEL_MASK))
+		smpboot_control = smp_processor_id();
 #endif
 	initial_code = (unsigned long)wakeup_long64;
 	saved_magic = 0x123456789abcdef0L;
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 20d9a604da7c..ac1d7e5da1f2 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2377,7 +2377,7 @@ static int nr_logical_cpuids = 1;
 /*
  * Used to store mapping between logical CPU IDs and APIC IDs.
  */
-static int cpuid_to_apicid[] = {
+int cpuid_to_apicid[] = {
 	[0 ... NR_CPUS - 1] = -1,
 };
 
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 069191e33490..17bdd6122dca 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -25,6 +25,7 @@
 #include <asm/export.h>
 #include <asm/nospec-branch.h>
 #include <asm/fixmap.h>
+#include <asm/smp.h>
 
 /*
  * We are not able to switch in one step to the final KERNEL ADDRESS SPACE
@@ -234,8 +235,57 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	ANNOTATE_NOENDBR // above
 
 #ifdef CONFIG_SMP
+	/*
+	 * For parallel boot, the APIC ID is retrieved from CPUID, and then
+	 * used to look up the CPU number.  For booting a single CPU, the
+	 * CPU number is encoded in smpboot_control.
+	 *
+	 * Bit 31	STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
+	 * Bit 30	STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
+	 * Bit 0-24	CPU# if STARTUP_APICID_CPUID_xx flags are not set
+	 */
 	movl	smpboot_control(%rip), %ecx
+	testl	$STARTUP_APICID_CPUID_0B, %ecx
+	jnz	.Luse_cpuid_0b
+	testl	$STARTUP_APICID_CPUID_01, %ecx
+	jnz	.Luse_cpuid_01
+	andl	$0x0FFFFFFF, %ecx
+	jmp	.Lsetup_cpu
+
+.Luse_cpuid_01:
+	mov	$0x01, %eax
+	cpuid
+	mov	%ebx, %edx
+	shr	$24, %edx
+	jmp	.Lsetup_AP
+
+.Luse_cpuid_0b:
+	mov	$0x0B, %eax
+	xorl	%ecx, %ecx
+	cpuid
+
+.Lsetup_AP:
+	/* EDX contains the APIC ID of the current CPU */
+	xorq	%rcx, %rcx
+	leaq	cpuid_to_apicid(%rip), %rbx
 
+.Lfind_cpunr:
+	cmpl	(%rbx,%rcx,4), %edx
+	jz	.Lsetup_cpu
+	inc	%ecx
+	cmpl	nr_cpu_ids(%rip), %ecx
+	jb	.Lfind_cpunr
+
+	/*  APIC ID not found in the table. Drop the trampoline lock and bail. */
+	movq	trampoline_lock(%rip), %rax
+	lock
+	btrl	$0, (%rax)
+
+1:	cli
+	hlt
+	jmp	1b
+
+.Lsetup_cpu:
 	/* Get the per cpu offset for the given CPU# which is in ECX */
 	movq	__per_cpu_offset(,%rcx,8), %rdx
 #else
@@ -244,7 +294,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 
 	/*
 	 * Setup a boot time stack - Any secondary CPU will have lost its stack
-	 * by now because the cr3-switch above unmaps the real-mode stack.
+	 * by now because the cr3-switch above unmaps the real-mode stack
 	 *
 	 * RDX contains the per-cpu offset
 	 */
@@ -293,6 +343,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	shrq	$32, %rdx
 	wrmsr
 
+	/* Drop the realmode protection. For the boot CPU the pointer is NULL! */
+	movq	trampoline_lock(%rip), %rax
+	testq	%rax, %rax
+	jz	.Lsetup_idt
+	lock
+	btrl	$0, (%rax)
+
+.Lsetup_idt:
 	/* Setup and Load IDT */
 	pushq	%rsi
 	call	early_setup_idt
@@ -435,6 +493,8 @@ SYM_DATA(initial_code,	.quad x86_64_start_kernel)
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
 #endif
+
+SYM_DATA(trampoline_lock, .quad 0);
 	__FINITDATA
 
 	__INIT
@@ -665,7 +725,6 @@ SYM_DATA_END(level1_fixmap_pgt)
 
 	.data
 	.align 16
-
 SYM_DATA(smpboot_control,		.long 0)
 
 	.align 16
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b04520085582..19b9b89b7458 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -797,6 +797,16 @@ static int __init cpu_init_udelay(char *str)
 }
 early_param("cpu_init_udelay", cpu_init_udelay);
 
+static bool do_parallel_bringup __ro_after_init = true;
+
+static int __init no_parallel_bringup(char *str)
+{
+	do_parallel_bringup = false;
+
+	return 0;
+}
+early_param("no_parallel_bringup", no_parallel_bringup);
+
 static void __init smp_quirk_init_udelay(void)
 {
 	/* if cmdline changed it from default, leave it alone */
@@ -1113,7 +1123,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	if (IS_ENABLED(CONFIG_X86_32)) {
 		early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 		initial_stack  = idle->thread.sp;
-	} else {
+	} else if (!do_parallel_bringup) {
 		smpboot_control = cpu;
 	}
 
@@ -1515,6 +1525,47 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 
 	speculative_store_bypass_ht_init();
 
+	/*
+	 * We can do 64-bit AP bringup in parallel if the CPU reports
+	 * its APIC ID in CPUID (either leaf 0x0B if we need the full
+	 * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
+	 * sufficient). Otherwise it's too hard. And not for SEV-ES
+	 * guests because they can't use CPUID that early.
+	 */
+	if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1 ||
+	    (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
+	    cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+		do_parallel_bringup = false;
+
+	if (do_parallel_bringup && x2apic_mode) {
+		unsigned int eax, ebx, ecx, edx;
+
+		/*
+		 * To support parallel bringup in x2apic mode, the AP will need
+		 * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
+		 * only 8 bits. Check that it is present and seems correct.
+		 */
+		cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
+
+		/*
+		 * AMD says that if executed with an umimplemented level in
+		 * ECX, then it will return all zeroes in EAX. Intel says it
+		 * will return zeroes in both EAX and EBX. Checking only EAX
+		 * should be sufficient.
+		 */
+		if (eax) {
+			pr_debug("Using CPUID 0xb for parallel CPU startup\n");
+			smpboot_control = STARTUP_APICID_CPUID_0B;
+		} else {
+			pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
+			do_parallel_bringup = false;
+		}
+	} else if (do_parallel_bringup) {
+		/* Without X2APIC, what's in CPUID 0x01 should suffice. */
+		pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
+		smpboot_control = STARTUP_APICID_CPUID_01;
+	}
+
 	snp_set_wakeup_secondary_cpu();
 }
 
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index af565816d2ba..788e5559549f 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -154,6 +154,9 @@ static void __init setup_real_mode(void)
 
 	trampoline_header->flags = 0;
 
+	trampoline_lock = &trampoline_header->lock;
+	*trampoline_lock = 0;
+
 	trampoline_pgd = (u64 *) __va(real_mode_header->trampoline_pgd);
 
 	/* Map the real mode stub as virtual == physical */
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index e38d61d6562e..2dfb1c400167 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -37,6 +37,24 @@
 	.text
 	.code16
 
+.macro LOAD_REALMODE_ESP
+	/*
+	 * Make sure only one CPU fiddles with the realmode stack
+	 */
+.Llock_rm\@:
+	btl	$0, tr_lock
+	jnc	2f
+	pause
+	jmp	.Llock_rm\@
+2:
+	lock
+	btsl	$0, tr_lock
+	jc	.Llock_rm\@
+
+	# Setup stack
+	movl	$rm_stack_end, %esp
+.endm
+
 	.balign	PAGE_SIZE
 SYM_CODE_START(trampoline_start)
 	cli			# We should be safe anyway
@@ -49,8 +67,7 @@ SYM_CODE_START(trampoline_start)
 	mov	%ax, %es
 	mov	%ax, %ss
 
-	# Setup stack
-	movl	$rm_stack_end, %esp
+	LOAD_REALMODE_ESP
 
 	call	verify_cpu		# Verify the cpu supports long mode
 	testl   %eax, %eax		# Check for return code
@@ -93,8 +110,7 @@ SYM_CODE_START(sev_es_trampoline_start)
 	mov	%ax, %es
 	mov	%ax, %ss
 
-	# Setup stack
-	movl	$rm_stack_end, %esp
+	LOAD_REALMODE_ESP
 
 	jmp	.Lswitch_to_protected
 SYM_CODE_END(sev_es_trampoline_start)
@@ -177,7 +193,7 @@ SYM_CODE_START(pa_trampoline_compat)
 	 * In compatibility mode.  Prep ESP and DX for startup_32, then disable
 	 * paging and complete the switch to legacy 32-bit mode.
 	 */
-	movl	$rm_stack_end, %esp
+	LOAD_REALMODE_ESP
 	movw	$__KERNEL_DS, %dx
 
 	movl	$(CR0_STATE & ~X86_CR0_PG), %eax
@@ -241,6 +257,7 @@ SYM_DATA_START(trampoline_header)
 	SYM_DATA(tr_efer,		.space 8)
 	SYM_DATA(tr_cr4,		.space 4)
 	SYM_DATA(tr_flags,		.space 4)
+	SYM_DATA(tr_lock,		.space 4)
 SYM_DATA_END(trampoline_header)
 
 #include "trampoline_common.S"
-- 
2.25.1

[PATCH v12 10/11] x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

When the APs can find their own APIC ID without assistance, perform the
AP bringup in parallel.

Register a CPUHP_BP_PARALLEL_DYN stage "x86/cpu:kick" which just calls
do_boot_cpu() to deliver INIT/SIPI/SIPI to each AP in turn before the
normal native_cpu_up() does the rest of the hand-holding.

The APs will then take turns through the real mode code (which has its
own bitlock for exclusion) until they make it to their own stack, then
proceed through the first few lines of start_secondary() and execute
these parts in parallel:

 start_secondary()
    -> cr4_init()
    -> (some 32-bit only stuff so not in the parallel cases)
    -> cpu_init_secondary()
       -> cpu_init_exception_handling()
       -> cpu_init()
          -> wait_for_master_cpu()

At this point they wait for the BSP to set their bit in cpu_callout_mask
(from do_wait_cpu_initialized()), and release them to continue through
the rest of cpu_init() and beyond.

This reduces the time taken for bringup on my 28-thread Haswell system
from about 120ms to 80ms. On a socket 96-thread Skylake it takes the
bringup time from 500ms to 100ms.

There is more speedup to be had by doing the remaining parts in parallel
too — especially notify_cpu_starting() in which the AP takes itself
through all the stages from CPUHP_BRINGUP_CPU to CPUHP_ONLINE. But those
require careful auditing to ensure they are reentrant, before we can go
that far.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 arch/x86/kernel/smpboot.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 19b9b89b7458..711573cd9b87 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -57,6 +57,7 @@
 #include <linux/pgtable.h>
 #include <linux/overflow.h>
 #include <linux/stackprotector.h>
+#include <linux/smpboot.h>
 
 #include <asm/acpi.h>
 #include <asm/cacheinfo.h>
@@ -1325,9 +1326,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
 	int ret;
 
-	ret = do_cpu_up(cpu, tidle);
-	if (ret)
-		return ret;
+	/* If parallel AP bringup isn't enabled, perform the first steps now. */
+	if (!do_parallel_bringup) {
+		ret = do_cpu_up(cpu, tidle);
+		if (ret)
+			return ret;
+	}
 
 	ret = do_wait_cpu_initialized(cpu);
 	if (ret)
@@ -1349,6 +1353,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	return ret;
 }
 
+/* Bringup step one: Send INIT/SIPI to the target AP */
+static int native_cpu_kick(unsigned int cpu)
+{
+	return do_cpu_up(cpu, idle_thread_get(cpu));
+}
+
 /**
  * arch_disable_smp_support() - disables SMP support for x86 at runtime
  */
@@ -1566,6 +1576,11 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 		smpboot_control = STARTUP_APICID_CPUID_01;
 	}
 
+	if (do_parallel_bringup) {
+		cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
+					  native_cpu_kick, NULL);
+	}
+
 	snp_set_wakeup_secondary_cpu();
 }
 
-- 
2.25.1

[PATCH v12 11/11] x86/smpboot: Serialize topology updates for secondary bringup

[Usama Arif]

From: David Woodhouse <dwmw@amazon.co.uk>

The toplogy update is performed by the AP via smp_callin() after the BSP
has called do_wait_cpu_initialized(), setting the AP's bit in
cpu_callout_mask to allow it to proceed.

In preparation to enable further parallelism of AP bringup, add locking to
serialize the update even if multiple APs are (in future) permitted to
proceed through the next stages of bringup in parallel.

Without such ordering (and with that future extra parallelism), confusion
ensues:

[    1.360149] x86: Booting SMP configuration:
[    1.360221] .... node  #0, CPUs:        #1  #2  #3  #4  #5  #6  #7  #8  #9 #10 #11 #12 #13 #14 #15 #16 #17 #18 #19 #20 #21 #22 #23
[    1.366225] .... node  #1, CPUs:   #24 #25 #26 #27 #28 #29 #30 #31 #32 #33 #34 #35 #36 #37 #38 #39 #40 #41 #42 #43 #44 #45 #46 #47
[    1.370219] .... node  #0, CPUs:   #48 #49 #50 #51 #52 #53 #54 #55 #56 #57 #58 #59 #60 #61 #62 #63 #64 #65 #66 #67 #68 #69 #70 #71
[    1.378226] .... node  #1, CPUs:   #72 #73 #74 #75 #76 #77 #78 #79 #80 #81 #82 #83 #84 #85 #86 #87 #88 #89 #90 #91 #92 #93 #94 #95
[    1.382037] Brought 96 CPUs to x86/cpu:kick in 72232606 cycles
[    0.104104] smpboot: CPU 26 Converting physical 0 to logical die 1
[    0.104104] smpboot: CPU 27 Converting physical 1 to logical package 2
[    0.104104] smpboot: CPU 24 Converting physical 1 to logical package 3
[    0.104104] smpboot: CPU 27 Converting physical 0 to logical die 2
[    0.104104] smpboot: CPU 25 Converting physical 1 to logical package 4
[    1.385609] Brought 96 CPUs to x86/cpu:wait-init in 9269218 cycles
[    1.395285] Brought CPUs online in 28930764 cycles
[    1.395469] smp: Brought up 2 nodes, 96 CPUs
[    1.395689] smpboot: Max logical packages: 2
[    1.396222] smpboot: Total of 96 processors activated (576000.00 BogoMIPS)

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 arch/x86/include/asm/smp.h      |  4 +-
 arch/x86/include/asm/topology.h |  2 -
 arch/x86/kernel/cpu/common.c    |  6 +--
 arch/x86/kernel/smpboot.c       | 73 ++++++++++++++++++++-------------
 arch/x86/xen/smp_pv.c           |  4 +-
 5 files changed, 48 insertions(+), 41 deletions(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 1cf4f1e57570..defe76ee9e64 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -48,8 +48,6 @@ struct smp_ops {
 };
 
 /* Globals due to paravirt */
-extern void set_cpu_sibling_map(int cpu);
-
 #ifdef CONFIG_SMP
 extern struct smp_ops smp_ops;
 
@@ -137,7 +135,7 @@ void native_send_call_func_single_ipi(int cpu);
 void x86_idle_thread_init(unsigned int cpu, struct task_struct *idle);
 
 void smp_store_boot_cpu_info(void);
-void smp_store_cpu_info(int id);
+void smp_store_cpu_info(int id, bool force_single_core);
 
 asmlinkage __visible void smp_reboot_interrupt(void);
 __visible void smp_reschedule_interrupt(struct pt_regs *regs);
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 458c891a8273..4bccbd949a99 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -136,8 +136,6 @@ static inline int topology_max_smt_threads(void)
 	return __max_smt_threads;
 }
 
-int topology_update_package_map(unsigned int apicid, unsigned int cpu);
-int topology_update_die_map(unsigned int dieid, unsigned int cpu);
 int topology_phys_to_logical_pkg(unsigned int pkg);
 int topology_phys_to_logical_die(unsigned int die, unsigned int cpu);
 bool topology_is_primary_thread(unsigned int cpu);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f3cc7699e1e1..06d7f9e55d45 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1771,7 +1771,7 @@ static void generic_identify(struct cpuinfo_x86 *c)
  * Validate that ACPI/mptables have the same information about the
  * effective APIC id and update the package map.
  */
-static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
+static void validate_apic_id(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
 	unsigned int apicid, cpu = smp_processor_id();
@@ -1782,8 +1782,6 @@ static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
 		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x APIC: %x\n",
 		       cpu, apicid, c->initial_apicid);
 	}
-	BUG_ON(topology_update_package_map(c->phys_proc_id, cpu));
-	BUG_ON(topology_update_die_map(c->cpu_die_id, cpu));
 #else
 	c->logical_proc_id = 0;
 #endif
@@ -1974,7 +1972,7 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
 #ifdef CONFIG_X86_32
 	enable_sep_cpu();
 #endif
-	validate_apic_and_package_id(c);
+	validate_apic_id(c);
 	x86_spec_ctrl_setup_ap();
 	update_srbds_msr();
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 711573cd9b87..9d956571ecc1 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -180,16 +180,12 @@ static void smp_callin(void)
 	apic_ap_setup();
 
 	/*
-	 * Save our processor parameters. Note: this information
-	 * is needed for clock calibration.
-	 */
-	smp_store_cpu_info(cpuid);
-
-	/*
+	 * Save our processor parameters and update topology.
+	 * Note: this information is needed for clock calibration.
 	 * The topology information must be up to date before
 	 * calibrate_delay() and notify_cpu_starting().
 	 */
-	set_cpu_sibling_map(raw_smp_processor_id());
+	smp_store_cpu_info(cpuid, false);
 
 	ap_init_aperfmperf();
 
@@ -243,6 +239,12 @@ static void notrace start_secondary(void *unused)
 	 * its bit in cpu_callout_mask to release it.
 	 */
 	cpu_init_secondary();
+
+	/*
+	 * Even though notify_cpu_starting() will do this, it does so too late
+	 * as the AP may already have triggered lockdep splats by then. See
+	 * commit 29368e093 ("x86/smpboot:  Move rcu_cpu_starting() earlier").
+	 */
 	rcu_cpu_starting(raw_smp_processor_id());
 	x86_cpuinit.early_percpu_clock_init();
 
@@ -351,7 +353,7 @@ EXPORT_SYMBOL(topology_phys_to_logical_die);
  * @pkg:	The physical package id as retrieved via CPUID
  * @cpu:	The cpu for which this is updated
  */
-int topology_update_package_map(unsigned int pkg, unsigned int cpu)
+static int topology_update_package_map(unsigned int pkg, unsigned int cpu)
 {
 	int new;
 
@@ -374,7 +376,7 @@ int topology_update_package_map(unsigned int pkg, unsigned int cpu)
  * @die:	The die id as retrieved via CPUID
  * @cpu:	The cpu for which this is updated
  */
-int topology_update_die_map(unsigned int die, unsigned int cpu)
+static int topology_update_die_map(unsigned int die, unsigned int cpu)
 {
 	int new;
 
@@ -405,25 +407,7 @@ void __init smp_store_boot_cpu_info(void)
 	c->initialized = true;
 }
 
-/*
- * The bootstrap kernel entry code has set these up. Save them for
- * a given CPU
- */
-void smp_store_cpu_info(int id)
-{
-	struct cpuinfo_x86 *c = &cpu_data(id);
-
-	/* Copy boot_cpu_data only on the first bringup */
-	if (!c->initialized)
-		*c = boot_cpu_data;
-	c->cpu_index = id;
-	/*
-	 * During boot time, CPU0 has this setup already. Save the info when
-	 * bringing up AP or offlined CPU0.
-	 */
-	identify_secondary_cpu(c);
-	c->initialized = true;
-}
+static arch_spinlock_t topology_lock = __ARCH_SPIN_LOCK_UNLOCKED;
 
 static bool
 topology_same_node(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
@@ -629,7 +613,7 @@ static struct sched_domain_topology_level x86_topology[] = {
  */
 static bool x86_has_numa_in_package;
 
-void set_cpu_sibling_map(int cpu)
+static void set_cpu_sibling_map(int cpu)
 {
 	bool has_smt = smp_num_siblings > 1;
 	bool has_mp = has_smt || boot_cpu_data.x86_max_cores > 1;
@@ -708,6 +692,37 @@ void set_cpu_sibling_map(int cpu)
 	}
 }
 
+/*
+ * The bootstrap kernel entry code has set these up. Save them for
+ * a given CPU
+ */
+void smp_store_cpu_info(int id, bool force_single_core)
+{
+	struct cpuinfo_x86 *c = &cpu_data(id);
+
+	/* Copy boot_cpu_data only on the first bringup */
+	if (!c->initialized)
+		*c = boot_cpu_data;
+	c->cpu_index = id;
+	/*
+	 * During boot time, CPU0 has this setup already. Save the info when
+	 * bringing up AP or offlined CPU0.
+	 */
+	identify_secondary_cpu(c);
+
+	arch_spin_lock(&topology_lock);
+	BUG_ON(topology_update_package_map(c->phys_proc_id, id));
+	BUG_ON(topology_update_die_map(c->cpu_die_id, id));
+	c->initialized = true;
+
+	/* For Xen PV */
+	if (force_single_core)
+		c->x86_max_cores = 1;
+
+	set_cpu_sibling_map(id);
+	arch_spin_unlock(&topology_lock);
+}
+
 /* maps the cpu to the sched domain representing multi-core */
 const struct cpumask *cpu_coregroup_mask(int cpu)
 {
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 6175f2c5c822..09f94f940689 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -71,9 +71,7 @@ static void cpu_bringup(void)
 		xen_enable_syscall();
 	}
 	cpu = smp_processor_id();
-	smp_store_cpu_info(cpu);
-	cpu_data(cpu).x86_max_cores = 1;
-	set_cpu_sibling_map(cpu);
+	smp_store_cpu_info(cpu, true);
 
 	speculative_store_bypass_ht_init();
 
-- 
2.25.1

Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Oleksandr Natalenko]

Hello.

On neděle 26. února 2023 12:07:51 CET Usama Arif wrote:
> The main code change over v11 is the build error fix by Brian Gerst and
> acquiring tr_lock in trampoline_64.S whenever the stack is setup.
> 
> The git history is also rewritten to move the commits that removed
> initial_stack, early_gdt_descr and initial_gs earlier in the patchset.
> 
> Thanks,
> Usama
> 
> Changes across versions:
> v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
> v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
>     in preparation for more parallelisation.
> v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
>     avoid scribbling on initial_gs in common_cpu_up(), and to allow all
>     24 bits of the physical X2APIC ID to be used. That patch still needs
>     a Signed-off-by from its original author, who once claimed not to
>     remember writing it at all. But now we've fixed it, hopefully he'll
>     admit it now :)
> v5: rebase to v6.1 and remeasure performance, disable parallel bringup
>     for AMD CPUs.
> v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
>     reused timer calibration for secondary CPUs.
> v7: [David Woodhouse] iterate over all possible CPUs to find any existing
>     cluster mask in alloc_clustermask. (patch 1/9)
>     Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
>     0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
>     Included sanity checks for APIC id from 0x0B. (patch 6/9)
>     Removed patch for reusing timer calibration for secondary CPUs.
>     commit message and code improvements.
> v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
>     early_gdt_descr.
>     Drop trampoline lock and bail if APIC ID not found in find_cpunr.
>     Code comments improved and debug prints added.
> v9: Drop patch to avoid repeated saves of MTRR at boot time.
>     rebased and retested at v6.2-rc8.
>     added kernel doc for no_parallel_bringup and made do_parallel_bringup
>     __ro_after_init.
> v10: Fixed suspend/resume not working with parallel smpboot.
>      rebased and retested to 6.2.
>      fixed checkpatch errors.
> v11: Added patches from Brian Gerst to remove the global variables initial_gs,
>      initial_stack, and early_gdt_descr from the 64-bit boot code
>      (https://lore.kernel.org/all/20230222221301.245890-1-brgerst@gmail.com/).
> v12: Fixed compilation errors, acquire tr_lock for every stack setup in
>      trampoline_64.S.
>      Rearranged commits for a cleaner git history.
> 
> Brian Gerst (3):
>   x86/smpboot: Remove initial_stack on 64-bit
>   x86/smpboot: Remove early_gdt_descr on 64-bit
>   x86/smpboot: Remove initial_gs
> 
> David Woodhouse (8):
>   x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
>   cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
>   cpu/hotplug: Add dynamic parallel bringup states before
>     CPUHP_BRINGUP_CPU
>   x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
>   x86/smpboot: Split up native_cpu_up into separate phases and document
>     them
>   x86/smpboot: Support parallel startup of secondary CPUs
>   x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
>   x86/smpboot: Serialize topology updates for secondary bringup
> 
>  .../admin-guide/kernel-parameters.txt         |   3 +
>  arch/x86/include/asm/processor.h              |   6 +-
>  arch/x86/include/asm/realmode.h               |   4 +-
>  arch/x86/include/asm/smp.h                    |  15 +-
>  arch/x86/include/asm/topology.h               |   2 -
>  arch/x86/kernel/acpi/sleep.c                  |  15 +-
>  arch/x86/kernel/apic/apic.c                   |   2 +-
>  arch/x86/kernel/apic/x2apic_cluster.c         | 126 ++++---
>  arch/x86/kernel/asm-offsets.c                 |   1 +
>  arch/x86/kernel/cpu/common.c                  |   6 +-
>  arch/x86/kernel/head_64.S                     | 129 +++++--
>  arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>  arch/x86/realmode/init.c                      |   3 +
>  arch/x86/realmode/rm/trampoline_64.S          |  27 +-
>  arch/x86/xen/smp_pv.c                         |   4 +-
>  arch/x86/xen/xen-head.S                       |   2 +-
>  include/linux/cpuhotplug.h                    |   2 +
>  include/linux/smpboot.h                       |   7 +
>  kernel/cpu.c                                  |  31 +-
>  kernel/smpboot.h                              |   2 -
>  20 files changed, 537 insertions(+), 200 deletions(-)

With `CONFIG_FORCE_NR_CPUS=y` this results in:

```
ld: vmlinux.o: in function `secondary_startup_64_no_verify':
(.head.text+0x10c): undefined reference to `nr_cpu_ids'
```

That's because in `arch/x86/kernel/head_64.S` `secondary_startup_64_no_verify()` refers to `nr_cpu_ids` under `#ifdef CONFIG_SMP`, but this symbol is available under the following conditions:

```
38 #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
39 #define nr_cpu_ids ((unsigned int)NR_CPUS)
40 #else
41 extern unsigned int nr_cpu_ids;
42 #endif

1090 #if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
1091 /* Setup number of possible processor ids */
1092 unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
1093 EXPORT_SYMBOL(nr_cpu_ids);
1094 #endif
```

So having `CONFIG_SMP=y` and, for instance, `CONFIG_NR_CPUS=320`, it is possible to compile out `EXPORT_SYMBOL(nr_cpu_ids);` if `CONFIG_FORCE_NR_CPUS=y` is set.

-- 
Oleksandr Natalenko (post-factum)

Re: [External] Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Usama Arif]

On 26/02/2023 18:31, Oleksandr Natalenko wrote:
> Hello.
> 
> On neděle 26. února 2023 12:07:51 CET Usama Arif wrote:
>> The main code change over v11 is the build error fix by Brian Gerst and
>> acquiring tr_lock in trampoline_64.S whenever the stack is setup.
>>
>> The git history is also rewritten to move the commits that removed
>> initial_stack, early_gdt_descr and initial_gs earlier in the patchset.
>>
>> Thanks,
>> Usama
>>
>> Changes across versions:
>> v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
>> v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
>>      in preparation for more parallelisation.
>> v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
>>      avoid scribbling on initial_gs in common_cpu_up(), and to allow all
>>      24 bits of the physical X2APIC ID to be used. That patch still needs
>>      a Signed-off-by from its original author, who once claimed not to
>>      remember writing it at all. But now we've fixed it, hopefully he'll
>>      admit it now :)
>> v5: rebase to v6.1 and remeasure performance, disable parallel bringup
>>      for AMD CPUs.
>> v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
>>      reused timer calibration for secondary CPUs.
>> v7: [David Woodhouse] iterate over all possible CPUs to find any existing
>>      cluster mask in alloc_clustermask. (patch 1/9)
>>      Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
>>      0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
>>      Included sanity checks for APIC id from 0x0B. (patch 6/9)
>>      Removed patch for reusing timer calibration for secondary CPUs.
>>      commit message and code improvements.
>> v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
>>      early_gdt_descr.
>>      Drop trampoline lock and bail if APIC ID not found in find_cpunr.
>>      Code comments improved and debug prints added.
>> v9: Drop patch to avoid repeated saves of MTRR at boot time.
>>      rebased and retested at v6.2-rc8.
>>      added kernel doc for no_parallel_bringup and made do_parallel_bringup
>>      __ro_after_init.
>> v10: Fixed suspend/resume not working with parallel smpboot.
>>       rebased and retested to 6.2.
>>       fixed checkpatch errors.
>> v11: Added patches from Brian Gerst to remove the global variables initial_gs,
>>       initial_stack, and early_gdt_descr from the 64-bit boot code
>>       (https://lore.kernel.org/all/20230222221301.245890-1-brgerst@gmail.com/).
>> v12: Fixed compilation errors, acquire tr_lock for every stack setup in
>>       trampoline_64.S.
>>       Rearranged commits for a cleaner git history.
>>
>> Brian Gerst (3):
>>    x86/smpboot: Remove initial_stack on 64-bit
>>    x86/smpboot: Remove early_gdt_descr on 64-bit
>>    x86/smpboot: Remove initial_gs
>>
>> David Woodhouse (8):
>>    x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
>>    cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
>>    cpu/hotplug: Add dynamic parallel bringup states before
>>      CPUHP_BRINGUP_CPU
>>    x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
>>    x86/smpboot: Split up native_cpu_up into separate phases and document
>>      them
>>    x86/smpboot: Support parallel startup of secondary CPUs
>>    x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
>>    x86/smpboot: Serialize topology updates for secondary bringup
>>
>>   .../admin-guide/kernel-parameters.txt         |   3 +
>>   arch/x86/include/asm/processor.h              |   6 +-
>>   arch/x86/include/asm/realmode.h               |   4 +-
>>   arch/x86/include/asm/smp.h                    |  15 +-
>>   arch/x86/include/asm/topology.h               |   2 -
>>   arch/x86/kernel/acpi/sleep.c                  |  15 +-
>>   arch/x86/kernel/apic/apic.c                   |   2 +-
>>   arch/x86/kernel/apic/x2apic_cluster.c         | 126 ++++---
>>   arch/x86/kernel/asm-offsets.c                 |   1 +
>>   arch/x86/kernel/cpu/common.c                  |   6 +-
>>   arch/x86/kernel/head_64.S                     | 129 +++++--
>>   arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>>   arch/x86/realmode/init.c                      |   3 +
>>   arch/x86/realmode/rm/trampoline_64.S          |  27 +-
>>   arch/x86/xen/smp_pv.c                         |   4 +-
>>   arch/x86/xen/xen-head.S                       |   2 +-
>>   include/linux/cpuhotplug.h                    |   2 +
>>   include/linux/smpboot.h                       |   7 +
>>   kernel/cpu.c                                  |  31 +-
>>   kernel/smpboot.h                              |   2 -
>>   20 files changed, 537 insertions(+), 200 deletions(-)
> 
> With `CONFIG_FORCE_NR_CPUS=y` this results in:
> 
> ```
> ld: vmlinux.o: in function `secondary_startup_64_no_verify':
> (.head.text+0x10c): undefined reference to `nr_cpu_ids'
> ```
> 
> That's because in `arch/x86/kernel/head_64.S` `secondary_startup_64_no_verify()` refers to `nr_cpu_ids` under `#ifdef CONFIG_SMP`, but this symbol is available under the following conditions:
> 
> ```
> 38 #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
> 39 #define nr_cpu_ids ((unsigned int)NR_CPUS)
> 40 #else
> 41 extern unsigned int nr_cpu_ids;
> 42 #endif
> 
> 1090 #if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
> 1091 /* Setup number of possible processor ids */
> 1092 unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
> 1093 EXPORT_SYMBOL(nr_cpu_ids);
> 1094 #endif
> ```
> 
> So having `CONFIG_SMP=y` and, for instance, `CONFIG_NR_CPUS=320`, it is possible to compile out `EXPORT_SYMBOL(nr_cpu_ids);` if `CONFIG_FORCE_NR_CPUS=y` is set.
> 

I think something like below diff should work in all scenarios?

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index c2aa0aa26b45..e3727dab9cab 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -35,7 +35,7 @@ typedef struct cpumask { DECLARE_BITMAP(bits, 
NR_CPUS); } cpumask_t;
   */
  #define cpumask_pr_args(maskp)         nr_cpu_ids, cpumask_bits(maskp)

-#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
+#if ((NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)) && 
!defined(CONFIG_SMP)
  #define nr_cpu_ids ((unsigned int)NR_CPUS)
  #else
  extern unsigned int nr_cpu_ids;
@@ -43,7 +43,7 @@ extern unsigned int nr_cpu_ids;

  static inline void set_nr_cpu_ids(unsigned int nr)
  {
-#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
+#if ((NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)) && 
!defined(CONFIG_SMP)
         WARN_ON(nr != nr_cpu_ids);
  #else
         nr_cpu_ids = nr;
diff --git a/kernel/smp.c b/kernel/smp.c
index 06a413987a14..a051b16d4a24 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -1087,11 +1087,9 @@ static int __init maxcpus(char *str)

  early_param("maxcpus", maxcpus);

-#if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
  /* Setup number of possible processor ids */
  unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
  EXPORT_SYMBOL(nr_cpu_ids);
-#endif

  /* An arch may set nr_cpu_ids earlier if needed, so this would be 
redundant */
  void __init setup_nr_cpu_ids(void)

Re: [External] Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[David Woodhouse]

On 26 February 2023 20:59:17 GMT, Usama Arif <usama.arif@bytedance.com> wrote:
>
>
>On 26/02/2023 18:31, Oleksandr Natalenko wrote:
>> Hello.
>> 
>> On neděle 26. února 2023 12:07:51 CET Usama Arif wrote:
>>> The main code change over v11 is the build error fix by Brian Gerst and
>>> acquiring tr_lock in trampoline_64.S whenever the stack is setup.
>>> 
>>> The git history is also rewritten to move the commits that removed
>>> initial_stack, early_gdt_descr and initial_gs earlier in the patchset.
>>> 
>>> Thanks,
>>> Usama
>>> 
>>> Changes across versions:
>>> v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
>>> v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
>>>      in preparation for more parallelisation.
>>> v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
>>>      avoid scribbling on initial_gs in common_cpu_up(), and to allow all
>>>      24 bits of the physical X2APIC ID to be used. That patch still needs
>>>      a Signed-off-by from its original author, who once claimed not to
>>>      remember writing it at all. But now we've fixed it, hopefully he'll
>>>      admit it now :)
>>> v5: rebase to v6.1 and remeasure performance, disable parallel bringup
>>>      for AMD CPUs.
>>> v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
>>>      reused timer calibration for secondary CPUs.
>>> v7: [David Woodhouse] iterate over all possible CPUs to find any existing
>>>      cluster mask in alloc_clustermask. (patch 1/9)
>>>      Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
>>>      0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
>>>      Included sanity checks for APIC id from 0x0B. (patch 6/9)
>>>      Removed patch for reusing timer calibration for secondary CPUs.
>>>      commit message and code improvements.
>>> v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
>>>      early_gdt_descr.
>>>      Drop trampoline lock and bail if APIC ID not found in find_cpunr.
>>>      Code comments improved and debug prints added.
>>> v9: Drop patch to avoid repeated saves of MTRR at boot time.
>>>      rebased and retested at v6.2-rc8.
>>>      added kernel doc for no_parallel_bringup and made do_parallel_bringup
>>>      __ro_after_init.
>>> v10: Fixed suspend/resume not working with parallel smpboot.
>>>       rebased and retested to 6.2.
>>>       fixed checkpatch errors.
>>> v11: Added patches from Brian Gerst to remove the global variables initial_gs,
>>>       initial_stack, and early_gdt_descr from the 64-bit boot code
>>>       (https://lore.kernel.org/all/20230222221301.245890-1-brgerst@gmail.com/).
>>> v12: Fixed compilation errors, acquire tr_lock for every stack setup in
>>>       trampoline_64.S.
>>>       Rearranged commits for a cleaner git history.
>>> 
>>> Brian Gerst (3):
>>>    x86/smpboot: Remove initial_stack on 64-bit
>>>    x86/smpboot: Remove early_gdt_descr on 64-bit
>>>    x86/smpboot: Remove initial_gs
>>> 
>>> David Woodhouse (8):
>>>    x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
>>>    cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
>>>    cpu/hotplug: Add dynamic parallel bringup states before
>>>      CPUHP_BRINGUP_CPU
>>>    x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
>>>    x86/smpboot: Split up native_cpu_up into separate phases and document
>>>      them
>>>    x86/smpboot: Support parallel startup of secondary CPUs
>>>    x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
>>>    x86/smpboot: Serialize topology updates for secondary bringup
>>> 
>>>   .../admin-guide/kernel-parameters.txt         |   3 +
>>>   arch/x86/include/asm/processor.h              |   6 +-
>>>   arch/x86/include/asm/realmode.h               |   4 +-
>>>   arch/x86/include/asm/smp.h                    |  15 +-
>>>   arch/x86/include/asm/topology.h               |   2 -
>>>   arch/x86/kernel/acpi/sleep.c                  |  15 +-
>>>   arch/x86/kernel/apic/apic.c                   |   2 +-
>>>   arch/x86/kernel/apic/x2apic_cluster.c         | 126 ++++---
>>>   arch/x86/kernel/asm-offsets.c                 |   1 +
>>>   arch/x86/kernel/cpu/common.c                  |   6 +-
>>>   arch/x86/kernel/head_64.S                     | 129 +++++--
>>>   arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>>>   arch/x86/realmode/init.c                      |   3 +
>>>   arch/x86/realmode/rm/trampoline_64.S          |  27 +-
>>>   arch/x86/xen/smp_pv.c                         |   4 +-
>>>   arch/x86/xen/xen-head.S                       |   2 +-
>>>   include/linux/cpuhotplug.h                    |   2 +
>>>   include/linux/smpboot.h                       |   7 +
>>>   kernel/cpu.c                                  |  31 +-
>>>   kernel/smpboot.h                              |   2 -
>>>   20 files changed, 537 insertions(+), 200 deletions(-)
>> 
>> With `CONFIG_FORCE_NR_CPUS=y` this results in:
>> 
>> ```
>> ld: vmlinux.o: in function `secondary_startup_64_no_verify':
>> (.head.text+0x10c): undefined reference to `nr_cpu_ids'
>> ```
>> 
>> That's because in `arch/x86/kernel/head_64.S` `secondary_startup_64_no_verify()` refers to `nr_cpu_ids` under `#ifdef CONFIG_SMP`, but this symbol is available under the following conditions:
>> 
>> ```
>> 38 #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
>> 39 #define nr_cpu_ids ((unsigned int)NR_CPUS)
>> 40 #else
>> 41 extern unsigned int nr_cpu_ids;
>> 42 #endif
>> 
>> 1090 #if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
>> 1091 /* Setup number of possible processor ids */
>> 1092 unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
>> 1093 EXPORT_SYMBOL(nr_cpu_ids);
>> 1094 #endif
>> ```
>> 
>> So having `CONFIG_SMP=y` and, for instance, `CONFIG_NR_CPUS=320`, it is possible to compile out `EXPORT_SYMBOL(nr_cpu_ids);` if `CONFIG_FORCE_NR_CPUS=y` is set.
>> 
>
>I think something like below diff should work in all scenarios?

I'd've changed the asm side to use the constant limit.

Re: [External] Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Usama Arif]

On 27/02/2023 06:13, David Woodhouse wrote:
> 
> 
> On 26 February 2023 20:59:17 GMT, Usama Arif <usama.arif@bytedance.com> wrote:
>>
>>
>> On 26/02/2023 18:31, Oleksandr Natalenko wrote:
>>> Hello.
>>>
>>> On neděle 26. února 2023 12:07:51 CET Usama Arif wrote:
>>>> The main code change over v11 is the build error fix by Brian Gerst and
>>>> acquiring tr_lock in trampoline_64.S whenever the stack is setup.
>>>>
>>>> The git history is also rewritten to move the commits that removed
>>>> initial_stack, early_gdt_descr and initial_gs earlier in the patchset.
>>>>
>>>> Thanks,
>>>> Usama
>>>>
>>>> Changes across versions:
>>>> v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
>>>> v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
>>>>       in preparation for more parallelisation.
>>>> v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
>>>>       avoid scribbling on initial_gs in common_cpu_up(), and to allow all
>>>>       24 bits of the physical X2APIC ID to be used. That patch still needs
>>>>       a Signed-off-by from its original author, who once claimed not to
>>>>       remember writing it at all. But now we've fixed it, hopefully he'll
>>>>       admit it now :)
>>>> v5: rebase to v6.1 and remeasure performance, disable parallel bringup
>>>>       for AMD CPUs.
>>>> v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
>>>>       reused timer calibration for secondary CPUs.
>>>> v7: [David Woodhouse] iterate over all possible CPUs to find any existing
>>>>       cluster mask in alloc_clustermask. (patch 1/9)
>>>>       Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
>>>>       0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
>>>>       Included sanity checks for APIC id from 0x0B. (patch 6/9)
>>>>       Removed patch for reusing timer calibration for secondary CPUs.
>>>>       commit message and code improvements.
>>>> v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
>>>>       early_gdt_descr.
>>>>       Drop trampoline lock and bail if APIC ID not found in find_cpunr.
>>>>       Code comments improved and debug prints added.
>>>> v9: Drop patch to avoid repeated saves of MTRR at boot time.
>>>>       rebased and retested at v6.2-rc8.
>>>>       added kernel doc for no_parallel_bringup and made do_parallel_bringup
>>>>       __ro_after_init.
>>>> v10: Fixed suspend/resume not working with parallel smpboot.
>>>>        rebased and retested to 6.2.
>>>>        fixed checkpatch errors.
>>>> v11: Added patches from Brian Gerst to remove the global variables initial_gs,
>>>>        initial_stack, and early_gdt_descr from the 64-bit boot code
>>>>        (https://lore.kernel.org/all/20230222221301.245890-1-brgerst@gmail.com/).
>>>> v12: Fixed compilation errors, acquire tr_lock for every stack setup in
>>>>        trampoline_64.S.
>>>>        Rearranged commits for a cleaner git history.
>>>>
>>>> Brian Gerst (3):
>>>>     x86/smpboot: Remove initial_stack on 64-bit
>>>>     x86/smpboot: Remove early_gdt_descr on 64-bit
>>>>     x86/smpboot: Remove initial_gs
>>>>
>>>> David Woodhouse (8):
>>>>     x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
>>>>     cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
>>>>     cpu/hotplug: Add dynamic parallel bringup states before
>>>>       CPUHP_BRINGUP_CPU
>>>>     x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
>>>>     x86/smpboot: Split up native_cpu_up into separate phases and document
>>>>       them
>>>>     x86/smpboot: Support parallel startup of secondary CPUs
>>>>     x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
>>>>     x86/smpboot: Serialize topology updates for secondary bringup
>>>>
>>>>    .../admin-guide/kernel-parameters.txt         |   3 +
>>>>    arch/x86/include/asm/processor.h              |   6 +-
>>>>    arch/x86/include/asm/realmode.h               |   4 +-
>>>>    arch/x86/include/asm/smp.h                    |  15 +-
>>>>    arch/x86/include/asm/topology.h               |   2 -
>>>>    arch/x86/kernel/acpi/sleep.c                  |  15 +-
>>>>    arch/x86/kernel/apic/apic.c                   |   2 +-
>>>>    arch/x86/kernel/apic/x2apic_cluster.c         | 126 ++++---
>>>>    arch/x86/kernel/asm-offsets.c                 |   1 +
>>>>    arch/x86/kernel/cpu/common.c                  |   6 +-
>>>>    arch/x86/kernel/head_64.S                     | 129 +++++--
>>>>    arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>>>>    arch/x86/realmode/init.c                      |   3 +
>>>>    arch/x86/realmode/rm/trampoline_64.S          |  27 +-
>>>>    arch/x86/xen/smp_pv.c                         |   4 +-
>>>>    arch/x86/xen/xen-head.S                       |   2 +-
>>>>    include/linux/cpuhotplug.h                    |   2 +
>>>>    include/linux/smpboot.h                       |   7 +
>>>>    kernel/cpu.c                                  |  31 +-
>>>>    kernel/smpboot.h                              |   2 -
>>>>    20 files changed, 537 insertions(+), 200 deletions(-)
>>>
>>> With `CONFIG_FORCE_NR_CPUS=y` this results in:
>>>
>>> ```
>>> ld: vmlinux.o: in function `secondary_startup_64_no_verify':
>>> (.head.text+0x10c): undefined reference to `nr_cpu_ids'
>>> ```
>>>
>>> That's because in `arch/x86/kernel/head_64.S` `secondary_startup_64_no_verify()` refers to `nr_cpu_ids` under `#ifdef CONFIG_SMP`, but this symbol is available under the following conditions:
>>>
>>> ```
>>> 38 #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
>>> 39 #define nr_cpu_ids ((unsigned int)NR_CPUS)
>>> 40 #else
>>> 41 extern unsigned int nr_cpu_ids;
>>> 42 #endif
>>>
>>> 1090 #if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
>>> 1091 /* Setup number of possible processor ids */
>>> 1092 unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
>>> 1093 EXPORT_SYMBOL(nr_cpu_ids);
>>> 1094 #endif
>>> ```
>>>
>>> So having `CONFIG_SMP=y` and, for instance, `CONFIG_NR_CPUS=320`, it is possible to compile out `EXPORT_SYMBOL(nr_cpu_ids);` if `CONFIG_FORCE_NR_CPUS=y` is set.
>>>
>>
>> I think something like below diff should work in all scenarios?
> 
> I'd've changed the asm side to use the constant limit.

Yup, just needed the morning coffee :) Had sent the proper fix in 
https://lore.kernel.org/all/5e8ad90a-1dc6-95c2-e020-5e95da6f9eda@bytedance.com/#t

I guess the diff is still small over v12 (including the cosmetic 
changes) to send out a new version so soon, probably better to wait a 
couple of days incase something else comes up as well?

Thanks,
Usama

Re: [External] Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Usama Arif]

On 26/02/2023 20:59, Usama Arif wrote:
> 
> 
> On 26/02/2023 18:31, Oleksandr Natalenko wrote:
>> Hello.
>>
>> On neděle 26. února 2023 12:07:51 CET Usama Arif wrote:
>>> The main code change over v11 is the build error fix by Brian Gerst and
>>> acquiring tr_lock in trampoline_64.S whenever the stack is setup.
>>>
>>> The git history is also rewritten to move the commits that removed
>>> initial_stack, early_gdt_descr and initial_gs earlier in the patchset.
>>>
>>> Thanks,
>>> Usama
>>>
>>> Changes across versions:
>>> v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
>>> v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
>>>      in preparation for more parallelisation.
>>> v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
>>>      avoid scribbling on initial_gs in common_cpu_up(), and to allow all
>>>      24 bits of the physical X2APIC ID to be used. That patch still 
>>> needs
>>>      a Signed-off-by from its original author, who once claimed not to
>>>      remember writing it at all. But now we've fixed it, hopefully he'll
>>>      admit it now :)
>>> v5: rebase to v6.1 and remeasure performance, disable parallel bringup
>>>      for AMD CPUs.
>>> v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
>>>      reused timer calibration for secondary CPUs.
>>> v7: [David Woodhouse] iterate over all possible CPUs to find any 
>>> existing
>>>      cluster mask in alloc_clustermask. (patch 1/9)
>>>      Keep parallel AMD support enabled in AMD, using APIC ID in CPUID 
>>> leaf
>>>      0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are 
>>> sufficient.
>>>      Included sanity checks for APIC id from 0x0B. (patch 6/9)
>>>      Removed patch for reusing timer calibration for secondary CPUs.
>>>      commit message and code improvements.
>>> v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
>>>      early_gdt_descr.
>>>      Drop trampoline lock and bail if APIC ID not found in find_cpunr.
>>>      Code comments improved and debug prints added.
>>> v9: Drop patch to avoid repeated saves of MTRR at boot time.
>>>      rebased and retested at v6.2-rc8.
>>>      added kernel doc for no_parallel_bringup and made 
>>> do_parallel_bringup
>>>      __ro_after_init.
>>> v10: Fixed suspend/resume not working with parallel smpboot.
>>>       rebased and retested to 6.2.
>>>       fixed checkpatch errors.
>>> v11: Added patches from Brian Gerst to remove the global variables 
>>> initial_gs,
>>>       initial_stack, and early_gdt_descr from the 64-bit boot code
>>>       
>>> (https://lore.kernel.org/all/20230222221301.245890-1-brgerst@gmail.com/).
>>> v12: Fixed compilation errors, acquire tr_lock for every stack setup in
>>>       trampoline_64.S.
>>>       Rearranged commits for a cleaner git history.
>>>
>>> Brian Gerst (3):
>>>    x86/smpboot: Remove initial_stack on 64-bit
>>>    x86/smpboot: Remove early_gdt_descr on 64-bit
>>>    x86/smpboot: Remove initial_gs
>>>
>>> David Woodhouse (8):
>>>    x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
>>>    cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
>>>    cpu/hotplug: Add dynamic parallel bringup states before
>>>      CPUHP_BRINGUP_CPU
>>>    x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
>>>    x86/smpboot: Split up native_cpu_up into separate phases and document
>>>      them
>>>    x86/smpboot: Support parallel startup of secondary CPUs
>>>    x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
>>>    x86/smpboot: Serialize topology updates for secondary bringup
>>>
>>>   .../admin-guide/kernel-parameters.txt         |   3 +
>>>   arch/x86/include/asm/processor.h              |   6 +-
>>>   arch/x86/include/asm/realmode.h               |   4 +-
>>>   arch/x86/include/asm/smp.h                    |  15 +-
>>>   arch/x86/include/asm/topology.h               |   2 -
>>>   arch/x86/kernel/acpi/sleep.c                  |  15 +-
>>>   arch/x86/kernel/apic/apic.c                   |   2 +-
>>>   arch/x86/kernel/apic/x2apic_cluster.c         | 126 ++++---
>>>   arch/x86/kernel/asm-offsets.c                 |   1 +
>>>   arch/x86/kernel/cpu/common.c                  |   6 +-
>>>   arch/x86/kernel/head_64.S                     | 129 +++++--
>>>   arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>>>   arch/x86/realmode/init.c                      |   3 +
>>>   arch/x86/realmode/rm/trampoline_64.S          |  27 +-
>>>   arch/x86/xen/smp_pv.c                         |   4 +-
>>>   arch/x86/xen/xen-head.S                       |   2 +-
>>>   include/linux/cpuhotplug.h                    |   2 +
>>>   include/linux/smpboot.h                       |   7 +
>>>   kernel/cpu.c                                  |  31 +-
>>>   kernel/smpboot.h                              |   2 -
>>>   20 files changed, 537 insertions(+), 200 deletions(-)
>>
>> With `CONFIG_FORCE_NR_CPUS=y` this results in:
>>
>> ```
>> ld: vmlinux.o: in function `secondary_startup_64_no_verify':
>> (.head.text+0x10c): undefined reference to `nr_cpu_ids'
>> ```
>>
>> That's because in `arch/x86/kernel/head_64.S` 
>> `secondary_startup_64_no_verify()` refers to `nr_cpu_ids` under 
>> `#ifdef CONFIG_SMP`, but this symbol is available under the following 
>> conditions:
>>
>> ```
>> 38 #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
>> 39 #define nr_cpu_ids ((unsigned int)NR_CPUS)
>> 40 #else
>> 41 extern unsigned int nr_cpu_ids;
>> 42 #endif
>>
>> 1090 #if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
>> 1091 /* Setup number of possible processor ids */
>> 1092 unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
>> 1093 EXPORT_SYMBOL(nr_cpu_ids);
>> 1094 #endif
>> ```
>>
>> So having `CONFIG_SMP=y` and, for instance, `CONFIG_NR_CPUS=320`, it 
>> is possible to compile out `EXPORT_SYMBOL(nr_cpu_ids);` if 
>> `CONFIG_FORCE_NR_CPUS=y` is set.
>>
> 
> I think something like below diff should work in all scenarios?
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index c2aa0aa26b45..e3727dab9cab 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -35,7 +35,7 @@ typedef struct cpumask { DECLARE_BITMAP(bits, 
> NR_CPUS); } cpumask_t;
>    */
>   #define cpumask_pr_args(maskp)         nr_cpu_ids, cpumask_bits(maskp)
> 
> -#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
> +#if ((NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)) && 
> !defined(CONFIG_SMP)
>   #define nr_cpu_ids ((unsigned int)NR_CPUS)
>   #else
>   extern unsigned int nr_cpu_ids;
> @@ -43,7 +43,7 @@ extern unsigned int nr_cpu_ids;
> 
>   static inline void set_nr_cpu_ids(unsigned int nr)
>   {
> -#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
> +#if ((NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)) && 
> !defined(CONFIG_SMP)
>          WARN_ON(nr != nr_cpu_ids);
>   #else
>          nr_cpu_ids = nr;
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 06a413987a14..a051b16d4a24 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -1087,11 +1087,9 @@ static int __init maxcpus(char *str)
> 
>   early_param("maxcpus", maxcpus);
> 
> -#if (NR_CPUS > 1) && !defined(CONFIG_FORCE_NR_CPUS)
>   /* Setup number of possible processor ids */
>   unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
>   EXPORT_SYMBOL(nr_cpu_ids);
> -#endif
> 
>   /* An arch may set nr_cpu_ids earlier if needed, so this would be 
> redundant */
>   void __init setup_nr_cpu_ids(void)

Or better just do below?

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 17bdd6122dca..5d709aa67df4 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -273,7 +273,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
SYM_L_GLOBAL)
         cmpl    (%rbx,%rcx,4), %edx
         jz      .Lsetup_cpu
         inc     %ecx
+#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
+       cmpl    $NR_CPUS, %ecx
+#else
         cmpl    nr_cpu_ids(%rip), %ecx
+#endif
         jb      .Lfind_cpunr

         /*  APIC ID not found in the table. Drop the trampoline lock 
and bail. */

Re: [External] Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Mon, 2023-02-27 at 06:14 +0000, Usama Arif wrote:
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 17bdd6122dca..5d709aa67df4 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -273,7 +273,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
> SYM_L_GLOBAL)
>          cmpl    (%rbx,%rcx,4), %edx
>          jz      .Lsetup_cpu
>          inc     %ecx
> +#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
> +       cmpl    $NR_CPUS, %ecx
> +#else
>          cmpl    nr_cpu_ids(%rip), %ecx
> +#endif
>          jb      .Lfind_cpunr
> 
>          /*  APIC ID not found in the table. Drop the trampoline lock
> and bail. */

The whitespace looks dodgy there but maybe that's just your mail client?

Given this code is already in #ifdef CONFIG_SMP, can NR_CPUS be 1?

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

Re: [External] Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Usama Arif]

On 27/02/2023 15:29, David Woodhouse wrote:
> On Mon, 2023-02-27 at 06:14 +0000, Usama Arif wrote:
>>
>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> index 17bdd6122dca..5d709aa67df4 100644
>> --- a/arch/x86/kernel/head_64.S
>> +++ b/arch/x86/kernel/head_64.S
>> @@ -273,7 +273,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify,
>> SYM_L_GLOBAL)
>>           cmpl    (%rbx,%rcx,4), %edx
>>           jz      .Lsetup_cpu
>>           inc     %ecx
>> +#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
>> +       cmpl    $NR_CPUS, %ecx
>> +#else
>>           cmpl    nr_cpu_ids(%rip), %ecx
>> +#endif
>>           jb      .Lfind_cpunr
>>
>>           /*  APIC ID not found in the table. Drop the trampoline lock
>> and bail. */
> 
> The whitespace looks dodgy there but maybe that's just your mail client?
> 
> Given this code is already in #ifdef CONFIG_SMP, can NR_CPUS be 1?

Ah yes, we have

config NR_CPUS_RANGE_BEGIN
	int
	default NR_CPUS_RANGE_END if MAXSMP
	default    1 if !SMP
	default    2

in arch/x86/Kconfig which doesn't let us select 1 for NR_CPUS if SMP is 
enabled, so this should be enough
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 17bdd6122dca..c79ae67492e1 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -273,7 +273,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
SYM_L_GLOBAL)
         cmpl    (%rbx,%rcx,4), %edx
         jz      .Lsetup_cpu
         inc     %ecx
+#if defined(CONFIG_FORCE_NR_CPUS)
+       cmpl    $NR_CPUS, %ebx
+#else
         cmpl    nr_cpu_ids(%rip), %ecx
+#endif
         jb      .Lfind_cpunr

Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Guilherme G. Piccoli]

Hi Usama and David, thanks for the great series!

I've tested it on Steam Deck (with and without the "no_parallel_bringup"
parameter), it works fine - also tested S3/deep sleep-resume cycle.

Feel free to add (to the series):
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Also, just taking the opportunity since I'm already replying here: on
patch 09, found two typos:

s/correect/correct (commit message)
s/brinugp/bring-up (kernel-parameters.txt)

Cheers,


Guilherme

Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Mon, 2023-02-27 at 18:39 -0300, Guilherme G. Piccoli wrote:
> Hi Usama and David, thanks for the great series!
> 
> I've tested it on Steam Deck (with and without the
> "no_parallel_bringup"
> parameter), it works fine - also tested S3/deep sleep-resume cycle.
> 
> Feel free to add (to the series):
> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Also, just taking the opportunity since I'm already replying here: on
> patch 09, found two typos:
> 
> s/correect/correct (commit message)
> s/brinugp/bring-up (kernel-parameters.txt)
> 
> Cheers,

Thanks. I've done that and pushed it out to
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-rc8-v12bis
ready for the next round.

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

Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Paul Menzel]

Dear Guilherme,


Am 27.02.23 um 22:39 schrieb Guilherme G. Piccoli:

> I've tested it on Steam Deck (with and without the "no_parallel_bringup"
> parameter), it works fine - also tested S3/deep sleep-resume cycle.
> 
> Feel free to add (to the series):
> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Thank you for testing the series. It’d be great if you could share the 
timing differences.

[…]


Kind regards,

Paul

Re: [PATCH v12 00/11] Parallel CPU bringup for x86_64

[Guilherme G. Piccoli]

On 28/02/2023 07:13, Paul Menzel wrote:
> Dear Guilherme,
> 
> 
> Am 27.02.23 um 22:39 schrieb Guilherme G. Piccoli:
> 
>> I've tested it on Steam Deck (with and without the "no_parallel_bringup"
>> parameter), it works fine - also tested S3/deep sleep-resume cycle.
>>
>> Feel free to add (to the series):
>> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Thank you for testing the series. It’d be great if you could share the 
> timing differences.
> 
> […]
> 
> 
> Kind regards,
> 
> Paul

Hi Paul!

The results...weren't so great, I felt no difference heh
Which is also not bad, it seems the series favors big SMP systems, Deck
has only 8 CPUs.

But maybe the way I measured is not ideal? I just compared timestamps on
dmesg from the first SMP message up to the one that says the boot of
secondary CPUs is complete. Do you have a better suggestion? I can try
things here.

Cheers,


Guilherme