[PATCH 0/5] arm64: assorted fixes for dcache_by_line_op

[PATCH 0/5] arm64: assorted fixes for dcache_by_line_op

[Ard Biesheuvel]

This fixes two issues in dcache_by_line_op: patch #4 fixes the logic
that is applied when patching DC CVAP instructions, and patch #5 gets
rid of some unintended undefined symbols resulting from incorrect use
of conditional GAS directives.

Patches #1 to #3 are groundwork for #4.

Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Suzuki Poulose <suzuki.poulose@arm.com>
Cc: Dave Martin <Dave.Martin@arm.com>

Ard Biesheuvel (5):
  arm64/alternative_cb: move callback reference into replacements
    section
  arm64/alternative_cb: add nr_alts parameter to callback handlers
  arm64/alternative_cb: add support for alternative sequences
  arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions
  arm64/mm: use string comparisons in dcache_by_line_op

 arch/arm64/include/asm/alternative.h | 54 +++++++++++---------
 arch/arm64/include/asm/assembler.h   | 17 +++---
 arch/arm64/include/asm/kvm_mmu.h     |  4 +-
 arch/arm64/kernel/alternative.c      | 22 +++++---
 arch/arm64/kernel/cpu_errata.c       | 24 ++++++---
 arch/arm64/kvm/va_layout.c           |  8 +--
 6 files changed, 79 insertions(+), 50 deletions(-)

-- 
2.19.2

[PATCH 1/5] arm64/alternative_cb: move callback reference into replacements section

[Ard Biesheuvel]

In preparation of permitting callback type alternatives patching
routines to use any number of alternative instructions, move the
relative reference to the callback routine out of the primary
'struct alt_instr' data structure, where we are currently overloading
the 'alt_offset' member depending on the feature id. Instead, put
a relative reference to the callback at the start of the alternative
sequence.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/alternative.h | 39 +++++++++-----------
 arch/arm64/kernel/alternative.c      | 11 ++++--
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 4b650ec1d7dd..77da798e888b 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -24,6 +24,11 @@ struct alt_instr {
 	u8  alt_len;		/* size of new instruction(s), <= orig_len */
 };
 
+struct alt_instr_cb {
+	s32 cb_offset;		/* offset to callback handler */
+	__le32 insn[];		/* sequence of alternative instructions */
+};
+
 typedef void (*alternative_cb_t)(struct alt_instr *alt,
 				 __le32 *origptr, __le32 *updptr, int nr_inst);
 
@@ -35,13 +40,9 @@ void apply_alternatives_module(void *start, size_t length);
 static inline void apply_alternatives_module(void *start, size_t length) { }
 #endif
 
-#define ALTINSTR_ENTRY(feature,cb)					      \
+#define ALTINSTR_ENTRY(feature)						      \
 	" .word 661b - .\n"				/* label           */ \
-	" .if " __stringify(cb) " == 0\n"				      \
 	" .word 663f - .\n"				/* new instruction */ \
-	" .else\n"							      \
-	" .word " __stringify(cb) "- .\n"		/* callback */	      \
-	" .endif\n"							      \
 	" .hword " __stringify(feature) "\n"		/* feature bit     */ \
 	" .byte 662b-661b\n"				/* source len      */ \
 	" .byte 664f-663f\n"				/* replacement len */
@@ -59,36 +60,29 @@ static inline void apply_alternatives_module(void *start, size_t length) { }
  * 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).
- *
- * Alternatives with callbacks do not generate replacement instructions.
  */
-#define __ALTERNATIVE_CFG(oldinstr, newinstr, feature, cfg_enabled, cb)	\
-	".if "__stringify(cfg_enabled)" == 1\n"				\
+#define __ALTERNATIVE_CFG(oldinstr, newinstr, feature)			\
 	"661:\n\t"							\
 	oldinstr "\n"							\
 	"662:\n"							\
 	".pushsection .altinstructions,\"a\"\n"				\
-	ALTINSTR_ENTRY(feature,cb)					\
+	ALTINSTR_ENTRY(feature)						\
 	".popsection\n"							\
-	" .if " __stringify(cb) " == 0\n"				\
 	".pushsection .altinstr_replacement, \"a\"\n"			\
 	"663:\n\t"							\
 	newinstr "\n"							\
 	"664:\n\t"							\
-	".popsection\n\t"						\
+	".popsection\n\t"
+
+#define _ALTERNATIVE_CFG(oldinstr, newinstr, feature, cfg, ...)		\
+	".if "__stringify(IS_ENABLED(cfg))" == 1\n"			\
+	__ALTERNATIVE_CFG(oldinstr, newinstr, feature)			\
 	".org	. - (664b-663b) + (662b-661b)\n\t"			\
 	".org	. - (662b-661b) + (664b-663b)\n"			\
-	".else\n\t"							\
-	"663:\n\t"							\
-	"664:\n\t"							\
-	".endif\n"							\
 	".endif\n"
 
-#define _ALTERNATIVE_CFG(oldinstr, newinstr, feature, cfg, ...)	\
-	__ALTERNATIVE_CFG(oldinstr, newinstr, feature, IS_ENABLED(cfg), 0)
-
 #define ALTERNATIVE_CB(oldinstr, cb) \
-	__ALTERNATIVE_CFG(oldinstr, "NOT_AN_INSTRUCTION", ARM64_CB_PATCH, 1, cb)
+	__ALTERNATIVE_CFG(oldinstr, ".word " __stringify(cb) " - .\n", ARM64_CB_PATCH)
 #else
 
 #include <asm/assembler.h>
@@ -158,8 +152,11 @@ static inline void apply_alternatives_module(void *start, size_t length) { }
 .macro alternative_cb cb
 	.set .Lasm_alt_mode, 0
 	.pushsection .altinstructions, "a"
-	altinstruction_entry 661f, \cb, ARM64_CB_PATCH, 662f-661f, 0
+	altinstruction_entry 661f, 663f, ARM64_CB_PATCH, 662f-661f, 664f-663f
 	.popsection
+	.pushsection .altinstr_replacement, "ax"
+663:	.word	\cb - .
+664:	.popsection
 661:
 .endm
 
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index b5d603992d40..a49930843784 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -151,6 +151,7 @@ static void __apply_alternatives(void *alt_region, bool is_module)
 	struct alt_region *region = alt_region;
 	__le32 *origptr, *updptr;
 	alternative_cb_t alt_cb;
