Re: [PATCH v8 03/15] x86/mm: rewrite virt_to_xen_l*e

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2020 16:21, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> Rewrite those functions to use the new APIs. Modify its callers to unmap
> the pointer returned. Since alloc_xen_pagetable_new() is almost never
> useful unless accompanied by page clearing and a mapping, introduce a
> helper alloc_map_clear_xen_pt() for this sequence.
> 
> Note that the change of virt_to_xen_l1e() also requires vmap_to_mfn() to
> unmap the page, which requires domain_page.h header in vmap.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> ---
> Changed in v8:
> - s/virtual address/linear address/.
> - BUG_ON() on NULL return in vmap_to_mfn().

The justification for this should be recorded in the description. In
reply to v7 I did even suggest how to easily address the issue you
did notice with large pages, as well as alternative behavior for
vmap_to_mfn().

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -291,7 +291,15 @@ void copy_page_sse2(void *, const void *);
>  #define pfn_to_paddr(pfn)   __pfn_to_paddr(pfn)
>  #define paddr_to_pfn(pa)    __paddr_to_pfn(pa)
>  #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
> -#define vmap_to_mfn(va)     _mfn(l1e_get_pfn(*virt_to_xen_l1e((unsigned 
> long)(va))))
> +
> +#define vmap_to_mfn(va) ({                                                  \
> +        const l1_pgentry_t *pl1e_ = virt_to_xen_l1e((unsigned long)(va));   \
> +        mfn_t mfn_;                                                         \
> +        BUG_ON(!pl1e_);                                                     \
> +        mfn_ = l1e_get_mfn(*pl1e_);                                         \
> +        unmap_domain_page(pl1e_);                                           \
> +        mfn_; })

Additionally - no idea why I only notice this now, this wants some
further formatting adjustment: Either

#define vmap_to_mfn(va) ({                                                \
        const l1_pgentry_t *pl1e_ = virt_to_xen_l1e((unsigned long)(va)); \
        mfn_t mfn_;                                                       \
        BUG_ON(!pl1e_);                                                   \
        mfn_ = l1e_get_mfn(*pl1e_);                                       \
        unmap_domain_page(pl1e_);                                         \
        mfn_;                                                             \
    })

or (preferably imo)

#define vmap_to_mfn(va) ({                                            \
    const l1_pgentry_t *pl1e_ = virt_to_xen_l1e((unsigned long)(va)); \
    mfn_t mfn_;                                                       \
    BUG_ON(!pl1e_);                                                   \
    mfn_ = l1e_get_mfn(*pl1e_);                                       \
    unmap_domain_page(pl1e_);                                         \
    mfn_;                                                             \
})

Jan