This patch is actually the v2 version of the this patch[1]. In the v1 version,
Alexandre suggested that I should make the patch more general, so I even changed
the title accordingly. Many thanks to Alexandre.
And Alexandre also submitted a similar patch[2] recently, in which he implemented
TLB flushing via flush_cache_vmap() instead. However, it seems that flush_cache_vmap()
does not cover every place where arch_sync_kernel_mappings() performs TLB flushing.
IMHO, arch_sync_kernel_mappings() cannot be replaced by flush_cache_vmap().
Please correct me if I'm wrong!
[1]: [PATCH] riscv: Flush stale TLB entry with VMAP_STACK enabled
http://lists.infradead.org/pipermail/linux-riscv/2023-August/037503.html
[2]: [PATCH -fixes] riscv: Implement flush_cache_vmap()
https://www.spinics.net/lists/kernel/msg4873712.html
Dylan Jhong (1):
riscv: Implement arch_sync_kernel_mappings() for "preventive" TLB
flush
arch/riscv/include/asm/page.h | 2 ++
arch/riscv/mm/tlbflush.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
--
2.34.1
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 <[email protected]>
---
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)
+ flush_tlb_all();
+}
\ No newline at end of file
--
2.34.1
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/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230807/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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)
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230807/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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 <[email protected]>
> ---
> 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/[email protected]/).
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 <[email protected]>
> > ---
> > 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/[email protected]/).
>
> 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