[PATCH 0/2] add interworking support to Thumb2 kernel

[PATCH 0/2] add interworking support to Thumb2 kernel

[Ard Biesheuvel]

In the core kernel, interworking is supported since vmlinux is a static
binary, and the linker takes care of fixing up the bl instructions and
emitting veneers if necessary. For modules, however, all function objects
must currently be in the same mode, since the kernel's module loader does
not handle interworking switches at all.

However, now that we have optimized PLT support, we can simply enable it
unconditionally for Thumb2 builds, which by itself is reasonable since
the module area is pretty small, especially with multiplatform kernels
(current multi_v7_defconfig results in a .text section that exceeds 8 MB,
leaving less than 8 MB for modules when not using PLTs).
PLT support also allows us to reuse the veneer emitting routines to emit
veneers for interworking branches that cannot be fixed up by tweaking
the instruction encoding.

Patch #1 fixes an issue where ARM functions exported from other modules
will be mistaken for Thumb2 functions due to the missing STT_FUNC annotation
on SHN_UNDEF symbols.

Patch #2 implements the interworking support, by tweaking the instruction
encoding for unconditional bl instructions, and emitting a PLT veneer in
all other cases.

Ard Biesheuvel (2):
  ARM: Thumb-2: infer function annotation for external ksyms
  ARM: add interworking support to Thumb-2 kernel

 arch/arm/Kconfig              |  1 +
 arch/arm/include/asm/module.h |  3 +-
 arch/arm/kernel/module-plts.c | 21 +++++--
 arch/arm/kernel/module.c      | 64 ++++++++++++++++----
 4 files changed, 69 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/2] ARM: Thumb-2: infer function annotation for external ksyms

[Ard Biesheuvel]

Commit 9a00318eadbb ("Thumb-2: Relax relocation requirements for
non-function symbols") updated the Thumb-2 jump and call relocation
handling so that non-function symbols with the Thumb bit (bit 0)
cleared are not treated as A32 symbols requiring an interworking
mode switch.

However, since ksyms are stripped of their function annotation by
EXPORT_SYMBOL(), A32 symbols that do require an interworking mode
switch will be called in the wrong mode if the relocation is resolved
across a module boundary.

This patch enhances the function symbol check by including untyped
symbols that resolve to external ksyms.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/kernel/module.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 4f14b5ce6535..6c22b13cbd12 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -182,13 +182,20 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			/*
 			 * For function symbols, only Thumb addresses are
 			 * allowed (no interworking).
+			 * This applies equally to untyped symbols that
+			 * resolve to external ksyms: EXPORT_SYMBOL()
+			 * strips the function annotation, but we can
+			 * infer from the relocation type that the target
+			 * must be a function.
 			 *
 			 * For non-function symbols, the destination
 			 * has no specific ARM/Thumb disposition, so
 			 * the branch is resolved under the assumption
 			 * that interworking is not required.
 			 */
-			if (ELF32_ST_TYPE(sym->st_info) == STT_FUNC &&
+			if ((ELF32_ST_TYPE(sym->st_info) == STT_FUNC ||
+			     (ELF32_ST_TYPE(sym->st_info) == STT_NOTYPE &&
+			      sym->st_shndx == SHN_UNDEF)) &&
 			    !(sym->st_value & 1)) {
 				pr_err("%s: section %u reloc %u sym '%s': unsupported interworking call (Thumb -> ARM)\n",
 				       module->name, relindex, i, symname);
-- 
2.7.4

[PATCH 2/2] ARM: add interworking support to Thumb-2 kernel

[Ard Biesheuvel]

ARM/Thumb interworking is currently not allowed in modules at all:
external module dependencies can only be fulfilled by symbols of the
same flavour, but even inside a module, jump and call relocations
between objects are rejected if they would incur a mode switch.

This patch relaxes that restriction, by allowing function calls ('bl')
from T32 into A32 code, and vice versa, by fixing up the bl instruction
to a blx instruction, with the appropriate rounding applied to the offset.

For jump ('b') instructions, this is not possible (the ISA does not
provide jump instructions that switch mode) and so a PLT entry emitted
instead, which is inherently interworking-aware since it uses a
'ldr pc, =xxx' instruction. Since this requires module PLT support,
add the 'select ARM_MODULE_PLTS ' to config THUMB2_KERNEL.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/Kconfig              |  1 +
 arch/arm/include/asm/module.h |  3 +-
 arch/arm/kernel/module-plts.c | 21 +++++---
 arch/arm/kernel/module.c      | 55 +++++++++++++++-----
 4 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a9c4e48bb7ec..c786337f3c8b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1544,6 +1544,7 @@ config THUMB2_KERNEL
 	select AEABI
 	select ARM_ASM_UNIFIED
 	select ARM_UNWIND
+	select ARM_MODULE_PLTS
 	help
 	  By enabling this option, the kernel will be compiled in
 	  Thumb-2 mode. A compiler/assembler that understand the unified
diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index 464748b9fd7d..82ba2a82f9a3 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -28,7 +28,8 @@ struct mod_arch_specific {
 #endif
 };
 
-u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val);
+u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val,
+		   bool from_thumb);
 
 /*
  * Add the ARM architecture version to the version magic string
diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index a910f1db0c14..c239bc92f81b 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -18,12 +18,15 @@
 #define PLT_ENT_COUNT		(PLT_ENT_STRIDE / sizeof(u32))
 #define PLT_ENT_SIZE		(sizeof(struct plt_entries) / PLT_ENT_COUNT)
 
-#ifdef CONFIG_THUMB2_KERNEL
-#define PLT_ENT_LDR		__opcode_to_mem_thumb32(0xf8dff000 | \
+#define PLT_ENT_LDR_ARM		__opcode_to_mem_arm(0xe59ff000 | \
+						    (PLT_ENT_STRIDE - 8))
+#define PLT_ENT_LDR_THUMB	__opcode_to_mem_thumb32(0xf8dff000 | \
 							(PLT_ENT_STRIDE - 4))
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define PLT_ENT_LDR		PLT_ENT_LDR_THUMB
 #else
-#define PLT_ENT_LDR		__opcode_to_mem_arm(0xe59ff000 | \
-						    (PLT_ENT_STRIDE - 8))
+#define PLT_ENT_LDR		PLT_ENT_LDR_ARM
 #endif
 
 struct plt_entries {
@@ -31,7 +34,8 @@ struct plt_entries {
 	u32	lit[PLT_ENT_COUNT];
 };
 
-u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
+u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val,
+		   bool from_thumb)
 {
 	struct plt_entries *plt = (struct plt_entries *)mod->arch.plt->sh_addr;
 	int idx = 0;
@@ -45,7 +49,10 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
 		plt += (mod->arch.plt_count - 1) / PLT_ENT_COUNT;
 		idx = (mod->arch.plt_count - 1) % PLT_ENT_COUNT;
 
-		if (plt->lit[idx] == val)
+		if (plt->lit[idx] == val &&
+		    (!IS_ENABLED(CONFIG_THUMB2_KERNEL) ||
+		     plt->ldr[idx] == from_thumb ? PLT_ENT_LDR_THUMB :
+						   PLT_ENT_LDR_ARM))
 			return (u32)&plt->ldr[idx];
 
 		idx = (idx + 1) % PLT_ENT_COUNT;
@@ -65,6 +72,8 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
 	else
 		plt->lit[idx] = val;
 
+	if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && !from_thumb)
+		plt->ldr[idx] = PLT_ENT_LDR_ARM;
 	return (u32)&plt->ldr[idx];
 }
 
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 6c22b13cbd12..f6c4059d0006 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -103,7 +103,8 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		case R_ARM_PC24:
 		case R_ARM_CALL:
 		case R_ARM_JUMP24:
-			if (sym->st_value & 3) {
+			if (!IS_ENABLED(CONFIG_THUMB2_KERNEL) &&
+			    sym->st_value & 3) {
 				pr_err("%s: section %u reloc %u sym '%s': unsupported interworking call (ARM -> Thumb)\n",
 				       module->name, relindex, i, symname);
 				return -ENOEXEC;
@@ -121,12 +122,17 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			 * supported range. Note that 'offset + loc + 8'
 			 * contains the absolute jump target, i.e.,
 			 * @sym + addend, corrected for the +8 PC bias.
+			 * Also emit PLT entries for interworking jumps,
+			 * and for conditional interworking call instructions.
 			 */
 			if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) &&
 			    (offset <= (s32)0xfe000000 ||
-			     offset >= (s32)0x02000000))
+			     offset >= (s32)0x02000000 ||
+			     ((sym->st_value & 1) != 0 &&
+			      (ELF32_R_TYPE(rel->r_info) != R_ARM_CALL ||
+			       __mem_to_opcode_arm(*(u32 *)loc) >> 28 != 0xe))))
 				offset = get_module_plt(module, loc,
-							offset + loc + 8)
+							offset + loc + 8, false)
 					 - loc - 8;
 
 			if (offset <= (s32)0xfe000000 ||
@@ -138,11 +144,19 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 				return -ENOEXEC;
 			}
 
