[PATCH 4/6] arm: Use the WEAK assembly entry point consistently
Pali Rohár
pali at kernel.org
Wed Nov 23 23:50:59 CET 2022
On Tuesday 22 November 2022 12:31:56 Tom Rini wrote:
> It is a bad idea, and more modern toolchains will fail, if you declare
> an assembly function to be global and then weak, instead of declaring it
> weak to start with. Update assorted assembly files to use the WEAK macro
> directly.
>
> Signed-off-by: Tom Rini <trini at konsulko.com>
During debugging of Nokia N900 code I was looking at this and I
originally thought that this redefinition is the issue why N900 u-boot
did not work... (but as we now know, the n900 issue was somewhere else).
So I agree with this change, feel free to add my:
Reviewed-by: Pali Rohár <pali at kernel.org>
... but even after this change, linked u-boot.bin binary is
not-so-correct. It works but has an issue: In final u-boot.bin binary
there is both weak and non-weak version of every weak function. You can
verify it for example by looking at "save_boot_params" code (really
code, not just symbol) in u-boot ELF binary.
The reason for this is that linker (even LTO enabled) cannot eliminate
code for weak version of function because it does not know how to
"drop" it from binary/assembly code. So linker just set that non-weak
version of function is active and let non-weak version present in binary
as probably dead code.
This is affected only by assembly files, not by C files, because gcc is
called with -ffunction-sections -fdata-sections flags which cause that
every (weak) function is in its separate section (so C function
"int abc() { ... }" is put into the section ".text.abc" instead of
".text") and linker's flag --gc-sections (or LTO optimization) then drop
all unreferenced sections.
I do not know how fix this issue in assembly files. But cannot be WEAK
macro modified to change section to ".text.<entry_name>" to mimic C
compiler behavior? Would this cause any issues?
> ---
> arch/arm/cpu/armv7/nonsec_virt.S | 3 +-
> arch/arm/cpu/armv7/psci.S | 77 +++++++++++---------------------
> arch/arm/cpu/armv7/start.S | 6 +--
> arch/arm/cpu/armv8/psci.S | 12 ++---
> arch/arm/lib/relocate.S | 3 +-
> 5 files changed, 34 insertions(+), 67 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index 39aeeb423f08..5ee6577c2c3c 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -207,7 +207,7 @@ ENDPROC(_nonsec_init)
>
> #ifdef CONFIG_SMP_PEN_ADDR
> /* void __weak smp_waitloop(unsigned previous_address); */
> -ENTRY(smp_waitloop)
> +WEAK(smp_waitloop)
> wfi
> ldr r1, =CONFIG_SMP_PEN_ADDR @ load start address
> ldr r1, [r1]
> @@ -219,7 +219,6 @@ ENTRY(smp_waitloop)
> mov r0, r1
> b _do_nonsec_entry
> ENDPROC(smp_waitloop)
> -.weak smp_waitloop
> #endif
>
> .popsection
> diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
> index 983cd9044295..6c066e50d91e 100644
> --- a/arch/arm/cpu/armv7/psci.S
> +++ b/arch/arm/cpu/armv7/psci.S
> @@ -36,34 +36,32 @@ _psci_vectors:
> b default_psci_vector @ irq
> b psci_fiq_enter @ fiq
>
> -ENTRY(psci_fiq_enter)
> +WEAK(psci_fiq_enter)
> movs pc, lr
> ENDPROC(psci_fiq_enter)
> -.weak psci_fiq_enter
>
> -ENTRY(default_psci_vector)
> +WEAK(default_psci_vector)
> movs pc, lr
> ENDPROC(default_psci_vector)
> -.weak default_psci_vector
> -
> -ENTRY(psci_version)
> -ENTRY(psci_cpu_suspend)
> -ENTRY(psci_cpu_off)
> -ENTRY(psci_cpu_on)
> -ENTRY(psci_affinity_info)
> -ENTRY(psci_migrate)
> -ENTRY(psci_migrate_info_type)
> -ENTRY(psci_migrate_info_up_cpu)
> -ENTRY(psci_system_off)
> -ENTRY(psci_system_reset)
> -ENTRY(psci_features)
> -ENTRY(psci_cpu_freeze)
> -ENTRY(psci_cpu_default_suspend)
> -ENTRY(psci_node_hw_state)
> -ENTRY(psci_system_suspend)
> -ENTRY(psci_set_suspend_mode)
> -ENTRY(psi_stat_residency)
> -ENTRY(psci_stat_count)
> +
> +WEAK(psci_version)
> +WEAK(psci_cpu_suspend)
> +WEAK(psci_cpu_off)
> +WEAK(psci_cpu_on)
> +WEAK(psci_affinity_info)
> +WEAK(psci_migrate)
> +WEAK(psci_migrate_info_type)
> +WEAK(psci_migrate_info_up_cpu)
> +WEAK(psci_system_off)
> +WEAK(psci_system_reset)
> +WEAK(psci_features)
> +WEAK(psci_cpu_freeze)
> +WEAK(psci_cpu_default_suspend)
> +WEAK(psci_node_hw_state)
> +WEAK(psci_system_suspend)
> +WEAK(psci_set_suspend_mode)
> +WEAK(psi_stat_residency)
> +WEAK(psci_stat_count)
> mov r0, #ARM_PSCI_RET_NI @ Return -1 (Not Implemented)
> mov pc, lr
> ENDPROC(psci_stat_count)
> @@ -84,24 +82,6 @@ ENDPROC(psci_cpu_on)
> ENDPROC(psci_cpu_off)
> ENDPROC(psci_cpu_suspend)
> ENDPROC(psci_version)
> -.weak psci_version
> -.weak psci_cpu_suspend
> -.weak psci_cpu_off
> -.weak psci_cpu_on
> -.weak psci_affinity_info
> -.weak psci_migrate
> -.weak psci_migrate_info_type
> -.weak psci_migrate_info_up_cpu
> -.weak psci_system_off
> -.weak psci_system_reset
> -.weak psci_features
> -.weak psci_cpu_freeze
> -.weak psci_cpu_default_suspend
> -.weak psci_node_hw_state
> -.weak psci_system_suspend
> -.weak psci_set_suspend_mode
> -.weak psi_stat_residency
> -.weak psci_stat_count
>
> _psci_table:
> .word ARM_PSCI_FN_CPU_SUSPEND
> @@ -179,12 +159,11 @@ _smc_psci:
> movs pc, lr @ Return to the kernel
>
> @ Requires dense and single-cluster CPU ID space
> -ENTRY(psci_get_cpu_id)
> +WEAK(psci_get_cpu_id)
> mrc p15, 0, r0, c0, c0, 5 /* read MPIDR */
> and r0, r0, #0xff /* return CPU ID in cluster */
> bx lr
> ENDPROC(psci_get_cpu_id)
> -.weak psci_get_cpu_id
>
> /* Imported from Linux kernel */
> ENTRY(psci_v7_flush_dcache_all)
> @@ -236,7 +215,7 @@ finished:
> bx lr
> ENDPROC(psci_v7_flush_dcache_all)
>
> -ENTRY(psci_disable_smp)
> +WEAK(psci_disable_smp)
> mrc p15, 0, r0, c1, c0, 1 @ ACTLR
> bic r0, r0, #(1 << 6) @ Clear SMP bit
> mcr p15, 0, r0, c1, c0, 1 @ ACTLR
> @@ -244,16 +223,14 @@ ENTRY(psci_disable_smp)
> dsb
> bx lr
> ENDPROC(psci_disable_smp)
> -.weak psci_disable_smp
>
> -ENTRY(psci_enable_smp)
> +WEAK(psci_enable_smp)
> mrc p15, 0, r0, c1, c0, 1 @ ACTLR
> orr r0, r0, #(1 << 6) @ Set SMP bit
> mcr p15, 0, r0, c1, c0, 1 @ ACTLR
> isb
> bx lr
> ENDPROC(psci_enable_smp)
> -.weak psci_enable_smp
>
> ENTRY(psci_cpu_off_common)
> push {lr}
> @@ -316,15 +293,13 @@ ENTRY(psci_stack_setup)
> bx r6
> ENDPROC(psci_stack_setup)
>
> -ENTRY(psci_arch_init)
> +WEAK(psci_arch_init)
> mov pc, lr
> ENDPROC(psci_arch_init)
> -.weak psci_arch_init
>
> -ENTRY(psci_arch_cpu_entry)
> +WEAK(psci_arch_cpu_entry)
> mov pc, lr
> ENDPROC(psci_arch_cpu_entry)
> -.weak psci_arch_cpu_entry
>
> ENTRY(psci_cpu_entry)
> bl psci_enable_smp
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index 4f6327fe3ab7..7d7aac021e22 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -151,16 +151,14 @@ ENDPROC(c_runtime_cpu_setup)
> * Don't save anything to stack even if compiled with -O0
> *
> *************************************************************************/
> -ENTRY(save_boot_params)
> +WEAK(save_boot_params)
> b save_boot_params_ret @ back to my caller
> ENDPROC(save_boot_params)
> - .weak save_boot_params
>
> #ifdef CONFIG_ARMV7_LPAE
> -ENTRY(switch_to_hypervisor)
> +WEAK(switch_to_hypervisor)
> b switch_to_hypervisor_ret
> ENDPROC(switch_to_hypervisor)
> - .weak switch_to_hypervisor
> #endif
>
> /*************************************************************************
> diff --git a/arch/arm/cpu/armv8/psci.S b/arch/arm/cpu/armv8/psci.S
> index 7ffc8dbadbe0..6aece1198712 100644
> --- a/arch/arm/cpu/armv8/psci.S
> +++ b/arch/arm/cpu/armv8/psci.S
> @@ -12,11 +12,10 @@
>
> /* Default PSCI function, return -1, Not Implemented */
> #define PSCI_DEFAULT(__fn) \
> - ENTRY(__fn); \
> + WEAK(__fn); \
> mov w0, #ARM_PSCI_RET_NI; \
> ret; \
> ENDPROC(__fn); \
> - .weak __fn
>
> /* PSCI function and ID table definition*/
> #define PSCI_TABLE(__id, __fn) \
> @@ -207,7 +206,7 @@ handle_smc64:
> * used for the return value, while in this PSCI environment, X0 usually holds
> * the SMC function identifier, so X0 should be saved by caller function.
> */
> -ENTRY(psci_get_cpu_id)
> +WEAK(psci_get_cpu_id)
> #ifdef CONFIG_ARMV8_PSCI_CPUS_PER_CLUSTER
> mrs x9, MPIDR_EL1
> ubfx x9, x9, #8, #8
> @@ -221,7 +220,6 @@ ENTRY(psci_get_cpu_id)
> add x0, x10, x9
> ret
> ENDPROC(psci_get_cpu_id)
> -.weak psci_get_cpu_id
>
> /* CPU ID input in x0, stack top output in x0*/
> LENTRY(psci_get_cpu_stack_top)
> @@ -261,10 +259,9 @@ handle_sync:
> * Override this function if custom error handling is
> * needed for asynchronous aborts
> */
> -ENTRY(plat_error_handler)
> +WEAK(plat_error_handler)
> ret
> ENDPROC(plat_error_handler)
> -.weak plat_error_handler
>
> handle_error:
> bl psci_get_cpu_id
> @@ -323,9 +320,8 @@ ENTRY(psci_setup_vectors)
> ret
> ENDPROC(psci_setup_vectors)
>
> -ENTRY(psci_arch_init)
> +WEAK(psci_arch_init)
> ret
> ENDPROC(psci_arch_init)
> -.weak psci_arch_init
>
> .popsection
> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> index dd6f2e3bd5e0..345e282e3e65 100644
> --- a/arch/arm/lib/relocate.S
> +++ b/arch/arm/lib/relocate.S
> @@ -23,9 +23,8 @@
> */
>
> .section .text.relocate_vectors,"ax",%progbits
> - .weak relocate_vectors
>
> -ENTRY(relocate_vectors)
> +WEAK(relocate_vectors)
>
> #ifdef CONFIG_CPU_V7M
> /*
> --
> 2.25.1
>
More information about the U-Boot
mailing list