| Message ID | 20230320065324.1045276-1-vincent.chen@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: mm: execute local TLB flush after populating vmemmap | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 1 and now 1 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 2275 this patch: 2275 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 17624 this patch: 17624 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Hi Vincent, On 3/20/23 07:53, Vincent Chen wrote: > The vmemmap_populate() creates VA to PA mapping for the VMEMMAP area, where > all "strcut page" are located once CONFIG_SPARSEMEM_VMEMMAP is defined. > These "struct page" are later initialized in the zone_sizes_init() > function. However, during this process, no sfence.vma instruction is > executed for this VMEMMAP area. This omission may cause the core to fail to > perform page table work because some data related to the address > translation is invisible to the core. To solve this issue, here let > zone_sizes_init() call local_flush_tlb_kernel_range() to execute a > sfence.vma instruction for the VMEMMAP area, ensuring that all data related > to the address translation is visible to the core. > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > arch/riscv/include/asm/tlbflush.h | 7 +++++++ > arch/riscv/mm/init.c | 3 +++ > 2 files changed, 10 insertions(+) > > diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h > index 801019381dea..b7696974e3c6 100644 > --- a/arch/riscv/include/asm/tlbflush.h > +++ b/arch/riscv/include/asm/tlbflush.h > @@ -59,4 +59,11 @@ static inline void flush_tlb_kernel_range(unsigned long start, > flush_tlb_all(); > } > > +/* Flush a range of kernel pages without broadcasting */ > +static inline void local_flush_tlb_kernel_range(unsigned long start, > + unsigned long end) > +{ > + local_flush_tlb_all(); > +} > + > #endif /* _ASM_RISCV_TLBFLUSH_H */ > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > index 2c4a64e97aec..62eea656e341 100644 > --- a/arch/riscv/mm/init.c > +++ b/arch/riscv/mm/init.c > @@ -72,6 +72,9 @@ static void __init zone_sizes_init(void) > #endif > max_zone_pfns[ZONE_NORMAL] = max_low_pfn; > > +#ifdef CONFIG_SPARSEMEM_VMEMMAP > + local_flush_tlb_kernel_range(VMEMMAP_START, VMEMMAP_END); > +#endif > free_area_init(max_zone_pfns); > } > Just one nit: I would have added the flush to the vmemmap_populate function instead, so that we don't flush uselessly when !SPARSE_VMEMMAP and we know exactly why we flush. Otherwise, you can add: Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks, Alex
Hi Vincent, On 4/13/23 02:48, Vincent Chen wrote: > > > On Wed, Mar 22, 2023 at 12:11 AM Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Vincent, > > On 3/20/23 07:53, Vincent Chen wrote: > > The vmemmap_populate() creates VA to PA mapping for the VMEMMAP > area, where > > all "strcut page" are located once CONFIG_SPARSEMEM_VMEMMAP is > defined. > > These "struct page" are later initialized in the zone_sizes_init() > > function. However, during this process, no sfence.vma instruction is > > executed for this VMEMMAP area. This omission may cause the core > to fail to > > perform page table work because some data related to the address > > translation is invisible to the core. To solve this issue, here let > > zone_sizes_init() call local_flush_tlb_kernel_range() to execute a > > sfence.vma instruction for the VMEMMAP area, ensuring that all > data related > > to the address translation is visible to the core. > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > --- > > arch/riscv/include/asm/tlbflush.h | 7 +++++++ > > arch/riscv/mm/init.c | 3 +++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/arch/riscv/include/asm/tlbflush.h > b/arch/riscv/include/asm/tlbflush.h > > index 801019381dea..b7696974e3c6 100644 > > --- a/arch/riscv/include/asm/tlbflush.h > > +++ b/arch/riscv/include/asm/tlbflush.h > > @@ -59,4 +59,11 @@ static inline void > flush_tlb_kernel_range(unsigned long start, > > flush_tlb_all(); > > } > > > > +/* Flush a range of kernel pages without broadcasting */ > > +static inline void local_flush_tlb_kernel_range(unsigned long > start, > > + unsigned long end) > > +{ > > + local_flush_tlb_all(); > > +} > > + > > #endif /* _ASM_RISCV_TLBFLUSH_H */ > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > > index 2c4a64e97aec..62eea656e341 100644 > > --- a/arch/riscv/mm/init.c > > +++ b/arch/riscv/mm/init.c > > @@ -72,6 +72,9 @@ static void __init zone_sizes_init(void) > > #endif > > max_zone_pfns[ZONE_NORMAL] = max_low_pfn; > > > > +#ifdef CONFIG_SPARSEMEM_VMEMMAP > > + local_flush_tlb_kernel_range(VMEMMAP_START, VMEMMAP_END); > > +#endif > > free_area_init(max_zone_pfns); > > } > > > > Just one nit: I would have added the flush to the vmemmap_populate > function instead, so that we don't flush uselessly when > !SPARSE_VMEMMAP > and we know exactly why we flush. > > > Sorry for the late reply. > > To be honest, initially, I placed the TLB flush in the > vmemmap_populate function like you suggested. However, I later > discovered that this function could be called multiple times during > the booting process, causing the TLB flush to be executed multiple > times. To improve performance, I decided to move the TLB flush to the > zone_sizes_init function instead, as this is where the kernel first > accesses the VMEMMAP region after vmemmap_populate. This allows the > kernel to perform only one TLB flush for the region, but the tradeoff > is that the code readability is reduced. Do you think this tradeoff is > worth it? or Maybe I can add some comments in the vmemmap_populate > function to describe the purpose of this TLB flush. Is it OK with you? I see, I did not know but it is indeed called many times. To me zone_sizes_init() is not the right place, what about adding this flush (with a comment) right after sparse_init() in misc_mem_init()? So that if someday we move zone_sizes_init(), we don't lose the flush! Thanks, Alex > > > Otherwise, you can add: > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > Thanks, > > Alex >
diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h index 801019381dea..b7696974e3c6 100644 --- a/arch/riscv/include/asm/tlbflush.h +++ b/arch/riscv/include/asm/tlbflush.h @@ -59,4 +59,11 @@ static inline void flush_tlb_kernel_range(unsigned long start, flush_tlb_all(); } +/* Flush a range of kernel pages without broadcasting */ +static inline void local_flush_tlb_kernel_range(unsigned long start, + unsigned long end) +{ + local_flush_tlb_all(); +} + #endif /* _ASM_RISCV_TLBFLUSH_H */ diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index 2c4a64e97aec..62eea656e341 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -72,6 +72,9 @@ static void __init zone_sizes_init(void) #endif max_zone_pfns[ZONE_NORMAL] = max_low_pfn; +#ifdef CONFIG_SPARSEMEM_VMEMMAP + local_flush_tlb_kernel_range(VMEMMAP_START, VMEMMAP_END); +#endif free_area_init(max_zone_pfns); }
The vmemmap_populate() creates VA to PA mapping for the VMEMMAP area, where all "strcut page" are located once CONFIG_SPARSEMEM_VMEMMAP is defined. These "struct page" are later initialized in the zone_sizes_init() function. However, during this process, no sfence.vma instruction is executed for this VMEMMAP area. This omission may cause the core to fail to perform page table work because some data related to the address translation is invisible to the core. To solve this issue, here let zone_sizes_init() call local_flush_tlb_kernel_range() to execute a sfence.vma instruction for the VMEMMAP area, ensuring that all data related to the address translation is visible to the core. Signed-off-by: Vincent Chen <vincent.chen@sifive.com> --- arch/riscv/include/asm/tlbflush.h | 7 +++++++ arch/riscv/mm/init.c | 3 +++ 2 files changed, 10 insertions(+)