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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.03.2024 16:11, Krystian Hebel wrote:
> 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
>>> +        /* 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?

Preferably, yes.

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

Conversion takes time, so in new code we aim at having those blanks.
Over time we hope to have them nearly everywhere, at which point it
may make sense to to a final cleanup sweep.

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

Ah, right. Yet better error checking / reporting is going to be needed
anyway.

Jan