Date: Mon, 24 Jun 2024 15:03:02 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: Mike Rapoport <rppt@...nel.org>
Cc: Charlie Jenkins <charlie@...osinc.com>, 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

Hi Mike,

On Sun, Jun 23, 2024 at 5:10 PM Mike Rapoport <rppt@...nel.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@...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
>
> 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@...ts.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > > >
> > > > > Thanks,
> > > > > Yunhui
> > >
> > > Thanks,
> > > Yunhui
>
> --
> Sincerely yours,
> Mike.

Thanks,
Yunhui

Powered by blists - more mailing lists