Memory hotplug support for arm64 - complete patchset

From: Maciej Bielski <[email protected]>

This is a second and improved version of the patch previously released in [3].

It builds on the work by Scott Branden [2] and, henceforth, it needs to be applied on top of Scott's patches [2]. Comments are very welcome.

Changes from the original patchset and known issues:

- Compared to Scott's original patchset, this work adds the mapping of the new hotplugged pages into the kernel page tables. This is done by copying the old swapper_pg_dir over a new page, adding the new mappings, and then switching to the newly built pg_dir (see hotplug_paging in arch/arm64/mmu.c). There might be better ways to to this: suggestions are more than welcome.

arch/arm64/mm/mmu.c +4 -0

+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables

arch/arm64/include/asm/pgtable.h +10 -0

+static inline unsigned long pmd_page_vaddr(pmd_t pmd)
+{
+	return (unsigned long) __va(pmd_page_paddr(pmd));
+}
+
 /* Find an entry in the third-level page table. */
 #define pte_index(addr)		(((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))

@@ -450,6 +455,11 @@ static inline phys_addr_t pud_page_paddr(pud_t pud)
 	return pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK;
 }
 
+static inline unsigned long pud_page_vaddr(pud_t pud)
+{
+	return (unsigned long) __va(pud_page_paddr(pud));
+}
+
 /* Find an entry in the second-level page table. */
 #define pmd_index(addr)		(((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
 

+static void free_pagetable(struct page *page, int order, bool direct)

arch/arm64/mm/mmu.c +24 -0

+{
+	unsigned long magic;
+	unsigned int nr_pages = 1 << order;
+	struct vmem_altmap *altmap = to_vmem_altmap((unsigned long) page);
+
+	if (altmap) {
+		vmem_altmap_free(altmap, nr_pages);
+		return;
+	}
+
+	/* bootmem page has reserved flag */
+	if (PageReserved(page)) {
+		__ClearPageReserved(page);
+
+		magic = (unsigned long)page->lru.next;
+		if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) {
+			while (nr_pages--)
+				put_page_bootmem(page++);
+		} else {
+			while (nr_pages--)
+				free_reserved_page(page++);
+		}
+	} else {
+		/*

arch/arm64/mm/mmu.c +4 -0

+
+static void free_pte_table(pmd_t *pmd, bool direct)
+{
+	pte_t *pte_start, *pte;

arch/arm64/mm/mmu.c +22 -0

+			/*
+			 * Do not free direct mapping pages since they were
+			 * freed when offlining, or simplely not in use.
+			 */
+			if (!direct)
+				free_pagetable(pte_page(*pte), 0, direct);
+
+			/*
+			 * This spin lock could be only
+			 * taken in _pte_aloc_kernel in
+			 * mm/memory.c and nowhere else
+			 * (for arm64). Not sure if the
+			 * function above can be called
+			 * concurrently. In doubt,
+			 * I am living it here for now,
+			 * but it probably can be removed.
+			 */
+			spin_lock(&init_mm.page_table_lock);
+			pte_clear(&init_mm, addr, pte);
+			spin_unlock(&init_mm.page_table_lock);
+		} else {
+			/*

arch/arm64/mm/mmu.c +8 -0

+
+			page_addr = page_address(pte_page(*pte));
+			if (!memchr_inv(page_addr, PAGE_INUSE, PAGE_SIZE)) {
+				free_pagetable(pte_page(*pte), 0, direct);
+
+				/*
+				 * This spin lock could be only
+				 * taken in _pte_aloc_kernel in

+}

arch/arm64/mm/mmu.c +7 -0

+
+static void remove_pmd_table(pmd_t *pmd, unsigned long addr,
+	unsigned long end, bool direct)
+{
+	unsigned long next;
+	void *page_addr;
+	pte_t *pte;

arch/arm64/mm/mmu.c +31 -0

+				if (!direct) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+				}
+				/*
+				 * This spin lock could be only
+				 * taken in _pte_aloc_kernel in
+				 * mm/memory.c and nowhere else
+				 * (for arm64). Not sure if the
+				 * function above can be called
+				 * concurrently. In doubt,
+				 * I am living it here for now,
+				 * but it probably can be removed.
+				 */
+				spin_lock(&init_mm.page_table_lock);
+				pmd_clear(pmd);
+				spin_unlock(&init_mm.page_table_lock);
+			} else {
+				/* If here, we are freeing vmemmap pages. */
+				memset((void *)addr, PAGE_INUSE, next - addr);
+
+				page_addr = page_address(pmd_page(*pmd));
+				if (!memchr_inv(page_addr, PAGE_INUSE,
+						PMD_SIZE)) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+
+					/*
+					 * This spin lock could be only
+					 * taken in _pte_aloc_kernel in
+					 * mm/memory.c and nowhere else

arch/arm64/mm/mmu.c +14 -0

+void remove_pagetable(unsigned long start, unsigned long end, bool direct)
+{
+	unsigned long next;
+	unsigned long addr;
+	pgd_t *pgd;
+	pud_t *pud;
+
+	for (addr = start; addr < end; addr = next) {
+		next = pgd_addr_end(addr, end);
+
+		pgd = pgd_offset_k(addr);
+		if (pgd_none(*pgd))
+			continue;
+

Hi,

Quoted 24 lines · Andrea Reale

arch/arm64/include/asm/pgtable.h +10 -0

+static inline unsigned long pmd_page_vaddr(pmd_t pmd)
+{
+	return (unsigned long) __va(pmd_page_paddr(pmd));
+}
+
 /* Find an entry in the third-level page table. */
 #define pte_index(addr)		(((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))

@@ -450,6 +455,11 @@ static inline phys_addr_t pud_page_paddr(pud_t pud)
 	return pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK;
 }
 
+static inline unsigned long pud_page_vaddr(pud_t pud)
+{
+	return (unsigned long) __va(pud_page_paddr(pud));
+}
+
 /* Find an entry in the second-level page table. */
 #define pmd_index(addr)		(((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
 

@@ -502,6 +512,11 @@ static inline phys_addr_t pgd_page_paddr(pgd_t pgd)
 	return pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK;
 }
 

arch/arm64/include/asm/pgtable.h static inline phys_addr_t pgd_page_paddr(pgd_t pgd) · line 519

Quoted 5 lines

arch/arm64/include/asm/pgtable.h +5 -0

+static inline unsigned long pgd_page_vaddr(pgd_t pgd)
+{
+	return (unsigned long) __va(pgd_page_paddr(pgd));
+}
+

These duplicate the existing p*d_offset*() functions, and I do not think they are necessary. More on that below.

[...]

Quoted 7 lines

arch/arm64/mm/init.c +0 -1

@@ -551,7 +551,6 @@ int arch_add_memory(int nid, u64 start, u64 size, bool for_device)
 	unsigned long nr_pages = size >> PAGE_SHIFT;
 	unsigned long end_pfn = start_pfn + nr_pages;
 	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
-	unsigned long pfn;
 	int ret;
 

arch/arm64/mm/init.c int arch_add_memory(int nid, u64 start, u64 size, bool for_device) · line 556

Quoted 1 line

arch/arm64/mm/init.c +0 -0

 	if (end_pfn > max_sparsemem_pfn) {

Should this have been part of a prior patch?

This patch doesn't remove any users of this variable.

[...]

Quoted 1 line

+static void free_pagetable(struct page *page, int order, bool direct)

This "direct" parameter needs a better name, and a description in a comment block above this function.

Quoted 29 lines

arch/arm64/mm/mmu.c +29 -0

+{
+	unsigned long magic;
+	unsigned int nr_pages = 1 << order;
+	struct vmem_altmap *altmap = to_vmem_altmap((unsigned long) page);
+
+	if (altmap) {
+		vmem_altmap_free(altmap, nr_pages);
+		return;
+	}
+
+	/* bootmem page has reserved flag */
+	if (PageReserved(page)) {
+		__ClearPageReserved(page);
+
+		magic = (unsigned long)page->lru.next;
+		if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) {
+			while (nr_pages--)
+				put_page_bootmem(page++);
+		} else {
+			while (nr_pages--)
+				free_reserved_page(page++);
+		}
+	} else {
+		/*
+		 * Only direct pagetable allocation (those allocated via
+		 * hotplug) call the pgtable_page_ctor; vmemmap pgtable
+		 * allocations don't.
+		 */
+		if (direct)

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 583

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+			pgtable_page_dtor(page);
+
+		free_pages((unsigned long)page_address(page), order);
+	}
+}

This largely looks like a copy of the x86 code. Why can that not be factored out to an arch-neutral location?

Quoted 4 lines

arch/arm64/mm/mmu.c +4 -0

+
+static void free_pte_table(pmd_t *pmd, bool direct)
+{
+	pte_t *pte_start, *pte;

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 597

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+	struct page *page;
+	int i;
+
+	pte_start =  (pte_t *) pmd_page_vaddr(*pmd);
+	/* Check if there is no valid entry in the PMD */
+	for (i = 0; i < PTRS_PER_PTE; i++) {
+		pte = pte_start + i;
+		if (!pte_none(*pte))
+			return;
+	}

If we must walk the tables in this way, please use the existing pattern from arch/arm64/mm/dump.c.

e.g.

	pte_t *pte;

	pte = pte_offset_kernel(pmd, 0UL);
	for (i = 0; i < PTRS_PER_PTE; i++, pte++)
		if (!pte_none)
			return;

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 601

Quoted 4 lines

arch/arm64/mm/mmu.c +4 -0

+
+	page = pmd_page(*pmd);
+
+	free_pagetable(page, 0, direct);

The page has been freed here, and may be subject to arbitrary modification...

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 610

Quoted 9 lines

arch/arm64/mm/mmu.c +9 -0

+
+	/*
+	 * This spin lock could be only taken in _pte_aloc_kernel
+	 * in mm/memory.c and nowhere else (for arm64). Not sure if
+	 * the function above can be called concurrently. In doubt,
+	 * I am living it here for now, but it probably can be removed
+	 */
+	spin_lock(&init_mm.page_table_lock);
+	pmd_clear(pmd);

... but we only remove it from the page tables here, so the page table walkers can see junk in the tables, were the page reused in the mean timer.

After clearing the PMD, it needs to be cleared from TLBs. We allow partial walks to be cached, so the TLBs may still start walking this entry or beyond.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	spin_unlock(&init_mm.page_table_lock);
+}

The same comments apply to all the free_p*d_table() functions, so I shan't repeat them.

[...]

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 691

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+static void remove_pte_table(pte_t *pte, unsigned long addr,
+	unsigned long end, bool direct)
+{
+	unsigned long next;
+	void *page_addr;
+
+	for (; addr < end; addr = next, pte++) {
+		next = (addr + PAGE_SIZE) & PAGE_MASK;
+		if (next > end)
+			next = end;

Please use the usual p*d_addr_end() functions. See alloc_init_pmd() and friends in arch/arm64/mm/mmu.c for examples of how to use them.

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 696

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+		if (!pte_present(*pte))
+			continue;
+
+		if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {

When would those addresses *not* be page-aligned? By construction, next must be. Unplugging partial pages of memory makes no sense, so surely addr is page-aligned when passed in?

Quoted 26 lines

arch/arm64/mm/mmu.c +26 -0

+			/*
+			 * Do not free direct mapping pages since they were
+			 * freed when offlining, or simplely not in use.
+			 */
+			if (!direct)
+				free_pagetable(pte_page(*pte), 0, direct);
+
+			/*
+			 * This spin lock could be only
+			 * taken in _pte_aloc_kernel in
+			 * mm/memory.c and nowhere else
+			 * (for arm64). Not sure if the
+			 * function above can be called
+			 * concurrently. In doubt,
+			 * I am living it here for now,
+			 * but it probably can be removed.
+			 */
+			spin_lock(&init_mm.page_table_lock);
+			pte_clear(&init_mm, addr, pte);
+			spin_unlock(&init_mm.page_table_lock);
+		} else {
+			/*
+			 * If we are here, we are freeing vmemmap pages since
+			 * direct mapped memory ranges to be freed are aligned.
+			 *
+			 * If we are not removing the whole page, it means

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 728

Quoted 6 lines

arch/arm64/mm/mmu.c +6 -0

+			 * other page structs in this page are being used and
+			 * we canot remove them. So fill the unused page_structs
+			 * with 0xFD, and remove the page when it is wholly
+			 * filled with 0xFD.
+			 */
+			memset((void *)addr, PAGE_INUSE, next - addr);

What's special about 0xFD?

Why do we need to mess with the page array in this manner? Why can't we detect when a range is free by querying memblock, for example?

Quoted 8 lines

arch/arm64/mm/mmu.c +8 -0

+
+			page_addr = page_address(pte_page(*pte));
+			if (!memchr_inv(page_addr, PAGE_INUSE, PAGE_SIZE)) {
+				free_pagetable(pte_page(*pte), 0, direct);
+
+				/*
+				 * This spin lock could be only
+				 * taken in _pte_aloc_kernel in

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 746

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+				 * mm/memory.c and nowhere else
+				 * (for arm64). Not sure if the
+				 * function above can be called
+				 * concurrently. In doubt,
+				 * I am living it here for now,
+				 * but it probably can be removed.
+				 */
+				spin_lock(&init_mm.page_table_lock);
+				pte_clear(&init_mm, addr, pte);
+				spin_unlock(&init_mm.page_table_lock);

This logic appears to be duplicated with the free_*_table functions, and looks incredibly suspicious.

To me, it doesn't make sense that we'd need the lock *only* to alter the leaf entries.

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 753

Quoted 7 lines

arch/arm64/mm/mmu.c +7 -0

+			}
+		}
+	}
+
+	// I am adding this flush here in simmetry to the x86 code.
+	// Why do I need to call it here and not in remove_p[mu]d
+	flush_tlb_all();

If the page tables weren't freed until this point, it might be that this is just amortizing the cost of removing them from the TLBs.

Given that they're freed first, this makes no sense to me.

Quoted 1 line

+}

The same commenst apply to all the remove_p*d_table() functions, so I shan't repeat them.

Quoted 12 lines

arch/arm64/mm/mmu.c +12 -0

+
+static void remove_pmd_table(pmd_t *pmd, unsigned long addr,
+	unsigned long end, bool direct)
+{
+	unsigned long next;
+	void *page_addr;
+	pte_t *pte;
+
+	for (; addr < end; addr = next, pmd++) {
+		next = pmd_addr_end(addr, end);
+
+		if (!pmd_present(*pmd))

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 771

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+			continue;
+
+		// check if we are using 2MB section mappings
+		if (pmd_sect(*pmd)) {
+			if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {

Surely you're intending to check if you can free the whole pmd? i.e. that addr and next are pmd-aligned?

Can we ever be in a situation where we're requested to free a partial pmd that could be section mapped?

If that's the case, we'll *need* to split the pmd, which we can't do on live page tables.

Quoted 31 lines

arch/arm64/mm/mmu.c +31 -0

+				if (!direct) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+				}
+				/*
+				 * This spin lock could be only
+				 * taken in _pte_aloc_kernel in
+				 * mm/memory.c and nowhere else
+				 * (for arm64). Not sure if the
+				 * function above can be called
+				 * concurrently. In doubt,
+				 * I am living it here for now,
+				 * but it probably can be removed.
+				 */
+				spin_lock(&init_mm.page_table_lock);
+				pmd_clear(pmd);
+				spin_unlock(&init_mm.page_table_lock);
+			} else {
+				/* If here, we are freeing vmemmap pages. */
+				memset((void *)addr, PAGE_INUSE, next - addr);
+
+				page_addr = page_address(pmd_page(*pmd));
+				if (!memchr_inv(page_addr, PAGE_INUSE,
+						PMD_SIZE)) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+
+					/*
+					 * This spin lock could be only
+					 * taken in _pte_aloc_kernel in
+					 * mm/memory.c and nowhere else

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 812

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+					 * (for arm64). Not sure if the
+					 * function above can be called
+					 * concurrently. In doubt,
+					 * I am living it here for now,
+					 * but it probably can be removed.
+					 */
+					spin_lock(&init_mm.page_table_lock);
+					pmd_clear(pmd);
+					spin_unlock(&init_mm.page_table_lock);
+				}

I don't think this is correct.

If we're getting rid of a partial pmd, we *must* split the pmd. Otherwise, we're leaving bits mapped that should not be. If we split the pmd, we can free the individual pages as we would for a non-section mapping.

As above, we can't split block entries within live tables, so that will be painful at best.

If we can't split a pmd, hen we cannot free a partial pmd, and will need to reject request to do so.

The same comments (with s/pmu/pud/, etc) apply for the higher level remove_p*d_table functions.

[...]

Quoted 14 lines

arch/arm64/mm/mmu.c +14 -0

+void remove_pagetable(unsigned long start, unsigned long end, bool direct)
+{
+	unsigned long next;
+	unsigned long addr;
+	pgd_t *pgd;
+	pud_t *pud;
+
+	for (addr = start; addr < end; addr = next) {
+		next = pgd_addr_end(addr, end);
+
+		pgd = pgd_offset_k(addr);
+		if (pgd_none(*pgd))
+			continue;
+

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 920

Quoted 10 lines

arch/arm64/mm/mmu.c +9 -0

+		pud = pud_offset(pgd, addr);
+		remove_pud_table(pud, addr, next, direct);
+		/*
+		 * When the PUD is folded on the PGD (three levels of paging),
+		 * I did already clear the PMD page in free_pmd_table,
+		 * and reset the corresponding PGD==PUD entry.
+		 */
+#if CONFIG_PGTABLE_LEVELS > 3
+		free_pud_table(pgd, direct);
 #endif

This looks suspicious. Shouldn't we have a similar check for PMD, to cater for CONFIG_PGTABLE_LEVELS == 2? e.g. 64K pages, 42-bit VA.

We should be able to hide this distinction in helpers for both cases.

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 753

Quoted 3 lines

arch/arm64/mm/mmu.c +3 -0

+	}
+
+	flush_tlb_all();

This is too late to be useful.

Thanks,
Mark.

I guess it is likely that I might have made assumptions that are true for x86_64 but do not hold for arm64. Whenever you feel this is the case, I would be really grateful if you could help identify them.

Quoted 4 lines · Andrea Reale

Quoted 2 lines

+static void free_pagetable(struct page *page, int order, bool direct)

Quoted 2 lines

This "direct" parameter needs a better name, and a description in a comment block above this function.

The name direct is inherited directly from the x86_64 hot remove code. It serves to distinguish if we are removing either a pagetable page that is mapping to the direct mapping space (I think it is called also linear mapping area somewhere else) or a pagetable page or a data page from vmemmap.

In this specific set of functions, the flag is needed because the various alloc_init_p*d used for allocating entries for direct memory mapping rely on pgd_pgtable_alloc, which in its turn calls pgtable_page_ctor; hence, we need to call the dtor.

On the contrary, vmemmap entries are created using vmemmap_alloc_block (from within vmemmap_populate), which does not call pgtable_page_ctor when allocating pages.

I am not sure I understand why the pgtable_page_ctor is not called when allocating vmemmap page tables, but that's the current situation.

From a quick scan, I see that it's necessary to use pgtable_page_ctor()

Quoted 29 lines

Quoted 29 lines

arch/arm64/mm/mmu.c +29 -0

+{
+	unsigned long magic;
+	unsigned int nr_pages = 1 << order;
+	struct vmem_altmap *altmap = to_vmem_altmap((unsigned long) page);
+
+	if (altmap) {
+		vmem_altmap_free(altmap, nr_pages);
+		return;
+	}
+
+	/* bootmem page has reserved flag */
+	if (PageReserved(page)) {
+		__ClearPageReserved(page);
+
+		magic = (unsigned long)page->lru.next;
+		if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) {
+			while (nr_pages--)
+				put_page_bootmem(page++);
+		} else {
+			while (nr_pages--)
+				free_reserved_page(page++);
+		}
+	} else {
+		/*
+		 * Only direct pagetable allocation (those allocated via
+		 * hotplug) call the pgtable_page_ctor; vmemmap pgtable
+		 * allocations don't.
+		 */
+		if (direct)

Quoted 8 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 583

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+			pgtable_page_dtor(page);
+
+		free_pages((unsigned long)page_address(page), order);
+	}
+}

This largely looks like a copy of the x86 code. Why can that not be factored out to an arch-neutral location?

Yes it probably can - the only difference being calling pgtable_page_dtor when it needs to - but I am not confident enough to say that it would really be architecture neutral or just specific to only arm64 and x86. For example, I don't see this used anywhere else for hot-removing memory.

(Actually, also a large part of remove_*_table and free_*_table could probably be factored, but I wouldn't be sure how to deal with the differences in the pgtable.h macros used throughout)

Quoted 21 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 601

Quoted 3 lines

arch/arm64/mm/mmu.c +3 -0

+	page = pmd_page(*pmd);
+
+	free_pagetable(page, 0, direct);

The page has been freed here, and may be subject to arbitrary modification...

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 610

Quoted 9 lines

arch/arm64/mm/mmu.c +9 -0

+
+	/*
+	 * This spin lock could be only taken in _pte_aloc_kernel
+	 * in mm/memory.c and nowhere else (for arm64). Not sure if
+	 * the function above can be called concurrently. In doubt,
+	 * I am living it here for now, but it probably can be removed
+	 */
+	spin_lock(&init_mm.page_table_lock);
+	pmd_clear(pmd);

... but we only remove it from the page tables here, so the page table walkers can see junk in the tables, were the page reused in the mean timer.

Quoted 3 lines

After clearing the PMD, it needs to be cleared from TLBs. We allow partial walks to be cached, so the TLBs may still start walking this entry or beyond.

I guess that the safe approach would be something along the lines:
1. clear the page table
2. flush the tlbs
3. free the page

When I am flushing intermediate p*d entries, would it be more appropriate to use something like __flush_tlb_pgtable() to clear cached partial walks rather than flushing the whole table? I mean, hot-remove is not going to be a frequent operation anyway, so I don't think that flushing the whole tlb would be a great deal of harm anyway.

My question at this point would be: how come that the code structure above works for x86_64 hot-remove?

My assumption, when I was writing this, was
that there would be no problem since this code is called when we are sure that all the memory mapped by these entries has been already offlined, so nobody should be using those VAs anyway (also considering that there cannot be any two mem hot-plug/remove actions running concurrently). Is that correct?

Quoted 5 lines

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+static void remove_pte_table(pte_t *pte, unsigned long addr,
+	unsigned long end, bool direct)
+{
+	unsigned long next;
+	void *page_addr;

Quoted 8 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 691

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+	for (; addr < end; addr = next, pte++) {
+		next = (addr + PAGE_SIZE) & PAGE_MASK;
+		if (next > end)
+			next = end;

Please use the usual p*d_addr_end() functions. See alloc_init_pmd() and friends in arch/arm64/mm/mmu.c for examples of how to use them.

we used the p*d_addr_end family of functions when dealing with p*d(s). I cannot identify an equivalent for pte entries.

Would you recommend adding a pte_addr_end macro in pgtable.h?

Quoted 9 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 696

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+		if (!pte_present(*pte))
+			continue;
+
+		if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {

When would those addresses *not* be page-aligned? By construction, next must be. Unplugging partial pages of memory makes no sense, so surely addr is page-aligned when passed in?

The issue here is that this function is called in one of two cases:
1. to clear pagetables of directly mapped (linear) memory
2. Pagetables (and corresponding data pages) for vmemmap.

It is my understanding that, in the second case, we might be clearing only part of the page content (i.e, only a few struct pages). Note that next is page aligned by construction only if next <= end.

Quoted 26 lines

Quoted 26 lines

arch/arm64/mm/mmu.c +26 -0

+			/*
+			 * Do not free direct mapping pages since they were
+			 * freed when offlining, or simplely not in use.
+			 */
+			if (!direct)
+				free_pagetable(pte_page(*pte), 0, direct);
+
+			/*
+			 * This spin lock could be only
+			 * taken in _pte_aloc_kernel in
+			 * mm/memory.c and nowhere else
+			 * (for arm64). Not sure if the
+			 * function above can be called
+			 * concurrently. In doubt,
+			 * I am living it here for now,
+			 * but it probably can be removed.
+			 */
+			spin_lock(&init_mm.page_table_lock);
+			pte_clear(&init_mm, addr, pte);
+			spin_unlock(&init_mm.page_table_lock);
+		} else {
+			/*
+			 * If we are here, we are freeing vmemmap pages since
+			 * direct mapped memory ranges to be freed are aligned.
+			 *
+			 * If we are not removing the whole page, it means

Quoted 8 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 728

Quoted 6 lines

arch/arm64/mm/mmu.c +6 -0

+			 * other page structs in this page are being used and
+			 * we canot remove them. So fill the unused page_structs
+			 * with 0xFD, and remove the page when it is wholly
+			 * filled with 0xFD.
+			 */
+			memset((void *)addr, PAGE_INUSE, next - addr);

What's special about 0xFD?

Just used it as a constant symmetrically to x86_64 code.

Quoted 2 lines

Why do we need to mess with the page array in this manner? Why can't we detect when a range is free by querying memblock, for example?

I am not sure I get your suggestion. I guess that the logic here is that I cannot be sure that I can free the full page because other entries might be in use for active vmemmap mappings. So we just "mark" the unused once and only free the page when all of it is marked. See again commit ae9aae9eda2db71, where all this comes from.

Quoted 11 lines

Quoted 11 lines

arch/arm64/mm/mmu.c +11 -0

+static void remove_pmd_table(pmd_t *pmd, unsigned long addr,
+	unsigned long end, bool direct)
+{
+	unsigned long next;
+	void *page_addr;
+	pte_t *pte;
+
+	for (; addr < end; addr = next, pmd++) {
+		next = pmd_addr_end(addr, end);
+
+		if (!pmd_present(*pmd))

Quoted 8 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 771

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+			continue;
+
+		// check if we are using 2MB section mappings
+		if (pmd_sect(*pmd)) {
+			if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {

Surely you're intending to check if you can free the whole pmd? i.e. that addr and next are pmd-aligned?

Indeed, that's a mistake. It should have been IS_ALIGNED(addr, PMD_SIZE).

Quoted 2 lines

Can we ever be in a situation where we're requested to free a partial pmd that could be section mapped?

Yes, as I said above, for vmemmap mappings.

Quoted 2 lines

If that's the case, we'll *need* to split the pmd, which we can't do on live page tables.

Please, see below.

Quoted 55 lines

Quoted 31 lines

arch/arm64/mm/mmu.c +31 -0

+				if (!direct) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+				}
+				/*
+				 * This spin lock could be only
+				 * taken in _pte_aloc_kernel in
+				 * mm/memory.c and nowhere else
+				 * (for arm64). Not sure if the
+				 * function above can be called
+				 * concurrently. In doubt,
+				 * I am living it here for now,
+				 * but it probably can be removed.
+				 */
+				spin_lock(&init_mm.page_table_lock);
+				pmd_clear(pmd);
+				spin_unlock(&init_mm.page_table_lock);
+			} else {
+				/* If here, we are freeing vmemmap pages. */
+				memset((void *)addr, PAGE_INUSE, next - addr);
+
+				page_addr = page_address(pmd_page(*pmd));
+				if (!memchr_inv(page_addr, PAGE_INUSE,
+						PMD_SIZE)) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+
+					/*
+					 * This spin lock could be only
+					 * taken in _pte_aloc_kernel in
+					 * mm/memory.c and nowhere else

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 812

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+					 * (for arm64). Not sure if the
+					 * function above can be called
+					 * concurrently. In doubt,
+					 * I am living it here for now,
+					 * but it probably can be removed.
+					 */
+					spin_lock(&init_mm.page_table_lock);
+					pmd_clear(pmd);
+					spin_unlock(&init_mm.page_table_lock);
+				}

I don't think this is correct.

If we're getting rid of a partial pmd, we *must* split the pmd. Otherwise, we're leaving bits mapped that should not be. If we split the pmd, we can free the individual pages as we would for a non-section mapping.

As above, we can't split block entries within live tables, so that will be painful at best.

If we can't split a pmd, hen we cannot free a partial pmd, and will need to reject request to do so.

Quoted 4 lines

The same comments (with s/pmu/pud/, etc) apply for the higher level remove_p*d_table functions.

[...]

This only happens when we are clearing vmemmap memory.

My understanding
is that the whole hack of marking the content of partially unused areas with the 0xFD constant is exactly to avoid splitting the PMD, but instead to only clear the full area when we realize that there's no valid struct page in it anymore. When would this kind of use be source of problems?

Quoted 3 lines · Andrea Reale

I guess it is likely that I might have made assumptions that are true for x86_64 but do not hold for arm64. Whenever you feel this is the case, I would be really grateful if you could help identify them.

Sure thing.

Quoted 5 lines · Mark Rutland

Quoted 4 lines · Andrea Reale

Quoted 2 lines

+static void free_pagetable(struct page *page, int order, bool direct)

Quoted 8 lines

Quoted 2 lines

This "direct" parameter needs a better name, and a description in a comment block above this function.

The name direct is inherited directly from the x86_64 hot remove code. It serves to distinguish if we are removing either a pagetable page that is mapping to the direct mapping space (I think it is called also linear mapping area somewhere else) or a pagetable page or a data page from vmemmap.

FWIW, I've largely heard the folk call that the "linear mapping", and x86 folk call that the "direct mapping". The two are interchangeable.

Quoted 4 lines

In this specific set of functions, the flag is needed because the various alloc_init_p*d used for allocating entries for direct memory mapping rely on pgd_pgtable_alloc, which in its turn calls pgtable_page_ctor; hence, we need to call the dtor.

AFAICT, that's not true for the arm64 linear map, since that's created with our early_pgtable_alloc(), which doesn't call pgtable_page_ctor().

Judging by commit:

1378dc3d4ba07ccd ("arm64: mm: run pgtable_page_ctor() on non-swapper
translation table pages")

... we only do this for non-swapper page tables.

Quoted 6 lines

On the contrary, vmemmap entries are created using vmemmap_alloc_block (from within vmemmap_populate), which does not call pgtable_page_ctor when allocating pages.

I am not sure I understand why the pgtable_page_ctor is not called when allocating vmemmap page tables, but that's the current situation.

From a quick scan, I see that it's necessary to use pgtable_page_ctor() for pages that will be used for userspace page tables, but it's not clear to me if it's ever necessary for pages used for kernel page tables.

If it is, we appear to have a bug on arm64.

Laura, Ard, thoughts?

Quoted 46 lines · Mark Rutland

Quoted 3 lines · Andrea Reale

I guess it is likely that I might have made assumptions that are true for x86_64 but do not hold for arm64. Whenever you feel this is the case, I would be really grateful if you could help identify them.

Sure thing.

Quoted 5 lines · Mark Rutland

Quoted 4 lines · Andrea Reale

Quoted 2 lines

+static void free_pagetable(struct page *page, int order, bool direct)

Quoted 8 lines

Quoted 2 lines

This "direct" parameter needs a better name, and a description in a comment block above this function.

The name direct is inherited directly from the x86_64 hot remove code. It serves to distinguish if we are removing either a pagetable page that is mapping to the direct mapping space (I think it is called also linear mapping area somewhere else) or a pagetable page or a data page from vmemmap.

FWIW, I've largely heard the folk call that the "linear mapping", and x86 folk call that the "direct mapping". The two are interchangeable.

Quoted 4 lines

In this specific set of functions, the flag is needed because the various alloc_init_p*d used for allocating entries for direct memory mapping rely on pgd_pgtable_alloc, which in its turn calls pgtable_page_ctor; hence, we need to call the dtor.

AFAICT, that's not true for the arm64 linear map, since that's created with our early_pgtable_alloc(), which doesn't call pgtable_page_ctor().

Judging by commit:

1378dc3d4ba07ccd ("arm64: mm: run pgtable_page_ctor() on non-swapper
translation table pages")

... we only do this for non-swapper page tables.

Quoted 6 lines

On the contrary, vmemmap entries are created using vmemmap_alloc_block (from within vmemmap_populate), which does not call pgtable_page_ctor when allocating pages.

I am not sure I understand why the pgtable_page_ctor is not called when allocating vmemmap page tables, but that's the current situation.

Quoted 9 lines

From a quick scan, I see that it's necessary to use pgtable_page_ctor()
for pages that will be used for userspace page tables, but it's not clear to me if it's ever necessary for pages used for kernel page tables.

If it is, we appear to have a bug on arm64.

Laura, Ard, thoughts?

The generic apply_to_page_range() will expect the PTE lock to be initialized for page table pages that are not part of init_mm. For arm64, that is precisely efi_mm as far as I am aware. For EFI, the locking is unnecessary but does no harm (the permissions are set once via apply_to_page_range() at boot), so I added this call when adding support for strict permissions in EFI rt services mappings.

So I think it is appropriate for create_pgd_mapping() to be in charge of calling the ctor(). We simply have no destroy_pgd_mapping() counterpart that would be the place for the dtor() call, given that we never take down EFI rt services mappi >
Whether it makes sense or not to lock/unlock in apply_to_page_range() is something I did not spend any brain cycles on at the time.

Quoted 6 lines

The name direct is inherited directly from the x86_64 hot remove code. It serves to distinguish if we are removing either a pagetable page that is mapping to the direct mapping space (I think it is called also linear mapping area somewhere else) or a pagetable page or a data page from vmemmap.

FWIW, I've largely heard the folk call that the "linear mapping", and x86 folk call that the "direct mapping". The two are interchangeable.

Quoted 4 lines

In this specific set of functions, the flag is needed because the various alloc_init_p*d used for allocating entries for direct memory mapping rely on pgd_pgtable_alloc, which in its turn calls pgtable_page_ctor; hence, we need to call the dtor.

AFAICT, that's not true for the arm64 linear map, since that's created with our early_pgtable_alloc(), which doesn't call pgtable_page_ctor().

Judging by commit:

1378dc3d4ba07ccd ("arm64: mm: run pgtable_page_ctor() on non-swapper
translation table pages")

... we only do this for non-swapper page tables.

Quoted 6 lines

On the contrary, vmemmap entries are created using vmemmap_alloc_block (from within vmemmap_populate), which does not call pgtable_page_ctor when allocating pages.

I am not sure I understand why the pgtable_page_ctor is not called when allocating vmemmap page tables, but that's the current situation.

From a quick scan, I see that it's necessary to use pgtable_page_ctor() for pages that will be used for userspace page tables, but it's not clear to me if it's ever necessary for pages used for kernel page tables.

If it is, we appear to have a bug on arm64.

Laura, Ard, thoughts?

Quoted 5 lines

I guess that the safe approach would be something along the lines:
1. clear the page table
2. flush the tlbs
3. free the page

Yes. That's the sequence we follow elsewhere.

Quoted 6 lines

When I am flushing intermediate p*d entries, would it be more appropriate to use something like __flush_tlb_pgtable() to clear cached partial walks rather than flushing the whole table? I mean, hot-remove is not going to be a frequent operation anyway, so I don't think that flushing the whole tlb would be a great deal of harm anyway.

Using __flush_tlb_pgtable() sounds sane to me. That's what we do when tearing down user mappings.

Quoted 2 lines

My question at this point would be: how come that the code structure above works for x86_64 hot-remove?

I don't know enough about x86 to say.

Quoted 6 lines

My assumption, when I was writing this, was
that there would be no problem since this code is called when we are sure that all the memory mapped by these entries has been already offlined, so nobody should be using those VAs anyway (also considering that there cannot be any two mem hot-plug/remove actions running concurrently). Is that correct?

The problem is that speculation, Out-of-Order execution, HW prefetching, and other such things *can* result in those VAs being accessed, regardless of what code explicitly does in a sequential execution.

If any table relevant to one of those VAs has been freed (and potentially modified by the allocator, or in another thread), it's possible that the CPU performs a page table walk and sees junk as a valid page table entry. As a result, a number of bad things can happen.

If the entry was a pgd, pud, or pmd, the CPU may try to continue to walk to the relevant pte, accessing a junk address. That could change the state of MMIO, trigger an SError, etc, or allocate more junk into the TLBs.

For any level, the CPU might allocate the entry into a TLB, regardless of whether an existing entry existed. The new entry can conflict with the existing one, either leading to a TLB conflict abort, or to the TLB returning junk for that address. Speculation and so on can now access junk based on that, etc.

[...]

Quoted 14 lines

Quoted 9 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 696

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+		if (!pte_present(*pte))
+			continue;
+
+		if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {

When would those addresses *not* be page-aligned? By construction, next must be. Unplugging partial pages of memory makes no sense, so surely addr is page-aligned when passed in?

The issue here is that this function is called in one of two cases:
1. to clear pagetables of directly mapped (linear) memory
2. Pagetables (and corresponding data pages) for vmemmap.

Quoted 3 lines

It is my understanding that, in the second case, we might be clearing only part of the page content (i.e, only a few struct pages). Note that next is page aligned by construction only if next <= end.

Ok. A comment to that effect immediately above this check would be helpful.

Quoted 37 lines

Quoted 26 lines

Quoted 26 lines

arch/arm64/mm/mmu.c +26 -0

+			/*
+			 * Do not free direct mapping pages since they were
+			 * freed when offlining, or simplely not in use.
+			 */
+			if (!direct)
+				free_pagetable(pte_page(*pte), 0, direct);
+
+			/*
+			 * This spin lock could be only
+			 * taken in _pte_aloc_kernel in
+			 * mm/memory.c and nowhere else
+			 * (for arm64). Not sure if the
+			 * function above can be called
+			 * concurrently. In doubt,
+			 * I am living it here for now,
+			 * but it probably can be removed.
+			 */
+			spin_lock(&init_mm.page_table_lock);
+			pte_clear(&init_mm, addr, pte);
+			spin_unlock(&init_mm.page_table_lock);
+		} else {
+			/*
+			 * If we are here, we are freeing vmemmap pages since
+			 * direct mapped memory ranges to be freed are aligned.
+			 *
+			 * If we are not removing the whole page, it means

Quoted 8 lines

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 728

Quoted 6 lines

arch/arm64/mm/mmu.c +6 -0

+			 * other page structs in this page are being used and
+			 * we canot remove them. So fill the unused page_structs
+			 * with 0xFD, and remove the page when it is wholly
+			 * filled with 0xFD.
+			 */
+			memset((void *)addr, PAGE_INUSE, next - addr);

What's special about 0xFD?

Just used it as a constant symmetrically to x86_64 code.

Quoted 8 lines

Quoted 2 lines

Why do we need to mess with the page array in this manner? Why can't we detect when a range is free by querying memblock, for example?

I am not sure I get your suggestion. I guess that the logic here is that I cannot be sure that I can free the full page because other entries might be in use for active vmemmap mappings. So we just "mark" the unused once and only free the page when all of it is marked. See again commit ae9aae9eda2db71, where all this comes from.

I understood that this is deferring freeing until a whole page of struct pages has been freed.

My concern is that filling the unused memory with an array of junk chars feels like a hack. We don't do this at the edges when we allocate memblock today, AFAICT, so this doesn't seem complete.

Is there no "real" datastructure we can use to keep track of what memory is present? e.g. memblock?

[...]

Quoted 60 lines

Quoted 2 lines

If that's the case, we'll *need* to split the pmd, which we can't do on live page tables.

Please, see below.

Quoted 55 lines

Quoted 31 lines

arch/arm64/mm/mmu.c +31 -0

+				if (!direct) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+				}
+				/*
+				 * This spin lock could be only
+				 * taken in _pte_aloc_kernel in
+				 * mm/memory.c and nowhere else
+				 * (for arm64). Not sure if the
+				 * function above can be called
+				 * concurrently. In doubt,
+				 * I am living it here for now,
+				 * but it probably can be removed.
+				 */
+				spin_lock(&init_mm.page_table_lock);
+				pmd_clear(pmd);
+				spin_unlock(&init_mm.page_table_lock);
+			} else {
+				/* If here, we are freeing vmemmap pages. */
+				memset((void *)addr, PAGE_INUSE, next - addr);
+
+				page_addr = page_address(pmd_page(*pmd));
+				if (!memchr_inv(page_addr, PAGE_INUSE,
+						PMD_SIZE)) {
+					free_pagetable(pmd_page(*pmd),
+						get_order(PMD_SIZE), direct);
+
+					/*
+					 * This spin lock could be only
+					 * taken in _pte_aloc_kernel in
+					 * mm/memory.c and nowhere else

arch/arm64/mm/mmu.c void hotplug_paging(phys_addr_t start, phys_addr_t size) · line 812

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+					 * (for arm64). Not sure if the
+					 * function above can be called
+					 * concurrently. In doubt,
+					 * I am living it here for now,
+					 * but it probably can be removed.
+					 */
+					spin_lock(&init_mm.page_table_lock);
+					pmd_clear(pmd);
+					spin_unlock(&init_mm.page_table_lock);
+				}

I don't think this is correct.

If we're getting rid of a partial pmd, we *must* split the pmd. Otherwise, we're leaving bits mapped that should not be. If we split the pmd, we can free the individual pages as we would for a non-section mapping.

As above, we can't split block entries within live tables, so that will be painful at best.

If we can't split a pmd, hen we cannot free a partial pmd, and will need to reject request to do so.

Quoted 6 lines

Quoted 4 lines

The same comments (with s/pmu/pud/, etc) apply for the higher level remove_p*d_table functions.

[...]

This only happens when we are clearing vmemmap memory.

Is that definitely the case?

Currently, I can't see what prevents adding 2M of memory, and then removing the first 4K of that. We'll use a 2M section for the linear map of that, but won't unmap the 4K when removing.

Likewise for the next level up, with s/2M/1G/ and s/4K/2M/.

Thanks,
Mark.

Quoted 2 lines · Ard Biesheuvel

Quoted 1 line · Mark Rutland

On Fri, Apr 14, 2017 at 03:01:58PM +0100, Andrea Reale wrote:

Quoted 18 lines

Quoted 10 lines

From a quick scan, I see that it's necessary to use pgtable_page_ctor() for pages that will be used for userspace page tables, but it's not clear to me if it's ever necessary for pages used for kernel page tables.

If it is, we appear to have a bug on arm64.

Laura, Ard, thoughts?

The generic apply_to_page_range() will expect the PTE lock to be initialized for page table pages that are not part of init_mm. For arm64, that is precisely efi_mm as far as I am aware. For EFI, the locking is unnecessary but does no harm (the permissions are set once via apply_to_page_range() at boot), so I added this call when adding support for strict permissions in EFI rt services mappings.

Quoted 7 lines

So I think it is appropriate for create_pgd_mapping() to be in charge of calling the ctor(). We simply have no destroy_pgd_mapping() counterpart that would be the place for the dtor() call, given that we never take down EFI rt services mappi >
Whether it makes sense or not to lock/unlock in apply_to_page_range() is something I did not spend any brain cycles on at the time.

Agreed there shouldn't be a problem right now. I do think the locking is appropriate in apply_to_page_range given what other functions also get locked.

I really wish this were less asymmetrical though since it get hard to reason about. It looks like hotplug_paging will call the ctor, so is there an issue with calling hot-remove on memory that was once hot-added or is that not a concern?

Thanks,
Laura

Hi,

Quoted 9 lines · Andrea Reale

From: Maciej Bielski <[email protected]>

This is a second and improved version of the patch previously released in [3].

It builds on the work by Scott Branden [2] and, henceforth, it needs to be applied on top of Scott's patches [2]. Comments are very welcome.

Quoted 8 lines

Changes from the original patchset and known issues:

- Compared to Scott's original patchset, this work adds the mapping of the new hotplugged pages into the kernel page tables. This is done by copying the old swapper_pg_dir over a new page, adding the new mappings, and then switching to the newly built pg_dir (see hotplug_paging in arch/arm64/mmu.c). There might be better ways to to this: suggestions are more than welcome.

For this reply, I'm just going to focus on the PGD replacement.

It's not clear to me if we need to swap the PGD, since everything we do here is strictly additive and non-conflicting, and it should be safe to add things to the swapper_pg_dir live.

However, assuming there's something I've missed, I have some comments below.

[...]

Quoted 5 lines

arch/arm64/mm/mmu.c +4 -0

+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables

arch/arm64/mm/mmu.c void __init paging_init(void) · line 533

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+ * for hot added memory.
+ */
+void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+
+	struct page *pg;
+	phys_addr_t pgd_phys = pgd_pgtable_alloc();
+	pgd_t *pgd = pgd_set_fixmap(pgd_phys);
+
+	memcpy(pgd, swapper_pg_dir, PAGE_SIZE);

s/PAGE_SIZE/PGD_SIZE/ (and below, too).

See commit 12f043ff2b28fa64 ("arm64: fix warning about swapper_pg_dir overflow").

arch/arm64/mm/mmu.c void __init paging_init(void) · line 538

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+	__create_pgd_mapping(pgd, start, __phys_to_virt(start), size,
+		PAGE_KERNEL, pgd_pgtable_alloc, false);
+
+	cpu_replace_ttbr1(__va(pgd_phys));

Other CPUs may be online at this point, and cpu_replace_ttbr1() was only intended for UP operation. Other CPUs will still have swapper_pg_dir in their TTBR1_EL1 at this point in time...

arch/arm64/mm/mmu.c void __init paging_init(void) · line 539

Quoted 1 line

arch/arm64/mm/mmu.c +1 -0

+	memcpy(swapper_pg_dir, pgd, PAGE_SIZE);

... which this will alter, in an unsafe fashion.

The other CPUs are live, and might be altering swapper. e.g. one might be using the fixmap to alter code. We will need some stop_machine()-like machinery here to synchronise with other CPUs, ensuring that they don't have swapper_pg_dir live.

Unfortunately, we can't change other to the temporary pgdir in a cross call, then fix things up, then do another cross call. If any exception is taken when we're in the temporary pgdir, __uaccess_ttbr0_disable will install junk into TTBR0, and we risk the usual set of pain junk TLBs entail.

We appear to have a latent bug at boot time along those lines, when setting up the page tables and initialising KASAN. I'll see about cleaning that up.

Thanks,
Mark.

Hi,

Quoted 9 lines · Andrea Reale

From: Maciej Bielski <[email protected]>

This is a second and improved version of the patch previously released in [3].

It builds on the work by Scott Branden [2] and, henceforth, it needs to be applied on top of Scott's patches [2]. Comments are very welcome.

Quoted 8 lines

Changes from the original patchset and known issues:

- Compared to Scott's original patchset, this work adds the mapping of the new hotplugged pages into the kernel page tables. This is done by copying the old swapper_pg_dir over a new page, adding the new mappings, and then switching to the newly built pg_dir (see hotplug_paging in arch/arm64/mmu.c). There might be better ways to to this: suggestions are more than welcome.

For this reply, I'm just going to focus on the PGD replacement.

It's not clear to me if we need to swap the PGD, since everything we do here is strictly additive and non-conflicting, and it should be safe to add things to the swapper_pg_dir live.

However, assuming there's something I've missed, I have some comments below.

[...]

Quoted 5 lines

arch/arm64/mm/mmu.c +4 -0

+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables

arch/arm64/mm/mmu.c void __init paging_init(void) · line 533

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+ * for hot added memory.
+ */
+void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+
+	struct page *pg;
+	phys_addr_t pgd_phys = pgd_pgtable_alloc();
+	pgd_t *pgd = pgd_set_fixmap(pgd_phys);
+
+	memcpy(pgd, swapper_pg_dir, PAGE_SIZE);

s/PAGE_SIZE/PGD_SIZE/ (and below, too).

See commit 12f043ff2b28fa64 ("arm64: fix warning about swapper_pg_dir overflow").

arch/arm64/mm/mmu.c void __init paging_init(void) · line 538

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+	__create_pgd_mapping(pgd, start, __phys_to_virt(start), size,
+		PAGE_KERNEL, pgd_pgtable_alloc, false);
+
+	cpu_replace_ttbr1(__va(pgd_phys));

Other CPUs may be online at this point, and cpu_replace_ttbr1() was only intended for UP operation. Other CPUs will still have swapper_pg_dir in their TTBR1_EL1 at this point in time...

arch/arm64/mm/mmu.c void __init paging_init(void) · line 539

Quoted 1 line

arch/arm64/mm/mmu.c +1 -0

+	memcpy(swapper_pg_dir, pgd, PAGE_SIZE);

... which this will alter, in an unsafe fashion.

The other CPUs are live, and might be altering swapper. e.g. one might be using the fixmap to alter code. We will need some stop_machine()-like machinery here to synchronise with other CPUs, ensuring that they don't have swapper_pg_dir live.

Unfortunately, we can't change other to the temporary pgdir in a cross call, then fix things up, then do another cross call. If any exception is taken when we're in the temporary pgdir, __uaccess_ttbr0_disable will install junk into TTBR0, and we risk the usual set of pain junk TLBs entail.

We appear to have a latent bug at boot time along those lines, when setting up the page tables and initialising KASAN. I'll see about cleaning that up.

Thanks,
Mark.

Hi all,

thanks for taking the time to comment. Replies in-line.

Quoted 3 lines · Laura Abbott

Quoted 2 lines · Ard Biesheuvel

Quoted 1 line · Mark Rutland

On Fri, Apr 14, 2017 at 03:01:58PM +0100, Andrea Reale wrote:

[...]

Quoted 30 lines

Quoted 18 lines

Quoted 10 lines

From a quick scan, I see that it's necessary to use pgtable_page_ctor() for pages that will be used for userspace page tables, but it's not clear to me if it's ever necessary for pages used for kernel page tables.

If it is, we appear to have a bug on arm64.

Laura, Ard, thoughts?

The generic apply_to_page_range() will expect the PTE lock to be initialized for page table pages that are not part of init_mm. For arm64, that is precisely efi_mm as far as I am aware. For EFI, the locking is unnecessary but does no harm (the permissions are set once via apply_to_page_range() at boot), so I added this call when adding support for strict permissions in EFI rt services mappings.

Quoted 7 lines

So I think it is appropriate for create_pgd_mapping() to be in charge of calling the ctor(). We simply have no destroy_pgd_mapping() counterpart that would be the place for the dtor() call, given that we never take down EFI rt services mappi >
Whether it makes sense or not to lock/unlock in apply_to_page_range() is something I did not spend any brain cycles on at the time.

Agreed there shouldn't be a problem right now. I do think the locking is appropriate in apply_to_page_range given what other functions also get locked.

Quoted 7 lines

I really wish this were less asymmetrical though since it get hard to reason about. It looks like hotplug_paging will call the ctor, so is there an issue with calling hot-remove on memory that was once hot-added or is that not a concern?

Thanks,
Laura

I think the confusion comes from the fact that, in hotplug_paging, we are passing pgd_pgtable_alloc as the page allocator for __create_pgd_mapping, which always calls the ctor.

If I got things right (but, please, correct me if I am wrong), we don't need to get the pte_lock that the ctor gets since - in hotplug - we are adding to init_mm.

Moreover, I am just realizing that calling the dtor while hot-removing might create problems when removing memory that *was not* previously hotplugged, as we are calling a dtor on something that was never ctor'ed. Is that what you were hinting at, Laura?

Thanks and best regards,
Andrea