In a couple of places, the early boot code uses non-standard argument,
return value or return address registers when calling functions. This makes
the code more complicated than it needs to be, which was not a problem in the
early days, but with all the recent changes for KASLR, hibernate etc, it
makes sense to clean this up once and for all. This code removes all uses of
callee saved registers on the secondary boot and resume paths, and on the
primary boot path, it only leaves the necessary ones, and documents them
explicitly in patch #7.
I will leave it to the honourable arm64 maintainers to decide if any of
these improvements weigh up against the churn, given that this code has
already been updated numerous times over the past couple of kernel versions.
NOTE: this series applies onto today's for-next/core with fixes/core merged
on top, since it depends on Mark's commit fd363bd417ddb610 ("arm64: avoid TLB
conflict with CONFIG_RANDOMIZE_BASE")
Changes since v2:
- dropped patch that gets rid of x25/x26 as pgdir pointers, it has been merged
into for-next/core in the mean time
- fixed commit log of #1 to indicate since when the comment that it fixes had
been incorrect
- merged changes relating to the use of x27 by __enable_mmu() into a single
patch (#4)
- added Mark's R-b to all patches
Patch #1 fixes some style issues in sleep.S with no functional changes.
Patch #2 removes the use of x20 between el2_setup() and set_cpu_boot_mode_flag()
Patch #3 moves the part of the KASLR processing that resides in __enable_mmu()
into primary_switch() (which is a more suitable place, given that only the
primary boot path ever invokes it)
Patch #4 replaces the special x27 return address of __enable_mmu() with x30/lr.
Given that we can no longer dereference literals containing virtual addresses,
all callers have already been updated to return from __enable_mmu() back to the
idmap before performing a literal load + jump. Using x30 instead of x27 allows
us to merge the code that executes before __enable_mmu() with the code that
executes after it, and to change the invocation of __enable_mmu() itself into a
simple bl instruction.
Patch #5 removes the 'global' x24 register in head.S, containing __PHYS_OFFSET
Patch #6 removes the use of x28 in __primary_switched(), and replaces it with
an ordinary stack frame to preserve the return address.
Ard Biesheuvel (7):
arm64: kernel: fix style issues in sleep.S
arm64: kernel: use ordinary return/argument register for el2_setup()
arm64: head.S: move KASLR processing out of __enable_mmu()
arm64: kernel: use x30 for __enable_mmu return address
arm64: kernel: drop use of x24 from primary boot path
arm64: head.S: use ordinary stack frame for __primary_switched()
arm64: head.S: document the use of callee saved registers
arch/arm64/kernel/head.S | 147 +++++++++++---------
arch/arm64/kernel/sleep.S | 27 ++--
2 files changed, 94 insertions(+), 80 deletions(-)
--
2.7.4
This fixes a number of style issues in sleep.S. No functional changes are
intended:
- replace absolute literal references with relative references in
__cpu_suspend_enter(), which executes from its virtual address
- replace explicit lr assignment plus branch with bl in cpu_resume(), which
aligns it with stext() and secondary_startup()
- don't export _cpu_resume()
- use adr_l for mpidr_hash reference, and fix the incorrect accompanying
comment, which has been out of date since commit cabe1c81ea5be983 ("arm64:
Change cpu_resume() to enable mmu early then access sleep_sp by va")
- replace leading spaces with tabs, and add a bit of whitespace for
readability
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/sleep.S | 21 ++++++++++----------
1 file changed, 10 insertions(+), 11 deletions(-)
@@ -113,16 +111,17 @@ ENDPROC(_resume_switched).ltorg.popsection-ENTRY(_cpu_resume)+_cpu_resume:mrsx1,mpidr_el1-adrpx8,mpidr_hash-addx8,x8,#:lo12:mpidr_hash // x8 = struct mpidr_hash phys address-/*retrievempidr_hashmemberstocomputethehash*/+adr_lx8,mpidr_hash//x8=structmpidr_hashvirtaddress++/*retrievempidr_hashmemberstocomputethehash*/ldrx2,[x8,#MPIDR_HASH_MASK]ldpw3,w4,[x8,#MPIDR_HASH_SHIFTS]ldpw5,w6,[x8,#(MPIDR_HASH_SHIFTS + 8)]compute_mpidr_hashx7,x3,x4,x5,x6,x1,x2-/*x7containshashindex,let's use it to grab context pointer */++/*x7containshashindex,let's use it to grab context pointer */ldr_lx0,sleep_save_stashldrx0,[x0,x7,lsl#3]addx29,x0,#SLEEP_STACK_DATA_CALLEE_REGS
The function el2_setup() passes its return value in register w20, and
in the two cases where the caller actually cares about this return value,
it is passed into set_cpu_boot_mode_flag() [almost] directly, which
expects its input in w20 as well.
So there is no reason to use a 'special' callee saved register here, but
we can simply follow the PCS for return value and first argument,
respectively.
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
@@ -488,7 +488,7 @@ CPU_LE( bic x0, x0, #(1 << 25) ) // Clear the EE bit for EL2CPU_BE(orrx0,x0,#(3 << 24) ) // Set the EE and E0E bits for EL1CPU_LE(bicx0,x0,#(3 << 24) ) // Clear the EE and E0E bits for EL1msrsctlr_el1,x0-movw20,#BOOT_CPU_MODE_EL1 // This cpu booted in EL1+movw0,#BOOT_CPU_MODE_EL1 // This cpu booted in EL1isbret
@@ -584,7 +584,7 @@ CPU_LE( movk x0, #0x30d0, lsl #16 ) // Clear EE and E0E on LE systemscbzx2,install_el2_stub-movw20,#BOOT_CPU_MODE_EL2 // This CPU booted in EL2+movw0,#BOOT_CPU_MODE_EL2 // This CPU booted in EL2isbret
@@ -599,7 +599,7 @@ install_el2_stub:PSR_MODE_EL1h)msrspsr_el2,x0msrelr_el2,lr-movw20,#BOOT_CPU_MODE_EL2 // This CPU booted in EL2+movw0,#BOOT_CPU_MODE_EL2 // This CPU booted in EL2eretENDPROC(el2_setup)
@@ -649,7 +649,7 @@ ENTRY(__early_cpu_boot_status)*coresarehelduntilwe're ready for them to initialise.*/ENTRY(secondary_holding_pen)-blel2_setup//DroptoEL1,w20=cpu_boot_mode+blel2_setup//DroptoEL1,w0=cpu_boot_modeblset_cpu_boot_mode_flagmrsx0,mpidr_el1mov_qx1,MPIDR_HWID_BITMASK
The KASLR processing is only used by the primary boot path, and
complements the processing that takes place in __primary_switch().
Move the two parts together, to make the code easier to understand.
Also, fix up a minor whitespace issue.
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 72 ++++++++++++--------
1 file changed, 42 insertions(+), 30 deletions(-)
@@ -726,7 +724,6 @@ ENDPROC(__secondary_switched)*Ifitisn't, park the CPU*/ENTRY(__enable_mmu)-mrsx22,sctlr_el1//preserveoldSCTLR_EL1valuemrsx1,ID_AA64MMFR0_EL1ubfxx2,x1,#ID_AA64MMFR0_TGRAN_SHIFT, 4cmpx2,#ID_AA64MMFR0_TGRAN_SUPPORTED
Using x27 for passing to __enable_mmu what is essentially the return
address makes the code look more complicated than it needs to be. So
switch to x30/lr, and update the secondary and cpu_resume call sites to
simply call __enable_mmu as an ordinary function, with a bl instruction.
This requires the callers to be covered by .idmap.text.
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 21 +++++++-------------
arch/arm64/kernel/sleep.S | 8 ++------
2 files changed, 9 insertions(+), 20 deletions(-)
@@ -716,9 +716,9 @@ ENDPROC(__secondary_switched)*EnabletheMMU.**x0=SCTLR_EL1valueforturningontheMMU.-*x27=*virtual*addresstojumptouponcompletion*-*Otherregistersdependonthefunctioncalleduponcompletion.+*Returnstothecallerviax30/lr.Thisrequiresthecallertobecovered+*bythe.idmap.textsection.**ChecksiftheselectedgranulesizeissupportedbytheCPU.*Ifitisn't, park the CPU
Keeping __PHYS_OFFSET in x24 is actually less clear than simply taking
the value of __PHYS_OFFSET using an adrp instruction in the three places
that we need it. So change that.
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
Instead of stashing the value of the link register in x28 before setting
up the stack and calling into C code, create an ordinary PCS compatible
stack frame so that we can push the return address onto the stack.
Since exception handlers require a stack as well, assign the stack pointer
register before installing the vector table.
Note that this accounts for the difference between THREAD_START_SP and
THREAD_SIZE, given that the stack pointer is always decremented before
calling into any C code.
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
Now that the only remaining occurrences of the use of callee saved
registers are on the primary boot path, add a comment to the code
which register is used for what.
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 10 ++++++++++
1 file changed, 10 insertions(+)
On 31 August 2016 at 12:05, Ard Biesheuvel [off-list ref] wrote:
This fixes a number of style issues in sleep.S. No functional changes are
intended:
- replace absolute literal references with relative references in
__cpu_suspend_enter(), which executes from its virtual address
- replace explicit lr assignment plus branch with bl in cpu_resume(), which
aligns it with stext() and secondary_startup()
- don't export _cpu_resume()
Will,
Apologies, I sent out the wrong version of this patch in v3. Not
exporting _cpu_resume() breaks hibernate, so I dropped it from v2
(which is the version that Mark reviewed), but I reintroduced it by
accident.
Please let me know how you would like to proceed, i.e., put a fixup on
top, or resend.
Thanks,
Ard.
quoted hunk
- use adr_l for mpidr_hash reference, and fix the incorrect accompanying
comment, which has been out of date since commit cabe1c81ea5be983 ("arm64:
Change cpu_resume() to enable mmu early then access sleep_sp by va")
- replace leading spaces with tabs, and add a bit of whitespace for
readability
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/sleep.S | 21 ++++++++++----------
1 file changed, 10 insertions(+), 11 deletions(-)
@@ -113,16 +111,17 @@ ENDPROC(_resume_switched).ltorg.popsection-ENTRY(_cpu_resume)+_cpu_resume:mrsx1,mpidr_el1-adrpx8,mpidr_hash-addx8,x8,#:lo12:mpidr_hash // x8 = struct mpidr_hash phys address-/*retrievempidr_hashmemberstocomputethehash*/+adr_lx8,mpidr_hash//x8=structmpidr_hashvirtaddress++/*retrievempidr_hashmemberstocomputethehash*/ldrx2,[x8,#MPIDR_HASH_MASK]ldpw3,w4,[x8,#MPIDR_HASH_SHIFTS]ldpw5,w6,[x8,#(MPIDR_HASH_SHIFTS + 8)]compute_mpidr_hashx7,x3,x4,x5,x6,x1,x2-/*x7containshashindex,let's use it to grab context pointer */++/*x7containshashindex,let's use it to grab context pointer */ldr_lx0,sleep_save_stashldrx0,[x0,x7,lsl#3]addx29,x0,#SLEEP_STACK_DATA_CALLEE_REGS--
From: Will Deacon <hidden> Date: 2016-09-05 09:02:44
Hi Ard,
On Sat, Sep 03, 2016 at 09:08:13PM +0100, Ard Biesheuvel wrote:
On 31 August 2016 at 12:05, Ard Biesheuvel [off-list ref] wrote:
quoted
This fixes a number of style issues in sleep.S. No functional changes are
intended:
- replace absolute literal references with relative references in
__cpu_suspend_enter(), which executes from its virtual address
- replace explicit lr assignment plus branch with bl in cpu_resume(), which
aligns it with stext() and secondary_startup()
- don't export _cpu_resume()
Apologies, I sent out the wrong version of this patch in v3. Not
exporting _cpu_resume() breaks hibernate, so I dropped it from v2
(which is the version that Mark reviewed), but I reintroduced it by
accident.
No problem, these things happen.
Please let me know how you would like to proceed, i.e., put a fixup on
top, or resend.
I already pushed this out on for-next/core, so please send a fixup on top.
I could've sworn I did an allmodconfig build test before pushing out, but
apparently not.
Will