+			if (offset & 1) {
+				/* convert bl instruction to blx */
+				tmp = 0xf0000000 | (offset & 2) << 23;
+				*(u32 *)loc &= __opcode_to_mem_arm(~BIT(24));
+				*(u32 *)loc |= __opcode_to_mem_arm(tmp);
+			}
+
 			offset >>= 2;
 			offset &= 0x00ffffff;
 
 			*(u32 *)loc &= __opcode_to_mem_arm(0xff000000);
 			*(u32 *)loc |= __opcode_to_mem_arm(offset);
+
 			break;
 
 	       case R_ARM_V4BX:
@@ -180,8 +194,9 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		case R_ARM_THM_CALL:
 		case R_ARM_THM_JUMP24:
 			/*
-			 * For function symbols, only Thumb addresses are
-			 * allowed (no interworking).
+			 * For function symbols, we need to force a
+			 * Thumb -> ARM mode switch if the destination
+			 * address has its Thumb bit (bit 0) cleared.
 			 * This applies equally to untyped symbols that
 			 * resolve to external ksyms: EXPORT_SYMBOL()
 			 * strips the function annotation, but we can
@@ -197,9 +212,9 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 			     (ELF32_ST_TYPE(sym->st_info) == STT_NOTYPE &&
 			      sym->st_shndx == SHN_UNDEF)) &&
 			    !(sym->st_value & 1)) {
-				pr_err("%s: section %u reloc %u sym '%s': unsupported interworking call (Thumb -> ARM)\n",
-				       module->name, relindex, i, symname);
-				return -ENOEXEC;
+				tmp = sym->st_value;
+			} else {
+				tmp = sym->st_value | 1;
 			}
 
 			upper = __mem_to_opcode_thumb16(*(u16 *)loc);
@@ -227,18 +242,34 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 				((lower & 0x07ff) << 1);
 			if (offset & 0x01000000)
 				offset -= 0x02000000;
-			offset += sym->st_value - loc;
+			offset += tmp;
+
+			/*
+			 * When fixing up a bl instruction to blx, the address
+			 * of the call site must be rounded down in the
+			 * calculation of 'offset'. As this could potentially
+			 * cause 'offset' to go out of range, we need to do
+			 * this before performing the range check.
+			 */
+			tmp = offset & 1 ? loc : loc & ~2;
+			offset -= tmp;
 
 			/*
 			 * Route through a PLT entry if 'offset' exceeds the
-			 * supported range.
+			 * supported range. Also emit PLT entries for
+			 * interworking jump instructions.
 			 */
 			if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) &&
 			    (offset <= (s32)0xff000000 ||
-			     offset >= (s32)0x01000000))
+			     offset >= (s32)0x01000000 ||
+			     (ELF32_R_TYPE(rel->r_info) != R_ARM_THM_CALL &&
+			      !(sym->st_value & 1))))
 				offset = get_module_plt(module, loc,
-							offset + loc + 4)
+							offset + tmp + 4, true)
 					 - loc - 4;
+			else if (!(offset & 1))
+				/* fix up bl -> blx */
+				lower &= ~(1 << 12);
 
 			if (offset <= (s32)0xff000000 ||
 			    offset >= (s32)0x01000000) {
-- 
2.7.4

[PATCH 0/2] add interworking support to Thumb2 kernel

[Russell King - ARM Linux]

On Sat, Aug 20, 2016 at 09:45:29AM +0200, Ard Biesheuvel wrote:
> In the core kernel, interworking is supported since vmlinux is a static
> binary, and the linker takes care of fixing up the bl instructions and
> emitting veneers if necessary. For modules, however, all function objects
> must currently be in the same mode, since the kernel's module loader does
> not handle interworking switches at all.

I'm not sure what the usefulness of this is - you're effectively
encouraging people to build modules with a different configuration
from the main kernel, which is bad news.  Since it's the configuration
which dictates whether the kernel (and modules) are built as ARM or
as Thumb2, you'd have to use two different configurations, one for the
main kernel and one for the modules.

That's really something we should not be encouraging.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/2] add interworking support to Thumb2 kernel

[Ard Biesheuvel]

On 20 August 2016 at 18:23, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Sat, Aug 20, 2016 at 09:45:29AM +0200, Ard Biesheuvel wrote:
>> In the core kernel, interworking is supported since vmlinux is a static
>> binary, and the linker takes care of fixing up the bl instructions and
>> emitting veneers if necessary. For modules, however, all function objects
>> must currently be in the same mode, since the kernel's module loader does
>> not handle interworking switches at all.
>
> I'm not sure what the usefulness of this is - you're effectively
> encouraging people to build modules with a different configuration
> from the main kernel, which is bad news.  Since it's the configuration
> which dictates whether the kernel (and modules) are built as ARM or
> as Thumb2, you'd have to use two different configurations, one for the
> main kernel and one for the modules.
>

No, that is not the point. The point is to lift the requirement that a
Thumb2 kernel must implement all its exported or modularised functions
in Thumb2, not only in the core kernel but also in modules themselves.
For example, the crypto modules I contributed contain .S core
implementations, and building those in Thumb2 requires tweaking the
implementations so that the assembler accepts it as Thumb2 code. There
are examples in the tree (sha1-v4 and aes-v4) where we had to add ugly
workarounds so that the code would build (and it was still broken for
a while). So in general, it would be preferable to have a single ARM
implementation for both modes. Also, it will become less likely for
the multi_v7_defconfig to break inadvertently due to the addition of
low level platform code with asm components that turn out not to build
in Thumb2.

So with these patches, it is no longer necessary for core helper
routines coded in assembler (or built in ARM mode for other reasons)
to go out of their way so that the assembler can turn them in both ARM
and Thumb2 code.

I think there may be a performance benefit in some cases as well,
although this is anecdotal hearsay. The primary benefit of Thumb2 is
code size, and multi_v7_defconfig is a couple of MBs smaller when
built in Thumb2. In any case, lifting the restriction only affects
configurations that set CONFIG_THUMB2_KERNEL globally, and THUMB2
modules can still only be loaded into a THUMB2 kernel (due to the
THUMB2 tag in the version string).

Oh, and the first patch is arguably a bugfix. An exported ARM function
will not be rejected atm, but called in Thumb2 mode.

-- 
Ard.

[PATCH 0/2] add interworking support to Thumb2 kernel

[Russell King - ARM Linux]

On Sat, Aug 20, 2016 at 06:36:44PM +0200, Ard Biesheuvel wrote:
> No, that is not the point. The point is to lift the requirement that a
> Thumb2 kernel must implement all its exported or modularised functions
> in Thumb2, not only in the core kernel but also in modules themselves.
> For example, the crypto modules I contributed contain .S core
> implementations, and building those in Thumb2 requires tweaking the
> implementations so that the assembler accepts it as Thumb2 code. There
> are examples in the tree (sha1-v4 and aes-v4) where we had to add ugly
> workarounds so that the code would build (and it was still broken for
> a while). So in general, it would be preferable to have a single ARM
> implementation for both modes. Also, it will become less likely for
> the multi_v7_defconfig to break inadvertently due to the addition of
> low level platform code with asm components that turn out not to build
> in Thumb2.

Right, so we're talking _purely_ about assembly level stuff here.

However, what I would point out is that some people will want to build
their kernel as definitely ARM-only or definitely Thumb2-only.  While
A-class accepts either, other class CPUs do not.  If we allow
interworking, then we start breaking that.

We do already support CPUs that only enter the kernel in Thumb2 mode or
only enter in ARM mode, because the CPU has no support for the other
instruction set.

> So with these patches, it is no longer necessary for core helper
> routines coded in assembler (or built in ARM mode for other reasons)
> to go out of their way so that the assembler can turn them in both ARM
> and Thumb2 code.

I'm not sure what "core helper routines" you're referring to - all C
functions will be built according to whether we want ARM mode or Thumb2
mode.  We aren't going to end up with a mixture for C functions within
the kernel.

> I think there may be a performance benefit in some cases as well,
> although this is anecdotal hearsay. The primary benefit of Thumb2 is
> code size, and multi_v7_defconfig is a couple of MBs smaller when
> built in Thumb2. In any case, lifting the restriction only affects
> configurations that set CONFIG_THUMB2_KERNEL globally, and THUMB2
> modules can still only be loaded into a THUMB2 kernel (due to the
> THUMB2 tag in the version string).
> 
> Oh, and the first patch is arguably a bugfix. An exported ARM function
> will not be rejected atm, but called in Thumb2 mode.

Sorry, I don't see it as a bug fix at all - I see it as a regression
causing change for those classes of CPU which only accept one or
other of the ISAs.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/2] add interworking support to Thumb2 kernel

[Ard Biesheuvel]

On 20 August 2016 at 18:55, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Sat, Aug 20, 2016 at 06:36:44PM +0200, Ard Biesheuvel wrote:
>> No, that is not the point. The point is to lift the requirement that a
>> Thumb2 kernel must implement all its exported or modularised functions
>> in Thumb2, not only in the core kernel but also in modules themselves.
>> For example, the crypto modules I contributed contain .S core
>> implementations, and building those in Thumb2 requires tweaking the
>> implementations so that the assembler accepts it as Thumb2 code. There
>> are examples in the tree (sha1-v4 and aes-v4) where we had to add ugly
>> workarounds so that the code would build (and it was still broken for
>> a while). So in general, it would be preferable to have a single ARM
>> implementation for both modes. Also, it will become less likely for
>> the multi_v7_defconfig to break inadvertently due to the addition of
>> low level platform code with asm components that turn out not to build
>> in Thumb2.
>
> Right, so we're talking _purely_ about assembly level stuff here.
>

Primarily, yes.

> However, what I would point out is that some people will want to build
> their kernel as definitely ARM-only or definitely Thumb2-only.  While
> A-class accepts either, other class CPUs do not.  If we allow
> interworking, then we start breaking that.
>
> We do already support CPUs that only enter the kernel in Thumb2 mode or
> only enter in ARM mode, because the CPU has no support for the other
> instruction set.
>
>> So with these patches, it is no longer necessary for core helper
>> routines coded in assembler (or built in ARM mode for other reasons)
>> to go out of their way so that the assembler can turn them in both ARM
>> and Thumb2 code.
>
> I'm not sure what "core helper routines" you're referring to - all C
> functions will be built according to whether we want ARM mode or Thumb2
> mode.  We aren't going to end up with a mixture for C functions within
> the kernel.
>

Yes, but taking the NEON code in arch/arm/crypto as an example, this
can only execute on A cores anyway. When it is built into the kernel,
we can happily use the ARM version on a Thumb2 kernel, but as a module
it suddenly has to be adapted so that it is emitted as Thumb2 (while
upstream OpenSSL builds it in ARM mode)

>> I think there may be a performance benefit in some cases as well,
>> although this is anecdotal hearsay. The primary benefit of Thumb2 is
>> code size, and multi_v7_defconfig is a couple of MBs smaller when
>> built in Thumb2. In any case, lifting the restriction only affects
>> configurations that set CONFIG_THUMB2_KERNEL globally, and THUMB2
>> modules can still only be loaded into a THUMB2 kernel (due to the
>> THUMB2 tag in the version string).
>>
>> Oh, and the first patch is arguably a bugfix. An exported ARM function
>> will not be rejected atm, but called in Thumb2 mode.
>
> Sorry, I don't see it as a bug fix at all - I see it as a regression
> causing change for those classes of CPU which only accept one or
> other of the ISAs.
>

At the moment, the module loader allows R_ARM_THM_CALL relocations
against imported ARM symbols, under the assumption that an untyped
symbol is a Thumb2 symbol that simply has its Thumb bit cleared due to
the fact that it lacks a function annotation. However, if the
relocation is resolved across a module boundary, this annotation is
missing as well, and so imported ARM symbols are mistaken for Thumb2
symbols, causing a crash when called. Patch #1 fixes just that.

So do you (or anyone else) have any comments on the performance delta
between Thumb2 and ARM? I can imagine fixed size instructions make for
more efficient pipelines, but as I said, this is only a hunch.