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

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

[Usama Arif]

The main change over v8 is dropping the patch to avoid repeated saves of MTRR
at boot time. It didn't make a difference to smpboot time and is independent
of parallel CPU bringup, so if needed can be explored in a separate patchset.

The patches have also been rebased to v6.2-rc8 and retested and the
improvement in boot time is the same as v8.

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.

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/realmode.h               |   3 +
 arch/x86/include/asm/smp.h                    |  14 +-
 arch/x86/include/asm/topology.h               |   2 -
 arch/x86/kernel/acpi/sleep.c                  |   1 +
 arch/x86/kernel/apic/apic.c                   |   2 +-
 arch/x86/kernel/apic/x2apic_cluster.c         | 130 ++++---
 arch/x86/kernel/cpu/common.c                  |   6 +-
 arch/x86/kernel/head_64.S                     |  99 ++++-
 arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
 arch/x86/realmode/init.c                      |   3 +
 arch/x86/realmode/rm/trampoline_64.S          |  14 +
 arch/x86/xen/smp_pv.c                         |   4 +-
 include/linux/cpuhotplug.h                    |   2 +
 include/linux/smpboot.h                       |   7 +
 kernel/cpu.c                                  |  31 +-
 kernel/smpboot.c                              |   2 +-
 kernel/smpboot.h                              |   2 -
 18 files changed, 515 insertions(+), 160 deletions(-)

-- 
2.25.1

[PATCH v9 1/8] 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>
---
 arch/x86/kernel/apic/x2apic_cluster.c | 130 +++++++++++++++++---------
 1 file changed, 84 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index e696e22d0531..d8f513a2287b 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)
-		return -ENOMEM;
-	cluster_hotplug_mask->node = node;
-	return 0;
+	/*
+	 * 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;
+	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

Re: [PATCH v9 1/8] x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel

[Kim Phillips]

On 2/15/23 8:54 AM, Usama Arif wrote:
> -
> -	cluster_hotplug_mask = kzalloc_node(sizeof(*cluster_hotplug_mask),
> -					    GFP_KERNEL, node);
> -	if (!cluster_hotplug_mask)
> -		return -ENOMEM;
> -	cluster_hotplug_mask->node = node;
> -	return 0;
> +	/*
> +	 * 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;

.git/rebase-apply/patch:167: space before tab in indent.
  		return -ENOMEM;

> +	per_cpu(cluster_masks, cpu) = cmsk;
> +	prefill_clustermask(cmsk, cpu, cluster);
> +
> + 	return 0;

.git/rebase-apply/patch:171: space before tab in indent.
  	return 0;
warning: 2 lines add whitespace errors.

Other than that:

Tested-by: Kim Phillips <kim.phillips@amd.com>

Thanks,

Kim

[PATCH v9 2/8] 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>
---
 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 v9 3/8] 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>
---
 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 v9 4/8] 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.

[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 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 v9 5/8] 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.

[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 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..b9366f86c433 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 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 v9 6/8] 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 the use of global variables for passing per-CPU information to
   the APs.

To achieve the latter, export the cpuid_to_apicid[] array so that each
AP can find its own per_cpu data (and thus initial_gs, initial_stack and
early_gdt_descr) by searching therein based on its APIC ID.

Introduce a global variable 'smpboot_control' indicating to the AP how
it should find its APIC ID. For a serialized bringup, the APIC ID is
explicitly passed in the low bits of smpboot_control, while 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.

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 per-cpu data 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() ]
Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 .../admin-guide/kernel-parameters.txt         |  3 +
 arch/x86/include/asm/realmode.h               |  3 +
 arch/x86/include/asm/smp.h                    | 10 +-
 arch/x86/kernel/acpi/sleep.c                  |  1 +
 arch/x86/kernel/apic/apic.c                   |  2 +-
 arch/x86/kernel/head_64.S                     | 99 ++++++++++++++++++-
 arch/x86/kernel/smpboot.c                     | 62 +++++++++++-
 arch/x86/realmode/init.c                      |  3 +
 arch/x86/realmode/rm/trampoline_64.S          | 14 +++
 kernel/smpboot.c                              |  2 +-
 10 files changed, 191 insertions(+), 8 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 a336feef0af1..f0357cfe2fb0 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
 };
 
@@ -65,6 +66,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 b4dbb20dab1a..33c0d5fd8af6 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,5 +199,13 @@ extern void nmi_selftest(void);
 #define nmi_selftest() do { } while (0)
 #endif
 
-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
+/* Control bits for startup_64 */
+#define STARTUP_SECONDARY	0x80000000
+#define STARTUP_APICID_CPUID_0B	0x40000000
+#define STARTUP_APICID_CPUID_01	0x20000000
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3b7f4cdbf2e0..06adf340a0f1 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -115,6 +115,7 @@ int x86_acpi_suspend_lowlevel(void)
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_rw(smp_processor_id());
 	initial_gs = per_cpu_offset(smp_processor_id());
+	smpboot_control = 0;
 #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 222efd4a09bc..0e4e53d231db 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
@@ -241,6 +242,83 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	UNWIND_HINT_EMPTY
 	ANNOTATE_NOENDBR // above
 
+#ifdef CONFIG_SMP
+	/*
+	 * Is this the boot CPU coming up? If so everything is available
+	 * in initial_gs, initial_stack and early_gdt_descr.
+	 */
+	movl	smpboot_control(%rip), %edx
+	testl	$STARTUP_SECONDARY, %edx
+	jz	.Lsetup_cpu
+
+	/*
+	 * Secondary CPUs find out the offsets via the APIC ID. For parallel
+	 * boot the APIC ID is retrieved from CPUID, otherwise it's encoded
+	 * in smpboot_control:
+	 * Bit 31	STARTUP_SECONDARY flag (checked above)
+	 * Bit 30	STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
+	 * Bit 29	STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
+	 * Bit 0-24	APIC ID if STARTUP_APICID_CPUID_xx flags are not set
+	 */
+	testl	$STARTUP_APICID_CPUID_0B, %edx
+	jnz	.Luse_cpuid_0b
+	testl	$STARTUP_APICID_CPUID_01, %edx
+	jnz	.Luse_cpuid_01
+	andl	$0x0FFFFFFF, %edx
+	jmp	.Lsetup_AP
+
+.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	.Linit_cpu_data
+	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
+
+.Linit_cpu_data:
+	/* Get the per cpu offset for the given CPU# which is in ECX */
+	leaq	__per_cpu_offset(%rip), %rbx
+	movq	(%rbx,%rcx,8), %rbx
+	/* Save it for GS BASE setup */
+	movq	%rbx, initial_gs(%rip)
+
+	/* Calculate the GDT address */
+	movq	$gdt_page, %rcx
+	addq	%rbx, %rcx
+	movq	%rcx, early_gdt_descr_base(%rip)
+
+	/* Find the idle task stack */
+	movq	idle_threads(%rbx), %rcx
+	movq	TASK_threadsp(%rcx), %rcx
+	movq	%rcx, initial_stack(%rip)
+#endif /* CONFIG_SMP */
+
+.Lsetup_cpu:
 	/*
 	 * 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
@@ -281,6 +359,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	 */
 	movq initial_stack(%rip), %rsp
 
+	/* 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
@@ -372,7 +458,14 @@ SYM_CODE_END(secondary_startup_64)
 SYM_CODE_START(start_cpu0)
 	ANNOTATE_NOENDBR
 	UNWIND_HINT_EMPTY
-	movq	initial_stack(%rip), %rsp
+	/* Load the per-cpu base for CPU#0 */
+	leaq	__per_cpu_offset(%rip), %rbx
+	movq	(%rbx), %rbx
+
+	/* Find the idle task stack */
+	movq	idle_threads(%rbx), %rcx
+	movq	TASK_threadsp(%rcx), %rsp
+
 	jmp	.Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif
@@ -426,6 +519,7 @@ SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
  * reliably detect the end of the stack.
  */
 SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
+SYM_DATA(trampoline_lock, .quad 0);
 	__FINITDATA
 
 	__INIT
@@ -660,6 +754,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 b9366f86c433..49d6563e4c23 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 */
@@ -1084,8 +1094,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;
 }
@@ -1110,9 +1118,14 @@ 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;
-	initial_stack  = idle->thread.sp;
+
+	if (IS_ENABLED(CONFIG_X86_32)) {
+		early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
+		initial_stack  = idle->thread.sp;
+	} else if (!do_parallel_bringup) {
+		smpboot_control = STARTUP_SECONDARY | apicid;
+	}
 
 	/* Enable the espfix hack for this CPU */
 	init_espfix_ap(cpu);
@@ -1512,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_SECONDARY | 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_SECONDARY | 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..49ebc1636ffd 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -49,6 +49,19 @@ SYM_CODE_START(trampoline_start)
 	mov	%ax, %es
 	mov	%ax, %ss
 
+	/*
+	 * 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
 
@@ -241,6 +254,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"
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index 2c7396da470c..a18a21dff9bc 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -25,7 +25,7 @@
  * For the hotplug case we keep the task structs around and reuse
  * them.
  */
-static DEFINE_PER_CPU(struct task_struct *, idle_threads);
+DEFINE_PER_CPU(struct task_struct *, idle_threads);
 
 struct task_struct *idle_thread_get(unsigned int cpu)
 {
-- 
2.25.1

[PATCH v9 7/8] 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.

[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 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 49d6563e4c23..4cea3a0ff503 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_SECONDARY | 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 v9 8/8] 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)

[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 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 33c0d5fd8af6..b4b29e052b6e 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 9cfca3d7d0e2..cf1a4eff7a76 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1766,7 +1766,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();
@@ -1777,8 +1777,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
@@ -1969,7 +1967,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 4cea3a0ff503..fecd934e72fb 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 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 v9 0/8] Parallel CPU bringup for x86_64

[Paul E. McKenney]

On Wed, Feb 15, 2023 at 02:54:17PM +0000, Usama Arif wrote:
> The main change over v8 is dropping the patch to avoid repeated saves of MTRR
> at boot time. It didn't make a difference to smpboot time and is independent
> of parallel CPU bringup, so if needed can be explored in a separate patchset.
> 
> The patches have also been rebased to v6.2-rc8 and retested and the
> improvement in boot time is the same as v8.

This version passes moderate torture testing.

Tested-by: Paul E. McKenney <paulmck@kernel.org>

> 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.
> 
> 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/realmode.h               |   3 +
>  arch/x86/include/asm/smp.h                    |  14 +-
>  arch/x86/include/asm/topology.h               |   2 -
>  arch/x86/kernel/acpi/sleep.c                  |   1 +
>  arch/x86/kernel/apic/apic.c                   |   2 +-
>  arch/x86/kernel/apic/x2apic_cluster.c         | 130 ++++---
>  arch/x86/kernel/cpu/common.c                  |   6 +-
>  arch/x86/kernel/head_64.S                     |  99 ++++-
>  arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>  arch/x86/realmode/init.c                      |   3 +
>  arch/x86/realmode/rm/trampoline_64.S          |  14 +
>  arch/x86/xen/smp_pv.c                         |   4 +-
>  include/linux/cpuhotplug.h                    |   2 +
>  include/linux/smpboot.h                       |   7 +
>  kernel/cpu.c                                  |  31 +-
>  kernel/smpboot.c                              |   2 +-
>  kernel/smpboot.h                              |   2 -
>  18 files changed, 515 insertions(+), 160 deletions(-)
> 
> -- 
> 2.25.1
> 

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

[Oleksandr Natalenko]

Hello.

On středa 15. února 2023 15:54:17 CET Usama Arif wrote:
> The main change over v8 is dropping the patch to avoid repeated saves of MTRR
> at boot time. It didn't make a difference to smpboot time and is independent
> of parallel CPU bringup, so if needed can be explored in a separate patchset.
> 
> The patches have also been rebased to v6.2-rc8 and retested and the
> improvement in boot time is the same as v8.
> 
> 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.
> 
> 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/realmode.h               |   3 +
>  arch/x86/include/asm/smp.h                    |  14 +-
>  arch/x86/include/asm/topology.h               |   2 -
>  arch/x86/kernel/acpi/sleep.c                  |   1 +
>  arch/x86/kernel/apic/apic.c                   |   2 +-
>  arch/x86/kernel/apic/x2apic_cluster.c         | 130 ++++---
>  arch/x86/kernel/cpu/common.c                  |   6 +-
>  arch/x86/kernel/head_64.S                     |  99 ++++-
>  arch/x86/kernel/smpboot.c                     | 350 +++++++++++++-----
>  arch/x86/realmode/init.c                      |   3 +
>  arch/x86/realmode/rm/trampoline_64.S          |  14 +
>  arch/x86/xen/smp_pv.c                         |   4 +-
>  include/linux/cpuhotplug.h                    |   2 +
>  include/linux/smpboot.h                       |   7 +
>  kernel/cpu.c                                  |  31 +-
>  kernel/smpboot.c                              |   2 +-
>  kernel/smpboot.h                              |   2 -
>  18 files changed, 515 insertions(+), 160 deletions(-)

I've applied this to the v6.2 kernel, and suspend/resume broke on my Ryzen 5950X desktop. The machine suspends just fine, but on resume the screen stays blank, and there's no visible disk I/O.

Reverting the series brings suspend/resume back to working state.

-- 
Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
> 
> I've applied this to the v6.2 kernel, and suspend/resume broke on my
> Ryzen 5950X desktop. The machine suspends just fine, but on resume
> the screen stays blank, and there's no visible disk I/O.
> 
> Reverting the series brings suspend/resume back to working state.

Hm, thanks. What if you add 'no_parallel_bringup' on the command line?

Is that using X2APIC? If so, what about 'nox2apic' on the command line?

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

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

[Oleksandr Natalenko]

Hello.

Thank you for your reply.

On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
> On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
> > 
> > I've applied this to the v6.2 kernel, and suspend/resume broke on my
> > Ryzen 5950X desktop. The machine suspends just fine, but on resume
> > the screen stays blank, and there's no visible disk I/O.
> > 
> > Reverting the series brings suspend/resume back to working state.
> 
> Hm, thanks. What if you add 'no_parallel_bringup' on the command line?

If the `no_parallel_bringup` param is added, the suspend/resume works.

> Is that using X2APIC?

Probably?

```
$ dmesg | grep -i x2apic
[    0.632740] x2apic: IRQ remapping doesn't support X2APIC mode
```

> If so, what about 'nox2apic' on the command line?

If `no_parallel_bringup` is removed and `nox2apic` is added, then the `x2apic: …` stuff disappears from dmesg, but suspend/resume remains broken.

-- 
Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
> Hello.
> 
> Thank you for your reply.
> 
> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
> > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
> > > 
> > > I've applied this to the v6.2 kernel, and suspend/resume broke on
> > > my
> > > Ryzen 5950X desktop. The machine suspends just fine, but on
> > > resume
> > > the screen stays blank, and there's no visible disk I/O.
> > > 
> > > Reverting the series brings suspend/resume back to working state.
> > 
> > Hm, thanks. What if you add 'no_parallel_bringup' on the command
> > line?
> 
> If the `no_parallel_bringup` param is added, the suspend/resume
> works.

Thanks for the testing. Can I ask you to do one further test: apply the
series only as far as patch 6/8 'x86/smpboot: Support parallel startup
of secondary CPUs'.

That will do the new startup asm sequence where each CPU finds its own
per-cpu data so it *could* work in parallel, but doesn't actually do
the bringup in parallel yet.

Does your box have a proper serial port? 

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

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

[Oleksandr Natalenko]

Hello.

On 20.02.2023 21:31, David Woodhouse wrote:
> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>> > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>> > >
>> > > I've applied this to the v6.2 kernel, and suspend/resume broke on
>> > > my
>> > > Ryzen 5950X desktop. The machine suspends just fine, but on
>> > > resume
>> > > the screen stays blank, and there's no visible disk I/O.
>> > >
>> > > Reverting the series brings suspend/resume back to working state.
>> >
>> > Hm, thanks. What if you add 'no_parallel_bringup' on the command
>> > line?
>> 
>> If the `no_parallel_bringup` param is added, the suspend/resume
>> works.
> 
> Thanks for the testing. Can I ask you to do one further test: apply the
> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
> of secondary CPUs'.
> 
> That will do the new startup asm sequence where each CPU finds its own
> per-cpu data so it *could* work in parallel, but doesn't actually do
> the bringup in parallel yet.

With patches 1 to 6 (including) applied and no extra cmdline params 
added the resume doesn't work.

> Does your box have a proper serial port?

No, sorry. I know it'd help with getting logs, and I do have a 
serial-to-USB cable that I use for another machine, but in this one the 
port is not routed to outside. I think I can put a header there as the 
motherboard does have pins, but I'd have to buy one first. In theory, I 
can do that, but that won't happen within the next few weeks.

P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS 
can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core 
when using smp boot patchset". Probably, he can reply to this thread and 
provide more details.

-- 
   Oleksandr Natalenko (post-factum)

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

[Piotr Gorski]

Hello.

In my case, admittedly, the error does not occur, while I have 
information from a friend that AMD FX 6300 only shows 1 core when using 
smp boot patchset, so mentioned Oleksandr. Probably soon the friend will 
join the discussion and will be able to provide more information.

W dniu 20.02.2023 o 22:23, Oleksandr Natalenko pisze:
> Hello.
>
> On 20.02.2023 21:31, David Woodhouse wrote:
>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>> > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>> > >
>>> > > I've applied this to the v6.2 kernel, and suspend/resume broke on
>>> > > my
>>> > > Ryzen 5950X desktop. The machine suspends just fine, but on
>>> > > resume
>>> > > the screen stays blank, and there's no visible disk I/O.
>>> > >
>>> > > Reverting the series brings suspend/resume back to working state.
>>> >
>>> > Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>> > line?
>>>
>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>> works.
>>
>> Thanks for the testing. Can I ask you to do one further test: apply the
>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>> of secondary CPUs'.
>>
>> That will do the new startup asm sequence where each CPU finds its own
>> per-cpu data so it *could* work in parallel, but doesn't actually do
>> the bringup in parallel yet.
>
> With patches 1 to 6 (including) applied and no extra cmdline params 
> added the resume doesn't work.
>
>> Does your box have a proper serial port?
>
> No, sorry. I know it'd help with getting logs, and I do have a 
> serial-to-USB cable that I use for another machine, but in this one 
> the port is not routed to outside. I think I can put a header there as 
> the motherboard does have pins, but I'd have to buy one first. In 
> theory, I can do that, but that won't happen within the next few weeks.
>
> P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS 
> can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core 
> when using smp boot patchset". Probably, he can reply to this thread 
> and provide more details.
>

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

[David Woodhouse]

On 20 February 2023 21:23:38 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>Hello.
>
>On 20.02.2023 21:31, David Woodhouse wrote:
>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>> > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>> > >
>>> > > I've applied this to the v6.2 kernel, and suspend/resume broke on
>>> > > my
>>> > > Ryzen 5950X desktop. The machine suspends just fine, but on
>>> > > resume
>>> > > the screen stays blank, and there's no visible disk I/O.
>>> > >
>>> > > Reverting the series brings suspend/resume back to working state.
>>> >
>>> > Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>> > line?
>>> 
>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>> works.
>> 
>> Thanks for the testing. Can I ask you to do one further test: apply the
>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>> of secondary CPUs'.
>> 
>> That will do the new startup asm sequence where each CPU finds its own
>> per-cpu data so it *could* work in parallel, but doesn't actually do
>> the bringup in parallel yet.
>
>With patches 1 to 6 (including) applied and no extra cmdline params added the resume doesn't work.

Hm. Kim, is there some weirdness with the way AMD CPUs get their APIC ID in CPUID 0x1? Especially after resume?

Perhaps we turn it off for any AMD CPU that doesn't have X2APIC and CPUID 0xB?

>> Does your box have a proper serial port?
>
>No, sorry. I know it'd help with getting logs, and I do have a serial-to-USB cable that I use for another machine, but in this one the port is not routed to outside. I think I can put a header there as the motherboard does have pins, but I'd have to buy one first. In theory, I can do that, but that won't happen within the next few weeks.
>
>P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core when using smp boot patchset". Probably, he can reply to this thread and provide more details.
>

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

[Kim Phillips]

On 2/20/23 3:39 PM, David Woodhouse wrote:
> On 20 February 2023 21:23:38 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>> Hello.
>>
>> On 20.02.2023 21:31, David Woodhouse wrote:
>>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>>>> On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>>>>>
>>>>>> I've applied this to the v6.2 kernel, and suspend/resume broke on
>>>>>> my
>>>>>> Ryzen 5950X desktop. The machine suspends just fine, but on
>>>>>> resume
>>>>>> the screen stays blank, and there's no visible disk I/O.
>>>>>>
>>>>>> Reverting the series brings suspend/resume back to working state.
>>>>>
>>>>> Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>>>> line?
>>>>
>>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>>> works.
>>>
>>> Thanks for the testing. Can I ask you to do one further test: apply the
>>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>>> of secondary CPUs'.
>>>
>>> That will do the new startup asm sequence where each CPU finds its own
>>> per-cpu data so it *could* work in parallel, but doesn't actually do
>>> the bringup in parallel yet.
>>
>> With patches 1 to 6 (including) applied and no extra cmdline params added the resume doesn't work.
> 
> Hm. Kim, is there some weirdness with the way AMD CPUs get their APIC ID in CPUID 0x1? Especially after resume?

Not to my knowledge.  Mario?

> Perhaps we turn it off for any AMD CPU that doesn't have X2APIC and CPUID 0xB?

Perhaps.

>>> Does your box have a proper serial port?
>>
>> No, sorry. I know it'd help with getting logs, and I do have a serial-to-USB cable that I use for another machine, but in this one the port is not routed to outside. I think I can put a header there as the motherboard does have pins, but I'd have to buy one first. In theory, I can do that, but that won't happen within the next few weeks.
>>
>> P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core when using smp boot patchset". Probably, he can reply to this thread and provide more details.
>>

I ran mem/disk versions of 'sudo rtcwake --mode mem -s 60'
on my Rome server, and multiple suspend/resumes succeeded, and
with all CPUs, but then the NETDEV WATCHDOG fired - not sure
if it's related:

...
[ 2751.335882] smpboot: Booting Node 1 Processor 127 APIC 0x7f
[ 2751.340124] ACPI: \_SB_.C07F: Found 2 idle states
[ 2751.392591] CPU127 is up
[ 2751.455650] nvme nvme0: 7/0/0 default/read/poll queues
[ 2751.466112] e1000e 0000:41:00.0 enp65s0: NIC Link is Up 1000 Mbps Full Duplex, Flow Control: Rx/Tx
[ 2751.635573] PM: Cannot find swap device, try swapon -a
[ 2751.641315] PM: Cannot get swap writer
[ 2751.926594] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[ 2751.928527] ata7.00: supports DRM functions and may not be fully accessible
[ 2751.933208] ata7.00: supports DRM functions and may not be fully accessible
[ 2751.937797] ata7.00: configured for UDMA/133
[ 2751.948170] ata7.00: Enabling discard_zeroes_data
[ 2762.428397] PM: hibernation: Basic memory bitmaps freed
[ 2762.429004] OOM killer enabled.
[ 2762.429008] Restarting tasks ... done.
[ 2762.433155] PM: hibernation: hibernation exit
[ 2762.447318] systemd-journald[1387]: Sent WATCHDOG=1 notification.
[ 2830.013372] systemd-journald[1387]: Sent WATCHDOG=1 notification.
[ 2919.099718] systemd-journald[1387]: Sent WATCHDOG=1 notification.
[ 2992.729927] ------------[ cut here ]------------
[ 2992.729965] NETDEV WATCHDOG: enp65s0 (e1000e): transmit queue 0 timed out
[ 2992.730012] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:525 dev_watchdog+0x234/0x250
[ 2992.730032] Modules linked in: intel_rapl_msr intel_rapl_common amd64_edac edac_mce_amd rapl ipmi_si wmi_bmof binfmt_misc kvm_amd kvm nls_iso8859_1 joydev input_leds k10temp mac_hid sch_fq_codel dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua ipmi_devintf ipmi_msghandler msr ramoops reed_solomon efi_pstore ip_tables x_tables autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq raid1 raid0 multipath linear ast i2c_algo_bit drm_shmem_helper drm_kms_helper syscopyarea sysfillrect crct10dif_pclmul crc32_pclmul ghash_clmulni_intel sha512_ssse3 aesni_intel crypto_simd cryptd hid_generic sysimgblt nvme usbhid ahci drm libahci hid nvme_core i2c_piix4 wmi
[ 2992.730711] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.2.0-rc8+ #20
[ 2992.730720] Hardware name: AMD Corporation ETHANOL_X/ETHANOL_X, BIOS RXM1007C 11/08/2021
[ 2992.730727] RIP: 0010:dev_watchdog+0x234/0x250
[ 2992.730732] Code: 00 e9 12 ff ff ff 4c 89 e7 c6 05 6f d5 42 01 01 e8 c1 55 f8 ff 44 89 f1 4c 89 e6 48 c7 c7 28 ff 88 b1 48 89 c2 e8 85 fc 1c 00 <0f> 0b e9 22 ff ff ff 0f 1f 44 00 00 e9 0b ff ff ff 66 66 2e 0f 1f
[ 2992.730740] RSP: 0018:ffffbf6100003e30 EFLAGS: 00010286
[ 2992.730754] RAX: 0000000000000000 RBX: ffff9a8606da8550 RCX: 0000000000000000
[ 2992.730764] RDX: 0000000000000103 RSI: ffffffffb1741bcc RDI: 00000000ffffffff
[ 2992.730770] RBP: ffffbf6100003e50 R08: 0000000000000000 R09: 00000000ffefffff
[ 2992.730775] R10: ffffbf6100003ca8 R11: ffff9b038d9fd668 R12: ffff9a8606da8000
[ 2992.730780] R13: ffff9a8606da8460 R14: 0000000000000000 R15: ffff9ac346fe2480
[ 2992.730787] FS:  0000000000000000(0000) GS:ffff9ac346e00000(0000) knlGS:0000000000000000
[ 2992.730788] systemd-journald[1387]: Compressed data object 717 -> 433 using ZSTD
[ 2992.730797] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2992.730804] CR2: 00007f4c419a0890 CR3: 0008004e47412003 CR4: 0000000000770ef0
[ 2992.730812] PKRU: 55555554
[ 2992.730819] Call Trace:
[ 2992.730826]  <IRQ>
[ 2992.730840]  ? __pfx_dev_watchdog+0x10/0x10
[ 2992.730852]  call_timer_fn+0xac/0x250
[ 2992.730868]  ? __pfx_dev_watchdog+0x10/0x10
[ 2992.730876]  __run_timers+0x22d/0x2e0
[ 2992.730884]  ? seqcount_lockdep_reader_access.constprop.0+0x45/0x60
[ 2992.730893]  ? ktime_get+0x28/0xc0
[ 2992.730903]  ? __pfx_read_tsc+0x10/0x10
[ 2992.730915]  ? ktime_get+0x56/0xc0
[ 2992.730922]  ? sched_clock+0xd/0x20
[ 2992.730930]  ? sched_clock_cpu+0x14/0xd0
[ 2992.730941]  run_timer_softirq+0x33/0x60
[ 2992.730947]  __do_softirq+0x12f/0x380
[ 2992.730960]  __irq_exit_rcu+0xaf/0x120
[ 2992.730970]  irq_exit_rcu+0x12/0x20
[ 2992.730976]  sysvec_apic_timer_interrupt+0xb4/0xd0
[ 2992.730984]  </IRQ>
[ 2992.730989]  <TASK>
[ 2992.730993]  asm_sysvec_apic_timer_interrupt+0x1f/0x30
[ 2992.731004] RIP: 0010:cpuidle_enter_state+0x12d/0x4d0
[ 2992.731015] Code: 00 31 ff e8 e5 c0 45 ff 80 7d d7 00 74 16 9c 58 0f 1f 40 00 f6 c4 02 0f 85 7b 03 00 00 31 ff e8 99 de 4d ff fb 0f 1f 44 00 00 <45> 85 ff 0f 88 d1 01 00 00 49 63 d7 4c 89 f1 48 2b 4d c8 48 8d 04
[ 2992.731018] RSP: 0018:ffffffffb1a03dd8 EFLAGS: 00000246
[ 2992.731024] RAX: ffff9ac346e00000 RBX: ffff9ac4ec3ce400 RCX: 000000000000001f
[ 2992.731027] RDX: 0000000000000000 RSI: ffffffffb1741bcc RDI: ffffffffb17470fa
[ 2992.731035] RBP: ffffffffb1a03e10 R08: 000002b8cc9a617d R09: 000002b8c7e0b83a
[ 2992.731041] R10: 000000000002bc82 R11: ffff9ac346ff2d44 R12: 0000000000000002
[ 2992.731048] R13: ffffffffb1f60bc0 R14: 000002b8cc9a617d R15: 0000000000000002
[ 2992.731059]  ? cpuidle_enter_state+0x10b/0x4d0
[ 2992.731067]  cpuidle_enter+0x32/0x50
[ 2992.731072]  call_cpuidle+0x23/0x50
[ 2992.731080]  do_idle+0x1dc/0x250
[ 2992.731090]  cpu_startup_entry+0x24/0x30
[ 2992.731096]  rest_init+0x108/0x110
[ 2992.731101]  arch_call_rest_init+0x12/0x35
[ 2992.731114]  start_kernel+0x6f3/0x71d
[ 2992.731120]  x86_64_start_reservations+0x28/0x2e
[ 2992.731127]  x86_64_start_kernel+0x80/0x8a
[ 2992.731133]  secondary_startup_64_no_verify+0x186/0x18b
[ 2992.731151]  </TASK>
[ 2992.731158] ---[ end trace 0000000000000000 ]---
[ 2992.731371] e1000e 0000:41:00.0 enp65s0: Reset adapter unexpectedly
[ 2992.870854] systemd-journald[1387]: Successfully sent stream file descriptor to service manager.
[ 2996.338900] e1000e 0000:41:00.0 enp65s0: NIC Link is Up 1000 Mbps Full Duplex, Flow Control: Rx/Tx

Thanks,

Kim

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

[David Woodhouse]

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

On Mon, 2023-02-20 at 17:23 -0600, Kim Phillips wrote:
> On 2/20/23 3:39 PM, David Woodhouse wrote:
> > On 20 February 2023 21:23:38 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
> > > Hello.
> > > 
> > > On 20.02.2023 21:31, David Woodhouse wrote:
> > > > On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
> > > > > On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
> > > > > > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
> > > > > > > 
> > > > > > > I've applied this to the v6.2 kernel, and suspend/resume broke on
> > > > > > > my
> > > > > > > Ryzen 5950X desktop. The machine suspends just fine, but on
> > > > > > > resume
> > > > > > > the screen stays blank, and there's no visible disk I/O.
> > > > > > > 
> > > > > > > Reverting the series brings suspend/resume back to working state.
> > > > > > 
> > > > > > Hm, thanks. What if you add 'no_parallel_bringup' on the command
> > > > > > line?
> > > > > 
> > > > > If the `no_parallel_bringup` param is added, the suspend/resume
> > > > > works.
> > > > 
> > > > Thanks for the testing. Can I ask you to do one further test: apply the
> > > > series only as far as patch 6/8 'x86/smpboot: Support parallel startup
> > > > of secondary CPUs'.
> > > > 
> > > > That will do the new startup asm sequence where each CPU finds its own
> > > > per-cpu data so it *could* work in parallel, but doesn't actually do
> > > > the bringup in parallel yet.
> > > 
> > > With patches 1 to 6 (including) applied and no extra cmdline
> > > params added the resume doesn't work.
> > 
> > Hm. Kim, is there some weirdness with the way AMD CPUs get their
> > APIC ID in CPUID 0x1? Especially after resume?
> 
> Not to my knowledge.  Mario?

Oleksandr, please could you show the output of 'cpuid' after a
successful resume?  I'm particularly looking for this part...


$ sudo cpuid | grep -A1 1/ebx
   miscellaneous (1/ebx):
      process local APIC physical ID = 0x0 (0)
--
   miscellaneous (1/ebx):
      process local APIC physical ID = 0x2 (2)
...


> > Perhaps we turn it off for any AMD CPU that doesn't have X2APIC and CPUID 0xB?
> 
> Perhaps.
> 
> > > > Does your box have a proper serial port?
> > > 
> > > No, sorry. I know it'd help with getting logs, and I do have a serial-to-USB cable that I use for another machine, but in this one the port is not routed to outside. I think I can put a header there as the motherboard does have pins, but I'd have to buy one first. In theory, I can do that, but that won't happen within the next few weeks.
> > > 
> > > P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core when using smp boot patchset". Probably, he can reply to this thread and provide more details.
> > > 
> 
> I ran mem/disk versions of 'sudo rtcwake --mode mem -s 60'
> on my Rome server, and multiple suspend/resumes succeeded, and
> with all CPUs, but then the NETDEV WATCHDOG fired - not sure
> if it's related:

I suspect not.


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

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

[Kim Phillips]

On 2/20/23 5:30 PM, David Woodhouse wrote:
> On Mon, 2023-02-20 at 17:23 -0600, Kim Phillips wrote:
>> On 2/20/23 3:39 PM, David Woodhouse wrote:
>>> On 20 February 2023 21:23:38 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>>>> On 20.02.2023 21:31, David Woodhouse wrote:
>>>>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>>>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>>>>>> On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>>>>>>>
>>>>>>>> I've applied this to the v6.2 kernel, and suspend/resume broke on
>>>>>>>> my
>>>>>>>> Ryzen 5950X desktop. The machine suspends just fine, but on
>>>>>>>> resume
>>>>>>>> the screen stays blank, and there's no visible disk I/O.
>>>>>>>>
>>>>>>>> Reverting the series brings suspend/resume back to working state.
>>>>>>>
>>>>>>> Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>>>>>> line?
>>>>>>
>>>>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>>>>> works.
>>>>>
>>>>> Thanks for the testing. Can I ask you to do one further test: apply the
>>>>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>>>>> of secondary CPUs'.
>>>>>
>>>>> That will do the new startup asm sequence where each CPU finds its own
>>>>> per-cpu data so it *could* work in parallel, but doesn't actually do
>>>>> the bringup in parallel yet.
>>>>
>>>> With patches 1 to 6 (including) applied and no extra cmdline
>>>> params added the resume doesn't work.
>>>
>>> Hm. Kim, is there some weirdness with the way AMD CPUs get their
>>> APIC ID in CPUID 0x1? Especially after resume?
>>
>> Not to my knowledge.  Mario?

I tested v9-up-to-6/8 on a Ryzen 3000 that passed your between-v6 & v7
tree commits (ce7e2d1e046a for the parallel-6.2-rc6-part1 tag
and 17bbd12ee03 for parallel-6.2-rc6), and it, too, fails to resume
v9-up-to-6/8 after suspend.

> Oleksandr, please could you show the output of 'cpuid' after a
> successful resume?  I'm particularly looking for this part...
> 
> 
> $ sudo cpuid | grep -A1 1/ebx
>     miscellaneous (1/ebx):
>        process local APIC physical ID = 0x0 (0)
> --
>     miscellaneous (1/ebx):
>        process local APIC physical ID = 0x2 (2)
> ...

The Ryzens have a different pattern it seems:

$ sudo cpuid | grep -A1 \(1/ebx
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x0 (0)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x1 (1)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x2 (2)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x3 (3)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x4 (4)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x5 (5)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x6 (6)
--
    miscellaneous (1/ebx):
       process local APIC physical ID = 0x7 (7)


I tested the v7 series on Ryzen, it also fails, so
Ryzen users were last known good with those two
aforementioned commits on your tree:

git://git.infradead.org/users/dwmw2/linux.git

Thanks,

Kim

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

[David Woodhouse]

On 21 February 2023 04:20:41 GMT, Kim Phillips <kim.phillips@amd.com> wrote:
>On 2/20/23 5:30 PM, David Woodhouse wrote:
>> On Mon, 2023-02-20 at 17:23 -0600, Kim Phillips wrote:
>>> On 2/20/23 3:39 PM, David Woodhouse wrote:
>>>> On 20 February 2023 21:23:38 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>>>>> On 20.02.2023 21:31, David Woodhouse wrote:
>>>>>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>>>>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>>>>>>> On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>>>>>>>> 
>>>>>>>>> I've applied this to the v6.2 kernel, and suspend/resume broke on
>>>>>>>>> my
>>>>>>>>> Ryzen 5950X desktop. The machine suspends just fine, but on
>>>>>>>>> resume
>>>>>>>>> the screen stays blank, and there's no visible disk I/O.
>>>>>>>>> 
>>>>>>>>> Reverting the series brings suspend/resume back to working state.
>>>>>>>> 
>>>>>>>> Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>>>>>>> line?
>>>>>>> 
>>>>>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>>>>>> works.
>>>>>> 
>>>>>> Thanks for the testing. Can I ask you to do one further test: apply the
>>>>>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>>>>>> of secondary CPUs'.
>>>>>> 
>>>>>> That will do the new startup asm sequence where each CPU finds its own
>>>>>> per-cpu data so it *could* work in parallel, but doesn't actually do
>>>>>> the bringup in parallel yet.
>>>>> 
>>>>> With patches 1 to 6 (including) applied and no extra cmdline
>>>>> params added the resume doesn't work.
>>>> 
>>>> Hm. Kim, is there some weirdness with the way AMD CPUs get their
>>>> APIC ID in CPUID 0x1? Especially after resume?
>>> 
>>> Not to my knowledge.  Mario?
>
>I tested v9-up-to-6/8 on a Ryzen 3000 that passed your between-v6 & v7
>tree commits (ce7e2d1e046a for the parallel-6.2-rc6-part1 tag
>and 17bbd12ee03 for parallel-6.2-rc6), and it, too, fails to resume
>v9-up-to-6/8 after suspend.
>
>> Oleksandr, please could you show the output of 'cpuid' after a
>> successful resume?  I'm particularly looking for this part...
>> 
>> 
>> $ sudo cpuid | grep -A1 1/ebx
>>     miscellaneous (1/ebx):
>>        process local APIC physical ID = 0x0 (0)
>> --
>>     miscellaneous (1/ebx):
>>        process local APIC physical ID = 0x2 (2)
>> ...
>
>The Ryzens have a different pattern it seems:
>
>$ sudo cpuid | grep -A1 \(1/ebx
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x0 (0)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x1 (1)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x2 (2)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x3 (3)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x4 (4)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x5 (5)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x6 (6)
>--
>   miscellaneous (1/ebx):
>      process local APIC physical ID = 0x7 (7)
>
>
>I tested the v7 series on Ryzen, it also fails, so
>Ryzen users were last known good with those two
>aforementioned commits on your tree:
>
>git://git.infradead.org/users/dwmw2/linux.git

That was when it was only using (and validating) CPUID 0xB and never trusting CPUID 0x1, right?

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

[Oleksandr Natalenko]

On 21.02.2023 00:30, David Woodhouse wrote:
> Oleksandr, please could you show the output of 'cpuid' after a
> successful resume?  I'm particularly looking for this part...
> 
> 
> $ sudo cpuid | grep -A1 1/ebx
>    miscellaneous (1/ebx):
>       process local APIC physical ID = 0x0 (0)
> --
>    miscellaneous (1/ebx):
>       process local APIC physical ID = 0x2 (2)
> ...

For me this command doesn't produce any output. Also, no output from the 
command Kim used in response to you. With no `grep` it just dumps a 
table of raw hex data.

It's `msr-tools` 1.3-4 from Arch. Should I run this command on a patched 
kernel booted with `no_parallel_bringup`, or on unpatched kernel (if 
that makes any difference)?

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

On 21 February 2023 07:27:13 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>On 21.02.2023 00:30, David Woodhouse wrote:
>> Oleksandr, please could you show the output of 'cpuid' after a
>> successful resume?  I'm particularly looking for this part...
>> 
>> 
>> $ sudo cpuid | grep -A1 1/ebx
>>    miscellaneous (1/ebx):
>>       process local APIC physical ID = 0x0 (0)
>> --
>>    miscellaneous (1/ebx):
>>       process local APIC physical ID = 0x2 (2)
>> ...
>
>For me this command doesn't produce any output. Also, no output from the command Kim used in response to you. With no `grep` it just dumps a table of raw hex data.

I'll take the hex please.

>It's `msr-tools` 1.3-4 from Arch. Should I run this command on a patched kernel booted with `no_parallel_bringup`, or on unpatched kernel (if that makes any difference)?

Either works as long as it's after a resume that wouldn't have worked. Thanks.

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

[Oleksandr Natalenko]

On 21.02.2023 08:53, David Woodhouse wrote:
> On 21 February 2023 07:27:13 GMT, Oleksandr Natalenko 
> <oleksandr@natalenko.name> wrote:
>> On 21.02.2023 00:30, David Woodhouse wrote:
>>> Oleksandr, please could you show the output of 'cpuid' after a
>>> successful resume?  I'm particularly looking for this part...
>>> 
>>> 
>>> $ sudo cpuid | grep -A1 1/ebx
>>>    miscellaneous (1/ebx):
>>>       process local APIC physical ID = 0x0 (0)
>>> --
>>>    miscellaneous (1/ebx):
>>>       process local APIC physical ID = 0x2 (2)
>>> ...
>> 
>> For me this command doesn't produce any output. Also, no output from 
>> the command Kim used in response to you. With no `grep` it just dumps 
>> a table of raw hex data.
> 
> I'll take the hex please.

Please see here: http://ix.io/4oLm

>> It's `msr-tools` 1.3-4 from Arch. Should I run this command on a 
>> patched kernel booted with `no_parallel_bringup`, or on unpatched 
>> kernel (if that makes any difference)?
> 
> Either works as long as it's after a resume that wouldn't have worked. 
> Thanks.

OK, I'm on an unpatched kernel now.

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Tue, 2023-02-21 at 09:05 +0100, Oleksandr Natalenko wrote:
> 
> Please see here: http://ix.io/4oLm

Was that just for one CPU? Can we have them all please?

The interesting part is the line starting 00000001. We're looking at
the top 8 bits of EBX:

Leaf     Subleaf    EAX            EBX            ECX            EDX            
00000000 00000000:  00000010 ....  68747541 Auth  444d4163 cAMD  69746e65 enti
00000001 00000000:  00a20f12 ....  00200800 .. .  7ef8320b .2.~  178bfbff ....
                                   ↑↑

So the first CPU is CPU0. Could have told you that... what about the others? :)

If anyone can reproduce this with a serial port, can you try this?

From 98ad11d0fb88f081f49f7b1496420dbfbeff8833 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw@amazon.co.uk>
Date: Sat, 4 Feb 2023 15:20:24 +0000
Subject: [PATCH] parallel debug

---
 arch/x86/kernel/head_64.S | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 0e4e53d231db..da7f4d2d9951 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -281,6 +281,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 
 .Lsetup_AP:
 	/* EDX contains the APIC ID of the current CPU */
+#if 1
+	/* Test hack: Print APIC ID and then CPU# when we find it. */
+	mov	%edx, %ecx
+	mov	%edx, %eax
+	addb	$'A', %al
+	mov	$0x3f8, %dx
+	outb    %al, %dx
+	mov	%ecx, %edx
+#endif
 	xorq	%rcx, %rcx
 	leaq	cpuid_to_apicid(%rip), %rbx
 
@@ -302,6 +311,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 
 .Linit_cpu_data:
 	/* Get the per cpu offset for the given CPU# which is in ECX */
+#if 1
+	mov	%rcx, %rax
+	shr	$3, %rax
+	addb	$'a', %al
+
+	mov	$0x3f8, %dx
+	outb    %al, %dx
+#endif
 	leaq	__per_cpu_offset(%rip), %rbx
 	movq	(%rbx,%rcx,8), %rbx
 	/* Save it for GS BASE setup */
-- 
2.39.0



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

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

[Oleksandr Natalenko]

On 21.02.2023 09:17, David Woodhouse wrote:
> On Tue, 2023-02-21 at 09:05 +0100, Oleksandr Natalenko wrote:
>> 
>> Please see here: http://ix.io/4oLm
> 
> Was that just for one CPU? Can we have them all please?
> 
> The interesting part is the line starting 00000001. We're looking at
> the top 8 bits of EBX:
> 
> Leaf     Subleaf    EAX            EBX            ECX            EDX
> 00000000 00000000:  00000010 ....  68747541 Auth  444d4163 cAMD  
> 69746e65 enti
> 00000001 00000000:  00a20f12 ....  00200800 .. .  7ef8320b .2.~  
> 178bfbff ....
>                                    ↑↑
> 
> So the first CPU is CPU0. Could have told you that... what about the 
> others? :)

Right, sorry. Here it is: http://ix.io/4oLq

> If anyone can reproduce this with a serial port, can you try this?
> 
> From 98ad11d0fb88f081f49f7b1496420dbfbeff8833 Mon Sep 17 00:00:00 2001
> From: David Woodhouse <dwmw@amazon.co.uk>
> Date: Sat, 4 Feb 2023 15:20:24 +0000
> Subject: [PATCH] parallel debug
> 
> ---
>  arch/x86/kernel/head_64.S | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 0e4e53d231db..da7f4d2d9951 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -281,6 +281,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
> SYM_L_GLOBAL)
> 
>  .Lsetup_AP:
>  	/* EDX contains the APIC ID of the current CPU */
> +#if 1
> +	/* Test hack: Print APIC ID and then CPU# when we find it. */
> +	mov	%edx, %ecx
> +	mov	%edx, %eax
> +	addb	$'A', %al
> +	mov	$0x3f8, %dx
> +	outb    %al, %dx
> +	mov	%ecx, %edx
> +#endif
>  	xorq	%rcx, %rcx
>  	leaq	cpuid_to_apicid(%rip), %rbx
> 
> @@ -302,6 +311,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
> SYM_L_GLOBAL)
> 
>  .Linit_cpu_data:
>  	/* Get the per cpu offset for the given CPU# which is in ECX */
> +#if 1
> +	mov	%rcx, %rax
> +	shr	$3, %rax
> +	addb	$'a', %al
> +
> +	mov	$0x3f8, %dx
> +	outb    %al, %dx
> +#endif
>  	leaq	__per_cpu_offset(%rip), %rbx
>  	movq	(%rbx,%rcx,8), %rbx
>  	/* Save it for GS BASE setup */

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Tue, 2023-02-21 at 09:25 +0100, Oleksandr Natalenko wrote:
> 
> 
> Right, sorry. Here it is: http://ix.io/4oLq

$ echo `grep ^00000001 4oLq  | cut -c36-37`
00 02 04 06 08 0a 0c 0e 10 12 14 16 18 1a 1c 1e 01 03 05 07 09 0b 0d 0f
11 13 15 17 19 1b 1d 1f

Well they look sane enough. All even APIC IDs and then all the odd ones
is a topology that isn't massively surprising.

Does it match what you get *before* suspend/resume?

Obviously we could stick our fingers in our ears and go "la la la" and
just disable it for non-X2APIC, for AMD without X2APIC, or perhaps
disable it on *resume* but still use it at boot. But I'd really like to
understand what's going on and not do voodoo. Thanks for helping!

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

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

[Oleksandr Natalenko]

On 21.02.2023 09:35, David Woodhouse wrote:
> On Tue, 2023-02-21 at 09:25 +0100, Oleksandr Natalenko wrote:
>> 
>> 
>> Right, sorry. Here it is: http://ix.io/4oLq
> 
> $ echo `grep ^00000001 4oLq  | cut -c36-37`
> 00 02 04 06 08 0a 0c 0e 10 12 14 16 18 1a 1c 1e 01 03 05 07 09 0b 0d 0f
> 11 13 15 17 19 1b 1d 1f
> 
> Well they look sane enough. All even APIC IDs and then all the odd ones
> is a topology that isn't massively surprising.
> 
> Does it match what you get *before* suspend/resume?

Yes, the output is completely the same after a fresh boot and after a 
suspend/resume cycle.

> Obviously we could stick our fingers in our ears and go "la la la" and
> just disable it for non-X2APIC, for AMD without X2APIC, or perhaps
> disable it on *resume* but still use it at boot. But I'd really like to
> understand what's going on and not do voodoo. Thanks for helping!

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Tue, 2023-02-21 at 09:44 +0100, Oleksandr Natalenko wrote:
> On 21.02.2023 09:35, David Woodhouse wrote:
> > On Tue, 2023-02-21 at 09:25 +0100, Oleksandr Natalenko wrote:
> > > 
> > > 
> > > Right, sorry. Here it is: http://ix.io/4oLq
> > 
> > $ echo `grep ^00000001 4oLq  | cut -c36-37`
> > 00 02 04 06 08 0a 0c 0e 10 12 14 16 18 1a 1c 1e 01 03 05 07 09 0b 0d 0f
> > 11 13 15 17 19 1b 1d 1f
> > 
> > Well they look sane enough. All even APIC IDs and then all the odd ones
> > is a topology that isn't massively surprising.
> > 
> > Does it match what you get *before* suspend/resume?
> 
> Yes, the output is completely the same after a fresh boot and after a
> suspend/resume cycle.
> 
> > Obviously we could stick our fingers in our ears and go "la la la" and
> > just disable it for non-X2APIC, for AMD without X2APIC, or perhaps
> > disable it on *resume* but still use it at boot. But I'd really like to
> > understand what's going on and not do voodoo. Thanks for helping!
> 

Hm...

Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set 

    initial_gs = per_cpu_offset(smp_processor_id()) ? 

Would it not be CPU#0 that comes back up, and should it not get
per_cpu_offset(0) ? 

Or maybe we should just set up smpboot_control for the CPU to find its
own stuff, *even* on waking. Since the structures are already set up,
it isn't like a clean boot.

If you let it boot in parallel mode, what if you just *remove* the line
that sets smpboot_control=0 ? 


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

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

[Oleksandr Natalenko]

On 21.02.2023 10:06, David Woodhouse wrote:
> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set
> 
>     initial_gs = per_cpu_offset(smp_processor_id()) ?
> 
> Would it not be CPU#0 that comes back up, and should it not get
> per_cpu_offset(0) ?

Wanna me try `initial_gs = per_cpu_offset(0);` too?

> Or maybe we should just set up smpboot_control for the CPU to find its
> own stuff, *even* on waking. Since the structures are already set up,
> it isn't like a clean boot.
> 
> If you let it boot in parallel mode, what if you just *remove* the line
> that sets smpboot_control=0 ?

If the `smpboot_control = 0;` line in 
arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() is commented 
out, and the system is booted in parallel mode, then suspend/resume 
works.

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>On 21.02.2023 10:06, David Woodhouse wrote:
>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set
>> 
>>     initial_gs = per_cpu_offset(smp_processor_id()) ?
>> 
>> Would it not be CPU#0 that comes back up, and should it not get
>> per_cpu_offset(0) ?
>
>Wanna me try `initial_gs = per_cpu_offset(0);` too?

Hm, yes please. There's another one to make zero on the next line up, I think?

>> Or maybe we should just set up smpboot_control for the CPU to find its
>> own stuff, *even* on waking. Since the structures are already set up,
>> it isn't like a clean boot.
>> 
>> If you let it boot in parallel mode, what if you just *remove* the line
>> that sets smpboot_control=0 ?
>
>If the `smpboot_control = 0;` line in arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() is commented out, and the system is booted in parallel mode, then suspend/resume works.

Well that's entertaining. Now, can we come up with any theory which doesn't leave us wondering why it ever worked in the first place...?

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

[Usama Arif]

On 21/02/2023 10:27, David Woodhouse wrote:
> 
> 
> On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>> On 21.02.2023 10:06, David Woodhouse wrote:
>>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set
>>>
>>>      initial_gs = per_cpu_offset(smp_processor_id()) ?
>>>
>>> Would it not be CPU#0 that comes back up, and should it not get
>>> per_cpu_offset(0) ?
>>
>> Wanna me try `initial_gs = per_cpu_offset(0);` too?
> 

I think it might be smp_processor_id() and not 0 incase CPU0 was offline 
at the point the system was suspended?

> Hm, yes please. There's another one to make zero on the next line up, I think?
> 
>>> Or maybe we should just set up smpboot_control for the CPU to find its
>>> own stuff, *even* on waking. Since the structures are already set up,
>>> it isn't like a clean boot.
>>>
>>> If you let it boot in parallel mode, what if you just *remove* the line
>>> that sets smpboot_control=0 ?
>>
>> If the `smpboot_control = 0;` line in arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() is commented out, and the system is booted in parallel mode, then suspend/resume works.
> 
> Well that's entertaining. Now, can we come up with any theory which doesn't leave us wondering why it ever worked in the first place...?

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

[Oleksandr Natalenko]

On 21.02.2023 11:47, Usama Arif wrote:
> On 21/02/2023 10:27, David Woodhouse wrote:
>> 
>> On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko 
>> <oleksandr@natalenko.name> wrote:
>>> On 21.02.2023 10:06, David Woodhouse wrote:
>>>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() 
>>>> set
>>>> 
>>>>      initial_gs = per_cpu_offset(smp_processor_id()) ?
>>>> 
>>>> Would it not be CPU#0 that comes back up, and should it not get
>>>> per_cpu_offset(0) ?
>>> 
>>> Wanna me try `initial_gs = per_cpu_offset(0);` too?
>> 
> 
> I think it might be smp_processor_id() and not 0 incase CPU0 was 
> offline at the point the system was suspended?

Is it even possible for CPU 0 to be offline, at least on x86?

-- 
   Oleksandr Natalenko (post-factum)

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

[Usama Arif]

On 21/02/2023 11:42, Oleksandr Natalenko wrote:
> On 21.02.2023 11:47, Usama Arif wrote:
>> On 21/02/2023 10:27, David Woodhouse wrote:
>>>
>>> On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko 
>>> <oleksandr@natalenko.name> wrote:
>>>> On 21.02.2023 10:06, David Woodhouse wrote:
>>>>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set
>>>>>
>>>>>      initial_gs = per_cpu_offset(smp_processor_id()) ?
>>>>>
>>>>> Would it not be CPU#0 that comes back up, and should it not get
>>>>> per_cpu_offset(0) ?
>>>>
>>>> Wanna me try `initial_gs = per_cpu_offset(0);` too?
>>>
>>
>> I think it might be smp_processor_id() and not 0 incase CPU0 was 
>> offline at the point the system was suspended?
> 
> Is it even possible for CPU 0 to be offline, at least on x86?
> 

It is possible on x86 (using BOOTPARAM_HOTPLUG_CPU0), but I just read 
the Kconfig option and it says:

"resume from hibernate or suspend always starts from CPU0.
So hibernate and suspend are prevented if CPU0 is offline."

so I guess switching to 0 should be ok.

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

[David Woodhouse]

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

On Tue, 2023-02-21 at 11:54 +0000, Usama Arif wrote:
> 
> 
> On 21/02/2023 11:42, Oleksandr Natalenko wrote:
> > On 21.02.2023 11:47, Usama Arif wrote:
> > > On 21/02/2023 10:27, David Woodhouse wrote:
> > > > 
> > > > On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko 
> > > > <oleksandr@natalenko.name> wrote:
> > > > > On 21.02.2023 10:06, David Woodhouse wrote:
> > > > > > Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set
> > > > > > 
> > > > > >      initial_gs = per_cpu_offset(smp_processor_id()) ?
> > > > > > 
> > > > > > Would it not be CPU#0 that comes back up, and should it not get
> > > > > > per_cpu_offset(0) ?
> > > > > 
> > > > > Wanna me try `initial_gs = per_cpu_offset(0);` too?
> > > > 
> > > 
> > > I think it might be smp_processor_id() and not 0 incase CPU0 was 
> > > offline at the point the system was suspended?
> > 
> > Is it even possible for CPU 0 to be offline, at least on x86?
> > 
> 
> It is possible on x86 (using BOOTPARAM_HOTPLUG_CPU0), but I just read
> the Kconfig option and it says:
> 
> "resume from hibernate or suspend always starts from CPU0.
> So hibernate and suspend are prevented if CPU0 is offline."
> 
> so I guess switching to 0 should be ok.

The interesting question is who ends up using whose stack?

Leaving smpboot_control alone (for the parallel case only; we can't
just leave it with the APIC ID of the last CPU started in the serial
case) will make each CPU find the same CPU# and stack it used to have,
from its own APIC ID and the cpuid_to_apicid[] table.

I don't really grok what's happening in the non-parallel case. We leave
behind the stack for the CPU that happens to be running the suspend
function. And then whichever CPU comes back, it'll get *that* stack.

I don't understand why the parallel bringup changes this. Does the
cpuid to apicid mapping *change* on resume, if the suspending and
resuming CPU are different? Do they swap stacks and CPU# somehow?

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

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

[Oleksandr Natalenko]

On 21.02.2023 11:27, David Woodhouse wrote:
> On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko 
> <oleksandr@natalenko.name> wrote:
>> On 21.02.2023 10:06, David Woodhouse wrote:
>>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() 
>>> set
>>> 
>>>     initial_gs = per_cpu_offset(smp_processor_id()) ?
>>> 
>>> Would it not be CPU#0 that comes back up, and should it not get
>>> per_cpu_offset(0) ?
>> 
>> Wanna me try `initial_gs = per_cpu_offset(0);` too?
> 
> Hm, yes please. There's another one to make zero on the next line up, I 
> think?

So,

```
early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
initial_gs = per_cpu_offset(0);
```

?

Should I leave `smpboot_control = 0;` commented out, or I should 
uncomment it back?

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

On 21 February 2023 11:46:04 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>On 21.02.2023 11:27, David Woodhouse wrote:
>> On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>>> On 21.02.2023 10:06, David Woodhouse wrote:
>>>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() set
>>>> 
>>>>     initial_gs = per_cpu_offset(smp_processor_id()) ?
>>>> 
>>>> Would it not be CPU#0 that comes back up, and should it not get
>>>> per_cpu_offset(0) ?
>>> 
>>> Wanna me try `initial_gs = per_cpu_offset(0);` too?
>> 
>> Hm, yes please. There's another one to make zero on the next line up, I think?
>
>So,
>
>```
>early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
>initial_gs = per_cpu_offset(0);
>```
>
>?
>
>Should I leave `smpboot_control = 0;` commented out, or I should uncomment it back?

Put it back, else those things don't matter. Thanks.

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

[Oleksandr Natalenko]

On 21.02.2023 12:49, David Woodhouse wrote:
> On 21 February 2023 11:46:04 GMT, Oleksandr Natalenko 
> <oleksandr@natalenko.name> wrote:
>> On 21.02.2023 11:27, David Woodhouse wrote:
>>> On 21 February 2023 09:49:51 GMT, Oleksandr Natalenko 
>>> <oleksandr@natalenko.name> wrote:
>>>> On 21.02.2023 10:06, David Woodhouse wrote:
>>>>> Why does arch/x86/kernel/acpi/sleep.c::x86_acpi_suspend_lowlevel() 
>>>>> set
>>>>> 
>>>>>     initial_gs = per_cpu_offset(smp_processor_id()) ?
>>>>> 
>>>>> Would it not be CPU#0 that comes back up, and should it not get
>>>>> per_cpu_offset(0) ?
>>>> 
>>>> Wanna me try `initial_gs = per_cpu_offset(0);` too?
>>> 
>>> Hm, yes please. There's another one to make zero on the next line up, 
>>> I think?
>> 
>> So,
>> 
>> ```
>> early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
>> initial_gs = per_cpu_offset(0);
>> ```
>> 
>> ?
>> 
>> Should I leave `smpboot_control = 0;` commented out, or I should 
>> uncomment it back?
> 
> Put it back, else those things don't matter. Thanks.

With this in place:

```
	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
	initial_gs = per_cpu_offset(0);
	smpboot_control = 0;
```

the resume does not work.

-- 
   Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Tue, 2023-02-21 at 13:14 +0100, Oleksandr Natalenko wrote:
> 
> With this in place:
> 
> ```
>         early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
>         initial_gs = per_cpu_offset(0);
>         smpboot_control = 0;
> ```
> 
> the resume does not work.

Yeah, I think it's always running on CPU0 after the other CPUs are
taken down anyway.

We definitely *do* need to clear smpboot_control because we absolutely
want it using the temp_stack we explicitly put into initial_stack, not
finding its own idle thread.

The problem was that it was never being restored to STARTUP_SECONDARY
in the parallel modes, because that's a one-time setup in
native_smp_prepare_cpus(). So we can just restore it in
arch_thaw_secondary_cpus_begin() too, by working this into patch 6 of
the series.

(Usama, I think my tree is fairly out of date now so I'll let you do
that? Thanks!).

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 50621793671d..3db77a257ae2 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1571,6 +1571,17 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 
 void arch_thaw_secondary_cpus_begin(void)
 {
+	/*
+	 * On suspend, smpboot_control will have been zeroed to allow the
+	 * boot CPU to use explicitly passed values including a temporary
+	 * stack. Since native_smp_prepare_cpus() won't be called again,
+	 * restore the appropriate value for the parallel startup modes.
+	 */
+	if (do_parallel_bringup) {
+		smpboot_control = STARTUP_SECONDARY |
+			(x2apic_mode ? STARTUP_APICID_CPUID_0B : STARTUP_APICID_CPUID_01);
+	}
+
 	set_cache_aps_delayed_init(true);
 }
 


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

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

[Usama Arif]

On 21/02/2023 19:10, David Woodhouse wrote:
> On Tue, 2023-02-21 at 13:14 +0100, Oleksandr Natalenko wrote:
>>
>> With this in place:
>>
>> ```
>>          early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
>>          initial_gs = per_cpu_offset(0);
>>          smpboot_control = 0;
>> ```
>>
>> the resume does not work.
> 
> Yeah, I think it's always running on CPU0 after the other CPUs are
> taken down anyway.
> 
> We definitely *do* need to clear smpboot_control because we absolutely
> want it using the temp_stack we explicitly put into initial_stack, not
> finding its own idle thread.
> 
> The problem was that it was never being restored to STARTUP_SECONDARY
> in the parallel modes, because that's a one-time setup in
> native_smp_prepare_cpus(). So we can just restore it in
> arch_thaw_secondary_cpus_begin() too, by working this into patch 6 of
> the series.
> 
> (Usama, I think my tree is fairly out of date now so I'll let you do
> that? Thanks!).
>Sounds good! Will send out the next revision with below diff, checkpatch 
fixes and rebased to 6.2 (testing it now). The below fix looks good! 
Oleksandr, would you mind testing out suspend/resume with the below diff 
on your AMD machine just to make sure it fixes the issue before I send 
out the next revision with it. Thanks!

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 50621793671d..3db77a257ae2 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1571,6 +1571,17 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
>   
>   void arch_thaw_secondary_cpus_begin(void)
>   {
> +	/*
> +	 * On suspend, smpboot_control will have been zeroed to allow the
> +	 * boot CPU to use explicitly passed values including a temporary
> +	 * stack. Since native_smp_prepare_cpus() won't be called again,
> +	 * restore the appropriate value for the parallel startup modes.
> +	 */
> +	if (do_parallel_bringup) {
> +		smpboot_control = STARTUP_SECONDARY |
> +			(x2apic_mode ? STARTUP_APICID_CPUID_0B : STARTUP_APICID_CPUID_01);
> +	}
> +
>   	set_cache_aps_delayed_init(true);
>   }
>   
> 

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

[Oleksandr Natalenko]

On 21.02.2023 21:04, Usama Arif wrote:
> On 21/02/2023 19:10, David Woodhouse wrote:
>> On Tue, 2023-02-21 at 13:14 +0100, Oleksandr Natalenko wrote:
>>> 
>>> With this in place:
>>> 
>>> ```
>>>          early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
>>>          initial_gs = per_cpu_offset(0);
>>>          smpboot_control = 0;
>>> ```
>>> 
>>> the resume does not work.
>> 
>> Yeah, I think it's always running on CPU0 after the other CPUs are
>> taken down anyway.
>> 
>> We definitely *do* need to clear smpboot_control because we absolutely
>> want it using the temp_stack we explicitly put into initial_stack, not
>> finding its own idle thread.
>> 
>> The problem was that it was never being restored to STARTUP_SECONDARY
>> in the parallel modes, because that's a one-time setup in
>> native_smp_prepare_cpus(). So we can just restore it in
>> arch_thaw_secondary_cpus_begin() too, by working this into patch 6 of
>> the series.
>> 
>> (Usama, I think my tree is fairly out of date now so I'll let you do
>> that? Thanks!).
> 
> Sounds good! Will send out the next revision with below diff, 
> checkpatch
> fixes and rebased to 6.2 (testing it now). The below fix looks good! 
> Oleksandr, would you mind testing out suspend/resume with the below 
> diff on your AMD machine just to make sure it fixes the issue before I 
> send out the next revision with it. Thanks!

Right, so I applied the whole series + this fix, and the suspend/resume 
works. Thanks!

Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>

There's another open question pending though: why would this series 
cause booting up one CPU only on an older AMD CPU. But I'd expect 
Piotr's fellow to chime in occasionally.

