[PATCH v3 0/4] arm64: avoid early __va translations

[PATCH v3 0/4] arm64: avoid early __va translations

[Ard Biesheuvel]

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(-)

-- 
2.5.0

[PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end

[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 <robh@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/of/fdt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 1f98156f8996..3e90bce70545 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -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);
-- 
2.5.0

[PATCH v3 2/4] arm64: defer __va translation of initrd_start and initrd_end

[Ard Biesheuvel]

Commit 66f51676a095 ("arm64: allow kernel Image to be loaded anywhere in
physical memory") defers the assignment of memstart_addr to the point where
all memory has been discovered and possibly clipped based on the size of
the linear region and the presence of a mem= command line parameter.

However, this results in __va() translations that have been performed up
to this point to have been carried out with a preliminary, incorrect value
of memstart_addr, and these values need to be fixed up. So wire up the
generic support to declare the initrd addresses, and implement it without
__va() translations, and perform the translation after memstart_addr has
been assigned.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/memory.h |  8 ++++++++
 arch/arm64/mm/init.c            | 13 +++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 083361531a61..c900883a3119 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -121,6 +121,14 @@
 #define IOREMAP_MAX_ORDER	(PMD_SHIFT)
 #endif
 
+#ifdef CONFIG_BLK_DEV_INITRD
+#define __early_init_dt_declare_initrd(__start, __end)			\
+	do {								\
+		initrd_start = (__start);				\
+		initrd_end = (__end);					\
+	} while (0)
+#endif
+
 #ifndef __ASSEMBLY__
 
 extern phys_addr_t		memstart_addr;
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 3a9fc46cbf80..ed85778b32e5 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -61,8 +61,8 @@ static int __init early_initrd(char *p)
 	if (*endp == ',') {
 		size = memparse(endp + 1, NULL);
 
-		initrd_start = (unsigned long)__va(start);
-		initrd_end = (unsigned long)__va(start + size);
+		initrd_start = start;
+		initrd_end = start + size;
 	}
 	return 0;
 }
@@ -213,8 +213,13 @@ void __init arm64_memblock_init(void)
 	 */
 	memblock_reserve(__pa(_text), _end - _text);
 #ifdef CONFIG_BLK_DEV_INITRD
-	if (initrd_start)
-		memblock_reserve(__virt_to_phys(initrd_start), initrd_end - initrd_start);
+	if (initrd_start) {
+		memblock_reserve(initrd_start, initrd_end - initrd_start);
+
+		/* the generic initrd code expects virtual addresses */
+		initrd_start = __phys_to_virt(initrd_start);
+		initrd_end = __phys_to_virt(initrd_end);
+	}
 #endif
 
 	early_init_fdt_scan_reserved_mem();
-- 
2.5.0

[PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h

[Ard Biesheuvel]

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 <Dave.Martin@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 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(-)

diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
index 4a748ce9ba1a..679d49221998 100644
--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -18,7 +18,7 @@
 #ifndef _ARCH_ARM64_ASM_BUG_H
 #define _ARCH_ARM64_ASM_BUG_H
 
-#include <asm/debug-monitors.h>
+#define BUG_BRK_IMM			0x800
 
 #ifdef CONFIG_GENERIC_BUG
 #define HAVE_ARCH_BUG
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 279c85b5ec09..e893a1fca9c2 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -20,6 +20,7 @@
 
 #include <linux/errno.h>
 #include <linux/types.h>
+#include <asm/bug.h>
 #include <asm/esr.h>
 #include <asm/insn.h>
 #include <asm/ptrace.h>
@@ -57,7 +58,6 @@
 #define FAULT_BRK_IMM			0x100
 #define KGDB_DYN_DBG_BRK_IMM		0x400
 #define KGDB_COMPILED_DBG_BRK_IMM	0x401
-#define BUG_BRK_IMM			0x800
 
 /*
  * BRK instruction encoding
diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c
index c9c1e97501a9..2f8bca8af295 100644
--- a/arch/arm64/kvm/hyp/debug-sr.c
+++ b/arch/arm64/kvm/hyp/debug-sr.c
@@ -18,6 +18,7 @@
 #include <linux/compiler.h>
 #include <linux/kvm_host.h>
 
+#include <asm/debug-monitors.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmu.h>
 
-- 
2.5.0

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/memory.h | 4 +++-
 arch/arm64/mm/init.c            | 8 +++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index c900883a3119..ae398919fb5f 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -24,6 +24,7 @@
 #include <linux/compiler.h>
 #include <linux/const.h>
 #include <linux/types.h>
+#include <asm/bug.h>
 #include <asm/sizes.h>
 
 /*
@@ -133,7 +134,8 @@
 
 extern phys_addr_t		memstart_addr;
 /* PHYS_OFFSET - the physical address of the start of memory. */
-#define PHYS_OFFSET		({ memstart_addr; })
+#define PHYS_OFFSET		\
+	({ BUG_ON(memstart_addr > LLONG_MAX); memstart_addr; })
 
 /* the virtual base of the kernel image (minus TEXT_OFFSET) */
 extern u64			kimage_vaddr;
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index ed85778b32e5..023c41f22b5b 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -48,7 +48,13 @@
 
 #include "mm.h"
 
-phys_addr_t memstart_addr __read_mostly = 0;
+/*
+ * We need to be able to catch inadvertent references to memstart_addr
+ * that occur (potentially in generic code) before arm64_memblock_init()
+ * executes, which assigns it its actual value. So use a default value
+ * that cannot be mistaken for a real physical address.
+ */
+phys_addr_t memstart_addr __read_mostly = ULLONG_MAX;
 phys_addr_t arm64_dma_phys_limit __read_mostly;
 
 #ifdef CONFIG_BLK_DEV_INITRD
-- 
2.5.0

[PATCH v3 0/4] arm64: avoid early __va translations

[Laurentiu Tudor]

Hi Ard,

On 02/12/2016 04:57 PM, Ard Biesheuvel wrote:
> 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(-)

Tried these on one of our LS2 boards so here's a:

Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>

---
Best Regards, Laurentiu

[PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end

[Will Deacon]

On Fri, Feb 12, 2016 at 03:57:23PM +0100, Ard Biesheuvel wrote:
> 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 <robh@kernel.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/of/fdt.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Mark, Rob, any comments on this?

Will

> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 1f98156f8996..3e90bce70545 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -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);
> -- 
> 2.5.0
> 

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

[Will Deacon]

On Fri, Feb 12, 2016 at 03:57:26PM +0100, Ard Biesheuvel wrote:
> 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.

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

Will

[PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end

[Ard Biesheuvel]

On 22 February 2016 at 17:49, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Feb 12, 2016 at 03:57:23PM +0100, Ard Biesheuvel wrote:
>> 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 <robh@kernel.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/of/fdt.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> Mark, Rob, any comments on this?
>

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

[PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end

[Will Deacon]

On Mon, Feb 22, 2016 at 05:56:40PM +0100, Ard Biesheuvel wrote:
> On 22 February 2016 at 17:49, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Feb 12, 2016 at 03:57:23PM +0100, Ard Biesheuvel wrote:
> >> 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 <robh@kernel.org>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  drivers/of/fdt.c | 14 +++++++++++---
> >>  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > Mark, Rob, any comments on this?
> >
> 
> 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.

Will

[PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end

[Ard Biesheuvel]

On 22 February 2016 at 18:09, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Feb 22, 2016 at 05:56:40PM +0100, Ard Biesheuvel wrote:
>> On 22 February 2016 at 17:49, Will Deacon <will.deacon@arm.com> wrote:
>> > On Fri, Feb 12, 2016 at 03:57:23PM +0100, Ard Biesheuvel wrote:
>> >> 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 <robh@kernel.org>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  drivers/of/fdt.c | 14 +++++++++++---
>> >>  1 file changed, 11 insertions(+), 3 deletions(-)
>> >
>> > Mark, Rob, any comments on this?
>> >
>>
>> 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.
>

Apologies, I removed you from cc unintentionally.

http://thread.gmane.org/gmane.linux.ports.arm.kernel/478714

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

[Ard Biesheuvel]

On 22 February 2016 at 17:52, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Feb 12, 2016 at 03:57:26PM +0100, Ard Biesheuvel wrote:
>> 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.
>
> 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?

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

[Catalin Marinas]

On Mon, Feb 22, 2016 at 04:52:09PM +0000, Will Deacon wrote:
> On Fri, Feb 12, 2016 at 03:57:26PM +0100, Ard Biesheuvel wrote:
> > 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.
> 
> 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.

-- 
Catalin

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

[Ard Biesheuvel]

On 22 February 2016 at 18:26, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Mon, Feb 22, 2016 at 04:52:09PM +0000, Will Deacon wrote:
>> On Fri, Feb 12, 2016 at 03:57:26PM +0100, Ard Biesheuvel wrote:
>> > 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.
>>
>> 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.
>

OK, let me code that up

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

[Catalin Marinas]

On Mon, Feb 22, 2016 at 06:17:40PM +0100, Ard Biesheuvel wrote:
> On 22 February 2016 at 17:52, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Feb 12, 2016 at 03:57:26PM +0100, Ard Biesheuvel wrote:
> >> 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.
> >
> > 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.

-- 
Catalin

[PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned

[Ard Biesheuvel]

On 22 February 2016 at 18:41, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Mon, Feb 22, 2016 at 06:17:40PM +0100, Ard Biesheuvel wrote:
>> On 22 February 2016 at 17:52, Will Deacon <will.deacon@arm.com> wrote:
>> > On Fri, Feb 12, 2016 at 03:57:26PM +0100, Ard Biesheuvel wrote:
>> >> 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.
>> >
>> > 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.
>

I guess virt_to_page() is only valid on linear addresses, so we could
reimplement it not to use the comparison with PAGE_OFFSET that I added
to __pa() for the kernmap stuff

[PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h

[Will Deacon]

On Fri, Feb 12, 2016 at 03:57:25PM +0100, Ard Biesheuvel wrote:
> 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 <Dave.Martin@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  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(-)
> 
> diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
> index 4a748ce9ba1a..679d49221998 100644
> --- a/arch/arm64/include/asm/bug.h
> +++ b/arch/arm64/include/asm/bug.h
> @@ -18,7 +18,7 @@
>  #ifndef _ARCH_ARM64_ASM_BUG_H
>  #define _ARCH_ARM64_ASM_BUG_H
>  
> -#include <asm/debug-monitors.h>
> +#define BUG_BRK_IMM			0x800

I'd really like to keep all the BRK immediates together, otherwise it's
error-prone if/when we allocate new encodings. Maybe we could add a new
header file for them, if we really need to?

Alternatively, given that this is all behind DEBUG_VM, you could rewrite
the check as if (unlikely(...)) panic(...);

Will