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?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> 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 thisI''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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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?> > -- KeirJun --- Intel Open Source Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > 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, Iguess> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel