Date:   Tue, 19 May 2020 13:21:09 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Lukasz Stelmach <l.stelmach@...sung.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Nicolas Pitre <nico@...xnic.net>,
        Arnd Bergmann <arnd@...db.de>,
        Eric Miao <eric.miao@...dia.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Chris Brandt <chris.brandt@...esas.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>,
        Grant Likely <grant.likely@....com>
Subject: Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB

Hi Russell,

CC devicetree

On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
<linux@...linux.org.uk> 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@...sung.com> 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@...xnic.net>
> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> > > > Reviewed-by: Nicolas Pitre <nico@...xnic.net>
> > > > Reviewed-by: Ard Biesheuvel <ardb@...nel.org>
> > > > Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
> > > > Tested-by: Dmitry Osipenko <digetx@...il.com>
> > > > ---
> > >
> > > [...]
> > >
> > > 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.

DT describes the hardware.  In general, where to put the kernel is a
software policy, and thus doesn't belong in DT, except perhaps under
/chosen.  But that would open another can of worms, as people usually
have no business in specifying where the kernel should be located.
In the crashkernel case, there is a clear separation between memory to
be used by the crashkernel, and memory to be solely inspected by the
crashkernel.

Devicetree Specification, Release v0.3, Section 3.4 "/memory node" says:

    "The client program may access memory not covered by any memory
     reservations (see section 5.3)"

(Section 5.3 "Memory Reservation Block" only talks about structures in
the FDT, not about DTS)

Hence according to the above, the crashkernel is rightfully allowed to
do whatever it wants with all memory under the /memory node.
However, there is also
Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.
This suggests the crashkernel should be passed a DTB that contains a
/reserved-memory node, describing which memory cannot be used freely.
Then the decompressor needs to take this into account when deciding
where the put the kernel.

Yes, the above requires changing code. But at least it provides a
path forward, getting rid of the fragile old masking technique.

Thanks for your comments!


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists