Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
[WIP] amd/stoneyridge: Fix CAR stack location
As far as I can see, for BSP, gcccar.inc exits with %esp at _car_region_end, not _car_stack_end.
On exit from ENABLE_AMD_STACK, %esp will have value of DCACHE_RAM_BASE + DCACHE_RAM_SIZE. This was correct for the stack placement with C_ENVIRONMENT_BOOTBLOCK=n.
It would be nice if AMD code had guards so we knew how much stack it needs. Setting of DCACHE_BSP_STACK_SIZE works with inverse logic now, the higher you set it, the less space is where the stack is located. This raises the question how its size was determined.
00030000 B _car_region_start 00030000 B _car_stack_start 00034000 B _car_stack_end 00034000 B _preram_cbmem_console 00035600 B _car_relocatable_data_start 00035850 B _car_global_start 00035860 B _car_global_end 00035860 B _car_relocatable_data_end 00040000 B _car_region_end
Use the symbol _car_stack_end to have early stage use the location assigned by the linker script car.ld.
Change-Id: Id91393f1e7faf86b01fdc113e7940893673a27a7 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/34883/1
diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 4f0bb3f..c23939c 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -31,6 +31,8 @@ .globl _cache_as_ram_setup, _cache_as_ram_setup_end .globl chipset_teardown_car
+#if ENV_ROMSTAGE + _cache_as_ram_setup:
/* Preserve BIST. */ @@ -49,6 +51,10 @@
AMD_ENABLE_STACK
+ /* Let arch/x86/car.ld determine our stack location. + * FIXME: Only do this for BSP. !! */ + movl $_car_stack_end, %esp + /* Align the stack. */ and $0xFFFFFFF0, %esp
@@ -107,6 +113,8 @@ pushl %eax call romstage_main
+#endif /* ENV_ROMSTAGE */ + #if CONFIG(POSTCAR_STAGE)
/* We do not return. Execution continues with run_postcar_phase() diff --git a/src/soc/amd/common/block/cpu/car/cache_as_ram.S b/src/soc/amd/common/block/cpu/car/cache_as_ram.S index 402da3a..ad0ea53 100644 --- a/src/soc/amd/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/amd/common/block/cpu/car/cache_as_ram.S @@ -38,6 +38,10 @@
AMD_ENABLE_STACK
+ /* Let arch/x86/car.ld determine our stack location. + * FIXME: Only do this for BSP. !! */ + movl $_car_stack_end, %esp + /* Align the stack and keep aligned for call to bootblock_c_entry() */ and $0xfffffff0, %esp sub $8, %esp
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1: Code-Review-2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
ping-ping
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Why do code that has always worked are now marked as problem? In particular, why are globals prohibited in romstage? That said, what would happen if an AP executed the code you introduced? Would it be easier to modify the macro AMD_ENABLE_STACK instead (though more files)? The macro already detect BSP when setting the stack. I believe you might avoid the global issue by modifying the macro itself.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Patch Set 1:
Why do code that has always worked are now marked as problem? In particular, why are globals prohibited in romstage?
Placement of valuables must agree with the linker script. Otherwise it only works by luck, not by design. In other words, the codebase is volatile; increase pre-ram cbmem console or timestamps stash and it will crash runtime instead of failing already at buildtime.
Prohibited globals in romstage goes back to days with CAR_GLOBAL_MIGRATION=y, did you come across some stale comments about it or why did you bring it up here?
That said, what would happen if an AP executed the code you introduced? Would it be easier to modify the macro AMD_ENABLE_STACK instead (though more files)? The macro already detect BSP when setting the stack. I believe you might avoid the global issue by modifying the macro itself.
This code without fix? AP would overwrite BSP stack.
AMD_ENABLE_STACK is a mammoth. IMO, preferably assembly would reference the stack locations by the symbol names from car.ld to avoid the mismatch. Linker script would have authority. But car.ld does not even account for the CAR region APs use...
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Why do code that has always worked are now marked as problem? In particular, why are globals prohibited in romstage?
Placement of valuables must agree with the linker script. Otherwise it only works by luck, not by design. In other words, the codebase is volatile; increase pre-ram cbmem console or timestamps stash and it will crash runtime instead of failing already at buildtime.
Prohibited globals in romstage goes back to days with CAR_GLOBAL_MIGRATION=y, did you come across some stale comments about it or why did you bring it up here?
That said, what would happen if an AP executed the code you introduced? Would it be easier to modify the macro AMD_ENABLE_STACK instead (though more files)? The macro already detect BSP when setting the stack. I believe you might avoid the global issue by modifying the macro itself.
This code without fix? AP would overwrite BSP stack.
AMD_ENABLE_STACK is a mammoth. IMO, preferably assembly would reference the stack locations by the symbol names from car.ld to avoid the mismatch. Linker script would have authority. But car.ld does not even account for the CAR region APs use...
did you come across some stale comments about it or why did you bring it up here? Because it was the reason for Jenkins to give you a -1... and because I did came across it with a code I was developing, though it did work (and booted) when building locally. But when I tried in a google environment, it would fail to build. All I was doing was declaring a structure in one file and declaring it as external in 2 other files.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
did you come across some stale comments about it or why did you bring it up here? Because it was the reason for Jenkins to give you a -1...
Ahh.. that Jenkins -1 was some bug on previous commits on this branch. They were on intel...
and because I did came across it with a code I was developing, though it did work (and booted) when building locally. But when I tried in a google environment, it would fail to build. All I was doing was declaring a structure in one file and declaring it as external in 2 other files.
We recently flipped from NO_CAR_GLOBAL_MIGRATION to CAR_GLOBAL_MIGRATION Kconfig in upstream. If you move board to older branch you probably have to put 'select NO_CAR_GLOBAL_MIGRATION' or 'select POSTCAR_STAGE' lines back in the board Kconfig. Having that wrong would trigger those forbidden globals in romstage -errors.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
I think you've correctly identified the problem although I'm not sure what other behaviors this change might create. Short of rewriting all the gcccar.inc code, I wonder if a better solution is to make DCACHE_BSP_STACK_SIZE match gcccar's BSP_STACK_SIZE. It seems like a while back I'd tried experimenting with using the CONFIG_ values in gcccar but saw it breaking more than it fixed.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Patch Set 1:
I think you've correctly identified the problem although I'm not sure what other behaviors this change might create. Short of rewriting all the gcccar.inc code, I wonder if a better solution is to make DCACHE_BSP_STACK_SIZE match gcccar's BSP_STACK_SIZE. It seems like a while back I'd tried experimenting with using the CONFIG_ values in gcccar but saw it breaking more than it fixed.
You can't do that. DCACHE_RAM_SIZE has to match BSP_STACK_SIZE. And within DCACHE_RAM_SIZE you need the room for vboot, console, timestamps and .bss too.
For the production branch, you also cannot change CAR layout from what (RO) bootblock was built with originally. And we don't know if 0x4000 is a sufficient DCACHE_BSP_STACK_SIZE, I assume the value was chosen with infamous copy-paste method.
That leaves us with the option of fixing car.ld to match gcccar.inc (or cache_as_ram.S). That is, tell the linker stack grows downwards from _car_region_end. I just hate to put a platform-specific hacks into that file... We do have that alternative definition of stack located at the end, so I don't think it would look that bad to take this approach.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Thank you both.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Abandoned
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I think you've correctly identified the problem although I'm not sure what other behaviors this change might create. Short of rewriting all the gcccar.inc code, I wonder if a better solution is to make DCACHE_BSP_STACK_SIZE match gcccar's BSP_STACK_SIZE. It seems like a while back I'd tried experimenting with using the CONFIG_ values in gcccar but saw it breaking more than it fixed.
You can't do that. DCACHE_RAM_SIZE has to match BSP_STACK_SIZE. And within DCACHE_RAM_SIZE you need the room for vboot, console, timestamps and .bss too.
For the production branch, you also cannot change CAR layout from what (RO) bootblock was built with originally. And we don't know if 0x4000 is a sufficient DCACHE_BSP_STACK_SIZE, I assume the value was chosen with infamous copy-paste method.
That leaves us with the option of fixing car.ld to match gcccar.inc (or cache_as_ram.S). That is, tell the linker stack grows downwards from _car_region_end. I just hate to put a platform-specific hacks into that file... We do have that alternative definition of stack located at the end, so I don't think it would look that bad to take this approach.
Can we address the issue like that? : https://review.coreboot.org/c/coreboot/+/36914/8/src/drivers/amd/agesa/cache...
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Michał Żygowski (Code Review)
-
Richard Spiegel (Code Review)