mbox series

[v5,0/9] Add generic support for kdump DT properties

Message ID cover.1628670468.git.geert+renesas@glider.be
Headers show
Series Add generic support for kdump DT properties | expand

Message

Geert Uytterhoeven Aug. 11, 2021, 8:50 a.m. UTC
Hi all,

This patch series adds generic support for parsing DT properties related
to crash dump kernels ("linux,elfcorehdr" and "linux,elfcorehdr" under
the "/chosen" node), makes use of it on arm32, and performs a few
cleanups.  It is an evolution of the combination of [1] and [2].

The series consists of 6 parts:
  1. Patch 1 prepares architecture-specific code (needed for MIPS only)
     to avoid duplicating elf core header reservation later.
  2. Patch 2 prepares the visibility of variables used to hold
     information retrieved from the DT properties.
  3. Patches 3-5 add support to the FDT core for handling the
     properties.
     This can co-exist safely with architecture-specific handling, until
     the latter has been removed.
  4. Patch 6 removes the non-standard handling of "linux,elfcorehdr" on
     riscv.
  5. Patches 7-8 convert arm64 to use the generic handling instead of
     its own implementation.
  6. Patch 9 adds support for kdump properties to arm32.
     The corresponding patch for kexec-tools is "[PATCH] arm: kdump: Add
     DT properties to crash dump kernel's DTB"[3], which is still valid.

Changes compared to v4[4]:
  - New patch "[PATCH v5 1/9] MIPS: Avoid future duplicate elf core
    header reservation",
  - Drop "memblock: Add variables for usable memory limitation", as this
    is now handled in FDT core code,
  - Make ELFCORE_ADDR_{MAX,ERR} visible, too,
  - Handle the actual elf core header reservation and memory capping in
    FDT core code,
  - Add Reviewed-by, Acked-by,
  - Remove all architecture-specific handling on arm64,
  - Drop "arm64: kdump: Use IS_ENABLED(CONFIG_CRASH_DUMP) instead of
    #ifdef", as the affected code is gone,
  - Remove the addition of "linux,elfcorehdr" and
    "linux,usable-memory-range" handling to arch/arm/mm/init.c.

Changes compared to older versions:
  - Make elfcorehdr_{addr,size} always visible,
  - Add variables for usable memory limitation,
  - Use IS_ENABLED() instead of #ifdef (incl. initrd and arm64),
  - Clarify what architecture-specific code is still responsible for,
  - Add generic support for parsing linux,usable-memory-range,
  - Remove custom linux,usable-memory-range parsing on arm64,
  - Use generic handling on ARM.
  
This has been tested with kexec/kdump on arm32 and arm64, and
boot-tested on riscv64 and DT-less MIPS.

Thanks!

[1] "[PATCH v3] ARM: Parse kdump DT properties"
    https://lore.kernel.org/r/20210317113130.2554368-1-geert+renesas@glider.be/
[2] "[PATCH 0/3] Add generic-support for linux,elfcorehdr and fix riscv"
    https://lore.kernel.org/r/cover.1623780059.git.geert+renesas@glider.be/
[3] "[PATCH] arm: kdump: Add DT properties to crash dump kernel's DTB"
    https://lore.kernel.org/r/20200902154129.6358-1-geert+renesas@glider.be/
[4] "[PATCH v4 00/10] Add generic support for kdump DT properties"
    https://lore.kernel.org/r/cover.1626266516.git.geert+renesas@glider.be

Geert Uytterhoeven (9):
  MIPS: Avoid future duplicate elf core header reservation
  crash_dump: Make elfcorehdr address/size symbols always visible
  of: fdt: Add generic support for handling elf core headers property
  of: fdt: Add generic support for handling usable memory range property
  of: fdt: Use IS_ENABLED(CONFIG_BLK_DEV_INITRD) instead of #ifdef
  riscv: Remove non-standard linux,elfcorehdr handling
  arm64: kdump: Remove custom linux,elfcorehdr handling
  arm64: kdump: Remove custom linux,usable-memory-range handling
  ARM: uncompress: Parse "linux,usable-memory-range" DT property

 Documentation/devicetree/bindings/chosen.txt  | 12 +--
 .../arm/boot/compressed/fdt_check_mem_start.c | 48 ++++++++--
 arch/arm64/mm/init.c                          | 88 -----------------
 arch/mips/kernel/setup.c                      |  3 +-
 arch/riscv/mm/init.c                          | 20 ----
 drivers/of/fdt.c                              | 94 +++++++++++++++++--
 include/linux/crash_dump.h                    |  3 +-
 7 files changed, 139 insertions(+), 129 deletions(-)

Comments

Geert Uytterhoeven Aug. 11, 2021, 3:35 p.m. UTC | #1
On Wed, Aug 11, 2021 at 10:51 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Prepare for early_init_fdt_scan_reserved_mem() reserving the memory
> occupied by an elf core header described in the device tree.
> As arch_mem_init() calls early_init_fdt_scan_reserved_mem() before
> mips_reserve_vmcore(), the latter needs to check if the memory has
> already been reserved before.
>
> Note that mips_reserve_vmcore() cannot just be removed, as not all MIPS
> systems use DT.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v5:
>   - New.
> ---
>  arch/mips/kernel/setup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 23a140327a0bac1b..4693add05743d78b 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -429,7 +429,8 @@ static void __init mips_reserve_vmcore(void)
>         pr_info("Reserving %ldKB of memory at %ldKB for kdump\n",
>                 (unsigned long)elfcorehdr_size >> 10, (unsigned long)elfcorehdr_addr >> 10);
>
> -       memblock_reserve(elfcorehdr_addr, elfcorehdr_size);
> +       if (!memblock_is_region_reserved(elfcorehdr_addr, elfcorehdr_size)

As pointed out by lkp, there's a closing parenthesis missing.

/me hides back under his rock.

> +               memblock_reserve(elfcorehdr_addr, elfcorehdr_size);
>  #endif

Gr{oetje,eeting}s,

                        Geert
Rob Herring Aug. 15, 2021, 3:25 p.m. UTC | #2
On Wed, Aug 11, 2021 at 10:50:58AM +0200, Geert Uytterhoeven wrote:
> 	Hi all,

> 

> This patch series adds generic support for parsing DT properties related

> to crash dump kernels ("linux,elfcorehdr" and "linux,elfcorehdr" under

> the "/chosen" node), makes use of it on arm32, and performs a few

> cleanups.  It is an evolution of the combination of [1] and [2].


The DT bits look fine to me. How do you expect this to be merged? I'm 
happy to take it if arch maintainers can ack it.

> 

> The series consists of 6 parts:

>   1. Patch 1 prepares architecture-specific code (needed for MIPS only)

>      to avoid duplicating elf core header reservation later.

>   2. Patch 2 prepares the visibility of variables used to hold

>      information retrieved from the DT properties.

>   3. Patches 3-5 add support to the FDT core for handling the

>      properties.

>      This can co-exist safely with architecture-specific handling, until

>      the latter has been removed.


Looks like patch 5 doesn't have any dependencies with the series?

>   4. Patch 6 removes the non-standard handling of "linux,elfcorehdr" on

>      riscv.


I thought this should be applied for 5.14?

>   5. Patches 7-8 convert arm64 to use the generic handling instead of

>      its own implementation.

>   6. Patch 9 adds support for kdump properties to arm32.

>      The corresponding patch for kexec-tools is "[PATCH] arm: kdump: Add

>      DT properties to crash dump kernel's DTB"[3], which is still valid.


This one can be applied on its own, right?

Rob
Mike Rapoport Aug. 16, 2021, 5:52 a.m. UTC | #3
Hi Geert,

On Wed, Aug 11, 2021 at 10:50:59AM +0200, Geert Uytterhoeven wrote:
> Prepare for early_init_fdt_scan_reserved_mem() reserving the memory

> occupied by an elf core header described in the device tree.

> As arch_mem_init() calls early_init_fdt_scan_reserved_mem() before

> mips_reserve_vmcore(), the latter needs to check if the memory has

> already been reserved before.


Doing memblock_reserve() for the same region is usually fine, did you
encounter any issues without this patch?
 
> Note that mips_reserve_vmcore() cannot just be removed, as not all MIPS

> systems use DT.

> 

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> ---

> v5:

>   - New.

> ---

>  arch/mips/kernel/setup.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c

> index 23a140327a0bac1b..4693add05743d78b 100644

> --- a/arch/mips/kernel/setup.c

> +++ b/arch/mips/kernel/setup.c

> @@ -429,7 +429,8 @@ static void __init mips_reserve_vmcore(void)

>  	pr_info("Reserving %ldKB of memory at %ldKB for kdump\n",

>  		(unsigned long)elfcorehdr_size >> 10, (unsigned long)elfcorehdr_addr >> 10);

>  

> -	memblock_reserve(elfcorehdr_addr, elfcorehdr_size);

> +	if (!memblock_is_region_reserved(elfcorehdr_addr, elfcorehdr_size)

> +		memblock_reserve(elfcorehdr_addr, elfcorehdr_size);

>  #endif

>  }

>  

> -- 

> 2.25.1

> 


-- 
Sincerely yours,
Mike.
Geert Uytterhoeven Aug. 23, 2021, 10:13 a.m. UTC | #4
Hi Rob,

On Sun, Aug 15, 2021 at 5:25 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Aug 11, 2021 at 10:50:58AM +0200, Geert Uytterhoeven wrote:

> > This patch series adds generic support for parsing DT properties related

> > to crash dump kernels ("linux,elfcorehdr" and "linux,elfcorehdr" under

> > the "/chosen" node), makes use of it on arm32, and performs a few

> > cleanups.  It is an evolution of the combination of [1] and [2].

>

> The DT bits look fine to me. How do you expect this to be merged? I'm

> happy to take it if arch maintainers can ack it.


I had hoped you could take the series...

> > The series consists of 6 parts:

> >   1. Patch 1 prepares architecture-specific code (needed for MIPS only)

> >      to avoid duplicating elf core header reservation later.

> >   2. Patch 2 prepares the visibility of variables used to hold

> >      information retrieved from the DT properties.

> >   3. Patches 3-5 add support to the FDT core for handling the

> >      properties.

> >      This can co-exist safely with architecture-specific handling, until

> >      the latter has been removed.

>

> Looks like patch 5 doesn't have any dependencies with the series?


Indeed. So you can take it independently.

> >   4. Patch 6 removes the non-standard handling of "linux,elfcorehdr" on

> >      riscv.

>

> I thought this should be applied for 5.14?


Me too, but unfortunately that hasn't happened yet...

> >   5. Patches 7-8 convert arm64 to use the generic handling instead of

> >      its own implementation.

> >   6. Patch 9 adds support for kdump properties to arm32.

> >      The corresponding patch for kexec-tools is "[PATCH] arm: kdump: Add

> >      DT properties to crash dump kernel's DTB"[3], which is still valid.

>

> This one can be applied on its own, right?


While that wouldn't break anything (i.e. no regression), it still
wouldn't work if the DT properties are present, and the now-legacy
"mem=" kernel command line parameter is not.

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
Geert Uytterhoeven Aug. 23, 2021, 10:17 a.m. UTC | #5
Hi Mike,

On Mon, Aug 16, 2021 at 7:52 AM Mike Rapoport <rppt@kernel.org> wrote:
> On Wed, Aug 11, 2021 at 10:50:59AM +0200, Geert Uytterhoeven wrote:

> > Prepare for early_init_fdt_scan_reserved_mem() reserving the memory

> > occupied by an elf core header described in the device tree.

> > As arch_mem_init() calls early_init_fdt_scan_reserved_mem() before

> > mips_reserve_vmcore(), the latter needs to check if the memory has

> > already been reserved before.

>

> Doing memblock_reserve() for the same region is usually fine, did you

> encounter any issues without this patch?


Does it also work if the same region is part of an earlier larger
reservation?  I am no memblock expert, so I don't know.
I didn't run into any issues, as my MIPS platform is non-DT, but I
assume arch/arm64/mm/init.c:reserve_elfcorehdr() had the check for
a reason.

Thanks!

>

> > Note that mips_reserve_vmcore() cannot just be removed, as not all MIPS

> > systems use DT.

> >

> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > ---

> > v5:

> >   - New.

> > ---

> >  arch/mips/kernel/setup.c | 3 ++-

> >  1 file changed, 2 insertions(+), 1 deletion(-)

> >

> > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c

> > index 23a140327a0bac1b..4693add05743d78b 100644

> > --- a/arch/mips/kernel/setup.c

> > +++ b/arch/mips/kernel/setup.c

> > @@ -429,7 +429,8 @@ static void __init mips_reserve_vmcore(void)

> >       pr_info("Reserving %ldKB of memory at %ldKB for kdump\n",

> >               (unsigned long)elfcorehdr_size >> 10, (unsigned long)elfcorehdr_addr >> 10);

> >

> > -     memblock_reserve(elfcorehdr_addr, elfcorehdr_size);

> > +     if (!memblock_is_region_reserved(elfcorehdr_addr, elfcorehdr_size)

> > +             memblock_reserve(elfcorehdr_addr, elfcorehdr_size);

> >  #endif

> >  }


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
Catalin Marinas Aug. 23, 2021, 12:55 p.m. UTC | #6
On Wed, Aug 11, 2021 at 10:51:05AM +0200, Geert Uytterhoeven wrote:
> Remove the architecture-specific code for handling the

> "linux,elfcorehdr" property under the "/chosen" node in DT, as the

> platform-agnostic handling in the FDT core code already takes care of

> this.

> 

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>


Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Catalin Marinas Aug. 23, 2021, 12:55 p.m. UTC | #7
On Wed, Aug 11, 2021 at 10:51:06AM +0200, Geert Uytterhoeven wrote:
> Remove the architecture-specific code for handling the

> "linux,usable-memory-range" property under the "/chosen" node in DT, as

> the platform-agnostic FDT core code already takes care of this.

> 

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>


Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Mike Rapoport Aug. 23, 2021, 1:09 p.m. UTC | #8
On Mon, Aug 23, 2021 at 12:17:50PM +0200, Geert Uytterhoeven wrote:
> Hi Mike,

> 

> On Mon, Aug 16, 2021 at 7:52 AM Mike Rapoport <rppt@kernel.org> wrote:

> > On Wed, Aug 11, 2021 at 10:50:59AM +0200, Geert Uytterhoeven wrote:

> > > Prepare for early_init_fdt_scan_reserved_mem() reserving the memory

> > > occupied by an elf core header described in the device tree.

> > > As arch_mem_init() calls early_init_fdt_scan_reserved_mem() before

> > > mips_reserve_vmcore(), the latter needs to check if the memory has

> > > already been reserved before.

> >

> > Doing memblock_reserve() for the same region is usually fine, did you

> > encounter any issues without this patch?

> 

> Does it also work if the same region is part of an earlier larger

> reservation?  I am no memblock expert, so I don't know.

> I didn't run into any issues, as my MIPS platform is non-DT, but I

> assume arch/arm64/mm/init.c:reserve_elfcorehdr() had the check for

> a reason.


The memory will be reserved regardless of the earlier reservation, the
issue may appear when the reservations are made for different purpose. E.g.
if there was crash kernel allocation before the reservation of elfcorehdr.

The check in such case will prevent the second reservation, but, at least
in arch/arm64/mm/init.c:reserve_elfcorehdr() it does not seem to prevent
different users of the overlapping regions to step on each others toes.

Moreover, arm64::reserve_elfcorehdr() seems buggy to me, because of there
is only a partial overlap of the elfcorehdr with the previous reservation,
the non-overlapping part of elfcorehdr won't get reserved at all.

> Thanks!

> 

> >

> > > Note that mips_reserve_vmcore() cannot just be removed, as not all MIPS

> > > systems use DT.

> > >

> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > > ---

> > > v5:

> > >   - New.

> > > ---

> > >  arch/mips/kernel/setup.c | 3 ++-

> > >  1 file changed, 2 insertions(+), 1 deletion(-)

> > >

> > > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c

> > > index 23a140327a0bac1b..4693add05743d78b 100644

> > > --- a/arch/mips/kernel/setup.c

> > > +++ b/arch/mips/kernel/setup.c

> > > @@ -429,7 +429,8 @@ static void __init mips_reserve_vmcore(void)

> > >       pr_info("Reserving %ldKB of memory at %ldKB for kdump\n",

> > >               (unsigned long)elfcorehdr_size >> 10, (unsigned long)elfcorehdr_addr >> 10);

> > >

> > > -     memblock_reserve(elfcorehdr_addr, elfcorehdr_size);

> > > +     if (!memblock_is_region_reserved(elfcorehdr_addr, elfcorehdr_size)

> > > +             memblock_reserve(elfcorehdr_addr, elfcorehdr_size);

> > >  #endif

> > >  }

> 

> 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


-- 
Sincerely yours,
Mike.
Rob Herring Aug. 23, 2021, 2:44 p.m. UTC | #9
On Mon, Aug 23, 2021 at 8:10 AM Mike Rapoport <rppt@kernel.org> wrote:
>

> On Mon, Aug 23, 2021 at 12:17:50PM +0200, Geert Uytterhoeven wrote:

> > Hi Mike,

> >

> > On Mon, Aug 16, 2021 at 7:52 AM Mike Rapoport <rppt@kernel.org> wrote:

> > > On Wed, Aug 11, 2021 at 10:50:59AM +0200, Geert Uytterhoeven wrote:

> > > > Prepare for early_init_fdt_scan_reserved_mem() reserving the memory

> > > > occupied by an elf core header described in the device tree.

> > > > As arch_mem_init() calls early_init_fdt_scan_reserved_mem() before

> > > > mips_reserve_vmcore(), the latter needs to check if the memory has

> > > > already been reserved before.

> > >

> > > Doing memblock_reserve() for the same region is usually fine, did you

> > > encounter any issues without this patch?

> >

> > Does it also work if the same region is part of an earlier larger

> > reservation?  I am no memblock expert, so I don't know.

> > I didn't run into any issues, as my MIPS platform is non-DT, but I

> > assume arch/arm64/mm/init.c:reserve_elfcorehdr() had the check for

> > a reason.

>

> The memory will be reserved regardless of the earlier reservation, the

> issue may appear when the reservations are made for different purpose. E.g.

> if there was crash kernel allocation before the reservation of elfcorehdr.

>

> The check in such case will prevent the second reservation, but, at least

> in arch/arm64/mm/init.c:reserve_elfcorehdr() it does not seem to prevent

> different users of the overlapping regions to step on each others toes.


If the kernel has been passed in overlapping regions, is there
anything you can do other than hope to get a message out?

> Moreover, arm64::reserve_elfcorehdr() seems buggy to me, because of there

> is only a partial overlap of the elfcorehdr with the previous reservation,

> the non-overlapping part of elfcorehdr won't get reserved at all.


What do you suggest as the arm64 version is not the common version?

Rob
Rob Herring Aug. 23, 2021, 2:52 p.m. UTC | #10
On Mon, Aug 23, 2021 at 5:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Rob,

>

> On Sun, Aug 15, 2021 at 5:25 PM Rob Herring <robh@kernel.org> wrote:

> > On Wed, Aug 11, 2021 at 10:50:58AM +0200, Geert Uytterhoeven wrote:

> > > This patch series adds generic support for parsing DT properties related

> > > to crash dump kernels ("linux,elfcorehdr" and "linux,elfcorehdr" under

> > > the "/chosen" node), makes use of it on arm32, and performs a few

> > > cleanups.  It is an evolution of the combination of [1] and [2].

> >

> > The DT bits look fine to me. How do you expect this to be merged? I'm

> > happy to take it if arch maintainers can ack it.

>

> I had hoped you could take the series...


My current thought is I'll take 2-5, 7 and 8 given that's what I have
acks for and the others can be applied independently.

> > > The series consists of 6 parts:

> > >   1. Patch 1 prepares architecture-specific code (needed for MIPS only)

> > >      to avoid duplicating elf core header reservation later.

> > >   2. Patch 2 prepares the visibility of variables used to hold

> > >      information retrieved from the DT properties.

> > >   3. Patches 3-5 add support to the FDT core for handling the

> > >      properties.

> > >      This can co-exist safely with architecture-specific handling, until

> > >      the latter has been removed.

> >

> > Looks like patch 5 doesn't have any dependencies with the series?

>

> Indeed. So you can take it independently.

>

> > >   4. Patch 6 removes the non-standard handling of "linux,elfcorehdr" on

> > >      riscv.

> >

> > I thought this should be applied for 5.14?

>

> Me too, but unfortunately that hasn't happened yet...


Buried in the middle of this series is not going to encourage it to be
picked up as a fix.

Rob
Mike Rapoport Aug. 23, 2021, 3:20 p.m. UTC | #11
On Mon, Aug 23, 2021 at 09:44:55AM -0500, Rob Herring wrote:
> On Mon, Aug 23, 2021 at 8:10 AM Mike Rapoport <rppt@kernel.org> wrote:

> >

> > On Mon, Aug 23, 2021 at 12:17:50PM +0200, Geert Uytterhoeven wrote:

> > > Hi Mike,

> > >

> > > On Mon, Aug 16, 2021 at 7:52 AM Mike Rapoport <rppt@kernel.org> wrote:

> > > > On Wed, Aug 11, 2021 at 10:50:59AM +0200, Geert Uytterhoeven wrote:

> > > > > Prepare for early_init_fdt_scan_reserved_mem() reserving the memory

> > > > > occupied by an elf core header described in the device tree.

> > > > > As arch_mem_init() calls early_init_fdt_scan_reserved_mem() before

> > > > > mips_reserve_vmcore(), the latter needs to check if the memory has

> > > > > already been reserved before.

> > > >

> > > > Doing memblock_reserve() for the same region is usually fine, did you

> > > > encounter any issues without this patch?

> > >

> > > Does it also work if the same region is part of an earlier larger

> > > reservation?  I am no memblock expert, so I don't know.

> > > I didn't run into any issues, as my MIPS platform is non-DT, but I

> > > assume arch/arm64/mm/init.c:reserve_elfcorehdr() had the check for

> > > a reason.

> >

> > The memory will be reserved regardless of the earlier reservation, the

> > issue may appear when the reservations are made for different purpose. E.g.

> > if there was crash kernel allocation before the reservation of elfcorehdr.

> >

> > The check in such case will prevent the second reservation, but, at least

> > in arch/arm64/mm/init.c:reserve_elfcorehdr() it does not seem to prevent

> > different users of the overlapping regions to step on each others toes.

> 

> If the kernel has been passed in overlapping regions, is there

> anything you can do other than hope to get a message out?


Nothing really. I've been thinking about adding flags to memblock.reserved
to at least distinguish firmware regions from the kernel allocations, but I
never got to that.
 
> > Moreover, arm64::reserve_elfcorehdr() seems buggy to me, because of there

> > is only a partial overlap of the elfcorehdr with the previous reservation,

> > the non-overlapping part of elfcorehdr won't get reserved at all.

> 

> What do you suggest as the arm64 version is not the common version?


I'm not really familiar with crash dump internals, so I don't know if
resetting elfcorehdr_addr to ELFCORE_ADDR_ERR is a good idea. I think at
least arm64::reserve_elfcorehdr() should reserve the entire elfcorehdr area
regardless of the overlap. Otherwise it might get overwritten by a random
memblock_alloc().

> Rob


-- 
Sincerely yours,
Mike.
Geert Uytterhoeven Aug. 24, 2021, 11:55 a.m. UTC | #12
On Mon, Aug 23, 2021 at 4:52 PM Rob Herring <robh@kernel.org> wrote:
> On Mon, Aug 23, 2021 at 5:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Sun, Aug 15, 2021 at 5:25 PM Rob Herring <robh@kernel.org> wrote:

> > > On Wed, Aug 11, 2021 at 10:50:58AM +0200, Geert Uytterhoeven wrote:

> > > > This patch series adds generic support for parsing DT properties related

> > > > to crash dump kernels ("linux,elfcorehdr" and "linux,elfcorehdr" under

> > > > the "/chosen" node), makes use of it on arm32, and performs a few

> > > > cleanups.  It is an evolution of the combination of [1] and [2].

> > >

> > > The DT bits look fine to me. How do you expect this to be merged? I'm

> > > happy to take it if arch maintainers can ack it.

> >

> > I had hoped you could take the series...

>

> My current thought is I'll take 2-5, 7 and 8 given that's what I have

> acks for and the others can be applied independently.


Note that Palmer did ack patch 6, so you can include it.

Russell: any thoughts about patch 9?

Thanks!

> > > > The series consists of 6 parts:

> > > >   1. Patch 1 prepares architecture-specific code (needed for MIPS only)

> > > >      to avoid duplicating elf core header reservation later.

> > > >   2. Patch 2 prepares the visibility of variables used to hold

> > > >      information retrieved from the DT properties.

> > > >   3. Patches 3-5 add support to the FDT core for handling the

> > > >      properties.

> > > >      This can co-exist safely with architecture-specific handling, until

> > > >      the latter has been removed.

> > >

> > > Looks like patch 5 doesn't have any dependencies with the series?

> >

> > Indeed. So you can take it independently.

> >

> > > >   4. Patch 6 removes the non-standard handling of "linux,elfcorehdr" on

> > > >      riscv.

> > >

> > > I thought this should be applied for 5.14?

> >

> > Me too, but unfortunately that hasn't happened yet...

>

> Buried in the middle of this series is not going to encourage it to be

> picked up as a fix.


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
Rob Herring Aug. 24, 2021, 10:43 p.m. UTC | #13
On Tue, Aug 24, 2021 at 6:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> On Mon, Aug 23, 2021 at 4:52 PM Rob Herring <robh@kernel.org> wrote:

> > On Mon, Aug 23, 2021 at 5:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Sun, Aug 15, 2021 at 5:25 PM Rob Herring <robh@kernel.org> wrote:

> > > > On Wed, Aug 11, 2021 at 10:50:58AM +0200, Geert Uytterhoeven wrote:

> > > > > This patch series adds generic support for parsing DT properties related

> > > > > to crash dump kernels ("linux,elfcorehdr" and "linux,elfcorehdr" under

> > > > > the "/chosen" node), makes use of it on arm32, and performs a few

> > > > > cleanups.  It is an evolution of the combination of [1] and [2].

> > > >

> > > > The DT bits look fine to me. How do you expect this to be merged? I'm

> > > > happy to take it if arch maintainers can ack it.

> > >

> > > I had hoped you could take the series...

> >

> > My current thought is I'll take 2-5, 7 and 8 given that's what I have

> > acks for and the others can be applied independently.

>

> Note that Palmer did ack patch 6, so you can include it.


Right, but Palmer should have taken it if it's for 5.14.

Anyways, I've now applied patches 2-8. If we want to improve the
handling over what arm64 code did as discussed, I think that's a
separate patch anyways.

Rob