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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2024 19:24, Julien Grall wrote:
> On 14/11/2023 17:50, Krystian Hebel wrote:
>> This location is easier to access from assembly. Having it close to
>> other data required during initialization has also positive (although
>> rather small) impact on prefetching data from RAM.
> 
> I understand your goal but...
> 
>> --- a/xen/arch/x86/include/asm/cpufeature.h
>> +++ b/xen/arch/x86/include/asm/cpufeature.h
> 
> ... cpufeature.h feels a rather odd place for storing the stack. I am 
> not entirely sure where else to place. Andrew, Jan, Roger?

Well, without having looked at the patch/series itself yet, I can only
say that if struct cpuinfo_x86 really is the place to put this
information, then it's unavoidable to have the field added in this
header. That said, it certainly feels like an abuse - there's nothing
in common with other (collected) data in this structure. "Easier to
access from assembly" may be a fair reason, but then I'd expect the
downsides of alternatives to be discussed explicitly. For example, a
simple new array might be as "easily" accessible from assembly.

>> @@ -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;
> 
> AFAICT, this means there will be a padding before stack_base and ...
> 
>>       unsigned short x86_clflush_size;
> 
> ... another one here. Is there any particular reason the new field 
> wasn't added at the end?

With ...

>>   } __cacheline_aligned;

... this I'm not exactly sure this is a problem right now (there may
be ample padding space anyway, yet I didn't go count). But I agree
with your comment in principle.

>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -75,13 +75,15 @@ static enum cpu_state {
>>   } cpu_state;
>>   #define set_cpu_state(state) do { smp_mb(); cpu_state = (state); } while 
>> (0)
>>   
>> -void *stack_base[NR_CPUS];
>> -
>>   void initialize_cpu_data(unsigned int cpu)
>>   {
>>       uint32_t apicid = cpu_physical_id(cpu);
>> +    void *stack = cpu_data[cpu].stack_base;
>> +
>>       cpu_data[cpu] = boot_cpu_data;
>> +
>>       cpu_physical_id(cpu) = apicid;
>> +    cpu_data[cpu].stack_base = stack;
>>   }
>>   
>>   static bool smp_store_cpu_info(unsigned int id)
>> @@ -579,8 +581,6 @@ static int do_boot_cpu(int apicid, int cpu)
>>           printk("Booting processor %d/%d eip %lx\n",
>>                  cpu, apicid, start_eip);
>>   
>> -    stack_start = stack_base[cpu] + STACK_SIZE - sizeof(struct cpu_info);
>> -
> 
> You remove this line because I can't quite figure out where stack_start 
> is now set. This is used...

This line sets a global variable, which ...

>> @@ -856,7 +856,7 @@ int setup_cpu_root_pgt(unsigned int cpu)
>>   
>>       /* Install direct map page table entries for stack, IDT, and TSS. */
>>       for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
>> -        rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
>> +        rc = clone_mapping(__va(__pa(cpu_data[cpu].stack_base)) + off, rpt);
>>   
>>       if ( !rc )
>>           rc = clone_mapping(idt_tables[cpu], rpt);
>> @@ -1007,10 +1007,10 @@ static void cpu_smpboot_free(unsigned int cpu, bool 
>> remove)
>>           FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>>           FREE_XENHEAP_PAGE(idt_tables[cpu]);
>>   
>> -        if ( stack_base[cpu] )
>> +        if ( cpu_data[cpu].stack_base )
>>           {
>> -            memguard_unguard_stack(stack_base[cpu]);
>> -            FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
>> +            memguard_unguard_stack(cpu_data[cpu].stack_base);
>> +            FREE_XENHEAP_PAGES(cpu_data[cpu].stack_base, STACK_ORDER);
>>           }
>>       }
>>   }
>> @@ -1044,11 +1044,11 @@ static int cpu_smpboot_alloc(unsigned int cpu)
>>       if ( node != NUMA_NO_NODE )
>>           memflags = MEMF_node(node);
>>   
>> -    if ( stack_base[cpu] == NULL &&
>> -         (stack_base[cpu] = cpu_alloc_stack(cpu)) == NULL )
>> +    if ( cpu_data[cpu].stack_base == NULL &&
>> +         (cpu_data[cpu].stack_base = cpu_alloc_stack(cpu)) == NULL )
>>               goto out;
>>   
>> -    info = get_cpu_info_from_stack((unsigned long)stack_base[cpu]);
>> +    info = get_cpu_info_from_stack((unsigned long)cpu_data[cpu].stack_base);
> 
> ... here.

... pretty clearly is not used here (anymore). Instead I'd raise the
question of what the remaining purpose of that variable then is.
Looking through updates this patch alone makes to use sites of
stack_start, it's unclear whether the use from assembly code has gone
away already - brief checking suggests it hasn't.

Jan