Re: [PATCH 3/5] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

[Jan Beulich <jbeulich@suse.com>]

On 07.04.2020 17:11, Hongyan Xia wrote:
> On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote:
>> On 23.03.2020 10:41, Hongyan Xia wrote:
>>> --- a/xen/include/asm-x86/page.h
>>> +++ b/xen/include/asm-x86/page.h
>>> @@ -196,6 +196,24 @@ static inline l4_pgentry_t
>>> l4e_from_paddr(paddr_t pa, unsigned int flags)
>>>  #define map_l2t_from_l3e(x)        (l2_pgentry_t
>>> *)map_domain_page(l3e_get_mfn(x))
>>>  #define map_l3t_from_l4e(x)        (l3_pgentry_t
>>> *)map_domain_page(l4e_get_mfn(x))
>>>  
>>> +#define l1e_from_l2e(l2e, off) ({                   \
>>> +        l1_pgentry_t *l1t = map_l1t_from_l2e(l2e);  \
>>> +        l1_pgentry_t l1e = l1t[off];                \
>>> +        UNMAP_DOMAIN_PAGE(l1t);                     \
>>> +        l1e; })
>>> +
>>> +#define l2e_from_l3e(l3e, off) ({                   \
>>> +        l2_pgentry_t *l2t = map_l2t_from_l3e(l3e);  \
>>> +        l2_pgentry_t l2e = l2t[off];                \
>>> +        UNMAP_DOMAIN_PAGE(l2t);                     \
>>> +        l2e; })
>>> +
>>> +#define l3e_from_l4e(l4e, off) ({                   \
>>> +        l3_pgentry_t *l3t = map_l3t_from_l4e(l4e);  \
>>> +        l3_pgentry_t l3e = l3t[off];                \
>>> +        UNMAP_DOMAIN_PAGE(l3t);                     \
>>> +        l3e; })
>>
>> There's a reason these are macros rather than inline functions,
>> I assume? (This reason would be nice to be stated in the
>> description.)
> 
> Actually no specific reasons, just to keep them as macros to be
> consistent with other helpers above. Fairly trivial to convert them
> into inline functions if this is desired.

Where possible this would be the preferred route for new helpers.

>> I don't see why you use UNMAP_DOMAIN_PAGE() rather than
>> unmap_domain_page() here - the variables in question go out of
>> scope right afterwards, so the storing of NULL into them is
>> pointless (and very likely to be eliminated by the compiler
>> anyway).
> 
> My intention is to just avoid using the lower case one altogether, so
> that one day when we need to expand any function or macros we do not
> need to look back at the code and worry about whether any lower case
> ones need to be converted to upper case (sometimes for large functions
> this may not be trivial), and the compiler can deal with the extra
> nullification anyway.

I don't agree with this goal; perhaps others on Cc have an opinion?

Jan