[PATCH v3 0/6] arm64: Get rid of __early_init_dt_declare_initrd()

[PATCH v3 0/6] arm64: Get rid of __early_init_dt_declare_initrd()

[Florian Fainelli]

Hi all,

Changes in v3:

- use C conditionals in drivers/of/fdt.c
- added check on phys_initrd_size in arch/arm64/mm/init.c to determine
  whether initrd_start must be populated
- fixed a build warning with ARC that was just missing an (unsigned
  long) cast

Changes in v2:

- get rid of ARCH_HAS_PHYS_INITRD and instead define
  phys_initrd_start/phys_initrd_size in init/do_mounts_initrd.c

- make __early_init_dt_declare_initrd() account for ARM64 specific
  behavior with __va() when having CONFIG_DEBUG_VM enabled

- consolidate early_initrd() command line parsing into
  init/do_mounts_initrd.c

Because phys_initrd_start/phys_initrd_size are now compiled in
ini/do_mounts_initrd.c which is only built with CONFIG_BLK_DEV_INITRD=y,
we need to be a bit careful about the uses throughout architecture
specific code.

Previous discussions/submissions list here:

v3:
https://www.spinics.net/lists/arm-kernel/msg683566.html
v2:
https://lkml.org/lkml/2018/10/25/4

Florian Fainelli (6):
  nds32: Remove phys_initrd_start and phys_initrd_size
  arch: Make phys_initrd_start and phys_initrd_size global variables
  of/fdt: Populate phys_initrd_start/phys_initrd_size from FDT
  arm64: Utilize phys_initrd_start/phys_initrd_size
  of/fdt: Remove custom __early_init_dt_declare_initrd() implementation
  arch: Move initrd= parsing into do_mounts_initrd.c

 arch/arc/mm/init.c              | 25 +++++--------------------
 arch/arm/mm/init.c              | 28 ++--------------------------
 arch/arm64/include/asm/memory.h |  8 --------
 arch/arm64/mm/init.c            | 33 +++++++--------------------------
 arch/nds32/mm/init.c            |  2 --
 arch/unicore32/mm/init.c        | 24 +++++-------------------
 drivers/of/fdt.c                | 17 ++++++++++++-----
 include/linux/initrd.h          |  3 +++
 init/do_mounts_initrd.c         | 20 ++++++++++++++++++++
 9 files changed, 54 insertions(+), 106 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] nds32: Remove phys_initrd_start and phys_initrd_size

[Florian Fainelli]

This will conflict with a subsequent change making phys_initrd_start and
phys_initrd_size global variables. nds32 does not make use of those nor
provides a suitable declarations so just get rid of them.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/nds32/mm/init.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/nds32/mm/init.c b/arch/nds32/mm/init.c
index c713d2ad55dc..32f55a24ccbb 100644
--- a/arch/nds32/mm/init.c
+++ b/arch/nds32/mm/init.c
@@ -22,8 +22,6 @@
 DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
 DEFINE_SPINLOCK(anon_alias_lock);
 extern pgd_t swapper_pg_dir[PTRS_PER_PGD];
-extern unsigned long phys_initrd_start;
-extern unsigned long phys_initrd_size;
 
 /*
  * empty_zero_page is a special page that is used for
-- 
2.17.1

[PATCH v3 2/6] arch: Make phys_initrd_start and phys_initrd_size global variables

[Florian Fainelli]

Make phys_initrd_start and phys_initrd_size global variables declared in
init/do_mounts_initrd.c such that we can later have generic code in
drivers/of/fdt.c populate those variables for us.

This requires both the ARM and unicore32 implementations to be properly
guarded against CONFIG_BLK_DEV_INITRD, and also initialize the variables
to the expected default values (unicore32).

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/mm/init.c       |  5 ++---
 arch/unicore32/mm/init.c | 10 +++++++---
 include/linux/initrd.h   |  3 +++
 init/do_mounts_initrd.c  |  3 +++
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 0cc8e04295a4..87d59a53861d 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -51,9 +51,7 @@ unsigned long __init __clear_cr(unsigned long mask)
 }
 #endif
 
-static phys_addr_t phys_initrd_start __initdata = 0;
-static unsigned long phys_initrd_size __initdata = 0;
-
+#ifdef CONFIG_BLK_DEV_INITRD
 static int __init early_initrd(char *p)
 {
 	phys_addr_t start;
@@ -90,6 +88,7 @@ static int __init parse_tag_initrd2(const struct tag *tag)
 }
 
 __tagtable(ATAG_INITRD2, parse_tag_initrd2);
+#endif
 
 static void __init find_limits(unsigned long *min, unsigned long *max_low,
 			       unsigned long *max_high)
diff --git a/arch/unicore32/mm/init.c b/arch/unicore32/mm/init.c
index 8f8699e62bd5..f2f815d46846 100644
--- a/arch/unicore32/mm/init.c
+++ b/arch/unicore32/mm/init.c
@@ -31,9 +31,7 @@
 
 #include "mm.h"
 
-static unsigned long phys_initrd_start __initdata = 0x01000000;
-static unsigned long phys_initrd_size __initdata = SZ_8M;
-
+#ifdef CONFIG_BLK_DEV_INITRD
 static int __init early_initrd(char *p)
 {
 	unsigned long start, size;
@@ -49,6 +47,7 @@ static int __init early_initrd(char *p)
 	return 0;
 }
 early_param("initrd", early_initrd);
+#endif
 
 /*
  * This keeps memory configuration data used by a couple memory
@@ -157,6 +156,11 @@ void __init uc32_memblock_init(struct meminfo *mi)
 	memblock_reserve(__pa(_text), _end - _text);
 
 #ifdef CONFIG_BLK_DEV_INITRD
+	if (!phys_initrd_size) {
+		phys_initrd_start = 0x01000000;
+		phys_initrd_size = SZ_8M;
+	}
+
 	if (phys_initrd_size) {
 		memblock_reserve(phys_initrd_start, phys_initrd_size);
 
diff --git a/include/linux/initrd.h b/include/linux/initrd.h
index 84b423044088..14beaff9b445 100644
--- a/include/linux/initrd.h
+++ b/include/linux/initrd.h
@@ -21,4 +21,7 @@ extern int initrd_below_start_ok;
 extern unsigned long initrd_start, initrd_end;
 extern void free_initrd_mem(unsigned long, unsigned long);
 
+extern phys_addr_t phys_initrd_start;
+extern unsigned long phys_initrd_size;
+
 extern unsigned int real_root_dev;
diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index d1a5d885ce13..45865b72f4ea 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -16,6 +16,9 @@ int initrd_below_start_ok;
 unsigned int real_root_dev;	/* do_proc_dointvec cannot handle kdev_t */
 static int __initdata mount_initrd = 1;
 
+phys_addr_t phys_initrd_start __initdata;
+unsigned long phys_initrd_size __initdata;
+
 static int __init no_initrd(char *str)
 {
 	mount_initrd = 0;
-- 
2.17.1

[PATCH v3 3/6] of/fdt: Populate phys_initrd_start/phys_initrd_size from FDT

[Florian Fainelli]

Now that we have central and global variables holding the physical
address and size of the initrd, we can have
early_init_dt_check_for_initrd() populate
phys_initrd_start/phys_initrd_size for us.

This allows us to remove a chunk of code from arch/arm/mm/init.c
introduced with commit 65939301acdb ("arm: set initrd_start/initrd_end
for fdt scan").

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/mm/init.c | 6 ------
 drivers/of/fdt.c   | 2 ++
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 87d59a53861d..4bfa08e27319 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -236,12 +236,6 @@ static void __init arm_initrd_init(void)
 	phys_addr_t start;
 	unsigned long size;
 
-	/* FDT scan will populate initrd_start */
-	if (initrd_start && !phys_initrd_size) {
-		phys_initrd_start = __virt_to_phys(initrd_start);
-		phys_initrd_size = initrd_end - initrd_start;
-	}
-
 	initrd_start = initrd_end = 0;
 
 	if (!phys_initrd_size)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 76c83c1ffeda..e34cb49231b5 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -925,6 +925,8 @@ static void __init early_init_dt_check_for_initrd(unsigned long node)
 	end = of_read_number(prop, len/4);
 
 	__early_init_dt_declare_initrd(start, end);
+	phys_initrd_start = start;
+	phys_initrd_size = end - start;
 
 	pr_debug("initrd_start=0x%llx  initrd_end=0x%llx\n",
 		 (unsigned long long)start, (unsigned long long)end);
-- 
2.17.1

[PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Florian Fainelli]

ARM64 is the only architecture that re-defines
__early_init_dt_declare_initrd() in order for that function to populate
initrd_start/initrd_end with physical addresses instead of virtual
addresses. Instead of having an override we can leverage
drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
populate those variables for us.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/mm/init.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 3cf87341859f..00ef2166bb73 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
 	if (*endp == ',') {
 		size = memparse(endp + 1, NULL);
 
-		initrd_start = start;
-		initrd_end = start + size;
+		phys_initrd_start = start;
+		phys_initrd_size = size;
 	}
 	return 0;
 }
@@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
 		memblock_add(__pa_symbol(_text), (u64)(_end - _text));
 	}
 
-	if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
+	if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
 		/*
 		 * Add back the memory we just removed if it results in the
 		 * initrd to become inaccessible via the linear mapping.
 		 * Otherwise, this is a no-op
 		 */
-		u64 base = initrd_start & PAGE_MASK;
-		u64 size = PAGE_ALIGN(initrd_end) - base;
+		u64 base = phys_initrd_start & PAGE_MASK;
+		u64 size = PAGE_ALIGN(phys_initrd_size);
 
 		/*
 		 * We can only add back the initrd memory if we don't end up
@@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
 	 */
 	memblock_reserve(__pa_symbol(_text), _end - _text);
 #ifdef CONFIG_BLK_DEV_INITRD
-	if (initrd_start) {
-		memblock_reserve(initrd_start, initrd_end - initrd_start);
-
+	if (phys_initrd_size) {
 		/* the generic initrd code expects virtual addresses */
-		initrd_start = __phys_to_virt(initrd_start);
-		initrd_end = __phys_to_virt(initrd_end);
+		initrd_start = __phys_to_virt(phys_initrd_start);
+		initrd_end = initrd_start + phys_initrd_size;
+		initrd_below_start_ok = 0;
 	}
 #endif
 
-- 
2.17.1

[PATCH v3 5/6] of/fdt: Remove custom __early_init_dt_declare_initrd() implementation

[Florian Fainelli]

Now that ARM64 uses phys_initrd_start/phys_initrd_size, we can get rid
of its custom __early_init_dt_declare_initrd() which causes a fair
amount of objects rebuild when changing CONFIG_BLK_DEV_INITRD. In order
to make sure ARM64 does not produce a BUG() when VM debugging is turned
on though, we must avoid early calls to __va() which is what
__early_init_dt_declare_initrd() does and wrap this around to avoid
running that code on ARM64.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/include/asm/memory.h |  8 --------
 drivers/of/fdt.c                | 15 ++++++++++-----
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index b96442960aea..dc3ca21ba240 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -168,14 +168,6 @@
 #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__
 
 #include <linux/bitops.h>
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e34cb49231b5..4118a344cf45 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -892,15 +892,20 @@ 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;
+	/* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
+	 * enabled since __va() is called too early. ARM64 does make use
+	 * of phys_initrd_start/phys_initrd_size so we can skip this
+	 * conversion.
+	 */
+	if (!IS_ENABLED(CONFIG_ARM64)) {
+		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
-- 
2.17.1

[PATCH v3 6/6] arch: Move initrd= parsing into do_mounts_initrd.c

[Florian Fainelli]

ARC, ARM, ARM64 and Unicore32 are all capable of parsing the "initrd="
command line parameter to allow specifying the physical address and size
of an initrd. Move that parsing into init/do_mounts_initrd.c such that
we no longer duplicate that logic.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arc/mm/init.c       | 25 +++++--------------------
 arch/arm/mm/init.c       | 17 -----------------
 arch/arm64/mm/init.c     | 18 ------------------
 arch/unicore32/mm/init.c | 18 ------------------
 init/do_mounts_initrd.c  | 17 +++++++++++++++++
 5 files changed, 22 insertions(+), 73 deletions(-)

diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
index ba145065c579..55879a9dee0d 100644
--- a/arch/arc/mm/init.c
+++ b/arch/arc/mm/init.c
@@ -79,24 +79,6 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
 		base, TO_MB(size), !in_use ? "Not used":"");
 }
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	unsigned long start, size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		initrd_start = (unsigned long)__va(start);
-		initrd_end = (unsigned long)__va(start + size);
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-#endif
-
 /*
  * First memory setup routine called from setup_arch()
  * 1. setup swapper's mm @init_mm
@@ -141,8 +123,11 @@ void __init setup_arch_memory(void)
 	memblock_reserve(low_mem_start, __pa(_end) - low_mem_start);
 
 #ifdef CONFIG_BLK_DEV_INITRD
-	if (initrd_start)
-		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
+	if (phys_initrd_size) {
+		memblock_reserve(phys_initrd_start, phys_initrd_size);
+		initrd_start = (unsigned long)__va(phys_initrd_start);
+		initrd_end = initrd_start + phys_initrd_size;
+	}
 #endif
 
 	early_init_fdt_reserve_self();
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 4bfa08e27319..58ec68709606 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -52,23 +52,6 @@ unsigned long __init __clear_cr(unsigned long mask)
 #endif
 
 #ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	phys_addr_t start;
-	unsigned long size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		phys_initrd_start = start;
-		phys_initrd_size = size;
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-
 static int __init parse_tag_initrd(const struct tag *tag)
 {
 	pr_warn("ATAG_INITRD is deprecated; "
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 00ef2166bb73..d95a6cb205b8 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -62,24 +62,6 @@
 s64 memstart_addr __ro_after_init = -1;
 phys_addr_t arm64_dma_phys_limit __ro_after_init;
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	unsigned long start, size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		phys_initrd_start = start;
-		phys_initrd_size = size;
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-#endif
-
 #ifdef CONFIG_KEXEC_CORE
 /*
  * reserve_crashkernel() - reserves memory for crash kernel
diff --git a/arch/unicore32/mm/init.c b/arch/unicore32/mm/init.c
index f2f815d46846..99acdb829a7e 100644
--- a/arch/unicore32/mm/init.c
+++ b/arch/unicore32/mm/init.c
@@ -31,24 +31,6 @@
 
 #include "mm.h"
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	unsigned long start, size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		phys_initrd_start = start;
-		phys_initrd_size = size;
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-#endif
-
 /*
  * This keeps memory configuration data used by a couple memory
  * initialization functions, as well as show_mem() for the skipping
diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index 45865b72f4ea..732d21f4a637 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -27,6 +27,23 @@ static int __init no_initrd(char *str)
 
 __setup("noinitrd", no_initrd);
 
+static int __init early_initrd(char *p)
+{
+	phys_addr_t start;
+	unsigned long size;
+	char *endp;
+
+	start = memparse(p, &endp);
+	if (*endp == ',') {
+		size = memparse(endp + 1, NULL);
+
+		phys_initrd_start = start;
+		phys_initrd_size = size;
+	}
+	return 0;
+}
+early_param("initrd", early_initrd);
+
 static int init_linuxrc(struct subprocess_info *info, struct cred *new)
 {
 	ksys_unshare(CLONE_FS | CLONE_FILES);
-- 
2.17.1

Re: [PATCH v3 0/6] arm64: Get rid of __early_init_dt_declare_initrd()

[Rob Herring]

On Wed, Oct 31, 2018 at 12:28:37PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> Changes in v3:
> 
> - use C conditionals in drivers/of/fdt.c
> - added check on phys_initrd_size in arch/arm64/mm/init.c to determine
>   whether initrd_start must be populated
> - fixed a build warning with ARC that was just missing an (unsigned
>   long) cast
> 
> Changes in v2:
> 
> - get rid of ARCH_HAS_PHYS_INITRD and instead define
>   phys_initrd_start/phys_initrd_size in init/do_mounts_initrd.c
> 
> - make __early_init_dt_declare_initrd() account for ARM64 specific
>   behavior with __va() when having CONFIG_DEBUG_VM enabled
> 
> - consolidate early_initrd() command line parsing into
>   init/do_mounts_initrd.c
> 
> Because phys_initrd_start/phys_initrd_size are now compiled in
> ini/do_mounts_initrd.c which is only built with CONFIG_BLK_DEV_INITRD=y,
> we need to be a bit careful about the uses throughout architecture
> specific code.
> 
> Previous discussions/submissions list here:
> 
> v3:
> https://www.spinics.net/lists/arm-kernel/msg683566.html
> v2:
> https://lkml.org/lkml/2018/10/25/4
> 
> Florian Fainelli (6):
>   nds32: Remove phys_initrd_start and phys_initrd_size
>   arch: Make phys_initrd_start and phys_initrd_size global variables
>   of/fdt: Populate phys_initrd_start/phys_initrd_size from FDT
>   arm64: Utilize phys_initrd_start/phys_initrd_size
>   of/fdt: Remove custom __early_init_dt_declare_initrd() implementation
>   arch: Move initrd= parsing into do_mounts_initrd.c

This all looks good to me. I can take it via the DT if you want. I'll 
give folks some more time to review though.

Rob

> 
>  arch/arc/mm/init.c              | 25 +++++--------------------
>  arch/arm/mm/init.c              | 28 ++--------------------------
>  arch/arm64/include/asm/memory.h |  8 --------
>  arch/arm64/mm/init.c            | 33 +++++++--------------------------
>  arch/nds32/mm/init.c            |  2 --
>  arch/unicore32/mm/init.c        | 24 +++++-------------------
>  drivers/of/fdt.c                | 17 ++++++++++++-----
>  include/linux/initrd.h          |  3 +++
>  init/do_mounts_initrd.c         | 20 ++++++++++++++++++++
>  9 files changed, 54 insertions(+), 106 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Rob Herring]

On Wed, Oct 31, 2018 at 12:28:41PM -0700, Florian Fainelli wrote:
> ARM64 is the only architecture that re-defines
> __early_init_dt_declare_initrd() in order for that function to populate
> initrd_start/initrd_end with physical addresses instead of virtual
> addresses. Instead of having an override we can leverage
> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
> populate those variables for us.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm64/mm/init.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 3cf87341859f..00ef2166bb73 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>  	if (*endp == ',') {
>  		size = memparse(endp + 1, NULL);
>  
> -		initrd_start = start;
> -		initrd_end = start + size;
> +		phys_initrd_start = start;
> +		phys_initrd_size = size;
>  	}
>  	return 0;
>  }
> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>  		memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>  	}
>  
> -	if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
> +	if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>  		/*
>  		 * Add back the memory we just removed if it results in the
>  		 * initrd to become inaccessible via the linear mapping.
>  		 * Otherwise, this is a no-op
>  		 */
> -		u64 base = initrd_start & PAGE_MASK;
> -		u64 size = PAGE_ALIGN(initrd_end) - base;
> +		u64 base = phys_initrd_start & PAGE_MASK;
> +		u64 size = PAGE_ALIGN(phys_initrd_size);
>  
>  		/*
>  		 * We can only add back the initrd memory if we don't end up
> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>  	 */
>  	memblock_reserve(__pa_symbol(_text), _end - _text);
>  #ifdef CONFIG_BLK_DEV_INITRD
> -	if (initrd_start) {
> -		memblock_reserve(initrd_start, initrd_end - initrd_start);
> -
> +	if (phys_initrd_size) {

Since we're touching the if anyways, perhaps convert the #ifdef to a C 
IS_ENABLED().

>  		/* the generic initrd code expects virtual addresses */
> -		initrd_start = __phys_to_virt(initrd_start);
> -		initrd_end = __phys_to_virt(initrd_end);
> +		initrd_start = __phys_to_virt(phys_initrd_start);
> +		initrd_end = initrd_start + phys_initrd_size;
> +		initrd_below_start_ok = 0;
>  	}
>  #endif
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Ard Biesheuvel]

Hi Florian,

On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
> ARM64 is the only architecture that re-defines
> __early_init_dt_declare_initrd() in order for that function to populate
> initrd_start/initrd_end with physical addresses instead of virtual
> addresses. Instead of having an override we can leverage
> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
> populate those variables for us.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm64/mm/init.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 3cf87341859f..00ef2166bb73 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>         if (*endp == ',') {
>                 size = memparse(endp + 1, NULL);
>
> -               initrd_start = start;
> -               initrd_end = start + size;
> +               phys_initrd_start = start;
> +               phys_initrd_size = size;
>         }
>         return 0;
>  }
> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>         }
>
> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>                 /*
>                  * Add back the memory we just removed if it results in the
>                  * initrd to become inaccessible via the linear mapping.
>                  * Otherwise, this is a no-op
>                  */
> -               u64 base = initrd_start & PAGE_MASK;
> -               u64 size = PAGE_ALIGN(initrd_end) - base;
> +               u64 base = phys_initrd_start & PAGE_MASK;
> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>
>                 /*
>                  * We can only add back the initrd memory if we don't end up
> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>          */
>         memblock_reserve(__pa_symbol(_text), _end - _text);
>  #ifdef CONFIG_BLK_DEV_INITRD
> -       if (initrd_start) {
> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
> -
> +       if (phys_initrd_size) {
>                 /* the generic initrd code expects virtual addresses */
> -               initrd_start = __phys_to_virt(initrd_start);
> -               initrd_end = __phys_to_virt(initrd_end);
> +               initrd_start = __phys_to_virt(phys_initrd_start);
> +               initrd_end = initrd_start + phys_initrd_size;
> +               initrd_below_start_ok = 0;

Where is this assignment coming from?

>         }
>  #endif
>
> --
> 2.17.1
>

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Florian Fainelli]

On 11/5/18 12:39 PM, Ard Biesheuvel wrote:
> Hi Florian,
> 
> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> ARM64 is the only architecture that re-defines
>> __early_init_dt_declare_initrd() in order for that function to populate
>> initrd_start/initrd_end with physical addresses instead of virtual
>> addresses. Instead of having an override we can leverage
>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
>> populate those variables for us.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  arch/arm64/mm/init.c | 19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>> index 3cf87341859f..00ef2166bb73 100644
>> --- a/arch/arm64/mm/init.c
>> +++ b/arch/arm64/mm/init.c
>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>>         if (*endp == ',') {
>>                 size = memparse(endp + 1, NULL);
>>
>> -               initrd_start = start;
>> -               initrd_end = start + size;
>> +               phys_initrd_start = start;
>> +               phys_initrd_size = size;
>>         }
>>         return 0;
>>  }
>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>>         }
>>
>> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
>> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>>                 /*
>>                  * Add back the memory we just removed if it results in the
>>                  * initrd to become inaccessible via the linear mapping.
>>                  * Otherwise, this is a no-op
>>                  */
>> -               u64 base = initrd_start & PAGE_MASK;
>> -               u64 size = PAGE_ALIGN(initrd_end) - base;
>> +               u64 base = phys_initrd_start & PAGE_MASK;
>> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>>
>>                 /*
>>                  * We can only add back the initrd memory if we don't end up
>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>>          */
>>         memblock_reserve(__pa_symbol(_text), _end - _text);
>>  #ifdef CONFIG_BLK_DEV_INITRD
>> -       if (initrd_start) {
>> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
>> -
>> +       if (phys_initrd_size) {
>>                 /* the generic initrd code expects virtual addresses */
>> -               initrd_start = __phys_to_virt(initrd_start);
>> -               initrd_end = __phys_to_virt(initrd_end);
>> +               initrd_start = __phys_to_virt(phys_initrd_start);
>> +               initrd_end = initrd_start + phys_initrd_size;
>> +               initrd_below_start_ok = 0;
> 
> Where is this assignment coming from?

__early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though
after patch #5 this is not necessary any more.
-- 
Florian

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Ard Biesheuvel]

On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/5/18 12:39 PM, Ard Biesheuvel wrote:
>> Hi Florian,
>>
>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> ARM64 is the only architecture that re-defines
>>> __early_init_dt_declare_initrd() in order for that function to populate
>>> initrd_start/initrd_end with physical addresses instead of virtual
>>> addresses. Instead of having an override we can leverage
>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
>>> populate those variables for us.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>  arch/arm64/mm/init.c | 19 +++++++++----------
>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>>> index 3cf87341859f..00ef2166bb73 100644
>>> --- a/arch/arm64/mm/init.c
>>> +++ b/arch/arm64/mm/init.c
>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>>>         if (*endp == ',') {
>>>                 size = memparse(endp + 1, NULL);
>>>
>>> -               initrd_start = start;
>>> -               initrd_end = start + size;
>>> +               phys_initrd_start = start;
>>> +               phys_initrd_size = size;
>>>         }
>>>         return 0;
>>>  }
>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>>>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>>>         }
>>>
>>> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
>>> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>>>                 /*
>>>                  * Add back the memory we just removed if it results in the
>>>                  * initrd to become inaccessible via the linear mapping.
>>>                  * Otherwise, this is a no-op
>>>                  */
>>> -               u64 base = initrd_start & PAGE_MASK;
>>> -               u64 size = PAGE_ALIGN(initrd_end) - base;
>>> +               u64 base = phys_initrd_start & PAGE_MASK;
>>> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>>>
>>>                 /*
>>>                  * We can only add back the initrd memory if we don't end up
>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>>>          */
>>>         memblock_reserve(__pa_symbol(_text), _end - _text);
>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>> -       if (initrd_start) {
>>> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
>>> -
>>> +       if (phys_initrd_size) {
>>>                 /* the generic initrd code expects virtual addresses */
>>> -               initrd_start = __phys_to_virt(initrd_start);
>>> -               initrd_end = __phys_to_virt(initrd_end);
>>> +               initrd_start = __phys_to_virt(phys_initrd_start);
>>> +               initrd_end = initrd_start + phys_initrd_size;
>>> +               initrd_below_start_ok = 0;
>>
>> Where is this assignment coming from?
>
> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though
> after patch #5 this is not necessary any more.

Yes, but why? The original arm64 version of
__early_init_dt_declare_initrd() does not set it but now you set to 1
in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it
back to 0 here.

Or am I missing something?

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Florian Fainelli]

On 11/5/18 12:44 PM, Ard Biesheuvel wrote:
> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote:
>>> Hi Florian,
>>>
>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> ARM64 is the only architecture that re-defines
>>>> __early_init_dt_declare_initrd() in order for that function to populate
>>>> initrd_start/initrd_end with physical addresses instead of virtual
>>>> addresses. Instead of having an override we can leverage
>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
>>>> populate those variables for us.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>  arch/arm64/mm/init.c | 19 +++++++++----------
>>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>>>> index 3cf87341859f..00ef2166bb73 100644
>>>> --- a/arch/arm64/mm/init.c
>>>> +++ b/arch/arm64/mm/init.c
>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>>>>         if (*endp == ',') {
>>>>                 size = memparse(endp + 1, NULL);
>>>>
>>>> -               initrd_start = start;
>>>> -               initrd_end = start + size;
>>>> +               phys_initrd_start = start;
>>>> +               phys_initrd_size = size;
>>>>         }
>>>>         return 0;
>>>>  }
>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>>>>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>>>>         }
>>>>
>>>> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
>>>> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>>>>                 /*
>>>>                  * Add back the memory we just removed if it results in the
>>>>                  * initrd to become inaccessible via the linear mapping.
>>>>                  * Otherwise, this is a no-op
>>>>                  */
>>>> -               u64 base = initrd_start & PAGE_MASK;
>>>> -               u64 size = PAGE_ALIGN(initrd_end) - base;
>>>> +               u64 base = phys_initrd_start & PAGE_MASK;
>>>> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>>>>
>>>>                 /*
>>>>                  * We can only add back the initrd memory if we don't end up
>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>>>>          */
>>>>         memblock_reserve(__pa_symbol(_text), _end - _text);
>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>> -       if (initrd_start) {
>>>> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
>>>> -
>>>> +       if (phys_initrd_size) {
>>>>                 /* the generic initrd code expects virtual addresses */
>>>> -               initrd_start = __phys_to_virt(initrd_start);
>>>> -               initrd_end = __phys_to_virt(initrd_end);
>>>> +               initrd_start = __phys_to_virt(phys_initrd_start);
>>>> +               initrd_end = initrd_start + phys_initrd_size;
>>>> +               initrd_below_start_ok = 0;
>>>
>>> Where is this assignment coming from?
>>
>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though
>> after patch #5 this is not necessary any more.
> 
> Yes, but why? The original arm64 version of
> __early_init_dt_declare_initrd() does not set it but now you set to 1
> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it
> back to 0 here.

Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not
be taking that branch on an ARM64 kernel.

If you are saying the assignment is not necessary anymore after patch #5
, that is true, though this can only be done a part of part #5, not as
part of patch #4 in order not to break initrd functionality in-between
patches.

> 
> Or am I missing something?
> 

Not sure, I could be too, it's Monday after all :)
-- 
Florian

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Ard Biesheuvel]

On 5 November 2018 at 21:51, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/5/18 12:44 PM, Ard Biesheuvel wrote:
>> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote:
>>>> Hi Florian,
>>>>
>>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> ARM64 is the only architecture that re-defines
>>>>> __early_init_dt_declare_initrd() in order for that function to populate
>>>>> initrd_start/initrd_end with physical addresses instead of virtual
>>>>> addresses. Instead of having an override we can leverage
>>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
>>>>> populate those variables for us.
>>>>>
>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>> ---
>>>>>  arch/arm64/mm/init.c | 19 +++++++++----------
>>>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>>>>> index 3cf87341859f..00ef2166bb73 100644
>>>>> --- a/arch/arm64/mm/init.c
>>>>> +++ b/arch/arm64/mm/init.c
>>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>>>>>         if (*endp == ',') {
>>>>>                 size = memparse(endp + 1, NULL);
>>>>>
>>>>> -               initrd_start = start;
>>>>> -               initrd_end = start + size;
>>>>> +               phys_initrd_start = start;
>>>>> +               phys_initrd_size = size;
>>>>>         }
>>>>>         return 0;
>>>>>  }
>>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>>>>>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>>>>>         }
>>>>>
>>>>> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
>>>>> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>>>>>                 /*
>>>>>                  * Add back the memory we just removed if it results in the
>>>>>                  * initrd to become inaccessible via the linear mapping.
>>>>>                  * Otherwise, this is a no-op
>>>>>                  */
>>>>> -               u64 base = initrd_start & PAGE_MASK;
>>>>> -               u64 size = PAGE_ALIGN(initrd_end) - base;
>>>>> +               u64 base = phys_initrd_start & PAGE_MASK;
>>>>> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>>>>>
>>>>>                 /*
>>>>>                  * We can only add back the initrd memory if we don't end up
>>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>>>>>          */
>>>>>         memblock_reserve(__pa_symbol(_text), _end - _text);
>>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>>> -       if (initrd_start) {
>>>>> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
>>>>> -
>>>>> +       if (phys_initrd_size) {
>>>>>                 /* the generic initrd code expects virtual addresses */
>>>>> -               initrd_start = __phys_to_virt(initrd_start);
>>>>> -               initrd_end = __phys_to_virt(initrd_end);
>>>>> +               initrd_start = __phys_to_virt(phys_initrd_start);
>>>>> +               initrd_end = initrd_start + phys_initrd_size;
>>>>> +               initrd_below_start_ok = 0;
>>>>
>>>> Where is this assignment coming from?
>>>
>>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though
>>> after patch #5 this is not necessary any more.
>>
>> Yes, but why? The original arm64 version of
>> __early_init_dt_declare_initrd() does not set it but now you set to 1
>> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it
>> back to 0 here.
>
> Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not
> be taking that branch on an ARM64 kernel.
>

Right. So now that we are not setting it to 1 on arm64, there is no
longer a reason to set it to 0 again, no?

> If you are saying the assignment is not necessary anymore after patch #5
> , that is true, though this can only be done a part of part #5, not as
> part of patch #4 in order not to break initrd functionality in-between
> patches.
>
>>
>> Or am I missing something?
>>
>
> Not sure, I could be too, it's Monday after all :)

Yeah :-)

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Florian Fainelli]

On 11/5/18 1:00 PM, Ard Biesheuvel wrote:
> On 5 November 2018 at 21:51, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 11/5/18 12:44 PM, Ard Biesheuvel wrote:
>>> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote:
>>>>> Hi Florian,
>>>>>
>>>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>> ARM64 is the only architecture that re-defines
>>>>>> __early_init_dt_declare_initrd() in order for that function to populate
>>>>>> initrd_start/initrd_end with physical addresses instead of virtual
>>>>>> addresses. Instead of having an override we can leverage
>>>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
>>>>>> populate those variables for us.
>>>>>>
>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>> ---
>>>>>>  arch/arm64/mm/init.c | 19 +++++++++----------
>>>>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>>>>>> index 3cf87341859f..00ef2166bb73 100644
>>>>>> --- a/arch/arm64/mm/init.c
>>>>>> +++ b/arch/arm64/mm/init.c
>>>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>>>>>>         if (*endp == ',') {
>>>>>>                 size = memparse(endp + 1, NULL);
>>>>>>
>>>>>> -               initrd_start = start;
>>>>>> -               initrd_end = start + size;
>>>>>> +               phys_initrd_start = start;
>>>>>> +               phys_initrd_size = size;
>>>>>>         }
>>>>>>         return 0;
>>>>>>  }
>>>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>>>>>>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>>>>>>         }
>>>>>>
>>>>>> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
>>>>>> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>>>>>>                 /*
>>>>>>                  * Add back the memory we just removed if it results in the
>>>>>>                  * initrd to become inaccessible via the linear mapping.
>>>>>>                  * Otherwise, this is a no-op
>>>>>>                  */
>>>>>> -               u64 base = initrd_start & PAGE_MASK;
>>>>>> -               u64 size = PAGE_ALIGN(initrd_end) - base;
>>>>>> +               u64 base = phys_initrd_start & PAGE_MASK;
>>>>>> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>>>>>>
>>>>>>                 /*
>>>>>>                  * We can only add back the initrd memory if we don't end up
>>>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>>>>>>          */
>>>>>>         memblock_reserve(__pa_symbol(_text), _end - _text);
>>>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>>>> -       if (initrd_start) {
>>>>>> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
>>>>>> -
>>>>>> +       if (phys_initrd_size) {
>>>>>>                 /* the generic initrd code expects virtual addresses */
>>>>>> -               initrd_start = __phys_to_virt(initrd_start);
>>>>>> -               initrd_end = __phys_to_virt(initrd_end);
>>>>>> +               initrd_start = __phys_to_virt(phys_initrd_start);
>>>>>> +               initrd_end = initrd_start + phys_initrd_size;
>>>>>> +               initrd_below_start_ok = 0;
>>>>>
>>>>> Where is this assignment coming from?
>>>>
>>>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though
>>>> after patch #5 this is not necessary any more.
>>>
>>> Yes, but why? The original arm64 version of
>>> __early_init_dt_declare_initrd() does not set it but now you set to 1
>>> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it
>>> back to 0 here.
>>
>> Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not
>> be taking that branch on an ARM64 kernel.
>>
> 
> Right. So now that we are not setting it to 1 on arm64, there is no
> longer a reason to set it to 0 again, no?

Correct, and in fact, this is not a problem either at patch #4 (which
has the custom __early_init_dt_declare_initrd()) or #5 (which removes
it), any other feedback you would like me to address before addressing
Rob's suggestion?

> 
>> If you are saying the assignment is not necessary anymore after patch #5
>> , that is true, though this can only be done a part of part #5, not as
>> part of patch #4 in order not to break initrd functionality in-between
>> patches.
>>
>>>
>>> Or am I missing something?
>>>
>>
>> Not sure, I could be too, it's Monday after all :)
> 
> Yeah :-)
> 


-- 
Florian

Re: [PATCH v3 4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

[Ard Biesheuvel]

On 5 November 2018 at 22:05, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/5/18 1:00 PM, Ard Biesheuvel wrote:
>> On 5 November 2018 at 21:51, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 11/5/18 12:44 PM, Ard Biesheuvel wrote:
>>>> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote:
>>>>>> Hi Florian,
>>>>>>
>>>>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>> ARM64 is the only architecture that re-defines
>>>>>>> __early_init_dt_declare_initrd() in order for that function to populate
>>>>>>> initrd_start/initrd_end with physical addresses instead of virtual
>>>>>>> addresses. Instead of having an override we can leverage
>>>>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to
>>>>>>> populate those variables for us.
>>>>>>>
>>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>>> ---
>>>>>>>  arch/arm64/mm/init.c | 19 +++++++++----------
>>>>>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>>>>>>> index 3cf87341859f..00ef2166bb73 100644
>>>>>>> --- a/arch/arm64/mm/init.c
>>>>>>> +++ b/arch/arm64/mm/init.c
>>>>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p)
>>>>>>>         if (*endp == ',') {
>>>>>>>                 size = memparse(endp + 1, NULL);
>>>>>>>
>>>>>>> -               initrd_start = start;
>>>>>>> -               initrd_end = start + size;
>>>>>>> +               phys_initrd_start = start;
>>>>>>> +               phys_initrd_size = size;
>>>>>>>         }
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void)
>>>>>>>                 memblock_add(__pa_symbol(_text), (u64)(_end - _text));
>>>>>>>         }
>>>>>>>
>>>>>>> -       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) {
>>>>>>> +       if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
>>>>>>>                 /*
>>>>>>>                  * Add back the memory we just removed if it results in the
>>>>>>>                  * initrd to become inaccessible via the linear mapping.
>>>>>>>                  * Otherwise, this is a no-op
>>>>>>>                  */
>>>>>>> -               u64 base = initrd_start & PAGE_MASK;
>>>>>>> -               u64 size = PAGE_ALIGN(initrd_end) - base;
>>>>>>> +               u64 base = phys_initrd_start & PAGE_MASK;
>>>>>>> +               u64 size = PAGE_ALIGN(phys_initrd_size);
>>>>>>>
>>>>>>>                 /*
>>>>>>>                  * We can only add back the initrd memory if we don't end up
>>>>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void)
>>>>>>>          */
>>>>>>>         memblock_reserve(__pa_symbol(_text), _end - _text);
>>>>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>>>>> -       if (initrd_start) {
>>>>>>> -               memblock_reserve(initrd_start, initrd_end - initrd_start);
>>>>>>> -
>>>>>>> +       if (phys_initrd_size) {
>>>>>>>                 /* the generic initrd code expects virtual addresses */
>>>>>>> -               initrd_start = __phys_to_virt(initrd_start);
>>>>>>> -               initrd_end = __phys_to_virt(initrd_end);
>>>>>>> +               initrd_start = __phys_to_virt(phys_initrd_start);
>>>>>>> +               initrd_end = initrd_start + phys_initrd_size;
>>>>>>> +               initrd_below_start_ok = 0;
>>>>>>
>>>>>> Where is this assignment coming from?
>>>>>
>>>>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though
>>>>> after patch #5 this is not necessary any more.
>>>>
>>>> Yes, but why? The original arm64 version of
>>>> __early_init_dt_declare_initrd() does not set it but now you set to 1
>>>> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it
>>>> back to 0 here.
>>>
>>> Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not
>>> be taking that branch on an ARM64 kernel.
>>>
>>
>> Right. So now that we are not setting it to 1 on arm64, there is no
>> longer a reason to set it to 0 again, no?
>
> Correct, and in fact, this is not a problem either at patch #4 (which
> has the custom __early_init_dt_declare_initrd()) or #5 (which removes
> it), any other feedback you would like me to address before addressing
> Rob's suggestion?
>

No, I think this is ok. The conditional on arm64 in generic code is a
bit nasty, and it would be nicer generally if we only have to record a
single memory range, but if this fixes the issue you were addressing
originally, I'm fine with it.