Change in coreboot[master]: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
The smm_module_loader v2 places stack_top at DEFAULT_SMBASE + per_cpu_stack_size. The smm_stub assembly will put the stack for a CPU at stack_top - apicid * stack_size. So for loader v2 this means that areas below DEFAULT_SMBASE get used as a CPU stack which would cause lower memory corruption on S3 resume.
The solution is to use ramstage heap as a temporary place for the smm relocation stacks. This completely avoids using lower memory for CPU stacks.
On V1 all the CPU stacks would be properly allocated inside the DEFAULT_SMBASE but this change won't hurt either.
Tested: works fine on OCP/Deltalake.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/cpu/x86/smm/smm_module_loaderv2.c 3 files changed, 32 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/1
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 4870529..3deab6d 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -3,6 +3,7 @@ #include <console/console.h> #include <stddef.h> #include <stdint.h> +#include <stdlib.h> #include <string.h> #include <rmodule.h> #include <arch/cpu.h> @@ -764,19 +765,21 @@ runtime->apic_id_to_cpu[i] = cpu_get_apic_id(i); }
+void *smm_stacks; + static int install_relocation_handler(int num_cpus, size_t save_state_size) { - int cpus = num_cpus; -#if CONFIG(X86_SMM_LOADER_VERSION2) - /* Default SMRAM size is not big enough to concurrently - * handle relocation for more than ~32 CPU threads - * therefore, relocate 1 by 1. */ - cpus = 1; -#endif + smm_stacks = memalign(16, num_cpus * CONFIG_SMM_STUB_STACK_SIZE); + + if (smm_stacks == NULL) { + printk(BIOS_ERR, "%s: failed to allocate stacks.\n"); + return -1; + }
struct smm_loader_params smm_params = { + .stack_top = (void *)((uintptr_t)smm_stacks + num_cpus * CONFIG_SMM_STUB_STACK_SIZE), .per_cpu_stack_size = CONFIG_SMM_STUB_STACK_SIZE, - .num_concurrent_stacks = cpus, + .num_concurrent_stacks = num_cpus, .per_cpu_save_state_size = save_state_size, .num_concurrent_save_states = 1, .handler = smm_do_relocation, @@ -1104,6 +1107,8 @@
restore_default_smm_area(default_smm_area);
+ free(smm_stacks); + /* Signal callback on success if it's provided. */ if (ret == 0 && mp_state.ops.post_mp_init != NULL) mp_state.ops.post_mp_init(); diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 876fde6..3c0ed7e 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -244,8 +244,16 @@ size += params->per_cpu_save_state_size; }
- /* Place the stacks in the lower half of SMRAM. */ - stacks_top = smm_stub_place_stacks(base, size, params); + /* Use the smbase as a proxy to know if we are installing the stub for relocation + * or for permanent handling. In case of relocation the SMM relocation stack will + * have been allocated on the ramstage heap and programmed in the smm loader params. + */ + if (smbase == (void *)SMM_DEFAULT_BASE) + stacks_top = params->stack_top; + else + /* Place the stacks in the lower half of SMRAM. */ + stacks_top = smm_stub_place_stacks(base, size, params); + if (stacks_top == NULL) return -1;
diff --git a/src/cpu/x86/smm/smm_module_loaderv2.c b/src/cpu/x86/smm/smm_module_loaderv2.c index 7a72c10..54e46d4 100644 --- a/src/cpu/x86/smm/smm_module_loaderv2.c +++ b/src/cpu/x86/smm/smm_module_loaderv2.c @@ -414,8 +414,15 @@ * of SMRAM which is TSEG base */ const total_stack_size = params->num_concurrent_stacks * params->per_cpu_stack_size; - stacks_top = smm_stub_place_stacks((char *)params->smram_start, total_stack_size, - params); + /* Use the smbase as a proxy to know if we are installing the stub for relocation + * or for permanent handling. In case of relocation the SMM relocation stack will + * have been allocated on the ramstage heap and programmed in the smm loader params. + */ + if (smbase == (void *)SMM_DEFAULT_BASE) + stacks_top = params->stack_top; + else + stacks_top = smm_stub_place_stacks((char *)params->smram_start, size, params); + if (stacks_top == NULL) { printk(BIOS_ERR, "%s: not enough space for stacks\n", __func__); printk(BIOS_ERR, "%s: ....need -> %p : available -> %zx\n", __func__,
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47072/1/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/47072/1/src/cpu/x86/mp_init.c@780 PS1, Line 780: .stack_top = (void *)((uintptr_t)smm_stacks + num_cpus * CONFIG_SMM_STUB_STACK_SIZE), line over 96 characters
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 1: Code-Review+1
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47072
to look at the new patch set (#2).
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
The smm_module_loader v2 places stack_top at DEFAULT_SMBASE + per_cpu_stack_size. The smm_stub assembly will put the stack for a CPU at stack_top - apicid * stack_size. So for loader v2 this means that areas below DEFAULT_SMBASE get used as a CPU stack which would cause lower memory corruption on S3 resume.
The solution is to use ramstage heap as a temporary place for the smm relocation stacks. This completely avoids using lower memory for CPU stacks.
On V1 all the CPU stacks would be properly allocated inside the DEFAULT_SMBASE but this change won't hurt either.
Tested: works fine on OCP/Deltalake.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/cpu/x86/smm/smm_module_loaderv2.c 3 files changed, 33 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47072/2/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/47072/2/src/cpu/x86/mp_init.c@780 PS2, Line 780: .stack_top = (void *)((uintptr_t)smm_stacks + num_cpus * CONFIG_SMM_STUB_STACK_SIZE), line over 96 characters
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47072
to look at the new patch set (#3).
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
The smm_module_loader v2 places stack_top at DEFAULT_SMBASE + per_cpu_stack_size. The smm_stub assembly will put the stack for a CPU at stack_top - apicid * stack_size. So for loader v2 this means that areas below DEFAULT_SMBASE get used as a CPU stack. This would cause lower memory corruption on S3 resume.
The solution is to use ramstage heap as a temporary place for the smm relocation stacks. This completely avoids using lower memory for CPU stacks.
On v1 all the CPU stacks would be properly allocated inside the DEFAULT_SMBASE but this change won't hurt either.
Tested: works fine on OCP/Deltalake.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/cpu/x86/smm/smm_module_loaderv2.c 3 files changed, 33 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/3
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 3: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 3: Code-Review+1
Hello build bot (Jenkins), Jonathan Zhang, David Hendricks, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47072
to look at the new patch set (#4).
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
The smm_module_loader v2 places stack_top at DEFAULT_SMBASE + per_cpu_stack_size. The smm_stub assembly will put the stack for a CPU at stack_top - apicid * stack_size. So for loader v2 this means that areas below DEFAULT_SMBASE get used as a CPU stack. This would cause lower memory corruption on S3 resume.
The solution is to use ramstage heap as a temporary place for the smm relocation stacks. This completely avoids using lower memory for CPU stacks.
On v1 all the CPU stacks would be properly allocated inside the DEFAULT_SMBASE but this change won't hurt either.
Tested: works fine on OCP/Deltalake.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/cpu/x86/smm/smm_module_loaderv2.c 3 files changed, 33 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 4:
(1 comment)
File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/47072/comment/b13fdcad_18e3218d PS4, Line 786: .stack_top = (void *)((uintptr_t)smm_stacks + num_cpus * CONFIG_SMM_STUB_STACK_SIZE), line over 96 characters
Hello build bot (Jenkins), Jonathan Zhang, David Hendricks, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47072
to look at the new patch set (#5).
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
The smm_module_loader v2 places stack_top at DEFAULT_SMBASE + per_cpu_stack_size. The smm_stub assembly will put the stack for a CPU at stack_top - apicid * stack_size. So for loader v2 this means that areas below DEFAULT_SMBASE get used as a CPU stack. This would cause lower memory corruption on S3 resume.
The solution is to use ramstage heap as a temporary place for the smm relocation stacks. This completely avoids using lower memory for CPU stacks.
On v1 all the SMM relocation CPU stacks would normally be properly allocated inside the DEFAULT_SMBASE but this change won't hurt either.
Tested: works fine on OCP/Deltalake with the SMM relocation handler stack inside the ramstage heap.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/cpu/x86/smm/smm_module_loaderv2.c 3 files changed, 35 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 5:
(2 comments)
File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/47072/comment/d8f8f6a6_c8bb6ec2 PS5, Line 778: _Static_assert(CONFIG_HEAP_SIZE > CONFIG_MAX_CPUS * CONFIG_SMM_STUB_STACK_SIZE, Comparisons should place the constant on the right side of the test
https://review.coreboot.org/c/coreboot/+/47072/comment/98522ff6_359b57c3 PS5, Line 788: .stack_top = (void *)((uintptr_t)smm_stacks + num_cpus * CONFIG_SMM_STUB_STACK_SIZE), line over 96 characters
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
Patch Set 5: Code-Review-1
(1 comment)
Patchset:
PS5: Hmm is there any reason the relocation handler can't use the same stack it as the permanent handler? I'll try that.
Hello build bot (Jenkins), Jonathan Zhang, David Hendricks, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47072
to look at the new patch set (#6).
Change subject: cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap ......................................................................
cpu/x86/mp_init.c: Allocate the smm relocation stack on the heap
The smm_module_loader v2 places stack_top at DEFAULT_SMBASE + per_cpu_stack_size. The smm_stub assembly will put the stack for a CPU at stack_top - apicid * stack_size. So for loader v2 this means that areas below DEFAULT_SMBASE get used as a CPU stack. This would cause lower memory corruption on S3 resume.
The solution is to use ramstage heap as a temporary place for the smm relocation stacks. This completely avoids using lower memory for CPU stacks.
On v1 all the SMM relocation CPU stacks would normally be properly allocated inside the DEFAULT_SMBASE but this change won't hurt either.
Tested: works fine on OCP/Deltalake with the SMM relocation handler stack inside the ramstage heap.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loaderv2.c 2 files changed, 25 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/6
Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), Jonathan Zhang, David Hendricks, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47072
to look at the new patch set (#7).
Change subject: cpu/x86/smm_loaderv2: Use the smm_loader_params stack ......................................................................
cpu/x86/smm_loaderv2: Use the smm_loader_params stack
Use the provided smm_loader_params stack. Currently this is a noop as the stack_top entry is never set.
Change-Id: I125d3aa6526c923e154c41e2d586557c7d8f56c3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/smm/smm_module_loaderv2.c 1 file changed, 9 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47072/7
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/smm_loaderv2: Use the smm_loader_params stack ......................................................................
Patch Set 8: Code-Review-1
(1 comment)
File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/47072/comment/4c6fe1cc_0b90f4fa PS8, Line 251: if (params->stack_top != NULL) I don't end up using this...
Attention is currently required from: Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/smm_loaderv2: Use the smm_loader_params stack ......................................................................
Patch Set 8: Code-Review+1
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47072 )
Change subject: cpu/x86/smm_loaderv2: Use the smm_loader_params stack ......................................................................
Abandoned
Not used.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
David Hendricks (Code Review)