Message-ID: <Znfl8NDWmL1NFANn@kernel.org>
Date: Sun, 23 Jun 2024 12:08:00 +0300
From: Mike Rapoport <rppt@...nel.org>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: yunhui cui <cuiyunhui@...edance.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

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.

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

-- 
Sincerely yours,
Mike.

Powered by blists - more mailing lists