Re: [PATCH] domain_page: handle NULL within unmap_domain_page() itself

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/05/2020 16:43, Hongyan Xia wrote:
> From: Hongyan Xia <hongyxia@xxxxxxxxxx>
>
> The macro version UNMAP_DOMAIN_PAGE() does both NULL checking and
> variable clearing. Move NULL checking into the function itself so that
> the semantics is consistent with other similar constructs like XFREE().
> This also eases the use unmap_domain_page() in error handling paths,
> where we only care about NULL checking but not about variable clearing.
>
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

> ---
>   xen/arch/arm/mm.c             | 3 +++
>   xen/arch/x86/domain_page.c    | 2 +-
>   xen/include/xen/domain_page.h | 7 ++-----
>   3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 727107eefa..1b14f49345 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -498,6 +498,9 @@ void unmap_domain_page(const void *va)
>       lpae_t *map = this_cpu(xen_dommap);
>       int slot = ((unsigned long) va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
>   
> +    if ( !va )
> +        return;
> +
>       local_irq_save(flags);
>   
>       ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
> diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
> index dd32712d2f..b03728e18e 100644
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -181,7 +181,7 @@ void unmap_domain_page(const void *ptr)
>       unsigned long va = (unsigned long)ptr, mfn, flags;
>       struct vcpu_maphash_entry *hashent;
>   
> -    if ( va >= DIRECTMAP_VIRT_START )
> +    if ( !va || va >= DIRECTMAP_VIRT_START )
>           return;
>   
>       ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> index ab2be7b719..a182d33b67 100644
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -73,11 +73,8 @@ static inline void unmap_domain_page_global(const void *va) 
> {};
>   #endif /* !CONFIG_DOMAIN_PAGE */
>   
>   #define UNMAP_DOMAIN_PAGE(p) do {   \
> -    if ( p )                        \
> -    {                               \
> -        unmap_domain_page(p);       \
> -        (p) = NULL;                 \
> -    }                               \
> +    unmap_domain_page(p);           \
> +    (p) = NULL;                     \
>   } while ( false )
>   
>   #endif /* __XEN_DOMAIN_PAGE_H__ */
--
Julien Grall