ARM: v7: get rid of boot time mini stack
v2 cover ARM: v7: get rid of boot time mini stack
The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, as the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.
So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.
Patch #1 adds a missing ISB. This patch is included separately so it can be backported if desired.
Patch #2 rewrites v7_invalidate_l1 so it only uses 5 registers (not counting lr which it must preserve as well)
Patch #3 updates the callers to use spare registers instead of the mini stack to stash the values that need to be preserved across the calls to v7_invalidate_l1.
Changes since v1:
- use correct stop condition in outer loop (cc not mi)
- bring back ENDPROC() in patch #3
- add Nico's ack
This version of the series was build/boot tested here:
https://kernelci.org/build/ardb/branch/for-kernelci/kernel/v5.11-rc7-3-gafc656039a99/
https://kernelci.org/test/job/ardb/branch/for-kernelci/kernel/v5.11-rc7-3-gafc656039a99/plan/baseline/
Ard Biesheuvel (3):
ARM: cache-v7: add missing ISB after cache level selection
ARM: cache-v7: refactor v7_invalidate_l1 to avoid clobbering r5/r6
ARM: cache-v7: get rid of mini-stack
arch/arm/include/asm/memory.h | 15 ----- arch/arm/mm/cache-v7.S | 58 ++++++++++---------- arch/arm/mm/proc-v7.S | 39 ++++++------- 3 files changed, 47 insertions(+), 65 deletions(-)
Signature · 7 lines
2.30.0 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 1/3 ARM: cache-v7: add missing ISB after cache level selection
A write to CCSELR needs to complete before its results can be observed via CCSIDR. So add a ISB to ensure that this is the case.
--- arch/arm/mm/cache-v7.S | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
arch/arm/mm/cache-v7.S +4 -3
@@ -38,9 +38,10 @@ icache_size: * procedures. */ ENTRY(v7_invalidate_l1) - mov r0, #0 - mcr p15, 2, r0, c0, c0, 0 - mrc p15, 1, r0, c0, c0, 0 + mov r0, #0 + mcr p15, 2, r0, c0, c0, 0 @ select L1 data cache in CSSELR + isb + mrc p15, 1, r0, c0, c0, 0 @ read cache geometry from CCSIDR movw r1, #0x7fff and r2, r1, r0, lsr #13
Signature · 7 lines
2.30.0 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 2/3 ARM: cache-v7: refactor v7_invalidate_l1 to avoid clobbering r5/r6
In reply to Ard BiesheuvelThe cache invalidation code in v7_invalidate_l1 can be tweaked to re-read the associativity from CCSIDR, and keep the set/way identifier component in a single register that is assigned in the outer loop. This way, we need 2 registers less.
Given that the number of sets is typically much larger than the associativity, rearrange the code so that the outer loop has the fewer number of iterations, ensuring that the re-read of CCSIDR only occurs a handful of times in practice.
Fix the whitespace while at it, and update the comment to indicate that this code is no longer a clone of anything else.
--- arch/arm/mm/cache-v7.S | 51 ++++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-)
arch/arm/mm/cache-v7.S +25 -26
@@ -33,9 +33,8 @@ icache_size: * processor. We fix this by performing an invalidate, rather than a * clean + invalidate, before jumping into the kernel. * - * This function is cloned from arch/arm/mach-tegra/headsmp.S, and needs - * to be called for both secondary cores startup and primary core resume - * procedures. + * This function needs to be called for both secondary cores startup and + * primary core resume procedures. */ ENTRY(v7_invalidate_l1) mov r0, #0
@@ -43,32 +42,32 @@ ENTRY(v7_invalidate_l1) isb mrc p15, 1, r0, c0, c0, 0 @ read cache geometry from CCSIDR - movw r1, #0x7fff - and r2, r1, r0, lsr #13 + movw r3, #0x3ff + and r3, r3, r0, lsr #3 @ 'Associativity' in CCSIDR[12:3] + clz r1, r3 @ WayShift + mov r2, #1 + mov r3, r3, lsl r1 @ NumWays-1 shifted into bits [31:...] + movs r1, r2, lsl r1 @ #1 shifted left by same amount + moveq r1, #1 @ r1 needs value > 0 even if only 1 way - movw r1, #0x3ff + and r2, r0, #0x7 + add r2, r2, #4 @ SetShift - and r3, r1, r0, lsr #3 @ NumWays - 1 - add r2, r2, #1 @ NumSets +1: movw r4, #0x7fff + and r0, r4, r0, lsr #13 @ 'NumSets' in CCSIDR[27:13] - and r0, r0, #0x7 - add r0, r0, #4 @ SetShift - - clz r1, r3 @ WayShift - add r4, r3, #1 @ NumWays -1: sub r2, r2, #1 @ NumSets-- - mov r3, r4 @ Temp = NumWays -2: subs r3, r3, #1 @ Temp-- - mov r5, r3, lsl r1 - mov r6, r2, lsl r0 - orr r5, r5, r6 @ Reg = (Temp<<WayShift)|(NumSets<<SetShift) - mcr p15, 0, r5, c7, c6, 2 - bgt 2b - cmp r2, #0 - bgt 1b - dsb st - isb - ret lr +2: mov r4, r0, lsl r2 @ NumSet << SetShift + orr r4, r4, r3 @ Reg = (Temp<<WayShift)|(NumSets<<SetShift) + mcr p15, 0, r4, c7, c6, 2 + subs r0, r0, #1 @ Set-- + bpl 2b + subs r3, r3, r1 @ Way-- + bcc 3f + mrc p15, 1, r0, c0, c0, 0 @ re-read cache geometry from CCSIDR + b 1b +3: dsb st + isb + ret lr ENDPROC(v7_invalidate_l1) /*
Signature · 7 lines
2.30.0 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 3/3 ARM: cache-v7: get rid of mini-stack
In reply to Ard BiesheuvelNow that we have reduced the number of registers that we need to preserve when calling v7_invalidate_l1 from the boot code, we can use scratch registers to preserve the remaining ones, and get rid of the mini stack entirely. This works around any issues regarding cache behavior in relation to the uncached accesses to this memory, which is hard to get right in the general case (i.e., both bare metal and under virtualization)
While at it, switch v7_invalidate_l1 to using ip as a scratch register instead of r4. This makes the function AAPCS compliant, and removes the need to stash r4 in ip across the call.
--- arch/arm/include/asm/memory.h | 15 -------- arch/arm/mm/cache-v7.S | 10 ++--- arch/arm/mm/proc-v7.S | 39 +++++++++----------- 3 files changed, 23 insertions(+), 41 deletions(-)
arch/arm/include/asm/memory.h +0 -15
@@ -150,21 +150,6 @@ extern unsigned long vectors_base;*/ #define PLAT_PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) -#ifdef CONFIG_XIP_KERNEL -/* -* When referencing data in RAM from the XIP region in a relative manner -* with the MMU off, we need the relative offset between the two physical -* addresses. The macro below achieves this, which is: -* __pa(v_data) - __xip_pa(v_text) -*/ -#define PHYS_RELATIVE (v_data, v_text) \ - (((v_data) - PAGE_OFFSET + PLAT_PHYS_OFFSET) - \ - ((v_text) - XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR) + \ - CONFIG_XIP_PHYS_ADDR)) -#else -#define PHYS_RELATIVE (v_data, v_text) ((v_data) - (v_text)) -#endif -#ifndef __ASSEMBLY__/*
arch/arm/mm/cache-v7.S +5 -5
@@ -53,12 +53,12 @@ ENTRY(v7_invalidate_l1) and r2, r0, #0x7 add r2, r2, #4 @ SetShift -1: movw r4, #0x7fff - and r0, r4, r0, lsr #13 @ 'NumSets' in CCSIDR[27:13] +1: movw ip, #0x7fff + and r0, ip, r0, lsr #13 @ 'NumSets' in CCSIDR[27:13] -2: mov r4, r0, lsl r2 @ NumSet << SetShift - orr r4, r4, r3 @ Reg = (Temp<<WayShift)|(NumSets<<SetShift) - mcr p15, 0, r4, c7, c6, 2 +2: mov ip, r0, lsl r2 @ NumSet << SetShift + orr ip, ip, r3 @ Reg = (Temp<<WayShift)|(NumSets<<SetShift) + mcr p15, 0, ip, c7, c6, 2 subs r0, r0, #1 @ Set-- bpl 2b subs r3, r3, r1 @ Way--
arch/arm/mm/proc-v7.S +18 -21
@@ -256,6 +256,20 @@ ENDPROC(cpu_pj4b_do_resume) #endif + @ + @ Invoke the v7_invalidate_l1() function, which adheres to the AAPCS + @ rules, and so it may corrupt registers that we need to preserve. + @ + .macro do_invalidate_l1 + mov r6, r1 + mov r7, r2 + mov r10, lr + bl v7_invalidate_l1 @ corrupts {r0-r3, ip, lr} + mov r1, r6 + mov r2, r7 + mov lr, r10 + .endm + /* * __v7_setup *
@@ -277,6 +291,7 @@ __v7_ca5mp_setup: __v7_ca9mp_setup: __v7_cr7mp_setup: __v7_cr8mp_setup: + do_invalidate_l1 mov r10, #(1 << 0) @ Cache/TLB ops broadcasting b 1f __v7_ca7mp_setup:
@@ -284,13 +299,9 @@ __v7_ca12mp_setup: __v7_ca15mp_setup: __v7_b15mp_setup: __v7_ca17mp_setup: + do_invalidate_l1 mov r10, #0 -1: adr r0, __v7_setup_stack_ptr - ldr r12, [r0] - add r12, r12, r0 @ the local stack - stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 - bl v7_invalidate_l1 - ldmia r12, {r1-r6, lr} +1: #ifdef CONFIG_SMP orr r10, r10, #(1 << 6) @ Enable SMP/nAMP mode ALT_SMP(mrc p15, 0, r0, c1, c0, 1)
@@ -471,12 +482,7 @@ __v7_pj4b_setup: #endif /* CONFIG_CPU_PJ4B */ __v7_setup: - adr r0, __v7_setup_stack_ptr - ldr r12, [r0] - add r12, r12, r0 @ the local stack - stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 - bl v7_invalidate_l1 - ldmia r12, {r1-r6, lr} + do_invalidate_l1 __v7_setup_cont: and r0, r9, #0xff000000 @ ARM?
@@ -548,17 +554,8 @@ __errata_finish: orr r0, r0, r6 @ set them THUMB( orr r0, r0, #1 << 30 ) @ Thumb exceptions ret lr @ return to head.S:__ret - - .align 2 -__v7_setup_stack_ptr: - .word PHYS_RELATIVE(__v7_setup_stack, .) ENDPROC(__v7_setup) - .bss - .align 2 -__v7_setup_stack: - .space 4 * 7 @ 7 registers - __INITDATA .weak cpu_v7_bugs_init
Signature · 7 lines
2.30.0 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Russell King - ARM Linux admin
maintainerRe: [PATCH v2 3/3] ARM: cache-v7: get rid of mini-stack
Quoted 11 lines · Ard Biesheuvel
Now that we have reduced the number of registers that we need to preserve when calling
v7_invalidate_l1from the boot code, we can use scratch registers to preserve the remaining ones, and get rid of the mini stack entirely. This works around any issues regarding cache behavior in relation to the uncached accesses to this memory, which is hard to get right in the general case (i.e., both bare metal and under virtualization)While at it, switch
v7_invalidate_l1to using ip as a scratch register instead of r4. This makes the function AAPCS compliant, and removes the need to stash r4 in ip across the call.
You don't mention that we only do this for MP capable cores, which is in itself quite a big change - maybe that change should be a separate commit?
Signature · 7 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v2 3/3] ARM: cache-v7: get rid of mini-stack
Quoted 10 lines · Russell King - ARM Linux admin
Quoted 8 lines · Ard Biesheuvel
Now that we have reduced the number of registers that we need to preserve when calling
v7_invalidate_l1from the boot code, we can use scratch registers to preserve the remaining ones, and get rid of the mini stack entirely. This works around any issues regarding cache behavior in relation to the uncached accesses to this memory, which is hard to get right in the general case (i.e., both bare metal and under virtualization)
Quoted 8 lines
Quoted 3 lines
While at it, switch
v7_invalidate_l1to using ip as a scratch register instead of r4. This makes the function AAPCS compliant, and removes the need to stash r4 in ip across the call.You don't mention that we only do this for MP capable cores, which is in itself quite a big change - maybe that change should be a separate commit?
I'm not quite sure I understand what you are referring to here. All call sites are updated to use the macro to call v7_invalidate_l1(), so I don't think the UP-only and SMP-capable setup() paths are treated any differently.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 22 lines · Ard Biesheuvel
The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to
v7_invalidate_l1that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, as the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.So let's address this issue, by removing the need for this stack to exist in the first place:
v7_invalidate_l1can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.Patch #1 adds a missing ISB. This patch is included separately so it can be backported if desired.
Patch #2 rewrites
v7_invalidate_l1so it only uses 5 registers (not counting lr which it must preserve as well)
Quoted 8 lines
Patch #3 updates the callers to use spare registers instead of the mini stack to stash the values that need to be preserved across the calls to
v7_invalidate_l1.Changes since v1:
- use correct stop condition in outer loop (cc not mi)
- bring backENDPROC()in patch #3
- add Nico's ack
This looks good to me, so FWIW:
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel