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
