Re: [XEN PATCH 3/9] x86/smp: drop x86_cpu_to_apicid, use cpu_data[cpu].apicid instead

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2024 19:11, Julien Grall wrote:
> Hi,
> 
> On 14/11/2023 17:50, Krystian Hebel wrote:
>> Both fields held the same data.
>>
>> Signed-off-by: Krystian Hebel <krystian.hebel@xxxxxxxxx>
>> ---
>>   xen/arch/x86/boot/x86_64.S           |  8 +++++---
>>   xen/arch/x86/include/asm/asm_defns.h |  2 +-
>>   xen/arch/x86/include/asm/processor.h |  2 ++
>>   xen/arch/x86/include/asm/smp.h       |  4 ----
>>   xen/arch/x86/numa.c                  | 15 +++++++--------
>>   xen/arch/x86/smpboot.c               |  8 ++++----
>>   xen/arch/x86/x86_64/asm-offsets.c    |  4 +++-
>>   7 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
>> index b85b47b5c1a0..195550b5c0ea 100644
>> --- a/xen/arch/x86/boot/x86_64.S
>> +++ b/xen/arch/x86/boot/x86_64.S
>> @@ -20,15 +20,17 @@ ENTRY(__high_start)
>>           jz      .L_stack_set
>>   
>>           /* APs only: get stack base from APIC ID saved in %esp. */
>> -        mov     $-1, %rax
>> -        lea     x86_cpu_to_apicid(%rip), %rcx
>> +        mov     $0, %rax
>> +        lea     cpu_data(%rip), %rcx
>> +        /* cpu_data[0] is BSP, skip it. */
>>   1:
>>           add     $1, %rax
>> +        add     $CPUINFO_X86_sizeof, %rcx
>>           cmp     $NR_CPUS, %eax
>>           jb      2f
>>           hlt
>>   2:
>> -        cmp     %esp, (%rcx, %rax, 4)
>> +        cmp     %esp, CPUINFO_X86_apicid(%rcx)
>>           jne     1b
>>   
>>           /* %eax is now Xen CPU index. */
> 
> As mentioned in an earlier patch, I think you want to re-order the 
> patches. This will avoid to modify twice the same code within the same 
> series (it is best to avoid if you can).

I second this request. Even more so that there's an unexplained move
from starting at $-1 to starting at $0 (in which case you really want
to use xor, not mov).

>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -54,14 +54,13 @@ bool __init arch_numa_unavailable(void)
>>   /*
>>    * Setup early cpu_to_node.
>>    *
>> - * Populate cpu_to_node[] only if x86_cpu_to_apicid[],
>> - * and apicid_to_node[] tables have valid entries for a CPU.
>> - * This means we skip cpu_to_node[] initialisation for NUMA
>> - * emulation and faking node case (when running a kernel compiled
>> - * for NUMA on a non NUMA box), which is OK as cpu_to_node[]
>> - * is already initialized in a round robin manner at numa_init_array,
>> - * prior to this call, and this initialization is good enough
>> - * for the fake NUMA cases.
>> + * Populate cpu_to_node[] only if cpu_data[], and apicid_to_node[]

You mean cpu_physical_id() here, and then this change wants doing when
switching to that, imo.

>> + * tables have valid entries for a CPU. This means we skip
>> + * cpu_to_node[] initialisation for NUMA emulation and faking node
>> + * case (when running a kernel compiled for NUMA on a non NUMA box),
>> + * which is OK as cpu_to_node[] is already initialized in a round
>> + * robin manner at numa_init_array, prior to this call, and this
>> + * initialization is good enough for the fake NUMA cases.
>>    */

Also if you're already re-wrapping this comment, please make better use
of line width.

>> --- a/xen/arch/x86/x86_64/asm-offsets.c
>> +++ b/xen/arch/x86/x86_64/asm-offsets.c
>> @@ -159,7 +159,9 @@ void __dummy__(void)
>>       OFFSET(IRQSTAT_softirq_pending, irq_cpustat_t, __softirq_pending);
>>       BLANK();
>>   
>> -    OFFSET(CPUINFO_features, struct cpuinfo_x86, x86_capability);
>> +    OFFSET(CPUINFO_X86_features, struct cpuinfo_x86, x86_capability);
> 
> The rename seems to be unrelated to this patch. Can you clarify?

I agree some renaming wants doing, but separately. That's because we
use CPUINFO_ as a prefix for two entirely different structure's offsets
right now. I'm not convinced of CPUINFO_X86_ as the new prefix though:
Uses are against cpu_data[], so CPUDATA_ may be better. Might be good
if Andrew and/or Roger could voice their view.

Jan

>> +    OFFSET(CPUINFO_X86_apicid, struct cpuinfo_x86, apicid);
>> +    DEFINE(CPUINFO_X86_sizeof, sizeof(struct cpuinfo_x86));
>>       BLANK();
>>   
>>       OFFSET(MB_flags, multiboot_info_t, flags);
> 
> Cheers,
>