From 644a512e560147324ecf74ebce5e336bc57dd7bf Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Tue, 25 Aug 2020 16:00:44 -0700 Subject: [PATCH] symbols: Change implementation details of DECLARE_OPTIONAL_REGION() It seems that GCC's LTO doesn't like the way we implement DECLARE_OPTIONAL_REGION(). This patch changes it so that rather than having a normal DECLARE_REGION() in and then an extra DECLARE_OPTIONAL_REGION() in the C file using it, you just say DECLARE_OPTIONAL_REGION() directly in (in place and instead of the usual DECLARE_REGION()). This basically looks the same way in the resulting object file but somehow LTO seems to like it better. Signed-off-by: Julius Werner Change-Id: I6096207b311d70c8e9956cd9406bec45be04a4a2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44791 Tested-by: build bot (Jenkins) Reviewed-by: Jacob Garber Reviewed-by: Hung-Te Lin Reviewed-by: HAOUAS Elyes --- src/arch/arm/armv7/mmu.c | 3 --- src/arch/arm/tables.c | 2 -- src/arch/arm64/tables.c | 2 -- src/arch/riscv/tables.c | 2 -- src/include/symbols.h | 29 +++++++++++++---------- src/lib/bootblock.c | 2 -- src/lib/timestamp.c | 2 -- src/soc/cavium/common/bootblock.c | 2 -- src/vendorcode/google/chromeos/symbols.h | 2 +- src/vendorcode/google/chromeos/watchdog.c | 2 -- 10 files changed, 17 insertions(+), 31 deletions(-) diff --git a/src/arch/arm/armv7/mmu.c b/src/arch/arm/armv7/mmu.c index 51b4860768..66ce53392d 100644 --- a/src/arch/arm/armv7/mmu.c +++ b/src/arch/arm/armv7/mmu.c @@ -87,9 +87,6 @@ typedef uint32_t pte_t; static pte_t *const ttb_buff = (void *)_ttb; -/* Not all boards want to use subtables and declare them in memlayout.ld. */ -DECLARE_OPTIONAL_REGION(ttb_subtables); - static struct { pte_t value; const char *name; diff --git a/src/arch/arm/tables.c b/src/arch/arm/tables.c index 3b47a5bf0c..0c68fc7c51 100644 --- a/src/arch/arm/tables.c +++ b/src/arch/arm/tables.c @@ -11,8 +11,6 @@ void arch_write_tables(uintptr_t coreboot_table) void bootmem_arch_add_ranges(void) { - DECLARE_OPTIONAL_REGION(ttb_subtables); - bootmem_add_range((uintptr_t)_ttb, REGION_SIZE(ttb), BM_MEM_RAMSTAGE); bootmem_add_range((uintptr_t)_ttb_subtables, REGION_SIZE(ttb_subtables), BM_MEM_RAMSTAGE); diff --git a/src/arch/arm64/tables.c b/src/arch/arm64/tables.c index 321d348602..b97297c1b9 100644 --- a/src/arch/arm64/tables.c +++ b/src/arch/arm64/tables.c @@ -5,8 +5,6 @@ #include #include -DECLARE_OPTIONAL_REGION(bl31); - void arch_write_tables(uintptr_t coreboot_table) { } diff --git a/src/arch/riscv/tables.c b/src/arch/riscv/tables.c index 4935ef5ece..9fc75f455c 100644 --- a/src/arch/riscv/tables.c +++ b/src/arch/riscv/tables.c @@ -5,8 +5,6 @@ #include #include -DECLARE_OPTIONAL_REGION(opensbi); - void arch_write_tables(uintptr_t coreboot_table) { } diff --git a/src/include/symbols.h b/src/include/symbols.h index fe3f46ab80..371d84bf9b 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -13,8 +13,20 @@ extern u8 _dram[]; extern u8 _##name[]; \ extern u8 _e##name[]; +/* + * Regions can be declared optional if not all configurations provide them in + * memlayout and you want code to be able to check for their existence at + * runtime. Not every region that is architecture or platform-specific should + * use this -- only declare regions optional if the code *accessing* them runs + * both on configurations that have the region and those that don't. That code + * should then check (REGION_SIZE(name) != 0) before accessing it. + */ +#define DECLARE_OPTIONAL_REGION(name) \ + __weak extern u8 _##name[]; \ + __weak extern u8 _e##name[]; + DECLARE_REGION(sram) -DECLARE_REGION(timestamp) +DECLARE_OPTIONAL_REGION(timestamp) DECLARE_REGION(preram_cbmem_console) DECLARE_REGION(cbmem_init_hooks) DECLARE_REGION(stack) @@ -53,24 +65,15 @@ DECLARE_REGION(ramstage) DECLARE_REGION(pagetables) DECLARE_REGION(ttb) -DECLARE_REGION(ttb_subtables) +DECLARE_OPTIONAL_REGION(ttb_subtables) DECLARE_REGION(dma_coherent) DECLARE_REGION(soc_registers) DECLARE_REGION(framebuffer) DECLARE_REGION(pdpt) -DECLARE_REGION(opensbi) -DECLARE_REGION(bl31) +DECLARE_OPTIONAL_REGION(opensbi) +DECLARE_OPTIONAL_REGION(bl31) DECLARE_REGION(transfer_buffer) -/* - * Put this into a .c file accessing a linker script region to mark that region - * as "optional". If it is defined in memlayout.ld (or anywhere else), the - * values from that definition will be used. If not, start, end and size will - * all evaluate to 0. (We can't explicitly assign the symbols to 0 in the - * assembly due to https://sourceware.org/bugzilla/show_bug.cgi?id=1038.) - */ -#define DECLARE_OPTIONAL_REGION(name) asm (".weak _" #name ", _e" #name) - /* Returns true when pre-RAM symbols are known to the linker. * (Does not necessarily mean that the memory is accessible.) */ static inline int preram_symbols_available(void) diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c index 1c433d008e..7ee246e459 100644 --- a/src/lib/bootblock.c +++ b/src/lib/bootblock.c @@ -10,8 +10,6 @@ #include #include -DECLARE_OPTIONAL_REGION(timestamp); - __weak void bootblock_mainboard_early_init(void) { /* no-op */ } __weak void bootblock_soc_early_init(void) { /* do nothing */ } __weak void bootblock_soc_init(void) { /* do nothing */ } diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 5121eb859c..7347d07b16 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -11,8 +11,6 @@ #define MAX_TIMESTAMPS 192 -DECLARE_OPTIONAL_REGION(timestamp); - /* This points to the active timestamp_table and can change within a stage as CBMEM comes available. */ static struct timestamp_table *glob_ts_table; diff --git a/src/soc/cavium/common/bootblock.c b/src/soc/cavium/common/bootblock.c index 80a41fa375..2fca0abbac 100644 --- a/src/soc/cavium/common/bootblock.c +++ b/src/soc/cavium/common/bootblock.c @@ -8,8 +8,6 @@ #include #include -DECLARE_OPTIONAL_REGION(timestamp); - __attribute__((weak)) void bootblock_mainboard_early_init(void) { /* no-op */ } __attribute__((weak)) void bootblock_soc_early_init(void) { /* do nothing */ } __attribute__((weak)) void bootblock_soc_init(void) { /* do nothing */ } diff --git a/src/vendorcode/google/chromeos/symbols.h b/src/vendorcode/google/chromeos/symbols.h index 1e966da338..45f9c3e10b 100644 --- a/src/vendorcode/google/chromeos/symbols.h +++ b/src/vendorcode/google/chromeos/symbols.h @@ -5,6 +5,6 @@ #include -DECLARE_REGION(watchdog_tombstone) +DECLARE_OPTIONAL_REGION(watchdog_tombstone) #endif /* __CHROMEOS_SYMBOLS_H */ diff --git a/src/vendorcode/google/chromeos/watchdog.c b/src/vendorcode/google/chromeos/watchdog.c index 5b3f1844a7..d3273cb442 100644 --- a/src/vendorcode/google/chromeos/watchdog.c +++ b/src/vendorcode/google/chromeos/watchdog.c @@ -14,8 +14,6 @@ #define WATCHDOG_TOMBSTONE_MAGIC 0x9d2f41a7 -DECLARE_OPTIONAL_REGION(watchdog_tombstone); - static void elog_handle_watchdog_tombstone(void *unused) { bool flag = false;