[PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[Song Shuai]

We have encountered these two issues about huge-paged linear mapping since 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 3335068f8721 
("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in linear mapping.
The hibernation process attempted to save/restore these mapped regions resulting in access fault.

This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hibernation 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 0x80040000 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 alignment
before choosing a map size. and then a "VA hole" was created where page faults always occurred.

This issue was fixed by commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size"),
But this fixing commit has a side-effect ("the possible third one" as Alex said in this thread). 
There are numerous PTE allocations slowing down the boot time and consuming some system memory when UEFI booting
(Note that it's not involved when booting directly with OpenSbi, where phys_ram_base is the 2M-aligned base of DRAM).

In my test, compared with/out reverting both commit 49a0a3731596 and commit 3335068f8721,
I must wait ~20s for the linear mapping creation and mem_init_print_info() reported ~8M extra reserved memory.

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/P4D/PGD pages for the linear mapping").



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 mapping")
- commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size")
- commit ed309ce52218 ("RISC-V: mark hibernation as nonportable")
 
[1]: https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
[2]: https://lore.kernel.org/linux-kernel/20230530080425.18612-1-alexghiti@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/8153b2622b08802cc542f30a1fcba407a5667ab9

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Song Shuai]

This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.

With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
load address which was placed at a PMD boundary. And firmware always
correctly mark resident memory, or memory protected with PMP as
per the devicetree specification and/or the UEFI specification.

So those regions will not be mapped in the linear mapping and they
can be safely saved/restored by hibernation.

Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
---
 arch/riscv/Kconfig | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 5966ad97c30c..17b5fc7f54d4 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -800,11 +800,8 @@ menu "Power management options"
 
 source "kernel/power/Kconfig"
 
-# Hibernation is only possible on systems where the SBI implementation has
-# marked its reserved memory as not accessible from, or does not run
-# from the same memory as, Linux
 config ARCH_HIBERNATION_POSSIBLE
-	def_bool NONPORTABLE
+	def_bool y
 
 config ARCH_HIBERNATION_HEADER
 	def_bool HIBERNATION
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V1 2/3] Revert "riscv: Check the virtual alignment before choosing a map size"

[Song Shuai]

This reverts commit 49a0a3731596fc004db6eec3fc674d92a09ef383.

With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
linear mapping") reverted, best_map_size() only uses PMD_SIZE or PAGE_SIZE
for linear mapping and the phys_ram_base that va_pa_offset is based on
points the kernel load address which is 2M-aligned for rv64.

So no need to check the virtual alignment before choosing a map size.

Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
---
 arch/riscv/mm/init.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 4fa420faa780..38c4b4d6b64f 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -660,19 +660,18 @@ void __init create_pgd_mapping(pgd_t *pgdp,
 	create_pgd_next_mapping(nextp, va, pa, sz, prot);
 }
 
-static uintptr_t __init best_map_size(phys_addr_t pa, uintptr_t va,
-				      phys_addr_t size)
+static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
 {
-	if (!(pa & (PGDIR_SIZE - 1)) && !(va & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
+	if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
 		return PGDIR_SIZE;
 
-	if (!(pa & (P4D_SIZE - 1)) && !(va & (P4D_SIZE - 1)) && size >= P4D_SIZE)
+	if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
 		return P4D_SIZE;
 
-	if (!(pa & (PUD_SIZE - 1)) && !(va & (PUD_SIZE - 1)) && size >= PUD_SIZE)
+	if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
 		return PUD_SIZE;
 
-	if (!(pa & (PMD_SIZE - 1)) && !(va & (PMD_SIZE - 1)) && size >= PMD_SIZE)
+	if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
 		return PMD_SIZE;
 
 	return PAGE_SIZE;
@@ -1178,7 +1177,7 @@ static void __init create_linear_mapping_range(phys_addr_t start,
 	for (pa = start; pa < end; pa += map_size) {
 		va = (uintptr_t)__va(pa);
 		map_size = fixed_map_size ? fixed_map_size :
-					    best_map_size(pa, va, end - pa);
+					    best_map_size(pa, end - pa);
 
 		create_pgd_mapping(swapper_pg_dir, va, pa, map_size,
 				   pgprot_from_va(va));
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1013 bytes --]

On Sun, Jun 25, 2023 at 10:09:28PM +0800, Song Shuai wrote:
> We have encountered these two issues about huge-paged linear mapping since 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 3335068f8721 
> ("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in linear mapping.
> The hibernation process attempted to save/restore these mapped regions resulting in access fault.
> 
> This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hibernation 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.

This is a misunderstanding, I was not attempting to fix the issue, but
rather buy time to fix the problem properly, without regressing support
for hibernation when we do.

Cheers,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1490 bytes --]

On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
> This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
> 
> With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
> linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
> load address which was placed at a PMD boundary.

> And firmware always
> correctly mark resident memory, or memory protected with PMP as
> per the devicetree specification and/or the UEFI specification.

But this is not true? The versions of OpenSBI that you mention in your
cover letter do not do this.
Please explain.

> So those regions will not be mapped in the linear mapping and they
> can be safely saved/restored by hibernation.
> 
> Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
> ---
>  arch/riscv/Kconfig | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5966ad97c30c..17b5fc7f54d4 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -800,11 +800,8 @@ menu "Power management options"
>  
>  source "kernel/power/Kconfig"
>  
> -# Hibernation is only possible on systems where the SBI implementation has
> -# marked its reserved memory as not accessible from, or does not run
> -# from the same memory as, Linux
>  config ARCH_HIBERNATION_POSSIBLE
> -	def_bool NONPORTABLE
> +	def_bool y
>  
>  config ARCH_HIBERNATION_HEADER
>  	def_bool HIBERNATION
> -- 
> 2.20.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Song Shuai]

Sorry for the delayed reply,
My tinylab email went something wrong, I'll use gmail in this thread.

在 2023/6/25 22:18, Conor Dooley 写道:
> On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
>> This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
>>
>> With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
>> linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
>> load address which was placed at a PMD boundary.
> 
>> And firmware always
>> correctly mark resident memory, or memory protected with PMP as
>> per the devicetree specification and/or the UEFI specification.
> 
> But this is not true? The versions of OpenSBI that you mention in your
> cover letter do not do this.
> Please explain.
> 

At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey 
the DT/UEFI spec. This statement is excerpted from "Reserved memory for 
resident firmware" part from the upcoming riscv/boot.rst. It isn't 
accurate for now. How about deleting this one?

Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can 
avoid the mapping of firmware memory, I will make it clear in the next 
version.

>> So those regions will not be mapped in the linear mapping and they
>> can be safely saved/restored by hibernation.
>>
>> Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
>> ---
>>   arch/riscv/Kconfig | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 5966ad97c30c..17b5fc7f54d4 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -800,11 +800,8 @@ menu "Power management options"
>>   
>>   source "kernel/power/Kconfig"
>>   
>> -# Hibernation is only possible on systems where the SBI implementation has
>> -# marked its reserved memory as not accessible from, or does not run
>> -# from the same memory as, Linux
>>   config ARCH_HIBERNATION_POSSIBLE
>> -	def_bool NONPORTABLE
>> +	def_bool y
>>   
>>   config ARCH_HIBERNATION_HEADER
>>   	def_bool HIBERNATION
>> -- 
>> 2.20.1
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V1 3/3] Revert "riscv: Use PUD/P4D/PGD pages for the linear mapping"

[Song Shuai]

From: Song Shuai <songshuaishuai@tinylab.org>

This reverts commit 3335068f87217ea59d08f462187dc856652eea15.

This commit maps the PMP regions from some versions of OpenSbi in
the linear mapping, that will lead to an access fault when doing
hibernation[1] or some speculative accesses.

The best_map_size() function from this commit doesn't check the
virtual alignment before choosing a map size, that will cause a
page fault[2]. We can let best_map_size() take the VA into
consideration via commit 49a0a3731596 ("riscv: Check the virtual
alignment before choosing a map size"), but that commit slows down
the boot time and consumes some system memory when UEFI booting.

This commit uses PUD/P4D/PGD pages for the linear mapping to improve
the performance is marginal from a recent talk [3] from Mike Rapoport.
OpenSbi had marked all the PMP-protected regions as "no-map" [4]
to practice this talk.

For all those reasons, let's revert this commit.

[1] https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
[2] https://lore.kernel.org/linux-riscv/tencent_7C3B580B47C1B17C16488EC1@qq.com/
[3] https://lwn.net/Articles/931406/
[4] https://github.com/riscv-software-src/opensbi/commit/8153b2622b08802cc542f30a1fcba407a5667ab9

Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
---
 arch/riscv/include/asm/page.h | 16 ---------------
 arch/riscv/mm/init.c          | 38 ++++++-----------------------------
 arch/riscv/mm/physaddr.c      | 16 ---------------
 drivers/of/fdt.c              | 11 +++++-----
 4 files changed, 11 insertions(+), 70 deletions(-)

diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index b55ba20903ec..21b346ab81c2 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -89,14 +89,6 @@ typedef struct page *pgtable_t;
 #define PTE_FMT "%08lx"
 #endif
 
-#ifdef CONFIG_64BIT
-/*
- * We override this value as its generic definition uses __pa too early in
- * the boot process (before kernel_map.va_pa_offset is set).
- */
-#define MIN_MEMBLOCK_ADDR      0
-#endif
-
 #ifdef CONFIG_MMU
 #define ARCH_PFN_OFFSET		(PFN_DOWN((unsigned long)phys_ram_base))
 #else
@@ -128,11 +120,7 @@ extern phys_addr_t phys_ram_base;
 #define is_linear_mapping(x)	\
 	((x) >= PAGE_OFFSET && (!IS_ENABLED(CONFIG_64BIT) || (x) < PAGE_OFFSET + KERN_VIRT_SIZE))
 
-#ifndef CONFIG_DEBUG_VIRTUAL
 #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + kernel_map.va_pa_offset))
-#else
-void *linear_mapping_pa_to_va(unsigned long x);
-#endif
 #define kernel_mapping_pa_to_va(y)	({					\
 	unsigned long _y = (unsigned long)(y);					\
 	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ?			\
@@ -141,11 +129,7 @@ void *linear_mapping_pa_to_va(unsigned long x);
 	})
 #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
 
-#ifndef CONFIG_DEBUG_VIRTUAL
 #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - kernel_map.va_pa_offset)
-#else
-phys_addr_t linear_mapping_va_to_pa(unsigned long x);
-#endif
 #define kernel_mapping_va_to_pa(y) ({						\
 	unsigned long _y = (unsigned long)(y);					\
 	(IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 38c4b4d6b64f..4561781bcf60 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -216,14 +216,6 @@ static void __init setup_bootmem(void)
 	phys_ram_end = memblock_end_of_DRAM();
 	if (!IS_ENABLED(CONFIG_XIP_KERNEL))
 		phys_ram_base = memblock_start_of_DRAM();
-
-	/*
-	 * In 64-bit, any use of __va/__pa before this point is wrong as we
-	 * did not know the start of DRAM before.
-	 */
-	if (IS_ENABLED(CONFIG_64BIT))
-		kernel_map.va_pa_offset = PAGE_OFFSET - phys_ram_base;
-
 	/*
 	 * memblock allocator is not aware of the fact that last 4K bytes of
 	 * the addressable memory can not be mapped because of IS_ERR_VALUE
@@ -662,16 +654,9 @@ void __init create_pgd_mapping(pgd_t *pgdp,
 
 static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size)
 {
-	if (!(base & (PGDIR_SIZE - 1)) && size >= PGDIR_SIZE)
-		return PGDIR_SIZE;
-
-	if (!(base & (P4D_SIZE - 1)) && size >= P4D_SIZE)
-		return P4D_SIZE;
-
-	if (!(base & (PUD_SIZE - 1)) && size >= PUD_SIZE)
-		return PUD_SIZE;
-
-	if (!(base & (PMD_SIZE - 1)) && size >= PMD_SIZE)
+	/* Upgrade to PMD_SIZE mappings whenever possible */
+	base &= PMD_SIZE - 1;
+	if (!base && size >= PMD_SIZE)
 		return PMD_SIZE;
 
 	return PAGE_SIZE;
@@ -1037,22 +1022,11 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
 	set_satp_mode(dtb_pa);
 #endif
 
-	/*
-	 * In 64-bit, we defer the setup of va_pa_offset to setup_bootmem,
-	 * where we have the system memory layout: this allows us to align
-	 * the physical and virtual mappings and then make use of PUD/P4D/PGD
-	 * for the linear mapping. This is only possible because the kernel
-	 * mapping lies outside the linear mapping.
-	 * In 32-bit however, as the kernel resides in the linear mapping,
-	 * setup_vm_final can not change the mapping established here,
-	 * otherwise the same kernel addresses would get mapped to different
-	 * physical addresses (if the start of dram is different from the
-	 * kernel physical address start).
-	 */
-	kernel_map.va_pa_offset = IS_ENABLED(CONFIG_64BIT) ?
-				0UL : PAGE_OFFSET - kernel_map.phys_addr;
+	kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
 	kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
 
+	phys_ram_base = kernel_map.phys_addr;
+
 	/*
 	 * The default maximal physical memory size is KERN_VIRT_SIZE for 32-bit
 	 * kernel, whereas for 64-bit kernel, the end of the virtual address
diff --git a/arch/riscv/mm/physaddr.c b/arch/riscv/mm/physaddr.c
index 18706f457da7..9b18bda74154 100644
--- a/arch/riscv/mm/physaddr.c
+++ b/arch/riscv/mm/physaddr.c
@@ -33,19 +33,3 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
 	return __va_to_pa_nodebug(x);
 }
 EXPORT_SYMBOL(__phys_addr_symbol);
-
-phys_addr_t linear_mapping_va_to_pa(unsigned long x)
-{
-	BUG_ON(!kernel_map.va_pa_offset);
-
-	return ((unsigned long)(x) - kernel_map.va_pa_offset);
-}
-EXPORT_SYMBOL(linear_mapping_va_to_pa);
-
-void *linear_mapping_pa_to_va(unsigned long x)
-{
-	BUG_ON(!kernel_map.va_pa_offset);
-
-	return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
-}
-EXPORT_SYMBOL(linear_mapping_pa_to_va);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bf502ba8da95..c28aedd7ae1f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -888,13 +888,12 @@ const void * __init of_flat_dt_match_machine(const void *default_match,
 static void __early_init_dt_declare_initrd(unsigned long start,
 					   unsigned long end)
 {
-	/*
-	 * __va() is not yet available this early on some platforms. In that
-	 * case, the platform uses phys_initrd_start/phys_initrd_size instead
-	 * and does the VA conversion itself.
+	/* ARM64 would cause a BUG to occur here when CONFIG_DEBUG_VM is
+	 * enabled since __va() is called too early. ARM64 does make use
+	 * of phys_initrd_start/phys_initrd_size so we can skip this
+	 * conversion.
 	 */
-	if (!IS_ENABLED(CONFIG_ARM64) &&
-	    !(IS_ENABLED(CONFIG_RISCV) && IS_ENABLED(CONFIG_64BIT))) {
+	if (!IS_ENABLED(CONFIG_ARM64)) {
 		initrd_start = (unsigned long)__va(start);
 		initrd_end = (unsigned long)__va(end);
 		initrd_below_start_ok = 1;
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[Alexandre Ghiti]

Hi Song,

On Sun, Jun 25, 2023 at 4:10 PM Song Shuai <songshuaishuai@tinylab.org> wrote:
>
> We have encountered these two issues about huge-paged linear mapping since 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 3335068f8721
> ("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in linear mapping.
> The hibernation process attempted to save/restore these mapped regions resulting in access fault.
>
> This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hibernation 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 0x80040000 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 alignment
> before choosing a map size. and then a "VA hole" was created where page faults always occurred.
>
> This issue was fixed by commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size"),
> But this fixing commit has a side-effect ("the possible third one" as Alex said in this thread).
> There are numerous PTE allocations slowing down the boot time and consuming some system memory when UEFI booting
> (Note that it's not involved when booting directly with OpenSbi, where phys_ram_base is the 2M-aligned base of DRAM).
>
> In my test, compared with/out reverting both commit 49a0a3731596 and commit 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/P4D/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 mapping")
> - commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size")
> - commit ed309ce52218 ("RISC-V: mark hibernation as nonportable")
>
> [1]: https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
> [2]: https://lore.kernel.org/linux-kernel/20230530080425.18612-1-alexghiti@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/8153b2622b08802cc542f30a1fcba407a5667ab9
>
> 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
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3152 bytes --]

Hey,

On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote:

> Sorry for the delayed reply,

It wasn't really delayed at all actually, you replied within an hour or
so, AFAICT.

> My tinylab email went something wrong, I'll use gmail in this thread.
> 
> 在 2023/6/25 22:18, Conor Dooley 写道:
> > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
> > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
> > > 
> > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
> > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
> > > load address which was placed at a PMD boundary.
> > 
> > > And firmware always
> > > correctly mark resident memory, or memory protected with PMP as
> > > per the devicetree specification and/or the UEFI specification.
> > 
> > But this is not true? The versions of OpenSBI that you mention in your
> > cover letter do not do this.
> > Please explain.
> > 
> 
> At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the
> DT/UEFI spec. This statement is excerpted from "Reserved memory for resident
> firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now.
> How about deleting this one?

It is incorrect, so it will need to be removed, yes.
Unfortunately writing a doc does not fix the existing implementations :(

> Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can
> avoid the mapping of firmware memory, I will make it clear in the next
> version.

To be honest, I'd like to see this revert as the final commit in a
series that deals with the problem by actually reserving the regions,
rather than a set of reverts that go back to how we were.
I was hoping that someone who cares about hibernation support would be
interested in working on that - *cough* starfive *cough*, although maybe
they just fixed their OpenSBI and moved on.
If there were no volunteers, my intention was to add a firmware erratum
that would probe the SBI implementation & version IDs, and add a firmware
erratum that'd parse the DT for the offending regions and reserve them.

Cheers,
Conor.

> > > So those regions will not be mapped in the linear mapping and they
> > > can be safely saved/restored by hibernation.
> > > 
> > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
> > > ---
> > >   arch/riscv/Kconfig | 5 +----
> > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 5966ad97c30c..17b5fc7f54d4 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -800,11 +800,8 @@ menu "Power management options"
> > >   source "kernel/power/Kconfig"
> > > -# Hibernation is only possible on systems where the SBI implementation has
> > > -# marked its reserved memory as not accessible from, or does not run
> > > -# from the same memory as, Linux
> > >   config ARCH_HIBERNATION_POSSIBLE
> > > -	def_bool NONPORTABLE
> > > +	def_bool y
> > >   config ARCH_HIBERNATION_HEADER
> > >   	def_bool HIBERNATION
> > > -- 
> > > 2.20.1
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Palmer Dabbelt]

On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote:
> Hey,
>
> On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote:
>
>> Sorry for the delayed reply,
>
> It wasn't really delayed at all actually, you replied within an hour or
> so, AFAICT.
>
>> My tinylab email went something wrong, I'll use gmail in this thread.
>> 
>> 在 2023/6/25 22:18, Conor Dooley 写道:
>> > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
>> > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
>> > > 
>> > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
>> > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
>> > > load address which was placed at a PMD boundary.
>> > 
>> > > And firmware always
>> > > correctly mark resident memory, or memory protected with PMP as
>> > > per the devicetree specification and/or the UEFI specification.
>> > 
>> > But this is not true? The versions of OpenSBI that you mention in your
>> > cover letter do not do this.
>> > Please explain.
>> > 
>> 
>> At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the
>> DT/UEFI spec. This statement is excerpted from "Reserved memory for resident
>> firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now.
>> How about deleting this one?
>
> It is incorrect, so it will need to be removed, yes.
> Unfortunately writing a doc does not fix the existing implementations :(
>
>> Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can
>> avoid the mapping of firmware memory, I will make it clear in the next
>> version.
>
> To be honest, I'd like to see this revert as the final commit in a
> series that deals with the problem by actually reserving the regions,
> rather than a set of reverts that go back to how we were.
> I was hoping that someone who cares about hibernation support would be
> interested in working on that - *cough* starfive *cough*, although maybe
> they just fixed their OpenSBI and moved on.
> If there were no volunteers, my intention was to add a firmware erratum
> that would probe the SBI implementation & version IDs, and add a firmware
> erratum that'd parse the DT for the offending regions and reserve them.

Is there any actual use case for hibernation on these boards?  Maybe 
it's simpler to just add a "reserved regions actually work" sort of 
property and then have new firmware set it -- that way we can avoid 
sorting through all the old stuff nobody cares about and just get on 
with fixing the stuff people use.

>
> Cheers,
> Conor.
>
>> > > So those regions will not be mapped in the linear mapping and they
>> > > can be safely saved/restored by hibernation.
>> > > 
>> > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
>> > > ---
>> > >   arch/riscv/Kconfig | 5 +----
>> > >   1 file changed, 1 insertion(+), 4 deletions(-)
>> > > 
>> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > > index 5966ad97c30c..17b5fc7f54d4 100644
>> > > --- a/arch/riscv/Kconfig
>> > > +++ b/arch/riscv/Kconfig
>> > > @@ -800,11 +800,8 @@ menu "Power management options"
>> > >   source "kernel/power/Kconfig"
>> > > -# Hibernation is only possible on systems where the SBI implementation has
>> > > -# marked its reserved memory as not accessible from, or does not run
>> > > -# from the same memory as, Linux
>> > >   config ARCH_HIBERNATION_POSSIBLE
>> > > -	def_bool NONPORTABLE
>> > > +	def_bool y
>> > >   config ARCH_HIBERNATION_HEADER
>> > >   	def_bool HIBERNATION
>> > > -- 
>> > > 2.20.1
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3560 bytes --]

On Sun, Jun 25, 2023 at 03:36:06PM -0700, Palmer Dabbelt wrote:
> On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote:
> > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote:
> > > 在 2023/6/25 22:18, Conor Dooley 写道:
> > > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
> > > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
> > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages
> > > for the
> > > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
> > > > > load address which was placed at a PMD boundary.
> > > > > > And firmware always
> > > > > correctly mark resident memory, or memory protected with PMP as
> > > > > per the devicetree specification and/or the UEFI specification.
> > > > > But this is not true? The versions of OpenSBI that you mention
> > > in your
> > > > cover letter do not do this.
> > > > Please explain.
> > > >
> > > 
> > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the
> > > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident
> > > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now.
> > > How about deleting this one?
> > 
> > It is incorrect, so it will need to be removed, yes.
> > Unfortunately writing a doc does not fix the existing implementations :(
> > 
> > > Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can
> > > avoid the mapping of firmware memory, I will make it clear in the next
> > > version.
> > 
> > To be honest, I'd like to see this revert as the final commit in a
> > series that deals with the problem by actually reserving the regions,
> > rather than a set of reverts that go back to how we were.
> > I was hoping that someone who cares about hibernation support would be
> > interested in working on that - *cough* starfive *cough*, although maybe
> > they just fixed their OpenSBI and moved on.
> > If there were no volunteers, my intention was to add a firmware erratum
> > that would probe the SBI implementation & version IDs, and add a firmware
> > erratum that'd parse the DT for the offending regions and reserve them.
> 
> Is there any actual use case for hibernation on these boards?  Maybe it's
> simpler to just add a "reserved regions actually work" sort of property and
> then have new firmware set it -- that way we can avoid sorting through all
> the old stuff nobody cares about and just get on with fixing the stuff
> people use.

What is "old stuff nobody cares about"? The first version of OpenSBI with
the fix shipped only the other day, so effectively all current stuff has
this problem. Certainly everything shipping from vendors at the moment
has the problem, and probably whatever downstream, custom versions of
OpenSBI also have it.

Also, the problem isn't just limited to hibernation apparently. I
think it was mentioned in the cover letter that according to Rob,
without being marked as no-map we could also see speculative access &
potentially some of the memory debugging stuff walking these regions.

I'm not sure how you'd intend communicating "reserved regions actually
work", I figure you mean via DT?
I don't really see the benefit of adding a property for those who are
behaving, if we can detect the versions of the one relevant SBI
implementation that are broken at runtime. DT hat on, even less so.
Perhaps I am missing your point, and there's another angle (like trying
to per firmware code)?


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Palmer Dabbelt]

On Mon, 26 Jun 2023 06:34:43 PDT (-0700), Conor Dooley wrote:
> On Sun, Jun 25, 2023 at 03:36:06PM -0700, Palmer Dabbelt wrote:
>> On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote:
>> > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote:
>> > > 在 2023/6/25 22:18, Conor Dooley 写道:
>> > > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
>> > > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
>> > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages
>> > > for the
>> > > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
>> > > > > load address which was placed at a PMD boundary.
>> > > > > > And firmware always
>> > > > > correctly mark resident memory, or memory protected with PMP as
>> > > > > per the devicetree specification and/or the UEFI specification.
>> > > > > But this is not true? The versions of OpenSBI that you mention
>> > > in your
>> > > > cover letter do not do this.
>> > > > Please explain.
>> > > >
>> > > 
>> > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the
>> > > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident
>> > > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now.
>> > > How about deleting this one?
>> > 
>> > It is incorrect, so it will need to be removed, yes.
>> > Unfortunately writing a doc does not fix the existing implementations :(
>> > 
>> > > Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can
>> > > avoid the mapping of firmware memory, I will make it clear in the next
>> > > version.
>> > 
>> > To be honest, I'd like to see this revert as the final commit in a
>> > series that deals with the problem by actually reserving the regions,
>> > rather than a set of reverts that go back to how we were.
>> > I was hoping that someone who cares about hibernation support would be
>> > interested in working on that - *cough* starfive *cough*, although maybe
>> > they just fixed their OpenSBI and moved on.
>> > If there were no volunteers, my intention was to add a firmware erratum
>> > that would probe the SBI implementation & version IDs, and add a firmware
>> > erratum that'd parse the DT for the offending regions and reserve them.
>> 
>> Is there any actual use case for hibernation on these boards?  Maybe it's
>> simpler to just add a "reserved regions actually work" sort of property and
>> then have new firmware set it -- that way we can avoid sorting through all
>> the old stuff nobody cares about and just get on with fixing the stuff
>> people use.
>
> What is "old stuff nobody cares about"? The first version of OpenSBI with
> the fix shipped only the other day, so effectively all current stuff has
> this problem. Certainly everything shipping from vendors at the moment
> has the problem, and probably whatever downstream, custom versions of
> OpenSBI also have it.

Ya, so "old stuff" is everything -- but that's all already broken, so 
nothing we can do about it.  IIUC there's nothing shipping that 
functions correctly here, so it's just a matter of detecting everything 
before the bug.

> Also, the problem isn't just limited to hibernation apparently. I
> think it was mentioned in the cover letter that according to Rob,
> without being marked as no-map we could also see speculative access &
> potentially some of the memory debugging stuff walking these regions.

We've got a bunch of other problems around speculative accesses to these 
regions in M-mode, so we'll have to deal with it at some point anyway.

> I'm not sure how you'd intend communicating "reserved regions actually
> work", I figure you mean via DT?

Somewhere in DT.  I hadn't thought about it a ton, just adding some 
property that says "this doesn't have the bug" was roughly the idea.

> I don't really see the benefit of adding a property for those who are
> behaving, if we can detect the versions of the one relevant SBI
> implementation that are broken at runtime. DT hat on, even less so.
> Perhaps I am missing your point, and there's another angle (like trying
> to per firmware code)?

If it's easy to figure out which versions are broken that seems fine to 
me.  My worry was just that's hard to do (folks forking OpenSBI) and it 
might be easier to just 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable"

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 5389 bytes --]

On Mon, Jun 26, 2023 at 07:43:42AM -0700, Palmer Dabbelt wrote:
> On Mon, 26 Jun 2023 06:34:43 PDT (-0700), Conor Dooley wrote:
> > On Sun, Jun 25, 2023 at 03:36:06PM -0700, Palmer Dabbelt wrote:
> > > On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote:
> > > > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote:
> > > > > 在 2023/6/25 22:18, Conor Dooley 写道:
> > > > > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote:
> > > > > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
> > > > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages
> > > > > for the
> > > > > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
> > > > > > > load address which was placed at a PMD boundary.
> > > > > > > > And firmware always
> > > > > > > correctly mark resident memory, or memory protected with PMP as
> > > > > > > per the devicetree specification and/or the UEFI specification.
> > > > > > > But this is not true? The versions of OpenSBI that you mention
> > > > > in your
> > > > > > cover letter do not do this.
> > > > > > Please explain.
> > > > > >
> > > > > > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed
> > > don't obey the
> > > > > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident
> > > > > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now.
> > > > > How about deleting this one?
> > > > > It is incorrect, so it will need to be removed, yes.
> > > > Unfortunately writing a doc does not fix the existing implementations :(
> > > > > > Actually with 3335068f8721 reverted, the change of
> > > MIN_MEMBLOCK_ADDR can
> > > > > avoid the mapping of firmware memory, I will make it clear in the next
> > > > > version.
> > > > > To be honest, I'd like to see this revert as the final commit in
> > > a
> > > > series that deals with the problem by actually reserving the regions,
> > > > rather than a set of reverts that go back to how we were.
> > > > I was hoping that someone who cares about hibernation support would be
> > > > interested in working on that - *cough* starfive *cough*, although maybe
> > > > they just fixed their OpenSBI and moved on.
> > > > If there were no volunteers, my intention was to add a firmware erratum
> > > > that would probe the SBI implementation & version IDs, and add a firmware
> > > > erratum that'd parse the DT for the offending regions and reserve them.
> > > 
> > > Is there any actual use case for hibernation on these boards?  Maybe it's
> > > simpler to just add a "reserved regions actually work" sort of property and
> > > then have new firmware set it -- that way we can avoid sorting through all
> > > the old stuff nobody cares about and just get on with fixing the stuff
> > > people use.
> > 
> > What is "old stuff nobody cares about"? The first version of OpenSBI with
> > the fix shipped only the other day, so effectively all current stuff has
> > this problem. Certainly everything shipping from vendors at the moment
> > has the problem, and probably whatever downstream, custom versions of
> > OpenSBI also have it.
> 
> Ya, so "old stuff" is everything -- but that's all already broken, so
> nothing we can do about it.  IIUC there's nothing shipping that functions
> correctly here, so it's just a matter of detecting everything before the
> bug.

g5soc functions correctly, because our HSS (which a small portion of
OpenSBI is built into) doesn't run out of system memory.

> > Also, the problem isn't just limited to hibernation apparently. I
> > think it was mentioned in the cover letter that according to Rob,
> > without being marked as no-map we could also see speculative access &
> > potentially some of the memory debugging stuff walking these regions.
> 
> We've got a bunch of other problems around speculative accesses to these
> regions in M-mode, so we'll have to deal with it at some point anyway.
> 
> > I'm not sure how you'd intend communicating "reserved regions actually
> > work", I figure you mean via DT?
> 
> Somewhere in DT.  I hadn't thought about it a ton, just adding some property
> that says "this doesn't have the bug" was roughly the idea.
> 
> > I don't really see the benefit of adding a property for those who are
> > behaving, if we can detect the versions of the one relevant SBI
> > implementation that are broken at runtime. DT hat on, even less so.
> > Perhaps I am missing your point, and there's another angle (like trying
> > to per firmware code)?
> 
> If it's easy to figure out which versions are broken that seems fine to me.
> My worry was just that's hard to do (folks forking OpenSBI) and it might be
> easier to just

You cut yourself off here :/
If people are forking OpenSBI, but reusing the version/implementation
info, and changing behaviours, I am not really sure what we are supposed
to do in general. I'd vote for apply the erratum to those forks, based on
the version info, until they make changes
That's pretty much the same thing that'd happen under your proposal anyway?
I went and sent a PR last week to get a custom ID for the HSS on g5soc so
that we can be differentiated, everyone else that doesn't want to be
tarred with the same brush probably should too, if they are sufficiently
different.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[Alexandre Ghiti]

On Sun, Jun 25, 2023 at 10:36 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi Song,
>
> On Sun, Jun 25, 2023 at 4:10 PM Song Shuai <songshuaishuai@tinylab.org> wrote:
> >
> > We have encountered these two issues about huge-paged linear mapping since 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 3335068f8721
> > ("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in linear mapping.
> > The hibernation process attempted to save/restore these mapped regions resulting in access fault.
> >
> > This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hibernation 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 0x80040000 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 alignment
> > before choosing a map size. and then a "VA hole" was created where page faults always occurred.
> >
> > This issue was fixed by commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size"),
> > But this fixing commit has a side-effect ("the possible third one" as Alex said in this thread).
> > There are numerous PTE allocations slowing down the boot time and consuming some system memory when UEFI booting
> > (Note that it's not involved when booting directly with OpenSbi, where phys_ram_base is the 2M-aligned base of DRAM).
> >
> > In my test, compared with/out reverting both commit 49a0a3731596 and commit 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/P4D/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.

@Song Shuai : can you test the following please?

commit a35b5e5e3f29e415f54fea064176315e79e21fb7 (HEAD ->
dev/alex/align_va_pa_v1)
Author: Alexandre Ghiti <alexghiti@rivosinc.com>
Date:   Mon Jun 5 14:26:55 2023 +0000

    riscv: Start of DRAM should at least be aligned on PMD size for
the direct mapping

    So that we do not end up mapping the whole linear mapping using 4K
    pages, which is slow at boot time, and also very likely at runtime.

    So make sure we align the start of DRAM on a PMD boundary.

    Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 4fa420faa780..4a43ec275c6d 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -214,8 +214,13 @@ static void __init setup_bootmem(void)
        memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);

        phys_ram_end = memblock_end_of_DRAM();
+
+       /*
+        * Make sure we align the start of the memory on a PMD boundary so that
+        * at worst, we map the linear mapping with PMD mappings.
+        */
        if (!IS_ENABLED(CONFIG_XIP_KERNEL))
-               phys_ram_base = memblock_start_of_DRAM();
+               phys_ram_base = memblock_start_of_DRAM() & PMD_MASK;

        /*
         * In 64-bit, any use of __va/__pa before this point is wrong as we

An example of output when phys_ram_base is not aligned on a 2MB boundary:

---[ Linear mapping ]---
0xffffaf8000008000-0xffffaf8000200000    0x0000000080008000      2016K
PTE     D A G . . W R V
0xffffaf8000200000-0xffffaf8000e00000    0x0000000080200000        12M
PMD     D A G . . . R V
0xffffaf8000e00000-0xffffaf8001200000    0x0000000080e00000         4M
PMD     D A G . . W R V
0xffffaf8001200000-0xffffaf8001a00000    0x0000000081200000         8M
PMD     D A G . . . R V
0xffffaf8001a00000-0xffffaf807e600000    0x0000000081a00000      1996M
PMD     D A G . . W R V
0xffffaf807e600000-0xffffaf807e714000    0x00000000fe600000      1104K
PTE     D A G . . W R V
0xffffaf807e715000-0xffffaf807e718000    0x00000000fe715000        12K
PTE     D A G . . W R V
0xffffaf807e71b000-0xffffaf807e71c000    0x00000000fe71b000         4K
PTE     D A G . . W R V
0xffffaf807e720000-0xffffaf807e800000    0x00000000fe720000       896K
PTE     D A G . . W R V
0xffffaf807e800000-0xffffaf807fe00000    0x00000000fe800000        22M
PMD     D A G . . W R V
0xffffaf807fe00000-0xffffaf807ff54000    0x00000000ffe00000      1360K
PTE     D A G . . W R V
0xffffaf807ff55000-0xffffaf8080000000    0x00000000fff55000       684K
PTE     D A G . . W R V
0xffffaf8080000000-0xffffaf83c0000000    0x0000000100000000        13G
PUD     D A G . . W R V
0xffffaf83c0000000-0xffffaf83ffe00000    0x0000000440000000      1022M
PMD     D A G . . W R V
0xffffaf83ffe00000-0xffffaf8400000000    0x000000047fe00000         2M
PTE     D A G . . W R V

I found that it was easier to align phys_ram_base on the lower 2MB
boundary. Aligning on the upper boundary is more complicated to me
since there may be "something" between phys_ram_base and the upper 2MB
boundary that needs to be accessed using the linear mapping (DT is
accessed using fixmap so not a problem, initrd? ACPI tables? I don't
know actually).

Weirdly simple though, I may be missing something, so any comment/test
is welcome, it is currently running our internal CI.

Thanks,

Alex

>
> 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 mapping")
> > - commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size")
> > - commit ed309ce52218 ("RISC-V: mark hibernation as nonportable")
> >
> > [1]: https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
> > [2]: https://lore.kernel.org/linux-kernel/20230530080425.18612-1-alexghiti@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/8153b2622b08802cc542f30a1fcba407a5667ab9
> >
> > 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
> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[Song Shuai]

Hi Alex,

在 2023/6/27 19:47, Alexandre Ghiti 写道:
> On Sun, Jun 25, 2023 at 10:36 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>>
>> Hi Song,
>>
>> On Sun, Jun 25, 2023 at 4:10 PM Song Shuai <songshuaishuai@tinylab.org> wrote:
>>>
>>> We have encountered these two issues about huge-paged linear mapping since 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 3335068f8721
>>> ("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in linear mapping.
>>> The hibernation process attempted to save/restore these mapped regions resulting in access fault.
>>>
>>> This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hibernation 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 0x80040000 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 alignment
>>> before choosing a map size. and then a "VA hole" was created where page faults always occurred.
>>>
>>> This issue was fixed by commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size"),
>>> But this fixing commit has a side-effect ("the possible third one" as Alex said in this thread).
>>> There are numerous PTE allocations slowing down the boot time and consuming some system memory when UEFI booting
>>> (Note that it's not involved when booting directly with OpenSbi, where phys_ram_base is the 2M-aligned base of DRAM).
>>>
>>> In my test, compared with/out reverting both commit 49a0a3731596 and commit 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/P4D/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.
> 
> @Song Shuai : can you test the following please?
> 
> commit a35b5e5e3f29e415f54fea064176315e79e21fb7 (HEAD ->
> dev/alex/align_va_pa_v1)
> Author: Alexandre Ghiti <alexghiti@rivosinc.com>
> Date:   Mon Jun 5 14:26:55 2023 +0000
> 
>      riscv: Start of DRAM should at least be aligned on PMD size for
> the direct mapping
> 
>      So that we do not end up mapping the whole linear mapping using 4K
>      pages, which is slow at boot time, and also very likely at runtime.
> 
>      So make sure we align the start of DRAM on a PMD boundary.
> 
>      Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 4fa420faa780..4a43ec275c6d 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -214,8 +214,13 @@ static void __init setup_bootmem(void)
>          memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
> 
>          phys_ram_end = memblock_end_of_DRAM();
> +
> +       /*
> +        * Make sure we align the start of the memory on a PMD boundary so that
> +        * at worst, we map the linear mapping with PMD mappings.
> +        */
>          if (!IS_ENABLED(CONFIG_XIP_KERNEL))
> -               phys_ram_base = memblock_start_of_DRAM();
> +               phys_ram_base = memblock_start_of_DRAM() & PMD_MASK;
> 
>          /*
>           * In 64-bit, any use of __va/__pa before this point is wrong as we
> 
I tested your patch and it really fixed what I concerned :

`There are numerous PTE allocations slowing down the boot time and 
consuming some system memory when UEFI booting.`

FYI, I posted the `ptdmp` and the available memory with/out the patch 
from my test:

```
 >> v6.4

---[ Linear mapping ]---
0xff60000000000000-0xff600000001c0000    0x0000000080040000      1792K 
PTE     D A G . . W R V
0xff600000001c0000-0xff60000000bc0000    0x0000000080200000        10M 
PTE     D A G . . . R V
0xff60000000bc0000-0xff60000000fc0000    0x0000000080c00000         4M 
PTE     D A G . . W R V
0xff60000000fc0000-0xff600000015c0000    0x0000000081000000         6M 
PTE     D A G . . . R V
0xff600000015c0000-0xff600000ffdfd000    0x0000000081600000   4169972K 
PTE     D A G . . W R V
0xff600000fffbf000-0xff600000fffc0000    0x000000017ffff000         4K 
PTE     D A G . . W R V

 >> v6.4 with the patch

---[ Linear mapping ]---
0xff60000000040000-0xff60000000200000    0x0000000080040000      1792K 
PTE     D A G . . W R V
0xff60000000200000-0xff60000000c00000    0x0000000080200000        10M 
PMD     D A G . . . R V
0xff60000000c00000-0xff60000001000000    0x0000000080c00000         4M 
PMD     D A G . . W R V
0xff60000001000000-0xff60000001600000    0x0000000081000000         6M 
PMD     D A G . . . R V
0xff60000001600000-0xff60000040000000    0x0000000081600000      1002M 
PMD     D A G . . W R V
0xff60000040000000-0xff600000c0000000    0x00000000c0000000         2G 
PUD     D A G . . W R V
0xff600000c0000000-0xff600000ffe00000    0x0000000140000000      1022M 
PMD     D A G . . W R V
0xff600000ffe00000-0xff600000ffe3d000    0x000000017fe00000       244K 
PTE     D A G . . W R V
0xff600000fffff000-0xff60000100000000    0x000000017ffff000         4K 
PTE     D A G . . W R V
```

```
 >> v6.4

Memory: 3945340K/4194048K available (8391K kernel code, 4959K rwdata, 
4096K rodata, 2195K init, 476K bss, 248708K reserved, 0K cma-reserved)

 >> v6.4 with the patch

Memory: 3953524K/4194048K available (8391K kernel code, 4959K rwdata, 
4096K rodata, 2195K init, 476K bss, 240524K reserved, 0K cma-reserved)
```
> An example of output when phys_ram_base is not aligned on a 2MB boundary:
> 
> ---[ Linear mapping ]---
> 0xffffaf8000008000-0xffffaf8000200000    0x0000000080008000      2016K
> PTE     D A G . . W R V
> 0xffffaf8000200000-0xffffaf8000e00000    0x0000000080200000        12M
> PMD     D A G . . . R V
> 0xffffaf8000e00000-0xffffaf8001200000    0x0000000080e00000         4M
> PMD     D A G . . W R V
> 0xffffaf8001200000-0xffffaf8001a00000    0x0000000081200000         8M
> PMD     D A G . . . R V
> 0xffffaf8001a00000-0xffffaf807e600000    0x0000000081a00000      1996M
> PMD     D A G . . W R V
> 0xffffaf807e600000-0xffffaf807e714000    0x00000000fe600000      1104K
> PTE     D A G . . W R V
> 0xffffaf807e715000-0xffffaf807e718000    0x00000000fe715000        12K
> PTE     D A G . . W R V
> 0xffffaf807e71b000-0xffffaf807e71c000    0x00000000fe71b000         4K
> PTE     D A G . . W R V
> 0xffffaf807e720000-0xffffaf807e800000    0x00000000fe720000       896K
> PTE     D A G . . W R V
> 0xffffaf807e800000-0xffffaf807fe00000    0x00000000fe800000        22M
> PMD     D A G . . W R V
> 0xffffaf807fe00000-0xffffaf807ff54000    0x00000000ffe00000      1360K
> PTE     D A G . . W R V
> 0xffffaf807ff55000-0xffffaf8080000000    0x00000000fff55000       684K
> PTE     D A G . . W R V
> 0xffffaf8080000000-0xffffaf83c0000000    0x0000000100000000        13G
> PUD     D A G . . W R V
> 0xffffaf83c0000000-0xffffaf83ffe00000    0x0000000440000000      1022M
> PMD     D A G . . W R V
> 0xffffaf83ffe00000-0xffffaf8400000000    0x000000047fe00000         2M
> PTE     D A G . . W R V
> 
> I found that it was easier to align phys_ram_base on the lower 2MB
> boundary. Aligning on the upper boundary is more complicated to me
> since there may be "something" between phys_ram_base and the upper 2MB
> boundary that needs to be accessed using the linear mapping (DT is
> accessed using fixmap so not a problem, initrd? ACPI tables? I don't
> know actually).
> 

And would there be possible influence from the diff between 
phys_ram_base and memblock_start_of_DRAM()?

> Weirdly simple though, I may be missing something, so any comment/test
> is welcome, it is currently running our internal CI.
> 
> Thanks,
> 
> Alex
> 
>>
>> 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.
>>>

I noticed best_map_size() still creates some huge-paged mapping (like 
above 2G PUD) with this patch.
How about to revert best_map_size() to disable huge-paged mapping to 
practice the Mike's talk.

>>> For all those reasons, let's revert these related commits:
>>>
>>> - commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the linear mapping")
>>> - commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size")
>>> - commit ed309ce52218 ("RISC-V: mark hibernation as nonportable")
>>>
>>> [1]: https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
>>> [2]: https://lore.kernel.org/linux-kernel/20230530080425.18612-1-alexghiti@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/8153b2622b08802cc542f30a1fcba407a5667ab9
>>>
>>> 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
>>>
> 

-- 
Thanks
Song Shuai

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V1 0/3] Revert huge-paged linear mapping and its related fixups

[Alexandre Ghiti]

On Tue, Jun 27, 2023 at 5:13 PM Song Shuai <suagrfillet@gmail.com> wrote:
>
> Hi Alex,
>
> 在 2023/6/27 19:47, Alexandre Ghiti 写道:
> > On Sun, Jun 25, 2023 at 10:36 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >>
> >> Hi Song,
> >>
> >> On Sun, Jun 25, 2023 at 4:10 PM Song Shuai <songshuaishuai@tinylab.org> wrote:
> >>>
> >>> We have encountered these two issues about huge-paged linear mapping since 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 3335068f8721
> >>> ("riscv: Use PUD/P4D/PGD pages for the linear mapping") mapped them in linear mapping.
> >>> The hibernation process attempted to save/restore these mapped regions resulting in access fault.
> >>>
> >>> This issue was temporarily fixed by commit ed309ce52218 ("RISC-V: mark hibernation 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 0x80040000 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 alignment
> >>> before choosing a map size. and then a "VA hole" was created where page faults always occurred.
> >>>
> >>> This issue was fixed by commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size"),
> >>> But this fixing commit has a side-effect ("the possible third one" as Alex said in this thread).
> >>> There are numerous PTE allocations slowing down the boot time and consuming some system memory when UEFI booting
> >>> (Note that it's not involved when booting directly with OpenSbi, where phys_ram_base is the 2M-aligned base of DRAM).
> >>>
> >>> In my test, compared with/out reverting both commit 49a0a3731596 and commit 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/P4D/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.
> >
> > @Song Shuai : can you test the following please?
> >
> > commit a35b5e5e3f29e415f54fea064176315e79e21fb7 (HEAD ->
> > dev/alex/align_va_pa_v1)
> > Author: Alexandre Ghiti <alexghiti@rivosinc.com>
> > Date:   Mon Jun 5 14:26:55 2023 +0000
> >
> >      riscv: Start of DRAM should at least be aligned on PMD size for
> > the direct mapping
> >
> >      So that we do not end up mapping the whole linear mapping using 4K
> >      pages, which is slow at boot time, and also very likely at runtime.
> >
> >      So make sure we align the start of DRAM on a PMD boundary.
> >
> >      Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> >
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 4fa420faa780..4a43ec275c6d 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -214,8 +214,13 @@ static void __init setup_bootmem(void)
> >          memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
> >
> >          phys_ram_end = memblock_end_of_DRAM();
> > +
> > +       /*
> > +        * Make sure we align the start of the memory on a PMD boundary so that
> > +        * at worst, we map the linear mapping with PMD mappings.
> > +        */
> >          if (!IS_ENABLED(CONFIG_XIP_KERNEL))
> > -               phys_ram_base = memblock_start_of_DRAM();
> > +               phys_ram_base = memblock_start_of_DRAM() & PMD_MASK;
> >
> >          /*
> >           * In 64-bit, any use of __va/__pa before this point is wrong as we
> >
> I tested your patch and it really fixed what I concerned :
>
> `There are numerous PTE allocations slowing down the boot time and
> consuming some system memory when UEFI booting.`
>
> FYI, I posted the `ptdmp` and the available memory with/out the patch
> from my test:
>
> ```
>  >> v6.4
>
> ---[ Linear mapping ]---
> 0xff60000000000000-0xff600000001c0000    0x0000000080040000      1792K
> PTE     D A G . . W R V
> 0xff600000001c0000-0xff60000000bc0000    0x0000000080200000        10M
> PTE     D A G . . . R V
> 0xff60000000bc0000-0xff60000000fc0000    0x0000000080c00000         4M
> PTE     D A G . . W R V
> 0xff60000000fc0000-0xff600000015c0000    0x0000000081000000         6M
> PTE     D A G . . . R V
> 0xff600000015c0000-0xff600000ffdfd000    0x0000000081600000   4169972K
> PTE     D A G . . W R V
> 0xff600000fffbf000-0xff600000fffc0000    0x000000017ffff000         4K
> PTE     D A G . . W R V
>
>  >> v6.4 with the patch
>
> ---[ Linear mapping ]---
> 0xff60000000040000-0xff60000000200000    0x0000000080040000      1792K
> PTE     D A G . . W R V
> 0xff60000000200000-0xff60000000c00000    0x0000000080200000        10M
> PMD     D A G . . . R V
> 0xff60000000c00000-0xff60000001000000    0x0000000080c00000         4M
> PMD     D A G . . W R V
> 0xff60000001000000-0xff60000001600000    0x0000000081000000         6M
> PMD     D A G . . . R V
> 0xff60000001600000-0xff60000040000000    0x0000000081600000      1002M
> PMD     D A G . . W R V
> 0xff60000040000000-0xff600000c0000000    0x00000000c0000000         2G
> PUD     D A G . . W R V
> 0xff600000c0000000-0xff600000ffe00000    0x0000000140000000      1022M
> PMD     D A G . . W R V
> 0xff600000ffe00000-0xff600000ffe3d000    0x000000017fe00000       244K
> PTE     D A G . . W R V
> 0xff600000fffff000-0xff60000100000000    0x000000017ffff000         4K
> PTE     D A G . . W R V
> ```
>
> ```
>  >> v6.4
>
> Memory: 3945340K/4194048K available (8391K kernel code, 4959K rwdata,
> 4096K rodata, 2195K init, 476K bss, 248708K reserved, 0K cma-reserved)
>
>  >> v6.4 with the patch
>
> Memory: 3953524K/4194048K available (8391K kernel code, 4959K rwdata,
> 4096K rodata, 2195K init, 476K bss, 240524K reserved, 0K cma-reserved)
> ```
> > An example of output when phys_ram_base is not aligned on a 2MB boundary:
> >
> > ---[ Linear mapping ]---
> > 0xffffaf8000008000-0xffffaf8000200000    0x0000000080008000      2016K
> > PTE     D A G . . W R V
> > 0xffffaf8000200000-0xffffaf8000e00000    0x0000000080200000        12M
> > PMD     D A G . . . R V
> > 0xffffaf8000e00000-0xffffaf8001200000    0x0000000080e00000         4M
> > PMD     D A G . . W R V
> > 0xffffaf8001200000-0xffffaf8001a00000    0x0000000081200000         8M
> > PMD     D A G . . . R V
> > 0xffffaf8001a00000-0xffffaf807e600000    0x0000000081a00000      1996M
> > PMD     D A G . . W R V
> > 0xffffaf807e600000-0xffffaf807e714000    0x00000000fe600000      1104K
> > PTE     D A G . . W R V
> > 0xffffaf807e715000-0xffffaf807e718000    0x00000000fe715000        12K
> > PTE     D A G . . W R V
> > 0xffffaf807e71b000-0xffffaf807e71c000    0x00000000fe71b000         4K
> > PTE     D A G . . W R V
> > 0xffffaf807e720000-0xffffaf807e800000    0x00000000fe720000       896K
> > PTE     D A G . . W R V
> > 0xffffaf807e800000-0xffffaf807fe00000    0x00000000fe800000        22M
> > PMD     D A G . . W R V
> > 0xffffaf807fe00000-0xffffaf807ff54000    0x00000000ffe00000      1360K
> > PTE     D A G . . W R V
> > 0xffffaf807ff55000-0xffffaf8080000000    0x00000000fff55000       684K
> > PTE     D A G . . W R V
> > 0xffffaf8080000000-0xffffaf83c0000000    0x0000000100000000        13G
> > PUD     D A G . . W R V
> > 0xffffaf83c0000000-0xffffaf83ffe00000    0x0000000440000000      1022M
> > PMD     D A G . . W R V
> > 0xffffaf83ffe00000-0xffffaf8400000000    0x000000047fe00000         2M
> > PTE     D A G . . W R V
> >
> > I found that it was easier to align phys_ram_base on the lower 2MB
> > boundary. Aligning on the upper boundary is more complicated to me
> > since there may be "something" between phys_ram_base and the upper 2MB
> > boundary that needs to be accessed using the linear mapping (DT is
> > accessed using fixmap so not a problem, initrd? ACPI tables? I don't
> > know actually).
> >
>
> And would there be possible influence from the diff between
> phys_ram_base and memblock_start_of_DRAM()?

I don't know why there would be (doesn't mean there is not), and arm64
also does that https://elixir.bootlin.com/linux/latest/source/arch/arm64/mm/init.c#L285

>
> > Weirdly simple though, I may be missing something, so any comment/test
> > is welcome, it is currently running our internal CI.
> >
> > Thanks,
> >
> > Alex
> >
> >>
> >> 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.
> >>>
>
> I noticed best_map_size() still creates some huge-paged mapping (like
> above 2G PUD) with this patch.
> How about to revert best_map_size() to disable huge-paged mapping to
> practice the Mike's talk.
>

Mike's talk does not state that using PUD (and above) mappings is bad,
but just that it may not be worth the effort to try and keep them at
all cost during kernel life (they happen to be split by allocations)
since the performance benefit is marginal (if it even exists). On real
systems with terabytes of memory, this will help with boot duration
and memory consumption (ok, not significantly for this type of
systems).

Thanks for testing!


> >>> For all those reasons, let's revert these related commits:
> >>>
> >>> - commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the linear mapping")
> >>> - commit 49a0a3731596 ("riscv: Check the virtual alignment before choosing a map size")
> >>> - commit ed309ce52218 ("RISC-V: mark hibernation as nonportable")
> >>>
> >>> [1]: https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
> >>> [2]: https://lore.kernel.org/linux-kernel/20230530080425.18612-1-alexghiti@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/8153b2622b08802cc542f30a1fcba407a5667ab9
> >>>
> >>> 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
> >>>
> >
>
> --
> Thanks
> Song Shuai

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv