[PATCH 0/3] more arm64 early boot stuff

[PATCH 0/3] more arm64 early boot stuff

[Ard Biesheuvel]

This is a bit like washing your car: you make one clean spot, and you have
to go and wash the whole thing ...

Anyway, this is another couple of proposed improvements for head.S

Patch #1 removes the unnecessary split and branch of the MMU enable code.

Patch #2 is based on the kernel relocation preparatory patch that I sent
yesterday, but reworked into something coherent, i.e., replace the open coded
virt_to_phys() calculations with absolute/relative symbol references, as
appropriate

Patch #3 adds code to warn when x1 .. x3 are not all zero as the boot protocol
stipulates. This is to ensure the 'future use' these are reserved for can ever
become a reality.

Ard Biesheuvel (3):
  arm64: merge __enable_mmu and __turn_mmu_on
  arm64: remove __calc_phys_offset
  arm64: enforce x1|x2|x3 == 0 upon kernel entry as per boot protocol

 arch/arm64/kernel/head.S  | 87 +++++++++++++++--------------------------------
 arch/arm64/kernel/setup.c | 13 +++++++
 2 files changed, 41 insertions(+), 59 deletions(-)

-- 
1.8.3.2

[PATCH 1/3] arm64: merge __enable_mmu and __turn_mmu_on

[Ard Biesheuvel]

Enabling of the MMU is split into two functions, with an align and
a branch in the middle. On arm64, the entire kernel Image is ID mapped
so this is really not necessary, and we can just merge it into a
single function.

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

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 65c7de889c8c..fb912314d5e1 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -615,8 +615,13 @@ ENDPROC(__secondary_switched)
 #endif	/* CONFIG_SMP */
 
 /*
- * Setup common bits before finally enabling the MMU. Essentially this is just
- * loading the page table pointer and vector base registers.
+ * Enable the MMU. This completely changes the structure of the visible memory
+ * space. You will not be able to trace execution through this.
+ *
+ *  x0  = system control register
+ *  x27 = *virtual* address to jump to upon completion
+ *
+ * other registers depend on the function called upon completion
  *
  * On entry to this code, x0 must contain the SCTLR_EL1 value for turning on
  * the MMU.
@@ -627,29 +632,10 @@ __enable_mmu:
 	msr	ttbr0_el1, x25			// load TTBR0
 	msr	ttbr1_el1, x26			// load TTBR1
 	isb
-	b	__turn_mmu_on
-ENDPROC(__enable_mmu)
-
-/*
- * Enable the MMU. This completely changes the structure of the visible memory
- * space. You will not be able to trace execution through this.
- *
- *  x0  = system control register
- *  x27 = *virtual* address to jump to upon completion
- *
- * other registers depend on the function called upon completion
- *
- * We align the entire function to the smallest power of two larger than it to
- * ensure it fits within a single block map entry. Otherwise were PHYS_OFFSET
- * close to the end of a 512MB or 1GB block we might require an additional
- * table to map the entire function.
- */
-	.align	4
-__turn_mmu_on:
 	msr	sctlr_el1, x0
 	isb
 	br	x27
-ENDPROC(__turn_mmu_on)
+ENDPROC(__enable_mmu)
 
 /*
  * Calculate the start of physical memory.
-- 
1.8.3.2

[PATCH 1/3] arm64: merge __enable_mmu and __turn_mmu_on

[Mark Rutland]

On Tue, Mar 17, 2015 at 10:11:12AM +0000, Ard Biesheuvel wrote:
> Enabling of the MMU is split into two functions, with an align and
> a branch in the middle. On arm64, the entire kernel Image is ID mapped
> so this is really not necessary, and we can just merge it into a
> single function.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 65c7de889c8c..fb912314d5e1 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -615,8 +615,13 @@ ENDPROC(__secondary_switched)
>  #endif	/* CONFIG_SMP */
>  
>  /*
> - * Setup common bits before finally enabling the MMU. Essentially this is just
> - * loading the page table pointer and vector base registers.
> + * Enable the MMU. This completely changes the structure of the visible memory
> + * space. You will not be able to trace execution through this.
> + *
> + *  x0  = system control register
> + *  x27 = *virtual* address to jump to upon completion
> + *
> + * other registers depend on the function called upon completion
>   *
>   * On entry to this code, x0 must contain the SCTLR_EL1 value for turning on
>   * the MMU.

Minor nit: it's a bit redundant to have both comments regarding the
sctlr_el1.

I'd recommed we drop/fold the 2nd comment into the first like:

	x0  = SCTLR_EL1 value for turning on the MMU.

Otherwise, this looks good.

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

Mark.

> @@ -627,29 +632,10 @@ __enable_mmu:
>  	msr	ttbr0_el1, x25			// load TTBR0
>  	msr	ttbr1_el1, x26			// load TTBR1
>  	isb
> -	b	__turn_mmu_on
> -ENDPROC(__enable_mmu)
> -
> -/*
> - * Enable the MMU. This completely changes the structure of the visible memory
> - * space. You will not be able to trace execution through this.
> - *
> - *  x0  = system control register
> - *  x27 = *virtual* address to jump to upon completion
> - *
> - * other registers depend on the function called upon completion
> - *
> - * We align the entire function to the smallest power of two larger than it to
> - * ensure it fits within a single block map entry. Otherwise were PHYS_OFFSET
> - * close to the end of a 512MB or 1GB block we might require an additional
> - * table to map the entire function.
> - */
> -	.align	4
> -__turn_mmu_on:
>  	msr	sctlr_el1, x0
>  	isb
>  	br	x27
> -ENDPROC(__turn_mmu_on)
> +ENDPROC(__enable_mmu)
>  
>  /*
>   * Calculate the start of physical memory.
> -- 
> 1.8.3.2
> 
> 

[PATCH 1/3] arm64: merge __enable_mmu and __turn_mmu_on

[Christopher Covington]

On 03/17/2015 06:11 AM, Ard Biesheuvel wrote:
> Enabling of the MMU is split into two functions, with an align and
> a branch in the middle. On arm64, the entire kernel Image is ID mapped
> so this is really not necessary, and we can just merge it into a
> single function.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 65c7de889c8c..fb912314d5e1 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -615,8 +615,13 @@ ENDPROC(__secondary_switched)
>  #endif	/* CONFIG_SMP */
>  
>  /*
> - * Setup common bits before finally enabling the MMU. Essentially this is just
> - * loading the page table pointer and vector base registers.
> + * Enable the MMU. This completely changes the structure of the visible memory
> + * space. You will not be able to trace execution through this.

I don't understand the last sentence. I recall being able to read and
eventually understand simulator instruction traces of this code. Is the
sentence referring to the Embedded Trace Macrocell or something?

Chris

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] arm64: merge __enable_mmu and __turn_mmu_on

[Ard Biesheuvel]

On 17 March 2015 at 18:39, Christopher Covington <cov@codeaurora.org> wrote:
> On 03/17/2015 06:11 AM, Ard Biesheuvel wrote:
>> Enabling of the MMU is split into two functions, with an align and
>> a branch in the middle. On arm64, the entire kernel Image is ID mapped
>> so this is really not necessary, and we can just merge it into a
>> single function.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/head.S | 30 ++++++++----------------------
>>  1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 65c7de889c8c..fb912314d5e1 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -615,8 +615,13 @@ ENDPROC(__secondary_switched)
>>  #endif       /* CONFIG_SMP */
>>
>>  /*
>> - * Setup common bits before finally enabling the MMU. Essentially this is just
>> - * loading the page table pointer and vector base registers.
>> + * Enable the MMU. This completely changes the structure of the visible memory
>> + * space. You will not be able to trace execution through this.
>
> I don't understand the last sentence. I recall being able to read and
> eventually understand simulator instruction traces of this code. Is the
> sentence referring to the Embedded Trace Macrocell or something?
>

I guess the comment is a bit stale: it was inherited from the ARM
version, where older platforms only have a single TTBR register, and
switching address spaces is a bit more involved. On arm64, however,
there are always two TTBR registers at EL1, and the address spaced
they represent can never overlap, so there it isn't such a big deal.

[PATCH 1/3] arm64: merge __enable_mmu and __turn_mmu_on

[Mark Rutland]

> >> - * Setup common bits before finally enabling the MMU. Essentially this is just
> >> - * loading the page table pointer and vector base registers.
> >> + * Enable the MMU. This completely changes the structure of the visible memory
> >> + * space. You will not be able to trace execution through this.
> >
> > I don't understand the last sentence. I recall being able to read and
> > eventually understand simulator instruction traces of this code. Is the
> > sentence referring to the Embedded Trace Macrocell or something?
> >
> 
> I guess the comment is a bit stale: it was inherited from the ARM
> version, where older platforms only have a single TTBR register, and
> switching address spaces is a bit more involved. On arm64, however,
> there are always two TTBR registers at EL1, and the address spaced
> they represent can never overlap, so there it isn't such a big deal.

Indeed. Feel free to drop it if you want.

Mark.

[PATCH 2/3] arm64: remove __calc_phys_offset

[Ard Biesheuvel]

This removes the function __calc_phys_offset and all open coded
virtual to physical address translations using the offset kept
in x28.

Instead, just use absolute or PC-relative symbol references as
appropriate when referring to virtual or physical addresses,
respectively.

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

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index fb912314d5e1..1651c0fd50e6 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -36,8 +36,6 @@
 #include <asm/page.h>
 #include <asm/virt.h>
 
-#define KERNEL_RAM_VADDR	(PAGE_OFFSET + TEXT_OFFSET)
-
 #if (TEXT_OFFSET & 0xfff) != 0
 #error TEXT_OFFSET must be at least 4KB aligned
 #elif (PAGE_OFFSET & 0x1fffff) != 0
@@ -46,13 +44,6 @@
 #error TEXT_OFFSET must be less than 2MB
 #endif
 
-	.macro	pgtbl, ttb0, ttb1, virt_to_phys
-	ldr	\ttb1, =swapper_pg_dir
-	ldr	\ttb0, =idmap_pg_dir
-	add	\ttb1, \ttb1, \virt_to_phys
-	add	\ttb0, \ttb0, \virt_to_phys
-	.endm
-
 #ifdef CONFIG_ARM64_64K_PAGES
 #define BLOCK_SHIFT	PAGE_SHIFT
 #define BLOCK_SIZE	PAGE_SIZE
@@ -63,7 +54,7 @@
 #define TABLE_SHIFT	PUD_SHIFT
 #endif
 
-#define KERNEL_START	KERNEL_RAM_VADDR
+#define KERNEL_START	_text
 #define KERNEL_END	_end
 
 /*
@@ -242,7 +233,7 @@ section_table:
 ENTRY(stext)
 	mov	x21, x0				// x21=FDT
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
-	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
+	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
 	bl	set_cpu_boot_mode_flag
 
 	bl	__vet_fdt
@@ -343,7 +334,8 @@ ENDPROC(__vet_fdt)
  *   - pgd entry for fixed mappings (TTBR1)
  */
 __create_page_tables:
-	pgtbl	x25, x26, x28			// idmap_pg_dir and swapper_pg_dir addresses
+	adrp    x25, idmap_pg_dir
+	adrp    x26, swapper_pg_dir
 	mov	x27, lr
 
 	/*
@@ -372,12 +364,10 @@ __create_page_tables:
 	 * Create the identity mapping.
 	 */
 	mov	x0, x25				// idmap_pg_dir
-	ldr	x3, =KERNEL_START
-	add	x3, x3, x28			// __pa(KERNEL_START)
+	adrp	x3, KERNEL_START		// __pa(KERNEL_START)
 	create_pgd_entry x0, x3, x5, x6
-	ldr	x6, =KERNEL_END
 	mov	x5, x3				// __pa(KERNEL_START)
-	add	x6, x6, x28			// __pa(KERNEL_END)
+	adr_l	x6, KERNEL_END			// __pa(KERNEL_END)
 	create_block_map x0, x7, x3, x5, x6
 
 	/*
@@ -386,7 +376,7 @@ __create_page_tables:
 	mov	x0, x26				// swapper_pg_dir
 	mov	x5, #PAGE_OFFSET
 	create_pgd_entry x0, x5, x3, x6
-	ldr	x6, =KERNEL_END
+	ldr	x6, =KERNEL_END			// __va(KERNEL_END)
 	mov	x3, x24				// phys offset
 	create_block_map x0, x7, x3, x5, x6
 
@@ -538,8 +528,7 @@ ENDPROC(el2_setup)
  * in x20. See arch/arm64/include/asm/virt.h for more info.
  */
 ENTRY(set_cpu_boot_mode_flag)
-	ldr	x1, =__boot_cpu_mode		// Compute __boot_cpu_mode
-	add	x1, x1, x28
+	adr_l	x1, __boot_cpu_mode
 	cmp	w20, #BOOT_CPU_MODE_EL2
 	b.ne	1f
 	add	x1, x1, #4
@@ -570,7 +559,7 @@ ENTRY(__boot_cpu_mode)
 	 */
 ENTRY(secondary_holding_pen)
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
-	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
+	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
 	bl	set_cpu_boot_mode_flag
 	mrs	x0, mpidr_el1
 	ldr     x1, =MPIDR_HWID_BITMASK
@@ -589,7 +578,7 @@ ENDPROC(secondary_holding_pen)
 	 */
 ENTRY(secondary_entry)
 	bl	el2_setup			// Drop to EL1
-	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
+	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
 	bl	set_cpu_boot_mode_flag
 	b	secondary_startup
 ENDPROC(secondary_entry)
@@ -598,7 +587,8 @@ ENTRY(secondary_startup)
 	/*
 	 * Common entry point for secondary CPUs.
 	 */
-	pgtbl	x25, x26, x28			// x25=TTBR0, x26=TTBR1
+	adrp    x25, idmap_pg_dir
+	adrp    x26, swapper_pg_dir
 	bl	__cpu_setup			// initialise processor
 
 	ldr	x21, =secondary_data
@@ -636,18 +626,3 @@ __enable_mmu:
 	isb
 	br	x27
 ENDPROC(__enable_mmu)
-
-/*
- * Calculate the start of physical memory.
- */
-__calc_phys_offset:
-	adr	x0, 1f
-	ldp	x1, x2, [x0]
-	sub	x28, x0, x1			// x28 = PHYS_OFFSET - PAGE_OFFSET
-	add	x24, x2, x28			// x24 = PHYS_OFFSET
-	ret
-ENDPROC(__calc_phys_offset)
-
-	.align 3
-1:	.quad	.
-	.quad	PAGE_OFFSET
-- 
1.8.3.2

[PATCH 2/3] arm64: remove __calc_phys_offset

[Mark Rutland]

On Tue, Mar 17, 2015 at 10:11:13AM +0000, Ard Biesheuvel wrote:
> This removes the function __calc_phys_offset and all open coded
> virtual to physical address translations using the offset kept
> in x28.
> 
> Instead, just use absolute or PC-relative symbol references as
> appropriate when referring to virtual or physical addresses,
> respectively.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S | 49 ++++++++++++------------------------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index fb912314d5e1..1651c0fd50e6 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -36,8 +36,6 @@
>  #include <asm/page.h>
>  #include <asm/virt.h>
>  
> -#define KERNEL_RAM_VADDR	(PAGE_OFFSET + TEXT_OFFSET)
> -
>  #if (TEXT_OFFSET & 0xfff) != 0
>  #error TEXT_OFFSET must be at least 4KB aligned
>  #elif (PAGE_OFFSET & 0x1fffff) != 0
> @@ -46,13 +44,6 @@
>  #error TEXT_OFFSET must be less than 2MB
>  #endif
>  
> -	.macro	pgtbl, ttb0, ttb1, virt_to_phys
> -	ldr	\ttb1, =swapper_pg_dir
> -	ldr	\ttb0, =idmap_pg_dir
> -	add	\ttb1, \ttb1, \virt_to_phys
> -	add	\ttb0, \ttb0, \virt_to_phys
> -	.endm
> -
>  #ifdef CONFIG_ARM64_64K_PAGES
>  #define BLOCK_SHIFT	PAGE_SHIFT
>  #define BLOCK_SIZE	PAGE_SIZE
> @@ -63,7 +54,7 @@
>  #define TABLE_SHIFT	PUD_SHIFT
>  #endif
>  
> -#define KERNEL_START	KERNEL_RAM_VADDR
> +#define KERNEL_START	_text
>  #define KERNEL_END	_end
>  
>  /*
> @@ -242,7 +233,7 @@ section_table:
>  ENTRY(stext)
>  	mov	x21, x0				// x21=FDT
>  	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
> -	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
> +	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET

Neat!

Perhaps we could have:

#define	PHYS_OFFSET	(KERNEL_START - TEXT_OFFSET)

Which would make the all the PHYS_OFFSET calculations self-documenting.
It shouldn't clash with the asm/memory.h definition because that's in an
#ifndef __ASSEMBLY__ block.

Either way it all looks correct (and it's nice to see head.S shrink),
so:

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

I assume you'll add this to the series introducing adr_l and friends?

Mark.

>  	bl	set_cpu_boot_mode_flag
>  
>  	bl	__vet_fdt
> @@ -343,7 +334,8 @@ ENDPROC(__vet_fdt)
>   *   - pgd entry for fixed mappings (TTBR1)
>   */
>  __create_page_tables:
> -	pgtbl	x25, x26, x28			// idmap_pg_dir and swapper_pg_dir addresses
> +	adrp    x25, idmap_pg_dir
> +	adrp    x26, swapper_pg_dir
>  	mov	x27, lr
>  
>  	/*
> @@ -372,12 +364,10 @@ __create_page_tables:
>  	 * Create the identity mapping.
>  	 */
>  	mov	x0, x25				// idmap_pg_dir
> -	ldr	x3, =KERNEL_START
> -	add	x3, x3, x28			// __pa(KERNEL_START)
> +	adrp	x3, KERNEL_START		// __pa(KERNEL_START)
>  	create_pgd_entry x0, x3, x5, x6
> -	ldr	x6, =KERNEL_END
>  	mov	x5, x3				// __pa(KERNEL_START)
> -	add	x6, x6, x28			// __pa(KERNEL_END)
> +	adr_l	x6, KERNEL_END			// __pa(KERNEL_END)
>  	create_block_map x0, x7, x3, x5, x6
>  
>  	/*
> @@ -386,7 +376,7 @@ __create_page_tables:
>  	mov	x0, x26				// swapper_pg_dir
>  	mov	x5, #PAGE_OFFSET
>  	create_pgd_entry x0, x5, x3, x6
> -	ldr	x6, =KERNEL_END
> +	ldr	x6, =KERNEL_END			// __va(KERNEL_END)
>  	mov	x3, x24				// phys offset
>  	create_block_map x0, x7, x3, x5, x6
>  
> @@ -538,8 +528,7 @@ ENDPROC(el2_setup)
>   * in x20. See arch/arm64/include/asm/virt.h for more info.
>   */
>  ENTRY(set_cpu_boot_mode_flag)
> -	ldr	x1, =__boot_cpu_mode		// Compute __boot_cpu_mode
> -	add	x1, x1, x28
> +	adr_l	x1, __boot_cpu_mode
>  	cmp	w20, #BOOT_CPU_MODE_EL2
>  	b.ne	1f
>  	add	x1, x1, #4
> @@ -570,7 +559,7 @@ ENTRY(__boot_cpu_mode)
>  	 */
>  ENTRY(secondary_holding_pen)
>  	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
> -	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
> +	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
>  	bl	set_cpu_boot_mode_flag
>  	mrs	x0, mpidr_el1
>  	ldr     x1, =MPIDR_HWID_BITMASK
> @@ -589,7 +578,7 @@ ENDPROC(secondary_holding_pen)
>  	 */
>  ENTRY(secondary_entry)
>  	bl	el2_setup			// Drop to EL1
> -	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
> +	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
>  	bl	set_cpu_boot_mode_flag
>  	b	secondary_startup
>  ENDPROC(secondary_entry)
> @@ -598,7 +587,8 @@ ENTRY(secondary_startup)
>  	/*
>  	 * Common entry point for secondary CPUs.
>  	 */
> -	pgtbl	x25, x26, x28			// x25=TTBR0, x26=TTBR1
> +	adrp    x25, idmap_pg_dir
> +	adrp    x26, swapper_pg_dir
>  	bl	__cpu_setup			// initialise processor
>  
>  	ldr	x21, =secondary_data
> @@ -636,18 +626,3 @@ __enable_mmu:
>  	isb
>  	br	x27
>  ENDPROC(__enable_mmu)
> -
> -/*
> - * Calculate the start of physical memory.
> - */
> -__calc_phys_offset:
> -	adr	x0, 1f
> -	ldp	x1, x2, [x0]
> -	sub	x28, x0, x1			// x28 = PHYS_OFFSET - PAGE_OFFSET
> -	add	x24, x2, x28			// x24 = PHYS_OFFSET
> -	ret
> -ENDPROC(__calc_phys_offset)
> -
> -	.align 3
> -1:	.quad	.
> -	.quad	PAGE_OFFSET
> -- 
> 1.8.3.2
> 
> 

[PATCH 2/3] arm64: remove __calc_phys_offset

[Ard Biesheuvel]

On 17 March 2015 at 15:46, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Mar 17, 2015 at 10:11:13AM +0000, Ard Biesheuvel wrote:
>> This removes the function __calc_phys_offset and all open coded
>> virtual to physical address translations using the offset kept
>> in x28.
>>
>> Instead, just use absolute or PC-relative symbol references as
>> appropriate when referring to virtual or physical addresses,
>> respectively.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/head.S | 49 ++++++++++++------------------------------------
>>  1 file changed, 12 insertions(+), 37 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index fb912314d5e1..1651c0fd50e6 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -36,8 +36,6 @@
>>  #include <asm/page.h>
>>  #include <asm/virt.h>
>>
>> -#define KERNEL_RAM_VADDR     (PAGE_OFFSET + TEXT_OFFSET)
>> -
>>  #if (TEXT_OFFSET & 0xfff) != 0
>>  #error TEXT_OFFSET must be at least 4KB aligned
>>  #elif (PAGE_OFFSET & 0x1fffff) != 0
>> @@ -46,13 +44,6 @@
>>  #error TEXT_OFFSET must be less than 2MB
>>  #endif
>>
>> -     .macro  pgtbl, ttb0, ttb1, virt_to_phys
>> -     ldr     \ttb1, =swapper_pg_dir
>> -     ldr     \ttb0, =idmap_pg_dir
>> -     add     \ttb1, \ttb1, \virt_to_phys
>> -     add     \ttb0, \ttb0, \virt_to_phys
>> -     .endm
>> -
>>  #ifdef CONFIG_ARM64_64K_PAGES
>>  #define BLOCK_SHIFT  PAGE_SHIFT
>>  #define BLOCK_SIZE   PAGE_SIZE
>> @@ -63,7 +54,7 @@
>>  #define TABLE_SHIFT  PUD_SHIFT
>>  #endif
>>
>> -#define KERNEL_START KERNEL_RAM_VADDR
>> +#define KERNEL_START _text
>>  #define KERNEL_END   _end
>>
>>  /*
>> @@ -242,7 +233,7 @@ section_table:
>>  ENTRY(stext)
>>       mov     x21, x0                         // x21=FDT
>>       bl      el2_setup                       // Drop to EL1, w20=cpu_boot_mode
>> -     bl      __calc_phys_offset              // x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
>> +     adrp    x24, KERNEL_START - TEXT_OFFSET // x24=PHYS_OFFSET
>
> Neat!
>
> Perhaps we could have:
>
> #define PHYS_OFFSET     (KERNEL_START - TEXT_OFFSET)
>
> Which would make the all the PHYS_OFFSET calculations self-documenting.
> It shouldn't clash with the asm/memory.h definition because that's in an
> #ifndef __ASSEMBLY__ block.
>

I will use __PHYS_OFFSET instead, to prevent confusion.

Btw, I will drop the secondary 'adrp x24, __PHYS_OFFSET' lines
completely, as the secondary boot path never refers to x24.

> Either way it all looks correct (and it's nice to see head.S shrink),
> so:
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>

Thanks

> I assume you'll add this to the series introducing adr_l and friends?
>

Yes, that is the idea. I will resend the entire series today, to round
up the missing acks.

-- 
Ard.

>>       bl      set_cpu_boot_mode_flag
>>
>>       bl      __vet_fdt
>> @@ -343,7 +334,8 @@ ENDPROC(__vet_fdt)
>>   *   - pgd entry for fixed mappings (TTBR1)
>>   */
>>  __create_page_tables:
>> -     pgtbl   x25, x26, x28                   // idmap_pg_dir and swapper_pg_dir addresses
>> +     adrp    x25, idmap_pg_dir
>> +     adrp    x26, swapper_pg_dir
>>       mov     x27, lr
>>
>>       /*
>> @@ -372,12 +364,10 @@ __create_page_tables:
>>        * Create the identity mapping.
>>        */
>>       mov     x0, x25                         // idmap_pg_dir
>> -     ldr     x3, =KERNEL_START
>> -     add     x3, x3, x28                     // __pa(KERNEL_START)
>> +     adrp    x3, KERNEL_START                // __pa(KERNEL_START)
>>       create_pgd_entry x0, x3, x5, x6
>> -     ldr     x6, =KERNEL_END
>>       mov     x5, x3                          // __pa(KERNEL_START)
>> -     add     x6, x6, x28                     // __pa(KERNEL_END)
>> +     adr_l   x6, KERNEL_END                  // __pa(KERNEL_END)
>>       create_block_map x0, x7, x3, x5, x6
>>
>>       /*
>> @@ -386,7 +376,7 @@ __create_page_tables:
>>       mov     x0, x26                         // swapper_pg_dir
>>       mov     x5, #PAGE_OFFSET
>>       create_pgd_entry x0, x5, x3, x6
>> -     ldr     x6, =KERNEL_END
>> +     ldr     x6, =KERNEL_END                 // __va(KERNEL_END)
>>       mov     x3, x24                         // phys offset
>>       create_block_map x0, x7, x3, x5, x6
>>
>> @@ -538,8 +528,7 @@ ENDPROC(el2_setup)
>>   * in x20. See arch/arm64/include/asm/virt.h for more info.
>>   */
>>  ENTRY(set_cpu_boot_mode_flag)
>> -     ldr     x1, =__boot_cpu_mode            // Compute __boot_cpu_mode
>> -     add     x1, x1, x28
>> +     adr_l   x1, __boot_cpu_mode
>>       cmp     w20, #BOOT_CPU_MODE_EL2
>>       b.ne    1f
>>       add     x1, x1, #4
>> @@ -570,7 +559,7 @@ ENTRY(__boot_cpu_mode)
>>        */
>>  ENTRY(secondary_holding_pen)
>>       bl      el2_setup                       // Drop to EL1, w20=cpu_boot_mode
>> -     bl      __calc_phys_offset              // x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
>> +     adrp    x24, KERNEL_START - TEXT_OFFSET // x24=PHYS_OFFSET
>>       bl      set_cpu_boot_mode_flag
>>       mrs     x0, mpidr_el1
>>       ldr     x1, =MPIDR_HWID_BITMASK
>> @@ -589,7 +578,7 @@ ENDPROC(secondary_holding_pen)
>>        */
>>  ENTRY(secondary_entry)
>>       bl      el2_setup                       // Drop to EL1
>> -     bl      __calc_phys_offset              // x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
>> +     adrp    x24, KERNEL_START - TEXT_OFFSET // x24=PHYS_OFFSET
>>       bl      set_cpu_boot_mode_flag
>>       b       secondary_startup
>>  ENDPROC(secondary_entry)
>> @@ -598,7 +587,8 @@ ENTRY(secondary_startup)
>>       /*
>>        * Common entry point for secondary CPUs.
>>        */
>> -     pgtbl   x25, x26, x28                   // x25=TTBR0, x26=TTBR1
>> +     adrp    x25, idmap_pg_dir
>> +     adrp    x26, swapper_pg_dir
>>       bl      __cpu_setup                     // initialise processor
>>
>>       ldr     x21, =secondary_data
>> @@ -636,18 +626,3 @@ __enable_mmu:
>>       isb
>>       br      x27
>>  ENDPROC(__enable_mmu)
>> -
>> -/*
>> - * Calculate the start of physical memory.
>> - */
>> -__calc_phys_offset:
>> -     adr     x0, 1f
>> -     ldp     x1, x2, [x0]
>> -     sub     x28, x0, x1                     // x28 = PHYS_OFFSET - PAGE_OFFSET
>> -     add     x24, x2, x28                    // x24 = PHYS_OFFSET
>> -     ret
>> -ENDPROC(__calc_phys_offset)
>> -
>> -     .align 3
>> -1:   .quad   .
>> -     .quad   PAGE_OFFSET
>> --
>> 1.8.3.2
>>
>>

[PATCH 2/3] arm64: remove __calc_phys_offset

[Mark Rutland]

> >> -     bl      __calc_phys_offset              // x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
> >> +     adrp    x24, KERNEL_START - TEXT_OFFSET // x24=PHYS_OFFSET
> >
> > Neat!
> >
> > Perhaps we could have:
> >
> > #define PHYS_OFFSET     (KERNEL_START - TEXT_OFFSET)
> >
> > Which would make the all the PHYS_OFFSET calculations self-documenting.
> > It shouldn't clash with the asm/memory.h definition because that's in an
> > #ifndef __ASSEMBLY__ block.
> >
> 
> I will use __PHYS_OFFSET instead, to prevent confusion.
> 
> Btw, I will drop the secondary 'adrp x24, __PHYS_OFFSET' lines
> completely, as the secondary boot path never refers to x24.

Sounds good to me on both counts.

Mark.

[PATCH 3/3] arm64: enforce x1|x2|x3 == 0 upon kernel entry as per boot protocol

[Ard Biesheuvel]

According to the arm64 boot protocol, registers x1 to x3 should be
zero upon kernel entry, and non-zero values are reserved for future
use. This future use is going to be problematic if we never enforce
the current rules, so start enforcing them now, by emitting a warning
if non-zero values are detected.

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

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 1651c0fd50e6..fe5354eae069 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -231,6 +231,10 @@ section_table:
 #endif
 
 ENTRY(stext)
+	adr	x8, boot_regs			// record the contents of
+	stp	x0, x1, [x8]			// x0 .. x3 at kernel entry
+	stp	x2, x3, [x8, #16]
+
 	mov	x21, x0				// x21=FDT
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
 	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
@@ -251,6 +255,10 @@ ENTRY(stext)
 	b	__cpu_setup			// initialise processor
 ENDPROC(stext)
 
+	.align	3
+ENTRY(boot_regs)
+	.skip	4 * 8				// x0 .. x3
+
 /*
  * Determine validity of the x21 FDT pointer.
  * The dtb must be 8-byte aligned and live in the first 512M of memory.
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 6c5fb5aff325..2b81d0a907ce 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -532,3 +532,16 @@ const struct seq_operations cpuinfo_op = {
 	.stop	= c_stop,
 	.show	= c_show
 };
+
+static int verify_boot_protocol(void)
+{
+	extern u64 boot_regs[];
+
+	if (boot_regs[1] || boot_regs[2] || boot_regs[3]) {
+		pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
+			boot_regs[1], boot_regs[2], boot_regs[3]);
+		pr_err("WARNING: your bootloader may fail to load newer kernels\n");
+	}
+	return 0;
+}
+late_initcall(verify_boot_protocol);
-- 
1.8.3.2

[PATCH 3/3] arm64: enforce x1|x2|x3 == 0 upon kernel entry as per boot protocol

[Mark Rutland]

On Tue, Mar 17, 2015 at 10:11:14AM +0000, Ard Biesheuvel wrote:
> According to the arm64 boot protocol, registers x1 to x3 should be
> zero upon kernel entry, and non-zero values are reserved for future
> use. This future use is going to be problematic if we never enforce
> the current rules, so start enforcing them now, by emitting a warning
> if non-zero values are detected.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S  |  8 ++++++++
>  arch/arm64/kernel/setup.c | 13 +++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 1651c0fd50e6..fe5354eae069 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -231,6 +231,10 @@ section_table:
>  #endif
>  
>  ENTRY(stext)
> +	adr	x8, boot_regs			// record the contents of
> +	stp	x0, x1, [x8]			// x0 .. x3 at kernel entry
> +	stp	x2, x3, [x8, #16]

These accesses will be non-cacheable, but the caches could have clean
entries for this PA range.

Without an invalidate between the writes and reads, we might see stale
values (the other option is to preserve the registers across enabling
the MMU).

> +
>  	mov	x21, x0				// x21=FDT
>  	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
>  	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
> @@ -251,6 +255,10 @@ ENTRY(stext)
>  	b	__cpu_setup			// initialise processor
>  ENDPROC(stext)
>  
> +	.align	3
> +ENTRY(boot_regs)
> +	.skip	4 * 8				// x0 .. x3

Can't we can drop this here...

> +
>  /*
>   * Determine validity of the x21 FDT pointer.
>   * The dtb must be 8-byte aligned and live in the first 512M of memory.
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 6c5fb5aff325..2b81d0a907ce 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -532,3 +532,16 @@ const struct seq_operations cpuinfo_op = {
>  	.stop	= c_stop,
>  	.show	= c_show
>  };

... and here have

u64 boot_regs[4];

Using adrp + lo12 to address the symbol?

> +
> +static int verify_boot_protocol(void)
> +{
> +	extern u64 boot_regs[];
> +
> +	if (boot_regs[1] || boot_regs[2] || boot_regs[3]) {
> +		pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
> +			boot_regs[1], boot_regs[2], boot_regs[3]);
> +		pr_err("WARNING: your bootloader may fail to load newer kernels\n");
> +	}
> +	return 0;
> +}
> +late_initcall(verify_boot_protocol);

I think it would be nicer to plumb this into setup_arch rather than an
initcall. That way the warning will be at a consistent, prominent
location in the log.

Thanks,
Mark.

[PATCH 3/3] arm64: enforce x1|x2|x3 == 0 upon kernel entry as per boot protocol

[Christopher Covington]

On 03/17/2015 06:11 AM, Ard Biesheuvel wrote:
> According to the arm64 boot protocol, registers x1 to x3 should be
> zero upon kernel entry, and non-zero values are reserved for future
> use. This future use is going to be problematic if we never enforce
> the current rules, so start enforcing them now, by emitting a warning
> if non-zero values are detected.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S  |  8 ++++++++
>  arch/arm64/kernel/setup.c | 13 +++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 1651c0fd50e6..fe5354eae069 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -231,6 +231,10 @@ section_table:
>  #endif
>  
>  ENTRY(stext)
> +	adr	x8, boot_regs			// record the contents of
> +	stp	x0, x1, [x8]			// x0 .. x3 at kernel entry
> +	stp	x2, x3, [x8, #16]
> +
>  	mov	x21, x0				// x21=FDT
>  	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
>  	adrp	x24, KERNEL_START - TEXT_OFFSET	// x24=PHYS_OFFSET
> @@ -251,6 +255,10 @@ ENTRY(stext)
>  	b	__cpu_setup			// initialise processor
>  ENDPROC(stext)
>  
> +	.align	3
> +ENTRY(boot_regs)
> +	.skip	4 * 8				// x0 .. x3
> +
>  /*
>   * Determine validity of the x21 FDT pointer.
>   * The dtb must be 8-byte aligned and live in the first 512M of memory.
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 6c5fb5aff325..2b81d0a907ce 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -532,3 +532,16 @@ const struct seq_operations cpuinfo_op = {
>  	.stop	= c_stop,
>  	.show	= c_show
>  };
> +
> +static int verify_boot_protocol(void)
> +{
> +	extern u64 boot_regs[];
> +
> +	if (boot_regs[1] || boot_regs[2] || boot_regs[3]) {
> +		pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
> +			boot_regs[1], boot_regs[2], boot_regs[3]);
> +		pr_err("WARNING: your bootloader may fail to load newer kernels\n");

pr_warn?

> +	}
> +	return 0;
> +}
> +late_initcall(verify_boot_protocol);
> 

Chris

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] arm64: enforce x1|x2|x3 == 0 upon kernel entry as per boot protocol

[Ard Biesheuvel]

On 17 March 2015 at 18:47, Christopher Covington <cov@codeaurora.org> wrote:
> On 03/17/2015 06:11 AM, Ard Biesheuvel wrote:
>> According to the arm64 boot protocol, registers x1 to x3 should be
>> zero upon kernel entry, and non-zero values are reserved for future
>> use. This future use is going to be problematic if we never enforce
>> the current rules, so start enforcing them now, by emitting a warning
>> if non-zero values are detected.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/head.S  |  8 ++++++++
>>  arch/arm64/kernel/setup.c | 13 +++++++++++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 1651c0fd50e6..fe5354eae069 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -231,6 +231,10 @@ section_table:
>>  #endif
>>
>>  ENTRY(stext)
>> +     adr     x8, boot_regs                   // record the contents of
>> +     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
>> +     stp     x2, x3, [x8, #16]
>> +
>>       mov     x21, x0                         // x21=FDT
>>       bl      el2_setup                       // Drop to EL1, w20=cpu_boot_mode
>>       adrp    x24, KERNEL_START - TEXT_OFFSET // x24=PHYS_OFFSET
>> @@ -251,6 +255,10 @@ ENTRY(stext)
>>       b       __cpu_setup                     // initialise processor
>>  ENDPROC(stext)
>>
>> +     .align  3
>> +ENTRY(boot_regs)
>> +     .skip   4 * 8                           // x0 .. x3
>> +
>>  /*
>>   * Determine validity of the x21 FDT pointer.
>>   * The dtb must be 8-byte aligned and live in the first 512M of memory.
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 6c5fb5aff325..2b81d0a907ce 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -532,3 +532,16 @@ const struct seq_operations cpuinfo_op = {
>>       .stop   = c_stop,
>>       .show   = c_show
>>  };
>> +
>> +static int verify_boot_protocol(void)
>> +{
>> +     extern u64 boot_regs[];
>> +
>> +     if (boot_regs[1] || boot_regs[2] || boot_regs[3]) {
>> +             pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
>> +                     boot_regs[1], boot_regs[2], boot_regs[3]);
>> +             pr_err("WARNING: your bootloader may fail to load newer kernels\n");
>
> pr_warn?
>

Semantically more correct, perhaps, but pr_err() should be slightly
noisier, which is preferred here imo

>> +     }
>> +     return 0;
>> +}
>> +late_initcall(verify_boot_protocol);
>>
>
> Chris
>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project