2022-03-01 07:05:25

by Tiezhu Yang

[permalink] [raw]
Subject: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

According to Documentation/admin-guide/kernel-parameters.txt,
the kernel command-line parameter mem= means "Force usage of
a specific amount of memory", but when add "mem=3G" to the
command-line, kernel boot hangs in sparse_init().

This commit is similar with the implementation of the other
archs such as arm64, powerpc and riscv, refactor the function
early_parse_mem() and then use memblock_enforce_memory_limit()
to limit the memory size.

With this patch, when add "mem=3G" to the command-line, the
kernel boots successfully, we can see the following messages:

[ 0.000000] Memory limited to 3072MB
...
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000000200000-0x000000000effffff]
[ 0.000000] node 0: [mem 0x0000000090200000-0x00000000ffffffff]
[ 0.000000] node 0: [mem 0x0000000120000000-0x00000001613fffff]
...
[ 0.000000] Memory: 3005280K/3145728K available (...)

After login, the output of free command is consistent with the
above log.

Signed-off-by: Tiezhu Yang <[email protected]>
---
arch/mips/kernel/setup.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index f979adf..50396ba 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -339,27 +339,15 @@ static void __init bootmem_init(void)
#endif /* CONFIG_SGI_IP27 */

static int usermem __initdata;
+static phys_addr_t memory_limit;

static int __init early_parse_mem(char *p)
{
- phys_addr_t start, size;
-
- /*
- * If a user specifies memory size, we
- * blow away any automatically generated
- * size.
- */
- if (usermem == 0) {
- usermem = 1;
- memblock_remove(memblock_start_of_DRAM(),
- memblock_end_of_DRAM() - memblock_start_of_DRAM());
- }
- start = 0;
- size = memparse(p, &p);
- if (*p == '@')
- start = memparse(p + 1, &p);
+ if (!p)
+ return 1;

- memblock_add(start, size);
+ memory_limit = memparse(p, &p) & PAGE_MASK;
+ pr_notice("Memory limited to %lluMB\n", (u64)memory_limit >> 20);

return 0;
}
@@ -678,6 +666,10 @@ static void __init arch_mem_init(char **cmdline_p)
memblock_reserve(__pa_symbol(&__nosave_begin),
__pa_symbol(&__nosave_end) - __pa_symbol(&__nosave_begin));

+ /* Limit the memory. */
+ memblock_enforce_memory_limit(memory_limit);
+ memblock_allow_resize();
+
early_memtest(PFN_PHYS(ARCH_PFN_OFFSET), PFN_PHYS(max_low_pfn));
}

--
2.1.0


2022-03-04 19:56:34

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Fri, 4 Mar 2022, Thomas Bogendoerfer wrote:

> > > With this patch, when add "mem=3G" to the command-line, the
> > > kernel boots successfully, we can see the following messages:
> >
> > unfortunately this patch would break platforms without memory detection,
> > which simply use mem=32M for memory configuration. Not sure how many
> > rely on this mechanism. If we can make sure nobody uses it, I'm fine
> > with your patch.
>
> maybe we could add a CONFIG option, which will be selected by
> platforms, which don't need/want this usermem thing.

FWIW I don't understand what the issue is here beyond that we have a bug
that causes a system to hang when "mem=3G" is passed on the kernel command
line. That is assuming that system does have contiguous RAM available for
the kernel to use from address 0 up to 3GiB; otherwise it's a user error
to tell the kernel it has that memory available (I did get bitten by that
myself too): garbage in, garbage out.

I think having a CONFIG option automatically selected to disable the
ability to give a memory map override would handicap people in debugging
their systems or working around firmware bugs, so I would rather be
against it.

Maciej

