[ powerpc maintainers: Thoughts on replacing the ptesync in vmemmap_set_pmd() with a flush_cache_vmap() post-vmemmap-hotplug in the generic code? ]
On 7/6/26 11:00, Muchun Song wrote: >> On Jun 30, 2026, at 15:51, Vivian Wang <[email protected]> wrote: >> >> section_activate() does not flush TLB after populating new vmemmap >> pages. On most architectures, this is okay. However it is a problem on >> RISC-V since there the TLB caching non-present entries is permitted, >> which causes spurious faults on some hardwares. >> >> This seems to be most easily reproduced with DEBUG_VM=y and >> PAGE_POISONING=y, which causes these newly mapped struct pages to be >> poisoned i.e. written to immediately after mapping. >> >> Add a hook vmemmap_populate_finalize() in __populate_section_memmap() >> after population, to allow architectures to handle such situations as >> needed. Then implement it on RISC-V to arrange for the existing >> exception handler code to deal with these faults if they happen. >> >> Signed-off-by: Vivian Wang <[email protected]> >> --- >> Changes in v4: >> - Rebase on v7.2-rc1, drop dependencies >> - (No code changes otherwise) >> - (A concurrency fix for mark_new_valid_map was sent independently) >> >> https://lore.kernel.org/linux-riscv/20260629-riscv-mm-new-valid-map-ordering-v1-1-60d8c10c6...@iscas.ac.cn/ >> - Link to v3: >> https://patch.msgid.link/20260605-mark-after-vmemmap-populate-v3-1-a06001ac9...@iscas.ac.cn >> >> Changes in v3: >> - Merged back into one patch (Mike) >> - (No code changes otherwise.) >> - Link to v2: >> https://patch.msgid.link/20260604-mark-after-vmemmap-populate-v2-0-ab6a7d03b...@iscas.ac.cn >> >> Changes in v2: >> - Split patch in two, hook point and riscv hook >> - Explain hook necessity in patch 1 message (Mike) >> - Make hook #define based (Mike) >> - Call finalize hook only on populate success >> - Link to v1: >> https://patch.msgid.link/20260525-mark-after-vmemmap-populate-v1-1-e698d859b...@iscas.ac.cn >> --- >> arch/riscv/include/asm/pgtable.h | 4 ++++ >> arch/riscv/mm/init.c | 6 ++++++ >> mm/sparse-vmemmap.c | 8 ++++++++ >> 3 files changed, 18 insertions(+) >> >> diff --git a/arch/riscv/include/asm/pgtable.h >> b/arch/riscv/include/asm/pgtable.h >> index 5d5756bda82e..6b000c990ba7 100644 >> --- a/arch/riscv/include/asm/pgtable.h >> +++ b/arch/riscv/include/asm/pgtable.h >> @@ -1253,6 +1253,10 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) >> #define TASK_SIZE FIXADDR_START >> #endif >> >> +/* Needed on SPARSEMEM_VMEMMAP */ >> +#define vmemmap_populate_finalize vmemmap_populate_finalize >> +void __meminit vmemmap_populate_finalize(void); >> + >> #else /* CONFIG_MMU */ >> >> #define PAGE_SHARED __pgprot(0) >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c >> index 5b1b3c88b4d1..800cb5c007d1 100644 >> --- a/arch/riscv/mm/init.c >> +++ b/arch/riscv/mm/init.c >> @@ -1372,6 +1372,12 @@ int __meminit vmemmap_populate(unsigned long start, >> unsigned long end, int node, >> */ >> return vmemmap_populate_hugepages(start, end, node, altmap); >> } >> + >> +void __meminit vmemmap_populate_finalize(void) >> +{ >> + /* Avoid faults on cached non-present TLB entries. */ >> + mark_new_valid_map(); >> +} >> #endif >> >> #if defined(CONFIG_MMU) && defined(CONFIG_64BIT) >> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c >> index 99e2be39671b..290cafcfd723 100644 >> --- a/mm/sparse-vmemmap.c >> +++ b/mm/sparse-vmemmap.c >> @@ -544,6 +544,12 @@ static int __meminit >> vmemmap_populate_compound_pages(unsigned long start_pfn, >> >> #endif >> >> +#ifndef vmemmap_populate_finalize >> +static void __meminit vmemmap_populate_finalize(void) >> +{ >> +} >> +#endif >> + >> struct page * __meminit __populate_section_memmap(unsigned long pfn, >> unsigned long nr_pages, int nid, struct vmem_altmap *altmap, >> struct dev_pagemap *pgmap) >> @@ -564,6 +570,8 @@ struct page * __meminit >> __populate_section_memmap(unsigned long pfn, >> if (r < 0) >> return NULL; >> >> + vmemmap_populate_finalize(); > Does the hook of flush_cache_vmap() work for you? From the document of > cachetlb.rst, it said: > > 6) ``void flush_cache_vmap(unsigned long start, unsigned long end)`` > ``void flush_cache_vunmap(unsigned long start, unsigned long end)`` > > Here in these two interfaces we are flushing a specific range > of (kernel) virtual addresses from the cache. After running, > there will be no entries in the cache for the kernel address > space for virtual addresses in the range 'start' to 'end-1'. > > It seems that flush_cache_vmap() is supposed to be called after a new kernel > mapping has been installed, but before this address range is accessed. Good point. I had initially avoided flush_cache_vmap() because, to be honest, it looked scary. But the combination of "have vmemmap" and "have flush_cache_vmap()" is much less scary. I'd need to drop the address range check in RISC-V's flush_cache_vmap(), but might as well. > Therefore, invoking flush_cache_vmap() here aligns perfectly with its > interface > definition. But how would this change impact other architectures? Currently, > among architectures where CONFIG_SPARSEMEM=y, only RISC-V and PowerPC provide > a concrete implementation for flush_cache_vmap(). > > For PowerPC, the implementation is as follows: > > static inline void flush_cache_vmap(unsigned long start, unsigned long > end) > { > asm volatile("ptesync" ::: "memory"); > } > > Here, flush_cache_vmap() is also intended to supply the ptesync instruction > that the kernel address mapping lacks. However, flush_cache_vmap() is not > called in the __populate_section_memmap() path. So, how does PowerPC ensure > ptesync is executed there? The answer lies in the architecture-specific > vmemmap_set_pmd(), which injects the necessary ptesync. > > If we choose to reuse flush_cache_vmap() in the generic path, it yields the > following benefits: > > - Avoids architectural churn: We do not need to introduce a new hook > like vmemmap_populate_finalize(). > - Cleans up arch code: PowerPC can deprecate its custom vmemmap_set_pmd() > and migrate to the generic implementation. > > Let me know if this rationale makes sense, or if there are any hidden edge > cases I might have overlooked regarding other architectures. > > Thanks, > Muchun At least on the face of it, it makes sense. The main thing is I don't know much about powerpc. So I'm also hoping powerpc maintainers could chime in. I also don't know how this would affect new archs, but if they need flush_cache_vmap() they *probably* also want this treatment for vmemmap hotplug? I guess we can deal with that when it happens. Thanks, Vivian "dramforever" Wang
