[coreboot-gerrit] [S] Change in coreboot[main]: commonlib/bsd/lz4_wrapper.c: Fix misaligned access

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81910?usp=email )

Change subject: commonlib/bsd/lz4_wrapper.c: Fix misaligned access
......................................................................

commonlib/bsd/lz4_wrapper.c: Fix misaligned access

Currently the HiFive Unleashed produces the following exception:
[DEBUG]  Exception:          Load address misaligned
[DEBUG]  Hart ID:            0
[DEBUG]  Previous mode:      machine
[DEBUG]  Bad instruction pc: 0x080010d0
[DEBUG]  Bad address:        0x08026ab3
[DEBUG]  Stored ra:          0x080010c8
[DEBUG]  Stored sp:          0x08010cc8

The coreboot LZ4 decompression code does some misaligned access during
decompression which the FU540 apparently does not support in SRAM.

Make the compiler generate code that adheres to natural alignment by
fixing the LZ4_readLE16() function and creating LZ4_readLE32().

Signed-off-by: Maximilian Brune 
Change-Id: Id165829bfd35be2bce2bbb019c208a304f627add
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81910
Reviewed-by: Nico Huber 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/commonlib/bsd/lz4_wrapper.c
1 file changed, 21 insertions(+), 7 deletions(-)

Approvals:
  Angel Pons: Looks good to me, but someone else must approve
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/commonlib/bsd/lz4_wrapper.c b/src/commonlib/bsd/lz4_wrapper.c
index efe246b..566c2bf 100644
--- a/src/commonlib/bsd/lz4_wrapper.c
+++ b/src/commonlib/bsd/lz4_wrapper.c
@@ -5,15 +5,29 @@
 #include <commonlib/bsd/sysincludes.h>
 #include <stdint.h>
 #include <string.h>
+#include <endian.h>

-/* LZ4 comes with its own supposedly portable memory access functions, but they
- * seem to be very inefficient in practice (at least on ARM64). Since coreboot
- * knows about endianness and allows some basic assumptions (such as unaligned
- * access support), we can easily write the ones we need ourselves. */
+/*
+ * RISC-V and older ARM architectures do not mandate support for misaligned access.
+ * Our le16toh and friends functions assume misaligned access support. Writing the access
+ * like this causes the compiler to generate instructions using misaligned access (or not)
+ * depending on the architecture. So there is no performance penalty for platforms supporting
+ * misaligned access.
+ */
 static uint16_t LZ4_readLE16(const void *src)
 {
-	return le16toh(*(const uint16_t *)src);
+	return *((const uint8_t *)src + 1) << 8
+	      | *(const uint8_t *)src;
 }
+
+static uint32_t LZ4_readLE32(const void *src)
+{
+	return *((const uint8_t *)src + 3) << 24
+	     | *((const uint8_t *)src + 2) << 16
+	     | *((const uint8_t *)src + 1) << 8
+	     |  *(const uint8_t *)src;
+}
+
 static void LZ4_copy8(void *dst, const void *src)
 {
 /* ARM32 needs to be a special snowflake to prevent GCC from coalescing the
@@ -107,7 +121,7 @@
    		return 0;	/* input overrun */

/* We assume there's always only a single, standard frame. */
-		if (le32toh(h->magic) != LZ4F_MAGICNUMBER
+		if (LZ4_readLE32(&h->magic) != LZ4F_MAGICNUMBER
    	    || (h->flags & VERSION) != (1 << VERSION_SHIFT))
    		return 0;	/* unknown format */
    	if ((h->flags & RESERVED0) || (h->block_descriptor & RESERVED1_2))
@@ -127,7 +141,7 @@
    		break;          /* input overrun */

struct lz4_block_header b = {
-			.raw = le32toh(*(const uint32_t *)in)
+			.raw = LZ4_readLE32((const uint32_t *)in)
    	};
    	in += sizeof(struct lz4_block_header);