[coreboot-gerrit] Change in ...coreboot[master]: arch/x86: Enforce CPU stack alignment

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/26302 )

Change subject: arch/x86: Enforce CPU stack alignment
......................................................................

arch/x86: Enforce CPU stack alignment

When rmodule is loaded CPU stack alignment is only guaranteed
to 4kiB. Implementation of cpu_info() requires that each
CPU sees its stack aligned to CONFIG_STACK_SIZE.

Add one spare CPU for the stack reserve, such that alignment can
be enforced runtime.

Change-Id: Ie04956c64df0dc7bb156002d3d4f2629f92b340e
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/26302
Reviewed-by: Stefan Reinauer 
Reviewed-by: Aaron Durbin 
Tested-by: build bot (Jenkins) 
---
M src/arch/x86/c_start.S
M src/cpu/x86/lapic/lapic_cpu_init.c
M src/cpu/x86/mp_init.c
3 files changed, 21 insertions(+), 17 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Stefan Reinauer: Looks good to me, approved
  Aaron Durbin: Looks good to me, approved

diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S
index 3fbdac1..86147ec 100644
--- a/src/arch/x86/c_start.S
+++ b/src/arch/x86/c_start.S
@@ -19,9 +19,11 @@
 .global _stack
 .global _estack

+/* Stack alignment is not enforced with rmodule loader, reserve one
+ * extra CPU such that alignment can be enforced on entry. */
 .align CONFIG_STACK_SIZE
 _stack:
-.space CONFIG_MAX_CPUS*CONFIG_STACK_SIZE
+.space (CONFIG_MAX_CPUS+1)*CONFIG_STACK_SIZE
 _estack:
 #if IS_ENABLED(CONFIG_COOP_MULTITASKING)
 .global thread_stacks
@@ -70,8 +72,9 @@
    rep
    stosl

-	/* set new stack */
+	/* Set new stack with enforced alignment. */
    movl	$_estack, %esp
+	andl	$(~(CONFIG_STACK_SIZE-1)), %esp

#if IS_ENABLED(CONFIG_COOP_MULTITASKING)
    /* Push the thread pointer. */
diff --git a/src/cpu/x86/lapic/lapic_cpu_init.c b/src/cpu/x86/lapic/lapic_cpu_init.c
index 4498b97..7daca0a 100644
--- a/src/cpu/x86/lapic/lapic_cpu_init.c
+++ b/src/cpu/x86/lapic/lapic_cpu_init.c
@@ -263,8 +263,8 @@
 int start_cpu(struct device *cpu)
 {
    struct cpu_info *info;
-	unsigned long stack_end;
-	unsigned long stack_base;
+	uintptr_t stack_top;
+	uintptr_t stack_base;
    unsigned long apicid;
    unsigned int index;
    unsigned long count;
@@ -278,23 +278,23 @@
    /* Get an index for the new processor */
    index = ++last_cpu_index;

-	/* Find end of the new processor's stack */
-	stack_end = ((unsigned long)_estack) - (CONFIG_STACK_SIZE*index) -
-			sizeof(struct cpu_info);
-
-	stack_base = ((unsigned long)_estack) - (CONFIG_STACK_SIZE*(index+1));
-	printk(BIOS_SPEW, "CPU%d: stack_base %p, stack_end %p\n", index,
-		(void *)stack_base, (void *)stack_end);
+	/* Find boundaries of the new processor's stack */
+	stack_top = ALIGN_DOWN((uintptr_t)_estack, CONFIG_STACK_SIZE);
+	stack_top -= (CONFIG_STACK_SIZE*index);
+	stack_base = stack_top - CONFIG_STACK_SIZE;
+	stack_top -= sizeof(struct cpu_info);
+	printk(BIOS_SPEW, "CPU%d: stack_base %p, stack_top %p\n", index,
+		(void *)stack_base, (void *)stack_top);
    stacks[index] = (void *)stack_base;

/* Record the index and which CPU structure we are using */
-	info = (struct cpu_info *)stack_end;
+	info = (struct cpu_info *)stack_top;
    info->index = index;
    info->cpu   = cpu;
    thread_init_cpu_info_non_bsp(info);

/* Advertise the new stack and index to start_cpu */
-	secondary_stack = stack_end;
+	secondary_stack = stack_top;
    secondary_cpu_index = index;

/* Until the CPU starts up report the CPU is not enabled */
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c
index 9526824..3889c7d 100644
--- a/src/cpu/x86/mp_init.c
+++ b/src/cpu/x86/mp_init.c
@@ -18,6 +18,7 @@
 #include <stdint.h>
 #include <rmodule.h>
 #include <arch/cpu.h>
+#include <commonlib/helpers.h>
 #include <cpu/cpu.h>
 #include <cpu/intel/microcode.h>
 #include <cpu/x86/cache.h>
@@ -229,11 +230,11 @@

static void setup_default_sipi_vector_params(struct sipi_params *sp)
 {
-	sp->gdt = (uint32_t)&gdt;
-	sp->gdtlimit = (uint32_t)&gdt_end - (u32)&gdt - 1;
-	sp->idt_ptr = (uint32_t)&idtarg;
+	sp->gdt = (uintptr_t)&gdt;
+	sp->gdtlimit = (uintptr_t)&gdt_end - (uintptr_t)&gdt - 1;
+	sp->idt_ptr = (uintptr_t)&idtarg;
    sp->stack_size = CONFIG_STACK_SIZE;
-	sp->stack_top = (uint32_t)&_estack;
+	sp->stack_top = ALIGN_DOWN((uintptr_t)&_estack, CONFIG_STACK_SIZE);
    /* Adjust the stack top to take into account cpu_info. */
    sp->stack_top -= sizeof(struct cpu_info);
 }