32-on-64 support in xen-unstable?

32-on-64 support in xen-unstable?

[Chris Lalancette]

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

Re: 32-on-64 support in xen-unstable?

[Jan Beulich]

>>> 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

Re: 32-on-64 support in xen-unstable?

[Chris Lalancette]

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

Re: 32-on-64 support in xen-unstable?

[Keir Fraser]

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