Messages in this thread Patch in this message | | | Date | Mon, 14 Jan 2008 14:46:00 -0800 | From | Jeremy Fitzhardinge <> | Subject | Re: unify pagetable accessors patch causes double fault II |
| |
Andi Kleen wrote: >> OK, I see the problem. The problem is that the _PAGE_X defines are >> defined with _AC(UL, 1 << _PAGE_BIT_X), which has unsigned long type. >> This means that ~_PAGE_X also has unsigned long type, and so when cast >> to 64-bit in pte_mkX, it ends up &ing the pte with 0x00000000ffffffxxx, >> with predictable results. >> > > Actually I fixed some of that -- see the pgtable-nx patch on firstfloor -- but > it still doesn't work. Or maybe my patch was not complete. >
Yeah, that looks like the right sort of thing, but I wonder if there's other places doing an open-coded "pte_val(pte) & ~_PAGE_FOO". My patch changes the definition of _PAGE_FOO so it should be OK everywhere.
>> The original code just used signed constants for the _PAGE_X >> definitions, which will sign-extend when cast to 64-bit, and so have the >> upper bits set when masking. (Well, actually, the old code just >> operated on pte_low, so the problem didn't arise; however, pgtable_64.h >> also uses integers for its _PAGE_X, which has the same sign-extended >> 32->64 casting property). >> >> I'll put together a fixup patch now. >> > > I'm leaving now but can test later. >
Can you try this out? It applies after "x86: move all asm/pgtable constants into one place".
Subject: x86/pgtable: fix constant sign extension problem
When the _PAGE_FOO constants are defined as (1ul << _PAGE_BIT_FOO), they become unsigned longs. In 32-bit PAE mode, these end up being implicitly cast to 64-bit types when used to manipulate a pte, and because they're unsigned the top 32-bits are 0, destroying the upper bits of the pte.
When _PAGE_FOO constants are given a signed integer type, the cast to 64-bits will sign-extend so that the upper bits are all ones, preserving the upper pte bits in manipulations.
Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com> Cc: Andi Kleen <ak@suse.de>
--- include/asm-x86/pgtable.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
=================================================================== --- a/include/asm-x86/pgtable.h +++ b/include/asm-x86/pgtable.h @@ -19,18 +19,18 @@ #define _PAGE_BIT_UNUSED3 11 #define _PAGE_BIT_NX 63 /* No execute: only valid after cpuid check */ -#define _PAGE_PRESENT (_AC(1, UL)<<_PAGE_BIT_PRESENT) -#define _PAGE_RW (_AC(1, UL)<<_PAGE_BIT_RW) -#define _PAGE_USER (_AC(1, UL)<<_PAGE_BIT_USER) -#define _PAGE_PWT (_AC(1, UL)<<_PAGE_BIT_PWT) -#define _PAGE_PCD (_AC(1, UL)<<_PAGE_BIT_PCD) -#define _PAGE_ACCESSED (_AC(1, UL)<<_PAGE_BIT_ACCESSED) -#define _PAGE_DIRTY (_AC(1, UL)<<_PAGE_BIT_DIRTY) -#define _PAGE_PSE (_AC(1, UL)<<_PAGE_BIT_PSE) /* 2MB page */ -#define _PAGE_GLOBAL (_AC(1, UL)<<_PAGE_BIT_GLOBAL) /* Global TLB entry */ -#define _PAGE_UNUSED1 (_AC(1, UL)<<_PAGE_BIT_UNUSED1) -#define _PAGE_UNUSED2 (_AC(1, UL)<<_PAGE_BIT_UNUSED2) -#define _PAGE_UNUSED3 (_AC(1, UL)<<_PAGE_BIT_UNUSED3) +#define _PAGE_PRESENT (_AC(1, L)<<_PAGE_BIT_PRESENT) +#define _PAGE_RW (_AC(1, L)<<_PAGE_BIT_RW) +#define _PAGE_USER (_AC(1, L)<<_PAGE_BIT_USER) +#define _PAGE_PWT (_AC(1, L)<<_PAGE_BIT_PWT) +#define _PAGE_PCD (_AC(1, L)<<_PAGE_BIT_PCD) +#define _PAGE_ACCESSED (_AC(1, L)<<_PAGE_BIT_ACCESSED) +#define _PAGE_DIRTY (_AC(1, L)<<_PAGE_BIT_DIRTY) +#define _PAGE_PSE (_AC(1, L)<<_PAGE_BIT_PSE) /* 2MB page */ +#define _PAGE_GLOBAL (_AC(1, L)<<_PAGE_BIT_GLOBAL) /* Global TLB entry */ +#define _PAGE_UNUSED1 (_AC(1, L)<<_PAGE_BIT_UNUSED1) +#define _PAGE_UNUSED2 (_AC(1, L)<<_PAGE_BIT_UNUSED2) +#define _PAGE_UNUSED3 (_AC(1, L)<<_PAGE_BIT_UNUSED3) #if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE) #define _PAGE_NX (_AC(1, ULL) << _PAGE_BIT_NX)
| |