* [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end
2016-02-12 14:57 [PATCH v3 0/4] arm64: avoid early __va translations Ard Biesheuvel
@ 2016-02-12 14:57 ` Ard Biesheuvel
2016-02-22 16:49 ` Will Deacon
2016-02-12 14:57 ` [PATCH v3 2/4] arm64: defer __va translation of initrd_start and initrd_end Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end
2016-02-12 14:57 ` [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end Ard Biesheuvel
@ 2016-02-22 16:49 ` Will Deacon
2016-02-22 16:56 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2016-02-22 16:49 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end
2016-02-22 16:49 ` Will Deacon
@ 2016-02-22 16:56 ` Ard Biesheuvel
2016-02-22 17:09 ` Will Deacon
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-22 16:56 UTC (permalink / raw)
To: linux-arm-kernel
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 ...
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end
2016-02-22 16:56 ` Ard Biesheuvel
@ 2016-02-22 17:09 ` Will Deacon
2016-02-22 17:16 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2016-02-22 17:09 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end
2016-02-22 17:09 ` Will Deacon
@ 2016-02-22 17:16 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-22 17:16 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/4] arm64: defer __va translation of initrd_start and initrd_end
2016-02-12 14:57 [PATCH v3 0/4] arm64: avoid early __va translations Ard Biesheuvel
2016-02-12 14:57 ` [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end Ard Biesheuvel
@ 2016-02-12 14:57 ` Ard Biesheuvel
2016-02-12 14:57 ` [PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h
2016-02-12 14:57 [PATCH v3 0/4] arm64: avoid early __va translations Ard Biesheuvel
2016-02-12 14:57 ` [PATCH v3 1/4] of/fdt: factor out assignment of initrd_start/initrd_end Ard Biesheuvel
2016-02-12 14:57 ` [PATCH v3 2/4] arm64: defer __va translation of initrd_start and initrd_end Ard Biesheuvel
@ 2016-02-12 14:57 ` Ard Biesheuvel
2016-02-22 18:00 ` Will Deacon
2016-02-12 14:57 ` [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned Ard Biesheuvel
2016-02-15 10:42 ` [PATCH v3 0/4] arm64: avoid early __va translations Laurentiu Tudor
4 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h
2016-02-12 14:57 ` [PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h Ard Biesheuvel
@ 2016-02-22 18:00 ` Will Deacon
0 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2016-02-22 18:00 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-12 14:57 [PATCH v3 0/4] arm64: avoid early __va translations Ard Biesheuvel
` (2 preceding siblings ...)
2016-02-12 14:57 ` [PATCH v3 3/4] arm64: prevent potential circular header dependencies in asm/bug.h Ard Biesheuvel
@ 2016-02-12 14:57 ` Ard Biesheuvel
2016-02-22 16:52 ` Will Deacon
2016-02-15 10:42 ` [PATCH v3 0/4] arm64: avoid early __va translations Laurentiu Tudor
4 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-12 14:57 ` [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned Ard Biesheuvel
@ 2016-02-22 16:52 ` Will Deacon
2016-02-22 17:17 ` Ard Biesheuvel
2016-02-22 17:26 ` Catalin Marinas
0 siblings, 2 replies; 17+ messages in thread
From: Will Deacon @ 2016-02-22 16:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-22 16:52 ` Will Deacon
@ 2016-02-22 17:17 ` Ard Biesheuvel
2016-02-22 17:41 ` Catalin Marinas
2016-02-22 17:26 ` Catalin Marinas
1 sibling, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-22 17:17 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-22 17:17 ` Ard Biesheuvel
@ 2016-02-22 17:41 ` Catalin Marinas
2016-02-22 17:55 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Catalin Marinas @ 2016-02-22 17:41 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-22 17:41 ` Catalin Marinas
@ 2016-02-22 17:55 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-22 17:55 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-22 16:52 ` Will Deacon
2016-02-22 17:17 ` Ard Biesheuvel
@ 2016-02-22 17:26 ` Catalin Marinas
2016-02-22 17:38 ` Ard Biesheuvel
1 sibling, 1 reply; 17+ messages in thread
From: Catalin Marinas @ 2016-02-22 17:26 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned
2016-02-22 17:26 ` Catalin Marinas
@ 2016-02-22 17:38 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2016-02-22 17:38 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 0/4] arm64: avoid early __va translations
2016-02-12 14:57 [PATCH v3 0/4] arm64: avoid early __va translations Ard Biesheuvel
` (3 preceding siblings ...)
2016-02-12 14:57 ` [PATCH v3 4/4] arm64: prevent __va() translations before memstart_addr is assigned Ard Biesheuvel
@ 2016-02-15 10:42 ` Laurentiu Tudor
4 siblings, 0 replies; 17+ messages in thread
From: Laurentiu Tudor @ 2016-02-15 10:42 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread