Re: [XEN PATCH 4/9] x86/smp: move stack_base to cpu_data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2023 18:50, Krystian Hebel wrote:
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -33,9 +33,8 @@ ENTRY(__high_start)
>          cmp     %esp, CPUINFO_X86_apicid(%rcx)
>          jne     1b
>  
> -        /* %eax is now Xen CPU index. */
> -        lea     stack_base(%rip), %rcx
> -        mov     (%rcx, %rax, 8), %rsp
> +        /* %rcx is now cpu_data[cpu], read stack base from it. */
> +        mov     CPUINFO_X86_stack_base(%rcx), %rsp

Looks like you're not using the value in %eax anymore? If so, respective
code would want dropping. Which in turn again raises the question that
Julien already put up: By re-ordering the series, can't you avoid
altering the same code multiple times, in part even removing in a later
patch what an earlier one added?

That said, I remain unconvinced that ...

> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -37,6 +37,7 @@ struct cpuinfo_x86 {
>      unsigned int phys_proc_id;         /* package ID of each logical CPU */
>      unsigned int cpu_core_id;          /* core ID of each logical CPU */
>      unsigned int compute_unit_id;      /* AMD compute unit ID of each 
> logical CPU */
> +    void *stack_base;
>      unsigned short x86_clflush_size;
>  } __cacheline_aligned;

... this is a good place for the new data: As indicated before, it
doesn't fit (in nature) with everything else in this struct.

Additionally no matter where the data is put, I'd wonder if it
wouldn't better be const void *. You don't mean to ever write
through it, I suppose.

> @@ -1156,7 +1156,8 @@ void __init smp_prepare_cpus(void)
>      boot_cpu_physical_apicid = get_apic_id();
>      cpu_physical_id(0) = boot_cpu_physical_apicid;
>  
> -    stack_base[0] = (void *)((unsigned long)stack_start & ~(STACK_SIZE - 1));
> +    cpu_data[0].stack_base = (void *)
> +             ((unsigned long)stack_start & ~(STACK_SIZE - 1));

Nit: Too deep indentation. Each indentation level is 4 spaces. I also
think the cast would then also want to move on the 2nd line, such that
(see again ./CODING_STYLE) the assignment operator is last on the 1st
line.

Jan