[PATCH v7 0/9] Parallel CPU bringup for x86_64

[PATCH v7 0/9] Parallel CPU bringup for x86_64

[Usama Arif]

Tested on v7, doing INIT/SIPI/SIPI in parallel brings down the time for
smpboot from ~700ms to 100ms (85% improvement) on a server with 128 CPUs
split across 2 NUMA nodes.

The major change over v6 is keeping parallel smp support enabled in AMD.
APIC ID for parallel CPU bringup is now obtained from CPUID leaf 0x0B
(for x2APIC mode) otherwise CPUID leaf 0x1 (8 bits).

The patch for reusing timer calibration for secondary CPUs is also removed
from the series as its not part of parallel smp bringup and needs to be
further thought about.

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.

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: Send INIT/SIPI/SIPI to secondary CPUs in parallel
  x86/mtrr: Avoid repeated save of MTRRs on boot-time CPU bringup
  x86/smpboot: Serialize topology updates for secondary bringup

Thomas Gleixner (1):
  x86/smpboot: Support parallel startup of secondary CPUs

 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/cpu/mtrr/mtrr.c       |   9 +
 arch/x86/kernel/head_64.S             |  84 +++++++
 arch/x86/kernel/smpboot.c             | 349 +++++++++++++++++++-------
 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, 506 insertions(+), 159 deletions(-)

-- 
2.25.1

[PATCH v7 1/9] 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.

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

[PATCH v7 2/9] 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 v7 3/9] 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 v7 4/9] 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 v7 5/9] 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) Waits 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 | 182 +++++++++++++++++++++++++++-----------
 1 file changed, 128 insertions(+), 54 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3a793772a2aa..eee717086ab7 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,33 @@ static void notrace start_secondary(void *unused)
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
 #endif
+	/*
+	 * Sync point with do_wait_cpu_initialized(). On boot, all secondary
+	 * CPUs reach this stage after receiving INIT/SIPI from do_cpu_up()
+	 * in the x86/cpu:kick cpuhp stage. At the start of cpu_init() they
+	 * will wait for do_wait_cpu_initialized() to set their bit in
+	 * smp_callout_mask to release them.
+	 */
 	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 +274,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 +1104,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 +1164,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 +1298,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 +1309,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

Re: [PATCH v7 5/9] x86/smpboot: Split up native_cpu_up into separate phases and document them

[Yuan Yao]

On Tue, Feb 07, 2023 at 11:04:32PM +0000, Usama Arif wrote:
> 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) Waits 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 | 182 +++++++++++++++++++++++++++-----------
>  1 file changed, 128 insertions(+), 54 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 3a793772a2aa..eee717086ab7 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,33 @@ static void notrace start_secondary(void *unused)
>  	load_cr3(swapper_pg_dir);
>  	__flush_tlb_all();
>  #endif
> +	/*
> +	 * Sync point with do_wait_cpu_initialized(). On boot, all secondary
> +	 * CPUs reach this stage after receiving INIT/SIPI from do_cpu_up()
> +	 * in the x86/cpu:kick cpuhp stage. At the start of cpu_init() they
> +	 * will wait for do_wait_cpu_initialized() to set their bit in
> +	 * smp_callout_mask to release them.

The last sentence of the comment looks confused. The fact is:

For serial case, The BSP waits AP to set cpu_initialized_mask from
wait_for_master_cpu() after fired INIT/SIPI, then AP starts to wait
cpu_callout_mask set by BSP from do_boot_cpu().

Or the comments below "Bringup step two:..." which also looks clear
enough then above.

> +	 */
>  	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 +274,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 +1104,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 +1164,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 +1298,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 +1309,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
>

Re: [PATCH v7 5/9] x86/smpboot: Split up native_cpu_up into separate phases and document them

[David Woodhouse]

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

On Wed, 2023-02-08 at 18:03 +0800, Yuan Yao wrote:
> 
> >   #endif
> > +       /*
> > +        * Sync point with do_wait_cpu_initialized(). On boot, all secondary
> > +        * CPUs reach this stage after receiving INIT/SIPI from do_cpu_up()
> > +        * in the x86/cpu:kick cpuhp stage. At the start of cpu_init() they
> > +        * will wait for do_wait_cpu_initialized() to set their bit in
> > +        * smp_callout_mask to release them.
> 
> The last sentence of the comment looks confused. The fact is:
> 
> For serial case, The BSP waits AP to set cpu_initialized_mask from
> wait_for_master_cpu() after fired INIT/SIPI, then AP starts to wait
> cpu_callout_mask set by BSP from do_boot_cpu().
> 
> Or the comments below "Bringup step two:..." which also looks clear
> enough then above.


	/*
	 * Sync point with do_wait_cpu_initialized(). Before proceeding through
	 * cpu_init(), the AP will call wait_for_master_cpu() which sets its
	 * bit in cpu_initialized_mask and then waits for the BSP to set its
	 * bit in cpu_callout_mask to release it.
	 */
	cpu_init_secondary();


Better?

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

Re: [PATCH v7 5/9] x86/smpboot: Split up native_cpu_up into separate phases and document them

[Yuan Yao]

On Wed, Feb 08, 2023 at 12:02:55PM +0000, David Woodhouse wrote:
> On Wed, 2023-02-08 at 18:03 +0800, Yuan Yao wrote:
> >
> > >   #endif
> > > +       /*
> > > +        * Sync point with do_wait_cpu_initialized(). On boot, all secondary
> > > +        * CPUs reach this stage after receiving INIT/SIPI from do_cpu_up()
> > > +        * in the x86/cpu:kick cpuhp stage. At the start of cpu_init() they
> > > +        * will wait for do_wait_cpu_initialized() to set their bit in
> > > +        * smp_callout_mask to release them.
> >
> > The last sentence of the comment looks confused. The fact is:
> >
> > For serial case, The BSP waits AP to set cpu_initialized_mask from
> > wait_for_master_cpu() after fired INIT/SIPI, then AP starts to wait
> > cpu_callout_mask set by BSP from do_boot_cpu().
> >
> > Or the comments below "Bringup step two:..." which also looks clear
> > enough then above.
>
>
> 	/*
> 	 * Sync point with do_wait_cpu_initialized(). Before proceeding through
> 	 * cpu_init(), the AP will call wait_for_master_cpu() which sets its
> 	 * bit in cpu_initialized_mask and then waits for the BSP to set its
> 	 * bit in cpu_callout_mask to release it.
> 	 */
> 	cpu_init_secondary();
>
>
> Better?

Yes, it's better to me, thanks.

[PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[Usama Arif]

From: Thomas Gleixner <tglx@linutronix.de>

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.

[ dwmw2: Minor tweaks, write a commit message, add CPUID 0x01 support ]
[ seanc: Fix stray override of initial_gs in common_cpu_up() ]
Not-signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 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            | 84 ++++++++++++++++++++++++++++
 arch/x86/kernel/smpboot.c            | 60 ++++++++++++++++++--
 arch/x86/realmode/init.c             |  3 +
 arch/x86/realmode/rm/trampoline_64.S | 14 +++++
 kernel/smpboot.c                     |  2 +-
 9 files changed, 172 insertions(+), 7 deletions(-)

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..656e6018b9d4 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,77 @@ 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 APICID of the current CPU */
+	xorl	%ecx, %ecx
+	leaq	cpuid_to_apicid(%rip), %rbx
+
+.Lfind_cpunr:
+	cmpl	(%rbx), %edx
+	jz	.Linit_cpu_data
+	addq	$4, %rbx
+	addq	$8, %rcx
+	jmp	.Lfind_cpunr
+
+.Linit_cpu_data:
+	/* Get the per cpu offset */
+	leaq	__per_cpu_offset(%rip), %rbx
+	addq	%rcx, %rbx
+	movq	(%rbx), %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, %rcx
+	addq	%rbx, %rcx
+	movq	(%rcx), %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 +353,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
@@ -426,6 +506,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 +741,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 eee717086ab7..7abdf347493f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -798,6 +798,16 @@ static int __init cpu_init_udelay(char *str)
 }
 early_param("cpu_init_udelay", cpu_init_udelay);
 
+static bool do_parallel_bringup = 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 */
@@ -1085,8 +1095,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;
 }
@@ -1111,9 +1119,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);
@@ -1513,6 +1526,45 @@ 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) {
+			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. */
+		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

Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[Brian Gerst]

On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <usama.arif@bytedance.com> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> 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.

For the serialized bringup case, it would be simpler to just put the
cpu number in the lower bits instead of the APIC id, skipping the
lookup.

--
Brian Gerst

Re: [External] Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[Usama Arif]

On 08/02/2023 05:09, Brian Gerst wrote:
> On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <usama.arif@bytedance.com> wrote:
>>
>> From: Thomas Gleixner <tglx@linutronix.de>
>>
>> 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.
> 
> For the serialized bringup case, it would be simpler to just put the
> cpu number in the lower bits instead of the APIC id, skipping the
> lookup.
> 
> --
> Brian Gerst

I guess we could do something like below, it would save a few loops 
through find_cpunr in serial case, but probably is as simple as just 
using setup_AP and find_cpunr for all cases? Happy with either but if 
there is a strong preference for below, can change in next revision?

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 656e6018b9d4..30aa543a0114 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -265,7 +265,10 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
SYM_L_GLOBAL)
         testl   $STARTUP_APICID_CPUID_01, %edx
         jnz     .Luse_cpuid_01
         andl    $0x0FFFFFFF, %edx
-       jmp     .Lsetup_AP
+       mov     $8, %eax
+       mul     %edx
+       movq    %rax, %rcx
+       jmp     .Linit_cpu_data

  .Luse_cpuid_01:
         mov     $0x01, %eax
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 8ffec5de2e2e..73dd87bf2f29 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1141,7 +1141,7 @@ static int do_boot_cpu(int apicid, int cpu, struct 
task_struct *idle,
                 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;
+               smpboot_control = STARTUP_SECONDARY | cpu;
         }

         /* Enable the espfix hack for this CPU */

Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[David Woodhouse]

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

On Wed, 2023-02-08 at 00:09 -0500, Brian Gerst wrote:
> On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <usama.arif@bytedance.com> wrote:
> > 
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > 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.
> 
> For the serialized bringup case, it would be simpler to just put the
> cpu number in the lower bits instead of the APIC id, skipping the
> lookup.
> 

I seem to recall that was one of my first thoughts on seeing this
patch.

I quite liked the fact that the code path in head_64.S remained
basically the same for all cases, and was actually testing the lookup
via cpuid_to_apicid[] even when parallel boot isn't available.

But then again, we've tested that part fairly well now, so perhaps
you're right and we could do something like the below? It doesn't
actually add much more complexity; just define the entry conditions to
.Linit_cpu_data a bit more clearly and remember how to use scaled index
addressing so we don't have to explicitly multiply by 8.

I don't know that I care very much either way. It's Thomas's patch
originally, so I'll let him see if he can muster up an opinion.

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 656e6018b9d4..d878869472a2 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -252,20 +252,23 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	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:
+	 * Secondary CPUs find out the offsets via their per_cpu data. For
+	 * parallel bringup they find it via the cpuid_to_apicid[] array
+	 * with their APIC ID obtained from CPUID. Otherwise the CPU number
+	 * is encoded directly 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
+	 * Bit 0-24	CPU# 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
+	mov	%edx, %ecx
+	jmp	.Linit_cpu_data
 
 .Luse_cpuid_01:
 	mov	$0x01, %eax
@@ -288,14 +291,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	cmpl	(%rbx), %edx
 	jz	.Linit_cpu_data
 	addq	$4, %rbx
-	addq	$8, %rcx
+	inc	%rcx
 	jmp	.Lfind_cpunr
 
 .Linit_cpu_data:
+	/* ECX contains the CPU# of the current CPU */
 	/* Get the per cpu offset */
 	leaq	__per_cpu_offset(%rip), %rbx
-	addq	%rcx, %rbx
-	movq	(%rbx), %rbx
+	movq	(%rbx,%rcx,8), %rbx
 	/* Save it for GS BASE setup */
 	movq	%rbx, initial_gs(%rip)
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index cae916040c55..5149676881e0 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1146,7 +1146,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		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;
+		smpboot_control = STARTUP_SECONDARY | cpu;
 	}
 
 	/* Enable the espfix hack for this CPU */


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

Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[Thomas Gleixner]

On Wed, Feb 08 2023 at 00:09, Brian Gerst wrote:
> On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <usama.arif@bytedance.com> wrote:
>> 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.
>
> For the serialized bringup case, it would be simpler to just put the
> cpu number in the lower bits instead of the APIC id, skipping the
> lookup.

Yes and no. It can be done, but I rather prefer the consistency of the
data and the mechanism. The "overhead" or "win" is not even measurable.

Thanks,

        tglx

Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[David Woodhouse]

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

On Wed, 2023-02-08 at 14:13 +0100, Thomas Gleixner wrote:
> On Wed, Feb 08 2023 at 00:09, Brian Gerst wrote:
> > On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <usama.arif@bytedance.com> wrote:
> > > 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.
> > 
> > For the serialized bringup case, it would be simpler to just put the
> > cpu number in the lower bits instead of the APIC id, skipping the
> > lookup.
> 
> Yes and no. It can be done, but I rather prefer the consistency of the
> data and the mechanism. The "overhead" or "win" is not even measurable.


Yeah. I might do this much though, just to track CPU# simply in %ecx:

--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -285,17 +285,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify,
SYM_L_GLOBAL)
        leaq    cpuid_to_apicid(%rip), %rbx
 
 .Lfind_cpunr:
-       cmpl    (%rbx), %edx
+       cmpl    (%rbx,%rcx,4), %edx
        jz      .Linit_cpu_data
-       addq    $4, %rbx
-       addq    $8, %rcx
+       inc     %rcx
        jmp     .Lfind_cpunr
 
 .Linit_cpu_data:
-       /* Get the per cpu offset */
+       /* Get the per cpu offset for the given CPU# which is in ECX */
        leaq    __per_cpu_offset(%rip), %rbx
-       addq    %rcx, %rbx
-       movq    (%rbx), %rbx
+       movq    (%rbx,%rcx,8), %rbx
        /* Save it for GS BASE setup */
        movq    %rbx, initial_gs(%rip)
 


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

[PATCH v7 7/9] 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 7abdf347493f..7e7bcab6676e 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>
@@ -1326,9 +1327,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)
@@ -1350,6 +1354,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
  */
@@ -1565,6 +1575,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 v7 8/9] x86/mtrr: Avoid repeated save of MTRRs on boot-time CPU bringup

[Usama Arif]

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

There's no need to repeatedly save the BSP's MTRRs for each AP being
brought up at boot time. And no need to use smp_call_function_single()
even for the one time they do need to be saved.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 783f3210d582..b6eae3ad4414 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -721,11 +721,20 @@ void __init mtrr_bp_init(void)
  */
 void mtrr_save_state(void)
 {
+	static bool mtrr_saved;
 	int first_cpu;
 
 	if (!mtrr_enabled())
 		return;
 
+	if (system_state < SYSTEM_RUNNING) {
+		if (!mtrr_saved) {
+			mtrr_save_fixed_ranges(NULL);
+			mtrr_saved = true;
+		}
+		return;
+	}
+
 	first_cpu = cpumask_first(cpu_online_mask);
 	smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
 }
-- 
2.25.1

[PATCH v7 9/9] 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 7e7bcab6676e..8ffec5de2e2e 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();
 
@@ -244,6 +240,12 @@ static void notrace start_secondary(void *unused)
 	 * smp_callout_mask to release them.
 	 */
 	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();
 
@@ -352,7 +354,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;
 
@@ -375,7 +377,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;
 
@@ -406,25 +408,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)
@@ -630,7 +614,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;
@@ -709,6 +693,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 v7 0/9] Parallel CPU bringup for x86_64

[Paul E. McKenney]

On Tue, Feb 07, 2023 at 11:04:27PM +0000, Usama Arif wrote:
> Tested on v7, doing INIT/SIPI/SIPI in parallel brings down the time for
> smpboot from ~700ms to 100ms (85% improvement) on a server with 128 CPUs
> split across 2 NUMA nodes.
> 
> The major change over v6 is keeping parallel smp support enabled in AMD.
> APIC ID for parallel CPU bringup is now obtained from CPUID leaf 0x0B
> (for x2APIC mode) otherwise CPUID leaf 0x1 (8 bits).
> 
> The patch for reusing timer calibration for secondary CPUs is also removed
> from the series as its not part of parallel smp bringup and needs to be
> further thought about.

Running rcutorture on this got me the following NULL pointer dereference
on scenario TREE01:

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

[   34.662066] smpboot: CPU 0 is now offline
[   34.674075] rcu: NOCB: Cannot CB-offload offline CPU 25
[   35.038003] rcu: De-offloading 5
[   35.112997] rcu: Offloading 12
[   35.716011] smpboot: Booting Node 0 Processor 0 APIC 0x0
[   35.762685] BUG: kernel NULL pointer dereference, address: 0000000000000001
[   35.764278] #PF: supervisor instruction fetch in kernel mode
[   35.765530] #PF: error_code(0x0010) - not-present page
[   35.766700] PGD 0 P4D 0
[   35.767278] Oops: 0010 [#1] PREEMPT SMP PTI
[   35.768223] CPU: 36 PID: 0 Comm: swapper/36 Not tainted 6.2.0-rc1-00206-g18a37610b632-dirty #3563
[   35.770201] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014

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

Given an x86 system with KVM and qemu, this can be reproduced by running
the following from the top-level directory in the Linux-kernel source
tree:

	tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --configs "TREE01 TINY01" --trust-make

Out of 15 runs, 14 blew up just after the first attempt to bring CPU
0 back online.  The 15th run blew up just after the second attempt to
bring CPU 0 online, the first attempt having succeeded.

My guess is that the CONFIG_BOOTPARAM_HOTPLUG_CPU0=y Kconfig option is
tickling this bug.  This Kconfig option has been added to the TREE01
scenario in the -rcu tree's "dev" branch, which might mean that this test
would pass on mainline.  But CONFIG_BOOTPARAM_HOTPLUG_CPU0=y is not new,
only rcutorture's testing of it.

Thoughts?

							Thanx, Paul

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[Usama Arif]

On 09/02/2023 03:53, Paul E. McKenney wrote:
> On Tue, Feb 07, 2023 at 11:04:27PM +0000, Usama Arif wrote:
>> Tested on v7, doing INIT/SIPI/SIPI in parallel brings down the time for
>> smpboot from ~700ms to 100ms (85% improvement) on a server with 128 CPUs
>> split across 2 NUMA nodes.
>>
>> The major change over v6 is keeping parallel smp support enabled in AMD.
>> APIC ID for parallel CPU bringup is now obtained from CPUID leaf 0x0B
>> (for x2APIC mode) otherwise CPUID leaf 0x1 (8 bits).
>>
>> The patch for reusing timer calibration for secondary CPUs is also removed
>> from the series as its not part of parallel smp bringup and needs to be
>> further thought about.
> 
> Running rcutorture on this got me the following NULL pointer dereference
> on scenario TREE01:
> 
> ------------------------------------------------------------------------
> 
> [   34.662066] smpboot: CPU 0 is now offline
> [   34.674075] rcu: NOCB: Cannot CB-offload offline CPU 25
> [   35.038003] rcu: De-offloading 5
> [   35.112997] rcu: Offloading 12
> [   35.716011] smpboot: Booting Node 0 Processor 0 APIC 0x0
> [   35.762685] BUG: kernel NULL pointer dereference, address: 0000000000000001
> [   35.764278] #PF: supervisor instruction fetch in kernel mode
> [   35.765530] #PF: error_code(0x0010) - not-present page
> [   35.766700] PGD 0 P4D 0
> [   35.767278] Oops: 0010 [#1] PREEMPT SMP PTI
> [   35.768223] CPU: 36 PID: 0 Comm: swapper/36 Not tainted 6.2.0-rc1-00206-g18a37610b632-dirty #3563
> [   35.770201] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> 
> ------------------------------------------------------------------------
> 
> Given an x86 system with KVM and qemu, this can be reproduced by running
> the following from the top-level directory in the Linux-kernel source
> tree:
> 
> 	tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --configs "TREE01 TINY01" --trust-make
> 
> Out of 15 runs, 14 blew up just after the first attempt to bring CPU
> 0 back online.  The 15th run blew up just after the second attempt to
> bring CPU 0 online, the first attempt having succeeded.
> 
> My guess is that the CONFIG_BOOTPARAM_HOTPLUG_CPU0=y Kconfig option is
> tickling this bug.  This Kconfig option has been added to the TREE01
> scenario in the -rcu tree's "dev" branch, which might mean that this test
> would pass on mainline.  But CONFIG_BOOTPARAM_HOTPLUG_CPU0=y is not new,
> only rcutorture's testing of it.
> 
> Thoughts?
> 
> 							Thanx, Paul

It looks like its because of the initial_gs, initial_stack and
early_gdt_descr not being setup properly for CPU0 hotplug, i.e. 
init_cpu_data isnt called in cpu0 hotplug case.

Its easy to test, just by doing
echo 0 > /sys/devices/system/cpu/cpu0/online;
echo 1 > /sys/devices/system/cpu/cpu0/online;

As a quick check, if we do something like below (probably there is a 
much better place to set these..), the above hotplug commands will work.

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3ec5182d9698..184135c47ee5 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long 
start_ip, int apicid,
                                           wakeup_cpu0_nmi, 0, "wake_cpu0");

         if (!boot_error) {
+               initial_gs = per_cpu_offset(cpu);
                 enable_start_cpu0 = 1;
                 *cpu0_nmi_registered = 1;
                 id = apic->dest_mode_logical ? cpu0_logical_apicid : 
apicid;
@@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu, 
struct task_struct *idle,
                 boot_error = apic->wakeup_secondary_cpu_64(apicid, 
start_ip);
         else if (apic->wakeup_secondary_cpu)
                 boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
-       else
+       else {
+               if(!cpu) {
+                       early_gdt_descr.address = (unsigned 
long)get_cpu_gdt_rw(cpu);
+                       initial_stack  = idle->thread.sp;
+               }
                 boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
                                                      cpu0_nmi_registered);
-
+       }
         return boot_error;
  }

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
> 
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long 
> start_ip, int apicid,
>                                            wakeup_cpu0_nmi, 0, "wake_cpu0");
> 
>          if (!boot_error) {
> +               initial_gs = per_cpu_offset(cpu);

That's for 64-bit.

>                  enable_start_cpu0 = 1;
>                  *cpu0_nmi_registered = 1;
>                  id = apic->dest_mode_logical ? cpu0_logical_apicid :
> apicid;
> @@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu, 
> struct task_struct *idle,
>                  boot_error = apic->wakeup_secondary_cpu_64(apicid, 
> start_ip);
>          else if (apic->wakeup_secondary_cpu)
>                  boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
> -       else
> +       else {
> +               if(!cpu) {
> +                       early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> +                       initial_stack  = idle->thread.sp;
> +               }

And that's 32-bit.

These were previously done in common_cpu_up() or do_boot_cpu(), which
means they were done not only for the wakeup_cpu_via_init_nmi() code
path, but also when we call apic->wakeup_secondary_cpu() (or _64()) for
the esoteric UV/NumaConnect machines.

Thanks for diagnosing it so quickly; I'll work up a subtly different
fix.

>                  boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
>                                                       cpu0_nmi_registered);
> -
> +       }
>          return boot_error;
>   }
> 


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

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[Usama Arif]

On 09/02/2023 10:06, David Woodhouse wrote:
> On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
>>
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long
>> start_ip, int apicid,
>>                                             wakeup_cpu0_nmi, 0, "wake_cpu0");
>>
>>           if (!boot_error) {
>> +               initial_gs = per_cpu_offset(cpu);
> 
> That's for 64-bit.
> 
>>                   enable_start_cpu0 = 1;
>>                   *cpu0_nmi_registered = 1;
>>                   id = apic->dest_mode_logical ? cpu0_logical_apicid :
>> apicid;
>> @@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu,
>> struct task_struct *idle,
>>                   boot_error = apic->wakeup_secondary_cpu_64(apicid,
>> start_ip);
>>           else if (apic->wakeup_secondary_cpu)
>>                   boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
>> -       else
>> +       else {
>> +               if(!cpu) {
>> +                       early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
>> +                       initial_stack  = idle->thread.sp;
>> +               }
> 
> And that's 32-bit.
> 
> These were previously done in common_cpu_up() or do_boot_cpu(), which
> means they were done not only for the wakeup_cpu_via_init_nmi() code
> path, but also when we call apic->wakeup_secondary_cpu() (or _64()) for
> the esoteric UV/NumaConnect machines.
> 
> Thanks for diagnosing it so quickly; I'll work up a subtly different
> fix.
> 

Yeah, was just a hacky fix while I was trying to figure out the issue.

do_boot_cpu is only called in cpu0 in hotplug case, so I think this a 
much better diff:

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3ec5182d9698..f7ae82969ee4 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1136,13 +1136,16 @@ static int do_boot_cpu(int apicid, int cpu, 
struct task_struct *idle,
         idle->thread.sp = (unsigned long)task_pt_regs(idle);
         initial_code = (unsigned long)start_secondary;

-       if (IS_ENABLED(CONFIG_X86_32)) {
+       if (IS_ENABLED(CONFIG_X86_32) || !cpu) {
                 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;
         }

+       if (!cpu)
+               initial_gs = per_cpu_offset(cpu);
+
         /* Enable the espfix hack for this CPU */
         init_espfix_ap(cpu);



>>                   boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
>>                                                        cpu0_nmi_registered);
>> -
>> +       }
>>           return boot_error;
>>    }
>>
> 

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Thu, 2023-02-09 at 10:19 +0000, Usama Arif wrote:
> 
> 
> On 09/02/2023 10:06, David Woodhouse wrote:
> > On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
> > > 
> > > --- a/arch/x86/kernel/smpboot.c
> > > +++ b/arch/x86/kernel/smpboot.c
> > > @@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long
> > > start_ip, int apicid,
> > >                                             wakeup_cpu0_nmi, 0, "wake_cpu0");
> > > 
> > >           if (!boot_error) {
> > > +               initial_gs = per_cpu_offset(cpu);
> > 
> > That's for 64-bit.
> > 
> > >                   enable_start_cpu0 = 1;
> > >                   *cpu0_nmi_registered = 1;
> > >                   id = apic->dest_mode_logical ? cpu0_logical_apicid :
> > > apicid;
> > > @@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu,
> > > struct task_struct *idle,
> > >                   boot_error = apic->wakeup_secondary_cpu_64(apicid,
> > > start_ip);
> > >           else if (apic->wakeup_secondary_cpu)
> > >                   boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
> > > -       else
> > > +       else {
> > > +               if(!cpu) {
> > > +                       early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> > > +                       initial_stack  = idle->thread.sp;
> > > +               }
> > 
> > And that's 32-bit.
> > 
> > These were previously done in common_cpu_up() or do_boot_cpu(), which
> > means they were done not only for the wakeup_cpu_via_init_nmi() code
> > path, but also when we call apic->wakeup_secondary_cpu() (or _64()) for
> > the esoteric UV/NumaConnect machines.
> > 
> > Thanks for diagnosing it so quickly; I'll work up a subtly different
> > fix.
> > 
> 
> Yeah, was just a hacky fix while I was trying to figure out the issue.
> 
> do_boot_cpu is only called in cpu0 in hotplug case, so I think this a
> much better diff:
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 3ec5182d9698..f7ae82969ee4 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1136,13 +1136,16 @@ static int do_boot_cpu(int apicid, int cpu, 
> struct task_struct *idle,
>          idle->thread.sp = (unsigned long)task_pt_regs(idle);
>          initial_code = (unsigned long)start_secondary;
> 
> -       if (IS_ENABLED(CONFIG_X86_32)) {
> +       if (IS_ENABLED(CONFIG_X86_32) || !cpu) {
>                  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;
>          }
> 
> +       if (!cpu)
> +               initial_gs = per_cpu_offset(cpu);
> +
>          /* Enable the espfix hack for this CPU */
>          init_espfix_ap(cpu);
> 
> 
> 
> > >                   boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
> > >                                                        cpu0_nmi_registered);
> > > -
> > > +       }
> > >           return boot_error;
> > >    }
> > > 
> > 


I'm trying to find the actual startup path that CPU0 takes when woken
by an NMI.

Can't we make it take the secondary_startup_64 path that actually
checks smpboot_control, sees the STARTUP_SECONDARY flag set, and then
gets all these things from the per-cpu data as $DEITY (well, Thomas)
intended?


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

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
> 
> Its easy to test, just by doing
> echo 0 > /sys/devices/system/cpu/cpu0/online;
> echo 1 > /sys/devices/system/cpu/cpu0/online;

This one also fixes it for me. If we're happy with this approach, I'll
work it into Thomas's original patch (and hopefully eventually he'll be
happy enough with it and the commit message that he'll give us his
Signed-off-by for it.)


I could probably add a Co-developed-by: tglx for that first x2apic
patch in the series too, but then it would *also* need his SoB and I
didn't want to be owed two, so I just pasted his suggested code and
didn't credit him.


diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5462464fe3ef..ea6052a97619 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -450,7 +450,16 @@ 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, %rcx
+       addq    %rbx, %rcx
+       movq    (%rcx), %rcx
+       movq    TASK_threadsp(%rcx), %rsp
+
        jmp     .Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif

I cut and pasted some of that, I'm not entirely sure why we have three
instructions to do the equivalent of 'movq idle_threads(%ebx), %ecx'
and may fix that in the original as I work this in.

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

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[Usama Arif]

On 09/02/2023 11:03, David Woodhouse wrote:
> On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
>>
>> Its easy to test, just by doing
>> echo 0 > /sys/devices/system/cpu/cpu0/online;
>> echo 1 > /sys/devices/system/cpu/cpu0/online;
> 
> This one also fixes it for me. If we're happy with this approach, I'll
> work it into Thomas's original patch (and hopefully eventually he'll be
> happy enough with it and the commit message that he'll give us his
> Signed-off-by for it.)
> 

Yes, I think its better!

> 
> I could probably add a Co-developed-by: tglx for that first x2apic
> patch in the series too, but then it would *also* need his SoB and I
> didn't want to be owed two, so I just pasted his suggested code and
> didn't credit him.
> 
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5462464fe3ef..ea6052a97619 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -450,7 +450,16 @@ 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, %rcx
> +       addq    %rbx, %rcx
> +       movq    (%rcx), %rcx
> +       movq    TASK_threadsp(%rcx), %rsp
> +
>          jmp     .Ljump_to_C_code
>   SYM_CODE_END(start_cpu0)
>   #endif
> 
> I cut and pasted some of that, I'm not entirely sure why we have three
> instructions to do the equivalent of 'movq idle_threads(%ebx), %ecx'
> and may fix that in the original as I work this in.

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[Thomas Gleixner]

On Thu, Feb 09 2023 at 11:03, David Woodhouse wrote:
> This one also fixes it for me. If we're happy with this approach, I'll
> work it into Thomas's original patch (and hopefully eventually he'll be
> happy enough with it and the commit message that he'll give us his
> Signed-off-by for it.)

I'm happy enough by now, but I'm not sure how much of the original patch
is still left. Also you did the heavy lifting of making it work and
writing the nice changelog. So please make this:

From: David Woodhouse <dwmw2@infradead.org>

Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>

> I could probably add a Co-developed-by: tglx for that first x2apic
> patch in the series too, but then it would *also* need his SoB and I
> didn't want to be owed two, so I just pasted his suggested code and
> didn't credit him.

That's what Suggested-by: is for. For that I don't owe you anything. :)

Thanks

        tglx

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Thu, 2023-02-09 at 12:53 +0100, Thomas Gleixner wrote:
> On Thu, Feb 09 2023 at 11:03, David Woodhouse wrote:
> > This one also fixes it for me. If we're happy with this approach, I'll
> > work it into Thomas's original patch (and hopefully eventually he'll be
> > happy enough with it and the commit message that he'll give us his
> > Signed-off-by for it.)
> 
> I'm happy enough by now, but I'm not sure how much of the original patch
> is still left. Also you did the heavy lifting of making it work and
> writing the nice changelog. So please make this:
> 
> From: David Woodhouse <dwmw2@infradead.org>
> 
> Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

Thanks. I'll flip that to the Amazon address, of course. It's useless
for actual email (until I apply that LART some more) but I should still
use it for that.

I think I'm going to make one more change to that as I review it; in
the "should never happen" case of not finding the APIC ID in the
cpuid_to_apicid[] array it would just keep searching for ever. I don't
know if there's a better thing to do other than just dropping the
trampoline lock and 1:cli;hlt;jmp 1b but at least it's *attempting* to
handle the failure.

Patch below, and I'll update the tree shortly. There's a "what if
there's noise in the top 32 bits of %rcx" fix in there too.

> > I could probably add a Co-developed-by: tglx for that first x2apic
> > patch in the series too, but then it would *also* need his SoB and I
> > didn't want to be owed two, so I just pasted his suggested code and
> > didn't credit him.
> 
> That's what Suggested-by: is for. For that I don't owe you anything. :)

Well, I didn't think that was really for suggestions which come in
'diff -up' form, but OK :)

--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -280,15 +280,25 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
        cpuid
 
 .Lsetup_AP:
-       /* EDX contains the APICID of the current CPU */
-       xorl    %ecx, %ecx
+       /* 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     %rcx
-       jmp     .Lfind_cpunr
+       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 */
@@ -303,9 +313,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
        movq    %rcx, early_gdt_descr_base(%rip)
 
        /* Find the idle task stack */
-       movq    $idle_threads, %rcx
-       addq    %rbx, %rcx
-       movq    (%rcx), %rcx
+       movq    idle_threads(%rbx), %rcx
        movq    TASK_threadsp(%rcx), %rcx
        movq    %rcx, initial_stack(%rip)
 #endif /* CONFIG_SMP */
@@ -450,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


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

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

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

On Thu, 2023-02-09 at 12:10 +0000, David Woodhouse wrote:
> On Thu, 2023-02-09 at 12:53 +0100, Thomas Gleixner wrote:
> > On Thu, Feb 09 2023 at 11:03, David Woodhouse wrote:
> > > This one also fixes it for me. If we're happy with this approach, I'll
> > > work it into Thomas's original patch (and hopefully eventually he'll be
> > > happy enough with it and the commit message that he'll give us his
> > > Signed-off-by for it.)
> > 
> > I'm happy enough by now, but I'm not sure how much of the original patch
> > is still left. Also you did the heavy lifting of making it work and
> > writing the nice changelog. So please make this:
> > 
> > From: David Woodhouse <dwmw2@infradead.org>
> > 
> > Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 
> Thanks. I'll flip that to the Amazon address, of course. It's useless
> for actual email (until I apply that LART some more) but I should still
> use it for that.
> 
> I think I'm going to make one more change to that as I review it; in
> the "should never happen" case of not finding the APIC ID in the
> cpuid_to_apicid[] array it would just keep searching for ever. I don't
> know if there's a better thing to do other than just dropping the
> trampoline lock and 1:cli;hlt;jmp 1b but at least it's *attempting* to
> handle the failure.
> 
> Patch below, and I'll update the tree shortly. There's a "what if
> there's noise in the top 32 bits of %rcx" fix in there too.

All done and pushed out to parallel-6.2-rc7-part1 (and not -part1)
branches. Usama, are you able to redo the testing and take it from
here? Thanks for that; it's saving me a lot of time!


I'm mostly done for the week now as by this time tomorrow, I need to
have the skis on the roof of the car and be ready to pick the family up
from school and start driving south...

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

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[Usama Arif]

On 09/02/2023 13:48, David Woodhouse wrote:
> On Thu, 2023-02-09 at 12:10 +0000, David Woodhouse wrote:
>> On Thu, 2023-02-09 at 12:53 +0100, Thomas Gleixner wrote:
>>> On Thu, Feb 09 2023 at 11:03, David Woodhouse wrote:
>>>> This one also fixes it for me. If we're happy with this approach, I'll
>>>> work it into Thomas's original patch (and hopefully eventually he'll be
>>>> happy enough with it and the commit message that he'll give us his
>>>> Signed-off-by for it.)
>>>
>>> I'm happy enough by now, but I'm not sure how much of the original patch
>>> is still left. Also you did the heavy lifting of making it work and
>>> writing the nice changelog. So please make this:
>>>
>>> From: David Woodhouse <dwmw2@infradead.org>
>>>
>>> Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
>>
>> Thanks. I'll flip that to the Amazon address, of course. It's useless
>> for actual email (until I apply that LART some more) but I should still
>> use it for that.
>>
>> I think I'm going to make one more change to that as I review it; in
>> the "should never happen" case of not finding the APIC ID in the
>> cpuid_to_apicid[] array it would just keep searching for ever. I don't
>> know if there's a better thing to do other than just dropping the
>> trampoline lock and 1:cli;hlt;jmp 1b but at least it's *attempting* to
>> handle the failure.
>>
>> Patch below, and I'll update the tree shortly. There's a "what if
>> there's noise in the top 32 bits of %rcx" fix in there too.
> 
> All done and pushed out to parallel-6.2-rc7-part1 (and not -part1)
> branches. Usama, are you able to redo the testing and take it from
> here? Thanks for that; it's saving me a lot of time!
> 
Thanks! Will retest and repost now!

Usama

> 
> I'm mostly done for the week now as by this time tomorrow, I need to
> have the skis on the roof of the car and be ready to pick the family up
> from school and start driving south...

Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[Thomas Gleixner]

On Thu, Feb 09 2023 at 12:10, David Woodhouse wrote:
> On Thu, 2023-02-09 at 12:53 +0100, Thomas Gleixner wrote:
>> > I could probably add a Co-developed-by: tglx for that first x2apic
>> > patch in the series too, but then it would *also* need his SoB and I
>> > didn't want to be owed two, so I just pasted his suggested code and
>> > didn't credit him.
>> 
>> That's what Suggested-by: is for. For that I don't owe you anything. :)
>
> Well, I didn't think that was really for suggestions which come in
> 'diff -up' form, but OK :)

I can't even remember, but probably diff -up was way faster than writing
a novel :)

Thanks,

        tglx