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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.15 at 15:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/05/15 12:43, Jan Beulich wrote:
>>>>> On 18.05.15 at 12:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +        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).
> 
> It must absolutely be 0 for when we read a 32bit values into it,

Ah, of course!

> although I realise I do need to extend ~0U to ~0UL for compat guests.

Why is ~0 special here? It's not even valid input afaict, and hence
there's no point in massaging it in any way.

>>> +            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?
> 
> Certainly.
> 
> Does this mean that you are happy in principle with the raw_* use?

I'm not particularly happy about it, but for the purpose of code
consolidation I think it is acceptable here.

Jan


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