[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 3/9] x86/smp: drop x86_cpu_to_apicid, use cpu_data[cpu].apicid instead
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, >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |