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

[Krystian Hebel <krystian.hebel@xxxxxxxxx>]

Hi,

On 7.02.2024 17:41, Jan Beulich wrote:

> 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).
>     
    Will do. This may even result in squashing some patches together.


> > > --- 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.
>     
    You mean s/cpu_data[]/cpu_physical_id()/ or something else?

> > > + * 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.
>     
    Yes, this was because after adding APIC ID to this structure I tried
    to use
    CPUINFO_sizeof in the assembly, and bad things happened.
> 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,
> >
> >       
>     
    Best regards,
-- 
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com