Re: [PATCH v2 1/2] arm64: mmu: don't allocate page while spliting linear mapping

Ryan Roberts
2025-12-18T15:42:40Z

On 18/12/2025 15:01, Yeoreum Yun wrote:
> Hi Ryan,
>
>> On 17/12/2025 18:20, Yeoreum Yun wrote:
>>> Current linear_map_split_to_ptes() allocate the pagetable
>>> while split linear mapping to ptes by stop_machine() with GFP_ATOMIC.
>>>
>>> This is fine for non-PREEMPR_RT case.
>>> However It's a problem in PREEMPR_RT case since
>>> generic memory allocation/free APIs (e.x) pgtable_alloc(), __get_free_pages and etc
>>> couldn't be called in non-preemptible context except _nolock() APIs
>>> since generic memory allocation/free APIs are *sleepable* for using *spin_lock()*
>>>
>>> IOW, calling a pgtable_alloc() even with GFP_ATOMIC
>>> doesn't allow in __linear_map_split_to_pte() executed by stopper thread
>>> where preemption is disabled in PREEMPR_RT.
>>>
>>> To address this, divide linear_map_maybe_split_ptes:
>>> - collect number of pages to require for spliting.
>>> - allocate the required number of pages for spliting.
>>> - with pre-allocate page, split the linear map.
>>
>> Thanks for working on this fix!
>>
>> First some high level comments: I'm not a huge fan of the approach with the
>> different modes to modify behaviour.
>>
>> For the first step of figuring out the number of pages required; there is no
>> need to be inside stop_machine() for that. Can should be able to just walk the
>> linear map without any locking since it is created once and remains static, with
>> the exception of hotplug, but that is not enabled yet.
>>
>> I think it would be cleaner to just create separate walker callbacks rather than
>> repurpose the existing walker.
>
> Okay. I'll repsin with separate walker.
>
>>
>> Then you could simply change the gfp flag to a callback function pointer and
>> pass the desired allocator as a function pointer. This would match the existing
>> patterns used in mmu.c today.
>>
>> The preallocated page list could be stored in a global static variable and you
>> could create an allocation function that just strims a page from that list. See
>> kpti_ng_pgd_alloc() for an existing example. That assumes a contiguous block,
>> but you could generalize it to hold a list of struct pages and reuse it for both
>> cases?
>
> If we use walker callbacks, I think it not only pass the callback
> function, but it need to pass the gfp flag too since split_xxx() is
> called by another functions. (i.e) set_memory_xxx()
>
> I think I can remove the mode with the suggestion of @David
> but, I think it would be still use split_args with information of
> gfp and preallocate buffer.

I don't think that's necessary; all other users pass GFP_PGTABLE_KERNEL, so we
just need one allocator function that allocates with GFP_PGTABLE_KERNEL and
another allocator function that allocates from the prealloc'ed list. Then the
user passes the function they want to use.

That would work, I think?

>>
>>>
>>> Fixes: 3df6979d222b ("arm64: mm: split linear mapping if BBML2 unsupported on secondary CPUs")
>>> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
>>> ---
>>> arch/arm64/mm/mmu.c | 213 +++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 170 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index 9ae7ce00a7ef..e4e6c7e0a016 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -527,18 +527,28 @@ static void early_create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>>> panic("Failed to create page tables\n");
>>> }
>>>
>>> -static phys_addr_t __pgd_pgtable_alloc(struct mm_struct *mm, gfp_t gfp,
>>> - enum pgtable_type pgtable_type)
>>> -{
>>> - /* Page is zeroed by init_clear_pgtable() so don't duplicate effort. */
>>> - struct ptdesc *ptdesc = pagetable_alloc(gfp & ~__GFP_ZERO, 0);
>>> - phys_addr_t pa;
>>> -
>>> - if (!ptdesc)
>>> - return INVALID_PHYS_ADDR;
>>> +enum split_modes {
>>> + SPLIT_ALLOC,
>>> + SPLIT_PREALLOC,
>>> + SPLIT_COLLECT,
>>> +};
>>>
>>> - pa = page_to_phys(ptdesc_page(ptdesc));
>>> +struct split_args {
>>> + enum split_modes mode;
>>> + union {
>>> + struct {
>>> + unsigned long nr;
>>> + unsigned long i;
>>> + struct page **pages;
>>> + };
>>> + gfp_t gfp;
>>> + };
>>> +};
>>>
>>> +static __always_inline void __pgd_pgtable_init(struct mm_struct *mm,
>>> + struct ptdesc *ptdesc,
>>> + enum pgtable_type pgtable_type)
>>> +{
>>> switch (pgtable_type) {
>>> case TABLE_PTE:
>>> BUG_ON(!pagetable_pte_ctor(mm, ptdesc));
>>> @@ -554,19 +564,56 @@ static phys_addr_t __pgd_pgtable_alloc(struct mm_struct *mm, gfp_t gfp,
>>> break;
>>> }
>>>
>>> +}
>>> +
>>> +static phys_addr_t __pgd_pgtable_alloc(struct mm_struct *mm, gfp_t gfp,
>>> + enum pgtable_type pgtable_type)
>>> +{
>>> + /* Page is zeroed by init_clear_pgtable() so don't duplicate effort. */
>>> + struct ptdesc *ptdesc = pagetable_alloc(gfp & ~__GFP_ZERO, 0);
>>> + phys_addr_t pa;
>>> +
>>> + if (!ptdesc)
>>> + return INVALID_PHYS_ADDR;
>>> +
>>> + pa = page_to_phys(ptdesc_page(ptdesc));
>>> +
>>> + __pgd_pgtable_init(mm, ptdesc, pgtable_type);
>>> +
>>> return pa;
>>> }
>>>
>>> +static phys_addr_t __pgd_pgtable_prealloc(struct mm_struct *mm,
>>> + struct split_args *split_args,
>>> + enum pgtable_type pgtable_type)
>>> +{
>>> + struct page *page;
>>> +
>>> + BUG_ON(split_args->i >= split_args->nr);
>>> +
>>> + page = split_args->pages[split_args->i++];
>>> + if (!page)
>>> + return INVALID_PHYS_ADDR;
>>> +
>>> + __pgd_pgtable_init(mm, page_ptdesc(page), pgtable_type);
>>> +
>>> + return page_to_phys(page);
>>> +}
>>> +
>>> static phys_addr_t
>>> -pgd_pgtable_alloc_init_mm_gfp(enum pgtable_type pgtable_type, gfp_t gfp)
>>> +pgd_pgtable_alloc_split(enum pgtable_type pgtable_type,
>>> + struct split_args *split_args)
>>> {
>>> - return __pgd_pgtable_alloc(&init_mm, gfp, pgtable_type);
>>> + if (split_args->mode == SPLIT_ALLOC)
>>> + return __pgd_pgtable_alloc(&init_mm, split_args->gfp, pgtable_type);
>>> +
>>> + return __pgd_pgtable_prealloc(&init_mm, split_args, pgtable_type);
>>> }
>>>
>>> static phys_addr_t __maybe_unused
>>> pgd_pgtable_alloc_init_mm(enum pgtable_type pgtable_type)
>>> {
>>> - return pgd_pgtable_alloc_init_mm_gfp(pgtable_type, GFP_PGTABLE_KERNEL);
>>> + return __pgd_pgtable_alloc(&init_mm, GFP_PGTABLE_KERNEL, pgtable_type);
>>> }
>>>
>>> static phys_addr_t
>>> @@ -584,7 +631,9 @@ static void split_contpte(pte_t *ptep)
>>> __set_pte(ptep, pte_mknoncont(__ptep_get(ptep)));
>>> }
>>>
>>> -static int split_pmd(pmd_t *pmdp, pmd_t pmd, gfp_t gfp, bool to_cont)
>>> +static int split_pmd(pmd_t *pmdp, pmd_t pmd,
>>> + struct split_args *split_args,
>>> + bool to_cont)
>>> {
>>> pmdval_t tableprot = PMD_TYPE_TABLE | PMD_TABLE_UXN | PMD_TABLE_AF;
>>> unsigned long pfn = pmd_pfn(pmd);
>>> @@ -593,7 +642,7 @@ static int split_pmd(pmd_t *pmdp, pmd_t pmd, gfp_t gfp, bool to_cont)
>>> pte_t *ptep;
>>> int i;
>>>
>>> - pte_phys = pgd_pgtable_alloc_init_mm_gfp(TABLE_PTE, gfp);
>>> + pte_phys = pgd_pgtable_alloc_split(TABLE_PTE, split_args);
>>> if (pte_phys == INVALID_PHYS_ADDR)
>>> return -ENOMEM;
>>> ptep = (pte_t *)phys_to_virt(pte_phys);
>>> @@ -628,7 +677,9 @@ static void split_contpmd(pmd_t *pmdp)
>>> set_pmd(pmdp, pmd_mknoncont(pmdp_get(pmdp)));
>>> }
>>>
>>> -static int split_pud(pud_t *pudp, pud_t pud, gfp_t gfp, bool to_cont)
>>> +static int split_pud(pud_t *pudp, pud_t pud,
>>> + struct split_args *split_args,
>>> + bool to_cont)
>>> {
>>> pudval_t tableprot = PUD_TYPE_TABLE | PUD_TABLE_UXN | PUD_TABLE_AF;
>>> unsigned int step = PMD_SIZE >> PAGE_SHIFT;
>>> @@ -638,7 +689,7 @@ static int split_pud(pud_t *pudp, pud_t pud, gfp_t gfp, bool to_cont)
>>> pmd_t *pmdp;
>>> int i;
>>>
>>> - pmd_phys = pgd_pgtable_alloc_init_mm_gfp(TABLE_PMD, gfp);
>>> + pmd_phys = pgd_pgtable_alloc_split(TABLE_PMD, split_args);
>>> if (pmd_phys == INVALID_PHYS_ADDR)
>>> return -ENOMEM;
>>> pmdp = (pmd_t *)phys_to_virt(pmd_phys);
>>> @@ -672,6 +723,10 @@ static int split_kernel_leaf_mapping_locked(unsigned long addr)
>>> pmd_t *pmdp, pmd;
>>> pte_t *ptep, pte;
>>> int ret = 0;
>>> + struct split_args split_args = {
>>> + .mode = SPLIT_ALLOC,
>>> + .gfp = GFP_PGTABLE_KERNEL,
>>> + };
>>>
>>> /*
>>> * PGD: If addr is PGD aligned then addr already describes a leaf
>>> @@ -707,7 +762,7 @@ static int split_kernel_leaf_mapping_locked(unsigned long addr)
>>> if (!pud_present(pud))
>>> goto out;
>>> if (pud_leaf(pud)) {
>>> - ret = split_pud(pudp, pud, GFP_PGTABLE_KERNEL, true);
>>> + ret = split_pud(pudp, pud, &split_args, true);
>>> if (ret)
>>> goto out;
>>> }
>>> @@ -732,7 +787,7 @@ static int split_kernel_leaf_mapping_locked(unsigned long addr)
>>> */
>>> if (ALIGN_DOWN(addr, PMD_SIZE) == addr)
>>> goto out;
>>> - ret = split_pmd(pmdp, pmd, GFP_PGTABLE_KERNEL, true);
>>> + ret = split_pmd(pmdp, pmd, &split_args, true);
>>> if (ret)
>>> goto out;
>>> }
>>> @@ -831,12 +886,17 @@ int split_kernel_leaf_mapping(unsigned long start, unsigned long end)
>>> static int split_to_ptes_pud_entry(pud_t *pudp, unsigned long addr,
>>> unsigned long next, struct mm_walk *walk)
>>> {
>>> - gfp_t gfp = *(gfp_t *)walk->private;
>>> + struct split_args *split_args = (struct split_args *)walk->private;
>>
>> no need for the casting here; it's a void *.
>
> Right. I'll remove it.
>
>>
>>> pud_t pud = pudp_get(pudp);
>>> int ret = 0;
>>>
>>> - if (pud_leaf(pud))
>>> - ret = split_pud(pudp, pud, gfp, false);
>>> + if (!pud_leaf(pud))
>>> + return 0;
>>> +
>>> + if (split_args->mode == SPLIT_COLLECT)
>>> + split_args->nr++;
>>
>> bug: you're incrementing this for the pmd table that you would have inserted,
>> but because you're not actually splitting, split_to_ptes_pmd_entry() will never
>> be called for the 512 pte tables you also need. So you need:
>>
>> nr += 1 + PTRS_PER_PMD;
>>
>> I'm guessing you didn't test this as the BUG would have triggered when you ran
>> out of preallocated memory?
>>
>
> Good catch! Yes. This is definitely bug.
> The reason why I didn't hit the but there is no "block pud" in my enviroment.
>
>>> + else
>>> + ret = split_pud(pudp, pud, split_args, false);
>>>
>>> return ret;
>>> }
>>> @@ -844,22 +904,29 @@ static int split_to_ptes_pud_entry(pud_t *pudp, unsigned long addr,
>>> static int split_to_ptes_pmd_entry(pmd_t *pmdp, unsigned long addr,
>>> unsigned long next, struct mm_walk *walk)
>>> {
>>> - gfp_t gfp = *(gfp_t *)walk->private;
>>> + struct split_args *split_args = (struct split_args *)walk->private;
>>> pmd_t pmd = pmdp_get(pmdp);
>>> - int ret = 0;
>>> + int ret;
>>>
>>> - if (pmd_leaf(pmd)) {
>>> - if (pmd_cont(pmd))
>>> - split_contpmd(pmdp);
>>> - ret = split_pmd(pmdp, pmd, gfp, false);
>>> + if (!pmd_leaf(pmd))
>>> + return 0;
>>>
>>> - /*
>>> - * We have split the pmd directly to ptes so there is no need to
>>> - * visit each pte to check if they are contpte.
>>> - */
>>> - walk->action = ACTION_CONTINUE;
>>> + if (split_args->mode == SPLIT_COLLECT) {
>>> + split_args->nr++;
>>> + return 0;
>>> }
>>>
>>> + if (pmd_cont(pmd))
>>> + split_contpmd(pmdp);
>>> + ret = split_pmd(pmdp, pmd, split_args, false);
>>> +
>>> + /*
>>> + * We have split the pmd directly to ptes so there is no need to
>>> + * visit each pte to check if they are contpte.
>>> + */
>>> + walk->action = ACTION_CONTINUE;
>>> +
>>> +
>>> return ret;
>>> }
>>>
>>> @@ -880,13 +947,15 @@ static const struct mm_walk_ops split_to_ptes_ops = {
>>> .pte_entry = split_to_ptes_pte_entry,
>>> };
>>>
>>> -static int range_split_to_ptes(unsigned long start, unsigned long end, gfp_t gfp)
>>> +static int range_split_to_ptes(unsigned long start, unsigned long end,
>>> + struct split_args *split_args)
>>> {
>>> int ret;
>>>
>>> arch_enter_lazy_mmu_mode();
>>> ret = walk_kernel_page_table_range_lockless(start, end,
>>> - &split_to_ptes_ops, NULL, &gfp);
>>> + &split_to_ptes_ops, NULL,
>>> + split_args);
>>> arch_leave_lazy_mmu_mode();
>>>
>>> return ret;
>>> @@ -903,7 +972,7 @@ static void __init init_idmap_kpti_bbml2_flag(void)
>>> smp_mb();
>>> }
>>>
>>> -static int __init linear_map_split_to_ptes(void *__unused)
>>> +static int __init linear_map_split_to_ptes(void *data)
>>> {
>>> /*
>>> * Repainting the linear map must be done by CPU0 (the boot CPU) because
>>> @@ -911,6 +980,7 @@ static int __init linear_map_split_to_ptes(void *__unused)
>>> * be held in a waiting area with the idmap active.
>>> */
>>> if (!smp_processor_id()) {
>>> + struct split_args *split_args = data;
>>> unsigned long lstart = _PAGE_OFFSET(vabits_actual);
>>> unsigned long lend = PAGE_END;
>>> unsigned long kstart = (unsigned long)lm_alias(_stext);
>>> @@ -928,12 +998,13 @@ static int __init linear_map_split_to_ptes(void *__unused)
>>> * PTE. The kernel alias remains static throughout runtime so
>>> * can continue to be safely mapped with large mappings.
>>> */
>>> - ret = range_split_to_ptes(lstart, kstart, GFP_ATOMIC);
>>> + ret = range_split_to_ptes(lstart, lend, split_args);
>>> if (!ret)
>>> - ret = range_split_to_ptes(kend, lend, GFP_ATOMIC);
>>> + ret = range_split_to_ptes(kstart, kend, split_args);
>>> if (ret)
>>> panic("Failed to split linear map\n");
>>> - flush_tlb_kernel_range(lstart, lend);
>>> + if (split_args->mode != SPLIT_COLLECT)
>>> + flush_tlb_kernel_range(lstart, lend);
>>>
>>> /*
>>> * Relies on dsb in flush_tlb_kernel_range() to avoid reordering
>>> @@ -963,10 +1034,61 @@ static int __init linear_map_split_to_ptes(void *__unused)
>>>
>>> void __init linear_map_maybe_split_to_ptes(void)
>>> {
>>> - if (linear_map_requires_bbml2 && !system_supports_bbml2_noabort()) {
>>> - init_idmap_kpti_bbml2_flag();
>>> - stop_machine(linear_map_split_to_ptes, NULL, cpu_online_mask);
>>> + struct page **pages = NULL;
>>> + struct split_args split_args;
>>> + int order;
>>> + int nr_populated;
>>> + int err;
>>> +
>>> + if (!linear_map_requires_bbml2 || system_supports_bbml2_noabort())
>>> + return;
>>> +
>>> + /*
>>> + * Phase 1: Collect required extra pages to split.
>>> + */
>>> + split_args.mode = SPLIT_COLLECT;
>>> + split_args.nr = 0;
>>> +
>>> + init_idmap_kpti_bbml2_flag();
>>> + stop_machine(linear_map_split_to_ptes, &split_args, cpu_online_mask);
>>
>> This doesn't require stop_machine().
>
> Yes. I'll separate with new walker as you suggest.
>
>>
>>> +
>>> + /*
>>> + * Phase 2: Allocate necessary pages to split.
>>> + */
>>> + if (split_args.nr == 0) {
>>> + err = 0;
>>> + split_args.mode = SPLIT_ALLOC;
>>
>> How would we hit this condition? If everything is already split why not just
>> exit early?
>
> I'm thinking about if all of pages map with CONTPTE only then
> it need to be splited, but would you let me know whether It wouldn't be possible
> for cross checking?

Ahh, yes good point!

>
>>
>>> + } else {
>>> + err = -ENOMEM;
>>> + order = order_base_2(PAGE_ALIGN(split_args.nr *
>>> + sizeof(struct page *)) >> PAGE_SHIFT);
>>> +
>>> + pages = (struct page **)__get_free_pages(GFP_ATOMIC | __GFP_ZERO, order);
>>
>> As per the other patch, I'd prefer to see GFP_KERNEL | __GFP_ZERO here.
>>
>> Let's make this a list of struct ptdesc * though.
>
> Agree. I'll change it.
>
>>
>>> + if (!pages)
>>> + goto error;
>>> +
>>> + nr_populated = alloc_pages_bulk(GFP_ATOMIC | __GFP_ZERO, split_args.nr, pages);
>>
>> Just GFP_KERNEL. No need for __GFP_ZERO as they will all be fully overwritten
>> with PTEs.
>>
>> I'd prefer to use pagetable_alloc(GFP_KERNEL, 0) in a loop since that will
>> return a proper ptdesc.
>
> Okay.
>
>>
>>> + if (nr_populated < split_args.nr)
>>> + goto error;
>>> +
>>> + err = 0;
>>> + split_args.mode = SPLIT_PREALLOC;
>>> + split_args.i = 0;
>>> + split_args.pages = pages;
>>> }
>>> +
>>> + /*
>>> + * Phase 3: Split linear map.
>>> + */
>>> + init_idmap_kpti_bbml2_flag();
>>> + stop_machine(linear_map_split_to_ptes, &split_args, cpu_online_mask);
>>> +
>>> +error:
>>> + if (pages)
>>> + free_pages((unsigned long)pages, order);
>>
>> It's possible (but unlikely) that another CPU caused a split between figuring
>> out the required number of tables and actually doing the full split. In this
>> case, you would have preallocated pages left over that you will need to free here.
>
> You're right. I've overlooked this case.
> I'll handle it.
>
>
> Thanks for your review!
>
> --
> Sincerely,
> Yeoreum Yun