Re: [Xen-devel] [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.15 at 12:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> In tree, there is one single caller of XEN_DOMCTL_getpageframeinfo3
> (xc_get_pfn_type_batch()), and no callers of the older variants.
> 
> getpageframeinfo3 and getpageframeinfo2 are compatible if the parameter
> contents are considered to be unsigned long; a compat guest calling
> getpageframeinfo3 falls through into the getpageframeinfo2 handler.
> 
> However, getpageframeinfo3 and getpageframeinfo2 have different algorithms 
> for
> calculating the eventual frame type, which means that a toolstack will get
> different answers depending on whether it is compat or not, which is a 
> problem
> for all possible uses.

Is there any other difference besides the former being capable of
returning XEN_DOMCTL_PFINFO_BROKEN (which I would suppose to
have been a later addition that didn't get properly sync-ed to the
older handlers)?

> @@ -378,6 +147,81 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_getpageframeinfo3:
> +    {
> +        unsigned int num = domctl->u.getpageframeinfo3.num;
> +
> +        /* Games to allow this code block to handle a compat guest. */
> +        void * __user guest_handle = domctl->u.getpageframeinfo3.array.p;

The __used belongs between "void" and "*".

Also the blank line above looks somewhat misplaced (I guess you
added it to kind of emphasize the comment).

> +        unsigned int width = has_32bit_shinfo(currd) ? 32 : 64;

These are bit counts, yet where you use the value you want byte
granularity.

> +        if ( unlikely(num > 1024) ||
> +             unlikely(num != domctl->u.getpageframeinfo3.num) )
> +        {
> +            ret = -E2BIG;
> +            break;
> +        }
> +
> +        for ( i = 0; i < num; ++i )
> +        {
> +            unsigned long gfn = 0, type = 0;

gfn's initializer looks pointless (and if anything it should be INVALID_MFN
or some such).

> +            struct page_info *page;
> +            p2m_type_t t;
> +
> +            if ( raw_copy_from_guest(&gfn, guest_handle + (i * width), 
> width) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
> +
> +            if ( unlikely(!page) ||
> +                 unlikely(is_xen_heap_page(page)) )
> +            {
> +                if ( p2m_is_broken(t) )
> +                    type = XEN_DOMCTL_PFINFO_BROKEN;
> +                else
> +                    type = XEN_DOMCTL_PFINFO_XTAB;

Realizing that this was this way in the old code too, would you
nevertheless mind adding unlikely() and/or flip the if and else
branches?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel