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

On Tue, Nov 20, 2018 at 02:59:32AM +0000, Williams, Dan J wrote:

On Mon, 2018-11-19 at 15:43 -0800, Dave Hansen wrote:

...

On 11/19/18 3:19 PM, Dan Williams wrote:

...

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].

I _think_ this is OK.

But, could we sprinkle a few WARN_ON_ONCE(p*_present()) calls in there to help us sleep at night?

Well, I'm having nightmares now because my naive patch to sprinkle some WARN_ON_ONCE() calls is leading to my VM live locking at boot... no backtrace. If I revert the patch below and just go with the __flush_tlb_all() removal it seems fine.

I'm going to set this aside for a bit, but if anyone has any thoughts in the meantime I'd appreciate it.

Have you tried using early_printk ?

So kernel_physical_mapping_init() has a comment that states the virtual and physical addresses we create mappings for should be PMD aligned, which implies pud/p4d could have overlap between the mappings.

But in that case, I would expect the new and old values to match.

So maybe you should be checking something like:

WARN_ON_ONCE(pud_present(*pud) && !pud_same(pud, new));

@@ -573,6 +578,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))

• 		WARN_ON_ONCE(pud_present(*pud));
  	set_pud(pud, __pud(0));
  continue;
  

  }

@@ -610,6 +616,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);

• 	WARN_ON_ONCE(pud_present(*pud));
  set_pte((pte_t *)pud,
  	pfn_pte((paddr & PUD_MASK) >> PAGE_SHIFT,
  		PAGE_KERNEL_LARGE));
  

@@ -623,6 +630,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);

• WARN_ON_ONCE(pud_present(*pud));
  

  pud_populate(&init_mm, pud, pmd); spin_unlock(&init_mm.page_table_lock); }

@@ -657,6 +665,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))

• 		WARN_ON_ONCE(p4d_present(*p4d));
  	set_p4d(p4d, __p4d(0));
  continue;
  

  }

@@ -674,6 +683,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);

• WARN_ON_ONCE(p4d_present(*p4d));
  

  p4d_populate(&init_mm, p4d, pud); spin_unlock(&init_mm.page_table_lock); }