[PATCH] riscv: mm: execute local TLB flush after populating vmemmap

[Alexandre Ghiti]

Hi Vincent,

On 4/13/23 02:48, Vincent Chen wrote:
>
>
> On Wed, Mar 22, 2023 at 12:11 AM Alexandre Ghiti <alex at 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 at 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 at rivosinc.com>
>
>     Thanks,
>
>     Alex
>