* [PATCH] riscv: mm: init: Consolidate vars, functions
@ 2021-05-16 13:15 Jisheng Zhang
2021-05-16 13:19 ` Jisheng Zhang
2021-06-01 20:21 ` Nathan Chancellor
0 siblings, 2 replies; 13+ messages in thread
From: Jisheng Zhang @ 2021-05-16 13:15 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel
From: Jisheng Zhang <jszhang@kernel.org>
Consolidate the following items in init.c
Staticize global vars as much as possible;
Add __initdata mark if the global var isn't needed after init
Add __init mark if the func isn't needed after init
Add __ro_after_init if the global var is read only after init
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
arch/riscv/include/asm/set_memory.h | 2 +-
arch/riscv/mm/init.c | 36 +++++++++++++++--------------
2 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
index 086f757e8ba3..9d4d455726d4 100644
--- a/arch/riscv/include/asm/set_memory.h
+++ b/arch/riscv/include/asm/set_memory.h
@@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
#endif
#if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
-void protect_kernel_linear_mapping_text_rodata(void);
+void __init protect_kernel_linear_mapping_text_rodata(void);
#else
static inline void protect_kernel_linear_mapping_text_rodata(void) {}
#endif
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 4c4c92ce0bb8..eac2d5c27b3e 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -53,7 +53,7 @@ struct pt_alloc_ops {
#endif
};
-static phys_addr_t dma32_phys_limit __ro_after_init;
+static phys_addr_t dma32_phys_limit __initdata;
static void __init zone_sizes_init(void)
{
@@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
#endif /* CONFIG_XIP_KERNEL */
#ifdef CONFIG_MMU
-static struct pt_alloc_ops _pt_ops __ro_after_init;
+static struct pt_alloc_ops _pt_ops __initdata;
#ifdef CONFIG_XIP_KERNEL
#define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
@@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
#endif
/* Offset between kernel mapping virtual address and kernel load address */
#ifdef CONFIG_64BIT
-unsigned long va_kernel_pa_offset;
+unsigned long va_kernel_pa_offset __ro_after_init;
EXPORT_SYMBOL(va_kernel_pa_offset);
#endif
#ifdef CONFIG_XIP_KERNEL
#define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
#endif
-unsigned long va_kernel_xip_pa_offset;
+unsigned long va_kernel_xip_pa_offset __ro_after_init;
EXPORT_SYMBOL(va_kernel_xip_pa_offset);
#ifdef CONFIG_XIP_KERNEL
#define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
@@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
-pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
+static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
@@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
}
-static inline pte_t *get_pte_virt_late(phys_addr_t pa)
+static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
{
return (pte_t *) __va(pa);
}
@@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
}
-static phys_addr_t alloc_pte_late(uintptr_t va)
+static phys_addr_t __init alloc_pte_late(uintptr_t va)
{
unsigned long vaddr;
@@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
#ifndef __PAGETABLE_PMD_FOLDED
-pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
-pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
-pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
-pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
+static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
+static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
+static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
+static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
#ifdef CONFIG_XIP_KERNEL
#define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
@@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
}
-static pmd_t *get_pmd_virt_late(phys_addr_t pa)
+static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
{
return (pmd_t *) __va(pa);
}
@@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
}
-static phys_addr_t alloc_pmd_late(uintptr_t va)
+static phys_addr_t __init alloc_pmd_late(uintptr_t va)
{
unsigned long vaddr;
@@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
#error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
#endif
-uintptr_t load_pa, load_sz;
+static uintptr_t load_pa __initdata;
+static uintptr_t load_sz __initdata;
#ifdef CONFIG_XIP_KERNEL
#define load_pa (*((uintptr_t *)XIP_FIXUP(&load_pa)))
#define load_sz (*((uintptr_t *)XIP_FIXUP(&load_sz)))
#endif
#ifdef CONFIG_XIP_KERNEL
-uintptr_t xiprom, xiprom_sz;
+static uintptr_t xiprom __inidata;
+static uintptr_t xiprom_sz __initdata;
#define xiprom_sz (*((uintptr_t *)XIP_FIXUP(&xiprom_sz)))
#define xiprom (*((uintptr_t *)XIP_FIXUP(&xiprom)))
@@ -646,7 +648,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
}
#if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
-void protect_kernel_linear_mapping_text_rodata(void)
+void __init protect_kernel_linear_mapping_text_rodata(void)
{
unsigned long text_start = (unsigned long)lm_alias(_start);
unsigned long init_text_start = (unsigned long)lm_alias(__init_text_begin);
@@ -858,7 +860,7 @@ static void __init reserve_crashkernel(void)
* reserved once we call early_init_fdt_scan_reserved_mem()
* later on.
*/
-static int elfcore_hdr_setup(struct reserved_mem *rmem)
+static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
{
elfcorehdr_addr = rmem->base;
elfcorehdr_size = rmem->size;
--
2.31.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-05-16 13:15 [PATCH] riscv: mm: init: Consolidate vars, functions Jisheng Zhang
@ 2021-05-16 13:19 ` Jisheng Zhang
2021-05-16 19:10 ` Alex Ghiti
2021-06-01 20:21 ` Nathan Chancellor
1 sibling, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-05-16 13:19 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel
On Sun, 16 May 2021 21:15:56 +0800 Jisheng Zhang wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
>
> Consolidate the following items in init.c
>
> Staticize global vars as much as possible;
> Add __initdata mark if the global var isn't needed after init
> Add __init mark if the func isn't needed after init
> Add __ro_after_init if the global var is read only after init
Hi Alexandre,
I think kernel_virt_addr can be removed, users can directly use
the KERNEL_LINK_ADDR MACRO. Maybe I miss the reason of intrducing
it in commit 2bfc6cd81bd17e43 ("riscv: Move kernel mapping outside of
linear mapping"), could you please comment?
Thanks in advance
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> arch/riscv/include/asm/set_memory.h | 2 +-
> arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> index 086f757e8ba3..9d4d455726d4 100644
> --- a/arch/riscv/include/asm/set_memory.h
> +++ b/arch/riscv/include/asm/set_memory.h
> @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> #endif
>
> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> -void protect_kernel_linear_mapping_text_rodata(void);
> +void __init protect_kernel_linear_mapping_text_rodata(void);
> #else
> static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> #endif
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 4c4c92ce0bb8..eac2d5c27b3e 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> #endif
> };
>
> -static phys_addr_t dma32_phys_limit __ro_after_init;
> +static phys_addr_t dma32_phys_limit __initdata;
>
> static void __init zone_sizes_init(void)
> {
> @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> #endif /* CONFIG_XIP_KERNEL */
>
> #ifdef CONFIG_MMU
> -static struct pt_alloc_ops _pt_ops __ro_after_init;
> +static struct pt_alloc_ops _pt_ops __initdata;
>
> #ifdef CONFIG_XIP_KERNEL
> #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> #endif
> /* Offset between kernel mapping virtual address and kernel load address */
> #ifdef CONFIG_64BIT
> -unsigned long va_kernel_pa_offset;
> +unsigned long va_kernel_pa_offset __ro_after_init;
> EXPORT_SYMBOL(va_kernel_pa_offset);
> #endif
> #ifdef CONFIG_XIP_KERNEL
> #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> #endif
> -unsigned long va_kernel_xip_pa_offset;
> +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> #ifdef CONFIG_XIP_KERNEL
> #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
>
> pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
>
> pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
>
> @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> }
>
> -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> {
> return (pte_t *) __va(pa);
> }
> @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> }
>
> -static phys_addr_t alloc_pte_late(uintptr_t va)
> +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> {
> unsigned long vaddr;
>
> @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
>
> #ifndef __PAGETABLE_PMD_FOLDED
>
> -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>
> #ifdef CONFIG_XIP_KERNEL
> #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> }
>
> -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> {
> return (pmd_t *) __va(pa);
> }
> @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> }
>
> -static phys_addr_t alloc_pmd_late(uintptr_t va)
> +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> {
> unsigned long vaddr;
>
> @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> #endif
>
> -uintptr_t load_pa, load_sz;
> +static uintptr_t load_pa __initdata;
> +static uintptr_t load_sz __initdata;
> #ifdef CONFIG_XIP_KERNEL
> #define load_pa (*((uintptr_t *)XIP_FIXUP(&load_pa)))
> #define load_sz (*((uintptr_t *)XIP_FIXUP(&load_sz)))
> #endif
>
> #ifdef CONFIG_XIP_KERNEL
> -uintptr_t xiprom, xiprom_sz;
> +static uintptr_t xiprom __inidata;
> +static uintptr_t xiprom_sz __initdata;
> #define xiprom_sz (*((uintptr_t *)XIP_FIXUP(&xiprom_sz)))
> #define xiprom (*((uintptr_t *)XIP_FIXUP(&xiprom)))
>
> @@ -646,7 +648,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> }
>
> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> -void protect_kernel_linear_mapping_text_rodata(void)
> +void __init protect_kernel_linear_mapping_text_rodata(void)
> {
> unsigned long text_start = (unsigned long)lm_alias(_start);
> unsigned long init_text_start = (unsigned long)lm_alias(__init_text_begin);
> @@ -858,7 +860,7 @@ static void __init reserve_crashkernel(void)
> * reserved once we call early_init_fdt_scan_reserved_mem()
> * later on.
> */
> -static int elfcore_hdr_setup(struct reserved_mem *rmem)
> +static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
> {
> elfcorehdr_addr = rmem->base;
> elfcorehdr_size = rmem->size;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-05-16 13:19 ` Jisheng Zhang
@ 2021-05-16 19:10 ` Alex Ghiti
2021-05-17 14:40 ` Jisheng Zhang
0 siblings, 1 reply; 13+ messages in thread
From: Alex Ghiti @ 2021-05-16 19:10 UTC (permalink / raw)
To: Jisheng Zhang, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv, linux-kernel
Hi Jisheng,
On 16/05/2021 15:19, Jisheng Zhang wrote:
> On Sun, 16 May 2021 21:15:56 +0800 Jisheng Zhang wrote:
>
>> From: Jisheng Zhang <jszhang@kernel.org>
>>
>> Consolidate the following items in init.c
>>
>> Staticize global vars as much as possible;
>> Add __initdata mark if the global var isn't needed after init
>> Add __init mark if the func isn't needed after init
>> Add __ro_after_init if the global var is read only after init
>
> Hi Alexandre,
>
> I think kernel_virt_addr can be removed, users can directly use
> the KERNEL_LINK_ADDR MACRO. Maybe I miss the reason of intrducing
> it in commit 2bfc6cd81bd17e43 ("riscv: Move kernel mapping outside of
> linear mapping"), could you please comment?
>
> Thanks in advance
kernel_virt_addr will be used when I push my relocatable series since
then, the kernel virtual address will be different from
KERNEL_LINK_ADDR. I agree this was not needed in 2bfc6cd81bd17e43
("riscv: Move kernel mapping outside of linear mapping"), I'm a bit late
in pushing the relocatable series.
>
>>
>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>> ---
>> arch/riscv/include/asm/set_memory.h | 2 +-
>> arch/riscv/mm/init.c | 36 +++++++++++++++--------------
>> 2 files changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
>> index 086f757e8ba3..9d4d455726d4 100644
>> --- a/arch/riscv/include/asm/set_memory.h
>> +++ b/arch/riscv/include/asm/set_memory.h
>> @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
>> #endif
>>
>> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
>> -void protect_kernel_linear_mapping_text_rodata(void);
>> +void __init protect_kernel_linear_mapping_text_rodata(void);
>> #else
>> static inline void protect_kernel_linear_mapping_text_rodata(void) {}
>> #endif
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index 4c4c92ce0bb8..eac2d5c27b3e 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -53,7 +53,7 @@ struct pt_alloc_ops {
>> #endif
>> };
>>
>> -static phys_addr_t dma32_phys_limit __ro_after_init;
>> +static phys_addr_t dma32_phys_limit __initdata;
>>
>> static void __init zone_sizes_init(void)
>> {
>> @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
>> #endif /* CONFIG_XIP_KERNEL */
>>
>> #ifdef CONFIG_MMU
>> -static struct pt_alloc_ops _pt_ops __ro_after_init;
>> +static struct pt_alloc_ops _pt_ops __initdata;
>>
>> #ifdef CONFIG_XIP_KERNEL
>> #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
>> @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
>> #endif
>> /* Offset between kernel mapping virtual address and kernel load address */
>> #ifdef CONFIG_64BIT
>> -unsigned long va_kernel_pa_offset;
>> +unsigned long va_kernel_pa_offset __ro_after_init;
>> EXPORT_SYMBOL(va_kernel_pa_offset);
>> #endif
>> #ifdef CONFIG_XIP_KERNEL
>> #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
>> #endif
>> -unsigned long va_kernel_xip_pa_offset;
>> +unsigned long va_kernel_xip_pa_offset __ro_after_init;
>> EXPORT_SYMBOL(va_kernel_xip_pa_offset);
>> #ifdef CONFIG_XIP_KERNEL
>> #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
>> @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
>>
>> pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
>> pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
>> -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
>> +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
>>
>> pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
>>
>> @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
>> return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
>> }
>>
>> -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
>> +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
>> {
>> return (pte_t *) __va(pa);
>> }
>> @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
>> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
>> }
>>
>> -static phys_addr_t alloc_pte_late(uintptr_t va)
>> +static phys_addr_t __init alloc_pte_late(uintptr_t va)
>> {
>> unsigned long vaddr;
>>
>> @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
>>
>> #ifndef __PAGETABLE_PMD_FOLDED
>>
>> -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>> -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>> +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>> +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>>
>> #ifdef CONFIG_XIP_KERNEL
>> #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
>> @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
>> return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
>> }
>>
>> -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
>> +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
>> {
>> return (pmd_t *) __va(pa);
>> }
>> @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
>> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
>> }
>>
>> -static phys_addr_t alloc_pmd_late(uintptr_t va)
>> +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
>> {
>> unsigned long vaddr;
>>
>> @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
>> #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
>> #endif
>>
>> -uintptr_t load_pa, load_sz;
>> +static uintptr_t load_pa __initdata;
>> +static uintptr_t load_sz __initdata;
>> #ifdef CONFIG_XIP_KERNEL
>> #define load_pa (*((uintptr_t *)XIP_FIXUP(&load_pa)))
>> #define load_sz (*((uintptr_t *)XIP_FIXUP(&load_sz)))
>> #endif
>>
>> #ifdef CONFIG_XIP_KERNEL
>> -uintptr_t xiprom, xiprom_sz;
>> +static uintptr_t xiprom __inidata;
>> +static uintptr_t xiprom_sz __initdata;
>> #define xiprom_sz (*((uintptr_t *)XIP_FIXUP(&xiprom_sz)))
>> #define xiprom (*((uintptr_t *)XIP_FIXUP(&xiprom)))
>>
>> @@ -646,7 +648,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>> }
>>
>> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
>> -void protect_kernel_linear_mapping_text_rodata(void)
>> +void __init protect_kernel_linear_mapping_text_rodata(void)
>> {
>> unsigned long text_start = (unsigned long)lm_alias(_start);
>> unsigned long init_text_start = (unsigned long)lm_alias(__init_text_begin);
>> @@ -858,7 +860,7 @@ static void __init reserve_crashkernel(void)
>> * reserved once we call early_init_fdt_scan_reserved_mem()
>> * later on.
>> */
>> -static int elfcore_hdr_setup(struct reserved_mem *rmem)
>> +static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
>> {
>> elfcorehdr_addr = rmem->base;
>> elfcorehdr_size = rmem->size;
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-05-16 19:10 ` Alex Ghiti
@ 2021-05-17 14:40 ` Jisheng Zhang
2021-05-29 21:09 ` Palmer Dabbelt
0 siblings, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-05-17 14:40 UTC (permalink / raw)
To: Alex Ghiti
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv, linux-kernel
On Sun, 16 May 2021 21:10:48 +0200
Alex Ghiti <alex@ghiti.fr> wrote:
> Hi Jisheng,
>
> On 16/05/2021 15:19, Jisheng Zhang wrote:
> > On Sun, 16 May 2021 21:15:56 +0800 Jisheng Zhang wrote:
> >
> >> From: Jisheng Zhang <jszhang@kernel.org>
> >>
> >> Consolidate the following items in init.c
> >>
> >> Staticize global vars as much as possible;
> >> Add __initdata mark if the global var isn't needed after init
> >> Add __init mark if the func isn't needed after init
> >> Add __ro_after_init if the global var is read only after init
> >
> > Hi Alexandre,
> >
> > I think kernel_virt_addr can be removed, users can directly use
> > the KERNEL_LINK_ADDR MACRO. Maybe I miss the reason of intrducing
> > it in commit 2bfc6cd81bd17e43 ("riscv: Move kernel mapping outside of
> > linear mapping"), could you please comment?
> >
> > Thanks in advance
>
> kernel_virt_addr will be used when I push my relocatable series since
> then, the kernel virtual address will be different from
> KERNEL_LINK_ADDR. I agree this was not needed in 2bfc6cd81bd17e43
> ("riscv: Move kernel mapping outside of linear mapping"), I'm a bit late
> in pushing the relocatable series.
>
Thanks for your information. I think kernel_virt_addr could be __ro_after_init
But I will keep it unchanged until your relocatable series.
thanks
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-05-17 14:40 ` Jisheng Zhang
@ 2021-05-29 21:09 ` Palmer Dabbelt
0 siblings, 0 replies; 13+ messages in thread
From: Palmer Dabbelt @ 2021-05-29 21:09 UTC (permalink / raw)
To: jszhang3; +Cc: alex, Paul Walmsley, aou, linux-riscv, linux-kernel
On Mon, 17 May 2021 07:40:01 PDT (-0700), jszhang3@mail.ustc.edu.cn wrote:
> On Sun, 16 May 2021 21:10:48 +0200
> Alex Ghiti <alex@ghiti.fr> wrote:
>
>> Hi Jisheng,
>>
>> On 16/05/2021 15:19, Jisheng Zhang wrote:
>> > On Sun, 16 May 2021 21:15:56 +0800 Jisheng Zhang wrote:
>> >
>> >> From: Jisheng Zhang <jszhang@kernel.org>
>> >>
>> >> Consolidate the following items in init.c
>> >>
>> >> Staticize global vars as much as possible;
>> >> Add __initdata mark if the global var isn't needed after init
>> >> Add __init mark if the func isn't needed after init
>> >> Add __ro_after_init if the global var is read only after init
>> >
>> > Hi Alexandre,
>> >
>> > I think kernel_virt_addr can be removed, users can directly use
>> > the KERNEL_LINK_ADDR MACRO. Maybe I miss the reason of intrducing
>> > it in commit 2bfc6cd81bd17e43 ("riscv: Move kernel mapping outside of
>> > linear mapping"), could you please comment?
>> >
>> > Thanks in advance
>>
>> kernel_virt_addr will be used when I push my relocatable series since
>> then, the kernel virtual address will be different from
>> KERNEL_LINK_ADDR. I agree this was not needed in 2bfc6cd81bd17e43
>> ("riscv: Move kernel mapping outside of linear mapping"), I'm a bit late
>> in pushing the relocatable series.
>>
>
> Thanks for your information. I think kernel_virt_addr could be __ro_after_init
> But I will keep it unchanged until your relocatable series.
Thanks, this is on for-next.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-05-16 13:15 [PATCH] riscv: mm: init: Consolidate vars, functions Jisheng Zhang
2021-05-16 13:19 ` Jisheng Zhang
@ 2021-06-01 20:21 ` Nathan Chancellor
2021-06-02 15:12 ` Jisheng Zhang
1 sibling, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2021-06-01 20:21 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel
Hi Jisheng,
On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
>
> Consolidate the following items in init.c
>
> Staticize global vars as much as possible;
> Add __initdata mark if the global var isn't needed after init
> Add __init mark if the func isn't needed after init
> Add __ro_after_init if the global var is read only after init
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> arch/riscv/include/asm/set_memory.h | 2 +-
> arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> index 086f757e8ba3..9d4d455726d4 100644
> --- a/arch/riscv/include/asm/set_memory.h
> +++ b/arch/riscv/include/asm/set_memory.h
> @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> #endif
>
> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> -void protect_kernel_linear_mapping_text_rodata(void);
> +void __init protect_kernel_linear_mapping_text_rodata(void);
> #else
> static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> #endif
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 4c4c92ce0bb8..eac2d5c27b3e 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> #endif
> };
>
> -static phys_addr_t dma32_phys_limit __ro_after_init;
> +static phys_addr_t dma32_phys_limit __initdata;
>
> static void __init zone_sizes_init(void)
> {
> @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> #endif /* CONFIG_XIP_KERNEL */
>
> #ifdef CONFIG_MMU
> -static struct pt_alloc_ops _pt_ops __ro_after_init;
> +static struct pt_alloc_ops _pt_ops __initdata;
>
> #ifdef CONFIG_XIP_KERNEL
> #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> #endif
> /* Offset between kernel mapping virtual address and kernel load address */
> #ifdef CONFIG_64BIT
> -unsigned long va_kernel_pa_offset;
> +unsigned long va_kernel_pa_offset __ro_after_init;
> EXPORT_SYMBOL(va_kernel_pa_offset);
> #endif
> #ifdef CONFIG_XIP_KERNEL
> #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> #endif
> -unsigned long va_kernel_xip_pa_offset;
> +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> #ifdef CONFIG_XIP_KERNEL
> #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
>
> pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
>
> pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
>
> @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> }
>
> -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> {
> return (pte_t *) __va(pa);
> }
> @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> }
>
> -static phys_addr_t alloc_pte_late(uintptr_t va)
> +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> {
> unsigned long vaddr;
>
> @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
>
> #ifndef __PAGETABLE_PMD_FOLDED
>
> -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>
> #ifdef CONFIG_XIP_KERNEL
> #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> }
>
> -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> {
> return (pmd_t *) __va(pa);
> }
> @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> }
>
> -static phys_addr_t alloc_pmd_late(uintptr_t va)
> +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> {
> unsigned long vaddr;
>
> @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> #endif
>
> -uintptr_t load_pa, load_sz;
> +static uintptr_t load_pa __initdata;
Making load_pa static causing clang built kernels to no longer boot,
hanging after just a few lines of output in the console:
https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
I am not sure why that would make a difference nor why GCC is okay with
it. If it is a clang bug, it appears to be there for a while, given that
it reproduces back to clang-11.
If I can provide any more information or help debug, let me know!
Cheers,
Nathan
> +static uintptr_t load_sz __initdata;
> #ifdef CONFIG_XIP_KERNEL
> #define load_pa (*((uintptr_t *)XIP_FIXUP(&load_pa)))
> #define load_sz (*((uintptr_t *)XIP_FIXUP(&load_sz)))
> #endif
>
> #ifdef CONFIG_XIP_KERNEL
> -uintptr_t xiprom, xiprom_sz;
> +static uintptr_t xiprom __inidata;
> +static uintptr_t xiprom_sz __initdata;
> #define xiprom_sz (*((uintptr_t *)XIP_FIXUP(&xiprom_sz)))
> #define xiprom (*((uintptr_t *)XIP_FIXUP(&xiprom)))
>
> @@ -646,7 +648,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> }
>
> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> -void protect_kernel_linear_mapping_text_rodata(void)
> +void __init protect_kernel_linear_mapping_text_rodata(void)
> {
> unsigned long text_start = (unsigned long)lm_alias(_start);
> unsigned long init_text_start = (unsigned long)lm_alias(__init_text_begin);
> @@ -858,7 +860,7 @@ static void __init reserve_crashkernel(void)
> * reserved once we call early_init_fdt_scan_reserved_mem()
> * later on.
> */
> -static int elfcore_hdr_setup(struct reserved_mem *rmem)
> +static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
> {
> elfcorehdr_addr = rmem->base;
> elfcorehdr_size = rmem->size;
> --
> 2.31.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-01 20:21 ` Nathan Chancellor
@ 2021-06-02 15:12 ` Jisheng Zhang
2021-06-02 15:58 ` Jisheng Zhang
0 siblings, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-06-02 15:12 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel
On Tue, 1 Jun 2021 13:21:05 -0700
Nathan Chancellor <nathan@kernel.org> wrote:
> Hi Jisheng,
Hi Nathan,
>
> On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> > From: Jisheng Zhang <jszhang@kernel.org>
> >
> > Consolidate the following items in init.c
> >
> > Staticize global vars as much as possible;
> > Add __initdata mark if the global var isn't needed after init
> > Add __init mark if the func isn't needed after init
> > Add __ro_after_init if the global var is read only after init
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> > arch/riscv/include/asm/set_memory.h | 2 +-
> > arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> > 2 files changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> > index 086f757e8ba3..9d4d455726d4 100644
> > --- a/arch/riscv/include/asm/set_memory.h
> > +++ b/arch/riscv/include/asm/set_memory.h
> > @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> > #endif
> >
> > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > -void protect_kernel_linear_mapping_text_rodata(void);
> > +void __init protect_kernel_linear_mapping_text_rodata(void);
> > #else
> > static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> > #endif
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 4c4c92ce0bb8..eac2d5c27b3e 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> > #endif
> > };
> >
> > -static phys_addr_t dma32_phys_limit __ro_after_init;
> > +static phys_addr_t dma32_phys_limit __initdata;
> >
> > static void __init zone_sizes_init(void)
> > {
> > @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> > #endif /* CONFIG_XIP_KERNEL */
> >
> > #ifdef CONFIG_MMU
> > -static struct pt_alloc_ops _pt_ops __ro_after_init;
> > +static struct pt_alloc_ops _pt_ops __initdata;
> >
> > #ifdef CONFIG_XIP_KERNEL
> > #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> > @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> > #endif
> > /* Offset between kernel mapping virtual address and kernel load address */
> > #ifdef CONFIG_64BIT
> > -unsigned long va_kernel_pa_offset;
> > +unsigned long va_kernel_pa_offset __ro_after_init;
> > EXPORT_SYMBOL(va_kernel_pa_offset);
> > #endif
> > #ifdef CONFIG_XIP_KERNEL
> > #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> > #endif
> > -unsigned long va_kernel_xip_pa_offset;
> > +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> > EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> > #ifdef CONFIG_XIP_KERNEL
> > #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> > @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
> >
> > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> >
> > pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> >
> > @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> > return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> > }
> >
> > -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> > +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> > {
> > return (pte_t *) __va(pa);
> > }
> > @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > }
> >
> > -static phys_addr_t alloc_pte_late(uintptr_t va)
> > +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> > {
> > unsigned long vaddr;
> >
> > @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
> >
> > #ifndef __PAGETABLE_PMD_FOLDED
> >
> > -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> >
> > #ifdef CONFIG_XIP_KERNEL
> > #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> > @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> > return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> > }
> >
> > -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> > +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> > {
> > return (pmd_t *) __va(pa);
> > }
> > @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > }
> >
> > -static phys_addr_t alloc_pmd_late(uintptr_t va)
> > +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> > {
> > unsigned long vaddr;
> >
> > @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> > #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> > #endif
> >
> > -uintptr_t load_pa, load_sz;
> > +static uintptr_t load_pa __initdata;
>
> Making load_pa static causing clang built kernels to no longer boot,
> hanging after just a few lines of output in the console:
>
> https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
>
> I am not sure why that would make a difference nor why GCC is okay with
> it. If it is a clang bug, it appears to be there for a while, given that
> it reproduces back to clang-11.
I can reproduce the issue. Here are my findindings:
* gcc + binutils can't reproduce it
* clang + llvm-utils + ias can reproduce it
* clang + binutils can reproduce it
All below tests are done with clang + binutils.
Then I applied below modification:
-static uintptr_t load_pa __initdata;
+uintptr_t load_pa __initdata;
I got below panic log:
[ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
[ 0.016432] Oops [#1]
[ 0.016679] Modules linked in:
[ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
[ 0.017711] Hardware name: riscv-virtio,qemu (DT)
[ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
[ 0.018582] ra : restore_all+0xe/0x66
[ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
[ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
[ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
[ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
[ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
[ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
[ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
[ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
[ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
[ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
[ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
[ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
[ 0.025367] Call Trace:
[ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
[ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
[ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
[ 0.027827] Oops [#2]
[ 0.028013] Modules linked in:
[ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
[ 0.028839] Hardware name: riscv-virtio,qemu (DT)
[ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
[ 0.029505] ra : restore_all+0xe/0x66
[ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
[ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
[ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
[ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
[ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
[ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
[ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
[ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
[ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
[ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
[ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
[ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
[ 0.035492] Call Trace:
[ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
[ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
[ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
[ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
[ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
should exist there for a while. I never tried clang with riscv, did you remember
which last commit or version clang works, I may try to bisect.
Thanks
>
> If I can provide any more information or help debug, let me know!
>
> Cheers,
> Nathan
>
> > +static uintptr_t load_sz __initdata;
> > #ifdef CONFIG_XIP_KERNEL
> > #define load_pa (*((uintptr_t *)XIP_FIXUP(&load_pa)))
> > #define load_sz (*((uintptr_t *)XIP_FIXUP(&load_sz)))
> > #endif
> >
> > #ifdef CONFIG_XIP_KERNEL
> > -uintptr_t xiprom, xiprom_sz;
> > +static uintptr_t xiprom __inidata;
> > +static uintptr_t xiprom_sz __initdata;
> > #define xiprom_sz (*((uintptr_t *)XIP_FIXUP(&xiprom_sz)))
> > #define xiprom (*((uintptr_t *)XIP_FIXUP(&xiprom)))
> >
> > @@ -646,7 +648,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > }
> >
> > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > -void protect_kernel_linear_mapping_text_rodata(void)
> > +void __init protect_kernel_linear_mapping_text_rodata(void)
> > {
> > unsigned long text_start = (unsigned long)lm_alias(_start);
> > unsigned long init_text_start = (unsigned long)lm_alias(__init_text_begin);
> > @@ -858,7 +860,7 @@ static void __init reserve_crashkernel(void)
> > * reserved once we call early_init_fdt_scan_reserved_mem()
> > * later on.
> > */
> > -static int elfcore_hdr_setup(struct reserved_mem *rmem)
> > +static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
> > {
> > elfcorehdr_addr = rmem->base;
> > elfcorehdr_size = rmem->size;
> > --
> > 2.31.0
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-02 15:12 ` Jisheng Zhang
@ 2021-06-02 15:58 ` Jisheng Zhang
2021-06-02 19:54 ` Nathan Chancellor
0 siblings, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-06-02 15:58 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel
On Wed, 2 Jun 2021 23:12:26 +0800
Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> On Tue, 1 Jun 2021 13:21:05 -0700
> Nathan Chancellor <nathan@kernel.org> wrote:
>
> > Hi Jisheng,
>
> Hi Nathan,
>
> >
> > On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> > > From: Jisheng Zhang <jszhang@kernel.org>
> > >
> > > Consolidate the following items in init.c
> > >
> > > Staticize global vars as much as possible;
> > > Add __initdata mark if the global var isn't needed after init
> > > Add __init mark if the func isn't needed after init
> > > Add __ro_after_init if the global var is read only after init
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > > arch/riscv/include/asm/set_memory.h | 2 +-
> > > arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> > > 2 files changed, 20 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> > > index 086f757e8ba3..9d4d455726d4 100644
> > > --- a/arch/riscv/include/asm/set_memory.h
> > > +++ b/arch/riscv/include/asm/set_memory.h
> > > @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> > > #endif
> > >
> > > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > > -void protect_kernel_linear_mapping_text_rodata(void);
> > > +void __init protect_kernel_linear_mapping_text_rodata(void);
> > > #else
> > > static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> > > #endif
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index 4c4c92ce0bb8..eac2d5c27b3e 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> > > #endif
> > > };
> > >
> > > -static phys_addr_t dma32_phys_limit __ro_after_init;
> > > +static phys_addr_t dma32_phys_limit __initdata;
> > >
> > > static void __init zone_sizes_init(void)
> > > {
> > > @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> > > #endif /* CONFIG_XIP_KERNEL */
> > >
> > > #ifdef CONFIG_MMU
> > > -static struct pt_alloc_ops _pt_ops __ro_after_init;
> > > +static struct pt_alloc_ops _pt_ops __initdata;
> > >
> > > #ifdef CONFIG_XIP_KERNEL
> > > #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> > > @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> > > #endif
> > > /* Offset between kernel mapping virtual address and kernel load address */
> > > #ifdef CONFIG_64BIT
> > > -unsigned long va_kernel_pa_offset;
> > > +unsigned long va_kernel_pa_offset __ro_after_init;
> > > EXPORT_SYMBOL(va_kernel_pa_offset);
> > > #endif
> > > #ifdef CONFIG_XIP_KERNEL
> > > #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> > > #endif
> > > -unsigned long va_kernel_xip_pa_offset;
> > > +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> > > EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> > > #ifdef CONFIG_XIP_KERNEL
> > > #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> > > @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
> > >
> > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > >
> > > pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> > >
> > > @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> > > return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> > > }
> > >
> > > -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> > > +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> > > {
> > > return (pte_t *) __va(pa);
> > > }
> > > @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > }
> > >
> > > -static phys_addr_t alloc_pte_late(uintptr_t va)
> > > +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> > > {
> > > unsigned long vaddr;
> > >
> > > @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
> > >
> > > #ifndef __PAGETABLE_PMD_FOLDED
> > >
> > > -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > >
> > > #ifdef CONFIG_XIP_KERNEL
> > > #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> > > @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> > > return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> > > }
> > >
> > > -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> > > +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> > > {
> > > return (pmd_t *) __va(pa);
> > > }
> > > @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > }
> > >
> > > -static phys_addr_t alloc_pmd_late(uintptr_t va)
> > > +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> > > {
> > > unsigned long vaddr;
> > >
> > > @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> > > #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> > > #endif
> > >
> > > -uintptr_t load_pa, load_sz;
> > > +static uintptr_t load_pa __initdata;
> >
> > Making load_pa static causing clang built kernels to no longer boot,
> > hanging after just a few lines of output in the console:
> >
> > https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
> >
> > I am not sure why that would make a difference nor why GCC is okay with
> > it. If it is a clang bug, it appears to be there for a while, given that
> > it reproduces back to clang-11.
>
> I can reproduce the issue. Here are my findindings:
>
> * gcc + binutils can't reproduce it
> * clang + llvm-utils + ias can reproduce it
> * clang + binutils can reproduce it
>
>
> All below tests are done with clang + binutils.
>
> Then I applied below modification:
>
> -static uintptr_t load_pa __initdata;
> +uintptr_t load_pa __initdata;
>
> I got below panic log:
> [ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
> [ 0.016432] Oops [#1]
> [ 0.016679] Modules linked in:
> [ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
> [ 0.017711] Hardware name: riscv-virtio,qemu (DT)
> [ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
> [ 0.018582] ra : restore_all+0xe/0x66
> [ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
> [ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> [ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
> [ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
> [ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> [ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> [ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> [ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> [ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> [ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> [ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> [ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
> [ 0.025367] Call Trace:
> [ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> [ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
> [ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
> [ 0.027827] Oops [#2]
> [ 0.028013] Modules linked in:
> [ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
> [ 0.028839] Hardware name: riscv-virtio,qemu (DT)
> [ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
> [ 0.029505] ra : restore_all+0xe/0x66
> [ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
> [ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> [ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
> [ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
> [ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> [ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> [ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> [ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> [ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> [ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> [ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> [ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
> [ 0.035492] Call Trace:
> [ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> [ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
> [ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
> [ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
> [ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
>
> Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
> should exist there for a while. I never tried clang with riscv, did you remember
> which last commit or version clang works, I may try to bisect.
>
More findings:
*The above panic issue can also be seen from 5.12-rc2. If disable FTRACE, then
the panic disappears, kernel can boot
*so I retested riscv next tree w/ FTRACE disabled, kernel can boot w/ below
modification:
-static uintptr_t load_pa __initdata;
+uintptr_t load_pa __initdata;
This is a weird issue. Any clue is appreciated.
Thanks
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-02 15:58 ` Jisheng Zhang
@ 2021-06-02 19:54 ` Nathan Chancellor
2021-06-03 0:27 ` Jisheng Zhang
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2021-06-02 19:54 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel, clang-built-linux
On Wed, Jun 02, 2021 at 11:58:51PM +0800, Jisheng Zhang wrote:
> On Wed, 2 Jun 2021 23:12:26 +0800
> Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>
> > On Tue, 1 Jun 2021 13:21:05 -0700
> > Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > > Hi Jisheng,
> >
> > Hi Nathan,
> >
> > >
> > > On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> > > > From: Jisheng Zhang <jszhang@kernel.org>
> > > >
> > > > Consolidate the following items in init.c
> > > >
> > > > Staticize global vars as much as possible;
> > > > Add __initdata mark if the global var isn't needed after init
> > > > Add __init mark if the func isn't needed after init
> > > > Add __ro_after_init if the global var is read only after init
> > > >
> > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > ---
> > > > arch/riscv/include/asm/set_memory.h | 2 +-
> > > > arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> > > > 2 files changed, 20 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> > > > index 086f757e8ba3..9d4d455726d4 100644
> > > > --- a/arch/riscv/include/asm/set_memory.h
> > > > +++ b/arch/riscv/include/asm/set_memory.h
> > > > @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> > > > #endif
> > > >
> > > > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > > > -void protect_kernel_linear_mapping_text_rodata(void);
> > > > +void __init protect_kernel_linear_mapping_text_rodata(void);
> > > > #else
> > > > static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> > > > #endif
> > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > index 4c4c92ce0bb8..eac2d5c27b3e 100644
> > > > --- a/arch/riscv/mm/init.c
> > > > +++ b/arch/riscv/mm/init.c
> > > > @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> > > > #endif
> > > > };
> > > >
> > > > -static phys_addr_t dma32_phys_limit __ro_after_init;
> > > > +static phys_addr_t dma32_phys_limit __initdata;
> > > >
> > > > static void __init zone_sizes_init(void)
> > > > {
> > > > @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> > > > #endif /* CONFIG_XIP_KERNEL */
> > > >
> > > > #ifdef CONFIG_MMU
> > > > -static struct pt_alloc_ops _pt_ops __ro_after_init;
> > > > +static struct pt_alloc_ops _pt_ops __initdata;
> > > >
> > > > #ifdef CONFIG_XIP_KERNEL
> > > > #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> > > > @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> > > > #endif
> > > > /* Offset between kernel mapping virtual address and kernel load address */
> > > > #ifdef CONFIG_64BIT
> > > > -unsigned long va_kernel_pa_offset;
> > > > +unsigned long va_kernel_pa_offset __ro_after_init;
> > > > EXPORT_SYMBOL(va_kernel_pa_offset);
> > > > #endif
> > > > #ifdef CONFIG_XIP_KERNEL
> > > > #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> > > > #endif
> > > > -unsigned long va_kernel_xip_pa_offset;
> > > > +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> > > > EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> > > > #ifdef CONFIG_XIP_KERNEL
> > > > #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> > > > @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
> > > >
> > > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > >
> > > > pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> > > >
> > > > @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> > > > return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> > > > }
> > > >
> > > > -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> > > > +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> > > > {
> > > > return (pte_t *) __va(pa);
> > > > }
> > > > @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > }
> > > >
> > > > -static phys_addr_t alloc_pte_late(uintptr_t va)
> > > > +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> > > > {
> > > > unsigned long vaddr;
> > > >
> > > > @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
> > > >
> > > > #ifndef __PAGETABLE_PMD_FOLDED
> > > >
> > > > -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > >
> > > > #ifdef CONFIG_XIP_KERNEL
> > > > #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> > > > @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> > > > return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> > > > }
> > > >
> > > > -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> > > > +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> > > > {
> > > > return (pmd_t *) __va(pa);
> > > > }
> > > > @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > }
> > > >
> > > > -static phys_addr_t alloc_pmd_late(uintptr_t va)
> > > > +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> > > > {
> > > > unsigned long vaddr;
> > > >
> > > > @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> > > > #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> > > > #endif
> > > >
> > > > -uintptr_t load_pa, load_sz;
> > > > +static uintptr_t load_pa __initdata;
> > >
> > > Making load_pa static causing clang built kernels to no longer boot,
> > > hanging after just a few lines of output in the console:
> > >
> > > https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
> > >
> > > I am not sure why that would make a difference nor why GCC is okay with
> > > it. If it is a clang bug, it appears to be there for a while, given that
> > > it reproduces back to clang-11.
> >
> > I can reproduce the issue. Here are my findindings:
> >
> > * gcc + binutils can't reproduce it
> > * clang + llvm-utils + ias can reproduce it
> > * clang + binutils can reproduce it
Yes, this seems like something strictly related to clang so that is what
I have been testing with.
> > All below tests are done with clang + binutils.
> >
> > Then I applied below modification:
> >
> > -static uintptr_t load_pa __initdata;
> > +uintptr_t load_pa __initdata;
> >
> > I got below panic log:
> > [ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
> > [ 0.016432] Oops [#1]
> > [ 0.016679] Modules linked in:
> > [ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
> > [ 0.017711] Hardware name: riscv-virtio,qemu (DT)
> > [ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
> > [ 0.018582] ra : restore_all+0xe/0x66
> > [ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
> > [ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > [ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
> > [ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
> > [ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > [ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > [ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > [ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > [ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > [ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > [ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > [ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
> > [ 0.025367] Call Trace:
> > [ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > [ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
> > [ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
> > [ 0.027827] Oops [#2]
> > [ 0.028013] Modules linked in:
> > [ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
> > [ 0.028839] Hardware name: riscv-virtio,qemu (DT)
> > [ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
> > [ 0.029505] ra : restore_all+0xe/0x66
> > [ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
> > [ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > [ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
> > [ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
> > [ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > [ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > [ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > [ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > [ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > [ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > [ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > [ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
> > [ 0.035492] Call Trace:
> > [ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > [ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
> > [ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
> > [ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
> > [ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
> >
> > Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
> > should exist there for a while. I never tried clang with riscv, did you remember
> > which last commit or version clang works, I may try to bisect.
V> >
>
> More findings:
>
> *The above panic issue can also be seen from 5.12-rc2. If disable FTRACE, then
> the panic disappears, kernel can boot
>
> *so I retested riscv next tree w/ FTRACE disabled, kernel can boot w/ below
> modification:
Yeah, I do not enable CONFIG_FTRACE because it is not enabled in
ARCH=riscv defconfig by default.
> -static uintptr_t load_pa __initdata;
> +uintptr_t load_pa __initdata;
>
> This is a weird issue. Any clue is appreciated.
Unfortunately, this is outside of my realm of expertise, as I am
unfamiliar with RISC-V at this level. Maybe Palmer has some ideas. I
would think that changing this variable to static would be fine given
that the symbol is only used in this translation unit but clearly not. I
have attempted to debug this in gdb but that does not really get me
anywhere: I cannot break on start_kernel() for whatever reason and the
kernel never gets to my breakpoint in setup_vm().
I did decide to through a BUG() around arch/riscv/mm/init.c to see
exactly which statement causes everything to hang. I landed on:
csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
in setup_vm_final(). No idea how that is relevant to this.
Some people in the ClangBuiltLinux bi-weekly meeting today pointed out
that since load_pa is assigned to the linker defined symbol _start,
there could be some optimization that goes awry here. I have keyed that
list in now. For full context:
https://lore.kernel.org/r/YLaWseLdg5JYElVx@Ryzen-9-3900X.localdomain/
Cheers,
Nathan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-02 19:54 ` Nathan Chancellor
@ 2021-06-03 0:27 ` Jisheng Zhang
2021-06-03 11:54 ` Jisheng Zhang
0 siblings, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-06-03 0:27 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel, clang-built-linux
On Wed, 2 Jun 2021 12:54:40 -0700
Nathan Chancellor <nathan@kernel.org> wrote:
> On Wed, Jun 02, 2021 at 11:58:51PM +0800, Jisheng Zhang wrote:
> > On Wed, 2 Jun 2021 23:12:26 +0800
> > Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> >
> > > On Tue, 1 Jun 2021 13:21:05 -0700
> > > Nathan Chancellor <nathan@kernel.org> wrote:
> > >
> > > > Hi Jisheng,
> > >
> > > Hi Nathan,
> > >
> > > >
> > > > On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> > > > > From: Jisheng Zhang <jszhang@kernel.org>
> > > > >
> > > > > Consolidate the following items in init.c
> > > > >
> > > > > Staticize global vars as much as possible;
> > > > > Add __initdata mark if the global var isn't needed after init
> > > > > Add __init mark if the func isn't needed after init
> > > > > Add __ro_after_init if the global var is read only after init
> > > > >
> > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > > ---
> > > > > arch/riscv/include/asm/set_memory.h | 2 +-
> > > > > arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> > > > > 2 files changed, 20 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> > > > > index 086f757e8ba3..9d4d455726d4 100644
> > > > > --- a/arch/riscv/include/asm/set_memory.h
> > > > > +++ b/arch/riscv/include/asm/set_memory.h
> > > > > @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> > > > > #endif
> > > > >
> > > > > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > > > > -void protect_kernel_linear_mapping_text_rodata(void);
> > > > > +void __init protect_kernel_linear_mapping_text_rodata(void);
> > > > > #else
> > > > > static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> > > > > #endif
> > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > index 4c4c92ce0bb8..eac2d5c27b3e 100644
> > > > > --- a/arch/riscv/mm/init.c
> > > > > +++ b/arch/riscv/mm/init.c
> > > > > @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> > > > > #endif
> > > > > };
> > > > >
> > > > > -static phys_addr_t dma32_phys_limit __ro_after_init;
> > > > > +static phys_addr_t dma32_phys_limit __initdata;
> > > > >
> > > > > static void __init zone_sizes_init(void)
> > > > > {
> > > > > @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> > > > > #endif /* CONFIG_XIP_KERNEL */
> > > > >
> > > > > #ifdef CONFIG_MMU
> > > > > -static struct pt_alloc_ops _pt_ops __ro_after_init;
> > > > > +static struct pt_alloc_ops _pt_ops __initdata;
> > > > >
> > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> > > > > @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> > > > > #endif
> > > > > /* Offset between kernel mapping virtual address and kernel load address */
> > > > > #ifdef CONFIG_64BIT
> > > > > -unsigned long va_kernel_pa_offset;
> > > > > +unsigned long va_kernel_pa_offset __ro_after_init;
> > > > > EXPORT_SYMBOL(va_kernel_pa_offset);
> > > > > #endif
> > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> > > > > #endif
> > > > > -unsigned long va_kernel_xip_pa_offset;
> > > > > +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> > > > > EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> > > > > @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
> > > > >
> > > > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > > -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > > +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > >
> > > > > pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> > > > >
> > > > > @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> > > > > return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> > > > > }
> > > > >
> > > > > -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> > > > > +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> > > > > {
> > > > > return (pte_t *) __va(pa);
> > > > > }
> > > > > @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> > > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > > }
> > > > >
> > > > > -static phys_addr_t alloc_pte_late(uintptr_t va)
> > > > > +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> > > > > {
> > > > > unsigned long vaddr;
> > > > >
> > > > > @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
> > > > >
> > > > > #ifndef __PAGETABLE_PMD_FOLDED
> > > > >
> > > > > -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > >
> > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> > > > > @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> > > > > return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> > > > > }
> > > > >
> > > > > -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> > > > > +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> > > > > {
> > > > > return (pmd_t *) __va(pa);
> > > > > }
> > > > > @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> > > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > > }
> > > > >
> > > > > -static phys_addr_t alloc_pmd_late(uintptr_t va)
> > > > > +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> > > > > {
> > > > > unsigned long vaddr;
> > > > >
> > > > > @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> > > > > #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> > > > > #endif
> > > > >
> > > > > -uintptr_t load_pa, load_sz;
> > > > > +static uintptr_t load_pa __initdata;
> > > >
> > > > Making load_pa static causing clang built kernels to no longer boot,
> > > > hanging after just a few lines of output in the console:
> > > >
> > > > https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
> > > >
> > > > I am not sure why that would make a difference nor why GCC is okay with
> > > > it. If it is a clang bug, it appears to be there for a while, given that
> > > > it reproduces back to clang-11.
> > >
> > > I can reproduce the issue. Here are my findindings:
> > >
> > > * gcc + binutils can't reproduce it
> > > * clang + llvm-utils + ias can reproduce it
> > > * clang + binutils can reproduce it
>
> Yes, this seems like something strictly related to clang so that is what
> I have been testing with.
>
> > > All below tests are done with clang + binutils.
> > >
> > > Then I applied below modification:
> > >
> > > -static uintptr_t load_pa __initdata;
> > > +uintptr_t load_pa __initdata;
> > >
> > > I got below panic log:
> > > [ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
> > > [ 0.016432] Oops [#1]
> > > [ 0.016679] Modules linked in:
> > > [ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
> > > [ 0.017711] Hardware name: riscv-virtio,qemu (DT)
> > > [ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
> > > [ 0.018582] ra : restore_all+0xe/0x66
> > > [ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
> > > [ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > > [ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
> > > [ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
> > > [ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > > [ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > > [ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > > [ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > > [ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > > [ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > > [ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > > [ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
> > > [ 0.025367] Call Trace:
> > > [ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > > [ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
> > > [ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
> > > [ 0.027827] Oops [#2]
> > > [ 0.028013] Modules linked in:
> > > [ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
> > > [ 0.028839] Hardware name: riscv-virtio,qemu (DT)
> > > [ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
> > > [ 0.029505] ra : restore_all+0xe/0x66
> > > [ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
> > > [ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > > [ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
> > > [ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
> > > [ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > > [ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > > [ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > > [ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > > [ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > > [ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > > [ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > > [ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
> > > [ 0.035492] Call Trace:
> > > [ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > > [ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
> > > [ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
> > > [ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
> > > [ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
> > >
> > > Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
> > > should exist there for a while. I never tried clang with riscv, did you remember
> > > which last commit or version clang works, I may try to bisect.
> V> >
> >
> > More findings:
> >
> > *The above panic issue can also be seen from 5.12-rc2. If disable FTRACE, then
> > the panic disappears, kernel can boot
> >
> > *so I retested riscv next tree w/ FTRACE disabled, kernel can boot w/ below
> > modification:
>
> Yeah, I do not enable CONFIG_FTRACE because it is not enabled in
> ARCH=riscv defconfig by default.
>
> > -static uintptr_t load_pa __initdata;
> > +uintptr_t load_pa __initdata;
> >
> > This is a weird issue. Any clue is appreciated.
>
> Unfortunately, this is outside of my realm of expertise, as I am
> unfamiliar with RISC-V at this level. Maybe Palmer has some ideas. I
> would think that changing this variable to static would be fine given
> that the symbol is only used in this translation unit but clearly not. I
> have attempted to debug this in gdb but that does not really get me
> anywhere: I cannot break on start_kernel() for whatever reason and the
> kernel never gets to my breakpoint in setup_vm().
>
> I did decide to through a BUG() around arch/riscv/mm/init.c to see
> exactly which statement causes everything to hang. I landed on:
>
> csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
>
> in setup_vm_final(). No idea how that is relevant to this.
>
> Some people in the ClangBuiltLinux bi-weekly meeting today pointed out
> that since load_pa is assigned to the linker defined symbol _start,
> there could be some optimization that goes awry here. I have keyed that
Thanks for the inspiration. Below patch fixes the hang issue, but I didn't
go through all necessary WRITE_ONCE convertions.
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index ae32f78207f0..fa9336a2583f 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -504,7 +504,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
va_kernel_xip_pa_offset = kernel_virt_addr - xiprom;
#else
- load_pa = (uintptr_t)(&_start);
+ WRITE_ONCE(load_pa, (uintptr_t)(&_start));
load_sz = (uintptr_t)(&_end) - load_pa;
#endif
> list in now. For full context:
>
> https://lore.kernel.org/r/YLaWseLdg5JYElVx@Ryzen-9-3900X.localdomain/
>
> Cheers,
> Nathan
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-03 0:27 ` Jisheng Zhang
@ 2021-06-03 11:54 ` Jisheng Zhang
2021-06-04 17:09 ` Jisheng Zhang
0 siblings, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-06-03 11:54 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-riscv, linux-kernel, clang-built-linux
On Thu, 3 Jun 2021 08:27:14 +0800
Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> On Wed, 2 Jun 2021 12:54:40 -0700
> Nathan Chancellor <nathan@kernel.org> wrote:
>
> > On Wed, Jun 02, 2021 at 11:58:51PM +0800, Jisheng Zhang wrote:
> > > On Wed, 2 Jun 2021 23:12:26 +0800
> > > Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> > >
> > > > On Tue, 1 Jun 2021 13:21:05 -0700
> > > > Nathan Chancellor <nathan@kernel.org> wrote:
> > > >
> > > > > Hi Jisheng,
> > > >
> > > > Hi Nathan,
> > > >
> > > > >
> > > > > On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> > > > > > From: Jisheng Zhang <jszhang@kernel.org>
> > > > > >
> > > > > > Consolidate the following items in init.c
> > > > > >
> > > > > > Staticize global vars as much as possible;
> > > > > > Add __initdata mark if the global var isn't needed after init
> > > > > > Add __init mark if the func isn't needed after init
> > > > > > Add __ro_after_init if the global var is read only after init
> > > > > >
> > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > > > ---
> > > > > > arch/riscv/include/asm/set_memory.h | 2 +-
> > > > > > arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> > > > > > 2 files changed, 20 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> > > > > > index 086f757e8ba3..9d4d455726d4 100644
> > > > > > --- a/arch/riscv/include/asm/set_memory.h
> > > > > > +++ b/arch/riscv/include/asm/set_memory.h
> > > > > > @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> > > > > > #endif
> > > > > >
> > > > > > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > > > > > -void protect_kernel_linear_mapping_text_rodata(void);
> > > > > > +void __init protect_kernel_linear_mapping_text_rodata(void);
> > > > > > #else
> > > > > > static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> > > > > > #endif
> > > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > > index 4c4c92ce0bb8..eac2d5c27b3e 100644
> > > > > > --- a/arch/riscv/mm/init.c
> > > > > > +++ b/arch/riscv/mm/init.c
> > > > > > @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> > > > > > #endif
> > > > > > };
> > > > > >
> > > > > > -static phys_addr_t dma32_phys_limit __ro_after_init;
> > > > > > +static phys_addr_t dma32_phys_limit __initdata;
> > > > > >
> > > > > > static void __init zone_sizes_init(void)
> > > > > > {
> > > > > > @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> > > > > > #endif /* CONFIG_XIP_KERNEL */
> > > > > >
> > > > > > #ifdef CONFIG_MMU
> > > > > > -static struct pt_alloc_ops _pt_ops __ro_after_init;
> > > > > > +static struct pt_alloc_ops _pt_ops __initdata;
> > > > > >
> > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> > > > > > @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> > > > > > #endif
> > > > > > /* Offset between kernel mapping virtual address and kernel load address */
> > > > > > #ifdef CONFIG_64BIT
> > > > > > -unsigned long va_kernel_pa_offset;
> > > > > > +unsigned long va_kernel_pa_offset __ro_after_init;
> > > > > > EXPORT_SYMBOL(va_kernel_pa_offset);
> > > > > > #endif
> > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> > > > > > #endif
> > > > > > -unsigned long va_kernel_xip_pa_offset;
> > > > > > +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> > > > > > EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> > > > > > @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
> > > > > >
> > > > > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > > > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > > > -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > > > +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > > >
> > > > > > pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> > > > > >
> > > > > > @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> > > > > > return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> > > > > > }
> > > > > >
> > > > > > -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> > > > > > +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> > > > > > {
> > > > > > return (pte_t *) __va(pa);
> > > > > > }
> > > > > > @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> > > > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > > > }
> > > > > >
> > > > > > -static phys_addr_t alloc_pte_late(uintptr_t va)
> > > > > > +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> > > > > > {
> > > > > > unsigned long vaddr;
> > > > > >
> > > > > > @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
> > > > > >
> > > > > > #ifndef __PAGETABLE_PMD_FOLDED
> > > > > >
> > > > > > -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > >
> > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> > > > > > @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> > > > > > return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> > > > > > }
> > > > > >
> > > > > > -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> > > > > > +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> > > > > > {
> > > > > > return (pmd_t *) __va(pa);
> > > > > > }
> > > > > > @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> > > > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > > > }
> > > > > >
> > > > > > -static phys_addr_t alloc_pmd_late(uintptr_t va)
> > > > > > +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> > > > > > {
> > > > > > unsigned long vaddr;
> > > > > >
> > > > > > @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> > > > > > #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> > > > > > #endif
> > > > > >
> > > > > > -uintptr_t load_pa, load_sz;
> > > > > > +static uintptr_t load_pa __initdata;
> > > > >
> > > > > Making load_pa static causing clang built kernels to no longer boot,
> > > > > hanging after just a few lines of output in the console:
> > > > >
> > > > > https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
> > > > >
> > > > > I am not sure why that would make a difference nor why GCC is okay with
> > > > > it. If it is a clang bug, it appears to be there for a while, given that
> > > > > it reproduces back to clang-11.
> > > >
> > > > I can reproduce the issue. Here are my findindings:
> > > >
> > > > * gcc + binutils can't reproduce it
> > > > * clang + llvm-utils + ias can reproduce it
> > > > * clang + binutils can reproduce it
> >
> > Yes, this seems like something strictly related to clang so that is what
> > I have been testing with.
> >
> > > > All below tests are done with clang + binutils.
> > > >
> > > > Then I applied below modification:
> > > >
> > > > -static uintptr_t load_pa __initdata;
> > > > +uintptr_t load_pa __initdata;
> > > >
> > > > I got below panic log:
> > > > [ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
> > > > [ 0.016432] Oops [#1]
> > > > [ 0.016679] Modules linked in:
> > > > [ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
> > > > [ 0.017711] Hardware name: riscv-virtio,qemu (DT)
> > > > [ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
> > > > [ 0.018582] ra : restore_all+0xe/0x66
> > > > [ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
> > > > [ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > > > [ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
> > > > [ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
> > > > [ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > > > [ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > > > [ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > > > [ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > > > [ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > > > [ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > > > [ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > > > [ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
> > > > [ 0.025367] Call Trace:
> > > > [ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > > > [ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
> > > > [ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
> > > > [ 0.027827] Oops [#2]
> > > > [ 0.028013] Modules linked in:
> > > > [ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
> > > > [ 0.028839] Hardware name: riscv-virtio,qemu (DT)
> > > > [ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
> > > > [ 0.029505] ra : restore_all+0xe/0x66
> > > > [ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
> > > > [ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > > > [ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
> > > > [ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
> > > > [ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > > > [ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > > > [ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > > > [ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > > > [ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > > > [ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > > > [ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > > > [ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
> > > > [ 0.035492] Call Trace:
> > > > [ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > > > [ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
> > > > [ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
> > > > [ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
> > > > [ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
> > > >
> > > > Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
> > > > should exist there for a while. I never tried clang with riscv, did you remember
> > > > which last commit or version clang works, I may try to bisect.
> > V> >
> > >
> > > More findings:
> > >
> > > *The above panic issue can also be seen from 5.12-rc2. If disable FTRACE, then
> > > the panic disappears, kernel can boot
> > >
> > > *so I retested riscv next tree w/ FTRACE disabled, kernel can boot w/ below
> > > modification:
> >
> > Yeah, I do not enable CONFIG_FTRACE because it is not enabled in
> > ARCH=riscv defconfig by default.
> >
> > > -static uintptr_t load_pa __initdata;
> > > +uintptr_t load_pa __initdata;
> > >
> > > This is a weird issue. Any clue is appreciated.
> >
> > Unfortunately, this is outside of my realm of expertise, as I am
> > unfamiliar with RISC-V at this level. Maybe Palmer has some ideas. I
> > would think that changing this variable to static would be fine given
> > that the symbol is only used in this translation unit but clearly not. I
> > have attempted to debug this in gdb but that does not really get me
> > anywhere: I cannot break on start_kernel() for whatever reason and the
> > kernel never gets to my breakpoint in setup_vm().
> >
> > I did decide to through a BUG() around arch/riscv/mm/init.c to see
> > exactly which statement causes everything to hang. I landed on:
> >
> > csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
> >
> > in setup_vm_final(). No idea how that is relevant to this.
> >
> > Some people in the ClangBuiltLinux bi-weekly meeting today pointed out
> > that since load_pa is assigned to the linker defined symbol _start,
> > there could be some optimization that goes awry here. I have keyed that
>
> Thanks for the inspiration. Below patch fixes the hang issue, but I didn't
> go through all necessary WRITE_ONCE convertions.
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index ae32f78207f0..fa9336a2583f 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -504,7 +504,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>
> va_kernel_xip_pa_offset = kernel_virt_addr - xiprom;
> #else
> - load_pa = (uintptr_t)(&_start);
> + WRITE_ONCE(load_pa, (uintptr_t)(&_start));
> load_sz = (uintptr_t)(&_end) - load_pa;
> #endif
>
I think I found the root cause, but I dunno whether this is clang bug or
we need WRITE_ONCE patch.
W/O WRITE_ONCE, the setup_vm() is compiled to
ffffffff8040472a <setup_vm>:
ffffffff8040472a: 1101 addi sp,sp,-32
ffffffff8040472c: ec06 sd ra,24(sp)
ffffffff8040472e: e822 sd s0,16(sp)
ffffffff80404730: e426 sd s1,8(sp)
ffffffff80404732: e04a sd s2,0(sp)
ffffffff80404734: 1000 addi s0,sp,32
ffffffff80404736: 892a mv s2,a0
ffffffff80404738: 001fd517 auipc a0,0x1fd
ffffffff8040473c: 8c850513 addi a0,a0,-1848 # ffffffff80601000 <load_pa>
ffffffff80404740: 4585 li a1,1
ffffffff80404742: 00b50023 sb a1,0(a0) // BUG!
ffffffff80404746: 001fd517 auipc a0,0x1fd
ffffffff8040474a: 8c250513 addi a0,a0,-1854 # ffffffff80601008 <load_sz>
ffffffff8040474e: 00b50023 sb a1,0(a0) // BUG!
ffffffff80404752: ffbfc517 auipc a0,0xffbfc
ffffffff80404756: 8ae50513 addi a0,a0,-1874 # ffffffff80000000 <_start>
ffffffff8040475a: 55fd li a1,-1
ffffffff8040475c: 02559613 slli a2,a1,0x25
ffffffff80404760: 83018593 addi a1,gp,-2000 # ffffffff80ca6428 <kernel_virt_addr>
ffffffff80404764: 618c ld a1,0(a1)
ffffffff80404766: 8e09 sub a2,a2,a0
...
It seems load_pa and load_sz are stored with 1, this is obviously not what the
code expected.
W/ WRITE_ONCE, the setup_vm() is compiled to:
ffffffff8040472a <setup_vm>:
ffffffff8040472a: 1101 addi sp,sp,-32
ffffffff8040472c: ec06 sd ra,24(sp)
ffffffff8040472e: e822 sd s0,16(sp)
ffffffff80404730: e426 sd s1,8(sp)
ffffffff80404732: e04a sd s2,0(sp)
ffffffff80404734: 1000 addi s0,sp,32
ffffffff80404736: 892a mv s2,a0
ffffffff80404738: 001fd597 auipc a1,0x1fd
ffffffff8040473c: 8c858593 addi a1,a1,-1848 # ffffffff80601000 <load_pa>
ffffffff80404740: ffbfc517 auipc a0,0xffbfc
ffffffff80404744: 8c050513 addi a0,a0,-1856 # ffffffff80000000 <_start>
ffffffff80404748: e188 sd a0,0(a1)
ffffffff8040474a: 001fd597 auipc a1,0x1fd
ffffffff8040474e: 8be58593 addi a1,a1,-1858 # ffffffff80601008 <load_sz>
ffffffff80404752: 4605 li a2,1
ffffffff80404754: 00c58023 sb a2,0(a1)
ffffffff80404758: 55fd li a1,-1
ffffffff8040475a: 02559613 slli a2,a1,0x25
ffffffff8040475e: 83018593 addi a1,gp,-2000 # ffffffff80ca6428 <kernel_virt_addr>
ffffffff80404762: 618c ld a1,0(a1)
ffffffff80404764: 8e09 sub a2,a2,a0
...
This is what the code expected.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-03 11:54 ` Jisheng Zhang
@ 2021-06-04 17:09 ` Jisheng Zhang
2021-06-06 7:23 ` Alex Ghiti
0 siblings, 1 reply; 13+ messages in thread
From: Jisheng Zhang @ 2021-06-04 17:09 UTC (permalink / raw)
To: Nathan Chancellor, Alexandre Ghiti
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, clang-built-linux
On Thu, 3 Jun 2021 19:54:18 +0800
Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> On Thu, 3 Jun 2021 08:27:14 +0800
> Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>
> > On Wed, 2 Jun 2021 12:54:40 -0700
> > Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > > On Wed, Jun 02, 2021 at 11:58:51PM +0800, Jisheng Zhang wrote:
> > > > On Wed, 2 Jun 2021 23:12:26 +0800
> > > > Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> > > >
> > > > > On Tue, 1 Jun 2021 13:21:05 -0700
> > > > > Nathan Chancellor <nathan@kernel.org> wrote:
> > > > >
> > > > > > Hi Jisheng,
> > > > >
> > > > > Hi Nathan,
> > > > >
> > > > > >
> > > > > > On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
> > > > > > > From: Jisheng Zhang <jszhang@kernel.org>
> > > > > > >
> > > > > > > Consolidate the following items in init.c
> > > > > > >
> > > > > > > Staticize global vars as much as possible;
> > > > > > > Add __initdata mark if the global var isn't needed after init
> > > > > > > Add __init mark if the func isn't needed after init
> > > > > > > Add __ro_after_init if the global var is read only after init
> > > > > > >
> > > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > > > > ---
> > > > > > > arch/riscv/include/asm/set_memory.h | 2 +-
> > > > > > > arch/riscv/mm/init.c | 36 +++++++++++++++--------------
> > > > > > > 2 files changed, 20 insertions(+), 18 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
> > > > > > > index 086f757e8ba3..9d4d455726d4 100644
> > > > > > > --- a/arch/riscv/include/asm/set_memory.h
> > > > > > > +++ b/arch/riscv/include/asm/set_memory.h
> > > > > > > @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
> > > > > > > #endif
> > > > > > >
> > > > > > > #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
> > > > > > > -void protect_kernel_linear_mapping_text_rodata(void);
> > > > > > > +void __init protect_kernel_linear_mapping_text_rodata(void);
> > > > > > > #else
> > > > > > > static inline void protect_kernel_linear_mapping_text_rodata(void) {}
> > > > > > > #endif
> > > > > > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > > > > > index 4c4c92ce0bb8..eac2d5c27b3e 100644
> > > > > > > --- a/arch/riscv/mm/init.c
> > > > > > > +++ b/arch/riscv/mm/init.c
> > > > > > > @@ -53,7 +53,7 @@ struct pt_alloc_ops {
> > > > > > > #endif
> > > > > > > };
> > > > > > >
> > > > > > > -static phys_addr_t dma32_phys_limit __ro_after_init;
> > > > > > > +static phys_addr_t dma32_phys_limit __initdata;
> > > > > > >
> > > > > > > static void __init zone_sizes_init(void)
> > > > > > > {
> > > > > > > @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
> > > > > > > #endif /* CONFIG_XIP_KERNEL */
> > > > > > >
> > > > > > > #ifdef CONFIG_MMU
> > > > > > > -static struct pt_alloc_ops _pt_ops __ro_after_init;
> > > > > > > +static struct pt_alloc_ops _pt_ops __initdata;
> > > > > > >
> > > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > > #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
> > > > > > > @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
> > > > > > > #endif
> > > > > > > /* Offset between kernel mapping virtual address and kernel load address */
> > > > > > > #ifdef CONFIG_64BIT
> > > > > > > -unsigned long va_kernel_pa_offset;
> > > > > > > +unsigned long va_kernel_pa_offset __ro_after_init;
> > > > > > > EXPORT_SYMBOL(va_kernel_pa_offset);
> > > > > > > #endif
> > > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > > #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
> > > > > > > #endif
> > > > > > > -unsigned long va_kernel_xip_pa_offset;
> > > > > > > +unsigned long va_kernel_xip_pa_offset __ro_after_init;
> > > > > > > EXPORT_SYMBOL(va_kernel_xip_pa_offset);
> > > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > > #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
> > > > > > > @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
> > > > > > >
> > > > > > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > > > > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > > > > > -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > > > > +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
> > > > > > >
> > > > > > > pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> > > > > > >
> > > > > > > @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
> > > > > > > return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
> > > > > > > }
> > > > > > >
> > > > > > > -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
> > > > > > > +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
> > > > > > > {
> > > > > > > return (pte_t *) __va(pa);
> > > > > > > }
> > > > > > > @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
> > > > > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > > > > }
> > > > > > >
> > > > > > > -static phys_addr_t alloc_pte_late(uintptr_t va)
> > > > > > > +static phys_addr_t __init alloc_pte_late(uintptr_t va)
> > > > > > > {
> > > > > > > unsigned long vaddr;
> > > > > > >
> > > > > > > @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
> > > > > > >
> > > > > > > #ifndef __PAGETABLE_PMD_FOLDED
> > > > > > >
> > > > > > > -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > > -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > > -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > > -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > > +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > > +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > > > > > > +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > > +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > > > > > >
> > > > > > > #ifdef CONFIG_XIP_KERNEL
> > > > > > > #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
> > > > > > > @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
> > > > > > > return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
> > > > > > > }
> > > > > > >
> > > > > > > -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
> > > > > > > +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
> > > > > > > {
> > > > > > > return (pmd_t *) __va(pa);
> > > > > > > }
> > > > > > > @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
> > > > > > > return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
> > > > > > > }
> > > > > > >
> > > > > > > -static phys_addr_t alloc_pmd_late(uintptr_t va)
> > > > > > > +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
> > > > > > > {
> > > > > > > unsigned long vaddr;
> > > > > > >
> > > > > > > @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
> > > > > > > #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
> > > > > > > #endif
> > > > > > >
> > > > > > > -uintptr_t load_pa, load_sz;
> > > > > > > +static uintptr_t load_pa __initdata;
> > > > > >
> > > > > > Making load_pa static causing clang built kernels to no longer boot,
> > > > > > hanging after just a few lines of output in the console:
> > > > > >
> > > > > > https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
> > > > > >
> > > > > > I am not sure why that would make a difference nor why GCC is okay with
> > > > > > it. If it is a clang bug, it appears to be there for a while, given that
> > > > > > it reproduces back to clang-11.
> > > > >
> > > > > I can reproduce the issue. Here are my findindings:
> > > > >
> > > > > * gcc + binutils can't reproduce it
> > > > > * clang + llvm-utils + ias can reproduce it
> > > > > * clang + binutils can reproduce it
> > >
> > > Yes, this seems like something strictly related to clang so that is what
> > > I have been testing with.
> > >
> > > > > All below tests are done with clang + binutils.
> > > > >
> > > > > Then I applied below modification:
> > > > >
> > > > > -static uintptr_t load_pa __initdata;
> > > > > +uintptr_t load_pa __initdata;
> > > > >
> > > > > I got below panic log:
> > > > > [ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
> > > > > [ 0.016432] Oops [#1]
> > > > > [ 0.016679] Modules linked in:
> > > > > [ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
> > > > > [ 0.017711] Hardware name: riscv-virtio,qemu (DT)
> > > > > [ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
> > > > > [ 0.018582] ra : restore_all+0xe/0x66
> > > > > [ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
> > > > > [ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > > > > [ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
> > > > > [ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
> > > > > [ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > > > > [ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > > > > [ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > > > > [ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > > > > [ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > > > > [ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > > > > [ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > > > > [ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
> > > > > [ 0.025367] Call Trace:
> > > > > [ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > > > > [ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
> > > > > [ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
> > > > > [ 0.027827] Oops [#2]
> > > > > [ 0.028013] Modules linked in:
> > > > > [ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
> > > > > [ 0.028839] Hardware name: riscv-virtio,qemu (DT)
> > > > > [ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
> > > > > [ 0.029505] ra : restore_all+0xe/0x66
> > > > > [ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
> > > > > [ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
> > > > > [ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
> > > > > [ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
> > > > > [ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
> > > > > [ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
> > > > > [ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
> > > > > [ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
> > > > > [ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
> > > > > [ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
> > > > > [ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
> > > > > [ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
> > > > > [ 0.035492] Call Trace:
> > > > > [ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
> > > > > [ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
> > > > > [ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
> > > > > [ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
> > > > > [ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
> > > > >
> > > > > Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
> > > > > should exist there for a while. I never tried clang with riscv, did you remember
> > > > > which last commit or version clang works, I may try to bisect.
> > > V> >
> > > >
> > > > More findings:
> > > >
> > > > *The above panic issue can also be seen from 5.12-rc2. If disable FTRACE, then
> > > > the panic disappears, kernel can boot
> > > >
> > > > *so I retested riscv next tree w/ FTRACE disabled, kernel can boot w/ below
> > > > modification:
> > >
> > > Yeah, I do not enable CONFIG_FTRACE because it is not enabled in
> > > ARCH=riscv defconfig by default.
> > >
> > > > -static uintptr_t load_pa __initdata;
> > > > +uintptr_t load_pa __initdata;
> > > >
> > > > This is a weird issue. Any clue is appreciated.
> > >
> > > Unfortunately, this is outside of my realm of expertise, as I am
> > > unfamiliar with RISC-V at this level. Maybe Palmer has some ideas. I
> > > would think that changing this variable to static would be fine given
> > > that the symbol is only used in this translation unit but clearly not. I
> > > have attempted to debug this in gdb but that does not really get me
> > > anywhere: I cannot break on start_kernel() for whatever reason and the
> > > kernel never gets to my breakpoint in setup_vm().
> > >
> > > I did decide to through a BUG() around arch/riscv/mm/init.c to see
> > > exactly which statement causes everything to hang. I landed on:
> > >
> > > csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
> > >
> > > in setup_vm_final(). No idea how that is relevant to this.
> > >
> > > Some people in the ClangBuiltLinux bi-weekly meeting today pointed out
> > > that since load_pa is assigned to the linker defined symbol _start,
> > > there could be some optimization that goes awry here. I have keyed that
> >
> > Thanks for the inspiration. Below patch fixes the hang issue, but I didn't
> > go through all necessary WRITE_ONCE convertions.
> >
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index ae32f78207f0..fa9336a2583f 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -504,7 +504,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> >
> > va_kernel_xip_pa_offset = kernel_virt_addr - xiprom;
> > #else
> > - load_pa = (uintptr_t)(&_start);
> > + WRITE_ONCE(load_pa, (uintptr_t)(&_start));
> > load_sz = (uintptr_t)(&_end) - load_pa;
> > #endif
> >
>
> I think I found the root cause, but I dunno whether this is clang bug or
> we need WRITE_ONCE patch.
> W/O WRITE_ONCE, the setup_vm() is compiled to
>
> ffffffff8040472a <setup_vm>:
> ffffffff8040472a: 1101 addi sp,sp,-32
> ffffffff8040472c: ec06 sd ra,24(sp)
> ffffffff8040472e: e822 sd s0,16(sp)
> ffffffff80404730: e426 sd s1,8(sp)
> ffffffff80404732: e04a sd s2,0(sp)
> ffffffff80404734: 1000 addi s0,sp,32
> ffffffff80404736: 892a mv s2,a0
> ffffffff80404738: 001fd517 auipc a0,0x1fd
> ffffffff8040473c: 8c850513 addi a0,a0,-1848 # ffffffff80601000 <load_pa>
> ffffffff80404740: 4585 li a1,1
> ffffffff80404742: 00b50023 sb a1,0(a0) // BUG!
> ffffffff80404746: 001fd517 auipc a0,0x1fd
> ffffffff8040474a: 8c250513 addi a0,a0,-1854 # ffffffff80601008 <load_sz>
> ffffffff8040474e: 00b50023 sb a1,0(a0) // BUG!
> ffffffff80404752: ffbfc517 auipc a0,0xffbfc
> ffffffff80404756: 8ae50513 addi a0,a0,-1874 # ffffffff80000000 <_start>
> ffffffff8040475a: 55fd li a1,-1
> ffffffff8040475c: 02559613 slli a2,a1,0x25
> ffffffff80404760: 83018593 addi a1,gp,-2000 # ffffffff80ca6428 <kernel_virt_addr>
> ffffffff80404764: 618c ld a1,0(a1)
> ffffffff80404766: 8e09 sub a2,a2,a0
> ...
>
> It seems load_pa and load_sz are stored with 1, this is obviously not what the
> code expected.
>
>
> W/ WRITE_ONCE, the setup_vm() is compiled to:
>
> ffffffff8040472a <setup_vm>:
> ffffffff8040472a: 1101 addi sp,sp,-32
> ffffffff8040472c: ec06 sd ra,24(sp)
> ffffffff8040472e: e822 sd s0,16(sp)
> ffffffff80404730: e426 sd s1,8(sp)
> ffffffff80404732: e04a sd s2,0(sp)
> ffffffff80404734: 1000 addi s0,sp,32
> ffffffff80404736: 892a mv s2,a0
> ffffffff80404738: 001fd597 auipc a1,0x1fd
> ffffffff8040473c: 8c858593 addi a1,a1,-1848 # ffffffff80601000 <load_pa>
> ffffffff80404740: ffbfc517 auipc a0,0xffbfc
> ffffffff80404744: 8c050513 addi a0,a0,-1856 # ffffffff80000000 <_start>
> ffffffff80404748: e188 sd a0,0(a1)
> ffffffff8040474a: 001fd597 auipc a1,0x1fd
> ffffffff8040474e: 8be58593 addi a1,a1,-1858 # ffffffff80601008 <load_sz>
> ffffffff80404752: 4605 li a2,1
> ffffffff80404754: 00c58023 sb a2,0(a1)
> ffffffff80404758: 55fd li a1,-1
> ffffffff8040475a: 02559613 slli a2,a1,0x25
> ffffffff8040475e: 83018593 addi a1,gp,-2000 # ffffffff80ca6428 <kernel_virt_addr>
> ffffffff80404762: 618c ld a1,0(a1)
> ffffffff80404764: 8e09 sub a2,a2,a0
> ...
>
> This is what the code expected.
This issue can also be solved by avoiding global vars: load_pa and load_sz.
Hi Alexandre,
IMHO, the load_pa and load_sz can be removed, I have one patch to remove
them, it works. I'm not sure whether will the two vars be used in your
future patches?
Thanks in advance,
Jisheng
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] riscv: mm: init: Consolidate vars, functions
2021-06-04 17:09 ` Jisheng Zhang
@ 2021-06-06 7:23 ` Alex Ghiti
0 siblings, 0 replies; 13+ messages in thread
From: Alex Ghiti @ 2021-06-06 7:23 UTC (permalink / raw)
To: Jisheng Zhang, Nathan Chancellor
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, clang-built-linux
Hi Jisheng,
Le 4/06/2021 à 19:09, Jisheng Zhang a écrit :
> On Thu, 3 Jun 2021 19:54:18 +0800
> Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>
>> On Thu, 3 Jun 2021 08:27:14 +0800
>> Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>>
>>> On Wed, 2 Jun 2021 12:54:40 -0700
>>> Nathan Chancellor <nathan@kernel.org> wrote:
>>>
>>>> On Wed, Jun 02, 2021 at 11:58:51PM +0800, Jisheng Zhang wrote:
>>>>> On Wed, 2 Jun 2021 23:12:26 +0800
>>>>> Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>>>>>
>>>>>> On Tue, 1 Jun 2021 13:21:05 -0700
>>>>>> Nathan Chancellor <nathan@kernel.org> wrote:
>>>>>>
>>>>>>> Hi Jisheng,
>>>>>>
>>>>>> Hi Nathan,
>>>>>>
>>>>>>>
>>>>>>> On Sun, May 16, 2021 at 09:15:56PM +0800, Jisheng Zhang wrote:
>>>>>>>> From: Jisheng Zhang <jszhang@kernel.org>
>>>>>>>>
>>>>>>>> Consolidate the following items in init.c
>>>>>>>>
>>>>>>>> Staticize global vars as much as possible;
>>>>>>>> Add __initdata mark if the global var isn't needed after init
>>>>>>>> Add __init mark if the func isn't needed after init
>>>>>>>> Add __ro_after_init if the global var is read only after init
>>>>>>>>
>>>>>>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>>>>>>> ---
>>>>>>>> arch/riscv/include/asm/set_memory.h | 2 +-
>>>>>>>> arch/riscv/mm/init.c | 36 +++++++++++++++--------------
>>>>>>>> 2 files changed, 20 insertions(+), 18 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
>>>>>>>> index 086f757e8ba3..9d4d455726d4 100644
>>>>>>>> --- a/arch/riscv/include/asm/set_memory.h
>>>>>>>> +++ b/arch/riscv/include/asm/set_memory.h
>>>>>>>> @@ -27,7 +27,7 @@ static inline int set_memory_rw_nx(unsigned long addr, int numpages) { return 0;
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> #if defined(CONFIG_64BIT) && defined(CONFIG_STRICT_KERNEL_RWX)
>>>>>>>> -void protect_kernel_linear_mapping_text_rodata(void);
>>>>>>>> +void __init protect_kernel_linear_mapping_text_rodata(void);
>>>>>>>> #else
>>>>>>>> static inline void protect_kernel_linear_mapping_text_rodata(void) {}
>>>>>>>> #endif
>>>>>>>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>>>>>>>> index 4c4c92ce0bb8..eac2d5c27b3e 100644
>>>>>>>> --- a/arch/riscv/mm/init.c
>>>>>>>> +++ b/arch/riscv/mm/init.c
>>>>>>>> @@ -53,7 +53,7 @@ struct pt_alloc_ops {
>>>>>>>> #endif
>>>>>>>> };
>>>>>>>>
>>>>>>>> -static phys_addr_t dma32_phys_limit __ro_after_init;
>>>>>>>> +static phys_addr_t dma32_phys_limit __initdata;
>>>>>>>>
>>>>>>>> static void __init zone_sizes_init(void)
>>>>>>>> {
>>>>>>>> @@ -184,7 +184,7 @@ extern char _sdata[], _edata[];
>>>>>>>> #endif /* CONFIG_XIP_KERNEL */
>>>>>>>>
>>>>>>>> #ifdef CONFIG_MMU
>>>>>>>> -static struct pt_alloc_ops _pt_ops __ro_after_init;
>>>>>>>> +static struct pt_alloc_ops _pt_ops __initdata;
>>>>>>>>
>>>>>>>> #ifdef CONFIG_XIP_KERNEL
>>>>>>>> #define pt_ops (*(struct pt_alloc_ops *)XIP_FIXUP(&_pt_ops))
>>>>>>>> @@ -200,13 +200,13 @@ EXPORT_SYMBOL(va_pa_offset);
>>>>>>>> #endif
>>>>>>>> /* Offset between kernel mapping virtual address and kernel load address */
>>>>>>>> #ifdef CONFIG_64BIT
>>>>>>>> -unsigned long va_kernel_pa_offset;
>>>>>>>> +unsigned long va_kernel_pa_offset __ro_after_init;
>>>>>>>> EXPORT_SYMBOL(va_kernel_pa_offset);
>>>>>>>> #endif
>>>>>>>> #ifdef CONFIG_XIP_KERNEL
>>>>>>>> #define va_kernel_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_pa_offset)))
>>>>>>>> #endif
>>>>>>>> -unsigned long va_kernel_xip_pa_offset;
>>>>>>>> +unsigned long va_kernel_xip_pa_offset __ro_after_init;
>>>>>>>> EXPORT_SYMBOL(va_kernel_xip_pa_offset);
>>>>>>>> #ifdef CONFIG_XIP_KERNEL
>>>>>>>> #define va_kernel_xip_pa_offset (*((unsigned long *)XIP_FIXUP(&va_kernel_xip_pa_offset)))
>>>>>>>> @@ -216,7 +216,7 @@ EXPORT_SYMBOL(pfn_base);
>>>>>>>>
>>>>>>>> pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
>>>>>>>> pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
>>>>>>>> -pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
>>>>>>>> +static pte_t fixmap_pte[PTRS_PER_PTE] __page_aligned_bss;
>>>>>>>>
>>>>>>>> pgd_t early_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
>>>>>>>>
>>>>>>>> @@ -253,7 +253,7 @@ static inline pte_t *__init get_pte_virt_fixmap(phys_addr_t pa)
>>>>>>>> return (pte_t *)set_fixmap_offset(FIX_PTE, pa);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static inline pte_t *get_pte_virt_late(phys_addr_t pa)
>>>>>>>> +static inline pte_t *__init get_pte_virt_late(phys_addr_t pa)
>>>>>>>> {
>>>>>>>> return (pte_t *) __va(pa);
>>>>>>>> }
>>>>>>>> @@ -272,7 +272,7 @@ static inline phys_addr_t __init alloc_pte_fixmap(uintptr_t va)
>>>>>>>> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static phys_addr_t alloc_pte_late(uintptr_t va)
>>>>>>>> +static phys_addr_t __init alloc_pte_late(uintptr_t va)
>>>>>>>> {
>>>>>>>> unsigned long vaddr;
>>>>>>>>
>>>>>>>> @@ -296,10 +296,10 @@ static void __init create_pte_mapping(pte_t *ptep,
>>>>>>>>
>>>>>>>> #ifndef __PAGETABLE_PMD_FOLDED
>>>>>>>>
>>>>>>>> -pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
>>>>>>>> -pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
>>>>>>>> -pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>>>>>>>> -pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>>>>>>>> +static pmd_t trampoline_pmd[PTRS_PER_PMD] __page_aligned_bss;
>>>>>>>> +static pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
>>>>>>>> +static pmd_t early_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>>>>>>>> +static pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
>>>>>>>>
>>>>>>>> #ifdef CONFIG_XIP_KERNEL
>>>>>>>> #define trampoline_pmd ((pmd_t *)XIP_FIXUP(trampoline_pmd))
>>>>>>>> @@ -319,7 +319,7 @@ static pmd_t *__init get_pmd_virt_fixmap(phys_addr_t pa)
>>>>>>>> return (pmd_t *)set_fixmap_offset(FIX_PMD, pa);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static pmd_t *get_pmd_virt_late(phys_addr_t pa)
>>>>>>>> +static pmd_t *__init get_pmd_virt_late(phys_addr_t pa)
>>>>>>>> {
>>>>>>>> return (pmd_t *) __va(pa);
>>>>>>>> }
>>>>>>>> @@ -336,7 +336,7 @@ static phys_addr_t __init alloc_pmd_fixmap(uintptr_t va)
>>>>>>>> return memblock_phys_alloc(PAGE_SIZE, PAGE_SIZE);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static phys_addr_t alloc_pmd_late(uintptr_t va)
>>>>>>>> +static phys_addr_t __init alloc_pmd_late(uintptr_t va)
>>>>>>>> {
>>>>>>>> unsigned long vaddr;
>>>>>>>>
>>>>>>>> @@ -454,14 +454,16 @@ asmlinkage void __init __copy_data(void)
>>>>>>>> #error "setup_vm() is called from head.S before relocate so it should not use absolute addressing."
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> -uintptr_t load_pa, load_sz;
>>>>>>>> +static uintptr_t load_pa __initdata;
>>>>>>>
>>>>>>> Making load_pa static causing clang built kernels to no longer boot,
>>>>>>> hanging after just a few lines of output in the console:
>>>>>>>
>>>>>>> https://github.com/ClangBuiltLinux/continuous-integration2/runs/2717606254?check_suite_focus=true
>>>>>>>
>>>>>>> I am not sure why that would make a difference nor why GCC is okay with
>>>>>>> it. If it is a clang bug, it appears to be there for a while, given that
>>>>>>> it reproduces back to clang-11.
>>>>>>
>>>>>> I can reproduce the issue. Here are my findindings:
>>>>>>
>>>>>> * gcc + binutils can't reproduce it
>>>>>> * clang + llvm-utils + ias can reproduce it
>>>>>> * clang + binutils can reproduce it
>>>>
>>>> Yes, this seems like something strictly related to clang so that is what
>>>> I have been testing with.
>>>>
>>>>>> All below tests are done with clang + binutils.
>>>>>>
>>>>>> Then I applied below modification:
>>>>>>
>>>>>> -static uintptr_t load_pa __initdata;
>>>>>> +uintptr_t load_pa __initdata;
>>>>>>
>>>>>> I got below panic log:
>>>>>> [ 0.015418] Unable to handle kernel paging request at virtual address fffffffffffffff9
>>>>>> [ 0.016432] Oops [#1]
>>>>>> [ 0.016679] Modules linked in:
>>>>>> [ 0.017103] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc1+ #5
>>>>>> [ 0.017711] Hardware name: riscv-virtio,qemu (DT)
>>>>>> [ 0.018201] epc : trace_hardirqs_on+0x60/0xb2
>>>>>> [ 0.018582] ra : restore_all+0xe/0x66
>>>>>> [ 0.018879] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03dd0
>>>>>> [ 0.019376] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
>>>>>> [ 0.019870] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03e00
>>>>>> [ 0.020346] s1 : 0000000000000001 a0 : 0000000000000001 a1 : 0000000000000001
>>>>>> [ 0.020800] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
>>>>>> [ 0.021243] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
>>>>>> [ 0.021717] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
>>>>>> [ 0.022179] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
>>>>>> [ 0.022674] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
>>>>>> [ 0.023176] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
>>>>>> [ 0.023693] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
>>>>>> [ 0.024153] status: 0000000000000100 badaddr: fffffffffffffff9 cause: 000000000000000d
>>>>>> [ 0.025367] Call Trace:
>>>>>> [ 0.025749] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
>>>>>> [ 0.026402] [<ffffffff800027b8>] restore_all+0xe/0x66
>>>>>> [ 0.027261] Unable to handle kernel paging request at virtual address fffffffffffffffa
>>>>>> [ 0.027827] Oops [#2]
>>>>>> [ 0.028013] Modules linked in:
>>>>>> [ 0.028321] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D 5.13.0-rc1+ #5
>>>>>> [ 0.028839] Hardware name: riscv-virtio,qemu (DT)
>>>>>> [ 0.029166] epc : trace_hardirqs_on+0x60/0xb2
>>>>>> [ 0.029505] ra : restore_all+0xe/0x66
>>>>>> [ 0.029785] epc : ffffffff800cb09a ra : ffffffff800027b8 sp : ffffffff80c03a80
>>>>>> [ 0.030266] gp : ffffffff80d001c8 tp : ffffffff80c0c180 t0 : 0000000000000020
>>>>>> [ 0.030748] t1 : ffffffff80006e40 t2 : ffffffff800d2e0a s0 : ffffffff80c03ab0
>>>>>> [ 0.031227] s1 : 0000000000000001 a0 : 0000000000000002 a1 : 0000000000000002
>>>>>> [ 0.031717] a2 : 0000000000000001 a3 : 0000000000000000 a4 : 0000000000000000
>>>>>> [ 0.032199] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000054494d45
>>>>>> [ 0.032680] s2 : ffffffff800027b8 s3 : ffffffff80d35968 s4 : ffffffff8061e1d8
>>>>>> [ 0.033160] s5 : ffffffff80c0c180 s6 : ffffffe000e34b50 s7 : 00000000800130f0
>>>>>> [ 0.033642] s8 : 000000000000007f s9 : 0000000080012010 s10: 0000000000000000
>>>>>> [ 0.034123] s11: 0000000000000000 t3 : ffffffff80d00108 t4 : ffffffff80006e40
>>>>>> [ 0.034601] t5 : ffffffff80006e40 t6 : ffffffff800d2e0a
>>>>>> [ 0.034965] status: 0000000000000100 badaddr: fffffffffffffffa cause: 000000000000000d
>>>>>> [ 0.035492] Call Trace:
>>>>>> [ 0.035682] [<ffffffff800cb09a>] trace_hardirqs_on+0x60/0xb2
>>>>>> [ 0.036077] [<ffffffff800027b8>] restore_all+0xe/0x66
>>>>>> [ 0.036545] ---[ end trace 7f4fbff09d927668 ]---
>>>>>> [ 0.037188] Kernel panic - not syncing: Attempted to kill the idle task!
>>>>>> [ 0.038107] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
>>>>>>
>>>>>> Then I checked 5.13-rc1, above panic log can be reproduced too. So the issue
>>>>>> should exist there for a while. I never tried clang with riscv, did you remember
>>>>>> which last commit or version clang works, I may try to bisect.
>>>> V> >
>>>>>
>>>>> More findings:
>>>>>
>>>>> *The above panic issue can also be seen from 5.12-rc2. If disable FTRACE, then
>>>>> the panic disappears, kernel can boot
>>>>>
>>>>> *so I retested riscv next tree w/ FTRACE disabled, kernel can boot w/ below
>>>>> modification:
>>>>
>>>> Yeah, I do not enable CONFIG_FTRACE because it is not enabled in
>>>> ARCH=riscv defconfig by default.
>>>>
>>>>> -static uintptr_t load_pa __initdata;
>>>>> +uintptr_t load_pa __initdata;
>>>>>
>>>>> This is a weird issue. Any clue is appreciated.
>>>>
>>>> Unfortunately, this is outside of my realm of expertise, as I am
>>>> unfamiliar with RISC-V at this level. Maybe Palmer has some ideas. I
>>>> would think that changing this variable to static would be fine given
>>>> that the symbol is only used in this translation unit but clearly not. I
>>>> have attempted to debug this in gdb but that does not really get me
>>>> anywhere: I cannot break on start_kernel() for whatever reason and the
>>>> kernel never gets to my breakpoint in setup_vm().
>>>>
>>>> I did decide to through a BUG() around arch/riscv/mm/init.c to see
>>>> exactly which statement causes everything to hang. I landed on:
>>>>
>>>> csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
>>>>
>>>> in setup_vm_final(). No idea how that is relevant to this.
>>>>
>>>> Some people in the ClangBuiltLinux bi-weekly meeting today pointed out
>>>> that since load_pa is assigned to the linker defined symbol _start,
>>>> there could be some optimization that goes awry here. I have keyed that
>>>
>>> Thanks for the inspiration. Below patch fixes the hang issue, but I didn't
>>> go through all necessary WRITE_ONCE convertions.
>>>
>>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>>> index ae32f78207f0..fa9336a2583f 100644
>>> --- a/arch/riscv/mm/init.c
>>> +++ b/arch/riscv/mm/init.c
>>> @@ -504,7 +504,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>>>
>>> va_kernel_xip_pa_offset = kernel_virt_addr - xiprom;
>>> #else
>>> - load_pa = (uintptr_t)(&_start);
>>> + WRITE_ONCE(load_pa, (uintptr_t)(&_start));
>>> load_sz = (uintptr_t)(&_end) - load_pa;
>>> #endif
>>>
>>
>> I think I found the root cause, but I dunno whether this is clang bug or
>> we need WRITE_ONCE patch.
>> W/O WRITE_ONCE, the setup_vm() is compiled to
>>
>> ffffffff8040472a <setup_vm>:
>> ffffffff8040472a: 1101 addi sp,sp,-32
>> ffffffff8040472c: ec06 sd ra,24(sp)
>> ffffffff8040472e: e822 sd s0,16(sp)
>> ffffffff80404730: e426 sd s1,8(sp)
>> ffffffff80404732: e04a sd s2,0(sp)
>> ffffffff80404734: 1000 addi s0,sp,32
>> ffffffff80404736: 892a mv s2,a0
>> ffffffff80404738: 001fd517 auipc a0,0x1fd
>> ffffffff8040473c: 8c850513 addi a0,a0,-1848 # ffffffff80601000 <load_pa>
>> ffffffff80404740: 4585 li a1,1
>> ffffffff80404742: 00b50023 sb a1,0(a0) // BUG!
>> ffffffff80404746: 001fd517 auipc a0,0x1fd
>> ffffffff8040474a: 8c250513 addi a0,a0,-1854 # ffffffff80601008 <load_sz>
>> ffffffff8040474e: 00b50023 sb a1,0(a0) // BUG!
>> ffffffff80404752: ffbfc517 auipc a0,0xffbfc
>> ffffffff80404756: 8ae50513 addi a0,a0,-1874 # ffffffff80000000 <_start>
>> ffffffff8040475a: 55fd li a1,-1
>> ffffffff8040475c: 02559613 slli a2,a1,0x25
>> ffffffff80404760: 83018593 addi a1,gp,-2000 # ffffffff80ca6428 <kernel_virt_addr>
>> ffffffff80404764: 618c ld a1,0(a1)
>> ffffffff80404766: 8e09 sub a2,a2,a0
>> ...
>>
>> It seems load_pa and load_sz are stored with 1, this is obviously not what the
>> code expected.
>>
>>
>> W/ WRITE_ONCE, the setup_vm() is compiled to:
>>
>> ffffffff8040472a <setup_vm>:
>> ffffffff8040472a: 1101 addi sp,sp,-32
>> ffffffff8040472c: ec06 sd ra,24(sp)
>> ffffffff8040472e: e822 sd s0,16(sp)
>> ffffffff80404730: e426 sd s1,8(sp)
>> ffffffff80404732: e04a sd s2,0(sp)
>> ffffffff80404734: 1000 addi s0,sp,32
>> ffffffff80404736: 892a mv s2,a0
>> ffffffff80404738: 001fd597 auipc a1,0x1fd
>> ffffffff8040473c: 8c858593 addi a1,a1,-1848 # ffffffff80601000 <load_pa>
>> ffffffff80404740: ffbfc517 auipc a0,0xffbfc
>> ffffffff80404744: 8c050513 addi a0,a0,-1856 # ffffffff80000000 <_start>
>> ffffffff80404748: e188 sd a0,0(a1)
>> ffffffff8040474a: 001fd597 auipc a1,0x1fd
>> ffffffff8040474e: 8be58593 addi a1,a1,-1858 # ffffffff80601008 <load_sz>
>> ffffffff80404752: 4605 li a2,1
>> ffffffff80404754: 00c58023 sb a2,0(a1)
>> ffffffff80404758: 55fd li a1,-1
>> ffffffff8040475a: 02559613 slli a2,a1,0x25
>> ffffffff8040475e: 83018593 addi a1,gp,-2000 # ffffffff80ca6428 <kernel_virt_addr>
>> ffffffff80404762: 618c ld a1,0(a1)
>> ffffffff80404764G 8e09 sub a2,a2,a0
>> ...
>>
>> This is what the code expected.
>
> This issue can also be solved by avoiding global vars: load_pa and load_sz.
>
> Hi Alexandre,
>
> IMHO, the load_pa and load_sz can be removed, I have one patch to remove
> them, it works. I'm not sure whether will the two vars be used in your
> future patches?
>
More or less, I want to move all the address conversion macros to inline
functions as suggested by Christoph, and there I will need load_sz and
load_pa. But I'm not sure it is really necessary.
Anyway, that would "solve" this issue, but I think we all agree the real
problem should be solved at clang level.
> Thanks in advance,
> Jisheng
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-06-06 7:24 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-16 13:15 [PATCH] riscv: mm: init: Consolidate vars, functions Jisheng Zhang
2021-05-16 13:19 ` Jisheng Zhang
2021-05-16 19:10 ` Alex Ghiti
2021-05-17 14:40 ` Jisheng Zhang
2021-05-29 21:09 ` Palmer Dabbelt
2021-06-01 20:21 ` Nathan Chancellor
2021-06-02 15:12 ` Jisheng Zhang
2021-06-02 15:58 ` Jisheng Zhang
2021-06-02 19:54 ` Nathan Chancellor
2021-06-03 0:27 ` Jisheng Zhang
2021-06-03 11:54 ` Jisheng Zhang
2021-06-04 17:09 ` Jisheng Zhang
2021-06-06 7:23 ` Alex Ghiti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).