* [PATCH 0/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() @ 2018-10-29 19:00 Florian Fainelli 2018-10-29 19:00 ` [PATCH 1/2 " Florian Fainelli 2018-10-29 19:00 ` [PATCH 2/2 v5] of/fdt: Remove definition check for __early_init_dt_declare_initrd() Florian Fainelli 0 siblings, 2 replies; 7+ messages in thread From: Florian Fainelli @ 2018-10-29 19:00 UTC (permalink / raw) To: linux-kernel Cc: Florian Fainelli, Catalin Marinas, Will Deacon, Rob Herring, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, Andrey Ryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, Greg Hackmann, Kristina Martsenko, CHANDAN VN, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, rppt, linux Hi all, I numbered this v5 because this is still revolving around the same initial desire to cut the build time of an ARM64 kernel when toggling CONFIG_BLK_DEV_INITRD. This 4th version is basically the 3rd possible way to just get rid of __early_init_dt_declare_initrd() for ARM64. I previously mentioned that I was working on making phys_initrd_start/phys_initrd_size generic across architectures that make use of it, which would cover ARM (32-bit), unicore32 and possibly arm64, but upon second glance, this does not necessarily help, or rather this patch series actually helps make a smoother conversion in the future. Previous discussions/submissions list here: v3: https://www.spinics.net/lists/arm-kernel/msg683566.html v2: https://lkml.org/lkml/2018/10/25/4 Changes in v5: - incorporate Mike's version which properly takes care of command-line specified and FDT specified initrd location Changes in v4: - perform the physical to virtual initrd conversion straight within arm64_memblock_init() to get the correct memblock reservation to occur Changes in v3: - attempt to change drivers/of/fdt.c to absorb ARM64's specific behavior Changes in v2: - put an /* empty */ comment in the asm-generic/initrd.h file - trim down the CC list to maximize the chances of people receiving this Florian Fainelli (2): arm64: Get rid of __early_init_dt_declare_initrd() of/fdt: Remove definition check for __early_init_dt_declare_initrd() arch/arm64/include/asm/memory.h | 8 ------- arch/arm64/mm/init.c | 42 +++++++++++++++++++++------------ drivers/of/fdt.c | 2 -- 3 files changed, 27 insertions(+), 25 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() 2018-10-29 19:00 [PATCH 0/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() Florian Fainelli @ 2018-10-29 19:00 ` Florian Fainelli 2018-10-29 19:59 ` Rob Herring 2018-10-29 19:00 ` [PATCH 2/2 v5] of/fdt: Remove definition check for __early_init_dt_declare_initrd() Florian Fainelli 1 sibling, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2018-10-29 19:00 UTC (permalink / raw) To: linux-kernel Cc: Florian Fainelli, Mike Rapoport, Catalin Marinas, Will Deacon, Rob Herring, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, Andrey Ryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, Greg Hackmann, Kristina Martsenko, CHANDAN VN, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, linux 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, just get rid of that implementation and perform the virtual to physical conversion of these addresses in arm64_memblock_init() where relevant. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> --- arch/arm64/include/asm/memory.h | 8 ------- arch/arm64/mm/init.c | 42 +++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 23 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/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 3cf87341859f..292570b08f85 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -62,6 +62,8 @@ s64 memstart_addr __ro_after_init = -1; phys_addr_t arm64_dma_phys_limit __ro_after_init; +static phys_addr_t phys_initrd_start, phys_initrd_end; + #ifdef CONFIG_BLK_DEV_INITRD static int __init early_initrd(char *p) { @@ -72,8 +74,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_end = start + size; } return 0; } @@ -364,6 +366,7 @@ static void __init fdt_enforce_memory_region(void) void __init arm64_memblock_init(void) { const s64 linear_region_size = -(s64)PAGE_OFFSET; + u64 __maybe_unused base, size; /* Handle linux,usable-memory-range property */ fdt_enforce_memory_region(); @@ -408,14 +411,25 @@ 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) && + (initrd_start || phys_initrd_start)) { /* * 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; + if (phys_initrd_start) { + /* Command line specified the initrd location */ + initrd_start = __phys_to_virt(phys_initrd_start); + initrd_end = __phys_to_virt(phys_initrd_end); + } else if (initrd_start) { + /* FDT specified the initrd location */ + phys_initrd_start = __pa(initrd_start); + phys_initrd_end = __pa(initrd_end); + } + + base = phys_initrd_start & PAGE_MASK; + size = PAGE_ALIGN(phys_initrd_end - phys_initrd_start); /* * We can only add back the initrd memory if we don't end up @@ -434,6 +448,13 @@ void __init arm64_memblock_init(void) memblock_remove(base, size); /* clear MEMBLOCK_ flags */ memblock_add(base, size); memblock_reserve(base, size); + + /* + * initrd_below_start_ok can be changed by + * __early_init_dt_declare_initrd(), set it back to what + * we want here. + */ + initrd_below_start_ok = 0; } } @@ -455,19 +476,10 @@ void __init arm64_memblock_init(void) } /* - * Register the kernel text, kernel data, initrd, and initial + * Register the kernel text, kernel data and initial * pagetables with memblock. */ memblock_reserve(__pa_symbol(_text), _end - _text); -#ifdef CONFIG_BLK_DEV_INITRD - 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.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() 2018-10-29 19:00 ` [PATCH 1/2 " Florian Fainelli @ 2018-10-29 19:59 ` Rob Herring 2018-10-29 21:52 ` Florian Fainelli 2018-10-29 21:58 ` Ard Biesheuvel 0 siblings, 2 replies; 7+ messages in thread From: Rob Herring @ 2018-10-29 19:59 UTC (permalink / raw) To: Florian Fainelli, Ard Biesheuvel Cc: linux-kernel, rppt, Catalin Marinas, Will Deacon, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, aryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, ghackmann, Kristina Martsenko, chandan.vn, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, devicetree, Russell King +Ard who last touched this. On Mon, Oct 29, 2018 at 2:23 PM 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, just get rid of that > implementation and perform the virtual to physical conversion of these > addresses in arm64_memblock_init() where relevant. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > --- > arch/arm64/include/asm/memory.h | 8 ------- > arch/arm64/mm/init.c | 42 +++++++++++++++++++++------------ > 2 files changed, 27 insertions(+), 23 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/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 3cf87341859f..292570b08f85 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -62,6 +62,8 @@ > s64 memstart_addr __ro_after_init = -1; > phys_addr_t arm64_dma_phys_limit __ro_after_init; > > +static phys_addr_t phys_initrd_start, phys_initrd_end; > + > #ifdef CONFIG_BLK_DEV_INITRD > static int __init early_initrd(char *p) > { > @@ -72,8 +74,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_end = start + size; > } > return 0; > } > @@ -364,6 +366,7 @@ static void __init fdt_enforce_memory_region(void) > void __init arm64_memblock_init(void) > { > const s64 linear_region_size = -(s64)PAGE_OFFSET; > + u64 __maybe_unused base, size; > > /* Handle linux,usable-memory-range property */ > fdt_enforce_memory_region(); > @@ -408,14 +411,25 @@ 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) && > + (initrd_start || phys_initrd_start)) { I've tried to explain already that this is broken. The problem is __early_init_dt_declare_initrd using __va() which happens before this function is called. __va() uses PHYS_OFFSET which in turn is defined as memstart_addr. However, memstart_addr may be changed just above this hunk, so the earlier conversion to a VA may not be valid at this point. This is explained if you read Ard's commit that added all this mess. You could fix this by converting back to a PA before adjusting memstart_addr, but that's 2 wrongs making a right and fragile. The better solution is the other proposal making the DT code set phys_initrd_* (whatever the ARM code calls them). > /* > * 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; > + if (phys_initrd_start) { > + /* Command line specified the initrd location */ > + initrd_start = __phys_to_virt(phys_initrd_start); > + initrd_end = __phys_to_virt(phys_initrd_end); > + } else if (initrd_start) { > + /* FDT specified the initrd location */ > + phys_initrd_start = __pa(initrd_start); > + phys_initrd_end = __pa(initrd_end); Kind of inconsistent to mix __phys_to_virt and __pa flavors. Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() 2018-10-29 19:59 ` Rob Herring @ 2018-10-29 21:52 ` Florian Fainelli 2018-10-29 21:58 ` Ard Biesheuvel 1 sibling, 0 replies; 7+ messages in thread From: Florian Fainelli @ 2018-10-29 21:52 UTC (permalink / raw) To: Rob Herring, Ard Biesheuvel Cc: linux-kernel, rppt, Catalin Marinas, Will Deacon, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, aryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, ghackmann, Kristina Martsenko, chandan.vn, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, devicetree, Russell King On 10/29/18 12:59 PM, Rob Herring wrote: > +Ard who last touched this. > > On Mon, Oct 29, 2018 at 2:23 PM 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, just get rid of that >> implementation and perform the virtual to physical conversion of these >> addresses in arm64_memblock_init() where relevant. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> >> --- >> arch/arm64/include/asm/memory.h | 8 ------- >> arch/arm64/mm/init.c | 42 +++++++++++++++++++++------------ >> 2 files changed, 27 insertions(+), 23 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/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 3cf87341859f..292570b08f85 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -62,6 +62,8 @@ >> s64 memstart_addr __ro_after_init = -1; >> phys_addr_t arm64_dma_phys_limit __ro_after_init; >> >> +static phys_addr_t phys_initrd_start, phys_initrd_end; >> + >> #ifdef CONFIG_BLK_DEV_INITRD >> static int __init early_initrd(char *p) >> { >> @@ -72,8 +74,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_end = start + size; >> } >> return 0; >> } >> @@ -364,6 +366,7 @@ static void __init fdt_enforce_memory_region(void) >> void __init arm64_memblock_init(void) >> { >> const s64 linear_region_size = -(s64)PAGE_OFFSET; >> + u64 __maybe_unused base, size; >> >> /* Handle linux,usable-memory-range property */ >> fdt_enforce_memory_region(); >> @@ -408,14 +411,25 @@ 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) && >> + (initrd_start || phys_initrd_start)) { > > I've tried to explain already that this is broken. The problem is > __early_init_dt_declare_initrd using __va() which happens before this > function is called. __va() uses PHYS_OFFSET which in turn is defined > as memstart_addr. However, memstart_addr may be changed just above > this hunk, so the earlier conversion to a VA may not be valid at this > point. This is explained if you read Ard's commit that added all this > mess. Thanks for explaining this again, looks like I had overlooked that explanation in the "other branch" of the thread last time I read it. > > You could fix this by converting back to a PA before adjusting > memstart_addr, but that's 2 wrongs making a right and fragile. The > better solution is the other proposal making the DT code set > phys_initrd_* (whatever the ARM code calls them). OK, that sounds reasonable, will cook something doing that. Thanks! > >> /* >> * 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; >> + if (phys_initrd_start) { >> + /* Command line specified the initrd location */ >> + initrd_start = __phys_to_virt(phys_initrd_start); >> + initrd_end = __phys_to_virt(phys_initrd_end); >> + } else if (initrd_start) { >> + /* FDT specified the initrd location */ >> + phys_initrd_start = __pa(initrd_start); >> + phys_initrd_end = __pa(initrd_end); > > Kind of inconsistent to mix __phys_to_virt and __pa flavors. > > Rob > -- Florian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() 2018-10-29 19:59 ` Rob Herring 2018-10-29 21:52 ` Florian Fainelli @ 2018-10-29 21:58 ` Ard Biesheuvel 2018-10-29 23:24 ` Rob Herring 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2018-10-29 21:58 UTC (permalink / raw) To: Rob Herring Cc: Florian Fainelli, linux-kernel, rppt, Catalin Marinas, Will Deacon, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, Andrey Ryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, ghackmann, Kristina Martsenko, chandan.vn, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Devicetree List, Russell King On 29 October 2018 at 16:59, Rob Herring <robh+dt@kernel.org> wrote: > +Ard who last touched this. > > On Mon, Oct 29, 2018 at 2:23 PM 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, just get rid of that >> implementation and perform the virtual to physical conversion of these >> addresses in arm64_memblock_init() where relevant. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> >> --- >> arch/arm64/include/asm/memory.h | 8 ------- >> arch/arm64/mm/init.c | 42 +++++++++++++++++++++------------ >> 2 files changed, 27 insertions(+), 23 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/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 3cf87341859f..292570b08f85 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -62,6 +62,8 @@ >> s64 memstart_addr __ro_after_init = -1; >> phys_addr_t arm64_dma_phys_limit __ro_after_init; >> >> +static phys_addr_t phys_initrd_start, phys_initrd_end; >> + >> #ifdef CONFIG_BLK_DEV_INITRD >> static int __init early_initrd(char *p) >> { >> @@ -72,8 +74,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_end = start + size; >> } >> return 0; >> } >> @@ -364,6 +366,7 @@ static void __init fdt_enforce_memory_region(void) >> void __init arm64_memblock_init(void) >> { >> const s64 linear_region_size = -(s64)PAGE_OFFSET; >> + u64 __maybe_unused base, size; >> >> /* Handle linux,usable-memory-range property */ >> fdt_enforce_memory_region(); >> @@ -408,14 +411,25 @@ 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) && >> + (initrd_start || phys_initrd_start)) { > > I've tried to explain already that this is broken. The problem is > __early_init_dt_declare_initrd using __va() which happens before this > function is called. __va() uses PHYS_OFFSET which in turn is defined > as memstart_addr. However, memstart_addr may be changed just above > this hunk, so the earlier conversion to a VA may not be valid at this > point. This is explained if you read Ard's commit that added all this > mess. > > You could fix this by converting back to a PA before adjusting > memstart_addr, but that's 2 wrongs making a right and fragile. The > better solution is the other proposal making the DT code set > phys_initrd_* (whatever the ARM code calls them). > On arm64, we have #define PHYS_OFFSET \ ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; }) and s64 memstart_addr __ro_after_init = -1; IOW, any attempt to perform PA to VA translations before memstart_addr is assigned will BUG() if CONFIG_DEBUG_VM=y, so please enable that when playing with this code. The reason we have this code is because the start of the linear region might not coincide with memblock_start_of_DRAM(), which could happen, e.g., when running a 39-bit VA kernel on a system with a very sparse memory map (which is unfortunately what some silicon vendors think is what ARM recommends) and the kernel loaded near the top of that memory. The ability to load the kernel anywhere in physical memory was introduced to accommodate physical KASLR. Ideally, we'd fix this by only recording physical addresses for the initrd in generic code, and deferring the translation until the point where we actually do the access. >> /* >> * 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; >> + if (phys_initrd_start) { >> + /* Command line specified the initrd location */ >> + initrd_start = __phys_to_virt(phys_initrd_start); >> + initrd_end = __phys_to_virt(phys_initrd_end); >> + } else if (initrd_start) { >> + /* FDT specified the initrd location */ >> + phys_initrd_start = __pa(initrd_start); >> + phys_initrd_end = __pa(initrd_end); > > Kind of inconsistent to mix __phys_to_virt and __pa flavors. > > Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() 2018-10-29 21:58 ` Ard Biesheuvel @ 2018-10-29 23:24 ` Rob Herring 0 siblings, 0 replies; 7+ messages in thread From: Rob Herring @ 2018-10-29 23:24 UTC (permalink / raw) To: Ard Biesheuvel Cc: Florian Fainelli, linux-kernel, rppt, Catalin Marinas, Will Deacon, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, aryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, ghackmann, Kristina Martsenko, chandan.vn, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, devicetree, Russell King On Mon, Oct 29, 2018 at 4:58 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 29 October 2018 at 16:59, Rob Herring <robh+dt@kernel.org> wrote: > > +Ard who last touched this. > > > > On Mon, Oct 29, 2018 at 2:23 PM 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, just get rid of that > >> implementation and perform the virtual to physical conversion of these > >> addresses in arm64_memblock_init() where relevant. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > >> --- > >> arch/arm64/include/asm/memory.h | 8 ------- > >> arch/arm64/mm/init.c | 42 +++++++++++++++++++++------------ > >> 2 files changed, 27 insertions(+), 23 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/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > >> index 3cf87341859f..292570b08f85 100644 > >> --- a/arch/arm64/mm/init.c > >> +++ b/arch/arm64/mm/init.c > >> @@ -62,6 +62,8 @@ > >> s64 memstart_addr __ro_after_init = -1; > >> phys_addr_t arm64_dma_phys_limit __ro_after_init; > >> > >> +static phys_addr_t phys_initrd_start, phys_initrd_end; > >> + > >> #ifdef CONFIG_BLK_DEV_INITRD > >> static int __init early_initrd(char *p) > >> { > >> @@ -72,8 +74,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_end = start + size; > >> } > >> return 0; > >> } > >> @@ -364,6 +366,7 @@ static void __init fdt_enforce_memory_region(void) > >> void __init arm64_memblock_init(void) > >> { > >> const s64 linear_region_size = -(s64)PAGE_OFFSET; > >> + u64 __maybe_unused base, size; > >> > >> /* Handle linux,usable-memory-range property */ > >> fdt_enforce_memory_region(); > >> @@ -408,14 +411,25 @@ 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) && > >> + (initrd_start || phys_initrd_start)) { > > > > I've tried to explain already that this is broken. The problem is > > __early_init_dt_declare_initrd using __va() which happens before this > > function is called. __va() uses PHYS_OFFSET which in turn is defined > > as memstart_addr. However, memstart_addr may be changed just above > > this hunk, so the earlier conversion to a VA may not be valid at this > > point. This is explained if you read Ard's commit that added all this > > mess. > > > > You could fix this by converting back to a PA before adjusting > > memstart_addr, but that's 2 wrongs making a right and fragile. The > > better solution is the other proposal making the DT code set > > phys_initrd_* (whatever the ARM code calls them). > > > > On arm64, we have > > #define PHYS_OFFSET \ > ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; }) > > and > > s64 memstart_addr __ro_after_init = -1; > > IOW, any attempt to perform PA to VA translations before memstart_addr > is assigned will BUG() if CONFIG_DEBUG_VM=y, so please enable that > when playing with this code. Which will result in a crashed kernel with no console output unless you have earlycon enabled (or maybe EFI console will be up?). A WARN would be better. > The reason we have this code is because the start of the linear region > might not coincide with memblock_start_of_DRAM(), which could happen, > e.g., when running a 39-bit VA kernel on a system with a very sparse > memory map (which is unfortunately what some silicon vendors think is > what ARM recommends) and the kernel loaded near the top of that > memory. The ability to load the kernel anywhere in physical memory was > introduced to accommodate physical KASLR. > > Ideally, we'd fix this by only recording physical addresses for the > initrd in generic code, and deferring the translation until the point > where we actually do the access. The plan had been for the core code to set both the phys and virt addresses and let arm64 calculate the VA again. I guess we'll have to condition getting the VA on !ARM64 at least until we convert other platforms to use physaddr. Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2 v5] of/fdt: Remove definition check for __early_init_dt_declare_initrd() 2018-10-29 19:00 [PATCH 0/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() Florian Fainelli 2018-10-29 19:00 ` [PATCH 1/2 " Florian Fainelli @ 2018-10-29 19:00 ` Florian Fainelli 1 sibling, 0 replies; 7+ messages in thread From: Florian Fainelli @ 2018-10-29 19:00 UTC (permalink / raw) To: linux-kernel Cc: Florian Fainelli, Catalin Marinas, Will Deacon, Rob Herring, Frank Rowand, Andrew Morton, Marc Zyngier, Russell King, Andrey Ryabinin, Andrey Konovalov, Masahiro Yamada, Robin Murphy, Laura Abbott, Stefan Agner, Johannes Weiner, Greg Hackmann, Kristina Martsenko, CHANDAN VN, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, rppt, linux With the one and only architecture (ARM64) no longer defining a custom __early_init_dt_declare_initrd() function, just get rid of the check for that function being already defined. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/of/fdt.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 76c83c1ffeda..0f6430265e4d 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -892,7 +892,6 @@ 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) { @@ -900,7 +899,6 @@ static void __early_init_dt_declare_initrd(unsigned long 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-29 23:25 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-29 19:00 [PATCH 0/2 v5] arm64: Get rid of __early_init_dt_declare_initrd() Florian Fainelli 2018-10-29 19:00 ` [PATCH 1/2 " Florian Fainelli 2018-10-29 19:59 ` Rob Herring 2018-10-29 21:52 ` Florian Fainelli 2018-10-29 21:58 ` Ard Biesheuvel 2018-10-29 23:24 ` Rob Herring 2018-10-29 19:00 ` [PATCH 2/2 v5] of/fdt: Remove definition check for __early_init_dt_declare_initrd() Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).