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

[Hongyan Xia <hx242@xen.org>]

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.

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

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

Good point. Will const qualify in next revision.

Thanks,
Hongyan