arm64: avoid early __va translations
Subsystems (2)
v2 cover arm64: avoid early __va translations
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.
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) is moved into a __weak function which can be overridden.
Patch #2 overrides the __weak function to only record 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 is some test code I used to verify that all __va() translations are gone. Note that the open coded BUG_ON() implementation is required to avoid .h dependency hell.
Ard Biesheuvel (3):
of/fdt: factor out assignment of initrd_start/initrd_end
arm64: defer __va translation of initrd_start and initrd_end
arm64: prevent __va() translations before memstart_addr is assigned
arch/arm64/include/asm/memory.h | 10 +++++++++- arch/arm64/mm/init.c | 21 +++++++++++++++----- drivers/of/fdt.c | 12 ++++++++--- 3 files changed, 34 insertions(+), 9 deletions(-)
Signature · 1 line
2.5.0
PATCH v2 1/3 of/fdt: factor out assignment of initrd_start/initrd_end
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.
--- drivers/of/fdt.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/of/fdt.c +9 -3
@@ -760,6 +760,14 @@ const void * __init of_flat_dt_match_machine(const void *default_match, }#ifdef CONFIG_BLK_DEV_INITRD +void __weak __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 ; +} +/** * early_init_dt_check_for_initrd - Decode initrd location from flat tree * @node: reference to node containing initrd location ('chosen')
@@ -782,9 +790,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
PATCH v2 1/3 of/fdt: factor out assignment of initrd_start/initrd_end
drivers/of/fdt.c const void * __init of_flat_dt_match_machine(const void *default_match, · line 770
Quoted 11 lines · On Thursday 11 February 2016 17:48:00 Ard Biesheuvel wrote:
#ifdef
CONFIG_BLK_DEV_INITRDdrivers/of/fdt.c +8
-0+void __weak __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 ; +} +
I find __weak functions particularly hard to follow, I think a Kconfig symbols or a function you can override by defining a macro in asm/memory.h would be nicer.
Arnd
PATCH v2 1/3 of/fdt: factor out assignment of initrd_start/initrd_end
Quoted 17 lines · Arnd Bergmann
drivers/of/fdt.c const void * __init of_flat_dt_match_machine(const void *default_match, · line 770
Quoted 11 lines · On Thursday 11 February 2016 17:48:00 Ard Biesheuvel wrote:
#ifdef
CONFIG_BLK_DEV_INITRDdrivers/of/fdt.c +8
-0+void __weak __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 ; +} +I find
__weakfunctions particularly hard to follow, I think aKconfigsymbols or a function you can override by defining a macro inasm/memory.hwould be nicer.
OK, that makes sense.
I will give Catalin a chance to comment before respinning, though.
PATCH v2 2/3 arm64: defer __va translation of initrd_start and initrd_end
In reply to Ard BiesheuvelCommit 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.
--- arch/arm64/mm/init.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
arch/arm64/mm/init.c +15 -4
@@ -61,12 +61,18 @@ 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; }early_param ("initrd" , early_initrd); + +void __early_init_dt_declare_initrd (unsigned long start,unsigned long end) +{ + initrd_start = start; + initrd_end = end; +}#endif /*
@@ -213,8 +219,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 ();
Signature · 1 line
2.5.0
PATCH v2 3/3 arm64: prevent __va() translations before memstart_addr is assigned
In reply to Ard BiesheuvelJust a hack to check whether all early __va() calls are gone.
--- arch/arm64/include/asm/memory.h | 10 +++++++++- arch/arm64/mm/init.c | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-)
arch/arm64/include/asm/memory.h +9 -1
@@ -90,7 +90,9 @@ __x >=PAGE_OFFSET ? (__x -PAGE_OFFSET +PHYS_OFFSET ) : \ (__x - kimage_voffset); }) -#define __phys_to_virt (x) ((unsigned long)((x) - PHYS_OFFSET + PAGE_OFFSET)) +#define __phys_to_virt (x) ({ \ + assert_memstart_addr_assigned(); \ + (unsigned long)((x) - PHYS_OFFSET + PAGE_OFFSET); })#define __phys_to_kimg (x) ((unsigned long)((x) + kimage_voffset))/*
@@ -133,6 +135,12 @@ extern u64 kimage_vaddr;/* the offset between the kernel virtual and physical mappings */ extern u64 kimage_voffset; +static inline void assert_memstart_addr_assigned (void ) +{ +if (unlikely (memstart_addr == (phys_addr_t)-1 )) + asm("brk #%0" ::"I" (0x800 )); +} +/* * Allow all memory at the discovery stage. We will clip it later. */
arch/arm64/mm/init.c +1 -1
@@ -48,7 +48,7 @@#include "mm.h" -phys_addr_t memstart_addr__read_mostly =0 ; +phys_addr_t memstart_addr__read_mostly = (phys_addr_t)-1 ;phys_addr_t arm64_dma_phys_limit__read_mostly ;#ifdef CONFIG_BLK_DEV_INITRD
Signature · 1 line
2.5.0
PATCH v2 3/3 arm64: prevent __va() translations before memstart_addr is assigned
Quoted 22 lines · Ard Biesheuvel
Just a hack to check whether all early
__va()calls are gone.--- arch/arm64/include/asm/memory.h | 10 +++++++++- arch/arm64/mm/init.c | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/include/asm/memory.h +3
-1@@ -90,7 +90,9 @@ __x >=PAGE_OFFSET ? (__x -PAGE_OFFSET +PHYS_OFFSET ) : \ (__x - kimage_voffset); }) -#define __phys_to_virt (x) ((unsigned long)((x) - PHYS_OFFSET + PAGE_OFFSET)) +#define __phys_to_virt (x) ({ \ + assert_memstart_addr_assigned(); \ + (unsigned long)((x) - PHYS_OFFSET + PAGE_OFFSET); })#define __phys_to_kimg (x) ((unsigned long)((x) + kimage_voffset))/*
arch/arm64/include/asm/memory.h extern u64 kimage_vaddr; · line 141
Quoted 7 lines
arch/arm64/include/asm/memory.h +4
-0@@ -133,6 +135,12 @@ extern u64 kimage_vaddr;/* the offset between the kernel virtual and physical mappings */ extern u64 kimage_voffset; +static inline void assert_memstart_addr_assigned (void ) +{ +if (unlikely (memstart_addr == (phys_addr_t)-1 )) + asm("brk #%0" ::"I" (0x800 ));
Ok, I'll bite! Why isn't this just a BUG_ON?
Will
PATCH v2 3/3 arm64: prevent __va() translations before memstart_addr is assigned
Quoted 26 lines · Will Deacon
Quoted 25 lines · Ard Biesheuvel
Just a hack to check whether all early
__va()calls are gone.--- arch/arm64/include/asm/memory.h | 10 +++++++++- arch/arm64/mm/init.c | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/include/asm/memory.h +3
-1@@ -90,7 +90,9 @@ __x >=PAGE_OFFSET ? (__x -PAGE_OFFSET +PHYS_OFFSET ) : \ (__x - kimage_voffset); }) -#define __phys_to_virt (x) ((unsigned long)((x) - PHYS_OFFSET + PAGE_OFFSET)) +#define __phys_to_virt (x) ({ \ + assert_memstart_addr_assigned(); \ + (unsigned long)((x) - PHYS_OFFSET + PAGE_OFFSET); })#define __phys_to_kimg (x) ((unsigned long)((x) + kimage_voffset))/* @@ -133,6 +135,12 @@ extern u64 kimage_vaddr;/* the offset between the kernel virtual and physical mappings */ extern u64 kimage_voffset;
Quoted 7 lines
arch/arm64/include/asm/memory.h extern u64 kimage_vaddr; · line 141
Quoted 4 lines
arch/arm64/include/asm/memory.h +4
-0+static inline void assert_memstart_addr_assigned (void ) +{ +if (unlikely (memstart_addr == (phys_addr_t)-1 )) + asm("brk #%0" ::"I" (0x800 ));Ok, I'll bite! Why isn't this just a
BUG_ON?
Because circular header dependencies prevent BUG_ON() from being used here, and I was reluctant to move this function into a .c file.
Note that I am not necessarily suggesting that this patch be merged, but I included it since it's the code I used to confirm that no other early instance of __va() remain. I can try and clean it up if we want to keep it.
Signature · 1 line
Ard.
PATCH v2 3/3 arm64: prevent __va() translations before memstart_addr is assigned
Quoted 12 lines · On Friday 12 February 2016 12:51:40 Ard Biesheuvel wrote:
Quoted 11 lines
arch/arm64/include/asm/memory.h extern u64 kimage_vaddr; · line 141
Quoted 8 lines
arch/arm64/include/asm/memory.h +4
-0@@ -133,6 +135,12 @@ extern u64 kimage_vaddr;/* the offset between the kernel virtual and physical mappings */ extern u64 kimage_voffset; +static inline void assert_memstart_addr_assigned (void ) +{ +if (unlikely (memstart_addr == (phys_addr_t)-1 )) + asm("brk #%0" ::"I" (0x800 ));Ok, I'll bite! Why isn't this just a
BUG_ON?
Quoted 2 lines
Because circular header dependencies prevent
BUG_ON()from being used here, and I was reluctant to move this function into a .c file.
Maybe it works if you make assert_memstart_addr_assigned() a macro as well?
Arnd