Date: Thu, 20 Jun 2024 23:39:41 -0700
From: Charlie Jenkins <charlie@...osinc.com>
To: yunhui cui <cuiyunhui@...edance.com>
Cc: rppt@...nel.org, paul.walmsley@...ive.com, palmer@...belt.com,
	aou@...s.berkeley.edu, alexghiti@...osinc.com,
	akpm@...ux-foundation.org, bhe@...hat.com, dawei.li@...ngroup.cn,
	jszhang@...nel.org, namcao@...utronix.de, bjorn@...osinc.com,
	vishal.moola@...il.com, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] RISC-V: cmdline: Add support for 'memmap'
 parameter

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@...osinc.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@...osinc.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@...edance.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@...ts.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> > > Thanks,
> > > Yunhui
> 
> Thanks,
> Yunhui

Powered by blists - more mailing lists