Re: [Xen-devel] [PATCH for-4.12 v2 8/8] xen: Switch parameter in get_page_from_gfn to use typesafe gfn

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/12/2018 16:21, Julien Grall wrote:
>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>> index 3304921991..1efbc071c5 100644
>>> --- a/xen/include/asm-x86/p2m.h
>>> +++ b/xen/include/asm-x86/p2m.h
>>> @@ -491,18 +491,21 @@ struct page_info *p2m_get_page_from_gfn(struct
>>> p2m_domain *p2m, gfn_t gfn,
>>>                                           p2m_query_t q);
>>>     static inline struct page_info *get_page_from_gfn(
>>> -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>> +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>>>   {
>>>       struct page_info *page;
>>> +    mfn_t mfn;
>>>         if ( paging_mode_translate(d) )
>>> -        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn),
>>> t, NULL, q);
>>> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t,
>>> NULL, q);
>>>         /* Non-translated guests see 1-1 RAM / MMIO mappings
>>> everywhere */
>>>       if ( t )
>>>           *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
>>> -    page = mfn_to_page(_mfn(gfn));
>>> -    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
>>> +
>>> +    mfn = _mfn(gfn_x(gfn));
>>> +    page = mfn_to_page(mfn);
>>> +    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
>>
>> This unfortunately propagates some bad behaviour, because it is not safe
>> to use mfn_to_page(mfn); before mfn_valid(mfn) succeeds.  (In practice
>> it works because mfn_to_page() is just pointer arithmetic.)
>>
>> Pleas can you express this as:
>>
>> return (mfn_valid(mfn) &&
>>          (page = mfn_to_page(mfn), get_page(page, d))) ? page : NULL;
>>
>> which at least gets the order of operations in the correct order from
>> C's point of view.
>>
>> Alternatively, and perhaps easier to follow:
>>
>> if ( !mfn_valid(mfn) )
>>      return NULL;
>>
>> page = mfn_to_page(mfn);
>>
>> return get_page(page, d) ? page : NULL;
>
> I am happy to fix that. However, shouldn't this be handled in a
> separate patch? After all, the code is not worst than it currently is.

I don't think its worthy of a separate patch.  You're touching the code
anyway, so might as well do it all in one go.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel