[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 1/9] x86/boot: choose AP stack based on APIC ID
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |