| Message ID | 20170227191907.10196-1-albert.u.boot@aribaud.net |
|---|---|
| State | Accepted |
| Commit | 6b4e9426834580810790ecfa924979d61c5c1987 |
| Delegated to: | Tom Rini |
| Headers | show |
On Mon, Feb 27, 2017 at 08:19:07PM +0100, Albert ARIBAUD wrote: > Current ARM assembler helper for the 'return to caller' pseudo-instruction > turns 'ret lr' into 'mov pc, lr' for ARMv5TE. This causes the core to remain > in its current ARM state even when the routine doing the 'ret' was called > from Thumb-1 state, triggering an undefined instruction exception. > > This causes early run-time failures in all boards compiled using the Thumb-1 > instruction set (for instance the Open-RD family). > > ARMv5TE supports 'bx lr' which properly implements interworking and thus > correctly returns to Thumb-1 state from ARM state. > > This change makes 'ret lr' turn into 'bx lr' for ARMv5TE. > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> > --- > Note: this patch supersedes patch "openrd: disable private arch memset, > memcpy and libgcc" dated Sun, 26 Feb 2017 16:29:32 +0100. > > arch/arm/include/asm/assembler.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h > index ae1e42fc06..c56daf2a1f 100644 > --- a/arch/arm/include/asm/assembler.h > +++ b/arch/arm/include/asm/assembler.h > @@ -59,7 +59,7 @@ > > .irp c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo > .macro ret\c, reg > -#if defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) > +#if defined(__ARM_ARCH_5E__) > mov\c pc, \reg > #else > .ifeqs "\reg", "lr" Emperical wins over theory, so I'll take this patch so we don't break things. But looking at the kernel, the above is a test for __LINUX_ARM_ARCH__ < 6 instead. But the kernel is hardly ever built and run in Thumb mode rather than ARM mode, so they wouldn't be tickling this particular issue. I guess my big question now is, when can / should we not just use 'bx lr' over 'mov pc, lr' ?
Tom Rini <trini@konsulko.com> writes: > On Mon, Feb 27, 2017 at 08:19:07PM +0100, Albert ARIBAUD wrote: >> Current ARM assembler helper for the 'return to caller' pseudo-instruction >> turns 'ret lr' into 'mov pc, lr' for ARMv5TE. This causes the core to remain >> in its current ARM state even when the routine doing the 'ret' was called >> from Thumb-1 state, triggering an undefined instruction exception. >> >> This causes early run-time failures in all boards compiled using the Thumb-1 >> instruction set (for instance the Open-RD family). >> >> ARMv5TE supports 'bx lr' which properly implements interworking and thus >> correctly returns to Thumb-1 state from ARM state. >> >> This change makes 'ret lr' turn into 'bx lr' for ARMv5TE. >> >> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> >> --- >> Note: this patch supersedes patch "openrd: disable private arch memset, >> memcpy and libgcc" dated Sun, 26 Feb 2017 16:29:32 +0100. >> >> arch/arm/include/asm/assembler.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h >> index ae1e42fc06..c56daf2a1f 100644 >> --- a/arch/arm/include/asm/assembler.h >> +++ b/arch/arm/include/asm/assembler.h >> @@ -59,7 +59,7 @@ >> >> .irp c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo >> .macro ret\c, reg >> -#if defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) >> +#if defined(__ARM_ARCH_5E__) >> mov\c pc, \reg >> #else >> .ifeqs "\reg", "lr" > > Emperical wins over theory, so I'll take this patch so we don't break > things. But looking at the kernel, the above is a test for > __LINUX_ARM_ARCH__ < 6 instead. But the kernel is hardly ever built and > run in Thumb mode rather than ARM mode, so they wouldn't be tickling > this particular issue. The kernel needs Thumb-2, so the requirements are a bit different there. > I guess my big question now is, when can / should we not just use 'bx > lr' over 'mov pc, lr' ? All cores with Thumb support the bx instruction. The only time you want to use "mov pc" is with non-Thumb v4 and v5 cores.
On Thu, Mar 02, 2017 at 02:27:03PM +0000, Måns Rullgård wrote: > Tom Rini <trini@konsulko.com> writes: > > > On Mon, Feb 27, 2017 at 08:19:07PM +0100, Albert ARIBAUD wrote: > >> Current ARM assembler helper for the 'return to caller' pseudo-instruction > >> turns 'ret lr' into 'mov pc, lr' for ARMv5TE. This causes the core to remain > >> in its current ARM state even when the routine doing the 'ret' was called > >> from Thumb-1 state, triggering an undefined instruction exception. > >> > >> This causes early run-time failures in all boards compiled using the Thumb-1 > >> instruction set (for instance the Open-RD family). > >> > >> ARMv5TE supports 'bx lr' which properly implements interworking and thus > >> correctly returns to Thumb-1 state from ARM state. > >> > >> This change makes 'ret lr' turn into 'bx lr' for ARMv5TE. > >> > >> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> > >> --- > >> Note: this patch supersedes patch "openrd: disable private arch memset, > >> memcpy and libgcc" dated Sun, 26 Feb 2017 16:29:32 +0100. > >> > >> arch/arm/include/asm/assembler.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h > >> index ae1e42fc06..c56daf2a1f 100644 > >> --- a/arch/arm/include/asm/assembler.h > >> +++ b/arch/arm/include/asm/assembler.h > >> @@ -59,7 +59,7 @@ > >> > >> .irp c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo > >> .macro ret\c, reg > >> -#if defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) > >> +#if defined(__ARM_ARCH_5E__) > >> mov\c pc, \reg > >> #else > >> .ifeqs "\reg", "lr" > > > > Emperical wins over theory, so I'll take this patch so we don't break > > things. But looking at the kernel, the above is a test for > > __LINUX_ARM_ARCH__ < 6 instead. But the kernel is hardly ever built and > > run in Thumb mode rather than ARM mode, so they wouldn't be tickling > > this particular issue. > > The kernel needs Thumb-2, so the requirements are a bit different there. Ah, right. > > I guess my big question now is, when can / should we not just use 'bx > > lr' over 'mov pc, lr' ? > > All cores with Thumb support the bx instruction. The only time you want > to use "mov pc" is with non-Thumb v4 and v5 cores. So, of ARM720T, ARM920T, ARM926EJS and ARM946ES, which fall into that category? Or is it harder to say than that?
Tom Rini <trini@konsulko.com> writes: > On Thu, Mar 02, 2017 at 02:27:03PM +0000, Måns Rullgård wrote: >> Tom Rini <trini@konsulko.com> writes: >> >> > On Mon, Feb 27, 2017 at 08:19:07PM +0100, Albert ARIBAUD wrote: >> >> Current ARM assembler helper for the 'return to caller' pseudo-instruction >> >> turns 'ret lr' into 'mov pc, lr' for ARMv5TE. This causes the core to remain >> >> in its current ARM state even when the routine doing the 'ret' was called >> >> from Thumb-1 state, triggering an undefined instruction exception. >> >> >> >> This causes early run-time failures in all boards compiled using the Thumb-1 >> >> instruction set (for instance the Open-RD family). >> >> >> >> ARMv5TE supports 'bx lr' which properly implements interworking and thus >> >> correctly returns to Thumb-1 state from ARM state. >> >> >> >> This change makes 'ret lr' turn into 'bx lr' for ARMv5TE. >> >> >> >> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> >> >> --- >> >> Note: this patch supersedes patch "openrd: disable private arch memset, >> >> memcpy and libgcc" dated Sun, 26 Feb 2017 16:29:32 +0100. >> >> >> >> arch/arm/include/asm/assembler.h | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h >> >> index ae1e42fc06..c56daf2a1f 100644 >> >> --- a/arch/arm/include/asm/assembler.h >> >> +++ b/arch/arm/include/asm/assembler.h >> >> @@ -59,7 +59,7 @@ >> >> >> >> .irp c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo >> >> .macro ret\c, reg >> >> -#if defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) >> >> +#if defined(__ARM_ARCH_5E__) >> >> mov\c pc, \reg >> >> #else >> >> .ifeqs "\reg", "lr" >> > >> > Emperical wins over theory, so I'll take this patch so we don't break >> > things. But looking at the kernel, the above is a test for >> > __LINUX_ARM_ARCH__ < 6 instead. But the kernel is hardly ever built and >> > run in Thumb mode rather than ARM mode, so they wouldn't be tickling >> > this particular issue. >> >> The kernel needs Thumb-2, so the requirements are a bit different there. > > Ah, right. > >> > I guess my big question now is, when can / should we not just use 'bx >> > lr' over 'mov pc, lr' ? >> >> All cores with Thumb support the bx instruction. The only time you want >> to use "mov pc" is with non-Thumb v4 and v5 cores. > > So, of ARM720T, ARM920T, ARM926EJS and ARM946ES, which fall into that > category? Or is it harder to say than that? All of those support Thumb and thus the bx instruction. The blx instruction, which simplifies interworking, is only available from v5T, but that's not relevant here.
On Mon, Feb 27, 2017 at 08:19:07PM +0100, Albert ARIBAUD wrote: > Current ARM assembler helper for the 'return to caller' pseudo-instruction > turns 'ret lr' into 'mov pc, lr' for ARMv5TE. This causes the core to remain > in its current ARM state even when the routine doing the 'ret' was called > from Thumb-1 state, triggering an undefined instruction exception. > > This causes early run-time failures in all boards compiled using the Thumb-1 > instruction set (for instance the Open-RD family). > > ARMv5TE supports 'bx lr' which properly implements interworking and thus > correctly returns to Thumb-1 state from ARM state. > > This change makes 'ret lr' turn into 'bx lr' for ARMv5TE. > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> Applied to u-boot/master, thanks!
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h index ae1e42fc06..c56daf2a1f 100644 --- a/arch/arm/include/asm/assembler.h +++ b/arch/arm/include/asm/assembler.h @@ -59,7 +59,7 @@ .irp c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo .macro ret\c, reg -#if defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) +#if defined(__ARM_ARCH_5E__) mov\c pc, \reg #else .ifeqs "\reg", "lr"
Current ARM assembler helper for the 'return to caller' pseudo-instruction turns 'ret lr' into 'mov pc, lr' for ARMv5TE. This causes the core to remain in its current ARM state even when the routine doing the 'ret' was called from Thumb-1 state, triggering an undefined instruction exception. This causes early run-time failures in all boards compiled using the Thumb-1 instruction set (for instance the Open-RD family). ARMv5TE supports 'bx lr' which properly implements interworking and thus correctly returns to Thumb-1 state from ARM state. This change makes 'ret lr' turn into 'bx lr' for ARMv5TE. Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> --- Note: this patch supersedes patch "openrd: disable private arch memset, memcpy and libgcc" dated Sun, 26 Feb 2017 16:29:32 +0100. arch/arm/include/asm/assembler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)