[PATCH v2 0/4] arm64: alternative patching rework

[PATCH v2 0/4] arm64: alternative patching rework

[Marc Zyngier]

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 of .org
directives in a creative way. To make this a bit easier,
alternative-asm.h is folded into alternative.h.

This has been tested on v4.1-rc5.

Marc Zyngier (4):
  arm64: insn: Add aarch64_{get,set}_branch_offset
  arm64: alternative: Allow immediate branch as alternative instruction
  arm64: alternative: Merge alternative-asm.h into alternative.h
  arm64: alternative: Work around .inst assembler bugs

 arch/arm64/include/asm/alternative-asm.h | 29 -------------
 arch/arm64/include/asm/alternative.h     | 46 +++++++++++++++++++--
 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, 173 insertions(+), 40 deletions(-)
 delete mode 100644 arch/arm64/include/asm/alternative-asm.h

-- 
2.1.4

[PATCH v2 1/4] arm64: insn: Add aarch64_{get,set}_branch_offset

[Marc Zyngier]

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.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/insn.h |  3 +++
 arch/arm64/kernel/insn.c      | 60 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index f81b328..30e50eb 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -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);
 
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 9249020..dd9671c 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -77,6 +77,14 @@ bool __kprobes aarch64_insn_is_nop(u32 insn)
 	}
 }
 
+bool aarch64_insn_is_branch_imm(u32 insn)
+{
+	return (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn) ||
+		aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn) ||
+		aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
+		aarch64_insn_is_bcond(insn));
+}
+
 static DEFINE_SPINLOCK(patch_lock);
 
 static void __kprobes *patch_map(void *addr, int fixmap)
@@ -1057,6 +1065,58 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_6, insn, shift);
 }
 
+/*
+ * Decode the imm field of a branch, and return the byte offset as a
+ * signed value (so it can be used when computing a new branch
+ * target).
+ */
+s32 aarch64_get_branch_offset(u32 insn)
+{
+	s32 imm;
+
+	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn)) {
+		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_26, insn);
+		return (imm << 6) >> 4;
+	}
+
+	if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
+	    aarch64_insn_is_bcond(insn)) {
+		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_19, insn);
+		return (imm << 13) >> 11;
+	}
+
+	if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn)) {
+		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_14, insn);
+		return (imm << 18) >> 16;
+	}
+
+	/* Unhandled instruction */
+	BUG();
+}
+
+/*
+ * Encode the displacement of a branch in the imm field and return the
+ * updated instruction.
+ */
+u32 aarch64_set_branch_offset(u32 insn, s32 offset)
+{
+	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn))
+		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
+						     offset >> 2);
+
+	if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
+	    aarch64_insn_is_bcond(insn))
+		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_19, insn,
+						     offset >> 2);
+
+	if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn))
+		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_14, insn,
+						     offset >> 2);
+
+	/* Unhandled instruction */
+	BUG();
+}
+
 bool aarch32_insn_is_wide(u32 insn)
 {
 	return insn >= 0xe800;
-- 
2.1.4

[PATCH v2 2/4] arm64: alternative: Allow immediate branch as alternative instruction

[Marc Zyngier]

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.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/alternative.c | 71 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 28f8365..df4bf15 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -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

[PATCH v2 3/4] arm64: alternative: Merge alternative-asm.h into alternative.h

[Marc Zyngier]

asm/alternative-asm.h and asm/alternative.h are extremely similar,
and really deserve to live in the same file (as this makes further
modufications a bit easier).

Fold the content of alternative-asm.h into alternative.h, and
update the few users.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/alternative-asm.h | 29 -----------------------------
 arch/arm64/include/asm/alternative.h     | 27 +++++++++++++++++++++++++++
 arch/arm64/kernel/entry.S                |  2 +-
 arch/arm64/mm/cache.S                    |  2 +-
 4 files changed, 29 insertions(+), 31 deletions(-)
 delete mode 100644 arch/arm64/include/asm/alternative-asm.h

diff --git a/arch/arm64/include/asm/alternative-asm.h b/arch/arm64/include/asm/alternative-asm.h
deleted file mode 100644
index 919a678..0000000
--- a/arch/arm64/include/asm/alternative-asm.h
+++ /dev/null
@@ -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 */
diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index d261f01..265b13e 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_ALTERNATIVE_H
 #define __ASM_ALTERNATIVE_H
 
+#ifndef __ASSEMBLY__
+
 #include <linux/types.h>
 #include <linux/stddef.h>
 #include <linux/stringify.h>
@@ -41,4 +43,29 @@ void free_alternatives_memory(void);
 	"	.error \"Alternatives instruction length mismatch\"\n\t"\
 	".endif\n"
 
+#else
+
+.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_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 959fe87..00df926 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -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>
diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
index 2560e1e..70a79cb 100644
--- a/arch/arm64/mm/cache.S
+++ b/arch/arm64/mm/cache.S
@@ -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

[PATCH v2 4/4] arm64: alternative: Work around .inst assembler bugs

[Marc Zyngier]

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

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/alternative.h | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 265b13e..c385a0c 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -26,7 +26,20 @@ void free_alternatives_memory(void);
 	" .byte 662b-661b\n"				/* source len      */ \
 	" .byte 664f-663f\n"				/* replacement len */
 
-/* alternative assembly primitive: */
+/*
+ * alternative assembly primitive:
+ *
+ * If any of these .org directive fail, it means that insn1 and insn2
+ * don't have the same length. This used to be written as
+ *
+ * .if ((664b-663b) != (662b-661b))
+ * 	.error "Alternatives instruction length mismatch"
+ * .endif
+ *
+ * but most assemblers die if insn1 or insn2 have a .inst. This should
+ * be fixed in a binutils release posterior to 2.25.51.0.2 (anything
+ * containing commit 4e4d08cf7399b606 or c1baaddf8861).
+ */
 #define ALTERNATIVE(oldinstr, newinstr, feature)			\
 	"661:\n\t"							\
 	oldinstr "\n"							\
@@ -39,9 +52,8 @@ void free_alternatives_memory(void);
 	newinstr "\n"							\
 	"664:\n\t"							\
 	".popsection\n\t"						\
-	".if ((664b-663b) != (662b-661b))\n\t"				\
-	"	.error \"Alternatives instruction length mismatch\"\n\t"\
-	".endif\n"
+	".org	. - (664b-663b) + (662b-661b)\n\t"			\
+	".org	. - (662b-661b) + (664b-663b)\n"
 
 #else
 
@@ -61,9 +73,8 @@ void free_alternatives_memory(void);
 	.pushsection .altinstr_replacement, "ax"
 663:	\insn2
 664:	.popsection
-	.if ((664b-663b) != (662b-661b))
-		.error "Alternatives instruction length mismatch"
-	.endif
+	.org	. - (664b-663b) + (662b-661b)
+	.org	. - (662b-661b) + (664b-663b)
 .endm
 
 #endif  /*  __ASSEMBLY__  */
-- 
2.1.4

[PATCH v2 0/4] arm64: alternative patching rework

[Catalin Marinas]

On Mon, Jun 01, 2015 at 10:47:38AM +0100, Marc Zyngier wrote:
> 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 of .org
> directives in a creative way. To make this a bit easier,
> alternative-asm.h is folded into alternative.h.
> 
> This has been tested on v4.1-rc5.
> 
> Marc Zyngier (4):
>   arm64: insn: Add aarch64_{get,set}_branch_offset
>   arm64: alternative: Allow immediate branch as alternative instruction
>   arm64: alternative: Merge alternative-asm.h into alternative.h
>   arm64: alternative: Work around .inst assembler bugs

Applied, thanks.

-- 
Catalin

[PATCH v2 0/4] arm64: alternative patching rework

[Catalin Marinas]

On Tue, Jun 02, 2015 at 06:47:15PM +0100, Catalin Marinas wrote:
> On Mon, Jun 01, 2015 at 10:47:38AM +0100, Marc Zyngier wrote:
> > 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 of .org
> > directives in a creative way. To make this a bit easier,
> > alternative-asm.h is folded into alternative.h.
> > 
> > This has been tested on v4.1-rc5.
> > 
> > Marc Zyngier (4):
> >   arm64: insn: Add aarch64_{get,set}_branch_offset
> >   arm64: alternative: Allow immediate branch as alternative instruction
> >   arm64: alternative: Merge alternative-asm.h into alternative.h
> >   arm64: alternative: Work around .inst assembler bugs
> 
> Applied, thanks.

Applied, but not pushed out yet. Testing on Juno gives:

alternatives: patching kernel code
BUG: failure at /work/Linux/linux-2.6-aarch64/arch/arm64/kernel/alternative.c:59/branch_insn_requires_update()!
Kernel panic - not syncing: BUG!
CPU: 0 PID: 10 Comm: migration/0 Not tainted 4.1.0-rc4+ #224
Hardware name: Juno (DT)
Call trace:
[<ffffffc00008992c>] dump_backtrace+0x0/0x11c
[<ffffffc000089a58>] show_stack+0x10/0x1c
[<ffffffc0005b49b4>] dump_stack+0x88/0xc8
[<ffffffc0005b38a8>] panic+0xe0/0x220
[<ffffffc00008e3b0>] __apply_alternatives+0x1ac/0x1cc
[<ffffffc000123aa8>] multi_cpu_stop+0xf8/0x120
[<ffffffc000123d60>] cpu_stopper_thread+0xb0/0x148
[<ffffffc0000d12ec>] smpboot_thread_fn+0x150/0x274
[<ffffffc0000cdedc>] kthread+0xd8/0xf0
SMP: failed to stop secondary CPUs


I haven't investigated yet, I'll have a look tomorrow.

-- 
Catalin

[PATCH v2 0/4] arm64: alternative patching rework

[Marc Zyngier]

Hi Catalin,

On 02/06/15 18:59, Catalin Marinas wrote:
> On Tue, Jun 02, 2015 at 06:47:15PM +0100, Catalin Marinas wrote:
>> On Mon, Jun 01, 2015 at 10:47:38AM +0100, Marc Zyngier wrote:
>>> 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 of .org
>>> directives in a creative way. To make this a bit easier,
>>> alternative-asm.h is folded into alternative.h.
>>>
>>> This has been tested on v4.1-rc5.
>>>
>>> Marc Zyngier (4):
>>>   arm64: insn: Add aarch64_{get,set}_branch_offset
>>>   arm64: alternative: Allow immediate branch as alternative instruction
>>>   arm64: alternative: Merge alternative-asm.h into alternative.h
>>>   arm64: alternative: Work around .inst assembler bugs
>>
>> Applied, thanks.
> 
> Applied, but not pushed out yet. Testing on Juno gives:
> 
> alternatives: patching kernel code
> BUG: failure at /work/Linux/linux-2.6-aarch64/arch/arm64/kernel/alternative.c:59/branch_insn_requires_update()!
> Kernel panic - not syncing: BUG!
> CPU: 0 PID: 10 Comm: migration/0 Not tainted 4.1.0-rc4+ #224
> Hardware name: Juno (DT)
> Call trace:
> [<ffffffc00008992c>] dump_backtrace+0x0/0x11c
> [<ffffffc000089a58>] show_stack+0x10/0x1c
> [<ffffffc0005b49b4>] dump_stack+0x88/0xc8
> [<ffffffc0005b38a8>] panic+0xe0/0x220
> [<ffffffc00008e3b0>] __apply_alternatives+0x1ac/0x1cc
> [<ffffffc000123aa8>] multi_cpu_stop+0xf8/0x120
> [<ffffffc000123d60>] cpu_stopper_thread+0xb0/0x148
> [<ffffffc0000d12ec>] smpboot_thread_fn+0x150/0x274
> [<ffffffc0000cdedc>] kthread+0xd8/0xf0
> SMP: failed to stop secondary CPUs
> 
> 
> I haven't investigated yet, I'll have a look tomorrow.

I reproduced this. Two issues:

- the workaround for CONFIG_ARM64_ERRATUM_845719 is written with two 
alternate sequences, and the first one branches in to the second. 
Exactly what this series disallows... I'll post a rewrite of this 
sequence in a minute.

- there is a small bug that the above also triggers, as it branches 
just *after* the last instruction of the sequence. This doesn't 
generate any relocation problem, and can be accepted. Can you fold the 
patchlet below into the second patch of the series?

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index df4bf15..221b983 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -49,7 +49,7 @@ static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
                return 1;
 
        replptr = (unsigned long)ALT_REPL_PTR(alt);
-       if (pc >= replptr && pc < (replptr + alt->alt_len))
+       if (pc >= replptr && pc <= (replptr + alt->alt_len))
                return 0;
 
        /*

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...