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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, June 7, 2022 5:13 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@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 v6 2/9] xen: do not free reserved memory into heap
> 
> Hi Penny,
> 

Hi Julien

> On 07/06/2022 08:30, 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>
> > ---
> > v6 changes:
> > - adapt to PGC_static
> > - remove #ifdef aroud function declaration
> > ---
> > 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 |  4 +++-
> >   xen/common/page_alloc.c       | 12 +++++++++---
> >   xen/include/xen/mm.h          |  2 --
> >   3 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/mm.h
> > b/xen/arch/arm/include/asm/mm.h index fbff11c468..7442893e77 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)
> > -  /* Page is static memory */
> 
> NITpicking: You added this comment in patch #1 and now removing the space.
> Any reason to drop the space?
> 
> > +#ifdef CONFIG_STATIC_MEMORY
> 
> I think this change ought to be explained in the commit message. AFAIU, this 
> is
> necessary to allow the compiler to remove code and avoid linking issues. Is
> that correct?
> 
> > +/* Page is static memory */
> >   #define _PGC_static    PG_shift(3)
> >   #define PGC_static     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
> > 9e5c757847..6876869fa6 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1443,6 +1443,13 @@ static void free_heap_pages(
> >
> >       ASSERT(order <= MAX_ORDER);
> >
> > +    if ( unlikely(pg->count_info & PGC_static) )
> > +    {
> > +        /* Pages of static memory shall not go back to the heap. */
> > +        free_staticmem_pages(pg, 1UL << order, need_scrub);
> I can't remember whether I asked this before (I couldn't find a thread).
> 
> free_staticmem_pages() doesn't seem to be protected by any lock. So how do
> you prevent the concurrent access to the page info with the acquire part?

True, last time you suggested that rsv_page_list needs to be protected with a
spinlock (mostly like d->page_alloc_lock). I haven't thought it thoroughly, 
sorry
about that.
So for freeing part, I shall get the lock at arch_free_heap_page(), where we 
insert
the page to the rsv_page_list, and release the lock at the end of the 
free_staticmem_page.
And for acquiring part, I've already put the lock around 
page = page_list_remove_head(&d->resv_page_list);

> Cheers,
> 
> --
> Julien Grall