Re: [PATCH v5 2/9] xen: do not free reserved memory into heap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.05.2022 05:12, Penny Zheng wrote:
> Pages used as guest RAM for static domain, shall be reserved to this
> domain only.
> So in case reserved pages being used for other purpose, users
> shall not free them back to heap, even when last ref gets dropped.
> 
> free_staticmem_pages will be called by free_heap_pages in runtime
> for static domain freeing memory resource, so let's drop the __init
> flag.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> v5 changes:
> - In order to avoid stub functions, we #define PGC_staticmem to non-zero only
> when CONFIG_STATIC_MEMORY
> - use "unlikely()" around pg->count_info & PGC_staticmem
> - remove pointless "if", since mark_page_free() is going to set count_info
> to PGC_state_free and by consequence clear PGC_staticmem
> - move #define PGC_staticmem 0 to mm.h
> ---
> v4 changes:
> - no changes
> ---
> v3 changes:
> - fix possible racy issue in free_staticmem_pages()
> - introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
> - move the change to free_heap_pages() to cover other potential call sites
> - fix the indentation
> ---
> v2 changes:
> - new commit
> ---
>  xen/arch/arm/include/asm/mm.h |  2 ++
>  xen/common/page_alloc.c       | 16 +++++++++-------
>  xen/include/xen/mm.h          |  6 +++++-
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 1226700085..56d0939318 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -108,9 +108,11 @@ struct page_info
>    /* Page is Xen heap? */
>  #define _PGC_xen_heap     PG_shift(2)
>  #define PGC_xen_heap      PG_mask(1, 2)
> +#ifdef CONFIG_STATIC_MEMORY
>    /* Page is static memory */
>  #define _PGC_staticmem    PG_shift(3)
>  #define PGC_staticmem     PG_mask(1, 3)
> +#endif
>  /* ... */
>  /* Page is broken? */
>  #define _PGC_broken       PG_shift(7)
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 44600dd9cd..6425761116 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -151,10 +151,6 @@
>  #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
>  #endif
>  
> -#ifndef PGC_staticmem
> -#define PGC_staticmem 0
> -#endif
> -

Is the moving of this into the header really a necessary part of this
change? Afaics the symbol is still only ever used in this one C file.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -85,10 +85,10 @@ bool scrub_free_pages(void);
>  } while ( false )
>  #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>  
> -#ifdef CONFIG_STATIC_MEMORY
>  /* These functions are for static memory */
>  void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                            bool need_scrub);
> +#ifdef CONFIG_STATIC_MEMORY
>  int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int 
> nr_mfns,
>                              unsigned int memflags);
>  #endif

Is the #ifdef really worth retaining at this point? Code is generally
better readable without.

Jan