Date:   Sat, 28 Sep 2019 13:30:57 +0530
From:   Anup Patel <anup@...infault.org>
To:     Bin Meng <bmeng.cn@...il.com>
Cc:     Albert Ou <aou@...s.berkeley.edu>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Anup Patel <Anup.Patel@....com>,
        Palmer Dabbelt <palmer@...ive.com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Paul Walmsley <paul.walmsley@...ive.com>
Subject: Re: [PATCH v2] riscv: Fix memblock reservation for device tree blob

On Sat, Sep 28, 2019 at 11:52 AM Bin Meng <bmeng.cn@...il.com> wrote:
>
> On Sat, Sep 28, 2019 at 7:14 AM Albert Ou <aou@...s.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@...infault.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@...il.com>
>
> > Signed-off-by: Albert Ou <aou@...s.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists