Mailing List Archive: [XEN PATCH 4/9] x86/smp: move stack_base to cpu

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

Nov 14, 2023, 9:50 AM

Post #1 of 6 (29 views)

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.

Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
---
xen/arch/x86/boot/x86_64.S | 5 ++---
xen/arch/x86/include/asm/cpufeature.h | 1 +
xen/arch/x86/include/asm/smp.h | 2 +-
xen/arch/x86/setup.c | 6 +++---
xen/arch/x86/smpboot.c | 25 +++++++++++++------------
xen/arch/x86/traps.c | 4 ++--
xen/arch/x86/x86_64/asm-offsets.c | 1 +
xen/include/xen/smp.h | 2 --
8 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 195550b5c0ea..8d61f270761f 100644
--- 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

test %rsp,%rsp
jnz 1f
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 06e1dd7f3332..ff0e18864cc7 100644
--- 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;

diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h
index 94c557491860..98739028a6ed 100644
--- a/xen/arch/x86/include/asm/smp.h
+++ b/xen/arch/x86/include/asm/smp.h
@@ -69,7 +69,7 @@ extern cpumask_t **socket_cpumask;
* by certain scheduling code only.
*/
#define get_cpu_current(cpu) \
- (get_cpu_info_from_stack((unsigned long)stack_base[cpu])->current_vcpu)
+ (get_cpu_info_from_stack((unsigned long)cpu_data[cpu].stack_base)->current_vcpu)

extern unsigned int disabled_cpus;
extern bool unaccounted_cpus;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a19fe219bbf6..b2c0679725ea 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -798,7 +798,7 @@ static void __init noreturn reinit_bsp_stack(void)
/* Update SYSCALL trampolines */
percpu_traps_init();

- stack_base[0] = stack;
+ cpu_data[0].stack_base = stack;

rc = setup_cpu_root_pgt(0);
if ( rc )
@@ -1959,8 +1959,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
/* Set up node_to_cpumask based on cpu_to_node[]. */
numa_add_cpu(i);

- if ( stack_base[i] == NULL )
- stack_base[i] = cpu_alloc_stack(i);
+ if ( cpu_data[i].stack_base == NULL )
+ cpu_data[i].stack_base = cpu_alloc_stack(i);
}

for_each_present_cpu ( i )
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index f061486e56eb..8ae65ab1769f 100644
--- 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);
-
/* This grunge runs the startup process for the targeted processor. */

set_cpu_state(CPU_STATE_INIT);
@@ -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);
info->processor_id = cpu;
info->per_cpu_offset = __per_cpu_offset[cpu];

@@ -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));

set_nr_sockets();

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e1356f696aba..90d9201d1c52 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -611,9 +611,9 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
unsigned long curr_stack_base = esp & ~(STACK_SIZE - 1);
unsigned long esp_top, esp_bottom;

- if ( _p(curr_stack_base) != stack_base[cpu] )
+ if ( _p(curr_stack_base) != cpu_data[cpu].stack_base )
printk("Current stack base %p differs from expected %p\n",
- _p(curr_stack_base), stack_base[cpu]);
+ _p(curr_stack_base), cpu_data[cpu].stack_base);

esp_bottom = (esp | (STACK_SIZE - 1)) + 1;
esp_top = esp_bottom - PRIMARY_STACK_SIZE;
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index e881cd5de0a0..d81a30344677 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -161,6 +161,7 @@ void __dummy__(void)

OFFSET(CPUINFO_X86_features, struct cpuinfo_x86, x86_capability);
OFFSET(CPUINFO_X86_apicid, struct cpuinfo_x86, apicid);
+ OFFSET(CPUINFO_X86_stack_base, struct cpuinfo_x86, stack_base);
DEFINE(CPUINFO_X86_sizeof, sizeof(struct cpuinfo_x86));
BLANK();

diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
index 0a9219173f0f..994fdc474200 100644
--- a/xen/include/xen/smp.h
+++ b/xen/include/xen/smp.h
@@ -67,8 +67,6 @@ void smp_send_call_function_mask(const cpumask_t *mask);

int alloc_cpu_id(void);

-extern void *stack_base[NR_CPUS];
-
void initialize_cpu_data(unsigned int cpu);
int setup_cpu_root_pgt(unsigned int cpu);

--
2.41.0

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

julien at xen

Feb 2, 2024, 10:24 AM

Post #2 of 6 (25 views)

Permalink

Hi,

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...

>
> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
> ---
> xen/arch/x86/boot/x86_64.S | 5 ++---
> xen/arch/x86/include/asm/cpufeature.h | 1 +
> xen/arch/x86/include/asm/smp.h | 2 +-
> xen/arch/x86/setup.c | 6 +++---
> xen/arch/x86/smpboot.c | 25 +++++++++++++------------
> xen/arch/x86/traps.c | 4 ++--
> xen/arch/x86/x86_64/asm-offsets.c | 1 +
> xen/include/xen/smp.h | 2 --
> 8 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> index 195550b5c0ea..8d61f270761f 100644
> --- 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
>
> test %rsp,%rsp
> jnz 1f
> diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
> index 06e1dd7f3332..ff0e18864cc7 100644
> --- 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?

