Hi Julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Thursday, June 9, 2022 5:22 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,
>
> On 09/06/2022 06:54, Penny Zheng wrote:
> >
> >
> >> -----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.
>
> In general, a lock is better to be lock/unlock in the same function because
> it is
> easier to verify. However, I am not sure that extending the locking from d-
> >page_alloc_lock up after free_heap_pages() is right.
>
> The first reason being that they are other callers of free_heap_pages().
> So now all the callers of the helpers would need to know whether they need to
> help d->page_alloc_lock.
>
> Secondly, free_staticmem_pages() is meant to be the reverse of
> prepare_staticmem_pages(). We should prevent both of them to be called
> concurrently. It sounds strange to use the d->page_alloc_lock to protect it (a
> page is technically not belonging to a domain at this point).
>
> To me it looks like we want to add the pages on the rsv_page_list
> *after* the pages have been freed. So we know that all the pages on that list
> have been marked as freed (i.e. free_staticmem_pages() completed).
>
Yes! That fixes everything!
If we add the pages on the rsv_page_list *after* the pages have been freed, we
could make sure that page acquired from rsv_page_list has been totally freed.
Hmmm, so in such case, do we still need to add lock here?
We already could make sure that page acquired from rsv_page_list must be totally
freed, without the lock.
Or in facts, we use the lock to let prepare_staticmem_pages() not fail if
free_staticmem_pages
happen concurrently?
Trying to understand the intents of the lock here more clearly, hope it's not a
dumb question~
`> In addition to that, we would need the code in free_staticmem_pages() to be
> protected by the heap_lock (at least so it is match
> prepare_staticmem_pages()).
>
> Any thoughts?
>
> Cheers,
>
> --
> Julien Grall
