updates for be8 patch series

PATCH New Ben Dooks 4 participants 18 messages Jul 22, 2013 – Jul 26, 2013 Arm (32-bit)

Share & tools

Ben Dooks

author

2013-07-22 16:33 UTC #0 lore raw

I've updated the BE8 patch series after running further tests on the kernel. I found an issue with the alignment handler (and I believe the same issue with the trap handler).

I also ran tests with the module loader, which shows that if we are in BE8 that it needs to fix up the relocations. Also the modules need to be built --be8 otherwise they cannot be easily relocated when loaded.

Should patches 3 and 4 be together (ie, alter the LDFLAGS and the module.c) and have I got the correct LDLAGS?

Comments and testing welcome. I will push out a new series later with these in and possibly rebased on 3.11-rc2.

Ben Dooks

author

2013-07-22 16:33 UTC #1 lore raw

PATCH 1/4 ARM: alignment: correctly decode instructions in BE8 mode.

If we are in BE8 mode, we must deal with the instruction stream being in LE order when data is being loaded in BE order. Ensure the data is swapped before processing to avoid thre following:

Change to using <asm/opcodes.h> to provide the necessary conversion functions to change the byte ordering.

Alignment trap: not handling instruction 030091e8 at [<80333e8c>] Unhandled fault: alignment exception (0x001) at 0xbfa09567

Signed-off-by Ben Dooks <[email protected]>

---
 arch/arm/mm/alignment.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

arch/arm/mm/alignment.c +6 -2

index db26e2e..295d615 100644
@@ -25,6 +25,7 @@
 #include <asm/cp15.h>
 #include <asm/system_info.h>
 #include <asm/unaligned.h>
+#include <asm/opcodes.h>
 
 #include "fault.h"

@@ -762,21 +763,24 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (thumb_mode(regs)) {
 		u16 *ptr = (u16 *)(instrptr & ~1);
 		fault = probe_kernel_address(ptr, tinstr);
+		tinstr = __mem_to_opcode_thumb16(tinstr);
 		if (!fault) {
 			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
 			    IS_T32(tinstr)) {
 				/* Thumb-2 32-bit */
 				u16 tinst2 = 0;
 				fault = probe_kernel_address(ptr + 1, tinst2);
+				tinst2 = __mem_to_opcode_thumb16(tinst2);
 				instr = (tinstr << 16) | tinst2;
 				thumb2_32b = 1;
 			} else {
 				isize = 2;
-				instr = thumb2arm(tinstr);
 			}
 		}
-	} else
+	} else {
 		fault = probe_kernel_address(instrptr, instr);
+		instr = __mem_to_opcode_arm(instr);
+	}
 
 	if (fault) {
 		type = TYPE_FAULT;

Signature · 1 line

1.7.10.4

Dave Martin

2013-07-24 17:16 UTC #2 lore raw

PATCH 1/4 ARM: alignment: correctly decode instructions in BE8 mode.

Quoted 28 lines · Ben Dooks

If we are in BE8 mode, we must deal with the instruction stream being in LE order when data is being loaded in BE order. Ensure the data is swapped before processing to avoid thre following:
Change to using <asm/opcodes.h> to provide the necessary conversion functions to change the byte ordering.
Alignment trap: not handling instruction 030091e8 at [<80333e8c>] Unhandled fault: alignment exception (0x001) at 0xbfa09567
Signed-off-by Ben Dooks <[email protected]>
---
 arch/arm/mm/alignment.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
arch/arm/mm/alignment.c +1 -0
index 6f4585b..f38145a 100644
@@ -25,6 +25,7 @@
 #include <asm/cp15.h>
 #include <asm/system_info.h>
 #include <asm/unaligned.h>
+#include <asm/opcodes.h>
 
 #include "fault.h"
 

arch/arm/mm/alignment.c do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs) · line 769

Quoted 7 lines

arch/arm/mm/alignment.c +1 -0

@@ -762,21 +763,24 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (thumb_mode(regs)) {
 		u16 *ptr = (u16 *)(instrptr & ~1);
 		fault = probe_kernel_address(ptr, tinstr);
+		tinstr = __mem_to_opcode_thumb16(tinstr);
 		if (!fault) {
 			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
 			    IS_T32(tinstr)) {

If patching this file anyway, we could take the opportunity to retire the local IS_T32() macro and use !__opcode_is_thumb16() instead.

arch/arm/mm/alignment.c do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs) · line 774

Quoted 5 lines

arch/arm/mm/alignment.c +1 -0

 				/* Thumb-2 32-bit */
 				u16 tinst2 = 0;
 				fault = probe_kernel_address(ptr + 1, tinst2);
+				tinst2 = __mem_to_opcode_thumb16(tinst2);
 				instr = (tinstr << 16) | tinst2;

Similarly, this could be __opcode_thumb32_compose(tinstr, tinst2).

arch/arm/mm/alignment.c do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs) · line 775

Quoted 4 lines

arch/arm/mm/alignment.c +0 -1

 				thumb2_32b = 1;
 			} else {
 				isize = 2;
-				instr = thumb2arm(tinstr);

eh? Is this is mis-edit?

Cheers
---Dave

Quoted 18 lines

arch/arm/mm/alignment.c +3 -1

 			}
 		}
-	} else
+	} else {
 		fault = probe_kernel_address(instrptr, instr);
+		instr = __mem_to_opcode_arm(instr);
+	}
 
 	if (fault) {
 		type = TYPE_FAULT;

Signature · 7 lines

1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ben Dooks

author

2013-07-25 11:48 UTC #3 lore raw

PATCH 1/4 ARM: alignment: correctly decode instructions in BE8 mode.

Quoted 1 line · Dave Martin

instr = thumb2arm(tinstr);

oops, removed it by accident. thanks for spotting.

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Ben Dooks

author

2013-07-22 16:33 UTC #4 lore raw

PATCH 2/4 ARM: traps: use <asm/opcodes.h> to get correct instruction order

In reply to Ben Dooks

The trap handler needs to take into account the endian configuration of the system when loading instructions. Use <asm/opcodes.h> to provide the necessary conversion functions.

Signed-off-by Ben Dooks <[email protected]>

---
 arch/arm/kernel/traps.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

arch/arm/kernel/traps.c +6 -4

index 1c08911..0a56e13 100644
@@ -34,6 +34,7 @@
 #include <asm/unwind.h>
 #include <asm/tls.h>
 #include <asm/system_misc.h>
+#include <asm/opcodes.h>
 
 #include "signal.h"

@@ -411,23 +412,24 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
 	if (processor_mode(regs) == SVC_MODE) {
 #ifdef CONFIG_THUMB2_KERNEL
 		if (thumb_mode(regs)) {
-			instr = ((u16 *)pc)[0];
+			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
 			if (is_wide_instruction(instr)) {
 				instr <<= 16;
-				instr |= ((u16 *)pc)[1];
+				instr |= __mem_to_opcode_thumb16(((u16 *)pc)[1]);
 			}
 		} else
 #endif
-			instr = *(u32 *) pc;
+			instr = __mem_to_opcode_arm(*(u32 *) pc);
 	} else if (thumb_mode(regs)) {
 		if (get_user(instr, (u16 __user *)pc))
 			goto die_sig;
+		instr = __mem_to_opcode_thumb16(instr);
 		if (is_wide_instruction(instr)) {
 			unsigned int instr2;
 			if (get_user(instr2, (u16 __user *)pc+1))
 				goto die_sig;
 			instr <<= 16;
-			instr |= instr2;
+			instr |= __mem_to_opcode_thumb16(instr2);
 		}
 	} else if (get_user(instr, (u32 __user *)pc)) {
 		goto die_sig;

Signature · 1 line

1.7.10.4

Dave Martin

2013-07-24 17:34 UTC #5 lore raw

PATCH 2/4 ARM: traps: use <asm/opcodes.h> to get correct instruction order

Quoted 14 lines · Ben Dooks

The trap handler needs to take into account the endian configuration of the system when loading instructions. Use <asm/opcodes.h> to provide the necessary conversion functions.
Signed-off-by Ben Dooks <[email protected]>
---
 arch/arm/kernel/traps.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
arch/arm/kernel/traps.c +0 -0
index 1c08911..0a56e13 100644

arch/arm/kernel/traps.c line 39

Quoted 6 lines

arch/arm/kernel/traps.c +1 -0

@@ -34,6 +34,7 @@
 #include <asm/unwind.h>
 #include <asm/tls.h>
 #include <asm/system_misc.h>
+#include <asm/opcodes.h>
 
 #include "signal.h"

Looks like is_valid_bugaddr() might need fixing too?

That will need __mem_to_opcode_thumb16() or __mem_to_opcode_arm() depending on CONFIG_THUMB2_KERNEL.

arch/arm/kernel/traps.c asmlinkage void __exception do_undefinstr(struct pt_regs *regs) · line 418

Quoted 11 lines

arch/arm/kernel/traps.c +2 -2

@@ -411,23 +412,24 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
 	if (processor_mode(regs) == SVC_MODE) {
 #ifdef CONFIG_THUMB2_KERNEL
 		if (thumb_mode(regs)) {
-			instr = ((u16 *)pc)[0];
+			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
 			if (is_wide_instruction(instr)) {
 				instr <<= 16;
-				instr |= ((u16 *)pc)[1];
+				instr |= __mem_to_opcode_thumb16(((u16 *)pc)[1]);

Could use __opcode_thumb32_compose(), in which case it might look like

			if (is_wide_instruction(instr)) {
				unsigned int instr2 = __mem_to_opcode_thumb16(
					((u16 *)pc)[1]);
				instr = __opcode_thumb32_compose(instr, instr2);

(Hmmm, I just noticed that is_wide_instruction and __opcode_is_thumb16() kinda duplicate each other. Could change that too, but it would look more like pointless churn... is_wide_instruction() is used in a fair few places already.)

Quoted 8 lines

arch/arm/kernel/traps.c +1 -1

 			}
 		} else
 #endif
-			instr = *(u32 *) pc;
+			instr = __mem_to_opcode_arm(*(u32 *) pc);
 	} else if (thumb_mode(regs)) {
 		if (get_user(instr, (u16 __user *)pc))
 			goto die_sig;

arch/arm/kernel/traps.c asmlinkage void __exception do_undefinstr(struct pt_regs *regs) · line 434

Quoted 10 lines

arch/arm/kernel/traps.c +2 -1

+		instr = __mem_to_opcode_thumb16(instr);
 		if (is_wide_instruction(instr)) {
 			unsigned int instr2;
 			if (get_user(instr2, (u16 __user *)pc+1))
 				goto die_sig;
 			instr <<= 16;
-			instr |= instr2;
+			instr |= __mem_to_opcode_thumb16(instr2);
 		}
 	} else if (get_user(instr, (u32 __user *)pc)) {

This also needs mem-to-opcode'ing.

Cheers
---Dave

Ben Dooks

author

2013-07-25 11:17 UTC #6 lore raw

PATCH 2/4 ARM: traps: use <asm/opcodes.h> to get correct instruction order

Quoted 20 lines · Dave Martin

Quoted 19 lines · Ben Dooks
The trap handler needs to take into account the endian configuration of the system when loading instructions. Use<asm/opcodes.h> to provide the necessary conversion functions.
Signed-off-by Ben Dooks<[email protected]>
---
  arch/arm/kernel/traps.c |   10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)
arch/arm/kernel/traps.c +1 -0
index 1c08911..0a56e13 100644
@@ -34,6 +34,7 @@
  #include<asm/unwind.h>
  #include<asm/tls.h>
  #include<asm/system_misc.h>
+#include<asm/opcodes.h>
 

Quoted 6 lines

arch/arm/kernel/traps.c line 39
Quoted 1 line
arch/arm/kernel/traps.c +0 -0
  #include "signal.h"
Looks like is_valid_bugaddr() might need fixing too?
That will need __mem_to_opcode_thumb16() or __mem_to_opcode_arm() depending on CONFIG_THUMB2_KERNEL.

I was looking at that, and was considering if this is the right thing to do or to change the value of BUG_INSTR_VALUE depending on this.

It is possible the linker may try and change the ordering of the BUG() instruction as it is being placed in the output with either a .hword or .word value. I will go and check this as soon as possible.

Quoted 18 lines

arch/arm/kernel/traps.c asmlinkage void __exception do_undefinstr(struct pt_regs *regs) · line 418

Quoted 10 lines

arch/arm/kernel/traps.c +2 -2

@@ -411,23 +412,24 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
  	if (processor_mode(regs) == SVC_MODE) {
  #ifdef CONFIG_THUMB2_KERNEL
  		if (thumb_mode(regs)) {
-			instr = ((u16 *)pc)[0];
+			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
  			if (is_wide_instruction(instr)) {
  				instr<<= 16;
-				instr |= ((u16 *)pc)[1];
+				instr |= __mem_to_opcode_thumb16(((u16 *)pc)[1]);

Could use __opcode_thumb32_compose(), in which case it might look like

			if (is_wide_instruction(instr)) {
				unsigned int instr2 = __mem_to_opcode_thumb16(
					((u16 *)pc)[1]);
				instr = __opcode_thumb32_compose(instr, instr2);

Quoted 4 lines

(Hmmm, I just noticed that is_wide_instruction and __opcode_is_thumb16() kinda duplicate each other. Could change that too, but it would look more like pointless churn... is_wide_instruction() is used in a fair few places already.)

It is possible that it could be changed, but I will avoid that for this particular patch

Quoted 12 lines

arch/arm/kernel/traps.c +2 -1

  			}
  		} else
  #endif
-			instr = *(u32 *) pc;
+			instr = __mem_to_opcode_arm(*(u32 *) pc);
  	} else if (thumb_mode(regs)) {
  		if (get_user(instr, (u16 __user *)pc))
  			goto die_sig;
+		instr = __mem_to_opcode_thumb16(instr);
  		if (is_wide_instruction(instr)) {
  			unsigned int instr2;
  			if (get_user(instr2, (u16 __user *)pc+1))

Quoted 8 lines

arch/arm/kernel/traps.c asmlinkage void __exception do_undefinstr(struct pt_regs *regs) · line 434
Quoted 6 lines
arch/arm/kernel/traps.c +1 -1
  				goto die_sig;
  			instr<<= 16;
-			instr |= instr2;
+			instr |= __mem_to_opcode_thumb16(instr2);
  		}
  	} else if (get_user(instr, (u32 __user *)pc)) {
This also needs mem-to-opcode'ing.

Thanks, will check.

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Dave Martin

2013-07-26 16:04 UTC #7 lore raw

PATCH 2/4 ARM: traps: use <asm/opcodes.h> to get correct instruction order

Quoted 28 lines · Ben Dooks

Quoted 20 lines · Dave Martin
Quoted 19 lines · Ben Dooks
The trap handler needs to take into account the endian configuration of the system when loading instructions. Use<asm/opcodes.h> to provide the necessary conversion functions.
Signed-off-by Ben Dooks<[email protected]>
---
 arch/arm/kernel/traps.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
arch/arm/kernel/traps.c +1 -0
index 1c08911..0a56e13 100644
@@ -34,6 +34,7 @@
 #include<asm/unwind.h>
 #include<asm/tls.h>
 #include<asm/system_misc.h>
+#include<asm/opcodes.h>
 
Quoted 6 lines
arch/arm/kernel/traps.c line 39
Quoted 1 line
arch/arm/kernel/traps.c +0 -0
 #include "signal.h"
Looks like is_valid_bugaddr() might need fixing too?
That will need __mem_to_opcode_thumb16() or __mem_to_opcode_arm() depending on CONFIG_THUMB2_KERNEL.

Quoted 7 lines

I was looking at that, and was considering if this is the right thing to do or to change the value of BUG_INSTR_VALUE depending on this.
It is possible the linker may try and change the ordering of the BUG() instruction as it is being placed in the output with either a .hword or .word value. I will go and check this as soon as possible.

I guess the way would be to leave BUG_INSTR_VALUE as-is, and inject it into the code using the appropriate __inst (similarly to the KGDB case).

Quoted 19 lines

arch/arm/kernel/traps.c asmlinkage void __exception do_undefinstr(struct pt_regs *regs) · line 418

Quoted 11 lines

arch/arm/kernel/traps.c +2 -2

@@ -411,23 +412,24 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
 	if (processor_mode(regs) == SVC_MODE) {
 #ifdef CONFIG_THUMB2_KERNEL
 		if (thumb_mode(regs)) {
-			instr = ((u16 *)pc)[0];
+			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
 			if (is_wide_instruction(instr)) {
 				instr<<= 16;
-				instr |= ((u16 *)pc)[1];
+				instr |= __mem_to_opcode_thumb16(((u16 *)pc)[1]);

Could use __opcode_thumb32_compose(), in which case it might look like

			if (is_wide_instruction(instr)) {
				unsigned int instr2 = __mem_to_opcode_thumb16(
					((u16 *)pc)[1]);
				instr = __opcode_thumb32_compose(instr, instr2);

Quoted 7 lines

Quoted 4 lines
(Hmmm, I just noticed that is_wide_instruction and __opcode_is_thumb16() kinda duplicate each other. Could change that too, but it would look more like pointless churn... is_wide_instruction() is used in a fair few places already.)
It is possible that it could be changed, but I will avoid that for this particular patch

No problem, that was just cosmetic.

Quoted 22 lines

Quoted 13 lines

arch/arm/kernel/traps.c +2 -1

 			}
 		} else
 #endif
-			instr = *(u32 *) pc;
+			instr = __mem_to_opcode_arm(*(u32 *) pc);
 	} else if (thumb_mode(regs)) {
 		if (get_user(instr, (u16 __user *)pc))
 			goto die_sig;
+		instr = __mem_to_opcode_thumb16(instr);
 		if (is_wide_instruction(instr)) {
 			unsigned int instr2;
 			if (get_user(instr2, (u16 __user *)pc+1))

Quoted 8 lines

arch/arm/kernel/traps.c asmlinkage void __exception do_undefinstr(struct pt_regs *regs) · line 434
Quoted 6 lines
arch/arm/kernel/traps.c +1 -1
 				goto die_sig;
 			instr<<= 16;
-			instr |= instr2;
+			instr |= __mem_to_opcode_thumb16(instr2);
 		}
 	} else if (get_user(instr, (u32 __user *)pc)) {
This also needs mem-to-opcode'ing.

Quoted 1 line

Thanks, will check.

OK.

Cheers
---Dave

Ben Dooks

author

2013-07-22 16:33 UTC #8 lore raw

PATCH 3/4 ARM: module: correctly relocate instructions in BE8

In reply to Ben Dooks

When in BE8 mode, our instructions are not in the same ordering as the data, so use <asm/opcodes.h> to take this into account.

Note, also requires modules to be built --be8

Signed-off-by Ben Dooks <[email protected]>

---
 arch/arm/kernel/module.c |   57 +++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

arch/arm/kernel/module.c +34 ~~-23~~

index 1e9be5d..7e13787 100644
@@ -24,6 +24,7 @@
 #include <asm/sections.h>
 #include <asm/smp_plat.h>
 #include <asm/unwind.h>
+#include <asm/opcodes.h>
 
 #ifdef CONFIG_XIP_KERNEL
 /*

@@ -60,6 +61,7 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		Elf32_Sym *sym;
 		const char *symname;
 		s32 offset;
+		u32 tmp;
 #ifdef CONFIG_THUMB2_KERNEL
 		u32 upper, lower, sign, j1, j2;
 #endif

@@ -95,7 +97,8 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		case R_ARM_PC24:
 		case R_ARM_CALL:
 		case R_ARM_JUMP24:
-			offset = (*(u32 *)loc & 0x00ffffff) << 2;
+			offset = __mem_to_opcode_arm(*(u32 *)loc);
+			offset = (offset & 0x00ffffff) << 2;
 			if (offset & 0x02000000)
 				offset -= 0x04000000;

@@ -111,9 +114,10 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			}
 
 			offset >>= 2;
+			offset &= 0x00ffffff;
 
-			*(u32 *)loc &= 0xff000000;
-			*(u32 *)loc |= offset & 0x00ffffff;
+			*(u32 *)loc &= __opcode_to_mem_arm(0xff000000);
+			*(u32 *)loc |= __opcode_to_mem_arm(offset);
 			break;
 
 	       case R_ARM_V4BX:

@@ -121,8 +125,8 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			* other bits to re-code instruction as
 			* MOV PC,Rm.
 			*/
-		       *(u32 *)loc &= 0xf000000f;
-		       *(u32 *)loc |= 0x01a0f000;
+		       *(u32 *)loc &= __opcode_to_mem_arm(0xf000000f);
+		       *(u32 *)loc |= __opcode_to_mem_arm(0x01a0f000);
 		       break;
 
 		case R_ARM_PREL31:

@@ -132,7 +136,7 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 
 		case R_ARM_MOVW_ABS_NC:
 		case R_ARM_MOVT_ABS:
-			offset = *(u32 *)loc;
+			offset = tmp = __mem_to_opcode_arm(*(u32 *)loc);
 			offset = ((offset & 0xf0000) >> 4) | (offset & 0xfff);
 			offset = (offset ^ 0x8000) - 0x8000;

@@ -140,16 +144,18 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			if (ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_ABS)
 				offset >>= 16;
 
-			*(u32 *)loc &= 0xfff0f000;
-			*(u32 *)loc |= ((offset & 0xf000) << 4) |
-					(offset & 0x0fff);
+			tmp &= 0xfff0f000;
+			tmp |= ((offset & 0xf000) << 4) |
+				(offset & 0x0fff);
+
+			*(u32 *)loc = __opcode_to_mem_arm(tmp);
 			break;
 
 #ifdef CONFIG_THUMB2_KERNEL
 		case R_ARM_THM_CALL:
 		case R_ARM_THM_JUMP24:
-			upper = *(u16 *)loc;
-			lower = *(u16 *)(loc + 2);
+			upper = __mem_to_opcode_thumb16(*(u16 *)loc);
+			lower = __mem_to_opcode_thumb16(*(u16 *)(loc + 2));
 
 			/*
 			 * 25 bit signed address range (Thumb-2 BL and B.W

@@ -198,17 +204,20 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			sign = (offset >> 24) & 1;
 			j1 = sign ^ (~(offset >> 23) & 1);
 			j2 = sign ^ (~(offset >> 22) & 1);
-			*(u16 *)loc = (u16)((upper & 0xf800) | (sign << 10) |
+			upper = (u16)((upper & 0xf800) | (sign << 10) |
 					    ((offset >> 12) & 0x03ff));
-			*(u16 *)(loc + 2) = (u16)((lower & 0xd000) |
-						  (j1 << 13) | (j2 << 11) |
-						  ((offset >> 1) & 0x07ff));
+			lower = (u16)((lower & 0xd000) |
+				      (j1 << 13) | (j2 << 11) |
+				      ((offset >> 1) & 0x07ff));
+
+			*(u16 *)loc = __opcode_to_mem_thumb16(upper);
+			*(u16 *)(loc + 2) = __opcode_to_mem_thumb16(lower);
 			break;
 
 		case R_ARM_THM_MOVW_ABS_NC:
 		case R_ARM_THM_MOVT_ABS:
-			upper = *(u16 *)loc;
-			lower = *(u16 *)(loc + 2);
+			upper = __mem_to_opcode_thumb16(*(u16 *)loc);
+			lower = __mem_to_opcode_thumb16(*(u16 *)(loc + 2));
 
 			/*
 			 * MOVT/MOVW instructions encoding in Thumb-2:

@@ -229,12 +238,14 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			if (ELF32_R_TYPE(rel->r_info) == R_ARM_THM_MOVT_ABS)
 				offset >>= 16;
 
-			*(u16 *)loc = (u16)((upper & 0xfbf0) |
-					    ((offset & 0xf000) >> 12) |
-					    ((offset & 0x0800) >> 1));
-			*(u16 *)(loc + 2) = (u16)((lower & 0x8f00) |
-						  ((offset & 0x0700) << 4) |
-						  (offset & 0x00ff));
+			upper = (u16)((upper & 0xfbf0) |
+				      ((offset & 0xf000) >> 12) |
+				      ((offset & 0x0800) >> 1));
+			lower = (u16)((lower & 0x8f00) |
+				      ((offset & 0x0700) << 4) |
+				      (offset & 0x00ff));
+			*(u16 *)loc = __opcode_to_mem_thumb16(upper);
+			*(u16 *)(loc + 2) = __opcode_to_mem_thumb16(lower);
 			break;
 #endif

Signature · 1 line

1.7.10.4

Dave Martin

2013-07-24 17:52 UTC #9 lore raw

PATCH 3/4 ARM: module: correctly relocate instructions in BE8

Quoted 41 lines · Ben Dooks

When in BE8 mode, our instructions are not in the same ordering as the data, so use <asm/opcodes.h> to take this into account.

Note, also requires modules to be built --be8

Signed-off-by Ben Dooks <[email protected]>

---
 arch/arm/kernel/module.c |   57 +++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

arch/arm/kernel/module.c +4 -1

index 1e9be5d..7e13787 100644
@@ -24,6 +24,7 @@
 #include <asm/sections.h>
 #include <asm/smp_plat.h>
 #include <asm/unwind.h>
+#include <asm/opcodes.h>
 
 #ifdef CONFIG_XIP_KERNEL
 /*

@@ -60,6 +61,7 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		Elf32_Sym *sym;
 		const char *symname;
 		s32 offset;
+		u32 tmp;
 #ifdef CONFIG_THUMB2_KERNEL
 		u32 upper, lower, sign, j1, j2;
 #endif

@@ -95,7 +97,8 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		case R_ARM_PC24:
 		case R_ARM_CALL:
 		case R_ARM_JUMP24:
-			offset = (*(u32 *)loc & 0x00ffffff) << 2;
+			offset = __mem_to_opcode_arm(*(u32 *)loc);
+			offset = (offset & 0x00ffffff) << 2;
 			if (offset & 0x02000000)
 				offset -= 0x04000000;

arch/arm/kernel/module.c apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex, · line 120

Quoted 9 lines

arch/arm/kernel/module.c +3 -2

@@ -111,9 +114,10 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			}
 
 			offset >>= 2;
+			offset &= 0x00ffffff;
 
-			*(u32 *)loc &= 0xff000000;
-			*(u32 *)loc |= offset & 0x00ffffff;
+			*(u32 *)loc &= __opcode_to_mem_arm(0xff000000);
+			*(u32 *)loc |= __opcode_to_mem_arm(offset);

Here it's assumed that

__opcode_to_mem_arm(x | y) == __opcode_to_mem_arm(x) | __opcode_to_mem_arm(x)

(and related rules)

Since the macros really do just shuffle bits, that does work, but it may be a bit cleaner to build on the instruction in canonical representation and use __opcode_to_mem_arm() just once to avoid such subtleties.

Then we can simply treat instructions in memory as opaque data, which all flows into the code through __mem_to_opcode*() and back out through __opcode_to_mem*().

(analogous to ntohs() ... do something useful ... htons() etc.)

So:

			tmp = __mem_to_opcode_arm(*(u32 *)loc);
			tmp = (tmp & 0xff000000) | offset;
			*(u32 *)loc = __opcode_to_mem_arm(tmp);

I think that's cleaner, but it's open to debate. GCC might generate slightly inferior code, depending on how clever it is ... but this isn't a fast path.

The same style could be applied to all the relocations here. It could help to avoid confusion about which byte order a particular vairable or literal uses -- i.e., ARM ARM order is used for everything except the contents of memory.

I have to abort this review now ... I'll try to look at the rest tomorrow.

Cheers
---Dave

Quoted 103 lines

arch/arm/kernel/module.c +27 ~~-20~~

 			break;
 
 	       case R_ARM_V4BX:

@@ -121,8 +125,8 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			* other bits to re-code instruction as
 			* MOV PC,Rm.
 			*/
-		       *(u32 *)loc &= 0xf000000f;
-		       *(u32 *)loc |= 0x01a0f000;
+		       *(u32 *)loc &= __opcode_to_mem_arm(0xf000000f);
+		       *(u32 *)loc |= __opcode_to_mem_arm(0x01a0f000);
 		       break;
 
 		case R_ARM_PREL31:

@@ -132,7 +136,7 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 
 		case R_ARM_MOVW_ABS_NC:
 		case R_ARM_MOVT_ABS:
-			offset = *(u32 *)loc;
+			offset = tmp = __mem_to_opcode_arm(*(u32 *)loc);
 			offset = ((offset & 0xf0000) >> 4) | (offset & 0xfff);
 			offset = (offset ^ 0x8000) - 0x8000;

@@ -140,16 +144,18 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			if (ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_ABS)
 				offset >>= 16;
 
-			*(u32 *)loc &= 0xfff0f000;
-			*(u32 *)loc |= ((offset & 0xf000) << 4) |
-					(offset & 0x0fff);
+			tmp &= 0xfff0f000;
+			tmp |= ((offset & 0xf000) << 4) |
+				(offset & 0x0fff);
+
+			*(u32 *)loc = __opcode_to_mem_arm(tmp);
 			break;
 
 #ifdef CONFIG_THUMB2_KERNEL
 		case R_ARM_THM_CALL:
 		case R_ARM_THM_JUMP24:
-			upper = *(u16 *)loc;
-			lower = *(u16 *)(loc + 2);
+			upper = __mem_to_opcode_thumb16(*(u16 *)loc);
+			lower = __mem_to_opcode_thumb16(*(u16 *)(loc + 2));
 
 			/*
 			 * 25 bit signed address range (Thumb-2 BL and B.W

@@ -198,17 +204,20 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			sign = (offset >> 24) & 1;
 			j1 = sign ^ (~(offset >> 23) & 1);
 			j2 = sign ^ (~(offset >> 22) & 1);
-			*(u16 *)loc = (u16)((upper & 0xf800) | (sign << 10) |
+			upper = (u16)((upper & 0xf800) | (sign << 10) |
 					    ((offset >> 12) & 0x03ff));
-			*(u16 *)(loc + 2) = (u16)((lower & 0xd000) |
-						  (j1 << 13) | (j2 << 11) |
-						  ((offset >> 1) & 0x07ff));
+			lower = (u16)((lower & 0xd000) |
+				      (j1 << 13) | (j2 << 11) |
+				      ((offset >> 1) & 0x07ff));
+
+			*(u16 *)loc = __opcode_to_mem_thumb16(upper);
+			*(u16 *)(loc + 2) = __opcode_to_mem_thumb16(lower);
 			break;
 
 		case R_ARM_THM_MOVW_ABS_NC:
 		case R_ARM_THM_MOVT_ABS:
-			upper = *(u16 *)loc;
-			lower = *(u16 *)(loc + 2);
+			upper = __mem_to_opcode_thumb16(*(u16 *)loc);
+			lower = __mem_to_opcode_thumb16(*(u16 *)(loc + 2));
 
 			/*
 			 * MOVT/MOVW instructions encoding in Thumb-2:

@@ -229,12 +238,14 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			if (ELF32_R_TYPE(rel->r_info) == R_ARM_THM_MOVT_ABS)
 				offset >>= 16;
 
-			*(u16 *)loc = (u16)((upper & 0xfbf0) |
-					    ((offset & 0xf000) >> 12) |
-					    ((offset & 0x0800) >> 1));
-			*(u16 *)(loc + 2) = (u16)((lower & 0x8f00) |
-						  ((offset & 0x0700) << 4) |
-						  (offset & 0x00ff));
+			upper = (u16)((upper & 0xfbf0) |
+				      ((offset & 0xf000) >> 12) |
+				      ((offset & 0x0800) >> 1));
+			lower = (u16)((lower & 0x8f00) |
+				      ((offset & 0x0700) << 4) |
+				      (offset & 0x00ff));
+			*(u16 *)loc = __opcode_to_mem_thumb16(upper);
+			*(u16 *)(loc + 2) = __opcode_to_mem_thumb16(lower);
 			break;
 #endif

Signature · 7 lines

1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ben Dooks

author

2013-07-22 16:33 UTC #10 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

In reply to Ben Dooks

To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).

If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.

Signed-off-by Ben Dooks <[email protected]>

---
 arch/arm/Makefile |    1 +
 1 file changed, 1 insertion(+)

arch/arm/Makefile +1 -0

index ee4605f..8173883 100644
@@ -16,6 +16,7 @@ LDFLAGS		:=
 LDFLAGS_vmlinux	:=-p --no-undefined -X
 ifeq ($(CONFIG_CPU_ENDIAN_BE8),y)
 LDFLAGS_vmlinux	+= --be8
+LDFLAGS_MODULE	+= --be8
 endif
 
 OBJCOPYFLAGS	:=-O binary -R .comment -S

Signature · 1 line

1.7.10.4

Stephen Boyd

2013-07-22 17:05 UTC #11 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

Quoted 8 lines · Ben Dooks

To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).
If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.

If someone tries to load a be8 module on a non-be8 kernel will it still work? Or should we add an extra version magic string in asm/module.h to prevent that?

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Ben Dooks

author

2013-07-22 17:20 UTC #12 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

Quoted 5 lines · Stephen Boyd

Quoted 4 lines · Ben Dooks
To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).

Quoted 8 lines

Quoted 4 lines
If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.
If someone tries to load a be8 module on a non-be8 kernel will it still work? Or should we add an extra version magic string in asm/module.h to prevent that?

The ELF header changes the EI_DATA field in the ei_ident from ELFDATA2LSB to ELFDATA2MSB when compiling so we should be able to detect these when loading.

I have not checked to see if the kernel correctly checks for this.

I do not think it currently checks the ei_flags field for the EF_ARM_BE8 in ABI 4 and 5. I am not sure if this is really important?

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Nicolas Pitre

2013-07-22 18:53 UTC #13 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

Quoted 15 lines · Ben Dooks

Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Ben Dooks
To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).
Quoted 8 lines
Quoted 4 lines
If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.
If someone tries to load a be8 module on a non-be8 kernel will it still work? Or should we add an extra version magic string in asm/module.h to prevent that?

Quoted 8 lines

The ELF header changes the EI_DATA field in the ei_ident from ELFDATA2LSB to ELFDATA2MSB when compiling so we should be able to detect these when loading.
I have not checked to see if the kernel correctly checks for this.
I do not think it currently checks the ei_flags field for the EF_ARM_BE8 in ABI 4 and 5. I am not sure if this is really important?

If the information is already there and easily accessible, then it should be used.

Nicolas

Ben Dooks

author

2013-07-22 18:56 UTC #14 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

Quoted 23 lines · Nicolas Pitre

Quoted 21 lines · Ben Dooks
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Ben Dooks
To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).
Quoted 8 lines
Quoted 4 lines
If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.
If someone tries to load a be8 module on a non-be8 kernel will it still work? Or should we add an extra version magic string in asm/module.h to prevent that?
The ELF header changes the EI_DATA field in the ei_ident from ELFDATA2LSB to ELFDATA2MSB when compiling so we should be able to detect these when loading.
I have not checked to see if the kernel correctly checks for this.

Quoted 8 lines

Quoted 2 lines
I do not think it currently checks the ei_flags field for the EF_ARM_BE8 in ABI 4 and 5. I am not sure if this is really important?
If the information is already there and easily accessible, then it should be used.
Nicolas

I have something like this, it does not mess things up loading BE8 binaries on my current system. I think we're building stuff for EABI4 but haven't checked.

arch/arm/kernel/elf.c +0 -0

index d0d1e83..3b0351c 100644
@@ -34,6 +34,15 @@ int elf_check_arch(const struct elf32_hdr *x)
                 if (flt_fmt == EF_ARM_VFP_FLOAT && !(elf_hwcap &

HWCAP_VFP))

                         return 0;
         }

+
+ if ((eflags & EF_ARM_EABI_MASK) >= EF_ARM_EABI_VER4) {
+ if (eflags & EF_ARM_BE8) {
+ if (!IS_ENABLED(CONFIG_ARM_CPU_BE8))
+ return 1;
+ } else if (IS_ENABLED(CONFIG_ARM_CPU_BE8))
+ return 1;
+ }
+

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Ben Dooks

author

2013-07-23 08:16 UTC #15 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

Quoted 43 lines · Ben Dooks

Quoted 23 lines · Nicolas Pitre
Quoted 21 lines · Ben Dooks
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Ben Dooks
To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).
Quoted 8 lines
Quoted 4 lines
If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.
If someone tries to load a be8 module on a non-be8 kernel will it still work? Or should we add an extra version magic string in asm/module.h to prevent that?
The ELF header changes the EI_DATA field in the ei_ident from ELFDATA2LSB to ELFDATA2MSB when compiling so we should be able to detect these when loading.
I have not checked to see if the kernel correctly checks for this.
Quoted 8 lines
Quoted 2 lines
I do not think it currently checks the ei_flags field for the EF_ARM_BE8 in ABI 4 and 5. I am not sure if this is really important?
If the information is already there and easily accessible, then it should be used.
Nicolas
I have something like this, it does not mess things up loading BE8 binaries on my current system. I think we're building stuff for EABI4 but haven't checked.
arch/arm/kernel/elf.c +0 -0
index d0d1e83..3b0351c 100644
@@ -34,6 +34,15 @@ int elf_check_arch(const struct elf32_hdr *x)
 if (flt_fmt == EF_ARM_VFP_FLOAT && !(elf_hwcap & HWCAP_VFP))

arch/arm/kernel/elf.c int elf_check_arch(const struct elf32_hdr *x) · line 44

Quoted 10 lines

arch/arm/kernel/elf.c +8 -0

 return 0;
 }
+
+ if ((eflags & EF_ARM_EABI_MASK) >= EF_ARM_EABI_VER4) {
+ if (eflags & EF_ARM_BE8) {
+ if (!IS_ENABLED(CONFIG_ARM_CPU_BE8))
+ return 1;
+ } else if (IS_ENABLED(CONFIG_ARM_CPU_BE8))
+ return 1;
+ }

dis-regard this, it does not work. I blame the heat.

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Ben Dooks

author

2013-07-22 20:26 UTC #16 lore raw

PATCH 4/4 ARM: set --be8 when linking modules

In reply to Nicolas Pitre

Quoted 21 lines · Nicolas Pitre

Quoted 19 lines · Ben Dooks
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Ben Dooks
To avoid having to make every text section swap the instruction order of all instructions, make sure modules are built also built with --be8 (as is the current kernel final link).
Quoted 8 lines
Quoted 4 lines
If we do not do this, we would end up having to swap all instructions when loading a module, instead of just the instructions that we are applying ELF relocations to.
If someone tries to load a be8 module on a non-be8 kernel will it still work? Or should we add an extra version magic string in asm/module.h to prevent that?
The ELF header changes the EI_DATA field in the ei_ident from ELFDATA2LSB to ELFDATA2MSB when compiling so we should be able to detect these when loading.

Quoted 7 lines

Quoted 4 lines
I have not checked to see if the kernel correctly checks for this.
I do not think it currently checks the ei_flags field for the EF_ARM_BE8 in ABI 4 and 5. I am not sure if this is really important?
If the information is already there and easily accessible, then it should be used.

I added this, which seems to actually work unlike my last effort.

I do not think it actually gets triggered, the ELF format contains the endian-ness of the system it was built for and therefore fails somewhere else.

arch/arm/kernel/elf.c +0 -0

index d0d1e83..37c8e66 100644
@@ -34,6 +34,17 @@ int elf_check_arch(const struct elf32_hdr *x)
                 if (flt_fmt == EF_ARM_VFP_FLOAT && !(elf_hwcap &

HWCAP_VFP))

                         return 0;
         }

+
+ if ((eflags & EF_ARM_EABI_MASK) >= EF_ARM_EABI_VER4) {
+ bool is_be8 = IS_ENABLED(CONFIG_CPU_ENDIAN_BE8);
+
+ /* do some simple endian-ness verifications */
+ if (eflags & EF_ARM_BE8 && !is_be8)
+ return 0;
+ if (eflags & EF_ARM_LE8 && is_be8)
+ return 0;
+ }
+

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Ben Dooks

author

2013-07-22 16:49 UTC #17 lore raw

In reply to Ben Dooks

Quoted 8 lines · Ben Dooks

I've updated the BE8 patch series after running further tests on the kernel. I found an issue with the alignment handler (and I believe the same issue with the trap handler).
I also ran tests with the module loader, which shows that if we are in BE8 that it needs to fix up the relocations. Also the modules need to be built --be8 otherwise they cannot be easily relocated when loaded.

Quoted 5 lines

Should patches 3 and 4 be together (ie, alter the LDFLAGS and the module.c) and have I got the correct LDLAGS?
Comments and testing welcome. I will push out a new series later with these in and possibly rebased on 3.11-rc2.

New series based on 3.10 at:

	git://git.baserock.org/delta/linux.git
		baserock/311/be/core-v3
		baserock/311/be/atags-v3

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius