updates for be8 patch series

updates for be8 patch series

[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.

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.

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

[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 <ben.dooks@codethink.co.uk>
---
 arch/arm/mm/alignment.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index db26e2e..295d615 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -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;
-- 
1.7.10.4

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

[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 <ben.dooks@codethink.co.uk>
---
 arch/arm/kernel/traps.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 1c08911..0a56e13 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -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;
-- 
1.7.10.4

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

[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 <ben.dooks@codethink.co.uk>
---
 arch/arm/kernel/module.c |   57 +++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 1e9be5d..7e13787 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -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
 
-- 
1.7.10.4

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

[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 <ben.dooks@codethink.co.uk>
---
 arch/arm/Makefile |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index ee4605f..8173883 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -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
-- 
1.7.10.4

updates for be8 patch series

[Ben Dooks]

On 22/07/13 17:33, Ben Dooks wrote:
> 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.

New series based on 3.10 at:

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

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

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

[Stephen Boyd]

On 07/22, Ben Dooks wrote:
> 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?

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

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

[Ben Dooks]

On 22/07/13 18:05, Stephen Boyd wrote:
> On 07/22, Ben Dooks wrote:
>> 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?

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?

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

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

[Nicolas Pitre]

On Mon, 22 Jul 2013, Ben Dooks wrote:

> On 22/07/13 18:05, Stephen Boyd wrote:
> > On 07/22, Ben Dooks wrote:
> > > 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?
> 
> 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

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

[Ben Dooks]

On 22/07/13 19:53, Nicolas Pitre wrote:
> On Mon, 22 Jul 2013, Ben Dooks wrote:
>
>> On 22/07/13 18:05, Stephen Boyd wrote:
>>> On 07/22, Ben Dooks wrote:
>>>> 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?
>>
>> 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

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.

diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index d0d1e83..3b0351c 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -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;
+       }
+

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

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

[Ben Dooks]

On 22/07/13 19:53, Nicolas Pitre wrote:
> On Mon, 22 Jul 2013, Ben Dooks wrote:
>
>> On 22/07/13 18:05, Stephen Boyd wrote:
>>> On 07/22, Ben Dooks wrote:
>>>> 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?
>>
>> 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.

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.


diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index d0d1e83..37c8e66 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -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;
+       }
+

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

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

[Ben Dooks]

On 22/07/13 19:56, Ben Dooks wrote:
> On 22/07/13 19:53, Nicolas Pitre wrote:
>> On Mon, 22 Jul 2013, Ben Dooks wrote:
>>
>>> On 22/07/13 18:05, Stephen Boyd wrote:
>>>> On 07/22, Ben Dooks wrote:
>>>>> 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?
>>>
>>> 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
>
> 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.
>
> diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
> index d0d1e83..3b0351c 100644
> --- a/arch/arm/kernel/elf.c
> +++ b/arch/arm/kernel/elf.c
> @@ -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;
> + }

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

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

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

[Dave Martin]

On Wed, Jul 24, 2013 at 11:27:59AM +0100, Ben Dooks wrote:
> 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 <ben.dooks@codethink.co.uk>
> ---
>  arch/arm/mm/alignment.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 6f4585b..f38145a 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -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)) {

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

>  				/* 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).

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

eh?  Is this is mis-edit?

Cheers
---Dave

>  			}
>  		}
> -	} else
> +	} else {
>  		fault = probe_kernel_address(instrptr, instr);
> +		instr = __mem_to_opcode_arm(instr);
> +	}
>  
>  	if (fault) {
>  		type = TYPE_FAULT;
> -- 
> 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

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

[Dave Martin]

On Mon, Jul 22, 2013 at 05:33:51PM +0100, Ben Dooks wrote:
> 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 <ben.dooks@codethink.co.uk>
> ---
>  arch/arm/kernel/traps.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 1c08911..0a56e13 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -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.

>  
> @@ -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.)

>  			}
>  		} 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)) {

This also needs mem-to-opcode'ing.

Cheers
---Dave

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

[Dave Martin]

On Mon, Jul 22, 2013 at 05:33:52PM +0100, Ben Dooks wrote:
> 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 <ben.dooks@codethink.co.uk>
> ---
>  arch/arm/kernel/module.c |   57 +++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
> index 1e9be5d..7e13787 100644
> --- a/arch/arm/kernel/module.c
> +++ b/arch/arm/kernel/module.c
> @@ -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);

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

>  			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
>  
> -- 
> 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

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

[Ben Dooks]

On 24/07/13 18:34, Dave Martin wrote:
> On Mon, Jul 22, 2013 at 05:33:51PM +0100, Ben Dooks wrote:
>> 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<ben.dooks@codethink.co.uk>
>> ---
>>   arch/arm/kernel/traps.c |   10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index 1c08911..0a56e13 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -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.

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.

>> @@ -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.)

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

>>   			}
>>   		} 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)) {
>
> This also needs mem-to-opcode'ing.

Thanks, will check.

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

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

[Ben Dooks]

On 24/07/13 18:16, Dave Martin wrote:
> instr = thumb2arm(tinstr);

oops, removed it by accident. thanks for spotting.

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

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

[Dave Martin]

On Thu, Jul 25, 2013 at 12:17:59PM +0100, Ben Dooks wrote:
> On 24/07/13 18:34, Dave Martin wrote:
> >On Mon, Jul 22, 2013 at 05:33:51PM +0100, Ben Dooks wrote:
> >>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<ben.dooks@codethink.co.uk>
> >>---
> >>  arch/arm/kernel/traps.c |   10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> >>index 1c08911..0a56e13 100644
> >>--- a/arch/arm/kernel/traps.c
> >>+++ b/arch/arm/kernel/traps.c
> >>@@ -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.
> 
> 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).

> 
> >>@@ -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.)
> 
> It is possible that it could be changed, but I will avoid that for this
> particular patch

No problem, that was just cosmetic.

> 
> >>  			}
> >>  		} 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)) {
> >
> >This also needs mem-to-opcode'ing.
> 
> Thanks, will check.

OK.

Cheers
---Dave