Hello,
I''ve been browsing through the preemptible pagetable stuff, and
ran across
a piece of code that I don''t understand or is buggy. Looking at
arch/x86/mm.c:new_guest_cr3(), we have this code for 32-on-64 support:
if ( is_pv_32on64_domain(d) )
{
okay = paging_mode_refcounts(d)
? 0 /* Old code was broken, but what should it be? */
: mod_l4_entry(
__va(pagetable_get_paddr(curr->arch.guest_table)),
l4e_from_pfn(
mfn,
(_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_ACCESSED)),
pagetable_get_pfn(curr->arch.guest_table), 0, 0, curr) ==
0;
if ( unlikely(!okay) )
{
MEM_LOG("Error while installing new compat baseptr %lx",
mfn);
return 0;
}
However, looking at arch/x86/mm.c:mod_l4_entry(), it returns 0 on success and
<
0 on error. It seems to me that booting a 32-bit PV guest is always going to
fall into the !okay check on success, when it really shouldn''t.
Shouldn''t this
be something like:
okay = paging_mode_refcounts(d)
? 0 /* Old code was broken, but what should it be? */
: !mod_l4_entry(
__va(pagetable_get_paddr(curr->arch.guest_table)),
l4e_from_pfn(
mfn,
(_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_ACCESSED)),
pagetable_get_pfn(curr->arch.guest_table), 0, 0, curr) ==
0;
Or am I just missing something? (NOTE: this still leaves open the possibility
for failure if mod_l4_entry() returns EINTR or EAGAIN, but maybe we
don''t have
to worry about that here?)
Thanks,
--
Chris Lalancette
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
>>> Chris Lalancette <clalance@redhat.com> 06.07.09 16:18 >>> >Hello, > I''ve been browsing through the preemptible pagetable stuff, and ran across >a piece of code that I don''t understand or is buggy. Looking at >arch/x86/mm.c:new_guest_cr3(), we have this code for 32-on-64 support:You probably overlooked the == 0 et the end of the expression:> if ( is_pv_32on64_domain(d) ) > { > okay = paging_mode_refcounts(d) > ? 0 /* Old code was broken, but what should it be? */ > : mod_l4_entry( > __va(pagetable_get_paddr(curr->arch.guest_table)), > l4e_from_pfn( > mfn, > (_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_ACCESSED)), > pagetable_get_pfn(curr->arch.guest_table), 0, 0, curr) == 0;^^^^ Which basically is equivalent to what you were trying to suggest. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich wrote:>>>> Chris Lalancette <clalance@redhat.com> 06.07.09 16:18 >>> >> Hello, >> I''ve been browsing through the preemptible pagetable stuff, and ran across >> a piece of code that I don''t understand or is buggy. Looking at >> arch/x86/mm.c:new_guest_cr3(), we have this code for 32-on-64 support: > > You probably overlooked the == 0 et the end of the expression: > >> if ( is_pv_32on64_domain(d) ) >> { >> okay = paging_mode_refcounts(d) >> ? 0 /* Old code was broken, but what should it be? */ >> : mod_l4_entry( >> __va(pagetable_get_paddr(curr->arch.guest_table)), >> l4e_from_pfn( >> mfn, >> (_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_ACCESSED)), >> pagetable_get_pfn(curr->arch.guest_table), 0, 0, curr) == 0; > ^^^^ > > Which basically is equivalent to what you were trying to suggest.D''oh! Of course. I did overlook that, thanks for pointing it out. -- Chris Lalancette _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 06/07/2009 15:18, "Chris Lalancette" <clalance@redhat.com> wrote:> However, looking at arch/x86/mm.c:mod_l4_entry(), it returns 0 on success and > < > 0 on error. It seems to me that booting a 32-bit PV guest is always going to > fall into the !okay check on success, when it really shouldn''t.You didn''t spot the ''== 0'' tacked on the end of that long ternary expression? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel