> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Tuesday, June 7, 2022 5:13 PM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Jan Beulich <jbeul...@suse.com>; Wei Liu <w...@xen.org>
> 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.zh...@arm.com>
> > ---
> > 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
