Re: [XEN PATCH 1/9] x86/boot: choose AP stack based on APIC ID

[Krystian Hebel <krystian.hebel@xxxxxxxxx>]

On 7.02.2024 17:11, Jan Beulich wrote:

> On 14.11.2023 18:49, Krystian Hebel wrote:
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -72,6 +72,26 @@ trampoline_protmode_entry:
> >          mov     $X86_CR4_PAE,%ecx
> >          mov     %ecx,%cr4
> >  
> > +        /*
> > +         * Get APIC ID while we're in non-paged mode. Start by checking if
> > +         * x2APIC is enabled.
> > +         */
> > +        mov     $MSR_APIC_BASE, %ecx
> > +        rdmsr
> > +        and     $APIC_BASE_EXTD, %eax
> >       
> You don't use the result, in which case "test" is to be preferred over
> "and".
>     
    Ack
> > +        jnz     .Lx2apic
> > +
> > +        /* Not x2APIC, read from MMIO */
> > +        mov     0xfee00020, %esp
> >       
> Please don't open-code existing constants (APIC_ID here and below,
> APIC_DEFAULT_PHYS_BASE just here, and ...
>
> > +        shr     $24, %esp
> >       
> ... a to-be-introduced constant here (for {G,S}ET_xAPIC_ID() to use as
> well then). This is the only way of being able to easily identify all
> pieces of code accessing the same piece of hardware.
>

Yes, this was also caught in review done by Qubes OS team.

New constant and {G,S}ET_xAPIC_ID() should be in separate patch, I presume?

> > +        jmp     1f
> > +
> > +.Lx2apic:
> > +        mov     $(MSR_X2APIC_FIRST + (0x20 >> 4)), %ecx
> > +        rdmsr
> > +        mov     %eax, %esp
> > +1:
> >       
> Overall I'm worried of the use of %esp throughout here.
>
> > --- a/xen/arch/x86/boot/x86_64.S
> > +++ b/xen/arch/x86/boot/x86_64.S
> > @@ -15,7 +15,33 @@ ENTRY(__high_start)
> >          mov     $XEN_MINIMAL_CR4,%rcx
> >          mov     %rcx,%cr4
> >  
> > -        mov     stack_start(%rip),%rsp
> > +        test    %ebx,%ebx
> >       
> Nit (style): Elsewhere you have blanks after the commas, just here
> (and once again near the end of the hunk) you don't.
>     
    Is either style preferred? This file has both.

> > +        cmovz   stack_start(%rip), %rsp
> > +        jz      .L_stack_set
> > +
> > +        /* APs only: get stack base from APIC ID saved in %esp. */
> > +        mov     $-1, %rax
> >       
> Why a 64-bit insn here and ...
>
> > +        lea     x86_cpu_to_apicid(%rip), %rcx
> > +1:
> > +        add     $1, %rax
> >       
> ... here, when you only use (far less than) 32-bit values?
>     
    Fair question, no idea what I had in mind, will change.

> > +        cmp     $NR_CPUS, %eax
> > +        jb      2f
> > +        hlt
> > +2:
> > +        cmp     %esp, (%rcx, %rax, 4)
> > +        jne     1b
> >       
> Aren't you open-coding REPNE SCASD here?
>     
    Looks like I am, I missed that. I'm not sure if this will still
    apply after
    changes from further patches, but I'll keep that in mind.
> > +        /* %eax is now Xen CPU index. */
> > +        lea     stack_base(%rip), %rcx
> > +        mov     (%rcx, %rax, 8), %rsp
> > +
> > +        test    %rsp,%rsp
> > +        jnz     1f
> > +        hlt
> > +1:
> > +        add     $(STACK_SIZE - CPUINFO_sizeof), %rsp
> >       
> Even this post-adjustment is worrying me. Imo the stack pointer is
> better set exactly once, to its final value. Keeping it at its init
> value of 0 until then yields more predictable results in case it
> ends up being used somewhere.
>     
    It really shouldn't be used anywhere, but I'll change it.

> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1951,6 +1951,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >       */
> >      if ( !pv_shim )
> >      {
> > +        /* Separate loop to make parallel AP bringup possible. */
> >          for_each_present_cpu ( i )
> >          {
> >              /* Set up cpu_to_node[]. */
> > @@ -1958,6 +1959,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >              /* Set up node_to_cpumask based on cpu_to_node[]. */
> >              numa_add_cpu(i);
> >  
> > +            if ( stack_base[i] == NULL )
> > +                stack_base[i] = cpu_alloc_stack(i);
> > +        }
> >       
> Imo this wants accompanying by removal of the allocation in
> cpu_smpboot_alloc(). Which would then make more visible that there's
> error checking there, but not here (I realize there effectively is
> error checking in assembly code, but that'll end in HLT with no
> useful indication of what the problem is). Provided, as Julien has
> pointed out, that the change is necessary in the first place.
>

The allocation in cpu_smpboot_alloc() was left for hot-plug. This loops
over present CPUs, not possible ones.

> Jan
>     
    Best regards,

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