Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
arch/x86: Simplify <arch/early_variables.h>
This also removes ENV_CACHE_AS_RAM and allows the use of .bss section for ENV_BOOTBLOCK and ENV_VERSTAGE even with CAR_GLOBAL_MIGRATION=y.
In practice, boards with CAR_GLOBAL_MIGRATION=y currently build with romcc-bootblock so they will not be using .bss.
Change-Id: Ie9dc14f3e528d3e4f48304f4d7de50df448a8af6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/include/symbols.h 5 files changed, 21 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35016/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 5351fc7..641ae1f 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -76,8 +76,8 @@ * 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 = .; -#if !CONFIG(CAR_GLOBAL_MIGRATION) - /* Allow global unitialized variables when CAR_GLOBALs are not used. */ +#if ENV_STAGE_HAS_BSS_SECTION + /* Allow global uninitialized variables for stages without CAR teardown. */ *(.bss) *(.bss.*) *(.sbss) diff --git a/src/arch/x86/include/arch/early_variables.h b/src/arch/x86/include/arch/early_variables.h index 4860e49..e9e2610 100644 --- a/src/arch/x86/include/arch/early_variables.h +++ b/src/arch/x86/include/arch/early_variables.h @@ -20,7 +20,8 @@ #include <commonlib/helpers.h> #include <stdlib.h>
-#if ENV_CACHE_AS_RAM && CONFIG(CAR_GLOBAL_MIGRATION) +#if ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION) + asm(".section .car.global_data,"w",@nobits"); asm(".previous"); #ifdef __clang__ @@ -29,27 +30,6 @@ #define CAR_GLOBAL __attribute__((used, section(".car.global_data#"))) #endif /* __clang__ */
-/* - * In stages that use CAR (verstage, C bootblock) all CAR_GLOBAL variables are - * accessed unconditionally because cbmem is never initialized until romstage - * when dram comes up. - */ -#if !ENV_ROMSTAGE -static inline void *car_get_var_ptr(void *var) -{ - return var; -} - -static inline void *car_sync_var_ptr(void *var) -{ - return var; -} - -static inline int car_active(void) -{ - return 1; -} -#else /* Get the correct pointer for the CAR global variable. */ void *car_get_var_ptr(void *var);
@@ -58,7 +38,6 @@
/* Return 1 when currently running with globals in Cache-as-RAM, 0 otherwise. */ int car_active(void); -#endif /* !ENV_ROMSTAGE */
/* Get and set a primitive type global variable. */ #define car_get_var(var) \ @@ -81,26 +60,25 @@ #else
/* - * We might end up here if: - * 1. ENV_CACHE_AS_RAM is not set for the stage or - * 2. ENV_CACHE_AS_RAM is set for the stage but CONFIG_CAR_GLOBAL_MIGRATION - * is not set. In this case, there is no need to migrate CAR global - * variables. But, since we might still be running out of CAR, car_active needs - * to return 1 if ENV_CACHE_AS_RAM is set. + * For all stages other than romstage, all CAR_GLOBAL variables are accessed + * unconditionally as there is no migration of symbols. */
#define CAR_GLOBAL -static inline void *car_get_var_ptr(void *var) { return var; } - -#if ENV_CACHE_AS_RAM -static inline int car_active(void) { return 1; } -#else -static inline int car_active(void) { return 0; } -#endif /* ENV_CACHE_AS_RAM */ - #define car_get_var(var) (var) #define car_sync_var(var) (var) #define car_set_var(var, val) (var) = (val) -#endif /* ENV_CACHE_AS_RAM && CONFIG(CAR_GLOBAL_MIGRATION) */ + +static inline void *car_get_var_ptr(void *var) +{ + return var; +} + +static inline int car_active(void) +{ + return ENV_ROMSTAGE_OR_BEFORE; +} + +#endif
#endif /* ARCH_EARLY_VARIABLES_H */ diff --git a/src/cpu/x86/pae/pgtbl.c b/src/cpu/x86/pae/pgtbl.c index f54a1c3..4673de6 100644 --- a/src/cpu/x86/pae/pgtbl.c +++ b/src/cpu/x86/pae/pgtbl.c @@ -363,7 +363,7 @@
int paging_enable_for_car(const char *pdpt_name, const char *pt_name) { - if (!ENV_CACHE_AS_RAM) + if (!preram_symbols_available()) return -1;
if (read_from_cbfs(pdpt_name, _pdpt, REGION_SIZE(pdpt))) { @@ -383,7 +383,7 @@
static void *get_pdpt_addr(void) { - if (ENV_CACHE_AS_RAM) + if (preram_symbols_available()) return _pdpt; return (void *)(uintptr_t)read_cr3(); } diff --git a/src/include/rules.h b/src/include/rules.h index aec612e..e7ae05d 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -297,12 +297,4 @@ #define __SIMPLE_DEVICE__ #endif
-/* x86 specific. Indicates that the current stage is running with cache-as-ram - * enabled from the beginning of the stage in C code. */ -#if defined(__PRE_RAM__) -#define ENV_CACHE_AS_RAM CONFIG(ARCH_X86) -#else -#define ENV_CACHE_AS_RAM 0 -#endif - #endif /* _RULES_H */ diff --git a/src/include/symbols.h b/src/include/symbols.h index 76c9320..56df8d5 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -72,7 +72,7 @@ * (Does not necessarily mean that the memory is accessible.) */ static inline int preram_symbols_available(void) { - return !CONFIG(ARCH_X86) || ENV_CACHE_AS_RAM; + return !CONFIG(ARCH_X86) || ENV_ROMSTAGE_OR_BEFORE; }
#endif /* __SYMBOLS_H */
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
Patch Set 2: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
Patch Set 2: Code-Review+1
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35016
to look at the new patch set (#3).
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
arch/x86: Simplify <arch/early_variables.h>
This enables the use of .bss section for ENV_BOOTBLOCK and ENV_VERSTAGE even with CAR_GLOBAL_MIGRATION=y.
In practice, boards with CAR_GLOBAL_MIGRATION=y currently build with romcc-bootblock so they will not be using .bss.
Change-Id: Ie9dc14f3e528d3e4f48304f4d7de50df448a8af6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/include/symbols.h 5 files changed, 23 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35016/3
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35016
to look at the new patch set (#4).
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
arch/x86: Simplify <arch/early_variables.h>
This enables the use of .bss section for ENV_BOOTBLOCK and ENV_VERSTAGE even with CAR_GLOBAL_MIGRATION=y.
In practice, boards with CAR_GLOBAL_MIGRATION=y currently build with romcc-bootblock so they will not be using .bss.
Change-Id: Ie9dc14f3e528d3e4f48304f4d7de50df448a8af6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/include/symbols.h M src/lib/program.ld 6 files changed, 25 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35016/4
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35016
to look at the new patch set (#5).
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
arch/x86: Simplify <arch/early_variables.h>
This enables the use of .bss section for ENV_BOOTBLOCK and ENV_VERSTAGE even with CAR_GLOBAL_MIGRATION=y.
In practice, boards with CAR_GLOBAL_MIGRATION=y currently build with romcc-bootblock so they will not be using .bss.
Change-Id: Ie9dc14f3e528d3e4f48304f4d7de50df448a8af6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/lib/program.ld 5 files changed, 24 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35016/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35016/5/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/35016/5/src/lib/program.ld@141 PS5, Line 141: #if ENV_STAGE_HAS_HEAP_SECTION && !ENV_CACHE_AS_RAM nit: Isn't this one unnecessary?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35016/5/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/35016/5/src/lib/program.ld@141 PS5, Line 141: #if ENV_STAGE_HAS_HEAP_SECTION && !ENV_CACHE_AS_RAM
nit: Isn't this one unnecessary?
If we did add .heap inside CAR, this becomes equally necessary as the .bss case above. Then again, for consistency STAGE_HAS_DATA should have this added too? We can go either way.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35016
to look at the new patch set (#6).
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
arch/x86: Simplify <arch/early_variables.h>
This enables the use of .bss section for ENV_BOOTBLOCK and ENV_VERSTAGE even with CAR_GLOBAL_MIGRATION=y.
In practice, boards with CAR_GLOBAL_MIGRATION=y currently build with romcc-bootblock so they will not be using .bss.
Change-Id: Ie9dc14f3e528d3e4f48304f4d7de50df448a8af6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/lib/program.ld 5 files changed, 23 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/35016/6
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35016/5/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/35016/5/src/lib/program.ld@141 PS5, Line 141: #if ENV_STAGE_HAS_HEAP_SECTION && !ENV_CACHE_AS_RAM
If we did add .heap inside CAR, this becomes equally necessary as the .bss case above. […]
I'd say add it when we need it.
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/35016 )
Change subject: arch/x86: Simplify <arch/early_variables.h> ......................................................................
arch/x86: Simplify <arch/early_variables.h>
This enables the use of .bss section for ENV_BOOTBLOCK and ENV_VERSTAGE even with CAR_GLOBAL_MIGRATION=y.
In practice, boards with CAR_GLOBAL_MIGRATION=y currently build with romcc-bootblock so they will not be using .bss.
Change-Id: Ie9dc14f3e528d3e4f48304f4d7de50df448a8af6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35016 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/arch/x86/car.ld M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/lib/program.ld 5 files changed, 23 insertions(+), 45 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 5351fc7..641ae1f 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -76,8 +76,8 @@ * 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 = .; -#if !CONFIG(CAR_GLOBAL_MIGRATION) - /* Allow global unitialized variables when CAR_GLOBALs are not used. */ +#if ENV_STAGE_HAS_BSS_SECTION + /* Allow global uninitialized variables for stages without CAR teardown. */ *(.bss) *(.bss.*) *(.sbss) diff --git a/src/arch/x86/include/arch/early_variables.h b/src/arch/x86/include/arch/early_variables.h index 4860e49..3910a82 100644 --- a/src/arch/x86/include/arch/early_variables.h +++ b/src/arch/x86/include/arch/early_variables.h @@ -20,7 +20,8 @@ #include <commonlib/helpers.h> #include <stdlib.h>
-#if ENV_CACHE_AS_RAM && CONFIG(CAR_GLOBAL_MIGRATION) +#if ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION) + asm(".section .car.global_data,"w",@nobits"); asm(".previous"); #ifdef __clang__ @@ -29,27 +30,6 @@ #define CAR_GLOBAL __attribute__((used, section(".car.global_data#"))) #endif /* __clang__ */
-/* - * In stages that use CAR (verstage, C bootblock) all CAR_GLOBAL variables are - * accessed unconditionally because cbmem is never initialized until romstage - * when dram comes up. - */ -#if !ENV_ROMSTAGE -static inline void *car_get_var_ptr(void *var) -{ - return var; -} - -static inline void *car_sync_var_ptr(void *var) -{ - return var; -} - -static inline int car_active(void) -{ - return 1; -} -#else /* Get the correct pointer for the CAR global variable. */ void *car_get_var_ptr(void *var);
@@ -58,7 +38,6 @@
/* Return 1 when currently running with globals in Cache-as-RAM, 0 otherwise. */ int car_active(void); -#endif /* !ENV_ROMSTAGE */
/* Get and set a primitive type global variable. */ #define car_get_var(var) \ @@ -81,26 +60,25 @@ #else
/* - * We might end up here if: - * 1. ENV_CACHE_AS_RAM is not set for the stage or - * 2. ENV_CACHE_AS_RAM is set for the stage but CONFIG_CAR_GLOBAL_MIGRATION - * is not set. In this case, there is no need to migrate CAR global - * variables. But, since we might still be running out of CAR, car_active needs - * to return 1 if ENV_CACHE_AS_RAM is set. + * For all stages other than romstage, all CAR_GLOBAL variables are accessed + * unconditionally as there is no migration of symbols. */
#define CAR_GLOBAL -static inline void *car_get_var_ptr(void *var) { return var; } - -#if ENV_CACHE_AS_RAM -static inline int car_active(void) { return 1; } -#else -static inline int car_active(void) { return 0; } -#endif /* ENV_CACHE_AS_RAM */ - #define car_get_var(var) (var) #define car_sync_var(var) (var) #define car_set_var(var, val) (var) = (val) -#endif /* ENV_CACHE_AS_RAM && CONFIG(CAR_GLOBAL_MIGRATION) */ + +static inline void *car_get_var_ptr(void *var) +{ + return var; +} + +static inline int car_active(void) +{ + return ENV_CACHE_AS_RAM; +} + +#endif
#endif /* ARCH_EARLY_VARIABLES_H */ diff --git a/src/cpu/x86/pae/pgtbl.c b/src/cpu/x86/pae/pgtbl.c index f54a1c3..4673de6 100644 --- a/src/cpu/x86/pae/pgtbl.c +++ b/src/cpu/x86/pae/pgtbl.c @@ -363,7 +363,7 @@
int paging_enable_for_car(const char *pdpt_name, const char *pt_name) { - if (!ENV_CACHE_AS_RAM) + if (!preram_symbols_available()) return -1;
if (read_from_cbfs(pdpt_name, _pdpt, REGION_SIZE(pdpt))) { @@ -383,7 +383,7 @@
static void *get_pdpt_addr(void) { - if (ENV_CACHE_AS_RAM) + if (preram_symbols_available()) return _pdpt; return (void *)(uintptr_t)read_cr3(); } diff --git a/src/include/rules.h b/src/include/rules.h index 8a6945b..9fd7dc3 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -274,8 +274,8 @@ #define ENV_CACHE_AS_RAM ENV_ROMSTAGE_OR_BEFORE /* No .data sections with execute-in-place from ROM. */ #define ENV_STAGE_HAS_DATA_SECTION !ENV_CACHE_AS_RAM -/* No .bss sections with execute-in-place from ROM. */ -#define ENV_STAGE_HAS_BSS_SECTION !ENV_CACHE_AS_RAM +/* No .bss sections for stage with CAR teardown. */ +#define ENV_STAGE_HAS_BSS_SECTION !(ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION)) #else /* Both .data and .bss, sometimes SRAM not DRAM. */ #define ENV_STAGE_HAS_DATA_SECTION 1 diff --git a/src/lib/program.ld b/src/lib/program.ld index 1b30c0e..ad364a4 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -125,7 +125,7 @@ } #endif
-#if ENV_STAGE_HAS_BSS_SECTION +#if ENV_STAGE_HAS_BSS_SECTION && !ENV_CACHE_AS_RAM .bss . : { . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .;
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)