[PATCH v2 0/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[PATCH v2 0/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Dan Williams]

Changes since v1 [1]:
* Introduce _safe versions of the set_pte family of helpers (Dave)
* Fix the validation check to accept rewriting the same entry (Peter)
* Fix up the email reference links in the changelogs (Peter)

[1]: https://lkml.org/lkml/2018/11/20/300

---

>From patch 5:

    Commit f77084d96355 "x86/mm/pat: Disable preemption around
    __flush_tlb_all()" addressed a case where __flush_tlb_all() is called
    without preemption being disabled. It also left a warning to catch other
    cases where preemption is not disabled. That warning triggers for the
    memory hotplug path which is also used for persistent memory enabling:
    
     WARNING: CPU: 35 PID: 911 at ./arch/x86/include/asm/tlbflush.h:460
     RIP: 0010:__flush_tlb_all+0x1b/0x3a
     [..]
     Call Trace:
      phys_pud_init+0x29c/0x2bb
      kernel_physical_mapping_init+0xfc/0x219
      init_memory_mapping+0x1a5/0x3b0
      arch_add_memory+0x2c/0x50
      devm_memremap_pages+0x3aa/0x610
      pmem_attach_disk+0x585/0x700 [nd_pmem]
    
    Andy wondered why a path that can sleep was using __flush_tlb_all() [1]
    and Dave confirmed the expectation for TLB flush is for modifying /
    invalidating existing pte entries, but not initial population [2]. Drop
    the usage of __flush_tlb_all() in phys_{p4d,pud,pmd}_init() on the
    expectation that this path is only ever populating empty entries for the
    linear map. Note, at linear map teardown time there is a call to the
    all-cpu flush_tlb_all() to invalidate the removed mappings.

Additionally, Dave wanted some runtime assurances that
kernel_physical_mapping_init() is only populating and not changing
existing page table entries. Patches 1 - 4 are implementing a new
set_pte() family of helpers for that purpose.

Patch 5 is tagged for -stable because the false positive warning is now
showing up on 4.19-stable kernels. Patches 1 - 4 are not tagged for
-stable, but if the sanity checking is needed please mark them as such.

The hang that was observed while developing the sanity checking
implementation was resolved by Peter's suggestion to not trigger when
the same pte value is being rewritten.

---

Dan Williams (5):
      generic/pgtable: Make {pmd,pud}_same() unconditionally available
      generic/pgtable: Introduce {p4d,pgd}_same()
      generic/pgtable: Introduce set_pte_safe()
      x86/mm: Validate kernel_physical_mapping_init() pte population
      x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()


 arch/x86/include/asm/pgalloc.h           |   27 +++++++++++++++
 arch/x86/mm/init_64.c                    |   30 +++++++----------
 include/asm-generic/5level-fixup.h       |    1 +
 include/asm-generic/pgtable-nop4d-hack.h |    1 +
 include/asm-generic/pgtable-nop4d.h      |    1 +
 include/asm-generic/pgtable-nopud.h      |    1 +
 include/asm-generic/pgtable.h            |   53 +++++++++++++++++++++++++-----
 7 files changed, 87 insertions(+), 27 deletions(-)

[PATCH v2 0/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Dan Williams]

Changes since v1 [1]:
* Introduce _safe versions of the set_pte family of helpers (Dave)
* Fix the validation check to accept rewriting the same entry (Peter)
* Fix up the email reference links in the changelogs (Peter)

[1]: https://lkml.org/lkml/2018/11/20/300

---

>>From patch 5:

    Commit f77084d96355 "x86/mm/pat: Disable preemption around
    __flush_tlb_all()" addressed a case where __flush_tlb_all() is called
    without preemption being disabled. It also left a warning to catch other
    cases where preemption is not disabled. That warning triggers for the
    memory hotplug path which is also used for persistent memory enabling:
    
     WARNING: CPU: 35 PID: 911 at ./arch/x86/include/asm/tlbflush.h:460
     RIP: 0010:__flush_tlb_all+0x1b/0x3a
     [..]
     Call Trace:
      phys_pud_init+0x29c/0x2bb
      kernel_physical_mapping_init+0xfc/0x219
      init_memory_mapping+0x1a5/0x3b0
      arch_add_memory+0x2c/0x50
      devm_memremap_pages+0x3aa/0x610
      pmem_attach_disk+0x585/0x700 [nd_pmem]
    
    Andy wondered why a path that can sleep was using __flush_tlb_all() [1]
    and Dave confirmed the expectation for TLB flush is for modifying /
    invalidating existing pte entries, but not initial population [2]. Drop
    the usage of __flush_tlb_all() in phys_{p4d,pud,pmd}_init() on the
    expectation that this path is only ever populating empty entries for the
    linear map. Note, at linear map teardown time there is a call to the
    all-cpu flush_tlb_all() to invalidate the removed mappings.

Additionally, Dave wanted some runtime assurances that
kernel_physical_mapping_init() is only populating and not changing
existing page table entries. Patches 1 - 4 are implementing a new
set_pte() family of helpers for that purpose.

Patch 5 is tagged for -stable because the false positive warning is now
showing up on 4.19-stable kernels. Patches 1 - 4 are not tagged for
-stable, but if the sanity checking is needed please mark them as such.

The hang that was observed while developing the sanity checking
implementation was resolved by Peter's suggestion to not trigger when
the same pte value is being rewritten.

---

Dan Williams (5):
      generic/pgtable: Make {pmd,pud}_same() unconditionally available
      generic/pgtable: Introduce {p4d,pgd}_same()
      generic/pgtable: Introduce set_pte_safe()
      x86/mm: Validate kernel_physical_mapping_init() pte population
      x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()


 arch/x86/include/asm/pgalloc.h           |   27 +++++++++++++++
 arch/x86/mm/init_64.c                    |   30 +++++++----------
 include/asm-generic/5level-fixup.h       |    1 +
 include/asm-generic/pgtable-nop4d-hack.h |    1 +
 include/asm-generic/pgtable-nop4d.h      |    1 +
 include/asm-generic/pgtable-nopud.h      |    1 +
 include/asm-generic/pgtable.h            |   53 +++++++++++++++++++++++++-----
 7 files changed, 87 insertions(+), 27 deletions(-)

[PATCH v2 1/5] generic/pgtable: Make {pmd, pud}_same() unconditionally available

[Dan Williams]

In preparation for {pmd,pud}_same() to be used outside of transparent
huge page code paths, make them unconditionally available. This enables
them to be used in the definition of a new family of
set_{pte,pmd,pud,p4d,pgd}_safe() helpers.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/asm-generic/pgtable.h |   14 --------------
 1 file changed, 14 deletions(-)

diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 359fb935ded6..eea50ef8b8cd 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -375,7 +375,6 @@ static inline int pte_unused(pte_t pte)
 #endif
 
 #ifndef __HAVE_ARCH_PMD_SAME
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static inline int pmd_same(pmd_t pmd_a, pmd_t pmd_b)
 {
 	return pmd_val(pmd_a) == pmd_val(pmd_b);
@@ -385,19 +384,6 @@ static inline int pud_same(pud_t pud_a, pud_t pud_b)
 {
 	return pud_val(pud_a) == pud_val(pud_b);
 }
-#else /* CONFIG_TRANSPARENT_HUGEPAGE */
-static inline int pmd_same(pmd_t pmd_a, pmd_t pmd_b)
-{
-	BUILD_BUG();
-	return 0;
-}
-
-static inline int pud_same(pud_t pud_a, pud_t pud_b)
-{
-	BUILD_BUG();
-	return 0;
-}
-#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 #endif
 
 #ifndef __HAVE_ARCH_DO_SWAP_PAGE

[PATCH v2 2/5] generic/pgtable: Introduce {p4d,pgd}_same()

[Dan Williams]

In preparation for introducing '_safe' versions of page table entry 'set'
helpers, introduce generic versions of p4d_same() and pgd_same().

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/asm-generic/pgtable.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index eea50ef8b8cd..dae7f98babed 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -386,6 +386,20 @@ static inline int pud_same(pud_t pud_a, pud_t pud_b)
 }
 #endif
 
+#ifndef __HAVE_ARCH_P4D_SAME
+static inline int p4d_same(p4d_t p4d_a, p4d_t p4d_b)
+{
+	return p4d_val(p4d_a) == p4d_val(p4d_b);
+}
+#endif
+
+#ifndef __HAVE_ARCH_PGD_SAME
+static inline int pgd_same(pgd_t pgd_a, pgd_t pgd_b)
+{
+	return pgd_val(pgd_a) == pgd_val(pgd_b);
+}
+#endif
+
 #ifndef __HAVE_ARCH_DO_SWAP_PAGE
 /*
  * Some architectures support metadata associated with a page. When a

[PATCH v2 3/5] generic/pgtable: Introduce set_pte_safe()

[Dan Williams]

Commit f77084d96355 "x86/mm/pat: Disable preemption around
__flush_tlb_all()" introduced a warning to capture cases
__flush_tlb_all() is called without pre-emption disabled. It triggers a
false positive warning in the memory hotplug path. On investigation it
was found that the __flush_tlb_all() calls are not necessary. However,
they are only "not necessary" in practice provided the ptes are being
initially populated from the !present state. Introduce set_pte_safe() as
a sanity check that the pte is being updated in a way that does not
require a tlb flush.

Forgive the macro, the availability of the various of set_pte() levels
is hit and miss across architectures.

Link: https://lore.kernel.org/lkml/279dadae-9148-465c-7ec6-3f37e026c6c9@intel.com
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/asm-generic/pgtable.h |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index dae7f98babed..62be0d5e1a9a 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -400,6 +400,41 @@ static inline int pgd_same(pgd_t pgd_a, pgd_t pgd_b)
 }
 #endif
 
+/*
+ * The _safe versions of set_{pte,pmd,pud,p4d,pgd} validate that the
+ * entry was not populated previously. I.e. for cases where a flush-tlb
+ * is elided, double-check that there is no stale mapping to shoot down.
+ */
+#define set_pte_safe(ptep, pte) \
+({ \
+	WARN_ON_ONCE(pte_present(*ptep) && !pte_same(*ptep, pte)); \
+	set_pte(ptep, pte); \
+})
+
+#define set_pmd_safe(pmdp, pmd) \
+({ \
+	WARN_ON_ONCE(pmd_present(*pmdp) && !pmd_same(*pmdp, pmd)); \
+	set_pmd(pmdp, pmd); \
+})
+
+#define set_pud_safe(pudp, pud) \
+({ \
+	WARN_ON_ONCE(pud_present(*pudp) && !pud_same(*pudp, pud)); \
+	set_pud(pudp, pud); \
+})
+
+#define set_p4d_safe(p4dp, p4d) \
+({ \
+	WARN_ON_ONCE(p4d_present(*p4dp) && !p4d_same(*p4dp, p4d)); \
+	set_p4d(p4dp, p4d); \
+})
+
+#define set_pgd_safe(pgdp, pgd) \
+({ \
+	WARN_ON_ONCE(pgd_present(*pgdp) && !pgd_same(*pgdp, pgd)); \
+	set_pgd(pgdp, pgd); \
+})
+
 #ifndef __HAVE_ARCH_DO_SWAP_PAGE
 /*
  * Some architectures support metadata associated with a page. When a

Re: [PATCH v2 3/5] generic/pgtable: Introduce set_pte_safe()

[Dave Hansen]

On 11/30/18 4:35 PM, Dan Williams wrote:
> +/*
> + * The _safe versions of set_{pte,pmd,pud,p4d,pgd} validate that the
> + * entry was not populated previously. I.e. for cases where a flush-tlb
> + * is elided, double-check that there is no stale mapping to shoot down.
> + */

Functionally these look great to me.

The only thing I'd suggest is to make the comment more about when to use
these, instead of what they do:

	Use the set_p*_safe() version when confident that *no*
	TLB flush will be required as a result of the "set", such
	as setting non-present entries or when possibly superfluously
	re-setting an entry.

Re: [PATCH v2 3/5] generic/pgtable: Introduce set_pte_safe()

[Dan Williams]

On Mon, Dec 3, 2018 at 9:53 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 11/30/18 4:35 PM, Dan Williams wrote:
> > +/*
> > + * The _safe versions of set_{pte,pmd,pud,p4d,pgd} validate that the
> > + * entry was not populated previously. I.e. for cases where a flush-tlb
> > + * is elided, double-check that there is no stale mapping to shoot down.
> > + */
>
> Functionally these look great to me.
>
> The only thing I'd suggest is to make the comment more about when to use
> these, instead of what they do:
>
>         Use the set_p*_safe() version when confident that *no*
>         TLB flush will be required as a result of the "set", such
>         as setting non-present entries or when possibly superfluously
>         re-setting an entry.

The second sentence was meant to be a "why", but yes, it's entirely too subtle.

[PATCH v2 4/5] x86/mm: Validate kernel_physical_mapping_init() pte population

[Dan Williams]

The usage of __flush_tlb_all() in the kernel_physical_mapping_init()
path is not necessary. In general flushing the tlb is not required when
updating an entry from the !present state. However, to give confidence
in the future removal of tlb flushing in this path, use the new
set_pte_safe() family of helpers to assert that the !present assumption
is true in this path.

Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/include/asm/pgalloc.h           |   27 +++++++++++++++++++++++++++
 arch/x86/mm/init_64.c                    |   24 ++++++++++++------------
 include/asm-generic/5level-fixup.h       |    1 +
 include/asm-generic/pgtable-nop4d-hack.h |    1 +
 include/asm-generic/pgtable-nop4d.h      |    1 +
 include/asm-generic/pgtable-nopud.h      |    1 +
 6 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/pgalloc.h b/arch/x86/include/asm/pgalloc.h
index ec7f43327033..1ea41aaef68b 100644
--- a/arch/x86/include/asm/pgalloc.h
+++ b/arch/x86/include/asm/pgalloc.h
@@ -80,6 +80,13 @@ static inline void pmd_populate_kernel(struct mm_struct *mm,
 	set_pmd(pmd, __pmd(__pa(pte) | _PAGE_TABLE));
 }
 
+static inline void pmd_populate_kernel_safe(struct mm_struct *mm,
+				       pmd_t *pmd, pte_t *pte)
+{
+	paravirt_alloc_pte(mm, __pa(pte) >> PAGE_SHIFT);
+	set_pmd_safe(pmd, __pmd(__pa(pte) | _PAGE_TABLE));
+}
+
 static inline void pmd_populate(struct mm_struct *mm, pmd_t *pmd,
 				struct page *pte)
 {
@@ -132,6 +139,12 @@ static inline void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
 	paravirt_alloc_pmd(mm, __pa(pmd) >> PAGE_SHIFT);
 	set_pud(pud, __pud(_PAGE_TABLE | __pa(pmd)));
 }
+
+static inline void pud_populate_safe(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
+{
+	paravirt_alloc_pmd(mm, __pa(pmd) >> PAGE_SHIFT);
+	set_pud_safe(pud, __pud(_PAGE_TABLE | __pa(pmd)));
+}
 #endif	/* CONFIG_X86_PAE */
 
 #if CONFIG_PGTABLE_LEVELS > 3
@@ -141,6 +154,12 @@ static inline void p4d_populate(struct mm_struct *mm, p4d_t *p4d, pud_t *pud)
 	set_p4d(p4d, __p4d(_PAGE_TABLE | __pa(pud)));
 }
 
+static inline void p4d_populate_safe(struct mm_struct *mm, p4d_t *p4d, pud_t *pud)
+{
+	paravirt_alloc_pud(mm, __pa(pud) >> PAGE_SHIFT);
+	set_p4d_safe(p4d, __p4d(_PAGE_TABLE | __pa(pud)));
+}
+
 static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
@@ -173,6 +192,14 @@ static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, p4d_t *p4d)
 	set_pgd(pgd, __pgd(_PAGE_TABLE | __pa(p4d)));
 }
 
+static inline void pgd_populate_safe(struct mm_struct *mm, pgd_t *pgd, p4d_t *p4d)
+{
+	if (!pgtable_l5_enabled())
+		return;
+	paravirt_alloc_p4d(mm, __pa(p4d) >> PAGE_SHIFT);
+	set_pgd_safe(pgd, __pgd(_PAGE_TABLE | __pa(p4d)));
+}
+
 static inline p4d_t *p4d_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 5fab264948c2..3e25ac2793ef 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -432,7 +432,7 @@ phys_pte_init(pte_t *pte_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & PAGE_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
-				set_pte(pte, __pte(0));
+				set_pte_safe(pte, __pte(0));
 			continue;
 		}
 
@@ -452,7 +452,7 @@ phys_pte_init(pte_t *pte_page, unsigned long paddr, unsigned long paddr_end,
 			pr_info("   pte=%p addr=%lx pte=%016lx\n", pte, paddr,
 				pfn_pte(paddr >> PAGE_SHIFT, PAGE_KERNEL).pte);
 		pages++;
-		set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
+		set_pte_safe(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
 		paddr_last = (paddr & PAGE_MASK) + PAGE_SIZE;
 	}
 
@@ -487,7 +487,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & PMD_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
-				set_pmd(pmd, __pmd(0));
+				set_pmd_safe(pmd, __pmd(0));
 			continue;
 		}
 
@@ -524,7 +524,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
 		if (page_size_mask & (1<<PG_LEVEL_2M)) {
 			pages++;
 			spin_lock(&init_mm.page_table_lock);
-			set_pte((pte_t *)pmd,
+			set_pte_safe((pte_t *)pmd,
 				pfn_pte((paddr & PMD_MASK) >> PAGE_SHIFT,
 					__pgprot(pgprot_val(prot) | _PAGE_PSE)));
 			spin_unlock(&init_mm.page_table_lock);
@@ -536,7 +536,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
 		paddr_last = phys_pte_init(pte, paddr, paddr_end, new_prot);
 
 		spin_lock(&init_mm.page_table_lock);
-		pmd_populate_kernel(&init_mm, pmd, pte);
+		pmd_populate_kernel_safe(&init_mm, pmd, pte);
 		spin_unlock(&init_mm.page_table_lock);
 	}
 	update_page_count(PG_LEVEL_2M, pages);
@@ -573,7 +573,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & PUD_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
-				set_pud(pud, __pud(0));
+				set_pud_safe(pud, __pud(0));
 			continue;
 		}
 
@@ -611,7 +611,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 		if (page_size_mask & (1<<PG_LEVEL_1G)) {
 			pages++;
 			spin_lock(&init_mm.page_table_lock);
-			set_pte((pte_t *)pud,
+			set_pte_safe((pte_t *)pud,
 				pfn_pte((paddr & PUD_MASK) >> PAGE_SHIFT,
 					PAGE_KERNEL_LARGE));
 			spin_unlock(&init_mm.page_table_lock);
@@ -624,7 +624,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 					   page_size_mask, prot);
 
 		spin_lock(&init_mm.page_table_lock);
-		pud_populate(&init_mm, pud, pmd);
+		pud_populate_safe(&init_mm, pud, pmd);
 		spin_unlock(&init_mm.page_table_lock);
 	}
 	__flush_tlb_all();
@@ -659,7 +659,7 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & P4D_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
-				set_p4d(p4d, __p4d(0));
+				set_p4d_safe(p4d, __p4d(0));
 			continue;
 		}
 
@@ -677,7 +677,7 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 					   page_size_mask);
 
 		spin_lock(&init_mm.page_table_lock);
-		p4d_populate(&init_mm, p4d, pud);
+		p4d_populate_safe(&init_mm, p4d, pud);
 		spin_unlock(&init_mm.page_table_lock);
 	}
 	__flush_tlb_all();
@@ -723,9 +723,9 @@ kernel_physical_mapping_init(unsigned long paddr_start,
 
 		spin_lock(&init_mm.page_table_lock);
 		if (pgtable_l5_enabled())
-			pgd_populate(&init_mm, pgd, p4d);
+			pgd_populate_safe(&init_mm, pgd, p4d);
 		else
-			p4d_populate(&init_mm, p4d_offset(pgd, vaddr), (pud_t *) p4d);
+			p4d_populate_safe(&init_mm, p4d_offset(pgd, vaddr), (pud_t *) p4d);
 		spin_unlock(&init_mm.page_table_lock);
 		pgd_changed = true;
 	}
diff --git a/include/asm-generic/5level-fixup.h b/include/asm-generic/5level-fixup.h
index 73474bb52344..bb6cb347018c 100644
--- a/include/asm-generic/5level-fixup.h
+++ b/include/asm-generic/5level-fixup.h
@@ -26,6 +26,7 @@
 #define p4d_clear(p4d)			pgd_clear(p4d)
 #define p4d_val(p4d)			pgd_val(p4d)
 #define p4d_populate(mm, p4d, pud)	pgd_populate(mm, p4d, pud)
+#define p4d_populate_safe(mm, p4d, pud)	pgd_populate(mm, p4d, pud)
 #define p4d_page(p4d)			pgd_page(p4d)
 #define p4d_page_vaddr(p4d)		pgd_page_vaddr(p4d)
 
diff --git a/include/asm-generic/pgtable-nop4d-hack.h b/include/asm-generic/pgtable-nop4d-hack.h
index 1d6dd38c0e5e..829bdb0d6327 100644
--- a/include/asm-generic/pgtable-nop4d-hack.h
+++ b/include/asm-generic/pgtable-nop4d-hack.h
@@ -31,6 +31,7 @@ static inline void pgd_clear(pgd_t *pgd)	{ }
 #define pud_ERROR(pud)				(pgd_ERROR((pud).pgd))
 
 #define pgd_populate(mm, pgd, pud)		do { } while (0)
+#define pgd_populate_safe(mm, pgd, pud)		do { } while (0)
 /*
  * (puds are folded into pgds so this doesn't get actually called,
  * but the define is needed for a generic inline function.)
diff --git a/include/asm-generic/pgtable-nop4d.h b/include/asm-generic/pgtable-nop4d.h
index 04cb913797bc..aebab905e6cd 100644
--- a/include/asm-generic/pgtable-nop4d.h
+++ b/include/asm-generic/pgtable-nop4d.h
@@ -26,6 +26,7 @@ static inline void pgd_clear(pgd_t *pgd)	{ }
 #define p4d_ERROR(p4d)				(pgd_ERROR((p4d).pgd))
 
 #define pgd_populate(mm, pgd, p4d)		do { } while (0)
+#define pgd_populate_safe(mm, pgd, p4d)		do { } while (0)
 /*
  * (p4ds are folded into pgds so this doesn't get actually called,
  * but the define is needed for a generic inline function.)
diff --git a/include/asm-generic/pgtable-nopud.h b/include/asm-generic/pgtable-nopud.h
index 9bef475db6fe..c77a1d301155 100644
--- a/include/asm-generic/pgtable-nopud.h
+++ b/include/asm-generic/pgtable-nopud.h
@@ -35,6 +35,7 @@ static inline void p4d_clear(p4d_t *p4d)	{ }
 #define pud_ERROR(pud)				(p4d_ERROR((pud).p4d))
 
 #define p4d_populate(mm, p4d, pud)		do { } while (0)
+#define p4d_populate_safe(mm, p4d, pud)		do { } while (0)
 /*
  * (puds are folded into p4ds so this doesn't get actually called,
  * but the define is needed for a generic inline function.)

[PATCH v2 5/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Dan Williams]

Commit f77084d96355 "x86/mm/pat: Disable preemption around
__flush_tlb_all()" addressed a case where __flush_tlb_all() is called
without preemption being disabled. It also left a warning to catch other
cases where preemption is not disabled. That warning triggers for the
memory hotplug path which is also used for persistent memory enabling:

 WARNING: CPU: 35 PID: 911 at ./arch/x86/include/asm/tlbflush.h:460
 RIP: 0010:__flush_tlb_all+0x1b/0x3a
 [..]
 Call Trace:
  phys_pud_init+0x29c/0x2bb
  kernel_physical_mapping_init+0xfc/0x219
  init_memory_mapping+0x1a5/0x3b0
  arch_add_memory+0x2c/0x50
  devm_memremap_pages+0x3aa/0x610
  pmem_attach_disk+0x585/0x700 [nd_pmem]

Andy wondered why a path that can sleep was using __flush_tlb_all() [1]
and Dave confirmed the expectation for TLB flush is for modifying /
invalidating existing pte entries, but not initial population [2]. Drop
the usage of __flush_tlb_all() in phys_{p4d,pud,pmd}_init() on the
expectation that this path is only ever populating empty entries for the
linear map. Note, at linear map teardown time there is a call to the
all-cpu flush_tlb_all() to invalidate the removed mappings.

[1]: https://lore.kernel.org/lkml/9DFD717D-857D-493D-A606-B635D72BAC21@amacapital.net
[2]: https://lore.kernel.org/lkml/749919a4-cdb1-48a3-adb4-adb81a5fa0b5@intel.com

Fixes: f77084d96355 ("x86/mm/pat: Disable preemption around __flush_tlb_all()")
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: <stable@vger.kernel.org>
Reported-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/mm/init_64.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 3e25ac2793ef..484c1b92f078 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -584,7 +584,6 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 							   paddr_end,
 							   page_size_mask,
 							   prot);
-				__flush_tlb_all();
 				continue;
 			}
 			/*
@@ -627,7 +626,6 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 		pud_populate_safe(&init_mm, pud, pmd);
 		spin_unlock(&init_mm.page_table_lock);
 	}
-	__flush_tlb_all();
 
 	update_page_count(PG_LEVEL_1G, pages);
 
@@ -668,7 +666,6 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 			paddr_last = phys_pud_init(pud, paddr,
 					paddr_end,
 					page_size_mask);
-			__flush_tlb_all();
 			continue;
 		}
 
@@ -680,7 +677,6 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 		p4d_populate_safe(&init_mm, p4d, pud);
 		spin_unlock(&init_mm.page_table_lock);
 	}
-	__flush_tlb_all();
 
 	return paddr_last;
 }
@@ -733,8 +729,6 @@ kernel_physical_mapping_init(unsigned long paddr_start,
 	if (pgd_changed)
 		sync_global_pgds(vaddr_start, vaddr_end - 1);
 
-	__flush_tlb_all();
-
 	return paddr_last;
 }
 

Re: [PATCH v2 5/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Peter Zijlstra]

A small nit:

On Fri, Nov 30, 2018 at 04:35:32PM -0800, Dan Williams wrote:

> [1]: https://lore.kernel.org/lkml/9DFD717D-857D-493D-A606-B635D72BAC21@amacapital.net
> [2]: https://lore.kernel.org/lkml/749919a4-cdb1-48a3-adb4-adb81a5fa0b5@intel.com

I much prefer the form:

  https://lkml.kernel.org/r/${msgid}

over the direct lore links. Luckily both contain the full msgid which is
the important part.

(I still think lore has a horrible UI)

Re: [PATCH v2 5/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Sasha Levin]

On Fri, Nov 30, 2018 at 04:35:32PM -0800, Dan Williams wrote:
>Commit f77084d96355 "x86/mm/pat: Disable preemption around
>__flush_tlb_all()" addressed a case where __flush_tlb_all() is called
>without preemption being disabled. It also left a warning to catch other
>cases where preemption is not disabled. That warning triggers for the
>memory hotplug path which is also used for persistent memory enabling:
>
> WARNING: CPU: 35 PID: 911 at ./arch/x86/include/asm/tlbflush.h:460
> RIP: 0010:__flush_tlb_all+0x1b/0x3a
> [..]
> Call Trace:
>  phys_pud_init+0x29c/0x2bb
>  kernel_physical_mapping_init+0xfc/0x219
>  init_memory_mapping+0x1a5/0x3b0
>  arch_add_memory+0x2c/0x50
>  devm_memremap_pages+0x3aa/0x610
>  pmem_attach_disk+0x585/0x700 [nd_pmem]
>
>Andy wondered why a path that can sleep was using __flush_tlb_all() [1]
>and Dave confirmed the expectation for TLB flush is for modifying /
>invalidating existing pte entries, but not initial population [2]. Drop
>the usage of __flush_tlb_all() in phys_{p4d,pud,pmd}_init() on the
>expectation that this path is only ever populating empty entries for the
>linear map. Note, at linear map teardown time there is a call to the
>all-cpu flush_tlb_all() to invalidate the removed mappings.
>
>[1]: https://lore.kernel.org/lkml/9DFD717D-857D-493D-A606-B635D72BAC21@amacapital.net
>[2]: https://lore.kernel.org/lkml/749919a4-cdb1-48a3-adb4-adb81a5fa0b5@intel.com
>
>Fixes: f77084d96355 ("x86/mm/pat: Disable preemption around __flush_tlb_all()")
>Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: Peter Zijlstra <peterz@infradead.org>
>Cc: Borislav Petkov <bp@alien8.de>
>Cc: <stable@vger.kernel.org>
>Reported-by: Andy Lutomirski <luto@kernel.org>
>Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hi Dan,

This patch on it's own doesn't apply to any of the stable trees, does it
maybe depend on some of the previous patches in this series?

--
Thanks,
Sasha

Re: [PATCH v2 5/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Dan Williams]

On Sat, Dec 1, 2018 at 10:43 PM Sasha Levin <sashal@kernel.org> wrote:
>
> On Fri, Nov 30, 2018 at 04:35:32PM -0800, Dan Williams wrote:
> >Commit f77084d96355 "x86/mm/pat: Disable preemption around
> >__flush_tlb_all()" addressed a case where __flush_tlb_all() is called
> >without preemption being disabled. It also left a warning to catch other
> >cases where preemption is not disabled. That warning triggers for the
> >memory hotplug path which is also used for persistent memory enabling:
> >
> > WARNING: CPU: 35 PID: 911 at ./arch/x86/include/asm/tlbflush.h:460
> > RIP: 0010:__flush_tlb_all+0x1b/0x3a
> > [..]
> > Call Trace:
> >  phys_pud_init+0x29c/0x2bb
> >  kernel_physical_mapping_init+0xfc/0x219
> >  init_memory_mapping+0x1a5/0x3b0
> >  arch_add_memory+0x2c/0x50
> >  devm_memremap_pages+0x3aa/0x610
> >  pmem_attach_disk+0x585/0x700 [nd_pmem]
> >
> >Andy wondered why a path that can sleep was using __flush_tlb_all() [1]
> >and Dave confirmed the expectation for TLB flush is for modifying /
> >invalidating existing pte entries, but not initial population [2]. Drop
> >the usage of __flush_tlb_all() in phys_{p4d,pud,pmd}_init() on the
> >expectation that this path is only ever populating empty entries for the
> >linear map. Note, at linear map teardown time there is a call to the
> >all-cpu flush_tlb_all() to invalidate the removed mappings.
> >
> >[1]: https://lore.kernel.org/lkml/9DFD717D-857D-493D-A606-B635D72BAC21@amacapital.net
> >[2]: https://lore.kernel.org/lkml/749919a4-cdb1-48a3-adb4-adb81a5fa0b5@intel.com
> >
> >Fixes: f77084d96355 ("x86/mm/pat: Disable preemption around __flush_tlb_all()")
> >Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >Cc: Thomas Gleixner <tglx@linutronix.de>
> >Cc: Peter Zijlstra <peterz@infradead.org>
> >Cc: Borislav Petkov <bp@alien8.de>
> >Cc: <stable@vger.kernel.org>
> >Reported-by: Andy Lutomirski <luto@kernel.org>
> >Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> >Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Hi Dan,
>
> This patch on it's own doesn't apply to any of the stable trees, does it
> maybe depend on some of the previous patches in this series?

It does not strictly depend on them, but it does need to be rebased
without them. The minimum patch for -stable are these
__flush_tlb_all() removals, but without the
set_{pte,pmd,pud,p4d,pgd}_safe() conversion. It's also an option to
backport those helpers, and conversion but I'd defer to x86/mm folks
to make that call.

Re: [PATCH v2 5/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Dave Hansen]

On 12/2/18 9:04 AM, Dan Williams wrote:
>> This patch on it's own doesn't apply to any of the stable trees, does it
>> maybe depend on some of the previous patches in this series?
> It does not strictly depend on them, but it does need to be rebased
> without them. The minimum patch for -stable are these
> __flush_tlb_all() removals, but without the
> set_{pte,pmd,pud,p4d,pgd}_safe() conversion. It's also an option to
> backport those helpers, and conversion but I'd defer to x86/mm folks
> to make that call.

I think we should skip backporting the helpers for now.  Hopefully,
mainline will help us catch bugs with those helpers and we can get the
fixes for those bugs backported if they show up.

I mostly say this because with the addition of the 5-level paging code,
there's been a lot of churn in the area of these helpers.  They will be
a mild pain to backport, and I'd expect more bugs from the backports
than from the lack of the new helpers.

Re: [PATCH v2 0/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Peter Zijlstra]

On Fri, Nov 30, 2018 at 04:35:06PM -0800, Dan Williams wrote:
> 
> Dan Williams (5):
>       generic/pgtable: Make {pmd,pud}_same() unconditionally available
>       generic/pgtable: Introduce {p4d,pgd}_same()
>       generic/pgtable: Introduce set_pte_safe()
>       x86/mm: Validate kernel_physical_mapping_init() pte population
>       x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()
> 

Since you failed to send me 1,2, only for 3-5:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

although going by the subjects, the earlier two patches should be fine
too.

Re: [PATCH v2 0/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Dan Williams]

On Sat, Dec 1, 2018 at 2:28 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Nov 30, 2018 at 04:35:06PM -0800, Dan Williams wrote:
> >
> > Dan Williams (5):
> >       generic/pgtable: Make {pmd,pud}_same() unconditionally available
> >       generic/pgtable: Introduce {p4d,pgd}_same()
> >       generic/pgtable: Introduce set_pte_safe()
> >       x86/mm: Validate kernel_physical_mapping_init() pte population
> >       x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()
> >
>
> Since you failed to send me 1,2, only for 3-5:

Whoops, sorry about that, I'll add you to my "auto-cc on all if cc'd
on one" list.

> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks.

> although going by the subjects, the earlier two patches should be fine
> too.

Re: [PATCH v2 0/5] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

[Kirill A. Shutemov]

On Fri, Nov 30, 2018 at 04:35:06PM -0800, Dan Williams wrote:
> Dan Williams (5):
>       generic/pgtable: Make {pmd,pud}_same() unconditionally available
>       generic/pgtable: Introduce {p4d,pgd}_same()
>       generic/pgtable: Introduce set_pte_safe()
>       x86/mm: Validate kernel_physical_mapping_init() pte population
>       x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping_init()

Looks good to me. For the whole patchset:

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov