[PATCH 0/8] arm64: 2016 head.S spring cleaning

[PATCH 0/8] arm64: 2016 head.S spring cleaning

[Ard Biesheuvel]

After having been responsible for obfuscating the head.S code in the v4.6
cycle to implement KASLR support, this series attempts to make amends by
performing some janitorial duties as well.

The main point of this series is to get rid of the file scoped callee saved
registers in head.S to store the FDT pointer, ID map and swapper dir addresses,
KASLR offset, PHYS offset etc. The fact that these are file scoped makes it
unnecessarily complicated to reason about lifetimes etc, especially since the
code is called by both the boot CPU and secondaries.

Since some of these functions are called without a stack, we still need to use
callee saved registers to preserve values across function calls without
stacking/unstacking the previous values, but these instances are now function
scoped, not file scoped, and documented.

Patch #1 gets rid of the confusing subtraction of address and offset values to
obtain __secondary_switched.

Patch #2 gets rid of x21 in head.S

Patch #3 creates moves the stack pointer init to an earlier time in
__mmap_switched, and creates a proper stack frame.

Patch #4 changes the EL2 detection code to simply use x0 as a return value and
first argument.

Patch #5 gets rid of x25 and x26 to store idmap_pg_dir and swapper_pg_dir

Patch #6 gets rid of x27 to store __mmap_switched or __secondary_switched

Patch #7 gets rid of x24 to store PHYS_OFFSET

Patch #8 gets rid of x23 to store the kaslr offset

@Maintainers: feel free to cherry pick if not all [or none] of these patches
make sense to you

Ard Biesheuvel (8):
  arm64/kernel: use literal for relocated address of
    __secondary_switched
  arm64/kernel: reuse boot_args array to get to __fdt_pointer
  arm64/kernel: use a proper stack frame in __mmap_switched()
  arm64/kernel: use ordinary calling convention for EL2 setup
  arm64/kernel: refer to idmap_pg_dir and swapper_pg_dir directly
  arm64/kernel: pass virtual entry point as __enable_mmu() argument
  arm64/kernel: drop __PHYS_OFFSET register with file scope from head.S
  arm64/kernel: drop global kaslr_offset in x23 from head.S

 arch/arm64/kernel/head.S  | 177 +++++++++++---------
 arch/arm64/kernel/setup.c |   4 +-
 2 files changed, 95 insertions(+), 86 deletions(-)

-- 
2.5.0

[PATCH 1/8] arm64/kernel: use literal for relocated address of __secondary_switched

[Ard Biesheuvel]

We can simply use a relocated 64-bit literal to store the address of
__secondary_switched(), and the relocation code will ensure that it
holds the correct value at secondary entry time, as long as we make sure
that the literal value is visible to the secondaries before they enable
their MMUs. So place the literal next to kimage_vaddr, and set the alignment
so that it is covered by the same cacheline that we already have to clean
for a similar purpose.

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 4203d5f257bc..69b33535911e 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -471,7 +471,8 @@ __mmap_switched:
 	b	0b
 
 2:	adr_l	x8, kimage_vaddr		// make relocated kimage_vaddr
-	dc	cvac, x8			// value visible to secondaries
+						// and __secondary_switched
+	dc	cvac, x8			// values visible to secondaries
 	dsb	sy				// with MMU off
 #endif
 
@@ -506,10 +507,12 @@ ENDPROC(__mmap_switched)
  * end early head section, begin head code that is also used for
  * hotplug and needs to have the same protections as the text region
  */
-	.section ".text","ax"
-
+	.section	".text","ax"
+	.align		4
 ENTRY(kimage_vaddr)
 	.quad		_text - TEXT_OFFSET
+.L__secondary_switched:
+	.quad		__secondary_switched
 
 /*
  * If we're fortunate enough to boot at EL2, ensure that the world is
@@ -701,12 +704,9 @@ ENTRY(secondary_startup)
 	adrp	x26, swapper_pg_dir
 	bl	__cpu_setup			// initialise processor
 
-	ldr	x8, kimage_vaddr
-	ldr	w9, 0f
-	sub	x27, x8, w9, sxtw		// address to jump to after enabling the MMU
+	ldr	x27, .L__secondary_switched
 	b	__enable_mmu
 ENDPROC(secondary_startup)
-0:	.long	(_text - TEXT_OFFSET) - __secondary_switched
 
 ENTRY(__secondary_switched)
 	adr_l	x5, vectors
-- 
2.5.0

[PATCH 2/8] arm64/kernel: reuse boot_args array to get to __fdt_pointer

[Ard Biesheuvel]

Since we record the boot_args [x0 .. x3] early at boot anyway, there is
no need to keep the FDT address in a callee saved register with file scope
in head.S. So simply refer to boot_args[0] directly in the call to
setup_machine(), and drop the handling of x21 from head.S entirely.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S  | 14 ++++++--------
 arch/arm64/kernel/setup.c |  4 +---
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 69b33535911e..9d8f928c355c 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -234,16 +234,15 @@ ENDPROC(stext)
  * Preserve the arguments passed by the bootloader in x0 .. x3
  */
 preserve_boot_args:
-	mov	x21, x0				// x21=FDT
-
-	adr_l	x0, boot_args			// record the contents of
-	stp	x21, x1, [x0]			// x0 .. x3 at kernel entry
-	stp	x2, x3, [x0, #16]
+	adr_l	x4, boot_args			// record the contents of
+	stp	x0, x1, [x4]			// x0 .. x3 at kernel entry
+	stp	x2, x3, [x4, #16]
 
 	dmb	sy				// needed before dc ivac with
 						// MMU off
 
-	add	x1, x0, #0x20			// 4 x 8 bytes
+	mov	x0, x4
+	add	x1, x4, #0x20			// 4 x 8 bytes
 	b	__inval_cache_range		// tail call
 ENDPROC(preserve_boot_args)
 
@@ -480,7 +479,6 @@ __mmap_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
@@ -492,7 +490,7 @@ __mmap_switched:
 #endif
 #ifdef CONFIG_RANDOMIZE_BASE
 	cbnz	x23, 0f				// already running randomized?
-	mov	x0, x21				// pass FDT address in x0
+	ldr_l	x0, boot_args			// pass FDT address in x0
 	bl	kaslr_early_init		// parse FDT for KASLR options
 	cbz	x0, 0f				// KASLR disabled? just proceed
 	mov	x23, x0				// record KASLR offset
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 9dc67769b6a4..83a3e484a90a 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -64,8 +64,6 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/mmu_context.h>
 
-phys_addr_t __fdt_pointer __initdata;
-
 /*
  * Standard memory resources
  */
@@ -304,7 +302,7 @@ void __init setup_arch(char **cmdline_p)
 	early_fixmap_init();
 	early_ioremap_init();
 
-	setup_machine_fdt(__fdt_pointer);
+	setup_machine_fdt(boot_args[0]);
 
 	parse_early_param();
 
-- 
2.5.0

[PATCH 3/8] arm64/kernel: use a proper stack frame in __mmap_switched()

[Ard Biesheuvel]

Considering that we can expect stack accesses from the moment we assign
VBAR_EL1, let's initialize the stack pointer first in __mmap_switched(),
and set up a proper stack frame while we're at it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 9d8f928c355c..f441fc73a7a2 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -421,9 +421,14 @@ kernel_img_size:
 /*
  * The following fragment of code is executed with the MMU enabled.
  */
-	.set	initial_sp, init_thread_union + THREAD_START_SP
 __mmap_switched:
-	mov	x28, lr				// preserve LR
+	adrp	x4, init_thread_union
+	add	sp, x4, #THREAD_SIZE
+	msr	sp_el0, x4			// Save thread_info
+
+	stp	xzr, x30, [sp, #-16]!
+	mov	x29, sp
+
 	adr_l	x8, vectors			// load VBAR_EL1 with virtual
 	msr	vbar_el1, x8			// vector table address
 	isb
@@ -475,16 +480,9 @@ __mmap_switched:
 	dsb	sy				// with MMU off
 #endif
 
-	adr_l	sp, initial_sp, x4
-	mov	x4, sp
-	and	x4, x4, #~(THREAD_SIZE - 1)
-	msr	sp_el0, x4			// Save thread_info
-
 	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
 #endif
@@ -494,8 +492,8 @@ __mmap_switched:
 	bl	kaslr_early_init		// parse FDT for KASLR options
 	cbz	x0, 0f				// KASLR disabled? just proceed
 	mov	x23, x0				// record KASLR offset
-	ret	x28				// we must enable KASLR, return
-						// to __enable_mmu()
+	ldp	x29, x30, [sp], #16		// we must enable KASLR, return
+	ret					// to __enable_mmu()
 0:
 #endif
 	b	start_kernel
-- 
2.5.0

[PATCH 4/8] arm64/kernel: use ordinary calling convention for EL2 setup

[Ard Biesheuvel]

Since the return value of el2_setup() is almost immediately passed to
set_cpu_boot_mode_flag() in all cases, there is no need to use a callee
saved register w20, but we can use simply use w0 instead.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index f441fc73a7a2..2a1b3ba1d81c 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -211,7 +211,7 @@ section_table:
 
 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
 	mov	x23, xzr			// KASLR offset, defaults to 0
 	adrp	x24, __PHYS_OFFSET
 	bl	set_cpu_boot_mode_flag
@@ -530,7 +530,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
 
@@ -616,7 +616,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
 
@@ -631,20 +631,20 @@ 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)
 
 /*
  * Sets the __boot_cpu_mode flag depending on the CPU boot mode passed
- * in x20. See arch/arm64/include/asm/virt.h for more info.
+ * in x0. See arch/arm64/include/asm/virt.h for more info.
  */
 ENTRY(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
@@ -669,7 +669,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
 	ldr     x1, =MPIDR_HWID_BITMASK
-- 
2.5.0

[PATCH 5/8] arm64/kernel: refer to idmap_pg_dir and swapper_pg_dir directly

[Ard Biesheuvel]

Instead of keeping their addresses in globals in x25 and x26 in head.S,
just refer to idmap_pg_dir or swapper_pg_dir directly using adrp
instructions when needed. This reduces the number of callee saved register
with file scope, which should increase maintainability.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 28 +++++++++-----------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 2a1b3ba1d81c..9201cddb53bc 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -215,7 +215,7 @@ ENTRY(stext)
 	mov	x23, xzr			// KASLR offset, defaults to 0
 	adrp	x24, __PHYS_OFFSET
 	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.
@@ -313,23 +313,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
@@ -342,7 +340,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
@@ -392,7 +390,7 @@ __create_page_tables:
 	/*
 	 * Map the kernel image (starting with PHYS_OFFSET).
 	 */
-	mov	x0, x26				// swapper_pg_dir
+	adrp	x0, swapper_pg_dir
 	ldr	x5, =KIMAGE_VADDR
 	add	x5, x5, x23			// add KASLR displacement
 	create_pgd_entry x0, x5, x3, x6
@@ -406,8 +404,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
 
@@ -696,8 +694,6 @@ ENTRY(secondary_startup)
 	/*
 	 * Common entry point for secondary CPUs.
 	 */
-	adrp	x25, idmap_pg_dir
-	adrp	x26, swapper_pg_dir
 	bl	__cpu_setup			// initialise processor
 
 	ldr	x27, .L__secondary_switched
@@ -760,8 +756,10 @@ __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	x4, idmap_pg_dir
+	adrp	x5, swapper_pg_dir
+	msr	ttbr0_el1, x4			// load TTBR0
+	msr	ttbr1_el1, x5			// load TTBR1
 	isb
 	msr	sctlr_el1, x0
 	isb
-- 
2.5.0

[PATCH 6/8] arm64/kernel: pass virtual entry point as __enable_mmu() argument

[Ard Biesheuvel]

Instead of keeping the virtual entry point to be invoked by __enable_mmu
in a callee saved register with file scope, simply pass it as the second
argument. This makes the code easier to maintain.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 27 ++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 9201cddb53bc..d28fc345bec3 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -222,10 +222,10 @@ ENTRY(stext)
 	 * On return, the CPU will be ready for the MMU to be turned on and
 	 * the TCR will have been set.
 	 */
-	ldr	x27, 0f				// address to jump to after
+	bl	__cpu_setup			// initialise processor
+	ldr	x1, 0f				// address to jump to after
 						// MMU has been enabled
-	adr_l	lr, __enable_mmu		// return (PIC) address
-	b	__cpu_setup			// initialise processor
+	b	__enable_mmu
 ENDPROC(stext)
 	.align	3
 0:	.quad	__mmap_switched - (_head - TEXT_OFFSET) + KIMAGE_VADDR
@@ -696,7 +696,7 @@ ENTRY(secondary_startup)
 	 */
 	bl	__cpu_setup			// initialise processor
 
-	ldr	x27, .L__secondary_switched
+	ldr	x1, .L__secondary_switched
 	b	__enable_mmu
 ENDPROC(secondary_startup)
 
@@ -741,7 +741,7 @@ 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
+ *  x1  = *virtual* address to jump to upon completion
  *
  * Other registers depend on the function called upon completion.
  *
@@ -751,11 +751,11 @@ ENTRY(__early_cpu_boot_status)
 	.section	".idmap.text", "ax"
 __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
+	mrs	x2, ID_AA64MMFR0_EL1
+	ubfx	x3, x2, #ID_AA64MMFR0_TGRAN_SHIFT, 4
+	cmp	x3, #ID_AA64MMFR0_TGRAN_SUPPORTED
 	b.ne	__no_granule_support
-	update_early_cpu_boot_status 0, x1, x2
+	update_early_cpu_boot_status 0, x2, x3
 	adrp	x4, idmap_pg_dir
 	adrp	x5, swapper_pg_dir
 	msr	ttbr0_el1, x4			// load TTBR0
@@ -771,9 +771,10 @@ __enable_mmu:
 	ic	iallu
 	dsb	nsh
 	isb
+	mov	x20, x1				// preserve branch target
 #ifdef CONFIG_RANDOMIZE_BASE
 	mov	x19, x0				// preserve new SCTLR_EL1 value
-	blr	x27
+	blr	x1
 
 	/*
 	 * If we return here, we have a KASLR displacement in x23 which we need
@@ -789,14 +790,14 @@ __enable_mmu:
 	ic	iallu				// flush instructions fetched
 	dsb	nsh				// via old mapping
 	isb
-	add	x27, x27, x23			// relocated __mmap_switched
+	add	x20, x20, x23			// relocated __mmap_switched
 #endif
-	br	x27
+	br	x20
 ENDPROC(__enable_mmu)
 
 __no_granule_support:
 	/* Indicate that this CPU can't boot and is stuck in the kernel */
-	update_early_cpu_boot_status CPU_STUCK_IN_KERNEL, x1, x2
+	update_early_cpu_boot_status CPU_STUCK_IN_KERNEL, x2, x3
 1:
 	wfe
 	wfi
-- 
2.5.0

[PATCH 7/8] arm64/kernel: drop __PHYS_OFFSET register with file scope from head.S

[Ard Biesheuvel]

Instead of keeping __PHYS_OFFSET in a callee saved register with file
scope in head.S, derive the value on demand. This makes for cleaner
and more maintainable code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 26 +++++++++++---------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index d28fc345bec3..23d03da7ecfe 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -213,7 +213,6 @@ ENTRY(stext)
 	bl	preserve_boot_args
 	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
 	mov	x23, xzr			// KASLR offset, defaults to 0
-	adrp	x24, __PHYS_OFFSET
 	bl	set_cpu_boot_mode_flag
 	bl	__create_page_tables
 	/*
@@ -396,7 +395,7 @@ __create_page_tables:
 	create_pgd_entry x0, x5, x3, x6
 	ldr	w6, kernel_img_size
 	add	x6, x6, x5
-	mov	x3, x24				// phys offset
+	adrp	x3, __PHYS_OFFSET
 	create_block_map x0, x7, x3, x5, x6
 
 	/*
@@ -417,7 +416,7 @@ kernel_img_size:
 	.ltorg
 
 /*
- * The following fragment of code is executed with the MMU enabled.
+ * __mmap_switched(u64 phys_offset) - virtual entry point for the boot CPU
  */
 __mmap_switched:
 	adrp	x4, init_thread_union
@@ -431,14 +430,6 @@ __mmap_switched:
 	msr	vbar_el1, x8			// vector table address
 	isb
 
-	// Clear BSS
-	adr_l	x0, __bss_start
-	mov	x1, xzr
-	adr_l	x2, __bss_stop
-	sub	x2, x2, x0
-	bl	__pi_memset
-	dsb	ishst				// Make zero page visible to PTW
-
 #ifdef CONFIG_RELOCATABLE
 
 	/*
@@ -479,8 +470,17 @@ __mmap_switched:
 #endif
 
 	ldr_l	x4, kimage_vaddr		// Save the offset between
-	sub	x4, x4, x24			// the kernel virtual and
+	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
+	adr_l	x2, __bss_stop
+	sub	x2, x2, x0
+	bl	__pi_memset
+	dsb	ishst				// Make zero page visible to PTW
+
 #ifdef CONFIG_KASAN
 	bl	kasan_early_init
 #endif
@@ -774,6 +774,7 @@ __enable_mmu:
 	mov	x20, x1				// preserve branch target
 #ifdef CONFIG_RANDOMIZE_BASE
 	mov	x19, x0				// preserve new SCTLR_EL1 value
+	adrp	x0, __PHYS_OFFSET
 	blr	x1
 
 	/*
@@ -792,6 +793,7 @@ __enable_mmu:
 	isb
 	add	x20, x20, x23			// relocated __mmap_switched
 #endif
+	adrp	x0, __PHYS_OFFSET
 	br	x20
 ENDPROC(__enable_mmu)
 
-- 
2.5.0

[PATCH 8/8] arm64/kernel: drop global kaslr_offset in x23 from head.S

[Ard Biesheuvel]

Instead of keeping a global kaslr_offset variable with file scope in
head.S, pass the kaslr_offset as an argument to __create_page_tables()
and __mmap_switched(), and return the new kaslr_offset from the latter
if it returns to __enable_mmu() in order to configure KASLR.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 46 +++++++++++++-------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 23d03da7ecfe..02e37f052263 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -212,8 +212,9 @@ section_table:
 ENTRY(stext)
 	bl	preserve_boot_args
 	bl	el2_setup			// Drop to EL1, w0=cpu_boot_mode
-	mov	x23, xzr			// KASLR offset, defaults to 0
 	bl	set_cpu_boot_mode_flag
+
+	mov	x0, xzr				// KASLR offset, defaults to 0
 	bl	__create_page_tables
 	/*
 	 * The following calls CPU setup code, see arch/arm64/mm/proc.S for
@@ -305,14 +306,19 @@ ENDPROC(preserve_boot_args)
 	.endm
 
 /*
+ * __create_page_tables(u64 kaslr_offset)
+ *
  * Setup the initial page tables. We only setup the barest amount which is
  * required to get the kernel running. The following sections are required:
  *   - identity mapping to enable the MMU (low address, TTBR0)
  *   - first few MB of the kernel linear mapping to jump to once the MMU has
  *     been enabled
+ *
+ * Clobbers callee saved registers x27 and x28
  */
 __create_page_tables:
 	mov	x28, lr
+	mov	x27, x0
 
 	/*
 	 * Invalidate the idmap and swapper page tables to avoid potential
@@ -391,7 +397,7 @@ __create_page_tables:
 	 */
 	adrp	x0, swapper_pg_dir
 	ldr	x5, =KIMAGE_VADDR
-	add	x5, x5, x23			// add KASLR displacement
+	add	x5, x5, x27			// add KASLR displacement
 	create_pgd_entry x0, x5, x3, x6
 	ldr	w6, kernel_img_size
 	add	x6, x6, x5
@@ -416,7 +422,10 @@ kernel_img_size:
 	.ltorg
 
 /*
- * __mmap_switched(u64 phys_offset) - virtual entry point for the boot CPU
+ * __mmap_switched(u64 phys_offset, u64 kaslr_offset) - virtual entry point for
+ *							the boot CPU
+ *
+ * Clobbers callee saved register x26
  */
 __mmap_switched:
 	adrp	x4, init_thread_union
@@ -431,6 +440,7 @@ __mmap_switched:
 	isb
 
 #ifdef CONFIG_RELOCATABLE
+	mov	x26, x1				// preserve kaslr_offset
 
 	/*
 	 * Iterate over each entry in the relocation table, and apply the
@@ -446,8 +456,8 @@ __mmap_switched:
 	ldr	x13, [x9, #-8]
 	cmp	w12, #R_AARCH64_RELATIVE
 	b.ne	1f
-	add	x13, x13, x23			// relocate
-	str	x13, [x11, x23]
+	add	x13, x13, x1			// relocate
+	str	x13, [x11, x1]
 	b	0b
 
 1:	cmp	w12, #R_AARCH64_ABS64
@@ -457,10 +467,10 @@ __mmap_switched:
 	ldrsh	w14, [x12, #6]			// Elf64_Sym::st_shndx
 	ldr	x15, [x12, #8]			// Elf64_Sym::st_value
 	cmp	w14, #-0xf			// SHN_ABS (0xfff1) ?
-	add	x14, x15, x23			// relocate
+	add	x14, x15, x1			// relocate
 	csel	x15, x14, x15, ne
 	add	x15, x13, x15
-	str	x15, [x11, x23]
+	str	x15, [x11, x1]
 	b	0b
 
 2:	adr_l	x8, kimage_vaddr		// make relocated kimage_vaddr
@@ -485,11 +495,10 @@ __mmap_switched:
 	bl	kasan_early_init
 #endif
 #ifdef CONFIG_RANDOMIZE_BASE
-	cbnz	x23, 0f				// already running randomized?
+	cbnz	x26, 0f				// already running randomized?
 	ldr_l	x0, boot_args			// pass FDT address in x0
 	bl	kaslr_early_init		// parse FDT for KASLR options
 	cbz	x0, 0f				// KASLR disabled? just proceed
-	mov	x23, x0				// record KASLR offset
 	ldp	x29, x30, [sp], #16		// we must enable KASLR, return
 	ret					// to __enable_mmu()
 0:
@@ -747,6 +756,8 @@ ENTRY(__early_cpu_boot_status)
  *
  * Checks if the selected granule size is supported by the CPU.
  * If it isn't, park the CPU
+ *
+ * Clobbers callee saved registers x22, x23, x24 and x25
  */
 	.section	".idmap.text", "ax"
 __enable_mmu:
@@ -771,30 +782,33 @@ __enable_mmu:
 	ic	iallu
 	dsb	nsh
 	isb
-	mov	x20, x1				// preserve branch target
+	mov	x25, x1				// preserve branch target
+	mov	x1, xzr
 #ifdef CONFIG_RANDOMIZE_BASE
-	mov	x19, x0				// preserve new SCTLR_EL1 value
+	mov	x24, x0				// preserve new SCTLR_EL1 value
 	adrp	x0, __PHYS_OFFSET
-	blr	x1
+	blr	x25
 
 	/*
-	 * If we return here, we have a KASLR displacement in x23 which we need
+	 * If we return here, we have a KASLR displacement in x0 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
+	mov	x23, x0				// preserve new kaslr_offset
 	bl	__create_page_tables		// recreate kernel mapping
 
-	msr	sctlr_el1, x19			// re-enable the MMU
+	msr	sctlr_el1, x24			// re-enable the MMU
 	isb
 	ic	iallu				// flush instructions fetched
 	dsb	nsh				// via old mapping
 	isb
-	add	x20, x20, x23			// relocated __mmap_switched
+	add	x25, x25, x23			// relocated __mmap_switched
+	mov	x1, x23
 #endif
 	adrp	x0, __PHYS_OFFSET
-	br	x20
+	br	x25
 ENDPROC(__enable_mmu)
 
 __no_granule_support:
-- 
2.5.0

[PATCH 2/8] arm64/kernel: reuse boot_args array to get to __fdt_pointer

[James Morse]

Hi Ard,

On 04/04/16 15:52, Ard Biesheuvel wrote:
> Since we record the boot_args [x0 .. x3] early at boot anyway, there is
> no need to keep the FDT address in a callee saved register with file scope
> in head.S. So simply refer to boot_args[0] directly in the call to
> setup_machine(), and drop the handling of x21 from head.S entirely.

Won't this break Big Endian?
(or more accurately: bootloader-endian != kernel-endian)

The stores in preserve_boot_args happen before the EE/E0E bits have been
set/cleared by el2_setup(), so they happen with boot-loader:endianness. The
loads after el2_setup() happen with kernel:endianness.


Thanks,

James


> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S  | 14 ++++++--------
>  arch/arm64/kernel/setup.c |  4 +---
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 69b33535911e..9d8f928c355c 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -234,16 +234,15 @@ ENDPROC(stext)
>   * Preserve the arguments passed by the bootloader in x0 .. x3
>   */
>  preserve_boot_args:
> -	mov	x21, x0				// x21=FDT
> -
> -	adr_l	x0, boot_args			// record the contents of
> -	stp	x21, x1, [x0]			// x0 .. x3 at kernel entry
> -	stp	x2, x3, [x0, #16]
> +	adr_l	x4, boot_args			// record the contents of
> +	stp	x0, x1, [x4]			// x0 .. x3 at kernel entry
> +	stp	x2, x3, [x4, #16]
>  
>  	dmb	sy				// needed before dc ivac with
>  						// MMU off
>  
> -	add	x1, x0, #0x20			// 4 x 8 bytes
> +	mov	x0, x4
> +	add	x1, x4, #0x20			// 4 x 8 bytes
>  	b	__inval_cache_range		// tail call
>  ENDPROC(preserve_boot_args)
>  
> @@ -480,7 +479,6 @@ __mmap_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
> @@ -492,7 +490,7 @@ __mmap_switched:
>  #endif
>  #ifdef CONFIG_RANDOMIZE_BASE
>  	cbnz	x23, 0f				// already running randomized?
> -	mov	x0, x21				// pass FDT address in x0
> +	ldr_l	x0, boot_args			// pass FDT address in x0
>  	bl	kaslr_early_init		// parse FDT for KASLR options
>  	cbz	x0, 0f				// KASLR disabled? just proceed
>  	mov	x23, x0				// record KASLR offset
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 9dc67769b6a4..83a3e484a90a 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -64,8 +64,6 @@
>  #include <asm/xen/hypervisor.h>
>  #include <asm/mmu_context.h>
>  
> -phys_addr_t __fdt_pointer __initdata;
> -
>  /*
>   * Standard memory resources
>   */
> @@ -304,7 +302,7 @@ void __init setup_arch(char **cmdline_p)
>  	early_fixmap_init();
>  	early_ioremap_init();
>  
> -	setup_machine_fdt(__fdt_pointer);
> +	setup_machine_fdt(boot_args[0]);
>  
>  	parse_early_param();
>  
> 

[PATCH 2/8] arm64/kernel: reuse boot_args array to get to __fdt_pointer

[Ard Biesheuvel]

On 4 April 2016 at 17:13, James Morse <james.morse@arm.com> wrote:
> Hi Ard,
>
> On 04/04/16 15:52, Ard Biesheuvel wrote:
>> Since we record the boot_args [x0 .. x3] early at boot anyway, there is
>> no need to keep the FDT address in a callee saved register with file scope
>> in head.S. So simply refer to boot_args[0] directly in the call to
>> setup_machine(), and drop the handling of x21 from head.S entirely.
>
> Won't this break Big Endian?
> (or more accurately: bootloader-endian != kernel-endian)
>
> The stores in preserve_boot_args happen before the EE/E0E bits have been
> set/cleared by el2_setup(), so they happen with boot-loader:endianness. The
> loads after el2_setup() happen with kernel:endianness.
>

That is a very good point.

[PATCH 3/8] arm64/kernel: use a proper stack frame in __mmap_switched()

[James Morse]

Hi Ard,

On 04/04/16 15:52, Ard Biesheuvel wrote:
> Considering that we can expect stack accesses from the moment we assign
> VBAR_EL1, let's initialize the stack pointer first in __mmap_switched(),
> and set up a proper stack frame while we're at it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 9d8f928c355c..f441fc73a7a2 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -421,9 +421,14 @@ kernel_img_size:
>  /*
>   * The following fragment of code is executed with the MMU enabled.
>   */
> -	.set	initial_sp, init_thread_union + THREAD_START_SP
>  __mmap_switched:
> -	mov	x28, lr				// preserve LR
> +	adrp	x4, init_thread_union
> +	add	sp, x4, #THREAD_SIZE
> +	msr	sp_el0, x4			// Save thread_info

(Terms describing the stack are confusing, here goes!)

The address you have in x4 is the 'highest' address on the stack, (the first
that gets used), thread_info is at the other end, the 'lowest' address.

You used THREAD_SIZE instead of THREAD_START_SP, so the x4 value points to the
'bottom' of the page after the stack, this isn't a problem because the pre-index
addressing decrements sp before it writes, but it does break the 'and	x4, x4,
#~(THREAD_SIZE - 1)' trick to find thread_info, which I think is what the '-16'
in THREAD_START_SP is for...

6cdf9c7ca687 ("arm64: Store struct thread_info in sp_el0") got rid of most of
the users of this masking trick, maybe we can get rid of THREAD_START_SP too?
(unless it is there for some other clever reason!)


Thanks,

James


> +
> +	stp	xzr, x30, [sp, #-16]!
> +	mov	x29, sp
> +
>  	adr_l	x8, vectors			// load VBAR_EL1 with virtual
>  	msr	vbar_el1, x8			// vector table address
>  	isb
> @@ -475,16 +480,9 @@ __mmap_switched:
>  	dsb	sy				// with MMU off
>  #endif
>  
> -	adr_l	sp, initial_sp, x4
> -	mov	x4, sp
> -	and	x4, x4, #~(THREAD_SIZE - 1)
> -	msr	sp_el0, x4			// Save thread_info
> -
>  	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
>  #endif
> @@ -494,8 +492,8 @@ __mmap_switched:
>  	bl	kaslr_early_init		// parse FDT for KASLR options
>  	cbz	x0, 0f				// KASLR disabled? just proceed
>  	mov	x23, x0				// record KASLR offset
> -	ret	x28				// we must enable KASLR, return
> -						// to __enable_mmu()
> +	ldp	x29, x30, [sp], #16		// we must enable KASLR, return
> +	ret					// to __enable_mmu()
>  0:
>  #endif
>  	b	start_kernel
> 

[PATCH 3/8] arm64/kernel: use a proper stack frame in __mmap_switched()

[Ard Biesheuvel]

On 4 April 2016 at 17:33, James Morse <james.morse@arm.com> wrote:
> Hi Ard,
>
> On 04/04/16 15:52, Ard Biesheuvel wrote:
>> Considering that we can expect stack accesses from the moment we assign
>> VBAR_EL1, let's initialize the stack pointer first in __mmap_switched(),
>> and set up a proper stack frame while we're at it.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/head.S | 20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 9d8f928c355c..f441fc73a7a2 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -421,9 +421,14 @@ kernel_img_size:
>>  /*
>>   * The following fragment of code is executed with the MMU enabled.
>>   */
>> -     .set    initial_sp, init_thread_union + THREAD_START_SP
>>  __mmap_switched:
>> -     mov     x28, lr                         // preserve LR
>> +     adrp    x4, init_thread_union
>> +     add     sp, x4, #THREAD_SIZE
>> +     msr     sp_el0, x4                      // Save thread_info
>
> (Terms describing the stack are confusing, here goes!)
>
> The address you have in x4 is the 'highest' address on the stack, (the first
> that gets used), thread_info is at the other end, the 'lowest' address.
>
> You used THREAD_SIZE instead of THREAD_START_SP, so the x4 value points to the
> 'bottom' of the page after the stack, this isn't a problem because the pre-index
> addressing decrements sp before it writes, but it does break the 'and   x4, x4,
> #~(THREAD_SIZE - 1)' trick to find thread_info, which I think is what the '-16'
> in THREAD_START_SP is for...
>

True. But since the sp is decremented by the same 16 bytes in the next
instruction, the net value of sp when entering start_kernel() is
identical.

> 6cdf9c7ca687 ("arm64: Store struct thread_info in sp_el0") got rid of most of
> the users of this masking trick, maybe we can get rid of THREAD_START_SP too?
> (unless it is there for some other clever reason!)
>

I agree that the whole point of the -16 is to make the masking work.
So if we can get rid of the masking, we can get rid of
THREAD_START_SP, and also of the natural alignment of the stacks.

[PATCH 2/8] arm64/kernel: reuse boot_args array to get to __fdt_pointer

[Ard Biesheuvel]

On 4 April 2016 at 17:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 4 April 2016 at 17:13, James Morse <james.morse@arm.com> wrote:
>> Hi Ard,
>>
>> On 04/04/16 15:52, Ard Biesheuvel wrote:
>>> Since we record the boot_args [x0 .. x3] early at boot anyway, there is
>>> no need to keep the FDT address in a callee saved register with file scope
>>> in head.S. So simply refer to boot_args[0] directly in the call to
>>> setup_machine(), and drop the handling of x21 from head.S entirely.
>>
>> Won't this break Big Endian?
>> (or more accurately: bootloader-endian != kernel-endian)
>>
>> The stores in preserve_boot_args happen before the EE/E0E bits have been
>> set/cleared by el2_setup(), so they happen with boot-loader:endianness. The
>> loads after el2_setup() happen with kernel:endianness.
>>
>
> That is a very good point.

I think it would not be unreasonable to factor out the configuration
of the endianness at the current exception level, and invoke it first
before recording the boot args, since we now rely on el2_setup() to do
that. That would also fix the 'bug' where the boot_args are recorded
in the endianness of the bootloader, although I doubt anyone cares
deeply about that one.

[PATCH 1/8] arm64/kernel: use literal for relocated address of __secondary_switched

[Will Deacon]

On Mon, Apr 04, 2016 at 04:52:17PM +0200, Ard Biesheuvel wrote:
> We can simply use a relocated 64-bit literal to store the address of
> __secondary_switched(), and the relocation code will ensure that it
> holds the correct value at secondary entry time, as long as we make sure
> that the literal value is visible to the secondaries before they enable
> their MMUs. So place the literal next to kimage_vaddr, and set the alignment
> so that it is covered by the same cacheline that we already have to clean
> for a similar purpose.
> 
> 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 4203d5f257bc..69b33535911e 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -471,7 +471,8 @@ __mmap_switched:
>  	b	0b
>  
>  2:	adr_l	x8, kimage_vaddr		// make relocated kimage_vaddr
> -	dc	cvac, x8			// value visible to secondaries
> +						// and __secondary_switched
> +	dc	cvac, x8			// values visible to secondaries
>  	dsb	sy				// with MMU off
>  #endif
>  
> @@ -506,10 +507,12 @@ ENDPROC(__mmap_switched)
>   * end early head section, begin head code that is also used for
>   * hotplug and needs to have the same protections as the text region
>   */
> -	.section ".text","ax"
> -
> +	.section	".text","ax"
> +	.align		4
>  ENTRY(kimage_vaddr)

Since ENTRY already has a .align 4, can you drop the explicit directive
here?

Will

[PATCH 1/8] arm64/kernel: use literal for relocated address of __secondary_switched

[Ard Biesheuvel]

On 7 April 2016 at 11:38, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Apr 04, 2016 at 04:52:17PM +0200, Ard Biesheuvel wrote:
>> We can simply use a relocated 64-bit literal to store the address of
>> __secondary_switched(), and the relocation code will ensure that it
>> holds the correct value at secondary entry time, as long as we make sure
>> that the literal value is visible to the secondaries before they enable
>> their MMUs. So place the literal next to kimage_vaddr, and set the alignment
>> so that it is covered by the same cacheline that we already have to clean
>> for a similar purpose.
>>
>> 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 4203d5f257bc..69b33535911e 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -471,7 +471,8 @@ __mmap_switched:
>>       b       0b
>>
>>  2:   adr_l   x8, kimage_vaddr                // make relocated kimage_vaddr
>> -     dc      cvac, x8                        // value visible to secondaries
>> +                                             // and __secondary_switched
>> +     dc      cvac, x8                        // values visible to secondaries
>>       dsb     sy                              // with MMU off
>>  #endif
>>
>> @@ -506,10 +507,12 @@ ENDPROC(__mmap_switched)
>>   * end early head section, begin head code that is also used for
>>   * hotplug and needs to have the same protections as the text region
>>   */
>> -     .section ".text","ax"
>> -
>> +     .section        ".text","ax"
>> +     .align          4
>>  ENTRY(kimage_vaddr)
>
> Since ENTRY already has a .align 4, can you drop the explicit directive
> here?
>

Sure, I hadn't realised that.