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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

> +        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.

> +        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.

> +        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?

> +        cmp     $NR_CPUS, %eax
> +        jb      2f
> +        hlt
> +2:
> +        cmp     %esp, (%rcx, %rax, 4)
> +        jne     1b

Aren't you open-coding REPNE SCASD here?

> +        /* %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.

> --- 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.

Jan