RE: [PATCH v9 2/8] xen: do not free reserved memory into heap

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, July 25, 2022 11:30 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 v9 2/8] xen: do not free reserved memory into heap
> 
> On 20.07.2022 07:46, 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.
> >
> > This commit introduces a new helper free_domstatic_page to free static
> > page in runtime, and free_staticmem_pages will be called by it in
> > runtime, so let's drop the __init flag.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> 
> Technically
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Nevertheless two remarks:
> 
> > +void free_domstatic_page(struct page_info *page) {
> > +    struct domain *d = page_get_owner(page);
> > +    bool drop_dom_ref;
> > +
> > +    ASSERT(d);
> 
> I wonder whether
> 
>     if ( unlikely(!d) )
>     {
>         ASSERT_UNREACHABLE();
>         return;
>     }
> 
> wouldn't be more robust looking forward.
> 
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -85,13 +85,12 @@ 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);
> > +void free_domstatic_page(struct page_info *page);
> >  int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> >                              unsigned int memflags); -#endif
> >
> >  /* Map machine page range in Xen virtual address space. */  int
> > map_pages_to_xen( @@ -212,6 +211,10 @@ extern struct domain
> *dom_cow;
> >
> >  #include <asm/mm.h>
> >
> > +#ifndef PGC_static
> > +#define PGC_static 0
> > +#endif
> 
> This disconnect from all other PGC_* values isn't very nice. I wonder as how
> bad it would be seen if Arm kept its #define to 0 private, with the generic
> fallback remaining in page_alloc.c.
> 

It, right now, is only used in xen/arch/arm/mm.c and xen/common/page_alloc.c.
It is ok to let Arm keep its #define to 0 private, with the generic
fallback remaining in page_alloc.c.

> Jan