* [PATCH 1/5] arm64/alternative_cb: move callback reference into replacements section
2018-12-06 15:57 [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Ard Biesheuvel
@ 2018-12-06 15:57 ` Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 2/5] arm64/alternative_cb: add nr_alts parameter to callback handlers Ard Biesheuvel
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 15:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Ard Biesheuvel, Robin Murphy, Will Deacon,
Catalin Marinas, Marc Zyngier, Suzuki Poulose, Dave Martin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/5] arm64/alternative_cb: add nr_alts parameter to callback handlers
2018-12-06 15:57 [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 1/5] arm64/alternative_cb: move callback reference into replacements section Ard Biesheuvel
@ 2018-12-06 15:57 ` Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 3/5] arm64/alternative_cb: add support for alternative sequences Ard Biesheuvel
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 15:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Ard Biesheuvel, Robin Murphy, Will Deacon,
Catalin Marinas, Marc Zyngier, Suzuki Poulose, Dave Martin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/5] arm64/alternative_cb: add support for alternative sequences
2018-12-06 15:57 [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 1/5] arm64/alternative_cb: move callback reference into replacements section Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 2/5] arm64/alternative_cb: add nr_alts parameter to callback handlers Ard Biesheuvel
@ 2018-12-06 15:57 ` Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 4/5] arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions Ard Biesheuvel
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 15:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Ard Biesheuvel, Robin Murphy, Will Deacon,
Catalin Marinas, Marc Zyngier, Suzuki Poulose, Dave Martin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/5] arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions
2018-12-06 15:57 [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Ard Biesheuvel
` (2 preceding siblings ...)
2018-12-06 15:57 ` [PATCH 3/5] arm64/alternative_cb: add support for alternative sequences Ard Biesheuvel
@ 2018-12-06 15:57 ` Ard Biesheuvel
2018-12-06 15:57 ` [PATCH 5/5] arm64/mm: use string comparisons in dcache_by_line_op Ard Biesheuvel
2018-12-07 17:53 ` [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Will Deacon
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 15:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Ard Biesheuvel, Robin Murphy, Will Deacon,
Catalin Marinas, Marc Zyngier, Suzuki Poulose, Dave Martin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/5] arm64/mm: use string comparisons in dcache_by_line_op
2018-12-06 15:57 [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Ard Biesheuvel
` (3 preceding siblings ...)
2018-12-06 15:57 ` [PATCH 4/5] arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions Ard Biesheuvel
@ 2018-12-06 15:57 ` Ard Biesheuvel
2018-12-07 17:53 ` [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Will Deacon
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 15:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Ard Biesheuvel, Robin Murphy, Will Deacon,
Catalin Marinas, Marc Zyngier, Suzuki Poulose, Dave Martin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op
2018-12-06 15:57 [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Ard Biesheuvel
` (4 preceding siblings ...)
2018-12-06 15:57 ` [PATCH 5/5] arm64/mm: use string comparisons in dcache_by_line_op Ard Biesheuvel
@ 2018-12-07 17:53 ` Will Deacon
2018-12-07 18:01 ` Ard Biesheuvel
2018-12-10 14:50 ` Robin Murphy
5 siblings, 2 replies; 9+ messages in thread
From: Will Deacon @ 2018-12-07 17:53 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel, linux-arm-kernel, Robin Murphy, Catalin Marinas,
Marc Zyngier, Suzuki Poulose, Dave Martin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op
2018-12-07 17:53 ` [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Will Deacon
@ 2018-12-07 18:01 ` Ard Biesheuvel
2018-12-10 14:50 ` Robin Murphy
1 sibling, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-12-07 18:01 UTC (permalink / raw)
To: Will Deacon
Cc: Linux Kernel Mailing List, linux-arm-kernel, Robin Murphy,
Catalin Marinas, Marc Zyngier, Suzuki K. Poulose, Dave Martin
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op
2018-12-07 17:53 ` [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op Will Deacon
2018-12-07 18:01 ` Ard Biesheuvel
@ 2018-12-10 14:50 ` Robin Murphy
1 sibling, 0 replies; 9+ messages in thread
From: Robin Murphy @ 2018-12-10 14:50 UTC (permalink / raw)
To: Will Deacon, Ard Biesheuvel
Cc: linux-kernel, linux-arm-kernel, Catalin Marinas, Marc Zyngier,
Suzuki Poulose, Dave Martin
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
>
^ permalink raw reply related [flat|nested] 9+ messages in thread