Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB

[Ard Biesheuvel]

On Tue, 19 May 2020 at 13:21, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Russell,
>
> CC devicetree
>
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@xxxxxxxxxxxxxxx> wrote:
> > On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote:
> > > > It was <2020-04-29 Åro 10:21>, when Geert Uytterhoeven wrote:
> > > > > Currently, the start address of physical memory is obtained by masking
> > > > > the program counter with a fixed mask of 0xf8000000.  This mask value
> > > > > was chosen as a balance between the requirements of different platforms.
> > > > > However, this does require that the start address of physical memory is
> > > > > a multiple of 128 MiB, precluding booting Linux on platforms where this
> > > > > requirement is not fulfilled.
> > > > >
> > > > > Fix this limitation by obtaining the start address from the DTB instead,
> > > > > if available (either explicitly passed, or appended to the kernel).
> > > > > Fall back to the traditional method when needed.
> > > > >
> > > > > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> > > > > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> > > > > i.e. not at a multiple of 128 MiB.
> > > > >
> > > > > Suggested-by: Nicolas Pitre <nico@xxxxxxxxxxx>
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > > > > Reviewed-by: Nicolas Pitre <nico@xxxxxxxxxxx>
> > > > > Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > > Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > > > > Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> > > > > ---
> > > >
> > > > [...]
> > > >
> > > > Apparently reading physical memory layout from DTB breaks crashdump
> > > > kernels. A crashdump kernel is loaded into a region of memory, that is
> > > > reserved in the original (i.e. to be crashed) kernel. The reserved
> > > > region is large enough for the crashdump kernel to run completely inside
> > > > it and don't modify anything outside it, just read and dump the remains
> > > > of the crashed kernel. Using the information from DTB makes the
> > > > decompressor place the kernel outside of the dedicated region.
> > > >
> > > > The log below shows that a zImage and DTB are loaded at 0x18eb8000 and
> > > > 0x193f6000 (physical). The kernel is expected to run at 0x18008000, but
> > > > it is decompressed to 0x00008000 (see r4 reported before jumping from
> > > > within __enter_kernel). If I were to suggest something, there need to be
> > > > one more bit of information passed in the DTB telling the decompressor
> > > > to use the old masking technique to determain kernel address. It would
> > > > be set in the DTB loaded along with the crashdump kernel.
> > >
> > > Shouldn't the DTB passed to the crashkernel describe which region of
> > > memory is to be used instead?
> >
> > Definitely not.  The crashkernel needs to know where the RAM in the
> > machine is, so that it can create a coredump of the crashed kernel.
>
> So the DTB should describe both ;-)
>
> > > Describing "to use the old masking technique" sounds a bit hackish to me.
> > > I guess it cannot just restrict the /memory node to the reserved region,
> > > as the crashkernel needs to be able to dump the remains of the crashed
> > > kernel, which lie outside this region.
> >
> > Correct.
> >
> > > However, something under /chosen should work.
> >
> > Yet another sticky plaster...
>
> IMHO the old masking technique is the hacky solution covered by
> plasters.
>

I think debating which solution is the hacky one will not get us anywhere.

The simple reality is that the existing solution works fine for
existing platforms, and so any changes in the logic will have to be
opt-in in one way or the other.

Since U-boot supports EFI boot these days, one potential option is to
rely on that. I have some changes implementing this that go on top of
this patch, but they don't actually rely on it - it was just to
prevent lexical conflicts.

The only remaining options imo are a kernel command line option, or a
DT property that tells the decompressor to look at the memory nodes.
But using the DT memory nodes on all platforms like this patch does is
obviously just too risky.

On another note, I do think the usable-memory-region property should
be implemented for ARM as well - relying on this rounding to ensure
that the decompressor does the right thing is too fragile.