| Message ID | 22109ebd7edef1140cb438a6ec5fa1726cdf2c12.1699982111.git.krystian.hebel@3mdeb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: parallelize AP bring-up during boot | expand |
On 14.11.2023 18:50, Krystian Hebel wrote: > CPU id is obtained as a side effect of searching for appropriate > stack for AP. It can be used as a parameter to start_secondary(). > Coincidentally this also makes further work on making AP bring-up > code parallel easier. It's not just "easier", but strictly a prereq I think? Such a global would get in the way of having multiple CPUs make it into start_secondary() in parallel. > --- a/xen/arch/x86/boot/x86_64.S > +++ b/xen/arch/x86/boot/x86_64.S > @@ -20,20 +20,24 @@ ENTRY(__high_start) > jz .L_stack_set > > /* APs only: get stack base from APIC ID saved in %esp. */ > - mov $0, %rax > + mov $0, %rbx > lea cpu_data(%rip), %rcx > /* cpu_data[0] is BSP, skip it. */ > 1: > - add $1, %rax > + add $1, %rbx > add $CPUINFO_X86_sizeof, %rcx > - cmp $NR_CPUS, %eax > + cmp $NR_CPUS, %rbx > jb 2f > hlt > 2: > cmp %esp, CPUINFO_X86_apicid(%rcx) > jne 1b Once again this is code you introduced a few patches ago. Why not use %ebx right away for that purpose? (And yes, this explains why in the earlier patch you retained that code. Just that again suitably ordering the series would make this look natural. Otherwise it needs at least mentioning why dead pieces are kept around.) > - /* %rcx is now cpu_data[cpu], read stack base from it. */ > + /* > + * At this point: > + * - %rcx is cpu_data[cpu], read stack base from it, > + * - %rbx (callee-save) is Xen cpu number, pass it to start_secondary(). > + */ > mov CPUINFO_X86_stack_base(%rcx), %rsp > > test %rsp,%rsp > @@ -101,6 +105,7 @@ ENTRY(__high_start) > .L_ap_cet_done: > #endif /* CONFIG_XEN_SHSTK || CONFIG_XEN_IBT */ > > + mov %rbx, %rdi > tailcall start_secondary As alluded to above and as mentioned before - please stick to 32-bit operations when you deal with 32 (or less) bits of data. > --- a/xen/arch/x86/smpboot.c > +++ b/xen/arch/x86/smpboot.c > @@ -222,8 +222,6 @@ static void smp_callin(void) > cpu_relax(); > } > > -static int booting_cpu; > - > /* CPUs for which sibling maps can be computed. */ > static cpumask_t cpu_sibling_setup_map; > > @@ -311,15 +309,14 @@ static void set_cpu_sibling_map(unsigned int cpu) > } > } > > -void start_secondary(void *unused) > +void start_secondary(unsigned int cpu) > { > struct cpu_info *info = get_cpu_info(); > > /* > - * Dont put anything before smp_callin(), SMP booting is so fragile that we > + * Don't put anything before smp_callin(), SMP booting is so fragile that we > * want to limit the things done here to the most necessary things. > */ > - unsigned int cpu = booting_cpu; > > /* Critical region without IDT or TSS. Any fault is deadly! */ > > @@ -346,9 +343,9 @@ void start_secondary(void *unused) > */ > spin_debug_disable(); > > - get_cpu_info()->use_pv_cr3 = false; > - get_cpu_info()->xen_cr3 = 0; > - get_cpu_info()->pv_cr3 = 0; > + info->use_pv_cr3 = false; > + info->xen_cr3 = 0; > + info->pv_cr3 = 0; This hunk looks unrelated. While tidying next to what's changed anyway may be okay if suitably mentioned in the description, in this case I think it needs splitting off. Jan
On 8.02.2024 12:39, Jan Beulich wrote: > On 14.11.2023 18:50, Krystian Hebel wrote: >> CPU id is obtained as a side effect of searching for appropriate >> stack for AP. It can be used as a parameter to start_secondary(). >> Coincidentally this also makes further work on making AP bring-up >> code parallel easier. > It's not just "easier", but strictly a prereq I think? Such a global > would get in the way of having multiple CPUs make it into > start_secondary() in parallel. start_secondary() could also repeat what was done in assembly to get that ID. This was what I've done in one of first attempts, and the commit messagewas about removing that lookup loop which was squashed along the way and no longer applies. I'll reword it a bit. >> --- a/xen/arch/x86/boot/x86_64.S >> +++ b/xen/arch/x86/boot/x86_64.S >> @@ -20,20 +20,24 @@ ENTRY(__high_start) >> jz .L_stack_set >> >> /* APs only: get stack base from APIC ID saved in %esp. */ >> - mov $0, %rax >> + mov $0, %rbx >> lea cpu_data(%rip), %rcx >> /* cpu_data[0] is BSP, skip it. */ >> 1: >> - add $1, %rax >> + add $1, %rbx >> add $CPUINFO_X86_sizeof, %rcx >> - cmp $NR_CPUS, %eax >> + cmp $NR_CPUS, %rbx >> jb 2f >> hlt >> 2: >> cmp %esp, CPUINFO_X86_apicid(%rcx) >> jne 1b > Once again this is code you introduced a few patches ago. Why not use > %ebx right away for that purpose? (And yes, this explains why in the > earlier patch you retained that code. Just that again suitably ordering > the series would make this look natural. Otherwise it needs at least > mentioning why dead pieces are kept around.) Will do. > >> - /* %rcx is now cpu_data[cpu], read stack base from it. */ >> + /* >> + * At this point: >> + * - %rcx is cpu_data[cpu], read stack base from it, >> + * - %rbx (callee-save) is Xen cpu number, pass it to start_secondary(). >> + */ >> mov CPUINFO_X86_stack_base(%rcx), %rsp >> >> test %rsp,%rsp >> @@ -101,6 +105,7 @@ ENTRY(__high_start) >> .L_ap_cet_done: >> #endif /* CONFIG_XEN_SHSTK || CONFIG_XEN_IBT */ >> >> + mov %rbx, %rdi >> tailcall start_secondary > As alluded to above and as mentioned before - please stick to 32-bit > operations when you deal with 32 (or less) bits of data. Ack > >> --- a/xen/arch/x86/smpboot.c >> +++ b/xen/arch/x86/smpboot.c >> @@ -222,8 +222,6 @@ static void smp_callin(void) >> cpu_relax(); >> } >> >> -static int booting_cpu; >> - >> /* CPUs for which sibling maps can be computed. */ >> static cpumask_t cpu_sibling_setup_map; >> >> @@ -311,15 +309,14 @@ static void set_cpu_sibling_map(unsigned int cpu) >> } >> } >> >> -void start_secondary(void *unused) >> +void start_secondary(unsigned int cpu) >> { >> struct cpu_info *info = get_cpu_info(); >> >> /* >> - * Dont put anything before smp_callin(), SMP booting is so fragile that we >> + * Don't put anything before smp_callin(), SMP booting is so fragile that we >> * want to limit the things done here to the most necessary things. >> */ >> - unsigned int cpu = booting_cpu; >> >> /* Critical region without IDT or TSS. Any fault is deadly! */ >> >> @@ -346,9 +343,9 @@ void start_secondary(void *unused) >> */ >> spin_debug_disable(); >> >> - get_cpu_info()->use_pv_cr3 = false; >> - get_cpu_info()->xen_cr3 = 0; >> - get_cpu_info()->pv_cr3 = 0; >> + info->use_pv_cr3 = false; >> + info->xen_cr3 = 0; >> + info->pv_cr3 = 0; > This hunk looks unrelated. While tidying next to what's changed anyway > may be okay if suitably mentioned in the description, in this case I > think it needs splitting off. OK, I'll split it off. > > Jan Best regards,
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index 8d61f270761f..ad01f20d548d 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -20,20 +20,24 @@ ENTRY(__high_start) jz .L_stack_set /* APs only: get stack base from APIC ID saved in %esp. */ - mov $0, %rax + mov $0, %rbx lea cpu_data(%rip), %rcx /* cpu_data[0] is BSP, skip it. */ 1: - add $1, %rax + add $1, %rbx add $CPUINFO_X86_sizeof, %rcx - cmp $NR_CPUS, %eax + cmp $NR_CPUS, %rbx jb 2f hlt 2: cmp %esp, CPUINFO_X86_apicid(%rcx) jne 1b - /* %rcx is now cpu_data[cpu], read stack base from it. */ + /* + * At this point: + * - %rcx is cpu_data[cpu], read stack base from it, + * - %rbx (callee-save) is Xen cpu number, pass it to start_secondary(). + */ mov CPUINFO_X86_stack_base(%rcx), %rsp test %rsp,%rsp @@ -101,6 +105,7 @@ ENTRY(__high_start) .L_ap_cet_done: #endif /* CONFIG_XEN_SHSTK || CONFIG_XEN_IBT */ + mov %rbx, %rdi tailcall start_secondary .L_bsp: diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index a3895dafa267..39ffd356dbbc 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -222,8 +222,6 @@ static void smp_callin(void) cpu_relax(); } -static int booting_cpu; - /* CPUs for which sibling maps can be computed. */ static cpumask_t cpu_sibling_setup_map; @@ -311,15 +309,14 @@ static void set_cpu_sibling_map(unsigned int cpu) } } -void start_secondary(void *unused) +void start_secondary(unsigned int cpu) { struct cpu_info *info = get_cpu_info(); /* - * Dont put anything before smp_callin(), SMP booting is so fragile that we + * Don't put anything before smp_callin(), SMP booting is so fragile that we * want to limit the things done here to the most necessary things. */ - unsigned int cpu = booting_cpu; /* Critical region without IDT or TSS. Any fault is deadly! */ @@ -346,9 +343,9 @@ void start_secondary(void *unused) */ spin_debug_disable(); - get_cpu_info()->use_pv_cr3 = false; - get_cpu_info()->xen_cr3 = 0; - get_cpu_info()->pv_cr3 = 0; + info->use_pv_cr3 = false; + info->xen_cr3 = 0; + info->pv_cr3 = 0; /* * BUG_ON() used in load_system_tables() and later code may end up calling @@ -575,8 +572,6 @@ static int do_boot_cpu(int apicid, int cpu) */ mtrr_save_state(); - booting_cpu = cpu; - start_eip = bootsym_phys(trampoline_realmode_entry); /* start_eip needs be page aligned, and below the 1M boundary. */
CPU id is obtained as a side effect of searching for appropriate stack for AP. It can be used as a parameter to start_secondary(). Coincidentally this also makes further work on making AP bring-up code parallel easier. Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> --- xen/arch/x86/boot/x86_64.S | 13 +++++++++---- xen/arch/x86/smpboot.c | 15 +++++---------- 2 files changed, 14 insertions(+), 14 deletions(-)