Raul Rangel has submitted this change. ( https://review.coreboot.org/c/coreboot/+/57627 )
Change subject: arch/x86,cpu/x86: Introduce new method for accessing cpu_info ......................................................................
arch/x86,cpu/x86: Introduce new method for accessing cpu_info
There is currently a fundamental flaw in the current cpu_info() implementation. It assumes that current stack is CONFIG_STACK_SIZE aligned. This assumption breaks down when performing SMM relocation.
The first step in performing SMM relocation is changing the SMBASE. This is accomplished by installing the smmstub at 0x00038000, which is the default SMM entry point. The stub is configured to set up a new stack with the size of 1 KiB (CONFIG_SMM_STUB_STACK_SIZE), and an entry point of smm_do_relocation located in RAMSTAGE RAM.
This means that when smm_do_relocation is executed, it is running in SMM with a different sized stack. When cpu_info() gets called it will be using CONFIG_STACK_SIZE to calculate the location of the cpu_info struct. This results in reading random memory. Since cpu_info() has to run in multiple environments, we can't use a compile time constant to locate the cpu_info struct.
This CL introduces a new way of locating cpu_info. It uses a per-cpu segment descriptor that points to a per-cpu segment that is allocated on the stack. By using a segment descriptor to point to the per-cpu data, we no longer need to calculate the location of the cpu_info struct. This has the following advantages: * Stacks no longer need to be CONFIG_STACK_SIZE aligned. * Accessing an unconfigured segment will result in an exception. This ensures no one can call cpu_info() from an unsupported environment. * Segment selectors are cleared when entering SMM and restored when leaving SMM. * There is a 1:1 mapping between cpu and cpu_info. When using COOP_MULTITASKING, a new cpu_info is currently allocated at the top of each thread's stack. This no longer needs to happen.
This CL guards most of the code with CONFIG(CPU_INFO_V2). I did this so reviewers can feel more comfortable knowing most of the CL is a no-op. I would eventually like to remove most of the guards though.
This CL does not touch the LEGACY_SMP_INIT code path. I don't have any way of testing it.
The %gs segment was chosen over the %fs segment because it's what the linux kernel uses for per-cpu data in x86_64 mode.
BUG=b:194391185, b:179699789 TEST=Boot guybrush with CPU_INFO_V2 and verify BSP and APs have correct %gs segment. Verify cpu_info looks sane. Verify booting to the OS works correctly with COOP_MULTITASKING enabled.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I79dce9597cb784acb39a96897fb3c2f2973bfd98 Reviewed-on: https://review.coreboot.org/c/coreboot/+/57627 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Eric Peers epeers@google.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/arch/x86/c_start.S M src/arch/x86/include/arch/cpu.h M src/cpu/x86/Kconfig M src/cpu/x86/cpu_info.S.inc M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/gdt.h 8 files changed, 186 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Karthik Ramasubramanian: Looks good to me, approved Eric Peers: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S index cb7d504..9e718fc 100644 --- a/src/arch/x86/c_start.S +++ b/src/arch/x86/c_start.S @@ -80,6 +80,20 @@
push_cpu_info
+#if CONFIG(CPU_INFO_V2) + /* Allocate the per_cpu_segment_data on the stack */ + push_per_cpu_segment_data + + /* + * Update the BSP's per_cpu_segment_descriptor to point to the + * per_cpu_segment_data that was allocated on the stack. + */ + set_segment_descriptor_base $per_cpu_segment_descriptors, %esp + + mov per_cpu_segment_selector, %eax + mov %eax, %gs +#endif + /* * Now we are finished. Memory is up, data is copied and * bss is cleared. Now we call the main routine and @@ -127,6 +141,7 @@ #endif
.globl gdt, gdt_end + .global per_cpu_segment_descriptors, per_cpu_segment_selector
gdtaddr: .word gdt_end - gdt - 1 @@ -136,7 +151,7 @@ .long gdt /* we know the offset */ #endif
- .data + .data
/* This is the gdt for GCC part of coreboot. * It is different from the gdt in ASM part of coreboot @@ -206,8 +221,26 @@ .word 0xffff, 0x0000 .byte 0x00, 0x9b, 0xaf, 0x00 #endif +#if CONFIG(CPU_INFO_V2) +per_cpu_segment_descriptors: + .rept CONFIG_MAX_CPUS + /* flat data segment */ + .word 0xffff, 0x0000 +#if ENV_X86_64 + .byte 0x00, 0x92, 0xcf, 0x00 +#else + .byte 0x00, 0x93, 0xcf, 0x00 +#endif + .endr +#endif /* CPU_INFO_V2 */ gdt_end:
+#if CONFIG(CPU_INFO_V2) +/* Segment selector pointing to the first per_cpu_segment_descriptor. */ +per_cpu_segment_selector: + .long per_cpu_segment_descriptors - gdt +#endif /* CPU_INFO_V2 */ + .section ".text._start", "ax", @progbits #if ENV_X86_64 SetCodeSelector: diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index f736a14..8e96fae 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -236,11 +236,40 @@ #endif };
+/* + * This structure describes the data allocated in the %gs segment for each CPU. + * In order to read from this structure you will need to use assembly to + * reference the segment. + * + * e.g., Reading the cpu_info pointer: + * %%gs:0 + */ +struct per_cpu_segment_data { + /* + * Instead of keeping a `struct cpu_info`, we actually keep a pointer + * pointing to the cpu_info struct located in %ds. This prevents + * needing specific access functions to read the fields in the cpu_info. + */ + struct cpu_info *cpu_info; +}; + static inline struct cpu_info *cpu_info(void) { +/* We use a #if because we don't want to mess with the &s below. */ +#if CONFIG(CPU_INFO_V2) + struct cpu_info *ci = NULL; + + __asm__("mov %%gs:%c[offset], %[ci]" + : [ci] "=r" (ci) + : [offset] "i" (offsetof(struct per_cpu_segment_data, cpu_info)) + ); + + return ci; +#else char s; uintptr_t info = ALIGN_UP((uintptr_t)&s, CONFIG_STACK_SIZE) - sizeof(struct cpu_info); return (struct cpu_info *)info; +#endif /* CPU_INFO_V2 */ }
struct cpuinfo_x86 { diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index a5c2a4d..99e33bb 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -193,3 +193,11 @@ the system BIOS and the last 2 are to be reserved for OS usage. However, modern OSes use PAT to control cacheability instead of using MTRRs. + +config CPU_INFO_V2 + bool + depends on PARALLEL_MP + help + Enables the new method of locating struct cpu_info. This new method + uses the %gs segment to locate the cpu_info pointer. The old method + relied on the stack being CONFIG_STACK_SIZE aligned. diff --git a/src/cpu/x86/cpu_info.S.inc b/src/cpu/x86/cpu_info.S.inc index 62b47ca..dffd1bc 100644 --- a/src/cpu/x86/cpu_info.S.inc +++ b/src/cpu/x86/cpu_info.S.inc @@ -8,3 +8,50 @@ push \index /* index */ push $0 /* *cpu */ .endm + +/* Push struct per_cpu_segment_data */ +.macro push_per_cpu_segment_data cpu_info_pointer=%esp + push \cpu_info_pointer /* *cpu_info */ +.endm + +/* + * Sets the base address in the segment descriptor array. + * + * A segment descriptor has the following structure: + * struct { + * uint16_t segment_limit_0_15; + * uint16_t base_address_0_15; + * uint8_t base_address_16_23; + * uint8_t attrs[2]; + * uint8_t base_address_24_31; + * }; + * + * @desc_array: Address of the descriptor table + * @base: Address to set in the descriptor + * @desc_index: Index of the descriptor in the table. Defaults to 0. Must be a + * register if specified. + * + * Clobbers %eax. + */ +.macro set_segment_descriptor_base desc_array:req, base:req, desc_index + mov \base, %eax + + push %ebx /* preserve ebx */ + mov \desc_array, %ebx + +.ifb \desc_index + movw %ax, 2(%ebx) + shr $16, %eax + movb %al, 4(%ebx) + shr $8, %eax + movb %al, 7(%ebx) +.else + movw %ax, 2(%ebx, \desc_index, 8) + shr $16, %eax + movb %al, 4(%ebx, \desc_index, 8) + shr $8, %eax + movb %al, 7(%ebx, \desc_index, 8) +.endif + + pop %ebx +.endm diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 93c143e..123b2b5 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -89,6 +89,8 @@ uint32_t gdt; uint16_t unused; uint32_t idt_ptr; + uint32_t per_cpu_segment_descriptors; + uint32_t per_cpu_segment_selector; uint32_t stack_top; uint32_t stack_size; uint32_t microcode_lock; /* 0xffffffff means parallel loading. */ @@ -215,10 +217,20 @@ sp->gdt = (uintptr_t)&gdt; sp->gdtlimit = (uintptr_t)&gdt_end - (uintptr_t)&gdt - 1; sp->idt_ptr = (uintptr_t)&idtarg; + if (CONFIG(CPU_INFO_V2)) { + sp->per_cpu_segment_descriptors = (uintptr_t)&per_cpu_segment_descriptors; + sp->per_cpu_segment_selector = per_cpu_segment_selector; + } sp->stack_size = CONFIG_STACK_SIZE; 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); + /* + * In the CPU_INFO_V2 case, we don't need to pre-allocate the space on the stack. + * Instead we push them onto the top of the stack in the sipi vector. + */ + if (!CONFIG(CPU_INFO_V2)) { + /* Adjust the stack top to take into account cpu_info. */ + sp->stack_top -= sizeof(struct cpu_info); + } }
#define NUM_FIXED_MTRRS 11 diff --git a/src/cpu/x86/sipi_vector.S b/src/cpu/x86/sipi_vector.S index 496fd34..491f1de 100644 --- a/src/cpu/x86/sipi_vector.S +++ b/src/cpu/x86/sipi_vector.S @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <cpu/x86/cpu_info.S.inc> #include <cpu/x86/cr.h> #include <cpu/amd/mtrr.h> #include <cpu/x86/msr.h> @@ -19,6 +20,10 @@ .word 0 /* unused */ idt_ptr: .long 0 +per_cpu_segment_descriptors: +.long 0 +per_cpu_segment_selector: +.long 0 stack_top: .long 0 stack_size: @@ -98,6 +103,23 @@ movl stack_top, %edx subl %eax, %edx mov %edx, %esp + +#if CONFIG(CPU_INFO_V2) + push_cpu_info index=%ecx + push_per_cpu_segment_data + + /* + * Update the AP's per_cpu_segment_descriptor to point to the + * per_cpu_segment_data that was allocated on the stack. + */ + set_segment_descriptor_base per_cpu_segment_descriptors, %esp, %ecx + + mov %ecx, %eax + shl $3, %eax /* The index is << 3 in the segment selector */ + add per_cpu_segment_selector, %eax + mov %eax, %gs +#endif + andl $0xfffffff0, %esp /* ensure stack alignment */
/* Save CPU number. */ diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index aa48ab6..e409983 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -9,6 +9,7 @@ * found in smm.h. */
+#include <cpu/x86/cpu_info.S.inc> #include <cpu/x86/cr.h> #include <cpu/x86/msr.h> #include <cpu/x86/lapic_def.h> @@ -82,8 +83,21 @@ /* gdt selector 0x20 tss segment */ .word 0xffff, 0x0000 .byte 0x00, 0x8b, 0x80, 0x00 + +#if CONFIG(CPU_INFO_V2) +per_cpu_segment_descriptors: + .rept CONFIG_MAX_CPUS + /* selgdt 0x28, flat data segment */ + .word 0xffff, 0x0000 + .byte 0x00, 0x93, 0xcf, 0x00 + .endr +#endif /* CPU_INFO_V2 */ smm_relocate_gdt_end:
+#if CONFIG(CPU_INFO_V2) +.set per_cpu_segment_selector, per_cpu_segment_descriptors - smm_relocate_gdt +#endif /* CPU_INFO_V2 */ + .align 4 .code32 .global smm_trampoline32 @@ -153,6 +167,22 @@ movl $0, 4(%ebx) #endif
+#if CONFIG(CPU_INFO_V2) + push_cpu_info index=%ecx + push_per_cpu_segment_data + + /* + * Update the AP's per_cpu_segment_descriptor to point to the + * per_cpu_segment_data that was allocated on the stack. + */ + set_segment_descriptor_base $per_cpu_segment_descriptors, %esp, %ecx + + mov %ecx, %eax + shl $3, %eax /* The index is << 3 in the segment selector */ + add $per_cpu_segment_selector, %eax + mov %eax, %gs +#endif + /* Create stack frame by pushing a NULL stack base pointer */ pushl $0x0 mov %esp, %ebp diff --git a/src/include/cpu/x86/gdt.h b/src/include/cpu/x86/gdt.h index bc4eaad..27a863e 100644 --- a/src/include/cpu/x86/gdt.h +++ b/src/include/cpu/x86/gdt.h @@ -5,6 +5,8 @@
/* These symbols are defined in c_start.S. */ extern char gdt[]; +extern char per_cpu_segment_descriptors[]; +extern uint32_t per_cpu_segment_selector; extern char gdt_end[]; extern char idtarg[];