Re: [XEN PATCH 7/9] x86/smp: drop booting_cpu variable

[Krystian Hebel <krystian.hebel@xxxxxxxxx>]

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
    message was 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,

-- 
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com