From 37513297d38967f6fc748dbaf357a1e83ac54b31 Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Tue, 20 May 2025 14:27:35 -0700 Subject: [PATCH] libpayload: Use Kconfig for architecture memcpy, not weak symbols Our mechanism to override the default (pure C) memory function implementations (memset, memcpy, memmove) with architecture-specific optimized assembly versions doesn't actually work: it turns out that weak functions don't work as you'd naively expect when you pack them together with a strong definition from a different object into a static library. When a linker tries to resolve a symbol from a static library, it just picks the first one it finds, even if it is weak. It doesn't evaluate all objects in the library to see if there are other strong definitions. To fix this, this patch gets rid of the weak symbols and uses Kconfigs instead. It adds an optimized memmove() implementation for x86 because that makes things easier (then all architectures either override all three functions or none of them). Also remove memcmp() from the functions that can be overridden for now because nobody ever needed that anyway. Change-Id: Iedf9898247f1999e56fde3233fad8b7cb36b1269 Signed-off-by: Julius Werner Reviewed-on: https://review.coreboot.org/c/coreboot/+/87766 Reviewed-by: Yu-Ping Wu Reviewed-by: Subrata Banik Tested-by: build bot (Jenkins) --- payloads/libpayload/Kconfig | 7 +++++ payloads/libpayload/arch/arm/Kconfig | 1 + payloads/libpayload/arch/arm64/Kconfig | 1 + payloads/libpayload/arch/x86/Kconfig | 1 + payloads/libpayload/arch/x86/string.c | 42 ++++++++++++++++++++++++++ payloads/libpayload/include/string.h | 9 ++++++ payloads/libpayload/libc/memory.c | 25 +++++++-------- 7 files changed, 72 insertions(+), 14 deletions(-) diff --git a/payloads/libpayload/Kconfig b/payloads/libpayload/Kconfig index b5da2d9a4a..ea9189463b 100644 --- a/payloads/libpayload/Kconfig +++ b/payloads/libpayload/Kconfig @@ -505,6 +505,13 @@ config IO_ADDRESS_SPACE This option is turned on if the target system has a separate IO address space. This is typically only the case on x86. +config ARCH_HAS_MEM_FUNCTIONS + default n + bool + help + Architectures must select this when they want to override memset, + memcpy and memmove with optimized assembly implementations. + source "arch/arm/Kconfig" source "arch/arm64/Kconfig" source "arch/x86/Kconfig" diff --git a/payloads/libpayload/arch/arm/Kconfig b/payloads/libpayload/arch/arm/Kconfig index 4b88b18ce2..35095115d7 100644 --- a/payloads/libpayload/arch/arm/Kconfig +++ b/payloads/libpayload/arch/arm/Kconfig @@ -30,3 +30,4 @@ config ARCH_ARM select LITTLE_ENDIAN + select ARCH_HAS_MEM_FUNCTIONS if GPL diff --git a/payloads/libpayload/arch/arm64/Kconfig b/payloads/libpayload/arch/arm64/Kconfig index 60beb0bd5a..ea5cd3a48c 100644 --- a/payloads/libpayload/arch/arm64/Kconfig +++ b/payloads/libpayload/arch/arm64/Kconfig @@ -30,6 +30,7 @@ config ARCH_ARM64 select LITTLE_ENDIAN + select ARCH_HAS_MEM_FUNCTIONS if ARCH_ARM64 diff --git a/payloads/libpayload/arch/x86/Kconfig b/payloads/libpayload/arch/x86/Kconfig index e59bd7ef6a..00b0ad0e70 100644 --- a/payloads/libpayload/arch/x86/Kconfig +++ b/payloads/libpayload/arch/x86/Kconfig @@ -31,6 +31,7 @@ config ARCH_X86 select LITTLE_ENDIAN select IO_ADDRESS_SPACE + select ARCH_HAS_MEM_FUNCTIONS if GPL if ARCH_X86 diff --git a/payloads/libpayload/arch/x86/string.c b/payloads/libpayload/arch/x86/string.c index 11bcae7acb..4c0716f446 100644 --- a/payloads/libpayload/arch/x86/string.c +++ b/payloads/libpayload/arch/x86/string.c @@ -103,3 +103,45 @@ void *memcpy(void *dest, const void *src, size_t n) return dest; } + +void *memmove(void *dest, const void *src, size_t n) +{ + unsigned long d0, d1, d2; + + if (dest < src) + return memcpy(dest, src, n); + +#if CONFIG(LP_ARCH_X86_64) + asm volatile( + "std\n\t" + "dec %%rdi\n\t" + "dec %%rsi\n\t" + "rep ; movsb\n\t" + "mov %4, %%rcx\n\t" + "sub $7, %%rdi\n\t" + "sub $7, %%rsi\n\t" + "rep ; movsq\n\t" + "cld\n\t" + : "=&c" (d0), "=&D" (d1), "=&S" (d2) + : "0" (n & 7), "g" (n >> 3), "1" (dest + n), "2" (src + n) + : "memory" + ); +#else + asm volatile( + "std\n\t" + "dec %%edi\n\t" + "dec %%esi\n\t" + "rep ; movsb\n\t" + "mov %4, %%ecx\n\t" + "sub $3, %%edi\n\t" + "sub $3, %%esi\n\t" + "rep ; movsl\n\t" + "cld\n\t" + : "=&c" (d0), "=&D" (d1), "=&S" (d2) + : "0" (n & 3), "g" (n >> 2), "1" (dest + n), "2" (src + n) + : "memory" + ); +#endif + + return dest; +} diff --git a/payloads/libpayload/include/string.h b/payloads/libpayload/include/string.h index b0028228eb..40776ff78e 100644 --- a/payloads/libpayload/include/string.h +++ b/payloads/libpayload/include/string.h @@ -43,6 +43,15 @@ void *memchr(const void *s, int c, size_t n); int memcmp(const void *s1, const void *s2, size_t len); /** @} */ +/** + * @defgroup default memory functions remain available under separate names, in + * case architecture implementations want to fall back to them in certain cases. + */ +void *default_memset(void *s, int c, size_t n); +void *default_memcpy(void *dst, const void *src, size_t n); +void *default_memmove(void *dst, const void *src, size_t n); +/** @} */ + /** * @defgroup string String functions * @{ diff --git a/payloads/libpayload/libc/memory.c b/payloads/libpayload/libc/memory.c index fff295e54c..80d7a5f507 100644 --- a/payloads/libpayload/libc/memory.c +++ b/payloads/libpayload/libc/memory.c @@ -32,7 +32,7 @@ #include -static void *default_memset(void *const s, const int c, size_t n) +void *default_memset(void *const s, const int c, size_t n) { size_t i; u8 *dst = s; @@ -57,10 +57,7 @@ static void *default_memset(void *const s, const int c, size_t n) return s; } -void *memset(void *s, int c, size_t n) - __attribute__((weak, alias("default_memset"))); - -static void *default_memcpy(void *dst, const void *src, size_t n) +void *default_memcpy(void *dst, const void *src, size_t n) { size_t i; void *ret = dst; @@ -81,10 +78,7 @@ static void *default_memcpy(void *dst, const void *src, size_t n) return ret; } -void *memcpy(void *dst, const void *src, size_t n) - __attribute__((weak, alias("default_memcpy"))); - -static void *default_memmove(void *dst, const void *src, size_t n) +void *default_memmove(void *dst, const void *src, size_t n) { size_t offs; ssize_t i; @@ -110,8 +104,14 @@ static void *default_memmove(void *dst, const void *src, size_t n) return dst; } +#if !CONFIG(LP_ARCH_HAS_MEM_FUNCTIONS) +void *memcpy(void *dst, const void *src, size_t n) + __attribute__((alias("default_memcpy"))); +void *memset(void *s, int c, size_t n) + __attribute__((alias("default_memset"))); void *memmove(void *dst, const void *src, size_t n) - __attribute__((weak, alias("default_memmove"))); + __attribute__((alias("default_memmove"))); +#endif /** * Compare two memory areas. @@ -124,7 +124,7 @@ void *memmove(void *dst, const void *src, size_t n) * greater than s2 respectively. */ -static int default_memcmp(const void *s1, const void *s2, size_t n) +int memcmp(const void *s1, const void *s2, size_t n) { size_t i = 0; const unsigned long *w1 = s1, *w2 = s2; @@ -142,9 +142,6 @@ static int default_memcmp(const void *s1, const void *s2, size_t n) return 0; } -int memcmp(const void *s1, const void *s2, size_t n) - __attribute__((weak, alias("default_memcmp"))); - void *memchr(const void *s, int c, size_t n) { unsigned char *p = (unsigned char *)s;