Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and POSTCAR_STAGE=n it is expected that ramstage decompression will run in romstage. Due the way MAYBE_STATIC was defined the scratchpad of ulzman() would remain on the stack. This would conflict with the small allocation made for the stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
Expand the definition of MAYBE_STATIC to cover the cases that have zero-initialisation or no initialisation. These can be located in .bss. As the large stack requirement now only applies to CAR_GLOBAL_MIGRATION=y case, stack location within CAR region can be based on that instead.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/cpu/Kconfig M src/cpu/intel/socket_FCBGA559/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/device/device_const.c M src/include/stddef.h M src/lib/lzma.c M src/lib/timestamp.c M src/northbridge/intel/haswell/Kconfig M src/security/tpm/tspi/log.c M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 14 files changed, 20 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 5351fc7..6911505 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -40,7 +40,7 @@ /* Stack for CAR stages. Since it persists across all stages that * use CAR it can be reused. The chipset/SoC is expected to provide * the stack size. */ -#if CONFIG(C_ENVIRONMENT_BOOTBLOCK) +#if !CONFIG(CAR_GLOBAL_MIGRATION) _car_stack_start = .; . += CONFIG_DCACHE_BSP_STACK_SIZE; _car_stack_end = .; @@ -91,7 +91,7 @@ _car_global_end = .; _car_relocatable_data_end = .;
-#if !CONFIG(C_ENVIRONMENT_BOOTBLOCK) +#if CONFIG(CAR_GLOBAL_MIGRATION) _car_stack_start = .; _car_stack_end = _car_region_end; #endif diff --git a/src/cpu/Kconfig b/src/cpu/Kconfig index 3c0bf89..7982569 100644 --- a/src/cpu/Kconfig +++ b/src/cpu/Kconfig @@ -21,7 +21,12 @@ hex
config DCACHE_BSP_STACK_SIZE + depends on !CAR_GLOBAL_MIGRATION hex + default 0x2000 + help + The amount of anticipated stack usage by bootblock and + other pre-ram stages.
config SMP bool diff --git a/src/cpu/intel/socket_FCBGA559/Kconfig b/src/cpu/intel/socket_FCBGA559/Kconfig index d3af4ca..b1b310d 100644 --- a/src/cpu/intel/socket_FCBGA559/Kconfig +++ b/src/cpu/intel/socket_FCBGA559/Kconfig @@ -21,11 +21,4 @@ hex default 0x4000
-config DCACHE_BSP_STACK_SIZE - hex - default 0x2000 - help - The amount of anticipated stack usage in CAR by bootblock and - other stages. - endif diff --git a/src/cpu/intel/socket_mPGA604/Kconfig b/src/cpu/intel/socket_mPGA604/Kconfig index 1453f99..65ac777 100644 --- a/src/cpu/intel/socket_mPGA604/Kconfig +++ b/src/cpu/intel/socket_mPGA604/Kconfig @@ -28,8 +28,4 @@ hex default 0x4000
-config DCACHE_BSP_STACK_SIZE - hex - default 0x2000 - endif # CPU_INTEL_SOCKET_MPGA604 diff --git a/src/device/device_const.c b/src/device/device_const.c index c472aea..ef16d4b 100644 --- a/src/device/device_const.c +++ b/src/device/device_const.c @@ -204,7 +204,7 @@ DEVTREE_CONST struct bus *pci_root_bus(void) { DEVTREE_CONST struct device *pci_domain; - MAYBE_STATIC DEVTREE_CONST struct bus *pci_root = NULL; + DECLARE_MAYBE_STATIC_ZERO(DEVTREE_CONST struct bus *pci_root);
if (pci_root) return pci_root; diff --git a/src/include/stddef.h b/src/include/stddef.h index 7cae2e6..26b9ef6 100644 --- a/src/include/stddef.h +++ b/src/include/stddef.h @@ -42,6 +42,14 @@ #define MAYBE_STATIC static #endif
+#if defined(__PRE_RAM__) && CONFIG(CAR_GLOBAL_MIGRATION) +#define MAYBE_STATIC_NOINIT +#define DECLARE_MAYBE_STATIC_ZERO(x) x = 0 +#else +#define MAYBE_STATIC_NOINIT static +#define DECLARE_MAYBE_STATIC_ZERO(x) static x +#endif + #ifndef __ROMCC__ /* Provide a pointer to address 0 that thwarts any "accessing this is * undefined behaviour and do whatever" trickery in compilers. diff --git a/src/lib/lzma.c b/src/lib/lzma.c index eecbb26..7700380 100644 --- a/src/lib/lzma.c +++ b/src/lib/lzma.c @@ -26,7 +26,7 @@ int res; CLzmaDecoderState state; SizeT mallocneeds; - MAYBE_STATIC unsigned char scratchpad[15980]; + MAYBE_STATIC_NOINIT unsigned char scratchpad[15980]; const unsigned char *cp;
memcpy(properties, src, LZMA_PROPERTIES_SIZE); diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 1319b86..931da3f 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -128,7 +128,7 @@
static struct timestamp_table *timestamp_table_get(void) { - MAYBE_STATIC struct timestamp_table *ts_table = NULL; + DECLARE_MAYBE_STATIC_ZERO(struct timestamp_table *ts_table); struct timestamp_cache *ts_cache;
if (ts_table != NULL) diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig index aad2674..847cc69 100644 --- a/src/northbridge/intel/haswell/Kconfig +++ b/src/northbridge/intel/haswell/Kconfig @@ -70,13 +70,6 @@ help The amount of cache-as-ram region required by the reference code.
-config DCACHE_BSP_STACK_SIZE - hex - default 0x2000 - help - The amount of anticipated stack usage in CAR by bootblock and - other stages. - config HAVE_MRC bool "Add a System Agent binary" help diff --git a/src/security/tpm/tspi/log.c b/src/security/tpm/tspi/log.c index 4019962..a513860 100644 --- a/src/security/tpm/tspi/log.c +++ b/src/security/tpm/tspi/log.c @@ -26,7 +26,7 @@
static struct tcpa_table *tcpa_cbmem_init(void) { - MAYBE_STATIC struct tcpa_table *tclt = NULL; + DECLARE_MAYBE_STATIC_ZERO(struct tcpa_table *tclt); if (tclt) return tclt;
@@ -47,7 +47,7 @@
static struct tcpa_table *tcpa_log_init(void) { - MAYBE_STATIC struct tcpa_table *tclt = NULL; + DECLARE_MAYBE_STATIC_ZERO(struct tcpa_table *tclt);
/* We are dealing here with pre CBMEM environment. * If cbmem isn't available use CAR or SRAM */ diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 26b2ec4..9f2bc06 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -87,7 +87,6 @@ default 0x10000
config DCACHE_BSP_STACK_SIZE - depends on C_ENVIRONMENT_BOOTBLOCK hex default 0x4000 help diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index f1d08c0..b494166 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -103,7 +103,6 @@ default 0x10000
config DCACHE_BSP_STACK_SIZE - depends on C_ENVIRONMENT_BOOTBLOCK hex default 0x4000 help diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig index 5d6438f..7b41f0c 100644 --- a/src/soc/intel/braswell/Kconfig +++ b/src/soc/intel/braswell/Kconfig @@ -50,13 +50,6 @@ select SOUTHBRIDGE_INTEL_COMMON_SMBUS select C_ENVIRONMENT_BOOTBLOCK
-config DCACHE_BSP_STACK_SIZE - hex - default 0x2000 - help - The amount of anticipated stack usage in CAR by bootblock and - other stages. - config C_ENV_BOOTBLOCK_SIZE hex default 0x8000 diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig index 5856ef1..dc4dd44 100644 --- a/src/soc/intel/broadwell/Kconfig +++ b/src/soc/intel/broadwell/Kconfig @@ -119,13 +119,6 @@ help The amount of cache-as-ram region required by the reference code.
-config DCACHE_BSP_STACK_SIZE - hex - default 0x2000 - help - The amount of anticipated stack usage in CAR by bootblock and - other stages. - config HAVE_MRC bool "Add a Memory Reference Code binary" help
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@47 PS1, Line 47: #define DECLARE_MAYBE_STATIC_ZERO(x) x = 0 Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@50 PS1, Line 50: #define DECLARE_MAYBE_STATIC_ZERO(x) static x Macros with complex values should be enclosed in parentheses
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@39 PS1, Line 39: #if defined(__PRE_RAM__) && CONFIG(ARCH_X86) Why aren't we just modifying these conditions to handle the lzma case (large buffers in early stages w/ CAR)? I believe that's the one object that matters. Everything else should continue to work.
It seems cleaner to:
1. allow static (thus .bss for 0 values) in the !CAR_GLOBAL_MIGRATION cases. 2. Purposefully use stack allocation for the lzma buffer specifically in that compilation unit for CAR_GLOBAL_MIGRATION cases.
Wouldn't that suggestion just be this?
#if defined(__PRE_RAM__) && CONFIG(CAR_GLOBAL_MIGRATION) #define MAYBE_STATIC #else #define MAYBE_STATIC static #endif #define ALLOC_LARGE_BUFFER MAYBE_STATIC
That handles case 1 correctly everywhere, ignoring lzma buffer which would use ALLOC_LARGE_BUFFER.
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@47 PS1, Line 47: #define DECLARE_MAYBE_STATIC_ZERO(x) x = 0
Macros with complex values should be enclosed in parentheses
I'm not a big fan of initializing pointers to 0-value ints.
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34882
to look at the new patch set (#2).
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and POSTCAR_STAGE=n it is expected that ramstage decompression will run in romstage. Due the way MAYBE_STATIC was previously defined, scratchpad of ulzman() would remain on the stack. This would conflict with the small allocation made for the stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
With the changed definition of MAYBE_STATIC the large scratchpad is now located in .bss and the stack location can be changed based on CAR_GLOBAL_MIGRATION=n.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/cpu/Kconfig M src/cpu/intel/socket_FCBGA559/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/northbridge/intel/haswell/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 9 files changed, 7 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@39 PS1, Line 39: #if defined(__PRE_RAM__) && CONFIG(ARCH_X86)
Why aren't we just modifying these conditions to handle the lzma case (large buffers in early stages […]
I did not realise it first, but I hit the AGESA -fno-zero-initialized-in-bss case with this approach. At the moment I don't know why even libagesa would need it.
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@47 PS1, Line 47: #define DECLARE_MAYBE_STATIC_ZERO(x) x = 0
I'm not a big fan of initializing pointers to 0-value ints.
Neither am I. Some cases here would be zero, some NULL. Let's hope we don't need these at all.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@39 PS1, Line 39: #if defined(__PRE_RAM__) && CONFIG(ARCH_X86)
I did not realise it first, but I hit the AGESA -fno-zero-initialized-in-bss case with this approach […]
Can you elaborate on what is happening? Is it only affecting AGESA dependent compiles?
I think those values are getting put in .data which are also being allowed in that case. I think the reason is that .bss wasn't figured out correctly for libagesa. I very much don't want to do any of this workaround because of some broken AGESA handling. We should be binding the c flags to the objects that need them and not applying them globally to coreboot compilation units.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34882/2/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/34882/2/src/arch/x86/car.ld@43 PS2, Line 43: #if !CONFIG(CAR_GLOBAL_MIGRATION) : _car_stack_start = .; : . += CONFIG_DCACHE_BSP_STACK_SIZE; : _car_stack_end = .; : #endif This can definitely goes wrong if platforms were in fact using more than the default CONFIG_DCACHE_BSP_STACK_SIZE...
It might be worth adding a check with -fstack-usage against CONFIG_DCACHE_BSP_STACK_SIZE at compiletime.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34882/2/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/34882/2/src/arch/x86/car.ld@43 PS2, Line 43: #if !CONFIG(CAR_GLOBAL_MIGRATION) : _car_stack_start = .; : . += CONFIG_DCACHE_BSP_STACK_SIZE; : _car_stack_end = .; : #endif
This can definitely goes wrong if platforms were in fact using more than the default CONFIG_DCACHE_B […]
With CAR_GLOBAL_MIGRATION=y, stack will not move. (FSP1_0, binaryPI).
With C_ENVIRONMENT_BOOTBLOCK=y, CAR_GLOBAL_MIGRATION=n, stack will not move.
With C_ENVIRONMENT_BOOTBLOCK=n, CAR_GLOBAL_MIGRATION=n, stack will move:
For intel, these platforms have thrown "Smashed stack detected in romstage!" previously, and I think we fixed those. That was for stack size of 0x2000 which is also the default DCACHE_BSP_STACK_SIZE.
Currently AGESA with C_ENVIRONMENT_BOOTBLOCK=n, POSTCAR_STAGE=y is the question mark. Interestingly enough, I found that production amd/stoneyridge increased DCACHE_BSP_STACK_SIZE to 0x4000, and then I came across CB:34883.
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34882
to look at the new patch set (#6).
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and POSTCAR_STAGE=n it is expected that ramstage decompression will run in romstage. Due the way MAYBE_STATIC was previously defined, scratchpad of ulzman() would remain on the stack. This would conflict with the small allocation made for the stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
With the changed definition of MAYBE_STATIC the large scratchpad is now located in .bss and the stack location can be changed based on CAR_GLOBAL_MIGRATION=n.
For cpu/intel/slot_1 pre-ram console and stack reserves are blindly adjusted to make build pass.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/cpu/Kconfig M src/cpu/intel/slot_1/Kconfig M src/cpu/intel/socket_FCBGA559/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/northbridge/intel/haswell/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 10 files changed, 15 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/6
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34882
to look at the new patch set (#8).
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and POSTCAR_STAGE=n it is expected that ramstage decompression will run in romstage. Due the way MAYBE_STATIC was previously defined, scratchpad of ulzman() was located on the stack. This would conflict with the small allocation made for the stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
With the new definition of MAYBE_STATIC_BSS the large scratchpad is now located in .bss and the stack location can be changed based on CAR_GLOBAL_MIGRATION=n.
For cpu/intel/slot_1 pre-ram console and stack reserves are blindly adjusted to make build pass.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/cpu/Kconfig M src/cpu/intel/slot_1/Kconfig M src/cpu/intel/socket_FCBGA559/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/northbridge/intel/haswell/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 10 files changed, 15 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/8
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 12: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 12:
Marshall, this might get rid of the "select C_ENVIRONMENT_BOOTBLOCK" dependency with Picasso.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34882
to look at the new patch set (#13).
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and POSTCAR_STAGE=n it is expected that ramstage decompression will run in romstage. Due the way MAYBE_STATIC was previously defined, scratchpad of ulzman() was located on the stack. This would conflict with the small allocation made for the stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
With the new definition of MAYBE_STATIC_BSS the large scratchpad is now located in .bss and the stack location can be changed based on CAR_GLOBAL_MIGRATION=n.
For cpu/intel/slot_1 pre-ram console and stack reserves are blindly adjusted to make build pass.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/cpu/Kconfig M src/cpu/intel/slot_1/Kconfig M src/cpu/intel/socket_FCBGA559/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/northbridge/intel/haswell/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 10 files changed, 16 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/13
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 14: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 15: Code-Review+2
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Julius Werner, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34882
to look at the new patch set (#18).
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and POSTCAR_STAGE=n it is expected that ramstage decompression will run in romstage. Due the way MAYBE_STATIC was previously defined, scratchpad of ulzman() was located on the stack. This would conflict with the small allocation made for the stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
With the new definition of MAYBE_STATIC_BSS the large scratchpad is now located in .bss and the stack location can be changed based on CAR_GLOBAL_MIGRATION=n.
For cpu/intel/slot_1 pre-ram console and stack reserves are blindly adjusted to make build pass.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/cpu/Kconfig M src/cpu/intel/slot_1/Kconfig M src/cpu/intel/socket_FCBGA559/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/northbridge/intel/haswell/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 10 files changed, 16 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/18
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@39 PS1, Line 39: #if defined(__PRE_RAM__) && CONFIG(ARCH_X86)
Can you elaborate on what is happening? Is it only affecting AGESA dependent compiles? […]
Done
https://review.coreboot.org/c/coreboot/+/34882/1/src/include/stddef.h@47 PS1, Line 47: #define DECLARE_MAYBE_STATIC_ZERO(x) x = 0
Neither am I. Some cases here would be zero, some NULL. Let's hope we don't need these at all.
Done
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 19: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34882/19/src/soc/amd/picasso/Kconfi... File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/34882/19/src/soc/amd/picasso/Kconfi... PS19, Line 89: DCACHE_BSP_STACK_SIZE I believe this will disappear for Picasso, as it does not uses CAR. I'm not sure on the details, last I heard (though there was a plan to change it somewhat) Picasso uses "HIBRID_ROMSTAGE" replacing regular ROMSTAGE in that memory is already enabled in this stage. Memory is trained and enabled before Picasso exits reset.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34882/19/src/soc/amd/picasso/Kconfi... File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/34882/19/src/soc/amd/picasso/Kconfi... PS19, Line 89: DCACHE_BSP_STACK_SIZE
I believe this will disappear for Picasso, as it does not uses CAR. […]
See CB:35035, there is no decision yet which linker script determines stack location for Picasso.
This commit is on hold until I have verified the stack usage with AGESA.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Patch Set 22: Code-Review-2
The linker script does not reflect reality before or after this commit.
Implementation of amd/stoneyridge, even though it has C_ENVIRONMENT_BOOTBLOCK, places BSP stack right below _car_region_end. No stack guards are used. CB:34883
FSP2.0 (FSP_USES_CB_STACK=n) places shared FSP heap+stack right below _car_region_end. There is runtime checking (die() in case of failure), but build-time assertion would be nicer.
I don't see why any of the followup works would depend on this, so I am suggesting to not merge this in its current state.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Richard Spiegel (Code Review)
-
Subrata Banik (Code Review)