From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:11:02
This series adds a work around for systems with mismatched {I,D}-cache
line sizes. When a thread of execution gets migrated to a different CPU,
the cache line size it had cached could be larger than that of the new
CPU. This could cause data corruption issues. We work around this by
- Dynamically patching the kernel to use the smallest line size on the
system (from the CPU feature infrastructure)
- Trapping the userspace access to CTR_EL0 (by clearing SCTLR_EL1.UCT) and
emulating it with the system wide safe value of CTR.
The series also adds support for alternative code patching of adrp
instructions by adjusting the PC-relative address offset to reflect
the new PC.
The series has been tested on Juno with a hack to forced enabling
of the capability.
Applies on aarch64: for-next/core. The tree is avaiable at :
git://linux-arm.org/linux-skp.git ctr-emulation
Suzuki K Poulose (8):
arm64: Set the safe value for L1 icache policy
arm64: Use consistent naming for errata handling
arm64: Rearrange CPU errata workaround checks
arm64: insn: Add helpers for adrp offsets
arm64: alternative: Add support for patching adrp instructions
arm64: Introduce raw_{d,i}cache_line_size
arm64: Refactor sysinstr exception handling
arm64: Work around systems with mismatched cache line sizes
arch/arm64/include/asm/assembler.h | 45 +++++++++++++++++--
arch/arm64/include/asm/cpufeature.h | 14 +++---
arch/arm64/include/asm/esr.h | 56 ++++++++++++++++++++++++
arch/arm64/include/asm/insn.h | 4 ++
arch/arm64/include/asm/sysreg.h | 1 +
arch/arm64/kernel/alternative.c | 13 ++++++
arch/arm64/kernel/asm-offsets.c | 2 +
arch/arm64/kernel/cpu_errata.c | 26 ++++++++++-
arch/arm64/kernel/cpufeature.c | 44 ++++++++++++++-----
arch/arm64/kernel/cpuinfo.c | 2 -
arch/arm64/kernel/hibernate-asm.S | 2 +-
arch/arm64/kernel/insn.c | 13 ++++++
arch/arm64/kernel/relocate_kernel.S | 2 +-
arch/arm64/kernel/smp.c | 8 +++-
arch/arm64/kernel/traps.c | 87 ++++++++++++++++++++++++++-----------
15 files changed, 264 insertions(+), 55 deletions(-)
--
2.7.4
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:11:43
adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.
Cc: Will Deacon <redacted>
Cc: Marc Zyngier <redacted>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/kernel/alternative.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:11:45
Systems with differing CPU i-cache/d-cache line sizes can cause
problems with the cache management by software when the execution
is migrated from one to another. Usually, the application reads
the cache size on a CPU and then uses that length to perform cache
operations. However, if it gets migrated to another CPU with a smaller
cache line size, things could go completely wrong. To prevent such
cases, always use the smallest cache line size among the CPUs. The
kernel CPU feature infrastructure already keeps track of the safe
value for all CPUID registers including CTR. This patch works around
the problem by :
For kernel, dynamically patch the kernel to read the cache size
from the system wide copy of CTR_EL0.
For applications, trap read accesses to CTR_EL0 (by clearing the SCTLR.UCT)
and emulate the mrs instruction to return the system wide safe value
of CTR_EL0.
For faster access (i.e, avoiding to lookup the system wide value of CTR_EL0
via read_system_reg), we keep track of the pointer to table entry for
CTR_EL0 in the CPU feature infrastructure.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/assembler.h | 25 +++++++++++++++++++++++--
arch/arm64/include/asm/cpufeature.h | 4 +++-
arch/arm64/include/asm/esr.h | 8 ++++++++
arch/arm64/include/asm/sysreg.h | 1 +
arch/arm64/kernel/asm-offsets.c | 2 ++
arch/arm64/kernel/cpu_errata.c | 22 ++++++++++++++++++++++
arch/arm64/kernel/cpufeature.c | 9 +++++++++
arch/arm64/kernel/traps.c | 14 ++++++++++++++
8 files changed, 82 insertions(+), 3 deletions(-)
@@ -216,6 +216,21 @@ lr .req x30 // link register.macrommid,rd,rnldr\rd,[\rn,#MM_CONTEXT_ID].endm+/*+*read_ctr-readCTR_EL0.Ifthesystemhasmismatched+*cachelinesizes,providethesystemwidesafevalue.+*/+.macroread_ctr,reg+alternative_if_notARM64_MISMATCHED_CACHE_LINE_SIZE+mrs\reg,ctr_el0// read CTR+nop+nop+alternative_else+ldr_l\reg,sys_ctr_ftr// Read system wide safe CTR value+ldr\reg,[\reg,#ARM64_FTR_SYSVAL]// from sys_ctr_ftr->sys_val+alternative_endif+.endm+/**raw_dcache_line_size-gettheminimumD-cachelinesizeonthisCPU
@@ -232,7 +247,10 @@ lr .req x30 // link register*dcache_line_size-getthesafeD-cachelinesizeacrossallCPUs*/.macrodcache_line_size,reg,tmp-raw_dcache_line_size\reg,\tmp+read_ctr\tmp+ubfm\tmp,\tmp,#16,#19// cache line size encoding+mov\reg,#4// bytes per word+lsl\reg,\reg,\tmp// actual cache line size.endm/*
@@ -250,7 +268,10 @@ lr .req x30 // link register*icache_line_size-getthesafeI-cachelinesizeacrossallCPUs*/.macroicache_line_size,reg,tmp-raw_icache_line_size\reg,\tmp+read_ctr\tmp+and\tmp,\tmp,#0xf// cache line size encoding+mov\reg,#4// bytes per word+lsl\reg,\reg,\tmp// actual cache line size.endm/*
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:12:13
Right now we trap some of the user space data cache operations
based on a few Errata (ARM 819472, 826319, 827319 and 824069).
We need to trap userspace access to CTR_EL0, if we detect mismatched
cache line size. Since both these traps share the EC, refactor
the handler a little bit to make it a bit more reader friendly.
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/esr.h | 48 +++++++++++++++++++++++++++++
arch/arm64/kernel/traps.c | 73 ++++++++++++++++++++++++++++----------------
2 files changed, 95 insertions(+), 26 deletions(-)
@@ -447,36 +447,29 @@ void cpu_enable_cache_maint_trap(void *__unused):"=r"(res)\:"r"(address),"i"(-EFAULT))-asmlinkagevoid__exceptiondo_sysinstr(unsignedintesr,structpt_regs*regs)+staticvoiduser_cache_maint_handler(unsignedintesr,structpt_regs*regs){unsignedlongaddress;-intret;+intrt=(esr&ESR_ELx_SYS64_ISS_RT_MASK)>>ESR_ELx_SYS64_ISS_RT_SHIFT;+intcrm=(esr&ESR_ELx_SYS64_ISS_CRm_MASK)>>ESR_ELx_SYS64_ISS_CRm_SHIFT;+intret=0;-/* if this is a write with: Op0=1, Op2=1, Op1=3, CRn=7 */-if((esr&0x01fffc01)==0x0012dc00){-intrt=(esr>>5)&0x1f;-intcrm=(esr>>1)&0x0f;+address=(rt==31)?0:regs->regs[rt];-address=(rt==31)?0:regs->regs[rt];--switch(crm){-case11:/* DC CVAU, gets promoted */-__user_cache_maint("dc civac",address,ret);-break;-case10:/* DC CVAC, gets promoted */-__user_cache_maint("dc civac",address,ret);-break;-case14:/* DC CIVAC */-__user_cache_maint("dc civac",address,ret);-break;-case5:/* IC IVAU */-__user_cache_maint("ic ivau",address,ret);-break;-default:-force_signal_inject(SIGILL,ILL_ILLOPC,regs,0);-return;-}-}else{+switch(crm){+caseESR_ELx_SYS64_ISS_CRm_DC_CVAU:/* DC CVAU, gets promoted */+__user_cache_maint("dc civac",address,ret);+break;+caseESR_ELx_SYS64_ISS_CRm_DC_CVAC:/* DC CVAC, gets promoted */+__user_cache_maint("dc civac",address,ret);+break;+caseESR_ELx_SYS64_ISS_CRm_DC_CIVAC:/* DC CIVAC */+__user_cache_maint("dc civac",address,ret);+break;+caseESR_ELx_SYS64_ISS_CRm_IC_IVAU:/* IC IVAU */+__user_cache_maint("ic ivau",address,ret);+break;+default:force_signal_inject(SIGILL,ILL_ILLOPC,regs,0);return;}
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:12:36
On systems with mismatched i/d cache min line sizes, we need to use
the smallest size possible across all CPUs. This will be done by fetching
the system wide safe value from CPU feature infrastructure.
However the some special users(e.g kexec, hibernate) would need the line
size on the CPU (rather than the system wide), when the system wide
feature may not be accessible. Provide another helper which will fetch
cache line size on the current CPU.
Cc: James Morse <james.morse@arm.com>
Cc: Geoff Levand <geoff@infradead.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/assembler.h | 24 ++++++++++++++++++++----
arch/arm64/kernel/hibernate-asm.S | 2 +-
arch/arm64/kernel/relocate_kernel.S | 2 +-
3 files changed, 22 insertions(+), 6 deletions(-)
@@ -218,9 +218,10 @@ lr .req x30 // link register.endm/*-*dcache_line_size-gettheminimumD-cachelinesizefromtheCTRregister.+*raw_dcache_line_size-gettheminimumD-cachelinesizeonthisCPU+*fromtheCTRregister.*/-.macrodcache_line_size,reg,tmp+.macroraw_dcache_line_size,reg,tmpmrs\tmp,ctr_el0// read CTRubfm\tmp,\tmp,#16,#19// cache line size encodingmov\reg,#4// bytes per word
@@ -228,9 +229,17 @@ lr .req x30 // link register.endm/*-*icache_line_size-gettheminimumI-cachelinesizefromtheCTRregister.+*dcache_line_size-getthesafeD-cachelinesizeacrossallCPUs*/-.macroicache_line_size,reg,tmp+.macrodcache_line_size,reg,tmp+raw_dcache_line_size\reg,\tmp+.endm++/*+*raw_icache_line_size-gettheminimumI-cachelinesizeonthisCPU+*fromtheCTRregister.+*/+.macroraw_icache_line_size,reg,tmpmrs\tmp,ctr_el0// read CTRand\tmp,\tmp,#0xf// cache line size encodingmov\reg,#4// bytes per word
@@ -238,6 +247,13 @@ lr .req x30 // link register.endm/*+*icache_line_size-getthesafeI-cachelinesizeacrossallCPUs+*/+.macroicache_line_size,reg,tmp+raw_icache_line_size\reg,\tmp+.endm++/**tcr_set_idmap_t0sz-updateTCR.T0SZsothatwecanloadtheIDmap*/.macrotcr_set_idmap_t0sz,valreg,tmpreg
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:13:07
Right now we use 0 as the safe value for CTR_EL0:L1Ip, which is
not defined at the moment. The safer value for the L1Ip should be
the weakest of the policies, which happens to be AIVIVT. While at it,
fix the comment about safe_val.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 2 +-
arch/arm64/kernel/cpufeature.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
@@ -63,7 +63,7 @@ struct arm64_ftr_bits {enumftr_typetype;u8shift;u8width;-s64safe_val;/* safe value for discrete features */+s64safe_val;/* safe value for FTR_EXACT features */};/*
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:13:26
Right now we run through the work around checks on a CPU
from __cpuinfo_store_cpu. There are some problems with that:
1) We initialise the system wide CPU feature registers only after the
Boot CPU updates its cpuinfo. Now, if a work around depends on the
variance of a CPU ID feature (e.g, check for Cache Line size mismatch),
we have no way of performing it cleanly for the boot CPU.
2) It is out of place, invoked from __cpuinfo_store_cpu() in cpuinfo.c. It
is not an obvious place for that.
This patch rearranges the CPU specific capability(aka work around) checks.
1) At the moment we use verify_local_cpu_capabilities() to check if a new
CPU has all the system advertised features. Use this for the secondary CPUs
to perform the work around check. For that we rename
verify_local_cpu_capabilities() => check_local_cpu_capabilities()
which:
If the system wide capabilities haven't been initialised (i.e, the CPU
is activated at the boot), update the system wide detected work arounds.
Otherwise (i.e a CPU hotplugged in later) verify that this CPU conforms to the
system wide capabilities.
2) Boot CPU updates the work arounds from smp_prepare_boot_cpu() after we have
initialised the system wide CPU feature values.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 4 ++--
arch/arm64/kernel/cpufeature.c | 30 ++++++++++++++++++++----------
arch/arm64/kernel/cpuinfo.c | 2 --
arch/arm64/kernel/smp.c | 8 +++++++-
4 files changed, 29 insertions(+), 15 deletions(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:13:40
Adds helpers for decoding/encoding the PC relative addresses for adrp.
This will be used for handling dynamic patching of 'adrp' instructions
in alternative code patching.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/insn.h | 4 ++++
arch/arm64/kernel/insn.c | 13 +++++++++++++
2 files changed, 17 insertions(+)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2016-08-18 13:13:43
This is a cosmetic change to rename the functions dealing with
the errata work arounds to be more consistent with their naming.
1) check_local_cpu_errata() => update_cpu_errata_workarounds()
check_local_cpu_errata() actually updates the system's errata work
arounds. So rename it to reflect the same.
2) verify_local_cpu_errata() => verify_local_cpu_errata_workarounds()
Use errata_workarounds instead of _errata.
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 4 ++--
arch/arm64/kernel/cpu_errata.c | 4 ++--
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kernel/cpuinfo.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
From: Marc Zyngier <hidden> Date: 2016-08-18 14:47:50
Hi Suzuki,
On 18/08/16 14:10, Suzuki K Poulose wrote:
quoted hunk
Adds helpers for decoding/encoding the PC relative addresses for adrp.
This will be used for handling dynamic patching of 'adrp' instructions
in alternative code patching.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/insn.h | 4 ++++
arch/arm64/kernel/insn.c | 13 +++++++++++++
2 files changed, 17 insertions(+)
I'm a bit bothered by this one. We end-up with both
aarch64_insn_is_adr_adrp() *and* aarch64_insn_is_adrp() (and their
respective getters).
How about dropping adr_adrp, and explicitly having adr and adrp? There
is only two users in the tree, so that should be easy to address.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2016-08-19 00:57:03
On 18/08/16 15:47, Marc Zyngier wrote:
Hi Suzuki,
On 18/08/16 14:10, Suzuki K Poulose wrote:
quoted
Adds helpers for decoding/encoding the PC relative addresses for adrp.
This will be used for handling dynamic patching of 'adrp' instructions
in alternative code patching.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/insn.h | 4 ++++
arch/arm64/kernel/insn.c | 13 +++++++++++++
2 files changed, 17 insertions(+)
On Thu, 2016-08-18 at 14:10 +0100, Suzuki K Poulose wrote:
quoted hunk
On systems with mismatched i/d cache min line sizes, we need to use
the smallest size possible across all CPUs. This will be done by fetching
the system wide safe value from CPU feature infrastructure.
However the some special users(e.g kexec, hibernate) would need the line
size on the CPU (rather than the system wide), when the system wide
feature may not be accessible. Provide another helper which will fetch
cache line size on the current CPU.
Cc: James Morse <james.morse@arm.com>
Cc: Geoff Levand <geoff@infradead.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
?arch/arm64/include/asm/assembler.h??| 24 ++++++++++++++++++++----
?arch/arm64/kernel/hibernate-asm.S???|??2 +-
?arch/arm64/kernel/relocate_kernel.S |??2 +-
?3 files changed, 22 insertions(+), 6 deletions(-)
? .endm
?
?/*
- * dcache_line_size - get the minimum D-cache line size from the CTR register.
+ * raw_dcache_line_size - get the minimum D-cache line size on this CPU
+ * from the CTR register.
? */
- .macro dcache_line_size, reg, tmp
+ .macro raw_dcache_line_size, reg, tmp
? mrs \tmp, ctr_el0 // read CTR
? ubfm \tmp, \tmp, #16, #19 // cache line size encoding
? mov \reg, #4 // bytes per word
Since this is just renaming dcache_line_size to raw_dcache_line_size,
and for kexec's relocate_kernel we need to know about the CPU we are
running on, this part of the change looks good.
Reviewed by: Geoff Levand [off-list ref]
From: Will Deacon <hidden> Date: 2016-08-22 10:00:43
On Thu, Aug 18, 2016 at 02:10:30PM +0100, Suzuki K Poulose wrote:
On systems with mismatched i/d cache min line sizes, we need to use
the smallest size possible across all CPUs. This will be done by fetching
the system wide safe value from CPU feature infrastructure.
However the some special users(e.g kexec, hibernate) would need the line
size on the CPU (rather than the system wide), when the system wide
feature may not be accessible. Provide another helper which will fetch
cache line size on the current CPU.
Why are these users "special"? Using a smaller line size shouldn't affect
correctness, and I don't see kexec and hibernate as being performance
critical in their cache maintenance.
Will
From: Will Deacon <hidden> Date: 2016-08-22 11:19:24
On Thu, Aug 18, 2016 at 02:10:29PM +0100, Suzuki K Poulose wrote:
quoted hunk
adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.
Cc: Will Deacon <redacted>
Cc: Marc Zyngier <redacted>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/kernel/alternative.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
I wonder if we shouldn't have a catch-all for any instructions performing
PC-relative operations here, because silent corruption of the instruction
stream is pretty horrible. What other instructions are there? ADR, LDR
(literal), ... ?
Will
On 18 August 2016 at 15:10, Suzuki K Poulose [off-list ref] wrote:
quoted hunk
adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.
Cc: Will Deacon <redacted>
Cc: Marc Zyngier <redacted>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/kernel/alternative.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
Are orig_offset and new_offset guaranteed to be equal modulo 4 KB?
Otherwise, you will have to track down and patch the associated :lo12:
add/ldr instruction as well.
From: Will Deacon <hidden> Date: 2016-08-22 12:53:45
On Thu, Aug 18, 2016 at 02:10:31PM +0100, Suzuki K Poulose wrote:
quoted hunk
Right now we trap some of the user space data cache operations
based on a few Errata (ARM 819472, 826319, 827319 and 824069).
We need to trap userspace access to CTR_EL0, if we detect mismatched
cache line size. Since both these traps share the EC, refactor
the handler a little bit to make it a bit more reader friendly.
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/esr.h | 48 +++++++++++++++++++++++++++++
arch/arm64/kernel/traps.c | 73 ++++++++++++++++++++++++++++----------------
2 files changed, 95 insertions(+), 26 deletions(-)
@@ -109,6 +109,54 @@((ESR_ELx_EC_BRK64<<ESR_ELx_EC_SHIFT)|ESR_ELx_IL|\((imm)&0xffff))+/* ISS field definitions for System instruction traps */
Can you add a similar comment for the ESR_ELx_* encodings that we already
have, please? Unfortunately, we've not namespaced things, so the
data/instruction abort encodings are described as e.g. ESR_ELx_ISV.
From: Will Deacon <hidden> Date: 2016-08-22 13:02:27
On Thu, Aug 18, 2016 at 02:10:32PM +0100, Suzuki K Poulose wrote:
Systems with differing CPU i-cache/d-cache line sizes can cause
problems with the cache management by software when the execution
is migrated from one to another. Usually, the application reads
the cache size on a CPU and then uses that length to perform cache
operations. However, if it gets migrated to another CPU with a smaller
cache line size, things could go completely wrong. To prevent such
cases, always use the smallest cache line size among the CPUs. The
kernel CPU feature infrastructure already keeps track of the safe
value for all CPUID registers including CTR. This patch works around
the problem by :
For kernel, dynamically patch the kernel to read the cache size
from the system wide copy of CTR_EL0.
Is it only CTR that is mismatched in practice, or do we need to worry
about DCZID_EL0 too?
quoted hunk
For applications, trap read accesses to CTR_EL0 (by clearing the SCTLR.UCT)
and emulate the mrs instruction to return the system wide safe value
of CTR_EL0.
For faster access (i.e, avoiding to lookup the system wide value of CTR_EL0
via read_system_reg), we keep track of the pointer to table entry for
CTR_EL0 in the CPU feature infrastructure.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/assembler.h | 25 +++++++++++++++++++++++--
arch/arm64/include/asm/cpufeature.h | 4 +++-
arch/arm64/include/asm/esr.h | 8 ++++++++
arch/arm64/include/asm/sysreg.h | 1 +
arch/arm64/kernel/asm-offsets.c | 2 ++
arch/arm64/kernel/cpu_errata.c | 22 ++++++++++++++++++++++
arch/arm64/kernel/cpufeature.c | 9 +++++++++
arch/arm64/kernel/traps.c | 14 ++++++++++++++
8 files changed, 82 insertions(+), 3 deletions(-)
@@ -216,6 +216,21 @@ lr .req x30 // link register.macrommid,rd,rnldr\rd,[\rn,#MM_CONTEXT_ID].endm+/*+*read_ctr-readCTR_EL0.Ifthesystemhasmismatched+*cachelinesizes,providethesystemwidesafevalue.+*/+.macroread_ctr,reg+alternative_if_notARM64_MISMATCHED_CACHE_LINE_SIZE+mrs\reg,ctr_el0// read CTR+nop+nop+alternative_else+ldr_l\reg,sys_ctr_ftr// Read system wide safe CTR value+ldr\reg,[\reg,#ARM64_FTR_SYSVAL]// from sys_ctr_ftr->sys_val+alternative_endif+.endm+/**raw_dcache_line_size-gettheminimumD-cachelinesizeonthisCPU
@@ -232,7 +247,10 @@ lr .req x30 // link register*dcache_line_size-getthesafeD-cachelinesizeacrossallCPUs*/.macrodcache_line_size,reg,tmp-raw_dcache_line_size\reg,\tmp+read_ctr\tmp+ubfm\tmp,\tmp,#16,#19// cache line size encoding+mov\reg,#4// bytes per word+lsl\reg,\reg,\tmp// actual cache line size.endm/*
@@ -250,7 +268,10 @@ lr .req x30 // link register*icache_line_size-getthesafeI-cachelinesizeacrossallCPUs*/.macroicache_line_size,reg,tmp-raw_icache_line_size\reg,\tmp+read_ctr\tmp+and\tmp,\tmp,#0xf// cache line size encoding+mov\reg,#4// bytes per word+lsl\reg,\reg,\tmp// actual cache line size.endm/*
Whilst this is correct, I wonder if there's any advantage in reporting a
*larger* size to userspace and avoid incurring additional trap overhead?
Any idea what sort of size typical JITs are using?
Will
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2016-08-23 09:18:02
On 22/08/16 12:45, Ard Biesheuvel wrote:
On 18 August 2016 at 15:10, Suzuki K Poulose [off-list ref] wrote:
quoted
adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.
Cc: Will Deacon <redacted>
Cc: Marc Zyngier <redacted>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/kernel/alternative.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
Are orig_offset and new_offset guaranteed to be equal modulo 4 KB?
Otherwise, you will have to track down and patch the associated :lo12:
add/ldr instruction as well.
We are modifying the alternative instruction to accommodate for the new PC,
where this instruction will be executed from, while the referenced symbol
remains the same. Hence the associated :lo12: doesn't change. Does that
address your concern ? Or did I miss something ?
Suzuki
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2016-08-23 09:40:30
On 22/08/16 12:19, Will Deacon wrote:
On Thu, Aug 18, 2016 at 02:10:29PM +0100, Suzuki K Poulose wrote:
quoted
adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.
Cc: Will Deacon <redacted>
Cc: Marc Zyngier <redacted>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/kernel/alternative.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
I wonder if we shouldn't have a catch-all for any instructions performing
PC-relative operations here, because silent corruption of the instruction
Correct, which is what happened initially when I didn't have the adrp handling ;-).
stream is pretty horrible. What other instructions are there? ADR, LDR
(literal), ... ?
From a quick look, all the instructions under "Load register (literal)" :
i.e,
LDR (literal) for GPR/FP_SIMD/32bit/64bit
LDRSW (literal)
PRFM (literal)
and Data processing instructions - immediate group with PC-relative addressing:
ADR, ADRP
I will add a check to catch the unsupported instructions in the alternative code.
Thanks
Suzuki
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2016-08-23 10:08:17
On 22/08/16 11:00, Will Deacon wrote:
On Thu, Aug 18, 2016 at 02:10:30PM +0100, Suzuki K Poulose wrote:
quoted
On systems with mismatched i/d cache min line sizes, we need to use
the smallest size possible across all CPUs. This will be done by fetching
the system wide safe value from CPU feature infrastructure.
However the some special users(e.g kexec, hibernate) would need the line
size on the CPU (rather than the system wide), when the system wide
feature may not be accessible. Provide another helper which will fetch
cache line size on the current CPU.
Why are these users "special"? Using a smaller line size shouldn't affect
With the alternate patched code, we refer to the kernel data structure for
CTR value. At least for kexec, it may overwrite the existing kernel image/data where
our data was stored and could possibly end up in receiving corrupted code.
For all special cases where it is ensured that the code is run on a
single CPU and will not be migrated to another CPU they can rely on
the raw value of CTR, hence the change.
correctness, and I don't see kexec and hibernate as being performance
critical in their cache maintenance.
Its not for performance, but for the safety.
Suzuki
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2016-08-23 10:19:52
On 22/08/16 13:53, Will Deacon wrote:
On Thu, Aug 18, 2016 at 02:10:31PM +0100, Suzuki K Poulose wrote:
quoted
Right now we trap some of the user space data cache operations
based on a few Errata (ARM 819472, 826319, 827319 and 824069).
We need to trap userspace access to CTR_EL0, if we detect mismatched
cache line size. Since both these traps share the EC, refactor
the handler a little bit to make it a bit more reader friendly.
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/esr.h | 48 +++++++++++++++++++++++++++++
arch/arm64/kernel/traps.c | 73 ++++++++++++++++++++++++++++----------------
2 files changed, 95 insertions(+), 26 deletions(-)
@@ -109,6 +109,54 @@((ESR_ELx_EC_BRK64<<ESR_ELx_EC_SHIFT)|ESR_ELx_IL|\((imm)&0xffff))+/* ISS field definitions for System instruction traps */
Can you add a similar comment for the ESR_ELx_* encodings that we already
have, please? Unfortunately, we've not namespaced things, so the
data/instruction abort encodings are described as e.g. ESR_ELx_ISV.
On 23 August 2016 at 11:16, Suzuki K Poulose [off-list ref] wrote:
On 22/08/16 12:45, Ard Biesheuvel wrote:
quoted
On 18 August 2016 at 15:10, Suzuki K Poulose [off-list ref]
wrote:
quoted
adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.
Cc: Will Deacon <redacted>
Cc: Marc Zyngier <redacted>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/kernel/alternative.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm64/kernel/alternative.c
b/arch/arm64/kernel/alternative.c
index d2ee1b2..71c6962 100644
*insnptr, u32 *altinsnptr)
offset = target - (unsigned long)insnptr;
insn = aarch64_set_branch_offset(insn, offset);
}
+ } else if (aarch64_insn_is_adrp(insn)) {
+ s32 orig_offset, new_offset;
+ unsigned long target;
+
+ /*
+ * If we're replacing an adrp instruction, which uses
PC-relative
+ * immediate addressing, adjust the offset to reflect the
new
+ * PC. adrp operates on 4K aligned addresses.
+ */
+ orig_offset = aarch64_insn_adrp_get_offset(insn);
+ target = ((unsigned long)altinsnptr & ~0xfffUL) +
orig_offset;
+ new_offset = target - ((unsigned long)insnptr &
~0xfffUL);
+ insn = aarch64_insn_adrp_set_offset(insn, new_offset);
Are orig_offset and new_offset guaranteed to be equal modulo 4 KB?
Otherwise, you will have to track down and patch the associated :lo12:
add/ldr instruction as well.
We are modifying the alternative instruction to accommodate for the new PC,
where this instruction will be executed from, while the referenced symbol
remains the same. Hence the associated :lo12: doesn't change. Does that
address your concern ? Or did I miss something ?
Ah, of course. Yes, that should work fine, given that the symbol stays
in place, and so its offset into the containing 4 KB page does not
change either.
Thanks,
Ard.
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2016-08-24 13:23:07
On 22/08/16 14:02, Will Deacon wrote:
On Thu, Aug 18, 2016 at 02:10:32PM +0100, Suzuki K Poulose wrote:
quoted
Systems with differing CPU i-cache/d-cache line sizes can cause
problems with the cache management by software when the execution
is migrated from one to another. Usually, the application reads
the cache size on a CPU and then uses that length to perform cache
operations. However, if it gets migrated to another CPU with a smaller
cache line size, things could go completely wrong. To prevent such
cases, always use the smallest cache line size among the CPUs. The
kernel CPU feature infrastructure already keeps track of the safe
value for all CPUID registers including CTR. This patch works around
the problem by :
For kernel, dynamically patch the kernel to read the cache size
from the system wide copy of CTR_EL0.
Is it only CTR that is mismatched in practice, or do we need to worry
about DCZID_EL0 too?
A mismatched DCZID_EL0 is quite possible. However, there is no way to
trap accesses to DCZID_EL0. Rather, we can trap DC ZVA if we clear
SCTLR_EL1.DZE. But then clearing the SCTLR_EL1.DZE implies reading DCZID.DZP
returns 1, indicating DC ZVA is not supported. So if a proper application
checks the DZP before issuing a DC ZVA, we may never be able to emulate it.
Or in other words, if there is a mismatch, the work around is to disable
the DC ZVA operations (which could possibly affect existing (incorrect) userspace
applications assuming DC ZVA is supported without checking the DZP bit).
Whilst this is correct, I wonder if there's any advantage in reporting a
*larger* size to userspace and avoid incurring additional trap overhead?
Combining the trapping of user space dc operations for Errata work around for
clean cache, we could possibly report a larger size and emulate it properly
in the kernel. But I think that can be a enhancement on top of this series.
Any idea what sort of size typical JITs are using?
I have no clue about it. I have Cc-ed Rodolph and Stuart, who may have better
idea about the JIT's usage.
Suzuki