[PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[Yunhui Cui]

Implement support for parsing 'memmap' kernel command line parameter.

This patch covers parsing of the following two formats for 'memmap'
parameter values:

- nn[KMG]@ss[KMG]
- nn[KMG]$ss[KMG]

([KMG] = K M or G (kilo, mega, giga))

These two allowed formats for parameter value are already documented
in file kernel-parameters.txt in Documentation/admin-guide folder.
Some architectures already support them, but Mips did not prior to
this patch.

Excerpt from Documentation/admin-guide/kernel-parameters.txt:

memmap=nn[KMG]@ss[KMG]
[KNL] Force usage of a specific region of memory.
Region of memory to be used is from ss to ss+nn.

memmap=nn[KMG]$ss[KMG]
Mark specific memory as reserved.
Region of memory to be reserved is from ss to ss+nn.
Example: Exclude memory from 0x18690000-0x1869ffff
memmap=64K$0x18690000
or
memmap=0x10000$0x18690000

There is no need to update this documentation file with respect to
this patch.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/mm/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index e3405e4b99af..7be7ec3092ad 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -208,6 +208,56 @@ static int __init early_mem(char *p)
 }
 early_param("mem", early_mem);
 
+static void __init parse_memmap_one(char *p)
+{
+	char *oldp;
+	unsigned long start_at, mem_size;
+
+	if (!p)
+		return;
+
+	oldp = p;
+	mem_size = memparse(p, &p);
+	if (p == oldp)
+		return;
+
+	switch (*p) {
+	case '@':
+		start_at = memparse(p + 1, &p);
+		memblock_add(start_at, mem_size);
+		break;
+
+	case '$':
+		start_at = memparse(p + 1, &p);
+		memblock_reserve(start_at, mem_size);
+		break;
+
+	case 0:
+		memblock_reserve(mem_size, -mem_size);
+		break;
+
+	default:
+		pr_warn("Unrecognized memmap syntax: %s\n", p);
+		break;
+	}
+}
+
+static int __init parse_memmap_opt(char *str)
+{
+	while (str) {
+		char *k = strchr(str, ',');
+
+		if (k)
+			*k++ = 0;
+
+		parse_memmap_one(str);
+		str = k;
+	}
+
+	return 0;
+}
+early_param("memmap", parse_memmap_opt);
+
 static void __init setup_bootmem(void)
 {
 	phys_addr_t vmlinux_end = __pa_symbol(&_end);
-- 
2.20.1

Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[Charlie Jenkins]

On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> Implement support for parsing 'memmap' kernel command line parameter.
> 
> This patch covers parsing of the following two formats for 'memmap'
> parameter values:
> 
> - nn[KMG]@ss[KMG]
> - nn[KMG]$ss[KMG]
> 
> ([KMG] = K M or G (kilo, mega, giga))
> 
> These two allowed formats for parameter value are already documented
> in file kernel-parameters.txt in Documentation/admin-guide folder.
> Some architectures already support them, but Mips did not prior to

Copy-paste from a Mips patch? Should say riscv :)

It looks like this code is duplicated from xtensa and is effectively the
same as mips. Can this code be placed in a generic file so that the code
can be shared between mips, riscv, and xtensa -- maybe a new config that
gets selected by mips/riscv/xtensa?

- Charlie

> this patch.
> 
> Excerpt from Documentation/admin-guide/kernel-parameters.txt:
> 
> memmap=nn[KMG]@ss[KMG]
> [KNL] Force usage of a specific region of memory.
> Region of memory to be used is from ss to ss+nn.
> 
> memmap=nn[KMG]$ss[KMG]
> Mark specific memory as reserved.
> Region of memory to be reserved is from ss to ss+nn.
> Example: Exclude memory from 0x18690000-0x1869ffff
> memmap=64K$0x18690000
> or
> memmap=0x10000$0x18690000
> 
> There is no need to update this documentation file with respect to
> this patch.
> 
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  arch/riscv/mm/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index e3405e4b99af..7be7ec3092ad 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
>  }
>  early_param("mem", early_mem);
>  
> +static void __init parse_memmap_one(char *p)
> +{
> +	char *oldp;
> +	unsigned long start_at, mem_size;
> +
> +	if (!p)
> +		return;
> +
> +	oldp = p;
> +	mem_size = memparse(p, &p);
> +	if (p == oldp)
> +		return;
> +
> +	switch (*p) {
> +	case '@':
> +		start_at = memparse(p + 1, &p);
> +		memblock_add(start_at, mem_size);
> +		break;
> +
> +	case '$':
> +		start_at = memparse(p + 1, &p);
> +		memblock_reserve(start_at, mem_size);
> +		break;
> +
> +	case 0:
> +		memblock_reserve(mem_size, -mem_size);
> +		break;
> +
> +	default:
> +		pr_warn("Unrecognized memmap syntax: %s\n", p);
> +		break;
> +	}
> +}
> +
> +static int __init parse_memmap_opt(char *str)
> +{
> +	while (str) {
> +		char *k = strchr(str, ',');
> +
> +		if (k)
> +			*k++ = 0;
> +
> +		parse_memmap_one(str);
> +		str = k;
> +	}
> +
> +	return 0;
> +}
> +early_param("memmap", parse_memmap_opt);
> +
>  static void __init setup_bootmem(void)
>  {
>  	phys_addr_t vmlinux_end = __pa_symbol(&_end);
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[yunhui cui]

Hi Charlie,

On Fri, Jun 21, 2024 at 9:03 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> > Implement support for parsing 'memmap' kernel command line parameter.
> >
> > This patch covers parsing of the following two formats for 'memmap'
> > parameter values:
> >
> > - nn[KMG]@ss[KMG]
> > - nn[KMG]$ss[KMG]
> >
> > ([KMG] = K M or G (kilo, mega, giga))
> >
> > These two allowed formats for parameter value are already documented
> > in file kernel-parameters.txt in Documentation/admin-guide folder.
> > Some architectures already support them, but Mips did not prior to
>
> Copy-paste from a Mips patch? Should say riscv :)
>
> It looks like this code is duplicated from xtensa and is effectively the
> same as mips. Can this code be placed in a generic file so that the code
> can be shared between mips, riscv, and xtensa -- maybe a new config that
> gets selected by mips/riscv/xtensa?

Yeah, that's actually what I was thinking. Which general file do you
think would be more suitable to put it in?

> - Charlie
>
> > this patch.
> >
> > Excerpt from Documentation/admin-guide/kernel-parameters.txt:
> >
> > memmap=nn[KMG]@ss[KMG]
> > [KNL] Force usage of a specific region of memory.
> > Region of memory to be used is from ss to ss+nn.
> >
> > memmap=nn[KMG]$ss[KMG]
> > Mark specific memory as reserved.
> > Region of memory to be reserved is from ss to ss+nn.
> > Example: Exclude memory from 0x18690000-0x1869ffff
> > memmap=64K$0x18690000
> > or
> > memmap=0x10000$0x18690000
> >
> > There is no need to update this documentation file with respect to
> > this patch.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >  arch/riscv/mm/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index e3405e4b99af..7be7ec3092ad 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
> >  }
> >  early_param("mem", early_mem);
> >
> > +static void __init parse_memmap_one(char *p)
> > +{
> > +     char *oldp;
> > +     unsigned long start_at, mem_size;
> > +
> > +     if (!p)
> > +             return;
> > +
> > +     oldp = p;
> > +     mem_size = memparse(p, &p);
> > +     if (p == oldp)
> > +             return;
> > +
> > +     switch (*p) {
> > +     case '@':
> > +             start_at = memparse(p + 1, &p);
> > +             memblock_add(start_at, mem_size);
> > +             break;
> > +
> > +     case '$':
> > +             start_at = memparse(p + 1, &p);
> > +             memblock_reserve(start_at, mem_size);
> > +             break;
> > +
> > +     case 0:
> > +             memblock_reserve(mem_size, -mem_size);
> > +             break;
> > +
> > +     default:
> > +             pr_warn("Unrecognized memmap syntax: %s\n", p);
> > +             break;
> > +     }
> > +}
> > +
> > +static int __init parse_memmap_opt(char *str)
> > +{
> > +     while (str) {
> > +             char *k = strchr(str, ',');
> > +
> > +             if (k)
> > +                     *k++ = 0;
> > +
> > +             parse_memmap_one(str);
> > +             str = k;
> > +     }
> > +
> > +     return 0;
> > +}
> > +early_param("memmap", parse_memmap_opt);
> > +
> >  static void __init setup_bootmem(void)
> >  {
> >       phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks,
Yunhui

Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[Charlie Jenkins]

On Fri, Jun 21, 2024 at 10:08:39AM +0800, yunhui cui wrote:
> Hi Charlie,
> 
> On Fri, Jun 21, 2024 at 9:03 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> > > Implement support for parsing 'memmap' kernel command line parameter.
> > >
> > > This patch covers parsing of the following two formats for 'memmap'
> > > parameter values:
> > >
> > > - nn[KMG]@ss[KMG]
> > > - nn[KMG]$ss[KMG]
> > >
> > > ([KMG] = K M or G (kilo, mega, giga))
> > >
> > > These two allowed formats for parameter value are already documented
> > > in file kernel-parameters.txt in Documentation/admin-guide folder.
> > > Some architectures already support them, but Mips did not prior to
> >
> > Copy-paste from a Mips patch? Should say riscv :)
> >
> > It looks like this code is duplicated from xtensa and is effectively the
> > same as mips. Can this code be placed in a generic file so that the code
> > can be shared between mips, riscv, and xtensa -- maybe a new config that
> > gets selected by mips/riscv/xtensa?
> 
> Yeah, that's actually what I was thinking. Which general file do you
> think would be more suitable to put it in?

I am not sure the best place to put it. What do you think about
mm/memblock.c next to the "memblock" early param?

> 
> > - Charlie
> >
> > > this patch.
> > >
> > > Excerpt from Documentation/admin-guide/kernel-parameters.txt:
> > >
> > > memmap=nn[KMG]@ss[KMG]
> > > [KNL] Force usage of a specific region of memory.
> > > Region of memory to be used is from ss to ss+nn.
> > >
> > > memmap=nn[KMG]$ss[KMG]
> > > Mark specific memory as reserved.
> > > Region of memory to be reserved is from ss to ss+nn.
> > > Example: Exclude memory from 0x18690000-0x1869ffff
> > > memmap=64K$0x18690000
> > > or
> > > memmap=0x10000$0x18690000
> > >
> > > There is no need to update this documentation file with respect to
> > > this patch.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > ---
> > >  arch/riscv/mm/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index e3405e4b99af..7be7ec3092ad 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
> > >  }
> > >  early_param("mem", early_mem);
> > >
> > > +static void __init parse_memmap_one(char *p)
> > > +{
> > > +     char *oldp;
> > > +     unsigned long start_at, mem_size;
> > > +
> > > +     if (!p)
> > > +             return;
> > > +
> > > +     oldp = p;
> > > +     mem_size = memparse(p, &p);
> > > +     if (p == oldp)
> > > +             return;
> > > +
> > > +     switch (*p) {
> > > +     case '@':
> > > +             start_at = memparse(p + 1, &p);
> > > +             memblock_add(start_at, mem_size);
> > > +             break;
> > > +
> > > +     case '$':
> > > +             start_at = memparse(p + 1, &p);
> > > +             memblock_reserve(start_at, mem_size);
> > > +             break;
> > > +
> > > +     case 0:
> > > +             memblock_reserve(mem_size, -mem_size);
> > > +             break;
> > > +
> > > +     default:
> > > +             pr_warn("Unrecognized memmap syntax: %s\n", p);
> > > +             break;
> > > +     }
> > > +}
> > > +
> > > +static int __init parse_memmap_opt(char *str)
> > > +{
> > > +     while (str) {
> > > +             char *k = strchr(str, ',');
> > > +
> > > +             if (k)
> > > +                     *k++ = 0;
> > > +
> > > +             parse_memmap_one(str);
> > > +             str = k;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +early_param("memmap", parse_memmap_opt);
> > > +
> > >  static void __init setup_bootmem(void)
> > >  {
> > >       phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > > --
> > > 2.20.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> Thanks,
> Yunhui

Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[yunhui cui]

Hi Charlie，

On Fri, Jun 21, 2024 at 11:10 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Jun 21, 2024 at 10:08:39AM +0800, yunhui cui wrote:
> > Hi Charlie,
> >
> > On Fri, Jun 21, 2024 at 9:03 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> > > > Implement support for parsing 'memmap' kernel command line parameter.
> > > >
> > > > This patch covers parsing of the following two formats for 'memmap'
> > > > parameter values:
> > > >
> > > > - nn[KMG]@ss[KMG]
> > > > - nn[KMG]$ss[KMG]
> > > >
> > > > ([KMG] = K M or G (kilo, mega, giga))
> > > >
> > > > These two allowed formats for parameter value are already documented
> > > > in file kernel-parameters.txt in Documentation/admin-guide folder.
> > > > Some architectures already support them, but Mips did not prior to
> > >
> > > Copy-paste from a Mips patch? Should say riscv :)
> > >
> > > It looks like this code is duplicated from xtensa and is effectively the
> > > same as mips. Can this code be placed in a generic file so that the code
> > > can be shared between mips, riscv, and xtensa -- maybe a new config that
> > > gets selected by mips/riscv/xtensa?
> >
> > Yeah, that's actually what I was thinking. Which general file do you
> > think would be more suitable to put it in?
>
> I am not sure the best place to put it. What do you think about
> mm/memblock.c next to the "memblock" early param?

Is it inappropriate to put it in memblock? The implementation of mips
is different from that of xtensa, and early_mem is also distributed in
various archs, so we still put memmap in riscv/, and then I will
modify the commit log.
What do you think?

>
> >
> > > - Charlie
> > >
> > > > this patch.
> > > >
> > > > Excerpt from Documentation/admin-guide/kernel-parameters.txt:
> > > >
> > > > memmap=nn[KMG]@ss[KMG]
> > > > [KNL] Force usage of a specific region of memory.
> > > > Region of memory to be used is from ss to ss+nn.
> > > >
> > > > memmap=nn[KMG]$ss[KMG]
> > > > Mark specific memory as reserved.
> > > > Region of memory to be reserved is from ss to ss+nn.
> > > > Example: Exclude memory from 0x18690000-0x1869ffff
> > > > memmap=64K$0x18690000
> > > > or
> > > > memmap=0x10000$0x18690000
> > > >
> > > > There is no need to update this documentation file with respect to
> > > > this patch.
> > > >
> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > ---
> > > >  arch/riscv/mm/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 50 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > index e3405e4b99af..7be7ec3092ad 100644
> > > > --- a/arch/riscv/mm/init.c
> > > > +++ b/arch/riscv/mm/init.c
> > > > @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
> > > >  }
> > > >  early_param("mem", early_mem);
> > > >
> > > > +static void __init parse_memmap_one(char *p)
> > > > +{
> > > > +     char *oldp;
> > > > +     unsigned long start_at, mem_size;
> > > > +
> > > > +     if (!p)
> > > > +             return;
> > > > +
> > > > +     oldp = p;
> > > > +     mem_size = memparse(p, &p);
> > > > +     if (p == oldp)
> > > > +             return;
> > > > +
> > > > +     switch (*p) {
> > > > +     case '@':
> > > > +             start_at = memparse(p + 1, &p);
> > > > +             memblock_add(start_at, mem_size);
> > > > +             break;
> > > > +
> > > > +     case '$':
> > > > +             start_at = memparse(p + 1, &p);
> > > > +             memblock_reserve(start_at, mem_size);
> > > > +             break;
> > > > +
> > > > +     case 0:
> > > > +             memblock_reserve(mem_size, -mem_size);
> > > > +             break;
> > > > +
> > > > +     default:
> > > > +             pr_warn("Unrecognized memmap syntax: %s\n", p);
> > > > +             break;
> > > > +     }
> > > > +}
> > > > +
> > > > +static int __init parse_memmap_opt(char *str)
> > > > +{
> > > > +     while (str) {
> > > > +             char *k = strchr(str, ',');
> > > > +
> > > > +             if (k)
> > > > +                     *k++ = 0;
> > > > +
> > > > +             parse_memmap_one(str);
> > > > +             str = k;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +early_param("memmap", parse_memmap_opt);
> > > > +
> > > >  static void __init setup_bootmem(void)
> > > >  {
> > > >       phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > > > --
> > > > 2.20.1
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> > Thanks,
> > Yunhui

Thanks,
Yunhui

Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[Charlie Jenkins]

On Fri, Jun 21, 2024 at 02:02:18PM +0800, yunhui cui wrote:
> Hi Charlie，
> 
> On Fri, Jun 21, 2024 at 11:10 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 10:08:39AM +0800, yunhui cui wrote:
> > > Hi Charlie,
> > >
> > > On Fri, Jun 21, 2024 at 9:03 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> > > > > Implement support for parsing 'memmap' kernel command line parameter.
> > > > >
> > > > > This patch covers parsing of the following two formats for 'memmap'
> > > > > parameter values:
> > > > >
> > > > > - nn[KMG]@ss[KMG]
> > > > > - nn[KMG]$ss[KMG]
> > > > >
> > > > > ([KMG] = K M or G (kilo, mega, giga))
> > > > >
> > > > > These two allowed formats for parameter value are already documented
> > > > > in file kernel-parameters.txt in Documentation/admin-guide folder.
> > > > > Some architectures already support them, but Mips did not prior to
> > > >
> > > > Copy-paste from a Mips patch? Should say riscv :)
> > > >
> > > > It looks like this code is duplicated from xtensa and is effectively the
> > > > same as mips. Can this code be placed in a generic file so that the code
> > > > can be shared between mips, riscv, and xtensa -- maybe a new config that
> > > > gets selected by mips/riscv/xtensa?
> > >
> > > Yeah, that's actually what I was thinking. Which general file do you
> > > think would be more suitable to put it in?
> >
> > I am not sure the best place to put it. What do you think about
> > mm/memblock.c next to the "memblock" early param?
> 
> Is it inappropriate to put it in memblock? The implementation of mips
> is different from that of xtensa, and early_mem is also distributed in
> various archs, so we still put memmap in riscv/, and then I will
> modify the commit log.
> What do you think?

The mips implementation is very close to being the same, but I am not
sure if the differences would prevent standardization. xtensa and now
riscv would have identical implementations though so a generic memmap
implementation could be only applied to those two archs.

The "mem" early param is also scattered across archs as you point out,
but that looks more fragmented in how the different architectures have
implemented it.

I will copy Mike Rapoport to see if he has any comments since he is the
maintainer of memblock and memory management initialization.

> 
> >
> > >
> > > > - Charlie
> > > >
> > > > > this patch.
> > > > >
> > > > > Excerpt from Documentation/admin-guide/kernel-parameters.txt:
> > > > >
> > > > > memmap=nn[KMG]@ss[KMG]
> > > > > [KNL] Force usage of a specific region of memory.
> > > > > Region of memory to be used is from ss to ss+nn.
> > > > >
> > > > > memmap=nn[KMG]$ss[KMG]
> > > > > Mark specific memory as reserved.
> > > > > Region of memory to be reserved is from ss to ss+nn.
> > > > > Example: Exclude memory from 0x18690000-0x1869ffff
> > > > > memmap=64K$0x18690000
> > > > > or
> > > > > memmap=0x10000$0x18690000
> > > > >
> > > > > There is no need to update this documentation file with respect to
> > > > > this patch.
> > > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > ---
> > > > >  arch/riscv/mm/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 50 insertions(+)
> > > > >
> > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > index e3405e4b99af..7be7ec3092ad 100644
> > > > > --- a/arch/riscv/mm/init.c
> > > > > +++ b/arch/riscv/mm/init.c
> > > > > @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
> > > > >  }
> > > > >  early_param("mem", early_mem);
> > > > >
> > > > > +static void __init parse_memmap_one(char *p)
> > > > > +{
> > > > > +     char *oldp;
> > > > > +     unsigned long start_at, mem_size;
> > > > > +
> > > > > +     if (!p)
> > > > > +             return;
> > > > > +
> > > > > +     oldp = p;
> > > > > +     mem_size = memparse(p, &p);
> > > > > +     if (p == oldp)
> > > > > +             return;
> > > > > +
> > > > > +     switch (*p) {
> > > > > +     case '@':
> > > > > +             start_at = memparse(p + 1, &p);
> > > > > +             memblock_add(start_at, mem_size);
> > > > > +             break;
> > > > > +
> > > > > +     case '$':
> > > > > +             start_at = memparse(p + 1, &p);
> > > > > +             memblock_reserve(start_at, mem_size);
> > > > > +             break;
> > > > > +
> > > > > +     case 0:
> > > > > +             memblock_reserve(mem_size, -mem_size);
> > > > > +             break;
> > > > > +
> > > > > +     default:
> > > > > +             pr_warn("Unrecognized memmap syntax: %s\n", p);
> > > > > +             break;
> > > > > +     }
> > > > > +}
> > > > > +
> > > > > +static int __init parse_memmap_opt(char *str)
> > > > > +{
> > > > > +     while (str) {
> > > > > +             char *k = strchr(str, ',');
> > > > > +
> > > > > +             if (k)
> > > > > +                     *k++ = 0;
> > > > > +
> > > > > +             parse_memmap_one(str);
> > > > > +             str = k;
> > > > > +     }
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +early_param("memmap", parse_memmap_opt);
> > > > > +
> > > > >  static void __init setup_bootmem(void)
> > > > >  {
> > > > >       phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > linux-riscv mailing list
> > > > > linux-riscv@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> > > Thanks,
> > > Yunhui
> 
> Thanks,
> Yunhui

Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[Mike Rapoport]

On Thu, Jun 20, 2024 at 11:39:41PM -0700, Charlie Jenkins wrote:
> On Fri, Jun 21, 2024 at 02:02:18PM +0800, yunhui cui wrote:
> > Hi Charlie，
> > 
> > On Fri, Jun 21, 2024 at 11:10 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > On Fri, Jun 21, 2024 at 10:08:39AM +0800, yunhui cui wrote:
> > > > Hi Charlie,
> > > >
> > > > On Fri, Jun 21, 2024 at 9:03 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > >
> > > > > On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> > > > > > Implement support for parsing 'memmap' kernel command line parameter.
> > > > > >
> > > > > > This patch covers parsing of the following two formats for 'memmap'
> > > > > > parameter values:
> > > > > >
> > > > > > - nn[KMG]@ss[KMG]
> > > > > > - nn[KMG]$ss[KMG]
> > > > > >
> > > > > > ([KMG] = K M or G (kilo, mega, giga))
> > > > > >
> > > > > > These two allowed formats for parameter value are already documented
> > > > > > in file kernel-parameters.txt in Documentation/admin-guide folder.
> > > > > > Some architectures already support them, but Mips did not prior to

I believe you should add RISCV to the list of supported architectures for
these options.

> > > > > Copy-paste from a Mips patch? Should say riscv :)
> > > > >
> > > > > It looks like this code is duplicated from xtensa and is effectively the
> > > > > same as mips. Can this code be placed in a generic file so that the code
> > > > > can be shared between mips, riscv, and xtensa -- maybe a new config that
> > > > > gets selected by mips/riscv/xtensa?
> > > >
> > > > Yeah, that's actually what I was thinking. Which general file do you
> > > > think would be more suitable to put it in?
> > >
> > > I am not sure the best place to put it. What do you think about
> > > mm/memblock.c next to the "memblock" early param?
> > 
> > Is it inappropriate to put it in memblock? The implementation of mips
> > is different from that of xtensa, and early_mem is also distributed in
> > various archs, so we still put memmap in riscv/, and then I will
> > modify the commit log.
> > What do you think?
> 
> The mips implementation is very close to being the same, but I am not
> sure if the differences would prevent standardization. xtensa and now
> riscv would have identical implementations though so a generic memmap
> implementation could be only applied to those two archs.

The memmap= is generally a hack to workaround issues with how firmware
describes memory to the kernel so in a way it belongs to arch/ code.

mips and xtensa already have different views on how this should be treated,
not mentioning x86 that handles memmap= on e820 level rather than with
memblock.

> > > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > > index e3405e4b99af..7be7ec3092ad 100644
> > > > > > --- a/arch/riscv/mm/init.c
> > > > > > +++ b/arch/riscv/mm/init.c
> > > > > > @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
> > > > > >  }
> > > > > >  early_param("mem", early_mem);
> > > > > >
> > > > > > +static void __init parse_memmap_one(char *p)
> > > > > > +{
> > > > > > +     char *oldp;
> > > > > > +     unsigned long start_at, mem_size;
> > > > > > +
> > > > > > +     if (!p)
> > > > > > +             return;
> > > > > > +
> > > > > > +     oldp = p;
> > > > > > +     mem_size = memparse(p, &p);
> > > > > > +     if (p == oldp)
> > > > > > +             return;
> > > > > > +
> > > > > > +     switch (*p) {
> > > > > > +     case '@':
> > > > > > +             start_at = memparse(p + 1, &p);
> > > > > > +             memblock_add(start_at, mem_size);
> > > > > > +             break;
> > > > > > +
> > > > > > +     case '$':
> > > > > > +             start_at = memparse(p + 1, &p);
> > > > > > +             memblock_reserve(start_at, mem_size);

This will add a region to memblock.reserved, but if there is no memory
there this information won't be available after boot, e.g. there won't be
struct pages with PG_Reserved for this region.
Is this your intention?

> > > > > > +             break;
> > > > > > +
> > > > > > +     case 0:
> > > > > > +             memblock_reserve(mem_size, -mem_size);

This corresponds to the case memmap=nn[KMG] and it is not documented in
kernel-parameters.txt.

Not sure it's needed at all as the same result can be achieved with
memmap=nn[KMG]$0.

> > > > > > +             break;
> > > > > > +
> > > > > > +     default:
> > > > > > +             pr_warn("Unrecognized memmap syntax: %s\n", p);
> > > > > > +             break;
> > > > > > +     }
> > > > > > +}
> > > > > > +
> > > > > > +static int __init parse_memmap_opt(char *str)
> > > > > > +{
> > > > > > +     while (str) {
> > > > > > +             char *k = strchr(str, ',');
> > > > > > +
> > > > > > +             if (k)
> > > > > > +                     *k++ = 0;
> > > > > > +
> > > > > > +             parse_memmap_one(str);
> > > > > > +             str = k;
> > > > > > +     }
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +early_param("memmap", parse_memmap_opt);
> > > > > > +
> > > > > >  static void __init setup_bootmem(void)
> > > > > >  {
> > > > > >       phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > > > > > --
> > > > > > 2.20.1
> > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > linux-riscv mailing list
> > > > > > linux-riscv@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > >
> > > > Thanks,
> > > > Yunhui
> > 
> > Thanks,
> > Yunhui

-- 
Sincerely yours,
Mike.

Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap' parameter

[yunhui cui]

Hi Mike,

On Sun, Jun 23, 2024 at 5:10 PM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Thu, Jun 20, 2024 at 11:39:41PM -0700, Charlie Jenkins wrote:
> > On Fri, Jun 21, 2024 at 02:02:18PM +0800, yunhui cui wrote:
> > > Hi Charlie，
> > >
> > > On Fri, Jun 21, 2024 at 11:10 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > >
> > > > On Fri, Jun 21, 2024 at 10:08:39AM +0800, yunhui cui wrote:
> > > > > Hi Charlie,
> > > > >
> > > > > On Fri, Jun 21, 2024 at 9:03 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 18, 2024 at 08:08:42PM +0800, Yunhui Cui wrote:
> > > > > > > Implement support for parsing 'memmap' kernel command line parameter.
> > > > > > >
> > > > > > > This patch covers parsing of the following two formats for 'memmap'
> > > > > > > parameter values:
> > > > > > >
> > > > > > > - nn[KMG]@ss[KMG]
> > > > > > > - nn[KMG]$ss[KMG]
> > > > > > >
> > > > > > > ([KMG] = K M or G (kilo, mega, giga))
> > > > > > >
> > > > > > > These two allowed formats for parameter value are already documented
> > > > > > > in file kernel-parameters.txt in Documentation/admin-guide folder.
> > > > > > > Some architectures already support them, but Mips did not prior to
>
> I believe you should add RISCV to the list of supported architectures for
> these options.

Okay, I will update it on v2.

>
> > > > > > Copy-paste from a Mips patch? Should say riscv :)
> > > > > >
> > > > > > It looks like this code is duplicated from xtensa and is effectively the
> > > > > > same as mips. Can this code be placed in a generic file so that the code
> > > > > > can be shared between mips, riscv, and xtensa -- maybe a new config that
> > > > > > gets selected by mips/riscv/xtensa?
> > > > >
> > > > > Yeah, that's actually what I was thinking. Which general file do you
> > > > > think would be more suitable to put it in?
> > > >
> > > > I am not sure the best place to put it. What do you think about
> > > > mm/memblock.c next to the "memblock" early param?
> > >
> > > Is it inappropriate to put it in memblock? The implementation of mips
> > > is different from that of xtensa, and early_mem is also distributed in
> > > various archs, so we still put memmap in riscv/, and then I will
> > > modify the commit log.
> > > What do you think?
> >
> > The mips implementation is very close to being the same, but I am not
> > sure if the differences would prevent standardization. xtensa and now
> > riscv would have identical implementations though so a generic memmap
> > implementation could be only applied to those two archs.
>
> The memmap= is generally a hack to workaround issues with how firmware
> describes memory to the kernel so in a way it belongs to arch/ code.
>
> mips and xtensa already have different views on how this should be treated,
> not mentioning x86 that handles memmap= on e820 level rather than with
> memblock.

OK, let's continue to put it in arch/.

>
> > > > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > > > index e3405e4b99af..7be7ec3092ad 100644
> > > > > > > --- a/arch/riscv/mm/init.c
> > > > > > > +++ b/arch/riscv/mm/init.c
> > > > > > > @@ -208,6 +208,56 @@ static int __init early_mem(char *p)
> > > > > > >  }
> > > > > > >  early_param("mem", early_mem);
> > > > > > >
> > > > > > > +static void __init parse_memmap_one(char *p)
> > > > > > > +{
> > > > > > > +     char *oldp;
> > > > > > > +     unsigned long start_at, mem_size;
> > > > > > > +
> > > > > > > +     if (!p)
> > > > > > > +             return;
> > > > > > > +
> > > > > > > +     oldp = p;
> > > > > > > +     mem_size = memparse(p, &p);
> > > > > > > +     if (p == oldp)
> > > > > > > +             return;
> > > > > > > +
> > > > > > > +     switch (*p) {
> > > > > > > +     case '@':
> > > > > > > +             start_at = memparse(p + 1, &p);
> > > > > > > +             memblock_add(start_at, mem_size);
> > > > > > > +             break;
> > > > > > > +
> > > > > > > +     case '$':
> > > > > > > +             start_at = memparse(p + 1, &p);
> > > > > > > +             memblock_reserve(start_at, mem_size);
>
> This will add a region to memblock.reserved, but if there is no memory
> there this information won't be available after boot, e.g. there won't be
> struct pages with PG_Reserved for this region.
> Is this your intention?

Yeah, we should use memmap to reserve in the known memory range.

>
> > > > > > > +             break;
> > > > > > > +
> > > > > > > +     case 0:
> > > > > > > +             memblock_reserve(mem_size, -mem_size);
>
> This corresponds to the case memmap=nn[KMG] and it is not documented in
> kernel-parameters.txt.
>
> Not sure it's needed at all as the same result can be achieved with
> memmap=nn[KMG]$0.

OK, just use case '$', because memblock_reserve() converts size to unsigned.

>
> > > > > > > +             break;
> > > > > > > +
> > > > > > > +     default:
> > > > > > > +             pr_warn("Unrecognized memmap syntax: %s\n", p);
> > > > > > > +             break;
> > > > > > > +     }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int __init parse_memmap_opt(char *str)
> > > > > > > +{
> > > > > > > +     while (str) {
> > > > > > > +             char *k = strchr(str, ',');
> > > > > > > +
> > > > > > > +             if (k)
> > > > > > > +                     *k++ = 0;
> > > > > > > +
> > > > > > > +             parse_memmap_one(str);
> > > > > > > +             str = k;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +early_param("memmap", parse_memmap_opt);
> > > > > > > +
> > > > > > >  static void __init setup_bootmem(void)
> > > > > > >  {
> > > > > > >       phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > > > > > > --
> > > > > > > 2.20.1
> > > > > > >
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > linux-riscv mailing list
> > > > > > > linux-riscv@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > > >
> > > > > Thanks,
> > > > > Yunhui
> > >
> > > Thanks,
> > > Yunhui
>
> --
> Sincerely yours,
> Mike.

Thanks,
Yunhui