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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Finally the pointers should each be "to const", as you're
only reading through them.

Jan