Received-SPF: pass (google.com: domain of linux-kernel+bounces-52302-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20230316131711.1284451-1-alexghiti@rivosinc.com>
 <CAK9=C2XJtSG2d_nsyDv7kU1v7Jj0chdevqrMc0MpJswukcEABA@mail.gmail.com>
 <CAHVXubhhxpzHDM-n91V_rceY5t_VqLvrwZj3RP_tNL2=F9mqjQ@mail.gmail.com>
 <CAK9=C2WVOpSqtt8r1U4hnzSZ=cc1PocpukgQjNyahP2XuPhozw@mail.gmail.com>
 <d0087922-4721-ccf1-80bf-9f74099d0948@ghiti.fr> <CAPqJEFr6MgUyARfbWAo7EeQKLVd4xRJz_LOYN68UC-kPD1Hr5A@mail.gmail.com>
 <20240118082346.GB31078@hsinchu15> <CAHVXubiQ5N+ngdy=Fk3j-hS_KkOEg272b++-hB4-oGeSSZKtNQ@mail.gmail.com>
 <CAHh=Yk-WCMbNWg7UCbXtZZzPmOHnh1-pv4fqykwnpJGtwTcc=A@mail.gmail.com>
In-Reply-To: <CAHh=Yk-WCMbNWg7UCbXtZZzPmOHnh1-pv4fqykwnpJGtwTcc=A@mail.gmail.com>
From: Alexandre Ghiti <alexghiti@rivosinc.com>
Date: Mon, 5 Feb 2024 10:32:28 +0100
Message-ID: <CAHVXubhE=RmFQ5gGo7J+H-mOEZKZQbCCH6F43UYMOy=Kgw+nug@mail.gmail.com>
Subject: Re: Fwd: [PATCH v8 0/4] riscv: Use PUD/P4D/PGD pages for the linear mapping
To: Nylon Chen <nylon.chen@sifive.com>
Cc: alex@ghiti.fr, apatel@ventanamicro.com, catalin.marinas@arm.com, 
	will@kernel.org, paul.walmsley@sifive.com, palmer@dabbelt.com, 
	aou@eecs.berkeley.edu, robh+dt@kernel.org, frowand.list@gmail.com, 
	rppt@kernel.org, akpm@linux-foundation.org, anup@brainfault.org, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	linux-riscv@lists.infradead.org, devicetree@vger.kernel.org, 
	linux-mm@kvack.org, zong.li@sifive.com, nylon7717@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Nylon,

On Fri, Jan 19, 2024 at 10:27=E2=80=AFAM Nylon Chen <nylon.chen@sifive.com>=
 wrote:
>
> Alexandre Ghiti <alexghiti@rivosinc.com> =E6=96=BC 2024=E5=B9=B41=E6=9C=
=8818=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=889:01=E5=AF=AB=E9=81=93=
=EF=BC=9A
> >
> > Hi Nylon,
> Hi Alexandre, thanks for your feedback,
> >
> > On Thu, Jan 18, 2024 at 9:23=E2=80=AFAM Nylon Chen <nylon.chen@sifive.c=
om> wrote:
> > >
> > > > On 3/23/23 15:55, Anup Patel wrote:
> > > > > On Thu, Mar 23, 2023 at 6:24=E2=80=AFPM Alexandre Ghiti <alexghit=
i@rivosinc.com> wrote:
> > > > >> Hi Anup,
> > > > >>
> > > > >> On Thu, Mar 23, 2023 at 1:18=E2=80=AFPM Anup Patel <apatel@venta=
namicro.com> wrote:
> > > > >>> Hi Alex,
> > > > >>>
> > > > >>> On Thu, Mar 16, 2023 at 6:48=E2=80=AFPM Alexandre Ghiti <alexgh=
iti@rivosinc.com> wrote:
> > > > >>>> This patchset intends to improve tlb utilization by using huge=
pages 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 permis=
sions 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 so=
me sort of
> > > > >>>> hack to solve this issue: it was built/boot tested successfull=
y.
> > > > >>>>
> > > > >>>> base-commit-tag: v6.3-rc1
> > > > >>>>
> > > > >>>> v8:
> > > > >>>> - Fix rv32, as reported by Anup
> > > > >>>> - Do not modify memblock_isolate_range and fixes comment, as s=
uggested by Mike
> > > > >>>> - Use the new memblock API for crash kernel too in arm64, as s=
uggested by Andrew
> > > > >>>> - Fix arm64 double mapping (which to me did not work in v7), b=
ut ends up not
> > > > >>>>    being pretty at all, will wait for comments from arm64 revi=
ewers, but
> > > > >>>>    this patch can easily be dropped if they do not want it.
> > > > >>>>
> > > > >>>> v7:
> > > > >>>> - Fix Anup bug report by introducing memblock_isolate_memory w=
hich
> > > > >>>>    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_sta=
rt/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 m=
apping
> > > > >>>>    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=3D/dev/ram rw
> > > > > console=3DttyS0 earlycon" -initrd ./rootfs_riscv64.img -smp 4
> > > >
> > > >
> > > > So splitting memblock.memory is the culprit, it "confuses" the reso=
urces
> > > > 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 differe=
nt,
> > > 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 Ope=
nSBI
>
> [ lib/sbi/sbi_domain.c ]
> +#define TEST_SIZE 0x200000
>
> -                                 (scratch->fw_size - scratch->fw_rw_offs=
et),
> +                                 (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(*n=
ext));
>                 type->cnt--;
>         }
> +       pr_info("Merged memblock_type: cnt =3D %lu, max =3D %lu,
> total_size =3D 0x%llx\n",type->cnt, type->max, type->total_size);
> +       for (i =3D 0; i < type->cnt; i++) {
> +               const char *region_type =3D
> memblock_is_memory(type->regions[i].base) ? "memory" : "reserve";
> +               pr_info("Region %d: base =3D 0x%llx, size =3D 0x%llx, typ=
e
> =3D %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 =3D 0x80000000, size =3D 0=
x200000
> [    0.000000] Merged memblock_type: cnt =3D 4, max =3D 128, total_size =
=3D 0x1628501
> [    0.000000] Region 0: base =3D 0x80000000, size =3D 0x1600000, type =
=3D reserve
> ...
>
> - after your patch
> ...
> [    0.000000] OF: fdt: Reserving memory: base =3D 0x80000000, size =3D 0=
x200000
> [    0.000000] Merged memblock_type: cnt =3D 4, max =3D 128, total_size =
=3D 0x180c42e
> [    0.000000] Region 0: base =3D 0x80000000, size =3D 0x1800000, type =
=3D memory

So the openSBI region is marked as memory, and not reserved because
this region is now described as nomap, and memblock_mark_nomap() does
not move this region into the reserved memblock list, but keep it in
the memory list with the nomap flag
(https://elixir.bootlin.com/linux/latest/source/drivers/of/fdt.c#L479).
But as stated in the description of memblock_mark_nomap()
(https://elixir.bootlin.com/linux/latest/source/mm/memblock.c#L969),
the pages associated with the region will be marked as PageReserved
and the region will not be covered in the linear mapping.

So to me, this is normal and we are safe. Let me know if I made a mistake.

And sorry for the long delay, that slipped my mind!

Thanks,

Alex

> ...
> [    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 th=
at
> > > > the simplest solution is to re-merge the memblock regions afterward=
s
> > > > (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@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv