Message-ID: <CAHh=Yk-WCMbNWg7UCbXtZZzPmOHnh1-pv4fqykwnpJGtwTcc=A@mail.gmail.com>
Date: Fri, 19 Jan 2024 17:26:59 +0800
From: Nylon Chen <nylon.chen@...ive.com>
To: Alexandre Ghiti <alexghiti@...osinc.com>
Cc: alex@...ti.fr, apatel@...tanamicro.com, catalin.marinas@....com, 
	will@...nel.org, paul.walmsley@...ive.com, palmer@...belt.com, 
	aou@...s.berkeley.edu, robh+dt@...nel.org, frowand.list@...il.com, 
	rppt@...nel.org, akpm@...ux-foundation.org, anup@...infault.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-mm@...ck.org, zong.li@...ive.com, nylon7717@...il.com
Subject: Re: Fwd: [PATCH v8 0/4] riscv: Use PUD/P4D/PGD pages for the linear mapping

Alexandre Ghiti <alexghiti@...osinc.com> 於 2024年1月18日 週四 下午9:01寫道：
>
> Hi Nylon,
Hi Alexandre, thanks for your feedback,
>
> On Thu, Jan 18, 2024 at 9:23 AM Nylon Chen <nylon.chen@...ive.com> wrote:
> >
> > > On 3/23/23 15:55, Anup Patel wrote:
> > > > On Thu, Mar 23, 2023 at 6:24 PM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
> > > >> Hi Anup,
> > > >>
> > > >> On Thu, Mar 23, 2023 at 1:18 PM Anup Patel <apatel@...tanamicro.com> wrote:
> > > >>> Hi Alex,
> > > >>>
> > > >>> On Thu, Mar 16, 2023 at 6:48 PM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
> > > >>>> This patchset intends to improve tlb utilization by using hugepages for
> > > >>>> the linear mapping.
> > > >>>>
> > > >>>> As reported by Anup in v6, when STRICT_KERNEL_RWX is enabled, we must
> > > >>>> take care of isolating the kernel text and rodata so that they are not
> > > >>>> mapped with a PUD mapping which would then assign wrong permissions to
> > > >>>> the whole region: it is achieved by introducing a new memblock API.
> > > >>>>
> > > >>>> Another patch makes use of this new API in arm64 which used some sort of
> > > >>>> hack to solve this issue: it was built/boot tested successfully.
> > > >>>>
> > > >>>> base-commit-tag: v6.3-rc1
> > > >>>>
> > > >>>> v8:
> > > >>>> - Fix rv32, as reported by Anup
> > > >>>> - Do not modify memblock_isolate_range and fixes comment, as suggested by Mike
> > > >>>> - Use the new memblock API for crash kernel too in arm64, as suggested by Andrew
> > > >>>> - Fix arm64 double mapping (which to me did not work in v7), but ends up not
> > > >>>>    being pretty at all, will wait for comments from arm64 reviewers, but
> > > >>>>    this patch can easily be dropped if they do not want it.
> > > >>>>
> > > >>>> v7:
> > > >>>> - Fix Anup bug report by introducing memblock_isolate_memory which
> > > >>>>    allows us to split the memblock mappings and then avoid to map the
> > > >>>>    the PUD which contains the kernel as read only
> > > >>>> - Add a patch to arm64 to use this newly introduced API
> > > >>>>
> > > >>>> v6:
> > > >>>> - quiet LLVM warning by casting phys_ram_base into an unsigned long
> > > >>>>
> > > >>>> v5:
> > > >>>> - Fix nommu builds by getting rid of riscv_pfn_base in patch 1, thanks
> > > >>>>    Conor
> > > >>>> - Add RB from Andrew
> > > >>>>
> > > >>>> v4:
> > > >>>> - Rebase on top of v6.2-rc3, as noted by Conor
> > > >>>> - Add Acked-by Rob
> > > >>>>
> > > >>>> v3:
> > > >>>> - Change the comment about initrd_start VA conversion so that it fits
> > > >>>>    ARM64 and RISCV64 (and others in the future if needed), as suggested
> > > >>>>    by Rob
> > > >>>>
> > > >>>> v2:
> > > >>>> - Add a comment on why RISCV64 does not need to set initrd_start/end that
> > > >>>>    early in the boot process, as asked by Rob
> > > >>>>
> > > >>>> Alexandre Ghiti (4):
> > > >>>>    riscv: Get rid of riscv_pfn_base variable
> > > >>>>    mm: Introduce memblock_isolate_memory
> > > >>>>    arm64: Make use of memblock_isolate_memory for the linear mapping
> > > >>>>    riscv: Use PUD/P4D/PGD pages for the linear mapping
> > > >>> Kernel boot fine on RV64 but there is a failure which is still not
> > > >>> addressed. You can see this failure as following message in
> > > >>> kernel boot log:
> > > >>>      0.000000] Failed to add a System RAM resource at 80200000
> > > >> Hmmm I don't get that in any of my test configs, would you mind
> > > >> sharing yours and your qemu command line?
> > > > Try alexghiti_test branch at
> > > > https://github.com/avpatel/linux.git
> > > >
> > > > I am building the kernel using defconfig and my rootfs is
> > > > based on busybox.
> > > >
> > > > My QEMU command is:
> > > > qemu-system-riscv64 -M virt -m 512M -nographic -bios
> > > > opensbi/build/platform/generic/firmware/fw_dynamic.bin -kernel
> > > > ./build-riscv64/arch/riscv/boot/Image -append "root=/dev/ram rw
> > > > console=ttyS0 earlycon" -initrd ./rootfs_riscv64.img -smp 4
> > >
> > >
> > > So splitting memblock.memory is the culprit, it "confuses" the resources
> > > addition and I can only find hacky ways to fix that...
> > Hi Alexandre,
> >
> > We encountered the same error as Anup. After adding your patch
> > (3335068f87217ea59d08f462187dc856652eea15), we will not encounter the
> > error again.
> >
> > What I have observed so far is
> >
> > - before your patch
> > When merging consecutive memblocks, if the memblock types are different,
> > they will be merged into reserved
> > - after your patch
> > When consecutive memblocks are merged, if the memblock types are
> > different, they will be merged into memory.
> >
> > Such a result will cause the memory location of OpenSBI to be changed
> > from reserved to memory. Will this have any side effects?
>
> I guess it will end up in the memory pool and pages from openSBI
> region will be allocated, so we should see very quickly bad stuff
> happening (either PMP violation or M-mode ecall never
> returning/trapping/etc).
>
> But I don't observe the same thing, I always see the openSBI region
> being reserved:
>
> reserved[0x0] [0x0000000080000000-0x000000008007ffff],
> 0x0000000000080000 bytes flags: 0x0
>
> Can you elaborate a bit more about "When consecutive memblocks are
> merged, if the memblock types are different, they will be merged into
> memory"? Where/when does this merge happen? Can you give me a config
> file and a kernel revision so that I can take a look?
Ok, If you want to reproduce the same results you just need to modify OpenSBI

[ lib/sbi/sbi_domain.c ]
+#define TEST_SIZE 0x200000

-                                 (scratch->fw_size - scratch->fw_rw_offset),
+                                 (TEST_SIZE - scratch->fw_rw_offset),

In addition, you can insert checks in the kernel merged function
[ mm/memblock.c ]
static void __init_memblock memblock_merge_regions(struct memblock_type *type)
        while (i < type->cnt - 1) {
         ...
                /* move forward from next + 1, index of which is i + 2 */
                memmove(next, next + 1, (type->cnt - (i + 2)) * sizeof(*next));
                type->cnt--;
        }
+       pr_info("Merged memblock_type: cnt = %lu, max = %lu,
total_size = 0x%llx\n",type->cnt, type->max, type->total_size);
+       for (i = 0; i < type->cnt; i++) {
+               const char *region_type =
memblock_is_memory(type->regions[i].base) ? "memory" : "reserve";
+               pr_info("Region %d: base = 0x%llx, size = 0x%llx, type
= %s\n", i, type->regions[i].base, type->regions[i].size,
region_type);
+       }
 }
This is kernel boot log
- before your patch
..
[    0.000000] OF: fdt: Reserving memory: base = 0x80000000, size = 0x200000
[    0.000000] Merged memblock_type: cnt = 4, max = 128, total_size = 0x1628501
[    0.000000] Region 0: base = 0x80000000, size = 0x1600000, type = reserve
..

- after your patch
..
[    0.000000] OF: fdt: Reserving memory: base = 0x80000000, size = 0x200000
[    0.000000] Merged memblock_type: cnt = 4, max = 128, total_size = 0x180c42e
[    0.000000] Region 0: base = 0x80000000, size = 0x1800000, type = memory
..
[    0.000000] Failed to add a system RAM resource at 80200000
..
>
> Thanks,
>
> Alex
>
> > >
> > > So given that the arm64 patch with the new API is not pretty and that
> > > the simplest solution is to re-merge the memblock regions afterwards
> > > (which is done by memblock_clear_nomap), I'll drop the new API and the
> > > arm64 patch to use the nomap API like arm64: I'll take advantage of that
> > > to clean setup_vm_final which I have wanted to do for a long time.
> > >
> > > @Mike Thanks for you reviews!
> > >
> > > @Anup Thanks for all your bug reports on this patchset, I have to
> > > improve my test flow (it is in the work :)).
> > >
> > >
> > > > Regards,
> > > > Anup
> > > >
> > > >> Thanks
> > > >>
> > > >>> Regards,
> > > >>> Anup
> > > >>>
> > > >>>>   arch/arm64/mm/mmu.c           | 25 +++++++++++------
> > > >>>>   arch/riscv/include/asm/page.h | 19 +++++++++++--
> > > >>>>   arch/riscv/mm/init.c          | 53 ++++++++++++++++++++++++++++-------
> > > >>>>   arch/riscv/mm/physaddr.c      | 16 +++++++++++
> > > >>>>   drivers/of/fdt.c              | 11 ++++----
> > > >>>>   include/linux/memblock.h      |  1 +
> > > >>>>   mm/memblock.c                 | 20 +++++++++++++
> > > >>>>   7 files changed, 119 insertions(+), 26 deletions(-)
> > > >>>>
> > > >>>> --
> > > >>>> 2.37.2
> > > >>>>
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@...ts.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@...ts.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists