Change in coreboot[master]: endian: Replace explicit byte swapping with compiler builtin
Hello Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37343
to review the following change.
Change subject: endian: Replace explicit byte swapping with compiler builtin ......................................................................
endian: Replace explicit byte swapping with compiler builtin
gcc seems to have some stupid problem with deciding when to inline byte swapping functions (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92716). Using the compiler builtin instead seems to solve the problem.
(This doesn't yet solve the issue for the read_be32()-family of functions, which we should maybe just get rid of at some point?)
Change-Id: Ia2a6d8ea98987266ccc32ffaa0a7f78965fca1cd Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/endian.h D payloads/libpayload/include/swab.h M src/include/endian.h D src/include/swab.h M src/vendorcode/eltan/security/mboot/mboot.c M src/vendorcode/eltan/security/mboot/mboot.h 6 files changed, 20 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/37343/1
diff --git a/payloads/libpayload/include/endian.h b/payloads/libpayload/include/endian.h index b387e66..7f8b93a 100644 --- a/payloads/libpayload/include/endian.h +++ b/payloads/libpayload/include/endian.h @@ -34,23 +34,6 @@ #include <arch/types.h> #include <libpayload-config.h>
-static inline uint16_t swap_bytes16(uint16_t in) -{ - return ((in & 0xFF) << 8) | ((in & 0xFF00) >> 8); -} - -static inline uint32_t swap_bytes32(uint32_t in) -{ - return ((in & 0xFF) << 24) | ((in & 0xFF00) << 8) | - ((in & 0xFF0000) >> 8) | ((in & 0xFF000000) >> 24); -} - -static inline uint64_t swap_bytes64(uint64_t in) -{ - return ((uint64_t)swap_bytes32((uint32_t)in) << 32) | - ((uint64_t)swap_bytes32((uint32_t)(in >> 32))); -} - /* Endian functions from glibc 2.9 / BSD "endian.h" */
#if CONFIG(LP_BIG_ENDIAN) @@ -59,15 +42,15 @@ #define htobe32(in) (in) #define htobe64(in) (in)
-#define htole16(in) swap_bytes16(in) -#define htole32(in) swap_bytes32(in) -#define htole64(in) swap_bytes64(in) +#define htole16(in) __builtin_bswap16(in) +#define htole32(in) __builtin_bswap32(in) +#define htole64(in) __builtin_bswap64(in)
#elif CONFIG(LP_LITTLE_ENDIAN)
-#define htobe16(in) swap_bytes16(in) -#define htobe32(in) swap_bytes32(in) -#define htobe64(in) swap_bytes64(in) +#define htobe16(in) __builtin_bswap16(in) +#define htobe32(in) __builtin_bswap32(in) +#define htobe64(in) __builtin_bswap64(in)
#define htole16(in) (in) #define htole32(in) (in) diff --git a/payloads/libpayload/include/swab.h b/payloads/libpayload/include/swab.h deleted file mode 100644 index 2198077..0000000 --- a/payloads/libpayload/include/swab.h +++ /dev/null @@ -1,44 +0,0 @@ -#ifndef _SWAB_H -#define _SWAB_H - -/* - * linux/byteorder/swab.h - * Byte-swapping, independently from CPU endianness - * swabXX[ps]?(foo) - * - * Francois-Rene Rideau fare@tunes.org 19971205 - * separated swab functions from cpu_to_XX, - * to clean up support for bizarre-endian architectures. - * - * See asm-i386/byteorder.h and suches for examples of how to provide - * architecture-dependent optimized versions - * - */ - -/* casts are necessary for constants, because we never know for sure - * how U/UL/ULL map to __u16, __u32, __u64. At least not in a portable way. - */ -#define swab16(x) \ - ((unsigned short)( \ - (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ - (((unsigned short)(x) & (unsigned short)0xff00U) >> 8))) - -#define swab32(x) \ - ((unsigned int)( \ - (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ - (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ - (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ - (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24))) - -#define swab64(x) \ - ((uint64_t)( \ - (((uint64_t)(x) & (uint64_t)0x00000000000000ffULL) << 56) | \ - (((uint64_t)(x) & (uint64_t)0x000000000000ff00ULL) << 40) | \ - (((uint64_t)(x) & (uint64_t)0x0000000000ff0000ULL) << 24) | \ - (((uint64_t)(x) & (uint64_t)0x00000000ff000000ULL) << 8) | \ - (((uint64_t)(x) & (uint64_t)0x000000ff00000000ULL) >> 8) | \ - (((uint64_t)(x) & (uint64_t)0x0000ff0000000000ULL) >> 24) | \ - (((uint64_t)(x) & (uint64_t)0x00ff000000000000ULL) >> 40) | \ - (((uint64_t)(x) & (uint64_t)0xff00000000000000ULL) >> 56))) - -#endif /* _SWAB_H */ diff --git a/src/include/endian.h b/src/include/endian.h index 8dc1854..d4b1707 100644 --- a/src/include/endian.h +++ b/src/include/endian.h @@ -17,7 +17,6 @@
#include <arch/byteorder.h> #include <stdint.h> -#include <swab.h>
#if defined(__LITTLE_ENDIAN) #define cpu_to_le64(x) ((uint64_t)(x)) @@ -26,19 +25,19 @@ #define le32_to_cpu(x) ((uint32_t)(x)) #define cpu_to_le16(x) ((uint16_t)(x)) #define le16_to_cpu(x) ((uint16_t)(x)) - #define cpu_to_be64(x) swab64(x) - #define be64_to_cpu(x) swab64(x) - #define cpu_to_be32(x) swab32(x) - #define be32_to_cpu(x) swab32(x) - #define cpu_to_be16(x) swab16(x) - #define be16_to_cpu(x) swab16(x) + #define cpu_to_be64(x) __builtin_bswap64(x) + #define be64_to_cpu(x) __builtin_bswap64(x) + #define cpu_to_be32(x) __builtin_bswap32(x) + #define be32_to_cpu(x) __builtin_bswap32(x) + #define cpu_to_be16(x) __builtin_bswap16(x) + #define be16_to_cpu(x) __builtin_bswap16(x) #elif defined(__BIG_ENDIAN) - #define cpu_to_le64(x) swab64(x) - #define le64_to_cpu(x) swab64(x) - #define cpu_to_le32(x) swab32(x) - #define le32_to_cpu(x) swab32(x) - #define cpu_to_le16(x) swab16(x) - #define le16_to_cpu(x) swab16(x) + #define cpu_to_le64(x) __builtin_bswap64(x) + #define le64_to_cpu(x) __builtin_bswap64(x) + #define cpu_to_le32(x) __builtin_bswap32(x) + #define le32_to_cpu(x) __builtin_bswap32(x) + #define cpu_to_le16(x) __builtin_bswap16(x) + #define le16_to_cpu(x) __builtin_bswap16(x) #define cpu_to_be64(x) ((uint64_t)(x)) #define be64_to_cpu(x) ((uint64_t)(x)) #define cpu_to_be32(x) ((uint32_t)(x)) diff --git a/src/include/swab.h b/src/include/swab.h deleted file mode 100644 index 956cfa5..0000000 --- a/src/include/swab.h +++ /dev/null @@ -1,47 +0,0 @@ -/* - * linux/byteorder/swab.h - * Byte-swapping, independently from CPU endianness - * swabXX[ps]?(foo) - * - * Francois-Rene Rideau fare@tunes.org 19971205 - * separated swab functions from cpu_to_XX, - * to clean up support for bizarre-endian architectures. - * - * See asm-i386/byteorder.h and such for examples of how to provide - * architecture-dependent optimized versions - * - */ - -/* casts are necessary for constants, because we never know how for sure - * how U/UL/ULL map to __u16, __u32, __u64. At least not in a portable way. - */ - -#ifndef _SWAB_H -#define _SWAB_H - -#include <stdint.h> - -#define swab16(x) \ - ((unsigned short)( \ - (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ - (((unsigned short)(x) & (unsigned short)0xff00U) >> 8))) - -#define swab32(x) \ - ((unsigned int)( \ - (((unsigned int)(x) & 0x000000ffUL) << 24) | \ - (((unsigned int)(x) & 0x0000ff00UL) << 8) | \ - (((unsigned int)(x) & 0x00ff0000UL) >> 8) | \ - (((unsigned int)(x) & 0xff000000UL) >> 24))) - -#define swab64(x) \ - ((uint64_t)( \ - (((uint64_t)(x) & (uint64_t)0x00000000000000ffULL) << 56) | \ - (((uint64_t)(x) & (uint64_t)0x000000000000ff00ULL) << 40) | \ - (((uint64_t)(x) & (uint64_t)0x0000000000ff0000ULL) << 24) | \ - (((uint64_t)(x) & (uint64_t)0x00000000ff000000ULL) << 8) | \ - (((uint64_t)(x) & (uint64_t)0x000000ff00000000ULL) >> 8) | \ - (((uint64_t)(x) & (uint64_t)0x0000ff0000000000ULL) >> 24) | \ - (((uint64_t)(x) & (uint64_t)0x00ff000000000000ULL) >> 40) | \ - (((uint64_t)(x) & (uint64_t)0xff00000000000000ULL) >> 56))) - -#endif /* _SWAB_H */ diff --git a/src/vendorcode/eltan/security/mboot/mboot.c b/src/vendorcode/eltan/security/mboot/mboot.c index c5523a5..f47d74b 100644 --- a/src/vendorcode/eltan/security/mboot/mboot.c +++ b/src/vendorcode/eltan/security/mboot/mboot.c @@ -100,8 +100,8 @@ Pcrs->count = TpmCap.data.assignedPCR.count; printk(BIOS_DEBUG, "Pcrs->count = %d\n", Pcrs->count); for (index = 0; index < Pcrs->count; index++) { - Pcrs->pcrSelections[index].hash = - swab16(TpmCap.data.assignedPCR.pcrSelections[index].hash); + Pcrs->pcrSelections[index].hash = __builtin_bswap16( + TpmCap.data.assignedPCR.pcrSelections[index].hash); printk(BIOS_DEBUG, "Pcrs->pcrSelections[%d].hash = 0x%x\n", index, Pcrs->pcrSelections[index].hash); Pcrs->pcrSelections[index].sizeofSelect = diff --git a/src/vendorcode/eltan/security/mboot/mboot.h b/src/vendorcode/eltan/security/mboot/mboot.h index 79f2308..3b08924 100644 --- a/src/vendorcode/eltan/security/mboot/mboot.h +++ b/src/vendorcode/eltan/security/mboot/mboot.h @@ -27,7 +27,6 @@ #include <boot/coreboot_tables.h> #include <security/tpm/tss/tcg-2.0/tss_structures.h> #include <security/tpm/tss.h> -#include <swab.h>
/* TPM2 interface */ #define EFI_TPM2_ACPI_TABLE_START_METHOD_TIS 6
Hello build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37343
to look at the new patch set (#2).
Change subject: endian: Replace explicit byte swapping with compiler builtin ......................................................................
endian: Replace explicit byte swapping with compiler builtin
gcc seems to have some stupid problem with deciding when to inline byte swapping functions (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92716). Using the compiler builtin instead seems to solve the problem.
(This doesn't yet solve the issue for the read_be32()-family of functions, which we should maybe just get rid of at some point?)
Change-Id: Ia2a6d8ea98987266ccc32ffaa0a7f78965fca1cd Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/endian.h D payloads/libpayload/include/swab.h M src/include/swab.h 3 files changed, 12 insertions(+), 67 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/37343/2
Hello build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37343
to look at the new patch set (#3).
Change subject: endian: Replace explicit byte swapping with compiler builtin ......................................................................
endian: Replace explicit byte swapping with compiler builtin
gcc seems to have some stupid problem with deciding when to inline byte swapping functions (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92716). Using the compiler builtin instead seems to solve the problem.
(This doesn't yet solve the issue for the read_be32()-family of functions, which we should maybe just get rid of at some point?)
Change-Id: Ia2a6d8ea98987266ccc32ffaa0a7f78965fca1cd Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/endian.h D payloads/libpayload/include/swab.h M src/include/swab.h 3 files changed, 12 insertions(+), 67 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/37343/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37343 )
Change subject: endian: Replace explicit byte swapping with compiler builtin ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37343 )
Change subject: endian: Replace explicit byte swapping with compiler builtin ......................................................................
endian: Replace explicit byte swapping with compiler builtin
gcc seems to have some stupid problem with deciding when to inline byte swapping functions (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92716). Using the compiler builtin instead seems to solve the problem.
(This doesn't yet solve the issue for the read_be32()-family of functions, which we should maybe just get rid of at some point?)
Change-Id: Ia2a6d8ea98987266ccc32ffaa0a7f78965fca1cd Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37343 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M payloads/libpayload/include/endian.h D payloads/libpayload/include/swab.h M src/include/swab.h 3 files changed, 12 insertions(+), 67 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/payloads/libpayload/include/endian.h b/payloads/libpayload/include/endian.h index ec0825d..dee45f2 100644 --- a/payloads/libpayload/include/endian.h +++ b/payloads/libpayload/include/endian.h @@ -34,23 +34,6 @@ #include <arch/types.h> #include <libpayload-config.h>
-static inline uint16_t swap_bytes16(uint16_t in) -{ - return ((in & 0xFF) << 8) | ((in & 0xFF00) >> 8); -} - -static inline uint32_t swap_bytes32(uint32_t in) -{ - return ((in & 0xFF) << 24) | ((in & 0xFF00) << 8) | - ((in & 0xFF0000) >> 8) | ((in & 0xFF000000) >> 24); -} - -static inline uint64_t swap_bytes64(uint64_t in) -{ - return ((uint64_t)swap_bytes32((uint32_t)in) << 32) | - ((uint64_t)swap_bytes32((uint32_t)(in >> 32))); -} - /* Endian functions from glibc 2.9 / BSD "endian.h" */
#if CONFIG(LP_BIG_ENDIAN) @@ -59,15 +42,15 @@ #define htobe32(in) (in) #define htobe64(in) (in)
-#define htole16(in) swap_bytes16(in) -#define htole32(in) swap_bytes32(in) -#define htole64(in) swap_bytes64(in) +#define htole16(in) ((uint16_t)__builtin_bswap16(in)) +#define htole32(in) ((uint32_t)__builtin_bswap32(in)) +#define htole64(in) ((uint64_t)__builtin_bswap64(in))
#elif CONFIG(LP_LITTLE_ENDIAN)
-#define htobe16(in) swap_bytes16(in) -#define htobe32(in) swap_bytes32(in) -#define htobe64(in) swap_bytes64(in) +#define htobe16(in) ((uint16_t)__builtin_bswap16(in)) +#define htobe32(in) ((uint32_t)__builtin_bswap32(in)) +#define htobe64(in) ((uint64_t)__builtin_bswap64(in))
#define htole16(in) (in) #define htole32(in) (in) diff --git a/payloads/libpayload/include/swab.h b/payloads/libpayload/include/swab.h deleted file mode 100644 index 2198077..0000000 --- a/payloads/libpayload/include/swab.h +++ /dev/null @@ -1,44 +0,0 @@ -#ifndef _SWAB_H -#define _SWAB_H - -/* - * linux/byteorder/swab.h - * Byte-swapping, independently from CPU endianness - * swabXX[ps]?(foo) - * - * Francois-Rene Rideau fare@tunes.org 19971205 - * separated swab functions from cpu_to_XX, - * to clean up support for bizarre-endian architectures. - * - * See asm-i386/byteorder.h and suches for examples of how to provide - * architecture-dependent optimized versions - * - */ - -/* casts are necessary for constants, because we never know for sure - * how U/UL/ULL map to __u16, __u32, __u64. At least not in a portable way. - */ -#define swab16(x) \ - ((unsigned short)( \ - (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ - (((unsigned short)(x) & (unsigned short)0xff00U) >> 8))) - -#define swab32(x) \ - ((unsigned int)( \ - (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ - (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ - (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ - (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24))) - -#define swab64(x) \ - ((uint64_t)( \ - (((uint64_t)(x) & (uint64_t)0x00000000000000ffULL) << 56) | \ - (((uint64_t)(x) & (uint64_t)0x000000000000ff00ULL) << 40) | \ - (((uint64_t)(x) & (uint64_t)0x0000000000ff0000ULL) << 24) | \ - (((uint64_t)(x) & (uint64_t)0x00000000ff000000ULL) << 8) | \ - (((uint64_t)(x) & (uint64_t)0x000000ff00000000ULL) >> 8) | \ - (((uint64_t)(x) & (uint64_t)0x0000ff0000000000ULL) >> 24) | \ - (((uint64_t)(x) & (uint64_t)0x00ff000000000000ULL) >> 40) | \ - (((uint64_t)(x) & (uint64_t)0xff00000000000000ULL) >> 56))) - -#endif /* _SWAB_H */ diff --git a/src/include/swab.h b/src/include/swab.h index 956cfa5..57fe5a2 100644 --- a/src/include/swab.h +++ b/src/include/swab.h @@ -21,6 +21,7 @@
#include <stdint.h>
+#if defined(__ROMCC__) || ENV_ARMV4 #define swab16(x) \ ((unsigned short)( \ (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ @@ -43,5 +44,10 @@ (((uint64_t)(x) & (uint64_t)0x0000ff0000000000ULL) >> 24) | \ (((uint64_t)(x) & (uint64_t)0x00ff000000000000ULL) >> 40) | \ (((uint64_t)(x) & (uint64_t)0xff00000000000000ULL) >> 56))) +#else /* __ROMCC__ || ENV_ARMV4 */ +#define swab16(x) ((uint16_t)__builtin_bswap16(x)) +#define swab32(x) ((uint32_t)__builtin_bswap32(x)) +#define swab64(x) ((uint64_t)__builtin_bswap64(x)) +#endif /* !(__ROMCC__ || ENV_ARMV4) */
#endif /* _SWAB_H */
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)