2022-03-04 20:22:05

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Tue, Mar 01, 2022 at 12:28:58PM +0800, Tiezhu Yang wrote:
> According to Documentation/admin-guide/kernel-parameters.txt,
> the kernel command-line parameter mem= means "Force usage of
> a specific amount of memory", but when add "mem=3G" to the
> command-line, kernel boot hangs in sparse_init().
>
> This commit is similar with the implementation of the other
> archs such as arm64, powerpc and riscv, refactor the function
> early_parse_mem() and then use memblock_enforce_memory_limit()
> to limit the memory size.
>
> With this patch, when add "mem=3G" to the command-line, the
> kernel boots successfully, we can see the following messages:

unfortunately this patch would break platforms without memory detection,
which simply use mem=32M for memory configuration. Not sure how many
rely on this mechanism. If we can make sure nobody uses it, I'm fine
with your patch.

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2022-03-04 20:54:08

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Fri, Mar 04, 2022 at 04:10:52PM +0100, Thomas Bogendoerfer wrote:
> On Tue, Mar 01, 2022 at 12:28:58PM +0800, Tiezhu Yang wrote:
> > According to Documentation/admin-guide/kernel-parameters.txt,
> > the kernel command-line parameter mem= means "Force usage of
> > a specific amount of memory", but when add "mem=3G" to the
> > command-line, kernel boot hangs in sparse_init().
> >
> > This commit is similar with the implementation of the other
> > archs such as arm64, powerpc and riscv, refactor the function
> > early_parse_mem() and then use memblock_enforce_memory_limit()
> > to limit the memory size.
> >
> > With this patch, when add "mem=3G" to the command-line, the
> > kernel boots successfully, we can see the following messages:
>
> unfortunately this patch would break platforms without memory detection,
> which simply use mem=32M for memory configuration. Not sure how many
> rely on this mechanism. If we can make sure nobody uses it, I'm fine
> with your patch.

maybe we could add a CONFIG option, which will be selected by
platforms, which don't need/want this usermem thing.

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2022-03-05 14:16:33

by Mike Rapoport

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Fri, Mar 04, 2022 at 05:11:44PM +0000, Maciej W. Rozycki wrote:
> On Fri, 4 Mar 2022, Thomas Bogendoerfer wrote:
>
> > > > With this patch, when add "mem=3G" to the command-line, the
> > > > kernel boots successfully, we can see the following messages:
> > >
> > > unfortunately this patch would break platforms without memory detection,
> > > which simply use mem=32M for memory configuration. Not sure how many
> > > rely on this mechanism. If we can make sure nobody uses it, I'm fine
> > > with your patch.
> >
> > maybe we could add a CONFIG option, which will be selected by
> > platforms, which don't need/want this usermem thing.
>
> FWIW I don't understand what the issue is here beyond that we have a bug
> that causes a system to hang when "mem=3G" is passed on the kernel command
> line. That is assuming that system does have contiguous RAM available for
> the kernel to use from address 0 up to 3GiB; otherwise it's a user error
> to tell the kernel it has that memory available (I did get bitten by that
> myself too): garbage in, garbage out.

This is exactly the case here because the system does not have contiguous
RAM:

[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000004000000-0x000000000effffff]
[ 0.000000] node 0: [mem 0x0000000090200000-0x00000000ffffffff]
[ 0.000000] node 0: [mem 0x0000000120000000-0x00000001653fffff]

(from patch 3/4 in this series)

I don't see what "bug" this patch is trying to fix. Is indeed possible to
make MIPS' mem= behave like it does not arm64 and ppc, but that would break
systems that use current semantics and I recall seeing some of OpenWRT
machines using mem= to override memory map supplied by firmware.

> Maciej

--
Sincerely yours,
Mike.

2022-03-07 18:17:44

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Fri, Mar 04, 2022 at 05:11:44PM +0000, Maciej W. Rozycki wrote:
> On Fri, 4 Mar 2022, Thomas Bogendoerfer wrote:
>
> > > > With this patch, when add "mem=3G" to the command-line, the
> > > > kernel boots successfully, we can see the following messages:
> > >
> > > unfortunately this patch would break platforms without memory detection,
> > > which simply use mem=32M for memory configuration. Not sure how many
> > > rely on this mechanism. If we can make sure nobody uses it, I'm fine
> > > with your patch.
> >
> > maybe we could add a CONFIG option, which will be selected by
> > platforms, which don't need/want this usermem thing.
>
> FWIW I don't understand what the issue is here beyond that we have a bug
> that causes a system to hang when "mem=3G" is passed on the kernel command
> line. That is assuming that system does have contiguous RAM available for
> the kernel to use from address 0 up to 3GiB; otherwise it's a user error
> to tell the kernel it has that memory available (I did get bitten by that
> myself too): garbage in, garbage out.

I did a quick test with an IP30:

>> bootp(): ip=dhcp root=/dev/nfs console=ttyS0 mem=384M
Setting $netaddr to 192.168.8.208 (from server )
Obtaining from server
9012640+181664 entry: 0xa800000020664a60
Linux version 5.17.0-rc3+ (tbogendoerfer@adalid) (mips64-linux-gnu-gcc (GCC) 6.1.1 20160621 (Red Hat Cross 6.1.1-2), GNU ld version 2.27-3.fc24) #155 SMP Mon Mar 7 13:12:01 CET 2022
ARCH: SGI-IP30
PROMLIB: ARC firmware Version 64 Revision 0
printk: bootconsole [early0] enabled
CPU0 revision is: 00000934 (R10000)
FPU revision is: 00000900
Detected 512MB of physical memory.
User-defined physical RAM map overwrite
Kernel sections are not in the memory maps
IP30: Slot: 0, PrID: 00000934, PhyID: 0, VirtID: 0
IP30: Slot: 1, PrID: 00000934, PhyID: 1, VirtID: 1
IP30: Detected 2 CPU(s) present.
Primary instruction cache 32kB, VIPT, 2-way, linesize 64 bytes.
Primary data cache 32kB, 2-way, VIPT, no aliases, linesize 32 bytes
Unified secondary cache 1024kB 2-way, linesize 128 bytes.
Zone ranges:
DMA32 [mem 0x0000000000000000-0x00000000ffffffff]
Normal empty
Movable zone start for each node
Early memory node ranges
node 0: [mem 0x0000000000000000-0x0000000017ffffff]
node 0: [mem 0x0000000020004000-0x00000000208c7fff]
Initmem setup node 0 [mem 0x0000000000000000-0x00000000208c7fff]

after that it's dead (it doesn't have memory starting at 0x0).
Most SGI systems will act broken with mem= in one way or another.
And I already had the need to limit the amount of memory.

> I think having a CONFIG option automatically selected to disable the
> ability to give a memory map override would handicap people in debugging
> their systems or working around firmware bugs, so I would rather be
> against it.

I'm thinking about a CONFIG option, which isn't user selectable, but
selected via Kconfig only. But that would give to differents semantics
for mem=

So can I just limit amount of memory without interfering with normal
memory detection ?

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2022-03-08 01:36:34

by Mike Rapoport

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Mon, Mar 07, 2022 at 05:29:09PM +0100, Thomas Bogendoerfer wrote:
> On Fri, Mar 04, 2022 at 05:11:44PM +0000, Maciej W. Rozycki wrote:
> > On Fri, 4 Mar 2022, Thomas Bogendoerfer wrote:
> >
> > > > > With this patch, when add "mem=3G" to the command-line, the
> > > > > kernel boots successfully, we can see the following messages:
> > > >
> > > > unfortunately this patch would break platforms without memory detection,
> > > > which simply use mem=32M for memory configuration. Not sure how many
> > > > rely on this mechanism. If we can make sure nobody uses it, I'm fine
> > > > with your patch.
> > >
> > > maybe we could add a CONFIG option, which will be selected by
> > > platforms, which don't need/want this usermem thing.
> >
> > FWIW I don't understand what the issue is here beyond that we have a bug
> > that causes a system to hang when "mem=3G" is passed on the kernel command
> > line. That is assuming that system does have contiguous RAM available for
> > the kernel to use from address 0 up to 3GiB; otherwise it's a user error
> > to tell the kernel it has that memory available (I did get bitten by that
> > myself too): garbage in, garbage out.
>
> I did a quick test with an IP30:
>
> >> bootp(): ip=dhcp root=/dev/nfs console=ttyS0 mem=384M
> Setting $netaddr to 192.168.8.208 (from server )
> Obtaining from server
> 9012640+181664 entry: 0xa800000020664a60
> Linux version 5.17.0-rc3+ (tbogendoerfer@adalid) (mips64-linux-gnu-gcc (GCC) 6.1.1 20160621 (Red Hat Cross 6.1.1-2), GNU ld version 2.27-3.fc24) #155 SMP Mon Mar 7 13:12:01 CET 2022
> ARCH: SGI-IP30
> PROMLIB: ARC firmware Version 64 Revision 0
> printk: bootconsole [early0] enabled
> CPU0 revision is: 00000934 (R10000)
> FPU revision is: 00000900
> Detected 512MB of physical memory.
> User-defined physical RAM map overwrite
> Kernel sections are not in the memory maps
> IP30: Slot: 0, PrID: 00000934, PhyID: 0, VirtID: 0
> IP30: Slot: 1, PrID: 00000934, PhyID: 1, VirtID: 1
> IP30: Detected 2 CPU(s) present.
> Primary instruction cache 32kB, VIPT, 2-way, linesize 64 bytes.
> Primary data cache 32kB, 2-way, VIPT, no aliases, linesize 32 bytes
> Unified secondary cache 1024kB 2-way, linesize 128 bytes.
> Zone ranges:
> DMA32 [mem 0x0000000000000000-0x00000000ffffffff]
> Normal empty
> Movable zone start for each node
> Early memory node ranges
> node 0: [mem 0x0000000000000000-0x0000000017ffffff]
> node 0: [mem 0x0000000020004000-0x00000000208c7fff]
> Initmem setup node 0 [mem 0x0000000000000000-0x00000000208c7fff]
>
> after that it's dead (it doesn't have memory starting at 0x0).
> Most SGI systems will act broken with mem= in one way or another.
> And I already had the need to limit the amount of memory.
>
> > I think having a CONFIG option automatically selected to disable the
> > ability to give a memory map override would handicap people in debugging
> > their systems or working around firmware bugs, so I would rather be
> > against it.
>
> I'm thinking about a CONFIG option, which isn't user selectable, but
> selected via Kconfig only. But that would give to differents semantics
> for mem=
>
> So can I just limit amount of memory without interfering with normal
> memory detection ?

Maybe it's better to add a new encoding to mem= that will have the semantics
of limiting amount of memory?

E.g.

mem=384M@

would mean "only use 384M of memory that firmware reported" while

mem=384M would mean "set memory to 0 - 384M" as it does now.

I think it's fine to have this MIPS specific because there is anyway no
consistency among architectures in mem= handling.

> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]

--
Sincerely yours,
Mike.

2022-03-09 00:18:13

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH v4 1/4] MIPS: Refactor early_parse_mem() to fix mem= parameter

On Tue, 8 Mar 2022, Mike Rapoport wrote:

> > So can I just limit amount of memory without interfering with normal
> > memory detection ?
>
> Maybe it's better to add a new encoding to mem= that will have the semantics
> of limiting amount of memory?
>
> E.g.
>
> mem=384M@
>
> would mean "only use 384M of memory that firmware reported" while
>
> mem=384M would mean "set memory to 0 - 384M" as it does now.

I think you're going in the right direction, we'd just need to sort out
the most reasonable syntax for the new semantics; `mem=384M@' just seems
too analogous to me to `mem=384M@0'. Maybe `mem=384M-'?

NB that would have to work with the existing overrides, for e.g.:

`mem=192M@0 mem=192M@256M mem=384M-'

to produce the following memory ranges available for use:

Normal [mem 0x0000000000000000-0x000000000bffffff]
Normal [mem 0x0000000010000000-0x0000000017ffffff]

(so that you can paste the final cap at some command prompt and still have
earlier parameters respected that may have been passed by the firmware or
bootloader, or built in).

Maciej