x86: fix PSE pagetable construction

When constructing the initial pagetable in pagetable_init, make sure that non-PSE pmds are updated to PSE ones. This fixes a bug in the paravirt pagetable init code, which otherwise tries to avoid overwrite existing mappings.

This moves the definition of pmd_huge() out of the hugetlbfs files into pgtable.h.

[ I know Eric would like to make larger changes to the way
pagetable init works, but this patch is the minimal fix to an existing bug. ]

---
 arch/i386/mm/hugetlbpage.c   |    6 +-----
 arch/i386/mm/init.c          |    2 +-
 include/asm-i386/pgtable.h   |    2 +-
 include/asm-x86_64/pgtable.h |    1 +
 include/linux/hugetlb.h      |    2 --
 5 files changed, 4 insertions(+), 9 deletions(-)

===================================================================

@@ -183,6 +183,7 @@ follow_huge_addr(struct mm_struct *mm, u
 	return page;
 }
 
+#undef pmd_huge
 int pmd_huge(pmd_t pmd)
 {
 	return 0;

@@ -201,11 +202,6 @@ follow_huge_addr(struct mm_struct *mm, u
 follow_huge_addr(struct mm_struct *mm, unsigned long address, int write)
 {
 	return ERR_PTR(-EINVAL);
-}
-
-int pmd_huge(pmd_t pmd)
-{
-	return !!(pmd_val(pmd) & _PAGE_PSE);
 }
 
 struct page *

===================================================================

@@ -172,7 +172,7 @@ static void __init kernel_physical_mappi
 			/* Map with big pages if possible, otherwise create normal page tables. */
 			if (cpu_has_pse) {
 				unsigned int address2 = (pfn + PTRS_PER_PTE - 1) * PAGE_SIZE + PAGE_OFFSET + PAGE_SIZE-1;
-				if (!pmd_present(*pmd)) {
+				if (!pmd_present(*pmd) || !pmd_huge(*pmd)) {
 					if (is_kernel_text(address) || is_kernel_text(address2))
 						set_pmd(pmd, pfn_pmd(pfn, PAGE_KERNEL_LARGE_EXEC));
 					else

===================================================================

@@ -211,7 +211,7 @@ extern unsigned long pg0[];
 #define pmd_none(x)	(!(unsigned long)pmd_val(x))
 #define pmd_present(x)	(pmd_val(x) & _PAGE_PRESENT)
 #define	pmd_bad(x)	((pmd_val(x) & (~PAGE_MASK & ~_PAGE_USER)) != _KERNPG_TABLE)
-
+#define pmd_huge(x)	((pmd_val(x) & _PAGE_PSE) != 0)
 
 #define pages_to_mb(x) ((x) >> (20-PAGE_SHIFT))
 

===================================================================

@@ -352,6 +352,7 @@ static inline int pmd_large(pmd_t pte) {
 			pmd_index(address))
 #define pmd_none(x)	(!pmd_val(x))
 #define pmd_present(x)	(pmd_val(x) & _PAGE_PRESENT)
+#define pmd_huge(x)	((pmd_val(x) & _PAGE_PSE) != 0)
 #define pmd_clear(xp)	do { set_pmd(xp, __pmd(0)); } while (0)
 #define pfn_pmd(nr,prot) (__pmd(((nr) << PAGE_SHIFT) | pgprot_val(prot)))
 #define pmd_pfn(x)  ((pmd_val(x) & __PHYSICAL_MASK) >> PAGE_SHIFT)

===================================================================

@@ -41,7 +41,6 @@ struct page *follow_huge_addr(struct mm_
 			      int write);
 struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
 				pmd_t *pmd, int write);
-int pmd_huge(pmd_t pmd);
 void hugetlb_change_protection(struct vm_area_struct *vma,
 		unsigned long address, unsigned long end, pgprot_t newprot);
 

@@ -114,7 +113,6 @@ static inline unsigned long hugetlb_tota
 #define hugetlb_report_node_meminfo(n, buf)	0
 #define follow_huge_pmd(mm, addr, pmd, write)	NULL
 #define prepare_hugepage_range(addr,len,pgoff)	(-EINVAL)
-#define pmd_huge(x)	0
 #define is_hugepage_only_range(mm, addr, len)	0
 #define hugetlb_free_pgd_range(tlb, addr, end, floor, ceiling) ({BUG(); 0; })
 #define hugetlb_fault(mm, vma, addr, write)	({ BUG(); 0; })

My preference would be for whoever had:
paravirt_ops-hooks-to-set-up-initial-pagetable.patch

queued to drop it until we can get a version that doesn't break early page table setup.

Your partial fix still leaves the real page tables in a partially incorrect state, and even if we removed your changes from the PSE path we still can wind up not failing to set _PAGE_NX in the appropriate places on 4K pages.

I have tried to be constructive and suggest how we can fix this cleanly.

Short of that this is what I see needing to happen to fix the above patches changes to arch/i386/mm/init.c.

Eric

...

Subject: [PATCH] i386: During page table initialization always set the leaf page table entries.

If we don't set the leaf page table entries it is quite possible that we will inherit and incorrect page table entry from the initial boot page table setup in head.S. So we need to redo the effort here.

I don't know what to do about hypervisors like Xen that require their page tables to be read only, as our identity mapped page table entries currently violate that requirement. All I know if the kernel doesn't work properly on native hardware it is a bug.

---
 arch/i386/mm/init.c |   52 +++++++++++++++++++-------------------------------
 1 files changed, 20 insertions(+), 32 deletions(-)

index b77a43c..dbe16f6 100644
@@ -63,18 +63,18 @@ static pmd_t * __init one_md_table_init(pgd_t *pgd)
 	pmd_t *pmd_table;
 		
 #ifdef CONFIG_X86_PAE
-	pmd_table = (pmd_t *) alloc_bootmem_low_pages(PAGE_SIZE);
-
-	paravirt_alloc_pd(__pa(pmd_table) >> PAGE_SHIFT);
-	set_pgd(pgd, __pgd(__pa(pmd_table) | _PAGE_PRESENT));
-	pud = pud_offset(pgd, 0);
-	if (pmd_table != pmd_offset(pud, 0)) 
-		BUG();
-#else
+	if (!(pgd_val(*pgd) & _PAGE_PRESENT)) {
+		pmd_table = (pmd_t *) alloc_bootmem_low_pages(PAGE_SIZE);
+
+		paravirt_alloc_pd(__pa(pmd_table) >> PAGE_SHIFT);
+		set_pgd(pgd, __pgd(__pa(pmd_table) | _PAGE_PRESENT));
+		pud = pud_offset(pgd, 0);
+		if (pmd_table != pmd_offset(pud, 0))
+			BUG();
+	}
+#endif
 	pud = pud_offset(pgd, 0);
 	pmd_table = pmd_offset(pud, 0);
-#endif
-
 	return pmd_table;
 }
 

@@ -84,7 +84,7 @@ static pmd_t * __init one_md_table_init(pgd_t *pgd)
  */
 static pte_t * __init one_page_table_init(pmd_t *pmd)
 {
-	if (pmd_none(*pmd)) {
+	if (!(pmd_val(*pmd) & _PAGE_PRESENT)) {
 		pte_t *page_table = (pte_t *) alloc_bootmem_low_pages(PAGE_SIZE);
 
 		paravirt_alloc_pt(__pa(page_table) >> PAGE_SHIFT);

@@ -109,7 +109,6 @@ static pte_t * __init one_page_table_init(pmd_t *pmd)
 static void __init page_table_range_init (unsigned long start, unsigned long end, pgd_t *pgd_base)
 {
 	pgd_t *pgd;
-	pud_t *pud;
 	pmd_t *pmd;
 	int pgd_idx, pmd_idx;
 	unsigned long vaddr;

@@ -120,13 +119,10 @@ static void __init page_table_range_init (unsigned long start, unsigned long end
 	pgd = pgd_base + pgd_idx;
 
 	for ( ; (pgd_idx < PTRS_PER_PGD) && (vaddr != end); pgd++, pgd_idx++) {
-		if (!(pgd_val(*pgd) & _PAGE_PRESENT))
-			one_md_table_init(pgd);
-		pud = pud_offset(pgd, vaddr);
-		pmd = pmd_offset(pud, vaddr);
+		pmd = one_md_table_init(pgd);
+		pmd = pmd + pmd_index(vaddr);
 		for (; (pmd_idx < PTRS_PER_PMD) && (vaddr != end); pmd++, pmd_idx++) {
-			if (pmd_none(*pmd)) 
-				one_page_table_init(pmd);
+			one_page_table_init(pmd);
 
 			vaddr += PMD_SIZE;
 		}

@@ -159,11 +155,7 @@ static void __init kernel_physical_mapping_init(pgd_t *pgd_base)
 	pfn = 0;
 
 	for (; pgd_idx < PTRS_PER_PGD; pgd++, pgd_idx++) {
-		if (!(pgd_val(*pgd) & _PAGE_PRESENT))
-			pmd = one_md_table_init(pgd);
-		else
-			pmd = pmd_offset(pud_offset(pgd, PAGE_OFFSET), PAGE_OFFSET);
-
+		pmd = one_md_table_init(pgd);
 		if (pfn >= max_low_pfn)
 			continue;
 		for (pmd_idx = 0; pmd_idx < PTRS_PER_PMD && pfn < max_low_pfn; pmd++, pmd_idx++) {

@@ -172,12 +164,11 @@ static void __init kernel_physical_mapping_init(pgd_t *pgd_base)
 			/* Map with big pages if possible, otherwise create normal page tables. */
 			if (cpu_has_pse) {
 				unsigned int address2 = (pfn + PTRS_PER_PTE - 1) * PAGE_SIZE + PAGE_OFFSET + PAGE_SIZE-1;
-				if (!pmd_present(*pmd)) {
-					if (is_kernel_text(address) || is_kernel_text(address2))
-						set_pmd(pmd, pfn_pmd(pfn, PAGE_KERNEL_LARGE_EXEC));
-					else
-						set_pmd(pmd, pfn_pmd(pfn, PAGE_KERNEL_LARGE));
-				}
+				if (is_kernel_text(address) || is_kernel_text(address2))
+					set_pmd(pmd, pfn_pmd(pfn, PAGE_KERNEL_LARGE_EXEC));
+				else
+					set_pmd(pmd, pfn_pmd(pfn, PAGE_KERNEL_LARGE));
+
 				pfn += PTRS_PER_PTE;
 			} else {
 				pte = one_page_table_init(pmd);

@@ -185,9 +176,6 @@ static void __init kernel_physical_mapping_init(pgd_t *pgd_base)
 				for (pte_ofs = 0;
 				     pte_ofs < PTRS_PER_PTE && pfn < max_low_pfn;
 				     pte++, pfn++, pte_ofs++, address += PAGE_SIZE) {
-					if (pte_present(*pte))
-						continue;
-
 					if (is_kernel_text(address))
 						set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
 					else

1.5.1.1.181.g2de0

Well, you've proposed:

1. write the pte entries unconditionally
2. a callback to ask what permissions each page should be mapped
3. a general mechanism to query for pages which require special mappings, such at PAT in the native case, and RO for Xen

and I've proposed:

4. if we're given a template pagetable, then don't overwrite any existing mapping

1 works in general, but it doesn't work for Xen because it ends up mapping pagetable pages RW.

If 2 is constrained to boot-time init then its reasonably possible to implement, but it will add a new paravirt_op which isn't going to make Andi happy. A general mechanism will be extremely hard to implement, because I don't know of an efficient way to identify whether a particular pfn is part of a pagetable or not.

3 suffers the same problem as the general form of 2, but its more ambitious since it would also allow identification of pages with an arbitrary set of special properties.

Perhaps 2 and 3 could be implemented by stashing some state in the page structure, but that's no use for pagetable_init, because page structures don't exist at that point.

4 is what I've implemented. It works for Xen, but it currently prevents non-PAE native booting from properly constructing a PSE mapping for the kernel. And as you point out, it also prevents harmless (from Xen's perspective) updates of the page permissions. Plus it just adds a bit more uglification entropy to mm/init.c.

So, in summary:

Given that 1 is unworkable for Xen, 2 is kind of groady, 3 is longer-term research project and 4 is buggy, we need something else.

I don't have any particularly bright ideas, but there is the other idea I mentioned. I could pre-initialize the pagetable with the Xen template, but use unconditional calls to set_pte in pagetable_init. However, the init-time xen_set_pte simply refuse to set _PAGE_WRITE on a pte which isn't already writable, thereby protecting the RO state of the pagetable pages. Once pagetable construction has finished, it would switch to the real xen_set_pte, which operates as normal.

This has the downside of making init-time xen_set_pte a little strange, but it does have the advantage of making the Xen constraints solely the responsibility of the Xen code, and it is mitigated by the fact that it is only in effect during pagetable_init.

Would that work for everyone?

J

For a short term solution it sounds reasonable.

Eric

That would be very messy because there are many followup dependencies.

But once we have a good fix aggreed by everybody I can merge it into the initial patch then.

-Andi

I'm working on an incremental fix.

J

Well, can I just get rid of the whole chunk of code that it's in? It's only for "testing".

J

...not that it matters because we're dropping this patch anyway.

J

Except for Xen my patch should work incrementally.

Just so long as we don't push this change upstream until we fix this, we should be ok.

Eric

I don't see Xen patches merged yet in your tree yet.

Jeremy currently intends to fix this in the Xen pte setting code.

So my patch posted earlier in this thread should be fine.

Eric

Hmm, I thought you were talking about paravirt-initial-pagetable ?

That one is in my tree already. The Xen patches that are still outstanding iirc are all addons patches, as they don't impact existing code paths anymore.

-Andi

Yes that is the patch with problems.

Ok.

Right now Xen is the only paravirtualization solution that wants to make page tables read only.

The current Xen methodology is to setup the identity mapping with the initial page table set to read-only, before calling paging_init(). kernel_physical_mapping_init was modified to skip pages which are already in the page table.

However in other cases this causes us to fail to setup important bits like PSE and NX where we want them on the page tables. So for the leaf pages we need to continue setting things at least on real hardware.

The current plan was that Jeremy was going to go and modify the Xen set_pte early in boot, to preserve to mask off _PAGE_RW in early boot for pages that don't currently have _PAGE_RW set.

So the fix is two parts.
- Part one unconditionally set the page table entries for 4K and pse pages.
- Fix the Xen set_pte.

Short of making the identity mapping setup paravirt specific I don't know what we can do.

Eric

Yes, your patch is good. I have it in my series followed by the appropriate Xen patch, and it boots OK both Xen and native.

J