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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel