Re: [PATCH v2] x86/vmap: handle superpages in vmap_to_mfn()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2020 12:21, Hongyan Xia wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>          }                                          \
>      } while ( false )
>  
> +/* Translate mapped Xen address to MFN. */
> +mfn_t xen_map_to_mfn(unsigned long va)
> +{
> +#define CHECK_MAPPED(cond_)     \
> +    if ( !(cond_) )             \
> +    {                           \
> +        ASSERT_UNREACHABLE();   \
> +        ret = INVALID_MFN;      \
> +        goto out;               \
> +    }                           \

This should be coded such that use sites ...

> +    bool locking = system_state > SYS_STATE_boot;
> +    unsigned int l2_offset = l2_table_offset(va);
> +    unsigned int l1_offset = l1_table_offset(va);
> +    const l3_pgentry_t *pl3e = virt_to_xen_l3e(va);
> +    const l2_pgentry_t *pl2e = NULL;
> +    const l1_pgentry_t *pl1e = NULL;
> +    struct page_info *l3page;
> +    mfn_t ret;
> +
> +    L3T_INIT(l3page);
> +    CHECK_MAPPED(pl3e)
> +    l3page = virt_to_page(pl3e);
> +    L3T_LOCK(l3page);
> +
> +    CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT)

... will properly require a statement-ending semicolon. With
additionally the trailing underscore dropped from the macro's
parameter name
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Or wait,

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void);
>  void free_xen_pagetable_new(mfn_t mfn);
>  
>  l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
> +mfn_t xen_map_to_mfn(unsigned long va);

This is now a pretty proper companion of map_page_to_xen(), and
hence imo ought to be declared next to that one rather than here.
Ultimately Arm may also need to gain an implementation.

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -291,7 +291,7 @@ 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)     l1e_get_mfn(*virt_to_xen_l1e((unsigned 
> long)(va)))
> +#define vmap_to_mfn(va)     xen_map_to_mfn((unsigned long)va)

You've lost parentheses around va.

Jan