[v4] riscv: Use PUD/P4D/PGD pages for the linear mapping

Commit Message

Alexandre Ghiti Jan. 23, 2023, 11:28 a.m. UTC

During the early page table creation, we used to set the mapping for
PAGE_OFFSET to the kernel load address: but the kernel load address is
always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
PAGE_OFFSET is).

But actually we don't have to establish this mapping (ie set va_pa_offset)
that early in the boot process because:

- first, setup_vm installs a temporary kernel mapping and among other
  things, discovers the system memory,
- then, setup_vm_final creates the final kernel mapping and takes
  advantage of the discovered system memory to create the linear
  mapping.

During the first phase, we don't know the start of the system memory and
then until the second phase is finished, we can't use the linear mapping at
all and phys_to_virt/virt_to_phys translations must not be used because it
would result in a different translation from the 'real' one once the final
mapping is installed.

So here we simply delay the initialization of va_pa_offset to after the
system memory discovery. But to make sure noone uses the linear mapping
before, we add some guard in the DEBUG_VIRTUAL config.

Finally we can use PUD/P4D/PGD hugepages when possible, which will result
in a better TLB utilization.

Note that we rely on the firmware to protect itself using PMP.

Acked-by: Rob Herring <[e-mail]> # DT bits
Signed-off-by: Alexandre Ghiti <[e-mail]>
---

v4:
- Rebase on top of v6.2-rc3, as noted by Conor
- Add Acked-by Rob

v3:
- Change the comment about initrd_start VA conversion so that it fits
  ARM64 and RISCV64 (and others in the future if needed), as suggested
  by Rob

v2:
- Add a comment on why RISCV64 does not need to set initrd_start/end that
  early in the boot process, as asked by Rob

 arch/riscv/include/asm/page.h | 16 ++++++++++++++++
 arch/riscv/mm/init.c          | 25 +++++++++++++++++++------
 arch/riscv/mm/physaddr.c      | 16 ++++++++++++++++
 drivers/of/fdt.c              | 11 ++++++-----
 4 files changed, 57 insertions(+), 11 deletions(-)

Comments

Andrew Jones Jan. 23, 2023, 2:25 p.m. UTC | #1

On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> During the early page table creation, we used to set the mapping for
> PAGE_OFFSET to the kernel load address: but the kernel load address is
> always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> PAGE_OFFSET is).
> 
> But actually we don't have to establish this mapping (ie set va_pa_offset)
> that early in the boot process because:
> 
> - first, setup_vm installs a temporary kernel mapping and among other
>   things, discovers the system memory,
> - then, setup_vm_final creates the final kernel mapping and takes
>   advantage of the discovered system memory to create the linear
>   mapping.
> 
> During the first phase, we don't know the start of the system memory and
> then until the second phase is finished, we can't use the linear mapping at
> all and phys_to_virt/virt_to_phys translations must not be used because it
> would result in a different translation from the 'real' one once the final
> mapping is installed.
> 
> So here we simply delay the initialization of va_pa_offset to after the
> system memory discovery. But to make sure noone uses the linear mapping
> before, we add some guard in the DEBUG_VIRTUAL config.
> 
> Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> in a better TLB utilization.
> 
> Note that we rely on the firmware to protect itself using PMP.
> 
> Acked-by: Rob Herring <[e-mail]> # DT bits
> Signed-off-by: Alexandre Ghiti <[e-mail]>
> ---
> 
> v4:
> - Rebase on top of v6.2-rc3, as noted by Conor
> - Add Acked-by Rob
> 
> v3:
> - Change the comment about initrd_start VA conversion so that it fits
>   ARM64 and RISCV64 (and others in the future if needed), as suggested
>   by Rob
> 
> v2:
> - Add a comment on why RISCV64 does not need to set initrd_start/end that
>   early in the boot process, as asked by Rob
> 
>  arch/riscv/include/asm/page.h | 16 ++++++++++++++++
>  arch/riscv/mm/init.c          | 25 +++++++++++++++++++------
>  arch/riscv/mm/physaddr.c      | 16 ++++++++++++++++
>  drivers/of/fdt.c              | 11 ++++++-----
>  4 files changed, 57 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> index 9f432c1b5289..7fe84c89e572 100644
> --- a/arch/riscv/include/asm/page.h
> +++ b/arch/riscv/include/asm/page.h
> @@ -90,6 +90,14 @@ typedef struct page *pgtable_t;
>  #define PTE_FMT "%08lx"
>  #endif
>  
> +#ifdef CONFIG_64BIT
> +/*
> + * We override this value as its generic definition uses __pa too early in
> + * the boot process (before kernel_map.va_pa_offset is set).
> + */
> +#define MIN_MEMBLOCK_ADDR      0
> +#endif
> +
>  #ifdef CONFIG_MMU
>  extern unsigned long riscv_pfn_base;
>  #define ARCH_PFN_OFFSET		(riscv_pfn_base)
> @@ -122,7 +130,11 @@ extern phys_addr_t phys_ram_base;
>  #define is_linear_mapping(x)	\
>  	((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
>  
> +#ifndef CONFIG_DEBUG_VIRTUAL
>  #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
> +#else
> +void *linear_mapping_pa_to_va(unsigned long x);
> +#endif
>  #define kernel_mapping_pa_to_va(y)	({					\
>  	unsigned long _y = (unsigned long)(y);					\
>  	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?			\
> @@ -131,7 +143,11 @@ extern phys_addr_t phys_ram_base;
>  	})
>  #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
>  
> +#ifndef CONFIG_DEBUG_VIRTUAL
>  #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - kernel_map.va_pa_offset)
> +#else
> +phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> +#endif
>  #define kernel_mapping_va_to_pa(y) ({						\
>  	unsigned long _y = (unsigned long)(y);					\
>  	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 478d6763a01a..cc892ba9f787 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -213,6 +213,14 @@ static void __init setup_bootmem(void)
>  	phys_ram_end = memblock_end_of_DRAM();
>  	if (!IS_ENABLED(CONFIG_XIP_KERNEL))
>  		phys_ram_base = memblock_start_of_DRAM();
> +
> +	/*
> +	 * Any use of __va/__pa before this point is wrong as we did not know the
> +	 * start of DRAM before.
> +	 */
> +	kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
> +	riscv_pfn_base = PFN_DOWN(phys_ram_base);
> +
>  	/*
>  	 * memblock allocator is not aware of the fact that last 4K bytes of
>  	 * the addressable memory can not be mapped because of IS_ERR_VALUE
> @@ -671,9 +679,16 @@ void __init create_pgd_mapping(pgd_t *pgdp,
>  
>  static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
>  {
> -	/* Upgrade to PMD_SIZE mappings whenever possible */
> -	base &= PMD_SIZE - 1;
> -	if (!base && size >= PMD_SIZE)
> +	if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
> +		return PGDIR_SIZE;
> +
> +	if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
> +		return P4D_SIZE;
> +
> +	if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
> +		return PUD_SIZE;
> +
> +	if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
>  		return PMD_SIZE;
>  
>  	return PAGE_SIZE;
> @@ -982,11 +997,9 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>  	set_satp_mode();
>  #endif
>  
> -	kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> +	kernel_map.va_pa_offset = 0UL;
>  	kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
>  
> -	riscv_pfn_base = PFN_DOWN(kernel_map.phys_addr);
> -
>  	/*
>  	 * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
>  	 * kernel, whereas for 64-bit kernel, the end of the virtual address
> diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
> index 9b18bda74154..18706f457da7 100644
> --- a/arch/riscv/mm/physaddr.c
> +++ b/arch/riscv/mm/physaddr.c
> @@ -33,3 +33,19 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
>  	return __va_to_pa_nodebug(x);
>  }
>  EXPORT_SYMBOL(__phys_addr_symbol);
> +
> +phys_addr_t linear_mapping_va_to_pa(unsigned long x)
> +{
> +	BUG_ON(!kernel_map.va_pa_offset);
> +
> +	return ((unsigned long)(x) - kernel_map.va_pa_offset);
> +}
> +EXPORT_SYMBOL(linear_mapping_va_to_pa);
> +
> +void *linear_mapping_pa_to_va(unsigned long x)
> +{
> +	BUG_ON(!kernel_map.va_pa_offset);
> +
> +	return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
> +}
> +EXPORT_SYMBOL(linear_mapping_pa_to_va);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index f08b25195ae7..58107bd56f8f 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
>  static void __early_init_dt_declare_initrd(unsigned long start,
>  					   unsigned long end)
>  {
> -	/* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> -	 * enabled since __va() is called too early. ARM64 does make use
> -	 * of phys_initrd_start/phys_initrd_size so we can skip this
> -	 * conversion.
> +	/*
> +	 * __va() is not yet available this early on some platforms. In that
> +	 * case, the platform uses phys_initrd_start/phys_initrd_size instead
> +	 * and does the VA conversion itself.
>  	 */
> -	if (!IS_ENABLED(CONFIG_ARM64)) {
> +	if (!IS_ENABLED(CONFIG_ARM64) &&
> +	    !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {

There are now two architectures, so maybe it's time for a new config
symbol which would be selected by arm64 and riscv64 and then used here,
e.g.

  if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {

>  		initrd_start = (unsigned long)__va(start);
>  		initrd_end = (unsigned long)__va(end);
>  		initrd_below_start_ok = 1;
> -- 
> 2.37.2
>

Otherwise,

Reviewed-by: Andrew Jones <[e-mail]>

Thanks,
drew

Conor Dooley Jan. 23, 2023, 10:10 p.m. UTC | #2

On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> During the early page table creation, we used to set the mapping for
> PAGE_OFFSET to the kernel load address: but the kernel load address is
> always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> PAGE_OFFSET is).
> 
> But actually we don't have to establish this mapping (ie set va_pa_offset)
> that early in the boot process because:
> 
> - first, setup_vm installs a temporary kernel mapping and among other
>   things, discovers the system memory,
> - then, setup_vm_final creates the final kernel mapping and takes
>   advantage of the discovered system memory to create the linear
>   mapping.
> 
> During the first phase, we don't know the start of the system memory and
> then until the second phase is finished, we can't use the linear mapping at
> all and phys_to_virt/virt_to_phys translations must not be used because it
> would result in a different translation from the 'real' one once the final
> mapping is installed.
> 
> So here we simply delay the initialization of va_pa_offset to after the
> system memory discovery. But to make sure noone uses the linear mapping
> before, we add some guard in the DEBUG_VIRTUAL config.
> 
> Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> in a better TLB utilization.
> 
> Note that we rely on the firmware to protect itself using PMP.
> 
> Acked-by: Rob Herring <[e-mail]> # DT bits
> Signed-off-by: Alexandre Ghiti <[e-mail]>

No good on !MMU unfortunately Alex:
../arch/riscv/mm/init.c:222:2: error: use of undeclared identifier 'riscv_pfn_base'
        riscv_pfn_base = PFN_DOWN(phys_ram_base);
        ^

Reproduces with nommu_virt_defconfig.

Thanks,
Conor.

kernel test robot Jan. 24, 2023, 12:19 a.m. UTC | #3

Hi Alexandre,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.2-rc5 next-20230123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Ghiti/riscv-Use-PUD-P4D-PGD-pages-for-the-linear-mapping/20230123-192952
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230123112803.817534-1-alexghiti%40rivosinc.com
patch subject: [PATCH v4] riscv: Use PUD/P4D/PGD pages for the linear mapping
config: riscv-nommu_virt_defconfig (https://download.01.org/0day-ci/archive/20230124/[e-mail]/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b0cdf20b21efcc85ec67bffa6a10894dedd0d12e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Alexandre-Ghiti/riscv-Use-PUD-P4D-PGD-pages-for-the-linear-mapping/20230123-192952
        git checkout b0cdf20b21efcc85ec67bffa6a10894dedd0d12e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash arch/riscv/mm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   arch/riscv/mm/init.c: In function 'setup_bootmem':
>> arch/riscv/mm/init.c:222:9: error: 'riscv_pfn_base' undeclared (first use in this function)
     222 |         riscv_pfn_base = PFN_DOWN(phys_ram_base);
         |         ^~~~~~~~~~~~~~
   arch/riscv/mm/init.c:222:9: note: each undeclared identifier is reported only once for each function it appears in


vim +/riscv_pfn_base +222 arch/riscv/mm/init.c

   187	
   188	static void __init setup_bootmem(void)
   189	{
   190		phys_addr_t vmlinux_end = __pa_symbol(&_end);
   191		phys_addr_t max_mapped_addr;
   192		phys_addr_t phys_ram_end, vmlinux_start;
   193	
   194		if (IS_ENABLED(CONFIG_XIP_KERNEL))
   195			vmlinux_start = __pa_symbol(&_sdata);
   196		else
   197			vmlinux_start = __pa_symbol(&_start);
   198	
   199		memblock_enforce_memory_limit(memory_limit);
   200	
   201		/*
   202		 * Make sure we align the reservation on PMD_SIZE since we will
   203		 * map the kernel in the linear mapping as read-only: we do not want
   204		 * any allocation to happen between _end and the next pmd aligned page.
   205		 */
   206		if (IS_ENABLED(CONFIG_64BIT) && IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
   207			vmlinux_end = (vmlinux_end + PMD_SIZE - 1) & PMD_MASK;
   208		/*
   209		 * Reserve from the start of the kernel to the end of the kernel
   210		 */
   211		memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
   212	
   213		phys_ram_end = memblock_end_of_DRAM();
   214		if (!IS_ENABLED(CONFIG_XIP_KERNEL))
   215			phys_ram_base = memblock_start_of_DRAM();
   216	
   217		/*
   218		 * Any use of __va/__pa before this point is wrong as we did not know the
   219		 * start of DRAM before.
   220		 */
   221		kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
 > 222		riscv_pfn_base = PFN_DOWN(phys_ram_base);
   223	
   224		/*
   225		 * memblock allocator is not aware of the fact that last 4K bytes of
   226		 * the addressable memory can not be mapped because of IS_ERR_VALUE
   227		 * macro. Make sure that last 4k bytes are not usable by memblock
   228		 * if end of dram is equal to maximum addressable memory.  For 64-bit
   229		 * kernel, this problem can't happen here as the end of the virtual
   230		 * address space is occupied by the kernel mapping then this check must
   231		 * be done as soon as the kernel mapping base address is determined.
   232		 */
   233		if (!IS_ENABLED(CONFIG_64BIT)) {
   234			max_mapped_addr = __pa(~(ulong)0);
   235			if (max_mapped_addr == (phys_ram_end - 1))
   236				memblock_set_current_limit(max_mapped_addr - 4096);
   237		}
   238	
   239		min_low_pfn = PFN_UP(phys_ram_base);
   240		max_low_pfn = max_pfn = PFN_DOWN(phys_ram_end);
   241		high_memory = (void *)(__va(PFN_PHYS(max_low_pfn)));
   242	
   243		dma32_phys_limit = min(4UL * SZ_1G, (unsigned long)PFN_PHYS(max_low_pfn));
   244		set_max_mapnr(max_low_pfn - ARCH_PFN_OFFSET);
   245	
   246		reserve_initrd_mem();
   247		/*
   248		 * If DTB is built in, no need to reserve its memblock.
   249		 * Otherwise, do reserve it but avoid using
   250		 * early_init_fdt_reserve_self() since __pa() does
   251		 * not work for DTB pointers that are fixmap addresses
   252		 */
   253		if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) {
   254			/*
   255			 * In case the DTB is not located in a memory region we won't
   256			 * be able to locate it later on via the linear mapping and
   257			 * get a segfault when accessing it via __va(dtb_early_pa).
   258			 * To avoid this situation copy DTB to a memory region.
   259			 * Note that memblock_phys_alloc will also reserve DTB region.
   260			 */
   261			if (!memblock_is_memory(dtb_early_pa)) {
   262				size_t fdt_size = fdt_totalsize(dtb_early_va);
   263				phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE);
   264				void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size);
   265	
   266				memcpy(new_dtb_early_va, dtb_early_va, fdt_size);
   267				early_memunmap(new_dtb_early_va, fdt_size);
   268				_dtb_early_pa = new_dtb_early_pa;
   269			} else
   270				memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
   271		}
   272	
   273		dma_contiguous_reserve(dma32_phys_limit);
   274		if (IS_ENABLED(CONFIG_64BIT))
   275			hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT);
   276		memblock_allow_resize();
   277	}
   278

Alexandre Ghiti Jan. 24, 2023, 9:04 a.m. UTC | #4

Hi Conor,

On Mon, Jan 23, 2023 at 11:11 PM Conor Dooley <[e-mail]> wrote:
>
> On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > During the early page table creation, we used to set the mapping for
> > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > PAGE_OFFSET is).
> >
> > But actually we don't have to establish this mapping (ie set va_pa_offset)
> > that early in the boot process because:
> >
> > - first, setup_vm installs a temporary kernel mapping and among other
> >   things, discovers the system memory,
> > - then, setup_vm_final creates the final kernel mapping and takes
> >   advantage of the discovered system memory to create the linear
> >   mapping.
> >
> > During the first phase, we don't know the start of the system memory and
> > then until the second phase is finished, we can't use the linear mapping at
> > all and phys_to_virt/virt_to_phys translations must not be used because it
> > would result in a different translation from the 'real' one once the final
> > mapping is installed.
> >
> > So here we simply delay the initialization of va_pa_offset to after the
> > system memory discovery. But to make sure noone uses the linear mapping
> > before, we add some guard in the DEBUG_VIRTUAL config.
> >
> > Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> > in a better TLB utilization.
> >
> > Note that we rely on the firmware to protect itself using PMP.
> >
> > Acked-by: Rob Herring <[e-mail]> # DT bits
> > Signed-off-by: Alexandre Ghiti <[e-mail]>
>
> No good on !MMU unfortunately Alex:
> ../arch/riscv/mm/init.c:222:2: error: use of undeclared identifier 'riscv_pfn_base'
>         riscv_pfn_base = PFN_DOWN(phys_ram_base);
>         ^
>
> Reproduces with nommu_virt_defconfig.

Thanks, fixed locally, I'll push the v5 soon.

Thanks again,

Alex

>
> Thanks,
> Conor.

Andrew Jones Jan. 25, 2023, 10:41 a.m. UTC | #5

On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > During the early page table creation, we used to set the mapping for
> > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > PAGE_OFFSET is).
> > 
> > But actually we don't have to establish this mapping (ie set va_pa_offset)
> > that early in the boot process because:
> > 
> > - first, setup_vm installs a temporary kernel mapping and among other
> >   things, discovers the system memory,
> > - then, setup_vm_final creates the final kernel mapping and takes
> >   advantage of the discovered system memory to create the linear
> >   mapping.
> > 
> > During the first phase, we don't know the start of the system memory and
> > then until the second phase is finished, we can't use the linear mapping at
> > all and phys_to_virt/virt_to_phys translations must not be used because it
> > would result in a different translation from the 'real' one once the final
> > mapping is installed.
> > 
> > So here we simply delay the initialization of va_pa_offset to after the
> > system memory discovery. But to make sure noone uses the linear mapping
> > before, we add some guard in the DEBUG_VIRTUAL config.
> > 
> > Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> > in a better TLB utilization.
> > 
> > Note that we rely on the firmware to protect itself using PMP.
> > 
> > Acked-by: Rob Herring <[e-mail]> # DT bits
> > Signed-off-by: Alexandre Ghiti <[e-mail]>
> > ---
> > 
> > v4:
> > - Rebase on top of v6.2-rc3, as noted by Conor
> > - Add Acked-by Rob
> > 
> > v3:
> > - Change the comment about initrd_start VA conversion so that it fits
> >   ARM64 and RISCV64 (and others in the future if needed), as suggested
> >   by Rob
> > 
> > v2:
> > - Add a comment on why RISCV64 does not need to set initrd_start/end that
> >   early in the boot process, as asked by Rob
> > 
> >  arch/riscv/include/asm/page.h | 16 ++++++++++++++++
> >  arch/riscv/mm/init.c          | 25 +++++++++++++++++++------
> >  arch/riscv/mm/physaddr.c      | 16 ++++++++++++++++
> >  drivers/of/fdt.c              | 11 ++++++-----
> >  4 files changed, 57 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > index 9f432c1b5289..7fe84c89e572 100644
> > --- a/arch/riscv/include/asm/page.h
> > +++ b/arch/riscv/include/asm/page.h
> > @@ -90,6 +90,14 @@ typedef struct page *pgtable_t;
> >  #define PTE_FMT "%08lx"
> >  #endif
> >  
> > +#ifdef CONFIG_64BIT
> > +/*
> > + * We override this value as its generic definition uses __pa too early in
> > + * the boot process (before kernel_map.va_pa_offset is set).
> > + */
> > +#define MIN_MEMBLOCK_ADDR      0
> > +#endif
> > +
> >  #ifdef CONFIG_MMU
> >  extern unsigned long riscv_pfn_base;
> >  #define ARCH_PFN_OFFSET		(riscv_pfn_base)
> > @@ -122,7 +130,11 @@ extern phys_addr_t phys_ram_base;
> >  #define is_linear_mapping(x)	\
> >  	((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
> >  
> > +#ifndef CONFIG_DEBUG_VIRTUAL
> >  #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
> > +#else
> > +void *linear_mapping_pa_to_va(unsigned long x);
> > +#endif
> >  #define kernel_mapping_pa_to_va(y)	({					\
> >  	unsigned long _y = (unsigned long)(y);					\
> >  	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?			\
> > @@ -131,7 +143,11 @@ extern phys_addr_t phys_ram_base;
> >  	})
> >  #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
> >  
> > +#ifndef CONFIG_DEBUG_VIRTUAL
> >  #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - kernel_map.va_pa_offset)
> > +#else
> > +phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> > +#endif
> >  #define kernel_mapping_va_to_pa(y) ({						\
> >  	unsigned long _y = (unsigned long)(y);					\
> >  	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 478d6763a01a..cc892ba9f787 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -213,6 +213,14 @@ static void __init setup_bootmem(void)
> >  	phys_ram_end = memblock_end_of_DRAM();
> >  	if (!IS_ENABLED(CONFIG_XIP_KERNEL))
> >  		phys_ram_base = memblock_start_of_DRAM();
> > +
> > +	/*
> > +	 * Any use of __va/__pa before this point is wrong as we did not know the
> > +	 * start of DRAM before.
> > +	 */
> > +	kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
> > +	riscv_pfn_base = PFN_DOWN(phys_ram_base);
> > +
> >  	/*
> >  	 * memblock allocator is not aware of the fact that last 4K bytes of
> >  	 * the addressable memory can not be mapped because of IS_ERR_VALUE
> > @@ -671,9 +679,16 @@ void __init create_pgd_mapping(pgd_t *pgdp,
> >  
> >  static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
> >  {
> > -	/* Upgrade to PMD_SIZE mappings whenever possible */
> > -	base &= PMD_SIZE - 1;
> > -	if (!base && size >= PMD_SIZE)
> > +	if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
> > +		return PGDIR_SIZE;
> > +
> > +	if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
> > +		return P4D_SIZE;
> > +
> > +	if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
> > +		return PUD_SIZE;
> > +
> > +	if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
> >  		return PMD_SIZE;
> >  
> >  	return PAGE_SIZE;
> > @@ -982,11 +997,9 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> >  	set_satp_mode();
> >  #endif
> >  
> > -	kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > +	kernel_map.va_pa_offset = 0UL;
> >  	kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
> >  
> > -	riscv_pfn_base = PFN_DOWN(kernel_map.phys_addr);
> > -
> >  	/*
> >  	 * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
> >  	 * kernel, whereas for 64-bit kernel, the end of the virtual address
> > diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
> > index 9b18bda74154..18706f457da7 100644
> > --- a/arch/riscv/mm/physaddr.c
> > +++ b/arch/riscv/mm/physaddr.c
> > @@ -33,3 +33,19 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
> >  	return __va_to_pa_nodebug(x);
> >  }
> >  EXPORT_SYMBOL(__phys_addr_symbol);
> > +
> > +phys_addr_t linear_mapping_va_to_pa(unsigned long x)
> > +{
> > +	BUG_ON(!kernel_map.va_pa_offset);
> > +
> > +	return ((unsigned long)(x) - kernel_map.va_pa_offset);
> > +}
> > +EXPORT_SYMBOL(linear_mapping_va_to_pa);
> > +
> > +void *linear_mapping_pa_to_va(unsigned long x)
> > +{
> > +	BUG_ON(!kernel_map.va_pa_offset);
> > +
> > +	return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
> > +}
> > +EXPORT_SYMBOL(linear_mapping_pa_to_va);
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index f08b25195ae7..58107bd56f8f 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> >  static void __early_init_dt_declare_initrd(unsigned long start,
> >  					   unsigned long end)
> >  {
> > -	/* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > -	 * enabled since __va() is called too early. ARM64 does make use
> > -	 * of phys_initrd_start/phys_initrd_size so we can skip this
> > -	 * conversion.
> > +	/*
> > +	 * __va() is not yet available this early on some platforms. In that
> > +	 * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > +	 * and does the VA conversion itself.
> >  	 */
> > -	if (!IS_ENABLED(CONFIG_ARM64)) {
> > +	if (!IS_ENABLED(CONFIG_ARM64) &&
> > +	    !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> 
> There are now two architectures, so maybe it's time for a new config
> symbol which would be selected by arm64 and riscv64 and then used here,
> e.g.
> 
>   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {

I see v5 left this as it was. Any comment on this suggestion?

Thanks,
drew

> 
> >  		initrd_start = (unsigned long)__va(start);
> >  		initrd_end = (unsigned long)__va(end);
> >  		initrd_below_start_ok = 1;
> > -- 
> > 2.37.2
> >
> 
> Otherwise,
> 
> Reviewed-by: Andrew Jones <[e-mail]>
> 
> Thanks,
> drew

Alexandre Ghiti Jan. 25, 2023, 12:12 p.m. UTC | #6

On Wed, Jan 25, 2023 at 11:41 AM Andrew Jones <[e-mail]> wrote:
>
> On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> > On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > > During the early page table creation, we used to set the mapping for
> > > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > > PAGE_OFFSET is).
> > >
> > > But actually we don't have to establish this mapping (ie set va_pa_offset)
> > > that early in the boot process because:
> > >
> > > - first, setup_vm installs a temporary kernel mapping and among other
> > >   things, discovers the system memory,
> > > - then, setup_vm_final creates the final kernel mapping and takes
> > >   advantage of the discovered system memory to create the linear
> > >   mapping.
> > >
> > > During the first phase, we don't know the start of the system memory and
> > > then until the second phase is finished, we can't use the linear mapping at
> > > all and phys_to_virt/virt_to_phys translations must not be used because it
> > > would result in a different translation from the 'real' one once the final
> > > mapping is installed.
> > >
> > > So here we simply delay the initialization of va_pa_offset to after the
> > > system memory discovery. But to make sure noone uses the linear mapping
> > > before, we add some guard in the DEBUG_VIRTUAL config.
> > >
> > > Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> > > in a better TLB utilization.
> > >
> > > Note that we rely on the firmware to protect itself using PMP.
> > >
> > > Acked-by: Rob Herring <[e-mail]> # DT bits
> > > Signed-off-by: Alexandre Ghiti <[e-mail]>
> > > ---
> > >
> > > v4:
> > > - Rebase on top of v6.2-rc3, as noted by Conor
> > > - Add Acked-by Rob
> > >
> > > v3:
> > > - Change the comment about initrd_start VA conversion so that it fits
> > >   ARM64 and RISCV64 (and others in the future if needed), as suggested
> > >   by Rob
> > >
> > > v2:
> > > - Add a comment on why RISCV64 does not need to set initrd_start/end that
> > >   early in the boot process, as asked by Rob
> > >
> > >  arch/riscv/include/asm/page.h | 16 ++++++++++++++++
> > >  arch/riscv/mm/init.c          | 25 +++++++++++++++++++------
> > >  arch/riscv/mm/physaddr.c      | 16 ++++++++++++++++
> > >  drivers/of/fdt.c              | 11 ++++++-----
> > >  4 files changed, 57 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > index 9f432c1b5289..7fe84c89e572 100644
> > > --- a/arch/riscv/include/asm/page.h
> > > +++ b/arch/riscv/include/asm/page.h
> > > @@ -90,6 +90,14 @@ typedef struct page *pgtable_t;
> > >  #define PTE_FMT "%08lx"
> > >  #endif
> > >
> > > +#ifdef CONFIG_64BIT
> > > +/*
> > > + * We override this value as its generic definition uses __pa too early in
> > > + * the boot process (before kernel_map.va_pa_offset is set).
> > > + */
> > > +#define MIN_MEMBLOCK_ADDR      0
> > > +#endif
> > > +
> > >  #ifdef CONFIG_MMU
> > >  extern unsigned long riscv_pfn_base;
> > >  #define ARCH_PFN_OFFSET            (riscv_pfn_base)
> > > @@ -122,7 +130,11 @@ extern phys_addr_t phys_ram_base;
> > >  #define is_linear_mapping(x)       \
> > >     ((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
> > >
> > > +#ifndef CONFIG_DEBUG_VIRTUAL
> > >  #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
> > > +#else
> > > +void *linear_mapping_pa_to_va(unsigned long x);
> > > +#endif
> > >  #define kernel_mapping_pa_to_va(y) ({                                      \
> > >     unsigned long _y = (unsigned long)(y);                                  \
> > >     (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?                 \
> > > @@ -131,7 +143,11 @@ extern phys_addr_t phys_ram_base;
> > >     })
> > >  #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> > >
> > > +#ifndef CONFIG_DEBUG_VIRTUAL
> > >  #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - kernel_map.va_pa_offset)
> > > +#else
> > > +phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> > > +#endif
> > >  #define kernel_mapping_va_to_pa(y) ({                                              \
> > >     unsigned long _y = (unsigned long)(y);                                  \
> > >     (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index 478d6763a01a..cc892ba9f787 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -213,6 +213,14 @@ static void __init setup_bootmem(void)
> > >     phys_ram_end = memblock_end_of_DRAM();
> > >     if (!IS_ENABLED(CONFIG_XIP_KERNEL))
> > >             phys_ram_base = memblock_start_of_DRAM();
> > > +
> > > +   /*
> > > +    * Any use of __va/__pa before this point is wrong as we did not know the
> > > +    * start of DRAM before.
> > > +    */
> > > +   kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
> > > +   riscv_pfn_base = PFN_DOWN(phys_ram_base);
> > > +
> > >     /*
> > >      * memblock allocator is not aware of the fact that last 4K bytes of
> > >      * the addressable memory can not be mapped because of IS_ERR_VALUE
> > > @@ -671,9 +679,16 @@ void __init create_pgd_mapping(pgd_t *pgdp,
> > >
> > >  static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
> > >  {
> > > -   /* Upgrade to PMD_SIZE mappings whenever possible */
> > > -   base &= PMD_SIZE - 1;
> > > -   if (!base && size >= PMD_SIZE)
> > > +   if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
> > > +           return PGDIR_SIZE;
> > > +
> > > +   if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
> > > +           return P4D_SIZE;
> > > +
> > > +   if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
> > > +           return PUD_SIZE;
> > > +
> > > +   if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
> > >             return PMD_SIZE;
> > >
> > >     return PAGE_SIZE;
> > > @@ -982,11 +997,9 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > >     set_satp_mode();
> > >  #endif
> > >
> > > -   kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > > +   kernel_map.va_pa_offset = 0UL;
> > >     kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
> > >
> > > -   riscv_pfn_base = PFN_DOWN(kernel_map.phys_addr);
> > > -
> > >     /*
> > >      * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
> > >      * kernel, whereas for 64-bit kernel, the end of the virtual address
> > > diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
> > > index 9b18bda74154..18706f457da7 100644
> > > --- a/arch/riscv/mm/physaddr.c
> > > +++ b/arch/riscv/mm/physaddr.c
> > > @@ -33,3 +33,19 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
> > >     return __va_to_pa_nodebug(x);
> > >  }
> > >  EXPORT_SYMBOL(__phys_addr_symbol);
> > > +
> > > +phys_addr_t linear_mapping_va_to_pa(unsigned long x)
> > > +{
> > > +   BUG_ON(!kernel_map.va_pa_offset);
> > > +
> > > +   return ((unsigned long)(x) - kernel_map.va_pa_offset);
> > > +}
> > > +EXPORT_SYMBOL(linear_mapping_va_to_pa);
> > > +
> > > +void *linear_mapping_pa_to_va(unsigned long x)
> > > +{
> > > +   BUG_ON(!kernel_map.va_pa_offset);
> > > +
> > > +   return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
> > > +}
> > > +EXPORT_SYMBOL(linear_mapping_pa_to_va);
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index f08b25195ae7..58107bd56f8f 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > >                                        unsigned long end)
> > >  {
> > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > -    * enabled since __va() is called too early. ARM64 does make use
> > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > -    * conversion.
> > > +   /*
> > > +    * __va() is not yet available this early on some platforms. In that
> > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > +    * and does the VA conversion itself.
> > >      */
> > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> >
> > There are now two architectures, so maybe it's time for a new config
> > symbol which would be selected by arm64 and riscv64 and then used here,
> > e.g.
> >
> >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
>
> I see v5 left this as it was. Any comment on this suggestion?

Introducing a config for this only use case sounds excessive to me,
but I'll let Rob decide what he wants to see here.

>
> Thanks,
> drew
>
> >
> > >             initrd_start = (unsigned long)__va(start);
> > >             initrd_end = (unsigned long)__va(end);
> > >             initrd_below_start_ok = 1;
> > > --
> > > 2.37.2
> > >
> >
> > Otherwise,
> >
> > Reviewed-by: Andrew Jones <[e-mail]>
> >
> > Thanks,
> > drew

Andrew Jones Jan. 25, 2023, 3:10 p.m. UTC | #7

On Wed, Jan 25, 2023 at 01:12:49PM +0100, Alexandre Ghiti wrote:
> On Wed, Jan 25, 2023 at 11:41 AM Andrew Jones <[e-mail]> wrote:
> >
> > On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> > > On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > > > During the early page table creation, we used to set the mapping for
> > > > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > > > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > > > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > > > PAGE_OFFSET is).
> > > >
> > > > But actually we don't have to establish this mapping (ie set va_pa_offset)
> > > > that early in the boot process because:
> > > >
> > > > - first, setup_vm installs a temporary kernel mapping and among other
> > > >   things, discovers the system memory,
> > > > - then, setup_vm_final creates the final kernel mapping and takes
> > > >   advantage of the discovered system memory to create the linear
> > > >   mapping.
> > > >
> > > > During the first phase, we don't know the start of the system memory and
> > > > then until the second phase is finished, we can't use the linear mapping at
> > > > all and phys_to_virt/virt_to_phys translations must not be used because it
> > > > would result in a different translation from the 'real' one once the final
> > > > mapping is installed.
> > > >
> > > > So here we simply delay the initialization of va_pa_offset to after the
> > > > system memory discovery. But to make sure noone uses the linear mapping
> > > > before, we add some guard in the DEBUG_VIRTUAL config.
> > > >
> > > > Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> > > > in a better TLB utilization.
> > > >
> > > > Note that we rely on the firmware to protect itself using PMP.
> > > >
> > > > Acked-by: Rob Herring <[e-mail]> # DT bits
> > > > Signed-off-by: Alexandre Ghiti <[e-mail]>
> > > > ---
> > > >
> > > > v4:
> > > > - Rebase on top of v6.2-rc3, as noted by Conor
> > > > - Add Acked-by Rob
> > > >
> > > > v3:
> > > > - Change the comment about initrd_start VA conversion so that it fits
> > > >   ARM64 and RISCV64 (and others in the future if needed), as suggested
> > > >   by Rob
> > > >
> > > > v2:
> > > > - Add a comment on why RISCV64 does not need to set initrd_start/end that
> > > >   early in the boot process, as asked by Rob
> > > >
> > > >  arch/riscv/include/asm/page.h | 16 ++++++++++++++++
> > > >  arch/riscv/mm/init.c          | 25 +++++++++++++++++++------
> > > >  arch/riscv/mm/physaddr.c      | 16 ++++++++++++++++
> > > >  drivers/of/fdt.c              | 11 ++++++-----
> > > >  4 files changed, 57 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > > index 9f432c1b5289..7fe84c89e572 100644
> > > > --- a/arch/riscv/include/asm/page.h
> > > > +++ b/arch/riscv/include/asm/page.h
> > > > @@ -90,6 +90,14 @@ typedef struct page *pgtable_t;
> > > >  #define PTE_FMT "%08lx"
> > > >  #endif
> > > >
> > > > +#ifdef CONFIG_64BIT
> > > > +/*
> > > > + * We override this value as its generic definition uses __pa too early in
> > > > + * the boot process (before kernel_map.va_pa_offset is set).
> > > > + */
> > > > +#define MIN_MEMBLOCK_ADDR      0
> > > > +#endif
> > > > +
> > > >  #ifdef CONFIG_MMU
> > > >  extern unsigned long riscv_pfn_base;
> > > >  #define ARCH_PFN_OFFSET            (riscv_pfn_base)
> > > > @@ -122,7 +130,11 @@ extern phys_addr_t phys_ram_base;
> > > >  #define is_linear_mapping(x)       \
> > > >     ((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
> > > >
> > > > +#ifndef CONFIG_DEBUG_VIRTUAL
> > > >  #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
> > > > +#else
> > > > +void *linear_mapping_pa_to_va(unsigned long x);
> > > > +#endif
> > > >  #define kernel_mapping_pa_to_va(y) ({                                      \
> > > >     unsigned long _y = (unsigned long)(y);                                  \
> > > >     (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?                 \
> > > > @@ -131,7 +143,11 @@ extern phys_addr_t phys_ram_base;
> > > >     })
> > > >  #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> > > >
> > > > +#ifndef CONFIG_DEBUG_VIRTUAL
> > > >  #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - kernel_map.va_pa_offset)
> > > > +#else
> > > > +phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> > > > +#endif
> > > >  #define kernel_mapping_va_to_pa(y) ({                                              \
> > > >     unsigned long _y = (unsigned long)(y);                                  \
> > > >     (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > index 478d6763a01a..cc892ba9f787 100644
> > > > --- a/arch/riscv/mm/init.c
> > > > +++ b/arch/riscv/mm/init.c
> > > > @@ -213,6 +213,14 @@ static void __init setup_bootmem(void)
> > > >     phys_ram_end = memblock_end_of_DRAM();
> > > >     if (!IS_ENABLED(CONFIG_XIP_KERNEL))
> > > >             phys_ram_base = memblock_start_of_DRAM();
> > > > +
> > > > +   /*
> > > > +    * Any use of __va/__pa before this point is wrong as we did not know the
> > > > +    * start of DRAM before.
> > > > +    */
> > > > +   kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
> > > > +   riscv_pfn_base = PFN_DOWN(phys_ram_base);
> > > > +
> > > >     /*
> > > >      * memblock allocator is not aware of the fact that last 4K bytes of
> > > >      * the addressable memory can not be mapped because of IS_ERR_VALUE
> > > > @@ -671,9 +679,16 @@ void __init create_pgd_mapping(pgd_t *pgdp,
> > > >
> > > >  static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
> > > >  {
> > > > -   /* Upgrade to PMD_SIZE mappings whenever possible */
> > > > -   base &= PMD_SIZE - 1;
> > > > -   if (!base && size >= PMD_SIZE)
> > > > +   if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
> > > > +           return PGDIR_SIZE;
> > > > +
> > > > +   if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
> > > > +           return P4D_SIZE;
> > > > +
> > > > +   if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
> > > > +           return PUD_SIZE;
> > > > +
> > > > +   if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
> > > >             return PMD_SIZE;
> > > >
> > > >     return PAGE_SIZE;
> > > > @@ -982,11 +997,9 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > > >     set_satp_mode();
> > > >  #endif
> > > >
> > > > -   kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > > > +   kernel_map.va_pa_offset = 0UL;
> > > >     kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
> > > >
> > > > -   riscv_pfn_base = PFN_DOWN(kernel_map.phys_addr);
> > > > -
> > > >     /*
> > > >      * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
> > > >      * kernel, whereas for 64-bit kernel, the end of the virtual address
> > > > diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
> > > > index 9b18bda74154..18706f457da7 100644
> > > > --- a/arch/riscv/mm/physaddr.c
> > > > +++ b/arch/riscv/mm/physaddr.c
> > > > @@ -33,3 +33,19 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
> > > >     return __va_to_pa_nodebug(x);
> > > >  }
> > > >  EXPORT_SYMBOL(__phys_addr_symbol);
> > > > +
> > > > +phys_addr_t linear_mapping_va_to_pa(unsigned long x)
> > > > +{
> > > > +   BUG_ON(!kernel_map.va_pa_offset);
> > > > +
> > > > +   return ((unsigned long)(x) - kernel_map.va_pa_offset);
> > > > +}
> > > > +EXPORT_SYMBOL(linear_mapping_va_to_pa);
> > > > +
> > > > +void *linear_mapping_pa_to_va(unsigned long x)
> > > > +{
> > > > +   BUG_ON(!kernel_map.va_pa_offset);
> > > > +
> > > > +   return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
> > > > +}
> > > > +EXPORT_SYMBOL(linear_mapping_pa_to_va);
> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index f08b25195ae7..58107bd56f8f 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > >                                        unsigned long end)
> > > >  {
> > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > -    * conversion.
> > > > +   /*
> > > > +    * __va() is not yet available this early on some platforms. In that
> > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > +    * and does the VA conversion itself.
> > > >      */
> > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > >
> > > There are now two architectures, so maybe it's time for a new config
> > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > e.g.
> > >
> > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> >
> > I see v5 left this as it was. Any comment on this suggestion?
> 
> Introducing a config for this only use case sounds excessive to me,
> but I'll let Rob decide what he wants to see here.

To me, the suggestion is less about trying to tidy up DT code and more
about bringing this comment about arm64 and riscv64 not being able to
use the linear map as early as other architectures up out of the
depths of DT code. Seeing an architecture select something like
NO_EARLY_LINEAR_MAP, which has a paragraph explaining what that
means, may help avoid other early uses of __va() which may or may
not fail quickly and cleanly with a BUG.

Thanks,
drew

Alexandre Ghiti Jan. 27, 2023, 8:45 a.m. UTC | #8

Hey Andrew,

Sorry about the delay.

On Wed, Jan 25, 2023 at 4:10 PM Andrew Jones <[e-mail]> wrote:
>
> On Wed, Jan 25, 2023 at 01:12:49PM +0100, Alexandre Ghiti wrote:
> > On Wed, Jan 25, 2023 at 11:41 AM Andrew Jones <[e-mail]> wrote:
> > >
> > > On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> > > > On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > > > > During the early page table creation, we used to set the mapping for
> > > > > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > > > > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > > > > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > > > > PAGE_OFFSET is).
> > > > >
> > > > > But actually we don't have to establish this mapping (ie set va_pa_offset)
> > > > > that early in the boot process because:
> > > > >
> > > > > - first, setup_vm installs a temporary kernel mapping and among other
> > > > >   things, discovers the system memory,
> > > > > - then, setup_vm_final creates the final kernel mapping and takes
> > > > >   advantage of the discovered system memory to create the linear
> > > > >   mapping.
> > > > >
> > > > > During the first phase, we don't know the start of the system memory and
> > > > > then until the second phase is finished, we can't use the linear mapping at
> > > > > all and phys_to_virt/virt_to_phys translations must not be used because it
> > > > > would result in a different translation from the 'real' one once the final
> > > > > mapping is installed.
> > > > >
> > > > > So here we simply delay the initialization of va_pa_offset to after the
> > > > > system memory discovery. But to make sure noone uses the linear mapping
> > > > > before, we add some guard in the DEBUG_VIRTUAL config.
> > > > >
> > > > > Finally we can use PUD/P4D/PGD hugepages when possible, which will result
> > > > > in a better TLB utilization.
> > > > >
> > > > > Note that we rely on the firmware to protect itself using PMP.
> > > > >
> > > > > Acked-by: Rob Herring <[e-mail]> # DT bits
> > > > > Signed-off-by: Alexandre Ghiti <[e-mail]>
> > > > > ---
> > > > >
> > > > > v4:
> > > > > - Rebase on top of v6.2-rc3, as noted by Conor
> > > > > - Add Acked-by Rob
> > > > >
> > > > > v3:
> > > > > - Change the comment about initrd_start VA conversion so that it fits
> > > > >   ARM64 and RISCV64 (and others in the future if needed), as suggested
> > > > >   by Rob
> > > > >
> > > > > v2:
> > > > > - Add a comment on why RISCV64 does not need to set initrd_start/end that
> > > > >   early in the boot process, as asked by Rob
> > > > >
> > > > >  arch/riscv/include/asm/page.h | 16 ++++++++++++++++
> > > > >  arch/riscv/mm/init.c          | 25 +++++++++++++++++++------
> > > > >  arch/riscv/mm/physaddr.c      | 16 ++++++++++++++++
> > > > >  drivers/of/fdt.c              | 11 ++++++-----
> > > > >  4 files changed, 57 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > > > index 9f432c1b5289..7fe84c89e572 100644
> > > > > --- a/arch/riscv/include/asm/page.h
> > > > > +++ b/arch/riscv/include/asm/page.h
> > > > > @@ -90,6 +90,14 @@ typedef struct page *pgtable_t;
> > > > >  #define PTE_FMT "%08lx"
> > > > >  #endif
> > > > >
> > > > > +#ifdef CONFIG_64BIT
> > > > > +/*
> > > > > + * We override this value as its generic definition uses __pa too early in
> > > > > + * the boot process (before kernel_map.va_pa_offset is set).
> > > > > + */
> > > > > +#define MIN_MEMBLOCK_ADDR      0
> > > > > +#endif
> > > > > +
> > > > >  #ifdef CONFIG_MMU
> > > > >  extern unsigned long riscv_pfn_base;
> > > > >  #define ARCH_PFN_OFFSET            (riscv_pfn_base)
> > > > > @@ -122,7 +130,11 @@ extern phys_addr_t phys_ram_base;
> > > > >  #define is_linear_mapping(x)       \
> > > > >     ((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
> > > > >
> > > > > +#ifndef CONFIG_DEBUG_VIRTUAL
> > > > >  #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
> > > > > +#else
> > > > > +void *linear_mapping_pa_to_va(unsigned long x);
> > > > > +#endif
> > > > >  #define kernel_mapping_pa_to_va(y) ({                                      \
> > > > >     unsigned long _y = (unsigned long)(y);                                  \
> > > > >     (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?                 \
> > > > > @@ -131,7 +143,11 @@ extern phys_addr_t phys_ram_base;
> > > > >     })
> > > > >  #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> > > > >
> > > > > +#ifndef CONFIG_DEBUG_VIRTUAL
> > > > >  #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - kernel_map.va_pa_offset)
> > > > > +#else
> > > > > +phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> > > > > +#endif
> > > > >  #define kernel_mapping_va_to_pa(y) ({                                              \
> > > > >     unsigned long _y = (unsigned long)(y);                                  \
> > > > >     (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > index 478d6763a01a..cc892ba9f787 100644
> > > > > --- a/arch/riscv/mm/init.c
> > > > > +++ b/arch/riscv/mm/init.c
> > > > > @@ -213,6 +213,14 @@ static void __init setup_bootmem(void)
> > > > >     phys_ram_end = memblock_end_of_DRAM();
> > > > >     if (!IS_ENABLED(CONFIG_XIP_KERNEL))
> > > > >             phys_ram_base = memblock_start_of_DRAM();
> > > > > +
> > > > > +   /*
> > > > > +    * Any use of __va/__pa before this point is wrong as we did not know the
> > > > > +    * start of DRAM before.
> > > > > +    */
> > > > > +   kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
> > > > > +   riscv_pfn_base = PFN_DOWN(phys_ram_base);
> > > > > +
> > > > >     /*
> > > > >      * memblock allocator is not aware of the fact that last 4K bytes of
> > > > >      * the addressable memory can not be mapped because of IS_ERR_VALUE
> > > > > @@ -671,9 +679,16 @@ void __init create_pgd_mapping(pgd_t *pgdp,
> > > > >
> > > > >  static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
> > > > >  {
> > > > > -   /* Upgrade to PMD_SIZE mappings whenever possible */
> > > > > -   base &= PMD_SIZE - 1;
> > > > > -   if (!base && size >= PMD_SIZE)
> > > > > +   if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
> > > > > +           return PGDIR_SIZE;
> > > > > +
> > > > > +   if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
> > > > > +           return P4D_SIZE;
> > > > > +
> > > > > +   if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
> > > > > +           return PUD_SIZE;
> > > > > +
> > > > > +   if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
> > > > >             return PMD_SIZE;
> > > > >
> > > > >     return PAGE_SIZE;
> > > > > @@ -982,11 +997,9 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > > > >     set_satp_mode();
> > > > >  #endif
> > > > >
> > > > > -   kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > > > > +   kernel_map.va_pa_offset = 0UL;
> > > > >     kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
> > > > >
> > > > > -   riscv_pfn_base = PFN_DOWN(kernel_map.phys_addr);
> > > > > -
> > > > >     /*
> > > > >      * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
> > > > >      * kernel, whereas for 64-bit kernel, the end of the virtual address
> > > > > diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
> > > > > index 9b18bda74154..18706f457da7 100644
> > > > > --- a/arch/riscv/mm/physaddr.c
> > > > > +++ b/arch/riscv/mm/physaddr.c
> > > > > @@ -33,3 +33,19 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
> > > > >     return __va_to_pa_nodebug(x);
> > > > >  }
> > > > >  EXPORT_SYMBOL(__phys_addr_symbol);
> > > > > +
> > > > > +phys_addr_t linear_mapping_va_to_pa(unsigned long x)
> > > > > +{
> > > > > +   BUG_ON(!kernel_map.va_pa_offset);
> > > > > +
> > > > > +   return ((unsigned long)(x) - kernel_map.va_pa_offset);
> > > > > +}
> > > > > +EXPORT_SYMBOL(linear_mapping_va_to_pa);
> > > > > +
> > > > > +void *linear_mapping_pa_to_va(unsigned long x)
> > > > > +{
> > > > > +   BUG_ON(!kernel_map.va_pa_offset);
> > > > > +
> > > > > +   return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
> > > > > +}
> > > > > +EXPORT_SYMBOL(linear_mapping_pa_to_va);
> > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > index f08b25195ae7..58107bd56f8f 100644
> > > > > --- a/drivers/of/fdt.c
> > > > > +++ b/drivers/of/fdt.c
> > > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > > >                                        unsigned long end)
> > > > >  {
> > > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > > -    * conversion.
> > > > > +   /*
> > > > > +    * __va() is not yet available this early on some platforms. In that
> > > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > > +    * and does the VA conversion itself.
> > > > >      */
> > > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > > >
> > > > There are now two architectures, so maybe it's time for a new config
> > > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > > e.g.
> > > >
> > > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> > >
> > > I see v5 left this as it was. Any comment on this suggestion?
> >
> > Introducing a config for this only use case sounds excessive to me,
> > but I'll let Rob decide what he wants to see here.
>
> To me, the suggestion is less about trying to tidy up DT code and more
> about bringing this comment about arm64 and riscv64 not being able to
> use the linear map as early as other architectures up out of the
> depths of DT code. Seeing an architecture select something like
> NO_EARLY_LINEAR_MAP, which has a paragraph explaining what that
> means, may help avoid other early uses of __va() which may or may
> not fail quickly and cleanly with a BUG.
>

You're right, do you have some bandwidth for doing that?

Thanks,

Alex

> Thanks,
> drew

Andrew Jones Jan. 27, 2023, 8:58 a.m. UTC | #9

On Fri, Jan 27, 2023 at 09:45:21AM +0100, Alexandre Ghiti wrote:
...
> > > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > > index f08b25195ae7..58107bd56f8f 100644
> > > > > > --- a/drivers/of/fdt.c
> > > > > > +++ b/drivers/of/fdt.c
> > > > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > > > >                                        unsigned long end)
> > > > > >  {
> > > > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > > > -    * conversion.
> > > > > > +   /*
> > > > > > +    * __va() is not yet available this early on some platforms. In that
> > > > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > > > +    * and does the VA conversion itself.
> > > > > >      */
> > > > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > > > >
> > > > > There are now two architectures, so maybe it's time for a new config
> > > > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > > > e.g.
> > > > >
> > > > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> > > >
> > > > I see v5 left this as it was. Any comment on this suggestion?
> > >
> > > Introducing a config for this only use case sounds excessive to me,
> > > but I'll let Rob decide what he wants to see here.
> >
> > To me, the suggestion is less about trying to tidy up DT code and more
> > about bringing this comment about arm64 and riscv64 not being able to
> > use the linear map as early as other architectures up out of the
> > depths of DT code. Seeing an architecture select something like
> > NO_EARLY_LINEAR_MAP, which has a paragraph explaining what that
> > means, may help avoid other early uses of __va() which may or may
> > not fail quickly and cleanly with a BUG.
> >
> 
> You're right, do you have some bandwidth for doing that?
>

Sure, I'll post something today.

Thanks,
drew

Andrew Jones Jan. 28, 2023, 10:13 a.m. UTC | #10

On Fri, Jan 27, 2023 at 09:58:03AM +0100, Andrew Jones wrote:
> On Fri, Jan 27, 2023 at 09:45:21AM +0100, Alexandre Ghiti wrote:
> ...
> > > > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > > > index f08b25195ae7..58107bd56f8f 100644
> > > > > > > --- a/drivers/of/fdt.c
> > > > > > > +++ b/drivers/of/fdt.c
> > > > > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > > > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > > > > >                                        unsigned long end)
> > > > > > >  {
> > > > > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > > > > -    * conversion.
> > > > > > > +   /*
> > > > > > > +    * __va() is not yet available this early on some platforms. In that
> > > > > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > > > > +    * and does the VA conversion itself.
> > > > > > >      */
> > > > > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > > > > >
> > > > > > There are now two architectures, so maybe it's time for a new config
> > > > > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > > > > e.g.
> > > > > >
> > > > > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> > > > >
> > > > > I see v5 left this as it was. Any comment on this suggestion?
> > > >
> > > > Introducing a config for this only use case sounds excessive to me,
> > > > but I'll let Rob decide what he wants to see here.
> > >
> > > To me, the suggestion is less about trying to tidy up DT code and more
> > > about bringing this comment about arm64 and riscv64 not being able to
> > > use the linear map as early as other architectures up out of the
> > > depths of DT code. Seeing an architecture select something like
> > > NO_EARLY_LINEAR_MAP, which has a paragraph explaining what that
> > > means, may help avoid other early uses of __va() which may or may
> > > not fail quickly and cleanly with a BUG.
> > >
> > 
> > You're right, do you have some bandwidth for doing that?
> >
> 
> Sure, I'll post something today.
>

Hi Alex,

So on second thought, and after a bunch of grepping, I don't think we need
to try and give architectures a way to declare that they have incomplete
early linear maps. It would actually be difficult to determine when and
why it should be selected, and, afaict, this is currently the one and
only place it would be helpful. So, rather than pulling this comment about
early __va() all the way up to the Kconfig level, I think pulling it into
arch-specific code should be sufficient, and actually better.

The situation we have here is that OF code is providing a convenience
by doing early setup of initrd_start and initrd_end, which is nice for the
large majority of architectures, but, as we see, it can't be used by all.
My new suggestion is that we expose this initrd setup function as a weak
function which architectures may override if needed. If those
architectures want to prepare a mapping in their function, they can, or,
if they know it's safe to defer the setup until later, then they can just
provide a stub. I've drafted a patch where arm64 just provides a stub.
I'll post it soon.

Thanks,
drew

Rob Herring Jan. 30, 2023, 1:48 p.m. UTC | #11

On Wed, Jan 25, 2023 at 6:13 AM Alexandre Ghiti <[e-mail]> wrote:
>
> On Wed, Jan 25, 2023 at 11:41 AM Andrew Jones <[e-mail]> wrote:
> >
> > On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> > > On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > > > During the early page table creation, we used to set the mapping for
> > > > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > > > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > > > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > > > PAGE_OFFSET is).

[...]

> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index f08b25195ae7..58107bd56f8f 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > >                                        unsigned long end)
> > > >  {
> > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > -    * conversion.
> > > > +   /*
> > > > +    * __va() is not yet available this early on some platforms. In that
> > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > +    * and does the VA conversion itself.
> > > >      */
> > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > >
> > > There are now two architectures, so maybe it's time for a new config
> > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > e.g.
> > >
> > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> >
> > I see v5 left this as it was. Any comment on this suggestion?
>
> Introducing a config for this only use case sounds excessive to me,
> but I'll let Rob decide what he wants to see here.

Agreed. Can we just keep it as is here.

> > > >             initrd_start = (unsigned long)__va(start);
> > > >             initrd_end = (unsigned long)__va(end);

I think long term, we should just get rid of needing to do this part
in the DT code and let the initrd code do this.

Rob

Andrew Jones Jan. 30, 2023, 2:19 p.m. UTC | #12

On Mon, Jan 30, 2023 at 07:48:04AM -0600, Rob Herring wrote:
> On Wed, Jan 25, 2023 at 6:13 AM Alexandre Ghiti <[e-mail]> wrote:
> >
> > On Wed, Jan 25, 2023 at 11:41 AM Andrew Jones <[e-mail]> wrote:
> > >
> > > On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> > > > On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > > > > During the early page table creation, we used to set the mapping for
> > > > > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > > > > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > > > > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > > > > PAGE_OFFSET is).
> 
> [...]
> 
> > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > index f08b25195ae7..58107bd56f8f 100644
> > > > > --- a/drivers/of/fdt.c
> > > > > +++ b/drivers/of/fdt.c
> > > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > > >                                        unsigned long end)
> > > > >  {
> > > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > > -    * conversion.
> > > > > +   /*
> > > > > +    * __va() is not yet available this early on some platforms. In that
> > > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > > +    * and does the VA conversion itself.
> > > > >      */
> > > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > > >
> > > > There are now two architectures, so maybe it's time for a new config
> > > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > > e.g.
> > > >
> > > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> > >
> > > I see v5 left this as it was. Any comment on this suggestion?
> >
> > Introducing a config for this only use case sounds excessive to me,
> > but I'll let Rob decide what he wants to see here.
> 
> Agreed. Can we just keep it as is here.
> 
> > > > >             initrd_start = (unsigned long)__va(start);
> > > > >             initrd_end = (unsigned long)__va(end);
> 
> I think long term, we should just get rid of needing to do this part
> in the DT code and let the initrd code do this.

initrd code provides reserve_initrd_mem() for this and riscv calls
it later on. afaict, this early setting in OF code is a convenience
which architectures could be taught not to depend on, and then it
could be removed. But, until then, some architectures will need to
avoid it. As I commented downthread, I also don't want to go with
a config anymore, but it'd be nice to keep arch-specifics out of
here, so I've posted a patch changing __early_init_dt_declare_initrd
to be a weak function.

Thanks,
drew

Rob Herring Jan. 30, 2023, 2:57 p.m. UTC | #13

On Mon, Jan 30, 2023 at 8:19 AM Andrew Jones <[e-mail]> wrote:
>
> On Mon, Jan 30, 2023 at 07:48:04AM -0600, Rob Herring wrote:
> > On Wed, Jan 25, 2023 at 6:13 AM Alexandre Ghiti <[e-mail]> wrote:
> > >
> > > On Wed, Jan 25, 2023 at 11:41 AM Andrew Jones <[e-mail]> wrote:
> > > >
> > > > On Mon, Jan 23, 2023 at 03:25:54PM +0100, Andrew Jones wrote:
> > > > > On Mon, Jan 23, 2023 at 12:28:02PM +0100, Alexandre Ghiti wrote:
> > > > > > During the early page table creation, we used to set the mapping for
> > > > > > PAGE_OFFSET to the kernel load address: but the kernel load address is
> > > > > > always offseted by PMD_SIZE which makes it impossible to use PUD/P4D/PGD
> > > > > > pages as this physical address is not aligned on PUD/P4D/PGD size (whereas
> > > > > > PAGE_OFFSET is).
> >
> > [...]
> >
> > > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > > index f08b25195ae7..58107bd56f8f 100644
> > > > > > --- a/drivers/of/fdt.c
> > > > > > +++ b/drivers/of/fdt.c
> > > > > > @@ -891,12 +891,13 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
> > > > > >  static void __early_init_dt_declare_initrd(unsigned long start,
> > > > > >                                        unsigned long end)
> > > > > >  {
> > > > > > -   /* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
> > > > > > -    * enabled since __va() is called too early. ARM64 does make use
> > > > > > -    * of phys_initrd_start/phys_initrd_size so we can skip this
> > > > > > -    * conversion.
> > > > > > +   /*
> > > > > > +    * __va() is not yet available this early on some platforms. In that
> > > > > > +    * case, the platform uses phys_initrd_start/phys_initrd_size instead
> > > > > > +    * and does the VA conversion itself.
> > > > > >      */
> > > > > > -   if (!IS_ENABLED(CONFIG_ARM64)) {
> > > > > > +   if (!IS_ENABLED(CONFIG_ARM64) &&
> > > > > > +       !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
> > > > >
> > > > > There are now two architectures, so maybe it's time for a new config
> > > > > symbol which would be selected by arm64 and riscv64 and then used here,
> > > > > e.g.
> > > > >
> > > > >   if (!IS_ENABLED(CONFIG_NO_EARLY_LINEAR_MAP)) {
> > > >
> > > > I see v5 left this as it was. Any comment on this suggestion?
> > >
> > > Introducing a config for this only use case sounds excessive to me,
> > > but I'll let Rob decide what he wants to see here.
> >
> > Agreed. Can we just keep it as is here.
> >
> > > > > >             initrd_start = (unsigned long)__va(start);
> > > > > >             initrd_end = (unsigned long)__va(end);
> >
> > I think long term, we should just get rid of needing to do this part
> > in the DT code and let the initrd code do this.
>
> initrd code provides reserve_initrd_mem() for this and riscv calls
> it later on. afaict, this early setting in OF code is a convenience
> which architectures could be taught not to depend on, and then it
> could be removed. But, until then, some architectures will need to
> avoid it. As I commented downthread, I also don't want to go with
> a config anymore, but it'd be nice to keep arch-specifics out of
> here, so I've posted a patch changing __early_init_dt_declare_initrd
> to be a weak function.

If this was *always* going to be architecture specific, then I'd
agree. But I think there are better paths to refactor this. I don't
want to go back to a weak function and encourage more implementations
of __early_init_dt_declare_initrd().

Rob

Patch

diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index 9f432c1b5289..7fe84c89e572 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -90,6 +90,14 @@  typedef struct page *pgtable_t;
 #define PTE_FMT "%08lx"
 #endif
 
+#ifdef CONFIG_64BIT
+/*
+ * We override this value as its generic definition uses __pa too early in
+ * the boot process (before kernel_map.va_pa_offset is set).
+ */
+#define MIN_MEMBLOCK_ADDR      0
+#endif
+
 #ifdef CONFIG_MMU
 extern unsigned long riscv_pfn_base;
 #define ARCH_PFN_OFFSET		(riscv_pfn_base)
@@ -122,7 +130,11 @@  extern phys_addr_t phys_ram_base;
 #define is_linear_mapping(x)	\
 	((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
 
+#ifndef CONFIG_DEBUG_VIRTUAL
 #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
+#else
+void *linear_mapping_pa_to_va(unsigned long x);
+#endif
 #define kernel_mapping_pa_to_va(y)	({					\
 	unsigned long _y = (unsigned long)(y);					\
 	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?			\
@@ -131,7 +143,11 @@  extern phys_addr_t phys_ram_base;
 	})
 #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
 
+#ifndef CONFIG_DEBUG_VIRTUAL
 #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - kernel_map.va_pa_offset)
+#else
+phys_addr_t linear_mapping_va_to_pa(unsigned long x);
+#endif
 #define kernel_mapping_va_to_pa(y) ({						\
 	unsigned long _y = (unsigned long)(y);					\
 	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 478d6763a01a..cc892ba9f787 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -213,6 +213,14 @@  static void __init setup_bootmem(void)
 	phys_ram_end = memblock_end_of_DRAM();
 	if (!IS_ENABLED(CONFIG_XIP_KERNEL))
 		phys_ram_base = memblock_start_of_DRAM();
+
+	/*
+	 * Any use of __va/__pa before this point is wrong as we did not know the
+	 * start of DRAM before.
+	 */
+	kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
+	riscv_pfn_base = PFN_DOWN(phys_ram_base);
+
 	/*
 	 * memblock allocator is not aware of the fact that last 4K bytes of
 	 * the addressable memory can not be mapped because of IS_ERR_VALUE
@@ -671,9 +679,16 @@  void __init create_pgd_mapping(pgd_t *pgdp,
 
 static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
 {
-	/* Upgrade to PMD_SIZE mappings whenever possible */
-	base &= PMD_SIZE - 1;
-	if (!base && size >= PMD_SIZE)
+	if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
+		return PGDIR_SIZE;
+
+	if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
+		return P4D_SIZE;
+
+	if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
+		return PUD_SIZE;
+
+	if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
 		return PMD_SIZE;
 
 	return PAGE_SIZE;
@@ -982,11 +997,9 @@  asmlinkage void __init setup_vm(uintptr_t dtb_pa)
 	set_satp_mode();
 #endif
 
-	kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
+	kernel_map.va_pa_offset = 0UL;
 	kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
 
-	riscv_pfn_base = PFN_DOWN(kernel_map.phys_addr);
-
 	/*
 	 * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
 	 * kernel, whereas for 64-bit kernel, the end of the virtual address
diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
index 9b18bda74154..18706f457da7 100644
--- a/arch/riscv/mm/physaddr.c
+++ b/arch/riscv/mm/physaddr.c
@@ -33,3 +33,19 @@  phys_addr_t __phys_addr_symbol(unsigned long x)
 	return __va_to_pa_nodebug(x);
 }
 EXPORT_SYMBOL(__phys_addr_symbol);
+
+phys_addr_t linear_mapping_va_to_pa(unsigned long x)
+{
+	BUG_ON(!kernel_map.va_pa_offset);
+
+	return ((unsigned long)(x) - kernel_map.va_pa_offset);
+}
+EXPORT_SYMBOL(linear_mapping_va_to_pa);
+
+void *linear_mapping_pa_to_va(unsigned long x)
+{
+	BUG_ON(!kernel_map.va_pa_offset);
+
+	return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
+}
+EXPORT_SYMBOL(linear_mapping_pa_to_va);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index f08b25195ae7..58107bd56f8f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -891,12 +891,13 @@  const void * __init of_flat_dt_match_machine(const void *default_match,
 static void __early_init_dt_declare_initrd(unsigned long start,
 					   unsigned long end)
 {
-	/* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
-	 * enabled since __va() is called too early. ARM64 does make use
-	 * of phys_initrd_start/phys_initrd_size so we can skip this
-	 * conversion.
+	/*
+	 * __va() is not yet available this early on some platforms. In that
+	 * case, the platform uses phys_initrd_start/phys_initrd_size instead
+	 * and does the VA conversion itself.
 	 */
-	if (!IS_ENABLED(CONFIG_ARM64)) {
+	if (!IS_ENABLED(CONFIG_ARM64) &&
+	    !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
 		initrd_start = (unsigned long)__va(start);
 		initrd_end = (unsigned long)__va(end);
 		initrd_below_start_ok = 1;