Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Marek Szyprowski]

Hi,

On 07.10.2020 10:39, Ard Biesheuvel wrote:
> On ARM, setting up the linear region is tricky, given the constraints
> around placement and alignment of the memblocks, and how the kernel
> itself as well as the DT are placed in physical memory.
>
> Let's simplify matters a bit, by moving the device tree mapping to the
> top of the address space, right between the end of the vmalloc region
> and the start of the the fixmap region, and create a read-only mapping
> for it that is independent of the size of the linear region, and how it
> is organized.
>
> Since this region was formerly used as a guard region, which will now be
> populated fully on LPAE builds by this read-only mapping (which will
> still be able to function as a guard region for stray writes), bump the
> start of the [underutilized] fixmap region by 512 KB as well, to ensure
> that there is always a proper guard region here. Doing so still leaves
> ample room for the fixmap space, even with NR_CPUS set to its maximum
> value of 32.
>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM: 
9012/1: move device tree mapping out of linear region"). Sadly it broke 
booting  almost all Samsung Exynos-based boards. The only one which 
booted, used an appended device tree. I can provide more information if 
needed, just let me know what to check. "Starting kernel ..." is the 
last message I see here. No output from earlycon.

> ---
>   Documentation/arm/memory.rst  |  7 ++++++-
>   arch/arm/include/asm/fixmap.h |  2 +-
>   arch/arm/include/asm/memory.h |  5 +++++
>   arch/arm/kernel/head.S        |  5 ++---
>   arch/arm/kernel/setup.c       | 11 ++++++++---
>   arch/arm/mm/init.c            |  1 -
>   arch/arm/mm/mmu.c             | 20 ++++++++++++++------
>   arch/arm/mm/pv-fixup-asm.S    |  4 ++--
>   8 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/arm/memory.rst b/Documentation/arm/memory.rst
> index 0521b4ce5c96..34bb23c44a71 100644
> --- a/Documentation/arm/memory.rst
> +++ b/Documentation/arm/memory.rst
> @@ -45,9 +45,14 @@ fffe8000	fffeffff	DTCM mapping area for platforms with
>   fffe0000	fffe7fff	ITCM mapping area for platforms with
>   				ITCM mounted inside the CPU.
>   
> -ffc00000	ffefffff	Fixmap mapping region.  Addresses provided
> +ffc80000	ffefffff	Fixmap mapping region.  Addresses provided
>   				by fix_to_virt() will be located here.
>   
> +ffc00000	ffc7ffff	Guard region
> +
> +ff800000	ffbfffff	Permanent, fixed read-only mapping of the
> +				firmware provided DT blob
> +
>   fee00000	feffffff	Mapping of PCI I/O space. This is a static
>   				mapping within the vmalloc space.
>   
> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> index fc56fc3e1931..9575b404019c 100644
> --- a/arch/arm/include/asm/fixmap.h
> +++ b/arch/arm/include/asm/fixmap.h
> @@ -2,7 +2,7 @@
>   #ifndef _ASM_FIXMAP_H
>   #define _ASM_FIXMAP_H
>   
> -#define FIXADDR_START		0xffc00000UL
> +#define FIXADDR_START		0xffc80000UL
>   #define FIXADDR_END		0xfff00000UL
>   #define FIXADDR_TOP		(FIXADDR_END - PAGE_SIZE)
>   
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 99035b5891ef..ad9dcf4f751d 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -67,6 +67,10 @@
>    */
>   #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
>   
> +#define FDT_FIXED_BASE		UL(0xff800000)
> +#define FDT_FIXED_SIZE		(2 * PMD_SIZE)
> +#define FDT_VIRT_ADDR(physaddr)	((void *)(FDT_FIXED_BASE | (physaddr) % PMD_SIZE))
> +
>   #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
>   /*
>    * Allow 16MB-aligned ioremap pages
> @@ -107,6 +111,7 @@ extern unsigned long vectors_base;
>   #define MODULES_VADDR		PAGE_OFFSET
>   
>   #define XIP_VIRT_ADDR(physaddr)  (physaddr)
> +#define FDT_VIRT_ADDR(physaddr)  (physaddr)
>   
>   #endif /* !CONFIG_MMU */
>   
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index f8904227e7fd..9b18d8c66129 100644
> --- a/arch/arm/kernel/head.S
> +++ b/arch/arm/kernel/head.S
> @@ -275,9 +275,8 @@ __create_page_tables:
>   	 */
>   	mov	r0, r2, lsr #SECTION_SHIFT
>   	movs	r0, r0, lsl #SECTION_SHIFT
> -	subne	r3, r0, r8
> -	addne	r3, r3, #PAGE_OFFSET
> -	addne	r3, r4, r3, lsr #(SECTION_SHIFT - PMD_ORDER)
> +	ldrne	r3, =FDT_FIXED_BASE >> (SECTION_SHIFT - PMD_ORDER)
> +	addne	r3, r3, r4
>   	orrne	r6, r7, r0
>   	strne	r6, [r3], #1 << PMD_ORDER
>   	addne	r6, r6, #1 << SECTION_SHIFT
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 58e56e23a19a..c08574cad748 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -18,6 +18,7 @@
>   #include <linux/of_platform.h>
>   #include <linux/init.h>
>   #include <linux/kexec.h>
> +#include <linux/libfdt.h>
>   #include <linux/of_fdt.h>
>   #include <linux/cpu.h>
>   #include <linux/interrupt.h>
> @@ -90,7 +91,6 @@ unsigned int cacheid __read_mostly;
>   EXPORT_SYMBOL(cacheid);
>   
>   unsigned int __atags_pointer __initdata;
> -void *atags_vaddr __initdata;
>   
>   unsigned int system_rev;
>   EXPORT_SYMBOL(system_rev);
> @@ -1078,13 +1078,18 @@ void __init hyp_mode_check(void)
>   void __init setup_arch(char **cmdline_p)
>   {
>   	const struct machine_desc *mdesc = NULL;
> +	void *atags_vaddr;
>   
>   	if (__atags_pointer)
> -		atags_vaddr = phys_to_virt(__atags_pointer);
> +		atags_vaddr = FDT_VIRT_ADDR(__atags_pointer);
>   
>   	setup_processor();
> -	if (atags_vaddr)
> +	if (atags_vaddr) {
>   		mdesc = setup_machine_fdt(atags_vaddr);
> +		if (mdesc)
> +			memblock_reserve(__atags_pointer,
> +					 fdt_totalsize(atags_vaddr));
> +	}
>   	if (!mdesc)
>   		mdesc = setup_machine_tags(atags_vaddr, __machine_arch_type);
>   	if (!mdesc) {
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 000c1b48e973..652a87bd065c 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -223,7 +223,6 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
>   	if (mdesc->reserve)
>   		mdesc->reserve();
>   
> -	early_init_fdt_reserve_self();
>   	early_init_fdt_scan_reserved_mem();
>   
>   	/* reserve memory for DMA contiguous allocations */
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 0665a0dbd040..a7231d151c63 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -40,6 +40,8 @@
>   #include "mm.h"
>   #include "tcm.h"
>   
> +extern unsigned long __atags_pointer;
> +
>   /*
>    * empty_zero_page is a special page that is used for
>    * zero-initialized data and COW.
> @@ -947,7 +949,7 @@ static void __init create_mapping(struct map_desc *md)
>   		return;
>   	}
>   
> -	if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
> +	if (md->type == MT_DEVICE &&
>   	    md->virtual >= PAGE_OFFSET && md->virtual < FIXADDR_START &&
>   	    (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
>   		pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
> @@ -1343,6 +1345,15 @@ static void __init devicemaps_init(const struct machine_desc *mdesc)
>   	for (addr = VMALLOC_START; addr < (FIXADDR_TOP & PMD_MASK); addr += PMD_SIZE)
>   		pmd_clear(pmd_off_k(addr));
>   
> +	if (__atags_pointer) {
> +		/* create a read-only mapping of the device tree */
> +		map.pfn = __phys_to_pfn(__atags_pointer & SECTION_MASK);
> +		map.virtual = FDT_FIXED_BASE;
> +		map.length = FDT_FIXED_SIZE;
> +		map.type = MT_ROM;
> +		create_mapping(&map);
> +	}
> +
>   	/*
>   	 * Map the kernel if it is XIP.
>   	 * It is always first in the modulearea.
> @@ -1503,8 +1514,7 @@ static void __init map_lowmem(void)
>   }
>   
>   #ifdef CONFIG_ARM_PV_FIXUP
> -extern void *atags_vaddr;
> -typedef void pgtables_remap(long long offset, unsigned long pgd, void *bdata);
> +typedef void pgtables_remap(long long offset, unsigned long pgd);
>   pgtables_remap lpae_pgtables_remap_asm;
>   
>   /*
> @@ -1517,7 +1527,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
>   	unsigned long pa_pgd;
>   	unsigned int cr, ttbcr;
>   	long long offset;
> -	void *boot_data;
>   
>   	if (!mdesc->pv_fixup)
>   		return;
> @@ -1534,7 +1543,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
>   	 */
>   	lpae_pgtables_remap = (pgtables_remap *)(unsigned long)__pa(lpae_pgtables_remap_asm);
>   	pa_pgd = __pa(swapper_pg_dir);
> -	boot_data = atags_vaddr;
>   	barrier();
>   
>   	pr_info("Switching physical address space to 0x%08llx\n",
> @@ -1570,7 +1578,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
>   	 * needs to be assembly.  It's fairly simple, as we're using the
>   	 * temporary tables setup by the initial assembly code.
>   	 */
> -	lpae_pgtables_remap(offset, pa_pgd, boot_data);
> +	lpae_pgtables_remap(offset, pa_pgd);
>   
>   	/* Re-enable the caches and cacheable TLB walks */
>   	asm volatile("mcr p15, 0, %0, c2, c0, 2" : : "r" (ttbcr));
> diff --git a/arch/arm/mm/pv-fixup-asm.S b/arch/arm/mm/pv-fixup-asm.S
> index 8eade0416739..5c5e1952000a 100644
> --- a/arch/arm/mm/pv-fixup-asm.S
> +++ b/arch/arm/mm/pv-fixup-asm.S
> @@ -39,8 +39,8 @@ ENTRY(lpae_pgtables_remap_asm)
>   
>   	/* Update level 2 entries for the boot data */
>   	add	r7, r2, #0x1000
> -	add	r7, r7, r3, lsr #SECTION_SHIFT - L2_ORDER
> -	bic	r7, r7, #(1 << L2_ORDER) - 1
> +	movw	r3, #FDT_FIXED_BASE >> (SECTION_SHIFT - L2_ORDER)
> +	add	r7, r7, r3
>   	ldrd	r4, r5, [r7]
>   	adds	r4, r4, r0
>   	adc	r5, r5, r1

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 10:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi,
>
> On 07.10.2020 10:39, Ard Biesheuvel wrote:
> > On ARM, setting up the linear region is tricky, given the constraints
> > around placement and alignment of the memblocks, and how the kernel
> > itself as well as the DT are placed in physical memory.
> >
> > Let's simplify matters a bit, by moving the device tree mapping to the
> > top of the address space, right between the end of the vmalloc region
> > and the start of the the fixmap region, and create a read-only mapping
> > for it that is independent of the size of the linear region, and how it
> > is organized.
> >
> > Since this region was formerly used as a guard region, which will now be
> > populated fully on LPAE builds by this read-only mapping (which will
> > still be able to function as a guard region for stray writes), bump the
> > start of the [underutilized] fixmap region by 512 KB as well, to ensure
> > that there is always a proper guard region here. Doing so still leaves
> > ample room for the fixmap space, even with NR_CPUS set to its maximum
> > value of 32.
> >
> > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> 9012/1: move device tree mapping out of linear region"). Sadly it broke
> booting  almost all Samsung Exynos-based boards. The only one which
> booted, used an appended device tree. I can provide more information if
> needed, just let me know what to check. "Starting kernel ..." is the
> last message I see here. No output from earlycon.
>

Thanks for the report. I will have a look later today.

Do these platforms happen to have any static device mappings that may
collide with this mapping of the FDT? Also, could this be related to
device drivers making changes in memory to the FDT image? Because the
permanent mapping of the FDT is read-only now.



> > ---
> >   Documentation/arm/memory.rst  |  7 ++++++-
> >   arch/arm/include/asm/fixmap.h |  2 +-
> >   arch/arm/include/asm/memory.h |  5 +++++
> >   arch/arm/kernel/head.S        |  5 ++---
> >   arch/arm/kernel/setup.c       | 11 ++++++++---
> >   arch/arm/mm/init.c            |  1 -
> >   arch/arm/mm/mmu.c             | 20 ++++++++++++++------
> >   arch/arm/mm/pv-fixup-asm.S    |  4 ++--
> >   8 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/arm/memory.rst b/Documentation/arm/memory.rst
> > index 0521b4ce5c96..34bb23c44a71 100644
> > --- a/Documentation/arm/memory.rst
> > +++ b/Documentation/arm/memory.rst
> > @@ -45,9 +45,14 @@ fffe8000   fffeffff        DTCM mapping area for platforms with
> >   fffe0000    fffe7fff        ITCM mapping area for platforms with
> >                               ITCM mounted inside the CPU.
> >
> > -ffc00000     ffefffff        Fixmap mapping region.  Addresses provided
> > +ffc80000     ffefffff        Fixmap mapping region.  Addresses provided
> >                               by fix_to_virt() will be located here.
> >
> > +ffc00000     ffc7ffff        Guard region
> > +
> > +ff800000     ffbfffff        Permanent, fixed read-only mapping of the
> > +                             firmware provided DT blob
> > +
> >   fee00000    feffffff        Mapping of PCI I/O space. This is a static
> >                               mapping within the vmalloc space.
> >
> > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> > index fc56fc3e1931..9575b404019c 100644
> > --- a/arch/arm/include/asm/fixmap.h
> > +++ b/arch/arm/include/asm/fixmap.h
> > @@ -2,7 +2,7 @@
> >   #ifndef _ASM_FIXMAP_H
> >   #define _ASM_FIXMAP_H
> >
> > -#define FIXADDR_START                0xffc00000UL
> > +#define FIXADDR_START                0xffc80000UL
> >   #define FIXADDR_END         0xfff00000UL
> >   #define FIXADDR_TOP         (FIXADDR_END - PAGE_SIZE)
> >
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index 99035b5891ef..ad9dcf4f751d 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -67,6 +67,10 @@
> >    */
> >   #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> >
> > +#define FDT_FIXED_BASE               UL(0xff800000)
> > +#define FDT_FIXED_SIZE               (2 * PMD_SIZE)
> > +#define FDT_VIRT_ADDR(physaddr)      ((void *)(FDT_FIXED_BASE | (physaddr) % PMD_SIZE))
> > +
> >   #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
> >   /*
> >    * Allow 16MB-aligned ioremap pages
> > @@ -107,6 +111,7 @@ extern unsigned long vectors_base;
> >   #define MODULES_VADDR               PAGE_OFFSET
> >
> >   #define XIP_VIRT_ADDR(physaddr)  (physaddr)
> > +#define FDT_VIRT_ADDR(physaddr)  (physaddr)
> >
> >   #endif /* !CONFIG_MMU */
> >
> > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> > index f8904227e7fd..9b18d8c66129 100644
> > --- a/arch/arm/kernel/head.S
> > +++ b/arch/arm/kernel/head.S
> > @@ -275,9 +275,8 @@ __create_page_tables:
> >        */
> >       mov     r0, r2, lsr #SECTION_SHIFT
> >       movs    r0, r0, lsl #SECTION_SHIFT
> > -     subne   r3, r0, r8
> > -     addne   r3, r3, #PAGE_OFFSET
> > -     addne   r3, r4, r3, lsr #(SECTION_SHIFT - PMD_ORDER)
> > +     ldrne   r3, =FDT_FIXED_BASE >> (SECTION_SHIFT - PMD_ORDER)
> > +     addne   r3, r3, r4
> >       orrne   r6, r7, r0
> >       strne   r6, [r3], #1 << PMD_ORDER
> >       addne   r6, r6, #1 << SECTION_SHIFT
> > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> > index 58e56e23a19a..c08574cad748 100644
> > --- a/arch/arm/kernel/setup.c
> > +++ b/arch/arm/kernel/setup.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/of_platform.h>
> >   #include <linux/init.h>
> >   #include <linux/kexec.h>
> > +#include <linux/libfdt.h>
> >   #include <linux/of_fdt.h>
> >   #include <linux/cpu.h>
> >   #include <linux/interrupt.h>
> > @@ -90,7 +91,6 @@ unsigned int cacheid __read_mostly;
> >   EXPORT_SYMBOL(cacheid);
> >
> >   unsigned int __atags_pointer __initdata;
> > -void *atags_vaddr __initdata;
> >
> >   unsigned int system_rev;
> >   EXPORT_SYMBOL(system_rev);
> > @@ -1078,13 +1078,18 @@ void __init hyp_mode_check(void)
> >   void __init setup_arch(char **cmdline_p)
> >   {
> >       const struct machine_desc *mdesc = NULL;
> > +     void *atags_vaddr;
> >
> >       if (__atags_pointer)
> > -             atags_vaddr = phys_to_virt(__atags_pointer);
> > +             atags_vaddr = FDT_VIRT_ADDR(__atags_pointer);
> >
> >       setup_processor();
> > -     if (atags_vaddr)
> > +     if (atags_vaddr) {
> >               mdesc = setup_machine_fdt(atags_vaddr);
> > +             if (mdesc)
> > +                     memblock_reserve(__atags_pointer,
> > +                                      fdt_totalsize(atags_vaddr));
> > +     }
> >       if (!mdesc)
> >               mdesc = setup_machine_tags(atags_vaddr, __machine_arch_type);
> >       if (!mdesc) {
> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 000c1b48e973..652a87bd065c 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -223,7 +223,6 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
> >       if (mdesc->reserve)
> >               mdesc->reserve();
> >
> > -     early_init_fdt_reserve_self();
> >       early_init_fdt_scan_reserved_mem();
> >
> >       /* reserve memory for DMA contiguous allocations */
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > index 0665a0dbd040..a7231d151c63 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -40,6 +40,8 @@
> >   #include "mm.h"
> >   #include "tcm.h"
> >
> > +extern unsigned long __atags_pointer;
> > +
> >   /*
> >    * empty_zero_page is a special page that is used for
> >    * zero-initialized data and COW.
> > @@ -947,7 +949,7 @@ static void __init create_mapping(struct map_desc *md)
> >               return;
> >       }
> >
> > -     if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
> > +     if (md->type == MT_DEVICE &&
> >           md->virtual >= PAGE_OFFSET && md->virtual < FIXADDR_START &&
> >           (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
> >               pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
> > @@ -1343,6 +1345,15 @@ static void __init devicemaps_init(const struct machine_desc *mdesc)
> >       for (addr = VMALLOC_START; addr < (FIXADDR_TOP & PMD_MASK); addr += PMD_SIZE)
> >               pmd_clear(pmd_off_k(addr));
> >
> > +     if (__atags_pointer) {
> > +             /* create a read-only mapping of the device tree */
> > +             map.pfn = __phys_to_pfn(__atags_pointer & SECTION_MASK);
> > +             map.virtual = FDT_FIXED_BASE;
> > +             map.length = FDT_FIXED_SIZE;
> > +             map.type = MT_ROM;
> > +             create_mapping(&map);
> > +     }
> > +
> >       /*
> >        * Map the kernel if it is XIP.
> >        * It is always first in the modulearea.
> > @@ -1503,8 +1514,7 @@ static void __init map_lowmem(void)
> >   }
> >
> >   #ifdef CONFIG_ARM_PV_FIXUP
> > -extern void *atags_vaddr;
> > -typedef void pgtables_remap(long long offset, unsigned long pgd, void *bdata);
> > +typedef void pgtables_remap(long long offset, unsigned long pgd);
> >   pgtables_remap lpae_pgtables_remap_asm;
> >
> >   /*
> > @@ -1517,7 +1527,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> >       unsigned long pa_pgd;
> >       unsigned int cr, ttbcr;
> >       long long offset;
> > -     void *boot_data;
> >
> >       if (!mdesc->pv_fixup)
> >               return;
> > @@ -1534,7 +1543,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> >        */
> >       lpae_pgtables_remap = (pgtables_remap *)(unsigned long)__pa(lpae_pgtables_remap_asm);
> >       pa_pgd = __pa(swapper_pg_dir);
> > -     boot_data = atags_vaddr;
> >       barrier();
> >
> >       pr_info("Switching physical address space to 0x%08llx\n",
> > @@ -1570,7 +1578,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> >        * needs to be assembly.  It's fairly simple, as we're using the
> >        * temporary tables setup by the initial assembly code.
> >        */
> > -     lpae_pgtables_remap(offset, pa_pgd, boot_data);
> > +     lpae_pgtables_remap(offset, pa_pgd);
> >
> >       /* Re-enable the caches and cacheable TLB walks */
> >       asm volatile("mcr p15, 0, %0, c2, c0, 2" : : "r" (ttbcr));
> > diff --git a/arch/arm/mm/pv-fixup-asm.S b/arch/arm/mm/pv-fixup-asm.S
> > index 8eade0416739..5c5e1952000a 100644
> > --- a/arch/arm/mm/pv-fixup-asm.S
> > +++ b/arch/arm/mm/pv-fixup-asm.S
> > @@ -39,8 +39,8 @@ ENTRY(lpae_pgtables_remap_asm)
> >
> >       /* Update level 2 entries for the boot data */
> >       add     r7, r2, #0x1000
> > -     add     r7, r7, r3, lsr #SECTION_SHIFT - L2_ORDER
> > -     bic     r7, r7, #(1 << L2_ORDER) - 1
> > +     movw    r3, #FDT_FIXED_BASE >> (SECTION_SHIFT - L2_ORDER)
> > +     add     r7, r7, r3
> >       ldrd    r4, r5, [r7]
> >       adds    r4, r4, r0
> >       adc     r5, r5, r1
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 10:22, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 28 Oct 2020 at 10:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >
> > Hi,
> >
> > On 07.10.2020 10:39, Ard Biesheuvel wrote:
> > > On ARM, setting up the linear region is tricky, given the constraints
> > > around placement and alignment of the memblocks, and how the kernel
> > > itself as well as the DT are placed in physical memory.
> > >
> > > Let's simplify matters a bit, by moving the device tree mapping to the
> > > top of the address space, right between the end of the vmalloc region
> > > and the start of the the fixmap region, and create a read-only mapping
> > > for it that is independent of the size of the linear region, and how it
> > > is organized.
> > >
> > > Since this region was formerly used as a guard region, which will now be
> > > populated fully on LPAE builds by this read-only mapping (which will
> > > still be able to function as a guard region for stray writes), bump the
> > > start of the [underutilized] fixmap region by 512 KB as well, to ensure
> > > that there is always a proper guard region here. Doing so still leaves
> > > ample room for the fixmap space, even with NR_CPUS set to its maximum
> > > value of 32.
> > >
> > > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> > 9012/1: move device tree mapping out of linear region"). Sadly it broke
> > booting  almost all Samsung Exynos-based boards. The only one which
> > booted, used an appended device tree. I can provide more information if
> > needed, just let me know what to check. "Starting kernel ..." is the
> > last message I see here. No output from earlycon.
> >
>
> Thanks for the report. I will have a look later today.
>
> Do these platforms happen to have any static device mappings that may
> collide with this mapping of the FDT? Also, could this be related to
> device drivers making changes in memory to the FDT image? Because the
> permanent mapping of the FDT is read-only now.
>

IOW, does using MT_MEMORY_RW instead of MT_ROM for the mapping make a
difference?

>
>
> > > ---
> > >   Documentation/arm/memory.rst  |  7 ++++++-
> > >   arch/arm/include/asm/fixmap.h |  2 +-
> > >   arch/arm/include/asm/memory.h |  5 +++++
> > >   arch/arm/kernel/head.S        |  5 ++---
> > >   arch/arm/kernel/setup.c       | 11 ++++++++---
> > >   arch/arm/mm/init.c            |  1 -
> > >   arch/arm/mm/mmu.c             | 20 ++++++++++++++------
> > >   arch/arm/mm/pv-fixup-asm.S    |  4 ++--
> > >   8 files changed, 38 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/Documentation/arm/memory.rst b/Documentation/arm/memory.rst
> > > index 0521b4ce5c96..34bb23c44a71 100644
> > > --- a/Documentation/arm/memory.rst
> > > +++ b/Documentation/arm/memory.rst
> > > @@ -45,9 +45,14 @@ fffe8000   fffeffff        DTCM mapping area for platforms with
> > >   fffe0000    fffe7fff        ITCM mapping area for platforms with
> > >                               ITCM mounted inside the CPU.
> > >
> > > -ffc00000     ffefffff        Fixmap mapping region.  Addresses provided
> > > +ffc80000     ffefffff        Fixmap mapping region.  Addresses provided
> > >                               by fix_to_virt() will be located here.
> > >
> > > +ffc00000     ffc7ffff        Guard region
> > > +
> > > +ff800000     ffbfffff        Permanent, fixed read-only mapping of the
> > > +                             firmware provided DT blob
> > > +
> > >   fee00000    feffffff        Mapping of PCI I/O space. This is a static
> > >                               mapping within the vmalloc space.
> > >
> > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> > > index fc56fc3e1931..9575b404019c 100644
> > > --- a/arch/arm/include/asm/fixmap.h
> > > +++ b/arch/arm/include/asm/fixmap.h
> > > @@ -2,7 +2,7 @@
> > >   #ifndef _ASM_FIXMAP_H
> > >   #define _ASM_FIXMAP_H
> > >
> > > -#define FIXADDR_START                0xffc00000UL
> > > +#define FIXADDR_START                0xffc80000UL
> > >   #define FIXADDR_END         0xfff00000UL
> > >   #define FIXADDR_TOP         (FIXADDR_END - PAGE_SIZE)
> > >
> > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > index 99035b5891ef..ad9dcf4f751d 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -67,6 +67,10 @@
> > >    */
> > >   #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> > >
> > > +#define FDT_FIXED_BASE               UL(0xff800000)
> > > +#define FDT_FIXED_SIZE               (2 * PMD_SIZE)
> > > +#define FDT_VIRT_ADDR(physaddr)      ((void *)(FDT_FIXED_BASE | (physaddr) % PMD_SIZE))
> > > +
> > >   #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
> > >   /*
> > >    * Allow 16MB-aligned ioremap pages
> > > @@ -107,6 +111,7 @@ extern unsigned long vectors_base;
> > >   #define MODULES_VADDR               PAGE_OFFSET
> > >
> > >   #define XIP_VIRT_ADDR(physaddr)  (physaddr)
> > > +#define FDT_VIRT_ADDR(physaddr)  (physaddr)
> > >
> > >   #endif /* !CONFIG_MMU */
> > >
> > > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> > > index f8904227e7fd..9b18d8c66129 100644
> > > --- a/arch/arm/kernel/head.S
> > > +++ b/arch/arm/kernel/head.S
> > > @@ -275,9 +275,8 @@ __create_page_tables:
> > >        */
> > >       mov     r0, r2, lsr #SECTION_SHIFT
> > >       movs    r0, r0, lsl #SECTION_SHIFT
> > > -     subne   r3, r0, r8
> > > -     addne   r3, r3, #PAGE_OFFSET
> > > -     addne   r3, r4, r3, lsr #(SECTION_SHIFT - PMD_ORDER)
> > > +     ldrne   r3, =FDT_FIXED_BASE >> (SECTION_SHIFT - PMD_ORDER)
> > > +     addne   r3, r3, r4
> > >       orrne   r6, r7, r0
> > >       strne   r6, [r3], #1 << PMD_ORDER
> > >       addne   r6, r6, #1 << SECTION_SHIFT
> > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> > > index 58e56e23a19a..c08574cad748 100644
> > > --- a/arch/arm/kernel/setup.c
> > > +++ b/arch/arm/kernel/setup.c
> > > @@ -18,6 +18,7 @@
> > >   #include <linux/of_platform.h>
> > >   #include <linux/init.h>
> > >   #include <linux/kexec.h>
> > > +#include <linux/libfdt.h>
> > >   #include <linux/of_fdt.h>
> > >   #include <linux/cpu.h>
> > >   #include <linux/interrupt.h>
> > > @@ -90,7 +91,6 @@ unsigned int cacheid __read_mostly;
> > >   EXPORT_SYMBOL(cacheid);
> > >
> > >   unsigned int __atags_pointer __initdata;
> > > -void *atags_vaddr __initdata;
> > >
> > >   unsigned int system_rev;
> > >   EXPORT_SYMBOL(system_rev);
> > > @@ -1078,13 +1078,18 @@ void __init hyp_mode_check(void)
> > >   void __init setup_arch(char **cmdline_p)
> > >   {
> > >       const struct machine_desc *mdesc = NULL;
> > > +     void *atags_vaddr;
> > >
> > >       if (__atags_pointer)
> > > -             atags_vaddr = phys_to_virt(__atags_pointer);
> > > +             atags_vaddr = FDT_VIRT_ADDR(__atags_pointer);
> > >
> > >       setup_processor();
> > > -     if (atags_vaddr)
> > > +     if (atags_vaddr) {
> > >               mdesc = setup_machine_fdt(atags_vaddr);
> > > +             if (mdesc)
> > > +                     memblock_reserve(__atags_pointer,
> > > +                                      fdt_totalsize(atags_vaddr));
> > > +     }
> > >       if (!mdesc)
> > >               mdesc = setup_machine_tags(atags_vaddr, __machine_arch_type);
> > >       if (!mdesc) {
> > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > > index 000c1b48e973..652a87bd065c 100644
> > > --- a/arch/arm/mm/init.c
> > > +++ b/arch/arm/mm/init.c
> > > @@ -223,7 +223,6 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
> > >       if (mdesc->reserve)
> > >               mdesc->reserve();
> > >
> > > -     early_init_fdt_reserve_self();
> > >       early_init_fdt_scan_reserved_mem();
> > >
> > >       /* reserve memory for DMA contiguous allocations */
> > > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > > index 0665a0dbd040..a7231d151c63 100644
> > > --- a/arch/arm/mm/mmu.c
> > > +++ b/arch/arm/mm/mmu.c
> > > @@ -40,6 +40,8 @@
> > >   #include "mm.h"
> > >   #include "tcm.h"
> > >
> > > +extern unsigned long __atags_pointer;
> > > +
> > >   /*
> > >    * empty_zero_page is a special page that is used for
> > >    * zero-initialized data and COW.
> > > @@ -947,7 +949,7 @@ static void __init create_mapping(struct map_desc *md)
> > >               return;
> > >       }
> > >
> > > -     if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
> > > +     if (md->type == MT_DEVICE &&
> > >           md->virtual >= PAGE_OFFSET && md->virtual < FIXADDR_START &&
> > >           (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
> > >               pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
> > > @@ -1343,6 +1345,15 @@ static void __init devicemaps_init(const struct machine_desc *mdesc)
> > >       for (addr = VMALLOC_START; addr < (FIXADDR_TOP & PMD_MASK); addr += PMD_SIZE)
> > >               pmd_clear(pmd_off_k(addr));
> > >
> > > +     if (__atags_pointer) {
> > > +             /* create a read-only mapping of the device tree */
> > > +             map.pfn = __phys_to_pfn(__atags_pointer & SECTION_MASK);
> > > +             map.virtual = FDT_FIXED_BASE;
> > > +             map.length = FDT_FIXED_SIZE;
> > > +             map.type = MT_ROM;
> > > +             create_mapping(&map);
> > > +     }
> > > +
> > >       /*
> > >        * Map the kernel if it is XIP.
> > >        * It is always first in the modulearea.
> > > @@ -1503,8 +1514,7 @@ static void __init map_lowmem(void)
> > >   }
> > >
> > >   #ifdef CONFIG_ARM_PV_FIXUP
> > > -extern void *atags_vaddr;
> > > -typedef void pgtables_remap(long long offset, unsigned long pgd, void *bdata);
> > > +typedef void pgtables_remap(long long offset, unsigned long pgd);
> > >   pgtables_remap lpae_pgtables_remap_asm;
> > >
> > >   /*
> > > @@ -1517,7 +1527,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> > >       unsigned long pa_pgd;
> > >       unsigned int cr, ttbcr;
> > >       long long offset;
> > > -     void *boot_data;
> > >
> > >       if (!mdesc->pv_fixup)
> > >               return;
> > > @@ -1534,7 +1543,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> > >        */
> > >       lpae_pgtables_remap = (pgtables_remap *)(unsigned long)__pa(lpae_pgtables_remap_asm);
> > >       pa_pgd = __pa(swapper_pg_dir);
> > > -     boot_data = atags_vaddr;
> > >       barrier();
> > >
> > >       pr_info("Switching physical address space to 0x%08llx\n",
> > > @@ -1570,7 +1578,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> > >        * needs to be assembly.  It's fairly simple, as we're using the
> > >        * temporary tables setup by the initial assembly code.
> > >        */
> > > -     lpae_pgtables_remap(offset, pa_pgd, boot_data);
> > > +     lpae_pgtables_remap(offset, pa_pgd);
> > >
> > >       /* Re-enable the caches and cacheable TLB walks */
> > >       asm volatile("mcr p15, 0, %0, c2, c0, 2" : : "r" (ttbcr));
> > > diff --git a/arch/arm/mm/pv-fixup-asm.S b/arch/arm/mm/pv-fixup-asm.S
> > > index 8eade0416739..5c5e1952000a 100644
> > > --- a/arch/arm/mm/pv-fixup-asm.S
> > > +++ b/arch/arm/mm/pv-fixup-asm.S
> > > @@ -39,8 +39,8 @@ ENTRY(lpae_pgtables_remap_asm)
> > >
> > >       /* Update level 2 entries for the boot data */
> > >       add     r7, r2, #0x1000
> > > -     add     r7, r7, r3, lsr #SECTION_SHIFT - L2_ORDER
> > > -     bic     r7, r7, #(1 << L2_ORDER) - 1
> > > +     movw    r3, #FDT_FIXED_BASE >> (SECTION_SHIFT - L2_ORDER)
> > > +     add     r7, r7, r3
> > >       ldrd    r4, r5, [r7]
> > >       adds    r4, r4, r0
> > >       adc     r5, r5, r1
> >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
> >

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Marek Szyprowski]

Hi Ard,

On 28.10.2020 10:24, Ard Biesheuvel wrote:
> On Wed, 28 Oct 2020 at 10:22, Ard Biesheuvel <ardb@kernel.org> wrote:
>> On Wed, 28 Oct 2020 at 10:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>> On 07.10.2020 10:39, Ard Biesheuvel wrote:
>>>> On ARM, setting up the linear region is tricky, given the constraints
>>>> around placement and alignment of the memblocks, and how the kernel
>>>> itself as well as the DT are placed in physical memory.
>>>>
>>>> Let's simplify matters a bit, by moving the device tree mapping to the
>>>> top of the address space, right between the end of the vmalloc region
>>>> and the start of the the fixmap region, and create a read-only mapping
>>>> for it that is independent of the size of the linear region, and how it
>>>> is organized.
>>>>
>>>> Since this region was formerly used as a guard region, which will now be
>>>> populated fully on LPAE builds by this read-only mapping (which will
>>>> still be able to function as a guard region for stray writes), bump the
>>>> start of the [underutilized] fixmap region by 512 KB as well, to ensure
>>>> that there is always a proper guard region here. Doing so still leaves
>>>> ample room for the fixmap space, even with NR_CPUS set to its maximum
>>>> value of 32.
>>>>
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
>>> 9012/1: move device tree mapping out of linear region"). Sadly it broke
>>> booting  almost all Samsung Exynos-based boards. The only one which
>>> booted, used an appended device tree. I can provide more information if
>>> needed, just let me know what to check. "Starting kernel ..." is the
>>> last message I see here. No output from earlycon.
>>>
>> Thanks for the report. I will have a look later today.
>>
>> Do these platforms happen to have any static device mappings that may
>> collide with this mapping of the FDT? Also, could this be related to
>> device drivers making changes in memory to the FDT image? Because the
>> permanent mapping of the FDT is read-only now.
>>
> IOW, does using MT_MEMORY_RW instead of MT_ROM for the mapping make a
> difference?

Nope, chaning it to MT_MEMORY_RW doesn't fix anything.

The only static mapping I'm aware is S5P_VA_CHIPID at 0xF6000000 + 
0x02000000.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Joel Stanley]

On Wed, 28 Oct 2020 at 09:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi,
>
> On 07.10.2020 10:39, Ard Biesheuvel wrote:
> > On ARM, setting up the linear region is tricky, given the constraints
> > around placement and alignment of the memblocks, and how the kernel
> > itself as well as the DT are placed in physical memory.
> >
> > Let's simplify matters a bit, by moving the device tree mapping to the
> > top of the address space, right between the end of the vmalloc region
> > and the start of the the fixmap region, and create a read-only mapping
> > for it that is independent of the size of the linear region, and how it
> > is organized.
> >
> > Since this region was formerly used as a guard region, which will now be
> > populated fully on LPAE builds by this read-only mapping (which will
> > still be able to function as a guard region for stray writes), bump the
> > start of the [underutilized] fixmap region by 512 KB as well, to ensure
> > that there is always a proper guard region here. Doing so still leaves
> > ample room for the fixmap space, even with NR_CPUS set to its maximum
> > value of 32.
> >
> > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> 9012/1: move device tree mapping out of linear region"). Sadly it broke
> booting  almost all Samsung Exynos-based boards. The only one which
> booted, used an appended device tree. I can provide more information if
> needed, just let me know what to check. "Starting kernel ..." is the
> last message I see here. No output from earlycon.

A bisection lead me to this patch after the next-20201028 failed to
boot on the aspeed systems in testing (aspeed_g5_defconfig).

You can reproduce this with today's next and qemu 5.1:

qemu-system-arm -M romulus-bmc -nographic \
 -kernel arch/arm/boot/zImage \
 -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb \
 -initrd any-old-file

It requires the initrd option to reproduce, but the initrd doesn't
need to be valid as we don't make it that far.

There is no output but attaching gdb shows the kernel is stuck in
setup_machine_tags. (If we enable CONFIG_ATAGS it is instead stuck in
calibrate_delay).

(gdb) bt
#0  setup_machine_tags (machine_nr=<optimized out>,
__atags_vaddr=<optimized out>) at ../arch/arm/kernel/atags.h:12
#1  setup_arch (cmdline_p=0x80c01fc4) at ../arch/arm/kernel/setup.c:1100
#2  0x80b00d2c in start_kernel () at ../init/main.c:862
#3  0x00000000 in ?? ()

Reverting 7a1be318f579 on top of next allowed the system to boot again.

Cheers,

Joel

>
> > ---
> >   Documentation/arm/memory.rst  |  7 ++++++-
> >   arch/arm/include/asm/fixmap.h |  2 +-
> >   arch/arm/include/asm/memory.h |  5 +++++
> >   arch/arm/kernel/head.S        |  5 ++---
> >   arch/arm/kernel/setup.c       | 11 ++++++++---
> >   arch/arm/mm/init.c            |  1 -
> >   arch/arm/mm/mmu.c             | 20 ++++++++++++++------
> >   arch/arm/mm/pv-fixup-asm.S    |  4 ++--
> >   8 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/arm/memory.rst b/Documentation/arm/memory.rst
> > index 0521b4ce5c96..34bb23c44a71 100644
> > --- a/Documentation/arm/memory.rst
> > +++ b/Documentation/arm/memory.rst
> > @@ -45,9 +45,14 @@ fffe8000   fffeffff        DTCM mapping area for platforms with
> >   fffe0000    fffe7fff        ITCM mapping area for platforms with
> >                               ITCM mounted inside the CPU.
> >
> > -ffc00000     ffefffff        Fixmap mapping region.  Addresses provided
> > +ffc80000     ffefffff        Fixmap mapping region.  Addresses provided
> >                               by fix_to_virt() will be located here.
> >
> > +ffc00000     ffc7ffff        Guard region
> > +
> > +ff800000     ffbfffff        Permanent, fixed read-only mapping of the
> > +                             firmware provided DT blob
> > +
> >   fee00000    feffffff        Mapping of PCI I/O space. This is a static
> >                               mapping within the vmalloc space.
> >
> > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> > index fc56fc3e1931..9575b404019c 100644
> > --- a/arch/arm/include/asm/fixmap.h
> > +++ b/arch/arm/include/asm/fixmap.h
> > @@ -2,7 +2,7 @@
> >   #ifndef _ASM_FIXMAP_H
> >   #define _ASM_FIXMAP_H
> >
> > -#define FIXADDR_START                0xffc00000UL
> > +#define FIXADDR_START                0xffc80000UL
> >   #define FIXADDR_END         0xfff00000UL
> >   #define FIXADDR_TOP         (FIXADDR_END - PAGE_SIZE)
> >
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index 99035b5891ef..ad9dcf4f751d 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -67,6 +67,10 @@
> >    */
> >   #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> >
> > +#define FDT_FIXED_BASE               UL(0xff800000)
> > +#define FDT_FIXED_SIZE               (2 * PMD_SIZE)
> > +#define FDT_VIRT_ADDR(physaddr)      ((void *)(FDT_FIXED_BASE | (physaddr) % PMD_SIZE))
> > +
> >   #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
> >   /*
> >    * Allow 16MB-aligned ioremap pages
> > @@ -107,6 +111,7 @@ extern unsigned long vectors_base;
> >   #define MODULES_VADDR               PAGE_OFFSET
> >
> >   #define XIP_VIRT_ADDR(physaddr)  (physaddr)
> > +#define FDT_VIRT_ADDR(physaddr)  (physaddr)
> >
> >   #endif /* !CONFIG_MMU */
> >
> > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> > index f8904227e7fd..9b18d8c66129 100644
> > --- a/arch/arm/kernel/head.S
> > +++ b/arch/arm/kernel/head.S
> > @@ -275,9 +275,8 @@ __create_page_tables:
> >        */
> >       mov     r0, r2, lsr #SECTION_SHIFT
> >       movs    r0, r0, lsl #SECTION_SHIFT
> > -     subne   r3, r0, r8
> > -     addne   r3, r3, #PAGE_OFFSET
> > -     addne   r3, r4, r3, lsr #(SECTION_SHIFT - PMD_ORDER)
> > +     ldrne   r3, =FDT_FIXED_BASE >> (SECTION_SHIFT - PMD_ORDER)
> > +     addne   r3, r3, r4
> >       orrne   r6, r7, r0
> >       strne   r6, [r3], #1 << PMD_ORDER
> >       addne   r6, r6, #1 << SECTION_SHIFT
> > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> > index 58e56e23a19a..c08574cad748 100644
> > --- a/arch/arm/kernel/setup.c
> > +++ b/arch/arm/kernel/setup.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/of_platform.h>
> >   #include <linux/init.h>
> >   #include <linux/kexec.h>
> > +#include <linux/libfdt.h>
> >   #include <linux/of_fdt.h>
> >   #include <linux/cpu.h>
> >   #include <linux/interrupt.h>
> > @@ -90,7 +91,6 @@ unsigned int cacheid __read_mostly;
> >   EXPORT_SYMBOL(cacheid);
> >
> >   unsigned int __atags_pointer __initdata;
> > -void *atags_vaddr __initdata;
> >
> >   unsigned int system_rev;
> >   EXPORT_SYMBOL(system_rev);
> > @@ -1078,13 +1078,18 @@ void __init hyp_mode_check(void)
> >   void __init setup_arch(char **cmdline_p)
> >   {
> >       const struct machine_desc *mdesc = NULL;
> > +     void *atags_vaddr;
> >
> >       if (__atags_pointer)
> > -             atags_vaddr = phys_to_virt(__atags_pointer);
> > +             atags_vaddr = FDT_VIRT_ADDR(__atags_pointer);
> >
> >       setup_processor();
> > -     if (atags_vaddr)
> > +     if (atags_vaddr) {
> >               mdesc = setup_machine_fdt(atags_vaddr);
> > +             if (mdesc)
> > +                     memblock_reserve(__atags_pointer,
> > +                                      fdt_totalsize(atags_vaddr));
> > +     }
> >       if (!mdesc)
> >               mdesc = setup_machine_tags(atags_vaddr, __machine_arch_type);
> >       if (!mdesc) {
> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 000c1b48e973..652a87bd065c 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -223,7 +223,6 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
> >       if (mdesc->reserve)
> >               mdesc->reserve();
> >
> > -     early_init_fdt_reserve_self();
> >       early_init_fdt_scan_reserved_mem();
> >
> >       /* reserve memory for DMA contiguous allocations */
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > index 0665a0dbd040..a7231d151c63 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -40,6 +40,8 @@
> >   #include "mm.h"
> >   #include "tcm.h"
> >
> > +extern unsigned long __atags_pointer;
> > +
> >   /*
> >    * empty_zero_page is a special page that is used for
> >    * zero-initialized data and COW.
> > @@ -947,7 +949,7 @@ static void __init create_mapping(struct map_desc *md)
> >               return;
> >       }
> >
> > -     if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
> > +     if (md->type == MT_DEVICE &&
> >           md->virtual >= PAGE_OFFSET && md->virtual < FIXADDR_START &&
> >           (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
> >               pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
> > @@ -1343,6 +1345,15 @@ static void __init devicemaps_init(const struct machine_desc *mdesc)
> >       for (addr = VMALLOC_START; addr < (FIXADDR_TOP & PMD_MASK); addr += PMD_SIZE)
> >               pmd_clear(pmd_off_k(addr));
> >
> > +     if (__atags_pointer) {
> > +             /* create a read-only mapping of the device tree */
> > +             map.pfn = __phys_to_pfn(__atags_pointer & SECTION_MASK);
> > +             map.virtual = FDT_FIXED_BASE;
> > +             map.length = FDT_FIXED_SIZE;
> > +             map.type = MT_ROM;
> > +             create_mapping(&map);
> > +     }
> > +
> >       /*
> >        * Map the kernel if it is XIP.
> >        * It is always first in the modulearea.
> > @@ -1503,8 +1514,7 @@ static void __init map_lowmem(void)
> >   }
> >
> >   #ifdef CONFIG_ARM_PV_FIXUP
> > -extern void *atags_vaddr;
> > -typedef void pgtables_remap(long long offset, unsigned long pgd, void *bdata);
> > +typedef void pgtables_remap(long long offset, unsigned long pgd);
> >   pgtables_remap lpae_pgtables_remap_asm;
> >
> >   /*
> > @@ -1517,7 +1527,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> >       unsigned long pa_pgd;
> >       unsigned int cr, ttbcr;
> >       long long offset;
> > -     void *boot_data;
> >
> >       if (!mdesc->pv_fixup)
> >               return;
> > @@ -1534,7 +1543,6 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> >        */
> >       lpae_pgtables_remap = (pgtables_remap *)(unsigned long)__pa(lpae_pgtables_remap_asm);
> >       pa_pgd = __pa(swapper_pg_dir);
> > -     boot_data = atags_vaddr;
> >       barrier();
> >
> >       pr_info("Switching physical address space to 0x%08llx\n",
> > @@ -1570,7 +1578,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
> >        * needs to be assembly.  It's fairly simple, as we're using the
> >        * temporary tables setup by the initial assembly code.
> >        */
> > -     lpae_pgtables_remap(offset, pa_pgd, boot_data);
> > +     lpae_pgtables_remap(offset, pa_pgd);
> >
> >       /* Re-enable the caches and cacheable TLB walks */
> >       asm volatile("mcr p15, 0, %0, c2, c0, 2" : : "r" (ttbcr));
> > diff --git a/arch/arm/mm/pv-fixup-asm.S b/arch/arm/mm/pv-fixup-asm.S
> > index 8eade0416739..5c5e1952000a 100644
> > --- a/arch/arm/mm/pv-fixup-asm.S
> > +++ b/arch/arm/mm/pv-fixup-asm.S
> > @@ -39,8 +39,8 @@ ENTRY(lpae_pgtables_remap_asm)
> >
> >       /* Update level 2 entries for the boot data */
> >       add     r7, r2, #0x1000
> > -     add     r7, r7, r3, lsr #SECTION_SHIFT - L2_ORDER
> > -     bic     r7, r7, #(1 << L2_ORDER) - 1
> > +     movw    r3, #FDT_FIXED_BASE >> (SECTION_SHIFT - L2_ORDER)
> > +     add     r7, r7, r3
> >       ldrd    r4, r5, [r7]
> >       adds    r4, r4, r0
> >       adc     r5, r5, r1
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 13:05, Joel Stanley <joel@jms.id.au> wrote:
>
> On Wed, 28 Oct 2020 at 09:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >
> > Hi,
> >
> > On 07.10.2020 10:39, Ard Biesheuvel wrote:
> > > On ARM, setting up the linear region is tricky, given the constraints
> > > around placement and alignment of the memblocks, and how the kernel
> > > itself as well as the DT are placed in physical memory.
> > >
> > > Let's simplify matters a bit, by moving the device tree mapping to the
> > > top of the address space, right between the end of the vmalloc region
> > > and the start of the the fixmap region, and create a read-only mapping
> > > for it that is independent of the size of the linear region, and how it
> > > is organized.
> > >
> > > Since this region was formerly used as a guard region, which will now be
> > > populated fully on LPAE builds by this read-only mapping (which will
> > > still be able to function as a guard region for stray writes), bump the
> > > start of the [underutilized] fixmap region by 512 KB as well, to ensure
> > > that there is always a proper guard region here. Doing so still leaves
> > > ample room for the fixmap space, even with NR_CPUS set to its maximum
> > > value of 32.
> > >
> > > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> > 9012/1: move device tree mapping out of linear region"). Sadly it broke
> > booting  almost all Samsung Exynos-based boards. The only one which
> > booted, used an appended device tree. I can provide more information if
> > needed, just let me know what to check. "Starting kernel ..." is the
> > last message I see here. No output from earlycon.
>
> A bisection lead me to this patch after the next-20201028 failed to
> boot on the aspeed systems in testing (aspeed_g5_defconfig).
>
> You can reproduce this with today's next and qemu 5.1:
>
> qemu-system-arm -M romulus-bmc -nographic \
>  -kernel arch/arm/boot/zImage \
>  -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb \
>  -initrd any-old-file
>
> It requires the initrd option to reproduce, but the initrd doesn't
> need to be valid as we don't make it that far.
>
> There is no output but attaching gdb shows the kernel is stuck in
> setup_machine_tags. (If we enable CONFIG_ATAGS it is instead stuck in
> calibrate_delay).
>
> (gdb) bt
> #0  setup_machine_tags (machine_nr=<optimized out>,
> __atags_vaddr=<optimized out>) at ../arch/arm/kernel/atags.h:12
> #1  setup_arch (cmdline_p=0x80c01fc4) at ../arch/arm/kernel/setup.c:1100
> #2  0x80b00d2c in start_kernel () at ../init/main.c:862
> #3  0x00000000 in ?? ()
>
> Reverting 7a1be318f579 on top of next allowed the system to boot again.
>

Does this help?

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index bb79e52aeb90..4f355bda872a 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -68,8 +68,8 @@
 #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))

 #define FDT_FIXED_BASE         UL(0xff800000)
-#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
-#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
(physaddr) % PMD_SIZE))
+#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
+#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
(physaddr) % SECTION_SIZE))

 #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
 /*

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Joel Stanley]

On Wed, 28 Oct 2020 at 12:53, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 28 Oct 2020 at 13:05, Joel Stanley <joel@jms.id.au> wrote:
> >
> > On Wed, 28 Oct 2020 at 09:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > > This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> > > 9012/1: move device tree mapping out of linear region"). Sadly it broke
> > > booting  almost all Samsung Exynos-based boards. The only one which
> > > booted, used an appended device tree. I can provide more information if
> > > needed, just let me know what to check. "Starting kernel ..." is the
> > > last message I see here. No output from earlycon.
> >
> > A bisection lead me to this patch after the next-20201028 failed to
> > boot on the aspeed systems in testing (aspeed_g5_defconfig).
> >
> > You can reproduce this with today's next and qemu 5.1:
> >
> > qemu-system-arm -M romulus-bmc -nographic \
> >  -kernel arch/arm/boot/zImage \
> >  -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb \
> >  -initrd any-old-file
> >
> > It requires the initrd option to reproduce, but the initrd doesn't
> > need to be valid as we don't make it that far.
> >
> > There is no output but attaching gdb shows the kernel is stuck in
> > setup_machine_tags. (If we enable CONFIG_ATAGS it is instead stuck in
> > calibrate_delay).
> >
> > (gdb) bt
> > #0  setup_machine_tags (machine_nr=<optimized out>,
> > __atags_vaddr=<optimized out>) at ../arch/arm/kernel/atags.h:12
> > #1  setup_arch (cmdline_p=0x80c01fc4) at ../arch/arm/kernel/setup.c:1100
> > #2  0x80b00d2c in start_kernel () at ../init/main.c:862
> > #3  0x00000000 in ?? ()
> >
> > Reverting 7a1be318f579 on top of next allowed the system to boot again.
> >
>
> Does this help?

Yes, that boots to userspace.

>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index bb79e52aeb90..4f355bda872a 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -68,8 +68,8 @@
>  #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
>
>  #define FDT_FIXED_BASE         UL(0xff800000)
> -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> (physaddr) % PMD_SIZE))
> +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> (physaddr) % SECTION_SIZE))
>
>  #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
>  /*

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 13:59, Joel Stanley <joel@jms.id.au> wrote:
>
> On Wed, 28 Oct 2020 at 12:53, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 28 Oct 2020 at 13:05, Joel Stanley <joel@jms.id.au> wrote:
> > >
> > > On Wed, 28 Oct 2020 at 09:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > > > This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> > > > 9012/1: move device tree mapping out of linear region"). Sadly it broke
> > > > booting  almost all Samsung Exynos-based boards. The only one which
> > > > booted, used an appended device tree. I can provide more information if
> > > > needed, just let me know what to check. "Starting kernel ..." is the
> > > > last message I see here. No output from earlycon.
> > >
> > > A bisection lead me to this patch after the next-20201028 failed to
> > > boot on the aspeed systems in testing (aspeed_g5_defconfig).
> > >
> > > You can reproduce this with today's next and qemu 5.1:
> > >
> > > qemu-system-arm -M romulus-bmc -nographic \
> > >  -kernel arch/arm/boot/zImage \
> > >  -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb \
> > >  -initrd any-old-file
> > >
> > > It requires the initrd option to reproduce, but the initrd doesn't
> > > need to be valid as we don't make it that far.
> > >
> > > There is no output but attaching gdb shows the kernel is stuck in
> > > setup_machine_tags. (If we enable CONFIG_ATAGS it is instead stuck in
> > > calibrate_delay).
> > >
> > > (gdb) bt
> > > #0  setup_machine_tags (machine_nr=<optimized out>,
> > > __atags_vaddr=<optimized out>) at ../arch/arm/kernel/atags.h:12
> > > #1  setup_arch (cmdline_p=0x80c01fc4) at ../arch/arm/kernel/setup.c:1100
> > > #2  0x80b00d2c in start_kernel () at ../init/main.c:862
> > > #3  0x00000000 in ?? ()
> > >
> > > Reverting 7a1be318f579 on top of next allowed the system to boot again.
> > >
> >
> > Does this help?
>
> Yes, that boots to userspace.
>

Thanks. I'll take that as a tested-by


> >
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index bb79e52aeb90..4f355bda872a 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -68,8 +68,8 @@
> >  #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> >
> >  #define FDT_FIXED_BASE         UL(0xff800000)
> > -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> > -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > (physaddr) % PMD_SIZE))
> > +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> > +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > (physaddr) % SECTION_SIZE))
> >
> >  #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
> >  /*

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Joel Stanley]

On Wed, 28 Oct 2020 at 13:00, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 28 Oct 2020 at 13:59, Joel Stanley <joel@jms.id.au> wrote:
> >
> > On Wed, 28 Oct 2020 at 12:53, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 28 Oct 2020 at 13:05, Joel Stanley <joel@jms.id.au> wrote:
> > > >
> > > > On Wed, 28 Oct 2020 at 09:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > > > > This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
> > > > > 9012/1: move device tree mapping out of linear region"). Sadly it broke
> > > > > booting  almost all Samsung Exynos-based boards. The only one which
> > > > > booted, used an appended device tree. I can provide more information if
> > > > > needed, just let me know what to check. "Starting kernel ..." is the
> > > > > last message I see here. No output from earlycon.
> > > >
> > > > A bisection lead me to this patch after the next-20201028 failed to
> > > > boot on the aspeed systems in testing (aspeed_g5_defconfig).
> > > >
> > > > You can reproduce this with today's next and qemu 5.1:
> > > >
> > > > qemu-system-arm -M romulus-bmc -nographic \
> > > >  -kernel arch/arm/boot/zImage \
> > > >  -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb \
> > > >  -initrd any-old-file
> > > >
> > > > It requires the initrd option to reproduce, but the initrd doesn't
> > > > need to be valid as we don't make it that far.
> > > >
> > > > There is no output but attaching gdb shows the kernel is stuck in
> > > > setup_machine_tags. (If we enable CONFIG_ATAGS it is instead stuck in
> > > > calibrate_delay).
> > > >
> > > > (gdb) bt
> > > > #0  setup_machine_tags (machine_nr=<optimized out>,
> > > > __atags_vaddr=<optimized out>) at ../arch/arm/kernel/atags.h:12
> > > > #1  setup_arch (cmdline_p=0x80c01fc4) at ../arch/arm/kernel/setup.c:1100
> > > > #2  0x80b00d2c in start_kernel () at ../init/main.c:862
> > > > #3  0x00000000 in ?? ()
> > > >
> > > > Reverting 7a1be318f579 on top of next allowed the system to boot again.
> > > >
> > >
> > > Does this help?
> >
> > Yes, that boots to userspace.
> >
>
> Thanks. I'll take that as a tested-by

Please do. Thanks for the quick fix.

Cheers,

Joel

>
>
> > >
> > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > index bb79e52aeb90..4f355bda872a 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -68,8 +68,8 @@
> > >  #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> > >
> > >  #define FDT_FIXED_BASE         UL(0xff800000)
> > > -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> > > -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > > (physaddr) % PMD_SIZE))
> > > +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> > > +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > > (physaddr) % SECTION_SIZE))
> > >
> > >  #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
> > >  /*

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Russell King - ARM Linux admin]

On Wed, Oct 28, 2020 at 01:52:48PM +0100, Ard Biesheuvel wrote:
> Does this help?
> 
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index bb79e52aeb90..4f355bda872a 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -68,8 +68,8 @@
>  #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> 
>  #define FDT_FIXED_BASE         UL(0xff800000)
> -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> (physaddr) % PMD_SIZE))
> +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> (physaddr) % SECTION_SIZE))

Is this correct? If the FDT fixed size is 2x, why does FDT_VIRT_ADDR()
only work for half of it?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 14:01, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Oct 28, 2020 at 01:52:48PM +0100, Ard Biesheuvel wrote:
> > Does this help?
> >
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index bb79e52aeb90..4f355bda872a 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -68,8 +68,8 @@
> >  #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> >
> >  #define FDT_FIXED_BASE         UL(0xff800000)
> > -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> > -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > (physaddr) % PMD_SIZE))
> > +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> > +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > (physaddr) % SECTION_SIZE))
>
> Is this correct? If the FDT fixed size is 2x, why does FDT_VIRT_ADDR()
> only work for half of it?
>

Perhaps the naming is confusing. This is only used to obtain the start
of the virtually mapped DT, which amounts to the virtual address of
the section plus the physical address modulo the section size.

I will rename physaddr to physbase when I respin this as a patch.
Shall I rename FDT_VIRT_ADDR to FDT_VIRT_BASE as well?

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Russell King - ARM Linux admin]

On Wed, Oct 28, 2020 at 02:04:41PM +0100, Ard Biesheuvel wrote:
> On Wed, 28 Oct 2020 at 14:01, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Oct 28, 2020 at 01:52:48PM +0100, Ard Biesheuvel wrote:
> > > Does this help?
> > >
> > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > index bb79e52aeb90..4f355bda872a 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -68,8 +68,8 @@
> > >  #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
> > >
> > >  #define FDT_FIXED_BASE         UL(0xff800000)
> > > -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> > > -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > > (physaddr) % PMD_SIZE))
> > > +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> > > +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> > > (physaddr) % SECTION_SIZE))
> >
> > Is this correct? If the FDT fixed size is 2x, why does FDT_VIRT_ADDR()
> > only work for half of it?
> >
> 
> Perhaps the naming is confusing. This is only used to obtain the start
> of the virtually mapped DT, which amounts to the virtual address of
> the section plus the physical address modulo the section size.
> 
> I will rename physaddr to physbase when I respin this as a patch.
> Shall I rename FDT_VIRT_ADDR to FDT_VIRT_BASE as well?

Yes please, I think that makes it clearer what is going on here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

[Marek Szyprowski]

Hi Ard,

On 28.10.2020 13:52, Ard Biesheuvel wrote:
> On Wed, 28 Oct 2020 at 13:05, Joel Stanley <joel@jms.id.au> wrote:
>> On Wed, 28 Oct 2020 at 09:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>> On 07.10.2020 10:39, Ard Biesheuvel wrote:
>>>> On ARM, setting up the linear region is tricky, given the constraints
>>>> around placement and alignment of the memblocks, and how the kernel
>>>> itself as well as the DT are placed in physical memory.
>>>>
>>>> Let's simplify matters a bit, by moving the device tree mapping to the
>>>> top of the address space, right between the end of the vmalloc region
>>>> and the start of the the fixmap region, and create a read-only mapping
>>>> for it that is independent of the size of the linear region, and how it
>>>> is organized.
>>>>
>>>> Since this region was formerly used as a guard region, which will now be
>>>> populated fully on LPAE builds by this read-only mapping (which will
>>>> still be able to function as a guard region for stray writes), bump the
>>>> start of the [underutilized] fixmap region by 512 KB as well, to ensure
>>>> that there is always a proper guard region here. Doing so still leaves
>>>> ample room for the fixmap space, even with NR_CPUS set to its maximum
>>>> value of 32.
>>>>
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> This patch landed in linux-next 20201028 as commit 7a1be318f579 ("ARM:
>>> 9012/1: move device tree mapping out of linear region"). Sadly it broke
>>> booting  almost all Samsung Exynos-based boards. The only one which
>>> booted, used an appended device tree. I can provide more information if
>>> needed, just let me know what to check. "Starting kernel ..." is the
>>> last message I see here. No output from earlycon.
>> A bisection lead me to this patch after the next-20201028 failed to
>> boot on the aspeed systems in testing (aspeed_g5_defconfig).
>>
>> You can reproduce this with today's next and qemu 5.1:
>>
>> qemu-system-arm -M romulus-bmc -nographic \
>>   -kernel arch/arm/boot/zImage \
>>   -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb \
>>   -initrd any-old-file
>>
>> It requires the initrd option to reproduce, but the initrd doesn't
>> need to be valid as we don't make it that far.
>>
>> There is no output but attaching gdb shows the kernel is stuck in
>> setup_machine_tags. (If we enable CONFIG_ATAGS it is instead stuck in
>> calibrate_delay).
>>
>> (gdb) bt
>> #0  setup_machine_tags (machine_nr=<optimized out>,
>> __atags_vaddr=<optimized out>) at ../arch/arm/kernel/atags.h:12
>> #1  setup_arch (cmdline_p=0x80c01fc4) at ../arch/arm/kernel/setup.c:1100
>> #2  0x80b00d2c in start_kernel () at ../init/main.c:862
>> #3  0x00000000 in ?? ()
>>
>> Reverting 7a1be318f579 on top of next allowed the system to boot again.
>>
> Does this help?
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index bb79e52aeb90..4f355bda872a 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -68,8 +68,8 @@
>   #define XIP_VIRT_ADDR(physaddr)  (MODULES_VADDR + ((physaddr) & 0x000fffff))
>
>   #define FDT_FIXED_BASE         UL(0xff800000)
> -#define FDT_FIXED_SIZE         (2 * PMD_SIZE)
> -#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> (physaddr) % PMD_SIZE))
> +#define FDT_FIXED_SIZE         (2 * SECTION_SIZE)
> +#define FDT_VIRT_ADDR(physaddr)        ((void *)(FDT_FIXED_BASE |
> (physaddr) % SECTION_SIZE))
>
>   #if !defined(CONFIG_SMP) && !defined(CONFIG_ARM_LPAE)
>   /*

Yes, this fixes booting of my Samsung Exynos-based test boards :)

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland