| Message ID | 92b6718f5618d5469f67b48fbea189cca0c12f4b.1628670468.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add generic support for kdump DT properties | expand |
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
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 >
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
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
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
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
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 }
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(-)