+	struct alt_instr_cb *alt_cb_insn;
 
 	for (alt = region->begin; alt < region->end; alt++) {
 		int nr_inst;
@@ -161,7 +162,7 @@ static void __apply_alternatives(void *alt_region, bool is_module)
 			continue;
 
 		if (alt->cpufeature == ARM64_CB_PATCH)
-			BUG_ON(alt->alt_len != 0);
+			BUG_ON(alt->alt_len < sizeof(*alt_cb_insn));
 		else
 			BUG_ON(alt->alt_len != alt->orig_len);
 
@@ -171,10 +172,12 @@ static void __apply_alternatives(void *alt_region, bool is_module)
 		updptr = is_module ? origptr : lm_alias(origptr);
 		nr_inst = alt->orig_len / AARCH64_INSN_SIZE;
 
-		if (alt->cpufeature < ARM64_CB_PATCH)
+		if (alt->cpufeature < ARM64_CB_PATCH) {
 			alt_cb = patch_alternative;
-		else
-			alt_cb  = ALT_REPL_PTR(alt);
+		} else {
+			alt_cb_insn = ALT_REPL_PTR(alt);
+			alt_cb = offset_to_ptr(&alt_cb_insn->cb_offset);
+		}
 
 		alt_cb(alt, origptr, updptr, nr_inst);
 
-- 
2.19.2

[PATCH 2/5] arm64/alternative_cb: add nr_alts parameter to callback handlers

[Ard Biesheuvel]

Callback handlers for alternative patching will shortly be able to
access a set of alternative instructions provided at assembly time.
So update the callback handler prototypes so we can pass this number.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/alternative.h |  4 ++--
 arch/arm64/include/asm/kvm_mmu.h     |  4 ++--
 arch/arm64/kernel/alternative.c      | 11 +++++++----
 arch/arm64/kernel/cpu_errata.c       | 10 ++++------
 arch/arm64/kvm/va_layout.c           |  8 ++++----
 5 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 77da798e888b..987c1514183a 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -29,8 +29,8 @@ struct alt_instr_cb {
 	__le32 insn[];		/* sequence of alternative instructions */
 };
 
-typedef void (*alternative_cb_t)(struct alt_instr *alt,
-				 __le32 *origptr, __le32 *updptr, int nr_inst);
+typedef void (*alternative_cb_t)(struct alt_instr *alt, __le32 *origptr,
+				 __le32 *updptr, int nr_inst, int nr_alts);
 
 void __init apply_alternatives_all(void);
 
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 658657367f2f..5e32e314b9f0 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -100,8 +100,8 @@ alternative_cb_end
 #include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 
-void kvm_update_va_mask(struct alt_instr *alt,
-			__le32 *origptr, __le32 *updptr, int nr_inst);
+void kvm_update_va_mask(struct alt_instr *alt, __le32 *origptr,
+			__le32 *updptr, int nr_insti, int nr_alts);
 
 static inline unsigned long __kern_hyp_va(unsigned long v)
 {
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index a49930843784..f55afa0bbaa4 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -107,8 +107,8 @@ static u32 get_alt_insn(struct alt_instr *alt, __le32 *insnptr, __le32 *altinsnp
 	return insn;
 }
 
-static void patch_alternative(struct alt_instr *alt,
-			      __le32 *origptr, __le32 *updptr, int nr_inst)
+static void patch_alternative(struct alt_instr *alt, __le32 *origptr,
+			      __le32 *updptr, int nr_inst, int nr_alts)
 {
 	__le32 *replptr;
 	int i;
@@ -154,7 +154,7 @@ static void __apply_alternatives(void *alt_region, bool is_module)
 	struct alt_instr_cb *alt_cb_insn;
 
 	for (alt = region->begin; alt < region->end; alt++) {
-		int nr_inst;
+		int nr_inst, nr_alts;
 
 		/* Use ARM64_CB_PATCH as an unconditional patch */
 		if (alt->cpufeature < ARM64_CB_PATCH &&
@@ -174,12 +174,15 @@ static void __apply_alternatives(void *alt_region, bool is_module)
 
 		if (alt->cpufeature < ARM64_CB_PATCH) {
 			alt_cb = patch_alternative;
+			nr_alts = alt->alt_len / AARCH64_INSN_SIZE;
 		} else {
 			alt_cb_insn = ALT_REPL_PTR(alt);
 			alt_cb = offset_to_ptr(&alt_cb_insn->cb_offset);
+			nr_alts = (alt->alt_len - sizeof(*alt_cb_insn)) / AARCH64_INSN_SIZE;
+
 		}
 
-		alt_cb(alt, origptr, updptr, nr_inst);
+		alt_cb(alt, origptr, updptr, nr_inst, nr_alts);
 
 		if (!is_module) {
 			clean_dcache_range_nopatch((u64)origptr,
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 6ad715d67df8..c5489b4612c5 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -305,9 +305,8 @@ static int __init ssbd_cfg(char *buf)
 }
 early_param("ssbd", ssbd_cfg);
 
-void __init arm64_update_smccc_conduit(struct alt_instr *alt,
-				       __le32 *origptr, __le32 *updptr,
-				       int nr_inst)
+void __init arm64_update_smccc_conduit(struct alt_instr *alt, __le32 *origptr,
+				       __le32 *updptr, int nr_inst, int nr_alts)
 {
 	u32 insn;
 
@@ -327,9 +326,8 @@ void __init arm64_update_smccc_conduit(struct alt_instr *alt,
 	*updptr = cpu_to_le32(insn);
 }
 
-void __init arm64_enable_wa2_handling(struct alt_instr *alt,
-				      __le32 *origptr, __le32 *updptr,
-				      int nr_inst)
+void __init arm64_enable_wa2_handling(struct alt_instr *alt, __le32 *origptr,
+				      __le32 *updptr, int nr_inst, int nr_alts)
 {
 	BUG_ON(nr_inst != 1);
 	/*
diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
index c712a7376bc1..db7ba73e306a 100644
--- a/arch/arm64/kvm/va_layout.c
+++ b/arch/arm64/kvm/va_layout.c
@@ -114,8 +114,8 @@ static u32 compute_instruction(int n, u32 rd, u32 rn)
 	return insn;
 }
 
-void __init kvm_update_va_mask(struct alt_instr *alt,
-			       __le32 *origptr, __le32 *updptr, int nr_inst)
+void __init kvm_update_va_mask(struct alt_instr *alt, __le32 *origptr,
+			       __le32 *updptr, int nr_inst, int nr_alts)
 {
 	int i;
 
@@ -154,8 +154,8 @@ void __init kvm_update_va_mask(struct alt_instr *alt,
 void *__kvm_bp_vect_base;
 int __kvm_harden_el2_vector_slot;
 
-void kvm_patch_vector_branch(struct alt_instr *alt,
-			     __le32 *origptr, __le32 *updptr, int nr_inst)
+void kvm_patch_vector_branch(struct alt_instr *alt, __le32 *origptr,
+			     __le32 *updptr, int nr_inst, int nr_alts)
 {
 	u64 addr;
 	u32 insn;
-- 
2.19.2

[PATCH 3/5] arm64/alternative_cb: add support for alternative sequences

[Ard Biesheuvel]

Permit callback type alternatives to carry a sequence of alternative
instructions, and leave it up to the callback handler to decide how
to use them.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/alternative.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 987c1514183a..6b7726ee6b0a 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -156,7 +156,7 @@ static inline void apply_alternatives_module(void *start, size_t length) { }
 	.popsection
 	.pushsection .altinstr_replacement, "ax"
 663:	.word	\cb - .
-664:	.popsection
+	.popsection
 661:
 .endm
 
@@ -185,10 +185,21 @@ static inline void apply_alternatives_module(void *start, size_t length) { }
 	.org	. - (662b-661b) + (664b-663b)
 .endm
 
+.macro alternative_cb_alt
+	.set .Lasm_alt_mode, 1
+	.pushsection .altinstr_replacement, "ax"
+	.org	663b + AARCH64_INSN_SIZE
+.endm
+
 /*
  * Callback-based alternative epilogue
  */
 .macro alternative_cb_end
+	.if .Lasm_alt_mode==0
+	.pushsection .altinstr_replacement, "ax"
+	.org	663b + AARCH64_INSN_SIZE
+	.endif
+664:	.popsection
 662:
 .endm
 
-- 
2.19.2

[PATCH 4/5] arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions

[Ard Biesheuvel]

Use the enhanced alternative_cb implementation to reimplement the code
patching logic for DC CVAP instructions so that we don't fall back to
DV CVAC instructions on systems that require those to be upgraded to
DC CIVAC to work around silicon errata. At the same time, we don't want
to use DV CIVAC needlessly, since doing so may adversely affect
performance.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/assembler.h |  9 +++++----
 arch/arm64/kernel/cpu_errata.c     | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 6142402c2eb4..09c5a5452f60 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -390,11 +390,12 @@ alternative_else
 	dc	civac, \kaddr
 alternative_endif
 	.elseif	(\op == cvap)
-alternative_if ARM64_HAS_DCPOP
-	sys 3, c7, c12, 1, \kaddr	// dc cvap
-alternative_else
+alternative_cb arm64_handle_dc_cvap
+	dc	civac, \kaddr
+alternative_cb_alt
+	sys	3, c7, c12, 1, \kaddr	// dc cvap
 	dc	cvac, \kaddr
-alternative_endif
+alternative_cb_end
 	.else
 	dc	\op, \kaddr
 	.endif
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index c5489b4612c5..a63e362da307 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -755,3 +755,17 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 	{
 	}
 };
+
+asmlinkage void __init arm64_handle_dc_cvap(struct alt_instr *alt,
+					    __le32 *origptr, __le32 *updptr,
+					    int nr_inst, int nr_alts)
+{
+	struct alt_instr_cb *alt_insn = offset_to_ptr(&alt->alt_offset);
+
+	BUG_ON(nr_inst != 1 || nr_alts != 2);
+
+	if (cpus_have_cap(ARM64_HAS_DCPOP))
+		updptr[0] = alt_insn->insn[0];
+	else if (!cpus_have_cap(ARM64_WORKAROUND_CLEAN_CACHE))
+		updptr[0] = alt_insn->insn[1];
+}
-- 
2.19.2

[PATCH 5/5] arm64/mm: use string comparisons in dcache_by_line_op

[Ard Biesheuvel]

The GAS directives that are currently being used in dcache_by_line_op
rely on assembler behavior that is not documented, and probably not
guaranteed to produce the correct behavior going forward.

Currently, we end up with some undefined symbols in cache.o:

$ nm arch/arm64/mm/cache.o
         ...
         U civac
         ...
         U cvac
         U cvap
         U cvau

This is due to the fact that the comparisons used to select the
operation type in the dcache_by_line_op macro are comparing symbols
not strings, and even though it seems that GAS is doing the right
thing here (undefined symbols by the same name are equal to each
other), it seems unwise to rely on this.

So let's switch to conditional directives that are documented as
operating on strings. Since these cannot be combined like ordinary
arithmetic expressions, invert the comparison logic.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/assembler.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 09c5a5452f60..df3043e76e6a 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -383,21 +383,23 @@ alternative_endif
 	sub	\tmp2, \tmp1, #1
 	bic	\kaddr, \kaddr, \tmp2
 9998:
-	.if	(\op == cvau || \op == cvac)
+	.ifnc	\op, civac
+	.ifnc	\op, cvap
 alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
 	dc	\op, \kaddr
 alternative_else
 	dc	civac, \kaddr
 alternative_endif
-	.elseif	(\op == cvap)
+	.else
 alternative_cb arm64_handle_dc_cvap
 	dc	civac, \kaddr
 alternative_cb_alt
 	sys	3, c7, c12, 1, \kaddr	// dc cvap
 	dc	cvac, \kaddr
 alternative_cb_end
+	.endif
 	.else
-	dc	\op, \kaddr
+	dc	civac, \kaddr
 	.endif
 	add	\kaddr, \kaddr, \tmp1
 	cmp	\kaddr, \size
-- 
2.19.2

Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op

[Will Deacon]

Hi Ard,

Cheers for looking at this.

On Thu, Dec 06, 2018 at 04:57:34PM +0100, Ard Biesheuvel wrote:
> This fixes two issues in dcache_by_line_op: patch #4 fixes the logic
> that is applied when patching DC CVAP instructions, and patch #5 gets
> rid of some unintended undefined symbols resulting from incorrect use
> of conditional GAS directives.
> 
> Patches #1 to #3 are groundwork for #4.
> 
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Suzuki Poulose <suzuki.poulose@arm.com>
> Cc: Dave Martin <Dave.Martin@arm.com>
> 
> Ard Biesheuvel (5):
>   arm64/alternative_cb: move callback reference into replacements
>     section
>   arm64/alternative_cb: add nr_alts parameter to callback handlers
>   arm64/alternative_cb: add support for alternative sequences
>   arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions
>   arm64/mm: use string comparisons in dcache_by_line_op
> 
>  arch/arm64/include/asm/alternative.h | 54 +++++++++++---------
>  arch/arm64/include/asm/assembler.h   | 17 +++---
>  arch/arm64/include/asm/kvm_mmu.h     |  4 +-
>  arch/arm64/kernel/alternative.c      | 22 +++++---
>  arch/arm64/kernel/cpu_errata.c       | 24 ++++++---
>  arch/arm64/kvm/va_layout.c           |  8 +--
>  6 files changed, 79 insertions(+), 50 deletions(-)

Whilst I can see that this solves the problem, I do wonder whether the
additional infrastructure is really worth it. Couldn't we just do something
really simple like the diff below instead?

Will

--->8

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 953208267252..8dea015b6599 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -390,27 +390,38 @@ alternative_endif
  * 	size:		size of the region
  * 	Corrupts:	kaddr, size, tmp1, tmp2
  */
+	.macro __dcache_op_workaround_clean_cache, op, kaddr
+alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
+	dc	\op, \kaddr
+alternative_else
+	dc	civac, \kaddr
+alternative_endif
+	.endm
+
 	.macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
 	dcache_line_size \tmp1, \tmp2
 	add	\size, \kaddr, \size
 	sub	\tmp2, \tmp1, #1
 	bic	\kaddr, \kaddr, \tmp2
 9998:
-	.if	(\op == cvau || \op == cvac)
-alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
-	dc	\op, \kaddr
-alternative_else
-	dc	civac, \kaddr
-alternative_endif
-	.elseif	(\op == cvap)
+	.ifc	\op, cvau
+	__dcache_op_workaround_clean_cache \op, \kaddr
+	.else
+	.ifc	\op, cvac
+	__dcache_op_workaround_clean_cache \op, \kaddr
+	.else
+	.ifc	\op, cvap
 alternative_if ARM64_HAS_DCPOP
 	sys 3, c7, c12, 1, \kaddr	// dc cvap
-alternative_else
-	dc	cvac, \kaddr
-alternative_endif
+	b	9996f
+alternative_else_nop_endif
+	__dcache_op_workaround_clean_cache cvac, \kaddr
+9996:
 	.else
 	dc	\op, \kaddr
 	.endif
+	.endif
+	.endif
 	add	\kaddr, \kaddr, \tmp1
 	cmp	\kaddr, \size
 	b.lo	9998b

Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op

[Ard Biesheuvel]

On Fri, 7 Dec 2018 at 18:53, Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Ard,
>
> Cheers for looking at this.
>
> On Thu, Dec 06, 2018 at 04:57:34PM +0100, Ard Biesheuvel wrote:
> > This fixes two issues in dcache_by_line_op: patch #4 fixes the logic
> > that is applied when patching DC CVAP instructions, and patch #5 gets
> > rid of some unintended undefined symbols resulting from incorrect use
> > of conditional GAS directives.
> >
> > Patches #1 to #3 are groundwork for #4.
> >
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Suzuki Poulose <suzuki.poulose@arm.com>
> > Cc: Dave Martin <Dave.Martin@arm.com>
> >
> > Ard Biesheuvel (5):
> >   arm64/alternative_cb: move callback reference into replacements
> >     section
> >   arm64/alternative_cb: add nr_alts parameter to callback handlers
> >   arm64/alternative_cb: add support for alternative sequences
> >   arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions
> >   arm64/mm: use string comparisons in dcache_by_line_op
> >
> >  arch/arm64/include/asm/alternative.h | 54 +++++++++++---------
> >  arch/arm64/include/asm/assembler.h   | 17 +++---
> >  arch/arm64/include/asm/kvm_mmu.h     |  4 +-
> >  arch/arm64/kernel/alternative.c      | 22 +++++---
> >  arch/arm64/kernel/cpu_errata.c       | 24 ++++++---
> >  arch/arm64/kvm/va_layout.c           |  8 +--
> >  6 files changed, 79 insertions(+), 50 deletions(-)
>
> Whilst I can see that this solves the problem, I do wonder whether the
> additional infrastructure is really worth it. Couldn't we just do something
> really simple like the diff below instead?
>

That looks fine to me, although not as elegant :-) You are in a better
position to assess if the additional infrastructure may see other uses
in the near future.

In any case, I don't feel strongly about it, so pick whatever you feel
is most suitable. (I just spotted the undefined symbols when I was
playing with lld.)


> --->8
>
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 953208267252..8dea015b6599 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -390,27 +390,38 @@ alternative_endif
>   *     size:           size of the region
>   *     Corrupts:       kaddr, size, tmp1, tmp2
>   */
> +       .macro __dcache_op_workaround_clean_cache, op, kaddr
> +alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> +       dc      \op, \kaddr
> +alternative_else
> +       dc      civac, \kaddr
> +alternative_endif
> +       .endm
> +
>         .macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
>         dcache_line_size \tmp1, \tmp2
>         add     \size, \kaddr, \size
>         sub     \tmp2, \tmp1, #1
>         bic     \kaddr, \kaddr, \tmp2
>  9998:
> -       .if     (\op == cvau || \op == cvac)
> -alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> -       dc      \op, \kaddr
> -alternative_else
> -       dc      civac, \kaddr
> -alternative_endif
> -       .elseif (\op == cvap)
> +       .ifc    \op, cvau
> +       __dcache_op_workaround_clean_cache \op, \kaddr
> +       .else
> +       .ifc    \op, cvac
> +       __dcache_op_workaround_clean_cache \op, \kaddr
> +       .else
> +       .ifc    \op, cvap
>  alternative_if ARM64_HAS_DCPOP
>         sys 3, c7, c12, 1, \kaddr       // dc cvap
> -alternative_else
> -       dc      cvac, \kaddr
> -alternative_endif
> +       b       9996f
> +alternative_else_nop_endif
> +       __dcache_op_workaround_clean_cache cvac, \kaddr
> +9996:
>         .else
>         dc      \op, \kaddr
>         .endif
> +       .endif
> +       .endif
>         add     \kaddr, \kaddr, \tmp1
>         cmp     \kaddr, \size
>         b.lo    9998b

Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op

[Robin Murphy]

Hi Will,

On 07/12/2018 17:53, Will Deacon wrote:
> Hi Ard,
> 
> Cheers for looking at this.
> 
> On Thu, Dec 06, 2018 at 04:57:34PM +0100, Ard Biesheuvel wrote:
>> This fixes two issues in dcache_by_line_op: patch #4 fixes the logic
>> that is applied when patching DC CVAP instructions, and patch #5 gets
>> rid of some unintended undefined symbols resulting from incorrect use
>> of conditional GAS directives.
>>
>> Patches #1 to #3 are groundwork for #4.
>>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Suzuki Poulose <suzuki.poulose@arm.com>
>> Cc: Dave Martin <Dave.Martin@arm.com>
>>
>> Ard Biesheuvel (5):
>>    arm64/alternative_cb: move callback reference into replacements
>>      section
>>    arm64/alternative_cb: add nr_alts parameter to callback handlers
>>    arm64/alternative_cb: add support for alternative sequences
>>    arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions
>>    arm64/mm: use string comparisons in dcache_by_line_op
>>
>>   arch/arm64/include/asm/alternative.h | 54 +++++++++++---------
>>   arch/arm64/include/asm/assembler.h   | 17 +++---
>>   arch/arm64/include/asm/kvm_mmu.h     |  4 +-
>>   arch/arm64/kernel/alternative.c      | 22 +++++---
>>   arch/arm64/kernel/cpu_errata.c       | 24 ++++++---
>>   arch/arm64/kvm/va_layout.c           |  8 +--
>>   6 files changed, 79 insertions(+), 50 deletions(-)
> 
> Whilst I can see that this solves the problem, I do wonder whether the
> additional infrastructure is really worth it. Couldn't we just do something
> really simple like the diff below instead?

Given that there's really only one place we expect CVAP to be used, I 
reckon we could factor that out beforehand to make life even simpler, as 
below.

Robin.

----->8-----
diff --git a/arch/arm64/include/asm/assembler.h 
b/arch/arm64/include/asm/assembler.h
index 6142402c2eb4..8d88d3f1e90e 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -390,11 +390,7 @@ alternative_else
  	dc	civac, \kaddr
  alternative_endif
  	.elseif	(\op == cvap)
-alternative_if ARM64_HAS_DCPOP
  	sys 3, c7, c12, 1, \kaddr	// dc cvap
-alternative_else
-	dc	cvac, \kaddr
-alternative_endif
  	.else
  	dc	\op, \kaddr
  	.endif
diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
index 0c22ede52f90..98b17bee4f9d 100644
--- a/arch/arm64/mm/cache.S
+++ b/arch/arm64/mm/cache.S
@@ -212,6 +212,9 @@ ENDPROC(__dma_clean_area)
   *	- size    - size in question
   */
  ENTRY(__clean_dcache_area_pop)
+	alternative_if_not ARM64_HAS_DCPOP
+	b __clean_dcache_area_poc
+	alternative_else_nop_endif
  	dcache_by_line_op cvap, sy, x0, x1, x2, x3
  	ret
  ENDPIPROC(__clean_dcache_area_pop)
-----8<----

> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 953208267252..8dea015b6599 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -390,27 +390,38 @@ alternative_endif
>    * 	size:		size of the region
>    * 	Corrupts:	kaddr, size, tmp1, tmp2
>    */
> +	.macro __dcache_op_workaround_clean_cache, op, kaddr
> +alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> +	dc	\op, \kaddr
> +alternative_else
> +	dc	civac, \kaddr
> +alternative_endif
> +	.endm
> +
>   	.macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
>   	dcache_line_size \tmp1, \tmp2
>   	add	\size, \kaddr, \size
>   	sub	\tmp2, \tmp1, #1
>   	bic	\kaddr, \kaddr, \tmp2
>   9998:
> -	.if	(\op == cvau || \op == cvac)
> -alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> -	dc	\op, \kaddr
> -alternative_else
> -	dc	civac, \kaddr
> -alternative_endif
> -	.elseif	(\op == cvap)
> +	.ifc	\op, cvau
> +	__dcache_op_workaround_clean_cache \op, \kaddr
> +	.else
> +	.ifc	\op, cvac
> +	__dcache_op_workaround_clean_cache \op, \kaddr
> +	.else
> +	.ifc	\op, cvap
>   alternative_if ARM64_HAS_DCPOP
>   	sys 3, c7, c12, 1, \kaddr	// dc cvap
> -alternative_else
> -	dc	cvac, \kaddr
> -alternative_endif
> +	b	9996f
> +alternative_else_nop_endif
> +	__dcache_op_workaround_clean_cache cvac, \kaddr
> +9996:
>   	.else
>   	dc	\op, \kaddr
>   	.endif
> +	.endif
> +	.endif
>   	add	\kaddr, \kaddr, \tmp1
>   	cmp	\kaddr, \size
>   	b.lo	9998b
>