>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 50621793671d..3db77a257ae2 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1571,6 +1571,17 @@ void __init native_smp_prepare_cpus(unsigned 
>> int max_cpus)
>>     void arch_thaw_secondary_cpus_begin(void)
>>   {
>> +	/*
>> +	 * On suspend, smpboot_control will have been zeroed to allow the
>> +	 * boot CPU to use explicitly passed values including a temporary
>> +	 * stack. Since native_smp_prepare_cpus() won't be called again,
>> +	 * restore the appropriate value for the parallel startup modes.
>> +	 */
>> +	if (do_parallel_bringup) {
>> +		smpboot_control = STARTUP_SECONDARY |
>> +			(x2apic_mode ? STARTUP_APICID_CPUID_0B : STARTUP_APICID_CPUID_01);
>> +	}
>> +
>>   	set_cache_aps_delayed_init(true);
>>   }
>> 

-- 
   Oleksandr Natalenko (post-factum)

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

[Thomas Gleixner]

David!

On Tue, Feb 21 2023 at 19:10, David Woodhouse wrote:
> On Tue, 2023-02-21 at 13:14 +0100, Oleksandr Natalenko wrote:
> (Usama, I think my tree is fairly out of date now so I'll let you do
> that? Thanks!).
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 50621793671d..3db77a257ae2 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1571,6 +1571,17 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
>  
>  void arch_thaw_secondary_cpus_begin(void)
>  {
> +	/*
> +	 * On suspend, smpboot_control will have been zeroed to allow the
> +	 * boot CPU to use explicitly passed values including a temporary
> +	 * stack. Since native_smp_prepare_cpus() won't be called again,
> +	 * restore the appropriate value for the parallel startup modes.
> +	 */
> +	if (do_parallel_bringup) {
> +		smpboot_control = STARTUP_SECONDARY |
> +			(x2apic_mode ? STARTUP_APICID_CPUID_0B : STARTUP_APICID_CPUID_01);
> +	}

My bad taste sensor reports: "Out of effective range"

Why on earth can't you fix the wreckage exactly where it happens,
i.e. in x86_acpi_suspend_lowlevel() ?

--- 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"
@@ -57,6 +58,7 @@ asmlinkage acpi_status __visible x86_acp
  */
 int x86_acpi_suspend_lowlevel(void)
 {
+	unsigned int __maybe_unused saved_smpboot_ctrl;
 	struct wakeup_header *header =
 		(struct wakeup_header *) __va(real_mode_header->wakeup_header);
 
@@ -115,7 +117,8 @@ int x86_acpi_suspend_lowlevel(void)
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_rw(smp_processor_id());
 	initial_gs = per_cpu_offset(smp_processor_id());
-	smpboot_control = 0;
+	/* Force the startup into boot mode */
+	saved_smpboot_ctrl = xchg(&smpboot_control, 0);
 #endif
 	initial_code = (unsigned long)wakeup_long64;
        saved_magic = 0x123456789abcdef0L;
@@ -128,6 +131,9 @@ int x86_acpi_suspend_lowlevel(void)
 	pause_graph_tracing();
 	do_suspend_lowlevel();
 	unpause_graph_tracing();
+
+	if (IS_ENABLED(CONFIG_64BIT) && IS_ENABLED(CONFIG_SMP))
+		smpboot_control = saved_smpboot_ctrl;
 	return 0;
 }
 

That's too bloody obvious, too self explaining, not enough duplicated
code and does not need any fixups when the smpboot_control bits are
changed later, right?

Thanks,

        tglx

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

[David Woodhouse]

On 21 February 2023 21:41:32 GMT, Thomas Gleixner <tglx@linutronix.de> wrote:
>David!
>
>On Tue, Feb 21 2023 at 19:10, David Woodhouse wrote:
>> On Tue, 2023-02-21 at 13:14 +0100, Oleksandr Natalenko wrote:
>> (Usama, I think my tree is fairly out of date now so I'll let you do
>> that? Thanks!).
>>
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 50621793671d..3db77a257ae2 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1571,6 +1571,17 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
>>  
>>  void arch_thaw_secondary_cpus_begin(void)
>>  {
>> +	/*
>> +	 * On suspend, smpboot_control will have been zeroed to allow the
>> +	 * boot CPU to use explicitly passed values including a temporary
>> +	 * stack. Since native_smp_prepare_cpus() won't be called again,
>> +	 * restore the appropriate value for the parallel startup modes.
>> +	 */
>> +	if (do_parallel_bringup) {
>> +		smpboot_control = STARTUP_SECONDARY |
>> +			(x2apic_mode ? STARTUP_APICID_CPUID_0B : STARTUP_APICID_CPUID_01);
>> +	}
>
>My bad taste sensor reports: "Out of effective range"
>
>Why on earth can't you fix the wreckage exactly where it happens,
>i.e. in x86_acpi_suspend_lowlevel() ?

Er, that was my first instinct but for some reason I concluded that I couldn't put it back there, and it has to be done later because this was just a function called on the way down to suspend. Wrongly, it seems. :)

>--- 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"
>@@ -57,6 +58,7 @@ asmlinkage acpi_status __visible x86_acp
>  */
> int x86_acpi_suspend_lowlevel(void)
> {
>+	unsigned int __maybe_unused saved_smpboot_ctrl;
> 	struct wakeup_header *header =
> 		(struct wakeup_header *) __va(real_mode_header->wakeup_header);
> 
>@@ -115,7 +117,8 @@ int x86_acpi_suspend_lowlevel(void)
> 	early_gdt_descr.address =
> 			(unsigned long)get_cpu_gdt_rw(smp_processor_id());
> 	initial_gs = per_cpu_offset(smp_processor_id());
>-	smpboot_control = 0;
>+	/* Force the startup into boot mode */
>+	saved_smpboot_ctrl = xchg(&smpboot_control, 0);
> #endif
> 	initial_code = (unsigned long)wakeup_long64;
>        saved_magic = 0x123456789abcdef0L;
>@@ -128,6 +131,9 @@ int x86_acpi_suspend_lowlevel(void)
> 	pause_graph_tracing();
> 	do_suspend_lowlevel();
> 	unpause_graph_tracing();
>+
>+	if (IS_ENABLED(CONFIG_64BIT) && IS_ENABLED(CONFIG_SMP))
>+		smpboot_control = saved_smpboot_ctrl;
> 	return 0;
> }
> 
>
>That's too bloody obvious, too self explaining, not enough duplicated
>code and does not need any fixups when the smpboot_control bits are
>changed later, right?
>
>Thanks,
>
>        tglx

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

[David Woodhouse]

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

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

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

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


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

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

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



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

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

[Usama Arif]

On 21/02/2023 23:18, David Woodhouse wrote:
> On Tue, 2023-02-21 at 22:41 +0100, Thomas Gleixner wrote:
>>
>> @@ -57,6 +58,7 @@ asmlinkage acpi_status __visible x86_acp
>>    */
>>   int x86_acpi_suspend_lowlevel(void)
>>   {
>> +       unsigned int __maybe_unused saved_smpboot_ctrl;
>>          struct wakeup_header *header =
>>                  (struct wakeup_header *) __va(real_mode_header->wakeup_header);
>>   
>> @@ -115,7 +117,8 @@ int x86_acpi_suspend_lowlevel(void)
>>          early_gdt_descr.address =
>>                          (unsigned long)get_cpu_gdt_rw(smp_processor_id());
>>          initial_gs = per_cpu_offset(smp_processor_id());
>> -       smpboot_control = 0;
>> +       /* Force the startup into boot mode */
>> +       saved_smpboot_ctrl = xchg(&smpboot_control, 0);
>>   #endif
>>          initial_code = (unsigned long)wakeup_long64;
>>          saved_magic = 0x123456789abcdef0L;
>> @@ -128,6 +131,9 @@ int x86_acpi_suspend_lowlevel(void)
>>          pause_graph_tracing();
>>          do_suspend_lowlevel();
>>          unpause_graph_tracing();
>> +
>> +       if (IS_ENABLED(CONFIG_64BIT) && IS_ENABLED(CONFIG_SMP))
>> +               smpboot_control = saved_smpboot_ctrl;
>>          return 0;
>>   }
>>   
> 
> But wait, why is this giving it a dedicated temp_stack anyway? Why
> can't it use that CPU's idle thread stack like we usually do? I already
> made idle_thread_get() accessible from here. So we could do this...
> 
> @@ -111,14 +112,16 @@ int x86_acpi_suspend_lowlevel(void)
>          saved_magic = 0x12345678;
>   #else /* CONFIG_64BIT */
>   #ifdef CONFIG_SMP
> -       initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
> -       early_gdt_descr.address =
> -                       (unsigned long)get_cpu_gdt_rw(smp_processor_id());
> -       initial_gs = per_cpu_offset(smp_processor_id());
> -       smpboot_control = 0;
> +       if (!(smpboot_control & STARTUP_PARALLEL_MASK)) {
> +               unsigned int cpu = smp_processor_id();
> +               initial_stack = (unsigned long)idle_thread_get(cpu)->thread.sp;
> +               early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> +               initial_gs = per_cpu_offset(cpu);
> +               smpboot_control = 0;
> +       }
>   #endif
>          initial_code = (unsigned long)wakeup_long64;
> 
> 
> But that's a whole bunch of pointless, because it can be even further
> simplified to just let the find its own crap like the secondaries do,
> except in the 'OMG CPUID won't tell me' case where it has to be told:
> 
> So how about we just do something more like this. I'd *quite* like to
> put the actual handling of smpboot_control into a function we call in
> smpboot.c. and that whole x86_acpi_suspend_lowlevel() function wants
> all its horrid 64bit/smp ifdefs fixed up (and is there any reason
> there's a generic part saving CR0 and IA32_MISC_ENABLE right in the
> middle of some !CONFIG_64BIT parts? I don't see ordering constraints
> there). But this should work, I think:
> 
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 33c0d5fd8af6..72b9375fec7c 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -208,4 +208,6 @@ extern unsigned int smpboot_control;
>   #define STARTUP_APICID_CPUID_0B	0x40000000
>   #define STARTUP_APICID_CPUID_01	0x20000000
>   
> +#define STARTUP_PARALLEL_MASK	0x60000000
> +

Probably could define STARTUP_PARALLEL_MASK as STARTUP_APICID_CPUID_0B | 
STARTUP_APICID_CPUID_01 instead? otherwise if its a separate bit, it 
needs to be set in native_smp_prepare_cpus as well for this to work?

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

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

[David Woodhouse]

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

On Wed, 2023-02-22 at 00:00 +0000, Usama Arif wrote:
> 
> > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> > index 33c0d5fd8af6..72b9375fec7c 100644
> > --- a/arch/x86/include/asm/smp.h
> > +++ b/arch/x86/include/asm/smp.h
> > @@ -208,4 +208,6 @@ extern unsigned int smpboot_control;
> >    #define STARTUP_APICID_CPUID_0B      0x40000000
> >    #define STARTUP_APICID_CPUID_01      0x20000000
> >    
> > +#define STARTUP_PARALLEL_MASK          0x60000000
> > +
> 
> Probably could define STARTUP_PARALLEL_MASK as STARTUP_APICID_CPUID_0B | 
> STARTUP_APICID_CPUID_01 instead? otherwise if its a separate bit, it 
> needs to be set in native_smp_prepare_cpus as well for this to work?

It is CPUID_0B | CPUID_01, unless I was being really stupid last night. 
Sips coffee... yep, 6 = 4 | 2. I could have made that more obvious with
a bit more typing, I suppose.

However, I don't think this approach is correct. The idle thread stacks
for the other CPUs are *unused* because those CPUs are offline. So we
can use them with impunity, and when those CPUs come back online
they'll call into cpu_startup_entry(CPUHP_AP_ONLINE_IDLE) using the
correct stack.

But the BSP/CPU0 is different. It hasn't actually been taken offline,
and its idle thread context is still in cpu_startup_entry(CPUHP_ONLINE)
which got called from rest_init().

In testing I probably got away with it because we're only using the
*top* of the stack, don't use anything of the red zone, and thus don't
actually bother the true idle thread which is never going to return.
But I don't think it's correct; we really ought to have that temp_stack
unless we're going to refactor the wakeup_64 code to *become* the idle
thread just as startup_secondary() does, and *schedule* to the context
that was saved in the suspend code.

That might be an interesting cleanup, and let us use the normal
__switch_to() to save and restore a bunch of context which is currently
done by hand.

But not today.

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

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

[Thomas Gleixner]

On Wed, Feb 22 2023 at 08:19, David Woodhouse wrote:
> But the BSP/CPU0 is different. It hasn't actually been taken offline,
> and its idle thread context is still in cpu_startup_entry(CPUHP_ONLINE)
> which got called from rest_init().
>
> In testing I probably got away with it because we're only using the
> *top* of the stack, don't use anything of the red zone, and thus don't
> actually bother the true idle thread which is never going to return.

:)

> But I don't think it's correct; we really ought to have that temp_stack
> unless we're going to refactor the wakeup_64 code to *become* the idle
> thread just as startup_secondary() does, and *schedule* to the context
> that was saved in the suspend code.

And thereby messing up the scheduler state...

Thanks,

        tglx

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

[David Woodhouse]

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

On Wed, 2023-02-22 at 10:46 +0100, Thomas Gleixner wrote:
> On Wed, Feb 22 2023 at 08:19, David Woodhouse wrote:
> > But the BSP/CPU0 is different. It hasn't actually been taken offline,
> > and its idle thread context is still in cpu_startup_entry(CPUHP_ONLINE)
> > which got called from rest_init().
> > 
> > In testing I probably got away with it because we're only using the
> > *top* of the stack, don't use anything of the red zone, and thus don't
> > actually bother the true idle thread which is never going to return.
> 
> :)
> 
> > But I don't think it's correct; we really ought to have that temp_stack
> > unless we're going to refactor the wakeup_64 code to *become* the idle
> > thread just as startup_secondary() does, and *schedule* to the context
> > that was saved in the suspend code.
> 
> And thereby messing up the scheduler state...

Indeed. Which is probably fixable but also probably more of a wart in
the scheduler code, than it's worth for the negligible cleanup in the
suspend code.

Hence "not today". Which is code for "not ever". But don't tell my
children that.

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

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

[Thomas Gleixner]

On Tue, Feb 21 2023 at 23:18, David Woodhouse wrote:
> On Tue, 2023-02-21 at 22:41 +0100, Thomas Gleixner wrote:
>> +
>> +       if (IS_ENABLED(CONFIG_64BIT) && IS_ENABLED(CONFIG_SMP))
>> +               smpboot_control = saved_smpboot_ctrl;
>>         return 0;
>>  }
>>  
>
> But wait, why is this giving it a dedicated temp_stack anyway? Why
> can't it use that CPU's idle thread stack like we usually do? I already
> made idle_thread_get() accessible from here. So we could do this...

Because this very CPU is still online and from the kernels POV is does
not go offline. It goes into the firmware blackhole and comes back
magically through the startup code.

That means this very CPUs indle thread stack is in use and the resume
path will scribble over it. Maybe you won't notice because it only
clobbers top of stack which is never used again because the idle thread
does not return. But correct is something different.

Thanks,

        tglx

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

[Piotr Gorski]

Hello.

In my case, admittedly, the error does not occur, while I have 
information from a friend that AMD FX 6300 only shows 1 core when using 
smp boot patchset, so mentioned Oleksandr. Probably soon friend will 
join the discussion and will be able to provide more information.

W dniu 20.02.2023 o 22:23, Oleksandr Natalenko pisze:
> Hello.
>
> On 20.02.2023 21:31, David Woodhouse wrote:
>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>> > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>> > >
>>> > > I've applied this to the v6.2 kernel, and suspend/resume broke on
>>> > > my
>>> > > Ryzen 5950X desktop. The machine suspends just fine, but on
>>> > > resume
>>> > > the screen stays blank, and there's no visible disk I/O.
>>> > >
>>> > > Reverting the series brings suspend/resume back to working state.
>>> >
>>> > Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>> > line?
>>>
>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>> works.
>>
>> Thanks for the testing. Can I ask you to do one further test: apply the
>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>> of secondary CPUs'.
>>
>> That will do the new startup asm sequence where each CPU finds its own
>> per-cpu data so it *could* work in parallel, but doesn't actually do
>> the bringup in parallel yet.
>
> With patches 1 to 6 (including) applied and no extra cmdline params 
> added the resume doesn't work.
>
>> Does your box have a proper serial port?
>
> No, sorry. I know it'd help with getting logs, and I do have a 
> serial-to-USB cable that I use for another machine, but in this one 
> the port is not routed to outside. I think I can put a header there as 
> the motherboard does have pins, but I'd have to buy one first. In 
> theory, I can do that, but that won't happen within the next few weeks.
>
> P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS 
> can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core 
> when using smp boot patchset". Probably, he can reply to this thread 
> and provide more details.
>

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

[Usama Arif]

On 20/02/2023 21:23, Oleksandr Natalenko wrote:
> Hello.
> 
> On 20.02.2023 21:31, David Woodhouse wrote:
>> On Mon, 2023-02-20 at 17:40 +0100, Oleksandr Natalenko wrote:
>>> On pondělí 20. února 2023 17:20:13 CET David Woodhouse wrote:
>>> > On Mon, 2023-02-20 at 17:08 +0100, Oleksandr Natalenko wrote:
>>> > >
>>> > > I've applied this to the v6.2 kernel, and suspend/resume broke on
>>> > > my
>>> > > Ryzen 5950X desktop. The machine suspends just fine, but on
>>> > > resume
>>> > > the screen stays blank, and there's no visible disk I/O.
>>> > >
>>> > > Reverting the series brings suspend/resume back to working state.
>>> >
>>> > Hm, thanks. What if you add 'no_parallel_bringup' on the command
>>> > line?
>>>
>>> If the `no_parallel_bringup` param is added, the suspend/resume
>>> works.
>>
>> Thanks for the testing. Can I ask you to do one further test: apply the
>> series only as far as patch 6/8 'x86/smpboot: Support parallel startup
>> of secondary CPUs'.
>>
>> That will do the new startup asm sequence where each CPU finds its own
>> per-cpu data so it *could* work in parallel, but doesn't actually do
>> the bringup in parallel yet.
> 
> With patches 1 to 6 (including) applied and no extra cmdline params 
> added the resume doesn't work.
> 
>> Does your box have a proper serial port?
> 
> No, sorry. I know it'd help with getting logs, and I do have a 
> serial-to-USB cable that I use for another machine, but in this one the 
> port is not routed to outside. I think I can put a header there as the 
> motherboard does have pins, but I'd have to buy one first. In theory, I 
> can do that, but that won't happen within the next few weeks.
> 
> P.S. Piotr Gorski (in Cc) also reported this: "My friend from CachyOS 
> can confirm bugs with smpboot patches. AMD FX 6300 only shows 1 core 
> when using smp boot patchset". Probably, he can reply to this thread and 
> provide more details.
> 

Hi Oleksandr,

So for initial boot, do all CPUs comes up for you when parallel smp boot 
is enabled or only 1?

I don't have access to Ryzen hardware so can only say from code, but it 
would be weird if initial boot is fine but resume is broken if the same 
code path is being taken.

Thanks,
Usama

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

[Oleksandr Natalenko]

Hello.

On pondělí 20. února 2023 23:23:43 CET Usama Arif wrote:
> So for initial boot, do all CPUs comes up for you when parallel smp boot 
> is enabled or only 1?

All the CPUs come up during the initial boot.

> I don't have access to Ryzen hardware so can only say from code, but it 
> would be weird if initial boot is fine but resume is broken if the same 
> code path is being taken.

-- 
Oleksandr Natalenko (post-factum)

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

[David Woodhouse]

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

On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
> The main change over v8 is dropping the patch to avoid repeated saves of MTRR
> at boot time. It didn't make a difference to smpboot time and is independent
> of parallel CPU bringup, so if needed can be explored in a separate patchset.
> 
> The patches have also been rebased to v6.2-rc8 and retested and the
> improvement in boot time is the same as v8.

Thanks for picking this up, Usama.

So the next thing that might be worth looking at is allowing the APs
all to be running their hotplug thread simultaneously, bringing
themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
the initial INIT/SIPI/SIPI latency, but if there's any significant time
in the AP hotplug thread, that could be worth parallelising.

There may be further wins in the INIT/SIPI/SIPI too. Currently we
process each CPU at a time, sending INIT, SIPI, waiting 10µs and
sending another SIPI.

What if we sent the first INIT+SIPI to all CPUs, then did another pass
sending another SIPI only to those which hadn't already started running
and set their bit in cpu_initialized_mask ? 

Might not be worth it, and there's an added complexity that they all
have to wait for each other (on the real mode trampoline lock) before
they can take their turn and get as far as setting their bit in
cpu_initialized_mask. So we'd probably end up sending the second SIPI
to most of them *anyway*.

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

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

[Usama Arif]

On 22/02/2023 10:11, David Woodhouse wrote:
> On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
>> The main change over v8 is dropping the patch to avoid repeated saves of MTRR
>> at boot time. It didn't make a difference to smpboot time and is independent
>> of parallel CPU bringup, so if needed can be explored in a separate patchset.
>>
>> The patches have also been rebased to v6.2-rc8 and retested and the
>> improvement in boot time is the same as v8.
> 
> Thanks for picking this up, Usama.
> 
> So the next thing that might be worth looking at is allowing the APs
> all to be running their hotplug thread simultaneously, bringing
> themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
> the initial INIT/SIPI/SIPI latency, but if there's any significant time
> in the AP hotplug thread, that could be worth parallelising.
> 
> There may be further wins in the INIT/SIPI/SIPI too. Currently we
> process each CPU at a time, sending INIT, SIPI, waiting 10µs and
> sending another SIPI.
> 
> What if we sent the first INIT+SIPI to all CPUs, then did another pass
> sending another SIPI only to those which hadn't already started running
> and set their bit in cpu_initialized_mask ?
> 
> Might not be worth it, and there's an added complexity that they all
> have to wait for each other (on the real mode trampoline lock) before
> they can take their turn and get as far as setting their bit in
> cpu_initialized_mask. So we'd probably end up sending the second SIPI
> to most of them *anyway*.

Thanks! I think I sent out v10 a bit too early, but hopefully it looks 
like everyone agrees on the suspend code in it at the moment?

As a next step, I was thinking of reposting and starting a discussion on 
the reuse timer calibration patch separately. Its not part of parallel 
smp, but in my testing, it takes away (70ms) ~70% of the remaining 
parallel smpboot time. With the machine and kernel I am testing, the 
kexec reboot time after parallel smp is just under a second, so this 
represents ~7% of the boot time, which is a notable percentage reduction 
in server downtime. Or maybe someone could reply to this thread saying 
its not a good idea to post it as I remember there were quite a few 
reservations about it? :)

Thanks,
Usama

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

[Brian Gerst]

On Wed, Feb 22, 2023 at 5:44 AM David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
> > The main change over v8 is dropping the patch to avoid repeated saves of MTRR
> > at boot time. It didn't make a difference to smpboot time and is independent
> > of parallel CPU bringup, so if needed can be explored in a separate patchset.
> >
> > The patches have also been rebased to v6.2-rc8 and retested and the
> > improvement in boot time is the same as v8.
>
> Thanks for picking this up, Usama.
>
> So the next thing that might be worth looking at is allowing the APs
> all to be running their hotplug thread simultaneously, bringing
> themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
> the initial INIT/SIPI/SIPI latency, but if there's any significant time
> in the AP hotplug thread, that could be worth parallelising.
>
> There may be further wins in the INIT/SIPI/SIPI too. Currently we
> process each CPU at a time, sending INIT, SIPI, waiting 10µs and
> sending another SIPI.
>
> What if we sent the first INIT+SIPI to all CPUs, then did another pass
> sending another SIPI only to those which hadn't already started running
> and set their bit in cpu_initialized_mask ?
>
> Might not be worth it, and there's an added complexity that they all
> have to wait for each other (on the real mode trampoline lock) before
> they can take their turn and get as far as setting their bit in
> cpu_initialized_mask. So we'd probably end up sending the second SIPI
> to most of them *anyway*.

Speaking of next steps, I have a followup patchset ready to go that
removes the global variables initial_stack, initial_gs, and
early_gdt_descr.  Should I send that now or wait until this patchset
lands in -tip?

--
Brian Gerst

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

[David Woodhouse]

On 22 February 2023 12:08:04 GMT, Brian Gerst <brgerst@gmail.com> wrote:
>On Wed, Feb 22, 2023 at 5:44 AM David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
>> > The main change over v8 is dropping the patch to avoid repeated saves of MTRR
>> > at boot time. It didn't make a difference to smpboot time and is independent
>> > of parallel CPU bringup, so if needed can be explored in a separate patchset.
>> >
>> > The patches have also been rebased to v6.2-rc8 and retested and the
>> > improvement in boot time is the same as v8.
>>
>> Thanks for picking this up, Usama.
>>
>> So the next thing that might be worth looking at is allowing the APs
>> all to be running their hotplug thread simultaneously, bringing
>> themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
>> the initial INIT/SIPI/SIPI latency, but if there's any significant time
>> in the AP hotplug thread, that could be worth parallelising.
>>
>> There may be further wins in the INIT/SIPI/SIPI too. Currently we
>> process each CPU at a time, sending INIT, SIPI, waiting 10µs and
>> sending another SIPI.
>>
>> What if we sent the first INIT+SIPI to all CPUs, then did another pass
>> sending another SIPI only to those which hadn't already started running
>> and set their bit in cpu_initialized_mask ?
>>
>> Might not be worth it, and there's an added complexity that they all
>> have to wait for each other (on the real mode trampoline lock) before
>> they can take their turn and get as far as setting their bit in
>> cpu_initialized_mask. So we'd probably end up sending the second SIPI
>> to most of them *anyway*.
>
>Speaking of next steps, I have a followup patchset ready to go that
>removes the global variables initial_stack, initial_gs, and
>early_gdt_descr.  Should I send that now or wait until this patchset
>lands in -tip?

Happy either way. Want to send it and we can take a look at whether to work it in with this?

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

[Thomas Gleixner]

David!

On Wed, Feb 22 2023 at 10:11, David Woodhouse wrote:
> On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
> So the next thing that might be worth looking at is allowing the APs
> all to be running their hotplug thread simultaneously, bringing
> themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
> the initial INIT/SIPI/SIPI latency, but if there's any significant time
> in the AP hotplug thread, that could be worth parallelising.

On a 112 CPU machine (64 cores, HT enabled) the bringup takes

Setup and SIPIs sent: 	 49 ms
Bringup each CPU:	516 ms

That's about 500 ms faster than a non-parallel bringup!

Now looking at the 516 ms, which is ~4.7 ms/CPU. The vast majority of the
time is spent on the APs in

     cpu_init() -> ucode_cpu_init()

for the primary threads of each core. The secondary threads are quickly
(1us) out of ucode_cpu_init() because the primary thread already loaded
it.

A microcode load on that machine takes ~7.5 ms per primary thread on
average which sums up to 7.5 * 55 = 412.5 ms

The threaded bringup after CPU_AP_ONLINE takes about 100us per CPU.

identify_secondary_cpu() is one of the longer functions which takes
~125us / CPU summing up to 13ms

The TSC sync check for the first CPU on the second socket consumes
20ms. That's only once per socket, intra socket is using MSR_TSC_ADJUST,
which is more or less free.

So the 516 ms are wasted here:

   total                                516 ms
   ucode_cpu_init()                     412 ms
   identify_secondary_cpu()              13 ms
   2ndsocket_tsc_sync			 20 ms
   threaded bringup                      12 ms
   rest 		                 59 ms

So the rest is about 530us per CPU, which is just the sum of many small
functions, lock contentions...

Getting rid of the micro code overhead is possible. There is no reason
to serialize that between the cores. But it needs serialization vs. HT
siblings, which requires to move identify_secondary_cpu() and its caller
smp_store_cpu_info() ahead of the synchronization point and then have
serialization between the siblings. That's going to be a major surgery
and inspection effort to ensure that there are no hidden assumptions
about global hotplug serialization. 

So that would cut the total cost down to ~100ms plus the
preparatory/SIPI stage of 60ms which sums up to about 160ms and about
1.5ms per CPU total.

Further optimization starts to be questionable IMO. It's surely possible
somehow, but then you really have to go and inspect each and every
function in those code pathes, add local locking, etc. Not to talk about
the required mess in the core code to support that.

The low hanging fruit which brings most is the identification/topology
muck and the microcode loading. That needs to be addressed first anyway.

Thanks,

        tglx

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

[David Woodhouse]

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

On Wed, 2023-02-22 at 17:42 +0100, Thomas Gleixner wrote:
> David!
> 
> On Wed, Feb 22 2023 at 10:11, David Woodhouse wrote:
> > On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
> > So the next thing that might be worth looking at is allowing the APs
> > all to be running their hotplug thread simultaneously, bringing
> > themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
> > the initial INIT/SIPI/SIPI latency, but if there's any significant time
> > in the AP hotplug thread, that could be worth parallelising.
> 
> On a 112 CPU machine (64 cores, HT enabled) the bringup takes
> 
> Setup and SIPIs sent:    49 ms
> Bringup each CPU:       516 ms
> 
> That's about 500 ms faster than a non-parallel bringup!
> 
> Now looking at the 516 ms, which is ~4.7 ms/CPU. The vast majority of the
> time is spent on the APs in
> 
>      cpu_init() -> ucode_cpu_init()
> 
> for the primary threads of each core. The secondary threads are quickly
> (1us) out of ucode_cpu_init() because the primary thread already loaded
> it.
> 
> A microcode load on that machine takes ~7.5 ms per primary thread on
> average which sums up to 7.5 * 55 = 412.5 ms
> 
> The threaded bringup after CPU_AP_ONLINE takes about 100us per CPU.

Nice analysis; thanks!

> identify_secondary_cpu() is one of the longer functions which takes
> ~125us / CPU summing up to 13ms

Hm, shouldn't that one already be parallelised by my 'part 2' patch?

It's called from smp_store_cpu_info(), from smp_callin(), which is
called from somewhere in the middle of start_secondary().

And if the comments I helpfully added to that function for the benefit
of our future selves are telling the truth, the AP is free to get that
far once the BSP has set its bit in cpu_callout_mask, which happens in
do_wait_cpu_initialized().

So
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/4b5731e05b0#patch3
ought to parallelise that. But Usama emirically reported that 'part 2'
didn't add any noticeable benefit, not even those 13ms? On a *larger*
machine.


> The TSC sync check for the first CPU on the second socket consumes
> 20ms. That's only once per socket, intra socket is using MSR_TSC_ADJUST,
> which is more or less free.
> 
> So the 516 ms are wasted here:
> 
>    total                                516 ms
>    ucode_cpu_init()                     412 ms
>    identify_secondary_cpu()              13 ms
>    2ndsocket_tsc_sync                    20 ms
>    threaded bringup                      12 ms
>    rest                                  59 ms
> 
> So the rest is about 530us per CPU, which is just the sum of many small
> functions, lock contentions...
> 
> Getting rid of the micro code overhead is possible. There is no reason
> to serialize that between the cores. But it needs serialization vs. HT
> siblings, which requires to move identify_secondary_cpu() and its caller
> smp_store_cpu_info() ahead of the synchronization point and then have
> serialization between the siblings. That's going to be a major surgery
> and inspection effort to ensure that there are no hidden assumptions
> about global hotplug serialization. 
> 
> So that would cut the total cost down to ~100ms plus the
> preparatory/SIPI stage of 60ms which sums up to about 160ms and about
> 1.5ms per CPU total.
> 
> Further optimization starts to be questionable IMO. It's surely possible
> somehow, but then you really have to go and inspect each and every
> function in those code pathes, add local locking, etc. Not to talk about
> the required mess in the core code to support that.
> 
> The low hanging fruit which brings most is the identification/topology
> muck and the microcode loading. That needs to be addressed first anyway.

Agreed, thanks.


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

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

[Thomas Gleixner]

David!

On Thu, Feb 23 2023 at 11:07, David Woodhouse wrote:
> On Wed, 2023-02-22 at 17:42 +0100, Thomas Gleixner wrote:
>> The low hanging fruit which brings most is the identification/topology
>> muck and the microcode loading. That needs to be addressed first anyway.
>
> Agreed, thanks.

So the problem with microcode loading is that we must ensure that a HT
sibling is not executing anything else than a trivial loop waiting for
the update to complete. So something like this should work:

   1) Kick all CPUs into life and let them run up to cpu_init() and
      retrieve only the topology information.

   2) Wait for all CPUs to reach this point

   3) Release all primary HT threads so they can load microcode in
      parallel. The secondary HT threads stay in the wait loop and are
      released once the primary thread has finished the microcode
      update.

   4) Let the CPUs do the full CPUID readout and let them synchronize
      with the control CPU again.

   5) Complete bringup one by one

Thanks,

        tglx
      


   

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

[David Woodhouse]

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

On Thu, 2023-02-23 at 15:37 +0100, Thomas Gleixner wrote:
> David!
> 
> On Thu, Feb 23 2023 at 11:07, David Woodhouse wrote:
> > On Wed, 2023-02-22 at 17:42 +0100, Thomas Gleixner wrote:
> > > The low hanging fruit which brings most is the identification/topology
> > > muck and the microcode loading. That needs to be addressed first anyway.
> > 
> > Agreed, thanks.
> 
> So the problem with microcode loading is that we must ensure that a HT
> sibling is not executing anything else than a trivial loop waiting for
> the update to complete. So something like this should work:
> 
>    1) Kick all CPUs into life and let them run up to cpu_init() and
>       retrieve only the topology information.
>
>    2) Wait for all CPUs to reach this point
>
>    3) Release all primary HT threads so they can load microcode in
>       parallel. The secondary HT threads stay in the wait loop and are
>       released once the primary thread has finished the microcode
>       update.
> 
>    4) Let the CPUs do the full CPUID readout and let them synchronize
>       with the control CPU again.
> 
>    5) Complete bringup one by one


Can we move the microcode loading to happen earlier, during the x86-
specific CPUHP_BP_PARALLEL_DYN stage(s) while they're running in
parallel.

In the existing set of patches, we send INIT/SIPI/SIPI to each CPU in
parallel and they run to the first part of start_secondary(), up to the
point where it calls cpu_init_secondary() and sets their bit in
cpu_initialized_mask, then spinning and waiting for cpu_callout_mask.


My "part 2" test patch does another round in parallel, setting each
CPU's bit in 'cpu_callout_mask' and letting them run a bit further
through start_secondary() until they get to the end of smp_callin(),
where they set their bit in smp_callin_mask and (in my patch) wait for
their bit in a new cpu_finishup_mask to be set — which is what releases
them to proceed to completion in the final native_cpu_up() bringup.

So perhaps the BSP doesn't need to coordinate anything here, if we can
let the siblings work it out between themselves in the (now-)parallel
stage at the end of smp_callin()? And only set their bit in
smp_callin_mask when the microcode update is done?

Hm, maybe it's as simple as the first¹ thread on a core waiting for all
its siblings' bits in cpu_callin_mask to be set, and *then* doing the
update before setting its own bit?

¹ As long as we define "first" as the one with the lowest CPU#, which
means that the BSP won't release any of the siblings before it releases
the "first".

Then the siblings are just spinning on cpu_callin_mask anyway; they
don't need to do anything *more*.

Probably worth knocking it up and seeing how badly it explodes?



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

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

[Usama Arif]

On 23/02/2023 11:07, David Woodhouse wrote:
> On Wed, 2023-02-22 at 17:42 +0100, Thomas Gleixner wrote:
>> David!
>>
>> On Wed, Feb 22 2023 at 10:11, David Woodhouse wrote:
>>> On Wed, 2023-02-15 at 14:54 +0000, Usama Arif wrote:
>>> So the next thing that might be worth looking at is allowing the APs
>>> all to be running their hotplug thread simultaneously, bringing
>>> themselves from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE. This series eats
>>> the initial INIT/SIPI/SIPI latency, but if there's any significant time
>>> in the AP hotplug thread, that could be worth parallelising.
>>
>> On a 112 CPU machine (64 cores, HT enabled) the bringup takes
>>
>> Setup and SIPIs sent:    49 ms
>> Bringup each CPU:       516 ms
>>
>> That's about 500 ms faster than a non-parallel bringup!
>>
>> Now looking at the 516 ms, which is ~4.7 ms/CPU. The vast majority of the
>> time is spent on the APs in
>>
>>       cpu_init() -> ucode_cpu_init()
>>
>> for the primary threads of each core. The secondary threads are quickly
>> (1us) out of ucode_cpu_init() because the primary thread already loaded
>> it.
>>
>> A microcode load on that machine takes ~7.5 ms per primary thread on
>> average which sums up to 7.5 * 55 = 412.5 ms
>>
>> The threaded bringup after CPU_AP_ONLINE takes about 100us per CPU.
> 
> Nice analysis; thanks!
> 
>> identify_secondary_cpu() is one of the longer functions which takes
>> ~125us / CPU summing up to 13ms
> 
> Hm, shouldn't that one already be parallelised by my 'part 2' patch?
> 
> It's called from smp_store_cpu_info(), from smp_callin(), which is
> called from somewhere in the middle of start_secondary().
> 
> And if the comments I helpfully added to that function for the benefit
> of our future selves are telling the truth, the AP is free to get that
> far once the BSP has set its bit in cpu_callout_mask, which happens in
> do_wait_cpu_initialized().
> 
> So
> https://git.infradead.org/users/dwmw2/linux.git/commitdiff/4b5731e05b0#patch3
> ought to parallelise that. But Usama emirically reported that 'part 2'
> didn't add any noticeable benefit, not even those 13ms? On a *larger*
> machine.
> 

So I am using a similar machine to Thomas 128 CPU machine (64 cores, HT 
enabled). I have microcode config disabled, so I guess I get similar 
numbers to Thomas, i.e. 100ms (516 - 412) ms. I do see a difference of 
~3ms with part2 which I thought is maybe within the margin of error for 
measuring, but I guess it isn't. After seeing the ~70ms that is cut with 
reusing timer calibration, I didnt really then focus much on part 2 
then. I guess that ~70ms is the "rest" from Thomas' table below?

Thanks,
Usama

> 
>> The TSC sync check for the first CPU on the second socket consumes
>> 20ms. That's only once per socket, intra socket is using MSR_TSC_ADJUST,
>> which is more or less free.
>>
>> So the 516 ms are wasted here:
>>
>>     total                                516 ms
>>     ucode_cpu_init()                     412 ms
>>     identify_secondary_cpu()              13 ms
>>     2ndsocket_tsc_sync                    20 ms
>>     threaded bringup                      12 ms
>>     rest                                  59 ms
>>
>> So the rest is about 530us per CPU, which is just the sum of many small
>> functions, lock contentions...
>>
>> Getting rid of the micro code overhead is possible. There is no reason
>> to serialize that between the cores. But it needs serialization vs. HT
>> siblings, which requires to move identify_secondary_cpu() and its caller
>> smp_store_cpu_info() ahead of the synchronization point and then have
>> serialization between the siblings. That's going to be a major surgery
>> and inspection effort to ensure that there are no hidden assumptions
>> about global hotplug serialization.
>>
>> So that would cut the total cost down to ~100ms plus the
>> preparatory/SIPI stage of 60ms which sums up to about 160ms and about
>> 1.5ms per CPU total.
>>
>> Further optimization starts to be questionable IMO. It's surely possible
>> somehow, but then you really have to go and inspect each and every
>> function in those code pathes, add local locking, etc. Not to talk about
>> the required mess in the core code to support that.
>>
>> The low hanging fruit which brings most is the identification/topology
>> muck and the microcode loading. That needs to be addressed first anyway.
> 
> Agreed, thanks.
>