[PATCH] x86: fix PSE pagetable construction

[PATCH] x86: fix PSE pagetable construction

[Jeremy Fitzhardinge]

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

Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>

---
 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(-)

===================================================================
--- a/arch/i386/mm/hugetlbpage.c
+++ b/arch/i386/mm/hugetlbpage.c
@@ -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 *
===================================================================
--- a/arch/i386/mm/init.c
+++ b/arch/i386/mm/init.c
@@ -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
===================================================================
--- a/include/asm-i386/pgtable.h
+++ b/include/asm-i386/pgtable.h
@@ -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))
 
===================================================================
--- a/include/asm-x86_64/pgtable.h
+++ b/include/asm-x86_64/pgtable.h
@@ -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)
===================================================================
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -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; })

Re: [PATCH] x86: fix PSE pagetable construction

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

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

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.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/i386/mm/init.c |   52 +++++++++++++++++++-------------------------------
 1 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/arch/i386/mm/init.c b/arch/i386/mm/init.c
index b77a43c..dbe16f6 100644
--- a/arch/i386/mm/init.c
+++ b/arch/i386/mm/init.c
@@ -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

Re: [PATCH] x86: fix PSE pagetable construction

[Jeremy Fitzhardinge]

Eric W. Biederman wrote:
> Jeremy Fitzhardinge <jeremy@goop.org> writes:
>
>   
>> 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. ]
>>     
>
> 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.
>   

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

Re: [PATCH] x86: fix PSE pagetable construction

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> 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?

For a short term solution it sounds reasonable.

Eric

Re: [PATCH] x86: fix PSE pagetable construction

[Andi Kleen]

> ===================================================================
> --- a/arch/i386/mm/hugetlbpage.c
> +++ b/arch/i386/mm/hugetlbpage.c
> @@ -183,6 +183,7 @@ follow_huge_addr(struct mm_struct *mm, u
>  	return page;
>  }
>  
> +#undef pmd_huge
>  int pmd_huge(pmd_t pmd)

Please handle that in the headers or rename it

-Andi

Re: [PATCH] x86: fix PSE pagetable construction

[Andi Kleen]

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

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

Re: [PATCH] x86: fix PSE pagetable construction

[Jeremy Fitzhardinge]

Andi Kleen wrote:
>> My preference would be for whoever had:
>> paravirt_ops-hooks-to-set-up-initial-pagetable.patch
>>     
>
> 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.

I'm working on an incremental fix.

    J

Re: [PATCH] x86: fix PSE pagetable construction

[Jeremy Fitzhardinge]

Andi Kleen wrote:
>> ===================================================================
>> --- a/arch/i386/mm/hugetlbpage.c
>> +++ b/arch/i386/mm/hugetlbpage.c
>> @@ -183,6 +183,7 @@ follow_huge_addr(struct mm_struct *mm, u
>>  	return page;
>>  }
>>  
>> +#undef pmd_huge
>>  int pmd_huge(pmd_t pmd)
>>     
>
> Please handle that in the headers or rename it
>   

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

    J

Re: [PATCH] x86: fix PSE pagetable construction

[Jeremy Fitzhardinge]

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

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

    J

Re: [PATCH] x86: fix PSE pagetable construction

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> Andi Kleen wrote:
>>> My preference would be for whoever had:
>>> paravirt_ops-hooks-to-set-up-initial-pagetable.patch
>>>     
>>
>> 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.
>
> I'm working on an incremental fix.

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

Re: [PATCH] x86: fix PSE pagetable construction

[Eric W. Biederman]

Andi Kleen <ak@suse.de> writes:

>> My preference would be for whoever had:
>> paravirt_ops-hooks-to-set-up-initial-pagetable.patch
>
> 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.

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

Re: [PATCH] x86: fix PSE pagetable construction

[Andi Kleen]

On Saturday 28 April 2007 19:23:55 Eric W. Biederman wrote:
> Andi Kleen <ak@suse.de> writes:
> 
> >> My preference would be for whoever had:
> >> paravirt_ops-hooks-to-set-up-initial-pagetable.patch
> >
> > 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.
> 
> I don't see Xen patches merged yet in your tree yet.

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

Re: [PATCH] x86: fix PSE pagetable construction

[Eric W. Biederman]

Andi Kleen <ak@suse.de> writes:

> On Saturday 28 April 2007 19:23:55 Eric W. Biederman wrote:
>> Andi Kleen <ak@suse.de> writes:
>> 
>> >> My preference would be for whoever had:
>> >> paravirt_ops-hooks-to-set-up-initial-pagetable.patch
>> >
>> > 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.
>> 
>> I don't see Xen patches merged yet in your tree yet.
>
> Hmm, I thought you were talking about paravirt-initial-pagetable ?

Yes that is the patch with problems.

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

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

Re: [PATCH] x86: fix PSE pagetable construction

[Jeremy Fitzhardinge]

Eric W. Biederman wrote:
> Except for Xen my patch should work incrementally.

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