Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] riscv: Fix memblock reservation for device tree blob
@ 2019-09-27 23:14 Albert Ou
  2019-09-28  6:22 ` Bin Meng
  2019-10-04 17:34 ` Paul Walmsley
  0 siblings, 2 replies; 4+ messages in thread
From: Albert Ou @ 2019-09-27 23:14 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Anup Patel, linux-riscv, linux-kernel

This fixes an error with how the FDT blob is reserved in memblock.
An incorrect physical address calculation exposed the FDT header to
unintended corruption, which typically manifested with of_fdt_raw_init()
faulting during late boot after fdt_totalsize() returned a wrong value.
Systems with smaller physical memory sizes more frequently trigger this
issue, as the kernel is more likely to allocate from the DMA32 zone
where bbl places the DTB after the kernel image.

Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
changed the mapping of the DTB to reside in the fixmap area.
Consequently, early_init_fdt_reserve_self() cannot be used anymore in
setup_bootmem() since it relies on __pa() to derive a physical address,
which does not work with dtb_early_va that is no longer a valid kernel
logical address.

The reserved[0x1] region shows the effect of the pointer underflow
resulting from the __pa(initial_boot_params) offset subtraction:

[    0.000000] MEMBLOCK configuration:
[    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
[    0.000000]  memory.cnt  = 0x1
[    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
[    0.000000]  reserved.cnt  = 0x2
[    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
[    0.000000]  reserved[0x1]   [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0

With the fix applied:

[    0.000000] MEMBLOCK configuration:
[    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
[    0.000000]  memory.cnt  = 0x1
[    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
[    0.000000]  reserved.cnt  = 0x2
[    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
[    0.000000]  reserved[0x1]   [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0

Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
Signed-off-by: Albert Ou <aou@eecs.berkeley.edu>
---

Changes in v2:
* Changed variable identifier to dtb_early_pa per reviewer feedback.
* Removed whitespace change.

 arch/riscv/mm/init.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index f0ba713..83f7d12 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -11,6 +11,7 @@
 #include <linux/swap.h>
 #include <linux/sizes.h>
 #include <linux/of_fdt.h>
+#include <linux/libfdt.h>
 
 #include <asm/fixmap.h>
 #include <asm/tlbflush.h>
@@ -82,6 +83,8 @@ static void __init setup_initrd(void)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+static phys_addr_t dtb_early_pa __initdata;
+
 void __init setup_bootmem(void)
 {
 	struct memblock_region *reg;
@@ -117,7 +120,12 @@ void __init setup_bootmem(void)
 	setup_initrd();
 #endif /* CONFIG_BLK_DEV_INITRD */
 
-	early_init_fdt_reserve_self();
+	/*
+	 * Avoid using early_init_fdt_reserve_self() since __pa() does
+	 * not work for DTB pointers that are fixmap addresses
+	 */
+	memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
+
 	early_init_fdt_scan_reserved_mem();
 	memblock_allow_resize();
 	memblock_dump_all();
@@ -393,6 +401,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
 
 	/* Save pointer to DTB for early FDT parsing */
 	dtb_early_va = (void *)fix_to_virt(FIX_FDT) + (dtb_pa & ~PAGE_MASK);
+	/* Save physical address for memblock reservation */
+	dtb_early_pa = dtb_pa;
 }
 
 static void __init setup_vm_final(void)
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] riscv: Fix memblock reservation for device tree blob
  2019-09-27 23:14 [PATCH v2] riscv: Fix memblock reservation for device tree blob Albert Ou
@ 2019-09-28  6:22 ` Bin Meng
  2019-09-28  8:00   ` Anup Patel
  2019-10-04 17:34 ` Paul Walmsley
  1 sibling, 1 reply; 4+ messages in thread
From: Bin Meng @ 2019-09-28  6:22 UTC (permalink / raw)
  To: Albert Ou
  Cc: linux-kernel, Anup Patel, Palmer Dabbelt, linux-riscv, Paul Walmsley

On Sat, Sep 28, 2019 at 7:14 AM Albert Ou <aou@eecs.berkeley.edu> wrote:
>
> This fixes an error with how the FDT blob is reserved in memblock.
> An incorrect physical address calculation exposed the FDT header to
> unintended corruption, which typically manifested with of_fdt_raw_init()
> faulting during late boot after fdt_totalsize() returned a wrong value.
> Systems with smaller physical memory sizes more frequently trigger this
> issue, as the kernel is more likely to allocate from the DMA32 zone
> where bbl places the DTB after the kernel image.
>
> Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
> changed the mapping of the DTB to reside in the fixmap area.
> Consequently, early_init_fdt_reserve_self() cannot be used anymore in
> setup_bootmem() since it relies on __pa() to derive a physical address,
> which does not work with dtb_early_va that is no longer a valid kernel
> logical address.
>
> The reserved[0x1] region shows the effect of the pointer underflow
> resulting from the __pa(initial_boot_params) offset subtraction:
>
> [    0.000000] MEMBLOCK configuration:
> [    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
> [    0.000000]  memory.cnt  = 0x1
> [    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
> [    0.000000]  reserved.cnt  = 0x2
> [    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
> [    0.000000]  reserved[0x1]   [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0
>
> With the fix applied:
>
> [    0.000000] MEMBLOCK configuration:
> [    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
> [    0.000000]  memory.cnt  = 0x1
> [    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
> [    0.000000]  reserved.cnt  = 0x2
> [    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
> [    0.000000]  reserved[0x1]   [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0
>
> Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")

I also found that with commit 671f9a3e2e24 ("RISC-V: Setup initial
page tables in two stages"), when booting the kernel from U-Boot via:

=> bootm $kernel_addr_t - $fdtcontroladdr
($kernel_addr_t = 0x84000000 and $fdtcontroladdr = 0xff741c60)

kernel does not boot. I looked at people's instructions of booting
Linux kernel, and they seem to have such:

=> cp.l ${fdtcontroladdr} ${fdt_addr_r} 0x10000
=> bootm ${kernel_addr_r} - ${fdt_addr_r}

where ${fdt_addr_r} is 0x88000000, and "bootm 84000000  - 88000000"
can make the kernel boot.

Thanks for the patch. Now "bootm $kernel_addr_t - $fdtcontroladdr" works again!

Tested-by: Bin Meng <bmeng.cn@gmail.com>

> Signed-off-by: Albert Ou <aou@eecs.berkeley.edu>
> ---
>
> Changes in v2:
> * Changed variable identifier to dtb_early_pa per reviewer feedback.
> * Removed whitespace change.
>
>  arch/riscv/mm/init.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>

Regards,
Bin

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] riscv: Fix memblock reservation for device tree blob
  2019-09-28  6:22 ` Bin Meng
@ 2019-09-28  8:00   ` Anup Patel
  0 siblings, 0 replies; 4+ messages in thread
From: Anup Patel @ 2019-09-28  8:00 UTC (permalink / raw)
  To: Bin Meng
  Cc: Albert Ou, Anup Patel, linux-kernel, Palmer Dabbelt,
	Paul Walmsley, linux-riscv

On Sat, Sep 28, 2019 at 11:52 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 28, 2019 at 7:14 AM Albert Ou <aou@eecs.berkeley.edu> wrote:
> >
> > This fixes an error with how the FDT blob is reserved in memblock.
> > An incorrect physical address calculation exposed the FDT header to
> > unintended corruption, which typically manifested with of_fdt_raw_init()
> > faulting during late boot after fdt_totalsize() returned a wrong value.
> > Systems with smaller physical memory sizes more frequently trigger this
> > issue, as the kernel is more likely to allocate from the DMA32 zone
> > where bbl places the DTB after the kernel image.
> >
> > Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
> > changed the mapping of the DTB to reside in the fixmap area.
> > Consequently, early_init_fdt_reserve_self() cannot be used anymore in
> > setup_bootmem() since it relies on __pa() to derive a physical address,
> > which does not work with dtb_early_va that is no longer a valid kernel
> > logical address.
> >
> > The reserved[0x1] region shows the effect of the pointer underflow
> > resulting from the __pa(initial_boot_params) offset subtraction:
> >
> > [    0.000000] MEMBLOCK configuration:
> > [    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
> > [    0.000000]  memory.cnt  = 0x1
> > [    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
> > [    0.000000]  reserved.cnt  = 0x2
> > [    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
> > [    0.000000]  reserved[0x1]   [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0
> >
> > With the fix applied:
> >
> > [    0.000000] MEMBLOCK configuration:
> > [    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
> > [    0.000000]  memory.cnt  = 0x1
> > [    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
> > [    0.000000]  reserved.cnt  = 0x2
> > [    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
> > [    0.000000]  reserved[0x1]   [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0
> >
> > Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
>
> I also found that with commit 671f9a3e2e24 ("RISC-V: Setup initial
> page tables in two stages"), when booting the kernel from U-Boot via:
>
> => bootm $kernel_addr_t - $fdtcontroladdr
> ($kernel_addr_t = 0x84000000 and $fdtcontroladdr = 0xff741c60)
>
> kernel does not boot. I looked at people's instructions of booting
> Linux kernel, and they seem to have such:

Thanks for reporting this issue Bin.

I will add more information about the relation of FDT location
with the commit Bin mentioned:
1. With two staged initial page tables in-place, the first stage
only maps kernel image so we used FIXMAP to map FDT
in setup_vm()
2. Another advantage of using FIXMAP to map FDT is that
we can now place FDT anywhere in entire memory map. This
was not possible previously because FDT had to be placed
after kernel load address for __va() and __pa() to work fine.

The one thing which two-staged page tables missed was to
remove use of early_init_fdt_reserve_self() which is what
this patch does.

Also, the ARM64 Linux also used FIXMAP to map FDT and
it does not use early_init_fdt_reserve_self() to reserved FDT.

Anyways, my comments to previous version of this patch
are taken case so...

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

>
> => cp.l ${fdtcontroladdr} ${fdt_addr_r} 0x10000
> => bootm ${kernel_addr_r} - ${fdt_addr_r}
>
> where ${fdt_addr_r} is 0x88000000, and "bootm 84000000  - 88000000"
> can make the kernel boot.
>
> Thanks for the patch. Now "bootm $kernel_addr_t - $fdtcontroladdr" works again!
>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>
> > Signed-off-by: Albert Ou <aou@eecs.berkeley.edu>
> > ---
> >
> > Changes in v2:
> > * Changed variable identifier to dtb_early_pa per reviewer feedback.
> > * Removed whitespace change.
> >
> >  arch/riscv/mm/init.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
>
> Regards,
> Bin
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] riscv: Fix memblock reservation for device tree blob
  2019-09-27 23:14 [PATCH v2] riscv: Fix memblock reservation for device tree blob Albert Ou
  2019-09-28  6:22 ` Bin Meng
@ 2019-10-04 17:34 ` Paul Walmsley
  1 sibling, 0 replies; 4+ messages in thread
From: Paul Walmsley @ 2019-10-04 17:34 UTC (permalink / raw)
  To: Albert Ou; +Cc: Anup Patel, Palmer Dabbelt, linux-riscv, linux-kernel

On Fri, 27 Sep 2019, Albert Ou wrote:

> This fixes an error with how the FDT blob is reserved in memblock.
> An incorrect physical address calculation exposed the FDT header to
> unintended corruption, which typically manifested with of_fdt_raw_init()
> faulting during late boot after fdt_totalsize() returned a wrong value.
> Systems with smaller physical memory sizes more frequently trigger this
> issue, as the kernel is more likely to allocate from the DMA32 zone
> where bbl places the DTB after the kernel image.
> 
> Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
> changed the mapping of the DTB to reside in the fixmap area.
> Consequently, early_init_fdt_reserve_self() cannot be used anymore in
> setup_bootmem() since it relies on __pa() to derive a physical address,
> which does not work with dtb_early_va that is no longer a valid kernel
> logical address.
> 
> The reserved[0x1] region shows the effect of the pointer underflow
> resulting from the __pa(initial_boot_params) offset subtraction:
> 
> [    0.000000] MEMBLOCK configuration:
> [    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
> [    0.000000]  memory.cnt  = 0x1
> [    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
> [    0.000000]  reserved.cnt  = 0x2
> [    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
> [    0.000000]  reserved[0x1]   [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0
> 
> With the fix applied:
> 
> [    0.000000] MEMBLOCK configuration:
> [    0.000000]  memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514
> [    0.000000]  memory.cnt  = 0x1
> [    0.000000]  memory[0x0]     [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0
> [    0.000000]  reserved.cnt  = 0x2
> [    0.000000]  reserved[0x0]   [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0
> [    0.000000]  reserved[0x1]   [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0
> 
> Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages")
> Signed-off-by: Albert Ou <aou@eecs.berkeley.edu>

Thanks, queued for v5.4-rc.  Welcome back Albert :-)


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-27 23:14 [PATCH v2] riscv: Fix memblock reservation for device tree blob Albert Ou
2019-09-28  6:22 ` Bin Meng
2019-09-28  8:00   ` Anup Patel
2019-10-04 17:34 ` Paul Walmsley

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org infradead-linux-riscv@archiver.kernel.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox