[PATCH] powerpc: fix regression in arch_initr_trap()
The assembly output of the arch_initr_trap() function differed by a single byte after common.h was removed from traps.c:
fff49a18 <arch_initr_trap>: fff49a18: 94 21 ff f0 stwu r1,-16(r1) fff49a1c: 7c 08 02 a6 mflr r0 fff49a20: 90 01 00 14 stw r0,20(r1) -fff49a24: 80 62 00 44 lwz r3,68(r2) +fff49a24: 80 62 00 38 lwz r3,56(r2) fff49a28: 4b ff 76 19 bl fff41040 <trap_init> fff49a2c: 80 01 00 14 lwz r0,20(r1) fff49a30: 38 60 00 00 li r3,0 fff49a34: 38 21 00 10 addi r1,r1,16 fff49a38: 7c 08 03 a6 mtlr r0
This was causing a consistent hard lockup during the MMC read / loading of the QoriQ FMan firmware on a P2041RDB board.
Re-adding the header causes identical assembly to be emitted and allows the firmware loading and subsequent boot to succeed.
Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header") Signed-off-by: Matt Merhar mattmerhar@protonmail.com ---
arch/powerpc/lib/traps.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/lib/traps.c b/arch/powerpc/lib/traps.c index c7bce82a44..ab8ca269a5 100644 --- a/arch/powerpc/lib/traps.c +++ b/arch/powerpc/lib/traps.c @@ -4,6 +4,7 @@ * Wolfgang Denk, DENX Software Engineering, wd@denx.de. */
+#include <common.h> #include <init.h> #include <asm/global_data.h>
Hi Matt,
On 17.05.21 19:32, Matt Merhar wrote:
The assembly output of the arch_initr_trap() function differed by a single byte after common.h was removed from traps.c:
fff49a18 <arch_initr_trap>: fff49a18: 94 21 ff f0 stwu r1,-16(r1) fff49a1c: 7c 08 02 a6 mflr r0 fff49a20: 90 01 00 14 stw r0,20(r1) -fff49a24: 80 62 00 44 lwz r3,68(r2) +fff49a24: 80 62 00 38 lwz r3,56(r2) fff49a28: 4b ff 76 19 bl fff41040 <trap_init> fff49a2c: 80 01 00 14 lwz r0,20(r1) fff49a30: 38 60 00 00 li r3,0 fff49a34: 38 21 00 10 addi r1,r1,16 fff49a38: 7c 08 03 a6 mtlr r0
This was causing a consistent hard lockup during the MMC read / loading of the QoriQ FMan firmware on a P2041RDB board.
Re-adding the header causes identical assembly to be emitted and allows the firmware loading and subsequent boot to succeed.
Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header") Signed-off-by: Matt Merhar mattmerhar@protonmail.com
Did you try to investigate what exactly causes this difference in the assembly code, when the header is not included? Re-adding this header seems like "papering over" the real problem to me.
Thanks, Stefan
arch/powerpc/lib/traps.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/lib/traps.c b/arch/powerpc/lib/traps.c index c7bce82a44..ab8ca269a5 100644 --- a/arch/powerpc/lib/traps.c +++ b/arch/powerpc/lib/traps.c @@ -4,6 +4,7 @@
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
*/
+#include <common.h> #include <init.h> #include <asm/global_data.h>
Viele Grüße, Stefan
On 18/05/2021 07.50, Stefan Roese wrote:
Hi Matt,
On 17.05.21 19:32, Matt Merhar wrote:
The assembly output of the arch_initr_trap() function differed by a single byte after common.h was removed from traps.c:
fff49a18 <arch_initr_trap>: fff49a18: 94 21 ff f0 stwu r1,-16(r1) fff49a1c: 7c 08 02 a6 mflr r0 fff49a20: 90 01 00 14 stw r0,20(r1) -fff49a24: 80 62 00 44 lwz r3,68(r2) +fff49a24: 80 62 00 38 lwz r3,56(r2) fff49a28: 4b ff 76 19 bl fff41040 <trap_init> fff49a2c: 80 01 00 14 lwz r0,20(r1) fff49a30: 38 60 00 00 li r3,0 fff49a34: 38 21 00 10 addi r1,r1,16 fff49a38: 7c 08 03 a6 mtlr r0
This was causing a consistent hard lockup during the MMC read / loading of the QoriQ FMan firmware on a P2041RDB board.
Re-adding the header causes identical assembly to be emitted and allows the firmware loading and subsequent boot to succeed.
Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header") Signed-off-by: Matt Merhar mattmerhar@protonmail.com
Did you try to investigate what exactly causes this difference in the assembly code, when the header is not included? Re-adding this header seems like "papering over" the real problem to me.
Running pahole on traps.o without and with that patch shows (I took P2041RDB_defconfig)
struct global_data { struct bd_info * bd; /* 0 4 */ long unsigned int flags; /* 4 4 */ unsigned int baudrate; /* 8 4 */ long unsigned int cpu_clk; /* 12 4 */ long unsigned int bus_clk; /* 16 4 */ long unsigned int pci_clk; /* 20 4 */ long unsigned int mem_clk; /* 24 4 */ long unsigned int have_console; /* 28 4 */ long unsigned int env_addr; /* 32 4 */ long unsigned int env_valid; /* 36 4 */ long unsigned int env_has_init; /* 40 4 */ int env_load_prio; /* 44 4 */ long unsigned int ram_base; /* 48 4 */
/* XXX 4 bytes hole, try to pack */
phys_addr_t ram_top; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ long unsigned int relocaddr; /* 64 4 */
vs
struct global_data { struct bd_info * bd; /* 0 4 */ long unsigned int flags; /* 4 4 */ unsigned int baudrate; /* 8 4 */ long unsigned int cpu_clk; /* 12 4 */ long unsigned int bus_clk; /* 16 4 */ long unsigned int pci_clk; /* 20 4 */ long unsigned int mem_clk; /* 24 4 */ long unsigned int post_log_word; /* 28 4 */ long unsigned int post_log_res; /* 32 4 */ long unsigned int post_init_f_time; /* 36 4 */ long unsigned int have_console; /* 40 4 */ long unsigned int env_addr; /* 44 4 */ long unsigned int env_valid; /* 48 4 */ long unsigned int env_has_init; /* 52 4 */ int env_load_prio; /* 56 4 */ long unsigned int ram_base; /* 60 4 */ /* --- cacheline 1 boundary (64 bytes) --- */ phys_addr_t ram_top; /* 64 8 */ long unsigned int relocaddr; /* 72 4 */
so it seems to be the
#if defined(CONFIG_POST)
in include/asm-generic/global_data.h which is broken or unreliable or whatever the appropriate way to describe it is.
It would be nice if the linker could catch that kind of thing when collecting debug info from various TUs.
Rasmus
I haven't done a whole lot of testing of these, just enough to see that it normally works _and_ that it would catch the bug Matt reported. I don't have time or resources to find and fix the bugs this would reveal, hence just an RFC for people to consider.
Rasmus Villemoes (2): build_bug.h: add wrapper for _Static_assert global-data.h: add build-time sanity check of sizeof(struct global_data)
include/asm-generic/global_data.h | 5 +++++ include/linux/build_bug.h | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+)
[Linux commit 6bab69c65013bed5fce9f101a64a84d0385b3946]
BUILD_BUG_ON() is a little annoying, since it cannot be used outside function scope. So one cannot put assertions about the sizeof() a struct next to the struct definition, but has to hide that in some more or less arbitrary function.
Since gcc 4.6 (which is now also the required minimum), there is support for the C11 _Static_assert in all C modes, including gnu89. So add a simple wrapper for that.
_Static_assert() requires a message argument, which is usually quite redundant (and I believe that bug got fixed at least in newer C++ standards), but we can easily work around that with a little macro magic, making it optional.
For example, adding
static_assert(sizeof(struct printf_spec) == 8);
in vsprintf.c and modifying that struct to violate it, one gets
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8" #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
godbolt.org suggests that _Static_assert() has been support by clang since at least 3.0.0.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- include/linux/build_bug.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h index b7d22d6000..9c7088bafa 100644 --- a/include/linux/build_bug.h +++ b/include/linux/build_bug.h @@ -79,6 +79,25 @@ */ #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
+/** + * static_assert - check integer constant expression at build time + * + * static_assert() is a wrapper for the C11 _Static_assert, with a + * little macro magic to make the message optional (defaulting to the + * stringification of the tested expression). + * + * Contrary to BUILD_BUG_ON(), static_assert() can be used at global + * scope, but requires the expression to be an integer constant + * expression (i.e., it is not enough that __builtin_constant_p() is + * true for expr). + * + * Also note that BUILD_BUG_ON() fails the build if the condition is + * true, while static_assert() fails the build if the expression is + * false. + */ +#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) +#define __static_assert(expr, msg, ...) _Static_assert(expr, msg) + #endif /* __CHECKER__ */
#endif /* _LINUX_BUILD_BUG_H */
On Tue, 18 May 2021 at 03:19, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
[Linux commit 6bab69c65013bed5fce9f101a64a84d0385b3946]
BUILD_BUG_ON() is a little annoying, since it cannot be used outside function scope. So one cannot put assertions about the sizeof() a struct next to the struct definition, but has to hide that in some more or less arbitrary function.
Since gcc 4.6 (which is now also the required minimum), there is support for the C11 _Static_assert in all C modes, including gnu89. So add a simple wrapper for that.
_Static_assert() requires a message argument, which is usually quite redundant (and I believe that bug got fixed at least in newer C++ standards), but we can easily work around that with a little macro magic, making it optional.
For example, adding
static_assert(sizeof(struct printf_spec) == 8);
in vsprintf.c and modifying that struct to violate it, one gets
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8" #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
godbolt.org suggests that _Static_assert() has been support by clang since at least 3.0.0.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
include/linux/build_bug.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Tue, May 18, 2021 at 11:19:46AM +0200, Rasmus Villemoes wrote:
[Linux commit 6bab69c65013bed5fce9f101a64a84d0385b3946]
BUILD_BUG_ON() is a little annoying, since it cannot be used outside function scope. So one cannot put assertions about the sizeof() a struct next to the struct definition, but has to hide that in some more or less arbitrary function.
Since gcc 4.6 (which is now also the required minimum), there is support for the C11 _Static_assert in all C modes, including gnu89. So add a simple wrapper for that.
_Static_assert() requires a message argument, which is usually quite redundant (and I believe that bug got fixed at least in newer C++ standards), but we can easily work around that with a little macro magic, making it optional.
For example, adding
static_assert(sizeof(struct printf_spec) == 8);
in vsprintf.c and modifying that struct to violate it, one gets
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8" #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
godbolt.org suggests that _Static_assert() has been support by clang since at least 3.0.0.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/next, thanks!
The layout and contents of struct global_data depends on a lot of CONFIG_* preprocessor macros, not all of which are entirely converted to Kconfig - not to mention weird games played here and there. This can result in one translation unit using one definition of struct global_data while the actual layout is another.
That can be very hard to debug. But we already have a mechanism that can help catch such bugs at build time, namely the asm-offsets machinery which is necessary anyway to provide assembly code with the necessary constants. So make sure that every C translation unit that include global_data.h actually sees the same size of struct global_data as that which was seen by the asm-offsets.c TU.
It is likely that this patch will break the build of some boards. For example, without the patch from Matt Merhar (https://lists.denx.de/pipermail/u-boot/2021-May/450135.html) or some other fix, this breaks P2041RDB_defconfig:
CC arch/powerpc/lib/traps.o AS arch/powerpc/cpu/mpc85xx/start.o In file included from include/asm-generic/global_data.h:26, from ./arch/powerpc/include/asm/global_data.h:109, from include/init.h:21, from arch/powerpc/lib/traps.c:7: include/linux/build_bug.h:99:41: error: static assertion failed: "sizeof(struct global_data) == GD_SIZE" 99 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~~~~~~~~~~~ include/linux/build_bug.h:98:34: note: in expansion of macro ‘__static_assert’ 98 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) | ^~~~~~~~~~~~~~~ include/asm-generic/global_data.h:470:1: note: in expansion of macro ‘static_assert’ 470 | static_assert(sizeof(struct global_data) == GD_SIZE); | ^~~~~~~~~~~~~ make[1]: *** [scripts/Makefile.build:266: arch/powerpc/lib/traps.o] Error 1 make: *** [Makefile:1753: arch/powerpc/lib] Error 2 make: *** Waiting for unfinished jobs....
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- include/asm-generic/global_data.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 47921d27b1..1233ec0ed6 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -23,6 +23,8 @@ #include <fdtdec.h> #include <membuff.h> #include <linux/list.h> +#include <linux/build_bug.h> +#include <asm-offsets.h>
struct acpi_ctx; struct driver_rt; @@ -464,6 +466,9 @@ struct global_data { char *smbios_version; #endif }; +#ifndef DO_DEPS_ONLY +static_assert(sizeof(struct global_data) == GD_SIZE); +#endif
/** * gd_board_type() - retrieve board type
On Tue, 18 May 2021 at 03:20, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
The layout and contents of struct global_data depends on a lot of CONFIG_* preprocessor macros, not all of which are entirely converted to Kconfig - not to mention weird games played here and there. This can result in one translation unit using one definition of struct global_data while the actual layout is another.
That can be very hard to debug. But we already have a mechanism that can help catch such bugs at build time, namely the asm-offsets machinery which is necessary anyway to provide assembly code with the necessary constants. So make sure that every C translation unit that include global_data.h actually sees the same size of struct global_data as that which was seen by the asm-offsets.c TU.
It is likely that this patch will break the build of some boards. For example, without the patch from Matt Merhar (https://lists.denx.de/pipermail/u-boot/2021-May/450135.html) or some other fix, this breaks P2041RDB_defconfig:
CC arch/powerpc/lib/traps.o AS arch/powerpc/cpu/mpc85xx/start.o In file included from include/asm-generic/global_data.h:26, from ./arch/powerpc/include/asm/global_data.h:109, from include/init.h:21, from arch/powerpc/lib/traps.c:7: include/linux/build_bug.h:99:41: error: static assertion failed: "sizeof(struct global_data) == GD_SIZE" 99 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~~~~~~~~~~~ include/linux/build_bug.h:98:34: note: in expansion of macro ‘__static_assert’ 98 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) | ^~~~~~~~~~~~~~~ include/asm-generic/global_data.h:470:1: note: in expansion of macro ‘static_assert’ 470 | static_assert(sizeof(struct global_data) == GD_SIZE); | ^~~~~~~~~~~~~ make[1]: *** [scripts/Makefile.build:266: arch/powerpc/lib/traps.o] Error 1 make: *** [Makefile:1753: arch/powerpc/lib] Error 2 make: *** Waiting for unfinished jobs....
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
include/asm-generic/global_data.h | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Tue, May 18, 2021 at 11:19:47AM +0200, Rasmus Villemoes wrote:
The layout and contents of struct global_data depends on a lot of CONFIG_* preprocessor macros, not all of which are entirely converted to Kconfig - not to mention weird games played here and there. This can result in one translation unit using one definition of struct global_data while the actual layout is another.
That can be very hard to debug. But we already have a mechanism that can help catch such bugs at build time, namely the asm-offsets machinery which is necessary anyway to provide assembly code with the necessary constants. So make sure that every C translation unit that include global_data.h actually sees the same size of struct global_data as that which was seen by the asm-offsets.c TU.
It is likely that this patch will break the build of some boards. For example, without the patch from Matt Merhar (https://lists.denx.de/pipermail/u-boot/2021-May/450135.html) or some other fix, this breaks P2041RDB_defconfig:
CC arch/powerpc/lib/traps.o AS arch/powerpc/cpu/mpc85xx/start.o In file included from include/asm-generic/global_data.h:26, from ./arch/powerpc/include/asm/global_data.h:109, from include/init.h:21, from arch/powerpc/lib/traps.c:7: include/linux/build_bug.h:99:41: error: static assertion failed: "sizeof(struct global_data) == GD_SIZE" 99 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~~~~~~~~~~~ include/linux/build_bug.h:98:34: note: in expansion of macro ‘__static_assert’ 98 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) | ^~~~~~~~~~~~~~~ include/asm-generic/global_data.h:470:1: note: in expansion of macro ‘static_assert’ 470 | static_assert(sizeof(struct global_data) == GD_SIZE); | ^~~~~~~~~~~~~ make[1]: *** [scripts/Makefile.build:266: arch/powerpc/lib/traps.o] Error 1 make: *** [Makefile:1753: arch/powerpc/lib] Error 2 make: *** Waiting for unfinished jobs....
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/next, thanks!
As there is an arch/powerpc/include/asm/config.h file using "" to get config.h here can lead to using that rather than include/config.h. This in turn can lead to a mismatch in the size of gd.
Cc: Matt Merhar mattmerhar@protonmail.com Signed-off-by: Tom Rini trini@konsulko.com --- arch/powerpc/include/asm/global_data.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h index 192a02d347e7..90bf5a2aea5d 100644 --- a/arch/powerpc/include/asm/global_data.h +++ b/arch/powerpc/include/asm/global_data.h @@ -8,7 +8,7 @@ #ifndef __ASM_GBL_DATA_H #define __ASM_GBL_DATA_H
-#include "config.h" +#include <config.h> #include "asm/types.h"
/* Architecture-specific global data */
With the changes in commit 588efcdd72fc ("powerpc: Don't use relative include for config.h in global_data.h") fixing the root of the problem, we no longer need this re-inclusion.
This reverts commit f6c0d365d3e8ee8e4fd3ebe2ed957c2bca9d3328.
Cc: Matt Merhar mattmerhar@protonmail.com Signed-off-by: Tom Rini trini@konsulko.com --- I will need to reword this to have the right commit ID when merging. --- arch/powerpc/lib/traps.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/lib/traps.c b/arch/powerpc/lib/traps.c index ab8ca269a50c..c7bce82a44b3 100644 --- a/arch/powerpc/lib/traps.c +++ b/arch/powerpc/lib/traps.c @@ -4,7 +4,6 @@ * Wolfgang Denk, DENX Software Engineering, wd@denx.de. */
-#include <common.h> #include <init.h> #include <asm/global_data.h>
On Thu, Jun 03, 2021 at 09:39:00AM -0400, Tom Rini wrote:
With the changes in commit 588efcdd72fc ("powerpc: Don't use relative include for config.h in global_data.h") fixing the root of the problem, we no longer need this re-inclusion.
This reverts commit f6c0d365d3e8ee8e4fd3ebe2ed957c2bca9d3328.
Cc: Matt Merhar mattmerhar@protonmail.com Signed-off-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
Based on the comment in socfpga_soc64_common.h, the intention is for CONFIG_SYS_MEM_RESERVE_SECURE to be unused. However, in the code we do: ...
and that will evaluate to true. This leads to unwanted code being compiled. Further, as CONFIG_SYS_MEM_RESERVE_SECURE has not been migrated to Kconfig, this leads to a mismatch in the size of gd depending on if we have or have not also had <configs/BOARD.h> also included yet.
Remove the define as it's not needed.
Cc: Siew Chin Lim elly.siew.chin.lim@intel.com Cc: Chee Hong Ang chee.hong.ang@intel.com Cc: Dalon Westergreen dalon.westergreen@intel.com Signed-off-by: Tom Rini trini@konsulko.com --- include/configs/socfpga_soc64_common.h | 1 - 1 file changed, 1 deletion(-)
diff --git a/include/configs/socfpga_soc64_common.h b/include/configs/socfpga_soc64_common.h index 5afdb104543e..38fd775b5b6e 100644 --- a/include/configs/socfpga_soc64_common.h +++ b/include/configs/socfpga_soc64_common.h @@ -21,7 +21,6 @@ /* sysmgr.boot_scratch_cold4 & 5 (64bit) will be used for PSCI_CPU_ON call */ #define CPU_RELEASE_ADDR 0xFFD12210 #define CONFIG_SYS_CACHELINE_SIZE 64 -#define CONFIG_SYS_MEM_RESERVE_SECURE 0 /* using OCRAM, not DDR */
/* * U-Boot console configurations
On 03/06/2021 15.39, Tom Rini wrote:
Based on the comment in socfpga_soc64_common.h, the intention is for CONFIG_SYS_MEM_RESERVE_SECURE to be unused. However, in the code we do: ...
and that will evaluate to true. This leads to unwanted code being
Some cleanup made lines beginning with # disappear? I can _strongly_ recommend setting commit.cleanup to scissors; I have lost count of the number of times I've had a commit message mangled and become practically useless because important lines got removed.
For example http://lists.busybox.net/pipermail/busybox/2018-September/086647.html became https://git.busybox.net/busybox/commit/?id=571e525a141a2de87b9c2ced485745e96418d921
Rasmus
On Thu, Jun 03, 2021 at 05:07:17PM +0200, Rasmus Villemoes wrote:
On 03/06/2021 15.39, Tom Rini wrote:
Based on the comment in socfpga_soc64_common.h, the intention is for CONFIG_SYS_MEM_RESERVE_SECURE to be unused. However, in the code we do: ...
and that will evaluate to true. This leads to unwanted code being
Some cleanup made lines beginning with # disappear? I can _strongly_ recommend setting commit.cleanup to scissors; I have lost count of the number of times I've had a commit message mangled and become practically useless because important lines got removed.
For example http://lists.busybox.net/pipermail/busybox/2018-September/086647.html became https://git.busybox.net/busybox/commit/?id=571e525a141a2de87b9c2ced485745e96418d921
Bah! Yes, thanks. It should have said #ifdef CONFIG_SYS_MEM_RESERVE_SECURE ... #endif
On Thu, Jun 03, 2021 at 09:39:01AM -0400, Tom Rini wrote:
Based on the comment in socfpga_soc64_common.h, the intention is for CONFIG_SYS_MEM_RESERVE_SECURE to be unused. However, in the code we do: ...
and that will evaluate to true. This leads to unwanted code being compiled. Further, as CONFIG_SYS_MEM_RESERVE_SECURE has not been migrated to Kconfig, this leads to a mismatch in the size of gd depending on if we have or have not also had <configs/BOARD.h> also included yet.
Remove the define as it's not needed.
Cc: Siew Chin Lim elly.siew.chin.lim@intel.com Cc: Chee Hong Ang chee.hong.ang@intel.com Cc: Dalon Westergreen dalon.westergreen@intel.com Signed-off-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
All symbols that are defined in Kconfig will always be defined (or not) prior to preprocessing due to the -include directive while building. However, symbols which are not yet migrated will only be defined (or not) once the board config.h is included, via <config.h>. While the end goal must be to migrate all symbols, today we have cases where the size of gd will get mismatched within the build, based on include order. Mitigate this by making sure that any <asm/global_data.h> that uses symbols not in Kconfig does start with <config.h>. Remove this when not needed.
Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Huan Wang alison.wang@nxp.com Cc: Angelo Dureghello angelo@sysam.it Cc: Rick Chen rick@andestech.com Signed-off-by: Tom Rini trini@konsulko.com --- arch/arc/include/asm/global_data.h | 2 -- arch/arm/include/asm/global_data.h | 2 ++ arch/m68k/include/asm/global_data.h | 2 ++ arch/nds32/include/asm/global_data.h | 2 ++ 4 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/arc/include/asm/global_data.h b/arch/arc/include/asm/global_data.h index 8f9c83d3c28d..e35a26f1eb14 100644 --- a/arch/arc/include/asm/global_data.h +++ b/arch/arc/include/asm/global_data.h @@ -6,8 +6,6 @@ #ifndef __ASM_ARC_GLOBAL_DATA_H #define __ASM_ARC_GLOBAL_DATA_H
-#include <config.h> - #ifndef __ASSEMBLY__ /* Architecture-specific global data */ struct arch_global_data { diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h index 2aff1c467c14..79432f3bbd24 100644 --- a/arch/arm/include/asm/global_data.h +++ b/arch/arm/include/asm/global_data.h @@ -9,6 +9,8 @@
#ifndef __ASSEMBLY__
+#include <config.h> + #include <asm/types.h> #include <linux/types.h>
diff --git a/arch/m68k/include/asm/global_data.h b/arch/m68k/include/asm/global_data.h index 188055e9d314..273e843c4ae6 100644 --- a/arch/m68k/include/asm/global_data.h +++ b/arch/m68k/include/asm/global_data.h @@ -7,6 +7,8 @@ #ifndef __ASM_GBL_DATA_H #define __ASM_GBL_DATA_H
+#include <config.h> + /* Architecture-specific global data */ struct arch_global_data { #ifdef CONFIG_SYS_I2C_FSL diff --git a/arch/nds32/include/asm/global_data.h b/arch/nds32/include/asm/global_data.h index be04a18857a1..297481beaaef 100644 --- a/arch/nds32/include/asm/global_data.h +++ b/arch/nds32/include/asm/global_data.h @@ -17,6 +17,8 @@ #ifndef __ASM_GBL_DATA_H #define __ASM_GBL_DATA_H
+#include <config.h> + /* Architecture-specific global data */ struct arch_global_data { };
On Thu, Jun 03, 2021 at 09:39:02AM -0400, Tom Rini wrote:
All symbols that are defined in Kconfig will always be defined (or not) prior to preprocessing due to the -include directive while building. However, symbols which are not yet migrated will only be defined (or not) once the board config.h is included, via <config.h>. While the end goal must be to migrate all symbols, today we have cases where the size of gd will get mismatched within the build, based on include order. Mitigate this by making sure that any <asm/global_data.h> that uses symbols not in Kconfig does start with <config.h>. Remove this when not needed.
Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Huan Wang alison.wang@nxp.com Cc: Angelo Dureghello angelo@sysam.it Cc: Rick Chen rick@andestech.com Signed-off-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
On Thu, 03 Jun 2021 09:38:59 -0400 "Tom Rini" trini@konsulko.com wrote:
As there is an arch/powerpc/include/asm/config.h file using "" to get config.h here can lead to using that rather than include/config.h. This in turn can lead to a mismatch in the size of gd.
Cc: Matt Merhar mattmerhar@protonmail.com Signed-off-by: Tom Rini trini@konsulko.com
arch/powerpc/include/asm/global_data.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h index 192a02d347e7..90bf5a2aea5d 100644 --- a/arch/powerpc/include/asm/global_data.h +++ b/arch/powerpc/include/asm/global_data.h @@ -8,7 +8,7 @@ #ifndef __ASM_GBL_DATA_H #define __ASM_GBL_DATA_H
-#include "config.h" +#include <config.h> #include "asm/types.h"
/* Architecture-specific global data */
2.17.1
I think this is a better solution, as it also appears to fix another global_data size mismatch caught by the RFC from Rasmus "global-data.h: add build-time sanity check of sizeof(struct global_data)".
Tested-by: Matt Merhar mattmerhar@protonmail.com
On Thu, Jun 03, 2021 at 09:38:59AM -0400, Tom Rini wrote:
As there is an arch/powerpc/include/asm/config.h file using "" to get config.h here can lead to using that rather than include/config.h. This in turn can lead to a mismatch in the size of gd.
Cc: Matt Merhar mattmerhar@protonmail.com Signed-off-by: Tom Rini trini@konsulko.com Tested-by: Matt Merhar mattmerhar@protonmail.com
Applied to u-boot/master, thanks!
On Mon, May 17, 2021 at 05:32:48PM +0000, Matt Merhar wrote:
The assembly output of the arch_initr_trap() function differed by a single byte after common.h was removed from traps.c:
fff49a18 <arch_initr_trap>: fff49a18: 94 21 ff f0 stwu r1,-16(r1) fff49a1c: 7c 08 02 a6 mflr r0 fff49a20: 90 01 00 14 stw r0,20(r1) -fff49a24: 80 62 00 44 lwz r3,68(r2) +fff49a24: 80 62 00 38 lwz r3,56(r2) fff49a28: 4b ff 76 19 bl fff41040 <trap_init> fff49a2c: 80 01 00 14 lwz r0,20(r1) fff49a30: 38 60 00 00 li r3,0 fff49a34: 38 21 00 10 addi r1,r1,16 fff49a38: 7c 08 03 a6 mtlr r0
This was causing a consistent hard lockup during the MMC read / loading of the QoriQ FMan firmware on a P2041RDB board.
Re-adding the header causes identical assembly to be emitted and allows the firmware loading and subsequent boot to succeed.
Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header") Signed-off-by: Matt Merhar mattmerhar@protonmail.com
Applied to u-boot/master, thanks!
participants (5)
-
Matt Merhar -
Rasmus Villemoes -
Simon Glass -
Stefan Roese -
Tom Rini