[PATCH v2 0/9] arm64: clean up early boot function calls

[PATCH v2 0/9] arm64: clean up early boot function calls

[Ard Biesheuvel]

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

[PATCH v2 1/9] arm64: kernel: get rid of x25 and x26 with 'global' scope

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S  | 28 +++++++++-----------
 arch/arm64/kernel/sleep.S |  2 --
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index b77f58355da1..219676253dbc 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -214,7 +214,7 @@ ENTRY(stext)
 	adrp	x24, __PHYS_OFFSET
 	and	x23, x24, MIN_KIMG_ALIGN - 1	// KASLR offset, defaults to 0
 	bl	set_cpu_boot_mode_flag
-	bl	__create_page_tables		// x25=TTBR0, x26=TTBR1
+	bl	__create_page_tables
 	/*
 	 * The following calls CPU setup code, see arch/arm64/mm/proc.S for
 	 * details.
@@ -311,23 +311,21 @@ ENDPROC(preserve_boot_args)
  *     been enabled
  */
 __create_page_tables:
-	adrp	x25, idmap_pg_dir
-	adrp	x26, swapper_pg_dir
 	mov	x28, lr
 
 	/*
 	 * Invalidate the idmap and swapper page tables to avoid potential
 	 * dirty cache lines being evicted.
 	 */
-	mov	x0, x25
-	add	x1, x26, #SWAPPER_DIR_SIZE
+	adrp	x0, idmap_pg_dir
+	adrp	x1, swapper_pg_dir + SWAPPER_DIR_SIZE
 	bl	__inval_cache_range
 
 	/*
 	 * Clear the idmap and swapper page tables.
 	 */
-	mov	x0, x25
-	add	x6, x26, #SWAPPER_DIR_SIZE
+	adrp	x0, idmap_pg_dir
+	adrp	x6, swapper_pg_dir + SWAPPER_DIR_SIZE
 1:	stp	xzr, xzr, [x0], #16
 	stp	xzr, xzr, [x0], #16
 	stp	xzr, xzr, [x0], #16
@@ -340,7 +338,7 @@ __create_page_tables:
 	/*
 	 * Create the identity mapping.
 	 */
-	mov	x0, x25				// idmap_pg_dir
+	adrp	x0, idmap_pg_dir
 	adrp	x3, __idmap_text_start		// __pa(__idmap_text_start)
 
 #ifndef CONFIG_ARM64_VA_BITS_48
@@ -390,7 +388,7 @@ __create_page_tables:
 	/*
 	 * Map the kernel image (starting with PHYS_OFFSET).
 	 */
-	mov	x0, x26				// swapper_pg_dir
+	adrp	x0, swapper_pg_dir
 	mov_q	x5, KIMAGE_VADDR + TEXT_OFFSET	// compile time __va(_text)
 	add	x5, x5, x23			// add KASLR displacement
 	create_pgd_entry x0, x5, x3, x6
@@ -405,8 +403,8 @@ __create_page_tables:
 	 * accesses (MMU disabled), invalidate the idmap and swapper page
 	 * tables again to remove any speculatively loaded cache lines.
 	 */
-	mov	x0, x25
-	add	x1, x26, #SWAPPER_DIR_SIZE
+	adrp	x0, idmap_pg_dir
+	adrp	x1, swapper_pg_dir + SWAPPER_DIR_SIZE
 	dmb	sy
 	bl	__inval_cache_range
 
@@ -666,8 +664,6 @@ secondary_startup:
 	/*
 	 * Common entry point for secondary CPUs.
 	 */
-	adrp	x25, idmap_pg_dir
-	adrp	x26, swapper_pg_dir
 	bl	__cpu_setup			// initialise processor
 
 	adr_l	x27, __secondary_switch		// address to jump to after enabling the MMU
@@ -731,8 +727,10 @@ ENTRY(__enable_mmu)
 	cmp	x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
 	b.ne	__no_granule_support
 	update_early_cpu_boot_status 0, x1, x2
-	msr	ttbr0_el1, x25			// load TTBR0
-	msr	ttbr1_el1, x26			// load TTBR1
+	adrp	x1, idmap_pg_dir
+	adrp	x2, swapper_pg_dir
+	msr	ttbr0_el1, x1			// load TTBR0
+	msr	ttbr1_el1, x2			// load TTBR1
 	isb
 	msr	sctlr_el1, x0
 	isb
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index ccf79d849e0a..182129b60fdf 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -102,8 +102,6 @@ ENTRY(cpu_resume)
 	/* enable the MMU early - so we can access sleep_save_stash by va */
 	adr_l	lr, __enable_mmu	/* __cpu_setup will return here */
 	adr_l	x27, _resume_switched	/* __enable_mmu will branch here */
-	adrp	x25, idmap_pg_dir
-	adrp	x26, swapper_pg_dir
 	b	__cpu_setup
 ENDPROC(cpu_resume)
 
-- 
2.7.4

[PATCH v2 2/9] arm64: kernel: fix style issues in sleep.S

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/sleep.S | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index 182129b60fdf..bbc63099223b 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -73,10 +73,9 @@ ENTRY(__cpu_suspend_enter)
 	str	x2, [x0, #SLEEP_STACK_DATA_SYSTEM_REGS + CPU_CTX_SP]
 
 	/* find the mpidr_hash */
-	ldr	x1, =sleep_save_stash
-	ldr	x1, [x1]
+	ldr_l	x1, sleep_save_stash
 	mrs	x7, mpidr_el1
-	ldr	x9, =mpidr_hash
+	adr_l	x9, mpidr_hash
 	ldr	x10, [x9, #MPIDR_HASH_MASK]
 	/*
 	 * Following code relies on the struct mpidr_hash
@@ -95,14 +94,13 @@ ENTRY(__cpu_suspend_enter)
 	mov	x0, #1
 	ret
 ENDPROC(__cpu_suspend_enter)
-	.ltorg
 
 ENTRY(cpu_resume)
 	bl	el2_setup		// if in EL2 drop to EL1 cleanly
+	bl	__cpu_setup
 	/* enable the MMU early - so we can access sleep_save_stash by va */
-	adr_l	lr, __enable_mmu	/* __cpu_setup will return here */
 	adr_l	x27, _resume_switched	/* __enable_mmu will branch here */
-	b	__cpu_setup
+	b	__enable_mmu
 ENDPROC(cpu_resume)
 
 	.pushsection	".idmap.text", "ax"
@@ -115,14 +113,15 @@ ENDPROC(_resume_switched)
 
 ENTRY(_cpu_resume)
 	mrs	x1, mpidr_el1
-	adrp	x8, mpidr_hash
-	add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address
-        /* retrieve mpidr_hash members to compute the hash */
+	adr_l	x8, mpidr_hash		// x8 = struct mpidr_hash virt address
+
+	/* retrieve mpidr_hash members to compute the hash */
 	ldr	x2, [x8, #MPIDR_HASH_MASK]
 	ldp	w3, w4, [x8, #MPIDR_HASH_SHIFTS]
 	ldp	w5, w6, [x8, #(MPIDR_HASH_SHIFTS + 8)]
 	compute_mpidr_hash x7, x3, x4, x5, x6, x1, x2
-        /* x7 contains hash index, let's use it to grab context pointer */
+
+	/* x7 contains hash index, let's use it to grab context pointer */
 	ldr_l	x0, sleep_save_stash
 	ldr	x0, [x0, x7, lsl #3]
 	add	x29, x0, #SLEEP_STACK_DATA_CALLEE_REGS
-- 
2.7.4

[PATCH v2 2/9] arm64: kernel: fix style issues in sleep.S

[Mark Rutland]

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 <ard.biesheuvel@linaro.org>

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.

> ---
>  arch/arm64/kernel/sleep.S | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> index 182129b60fdf..bbc63099223b 100644
> --- a/arch/arm64/kernel/sleep.S
> +++ b/arch/arm64/kernel/sleep.S
> @@ -73,10 +73,9 @@ ENTRY(__cpu_suspend_enter)
>  	str	x2, [x0, #SLEEP_STACK_DATA_SYSTEM_REGS + CPU_CTX_SP]
>  
>  	/* find the mpidr_hash */
> -	ldr	x1, =sleep_save_stash
> -	ldr	x1, [x1]
> +	ldr_l	x1, sleep_save_stash
>  	mrs	x7, mpidr_el1
> -	ldr	x9, =mpidr_hash
> +	adr_l	x9, mpidr_hash
>  	ldr	x10, [x9, #MPIDR_HASH_MASK]
>  	/*
>  	 * Following code relies on the struct mpidr_hash
> @@ -95,14 +94,13 @@ ENTRY(__cpu_suspend_enter)
>  	mov	x0, #1
>  	ret
>  ENDPROC(__cpu_suspend_enter)
> -	.ltorg
>  
>  ENTRY(cpu_resume)
>  	bl	el2_setup		// if in EL2 drop to EL1 cleanly
> +	bl	__cpu_setup
>  	/* enable the MMU early - so we can access sleep_save_stash by va */
> -	adr_l	lr, __enable_mmu	/* __cpu_setup will return here */
>  	adr_l	x27, _resume_switched	/* __enable_mmu will branch here */
> -	b	__cpu_setup
> +	b	__enable_mmu
>  ENDPROC(cpu_resume)
>  
>  	.pushsection	".idmap.text", "ax"
> @@ -115,14 +113,15 @@ ENDPROC(_resume_switched)
>  
>  ENTRY(_cpu_resume)
>  	mrs	x1, mpidr_el1
> -	adrp	x8, mpidr_hash
> -	add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address
> -        /* retrieve mpidr_hash members to compute the hash */
> +	adr_l	x8, mpidr_hash		// x8 = struct mpidr_hash virt address
> +
> +	/* retrieve mpidr_hash members to compute the hash */
>  	ldr	x2, [x8, #MPIDR_HASH_MASK]
>  	ldp	w3, w4, [x8, #MPIDR_HASH_SHIFTS]
>  	ldp	w5, w6, [x8, #(MPIDR_HASH_SHIFTS + 8)]
>  	compute_mpidr_hash x7, x3, x4, x5, x6, x1, x2
> -        /* x7 contains hash index, let's use it to grab context pointer */
> +
> +	/* x7 contains hash index, let's use it to grab context pointer */
>  	ldr_l	x0, sleep_save_stash
>  	ldr	x0, [x0, x7, lsl #3]
>  	add	x29, x0, #SLEEP_STACK_DATA_CALLEE_REGS
> -- 
> 2.7.4
> 

[PATCH v2 3/9] arm64: kernel: use ordinary return/argument register for el2_setup()

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 219676253dbc..2871271123e7 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -210,7 +210,7 @@ efi_header_end:
 
 ENTRY(stext)
 	bl	preserve_boot_args
-	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
+	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
 	adrp	x24, __PHYS_OFFSET
 	and	x23, x24, MIN_KIMG_ALIGN - 1	// KASLR offset, defaults to 0
 	bl	set_cpu_boot_mode_flag
@@ -488,7 +488,7 @@ CPU_LE(	bic	x0, x0, #(1 << 25)	)	// Clear the EE bit for EL2
 CPU_BE(	orr	x0, x0, #(3 << 24)	)	// Set the EE and E0E bits for EL1
 CPU_LE(	bic	x0, x0, #(3 << 24)	)	// Clear the EE and E0E bits for EL1
 	msr	sctlr_el1, x0
-	mov	w20, #BOOT_CPU_MODE_EL1		// This cpu booted in EL1
+	mov	w0, #BOOT_CPU_MODE_EL1		// This cpu booted in EL1
 	isb
 	ret
 
@@ -584,7 +584,7 @@ CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
 
 	cbz	x2, install_el2_stub
 
-	mov	w20, #BOOT_CPU_MODE_EL2		// This CPU booted in EL2
+	mov	w0, #BOOT_CPU_MODE_EL2		// This CPU booted in EL2
 	isb
 	ret
 
@@ -599,7 +599,7 @@ install_el2_stub:
 		      PSR_MODE_EL1h)
 	msr	spsr_el2, x0
 	msr	elr_el2, lr
-	mov	w20, #BOOT_CPU_MODE_EL2		// This CPU booted in EL2
+	mov	w0, #BOOT_CPU_MODE_EL2		// This CPU booted in EL2
 	eret
 ENDPROC(el2_setup)
 
@@ -609,10 +609,10 @@ ENDPROC(el2_setup)
  */
 set_cpu_boot_mode_flag:
 	adr_l	x1, __boot_cpu_mode
-	cmp	w20, #BOOT_CPU_MODE_EL2
+	cmp	w0, #BOOT_CPU_MODE_EL2
 	b.ne	1f
 	add	x1, x1, #4
-1:	str	w20, [x1]			// This CPU has booted in EL1
+1:	str	w0, [x1]			// This CPU has booted in EL1
 	dmb	sy
 	dc	ivac, x1			// Invalidate potentially stale cache line
 	ret
@@ -637,7 +637,7 @@ ENTRY(__boot_cpu_mode)
 	 * cores are held until we're ready for them to initialise.
 	 */
 ENTRY(secondary_holding_pen)
-	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
+	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
 	bl	set_cpu_boot_mode_flag
 	mrs	x0, mpidr_el1
 	mov_q	x1, MPIDR_HWID_BITMASK
-- 
2.7.4

[PATCH v2 3/9] arm64: kernel: use ordinary return/argument register for el2_setup()

[Mark Rutland]

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 <ard.biesheuvel@linaro.org>

Make sense to me. FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 66 ++++++++++++--------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 2871271123e7..d390feb92730 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -222,9 +222,7 @@ ENTRY(stext)
 	 * the TCR will have been set.
 	 */
 	bl	__cpu_setup			// initialise processor
-	adr_l	x27, __primary_switch		// address to jump to after
-						// MMU has been enabled
-	b	__enable_mmu
+	b	__primary_switch
 ENDPROC(stext)
 
 /*
@@ -453,7 +451,7 @@ __primary_switched:
 	cbz	x0, 0f				// KASLR disabled? just proceed
 	orr	x23, x23, x0			// record KASLR offset
 	ret	x28				// we must enable KASLR, return
-						// to __enable_mmu()
+						// to __primary_switch()
 0:
 #endif
 	b	start_kernel
@@ -721,7 +719,6 @@ ENTRY(__early_cpu_boot_status)
  */
 	.section	".idmap.text", "ax"
 ENTRY(__enable_mmu)
-	mrs	x22, sctlr_el1			// preserve old SCTLR_EL1 value
 	mrs	x1, ID_AA64MMFR0_EL1
 	ubfx	x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
 	cmp	x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
@@ -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)
 
@@ -770,11 +748,11 @@ __no_granule_support:
 1:
 	wfe
 	wfi
-	b 1b
+	b	1b
 ENDPROC(__no_granule_support)
 
-__primary_switch:
 #ifdef CONFIG_RELOCATABLE
+__relocate_kernel:
 	/*
 	 * Iterate over each entry in the relocation table, and apply the
 	 * relocations in place.
@@ -796,8 +774,42 @@ __primary_switch:
 	add	x13, x13, x23			// relocate
 	str	x13, [x11, x23]
 	b	0b
+1:	ret
+ENDPROC(__relocate_kernel)
+#endif
 
-1:
+__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
+0:
+#ifdef CONFIG_RELOCATABLE
+	bl	__relocate_kernel
+#ifdef CONFIG_RANDOMIZE_BASE
+	ldr	x8, =__primary_switched
+	blr	x8
+
+	/*
+	 * 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
+
+	msr	sctlr_el1, x19			// re-enable the MMU
+	isb
+	ic	iallu				// flush instructions fetched
+	dsb	nsh				// via old mapping
+	isb
+
+	bl	__relocate_kernel
+#endif
 #endif
 	ldr	x8, =__primary_switched
 	br	x8
-- 
2.7.4

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Mark Rutland]

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. :/

[...]

> @@ -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

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/451294.html

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Ard Biesheuvel]

On 24 August 2016 at 22:36, Mark Rutland <mark.rutland@arm.com> wrote:
> 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. :/
>
> [...]
>
>> @@ -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

>> +__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.

>> +     /*
>> +      * 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.
>

Indeed.

Thanks,
Ard.

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Mark Rutland]

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:
> > +__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.

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Ard Biesheuvel]

On 24 August 2016 at 21:46, Mark Rutland <mark.rutland@arm.com> wrote:
> 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:
>> > +__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.

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Mark Rutland]

On Thu, Aug 25, 2016 at 02:59:51PM +0100, Ard Biesheuvel wrote:
> On 24 August 2016 at 21:46, Mark Rutland <mark.rutland@arm.com> wrote:
> > 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:
> >> > +__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.

[PATCH v2 4/9] arm64: head.S: move KASLR processing out of __enable_mmu()

[Mark Rutland]

Hi,

On Wed, Aug 24, 2016 at 04:36:01PM +0200, Ard Biesheuvel wrote:
> @@ -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.

> @@ -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

[PATCH v2 5/9] arm64: kernel: use x30 for __enable_mmu return address

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S  | 12 +++++-------
 arch/arm64/kernel/sleep.S |  2 +-
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index d390feb92730..3e08e51578d5 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -664,7 +664,7 @@ secondary_startup:
 	 */
 	bl	__cpu_setup			// initialise processor
 
-	adr_l	x27, __secondary_switch		// address to jump to after enabling the MMU
+	adr_l	lr, __secondary_switch		// address to jump to after enabling the MMU
 	b	__enable_mmu
 ENDPROC(secondary_startup)
 
@@ -710,9 +710,9 @@ ENTRY(__early_cpu_boot_status)
  * Enable the MMU.
  *
  *  x0  = SCTLR_EL1 value for turning on the MMU.
- *  x27 = *virtual* address to jump to upon completion
  *
- * Other registers depend on the function called upon completion.
+ * Returns to the caller via x30/lr. This requires the caller to be covered
+ * by the .idmap.text section.
  *
  * Checks if the selected granule size is supported by the CPU.
  * If it isn't, park the CPU
@@ -739,7 +739,7 @@ ENTRY(__enable_mmu)
 	ic	iallu
 	dsb	nsh
 	isb
-	br	x27
+	ret
 ENDPROC(__enable_mmu)
 
 __no_granule_support:
@@ -784,9 +784,7 @@ __primary_switch:
 	mrs	x20, sctlr_el1			// preserve old SCTLR_EL1 value
 #endif
 
-	adr	x27, 0f
-	b	__enable_mmu
-0:
+	bl	__enable_mmu
 #ifdef CONFIG_RELOCATABLE
 	bl	__relocate_kernel
 #ifdef CONFIG_RANDOMIZE_BASE
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index bbc63099223b..4bce95cd656a 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -99,7 +99,7 @@ ENTRY(cpu_resume)
 	bl	el2_setup		// if in EL2 drop to EL1 cleanly
 	bl	__cpu_setup
 	/* enable the MMU early - so we can access sleep_save_stash by va */
-	adr_l	x27, _resume_switched	/* __enable_mmu will branch here */
+	adr_l	lr, _resume_switched	/* __enable_mmu will branch here */
 	b	__enable_mmu
 ENDPROC(cpu_resume)
 
-- 
2.7.4

[PATCH v2 6/9] arm64: call __enable_mmu as an ordinary function for secondary/resume

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S  | 14 ++++++--------
 arch/arm64/kernel/sleep.S | 10 +++-------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 3e08e51578d5..c112c153821e 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -634,6 +634,7 @@ ENTRY(__boot_cpu_mode)
 	 * This provides a "holding pen" for platforms to hold all secondary
 	 * cores are held until we're ready for them to initialise.
 	 */
+	.pushsection	".idmap.text", "ax"
 ENTRY(secondary_holding_pen)
 	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
 	bl	set_cpu_boot_mode_flag
@@ -663,10 +664,12 @@ secondary_startup:
 	 * Common entry point for secondary CPUs.
 	 */
 	bl	__cpu_setup			// initialise processor
-
-	adr_l	lr, __secondary_switch		// address to jump to after enabling the MMU
-	b	__enable_mmu
+	bl	__enable_mmu
+	ldr	x8, =__secondary_switched
+	br	x8
 ENDPROC(secondary_startup)
+	.ltorg
+	.popsection
 
 __secondary_switched:
 	adr_l	x5, vectors
@@ -812,8 +815,3 @@ __primary_switch:
 	ldr	x8, =__primary_switched
 	br	x8
 ENDPROC(__primary_switch)
-
-__secondary_switch:
-	ldr	x8, =__secondary_switched
-	br	x8
-ENDPROC(__secondary_switch)
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index 4bce95cd656a..1d4aba6fcc7a 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -95,19 +95,15 @@ ENTRY(__cpu_suspend_enter)
 	ret
 ENDPROC(__cpu_suspend_enter)
 
+	.pushsection	".idmap.text", "ax"
 ENTRY(cpu_resume)
 	bl	el2_setup		// if in EL2 drop to EL1 cleanly
 	bl	__cpu_setup
 	/* enable the MMU early - so we can access sleep_save_stash by va */
-	adr_l	lr, _resume_switched	/* __enable_mmu will branch here */
-	b	__enable_mmu
-ENDPROC(cpu_resume)
-
-	.pushsection	".idmap.text", "ax"
-_resume_switched:
+	bl	__enable_mmu
 	ldr	x8, =_cpu_resume
 	br	x8
-ENDPROC(_resume_switched)
+ENDPROC(cpu_resume)
 	.ltorg
 	.popsection
 
-- 
2.7.4

[PATCH v2 6/9] arm64: call __enable_mmu as an ordinary function for secondary/resume

[Mark Rutland]

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 <ard.biesheuvel@linaro.org>
> ---
>  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.

> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 3e08e51578d5..c112c153821e 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -634,6 +634,7 @@ ENTRY(__boot_cpu_mode)
>  	 * This provides a "holding pen" for platforms to hold all secondary
>  	 * cores are held until we're ready for them to initialise.
>  	 */
> +	.pushsection	".idmap.text", "ax"
>  ENTRY(secondary_holding_pen)
>  	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
>  	bl	set_cpu_boot_mode_flag
> @@ -663,10 +664,12 @@ secondary_startup:
>  	 * Common entry point for secondary CPUs.
>  	 */
>  	bl	__cpu_setup			// initialise processor
> -
> -	adr_l	lr, __secondary_switch		// address to jump to after enabling the MMU
> -	b	__enable_mmu
> +	bl	__enable_mmu
> +	ldr	x8, =__secondary_switched
> +	br	x8
>  ENDPROC(secondary_startup)
> +	.ltorg
> +	.popsection
>  
>  __secondary_switched:
>  	adr_l	x5, vectors
> @@ -812,8 +815,3 @@ __primary_switch:
>  	ldr	x8, =__primary_switched
>  	br	x8
>  ENDPROC(__primary_switch)
> -
> -__secondary_switch:
> -	ldr	x8, =__secondary_switched
> -	br	x8
> -ENDPROC(__secondary_switch)
> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> index 4bce95cd656a..1d4aba6fcc7a 100644
> --- a/arch/arm64/kernel/sleep.S
> +++ b/arch/arm64/kernel/sleep.S
> @@ -95,19 +95,15 @@ ENTRY(__cpu_suspend_enter)
>  	ret
>  ENDPROC(__cpu_suspend_enter)
>  
> +	.pushsection	".idmap.text", "ax"
>  ENTRY(cpu_resume)
>  	bl	el2_setup		// if in EL2 drop to EL1 cleanly
>  	bl	__cpu_setup
>  	/* enable the MMU early - so we can access sleep_save_stash by va */
> -	adr_l	lr, _resume_switched	/* __enable_mmu will branch here */
> -	b	__enable_mmu
> -ENDPROC(cpu_resume)
> -
> -	.pushsection	".idmap.text", "ax"
> -_resume_switched:
> +	bl	__enable_mmu
>  	ldr	x8, =_cpu_resume
>  	br	x8
> -ENDPROC(_resume_switched)
> +ENDPROC(cpu_resume)
>  	.ltorg
>  	.popsection
>  
> -- 
> 2.7.4
> 

[PATCH v2 7/9] arm64: kernel: drop use of x24 from primary boot path

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index c112c153821e..27f51272de68 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -211,8 +211,8 @@ efi_header_end:
 ENTRY(stext)
 	bl	preserve_boot_args
 	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
-	adrp	x24, __PHYS_OFFSET
-	and	x23, x24, MIN_KIMG_ALIGN - 1	// KASLR offset, defaults to 0
+	adrp	x23, __PHYS_OFFSET
+	and	x23, x23, MIN_KIMG_ALIGN - 1	// KASLR offset, defaults to 0
 	bl	set_cpu_boot_mode_flag
 	bl	__create_page_tables
 	/*
@@ -412,6 +412,8 @@ ENDPROC(__create_page_tables)
 
 /*
  * The following fragment of code is executed with the MMU enabled.
+ *
+ *   x0 = __PHYS_OFFSET
  */
 	.set	initial_sp, init_thread_union + THREAD_START_SP
 __primary_switched:
@@ -420,6 +422,12 @@ __primary_switched:
 	msr	vbar_el1, x8			// vector table address
 	isb
 
+	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
+
+	ldr_l	x4, kimage_vaddr		// Save the offset between
+	sub	x4, x4, x0			// the kernel virtual and
+	str_l	x4, kimage_voffset, x5		// physical mappings
+
 	// Clear BSS
 	adr_l	x0, __bss_start
 	mov	x1, xzr
@@ -432,12 +440,6 @@ __primary_switched:
 	mov	x4, sp
 	and	x4, x4, #~(THREAD_SIZE - 1)
 	msr	sp_el0, x4			// Save thread_info
-	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
-
-	ldr_l	x4, kimage_vaddr		// Save the offset between
-	sub	x4, x4, x24			// the kernel virtual and
-	str_l	x4, kimage_voffset, x5		// physical mappings
-
 	mov	x29, #0
 #ifdef CONFIG_KASAN
 	bl	kasan_early_init
@@ -792,6 +794,7 @@ __primary_switch:
 	bl	__relocate_kernel
 #ifdef CONFIG_RANDOMIZE_BASE
 	ldr	x8, =__primary_switched
+	adrp	x0, __PHYS_OFFSET
 	blr	x8
 
 	/*
@@ -813,5 +816,6 @@ __primary_switch:
 #endif
 #endif
 	ldr	x8, =__primary_switched
+	adrp	x0, __PHYS_OFFSET
 	br	x8
 ENDPROC(__primary_switch)
-- 
2.7.4

[PATCH v2 7/9] arm64: kernel: drop use of x24 from primary boot path

[Mark Rutland]

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 <ard.biesheuvel@linaro.org>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/head.S | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index c112c153821e..27f51272de68 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -211,8 +211,8 @@ efi_header_end:
>  ENTRY(stext)
>  	bl	preserve_boot_args
>  	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
> -	adrp	x24, __PHYS_OFFSET
> -	and	x23, x24, MIN_KIMG_ALIGN - 1	// KASLR offset, defaults to 0
> +	adrp	x23, __PHYS_OFFSET
> +	and	x23, x23, MIN_KIMG_ALIGN - 1	// KASLR offset, defaults to 0
>  	bl	set_cpu_boot_mode_flag
>  	bl	__create_page_tables
>  	/*
> @@ -412,6 +412,8 @@ ENDPROC(__create_page_tables)
>  
>  /*
>   * The following fragment of code is executed with the MMU enabled.
> + *
> + *   x0 = __PHYS_OFFSET
>   */
>  	.set	initial_sp, init_thread_union + THREAD_START_SP
>  __primary_switched:
> @@ -420,6 +422,12 @@ __primary_switched:
>  	msr	vbar_el1, x8			// vector table address
>  	isb
>  
> +	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
> +
> +	ldr_l	x4, kimage_vaddr		// Save the offset between
> +	sub	x4, x4, x0			// the kernel virtual and
> +	str_l	x4, kimage_voffset, x5		// physical mappings
> +
>  	// Clear BSS
>  	adr_l	x0, __bss_start
>  	mov	x1, xzr
> @@ -432,12 +440,6 @@ __primary_switched:
>  	mov	x4, sp
>  	and	x4, x4, #~(THREAD_SIZE - 1)
>  	msr	sp_el0, x4			// Save thread_info
> -	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
> -
> -	ldr_l	x4, kimage_vaddr		// Save the offset between
> -	sub	x4, x4, x24			// the kernel virtual and
> -	str_l	x4, kimage_voffset, x5		// physical mappings
> -
>  	mov	x29, #0
>  #ifdef CONFIG_KASAN
>  	bl	kasan_early_init
> @@ -792,6 +794,7 @@ __primary_switch:
>  	bl	__relocate_kernel
>  #ifdef CONFIG_RANDOMIZE_BASE
>  	ldr	x8, =__primary_switched
> +	adrp	x0, __PHYS_OFFSET
>  	blr	x8
>  
>  	/*
> @@ -813,5 +816,6 @@ __primary_switch:
>  #endif
>  #endif
>  	ldr	x8, =__primary_switched
> +	adrp	x0, __PHYS_OFFSET
>  	br	x8
>  ENDPROC(__primary_switch)
> -- 
> 2.7.4
> 

[PATCH v2 8/9] arm64: head.S: use ordinary stack frame for __primary_switched()

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 27f51272de68..ad1dc61d67ac 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -415,13 +415,18 @@ ENDPROC(__create_page_tables)
  *
  *   x0 = __PHYS_OFFSET
  */
-	.set	initial_sp, init_thread_union + THREAD_START_SP
 __primary_switched:
-	mov	x28, lr				// preserve LR
+	adrp	x4, init_thread_union
+	add	sp, x4, #THREAD_SIZE
+	msr	sp_el0, x4			// Save thread_info
+
 	adr_l	x8, vectors			// load VBAR_EL1 with virtual
 	msr	vbar_el1, x8			// vector table address
 	isb
 
+	stp	xzr, x30, [sp, #-16]!
+	mov	x29, sp
+
 	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
 
 	ldr_l	x4, kimage_vaddr		// Save the offset between
@@ -436,11 +441,6 @@ __primary_switched:
 	bl	__pi_memset
 	dsb	ishst				// Make zero page visible to PTW
 
-	adr_l	sp, initial_sp, x4
-	mov	x4, sp
-	and	x4, x4, #~(THREAD_SIZE - 1)
-	msr	sp_el0, x4			// Save thread_info
-	mov	x29, #0
 #ifdef CONFIG_KASAN
 	bl	kasan_early_init
 #endif
@@ -452,8 +452,8 @@ __primary_switched:
 	bl	kaslr_early_init		// parse FDT for KASLR options
 	cbz	x0, 0f				// KASLR disabled? just proceed
 	orr	x23, x23, x0			// record KASLR offset
-	ret	x28				// we must enable KASLR, return
-						// to __primary_switch()
+	ldp	x29, x30, [sp], #16		// we must enable KASLR, return
+	ret					// to __primary_switch()
 0:
 #endif
 	b	start_kernel
-- 
2.7.4

[PATCH v2 8/9] arm64: head.S: use ordinary stack frame for __primary_switched()

[Mark Rutland]

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 <ard.biesheuvel@linaro.org>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/head.S | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 27f51272de68..ad1dc61d67ac 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -415,13 +415,18 @@ ENDPROC(__create_page_tables)
>   *
>   *   x0 = __PHYS_OFFSET
>   */
> -	.set	initial_sp, init_thread_union + THREAD_START_SP
>  __primary_switched:
> -	mov	x28, lr				// preserve LR
> +	adrp	x4, init_thread_union
> +	add	sp, x4, #THREAD_SIZE
> +	msr	sp_el0, x4			// Save thread_info
> +
>  	adr_l	x8, vectors			// load VBAR_EL1 with virtual
>  	msr	vbar_el1, x8			// vector table address
>  	isb
>  
> +	stp	xzr, x30, [sp, #-16]!
> +	mov	x29, sp
> +
>  	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
>  
>  	ldr_l	x4, kimage_vaddr		// Save the offset between
> @@ -436,11 +441,6 @@ __primary_switched:
>  	bl	__pi_memset
>  	dsb	ishst				// Make zero page visible to PTW
>  
> -	adr_l	sp, initial_sp, x4
> -	mov	x4, sp
> -	and	x4, x4, #~(THREAD_SIZE - 1)
> -	msr	sp_el0, x4			// Save thread_info
> -	mov	x29, #0
>  #ifdef CONFIG_KASAN
>  	bl	kasan_early_init
>  #endif
> @@ -452,8 +452,8 @@ __primary_switched:
>  	bl	kaslr_early_init		// parse FDT for KASLR options
>  	cbz	x0, 0f				// KASLR disabled? just proceed
>  	orr	x23, x23, x0			// record KASLR offset
> -	ret	x28				// we must enable KASLR, return
> -						// to __primary_switch()
> +	ldp	x29, x30, [sp], #16		// we must enable KASLR, return
> +	ret					// to __primary_switch()
>  0:
>  #endif
>  	b	start_kernel
> -- 
> 2.7.4
> 

[PATCH v2 9/9] arm64: head.S: document the use of callee saved registers

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index ad1dc61d67ac..8bc9458f9add 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -208,6 +208,16 @@ efi_header_end:
 
 	__INIT
 
+	/*
+	 * The following callee saved general purpose registers are used on the
+	 * primary lowlevel boot path:
+	 *
+	 *  Register   Scope                      Purpose
+	 *  x21        stext() .. start_kernel()  FDT pointer passed at boot in x0
+	 *  x23        stext() .. start_kernel()  physical misalignment/KASLR offset
+	 *  x28        __create_page_tables()     callee preserved temp register
+	 *  x19/x20    __primary_switch()         callee preserved temp registers
+	 */
 ENTRY(stext)
 	bl	preserve_boot_args
 	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
-- 
2.7.4

[PATCH v2 9/9] arm64: head.S: document the use of callee saved registers

[Mark Rutland]

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 <ard.biesheuvel@linaro.org>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/head.S | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index ad1dc61d67ac..8bc9458f9add 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -208,6 +208,16 @@ efi_header_end:
>  
>  	__INIT
>  
> +	/*
> +	 * The following callee saved general purpose registers are used on the
> +	 * primary lowlevel boot path:
> +	 *
> +	 *  Register   Scope                      Purpose
> +	 *  x21        stext() .. start_kernel()  FDT pointer passed at boot in x0
> +	 *  x23        stext() .. start_kernel()  physical misalignment/KASLR offset
> +	 *  x28        __create_page_tables()     callee preserved temp register
> +	 *  x19/x20    __primary_switch()         callee preserved temp registers
> +	 */
>  ENTRY(stext)
>  	bl	preserve_boot_args
>  	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
> -- 
> 2.7.4
> 

[PATCH v2 0/9] arm64: clean up early boot function calls

[Mark Rutland]

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.

[PATCH v2 0/9] arm64: clean up early boot function calls

[Ard Biesheuvel]

On 30 August 2016 at 15:48, Mark Rutland <mark.rutland@arm.com> wrote:
> 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.
>

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.
>

Thanks a lot!

-- 
Ard.