[PATCH] ARM: aarch64: Re-implement most of barebox_arm_entry() in assembly

* [PATCH] ARM: aarch64: Re-implement most of barebox_arm_entry() in assembly
@ 2019-09-07 23:56 Andrey Smirnov
  2019-09-09  8:42 ` Sascha Hauer
  0 siblings, 1 reply; 3+ messages in thread
From: Andrey Smirnov @ 2019-09-07 23:56 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

GCC9 now produces the following warning:

common.h:51:2: warning: listing the stack pointer register ‘sp’ in a clobber list is deprecated [-Wdeprecated]
   51 |  __asm__ __volatile__("mov sp, %0"
      |  ^~~~~~~
common.h:51:2: note: the value of the stack pointer after  an ‘asm’ statement must be the same as it was before the statement

Stack pointer was added to clobber list in commit f9fc8254b2 ("ARM:
Mark SP as being clobbered in arm_setup_stack()") to prevent GCC from
generating code that would corrupt 'boarddata' pointer by trying to
restore it from invalid stack frame.

Interestingly enough, seemingly unrelated change in commit
64d95896cf ("ARM: aarch64: compile with general-regs-only") changed
generated code such that adding SP to clobber list became no longer
necessary.

While the above can probably a fix by itself, it seems a better and
more future proof approach would be to address the problem at its root
and re-implement offending startup sequence in assembly.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/cpu/Makefile         | 10 ++++++++--
 arch/arm/cpu/entry.c          | 27 ++++++++++++++++++++++++---
 arch/arm/cpu/entry_64.S       | 25 +++++++++++++++++++++++++
 arch/arm/include/asm/common.h |  3 +--
 4 files changed, 58 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm/cpu/entry_64.S

diff --git a/arch/arm/cpu/Makefile b/arch/arm/cpu/Makefile
index 97e4eb52e3..4c98dfa6e6 100644
--- a/arch/arm/cpu/Makefile
+++ b/arch/arm/cpu/Makefile
@@ -8,7 +8,13 @@ obj-pbl-$(CONFIG_CPU_32v7) += hyp.o
 AFLAGS_hyp.o :=-Wa,-march=armv7-a -Wa,-mcpu=all
 AFLAGS_pbl-hyp.o :=-Wa,-march=armv7-a -Wa,-mcpu=all
 
-obj-y += start.o entry.o
+#
+# ARM64 needs both entry.o (for barebox_arm_entry()) and entry_64.o
+# (for __barebox_arm_entry()). ARM only needs entry.o, but S64 will
+# resolve into empty string there, so the following will just end up
+# specifyin entry.o twice which shoulnd't be a problem
+#
+obj-y += start.o entry.o entry$(S64).o
 
 pbl-$(CONFIG_BOARD_ARM_GENERIC_DT) += board-dt-2nd.o
 pbl-$(CONFIG_BOARD_ARM_GENERIC_DT_AARCH64) += board-dt-2nd-aarch64.o
@@ -47,7 +53,7 @@ AFLAGS_cache-armv8.o       :=-Wa,-march=armv8-a
 obj-pbl-$(CONFIG_CPU_64v8) += cache-armv8.o
 AFLAGS_pbl-cache-armv8.o       :=-Wa,-march=armv8-a
 
-pbl-y += entry.o
+pbl-y += entry.o entry$(S64).o
 pbl-$(CONFIG_PBL_SINGLE_IMAGE) += start-pbl.o
 pbl-$(CONFIG_PBL_MULTI_IMAGES) += uncompress.o
 
diff --git a/arch/arm/cpu/entry.c b/arch/arm/cpu/entry.c
index b48c1ca11d..e7cc492386 100644
--- a/arch/arm/cpu/entry.c
+++ b/arch/arm/cpu/entry.c
@@ -24,10 +24,13 @@
  * be fine.
  */
 
-void NAKED __noreturn barebox_arm_entry(unsigned long membase,
-					  unsigned long memsize, void *boarddata)
+#ifdef CONFIG_CPU_32
+static __always_inline void __barebox_arm_entry(unsigned long membase,
+						unsigned long memsize,
+						void *boarddata,
+						unsigned long sp)
 {
-	arm_setup_stack(arm_mem_stack_top(membase, membase + memsize) - 16);
+	arm_setup_stack(sp);
 	arm_early_mmu_cache_invalidate();
 
 	if (IS_ENABLED(CONFIG_PBL_MULTI_IMAGES))
@@ -37,3 +40,21 @@ void NAKED __noreturn barebox_arm_entry(unsigned long membase,
 	else
 		barebox_non_pbl_start(membase, memsize, boarddata);
 }
+#else
+/*
+ * It can be hard to convince GCC to not use old stack pointer after
+ * we modify it with arm_setup_stack() on ARM64, so we implement the
+ * above code in assembly
+ */
+void __noreturn __barebox_arm_entry(unsigned long membase,
+				    unsigned long memsize,
+				    void *boarddata,
+				    unsigned long sp);
+#endif
+
+void NAKED __noreturn barebox_arm_entry(unsigned long membase,
+					unsigned long memsize, void *boarddata)
+{
+	__barebox_arm_entry(membase, memsize, boarddata,
+			  arm_mem_stack_top(membase, membase + memsize) - 16);
+}
diff --git a/arch/arm/cpu/entry_64.S b/arch/arm/cpu/entry_64.S
new file mode 100644
index 0000000000..c829bd6efd
--- /dev/null
+++ b/arch/arm/cpu/entry_64.S
@@ -0,0 +1,25 @@
+#include <linux/linkage.h>
+#include <asm/sections.h>
+
+/*
+ * x0: memory base
+ * x1: memory size
+ * x2: board data
+ * x3: new value for SP
+ */
+.section .text.__barebox_arm_entry
+ENTRY(__barebox_arm_entry)
+	mov	sp, x3
+	/*
+	 * arm_early_mmu_cache_invalidate is jsut a call to
+	 * v8_invalidate_icache_all() which doesn't clobber x0, x1 or x2
+ 	 */
+	bl	arm_early_mmu_cache_invalidate
+#if IS_ENABLED(CONFIG_PBL_MULTI_IMAGES)
+	b	barebox_multi_pbl_start
+#elif IS_ENABLED(CONFIG_PBL_SINGLE_IMAGE)
+	b	barebox_single_pbl_start
+#else
+	b	barebox_non_pbl_start
+#endif
+ENDPROC(__barebox_arm_entry)
\ No newline at end of file
diff --git a/arch/arm/include/asm/common.h b/arch/arm/include/asm/common.h
index c32cdfe5ec..d03ee6273f 100644
--- a/arch/arm/include/asm/common.h
+++ b/arch/arm/include/asm/common.h
@@ -50,8 +50,7 @@ static inline void arm_setup_stack(unsigned long top)
 {
 	__asm__ __volatile__("mov sp, %0"
 			     :
-			     : "r"(top)
-			     : "sp");
+			     : "r"(top));
 }
 
 #endif /* __ASM_ARM_COMMON_H */
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 3+ messages in thread