Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
arch/x86/car.ld: Use REGION macro
The advantages of using the REGION macro: - less boilerplate for defining start and end symbols - overlap between regions checking - automatic alignment
Change-Id: I9c327343eb40d3e2cc8513354ec3a83d803ff1ee Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/car.ld 1 file changed, 4 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/36696/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 74fc74b..c601d2e 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -18,9 +18,7 @@ #if CONFIG(PAGING_IN_CACHE_AS_RAM) /* Page table pre-allocation. CONFIG_DCACHE_RAM_BASE should be 4KiB * aligned when using this option. */ - _pagetables = . ; - . += 4096 * CONFIG_NUM_CAR_PAGE_TABLE_PAGES; - _epagetables = . ; + REGION(pagetables, ., 4096 * CONFIG_NUM_CAR_PAGE_TABLE_PAGES, 4096) #endif /* Vboot work buffer only needs to be available when verified boot * starts in bootblock. */ @@ -37,9 +35,7 @@ * use CAR it can be reused. The chipset/SoC is expected to provide * the stack size. */ #if CONFIG(C_ENVIRONMENT_BOOTBLOCK) - _car_stack = .; - . += CONFIG_DCACHE_BSP_STACK_SIZE; - _ecar_stack = .; + REGION(car_stack, ., CONFIG_DCACHE_BSP_STACK_SIZE, 16) #endif /* The pre-ram cbmem console as well as the timestamp region are fixed * in size. Therefore place them above the car global section so that @@ -47,22 +43,17 @@ * link address of these shared objects. */ PRERAM_CBMEM_CONSOLE(., CONFIG_PRERAM_CBMEM_CONSOLE_SIZE) #if CONFIG(PAGING_IN_CACHE_AS_RAM) - . = ALIGN(32); /* Page directory pointer table resides here. There are 4 8-byte entries * totalling 32 bytes that need to be 32-byte aligned. The reason the * pdpt are not colocated with the rest of the page tables is to reduce * fragmentation of the CAR space that persists across stages. */ - _pdpt = .; - . += 32; - _epdpt = .; + REGION(pdpt, ., 32, 32) #endif
TIMESTAMP(., 0x200)
- _car_ehci_dbg_info = .; /* Reserve sizeof(struct ehci_dbg_info). */ - . += 80; - _ecar_ehci_dbg_info = .; + REGION(car_ehci_dbg_info, ., 80, ARCH_POINTER_ALIGN_SIZE)
/* _bss and _ebss provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram @@ -114,9 +105,6 @@ }
_bogus = ASSERT((CONFIG_DCACHE_RAM_SIZE == 0) || (SIZEOF(.car.data) <= CONFIG_DCACHE_RAM_SIZE), "Cache as RAM area is too full"); -#if CONFIG(PAGING_IN_CACHE_AS_RAM) -_bogus2 = ASSERT(_pagetables == ALIGN(_pagetables, 4096), "_pagetables aren't 4KiB aligned"); -#endif #if CONFIG(C_ENVIRONMENT_BOOTBLOCK) _bogus3 = ASSERT(CONFIG_DCACHE_BSP_STACK_SIZE > 0x0, "BSP stack size not configured"); #endif
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@109 PS1, Line 109: 3 rename
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@38 PS1, Line 38: ., Should we add a new variant to memlayout.h that utilizes current location counter?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@38 PS1, Line 38: .,
Should we add a new variant to memlayout. […]
I think for consistency with the pre-defined region macros (VBOOT2_WORK(), etc.) it would be better to use REGION() as is here.
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@109 PS1, Line 109: 3
rename
BTW you can just use '_' as a throwaway variable (that's what the memlayout macros do). There's no need to give them all different names.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@38 PS1, Line 38: .,
I think for consistency with the pre-defined region macros (VBOOT2_WORK(), etc. […]
for this particular one I was thinking of using STACK() and adapt the code? Or is that too confusing?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@38 PS1, Line 38: .,
for this particular one I was thinking of using STACK() and adapt the code? Or is that too confusing […]
SGTM, that would be a bit more consistent with the other archs.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@38 PS1, Line 38: .,
SGTM, that would be a bit more consistent with the other archs.
The stack is in a different location for CAR stages (linker symbol), postcar (set up in postcar frame) and ramstage (in .bss), which could be confusing. Is it always at the same location in SRAM for ARM?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36696/1/src/arch/x86/car.ld@38 PS1, Line 38: .,
SGTM, that would be a bit more consistent with the other archs. […]
Yes, we keep using the same stack throughout all the stages.
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36696 )
Change subject: arch/x86/car.ld: Use REGION macro ......................................................................
Abandoned
Done in master.
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)