x86-64 machine_to_phys vs NX bit

x86-64 machine_to_phys vs NX bit

[Rik van Riel]

On x86-64 machine_to_phys looks like it should never succeed, yet I'm
guessing it must somehow be lucky...

The problem would be that the NX bit is bit 63 of the pte, meaning that
when pte_val is called we are working with a value 2^63 higher than we
should be.

#define pte_val(x)      (((x).pte & 1) ? machine_to_phys((x).pte) : \
                          (x).pte)

static inline paddr_t machine_to_phys(maddr_t machine)
{
         paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
         phys = (phys << PAGE_SHIFT) | (machine & ~PAGE_MASK);
         return phys;
}

Should we mask the 'machine' variable with PHYSICAL_MASK at
some point so we cut off the NX bit and other reserved bits?

Say, something like the following?

-    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
+    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) & PHYSICAL_MASK);

I'm still thinking I may have missed something in the code
somewhere, but I've been looking at this for over an hour now
and can't seem to find it...

Any ideas?

-- 
What is important?  What you want to be true, or what is true?

Re: x86-64 machine_to_phys vs NX bit

[Keir Fraser]

On 24/8/06 8:25 pm, "Rik van Riel" <riel@redhat.com> wrote:

> Say, something like the following?
> 
> -    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
> +    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) & PHYSICAL_MASK);
> 
> I'm still thinking I may have missed something in the code
> somewhere, but I've been looking at this for over an hour now
> and can't seem to find it...
> 
> Any ideas?

Your suggested patch looks reasonable but it'd be good to find out why this
hasn't caused us problems. For example, perhaps supported_pte_mask doesn't
include PAGE_NX, so we're never setting the NX bit on 64-bit PTEs? That must
be worth checking out, possibly also tracing machine_to_phys to find out
where that bit 63 goes -- I agree that it looks like mfn_to_pfn() shouldn#t
work if bit63 is set in the 'maddr' argument.

 -- Keir

Re: x86-64 machine_to_phys vs NX bit

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 25.08.06 09:32 >>>
>On 24/8/06 8:25 pm, "Rik van Riel" <riel@redhat.com> wrote:
>
>> Say, something like the following?
>> 
>> -    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
>> +    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) & PHYSICAL_MASK);
>> 
>> I'm still thinking I may have missed something in the code
>> somewhere, but I've been looking at this for over an hour now
>> and can't seem to find it...
>> 
>> Any ideas?
>
>Your suggested patch looks reasonable but it'd be good to find out why this

I'd suggest not changing machine_to_phys(), but its callers (where needed), in
order to prevent hiding errors.

>hasn't caused us problems. For example, perhaps supported_pte_mask doesn't
>include PAGE_NX, so we're never setting the NX bit on 64-bit PTEs? That must

__supported_pte_mask is all ones on a system I just checked this on; pte_val()
for a page with _PAGE_NX set indeed returns (end_pfn << PAGE_SHIFT) | prot_flags.
With that I guess there are just too few pages making use of _PAGE_NX yet,
so the problem went unnoticed so far.

>be worth checking out, possibly also tracing machine_to_phys to find out
>where that bit 63 goes -- I agree that it looks like mfn_to_pfn() shouldn#t
>work if bit63 is set in the 'maddr' argument.

Jan

Re: x86-64 machine_to_phys vs NX bit

[Rik van Riel]

Keir Fraser wrote:
> On 24/8/06 8:25 pm, "Rik van Riel" <riel@redhat.com> wrote:
> 
>> Say, something like the following?
>>
>> -    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
>> +    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) & PHYSICAL_MASK);
>>
>> I'm still thinking I may have missed something in the code
>> somewhere, but I've been looking at this for over an hour now
>> and can't seem to find it...
>>
>> Any ideas?
> 
> Your suggested patch looks reasonable but it'd be good to find out why this
> hasn't caused us problems. For example, perhaps supported_pte_mask doesn't
> include PAGE_NX, so we're never setting the NX bit on 64-bit PTEs?

We do set the NX bit.  Including on vmalloced pages...

> That must be worth checking out, possibly also tracing 
> machine_to_phys to find out where that bit 63 goes -- I agree that it
> looks like mfn_to_pfn() shouldn't work if bit63 is set in the
> 'maddr' argument.

Absolutely :)

-- 
What is important?  What you want to be true, or what is true?

RE: x86-64 machine_to_phys vs NX bit

[Nakajima, Jun]

Rik van Riel wrote:
> Keir Fraser wrote:
>> On 24/8/06 8:25 pm, "Rik van Riel" <riel@redhat.com> wrote:
>> 
>>> Say, something like the following?
>>> 
>>> -    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
>>> +    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) &
>>> PHYSICAL_MASK); 
>>> 
>>> I'm still thinking I may have missed something in the code
>>> somewhere, but I've been looking at this for over an hour now
>>> and can't seem to find it...
>>> 
>>> Any ideas?
>> 
>> Your suggested patch looks reasonable but it'd be good to find out
>> why this hasn't caused us problems. For example, perhaps
>> supported_pte_mask doesn't include PAGE_NX, so we're never setting
>> the NX bit on 64-bit PTEs? 
> 
> We do set the NX bit.  Including on vmalloced pages...
> 
>> That must be worth checking out, possibly also tracing
>> machine_to_phys to find out where that bit 63 goes -- I agree that it
>> looks like mfn_to_pfn() shouldn't work if bit63 is set in the
>> 'maddr' argument.
> 
> Absolutely :)

I agree, and I'm wondering why we don't have the same problem on i386?
To me it basically does the same thing.

static inline unsigned long long pte_val(pte_t x)
{
        unsigned long long ret;

        if (x.pte_low) {
                ret = x.pte_low | (unsigned long long)x.pte_high << 32;
                ret = machine_to_phys(ret) | 1;
        } else {
                ret = 0;
        }
        return ret;
}


Jun
---
Intel Open Source Technology Center

Re: x86-64 machine_to_phys vs NX bit

[Keir Fraser]

On 25/8/06 3:46 pm, "Nakajima, Jun" <jun.nakajima@intel.com> wrote:

> I agree, and I'm wondering why we don't have the same problem on i386?
> To me it basically does the same thing.

A long is only 32 bits there, so when we pass the MFN portion the NX bit is
conveniently truncated away!

 -- Keir

Re: x86-64 machine_to_phys vs NX bit

[Rik van Riel]

Keir Fraser wrote:
> 
> 
> On 25/8/06 3:46 pm, "Nakajima, Jun" <jun.nakajima@intel.com> wrote:
> 
>> I agree, and I'm wondering why we don't have the same problem on i386?
>> To me it basically does the same thing.
> 
> A long is only 32 bits there, so when we pass the MFN portion the NX bit is
> conveniently truncated away!

Which means it'll do the wrong thing for machine addresses > 4GB
on PAE, or am I overlooking something?

-- 
What is important?  What you want to be true, or what is true?

Re: x86-64 machine_to_phys vs NX bit

[Keir Fraser]

On 25/8/06 4:19 pm, "Rik van Riel" <riel@redhat.com> wrote:

>> A long is only 32 bits there, so when we pass the MFN portion the NX bit is
>> conveniently truncated away!
> 
> Which means it'll do the wrong thing for machine addresses > 4GB
> on PAE, or am I overlooking something?

No, it gets shifted right 12 and *then* converted to a long. So you get a
44-bit addressing capability (32+12). But NX bit is bit 63, so it gets
truncated.

 -- Keir

RE: x86-64 machine_to_phys vs NX bit

[Nakajima, Jun]

Keir Fraser wrote:
> On 25/8/06 4:19 pm, "Rik van Riel" <riel@redhat.com> wrote:
> 
>>> A long is only 32 bits there, so when we pass the MFN portion the
>>> NX bit is conveniently truncated away!
>> 
>> Which means it'll do the wrong thing for machine addresses > 4GB
>> on PAE, or am I overlooking something?
> 
> No, it gets shifted right 12 and *then* converted to a long. So you
> get a 44-bit addressing capability (32+12). But NX bit is bit 63, so
> it gets truncated.

Yes. For (on-going) 32-bit PV guests running on the 64-bit Xen, I guess
we should fix the convenient optimization now? 

> 
>  -- Keir

Jun
---
Intel Open Source Technology Center

RE: x86-64 machine_to_phys vs NX bit

[Ian Pratt]

> > No, it gets shifted right 12 and *then* converted to a long. So you
> > get a 44-bit addressing capability (32+12). But NX bit is bit 63, so
> > it gets truncated.
> 
> Yes. For (on-going) 32-bit PV guests running on the 64-bit Xen, I
guess
> we should fix the convenient optimization now?

Or we restrict 32b guests to the bottom 16 terabytes of memory. Please
send me a machine for testing the patch :-)

Ian

RE: x86-64 machine_to_phys vs NX bit

[Nakajima, Jun]

Ian Pratt wrote:
>>> No, it gets shifted right 12 and *then* converted to a long. So you
>>> get a 44-bit addressing capability (32+12). But NX bit is bit 63, so
>>> it gets truncated.
>> 
>> Yes. For (on-going) 32-bit PV guests running on the 64-bit Xen, I
>> guess we should fix the convenient optimization now?
> 
> Or we restrict 32b guests to the bottom 16 terabytes of memory. Please
> send me a machine for testing the patch :-)
> 
> Ian
> 

No, we don't :-) It cannot happen on the architecture implementations
that exist today. Maybe ASSERT() would be sufficient so that it can
remind us of the issues in years.

Jun
---
Intel Open Source Technology Center

Re: x86-64 machine_to_phys vs NX bit

[John Byrne]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

Keir Fraser wrote:
> On 24/8/06 8:25 pm, "Rik van Riel" <riel@redhat.com> wrote:
> 
>> Say, something like the following?
>> 
>> -    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
>> +    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) & PHYSICAL_MASK);
>> 
>> I'm still thinking I may have missed something in the code
>> somewhere, but I've been looking at this for over an hour now
>> and can't seem to find it...
>> 
>> Any ideas?
> 
> Your suggested patch looks reasonable but it'd be good to find out why this
> hasn't caused us problems. For example, perhaps supported_pte_mask doesn't
> include PAGE_NX, so we're never setting the NX bit on 64-bit PTEs? That must
> be worth checking out, possibly also tracing machine_to_phys to find out
> where that bit 63 goes -- I agree that it looks like mfn_to_pfn() shouldn#t
> work if bit63 is set in the 'maddr' argument.
> 
>  -- Keir
> 
> 

While trying to debug a migration problem in Xen 3.0.3 I have noticed
this issue. I don't see a fix in xen-unstable. Has this gotten dropped
on the floor?

The suggested patch above is not quite correct or complete. My proposed
patch aqainst xen-unstable changeset 12364:d19deb173503 is attached. 
Note that there is also an issue in x86 PAE: machine_to_phys() currently 
will strip the NX bit.

Signed-off-by: John Byrne <john.l.byrne@hp.com>








