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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.02.2020 11:27, 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)]);

Somehow you've lost the const. I think both this and ...

> --- 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 ) {                      \

... the brace placement here can be dealt with while committing.
With both of them in place
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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