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@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> Cc: Suzuki Poulose <suzuki.poulose@xxxxxxx>
> Cc: Dave Martin <Dave.Martin@xxxxxxx>
> 
> 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