[PATCH] ARM: Remove kernel booting call for thumb2 mode

[PATCH] ARM: Remove kernel booting call for thumb2 mode

[Trent Piepho]

The asm code to do the actual call into the kernel (or another
barebox) when compiled in thumb2 mode isn't necessary.  gcc generates
a perfectly good calling sequence from a normal function pointer call.
If it didn't, the code in bootstrap_boot() that uses a function
pointer to jump to the 2nd stage barebox from an xloader wouldn't
work.

It appears to be allowed that the call to kernel() could return, as
neither start_linux() nor kernel() are marked noreturn, and there is
code after calls to start_linux().

The asm code has a bug in this case, as it uses bx and not blx, and
thus doesn't set the link register.  Since it's a tail call, this
would be okay, but only if the LR value from the start of
start_linux() (and the callee-saved registers) are restored
beforehand, which isn't done.  The gcc generated call sequence will do
this.

Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
 arch/arm/lib/armlinux.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/arm/lib/armlinux.c b/arch/arm/lib/armlinux.c
index 16879f8..47b9bd3 100644
--- a/arch/arm/lib/armlinux.c
+++ b/arch/arm/lib/armlinux.c
@@ -277,17 +277,5 @@ void start_linux(void *adr, int swap, unsigned long initrd_address,
 		__asm__ __volatile__("mcr p15, 0, %0, c1, c0" :: "r" (reg));
 	}
 
-#ifdef CONFIG_THUMB2_BAREBOX
-	__asm__ __volatile__ (
-		"mov r0, #0\n"
-		"mov r1, %0\n"
-		"mov r2, %1\n"
-		"bx %2\n"
-		:
-		: "r" (architecture), "r" (params), "r" (kernel)
-		: "r0", "r1", "r2"
-	);
-#else
 	kernel(0, architecture, params);
-#endif
 }
-- 
1.8.3.1


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

Re: [PATCH] ARM: Remove kernel booting call for thumb2 mode

[Trent Piepho]

On Thu, 2015-12-03 at 01:24 +0000, Trent Piepho wrote:
> The asm code to do the actual call into the kernel (or another
> barebox) when compiled in thumb2 mode isn't necessary.  gcc generates
> a perfectly good calling sequence from a normal function pointer call.
> If it didn't, the code in bootstrap_boot() that uses a function
> pointer to jump to the 2nd stage barebox from an xloader wouldn't
> work.

The same commit that originally added this code also added
thumb2_execute(), which is used by the go command in a thumb2 build.

This doesn't seem necessary for the same reason as the code in
start_linux.  THUMB2 selects AEABI, which in turn turns on the aapcs
abi, which requires interworking safe indirect calls.  And again, the
bootstrap code doesn't use a special call sequence so if it didn't work
that would be broken too.

The code for thumb2_execute() is also odd in that it subtracts one from
argc, drops the 1st argv entry, and supplies zero for the first function
argument.  I.e., "go" in arm mode (and all non-arm arches) does this:
	func(argc - 1, &argv[1]);
and in thumb2 mode does this:
	func(0, argc - 2, &argv[2]);

I don't see why one would want this difference.

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

Re: [PATCH] ARM: Remove kernel booting call for thumb2 mode

[Sascha Hauer]

On Fri, Dec 04, 2015 at 02:28:40AM +0000, Trent Piepho wrote:
> On Thu, 2015-12-03 at 01:24 +0000, Trent Piepho wrote:
> > The asm code to do the actual call into the kernel (or another
> > barebox) when compiled in thumb2 mode isn't necessary.  gcc generates
> > a perfectly good calling sequence from a normal function pointer call.
> > If it didn't, the code in bootstrap_boot() that uses a function
> > pointer to jump to the 2nd stage barebox from an xloader wouldn't
> > work.
> 
> The same commit that originally added this code also added
> thumb2_execute(), which is used by the go command in a thumb2 build.
> 
> This doesn't seem necessary for the same reason as the code in
> start_linux.  THUMB2 selects AEABI, which in turn turns on the aapcs
> abi, which requires interworking safe indirect calls.  And again, the
> bootstrap code doesn't use a special call sequence so if it didn't work
> that would be broken too.
> 
> The code for thumb2_execute() is also odd in that it subtracts one from
> argc, drops the 1st argv entry, and supplies zero for the first function
> argument.  I.e., "go" in arm mode (and all non-arm arches) does this:
> 	func(argc - 1, &argv[1]);
> and in thumb2 mode does this:
> 	func(0, argc - 2, &argv[2]);
> 
> I don't see why one would want this difference.

This is a bug. We want to shift argv[] by one, not by two. I must
accidently have done this twice.
As you note the thumb2 execution code is unnecessary anyway, we can
remove the bug by removing the code.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH] ARM: Remove kernel booting call for thumb2 mode

[Trent Piepho]

On Fri, 2015-12-04 at 08:30 +0100, Sascha Hauer wrote:
> On Fri, Dec 04, 2015 at 02:28:40AM +0000, Trent Piepho wrote:
> > 
> > This doesn't seem necessary for the same reason as the code in
> > start_linux.  THUMB2 selects AEABI, which in turn turns on the aapcs
> > abi, which requires interworking safe indirect calls.  And again, the
> > bootstrap code doesn't use a special call sequence so if it didn't work
> > that would be broken too.
> > 

> 
> This is a bug. We want to shift argv[] by one, not by two. I must
> accidently have done this twice.
> As you note the thumb2 execution code is unnecessary anyway, we can
> remove the bug by removing the code.

Ok, I'll send a patch for it.  I thought maybe there was some sort of
gcc bug or deficiency back in 2011 that made this necessary.  Though I
did search for bug with thumb interworking or "mov pc, reg" and didn't
find any that looked like they would be relevant.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: Remove kernel booting call for thumb2 mode

[Sascha Hauer]

On Thu, Dec 03, 2015 at 01:24:56AM +0000, Trent Piepho wrote:
> The asm code to do the actual call into the kernel (or another
> barebox) when compiled in thumb2 mode isn't necessary.  gcc generates
> a perfectly good calling sequence from a normal function pointer call.
> If it didn't, the code in bootstrap_boot() that uses a function
> pointer to jump to the 2nd stage barebox from an xloader wouldn't
> work.
> 
> It appears to be allowed that the call to kernel() could return, as
> neither start_linux() nor kernel() are marked noreturn, and there is
> code after calls to start_linux().
> 
> The asm code has a bug in this case, as it uses bx and not blx, and
> thus doesn't set the link register.  Since it's a tail call, this
> would be okay, but only if the LR value from the start of
> start_linux() (and the callee-saved registers) are restored
> beforehand, which isn't done.  The gcc generated call sequence will do
> this.

The kernel won't return, so this bug shouldn't hit us. Anyway, nice
cleanup. Tested and applied, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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