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
