Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230625140931.1266216-1-songshuaishuai@tinylab.org>
In-Reply-To: <20230625140931.1266216-1-songshuaishuai@tinylab.org>
From:   Alexandre Ghiti <alexghiti@rivosinc.com>
Date:   Sun, 25 Jun 2023 22:36:11 +0200
Message-ID: <CAHVXubgJcGPq_uC6iJCLTys5J6Ooa1EYcfOhBjEQwYfd+baWxw@mail.gmail.com>
Subject: Re: [PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups
To:     Song Shuai <songshuaishuai@tinylab.org>
Cc:     paul.walmsley@sifive.com, palmer@dabbelt.com,
        aou@eecs.berkeley.edu, robh+dt@kernel.org, frowand.list@gmail.com,
        ajones@ventanamicro.com, mpe@ellerman.id.au, arnd@arndb.de,
        rppt@kernel.org, samuel@sholland.org, panqinglin2020@iscas.ac.cn,
        conor.dooley@microchip.com, anup@brainfault.org,
        xianting.tian@linux.alibaba.com, anshuman.khandual@arm.com,
        heiko@sntech.de, linux-riscv@lists.infradead.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Song,

On Sun, Jun 25, 2023 at 4:10=E2=80=AFPM Song Shuai <songshuaishuai@tinylab.=
org> wrote:
>
> We have encountered these two issues about huge-paged linear mapping sinc=
e v6.4-rc1:
>
> 1. Bug report: kernel paniced when system hibernates[1]
>
> OpenSbi [v0.8,v1.3) set the PMP regions as !no-map, and the commit 333506=
8f8721
> ("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in li=
near mapping.
> The hibernation process attempted to save/restore these mapped regions re=
sulting in access fault.
>
> This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hi=
bernation as nonportable").
> But as Alex's RFC and Rob's response stats in another thread [2] ,
> "Hibernation is only one case. Speculative accesses could also occur."
> So this fixing commit seems not the perfect answer to this issue.
>
>
> 2. Bug report: kernel paniced while booting (with UEFI )[3]
>
> During the booting with UEFI, UEFI Memory Mapping overwrote the memblock.
> The phys_ram_base was set as the end address of mmoderes0 (like 0x8004000=
0 for 256 KiB mmoderes0@80000000),
> which resulted the VA based on 2M-aligned PA was not 2M-aligned using va_=
pa_offset
> (PAGE_OFFSET - phys_ram_base) to translate.
>
> The best_map_size() from commit 3335068f8721 didn't check the virtual ali=
gnment
> before choosing a map size. and then a "VA hole" was created where page f=
aults always occurred.
>
> This issue was fixed by commit 49a0a3731596 ("riscv: Check the virtual al=
ignment before choosing a map size"),
> But this fixing commit has a side-effect ("the possible third one" as Ale=
x said in this thread).
> There are numerous PTE allocations slowing down the boot time and consumi=
ng some system memory when UEFI booting
> (Note that it's not involved when booting directly with OpenSbi, where ph=
ys_ram_base is the 2M-aligned base of DRAM).
>
> In my test, compared with/out reverting both commit 49a0a3731596 and comm=
it 3335068f8721,
> I must wait ~20s for the linear mapping creation and mem_init_print_info(=
) reported ~8M extra reserved memory.

Indeed, phys_ram_base is not aligned on a 2MB boundary when booting
with EDK2, IIRC that's because the first chunk of memory at
0x8000_0000 is marked as UC and is then completely evicted.

>
> To eliminate this side-effect, We should find a way to align VA and PA on=
 a 2MB boundary.
> The simplest way is reverting the commit 3335068f8721 ("riscv: Use PUD/P4=
D/PGD pages for the linear mapping").
>

I disagree, the simplest way is to align phys_ram_base on a 2MB
boundary, either by aligning to the upper boundary (and then wastes
memory, like we used to) or by aligning to the lower boundary (but I
want to make sure it works).

I'll come up with something tomorrow.

Thanks,

Alex

>
>
> Using PUD/P4D/PGD pages for the linear mapping to improve the performance=
 is marginal from a recent talk [4]
> from Mike Rapoport. OpenSbi had marked all the PMP-protected regions as "=
no-map" [5] to practice this talk.
>
> For all those reasons, let's revert these related commits:
>
> - commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the linear mappi=
ng")
> - commit 49a0a3731596 ("riscv: Check the virtual alignment before choosin=
g a map size")
> - commit ed309ce52218 ("RISC-V: mark hibernation as nonportable")
>
> [1]: https://lore.kernel.org/linux-riscv/CAAYs2=3DgQvkhTeioMmqRDVGjdtNF_v=
hB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
> [2]: https://lore.kernel.org/linux-kernel/20230530080425.18612-1-alexghit=
i@rivosinc.com/
> [3]: https://lore.kernel.org/linux-riscv/tencent_7C3B580B47C1B17C16488EC1=
@qq.com/
> [4]: https://lwn.net/Articles/931406/
> [5]: https://github.com/riscv-software-src/opensbi/commit/8153b2622b08802=
cc542f30a1fcba407a5667ab9
>
> Song Shuai (3):
>   Revert "RISC-V: mark hibernation as nonportable"
>   Revert "riscv: Check the virtual alignment before choosing a map size"
>   Revert "riscv: Use PUD/P4D/PGD pages for the linear mapping"
>
>  arch/riscv/Kconfig            |  5 +---
>  arch/riscv/include/asm/page.h | 16 -------------
>  arch/riscv/mm/init.c          | 43 +++++++----------------------------
>  arch/riscv/mm/physaddr.c      | 16 -------------
>  drivers/of/fdt.c              | 11 ++++-----
>  5 files changed, 14 insertions(+), 77 deletions(-)
>
> --
> 2.20.1
>