[1/1] riscv: Implement arch_sync_kernel_mappings() for "preventive" TLB flush

  Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB,
it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains
the correct kernel mapping.

The patch implements TLB flushing in arch_sync_kernel_mappings(), ensuring that kernel
page table mappings created via vmap/vmalloc() are updated before switching MM.

Signed-off-by: Dylan Jhong <dylan@andestech.com>
---
 arch/riscv/include/asm/page.h |  2 ++
 arch/riscv/mm/tlbflush.c      | 12 ++++++++++++
 2 files changed, 14 insertions(+)
  

Hi Dylan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.5-rc5 next-20230807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dylan-Jhong/riscv-Implement-arch_sync_kernel_mappings-for-preventive-TLB-flush/20230807-162922
base:   linus/master
patch link:    https://lore.kernel.org/r/20230807082305.198784-2-dylan%40andestech.com
patch subject: [PATCH 1/1] riscv: Implement arch_sync_kernel_mappings() for "preventive" TLB flush
config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20230807/202308071710.irjERWVF-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230807/202308071710.irjERWVF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308071710.irjERWVF-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/riscv/mm/tlbflush.c:159:6: warning: no previous prototype for 'arch_sync_kernel_mappings' [-Wmissing-prototypes]
     159 | void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/arch_sync_kernel_mappings +159 arch/riscv/mm/tlbflush.c

   152	
   153	/*
   154	 * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB,
   155	 * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains
   156	 * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel
   157	 * page table mappings created via vmap/vmalloc() are updated before switching MM.
   158	 */
 > 159	void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
  

Hi Dylan,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.5-rc5 next-20230807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dylan-Jhong/riscv-Implement-arch_sync_kernel_mappings-for-preventive-TLB-flush/20230807-162922
base:   linus/master
patch link:    https://lore.kernel.org/r/20230807082305.198784-2-dylan%40andestech.com
patch subject: [PATCH 1/1] riscv: Implement arch_sync_kernel_mappings() for "preventive" TLB flush
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20230807/202308072050.0T0FlSpT-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230807/202308072050.0T0FlSpT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308072050.0T0FlSpT-lkp@intel.com/

All errors (new ones prefixed by >>):

   riscv64-linux-ld: mm/memory.o: in function `.L1539':
>> memory.c:(.text+0x3b5c): undefined reference to `arch_sync_kernel_mappings'
   riscv64-linux-ld: mm/vmalloc.o: in function `.L301':
>> vmalloc.c:(.text+0xd24): undefined reference to `arch_sync_kernel_mappings'
   riscv64-linux-ld: mm/vmalloc.o: in function `vb_alloc.constprop.0':
   vmalloc.c:(.text+0x2c4e): undefined reference to `arch_sync_kernel_mappings'
   riscv64-linux-ld: mm/vmalloc.o: in function `.L0 ':
   vmalloc.c:(.text+0x2f2c): undefined reference to `arch_sync_kernel_mappings'
  

Hi Dylan,

On 07/08/2023 10:23, Dylan Jhong wrote:
> Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB,
> it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains
> the correct kernel mapping.
>
> The patch implements TLB flushing in arch_sync_kernel_mappings(), ensuring that kernel
> page table mappings created via vmap/vmalloc() are updated before switching MM.
>
> Signed-off-by: Dylan Jhong <dylan@andestech.com>
> ---
>   arch/riscv/include/asm/page.h |  2 ++
>   arch/riscv/mm/tlbflush.c      | 12 ++++++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> index b55ba20903ec..6c86ab69687e 100644
> --- a/arch/riscv/include/asm/page.h
> +++ b/arch/riscv/include/asm/page.h
> @@ -21,6 +21,8 @@
>   #define HPAGE_MASK              (~(HPAGE_SIZE - 1))
>   #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)
>   
> +#define ARCH_PAGE_TABLE_SYNC_MASK	PGTBL_PTE_MODIFIED
> +
>   /*
>    * PAGE_OFFSET -- the first address of the first page of memory.
>    * When not using MMU this corresponds to the first free page in
> diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c
> index 77be59aadc73..d63364948c85 100644
> --- a/arch/riscv/mm/tlbflush.c
> +++ b/arch/riscv/mm/tlbflush.c
> @@ -149,3 +149,15 @@ void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start,
>   	__flush_tlb_range(vma->vm_mm, start, end - start, PMD_SIZE);
>   }
>   #endif
> +
> +/*
> + * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB,
> + * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains
> + * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel
> + * page table mappings created via vmap/vmalloc() are updated before switching MM.
> + */
> +void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
> +{
> +	if (start < VMALLOC_END && end > VMALLOC_START)


This test is too restrictive, it should catch the range [MODULES_VADDR;  
MODULES_END[ too, sorry I did not notice that at first.


> +		flush_tlb_all();
> +}
> \ No newline at end of file


I have to admit that I *think* both your patch and mine are wrong: one 
of the problem that led to the removal of vmalloc_fault() is the 
possibility for tracing functions to actually allocate vmalloc regions 
in the vmalloc page fault path, which could give rise to nested 
exceptions (see 
https://lore.kernel.org/lkml/20200508144043.13893-1-joro@8bytes.org/).

Here, everytime we allocate a vmalloc region, we send an IPI. If a 
vmalloc allocation happens in this path (if it is traced for example), 
it will give rise to an IPI...and so on.

So I came to the conclusion that the only way to actually fix this issue 
is by resolving the vmalloc faults very early in the page fault path (by 
emitting a sfence.vma on uarch that cache invalid entries), before the 
kernel stack is even accessed. That's the best solution since it would 
completely remove all the preventive sfence.vma in 
flush_cache_vmap()/arch_sync_kernel_mappings(), we would rely on 
faulting which I assume should not happen a lot (?).

I'm implementing this solution, but I'm pretty sure it won't be ready 
for 6.5. In the meantime, we need either your patch or mine to fix your 
issue...
  

On Tue, Aug 08, 2023 at 12:16:50PM +0200, Alexandre Ghiti wrote:
> Hi Dylan,
> 
> On 07/08/2023 10:23, Dylan Jhong wrote:
> > Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB,
> > it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains
> > the correct kernel mapping.
> > 
> > The patch implements TLB flushing in arch_sync_kernel_mappings(), ensuring that kernel
> > page table mappings created via vmap/vmalloc() are updated before switching MM.
> > 
> > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > ---
> >   arch/riscv/include/asm/page.h |  2 ++
> >   arch/riscv/mm/tlbflush.c      | 12 ++++++++++++
> >   2 files changed, 14 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > index b55ba20903ec..6c86ab69687e 100644
> > --- a/arch/riscv/include/asm/page.h
> > +++ b/arch/riscv/include/asm/page.h
> > @@ -21,6 +21,8 @@
> >   #define HPAGE_MASK              (~(HPAGE_SIZE - 1))
> >   #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)
> > +#define ARCH_PAGE_TABLE_SYNC_MASK	PGTBL_PTE_MODIFIED
> > +
> >   /*
> >    * PAGE_OFFSET -- the first address of the first page of memory.
> >    * When not using MMU this corresponds to the first free page in
> > diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c
> > index 77be59aadc73..d63364948c85 100644
> > --- a/arch/riscv/mm/tlbflush.c
> > +++ b/arch/riscv/mm/tlbflush.c
> > @@ -149,3 +149,15 @@ void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start,
> >   	__flush_tlb_range(vma->vm_mm, start, end - start, PMD_SIZE);
> >   }
> >   #endif
> > +
> > +/*
> > + * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB,
> > + * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains
> > + * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel
> > + * page table mappings created via vmap/vmalloc() are updated before switching MM.
> > + */
> > +void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
> > +{
> > +	if (start < VMALLOC_END && end > VMALLOC_START)
> 
> 
> This test is too restrictive, it should catch the range [MODULES_VADDR; 
> MODULES_END[ too, sorry I did not notice that at first.
> 
> 
> > +		flush_tlb_all();
> > +}
> > \ No newline at end of file
> 
> 
> I have to admit that I *think* both your patch and mine are wrong: one of
> the problem that led to the removal of vmalloc_fault() is the possibility
> for tracing functions to actually allocate vmalloc regions in the vmalloc
> page fault path, which could give rise to nested exceptions (see
> https://lore.kernel.org/lkml/20200508144043.13893-1-joro@8bytes.org/).
> 
> Here, everytime we allocate a vmalloc region, we send an IPI. If a vmalloc
> allocation happens in this path (if it is traced for example), it will give
> rise to an IPI...and so on.
> 
> So I came to the conclusion that the only way to actually fix this issue is
> by resolving the vmalloc faults very early in the page fault path (by
> emitting a sfence.vma on uarch that cache invalid entries), before the
> kernel stack is even accessed. That's the best solution since it would
> completely remove all the preventive sfence.vma in
> flush_cache_vmap()/arch_sync_kernel_mappings(), we would rely on faulting
> which I assume should not happen a lot (?).
> 

Hi Alex,

Agree. 

If we could introduce a "new vmalloc_fault()" function before accessing the kernel stack,
which would trigger an SFENCE.VMA instruction, then each time we call vmalloc() or vmap()
to create new kernel mappings, we wouldn't need to execute flush_cache_vmap() or
arch_sync_kernel_mappings() to update the TLB. This should be able to balance both
performance and correctness.

> I'm implementing this solution, but I'm pretty sure it won't be ready for
> 6.5. In the meantime, we need either your patch or mine to fix your issue...
> 

If there are no others reporting this issues, I believe encountering this TLB flush problem
might not be so common. Perhaps we could wait until you've finished implementing the
"new vmalloc_fault()" feature. If anyone encounters problems in the meantime, I think they
can temporarily apply either my patch or yours to workaround the issue of updating TLB for
vmalloc.

Best regards,
Dylan Jhong