[-- Attachment #2: nxmask.patch --]
[-- Type: text/x-patch, Size: 3583 bytes --]

diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h
--- a/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h	Fri Nov 10 21:37:45 2006 -0600
@@ -127,10 +127,17 @@ static inline maddr_t phys_to_machine(pa
 	machine = (machine << PAGE_SHIFT) | (phys & ~PAGE_MASK);
 	return machine;
 }
+
 static inline paddr_t machine_to_phys(maddr_t machine)
 {
+	/*
+	 * In PAE mode, the NX bit needs to be dealt with in the value
+	 * passed to mfn_to_pfn(). On x86_64, we need to mask it off,
+	 * but for i386 the conversion to ulong for the argument will
+	 * clip it off.
+	 */
 	paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
-	phys = (phys << PAGE_SHIFT) | (machine & ~PAGE_MASK);
+	phys = (phys << PAGE_SHIFT) | (machine & ~PHYSICAL_PAGE_MASK);
 	return phys;
 }
 
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h
--- a/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h	Fri Nov 10 21:37:45 2006 -0600
@@ -5,6 +5,16 @@
 #define PAGE_SHIFT	12
 #define PAGE_SIZE	(1UL << PAGE_SHIFT)
 #define PAGE_MASK	(~(PAGE_SIZE-1))
+
+#ifdef CONFIG_X86_PAE
+#define __PHYSICAL_MASK_SHIFT	36
+#define __PHYSICAL_MASK		((1ULL << __PHYSICAL_MASK_SHIFT) - 1)
+#else
+#define __PHYSICAL_MASK_SHIFT	32
+#define __PHYSICAL_MASK		(~0UL)
+#endif
+
+#define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)
 
 #define LARGE_PAGE_MASK (~(LARGE_PAGE_SIZE-1))
 #define LARGE_PAGE_SIZE (1UL << PMD_SHIFT)
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h	Fri Nov 10 21:36:35 2006 -0600
@@ -122,8 +122,8 @@ static inline maddr_t phys_to_machine(pa
 
 static inline paddr_t machine_to_phys(maddr_t machine)
 {
-	paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
-	phys = (phys << PAGE_SHIFT) | (machine & ~PAGE_MASK);
+	paddr_t phys = mfn_to_pfn((machine & PHYSICAL_PAGE_MASK) >> PAGE_SHIFT);
+	phys = (phys << PAGE_SHIFT) | (machine & ~PHYSICAL_PAGE_MASK);
 	return phys;
 }
 
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h	Fri Nov 10 21:36:35 2006 -0600
@@ -33,6 +33,13 @@
 #define PAGE_SIZE	(1UL << PAGE_SHIFT)
 #endif
 #define PAGE_MASK	(~(PAGE_SIZE-1))
+
+/* See Documentation/x86_64/mm.txt for a description of the memory map. */
+#define __PHYSICAL_MASK_SHIFT	46
+#define __PHYSICAL_MASK		((1UL << __PHYSICAL_MASK_SHIFT) - 1)
+#define __VIRTUAL_MASK_SHIFT	48
+#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
+
 #define PHYSICAL_PAGE_MASK	(~(PAGE_SIZE-1) & __PHYSICAL_MASK)
 
 #define THREAD_ORDER 1 
@@ -162,12 +169,6 @@ static inline pgd_t __pgd(unsigned long 
 
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
-
-/* See Documentation/x86_64/mm.txt for a description of the memory map. */
-#define __PHYSICAL_MASK_SHIFT	46
-#define __PHYSICAL_MASK		((1UL << __PHYSICAL_MASK_SHIFT) - 1)
-#define __VIRTUAL_MASK_SHIFT	48
-#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 #define KERNEL_TEXT_SIZE  (40UL*1024*1024)
 #define KERNEL_TEXT_START 0xffffffff80000000UL 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: x86-64 machine_to_phys vs NX bit

[John Byrne]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

John Byrne wrote:
> Keir Fraser wrote:
>> On 24/8/06 8:25 pm, "Rik van Riel" <riel@redhat.com> wrote:
>>
>>> Say, something like the following?
>>>
>>> -    paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
>>> +    paddr_t phys = mfn_to_pfn((machine >> PAGE_SHIFT) & PHYSICAL_MASK);
>>>
>>> I'm still thinking I may have missed something in the code
>>> somewhere, but I've been looking at this for over an hour now
>>> and can't seem to find it...
>>>
>>> Any ideas?
>>
>> Your suggested patch looks reasonable but it'd be good to find out why 
>> this
>> hasn't caused us problems. For example, perhaps supported_pte_mask 
>> doesn't
>> include PAGE_NX, so we're never setting the NX bit on 64-bit PTEs? 
>> That must
>> be worth checking out, possibly also tracing machine_to_phys to find out
>> where that bit 63 goes -- I agree that it looks like mfn_to_pfn() 
>> shouldn#t
>> work if bit63 is set in the 'maddr' argument.
>>
>>  -- Keir
>>
>>
> 
> While trying to debug a migration problem in Xen 3.0.3 I have noticed
> this issue. I don't see a fix in xen-unstable. Has this gotten dropped
> on the floor?
> 
> The suggested patch above is not quite correct or complete. My proposed
> patch aqainst xen-unstable changeset 12364:d19deb173503 is attached. 
> Note that there is also an issue in x86 PAE: machine_to_phys() currently 
> will strip the NX bit.
> 
> Signed-off-by: John Byrne <john.l.byrne@hp.com>
> 
<...snipped...>

There was a bug in my previous patch. (There's nothing like trying to 
get to sleep and realizing you've screwed up.) The x86 pae 
PHYSICAL_PAGE_MASK I defined was incorrect because PAGE_MASK was only a 
long. I hope I haven't done anything else wrong.

John Byrne

Signed-off-by: John Byrne <john.l.byrne@hp.com>



[-- Attachment #2: nxmask2.patch --]
[-- Type: text/x-patch, Size: 3657 bytes --]

diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h
--- a/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h	Fri Nov 10 21:37:45 2006 -0600
@@ -127,10 +127,17 @@ static inline maddr_t phys_to_machine(pa
 	machine = (machine << PAGE_SHIFT) | (phys & ~PAGE_MASK);
 	return machine;
 }
+
 static inline paddr_t machine_to_phys(maddr_t machine)
 {
+	/*
+	 * In PAE mode, the NX bit needs to be dealt with in the value
+	 * passed to mfn_to_pfn(). On x86_64, we need to mask it off,
+	 * but for i386 the conversion to ulong for the argument will
+	 * clip it off.
+	 */
 	paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
-	phys = (phys << PAGE_SHIFT) | (machine & ~PAGE_MASK);
+	phys = (phys << PAGE_SHIFT) | (machine & ~PHYSICAL_PAGE_MASK);
 	return phys;
 }
 
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h
--- a/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h	Sat Nov 11 03:14:00 2006 -0600
@@ -5,6 +5,16 @@
 #define PAGE_SHIFT	12
 #define PAGE_SIZE	(1UL << PAGE_SHIFT)
 #define PAGE_MASK	(~(PAGE_SIZE-1))
+
+#ifdef CONFIG_X86_PAE
+#define __PHYSICAL_MASK_SHIFT	36
+#define __PHYSICAL_MASK		((1ULL << __PHYSICAL_MASK_SHIFT) - 1)
+#define PHYSICAL_PAGE_MASK	(~((1ULL << PAGE_SHIFT) - 1) & __PHYSICAL_MASK)
+#else
+#define __PHYSICAL_MASK_SHIFT	32
+#define __PHYSICAL_MASK		(~0UL)
+#define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)
+#endif
 
 #define LARGE_PAGE_MASK (~(LARGE_PAGE_SIZE-1))
 #define LARGE_PAGE_SIZE (1UL << PMD_SHIFT)
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h	Fri Nov 10 21:36:35 2006 -0600
@@ -122,8 +122,8 @@ static inline maddr_t phys_to_machine(pa
 
 static inline paddr_t machine_to_phys(maddr_t machine)
 {
-	paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
-	phys = (phys << PAGE_SHIFT) | (machine & ~PAGE_MASK);
+	paddr_t phys = mfn_to_pfn((machine & PHYSICAL_PAGE_MASK) >> PAGE_SHIFT);
+	phys = (phys << PAGE_SHIFT) | (machine & ~PHYSICAL_PAGE_MASK);
 	return phys;
 }
 
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h	Fri Nov 10 21:36:35 2006 -0600
@@ -33,6 +33,13 @@
 #define PAGE_SIZE	(1UL << PAGE_SHIFT)
 #endif
 #define PAGE_MASK	(~(PAGE_SIZE-1))
+
+/* See Documentation/x86_64/mm.txt for a description of the memory map. */
+#define __PHYSICAL_MASK_SHIFT	46
+#define __PHYSICAL_MASK		((1UL << __PHYSICAL_MASK_SHIFT) - 1)
+#define __VIRTUAL_MASK_SHIFT	48
+#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
+
 #define PHYSICAL_PAGE_MASK	(~(PAGE_SIZE-1) & __PHYSICAL_MASK)
 
 #define THREAD_ORDER 1 
@@ -162,12 +169,6 @@ static inline pgd_t __pgd(unsigned long 
 
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
-
-/* See Documentation/x86_64/mm.txt for a description of the memory map. */
-#define __PHYSICAL_MASK_SHIFT	46
-#define __PHYSICAL_MASK		((1UL << __PHYSICAL_MASK_SHIFT) - 1)
-#define __VIRTUAL_MASK_SHIFT	48
-#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 #define KERNEL_TEXT_SIZE  (40UL*1024*1024)
 #define KERNEL_TEXT_START 0xffffffff80000000UL 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: x86-64 machine_to_phys vs NX bit

[Jan Beulich]

>> While trying to debug a migration problem in Xen 3.0.3 I have noticed
>> this issue. I don't see a fix in xen-unstable. Has this gotten dropped
>> on the floor?
>> 
>> The suggested patch above is not quite correct or complete. My proposed
>> patch aqainst xen-unstable changeset 12364:d19deb173503 is attached. 
>> Note that there is also an issue in x86 PAE: machine_to_phys() currently 
>> will strip the NX bit.
>> 
>> Signed-off-by: John Byrne <john.l.byrne@hp.com>
>> 
><...snipped...>
>
>There was a bug in my previous patch. (There's nothing like trying to 
>get to sleep and realizing you've screwed up.) The x86 pae 
>PHYSICAL_PAGE_MASK I defined was incorrect because PAGE_MASK was only a 
>long. I hope I haven't done anything else wrong.

I don't think this is correct - machine_to_phys() translates a machine address
to a physical one, and in that translation the upper bits matter only as much
as mfn_to_pfn() should return an invalid indicator if any of them is set. In turn,
it should be the caller's responsibility to make sure the NX bit (and any potential
other one being set beyond bit 52) gets masked off *before* calling this
function. (Specifically, the preserving of the lower bits is to properly translate
a non-page aligned address, not to preserve attribute bits read from a page
table entry).

Jan

Re: x86-64 machine_to_phys vs NX bit

[Keir Fraser]

On 13/11/06 8:07 am, "Jan Beulich" <jbeulich@novell.com> wrote:

>> There was a bug in my previous patch. (There's nothing like trying to
>> get to sleep and realizing you've screwed up.) The x86 pae
>> PHYSICAL_PAGE_MASK I defined was incorrect because PAGE_MASK was only a
>> long. I hope I haven't done anything else wrong.
> 
> I don't think this is correct - machine_to_phys() translates a machine address
> to a physical one, and in that translation the upper bits matter only as much
> as mfn_to_pfn() should return an invalid indicator if any of them is set. In
> turn,
> it should be the caller's responsibility to make sure the NX bit (and any
> potential
> other one being set beyond bit 52) gets masked off *before* calling this
> function. (Specifically, the preserving of the lower bits is to properly
> translate
> a non-page aligned address, not to preserve attribute bits read from a page
> table entry).

Yes, we should keep the old machine_to_phys() definition and rename John's
new version as pte_machine_to_phys(). The latter should be used in all
contexts where machine_to_phys() currently operates on a pte (that's most of
its uses, actually). This is a worthwhile cleanup and clarification. Could
you respin the patch, John?

 Thanks,
 Keir

Re: x86-64 machine_to_phys vs NX bit

[John Byrne]

Keir Fraser wrote:
> On 13/11/06 8:07 am, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>>> There was a bug in my previous patch. (There's nothing like trying to
>>> get to sleep and realizing you've screwed up.) The x86 pae
>>> PHYSICAL_PAGE_MASK I defined was incorrect because PAGE_MASK was only a
>>> long. I hope I haven't done anything else wrong.
>> I don't think this is correct - machine_to_phys() translates a machine address
>> to a physical one, and in that translation the upper bits matter only as much
>> as mfn_to_pfn() should return an invalid indicator if any of them is set. In
>> turn,
>> it should be the caller's responsibility to make sure the NX bit (and any
>> potential
>> other one being set beyond bit 52) gets masked off *before* calling this
>> function. (Specifically, the preserving of the lower bits is to properly
>> translate
>> a non-page aligned address, not to preserve attribute bits read from a page
>> table entry).
> 
> Yes, we should keep the old machine_to_phys() definition and rename John's
> new version as pte_machine_to_phys(). The latter should be used in all
> contexts where machine_to_phys() currently operates on a pte (that's most of
> its uses, actually). This is a worthwhile cleanup and clarification. Could
> you respin the patch, John?
> 
>  Thanks,
>  Keir
> 
> 
> 

I've  made the change. I'll send it out after I've built and tested it.

John

Re: x86-64 machine_to_phys vs NX bit

[John Byrne]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

John Byrne wrote:
> Keir Fraser wrote:
>> On 13/11/06 8:07 am, "Jan Beulich" <jbeulich@novell.com> wrote:
>>
>>>> There was a bug in my previous patch. (There's nothing like trying to
>>>> get to sleep and realizing you've screwed up.) The x86 pae
>>>> PHYSICAL_PAGE_MASK I defined was incorrect because PAGE_MASK was only a
>>>> long. I hope I haven't done anything else wrong.
>>> I don't think this is correct - machine_to_phys() translates a 
>>> machine address
>>> to a physical one, and in that translation the upper bits matter only 
>>> as much
>>> as mfn_to_pfn() should return an invalid indicator if any of them is 
>>> set. In
>>> turn,
>>> it should be the caller's responsibility to make sure the NX bit (and 
>>> any
>>> potential
>>> other one being set beyond bit 52) gets masked off *before* calling this
>>> function. (Specifically, the preserving of the lower bits is to properly
>>> translate
>>> a non-page aligned address, not to preserve attribute bits read from 
>>> a page
>>> table entry).
>>
>> Yes, we should keep the old machine_to_phys() definition and rename 
>> John's
>> new version as pte_machine_to_phys(). The latter should be used in all
>> contexts where machine_to_phys() currently operates on a pte (that's 
>> most of
>> its uses, actually). This is a worthwhile cleanup and clarification. 
>> Could
>> you respin the patch, John?
>>
>>  Thanks,
>>  Keir
>>
>>
>>
> 
> I've  made the change. I'll send it out after I've built and tested it.
> 
> John
> 
> 

Here's the new patch.

Signed-off-by: John Byrne <john.l.byrne@hp.com>



[-- Attachment #2: nxmask3.patch --]
[-- Type: text/x-patch, Size: 6170 bytes --]

diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h
--- a/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/maddr.h	Mon Nov 13 14:32:15 2006 -0600
@@ -127,10 +127,24 @@ static inline maddr_t phys_to_machine(pa
 	machine = (machine << PAGE_SHIFT) | (phys & ~PAGE_MASK);
 	return machine;
 }
+
 static inline paddr_t machine_to_phys(maddr_t machine)
 {
 	paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
 	phys = (phys << PAGE_SHIFT) | (machine & ~PAGE_MASK);
+	return phys;
+}
+
+static inline paddr_t pte_machine_to_phys(maddr_t machine)
+{
+	/*
+	 * In PAE mode, the NX bit needs to be dealt with in the value
+	 * passed to mfn_to_pfn(). On x86_64, we need to mask it off,
+	 * but for i386 the conversion to ulong for the argument will
+	 * clip it off.
+	 */
+	paddr_t phys = mfn_to_pfn(machine >> PAGE_SHIFT);
+	phys = (phys << PAGE_SHIFT) | (machine & ~PHYSICAL_PAGE_MASK);
 	return phys;
 }
 
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h
--- a/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-i386/mach-xen/asm/page.h	Mon Nov 13 14:31:16 2006 -0600
@@ -5,6 +5,16 @@
 #define PAGE_SHIFT	12
 #define PAGE_SIZE	(1UL << PAGE_SHIFT)
 #define PAGE_MASK	(~(PAGE_SIZE-1))
+
+#ifdef CONFIG_X86_PAE
+#define __PHYSICAL_MASK_SHIFT	36
+#define __PHYSICAL_MASK		((1ULL << __PHYSICAL_MASK_SHIFT) - 1)
+#define PHYSICAL_PAGE_MASK	(~((1ULL << PAGE_SHIFT) - 1) & __PHYSICAL_MASK)
+#else
+#define __PHYSICAL_MASK_SHIFT	32
+#define __PHYSICAL_MASK		(~0UL)
+#define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)
+#endif
 
 #define LARGE_PAGE_MASK (~(LARGE_PAGE_SIZE-1))
 #define LARGE_PAGE_SIZE (1UL << PMD_SHIFT)
@@ -85,7 +95,7 @@ static inline unsigned long long pte_val
 
 	if (x.pte_low) {
 		ret = x.pte_low | (unsigned long long)x.pte_high << 32;
-		ret = machine_to_phys(ret) | 1;
+		ret = pte_machine_to_phys(ret) | 1;
 	} else {
 		ret = 0;
 	}
@@ -94,13 +104,13 @@ static inline unsigned long long pmd_val
 static inline unsigned long long pmd_val(pmd_t x)
 {
 	unsigned long long ret = x.pmd;
-	if (ret) ret = machine_to_phys(ret) | 1;
+	if (ret) ret = pte_machine_to_phys(ret) | 1;
 	return ret;
 }
 static inline unsigned long long pgd_val(pgd_t x)
 {
 	unsigned long long ret = x.pgd;
-	if (ret) ret = machine_to_phys(ret) | 1;
+	if (ret) ret = pte_machine_to_phys(ret) | 1;
 	return ret;
 }
 static inline unsigned long long pte_val_ma(pte_t x)
@@ -115,7 +125,8 @@ typedef struct { unsigned long pgprot; }
 #define pgprot_val(x)	((x).pgprot)
 #include <asm/maddr.h>
 #define boot_pte_t pte_t /* or would you rather have a typedef */
-#define pte_val(x)	(((x).pte_low & 1) ? machine_to_phys((x).pte_low) : \
+#define pte_val(x)	(((x).pte_low & 1) ? \
+			 pte_machine_to_phys((x).pte_low) : \
 			 (x).pte_low)
 #define pte_val_ma(x)	((x).pte_low)
 #define __pte(x) ({ unsigned long _x = (x); \
@@ -125,7 +136,7 @@ static inline unsigned long pgd_val(pgd_
 static inline unsigned long pgd_val(pgd_t x)
 {
 	unsigned long ret = x.pgd;
-	if (ret) ret = machine_to_phys(ret) | 1;
+	if (ret) ret = pte_machine_to_phys(ret) | 1;
 	return ret;
 }
 #define HPAGE_SHIFT	22
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/maddr.h	Mon Nov 13 14:35:30 2006 -0600
@@ -127,6 +127,14 @@ static inline paddr_t machine_to_phys(ma
 	return phys;
 }
 
+static inline paddr_t pte_machine_to_phys(maddr_t machine)
+{
+	paddr_t phys;
+	phys = mfn_to_pfn((machine & PHYSICAL_PAGE_MASK) >> PAGE_SHIFT);
+	phys = (phys << PAGE_SHIFT) | (machine & ~PHYSICAL_PAGE_MASK);
+	return phys;
+}
+
 /* VIRT <-> MACHINE conversion */
 #define virt_to_machine(v)	(phys_to_machine(__pa(v)))
 #define virt_to_mfn(v)		(pfn_to_mfn(__pa(v) >> PAGE_SHIFT))
diff -r d19deb173503 linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h	Fri Nov 10 15:27:22 2006 +0000
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/page.h	Mon Nov 13 14:33:02 2006 -0600
@@ -33,6 +33,13 @@
 #define PAGE_SIZE	(1UL << PAGE_SHIFT)
 #endif
 #define PAGE_MASK	(~(PAGE_SIZE-1))
+
+/* See Documentation/x86_64/mm.txt for a description of the memory map. */
+#define __PHYSICAL_MASK_SHIFT	46
+#define __PHYSICAL_MASK		((1UL << __PHYSICAL_MASK_SHIFT) - 1)
+#define __VIRTUAL_MASK_SHIFT	48
+#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
+
 #define PHYSICAL_PAGE_MASK	(~(PAGE_SIZE-1) & __PHYSICAL_MASK)
 
 #define THREAD_ORDER 1 
@@ -90,28 +97,28 @@ typedef struct { unsigned long pgd; } pg
 
 typedef struct { unsigned long pgprot; } pgprot_t;
 
-#define pte_val(x)	(((x).pte & 1) ? machine_to_phys((x).pte) : \
+#define pte_val(x)	(((x).pte & 1) ? pte_machine_to_phys((x).pte) : \
 			 (x).pte)
 #define pte_val_ma(x)	((x).pte)
 
 static inline unsigned long pmd_val(pmd_t x)
 {
 	unsigned long ret = x.pmd;
-	if (ret) ret = machine_to_phys(ret);
+	if (ret) ret = pte_machine_to_phys(ret);
 	return ret;
 }
 
 static inline unsigned long pud_val(pud_t x)
 {
 	unsigned long ret = x.pud;
-	if (ret) ret = machine_to_phys(ret);
+	if (ret) ret = pte_machine_to_phys(ret);
 	return ret;
 }
 
 static inline unsigned long pgd_val(pgd_t x)
 {
 	unsigned long ret = x.pgd;
-	if (ret) ret = machine_to_phys(ret);
+	if (ret) ret = pte_machine_to_phys(ret);
 	return ret;
 }
 
@@ -162,12 +169,6 @@ static inline pgd_t __pgd(unsigned long 
 
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
-
-/* See Documentation/x86_64/mm.txt for a description of the memory map. */
-#define __PHYSICAL_MASK_SHIFT	46
-#define __PHYSICAL_MASK		((1UL << __PHYSICAL_MASK_SHIFT) - 1)
-#define __VIRTUAL_MASK_SHIFT	48
-#define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
 #define KERNEL_TEXT_SIZE  (40UL*1024*1024)
 #define KERNEL_TEXT_START 0xffffffff80000000UL 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: x86-64 machine_to_phys vs NX bit

[Jan Beulich]

>Here's the new patch.

One more comment: Any reason __PHYSICAL_MASK_SHIFT etc. can't be
in maddr.h, where they would more logically belong?

Jan

Re: x86-64 machine_to_phys vs NX bit

[Keir Fraser]

On 14/11/06 8:05 am, "Jan Beulich" <jbeulich@novell.com> wrote:

>> Here's the new patch.
> 
> One more comment: Any reason __PHYSICAL_MASK_SHIFT etc. can't be
> in maddr.h, where they would more logically belong?

Those definitions already exist in x86/64's page.h, and maddr.h currently
contains only Xen-specific definitions. So it probably makes sense to leave
them in page.h on both architecture I think.

 -- Keir