| Message ID | 1429637818-11529-3-git-send-email-mporter@konsulko.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > On ARM v7M, the processor will return to ARM mode when executing > a blx instruction with bit 0 of the address == 0. Always set it but that's what the 'x' is for, right ? eXchange the CPU mode. > to 1 to stay in thumb mode. > > Signed-off-by: Matt Porter <mporter@konsulko.com> > --- > common/cmd_boot.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > index 8f2e070..20ce652 100644 > --- a/common/cmd_boot.c > +++ b/common/cmd_boot.c > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > * pass address parameter as argv[0] (aka command name), > * and all remaining args > */ > +#ifdef CONFIG_CPU_V7M > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > + addr++; > +#endif what if we were in ARM state when we reached this point ? You're now telling CPU to always switch to Thumb. Is this really what we want ? From ARM's instruction manual: " The BX and BLX instructions can change the processor state from ARM to Thumb, or from Thumb to ARM. BLX label always changes the state. BX Rm and BLX Rm derive the target state from bit[0] of Rm: if bit[0] of Rm is 0, the processor changes to, or remains in, ARM state if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb state. "
On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > On ARM v7M, the processor will return to ARM mode when executing > > a blx instruction with bit 0 of the address == 0. Always set it > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > > to 1 to stay in thumb mode. > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > --- > > common/cmd_boot.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > index 8f2e070..20ce652 100644 > > --- a/common/cmd_boot.c > > +++ b/common/cmd_boot.c > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > * pass address parameter as argv[0] (aka command name), > > * and all remaining args > > */ > > +#ifdef CONFIG_CPU_V7M > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > + addr++; > > +#endif > > what if we were in ARM state when we reached this point ? You're now > telling CPU to always switch to Thumb. Is this really what we want ? > > From ARM's instruction manual: > > > > " > The BX and BLX instructions can change the processor state from ARM to > Thumb, or from Thumb to ARM. > > BLX label always changes the state. > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > state > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > state. > " oh wait, this is cortex-m, it's supposed to be thumb2 only, why do we even need that bit ?
On Tue, Apr 21, 2015 at 12:54:26PM -0500, Felipe Balbi wrote: > On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > > On ARM v7M, the processor will return to ARM mode when executing > > > a blx instruction with bit 0 of the address == 0. Always set it > > > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > > > > to 1 to stay in thumb mode. > > > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > > --- > > > common/cmd_boot.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > > index 8f2e070..20ce652 100644 > > > --- a/common/cmd_boot.c > > > +++ b/common/cmd_boot.c > > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > * pass address parameter as argv[0] (aka command name), > > > * and all remaining args > > > */ > > > +#ifdef CONFIG_CPU_V7M > > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > > + addr++; > > > +#endif > > > > what if we were in ARM state when we reached this point ? You're now > > telling CPU to always switch to Thumb. Is this really what we want ? > > > > From ARM's instruction manual: > > > > > > > > " > > The BX and BLX instructions can change the processor state from ARM to > > Thumb, or from Thumb to ARM. > > > > BLX label always changes the state. > > > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > > state > > > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > > state. > > " > > oh wait, this is cortex-m, it's supposed to be thumb2 only, why do we > even need that bit ? seems like it must be set for cortex-m, but then shouldn't this be done by GCC ? Are we, perhaps, using wrong GCC arguments when building for cortex-m ?
On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > On ARM v7M, the processor will return to ARM mode when executing > > a blx instruction with bit 0 of the address == 0. Always set it > > but that's what the 'x' is for, right ? eXchange the CPU mode. Yes. > > to 1 to stay in thumb mode. > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > --- > > common/cmd_boot.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > index 8f2e070..20ce652 100644 > > --- a/common/cmd_boot.c > > +++ b/common/cmd_boot.c > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > * pass address parameter as argv[0] (aka command name), > > * and all remaining args > > */ > > +#ifdef CONFIG_CPU_V7M > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > + addr++; > > +#endif > > what if we were in ARM state when we reached this point ? You're now > telling CPU to always switch to Thumb. Is this really what we want ? We have no ARM state on this core so that's not possible. > From ARM's instruction manual: > > > > " > The BX and BLX instructions can change the processor state from ARM to > Thumb, or from Thumb to ARM. > > BLX label always changes the state. > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > state > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > state. Correct. The last statement is why this patch exists. There is no ARM mode on M3, we immediately fault. Having bit[0]=0 for BX/BLX is not permitted on V7M...something not covered in the generic description of these instructions. Incidentally, I forgot to update this with Kamil's comment that it should be implemented as |1 and will address that now. -Matt
On Tue, Apr 21, 2015 at 02:01:31PM -0400, Matt Porter wrote: > On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > > On ARM v7M, the processor will return to ARM mode when executing > > > a blx instruction with bit 0 of the address == 0. Always set it > > > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > Yes. > > > > to 1 to stay in thumb mode. > > > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > > --- > > > common/cmd_boot.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > > index 8f2e070..20ce652 100644 > > > --- a/common/cmd_boot.c > > > +++ b/common/cmd_boot.c > > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > * pass address parameter as argv[0] (aka command name), > > > * and all remaining args > > > */ > > > +#ifdef CONFIG_CPU_V7M > > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > > + addr++; > > > +#endif > > > > what if we were in ARM state when we reached this point ? You're now > > telling CPU to always switch to Thumb. Is this really what we want ? > > We have no ARM state on this core so that's not possible. > > > From ARM's instruction manual: > > > > > > > > " > > The BX and BLX instructions can change the processor state from ARM to > > Thumb, or from Thumb to ARM. > > > > BLX label always changes the state. > > > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > > state > > > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > > state. > > Correct. The last statement is why this patch exists. There is no ARM > mode on M3, we immediately fault. Having bit[0]=0 for BX/BLX is not > permitted on V7M...something not covered in the generic description > of these instructions. it just seems weird that bit0 wouldn't just be assume 1 by the core itself. I suppose as a consequence we can't use blx label with v7m either ? :-s cheers
On Tue, Apr 21, 2015 at 12:57:09PM -0500, Felipe Balbi wrote: > On Tue, Apr 21, 2015 at 12:54:26PM -0500, Felipe Balbi wrote: > > On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > > > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > > > On ARM v7M, the processor will return to ARM mode when executing > > > > a blx instruction with bit 0 of the address == 0. Always set it > > > > > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > > > > > > to 1 to stay in thumb mode. > > > > > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > > > --- > > > > common/cmd_boot.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > > > index 8f2e070..20ce652 100644 > > > > --- a/common/cmd_boot.c > > > > +++ b/common/cmd_boot.c > > > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > > * pass address parameter as argv[0] (aka command name), > > > > * and all remaining args > > > > */ > > > > +#ifdef CONFIG_CPU_V7M > > > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > > > + addr++; > > > > +#endif > > > > > > what if we were in ARM state when we reached this point ? You're now > > > telling CPU to always switch to Thumb. Is this really what we want ? > > > > > > From ARM's instruction manual: > > > > > > > > > > > > " > > > The BX and BLX instructions can change the processor state from ARM to > > > Thumb, or from Thumb to ARM. > > > > > > BLX label always changes the state. > > > > > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > > > > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > > > state > > > > > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > > > state. > > > " > > > > oh wait, this is cortex-m, it's supposed to be thumb2 only, why do we > > even need that bit ? > > seems like it must be set for cortex-m, but then shouldn't this be done > by GCC ? Are we, perhaps, using wrong GCC arguments when building for > cortex-m ? From "make V=1": ... -march=armv7-m -mthumb ... $ arm-none-eabi-gcc -v ... gcc version 4.8.3 20140913 (release) (4.8.3-11ubuntu1+11) -Matt
On Tue, Apr 21, 2015 at 01:05:24PM -0500, Felipe Balbi wrote: > On Tue, Apr 21, 2015 at 02:01:31PM -0400, Matt Porter wrote: > > On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > > > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > > > On ARM v7M, the processor will return to ARM mode when executing > > > > a blx instruction with bit 0 of the address == 0. Always set it > > > > > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > > > Yes. > > > > > > to 1 to stay in thumb mode. > > > > > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > > > --- > > > > common/cmd_boot.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > > > index 8f2e070..20ce652 100644 > > > > --- a/common/cmd_boot.c > > > > +++ b/common/cmd_boot.c > > > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > > * pass address parameter as argv[0] (aka command name), > > > > * and all remaining args > > > > */ > > > > +#ifdef CONFIG_CPU_V7M > > > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > > > + addr++; > > > > +#endif > > > > > > what if we were in ARM state when we reached this point ? You're now > > > telling CPU to always switch to Thumb. Is this really what we want ? > > > > We have no ARM state on this core so that's not possible. > > > > > From ARM's instruction manual: > > > > > > > > > > > > " > > > The BX and BLX instructions can change the processor state from ARM to > > > Thumb, or from Thumb to ARM. > > > > > > BLX label always changes the state. > > > > > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > > > > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > > > state > > > > > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > > > state. > > > > Correct. The last statement is why this patch exists. There is no ARM > > mode on M3, we immediately fault. Having bit[0]=0 for BX/BLX is not > > permitted on V7M...something not covered in the generic description > > of these instructions. > > it just seems weird that bit0 wouldn't just be assume 1 by the core > itself. I suppose as a consequence we can't use blx label with v7m > either ? :-s It also seems weird to me that it wouldn't just be ignored on anything armv7-m. Yes, blx label would be "very bad" which is why it is not supported on ARMv7-m :) And to clarify for those listening at home... From The ARMv7-M Instruction Set Appendix A4.1.1: " Thumb interworking is held as bit [0] of an interworking address. Interworking addresses are used in the following instructions: • BX or BLX • an LDR or LDM that loads the PC. ARMv7-M only supports the Thumb instruction execution state, therefore the value of address bit [0] must be 1 in interworking instructions, otherwise a fault occurs. All instructions ignore bit [0] and write bits [31:1]:’0’ when updating the PC. " Also: " A7.7.19 BLX (register) Branch with Link and Exchange calls a subroutine at an address and instruction set specified by a register. ARMv7-M only supports the Thumb instruction set. An attempt to change the instruction execution state causes the processor to take an exception on the instruction at the target address. " -Matt
On Tue, Apr 21, 2015 at 01:05:24PM -0500, Felipe Balbi wrote: > On Tue, Apr 21, 2015 at 02:01:31PM -0400, Matt Porter wrote: > > On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > > > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > > > On ARM v7M, the processor will return to ARM mode when executing > > > > a blx instruction with bit 0 of the address == 0. Always set it > > > > > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > > > Yes. > > > > > > to 1 to stay in thumb mode. > > > > > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > > > --- > > > > common/cmd_boot.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > > > index 8f2e070..20ce652 100644 > > > > --- a/common/cmd_boot.c > > > > +++ b/common/cmd_boot.c > > > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > > * pass address parameter as argv[0] (aka command name), > > > > * and all remaining args > > > > */ > > > > +#ifdef CONFIG_CPU_V7M > > > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > > > + addr++; > > > > +#endif > > > > > > what if we were in ARM state when we reached this point ? You're now > > > telling CPU to always switch to Thumb. Is this really what we want ? > > > > We have no ARM state on this core so that's not possible. > > > > > From ARM's instruction manual: > > > > > > > > > > > > " > > > The BX and BLX instructions can change the processor state from ARM to > > > Thumb, or from Thumb to ARM. > > > > > > BLX label always changes the state. > > > > > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > > > > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > > > state > > > > > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > > > state. > > > > Correct. The last statement is why this patch exists. There is no ARM > > mode on M3, we immediately fault. Having bit[0]=0 for BX/BLX is not > > permitted on V7M...something not covered in the generic description > > of these instructions. > > it just seems weird that bit0 wouldn't just be assume 1 by the core > itself. I suppose as a consequence we can't use blx label with v7m > either ? :-s At issue is that the code really reads like this (expanding the functions a little bit: printf ("## Starting application at 0x%08lX ...\n", addr); rc = (void *)addr(argc - 1, argv + 1); So the compiler translates this as "do what I say" and generates a branch to whatever addr is. If people really feel strongly about it being too wierd to do "addr |= 1" in the common code, do_go_exec is a weak function and we can put something into arch/arm/lib/ (new file) that provides a do_go_exec for Cortex-M and that ensures that we have the right bit set for the branch instruction that will be generated.
Hi, On Tue, Apr 21, 2015 at 03:07:48PM -0400, Tom Rini wrote: > On Tue, Apr 21, 2015 at 01:05:24PM -0500, Felipe Balbi wrote: > > On Tue, Apr 21, 2015 at 02:01:31PM -0400, Matt Porter wrote: > > > On Tue, Apr 21, 2015 at 12:47:24PM -0500, Felipe Balbi wrote: > > > > On Tue, Apr 21, 2015 at 01:36:54PM -0400, Matt Porter wrote: > > > > > On ARM v7M, the processor will return to ARM mode when executing > > > > > a blx instruction with bit 0 of the address == 0. Always set it > > > > > > > > but that's what the 'x' is for, right ? eXchange the CPU mode. > > > > > > Yes. > > > > > > > > to 1 to stay in thumb mode. > > > > > > > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > > > > --- > > > > > common/cmd_boot.c | 4 ++++ > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > diff --git a/common/cmd_boot.c b/common/cmd_boot.c > > > > > index 8f2e070..20ce652 100644 > > > > > --- a/common/cmd_boot.c > > > > > +++ b/common/cmd_boot.c > > > > > @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > > > * pass address parameter as argv[0] (aka command name), > > > > > * and all remaining args > > > > > */ > > > > > +#ifdef CONFIG_CPU_V7M > > > > > + /* For ARM V7M, set bit zero to stay in Thumb mode */ > > > > > + addr++; > > > > > +#endif > > > > > > > > what if we were in ARM state when we reached this point ? You're now > > > > telling CPU to always switch to Thumb. Is this really what we want ? > > > > > > We have no ARM state on this core so that's not possible. > > > > > > > From ARM's instruction manual: > > > > > > > > > > > > > > > > " > > > > The BX and BLX instructions can change the processor state from ARM to > > > > Thumb, or from Thumb to ARM. > > > > > > > > BLX label always changes the state. > > > > > > > > BX Rm and BLX Rm derive the target state from bit[0] of Rm: > > > > > > > > if bit[0] of Rm is 0, the processor changes to, or remains in, ARM > > > > state > > > > > > > > if bit[0] of Rm is 1, the processor changes to, or remains in, Thumb > > > > state. > > > > > > Correct. The last statement is why this patch exists. There is no ARM > > > mode on M3, we immediately fault. Having bit[0]=0 for BX/BLX is not > > > permitted on V7M...something not covered in the generic description > > > of these instructions. > > > > it just seems weird that bit0 wouldn't just be assume 1 by the core > > itself. I suppose as a consequence we can't use blx label with v7m > > either ? :-s > > At issue is that the code really reads like this (expanding the > functions a little bit: > printf ("## Starting application at 0x%08lX ...\n", addr); > rc = (void *)addr(argc - 1, argv + 1); > > So the compiler translates this as "do what I say" and generates a > branch to whatever addr is. yeah, you're right. There's nothing the compiler can do. Well, it could refuse from using bx/blx instructions, would a 'bl' do the same thing in this particular case ? (yes, off-topic, sorry) > If people really feel strongly about it being too wierd to do "addr |= > 1" in the common code, do_go_exec is a weak function and we can put > something into arch/arm/lib/ (new file) that provides a do_go_exec for > Cortex-M and that ensures that we have the right bit set for the branch > instruction that will be generated. I think that's probably a lot clearer, yes.
diff --git a/common/cmd_boot.c b/common/cmd_boot.c index 8f2e070..20ce652 100644 --- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -38,6 +38,10 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) * pass address parameter as argv[0] (aka command name), * and all remaining args */ +#ifdef CONFIG_CPU_V7M + /* For ARM V7M, set bit zero to stay in Thumb mode */ + addr++; +#endif rc = do_go_exec ((void *)addr, argc - 1, argv + 1); if (rc != 0) rcode = 1;
On ARM v7M, the processor will return to ARM mode when executing a blx instruction with bit 0 of the address == 0. Always set it to 1 to stay in thumb mode. Signed-off-by: Matt Porter <mporter@konsulko.com> --- common/cmd_boot.c | 4 ++++ 1 file changed, 4 insertions(+)