Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory

[Wei Liu <wl@xxxxxxx>]

On Thu, Feb 27, 2020 at 10:27:39AM +0000, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> The function will map and unmap pages on demand.
> 
> Since we now map and unmap Xen PTE pages, we would like to track the
> lifetime of mappings so that 1) we do not dereference memory through a
> variable after it is unmapped, 2) we do not unmap more than once.
> Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the
> variable after unmapping, and ignore NULL.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> 
> ---
> Changed in v2:
> - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid
>   adding the check in unmap_domain_page.
> - reword the commit message.
> ---
>  xen/arch/x86/mm.c             | 14 ++++++++------
>  xen/include/xen/domain_page.h |  7 +++++++
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 70b87c4830..9fcdcde5b7 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -356,19 +356,21 @@ void __init arch_init_memory(void)
>              ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS);
>              if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) )
>              {
> -                l3_pgentry_t *l3tab = alloc_xen_pagetable();
> +                mfn_t l3mfn = alloc_xen_pagetable_new();
>  
> -                if ( l3tab )
> +                if ( !mfn_eq(l3mfn, INVALID_MFN) )
>                  {
> -                    const l3_pgentry_t *l3idle =
> -                        l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]);
> +                    l3_pgentry_t *l3idle = map_l3t_from_l4e(
> +                            idle_pg_table[l4_table_offset(split_va)]);
> +                    l3_pgentry_t *l3tab = map_domain_page(l3mfn);
>  
>                      for ( i = 0; i < l3_table_offset(split_va); ++i )
>                          l3tab[i] = l3idle[i];
>                      for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
>                          l3tab[i] = l3e_empty();
> -                    split_l4e = l4e_from_mfn(virt_to_mfn(l3tab),
> -                                             __PAGE_HYPERVISOR_RW);
> +                    split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW);
> +                    UNMAP_DOMAIN_PAGE(l3idle);
> +                    UNMAP_DOMAIN_PAGE(l3tab);
>                  }
>                  else
>                      ++root_pgt_pv_xen_slots;
> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> index 32669a3339..bfc3bf6aeb 100644
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void 
> *va) {};
>  
>  #endif /* !CONFIG_DOMAIN_PAGE */
>  
> +#define UNMAP_DOMAIN_PAGE(p) do {   \
> +    if ( p ) {                      \

Coding style mandates { should be placed in the next line.

Other than this, the code looks correct to me.

Wei.

> +        unmap_domain_page(p);       \
> +        (p) = NULL;                 \
> +    }                               \
> +} while ( false )
> +
>  #endif /* __XEN_DOMAIN_PAGE_H__ */
> -- 
> 2.17.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel