[U-Boot] static var mem alignment issue/question
Greetings,
In debugging an issue with a rather old branch of U-Boot (2015-04) I found that the static assignment of a data buffer was not 32-bit aligned which caused data aborts. However I find that current U-boot master does not suffer this issue and no matter what I declare static before the particular variable its always 32-bit aligned. I don't see any code changes that would cause this and in both cases I'm building with the same gcc5.2.0 toolchain.
The code in question is this this from cmd/fdt.c:
} else if (argv[1][0] == 's') { char *pathp; /* path */ char *prop; /* property */ int nodeoffset; /* node offset from libfdt */ static char data[SCRATCHPAD]; /* storage for the property */ int len; /* new length of the property */ int ret;
What guarantee's that 'data' above is 32-bit aligned in master that is missing from the older U-Boot branch I'm using? Perhaps there is some arg in a Makefile that I'm missing?
Regards,
Tim
Tim Harvey tharvey@gateworks.com writes:
Greetings,
In debugging an issue with a rather old branch of U-Boot (2015-04) I found that the static assignment of a data buffer was not 32-bit aligned which caused data aborts. However I find that current U-boot master does not suffer this issue and no matter what I declare static before the particular variable its always 32-bit aligned. I don't see any code changes that would cause this and in both cases I'm building with the same gcc5.2.0 toolchain.
The code in question is this this from cmd/fdt.c:
} else if (argv[1][0] == 's') { char *pathp; /* path */ char *prop; /* property */ int nodeoffset; /* node offset from libfdt */ static char data[SCRATCHPAD]; /* storage for the property */ int len; /* new length of the property */ int ret;What guarantee's that 'data' above is 32-bit aligned in master that is missing from the older U-Boot branch I'm using? Perhaps there is some arg in a Makefile that I'm missing?
There are no such guarantees. Things often end up 4-byte aligned by chance simply because most of them are a multiple of 4 bytes in size. The code should be fixed, either by explicitly aligning the buffer or by using the put_unaligned_b32() accessor in fdt_parse_prop(). Here's an untested patch:
From 714f6a16062cc43e9aadfba29b295365fd4926de Mon Sep 17 00:00:00 2001
From: Mans Rullgard mans@mansr.com Date: Tue, 29 Nov 2016 11:59:37 +0000 Subject: [PATCH] cmd/fdt: fix unaligned memory access
The buffer passed to fdt_parse_prop() might be unaligned. Use the proper access helper when writing 32-bit values.
Signed-off-by: Mans Rullgard mans@mansr.com --- cmd/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cmd/fdt.c b/cmd/fdt.c index b503357dc3a8..560b8cc9df61 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -17,6 +17,7 @@ #include <fdt_support.h> #include <mapmem.h> #include <asm/io.h> +#include <asm/unaligned.h>
#define MAX_LEVEL 32 /* how deeply nested we will go */ #define SCRATCHPAD 1024 /* bytes of scratchpad memory */ @@ -766,7 +767,7 @@ static int fdt_parse_prop(char * const *newval, int count, char *data, int *len)
cp = newp; tmp = simple_strtoul(cp, &newp, 0); - *(__be32 *)data = __cpu_to_be32(tmp); + put_unaligned_be32(tmp, data); data += 4; *len += 4;
On Tue, Nov 29, 2016 at 4:06 AM, Måns Rullgård mans@mansr.com wrote:
Tim Harvey tharvey@gateworks.com writes:
Greetings,
In debugging an issue with a rather old branch of U-Boot (2015-04) I found that the static assignment of a data buffer was not 32-bit aligned which caused data aborts. However I find that current U-boot master does not suffer this issue and no matter what I declare static before the particular variable its always 32-bit aligned. I don't see any code changes that would cause this and in both cases I'm building with the same gcc5.2.0 toolchain.
The code in question is this this from cmd/fdt.c:
} else if (argv[1][0] == 's') { char *pathp; /* path */ char *prop; /* property */ int nodeoffset; /* node offset from libfdt */ static char data[SCRATCHPAD]; /* storage for the property */ int len; /* new length of the property */ int ret;What guarantee's that 'data' above is 32-bit aligned in master that is missing from the older U-Boot branch I'm using? Perhaps there is some arg in a Makefile that I'm missing?
There are no such guarantees. Things often end up 4-byte aligned by chance simply because most of them are a multiple of 4 bytes in size. The code should be fixed, either by explicitly aligning the buffer or by using the put_unaligned_b32() accessor in fdt_parse_prop(). Here's an untested patch:
Måns,
My thoughts as well but what I found on a 2015.04 U-Boot adding a 'static char foo[1]' before the 'static char data[SCRATCHPAD]' and printing the addrs: foo:4ff9e0b2 data:4ff9e0b3 (what I would have expected... no alignment guarantees and clearly not quad-byte aligned)
The same change on master: foo:4ff9fff4 data:4ff9fbf0 (not at all what I expected - quad byte aligned and more)
Thus I figured there was something in a Makefile that was directing the compiler (same compiler in both cases)
I find that merely including '<asm/unaligned.h>' (with no code changes) changes the alignment and aligns propery???
Tim
Tim Harvey tharvey@gateworks.com writes:
On Tue, Nov 29, 2016 at 4:06 AM, Måns Rullgård mans@mansr.com wrote:
Tim Harvey tharvey@gateworks.com writes:
Greetings,
In debugging an issue with a rather old branch of U-Boot (2015-04) I found that the static assignment of a data buffer was not 32-bit aligned which caused data aborts. However I find that current U-boot master does not suffer this issue and no matter what I declare static before the particular variable its always 32-bit aligned. I don't see any code changes that would cause this and in both cases I'm building with the same gcc5.2.0 toolchain.
The code in question is this this from cmd/fdt.c:
} else if (argv[1][0] == 's') { char *pathp; /* path */ char *prop; /* property */ int nodeoffset; /* node offset from libfdt */ static char data[SCRATCHPAD]; /* storage for the property */ int len; /* new length of the property */ int ret;What guarantee's that 'data' above is 32-bit aligned in master that is missing from the older U-Boot branch I'm using? Perhaps there is some arg in a Makefile that I'm missing?
There are no such guarantees. Things often end up 4-byte aligned by chance simply because most of them are a multiple of 4 bytes in size. The code should be fixed, either by explicitly aligning the buffer or by using the put_unaligned_b32() accessor in fdt_parse_prop(). Here's an untested patch:
Måns,
My thoughts as well but what I found on a 2015.04 U-Boot adding a 'static char foo[1]' before the 'static char data[SCRATCHPAD]' and printing the addrs: foo:4ff9e0b2 data:4ff9e0b3 (what I would have expected... no alignment guarantees and clearly not quad-byte aligned)
The same change on master: foo:4ff9fff4 data:4ff9fbf0 (not at all what I expected - quad byte aligned and more)
Thus I figured there was something in a Makefile that was directing the compiler (same compiler in both cases)
I find that merely including '<asm/unaligned.h>' (with no code changes) changes the alignment and aligns propery???
Compilers are fickle creatures.
participants (2)
-
Måns Rullgård -
Tim Harvey