[PATCH -fixes v2 0/3] Fixes for dtb mapping

[PATCH -fixes v2 0/3] Fixes for dtb mapping

[Alexandre Ghiti]

We used to map the dtb differently between early_pg_dir and
swapper_pg_dir which caused issues when we referenced addresses from
the early mapping with swapper_pg_dir (reserved_mem): move the dtb mapping
to the fixmap region in patch 1, which allows to simplify dtb handling in
patch 2.

base-commit-tag: v6.3-rc3

Changes in v2:
  * No functional changes!
  * Add RB/TB from Conor in patch 1
  * Split patch 2 into 2 separate patches, as suggested by Conor
  * Fix the changelog, add Fixes tag, as suggested by Conor

Alexandre Ghiti (3):
  riscv: Move early dtb mapping into the fixmap region
  riscv: Do not set initial_boot_params to the linear address of the dtb
  riscv: No need to relocate the dtb as it lies in the fixmap region

 Documentation/riscv/vm-layout.rst |  6 +--
 arch/riscv/include/asm/fixmap.h   |  8 +++
 arch/riscv/include/asm/pgtable.h  |  8 ++-
 arch/riscv/kernel/setup.c         |  6 +--
 arch/riscv/mm/init.c              | 82 ++++++++++++++-----------------
 5 files changed, 54 insertions(+), 56 deletions(-)

-- 
2.37.2

[PATCH -fixes v2 1/3] riscv: Move early dtb mapping into the fixmap region

[Alexandre Ghiti]

riscv establishes 2 virtual mappings:

- early_pg_dir maps the kernel which allows to discover the system
  memory
- swapper_pg_dir installs the final mapping (linear mapping included)

We used to map the dtb in early_pg_dir using DTB_EARLY_BASE_VA, and this
mapping was not carried over in swapper_pg_dir. It happens that
early_init_fdt_scan_reserved_mem() must be called before swapper_pg_dir is
setup otherwise we could allocate reserved memory defined in the dtb.
And this function initializes reserved_mem variable with addresses that
lie in the early_pg_dir dtb mapping: when those addresses are reused
with swapper_pg_dir, this mapping does not exist and then we trap.

The previous "fix" was incorrect as early_init_fdt_scan_reserved_mem()
must be called before swapper_pg_dir is set up otherwise we could
allocate in reserved memory defined in the dtb.

So move the dtb mapping in the fixmap region which is established in
early_pg_dir and handed over to swapper_pg_dir.

Fixes: 922b0375fc93 ("riscv: Fix memblock reservation for device tree blob")
Fixes: 8f3a2b4a96dc ("RISC-V: Move DT mapping outof fixmap")
Fixes: 50e63dd8ed92 ("riscv: fix reserved memory setup")
Reported-by: Conor Dooley <conor.dooley@microchip.com>
Link: https://lore.kernel.org/all/f8e67f82-103d-156c-deb0-d6d6e2756f5e@microchip.com/
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Tested-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/riscv/vm-layout.rst |  6 +--
 arch/riscv/include/asm/fixmap.h   |  8 ++++
 arch/riscv/include/asm/pgtable.h  |  8 +++-
 arch/riscv/kernel/setup.c         |  1 -
 arch/riscv/mm/init.c              | 61 +++++++++++++++++--------------
 5 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/Documentation/riscv/vm-layout.rst b/Documentation/riscv/vm-layout.rst
index 3be44e74ec5d..5462c84f4723 100644
--- a/Documentation/riscv/vm-layout.rst
+++ b/Documentation/riscv/vm-layout.rst
@@ -47,7 +47,7 @@ RISC-V Linux Kernel SV39
                                                               | Kernel-space virtual memory, shared between all processes:
   ____________________________________________________________|___________________________________________________________
                     |            |                  |         |
-   ffffffc6fee00000 | -228    GB | ffffffc6feffffff |    2 MB | fixmap
+   ffffffc6fea00000 | -228    GB | ffffffc6feffffff |    6 MB | fixmap
    ffffffc6ff000000 | -228    GB | ffffffc6ffffffff |   16 MB | PCI io
    ffffffc700000000 | -228    GB | ffffffc7ffffffff |    4 GB | vmemmap
    ffffffc800000000 | -224    GB | ffffffd7ffffffff |   64 GB | vmalloc/ioremap space
@@ -83,7 +83,7 @@ RISC-V Linux Kernel SV48
                                                               | Kernel-space virtual memory, shared between all processes:
   ____________________________________________________________|___________________________________________________________
                     |            |                  |         |
-   ffff8d7ffee00000 |  -114.5 TB | ffff8d7ffeffffff |    2 MB | fixmap
+   ffff8d7ffea00000 |  -114.5 TB | ffff8d7ffeffffff |    6 MB | fixmap
    ffff8d7fff000000 |  -114.5 TB | ffff8d7fffffffff |   16 MB | PCI io
    ffff8d8000000000 |  -114.5 TB | ffff8f7fffffffff |    2 TB | vmemmap
    ffff8f8000000000 |  -112.5 TB | ffffaf7fffffffff |   32 TB | vmalloc/ioremap space
@@ -119,7 +119,7 @@ RISC-V Linux Kernel SV57
                                                               | Kernel-space virtual memory, shared between all processes:
   ____________________________________________________________|___________________________________________________________
                     |            |                  |         |
-   ff1bfffffee00000 | -57     PB | ff1bfffffeffffff |    2 MB | fixmap
+   ff1bfffffea00000 | -57     PB | ff1bfffffeffffff |    6 MB | fixmap
    ff1bffffff000000 | -57     PB | ff1bffffffffffff |   16 MB | PCI io
    ff1c000000000000 | -57     PB | ff1fffffffffffff |    1 PB | vmemmap
    ff20000000000000 | -56     PB | ff5fffffffffffff |   16 PB | vmalloc/ioremap space
diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
index 5c3e7b97fcc6..0a55099bb734 100644
--- a/arch/riscv/include/asm/fixmap.h
+++ b/arch/riscv/include/asm/fixmap.h
@@ -22,6 +22,14 @@
  */
 enum fixed_addresses {
 	FIX_HOLE,
+	/*
+	 * The fdt fixmap mapping must be PMD aligned and will be mapped
+	 * using PMD entries in fixmap_pmd in 64-bit and a PGD entry in 32-bit.
+	 */
+	FIX_FDT_END,
+	FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
+
+	/* Below fixmaps will be mapped using fixmap_pte */
 	FIX_PTE,
 	FIX_PMD,
 	FIX_PUD,
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index ab05f892d317..f641837ccf31 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -87,9 +87,13 @@
 
 #define FIXADDR_TOP      PCI_IO_START
 #ifdef CONFIG_64BIT
-#define FIXADDR_SIZE     PMD_SIZE
+#define MAX_FDT_SIZE	 PMD_SIZE
+#define FIX_FDT_SIZE	 (MAX_FDT_SIZE + SZ_2M)
+#define FIXADDR_SIZE     (PMD_SIZE + FIX_FDT_SIZE)
 #else
-#define FIXADDR_SIZE     PGDIR_SIZE
+#define MAX_FDT_SIZE	 PGDIR_SIZE
+#define FIX_FDT_SIZE	 MAX_FDT_SIZE
+#define FIXADDR_SIZE     (PGDIR_SIZE + FIX_FDT_SIZE)
 #endif
 #define FIXADDR_START    (FIXADDR_TOP - FIXADDR_SIZE)
 
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 376d2827e736..542eed85ad2c 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -283,7 +283,6 @@ void __init setup_arch(char **cmdline_p)
 	else
 		pr_err("No DTB found in kernel mappings\n");
 #endif
-	early_init_fdt_scan_reserved_mem();
 	misc_mem_init();
 
 	init_resources();
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 478d6763a01a..fb78d6bbabae 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -57,7 +57,6 @@ unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)]
 EXPORT_SYMBOL(empty_zero_page);
 
 extern char _start[];
-#define DTB_EARLY_BASE_VA      PGDIR_SIZE
 void *_dtb_early_va __initdata;
 uintptr_t _dtb_early_pa __initdata;
 
@@ -236,6 +235,14 @@ static void __init setup_bootmem(void)
 	set_max_mapnr(max_low_pfn - ARCH_PFN_OFFSET);
 
 	reserve_initrd_mem();
+
+	/*
+	 * No allocation should be done before reserving the memory as defined
+	 * in the device tree, otherwise the allocation could end up in a
+	 * reserved region.
+	 */
+	early_init_fdt_scan_reserved_mem();
+
 	/*
 	 * If DTB is built in, no need to reserve its memblock.
 	 * Otherwise, do reserve it but avoid using
@@ -279,9 +286,6 @@ pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
 static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
 
 pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
-static p4d_t __maybe_unused early_dtb_p4d[PTRS_PER_P4D] __initdata __aligned(PAGE_SIZE);
-static pud_t __maybe_unused early_dtb_pud[PTRS_PER_PUD] __initdata __aligned(PAGE_SIZE);
-static pmd_t __maybe_unused early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
 
 #ifdef CONFIG_XIP_KERNEL
 #define pt_ops			(*(struct pt_alloc_ops *)XIP_FIXUP(&pt_ops))
@@ -626,9 +630,6 @@ static void __init create_p4d_mapping(p4d_t *p4dp,
 #define trampoline_pgd_next	(pgtable_l5_enabled ?			\
 		(uintptr_t)trampoline_p4d : (pgtable_l4_enabled ?	\
 		(uintptr_t)trampoline_pud : (uintptr_t)trampoline_pmd))
-#define early_dtb_pgd_next	(pgtable_l5_enabled ?			\
-		(uintptr_t)early_dtb_p4d : (pgtable_l4_enabled ?	\
-		(uintptr_t)early_dtb_pud : (uintptr_t)early_dtb_pmd))
 #else
 #define pgd_next_t		pte_t
 #define alloc_pgd_next(__va)	pt_ops.alloc_pte(__va)
@@ -636,7 +637,6 @@ static void __init create_p4d_mapping(p4d_t *p4dp,
 #define create_pgd_next_mapping(__nextp, __va, __pa, __sz, __prot)	\
 	create_pte_mapping(__nextp, __va, __pa, __sz, __prot)
 #define fixmap_pgd_next		((uintptr_t)fixmap_pte)
-#define early_dtb_pgd_next	((uintptr_t)early_dtb_pmd)
 #define create_p4d_mapping(__pmdp, __va, __pa, __sz, __prot) do {} while(0)
 #define create_pud_mapping(__pmdp, __va, __pa, __sz, __prot) do {} while(0)
 #define create_pmd_mapping(__pmdp, __va, __pa, __sz, __prot) do {} while(0)
@@ -860,32 +860,28 @@ static void __init create_kernel_page_table(pgd_t *pgdir, bool early)
  * this means 2 PMD entries whereas for 32-bit kernel, this is only 1 PGDIR
  * entry.
  */
-static void __init create_fdt_early_page_table(pgd_t *pgdir, uintptr_t dtb_pa)
+static void __init create_fdt_early_page_table(pgd_t *pgdir,
+					       uintptr_t fix_fdt_va,
+					       uintptr_t dtb_pa)
 {
-#ifndef CONFIG_BUILTIN_DTB
 	uintptr_t pa = dtb_pa & ~(PMD_SIZE - 1);
 
-	create_pgd_mapping(early_pg_dir, DTB_EARLY_BASE_VA,
-			   IS_ENABLED(CONFIG_64BIT) ? early_dtb_pgd_next : pa,
-			   PGDIR_SIZE,
-			   IS_ENABLED(CONFIG_64BIT) ? PAGE_TABLE : PAGE_KERNEL);
-
-	if (pgtable_l5_enabled)
-		create_p4d_mapping(early_dtb_p4d, DTB_EARLY_BASE_VA,
-				   (uintptr_t)early_dtb_pud, P4D_SIZE, PAGE_TABLE);
-
-	if (pgtable_l4_enabled)
-		create_pud_mapping(early_dtb_pud, DTB_EARLY_BASE_VA,
-				   (uintptr_t)early_dtb_pmd, PUD_SIZE, PAGE_TABLE);
+#ifndef CONFIG_BUILTIN_DTB
+	/* Make sure the fdt fixmap address is always aligned on PMD size */
+	BUILD_BUG_ON(FIX_FDT % (PMD_SIZE / PAGE_SIZE));
 
-	if (IS_ENABLED(CONFIG_64BIT)) {
-		create_pmd_mapping(early_dtb_pmd, DTB_EARLY_BASE_VA,
+	/* In 32-bit only, the fdt lies in its own PGD */
+	if (!IS_ENABLED(CONFIG_64BIT)) {
+		create_pgd_mapping(early_pg_dir, fix_fdt_va,
+				   pa, MAX_FDT_SIZE, PAGE_KERNEL);
+	} else {
+		create_pmd_mapping(fixmap_pmd, fix_fdt_va,
 				   pa, PMD_SIZE, PAGE_KERNEL);
-		create_pmd_mapping(early_dtb_pmd, DTB_EARLY_BASE_VA + PMD_SIZE,
+		create_pmd_mapping(fixmap_pmd, fix_fdt_va + PMD_SIZE,
 				   pa + PMD_SIZE, PMD_SIZE, PAGE_KERNEL);
 	}
 
-	dtb_early_va = (void *)DTB_EARLY_BASE_VA + (dtb_pa & (PMD_SIZE - 1));
+	dtb_early_va = (void *)fix_fdt_va + (dtb_pa & (PMD_SIZE - 1));
 #else
 	/*
 	 * For 64-bit kernel, __va can't be used since it would return a linear
@@ -1055,7 +1051,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
 	create_kernel_page_table(early_pg_dir, true);
 
 	/* Setup early mapping for FDT early scan */
-	create_fdt_early_page_table(early_pg_dir, dtb_pa);
+	create_fdt_early_page_table(early_pg_dir,
+				    __fix_to_virt(FIX_FDT), dtb_pa);
 
 	/*
 	 * Bootime fixmap only can handle PMD_SIZE mapping. Thus, boot-ioremap
@@ -1097,6 +1094,16 @@ static void __init setup_vm_final(void)
 	u64 i;
 
 	/* Setup swapper PGD for fixmap */
+#if !defined(CONFIG_64BIT)
+	/*
+	 * In 32-bit, the device tree lies in a pgd entry, so it must be copied
+	 * directly in swapper_pg_dir in addition to the pgd entry that points
+	 * to fixmap_pte.
+	 */
+	unsigned long idx = pgd_index(__fix_to_virt(FIX_FDT));
+
+	set_pgd(&swapper_pg_dir[idx], early_pg_dir[idx]);
+#endif
 	create_pgd_mapping(swapper_pg_dir, FIXADDR_START,
 			   __pa_symbol(fixmap_pgd_next),
 			   PGDIR_SIZE, PAGE_TABLE);
-- 
2.37.2

[PATCH -fixes v2 2/3] riscv: Do not set initial_boot_params to the linear address of the dtb

[Alexandre Ghiti]

early_init_dt_verify() is already called in parse_dtb() and since the dtb
address does not change anymore (it is now in the fixmap region), no need
to reset initial_boot_params by calling early_init_dt_verify() again.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 arch/riscv/kernel/setup.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 542eed85ad2c..a059b73f4ddb 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p)
 #if IS_ENABLED(CONFIG_BUILTIN_DTB)
 	unflatten_and_copy_device_tree();
 #else
-	if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
-		unflatten_device_tree();
-	else
-		pr_err("No DTB found in kernel mappings\n");
+	unflatten_device_tree();
 #endif
 	misc_mem_init();
 
-- 
2.37.2

Re: [PATCH -fixes v2 2/3] riscv: Do not set initial_boot_params to the linear address of the dtb

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Wed, Mar 29, 2023 at 10:19:31AM +0200, Alexandre Ghiti wrote:
> early_init_dt_verify() is already called in parse_dtb() and since the dtb
> address does not change anymore (it is now in the fixmap region), no need
> to reset initial_boot_params by calling early_init_dt_verify() again.
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  arch/riscv/kernel/setup.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 542eed85ad2c..a059b73f4ddb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p)
>  #if IS_ENABLED(CONFIG_BUILTIN_DTB)
>  	unflatten_and_copy_device_tree();
>  #else
> -	if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> -		unflatten_device_tree();

Silly question maybe, but since it isn't explicitly mentioned, the
XIP_FIXUP bits no longer matter?
Also, in related news, I assume you don't have a QEMU setup that can do
boot an XIP kernel?

Cheers,
Conor.

> -	else
> -		pr_err("No DTB found in kernel mappings\n");
> +	unflatten_device_tree();
>  #endif
>  	misc_mem_init();
>  
> -- 
> 2.37.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH -fixes v2 2/3] riscv: Do not set initial_boot_params to the linear address of the dtb

[Alexandre Ghiti]

On Wed, Mar 29, 2023 at 4:37 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Mar 29, 2023 at 10:19:31AM +0200, Alexandre Ghiti wrote:
> > early_init_dt_verify() is already called in parse_dtb() and since the dtb
> > address does not change anymore (it is now in the fixmap region), no need
> > to reset initial_boot_params by calling early_init_dt_verify() again.
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  arch/riscv/kernel/setup.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 542eed85ad2c..a059b73f4ddb 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p)
> >  #if IS_ENABLED(CONFIG_BUILTIN_DTB)
> >       unflatten_and_copy_device_tree();
> >  #else
> > -     if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> > -             unflatten_device_tree();
>
> Silly question maybe, but since it isn't explicitly mentioned, the
> XIP_FIXUP bits no longer matter?

The XIP_FIXUP is only needed when translating virtual to physical
addresses, but that does not mean I did not break it, I haven't
considered XIP at all...

> Also, in related news, I assume you don't have a QEMU setup that can do
> boot an XIP kernel?

I haven't booted a XIP kernel for a long time now, here are my notes
from that time:
https://github.com/AlexGhiti/alexghiti.github.io/blob/main/xip/XIP.md

>
> Cheers,
> Conor.
>
> > -     else
> > -             pr_err("No DTB found in kernel mappings\n");
> > +     unflatten_device_tree();
> >  #endif
> >       misc_mem_init();
> >
> > --
> > 2.37.2
> >

Re: [PATCH -fixes v2 2/3] riscv: Do not set initial_boot_params to the linear address of the dtb

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On Wed, Mar 29, 2023 at 04:52:45PM +0200, Alexandre Ghiti wrote:
> On Wed, Mar 29, 2023 at 4:37 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Mar 29, 2023 at 10:19:31AM +0200, Alexandre Ghiti wrote:
> > > early_init_dt_verify() is already called in parse_dtb() and since the dtb
> > > address does not change anymore (it is now in the fixmap region), no need
> > > to reset initial_boot_params by calling early_init_dt_verify() again.
> > >
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> > >  arch/riscv/kernel/setup.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 542eed85ad2c..a059b73f4ddb 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p)
> > >  #if IS_ENABLED(CONFIG_BUILTIN_DTB)
> > >       unflatten_and_copy_device_tree();
> > >  #else
> > > -     if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> > > -             unflatten_device_tree();
> >
> > Silly question maybe, but since it isn't explicitly mentioned, the
> > XIP_FIXUP bits no longer matter?
> 
> The XIP_FIXUP is only needed when translating virtual to physical
> addresses, but that does not mean I did not break it, I haven't
> considered XIP at all...

So, what currently happens is that, during early boot, we call
parse_dtb() right at the beginning of setup_arch().
That calls early_init_dt_scan(dtb_early_pa), which in turn calls
early_init_dt_verify(dtb_early_pa).

Here, relatively late during boot, we are coming along and calling the
function again. This existed prior to the XIP stuff landing, but the
specific XIP_FIXUP handling looks to be the fallout from:
https://lore.kernel.org/linux-riscv/82a05081-5662-c787-44e4-d480774ce31c@ghiti.fr/

The check in the first place was added by Anup's move away from fixmap
for dtb stuff, which makes me wonder - should this actually be part of
1/3? Something, something we no longer need to do this because these
addresses no longer change as per 1/3?

> > Also, in related news, I assume you don't have a QEMU setup that can do
> > boot an XIP kernel?
> 
> I haven't booted a XIP kernel for a long time now, here are my notes
> from that time:
> https://github.com/AlexGhiti/alexghiti.github.io/blob/main/xip/XIP.md

Right, I'll have to get around to trying that. We never put any
investment into QEMU internally, nor run any CI against it, so the HSS
doesn't actually work in QEMU anymore.
Assertion failures due to missing peripheral emulation :(
Probably don't need the HSS though and can do a direct kernel boot, I'll
have to see if that works for this XIP stuff, I know it does for regular
kernels.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH -fixes v2 3/3] riscv: No need to relocate the dtb as it lies in the fixmap region

[Alexandre Ghiti]

We used to access the dtb via its linear mapping address but now that the
dtb early mapping was moved in the fixmap region, we can keep using this
address since it is present in swapper_pg_dir, and remove the dtb
relocation.

Note that the relocation was wrong anyway since early_memremap() is
restricted to 256K whereas the maximum fdt size is 2MB.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 arch/riscv/mm/init.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index fb78d6bbabae..0f14f4a8d179 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -249,25 +249,8 @@ static void __init setup_bootmem(void)
 	 * early_init_fdt_reserve_self() since __pa() does
 	 * not work for DTB pointers that are fixmap addresses
 	 */
-	if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) {
-		/*
-		 * In case the DTB is not located in a memory region we won't
-		 * be able to locate it later on via the linear mapping and
-		 * get a segfault when accessing it via __va(dtb_early_pa).
-		 * To avoid this situation copy DTB to a memory region.
-		 * Note that memblock_phys_alloc will also reserve DTB region.
-		 */
-		if (!memblock_is_memory(dtb_early_pa)) {
-			size_t fdt_size = fdt_totalsize(dtb_early_va);
-			phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE);
-			void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size);
-
-			memcpy(new_dtb_early_va, dtb_early_va, fdt_size);
-			early_memunmap(new_dtb_early_va, fdt_size);
-			_dtb_early_pa = new_dtb_early_pa;
-		} else
-			memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
-	}
+	if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
+		memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
 
 	dma_contiguous_reserve(dma32_phys_limit);
 	if (IS_ENABLED(CONFIG_64BIT))
-- 
2.37.2

Re: [PATCH -fixes v2 3/3] riscv: No need to relocate the dtb as it lies in the fixmap region

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

On Wed, Mar 29, 2023 at 10:19:32AM +0200, Alexandre Ghiti wrote:
> We used to access the dtb via its linear mapping address but now that the
> dtb early mapping was moved in the fixmap region, we can keep using this
> address since it is present in swapper_pg_dir, and remove the dtb
> relocation.
> 
> Note that the relocation was wrong anyway since early_memremap() is
> restricted to 256K whereas the maximum fdt size is 2MB.

So, should this be marked as a fix, and backported along with 1/3?
Either way,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

And from v1 (although I didn't actually send it for idk what reason):
Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  arch/riscv/mm/init.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index fb78d6bbabae..0f14f4a8d179 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -249,25 +249,8 @@ static void __init setup_bootmem(void)
>  	 * early_init_fdt_reserve_self() since __pa() does
>  	 * not work for DTB pointers that are fixmap addresses
>  	 */
> -	if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) {
> -		/*
> -		 * In case the DTB is not located in a memory region we won't
> -		 * be able to locate it later on via the linear mapping and
> -		 * get a segfault when accessing it via __va(dtb_early_pa).
> -		 * To avoid this situation copy DTB to a memory region.
> -		 * Note that memblock_phys_alloc will also reserve DTB region.
> -		 */
> -		if (!memblock_is_memory(dtb_early_pa)) {
> -			size_t fdt_size = fdt_totalsize(dtb_early_va);
> -			phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE);
> -			void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size);
> -
> -			memcpy(new_dtb_early_va, dtb_early_va, fdt_size);
> -			early_memunmap(new_dtb_early_va, fdt_size);
> -			_dtb_early_pa = new_dtb_early_pa;
> -		} else
> -			memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
> -	}
> +	if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
> +		memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
>  
>  	dma_contiguous_reserve(dma32_phys_limit);
>  	if (IS_ENABLED(CONFIG_64BIT))
> -- 
> 2.37.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH -fixes v2 3/3] riscv: No need to relocate the dtb as it lies in the fixmap region

[Alexandre Ghiti]

On Wed, Mar 29, 2023 at 3:56 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Mar 29, 2023 at 10:19:32AM +0200, Alexandre Ghiti wrote:
> > We used to access the dtb via its linear mapping address but now that the
> > dtb early mapping was moved in the fixmap region, we can keep using this
> > address since it is present in swapper_pg_dir, and remove the dtb
> > relocation.
> >
> > Note that the relocation was wrong anyway since early_memremap() is
> > restricted to 256K whereas the maximum fdt size is 2MB.
>
> So, should this be marked as a fix, and backported along with 1/3?

Hmmm the whole series should be backported, it does not make sense to
move the dtb mapping and keep accessing it using its linear mapping
address since it could fail for the exact reason the relocation was
implemented in the first place and the relocation is wrong.

Maybe we should simply add a "Cc: stable@vger.kernel.org" on all the patches?

> Either way,
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> And from v1 (although I didn't actually send it for idk what reason):
> Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks!

>
> Thanks,
> Conor.
>
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  arch/riscv/mm/init.c | 21 ++-------------------
> >  1 file changed, 2 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index fb78d6bbabae..0f14f4a8d179 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -249,25 +249,8 @@ static void __init setup_bootmem(void)
> >        * early_init_fdt_reserve_self() since __pa() does
> >        * not work for DTB pointers that are fixmap addresses
> >        */
> > -     if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) {
> > -             /*
> > -              * In case the DTB is not located in a memory region we won't
> > -              * be able to locate it later on via the linear mapping and
> > -              * get a segfault when accessing it via __va(dtb_early_pa).
> > -              * To avoid this situation copy DTB to a memory region.
> > -              * Note that memblock_phys_alloc will also reserve DTB region.
> > -              */
> > -             if (!memblock_is_memory(dtb_early_pa)) {
> > -                     size_t fdt_size = fdt_totalsize(dtb_early_va);
> > -                     phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE);
> > -                     void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size);
> > -
> > -                     memcpy(new_dtb_early_va, dtb_early_va, fdt_size);
> > -                     early_memunmap(new_dtb_early_va, fdt_size);
> > -                     _dtb_early_pa = new_dtb_early_pa;
> > -             } else
> > -                     memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
> > -     }
> > +     if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
> > +             memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
> >
> >       dma_contiguous_reserve(dma32_phys_limit);
> >       if (IS_ENABLED(CONFIG_64BIT))
> > --
> > 2.37.2
> >

Re: [PATCH -fixes v2 3/3] riscv: No need to relocate the dtb as it lies in the fixmap region

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

On Wed, Mar 29, 2023 at 04:40:18PM +0200, Alexandre Ghiti wrote:
> On Wed, Mar 29, 2023 at 3:56 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Mar 29, 2023 at 10:19:32AM +0200, Alexandre Ghiti wrote:
> > > We used to access the dtb via its linear mapping address but now that the
> > > dtb early mapping was moved in the fixmap region, we can keep using this
> > > address since it is present in swapper_pg_dir, and remove the dtb
> > > relocation.
> > >
> > > Note that the relocation was wrong anyway since early_memremap() is
> > > restricted to 256K whereas the maximum fdt size is 2MB.
> >
> > So, should this be marked as a fix, and backported along with 1/3?
> 
> Hmmm the whole series should be backported, it does not make sense to
> move the dtb mapping and keep accessing it using its linear mapping
> address since it could fail for the exact reason the relocation was
> implemented in the first place and the relocation is wrong.
> 
> Maybe we should simply add a "Cc: stable@vger.kernel.org" on all the patches?

Yup, although hopefully Palmer can handle that if nothing else needs
changing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH -fixes v2 3/3] riscv: No need to relocate the dtb as it lies in the fixmap region

[Palmer Dabbelt]

On Wed, 29 Mar 2023 08:33:45 PDT (-0700), Conor Dooley wrote:
> On Wed, Mar 29, 2023 at 04:40:18PM +0200, Alexandre Ghiti wrote:
>> On Wed, Mar 29, 2023 at 3:56 PM Conor Dooley <conor@kernel.org> wrote:
>> >
>> > On Wed, Mar 29, 2023 at 10:19:32AM +0200, Alexandre Ghiti wrote:
>> > > We used to access the dtb via its linear mapping address but now that the
>> > > dtb early mapping was moved in the fixmap region, we can keep using this
>> > > address since it is present in swapper_pg_dir, and remove the dtb
>> > > relocation.
>> > >
>> > > Note that the relocation was wrong anyway since early_memremap() is
>> > > restricted to 256K whereas the maximum fdt size is 2MB.
>> >
>> > So, should this be marked as a fix, and backported along with 1/3?
>> 
>> Hmmm the whole series should be backported, it does not make sense to
>> move the dtb mapping and keep accessing it using its linear mapping
>> address since it could fail for the exact reason the relocation was
>> implemented in the first place and the relocation is wrong.
>> 
>> Maybe we should simply add a "Cc: stable@vger.kernel.org" on all the patches?
>
> Yup, although hopefully Palmer can handle that if nothing else needs
> changing.

The bots usually pick it up anyway, but in general I try and put the 
stable CC on there when I see stuff go by.  I actually missed it this 
time until seeing the comment, though...

Re: [PATCH -fixes v2 0/3] Fixes for dtb mapping

[Palmer Dabbelt]

On Wed, 29 Mar 2023 10:19:29 +0200, Alexandre Ghiti wrote:
> We used to map the dtb differently between early_pg_dir and
> swapper_pg_dir which caused issues when we referenced addresses from
> the early mapping with swapper_pg_dir (reserved_mem): move the dtb mapping
> to the fixmap region in patch 1, which allows to simplify dtb handling in
> patch 2.
> 
> base-commit-tag: v6.3-rc3
> 
> [...]

Applied, thanks!

[1/3] riscv: Move early dtb mapping into the fixmap region
      https://git.kernel.org/palmer/c/99a289623453
[2/3] riscv: Do not set initial_boot_params to the linear address of the dtb
      https://git.kernel.org/palmer/c/becc32e1f2ef
[3/3] riscv: No need to relocate the dtb as it lies in the fixmap region
      https://git.kernel.org/palmer/c/585da9dacc9c

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>

Re: [PATCH -fixes v2 0/3] Fixes for dtb mapping

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed, 29 Mar 2023 10:19:29 +0200 you wrote:
> We used to map the dtb differently between early_pg_dir and
> swapper_pg_dir which caused issues when we referenced addresses from
> the early mapping with swapper_pg_dir (reserved_mem): move the dtb mapping
> to the fixmap region in patch 1, which allows to simplify dtb handling in
> patch 2.
> 
> base-commit-tag: v6.3-rc3
> 
> [...]

Here is the summary with links:
  - [-fixes,v2,1/3] riscv: Move early dtb mapping into the fixmap region
    https://git.kernel.org/riscv/c/ef69d2559fe9
  - [-fixes,v2,2/3] riscv: Do not set initial_boot_params to the linear address of the dtb
    https://git.kernel.org/riscv/c/f1581626071c
  - [-fixes,v2,3/3] riscv: No need to relocate the dtb as it lies in the fixmap region
    https://git.kernel.org/riscv/c/1b50f956c8fe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html