arm64: alternative patching rework

The current alternative instruction framework is not kind to branches, potentially leading to all kind of hacks in the code that uses alternatives. This series expands it to deal with immediate and conditional branches.

This is a rewrite of fef7f2b20103, which got reverted in b9a95e85bbc as it was breaking unsuspecting branches inside an alternate sequence. It now also deals with conditional branches (instead of just asserting a BUG).

Another nit is addressed by the last patch, where GAS gets confused by the combinaison of a .inst directive (as used by the msr_s/mrs_s pseudo-instruction), a label, and a .if directive evaluating said label. As this is exactly what the alternative framework uses to detect length mismatch, this patch reverts to using a pair .org directives in a creative way.

This has been tested on v4.1-rc5.

Marc Zyngier (3):
arm64: insn: Add aarch64_{get,set}_branch_offset
arm64: alternative: Allow immediate branch as alternative instruction
arm64: alternative: Work around .inst assembler bugs

 arch/arm64/include/asm/alternative-asm.h | 29 -------------
 arch/arm64/include/asm/alternative.h     | 52 +++++++++++++++++++++--
 arch/arm64/include/asm/insn.h            |  3 ++
 arch/arm64/kernel/alternative.c          | 71 +++++++++++++++++++++++++++++---
 arch/arm64/kernel/entry.S                |  2 +-
 arch/arm64/kernel/insn.c                 | 60 +++++++++++++++++++++++++++
 arch/arm64/mm/cache.S                    |  2 +-
 7 files changed, 179 insertions(+), 40 deletions(-)
 delete mode 100644 arch/arm64/include/asm/alternative-asm.h

2.1.4

In order to deal with branches located in alternate sequences, but pointing to the main kernel text, it is required to extract the relative displacement encoded in the instruction, and to be able to update said instruction with a new offset (once it is known).

For this, we introduce three new helpers:
- aarch64_insn_is_branch_imm is a predicate indicating if the instruction is an immediate branch
- aarch64_get_branch_offset returns a signed value representing the byte offset encoded in a branch instruction
- aarch64_set_branch_offset takes an instruction and an offset, and returns the corresponding updated instruction.

---
 arch/arm64/include/asm/insn.h |  3 +++
 arch/arm64/kernel/insn.c      | 60 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

index f81b328..30e50eb 100644
@@ -281,6 +281,7 @@ __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
 #undef	__AARCH64_INSN_FUNCS
 
 bool aarch64_insn_is_nop(u32 insn);
+bool aarch64_insn_is_branch_imm(u32 insn);
 
 int aarch64_insn_read(void *addr, u32 *insnp);
 int aarch64_insn_write(void *addr, u32 insn);

@@ -351,6 +352,8 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 					 int shift,
 					 enum aarch64_insn_variant variant,
 					 enum aarch64_insn_logic_type type);
+s32 aarch64_get_branch_offset(u32 insn);
+u32 aarch64_set_branch_offset(u32 insn, s32 offset);
 
 bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
 

2.1.4

Since all branches are PC-relative on AArch64, these instructions cannot be used as an alternative with the simplistic approach we currently have (the immediate has been computed from the .altinstr_replacement section, and end-up being completely off if the target is outside of the replacement sequence).

This patch handles the branch instructions in a different way, using the insn framework to recompute the immediate, and generate the right displacement in the above case.

---
 arch/arm64/kernel/alternative.c | 71 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 5 deletions(-)

index 28f8365..df4bf15 100644
@@ -24,8 +24,13 @@
 #include <asm/cacheflush.h>
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
+#include <asm/insn.h>
 #include <linux/stop_machine.h>
 
+#define __ALT_PTR(a,f)		(u32 *)((void *)&(a)->f + (a)->f)
+#define ALT_ORIG_PTR(a)		__ALT_PTR(a, orig_offset)
+#define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
+
 extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
 
 struct alt_region {

@@ -33,13 +38,63 @@ struct alt_region {
 	struct alt_instr *end;
 };
 
+/*
+ * Check if the target PC is within an alternative block.
+ */
+static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
+{
+	unsigned long replptr;
+
+	if (kernel_text_address(pc))
+		return 1;
+
+	replptr = (unsigned long)ALT_REPL_PTR(alt);
+	if (pc >= replptr && pc < (replptr + alt->alt_len))
+		return 0;
+
+	/*
+	 * Branching into *another* alternate sequence is doomed, and
+	 * we're not even trying to fix it up.
+	 */
+	BUG();
+}
+
+static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
+{
+	u32 insn;
+
+	insn = le32_to_cpu(*altinsnptr);
+
+	if (aarch64_insn_is_branch_imm(insn)) {
+		s32 offset = aarch64_get_branch_offset(insn);
+		unsigned long target;
+
+		target = (unsigned long)altinsnptr + offset;
+
+		/*
+		 * If we're branching inside the alternate sequence,
+		 * do not rewrite the instruction, as it is already
+		 * correct. Otherwise, generate the new instruction.
+		 */
+		if (branch_insn_requires_update(alt, target)) {
+			offset = target - (unsigned long)insnptr;
+			insn = aarch64_set_branch_offset(insn, offset);
+		}
+	}
+
+	return insn;
+}
+
 static int __apply_alternatives(void *alt_region)
 {
 	struct alt_instr *alt;
 	struct alt_region *region = alt_region;
-	u8 *origptr, *replptr;
+	u32 *origptr, *replptr;
 
 	for (alt = region->begin; alt < region->end; alt++) {
+		u32 insn;
+		int i, nr_inst;
+
 		if (!cpus_have_cap(alt->cpufeature))
 			continue;
 

@@ -47,11 +102,17 @@ static int __apply_alternatives(void *alt_region)
 
 		pr_info_once("patching kernel code\n");
 
-		origptr = (u8 *)&alt->orig_offset + alt->orig_offset;
-		replptr = (u8 *)&alt->alt_offset + alt->alt_offset;
-		memcpy(origptr, replptr, alt->alt_len);
+		origptr = ALT_ORIG_PTR(alt);
+		replptr = ALT_REPL_PTR(alt);
+		nr_inst = alt->alt_len / sizeof(insn);
+
+		for (i = 0; i < nr_inst; i++) {
+			insn = get_alt_insn(alt, origptr + i, replptr + i);
+			*(origptr + i) = cpu_to_le32(insn);
+		}
+
 		flush_icache_range((uintptr_t)origptr,
-				   (uintptr_t)(origptr + alt->alt_len));
+				   (uintptr_t)(origptr + nr_inst));
 	}
 
 	return 0;

2.1.4

AArch64 toolchains suffer from the following bug:

$ cat blah.S
1:
	.inst	0x01020304
	.if ((. - 1b) != 4)
		.error	blah
	.endif
$ aarch64-linux-gnu-gcc -c blah.S
blah.S: Assembler messages:
blah.S:3: Error: non-constant expression in ".if" statement

which precludes the use of msr_s and co as part of alternatives.

We workaround this issue by not directly testing the labels themselves, but by moving the current output pointer by a value that should always be zero. if this value is not null, then we will trigger a backward move, which is expclicitely forbidden. This trigger the error we're after:

AS arch/arm64/kvm/hyp.o
arch/arm64/kvm/hyp.S: Assembler messages:
arch/arm64/kvm/hyp.S:1377: Error: attempt to move .org backwards scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp.o' failed make[1]: *** [arch/arm64/kvm/hyp.o] Error 1
Makefile:946: recipe for target 'arch/arm64/kvm' failed

Not pretty, but at least works on the current toolchains. Also merge asm/alternative-asm.h into alternative.h so that we slightly limit the duplication of this mess.

---
 arch/arm64/include/asm/alternative-asm.h | 29 ------------------
 arch/arm64/include/asm/alternative.h     | 52 +++++++++++++++++++++++++++++---
 arch/arm64/kernel/entry.S                |  2 +-
 arch/arm64/mm/cache.S                    |  2 +-
 4 files changed, 50 insertions(+), 35 deletions(-)
 delete mode 100644 arch/arm64/include/asm/alternative-asm.h

deleted file mode 100644
index 919a678..0000000
@@ -1,29 +0,0 @@
-#ifndef __ASM_ALTERNATIVE_ASM_H
-#define __ASM_ALTERNATIVE_ASM_H
-
-#ifdef __ASSEMBLY__
-
-.macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len
-	.word \orig_offset - .
-	.word \alt_offset - .
-	.hword \feature
-	.byte \orig_len
-	.byte \alt_len
-.endm
-
-.macro alternative_insn insn1 insn2 cap
-661:	\insn1
-662:	.pushsection .altinstructions, "a"
-	altinstruction_entry 661b, 663f, \cap, 662b-661b, 664f-663f
-	.popsection
-	.pushsection .altinstr_replacement, "ax"
-663:	\insn2
-664:	.popsection
-	.if ((664b-663b) != (662b-661b))
-		.error "Alternatives instruction length mismatch"
-	.endif
-.endm
-
-#endif  /*  __ASSEMBLY__  */
-
-#endif /* __ASM_ALTERNATIVE_ASM_H */

index 959fe87..00df926 100644
@@ -21,7 +21,7 @@
 #include <linux/init.h>
 #include <linux/linkage.h>
 
-#include <asm/alternative-asm.h>
+#include <asm/alternative.h>
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/cpufeature.h>

index 2560e1e..70a79cb 100644
@@ -22,7 +22,7 @@
 #include <linux/init.h>
 #include <asm/assembler.h>
 #include <asm/cpufeature.h>
-#include <asm/alternative-asm.h>
+#include <asm/alternative.h>
 
 #include "proc-macros.S"
 

2.1.4

This should probably be two patches.