Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Fix .bss section in CAR ......................................................................
arch/x86: Fix .bss section in CAR
Fix clearing CAR region, 'stosl' stores 4 bytes at a time.
Restrict the use of symbol names _car_global_[start|end] to be used exclusively with CAR_GLOBAL_MIGRATION=y. They just alias the start and end of .bss section in CAR.
Change-Id: I36c858a4f181516d4c61f9fd1d5005c7d2c06057 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/symbols.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S M src/soc/intel/quark/romstage/fsp2_0.c 6 files changed, 38 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/35034/1
diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 4ead9ea..b3871d6 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -35,12 +35,13 @@ /* reset stack pointer to CAR stack */ mov $_car_stack_end, %esp
- /* clear CAR_GLOBAL area as it is not shared */ + /* clear .bss section as it is not shared */ cld xor %eax, %eax - movl $(_car_global_end), %ecx - movl $(_car_global_start), %edi + movl $(_ebss), %ecx + movl $(_bss), %edi sub %edi, %ecx + shrl $2, %ecx rep stosl
#if ((ENV_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 822f7fd..bc0090b 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -68,11 +68,13 @@ . += 80; _car_ehci_dbg_info_end = .;
- /* _car_global_start and _car_global_end provide symbols to per-stage + /* _bss and _ebss provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram * cbmem console. This is useful for clearing this area on a per-stage * basis when more than one stage uses cache-as-ram for CAR_GLOBALs. */ - _car_global_start = .; + + . = ALIGN(ARCH_POINTER_ALIGN_SIZE); + _bss = .; #if ENV_STAGE_HAS_BSS_SECTION /* Allow global uninitialized variables for stages without CAR teardown. */ *(.bss) @@ -80,10 +82,12 @@ *(.sbss) *(.sbss.*) #else + _car_global_start = .; *(.car.global_data); + _car_global_end = .; #endif . = ALIGN(ARCH_POINTER_ALIGN_SIZE); - _car_global_end = .; + _ebss = .;
#if CONFIG(CAR_GLOBAL_MIGRATION) _car_stack_start = .; diff --git a/src/arch/x86/include/arch/early_variables.h b/src/arch/x86/include/arch/early_variables.h index b5db194..9e0441c 100644 --- a/src/arch/x86/include/arch/early_variables.h +++ b/src/arch/x86/include/arch/early_variables.h @@ -22,6 +22,15 @@
#if ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION)
+/* + * The _car_global_[start|end]symbols cover CAR data which is relocatable + * once memory comes online. Variables with CAR_GLOBAL decoration + * reside within this region. + */ +extern char _car_global_start[]; +extern char _car_global_end[]; +#define _car_global_size (_car_global_end - _car_global_start) + asm(".section .car.global_data,"w",@nobits"); asm(".previous"); #ifdef __clang__ diff --git a/src/arch/x86/include/arch/symbols.h b/src/arch/x86/include/arch/symbols.h index 5d65a7b..3f2e978 100644 --- a/src/arch/x86/include/arch/symbols.h +++ b/src/arch/x86/include/arch/symbols.h @@ -27,25 +27,26 @@ #define _car_region_size (_car_region_end - _car_region_start)
/* - * This is the stack used under CONFIG_C_ENVIRONMENT_BOOTBLOCK for - * all stages that execute when cache-as-ram is up. + * This is the stack area used for all stages that execute when cache-as-ram + * is up. Area is not cleared in between stages. */ extern char _car_stack_start[]; extern char _car_stack_end[]; #define _car_stack_size (_car_stack_end - _car_stack_start)
+/* + * This is the .bss section cleared between each stage. It is not + * available in romstage if CAR_GLOBAL_MIGRATION is enabled. + */ +extern char _bss[]; +extern char _ebss[]; +#define _bss_size (_ebss - _bss) + +extern char _car_unallocated_start; + extern char _car_ehci_dbg_info_start[]; extern char _car_ehci_dbg_info_end[]; #define _car_ehci_dbg_info_size \ (_car_ehci_dbg_info_end - _car_ehci_dbg_info_start)
-/* - * The _car_global_[start|end]symbols cover CAR data which is relocatable - * once memory comes online. Variables with CAR_GLOBAL decoration - * reside within this region. - */ -extern char _car_global_start[]; -extern char _car_global_end[]; -#define _car_global_size (_car_global_end - _car_global_start) - #endif diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index 9a8ab5b..091fc4a 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -89,11 +89,11 @@ /* Setup bootblock stack */ mov %edx, %esp
- /* clear CAR_GLOBAL area as it is not shared */ + /* clear .bss section as it is not shared */ cld xor %eax, %eax - movl $(_car_global_end), %ecx - movl $(_car_global_start), %edi + movl $(_ebss), %ecx + movl $(_bss), %edi sub %edi, %ecx shrl $2, %ecx rep stosl diff --git a/src/soc/intel/quark/romstage/fsp2_0.c b/src/soc/intel/quark/romstage/fsp2_0.c index e6da5dd..0cf2d80 100644 --- a/src/soc/intel/quark/romstage/fsp2_0.c +++ b/src/soc/intel/quark/romstage/fsp2_0.c @@ -14,6 +14,7 @@ */
#include <arch/cpu.h> +#include <arch/early_variables.h> #include <arch/romstage.h> #include <arch/symbols.h> #include <console/console.h> @@ -142,10 +143,10 @@ aupd->StackBase); printk(BIOS_SPEW, "| |\n"); printk(BIOS_SPEW, "+-------------------+ 0x%p\n", - _car_global_end); + _bss); printk(BIOS_SPEW, "| coreboot data |\n"); printk(BIOS_SPEW, "+-------------------+ 0x%p\n", - _car_stack_end); + _car_stack_start); printk(BIOS_SPEW, "| coreboot stack |\n"); printk(BIOS_SPEW, "+-------------------+ 0x80000000 - ESRAM start\n\n");
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35034
to look at the new patch set (#2).
Change subject: arch/x86: Fix .bss section in CAR ......................................................................
arch/x86: Fix .bss section in CAR
Fix clearing CAR region, 'stosl' stores 4 bytes at a time.
Restrict the use of symbol names _car_global_[start|end] to be used exclusively with CAR_GLOBAL_MIGRATION=y. They just alias the start and end of .bss section in CAR.
Change-Id: I36c858a4f181516d4c61f9fd1d5005c7d2c06057 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/symbols.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S M src/soc/intel/quark/romstage/fsp2_0.c 6 files changed, 26 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/35034/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Fix .bss section in CAR ......................................................................
Patch Set 2: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Fix .bss section in CAR ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35034/2/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35034/2/src/arch/x86/assembly_entry... PS2, Line 44: shrl $2, %ecx Please move this to its own patch so we can land the fix on its own.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35034
to look at the new patch set (#3).
Change subject: arch/x86: Restrict use of _car_global[start|end] ......................................................................
arch/x86: Restrict use of _car_global[start|end]
Restrict the use of symbol names _car_global_[start|end] to be used exclusively with CAR_GLOBAL_MIGRATION=y. They just alias the start and end of .bss section in CAR.
Change-Id: I36c858a4f181516d4c61f9fd1d5005c7d2c06057 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/symbols.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S M src/soc/intel/quark/romstage/fsp2_0.c 6 files changed, 25 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/35034/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Restrict use of _car_global[start|end] ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35034/2/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35034/2/src/arch/x86/assembly_entry... PS2, Line 44: shrl $2, %ecx
Please move this to its own patch so we can land the fix on its own.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Restrict use of _car_global[start|end] ......................................................................
Patch Set 3: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Restrict use of _car_global[start|end] ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Restrict use of _car_global[start|end] ......................................................................
arch/x86: Restrict use of _car_global[start|end]
Restrict the use of symbol names _car_global_[start|end] to be used exclusively with CAR_GLOBAL_MIGRATION=y. They just alias the start and end of .bss section in CAR.
Change-Id: I36c858a4f181516d4c61f9fd1d5005c7d2c06057 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35034 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/symbols.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S M src/soc/intel/quark/romstage/fsp2_0.c 6 files changed, 25 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Marshall Dawson: Looks good to me, approved
diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 5e84af2..c36dc1c 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -32,11 +32,11 @@ /* reset stack pointer to CAR stack */ mov $_car_stack_end, %esp
- /* clear CAR_GLOBAL area as it is not shared */ + /* clear .bss section as it is not shared */ cld xor %eax, %eax - movl $(_car_global_end), %ecx - movl $(_car_global_start), %edi + movl $(_ebss), %ecx + movl $(_bss), %edi sub %edi, %ecx shrl $2, %ecx rep stosl diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index a09150f..6ccbd8c 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -64,11 +64,13 @@ . += 80; _car_ehci_dbg_info_end = .;
- /* _car_global_start and _car_global_end provide symbols to per-stage + /* _bss and _ebss provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram * cbmem console. This is useful for clearing this area on a per-stage * basis when more than one stage uses cache-as-ram for CAR_GLOBALs. */ - _car_global_start = .; + + . = ALIGN(ARCH_POINTER_ALIGN_SIZE); + _bss = .; #if ENV_STAGE_HAS_BSS_SECTION /* Allow global uninitialized variables for stages without CAR teardown. */ *(.bss) @@ -76,10 +78,12 @@ *(.sbss) *(.sbss.*) #else + _car_global_start = .; *(.car.global_data); + _car_global_end = .; #endif . = ALIGN(ARCH_POINTER_ALIGN_SIZE); - _car_global_end = .; + _ebss = .; _car_unallocated_start = .;
#if !CONFIG(C_ENVIRONMENT_BOOTBLOCK) diff --git a/src/arch/x86/include/arch/early_variables.h b/src/arch/x86/include/arch/early_variables.h index 57f4306..b88495c 100644 --- a/src/arch/x86/include/arch/early_variables.h +++ b/src/arch/x86/include/arch/early_variables.h @@ -20,6 +20,15 @@
#if ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION)
+/* + * The _car_global_[start|end]symbols cover CAR data which is relocatable + * once memory comes online. Variables with CAR_GLOBAL decoration + * reside within this region. + */ +extern char _car_global_start[]; +extern char _car_global_end[]; +#define _car_global_size (_car_global_end - _car_global_start) + asm(".section .car.global_data,"w",@nobits"); asm(".previous"); #ifdef __clang__ diff --git a/src/arch/x86/include/arch/symbols.h b/src/arch/x86/include/arch/symbols.h index a516155..f715e0a 100644 --- a/src/arch/x86/include/arch/symbols.h +++ b/src/arch/x86/include/arch/symbols.h @@ -24,8 +24,8 @@ #define _car_region_size (_car_region_end - _car_region_start)
/* - * This is the stack used under CONFIG_C_ENVIRONMENT_BOOTBLOCK for - * all stages that execute when cache-as-ram is up. + * This is the stack area used for all stages that execute when cache-as-ram + * is up. Area is not cleared in between stages. */ extern char _car_stack_start[]; extern char _car_stack_end[]; @@ -38,13 +38,4 @@ #define _car_ehci_dbg_info_size \ (_car_ehci_dbg_info_end - _car_ehci_dbg_info_start)
-/* - * The _car_global_[start|end]symbols cover CAR data which is relocatable - * once memory comes online. Variables with CAR_GLOBAL decoration - * reside within this region. - */ -extern char _car_global_start[]; -extern char _car_global_end[]; -#define _car_global_size (_car_global_end - _car_global_start) - #endif diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index 9a8ab5b..091fc4a 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -89,11 +89,11 @@ /* Setup bootblock stack */ mov %edx, %esp
- /* clear CAR_GLOBAL area as it is not shared */ + /* clear .bss section as it is not shared */ cld xor %eax, %eax - movl $(_car_global_end), %ecx - movl $(_car_global_start), %edi + movl $(_ebss), %ecx + movl $(_bss), %edi sub %edi, %ecx shrl $2, %ecx rep stosl diff --git a/src/soc/intel/quark/romstage/fsp2_0.c b/src/soc/intel/quark/romstage/fsp2_0.c index c999bce..a64fed4 100644 --- a/src/soc/intel/quark/romstage/fsp2_0.c +++ b/src/soc/intel/quark/romstage/fsp2_0.c @@ -115,7 +115,7 @@ aupd->StackBase); printk(BIOS_SPEW, "| |\n"); printk(BIOS_SPEW, "+-------------------+ 0x%p\n", - _car_global_end); + _car_unallocated_start); printk(BIOS_SPEW, "| coreboot data |\n"); printk(BIOS_SPEW, "+-------------------+ 0x%p\n", _car_stack_end);
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35034 )
Change subject: arch/x86: Restrict use of _car_global[start|end] ......................................................................
Patch Set 8: Code-Review+2
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Subrata Banik (Code Review)