| Message ID | 20190927231418.83100-1-aou@eecs.berkeley.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] riscv: Fix memblock reservation for device tree blob | expand |
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
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
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
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)
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(-)