arm64: head.S cleanup

From: Marc Zyngier <[email protected]>

struct cpu_table is an artifact left from the (very) early days of the arm64 port, and its only real use is to allow the most beautiful "AArch64 Processor" string to be displayed at boot time.

Really? Yes, really.

Let's get rid of it. In order to avoid another BogoMips-gate, the aforementioned string is preserved.

Acked-by Catalin Marinas <[email protected]>

Acked-by Ard Biesheuvel <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/cputable.h | 30 ----------------
 arch/arm64/kernel/Makefile        |  2 +-
 arch/arm64/kernel/asm-offsets.c   |  4 ---
 arch/arm64/kernel/cputable.c      | 33 -----------------
 arch/arm64/kernel/head.S          | 76 +++------------------------------------
 arch/arm64/kernel/setup.c         | 16 ++-------
 6 files changed, 8 insertions(+), 153 deletions(-)
 delete mode 100644 arch/arm64/include/asm/cputable.h
 delete mode 100644 arch/arm64/kernel/cputable.c

arch/arm64/include/asm/cputable.h deleted +0 -30

deleted file mode 100644
index e3bd983d3661..000000000000
@@ -1,30 +0,0 @@
-/*
- * arch/arm64/include/asm/cputable.h
- *
- * Copyright (C) 2012 ARM Ltd.
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#ifndef __ASM_CPUTABLE_H
-#define __ASM_CPUTABLE_H
-
-struct cpu_info {
-	unsigned int	cpu_id_val;
-	unsigned int	cpu_id_mask;
-	const char	*cpu_name;
-	unsigned long	(*cpu_setup)(void);
-};
-
-extern struct cpu_info *lookup_processor_type(unsigned int);
-
-#endif

arch/arm64/kernel/Makefile +1 -1

index 5ee07eee80c2..d5e70747c7a2 100644
@@ -12,7 +12,7 @@ CFLAGS_REMOVE_insn.o = -pg
 CFLAGS_REMOVE_return_address.o = -pg
 
 # Object file lists.
-arm64-obj-y		:= cputable.o debug-monitors.o entry.o irq.o fpsimd.o	\
+arm64-obj-y		:= debug-monitors.o entry.o irq.o fpsimd.o		\
 			   entry-fpsimd.o process.o ptrace.o setup.o signal.o	\
 			   sys.o stacktrace.o time.o traps.o io.o vdso.o	\
 			   hyp-stub.o psci.o psci-call.o cpu_ops.o insn.o	\

arch/arm64/kernel/asm-offsets.c +0 -4

index f7fa65d4c352..14dd3d1afa57 100644
@@ -24,7 +24,6 @@
 #include <linux/kvm_host.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
-#include <asm/cputable.h>
 #include <asm/smp_plat.h>
 #include <asm/suspend.h>
 #include <asm/vdso_datapage.h>

@@ -71,9 +70,6 @@ int main(void)
   BLANK();
   DEFINE(PAGE_SZ,	       	PAGE_SIZE);
   BLANK();
-  DEFINE(CPU_INFO_SZ,		sizeof(struct cpu_info));
-  DEFINE(CPU_INFO_SETUP,	offsetof(struct cpu_info, cpu_setup));
-  BLANK();
   DEFINE(DMA_BIDIRECTIONAL,	DMA_BIDIRECTIONAL);
   DEFINE(DMA_TO_DEVICE,		DMA_TO_DEVICE);
   DEFINE(DMA_FROM_DEVICE,	DMA_FROM_DEVICE);

arch/arm64/kernel/cputable.c deleted +0 -33

deleted file mode 100644
index fd3993cb060f..000000000000
@@ -1,33 +0,0 @@
-/*
- * arch/arm64/kernel/cputable.c
- *
- * Copyright (C) 2012 ARM Ltd.
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/init.h>
-
-#include <asm/cputable.h>
-
-extern unsigned long __cpu_setup(void);
-
-struct cpu_info cpu_table[] = {
-	{
-		.cpu_id_val	= 0x000f0000,
-		.cpu_id_mask	= 0x000f0000,
-		.cpu_name	= "AArch64 Processor",
-		.cpu_setup	= __cpu_setup,
-	},
-	{ /* Empty */ },
-};

arch/arm64/kernel/head.S +5 -71

index 07f930540f4a..c3631ec8fe4b 100644
@@ -245,22 +245,12 @@ ENTRY(stext)
 	bl	__calc_phys_offset		// x24=PHYS_OFFSET, x28=PHYS_OFFSET-PAGE_OFFSET
 	bl	set_cpu_boot_mode_flag
 	mrs	x22, midr_el1			// x22=cpuid
-	mov	x0, x22
-	bl	lookup_processor_type
-	mov	x23, x0				// x23=current cpu_table
-	/*
-	 * __error_p may end up out of range for cbz if text areas are
-	 * aligned up to section sizes.
-	 */
-	cbnz	x23, 1f				// invalid processor (x23=0)?
-	b	__error_p
-1:
+
 	bl	__vet_fdt
 	bl	__create_page_tables		// x25=TTBR0, x26=TTBR1
 	/*
-	 * The following calls CPU specific code in a position independent
-	 * manner. See arch/arm64/mm/proc.S for details. x23 = base of
-	 * cpu_info structure selected by lookup_processor_type above.
+	 * The following calls CPU setup code, see arch/arm64/mm/proc.S for
+	 * details.
 	 * On return, the CPU will be ready for the MMU to be turned on and
 	 * the TCR will have been set.
 	 */

@@ -268,9 +258,7 @@ ENTRY(stext)
 						// MMU has been enabled
 	adrp	lr, __enable_mmu		// return (PIC) address
 	add	lr, lr, #:lo12:__enable_mmu
-	ldr	x12, [x23, #CPU_INFO_SETUP]
-	add	x12, x12, x28			// __virt_to_phys
-	br	x12				// initialise processor
+	b	__cpu_setup			// initialise processor
 ENDPROC(stext)
 
 /*

@@ -634,15 +622,9 @@ ENTRY(secondary_startup)
 	 * Common entry point for secondary CPUs.
 	 */
 	mrs	x22, midr_el1			// x22=cpuid
-	mov	x0, x22
-	bl	lookup_processor_type
-	mov	x23, x0				// x23=current cpu_table
-	cbz	x23, __error_p			// invalid processor (x23=0)?
 
 	pgtbl	x25, x26, x28			// x25=TTBR0, x26=TTBR1
-	ldr	x12, [x23, #CPU_INFO_SETUP]
-	add	x12, x12, x28			// __virt_to_phys
-	blr	x12				// initialise processor
+	bl	__cpu_setup			// initialise processor
 
 	ldr	x21, =secondary_data
 	ldr	x27, =__secondary_switched	// address to jump to after enabling the MMU

@@ -708,51 +690,3 @@ ENDPROC(__calc_phys_offset)
 	.align 3
 1:	.quad	.
 	.quad	PAGE_OFFSET
-
-/*
- * Exception handling. Something went wrong and we can't proceed. We ought to
- * tell the user, but since we don't have any guarantee that we're even
- * running on the right architecture, we do virtually nothing.
- */
-__error_p:
-ENDPROC(__error_p)
-
-__error:
-1:	nop
-	b	1b
-ENDPROC(__error)
-
-/*
- * This function gets the processor ID in w0 and searches the cpu_table[] for
- * a match. It returns a pointer to the struct cpu_info it found. The
- * cpu_table[] must end with an empty (all zeros) structure.
- *
- * This routine can be called via C code and it needs to work with the MMU
- * both disabled and enabled (the offset is calculated automatically).
- */
-ENTRY(lookup_processor_type)
-	adr	x1, __lookup_processor_type_data
-	ldp	x2, x3, [x1]
-	sub	x1, x1, x2			// get offset between VA and PA
-	add	x3, x3, x1			// convert VA to PA
-1:
-	ldp	w5, w6, [x3]			// load cpu_id_val and cpu_id_mask
-	cbz	w5, 2f				// end of list?
-	and	w6, w6, w0
-	cmp	w5, w6
-	b.eq	3f
-	add	x3, x3, #CPU_INFO_SZ
-	b	1b
-2:
-	mov	x3, #0				// unknown processor
-3:
-	mov	x0, x3
-	ret
-ENDPROC(lookup_processor_type)
-
-	.align	3
-	.type	__lookup_processor_type_data, %object
-__lookup_processor_type_data:
-	.quad	.
-	.quad	cpu_table
-	.size	__lookup_processor_type_data, . - __lookup_processor_type_data

arch/arm64/kernel/setup.c +2 -14

index e8420f635bd4..a19eae29497c 100644
@@ -50,7 +50,6 @@
 #include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/elf.h>
-#include <asm/cputable.h>
 #include <asm/cpufeature.h>
 #include <asm/cpu_ops.h>
 #include <asm/sections.h>

@@ -83,7 +82,6 @@ unsigned int compat_elf_hwcap2 __read_mostly;
 
 DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
-static const char *cpu_name;
 phys_addr_t __fdt_pointer __initdata;
 
 /*

@@ -209,22 +207,12 @@ static void __init smp_build_mpidr_hash(void)
 
 static void __init setup_processor(void)
 {
-	struct cpu_info *cpu_info;
 	u64 features, block;
 	u32 cwg;
 	int cls;
 
-	cpu_info = lookup_processor_type(read_cpuid_id());
-	if (!cpu_info) {
-		printk("CPU configuration botched (ID %08x), unable to continue.\n",
-		       read_cpuid_id());
-		while (1);
-	}
-
-	cpu_name = cpu_info->cpu_name;
-
-	printk("CPU: %s [%08x] revision %d\n",
-	       cpu_name, read_cpuid_id(), read_cpuid_id() & 15);
+	printk("CPU: AArch64 Processor [%08x] revision %d\n",
+	       read_cpuid_id(), read_cpuid_id() & 15);
 
 	sprintf(init_utsname()->machine, ELF_PLATFORM);
 	elf_hwcap = 0;

Signature · 1 line

1.8.3.2

From: Marc Zyngier <[email protected]>

struct cpu_table is an artifact left from the (very) early days of the arm64 port, and its only real use is to allow the most beautiful "AArch64 Processor" string to be displayed at boot time.

Really? Yes, really.

Let's get rid of it. In order to avoid another BogoMips-gate, the aforementioned string is preserved.

Acked-by Catalin Marinas <[email protected]>

Acked-by Ard Biesheuvel <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
  arch/arm64/include/asm/cputable.h | 30 ----------------
  arch/arm64/kernel/Makefile        |  2 +-
  arch/arm64/kernel/asm-offsets.c   |  4 ---
  arch/arm64/kernel/cputable.c      | 33 -----------------
  arch/arm64/kernel/head.S          | 76 +++------------------------------------
  arch/arm64/kernel/setup.c         | 16 ++-------
  6 files changed, 8 insertions(+), 153 deletions(-)

delete mode 100644 arch/arm64/include/asm/cputable.h delete mode 100644 arch/arm64/kernel/cputable.c

arch/arm64/kernel/setup.c +0 -7

@@ -50,7 +50,6 @@
  #include <asm/cpu.h>
  #include <asm/cputype.h>
  #include <asm/elf.h>
-#include <asm/cputable.h>
  #include <asm/cpufeature.h>
  #include <asm/cpu_ops.h>
  #include <asm/sections.h>

@@ -83,7 +82,6 @@ unsigned int compat_elf_hwcap2 __read_mostly;
 
  DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
-static const char *cpu_name;
  phys_addr_t __fdt_pointer __initdata;
 
  /*

@@ -209,22 +207,12 @@ static void __init smp_build_mpidr_hash(void)
 
  static void __init setup_processor(void)
  {
-       struct cpu_info *cpu_info;
         u64 features, block;
         u32 cwg;
         int cls;
 
-       cpu_info = lookup_processor_type(read_cpuid_id());
-       if (!cpu_info) {
-               printk("CPU configuration botched (ID %08x), unable to continue.\n",
-                      read_cpuid_id());

Quoted 13 lines · Ard Biesheuvel

arch/arm64/kernel/setup.c +0 -5

@@ -209,22 +207,12 @@ static void __init smp_build_mpidr_hash(void)
 
  static void __init setup_processor(void)
  {
-       struct cpu_info *cpu_info;
         u64 features, block;
         u32 cwg;
         int cls;
 
-       cpu_info = lookup_processor_type(read_cpuid_id());
-       if (!cpu_info) {
-               printk("CPU configuration botched (ID %08x), unable to continue.\n",
-                      read_cpuid_id());

arch/arm64/kernel/setup.c static void __init smp_build_mpidr_hash(void) · line 216

Quoted 10 lines

arch/arm64/kernel/setup.c +2 -7

-               while (1);
-       }
-
-       cpu_name = cpu_info->cpu_name;
-
-       printk("CPU: %s [%08x] revision %d\n",
-              cpu_name, read_cpuid_id(), read_cpuid_id() & 15);
+       printk("CPU: AArch64 Processor [%08x] revision %d\n",
+              read_cpuid_id(), read_cpuid_id() & 15);
 

While we are at it, does it make sense to change

s/CPU/Boot CPU/

to make it clear on a big.Little system ?

Quoted 27 lines · Suzuki K. Poulose

Quoted 13 lines · Ard Biesheuvel

arch/arm64/kernel/setup.c +0 -5

@@ -209,22 +207,12 @@ static void __init smp_build_mpidr_hash(void)
 
   static void __init setup_processor(void)
   {
-       struct cpu_info *cpu_info;
          u64 features, block;
          u32 cwg;
          int cls;
 
-       cpu_info = lookup_processor_type(read_cpuid_id());
-       if (!cpu_info) {
-               printk("CPU configuration botched (ID %08x), unable to continue.\n",
-                      read_cpuid_id());

arch/arm64/kernel/setup.c static void __init smp_build_mpidr_hash(void) · line 216

Quoted 10 lines

arch/arm64/kernel/setup.c +2 -7

-               while (1);
-       }
-
-       cpu_name = cpu_info->cpu_name;
-
-       printk("CPU: %s [%08x] revision %d\n",
-              cpu_name, read_cpuid_id(), read_cpuid_id() & 15);
+       printk("CPU: AArch64 Processor [%08x] revision %d\n",
+              read_cpuid_id(), read_cpuid_id() & 15);
 

While we are at it, does it make sense to change

Quoted 3 lines

s/CPU/Boot CPU/

to make it clear on a big.Little system ?

This is already queued, so I don't think it's worth the effort now.

The adrp instruction is mostly used in combination with either an add, a ldr or a str instruction with the low bits of the referenced symbol in the 12-bit immediate of the followup instruction.

Introduce the macros adr_l, ldr_l and str_l that encapsulate these common patterns.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/assembler.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

arch/arm64/include/asm/assembler.h +0 -0

index 750bac4e637e..f1804d4803fb 100644

arch/arm64/include/asm/assembler.h +25 -0

+	.macro	adr_l, dst, sym, tmp=
+	.ifb	\tmp
+	adrp	\dst, \sym
+	add	\dst, \dst, :lo12:\sym
+	.else
+	adrp	\tmp, \sym
+	add	\dst, \tmp, :lo12:\sym
+	.endif
+	.endm
+
+	.macro	ldr_l, dst, sym, tmp=
+	.ifb	\tmp
+	adrp	\dst, \sym
+	ldr	\dst, [\dst, :lo12:\sym]
+	.else
+	adrp	\tmp, \sym
+	ldr	\dst, [\tmp, :lo12:\sym]
+	.endif
+	.endm
+
+	.macro	str_l, src, sym, tmp
+	adrp	\tmp, \sym
+	str	\src, [\tmp, :lo12:\sym]
+	.endm
+
 #endif	/* __ASM_ASSEMBLER_H */

Signature · 1 line

1.8.3.2

Quoted 21 lines · Ard Biesheuvel

The adrp instruction is mostly used in combination with either an add, a ldr or a str instruction with the low bits of the referenced symbol in the 12-bit immediate of the followup instruction.

Introduce the macros adr_l, ldr_l and str_l that encapsulate these common patterns.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/assembler.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

arch/arm64/include/asm/assembler.h +0 -0

index 750bac4e637e..f1804d4803fb 100644
@@ -159,4 +159,33 @@ lr       .req    x30             // link register
      orr     \rd, \lbits, \hbits, lsl #32
      .endm
 

arch/arm64/include/asm/assembler.h lr .req x30 // link register · line 165

Quoted 4 lines

arch/arm64/include/asm/assembler.h +4 -0

+/*
+ * Pseudo-ops for PC-relative adr/ldr/str <reg>, <symbol> where
+ * <symbol> is within the range +/- 4 GB of the PC.
+ */

It would be nice to point out that tmp for adr_l and ldr_l is only necesssary when loading a value into the SP.

Otherwise this looks fine.

Mark.

Quoted 30 lines

arch/arm64/include/asm/assembler.h +25 -1

+     .macro  adr_l, dst, sym, tmp=
+     .ifb    \tmp
+     adrp    \dst, \sym
+     add     \dst, \dst, :lo12:\sym
+     .else
+     adrp    \tmp, \sym
+     add     \dst, \tmp, :lo12:\sym
+     .endif
+     .endm
+
+     .macro  ldr_l, dst, sym, tmp=
+     .ifb    \tmp
+     adrp    \dst, \sym
+     ldr     \dst, [\dst, :lo12:\sym]
+     .else
+     adrp    \tmp, \sym
+     ldr     \dst, [\tmp, :lo12:\sym]
+     .endif
+     .endm
+
+     .macro  str_l, src, sym, tmp
+     adrp    \tmp, \sym
+     str     \src, [\tmp, :lo12:\sym]
+     .endm
+
 #endif       /* __ASM_ASSEMBLER_H */
--

1.8.3.2

Quoted 22 lines · Mark Rutland

Quoted 21 lines · Ard Biesheuvel

The adrp instruction is mostly used in combination with either an add, a ldr or a str instruction with the low bits of the referenced symbol in the 12-bit immediate of the followup instruction.

Introduce the macros adr_l, ldr_l and str_l that encapsulate these common patterns.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/assembler.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

arch/arm64/include/asm/assembler.h +0 -0

index 750bac4e637e..f1804d4803fb 100644
@@ -159,4 +159,33 @@ lr       .req    x30             // link register
      orr     \rd, \lbits, \hbits, lsl #32
      .endm
 

Quoted 8 lines

arch/arm64/include/asm/assembler.h lr .req x30 // link register · line 165

Quoted 4 lines

arch/arm64/include/asm/assembler.h +4 -0

+/*
+ * Pseudo-ops for PC-relative adr/ldr/str <reg>, <symbol> where
+ * <symbol> is within the range +/- 4 GB of the PC.
+ */

It would be nice to point out that tmp for adr_l and ldr_l is only necesssary when loading a value into the SP.

For adr_l, it is for the sp.
For ldr_l, it is primarily for being able to load w registers as well.

Quoted 32 lines · Ard Biesheuvel

Quoted 30 lines · Mark Rutland

Quoted 18 lines · Ard Biesheuvel

The adrp instruction is mostly used in combination with either an add, a ldr or a str instruction with the low bits of the referenced symbol in the 12-bit immediate of the followup instruction.

Introduce the macros adr_l, ldr_l and str_l that encapsulate these common patterns.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/assembler.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

arch/arm64/include/asm/assembler.h +0 -0

index 750bac4e637e..f1804d4803fb 100644

arch/arm64/include/asm/assembler.h lr .req x30 // link register · line 165

Quoted 7 lines

arch/arm64/include/asm/assembler.h +4 -0

@@ -159,4 +159,33 @@ lr       .req    x30             // link register
      orr     \rd, \lbits, \hbits, lsl #32
      .endm
 
+/*
+ * Pseudo-ops for PC-relative adr/ldr/str <reg>, <symbol> where
+ * <symbol> is within the range +/- 4 GB of the PC.
+ */

It would be nice to point out that tmp for adr_l and ldr_l is only necesssary when loading a value into the SP.

Quoted 2 lines

For adr_l, it is for the sp.
For ldr_l, it is primarily for being able to load w registers as well.

Ah, I hadn't considered that. Are you happy to add something to the comment block mentioning those cases?

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 <[email protected]>

---
 arch/arm64/kernel/head.S  |  4 ++++
 arch/arm64/kernel/setup.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

arch/arm64/kernel/head.S +0 -0

index a0fbd99efb89..8636c3cef006 100644

arch/arm64/kernel/head.S +1 -0

+
 	mov	x21, x0				// x21=FDT
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
 	adrp	x24, __PHYS_OFFSET

arch/arm64/kernel/setup.c +15 -0

index 6c5fb5aff325..2d5cae2de679 100644
@@ -114,6 +114,11 @@ void __init early_print(const char *str, ...)
 	printk("%s", buf);
 }
 
+/*
+ * The recorded values of x0 .. x3 upon kernel entry.
+ */
+u64 __read_mostly boot_regs[4];
+
 void __init smp_setup_processor_id(void)
 {
 	u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

@@ -387,6 +392,16 @@ void __init setup_arch(char **cmdline_p)
 	conswitchp = &dummy_con;
 #endif
 #endif
+	/*
+	 * boot_regs[] is written by the boot CPU with the caches off, so we
+	 * need to ensure that we read the value from main memory
+	 */
+	__flush_dcache_area(boot_regs, sizeof(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");
+	}
 }
 
 static int __init arm64_device_init(void)

Signature · 1 line

1.8.3.2

Quoted 17 lines · 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 <[email protected]>

---
 arch/arm64/kernel/head.S  |  4 ++++
 arch/arm64/kernel/setup.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

arch/arm64/kernel/head.S +0 -0

index a0fbd99efb89..8636c3cef006 100644

arch/arm64/kernel/head.S section_table: · line 238

Quoted 6 lines

arch/arm64/kernel/head.S +3 -0

@@ -233,6 +233,10 @@ section_table:
 #endif
 
 ENTRY(stext)
+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Quoted 41 lines

arch/arm64/kernel/head.S +1 -0

+
      mov     x21, x0                         // x21=FDT
      bl      el2_setup                       // Drop to EL1, w20=cpu_boot_mode
      adrp    x24, __PHYS_OFFSET

arch/arm64/kernel/setup.c +15 -1

index 6c5fb5aff325..2d5cae2de679 100644
@@ -114,6 +114,11 @@ void __init early_print(const char *str, ...)
      printk("%s", buf);
 }
 
+/*
+ * The recorded values of x0 .. x3 upon kernel entry.
+ */
+u64 __read_mostly boot_regs[4];
+
 void __init smp_setup_processor_id(void)
 {
      u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

@@ -387,6 +392,16 @@ void __init setup_arch(char **cmdline_p)
      conswitchp = &dummy_con;
 #endif
 #endif
+     /*
+      * boot_regs[] is written by the boot CPU with the caches off, so we
+      * need to ensure that we read the value from main memory
+      */
+     __flush_dcache_area(boot_regs, sizeof(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");
+     }
 }
 
 static int __init arm64_device_init(void)
--

1.8.3.2

Quoted 28 lines · Mark Rutland

Quoted 17 lines · 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 <[email protected]>

---
 arch/arm64/kernel/head.S  |  4 ++++
 arch/arm64/kernel/setup.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

arch/arm64/kernel/head.S +0 -0

index a0fbd99efb89..8636c3cef006 100644

arch/arm64/kernel/head.S section_table: · line 238

Quoted 6 lines

arch/arm64/kernel/head.S +3 -0

@@ -233,6 +233,10 @@ section_table:
 #endif
 
 ENTRY(stext)
+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

Quoted 6 lines

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Quoted 41 lines

Quoted 41 lines

arch/arm64/kernel/head.S +1 -0

+
      mov     x21, x0                         // x21=FDT
      bl      el2_setup                       // Drop to EL1, w20=cpu_boot_mode
      adrp    x24, __PHYS_OFFSET

arch/arm64/kernel/setup.c +15 -1

index 6c5fb5aff325..2d5cae2de679 100644
@@ -114,6 +114,11 @@ void __init early_print(const char *str, ...)
      printk("%s", buf);
 }
 
+/*
+ * The recorded values of x0 .. x3 upon kernel entry.
+ */
+u64 __read_mostly boot_regs[4];
+
 void __init smp_setup_processor_id(void)
 {
      u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

@@ -387,6 +392,16 @@ void __init setup_arch(char **cmdline_p)
      conswitchp = &dummy_con;
 #endif
 #endif
+     /*
+      * boot_regs[] is written by the boot CPU with the caches off, so we
+      * need to ensure that we read the value from main memory
+      */
+     __flush_dcache_area(boot_regs, sizeof(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");
+     }
 }
 
 static int __init arm64_device_init(void)
--

1.8.3.2

Quoted 36 lines · Ard Biesheuvel

Quoted 34 lines · Mark Rutland

Quoted 17 lines · 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 <[email protected]>

---
 arch/arm64/kernel/head.S  |  4 ++++
 arch/arm64/kernel/setup.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

arch/arm64/kernel/head.S +0 -0

index a0fbd99efb89..8636c3cef006 100644

arch/arm64/kernel/head.S section_table: · line 238

Quoted 6 lines

arch/arm64/kernel/head.S +3 -0

@@ -233,6 +233,10 @@ section_table:
 #endif
 
 ENTRY(stext)
+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Quoted 4 lines

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Never mind, it's easier just to do the invalidate right after, and I can drop the flush before the access.

Quoted 42 lines · Ard Biesheuvel

Quoted 40 lines · Ard Biesheuvel

Quoted 28 lines · Mark Rutland

Quoted 17 lines · 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 <[email protected]>

---
 arch/arm64/kernel/head.S  |  4 ++++
 arch/arm64/kernel/setup.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

arch/arm64/kernel/head.S +0 -0

index a0fbd99efb89..8636c3cef006 100644

arch/arm64/kernel/head.S section_table: · line 238

Quoted 6 lines

arch/arm64/kernel/head.S +3 -0

@@ -233,6 +233,10 @@ section_table:
 #endif
 
 ENTRY(stext)
+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

Quoted 6 lines

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Quoted 2 lines

Never mind, it's easier just to do the invalidate right after, and I can drop the flush before the access.

Yup.

Annoyingly the minimum cache line size seems to be a word (given the defnition of CTR.DminLine), which means you need a few dc ivac instructions to be architecturally correct.

Quoted 23 lines

Quoted 22 lines

Quoted 15 lines

Quoted 5 lines

arch/arm64/kernel/head.S section_table: · line 238

Quoted 4 lines

ENTRY(stext)

arch/arm64/kernel/head.S +3 -0

+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

Quoted 9 lines

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Quoted 4 lines

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Never mind, it's easier just to do the invalidate right after, and I can drop the flush before the access.

Quoted 6 lines

Yup.

Annoyingly the minimum cache line size seems to be a word (given the defnition of CTR.DminLine), which means you need a few dc ivac instructions to be architecturally correct.

But that applies to cpu_boot_mode as well then?

I will add a call to __inval_cache_range() right after recording the initial values, that should do the right thing regarding llinesize

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 15 lines

Quoted 5 lines

Review

Quoted 4 lines

ENTRY(stext)

(quoted diff) +3 -0

+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

Quoted 9 lines

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Quoted 4 lines

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Never mind, it's easier just to do the invalidate right after, and I can drop the flush before the access.

Quoted 8 lines

Quoted 6 lines

Yup.

Annoyingly the minimum cache line size seems to be a word (given the defnition of CTR.DminLine), which means you need a few dc ivac instructions to be architecturally correct.

But that applies to cpu_boot_mode as well then?

It writes a single word, so it happens to be safe.

Quoted 2 lines

I will add a call to __inval_cache_range() right after recording the initial values, that should do the right thing regarding llinesize

That works, with one caveat: you'll need a dmb sy between the writes and the call -- dc instructions by VA only hazard against normal cacheable accesses, and __inval_cache_range assumes the caches are on and so doesn't have a dmb prior to the dc instructions.

Quoted 34 lines · Mark Rutland

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 15 lines

Quoted 5 lines

Review

Quoted 4 lines

ENTRY(stext)

(quoted diff) +3 -0

+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

Quoted 9 lines

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Quoted 4 lines

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Never mind, it's easier just to do the invalidate right after, and I can drop the flush before the access.

Quoted 8 lines

Quoted 6 lines

Yup.

Annoyingly the minimum cache line size seems to be a word (given the defnition of CTR.DminLine), which means you need a few dc ivac instructions to be architecturally correct.

But that applies to cpu_boot_mode as well then?

It writes a single word, so it happens to be safe.

Quoted 8 lines

Quoted 2 lines

I will add a call to __inval_cache_range() right after recording the initial values, that should do the right thing regarding llinesize

That works, with one caveat: you'll need a dmb sy between the writes and the call -- dc instructions by VA only hazard against normal cacheable accesses, and __inval_cache_range assumes the caches are on and so doesn't have a dmb prior to the dc instructions.

Does it matter at all that __inval_cache_range() will mostly end up doing a civac for the whole array, since it uses civac not ivac for both non-cachelined aligned ends of the region, and the typical cacheline size is larger then the size of the array? Couldn't that also clobber what we just wrote with a stale cacheline?

Quoted 44 lines · Ard Biesheuvel

Quoted 34 lines · Mark Rutland

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 15 lines

Quoted 5 lines

Review

Quoted 4 lines

ENTRY(stext)

(quoted diff) +3 -0

+     adr_l   x8, boot_regs                   // record the contents of
+     stp     x0, x1, [x8]                    // x0 .. x3 at kernel entry
+     stp     x2, x3, [x8, #16]

Quoted 9 lines

I think we should have a dc ivac here as we do for set_cpu_boot_mode_flag.

That avoids a potential issue with boot_regs sharing a cacheline with data we write with the MMU on -- using __flush_dcache_area will result in a civac, so we could write back dirty data atop of the boot_regs if there were clean entries in the cache when we did the non-cacheable write.

Quoted 4 lines

Hmm, I wondered about that.

Could we instead just make it u64 __initconst boot_regs[] in setup.c ?

Never mind, it's easier just to do the invalidate right after, and I can drop the flush before the access.

Quoted 8 lines

Quoted 6 lines

Yup.

Annoyingly the minimum cache line size seems to be a word (given the defnition of CTR.DminLine), which means you need a few dc ivac instructions to be architecturally correct.

But that applies to cpu_boot_mode as well then?

It writes a single word, so it happens to be safe.

Quoted 8 lines

Quoted 2 lines

I will add a call to __inval_cache_range() right after recording the initial values, that should do the right thing regarding llinesize

That works, with one caveat: you'll need a dmb sy between the writes and the call -- dc instructions by VA only hazard against normal cacheable accesses, and __inval_cache_range assumes the caches are on and so doesn't have a dmb prior to the dc instructions.

Quoted 5 lines

Does it matter at all that __inval_cache_range() will mostly end up doing a civac for the whole array, since it uses civac not ivac for both non-cachelined aligned ends of the region, and the typical cacheline size is larger then the size of the array? Couldn't that also clobber what we just wrote with a stale cacheline?

Yes, though only if the memory were outside the footprint of the loaded Image (which per the boot protocol should be clean to the PoC).

So I guess we should move the boot_regs structure back into head.S so it doesn't fall outside

Quoted 9 lines

Quoted 5 lines

Does it matter at all that __inval_cache_range() will mostly end up doing a civac for the whole array, since it uses civac not ivac for both non-cachelined aligned ends of the region, and the typical cacheline size is larger then the size of the array? Couldn't that also clobber what we just wrote with a stale cacheline?

Yes, though only if the memory were outside the footprint of the loaded Image (which per the boot protocol should be clean to the PoC).

Quoted 3 lines

So I guess we should move the boot_regs structure back into head.S so it doesn't fall outside

OK that means .data should be fine too. __cacheline_aligned variables are put into the .data section, so let me use that instead (.bss gets cleared after anyway, that is why i added the __read_mostly initially)

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 <[email protected]>

---
 arch/arm64/kernel/head.S  | 19 ++++++++++++++++++-
 arch/arm64/kernel/setup.c | 10 ++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

arch/arm64/kernel/head.S +18 -1

index f5ac337f9598..1fdf42041f42 100644
@@ -233,7 +233,7 @@ section_table:
 #endif
 
 ENTRY(stext)
-	mov	x21, x0				// x21=FDT
+	bl	preserve_boot_args
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
 	adrp	x24, __PHYS_OFFSET
 	bl	set_cpu_boot_mode_flag

@@ -253,6 +253,23 @@ ENTRY(stext)
 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]
+
+	dmb	sy				// needed before dc ivac with
+						// MMU off
+
+	add	x1, x0, #0x20			// 4 x 8 bytes
+	b	__inval_cache_range		// tail call
+ENDPROC(preserve_boot_args)
+
+/*
  * Determine validity of the x21 FDT pointer.
  * The dtb must be 8-byte aligned and live in the first 512M of memory.
  */

arch/arm64/kernel/setup.c +0 -0

index 1783b38cf4c0..2f384019b201 100644

Quoted 54 lines · 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 <[email protected]>

---
 arch/arm64/kernel/head.S  | 19 ++++++++++++++++++-
 arch/arm64/kernel/setup.c | 10 ++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

arch/arm64/kernel/head.S +18 -1

index f5ac337f9598..1fdf42041f42 100644
@@ -233,7 +233,7 @@ section_table:
 #endif
 
 ENTRY(stext)
-     mov     x21, x0                         // x21=FDT
+     bl      preserve_boot_args
      bl      el2_setup                       // Drop to EL1, w20=cpu_boot_mode
      adrp    x24, __PHYS_OFFSET
      bl      set_cpu_boot_mode_flag

@@ -253,6 +253,23 @@ ENTRY(stext)
 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]
+
+     dmb     sy                              // needed before dc ivac with
+                                             // MMU off
+
+     add     x1, x0, #0x20                   // 4 x 8 bytes
+     b       __inval_cache_range             // tail call
+ENDPROC(preserve_boot_args)
+
+/*
  * Determine validity of the x21 FDT pointer.
  * The dtb must be 8-byte aligned and live in the first 512M of memory.
  */

arch/arm64/kernel/setup.c +0 -0

index 1783b38cf4c0..2f384019b201 100644

arch/arm64/kernel/setup.c void __init early_print(const char *str, ...) · line 121

Quoted 7 lines

arch/arm64/kernel/setup.c +4 -0

@@ -115,6 +115,11 @@ void __init early_print(const char *str, ...)
      printk("%s", buf);
 }
 
+/*
+ * The recorded values of x0 .. x3 upon kernel entry.
+ */
+u64 __cacheline_aligned boot_args[4];

All the above looks correct to me.

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 423

Quoted 12 lines

arch/arm64/kernel/setup.c +5 -0

+
 void __init smp_setup_processor_id(void)
 {
      u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

@@ -412,6 +417,11 @@ void __init setup_arch(char **cmdline_p)
      conswitchp = &dummy_con;
 #endif
 #endif
+     if (boot_args[1] || boot_args[2] || boot_args[3]) {
+             pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
+                     boot_args[1], boot_args[2], boot_args[3]);
+             pr_err("WARNING: your bootloader may fail to load newer kernels\n");

If we ever decide to use x1-x3 for something, and try to boot an older kernel, that warning is going to be a bit misleading. That could matter for VMs where we're going to see old kernel images for a long time.

I would like the warning to mention that could be the case.

It would also be nice if the message were consistently spaced regardless of the values of x1-x3, so we should zero-pad them (and as that takes a resonable amount of space, let's give them a line each).

So could we change the warning to be something like:

pr_err("WARNING: x1-x3 nonzero in violation of boot protocol:\n"
"\tx1: %016llx\n\tx2: %016llx\n\tx3: %016llx\n" "This indicates a broken bootloader or old kernel\n", boot_args[1], boot_args[2], boot_args[3]);

Quoted 15 lines

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 423

Quoted 4 lines

arch/arm64/kernel/setup.c +4 -0

+     if (boot_args[1] || boot_args[2] || boot_args[3]) {
+             pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
+                     boot_args[1], boot_args[2], boot_args[3]);
+             pr_err("WARNING: your bootloader may fail to load newer kernels\n");

If we ever decide to use x1-x3 for something, and try to boot an older kernel, that warning is going to be a bit misleading. That could matter for VMs where we're going to see old kernel images for a long time.

I would like the warning to mention that could be the case.

It would also be nice if the message were consistently spaced regardless of the values of x1-x3, so we should zero-pad them (and as that takes a resonable amount of space, let's give them a line each).

Quoted 7 lines

So could we change the warning to be something like:

pr_err("WARNING: x1-x3 nonzero in violation of boot protocol:\n"
"\tx1: %016llx\n\tx2: %016llx\n\tx3: %016llx\n" "This indicates a broken bootloader or old kernel\n", boot_args[1], boot_args[2], boot_args[3]);

OK, I have applied this change.

But I would like to note that we should probably only extend the boot protocol in a way that would not trigger this on older kernels in the first place.
I.e., assign a bit in the flags field in the header, which indicates whether some boot protocol enhancement is supported by the kernel being loaded, and only allow x1/x2/x3 to be non-zero if said enhancement defines that.

Quoted 25 lines

Quoted 15 lines

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 423

Quoted 4 lines

arch/arm64/kernel/setup.c +4 -0

+     if (boot_args[1] || boot_args[2] || boot_args[3]) {
+             pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
+                     boot_args[1], boot_args[2], boot_args[3]);
+             pr_err("WARNING: your bootloader may fail to load newer kernels\n");

If we ever decide to use x1-x3 for something, and try to boot an older kernel, that warning is going to be a bit misleading. That could matter for VMs where we're going to see old kernel images for a long time.

I would like the warning to mention that could be the case.

It would also be nice if the message were consistently spaced regardless of the values of x1-x3, so we should zero-pad them (and as that takes a resonable amount of space, let's give them a line each).

Quoted 7 lines

So could we change the warning to be something like:

pr_err("WARNING: x1-x3 nonzero in violation of boot protocol:\n"
"\tx1: %016llx\n\tx2: %016llx\n\tx3: %016llx\n" "This indicates a broken bootloader or old kernel\n", boot_args[1], boot_args[2], boot_args[3]);

OK, I have applied this change.

Quoted 7 lines

But I would like to note that we should probably only extend the boot protocol in a way that would not trigger this on older kernels in the first place.
I.e., assign a bit in the flags field in the header, which indicates whether some boot protocol enhancement is supported by the kernel being loaded, and only allow x1/x2/x3 to be non-zero if said enhancement defines that.

Good point.

Given that, if you want to restore your original last line, that would be fine with me (and my Ack still applies).

Quoted 33 lines · Mark Rutland

Quoted 32 lines

Quoted 15 lines

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 423

Quoted 4 lines

arch/arm64/kernel/setup.c +4 -0

+     if (boot_args[1] || boot_args[2] || boot_args[3]) {
+             pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
+                     boot_args[1], boot_args[2], boot_args[3]);
+             pr_err("WARNING: your bootloader may fail to load newer kernels\n");

If we ever decide to use x1-x3 for something, and try to boot an older kernel, that warning is going to be a bit misleading. That could matter for VMs where we're going to see old kernel images for a long time.

I would like the warning to mention that could be the case.

It would also be nice if the message were consistently spaced regardless of the values of x1-x3, so we should zero-pad them (and as that takes a resonable amount of space, let's give them a line each).

Quoted 7 lines

So could we change the warning to be something like:

pr_err("WARNING: x1-x3 nonzero in violation of boot protocol:\n"
"\tx1: %016llx\n\tx2: %016llx\n\tx3: %016llx\n" "This indicates a broken bootloader or old kernel\n", boot_args[1], boot_args[2], boot_args[3]);

OK, I have applied this change.

But I would like to note that we should probably only extend the boot protocol in a way that would not trigger this on older kernels in the first place.
I.e., assign a bit in the flags field in the header, which indicates whether some boot protocol enhancement is supported by the kernel being loaded, and only allow x1/x2/x3 to be non-zero if said enhancement defines that.

Quoted 5 lines

Good point.

Given that, if you want to restore your original last line, that would be fine with me (and my Ack still applies).

I think it's fine to leave it as is

Quoted 40 lines · Ard Biesheuvel

Quoted 38 lines · Mark Rutland

Quoted 25 lines

Quoted 15 lines

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 423

Quoted 4 lines

arch/arm64/kernel/setup.c +4 -0

+     if (boot_args[1] || boot_args[2] || boot_args[3]) {
+             pr_err("WARNING: boot protocol violation detected (x1 == %llx, x2 == %llx, x3 == %llx)\n",
+                     boot_args[1], boot_args[2], boot_args[3]);
+             pr_err("WARNING: your bootloader may fail to load newer kernels\n");

If we ever decide to use x1-x3 for something, and try to boot an older kernel, that warning is going to be a bit misleading. That could matter for VMs where we're going to see old kernel images for a long time.

I would like the warning to mention that could be the case.

It would also be nice if the message were consistently spaced regardless of the values of x1-x3, so we should zero-pad them (and as that takes a resonable amount of space, let's give them a line each).

Quoted 7 lines

So could we change the warning to be something like:

pr_err("WARNING: x1-x3 nonzero in violation of boot protocol:\n"
"\tx1: %016llx\n\tx2: %016llx\n\tx3: %016llx\n" "This indicates a broken bootloader or old kernel\n", boot_args[1], boot_args[2], boot_args[3]);

OK, I have applied this change.

Quoted 7 lines

But I would like to note that we should probably only extend the boot protocol in a way that would not trigger this on older kernels in the first place.
I.e., assign a bit in the flags field in the header, which indicates whether some boot protocol enhancement is supported by the kernel being loaded, and only allow x1/x2/x3 to be non-zero if said enhancement defines that.

Good point.

Given that, if you want to restore your original last line, that would be fine with me (and my Ack still applies).

Quoted 1 line

I think it's fine to leave it as is

Yup, and this is sitting pretty on the arm64 devel branch.

Ard: I also pushed a kvm-bounce-page branch for you. Next step would be to merge everything into for-next/core and put your VA changes on top of that.

I'd appreciate a sanity check of the current branch first, though!

Annoyingly the minimum cache line size seems to be a word (given the defnition of CTR.DminLine), which means you need a few dc ivac instructions to be architecturally correct.

This some janitorial work on head.S, just stuff I noticed when making changes to it for other reasons.

This still needs acks on patches #2, #5 and #8

Changes since v4:
- included Marc Zyngier's struct cpu_table removal patch
- dropped __lookup_processor_type_data patch now that Marc has nuked the whole thing
- added patch to merge __enable_mmu() and __turn_mmu_on() into a single function, as there is no need for funky trampoline stuff when you have two non-overlapping TTBRs
- added patch to remove __calc_phys_offset and use PC-relative references or absolute (linker generated) references as appropriate
- added patch to complain when x1/x2/x3 are non-zero, which helps ensure that we will ever be able to use them for anything
- added R-b's

Changes since v3:
- added similar patch for secondary_holding_pen_release
- fixed bug in patch #1 (ldr_l)

Changes since v2:
- added separate patch to add macros for adrp/add, adrp/ldr and adrp/str
- added R-b's

Ard Biesheuvel (7):
arm64: add macros for common adrp usages
arm64: remove processor_id
arm64: remove __switch_data object from head.S
arm64: use PC-relative reference for secondary_holding_pen_release
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

Marc Zyngier (1):
arm64: Get rid of struct cpu_table

 arch/arm64/include/asm/assembler.h |  29 ++++++
 arch/arm64/include/asm/cputable.h  |  30 ------
 arch/arm64/kernel/Makefile         |   2 +-
 arch/arm64/kernel/asm-offsets.c    |   4 -
 arch/arm64/kernel/cputable.c       |  33 ------
 arch/arm64/kernel/head.S           | 203 +++++++------------------------------
 arch/arm64/kernel/setup.c          |  34 +++----
 7 files changed, 83 insertions(+), 252 deletions(-)
 delete mode 100644 arch/arm64/include/asm/cputable.h
 delete mode 100644 arch/arm64/kernel/cputable.c

Signature · 1 line

1.8.3.2

Quoted 4 lines · Ard Biesheuvel

This some janitorial work on head.S, just stuff I noticed when making changes to it for other reasons.

This still needs acks on patches #2, #5 and #8

I've given each patch of the series series a spin on Juno (using EFI+PSCI) and a model (using the bootwwapper + spin-table). For patches 1-7 the code looks sane, I see no build regressions on defconfig, and everything works, so for those feel free to add:

Tested-by Mark Rutland <[email protected]>

Reviewed-by Mark Rutland <[email protected]>

Quoted 49 lines

Changes since v4:
- included Marc Zyngier's struct cpu_table removal patch
- dropped __lookup_processor_type_data patch now that Marc has nuked the whole thing
- added patch to merge __enable_mmu() and __turn_mmu_on() into a single function, as there is no need for funky trampoline stuff when you have two non-overlapping TTBRs
- added patch to remove __calc_phys_offset and use PC-relative references or absolute (linker generated) references as appropriate
- added patch to complain when x1/x2/x3 are non-zero, which helps ensure that we will ever be able to use them for anything
- added R-b's

Changes since v3:
- added similar patch for secondary_holding_pen_release
- fixed bug in patch #1 (ldr_l)

Changes since v2:
- added separate patch to add macros for adrp/add, adrp/ldr and adrp/str
- added R-b's

Ard Biesheuvel (7):
arm64: add macros for common adrp usages
arm64: remove processor_id
arm64: remove __switch_data object from head.S
arm64: use PC-relative reference for secondary_holding_pen_release
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

Marc Zyngier (1):
arm64: Get rid of struct cpu_table

 arch/arm64/include/asm/assembler.h |  29 ++++++
 arch/arm64/include/asm/cputable.h  |  30 ------
 arch/arm64/kernel/Makefile         |   2 +-
 arch/arm64/kernel/asm-offsets.c    |   4 -
 arch/arm64/kernel/cputable.c       |  33 ------
 arch/arm64/kernel/head.S           | 203 +++++++------------------------------
 arch/arm64/kernel/setup.c          |  34 +++----
 7 files changed, 83 insertions(+), 252 deletions(-)
 delete mode 100644 arch/arm64/include/asm/cputable.h
 delete mode 100644 arch/arm64/kernel/cputable.c

--
1.8.3.2