[PATCH 8 of 8] xen: use PTE_MASK in pte_mfn()

[PATCH 8 of 8] xen: use PTE_MASK in pte_mfn()

[Jeremy Fitzhardinge]

Use PTE_MASK to extract mfn from pte.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/xen/page.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-x86/xen/page.h b/include/asm-x86/xen/page.h
--- a/include/asm-x86/xen/page.h
+++ b/include/asm-x86/xen/page.h
@@ -127,7 +127,7 @@
 
 static inline unsigned long pte_mfn(pte_t pte)
 {
-	return (pte.pte & ~_PAGE_NX) >> PAGE_SHIFT;
+	return (pte.pte & PTE_MASK) >> PAGE_SHIFT;
 }
 
 static inline pte_t mfn_pte(unsigned long page_nr, pgprot_t pgprot)

[PATCH 0 of 8] x86: use PTE_MASK consistently

[Jeremy Fitzhardinge]

[ Repost due to screwed up mail headers. ]

Hi all,

Here's a series to rationalize the use of PTE_MASK and remove some
amount of ad-hocery.

This gist of the series is:
 1. Fix the definition of PTE_MASK so that its equally applicable in
    all pagetable modes
 2. Use it consistently

I haven't tried to address the *_bad() stuff, other than to convert
pmd_bad_* to use PTE_MASK.

This series has had some testing in the x86.git tree, and hasn't shown
any problems.  Each patch is more or less absolutely trivial and the
series is very bisectable, to help track down any problems which might
arise (this area has always been a source of subtle problems).

      J

[PATCH 1 of 8] x86: define PTE_MASK in a universally useful way

[Jeremy Fitzhardinge]

Define PTE_MASK so that it contains a meaningful value for all x86
pagetable configurations.  Previously it was defined as a "long" which
means that it was too short to cover a 32-bit PAE pte entry.

It is now defined as a pteval_t, which is an integer type long enough
to contain a full pte (or pmd, pud, pgd).

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/page.h |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/asm-x86/page.h b/include/asm-x86/page.h
--- a/include/asm-x86/page.h
+++ b/include/asm-x86/page.h
@@ -10,8 +10,13 @@
 
 #ifdef __KERNEL__
 
-#define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)
-#define PTE_MASK		(_AT(long, PHYSICAL_PAGE_MASK))
+/* Cast PAGE_MASK to a signed type so that it is sign-extended if
+   virtual addresses are 32-bits but physical addresses are larger
+   (ie, 32-bit PAE). */
+#define PHYSICAL_PAGE_MASK	(((signed long)PAGE_MASK) & __PHYSICAL_MASK)
+
+/* PTE_MASK extracts the PFN from a (pte|pmd|pud|pgd)val_t */
+#define PTE_MASK		((pteval_t)PHYSICAL_PAGE_MASK)
 
 #define PMD_PAGE_SIZE		(_AC(1, UL) << PMD_SHIFT)
 #define PMD_PAGE_MASK		(~(PMD_PAGE_SIZE-1))
@@ -24,8 +29,8 @@
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
 
-#define __PHYSICAL_MASK		_AT(phys_addr_t, (_AC(1,ULL) << __PHYSICAL_MASK_SHIFT) - 1)
-#define __VIRTUAL_MASK		((_AC(1,UL) << __VIRTUAL_MASK_SHIFT) - 1)
+#define __PHYSICAL_MASK		((((phys_addr_t)1) << __PHYSICAL_MASK_SHIFT) - 1)
+#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 #ifndef __ASSEMBLY__
 #include <linux/types.h>

[PATCH 2 of 8] x86: fix warning on 32-bit non-PAE

[Jeremy Fitzhardinge]

Fix the warning:

include2/asm/pgtable.h: In function `pte_modify':
include2/asm/pgtable.h:290: warning: left shift count >= width of type

On 32-bit PAE the virtual and physical addresses are both 32-bits,
so it ends up evaluating 1<<32.  Do the shift as a 64-bit shift then
cast to the appropriate size.  This should all be done at compile time,
and so have no effect on generated code.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/page.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-x86/page.h b/include/asm-x86/page.h
--- a/include/asm-x86/page.h
+++ b/include/asm-x86/page.h
@@ -29,7 +29,7 @@
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
 
-#define __PHYSICAL_MASK		((((phys_addr_t)1) << __PHYSICAL_MASK_SHIFT) - 1)
+#define __PHYSICAL_MASK		((phys_addr_t)(1ULL << __PHYSICAL_MASK_SHIFT) - 1)
 #define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 #ifndef __ASSEMBLY__

[PATCH 3 of 8] x86: rearrange __(VIRTUAL|PHYSICAL)_MASK

[Jeremy Fitzhardinge]

Put the definitions of __(VIRTUAL|PHYSICAL)_MASK before their uses.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/page.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-x86/page.h b/include/asm-x86/page.h
--- a/include/asm-x86/page.h
+++ b/include/asm-x86/page.h
@@ -9,6 +9,9 @@
 #define PAGE_MASK	(~(PAGE_SIZE-1))
 
 #ifdef __KERNEL__
+
+#define __PHYSICAL_MASK		((phys_addr_t)(1ULL << __PHYSICAL_MASK_SHIFT) - 1)
+#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 /* Cast PAGE_MASK to a signed type so that it is sign-extended if
    virtual addresses are 32-bits but physical addresses are larger
@@ -28,9 +31,6 @@
 
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
-
-#define __PHYSICAL_MASK		((phys_addr_t)(1ULL << __PHYSICAL_MASK_SHIFT) - 1)
-#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 #ifndef __ASSEMBLY__
 #include <linux/types.h>

[PATCH 4 of 8] x86: use PTE_MASK in 32-bit PAE

[Jeremy Fitzhardinge]

Use PTE_MASK in 3-level pagetables (ie, 32-bit PAE).

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/pgtable-3level.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-x86/pgtable-3level.h b/include/asm-x86/pgtable-3level.h
--- a/include/asm-x86/pgtable-3level.h
+++ b/include/asm-x86/pgtable-3level.h
@@ -120,9 +120,9 @@
 		write_cr3(pgd);
 }
 
-#define pud_page(pud) ((struct page *) __va(pud_val(pud) & PAGE_MASK))
+#define pud_page(pud) ((struct page *) __va(pud_val(pud) & PTE_MASK))
 
-#define pud_page_vaddr(pud) ((unsigned long) __va(pud_val(pud) & PAGE_MASK))
+#define pud_page_vaddr(pud) ((unsigned long) __va(pud_val(pud) & PTE_MASK))
 
 
 /* Find an entry in the second-level page table.. */
@@ -160,7 +160,7 @@
 
 static inline unsigned long pte_pfn(pte_t pte)
 {
-	return (pte_val(pte) & ~_PAGE_NX) >> PAGE_SHIFT;
+	return (pte_val(pte) & PTE_MASK) >> PAGE_SHIFT;
 }
 
 /*

[PATCH 5 of 8] x86: use PTE_MASK in pgtable_32.h

[Jeremy Fitzhardinge]

---
 include/asm-x86/pgtable_32.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-x86/pgtable_32.h b/include/asm-x86/pgtable_32.h
--- a/include/asm-x86/pgtable_32.h
+++ b/include/asm-x86/pgtable_32.h
@@ -88,7 +88,7 @@
 /* To avoid harmful races, pmd_none(x) should check only the lower when PAE */
 #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_bad(x) ((pmd_val(x) & (~PTE_MASK & ~_PAGE_USER)) != _KERNPG_TABLE)
 
 #define pages_to_mb(x) ((x) >> (20-PAGE_SHIFT))
 
@@ -159,7 +159,7 @@
 #define pmd_page(pmd) (pfn_to_page(pmd_val((pmd)) >> PAGE_SHIFT))
 
 #define pmd_page_vaddr(pmd)					\
-	((unsigned long)__va(pmd_val((pmd)) & PAGE_MASK))
+	((unsigned long)__va(pmd_val((pmd)) & PTE_MASK))
 
 #if defined(CONFIG_HIGHPTE)
 #define pte_offset_map(dir, address)					\

[PATCH 6 of 8] x86: clarify use of _PAGE_CHG_MASK

[Jeremy Fitzhardinge]

_PAGE_CHG_MASK is defined as the set of bits not updated by
pte_modify(); specifically, the pfn itself, and the Accessed and Dirty
bits (which are updated by hardware).

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/pgtable.h |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/include/asm-x86/pgtable.h b/include/asm-x86/pgtable.h
--- a/include/asm-x86/pgtable.h
+++ b/include/asm-x86/pgtable.h
@@ -57,6 +57,7 @@
 #define _KERNPG_TABLE	(_PAGE_PRESENT | _PAGE_RW | _PAGE_ACCESSED |	\
 			 _PAGE_DIRTY)
 
+/* Set of bits not changed in pte_modify */
 #define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_PCD | _PAGE_PWT |		\
 			 _PAGE_ACCESSED | _PAGE_DIRTY)
 

[PATCH 7 of 8] x86: use PTE_MASK rather than ad-hoc mask

[Jeremy Fitzhardinge]

Use ~PTE_MASK to extract the non-pfn parts of the pte (ie, the pte
flags), rather than constructing an ad-hoc mask.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/pgtable.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-x86/pgtable.h b/include/asm-x86/pgtable.h
--- a/include/asm-x86/pgtable.h
+++ b/include/asm-x86/pgtable.h
@@ -305,7 +305,7 @@
 	return __pgprot(preservebits | addbits);
 }
 
-#define pte_pgprot(x) __pgprot(pte_val(x) & (0xfff | _PAGE_NX))
+#define pte_pgprot(x) __pgprot(pte_val(x) & ~PTE_MASK)
 
 #define canon_pgprot(p) __pgprot(pgprot_val(p) & __supported_pte_mask)
 

[PATCH 8 of 8] xen: use PTE_MASK in pte_mfn()

[Jeremy Fitzhardinge]

Use PTE_MASK to extract mfn from pte.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 include/asm-x86/xen/page.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-x86/xen/page.h b/include/asm-x86/xen/page.h
--- a/include/asm-x86/xen/page.h
+++ b/include/asm-x86/xen/page.h
@@ -127,7 +127,7 @@
 
 static inline unsigned long pte_mfn(pte_t pte)
 {
-	return (pte.pte & ~_PAGE_NX) >> PAGE_SHIFT;
+	return (pte.pte & PTE_MASK) >> PAGE_SHIFT;
 }
 
 static inline pte_t mfn_pte(unsigned long page_nr, pgprot_t pgprot)

Re: [PATCH 0 of 8] x86: use PTE_MASK consistently

[Hugh Dickins]

On Tue, 20 May 2008, Jeremy Fitzhardinge wrote:
> 
> Here's a series to rationalize the use of PTE_MASK and remove some
> amount of ad-hocery.
> 
> This gist of the series is:
>  1. Fix the definition of PTE_MASK so that its equally applicable in
>     all pagetable modes
>  2. Use it consistently
> 
> I haven't tried to address the *_bad() stuff, other than to convert
> pmd_bad_* to use PTE_MASK.
> 
> This series has had some testing in the x86.git tree, and hasn't shown
> any problems.  Each patch is more or less absolutely trivial and the
> series is very bisectable, to help track down any problems which might
> arise (this area has always been a source of subtle problems).

Yes, thanks Jeremy: I've checked that each stage builds and runs X on my
boxes here, x86_32 and x86_32+PAE and x86_64.  (So even 1/8 is enough to
fix the PAT pte_modify issue, though 2/8 then fixes compiler warnings.)

I'll leave it to you and Linus whether your way of defining PTE_MASK is
satisfactory as is, or needs to be improved to his way.  I've not tried
his suggestion of doing the _PAGE_BIT definitions: certainly it's
seemed odd to me that they were defined with L, but I've little
appetite to mess around with them now myself.

One thing I did do a few days ago, but not got around to posting,
was the *_bad() stuff.  I've retested and will post that now...

Hugh

[PATCH] x86: strengthen 64-bit p?d_bad()

[Hugh Dickins]

The x86_64 pgd_bad(), pud_bad(), pmd_bad() inlines have differed from
their x86_32 counterparts in a couple of ways: they've been unnecessarily
weak (e.g. letting 0 or 1 count as good), and were typed as unsigned long.
Strengthen them and return int.

The PAE pmd_bad was too weak before, allowing any junk in the upper half;
but got strengthened by the patch correcting its ~PAGE_MASK to ~PTE_MASK.
The PAE pud_bad already said ~PTE_MASK; and since it folds into pgd_bad,
and we don't set the protection bits at that level, it'll do as is.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
This could either go in along with Jeremy's PTE_MASK patches, or be
held back for next release - it's independent of them, and not vital.

 include/asm-x86/pgtable_64.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/include/asm-x86/pgtable_64.h
+++ b/include/asm-x86/pgtable_64.h
@@ -151,19 +151,19 @@ static inline void native_pgd_clear(pgd_
 
 #ifndef __ASSEMBLY__
 
-static inline unsigned long pgd_bad(pgd_t pgd)
+static inline int pgd_bad(pgd_t pgd)
 {
-	return pgd_val(pgd) & ~(PTE_MASK | _KERNPG_TABLE | _PAGE_USER);
+	return (pgd_val(pgd) & ~(PTE_MASK | _PAGE_USER)) != _KERNPG_TABLE;
 }
 
-static inline unsigned long pud_bad(pud_t pud)
+static inline int pud_bad(pud_t pud)
 {
-	return pud_val(pud) & ~(PTE_MASK | _KERNPG_TABLE | _PAGE_USER);
+	return (pud_val(pud) & ~(PTE_MASK | _PAGE_USER)) != _KERNPG_TABLE;
 }
 
-static inline unsigned long pmd_bad(pmd_t pmd)
+static inline int pmd_bad(pmd_t pmd)
 {
-	return pmd_val(pmd) & ~(PTE_MASK | _KERNPG_TABLE | _PAGE_USER);
+	return (pmd_val(pmd) & ~(PTE_MASK | _PAGE_USER)) != _KERNPG_TABLE;
 }
 
 #define pte_none(x)	(!pte_val((x)))

Re: [PATCH 0 of 8] x86: use PTE_MASK consistently

[Jeremy Fitzhardinge]

Hugh Dickins wrote:
> I'll leave it to you and Linus whether your way of defining PTE_MASK is
> satisfactory as is, or needs to be improved to his way.  I've not tried
> his suggestion of doing the _PAGE_BIT definitions: certainly it's
> seemed odd to me that they were defined with L, but I've little
> appetite to mess around with them now myself.
>   

Yes, well, that was me too, with the intention of making ~_PAGE_FOO 
generate an appropriately sized mask.  I guess it would be better to use 
pteval_t these days.

    J

Re: [PATCH 0 of 8] x86: use PTE_MASK consistently

[Linus Torvalds]

On Tue, 20 May 2008, Jeremy Fitzhardinge wrote:

> Hugh Dickins wrote:
> > I'll leave it to you and Linus whether your way of defining PTE_MASK is
> > satisfactory as is, or needs to be improved to his way.  I've not tried
> > his suggestion of doing the _PAGE_BIT definitions: certainly it's
> > seemed odd to me that they were defined with L, but I've little
> > appetite to mess around with them now myself.
> 
> Yes, well, that was me too, with the intention of making ~_PAGE_FOO generate
> an appropriately sized mask.  I guess it would be better to use pteval_t these
> days.

The thing is, making _PAGE_FOO be a signed long in no way guarantees any 
appropriately sized masks.

Why? Because 'long' mixes with 'unsigned long' and in the C type system, 
unsigned is stronger than signed.

As a result, if you depend on sign-extension of things like a binary 'not' 
operation, you're always going to have cases where it simply doesn't work, 
just because you happen to mix it up with other things that may have type 
'unsigned long' (or, on 32-bit, 'unsigned int' which has the same size: 
the 'unsigned' is so strong that it will convert to 'long' because it has 
the same size.

Try this one in a 32-bit environment:

	/*
	 * 'A' is the low bits mask - and explicitly signed so
	 * that you can do '~A' to get the high bits of a PTE
	 */
	#define A 0xfffl

	/*
	 * Some random value that just happens to be unsigned for other 
	 * reasons - perhaps it's a sizeof expression or whatever.
	 */
	#define B 5U

	int main(int argc, char **argv)
	{
	        unsigned long long pte;

	        pte = B | ~A;
	        printf("%llx\n", pte);

	        pte = ~A;
	        pte |= B;
	        printf("%llx\n", pte);

	        pte = B;
	        pte |= ~A;
	        printf("%llx\n", pte);
	}

and see what it generates. It does the EXACT SAME thing in all three 
cases: the expression is "B | ~A" in various guises, but they are not the 
same values because the order of the type expansion matters. Note how in 
one case the sign bits of the binary not did *not* percolate up to the 
upper 32 bits.

So making something signed and just assuming that it means that when it is 
expanded to a bigger type the bits will always be set is simply not true. 
It's *often* true, but it's untrue in many trivial cases. 

If you want the upper bits to behave reliably in a bigger type, then you 
have to actually make the values *be* of that type.

Or do people really want to live with a kernel where 

	pte = B | ~A;

is different from

	pte = B;
	pte |= ~A;

and making the "obvious" simplification will just generate a totaly 
different result?

I don't think we want to continue to rely on that kind of subtle C type 
tricks, especially since we know they haven't worked (ie we had this 
*exact* issue with mixing in PAGE_MASK that happened to be unsigned, and 
changed all the arithmetic for the signed longs that depended on sign 
extensions to bigger types).

Obviously, we could just choose to make *everything* be of a signed type, 
but we know that doesn't work very well, and it interacts badly with 
things like right-shifting (which we do quite a lot on the things 
involved: we extract fields like the PFN with a right shift and a mask).

			Linus

Re: [PATCH 0 of 8] x86: use PTE_MASK consistently

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> This series has had some testing in the x86.git tree, and hasn't shown 
> any problems.  Each patch is more or less absolutely trivial and the 
> series is very bisectable, to help track down any problems which might 
> arise (this area has always been a source of subtle problems).

yep, this has been problem-free in x86.git and then in -tip, we had a 
separate x86/ptemask topic for it that we originally intended for 
v2.6.27, and we dont mind to see it upstream now ;-)

	Ingo

Re: [PATCH 0 of 8] x86: use PTE_MASK consistently

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > This series has had some testing in the x86.git tree, and hasn't 
> > shown any problems.  Each patch is more or less absolutely trivial 
> > and the series is very bisectable, to help track down any problems 
> > which might arise (this area has always been a source of subtle 
> > problems).
> 
> yep, this has been problem-free in x86.git and then in -tip, we had a 
> separate x86/ptemask topic for it that we originally intended for 
> v2.6.27, and we dont mind to see it upstream now ;-)

the only delta between the 8 patches you posted here and tip/x86/ptemask 
topic is the comment below - so we are indeed on the safe side.

	Ingo

----------------->
diff --git a/include/asm-x86/pgtable.h b/include/asm-x86/pgtable.h
index 21fed4f..97c271b 100644
--- a/include/asm-x86/pgtable.h
+++ b/include/asm-x86/pgtable.h
@@ -57,6 +57,7 @@
 #define _KERNPG_TABLE	(_PAGE_PRESENT | _PAGE_RW | _PAGE_ACCESSED |	\
 			 _PAGE_DIRTY)
 
+/* Set of bits not changed in pte_modify */
 #define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_PCD | _PAGE_PWT |		\
 			 _PAGE_ACCESSED | _PAGE_DIRTY)
 

Re: [PATCH 0 of 8] x86: use PTE_MASK consistently

[Linus Torvalds]

On Tue, 20 May 2008, Ingo Molnar wrote:
> 
> yep, this has been problem-free in x86.git and then in -tip, we had a 
> separate x86/ptemask topic for it that we originally intended for 
> v2.6.27, and we dont mind to see it upstream now ;-)

Well, since we actually had a regression, I had to do something before 
actually doing 2.6.26.

So to fix the X crash problem with PAE, the choice was between either 
applying this series that cleans things up or alternatively add yet 
another hack to work around the problem.

And I'm much happier taking the proper fix, even if it was slightly 
bigger. It's not like it was a huge and complicated and scary fix ;)

		Linus