x86: use PTE_MASK consistently

[ 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

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

---
 include/asm-x86/page.h |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

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

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.

---
 include/asm-x86/page.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -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__

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

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

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

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

---
 include/asm-x86/pgtable-3level.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

@@ -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;
 }
 
 /*

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

@@ -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)					\

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

---
 include/asm-x86/pgtable.h |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

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

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

---
 include/asm-x86/pgtable.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Use PTE_MASK to extract mfn from pte.

---
 include/asm-x86/xen/page.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

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.

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

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

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

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

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

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

----------------->

index 21fed4f..97c271b 100644
@@ -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)
 

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