> @@ -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?

> } __cacheline_aligned;
>
> diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h
> index 94c557491860..98739028a6ed 100644
> --- a/xen/arch/x86/include/asm/smp.h
> +++ b/xen/arch/x86/include/asm/smp.h
> @@ -69,7 +69,7 @@ extern cpumask_t **socket_cpumask;
> * by certain scheduling code only.
> */
> #define get_cpu_current(cpu) \
> - (get_cpu_info_from_stack((unsigned long)stack_base[cpu])->current_vcpu)
> + (get_cpu_info_from_stack((unsigned long)cpu_data[cpu].stack_base)->current_vcpu)
>
> extern unsigned int disabled_cpus;
> extern bool unaccounted_cpus;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a19fe219bbf6..b2c0679725ea 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -798,7 +798,7 @@ static void __init noreturn reinit_bsp_stack(void)
> /* Update SYSCALL trampolines */
> percpu_traps_init();
>
> - stack_base[0] = stack;
> + cpu_data[0].stack_base = stack;
>
> rc = setup_cpu_root_pgt(0);
> if ( rc )
> @@ -1959,8 +1959,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> /* Set up node_to_cpumask based on cpu_to_node[]. */
> numa_add_cpu(i);
>
> - if ( stack_base[i] == NULL )
> - stack_base[i] = cpu_alloc_stack(i);
> + if ( cpu_data[i].stack_base == NULL )
> + cpu_data[i].stack_base = cpu_alloc_stack(i);
> }
>
> for_each_present_cpu ( i )
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index f061486e56eb..8ae65ab1769f 100644
> --- 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 grunge runs the startup process for the targeted processor. */
>
> set_cpu_state(CPU_STATE_INIT);
> @@ -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.

> info->processor_id = cpu;
> info->per_cpu_offset = __per_cpu_offset[cpu];
>
> @@ -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));
>
> set_nr_sockets();
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index e1356f696aba..90d9201d1c52 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -611,9 +611,9 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
> unsigned long curr_stack_base = esp & ~(STACK_SIZE - 1);
> unsigned long esp_top, esp_bottom;
>
> - if ( _p(curr_stack_base) != stack_base[cpu] )
> + if ( _p(curr_stack_base) != cpu_data[cpu].stack_base )
> printk("Current stack base %p differs from expected %p\n",
> - _p(curr_stack_base), stack_base[cpu]);
> + _p(curr_stack_base), cpu_data[cpu].stack_base);
>
> esp_bottom = (esp | (STACK_SIZE - 1)) + 1;
> esp_top = esp_bottom - PRIMARY_STACK_SIZE;
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
> index e881cd5de0a0..d81a30344677 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -161,6 +161,7 @@ void __dummy__(void)
>
> OFFSET(CPUINFO_X86_features, struct cpuinfo_x86, x86_capability);
> OFFSET(CPUINFO_X86_apicid, struct cpuinfo_x86, apicid);
> + OFFSET(CPUINFO_X86_stack_base, struct cpuinfo_x86, stack_base);
> DEFINE(CPUINFO_X86_sizeof, sizeof(struct cpuinfo_x86));
> BLANK();
>
> diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
> index 0a9219173f0f..994fdc474200 100644
> --- a/xen/include/xen/smp.h
> +++ b/xen/include/xen/smp.h
> @@ -67,8 +67,6 @@ void smp_send_call_function_mask(const cpumask_t *mask);
>
> int alloc_cpu_id(void);
>
> -extern void *stack_base[NR_CPUS];
> -
> void initialize_cpu_data(unsigned int cpu);
> int setup_cpu_root_pgt(unsigned int cpu);
>

Cheers,

--
Julien Grall

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

jbeulich at suse

Feb 5, 2024, 12:41 AM

Post #3 of 6 (24 views)

Permalink

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

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

julien at xen

Feb 5, 2024, 1:32 AM

Post #4 of 6 (24 views)

Permalink

Hi Jan,

On 05/02/2024 08:41, Jan Beulich wrote:
> On 02.02.2024 19:24, Julien Grall wrote:
>>> 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.

Whoops you are right. I am not sure how I thought this line was (still)
using stack_start.

Cheers,

--
Julien Grall

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

jbeulich at suse

Feb 7, 2024, 8:53 AM

Post #5 of 6 (21 views)

Permalink

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

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

krystian.hebel at 3mdeb

Mar 12, 2024, 8:59 AM

Post #6 of 6 (2 views)

Permalink

On 5.02.2024 09:41, Jan Beulich wrote:
> 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.

Initially I thought I'll be using more fields from this structure a lot,
like
booted_cores or apicid. I'll move this and cpu_state introduced in following
patch somewhere else.

>
>>> @@ -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.
I've checked that the size didn't change after adding. I also think that
I checked that adding it there wouldn't add any padding, but maybe I
miscalculated something. In any way, this will be moved from here.
>>> --- 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.
BSP still uses it, but APs don't. That said, comment above declaration says
otherwise, I'll change it, or maybe this variable can be removed altogether
since it always points to the same place, and there are only two consumers,
both in assembly.
>
> Jan
Best regards,

--
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com