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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, May 31, 2022 4:37 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 2/9] xen: do not free reserved memory into heap
> 
> 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.

Later, in commit "xen/arm: unpopulate memory when domain is static", 
we will use this flag in xen/arch/arm/include/asm/mm.h

> > --- 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.
> 

Sure, will remove

> Jan