arm64: avoid early __va translations

Since architectures may not yet have their linear mapping up and running when the initrd address is discovered from the DT, factor out the assignment of initrd_start and initrd_end, so that an architecture can override it and use the translation it needs.

Cc Rob Herring <[email protected]>

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

---
 drivers/of/fdt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

drivers/of/fdt.c +11 -3

index 1f98156f8996..3e90bce70545 100644
@@ -760,6 +760,16 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
 }
 
 #ifdef CONFIG_BLK_DEV_INITRD
+#ifndef __early_init_dt_declare_initrd
+static void __early_init_dt_declare_initrd(unsigned long start,
+					   unsigned long end)
+{
+	initrd_start = (unsigned long)__va(start);
+	initrd_end = (unsigned long)__va(end);
+	initrd_below_start_ok = 1;
+}
+#endif
+
 /**
  * early_init_dt_check_for_initrd - Decode initrd location from flat tree
  * @node: reference to node containing initrd location ('chosen')

@@ -782,9 +792,7 @@ static void __init early_init_dt_check_for_initrd(unsigned long node)
 		return;
 	end = of_read_number(prop, len/4);
 
-	initrd_start = (unsigned long)__va(start);
-	initrd_end = (unsigned long)__va(end);
-	initrd_below_start_ok = 1;
+	__early_init_dt_declare_initrd(start, end);
 
 	pr_debug("initrd_start=0x%llx  initrd_end=0x%llx\n",
 		 (unsigned long long)start, (unsigned long long)end);

Signature · 1 line

2.5.0

Quoted 5 lines · Ard Biesheuvel

Since architectures may not yet have their linear mapping up and running when the initrd address is discovered from the DT, factor out the assignment of initrd_start and initrd_end, so that an architecture can override it and use the translation it needs.

Quoted 5 lines

Cc Rob Herring <[email protected]>

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

---
 drivers/of/fdt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Mark, Rob, any comments on this?

Quoted 12 lines · Will Deacon

Quoted 10 lines · Ard Biesheuvel

Since architectures may not yet have their linear mapping up and running when the initrd address is discovered from the DT, factor out the assignment of initrd_start and initrd_end, so that an architecture can override it and use the translation it needs.

Cc Rob Herring <[email protected]>

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

---
 drivers/of/fdt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Quoted 2 lines

Mark, Rob, any comments on this?

Already acked by Rob, and pulled into -next by Catalin ...

Quoted 16 lines · Ard Biesheuvel

Quoted 14 lines · Will Deacon

Quoted 10 lines · Ard Biesheuvel

Since architectures may not yet have their linear mapping up and running when the initrd address is discovered from the DT, factor out the assignment of initrd_start and initrd_end, so that an architecture can override it and use the translation it needs.

Cc Rob Herring <[email protected]>

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

---
 drivers/of/fdt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Mark, Rob, any comments on this?

Quoted 1 line

Already acked by Rob, and pulled into -next by Catalin ...

Weird, I don't see that in my inbox or on the list archives. Where should I be looking?

I'd certainly like to see some performance measurement of the BUG_ON in PHYS_ADDR that is introduced later in the series too.

Currently, using BUG_ON() in header files is cumbersome, due to the fact that asm/bug.h transitively includes a lot of other header files, resulting in the actual BUG_ON() invocation appearing before its definition in the preprocessor input. So let's reverse the #include dependency between asm/bug.h and asm/debug-monitors.h, by moving the definition of BUG_BRK_IMM from the latter to the former. Also fix up one user of asm/debug-monitors.h which relied on a transitive include.

Cc Dave Martin <[email protected]>

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

---
 arch/arm64/include/asm/bug.h            | 2 +-
 arch/arm64/include/asm/debug-monitors.h | 2 +-
 arch/arm64/kvm/hyp/debug-sr.c           | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

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

index 4a748ce9ba1a..679d49221998 100644

Since memstart_addr is assigned relatively late in the boot code, after generic code like DT parsing and memblock manipulation has already occurred, we need to ensure that no __va() translation occur until memstart_addr has been set to a meaningful value.

So initialize memstart_addr to a value that cannot represent a valid physical address, and BUG() if memstart_addr is referenced while it still holds this value. Note that the > comparison against LLONG_MAX (not ULLONG_MAX) resolves to a single tbnz instruction that performs a conditional jump to a brk instruction that is emitted out of line.

Quoted 5 lines · Ard Biesheuvel

Since memstart_addr is assigned relatively late in the boot code, after generic code like DT parsing and memblock manipulation has already occurred, we need to ensure that no __va() translation occur until memstart_addr has been set to a meaningful value.

Quoted 5 lines

So initialize memstart_addr to a value that cannot represent a valid physical address, and BUG() if memstart_addr is referenced while it still holds this value. Note that the > comparison against LLONG_MAX (not ULLONG_MAX) resolves to a single tbnz instruction that performs a conditional jump to a brk instruction that is emitted out of line.

Even so, I'd imagine that having a measurable impact on system performance. Did you have a go at benchmarking this?

Quoted 12 lines · Will Deacon

Quoted 10 lines · Ard Biesheuvel

Since memstart_addr is assigned relatively late in the boot code, after generic code like DT parsing and memblock manipulation has already occurred, we need to ensure that no __va() translation occur until memstart_addr has been set to a meaningful value.

So initialize memstart_addr to a value that cannot represent a valid physical address, and BUG() if memstart_addr is referenced while it still holds this value. Note that the > comparison against LLONG_MAX (not ULLONG_MAX) resolves to a single tbnz instruction that performs a conditional jump to a brk instruction that is emitted out of line.

Quoted 2 lines

Even so, I'd imagine that having a measurable impact on system performance. Did you have a go at benchmarking this?

So in what kind of workload would the __pa() translation be on a hot path? If you're dealing with DMA or other things that involve physical addresses, surely, the single predicted non-taken branch instruction shouldn't hurt?

Quoted 13 lines · Ard Biesheuvel

Quoted 12 lines · Will Deacon

Quoted 10 lines · Ard Biesheuvel

Since memstart_addr is assigned relatively late in the boot code, after generic code like DT parsing and memblock manipulation has already occurred, we need to ensure that no __va() translation occur until memstart_addr has been set to a meaningful value.

So initialize memstart_addr to a value that cannot represent a valid physical address, and BUG() if memstart_addr is referenced while it still holds this value. Note that the > comparison against LLONG_MAX (not ULLONG_MAX) resolves to a single tbnz instruction that performs a conditional jump to a brk instruction that is emitted out of line.

Quoted 7 lines

Quoted 2 lines

Even so, I'd imagine that having a measurable impact on system performance. Did you have a go at benchmarking this?

So in what kind of workload would the __pa() translation be on a hot path? If you're dealing with DMA or other things that involve physical addresses, surely, the single predicted non-taken branch instruction shouldn't hurt?

I recall we looked at this in the early arm64 days and found a lot of memory accesses to memstart_addr but we decided to keep it as the alternatives would have been: (a) no more single Image or (b) always 4-levels page tables.

You could try perf to get some statistics but, for example, most of the code that works on pages (e.g. block I/O) and needs to access a page ends up doing a kmap(page) which in turns does a __va(). You also have lots of virt_to_page() calls in sl*b, so we need to see what impact this change has.

Quoted 5 lines · Ard Biesheuvel

Since memstart_addr is assigned relatively late in the boot code, after generic code like DT parsing and memblock manipulation has already occurred, we need to ensure that no __va() translation occur until memstart_addr has been set to a meaningful value.

Quoted 5 lines

So initialize memstart_addr to a value that cannot represent a valid physical address, and BUG() if memstart_addr is referenced while it still holds this value. Note that the > comparison against LLONG_MAX (not ULLONG_MAX) resolves to a single tbnz instruction that performs a conditional jump to a brk instruction that is emitted out of line.

Even so, I'd imagine that having a measurable impact on system performance. Did you have a go at benchmarking this?

Quoted 12 lines · Will Deacon

Quoted 10 lines · Ard Biesheuvel

Since memstart_addr is assigned relatively late in the boot code, after generic code like DT parsing and memblock manipulation has already occurred, we need to ensure that no __va() translation occur until memstart_addr has been set to a meaningful value.

So initialize memstart_addr to a value that cannot represent a valid physical address, and BUG() if memstart_addr is referenced while it still holds this value. Note that the > comparison against LLONG_MAX (not ULLONG_MAX) resolves to a single tbnz instruction that performs a conditional jump to a brk instruction that is emitted out of line.

Quoted 2 lines

Even so, I'd imagine that having a measurable impact on system performance. Did you have a go at benchmarking this?

Good point, I forgot about this discussion. We should revert this part indeed or at least make it dependent on some config option like DEBUG_VM.

This is a somewhat cleaner approach for dealing with the issue that the early FDT performs __va() translations before the linear mapping has been set up. Being able to defer the assignment of memstart_addr until after we have discovered all of memory is an important piece of functionality, not only for KASLR but also for mapping the linear region as efficiently as possible.

Changes since v2:
- replace __weak function in patches #1 and #2 with a preprocessor macro
- add patch to address the circular header dependency when including asm/bug.h in asm/memory.h
- turned the hack that adds the BUG_ON() into a proper patch

Patch #1 refactors the early FDT code so that the actual assignment of initrd_start and initrd_end (which is where the __va() translations are performed) can be overridden in architecture specific code.

Patch #2 performs the override, and only records the physical addresses as they are found in the /chosen node, or on the command line, and defers the __va() translation until after memstart_addr has been assigned.

Patch #3 reshuffles some #includes and #defines so that asm/bug.h can be included (and used) in asm/memory.h

Patch #4 implements a BUG_ON() check to ensure that no references to memstart_addr are made before it has been assigned.

Ard Biesheuvel (4):
of/fdt: factor out assignment of initrd_start/initrd_end
arm64: defer __va translation of initrd_start and initrd_end
arm64: prevent potential circular header dependencies in asm/bug.h
arm64: prevent __va() translations before memstart_addr is assigned

 arch/arm64/include/asm/bug.h            |  2 +-
 arch/arm64/include/asm/debug-monitors.h |  2 +-
 arch/arm64/include/asm/memory.h         | 12 ++++++++++-
 arch/arm64/kvm/hyp/debug-sr.c           |  1 +
 arch/arm64/mm/init.c                    | 21 +++++++++++++++-----
 drivers/of/fdt.c                        | 14 ++++++++++---
 6 files changed, 41 insertions(+), 11 deletions(-)