RE: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is static

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Wednesday, April 27, 2022 6:11 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Jan Beulich <jbeulich@xxxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> Hi Penny,
> 
> On 27/04/2022 10:27, Penny Zheng wrote:
> > Today when a domain unpopulates the memory on runtime, they will
> > always hand the memory back to the heap allocator. And it will be a
> > problem if domain is static.
> >
> > Pages as guest RAM for static domain shall be reserved to only this
> > domain and not be used for any other purposes, so they shall never go
> > back to heap allocator.
> >
> > This commit puts reserved pages on the new list resv_page_list only
> > after having taken them off the "normal" list, when the last ref dropped.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> > v3 changes:
> > - have page_list_del() just once out of the if()
> > - remove resv_pages counter
> > - make arch_free_heap_page be an expression, not a compound statement.
> > ---
> > v2 changes:
> > - put reserved pages on resv_page_list after having taken them off the
> > "normal" list
> > ---
> >   xen/arch/arm/include/asm/mm.h | 12 ++++++++++++
> >   xen/common/domain.c           |  4 ++++
> >   xen/include/xen/sched.h       |  3 +++
> >   3 files changed, 19 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/mm.h
> > b/xen/arch/arm/include/asm/mm.h index 424aaf2823..c6426c1705 100644
> > --- a/xen/arch/arm/include/asm/mm.h
> > +++ b/xen/arch/arm/include/asm/mm.h
> > @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info
> > *page);
> >
> >   unsigned int arch_get_dma_bitsize(void);
> >
> > +/*
> > + * Put free pages on the resv page list after having taken them
> > + * off the "normal" page list, when pages from static memory  */
> > +#ifdef CONFIG_STATIC_MEMORY
> > +#define arch_free_heap_page(d, pg) ({                   \
> > +    page_list_del(pg, page_to_list(d, pg));             \
> > +    if ( (pg)->count_info & PGC_reserved )              \
> > +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> > +})
> > +#endif
> 
> I am a bit puzzled how this is meant to work.
> 
> Looking at the code, arch_free_heap_page() will be called from
> free_domheap_pages(). If I am not mistaken, reserved pages are not
> considered as xen heap pages, so we would go in the else which will end up to
> call free_heap_pages().
> 
> free_heap_pages() will end up to add the page in the heap allocator and
> corrupt the d->resv_page_list because there are only one link list.
> 
> What did I miss?
> 

In my first commit "do not free reserved memory into heap", I've changed the 
behavior
for reserved pages in free_heap_pages()
+    if ( pg->count_info & PGC_reserved )
+        /* Reserved page shall not go back to the heap. */
+        return free_staticmem_pages(pg, 1UL << order, need_scrub);
+

> Cheers,
>
> --
> Julien Grall

Cheers,

--
Penny Zheng