[PATCH v2 0/3] arm64: avoid early __va translations

[PATCH v2 0/3] 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.

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

-- 
2.5.0

[PATCH v2 1/3] 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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/of/fdt.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

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

[PATCH v2 1/3] of/fdt: factor out assignment of initrd_start/initrd_end

[Arnd Bergmann]

On Thursday 11 February 2016 17:48:00 Ard Biesheuvel wrote:
> 
>  #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;
> +}
> +
> 

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

[Ard Biesheuvel]

On 11 February 2016 at 23:12, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 11 February 2016 17:48:00 Ard Biesheuvel wrote:
>>
>>  #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;
>> +}
>> +
>>
>
> 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.
>

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

[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/mm/init.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 3a9fc46cbf80..eff4751f8761 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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();
-- 
2.5.0

[PATCH v2 3/3] arm64: prevent __va() translations before memstart_addr is assigned

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

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 083361531a61..0d4d1b3b9695 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -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.
  */
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index eff4751f8761..e88db8acd181 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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
-- 
2.5.0

[PATCH v2 3/3] arm64: prevent __va() translations before memstart_addr is assigned

[Will Deacon]

On Thu, Feb 11, 2016 at 05:48:02PM +0100, Ard Biesheuvel wrote:
> 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(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 083361531a61..0d4d1b3b9695 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -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));

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

[Ard Biesheuvel]

On 12 February 2016 at 12:49, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Feb 11, 2016 at 05:48:02PM +0100, Ard Biesheuvel wrote:
>> 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(-)
>>
>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> index 083361531a61..0d4d1b3b9695 100644
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -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));
>
> 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.

-- 
Ard.

[PATCH v2 3/3] arm64: prevent __va() translations before memstart_addr is assigned

[Arnd Bergmann]

On Friday 12 February 2016 12:51:40 Ard Biesheuvel wrote:
> >> @@ -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?
> >
> 
> 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