This v2 series is a followup to the single patch #1, whose v1 I sent out
about a week ago.
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 th
primary boot path, it only leaves the necessary ones, and documents them
explicitly in patch #9.
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.
Adding James to cc since patch #6 may conflict with this hibernate/
debug-pagealloc series [0], to which I replied that merging .idmap.text
with .mmuoff.text would be a worthwhile simplification.
Patch #1 removes the de facto requirement of __enable_mmu() that the
addresses of idmap_pg_dir and swapper_pg_dir must to be passed in register
x25 and x26, respectively. (v2: added Mark's ack)
Patch #2 fixes some style issues in sleep.S with no functional changes.
Patch #3 removes the use of x20 between el2_setup and set_cpu_boot_mode_flag()
Patch #4 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 #5 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 been updated to return from __enable_mmu() back to the
idmap before performing a literal load + jump, this will allow us to simplify
the code in subsequent patches.
Patch #6 merges the code that executes before __enable_mmu() with the code
that executes after it, and changes the invocation of __enable_mmu() itself
into a simple bl instruction.
Patch #7 removes the 'global' x24 register in head.S, containing __PHYS_OFFSET
Patch #8 removes the use of x28 in __primary_switched(), and replaces it with
an ordinary stack frame to preserve the return address.
[0] http://marc.info/?l=linux-arm-kernel&m=147128126002457
Ard Biesheuvel (9):
arm64: kernel: get rid of x25 and x26 with 'global' scope
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: call __enable_mmu as an ordinary function for secondary/resume
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 | 170 +++++++++++---------
arch/arm64/kernel/sleep.S | 29 ++--
2 files changed, 105 insertions(+), 94 deletions(-)
--
2.7.4
Currently, x25 and x26 hold the physical addresses of idmap_pg_dir
and swapper_pg_dir, respectively, when running early boot code. But
having registers with 'global' scope in files that contain different
sections with different lifetimes, and that are called by different
CPUs at different times is a bit messy, especially since stashing the
values does not buy us anything in terms of code size or clarity.
So simply replace each reference to x25 or x26 with an adrp instruction
referring to idmap_pg_dir or swapper_pg_dir directly.
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 28 +++++++++-----------
arch/arm64/kernel/sleep.S | 2 --
2 files changed, 13 insertions(+), 17 deletions(-)
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()
- use adr_l for mpidr_hash reference, and fix the incorrect accompanying
comment
- replace leading spaces with tabs, and add a bit of whitespace for
readability
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/sleep.S | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
@@ -115,14 +113,15 @@ ENDPROC(_resume_switched)ENTRY(_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.
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)
@@ -637,7 +637,7 @@ ENTRY(__boot_cpu_mode)*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 in __enable_mmu() 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.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 66 ++++++++++++--------
1 file changed, 39 insertions(+), 27 deletions(-)
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
in preparation of further simplifications, switch to x30/lr.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 12 +++++-------
arch/arm64/kernel/sleep.S | 2 +-
2 files changed, 6 insertions(+), 8 deletions(-)
@@ -710,9 +710,9 @@ ENTRY(__early_cpu_boot_status)*EnabletheMMU.**x0=SCTLR_EL1valueforturningontheMMU.-*x27=*virtual*addresstojumptouponcompletion*-*Otherregistersdependonthefunctioncalleduponcompletion.+*Returnstothecallerviax30/lr.Thisrequiresthecallertobecovered+*bythe.idmap.textsection.**ChecksiftheselectedgranulesizeissupportedbytheCPU.*Ifitisn't, park the CPU
This updates 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.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 14 ++++++--------
arch/arm64/kernel/sleep.S | 10 +++-------
2 files changed, 9 insertions(+), 15 deletions(-)
@@ -634,6 +634,7 @@ ENTRY(__boot_cpu_mode)*Thisprovidesa"holding pen"forplatformstoholdallsecondary*coresarehelduntilwe're ready for them to initialise.*/+.pushsection".idmap.text","ax"ENTRY(secondary_holding_pen)blel2_setup//DroptoEL1,w0=cpu_boot_modeblset_cpu_boot_mode_flag
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.
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 stach 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.
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.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 10 ++++++++++
1 file changed, 10 insertions(+)
On Wed, Aug 24, 2016 at 04:35:59PM +0200, Ard Biesheuvel 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()
- use adr_l for mpidr_hash reference, and fix the incorrect accompanying
comment
It looks like the comment was true until commit cabe1c81ea5be983 ("arm64:
Change cpu_resume() to enable mmu early then access sleep_sp by va"), which may
be worth calling out here.
- replace leading spaces with tabs, and add a bit of whitespace for
readability
Signed-off-by: Ard Biesheuvel <redacted>
Regardless of the above, this looks like a good improvement to me, for all
points mentioned in the commit message. FWIW:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
@@ -115,14 +113,15 @@ ENDPROC(_resume_switched)ENTRY(_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
On Wed, Aug 24, 2016 at 04:36:00PM +0200, Ard Biesheuvel wrote:
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.
Signed-off-by: Ard Biesheuvel <redacted>
Make sense to me. FWIW:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
Hi,
On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
The KASLR processing in __enable_mmu() 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.
As a heads-up, while reviewing this I spotted an existing issue [1]. I'd meant
to comment so when posting that patch, but in my hubris from making
git-send-email work I forgot to do so. :/
[...]
quoted hunk
@@ -770,11 +748,11 @@ __no_granule_support: 1: wfe wfi- b 1b+ b 1b ENDPROC(__no_granule_support)
Unrelated change? Perhaps it's worth putting all the whitespace fixup in a
preparatory patch?
[...]
+__primary_switch:
+#ifdef CONFIG_RANDOMIZE_BASE
+ mov x19, x0 // preserve new SCTLR_EL1 value
+ mrs x20, sctlr_el1 // preserve old SCTLR_EL1 value
+#endif
+
+ adr x27, 0f
+ b __enable_mmu
As we do elsewhere, it's probably worth a comment on the line with the ADR into
x27, mentioning that __enable_mmu will branch there.
... or perhaps we should just have __enable_mmu return to the LR like a normal
AAPCS function, place the switch routines in the idmap, and use the idiomatic
sequence:
__thing_switch:
bl __enable_mmu
ldr xN, =__thing
blr xN
[...]
+ /*
+ * If we return here, we have a KASLR displacement in x23 which we need
+ * to take into account by discarding the current kernel mapping and
+ * creating a new one.
+ */
+ msr sctlr_el1, x20 // disable the MMU
+ isb
+ bl __create_page_tables // recreate kernel mapping
As per the issue I mentioned above [1], here we also need:
tlbi vmalle1
dsb nsh
... in order to avoid TLB conflicts and other issues resulting from BBM
violations.
+
+ msr sctlr_el1, x19 // re-enable the MMU
+ isb
+ ic iallu // flush instructions fetched
+ dsb nsh // via old mapping
+ isb
On 24 August 2016 at 22:36, Mark Rutland [off-list ref] wrote:
Hi,
On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
quoted
The KASLR processing in __enable_mmu() 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.
As a heads-up, while reviewing this I spotted an existing issue [1]. I'd meant
to comment so when posting that patch, but in my hubris from making
git-send-email work I forgot to do so. :/
[...]
quoted
@@ -770,11 +748,11 @@ __no_granule_support: 1: wfe wfi- b 1b+ b 1b ENDPROC(__no_granule_support)
Unrelated change? Perhaps it's worth putting all the whitespace fixup in a
preparatory patch?
[...]
I couldn't resist. It's the only occurrence in this series apart from #2
quoted
+__primary_switch:
+#ifdef CONFIG_RANDOMIZE_BASE
+ mov x19, x0 // preserve new SCTLR_EL1 value
+ mrs x20, sctlr_el1 // preserve old SCTLR_EL1 value
+#endif
+
+ adr x27, 0f
+ b __enable_mmu
As we do elsewhere, it's probably worth a comment on the line with the ADR into
x27, mentioning that __enable_mmu will branch there.
... or perhaps we should just have __enable_mmu return to the LR like a normal
AAPCS function, place the switch routines in the idmap, and use the idiomatic
sequence:
__thing_switch:
bl __enable_mmu
ldr xN, =__thing
blr xN
[...]
Yes, that is more or less the point of the two subsequent patches.
quoted
+ /*
+ * If we return here, we have a KASLR displacement in x23 which we need
+ * to take into account by discarding the current kernel mapping and
+ * creating a new one.
+ */
+ msr sctlr_el1, x20 // disable the MMU
+ isb
+ bl __create_page_tables // recreate kernel mapping
As per the issue I mentioned above [1], here we also need:
tlbi vmalle1
dsb nsh
... in order to avoid TLB conflicts and other issues resulting from BBM
violations.
On Wed, Aug 24, 2016 at 09:36:10PM +0100, Mark Rutland wrote:
On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
quoted
+__primary_switch:
+#ifdef CONFIG_RANDOMIZE_BASE
+ mov x19, x0 // preserve new SCTLR_EL1 value
+ mrs x20, sctlr_el1 // preserve old SCTLR_EL1 value
+#endif
+
+ adr x27, 0f
+ b __enable_mmu
As we do elsewhere, it's probably worth a comment on the line with the ADR into
x27, mentioning that __enable_mmu will branch there.
... or perhaps we should just have __enable_mmu return to the LR like a normal
AAPCS function, place the switch routines in the idmap, and use the idiomatic
sequence:
__thing_switch:
bl __enable_mmu
ldr xN, =__thing
blr xN
... and now I see that this is what subsequent patches do ;)
Is it possible to first AAPCS-ify __enable_mmu (with shuffling of callers as
above) in one patch, prior to this? That would avoid introducing the unusual 0f
label above, and the temporary x30 usage in a subsequent patch.
Thanks,
Mark.
On 24 August 2016 at 21:46, Mark Rutland [off-list ref] wrote:
On Wed, Aug 24, 2016 at 09:36:10PM +0100, Mark Rutland wrote:
quoted
On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
quoted
+__primary_switch:
+#ifdef CONFIG_RANDOMIZE_BASE
+ mov x19, x0 // preserve new SCTLR_EL1 value
+ mrs x20, sctlr_el1 // preserve old SCTLR_EL1 value
+#endif
+
+ adr x27, 0f
+ b __enable_mmu
As we do elsewhere, it's probably worth a comment on the line with the ADR into
x27, mentioning that __enable_mmu will branch there.
... or perhaps we should just have __enable_mmu return to the LR like a normal
AAPCS function, place the switch routines in the idmap, and use the idiomatic
sequence:
__thing_switch:
bl __enable_mmu
ldr xN, =__thing
blr xN
... and now I see that this is what subsequent patches do ;)
Is it possible to first AAPCS-ify __enable_mmu (with shuffling of callers as
above) in one patch, prior to this?
Yes, but that would result in an __enable_mmu() that needs to stash
the link register value, and essentially returns twice in the KASLR
case. As an intermediate step working towards the result after the
series, I think the adr + label above is the lesser evil
That would avoid introducing the unusual 0f
label above, and the temporary x30 usage in a subsequent patch.
Thanks,
Mark.
On Thu, Aug 25, 2016 at 02:59:51PM +0100, Ard Biesheuvel wrote:
On 24 August 2016 at 21:46, Mark Rutland [off-list ref] wrote:
quoted
On Wed, Aug 24, 2016 at 09:36:10PM +0100, Mark Rutland wrote:
quoted
On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
quoted
+__primary_switch:
+#ifdef CONFIG_RANDOMIZE_BASE
+ mov x19, x0 // preserve new SCTLR_EL1 value
+ mrs x20, sctlr_el1 // preserve old SCTLR_EL1 value
+#endif
+
+ adr x27, 0f
+ b __enable_mmu
As we do elsewhere, it's probably worth a comment on the line with the ADR into
x27, mentioning that __enable_mmu will branch there.
... or perhaps we should just have __enable_mmu return to the LR like a normal
AAPCS function, place the switch routines in the idmap, and use the idiomatic
sequence:
__thing_switch:
bl __enable_mmu
ldr xN, =__thing
blr xN
... and now I see that this is what subsequent patches do ;)
Is it possible to first AAPCS-ify __enable_mmu (with shuffling of callers as
above) in one patch, prior to this?
Yes, but that would result in an __enable_mmu() that needs to stash
the link register value, and essentially returns twice in the KASLR
case.
Ah, good point. I had missed that.
As an intermediate step working towards the result after the series, I
think the adr + label above is the lesser evil
Yes, it probably is.
I'll try to flip back into review mode, keeping the above in mind.
Thanks,
Mark.
Hi,
On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
quoted hunk
@@ -742,25 +739,6 @@ ENTRY(__enable_mmu) ic iallu dsb nsh isb-#ifdef CONFIG_RANDOMIZE_BASE- mov x19, x0 // preserve new SCTLR_EL1 value- blr x27-- /*- * If we return here, we have a KASLR displacement in x23 which we need- * to take into account by discarding the current kernel mapping and- * creating a new one.- */- msr sctlr_el1, x22 // disable the MMU- isb- bl __create_page_tables // recreate kernel mapping-- msr sctlr_el1, x19 // re-enable the MMU- isb- ic iallu // flush instructions fetched- dsb nsh // via old mapping- isb-#endif br x27 ENDPROC(__enable_mmu)
As a heads-up, this clashes with fd363bd417ddb610 ("arm64: avoid TLB
conflict with CONFIG_RANDOMIZE_BASE") [1], which went in for v4.8-rc4.
The fixup (moving the new TLBI; DSB into __primary_switch) is
trivial/obvious, but beyond git's automated resolution capabilities.
quoted hunk
@@ -770,11 +748,11 @@ __no_granule_support: 1: wfe wfi- b 1b+ b 1b ENDPROC(__no_granule_support)
As mentioned in another reply, it might be worth moving the whitespace
fixups into a preparatory patch, so as to make it less distracting when
looking at the diff.
Regardless, FWIW:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/451294.html
On Wed, Aug 24, 2016 at 04:36:03PM +0200, Ard Biesheuvel wrote:
This updates 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.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/kernel/head.S | 14 ++++++--------
arch/arm64/kernel/sleep.S | 10 +++-------
2 files changed, 9 insertions(+), 15 deletions(-)
Can we please fold this with patch 5?
From my PoV the combined diff is simpler to follow, and by doing so we
avoid a few temporary issues (misleading comment above __enable_mmu, fun
and games with the lr) that the series is generally cleaning up.
From a quick look at __idmap_text_{start,end}, we have plenty of
headroom in the idmap page, so moving a few more things in there is fine
by me.
FWIW, With the two patches folded:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
@@ -634,6 +634,7 @@ ENTRY(__boot_cpu_mode)*Thisprovidesa"holding pen"forplatformstoholdallsecondary*coresarehelduntilwe're ready for them to initialise.*/+.pushsection".idmap.text","ax"ENTRY(secondary_holding_pen)blel2_setup//DroptoEL1,w0=cpu_boot_modeblset_cpu_boot_mode_flag
On Wed, Aug 24, 2016 at 04:36:04PM +0200, Ard Biesheuvel wrote:
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.
Signed-off-by: Ard Biesheuvel <redacted>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
On Wed, Aug 24, 2016 at 04:36:05PM +0200, Ard Biesheuvel wrote:
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 stach pointer
register before installing the vector table.
Nit: s/stach/stack/
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.
Signed-off-by: Ard Biesheuvel <redacted>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
On Wed, Aug 24, 2016 at 04:36:06PM +0200, Ard Biesheuvel wrote:
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.
Signed-off-by: Ard Biesheuvel <redacted>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
Hi,
On Wed, Aug 24, 2016 at 04:35:57PM +0200, Ard Biesheuvel wrote:
This v2 series is a followup to the single patch #1, whose v1 I sent out
about a week ago.
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 th
primary boot path, it only leaves the necessary ones, and documents them
explicitly in patch #9.
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.
Which ones are honourable? ;)
FWIW, I think that overall this is a nice improvement in legibility for
the boot code.
Adding James to cc since patch #6 may conflict with this hibernate/
debug-pagealloc series [0], to which I replied that merging .idmap.text
with .mmuoff.text would be a worthwhile simplification.
I'll leave it to you, James, and the honourable maintainers to figure
out the details on that front.
I've given this a spin on Juno (R1) with KASAN and CONFIG_RANDOMIZE_BASE
selected (though I have no entropy source, so I'm bailing out early).
That all works, so for the series:
Tested-by: Mark Rutland <mark.rutland@arm.com>
For patch 1, feel free to upgrade the Acked-by to a Reviewed-by to match
the rest of the series.
Thanks,
Mark.
On 30 August 2016 at 15:48, Mark Rutland [off-list ref] wrote:
Hi,
On Wed, Aug 24, 2016 at 04:35:57PM +0200, Ard Biesheuvel wrote:
quoted
This v2 series is a followup to the single patch #1, whose v1 I sent out
about a week ago.
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 th
primary boot path, it only leaves the necessary ones, and documents them
explicitly in patch #9.
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.
Which ones are honourable? ;)
FWIW, I think that overall this is a nice improvement in legibility for
the boot code.
quoted
Adding James to cc since patch #6 may conflict with this hibernate/
debug-pagealloc series [0], to which I replied that merging .idmap.text
with .mmuoff.text would be a worthwhile simplification.
I'll leave it to you, James, and the honourable maintainers to figure
out the details on that front.
Thanks. James's patches have been queued already, so I can simply
rebase onto for-next/core, with the caveat that it does not have your
KASLR TLB fix yet.
I've given this a spin on Juno (R1) with KASAN and CONFIG_RANDOMIZE_BASE
selected (though I have no entropy source, so I'm bailing out early).
That all works, so for the series:
Tested-by: Mark Rutland <mark.rutland@arm.com>
For patch 1, feel free to upgrade the Acked-by to a Reviewed-by to match
the rest of the series.