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

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

[Geert Uytterhoeven]

Hi Russell,

CC devicetree

On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
<linux@armlinux.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@samsung.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@fluxnic.net>
> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > Tested-by: Dmitry Osipenko <digetx@gmail.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@linux-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

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

[Arnd Bergmann]

On Tue, May 19, 2020 at 1:21 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:

> >
> > > 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.

There is an existing "linux,usable-memory-range" property documented
in Documentation/devicetree/bindings/chosen.txt, which as I understand
is exactly what you are looking for, except that it is currently only
documented for arm64.

Would extending this to arm work?

      Arnd

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

[Geert Uytterhoeven]

Hi Arnd,

On Tue, May 19, 2020 at 1:28 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, May 19, 2020 at 1:21 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
>
> > >
> > > > 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.
>
> There is an existing "linux,usable-memory-range" property documented
> in Documentation/devicetree/bindings/chosen.txt, which as I understand
> is exactly what you are looking for, except that it is currently only
> documented for arm64.

Thank you, that looks appropriate!

It seems this is not really used by the early startup code.
Is that because the early startup code always runs in-place, and the
kernel image is not even copied?

> Would extending this to arm work?

Let's see.... Th arm early boot code seems to be more complex than the
arm64 code ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

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

[Russell King - ARM Linux admin]

On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> CC devicetree
> 
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.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@samsung.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@fluxnic.net>
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.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.

One line of code is not "covered by plasters".  There are no plasters.
It's a solution that works for 99.99% of people, unlike your approach
that has had a stream of issues over the last four months, and has
required many reworks of the code to fix each one.  That in itself
speaks volumes about the suitability of the approach.

> DT describes the hardware.

Right, so DT is correct.

> 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.

So you want to throw yet more complexity at this solution to try and
make it work...

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

It's hardly fragile when it's worked fine for the last 20+ years,
whereas your solution can't work without some regression being reported
within a couple of weeks of it being applied.  Again, that speaks
volumes about which one is better than the other.

Continually patching this approach to workaround one issue after another
shows that it is _this_ solution that is the fragile implementation.

A fragile implementation is by definition one that keeps breaking.
That's yours.  The masking approach hasn't "broken" for anyone, and
hasn't been the cause of one single regression anywhere.  Yes, there
are some platforms that it doesn't work for (because they choose to
reserve the first chunk of RAM for something) but that is not a
regression.

So, I'm not going to apply the next revision of this patch for at least
one whole kernel cycle - that means scheduling it for 5.10-rc at the
earliest.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

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

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 4479 bytes --]

It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
>> <linux@armlinux.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@samsung.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@fluxnic.net>
>> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
>> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.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.
>
> One line of code is not "covered by plasters".  There are no plasters.
> It's a solution that works for 99.99% of people, unlike your approach
> that has had a stream of issues over the last four months, and has
> required many reworks of the code to fix each one.  That in itself
> speaks volumes about the suitability of the approach.

As I have been working with kexec code (patches soon) I would like to
defend the DT approach a bit. It allows to avoid zImage relocation when
a decompressed kernel is larger than ~128MiB. In such case zImage isn't
small either and moving it around takes some time.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Russell King - ARM Linux admin]

On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
> > On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
> >> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> >> <linux@armlinux.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@samsung.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@fluxnic.net>
> >> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> >> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.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.
> >
> > One line of code is not "covered by plasters".  There are no plasters.
> > It's a solution that works for 99.99% of people, unlike your approach
> > that has had a stream of issues over the last four months, and has
> > required many reworks of the code to fix each one.  That in itself
> > speaks volumes about the suitability of the approach.
> 
> As I have been working with kexec code (patches soon) I would like to
> defend the DT approach a bit. It allows to avoid zImage relocation when
> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
> small either and moving it around takes some time.

... which is something that has been supported for a very long time,
before the days of DT.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

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

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 4262 bytes --]

It was <2020-05-19 wto 13:27>, when Russell King - ARM Linux admin wrote:
> On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
>> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
>>> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
>>>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
>>>> <linux@armlinux.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@samsung.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.
[...]
>>>>>>> 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.
[...]
>>>>>> 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.
>>>
>>> One line of code is not "covered by plasters".  There are no plasters.
>>> It's a solution that works for 99.99% of people, unlike your approach
>>> that has had a stream of issues over the last four months, and has
>>> required many reworks of the code to fix each one.  That in itself
>>> speaks volumes about the suitability of the approach.
>> 
>> As I have been working with kexec code (patches soon) I would like to
>> defend the DT approach a bit. It allows to avoid zImage relocation when
>> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
>> small either and moving it around takes some time.
>
> ... which is something that has been supported for a very long time,
> before the days of DT.

How? If a decompressed kernel requires >128M and a bootloader would like
to put a zImage high enough to *avoid* copying it once again, then the
decompressor can't see any memory below the 128M window it starts in and
can't decompress the kernel there. If we do not care about copying
zImage, then, indeed, everything works fine as it is today. You are
most probably right 99% doesn't require 128M kernel, but the case is
IMHO obvious enough, that it should be adressed somehow.

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Russell King - ARM Linux admin]

On Tue, May 19, 2020 at 02:49:57PM +0200, Lukasz Stelmach wrote:
> It was <2020-05-19 wto 13:27>, when Russell King - ARM Linux admin wrote:
> > On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
> >> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
> >>> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
> >>>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> >>>> <linux@armlinux.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@samsung.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.
> [...]
> >>>>>>> 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.
> [...]
> >>>>>> 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.
> >>>
> >>> One line of code is not "covered by plasters".  There are no plasters.
> >>> It's a solution that works for 99.99% of people, unlike your approach
> >>> that has had a stream of issues over the last four months, and has
> >>> required many reworks of the code to fix each one.  That in itself
> >>> speaks volumes about the suitability of the approach.
> >> 
> >> As I have been working with kexec code (patches soon) I would like to
> >> defend the DT approach a bit. It allows to avoid zImage relocation when
> >> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
> >> small either and moving it around takes some time.
> >
> > ... which is something that has been supported for a very long time,
> > before the days of DT.
> 
> How? If a decompressed kernel requires >128M and a bootloader would like
> to put a zImage high enough to *avoid* copying it once again, then the
> decompressor can't see any memory below the 128M window it starts in and
> can't decompress the kernel there.

Do you have such a large kernel?  It would be rather inefficient as
branch instructions could not be used; every function call would have
to be indirect.  The maximum is +/- 32MB for a branch.

> If we do not care about copying
> zImage, then, indeed, everything works fine as it is today. You are
> most probably right 99% doesn't require 128M kernel, but the case is
> IMHO obvious enough, that it should be adressed somehow.

If I have a kernel in excess of 4GB... "it should be addressed somehow"!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

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

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 5360 bytes --]

It was <2020-05-19 wto 14:12>, when Russell King - ARM Linux admin wrote:
> On Tue, May 19, 2020 at 02:49:57PM +0200, Lukasz Stelmach wrote:
>> It was <2020-05-19 wto 13:27>, when Russell King - ARM Linux admin wrote:
>>> On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
>>>> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
>>>>> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
>>>>>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
>>>>>> <linux@armlinux.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@samsung.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.
>> [...]
>>>>>>>>> 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.
>> [...]
>>>>>>>> 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.
>>>>>
>>>>> One line of code is not "covered by plasters".  There are no plasters.
>>>>> It's a solution that works for 99.99% of people, unlike your approach
>>>>> that has had a stream of issues over the last four months, and has
>>>>> required many reworks of the code to fix each one.  That in itself
>>>>> speaks volumes about the suitability of the approach.
>>>> 
>>>> As I have been working with kexec code (patches soon) I would like to
>>>> defend the DT approach a bit. It allows to avoid zImage relocation when
>>>> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
>>>> small either and moving it around takes some time.
>>>
>>> ... which is something that has been supported for a very long time,
>>> before the days of DT.
>> 
>> How? If a decompressed kernel requires >128M and a bootloader would like
>> to put a zImage high enough to *avoid* copying it once again, then the
>> decompressor can't see any memory below the 128M window it starts in and
>> can't decompress the kernel there.
>
> Do you have such a large kernel?  It would be rather inefficient as
> branch instructions could not be used; every function call would have
> to be indirect.  The maximum is +/- 32MB for a branch.

This number includes data, particularly initramfs which may be linked
into the kernel. Assuming kernel <32MB (15MB like min ATM) we are left
with slightly more than 100MB. Which isn't that much if you would like
it to be root file system for your device.

Of course initramfs could be loaded separately by a bootloader and yet
having both kernel and initramfs in one file has some advantages.

I am not here to argue. It's your call.

>> If we do not care about copying
>> zImage, then, indeed, everything works fine as it is today. You are
>> most probably right 99% doesn't require 128M kernel, but the case is
>> IMHO obvious enough, that it should be adressed somehow.
>
> If I have a kernel in excess of 4GB... "it should be addressed somehow"!

I believe software and hardware limitations should not be compared this
way. 

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 3210 bytes --]

It was <2020-05-19 wto 13:21>, when Geert Uytterhoeven wrote:
> Hi Russell,
>
> CC devicetree
>
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.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@samsung.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@fluxnic.net>
>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>> Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
>>>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> Tested-by: Dmitry Osipenko <digetx@gmail.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 ;-)

So we can do without the mem= cmdline option which is required
now. Sounds reasonable to me.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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@linux-m68k.org> wrote:
>
> Hi Russell,
>
> CC devicetree
>
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.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@samsung.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@fluxnic.net>
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.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.
>

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.

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

[Russell King - ARM Linux admin]

On Tue, May 19, 2020 at 03:56:59PM +0200, Ard Biesheuvel wrote:
> On Tue, 19 May 2020 at 13:21, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Russell,
> >
> > CC devicetree
> >
> > On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> > <linux@armlinux.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@samsung.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@fluxnic.net>
> > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > > Tested-by: Dmitry Osipenko <digetx@gmail.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.
> >
> 
> 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.

What is "too fragile" is trying to change this and expecting everything
to continue working as it did before.

How will switching to EFI help?  Won't the crashdump kernel detect EFI
and try to get the memory map from EFI, whereby it runs into exactly
the same issue that the DT approach does?

The current crashkernel situation works precisely because of the 128M
masking that is being done.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

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

[Ard Biesheuvel]

On Tue, 19 May 2020 at 16:29, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Tue, May 19, 2020 at 03:56:59PM +0200, Ard Biesheuvel wrote:
> > On Tue, 19 May 2020 at 13:21, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >
> > > Hi Russell,
> > >
> > > CC devicetree
> > >
> > > On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> > > <linux@armlinux.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@samsung.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@fluxnic.net>
> > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > > > Tested-by: Dmitry Osipenko <digetx@gmail.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.
> > >
> >
> > 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.
>
> What is "too fragile" is trying to change this and expecting everything
> to continue working as it did before.
>

That is my point.

> How will switching to EFI help?  Won't the crashdump kernel detect EFI
> and try to get the memory map from EFI, whereby it runs into exactly
> the same issue that the DT approach does?
>

No. If you boot from kexec, then the EFI stub is completely
circumvented, and things work as before.

> The current crashkernel situation works precisely because of the 128M
> masking that is being done.
>

Indeed. That is precisely my point.