Re: [PATCH v2, 5/5] PowerPC/mpc85xx: Add CPU hotplug support for E6500
From: Scott Wood <hidden>
Date: 2015-08-26 22:42:26
Also in:
lkml
On Wed, Aug 26, 2015 at 08:09:48PM +0800, Chenhui Zhao wrote:
+ .globl booting_thread_hwid +booting_thread_hwid: + .long INVALID_THREAD_HWID + .align 3
The commit message goes into no detail about the changes you're making to thread handling, nor are there relevant comments.
+/* + * r3 = the thread physical id + */ +_GLOBAL(book3e_stop_thread) + li r4, 1 + sld r4, r4, r3 + mtspr SPRN_TENC, r4 + isync + blr
Why did the C code not have an isync, if it's required here?
quoted hunk ↗ jump to hunk
_GLOBAL(fsl_secondary_thread_init) /* Enable branch prediction */ lis r3,BUCSR_INIT@h@@ -197,8 +236,10 @@ _GLOBAL(fsl_secondary_thread_init) * but the low bit right by two bits so that the cpu numbering is * continuous. */ - mfspr r3, SPRN_PIR - rlwimi r3, r3, 30, 2, 30 + bl 10f +10: mflr r5 + addi r5,r5,(booting_thread_hwid - 10b) + lwz r3,0(r5) mtspr SPRN_PIR, r3 #endif
I assume the reason for this is that, unlike the kexec case, the cpu has been reset so PIR has been reset? Don't make me guess -- document.
quoted hunk ↗ jump to hunk
@@ -245,6 +286,30 @@ _GLOBAL(generic_secondary_smp_init) mr r3,r24 mr r4,r25 bl book3e_secondary_core_init + +/* + * If we want to boot Thread1, start Thread1 and stop Thread0. + * Note that only Thread0 will run the piece of code. + */
What ensures that only thread 0 runs this? Especially if we're entering via kdump on thread 1? s/the piece/this piece/
+ LOAD_REG_ADDR(r3, booting_thread_hwid) + lwz r4, 0(r3) + cmpwi r4, INVALID_THREAD_HWID + beq 20f + cmpw r4, r24 + beq 20f
Do all cores get released from the spin table before the first thread gets kicked?
quoted hunk ↗ jump to hunk
+ + /* start Thread1 */ + LOAD_REG_ADDR(r5, fsl_secondary_thread_init) + ld r4, 0(r5) + li r3, 1 + bl book3e_start_thread + + /* stop Thread0 */ + li r3, 0 + bl book3e_stop_thread +10: + b 10b +20: #endif generic_secondary_common_init:diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c index 73eb994..61f68ad 100644 --- a/arch/powerpc/platforms/85xx/smp.c +++ b/arch/powerpc/platforms/85xx/smp.c@@ -181,17 +181,11 @@ static inline u32 read_spin_table_addr_l(void *spin_table) static void wake_hw_thread(void *info) { void fsl_secondary_thread_init(void); - unsigned long imsr1, inia1; - int nr = *(const int *)info; + unsigned long inia; + int hw_cpu = get_hard_smp_processor_id(*(const int *)info); - imsr1 = MSR_KERNEL; - inia1 = *(unsigned long *)fsl_secondary_thread_init; - - mttmr(TMRN_IMSR1, imsr1); - mttmr(TMRN_INIA1, inia1); - mtspr(SPRN_TENS, TEN_THREAD(1)); - - smp_generic_kick_cpu(nr); + inia = *(unsigned long *)fsl_secondary_thread_init; + book3e_start_thread(cpu_thread_in_core(hw_cpu), inia); } #endif@@ -279,7 +273,6 @@ static int smp_85xx_kick_cpu(int nr) int ret = 0; #ifdef CONFIG_PPC64 int primary = nr; - int primary_hw = get_hard_smp_processor_id(primary); #endif WARN_ON(nr < 0 || nr >= num_possible_cpus());@@ -287,33 +280,43 @@ static int smp_85xx_kick_cpu(int nr) pr_debug("kick CPU #%d\n", nr); #ifdef CONFIG_PPC64 + booting_thread_hwid = INVALID_THREAD_HWID; /* Threads don't use the spin table */ - if (cpu_thread_in_core(nr) != 0) { - int primary = cpu_first_thread_sibling(nr); + if (threads_per_core == 2) { + booting_thread_hwid = get_hard_smp_processor_id(nr);
What does setting booting_thread_hwid to INVALID_THREAD_HWID here accomplish? If threads_per_core != 2 it would never have been set to anything else, and if threads_per_core == 2 you immediately overwrite it.
+ primary = cpu_first_thread_sibling(nr);
if (WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_SMT)))
return -ENOENT;
- if (cpu_thread_in_core(nr) != 1) {
- pr_err("%s: cpu %d: invalid hw thread %d\n",
- __func__, nr, cpu_thread_in_core(nr));
- return -ENOENT;
- }
-
- if (!cpu_online(primary)) {
- pr_err("%s: cpu %d: primary %d not online\n",
- __func__, nr, primary);
- return -ENOENT;
+ /*
+ * If either one of threads in the same core is online,
+ * use the online one to start the other.
+ */
+ if (qoriq_pm_ops)
+ qoriq_pm_ops->cpu_up_prepare(nr);cpu_up_prepare does rcpm_v2_cpu_exit_state(cpu, E500_PM_PH20). How do you know the cpu is already in PH20? What if this is initial boot? Are you relying on it being a no-op in that case?
+
+ if (cpu_online(primary)) {
+ smp_call_function_single(primary,
+ wake_hw_thread, &nr, 1);
+ goto done;
+ } else if (cpu_online(primary + 1)) {
+ smp_call_function_single(primary + 1,
+ wake_hw_thread, &nr, 1);
+ goto done;
}
- smp_call_function_single(primary, wake_hw_thread, &nr, 0);
- return 0;
+ /* If both threads are offline, continue to star primary cpu */s/star/start/
+ } else if (threads_per_core > 2) {
+ pr_err("Do not support more than 2 threads per CPU.");WARN_ONCE(1, "More than 2 threads per core not supported: %d\n", threads_per_core); -Scott