diff mbox series

[1/3] MIPS: Crash kernel should be able to see old memories

Message ID 1600828257-31316-1-git-send-email-chenhc@lemote.com (mailing list archive)
State Superseded
Headers show
Series [1/3] MIPS: Crash kernel should be able to see old memories | expand

Commit Message

Huacai Chen Sept. 23, 2020, 2:30 a.m. UTC
Kexec-tools use mem=X@Y to pass usable memories to crash kernel, but in
commit a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map") all
BIOS passed memories are removed by early_parse_mem(). I think this is
reasonable for a normal kernel but not for a crash kernel, because a
crash kernel should be able to see all old memories, even though it is
not supposed to use them.

Fixes: a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map")
Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 arch/mips/kernel/setup.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Baoquan He Sept. 23, 2020, 2:45 a.m. UTC | #1
On 09/23/20 at 10:30am, Huacai Chen wrote:
> Kexec-tools use mem=X@Y to pass usable memories to crash kernel, but in
> commit a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map") all
> BIOS passed memories are removed by early_parse_mem(). I think this is
> reasonable for a normal kernel but not for a crash kernel, because a
> crash kernel should be able to see all old memories, even though it is
> not supposed to use them.

I am not familiar with MIPS code, but we analyze and fill memmap= to
pass usable memory to crashkenrel in kexec-tools, do you mean you are
specifying memmap= or mem= by hand?

And we don't have mem=X@Y, only mem=nn[KMG].

> 
> Fixes: a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map")
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  arch/mips/kernel/setup.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 4c04a86..e2804a2 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -392,8 +392,10 @@ static int __init early_parse_mem(char *p)
>  	 */
>  	if (usermem == 0) {
>  		usermem = 1;
> +#ifndef CONFIG_CRASH_DUMP
>  		memblock_remove(memblock_start_of_DRAM(),
>  			memblock_end_of_DRAM() - memblock_start_of_DRAM());
> +#endif
>  	}
>  	start = 0;
>  	size = memparse(p, &p);
> -- 
> 2.7.0
>
Huacai Chen Sept. 23, 2020, 2:58 a.m. UTC | #2
Hi, Baoquan,

On Wed, Sep 23, 2020 at 10:46 AM Baoquan He <bhe@redhat.com> wrote:
>
> On 09/23/20 at 10:30am, Huacai Chen wrote:
> > Kexec-tools use mem=X@Y to pass usable memories to crash kernel, but in
> > commit a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map") all
> > BIOS passed memories are removed by early_parse_mem(). I think this is
> > reasonable for a normal kernel but not for a crash kernel, because a
> > crash kernel should be able to see all old memories, even though it is
> > not supposed to use them.
>
> I am not familiar with MIPS code, but we analyze and fill memmap= to
> pass usable memory to crashkenrel in kexec-tools, do you mean you are
> specifying memmap= or mem= by hand?
Not by hand, but by code of kexec-tools via the "mem=" parameter.

As I know, kexec-tools of MIPS only use "mem=" to pass "usable"
memory, but not "visible" memory. "Visible" memory of the crash kernel
is still passed by BIOS (strictly, by the old kernel who duplicates
information from BIOS). If memblock_remove() executed here, it would
remove all "visible" memory and make "visible" memory the same as
"usable" memory, and I think this is not correct.

>
> And we don't have mem=X@Y, only mem=nn[KMG].
The relocatable kernel of MIPS is still unusable now, so MIPS should
use mem=X@Y, and the crash kernel is always different from normal
kernel.

Huacai

>
> >
> > Fixes: a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map")
> > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > ---
> >  arch/mips/kernel/setup.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> > index 4c04a86..e2804a2 100644
> > --- a/arch/mips/kernel/setup.c
> > +++ b/arch/mips/kernel/setup.c
> > @@ -392,8 +392,10 @@ static int __init early_parse_mem(char *p)
> >        */
> >       if (usermem == 0) {
> >               usermem = 1;
> > +#ifndef CONFIG_CRASH_DUMP
> >               memblock_remove(memblock_start_of_DRAM(),
> >                       memblock_end_of_DRAM() - memblock_start_of_DRAM());
> > +#endif
> >       }
> >       start = 0;
> >       size = memparse(p, &p);
> > --
> > 2.7.0
> >
>
Jinyang He Sept. 23, 2020, 3:02 a.m. UTC | #3
On 09/23/2020 10:30 AM, Huacai Chen wrote:
> Kexec-tools use mem=X@Y to pass usable memories to crash kernel, but in
> commit a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map") all
> BIOS passed memories are removed by early_parse_mem(). I think this is
> reasonable for a normal kernel but not for a crash kernel, because a
> crash kernel should be able to see all old memories, even though it is
> not supposed to use them.
>
> Fixes: a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map")
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>   arch/mips/kernel/setup.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 4c04a86..e2804a2 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -392,8 +392,10 @@ static int __init early_parse_mem(char *p)
>   	 */
>   	if (usermem == 0) {
>   		usermem = 1;
> +#ifndef CONFIG_CRASH_DUMP
>   		memblock_remove(memblock_start_of_DRAM(),
>   			memblock_end_of_DRAM() - memblock_start_of_DRAM());
> +#endif

Hi, Huacai,

For this patch, I knew something what had happened. "mem=" parsing
works wrong for Loongson64. You can referenced the follow patch:
https://patchwork.kernel.org/patch/11789555/

memblock_add() calls memblock_add_range(,,, MAX_NUMNODES,)
For Loongson64 enabling NUMA, we need memblock_add_node(). (Or
using memblock_set_node() after memblock_add())

Besides, "mem=" may be useless for kdump. Youling had submitted a patch
about removing "mem="  serveral days ago.

For Loongson64 platform, you can try crashkernel=SIZE@38M after fixed 
"mem=".
38M means 40M - 2M, 2M is needed because old firmware compatibility.

Thanks,
- Jinyang.

>   	}
>   	start = 0;
>   	size = memparse(p, &p);
Baoquan He Sept. 23, 2020, 3:08 a.m. UTC | #4
On 09/23/20 at 10:58am, Huacai Chen wrote:
> Hi, Baoquan,
> 
> On Wed, Sep 23, 2020 at 10:46 AM Baoquan He <bhe@redhat.com> wrote:
> >
> > On 09/23/20 at 10:30am, Huacai Chen wrote:
> > > Kexec-tools use mem=X@Y to pass usable memories to crash kernel, but in
> > > commit a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map") all
> > > BIOS passed memories are removed by early_parse_mem(). I think this is
> > > reasonable for a normal kernel but not for a crash kernel, because a
> > > crash kernel should be able to see all old memories, even though it is
> > > not supposed to use them.
> >
> > I am not familiar with MIPS code, but we analyze and fill memmap= to
> > pass usable memory to crashkenrel in kexec-tools, do you mean you are
> > specifying memmap= or mem= by hand?
> Not by hand, but by code of kexec-tools via the "mem=" parameter.

OK, please ignore my comments.

> 
> As I know, kexec-tools of MIPS only use "mem=" to pass "usable"
> memory, but not "visible" memory. "Visible" memory of the crash kernel
> is still passed by BIOS (strictly, by the old kernel who duplicates
> information from BIOS). If memblock_remove() executed here, it would
> remove all "visible" memory and make "visible" memory the same as
> "usable" memory, and I think this is not correct.
> 
> >
> > And we don't have mem=X@Y, only mem=nn[KMG].
> The relocatable kernel of MIPS is still unusable now, so MIPS should
> use mem=X@Y, and the crash kernel is always different from normal
> kernel.

Interesting. Seems MIPS does support mem=X@Y, even though the document
of 'mem=' says it's used to specify amount of memory, but not memory
region. Anyway, leave this to mips reviewers, thanks for replying.

~~~~~~~~~~~~~~~~~~~
        mem=nn[KMG]     [KNL,BOOT] Force usage of a specific amount of memory
                        Amount of memory to be used in cases as follows:

> 
> >
> > >
> > > Fixes: a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map")
> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > > ---
> > >  arch/mips/kernel/setup.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> > > index 4c04a86..e2804a2 100644
> > > --- a/arch/mips/kernel/setup.c
> > > +++ b/arch/mips/kernel/setup.c
> > > @@ -392,8 +392,10 @@ static int __init early_parse_mem(char *p)
> > >        */
> > >       if (usermem == 0) {
> > >               usermem = 1;
> > > +#ifndef CONFIG_CRASH_DUMP
> > >               memblock_remove(memblock_start_of_DRAM(),
> > >                       memblock_end_of_DRAM() - memblock_start_of_DRAM());
> > > +#endif
> > >       }
> > >       start = 0;
> > >       size = memparse(p, &p);
> > > --
> > > 2.7.0
> > >
> >
>
Huacai Chen Sept. 23, 2020, 3:13 a.m. UTC | #5
Hi, Jinyang,

On Wed, Sep 23, 2020 at 11:02 AM Jinyang He <hejinyang@loongson.cn> wrote:
>
> On 09/23/2020 10:30 AM, Huacai Chen wrote:
> > Kexec-tools use mem=X@Y to pass usable memories to crash kernel, but in
> > commit a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map") all
> > BIOS passed memories are removed by early_parse_mem(). I think this is
> > reasonable for a normal kernel but not for a crash kernel, because a
> > crash kernel should be able to see all old memories, even though it is
> > not supposed to use them.
> >
> > Fixes: a94e4f24ec836c8984f83959 ("MIPS: init: Drop boot_mem_map")
> > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > ---
> >   arch/mips/kernel/setup.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> > index 4c04a86..e2804a2 100644
> > --- a/arch/mips/kernel/setup.c
> > +++ b/arch/mips/kernel/setup.c
> > @@ -392,8 +392,10 @@ static int __init early_parse_mem(char *p)
> >        */
> >       if (usermem == 0) {
> >               usermem = 1;
> > +#ifndef CONFIG_CRASH_DUMP
> >               memblock_remove(memblock_start_of_DRAM(),
> >                       memblock_end_of_DRAM() - memblock_start_of_DRAM());
> > +#endif
>
> Hi, Huacai,
>
> For this patch, I knew something what had happened. "mem=" parsing
> works wrong for Loongson64. You can referenced the follow patch:
> https://patchwork.kernel.org/patch/11789555/
>
> memblock_add() calls memblock_add_range(,,, MAX_NUMNODES,)
> For Loongson64 enabling NUMA, we need memblock_add_node(). (Or
> using memblock_set_node() after memblock_add())
This seems like another story.

>
> Besides, "mem=" may be useless for kdump. Youling had submitted a patch
> about removing "mem="  serveral days ago.
kexec-tools use "mem=" to pass usable memory for the crash kernel,
Youling removes the base address of "mem=", but "mem=" is still here.

>
> For Loongson64 platform, you can try crashkernel=SIZE@38M after fixed
> "mem=".
> 38M means 40M - 2M, 2M is needed because old firmware compatibility.
"crashkernel=" is for the normal kernel specified by BIOS, and "mem="
is for the crash kernel specified by kexec-tools.

Huacai
>
> Thanks,
> - Jinyang.
>
> >       }
> >       start = 0;
> >       size = memparse(p, &p);
>
Maciej W. Rozycki Oct. 3, 2020, 9:07 p.m. UTC | #6
On Wed, 23 Sep 2020, Baoquan He wrote:

> > > And we don't have mem=X@Y, only mem=nn[KMG].
> > The relocatable kernel of MIPS is still unusable now, so MIPS should
> > use mem=X@Y, and the crash kernel is always different from normal
> > kernel.
> 
> Interesting. Seems MIPS does support mem=X@Y, even though the document
> of 'mem=' says it's used to specify amount of memory, but not memory
> region. Anyway, leave this to mips reviewers, thanks for replying.
> 
> ~~~~~~~~~~~~~~~~~~~
>         mem=nn[KMG]     [KNL,BOOT] Force usage of a specific amount of memory
>                         Amount of memory to be used in cases as follows:

 Yep, I implemented it for the DECstation back in 2000:

commit 97b7ae4257ef7ba8ed9b7944a4f56a49af3e8abb
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Mon Dec 11 16:41:05 2000 +0000

    Memmap fixes from Maciej.

(from the LMO repo) in line with the x86 syntax.  I don't know why it 
hasn't ever been documented in "Kernel Parameters" (for any port), but I'm 
fairly sure it has been somewhere.

 FWIW,

  Maciej
Maciej W. Rozycki Oct. 5, 2020, 12:40 a.m. UTC | #7
On Sat, 3 Oct 2020, Maciej W. Rozycki wrote:

> > Interesting. Seems MIPS does support mem=X@Y, even though the document
> > of 'mem=' says it's used to specify amount of memory, but not memory
> > region. Anyway, leave this to mips reviewers, thanks for replying.
> > 
> > ~~~~~~~~~~~~~~~~~~~
> >         mem=nn[KMG]     [KNL,BOOT] Force usage of a specific amount of memory
> >                         Amount of memory to be used in cases as follows:
> 
>  Yep, I implemented it for the DECstation back in 2000:
> 
> commit 97b7ae4257ef7ba8ed9b7944a4f56a49af3e8abb
> Author: Ralf Baechle <ralf@linux-mips.org>
> Date:   Mon Dec 11 16:41:05 2000 +0000
> 
>     Memmap fixes from Maciej.
> 
> (from the LMO repo) in line with the x86 syntax.  I don't know why it 
> hasn't ever been documented in "Kernel Parameters" (for any port), but I'm 
> fairly sure it has been somewhere.

 Self-correction: documentation used to be there, but was removed around 
Linux 2.5.65:

commit 041a679cb20e4fcb841665b09cf7c6d24ab4ad39
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Thu Jun 5 00:04:28 2003 +0000

    Merge with Linux 2.5.65.

(same repo) as the parameter was renamed to `memmap=' in the x86 port.  
Obviously whoever did that did not bother to check other ports, even 
though the parameter was marked (and `memmap=' still is) generic.

  Maciej
diff mbox series

Patch

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 4c04a86..e2804a2 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -392,8 +392,10 @@  static int __init early_parse_mem(char *p)
 	 */
 	if (usermem == 0) {
 		usermem = 1;
+#ifndef CONFIG_CRASH_DUMP
 		memblock_remove(memblock_start_of_DRAM(),
 			memblock_end_of_DRAM() - memblock_start_of_DRAM());
+#endif
 	}
 	start = 0;
 	size = memparse(p, &p);