RE: [PATCH v2 1/6] xen/arm: do not free reserved memory into heap

[Penny Zheng <Penny.Zheng@arm.com>]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 19, 2022 4:59 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 1/6] xen/arm: do not free reserved memory into heap
> 
> On 18.04.2022 14:22, Penny Zheng wrote:
> > Pages as guest RAM for static domain, shall be reserved to this domain only.
> 
> Is there "used" missing as the 2nd word of the sentence?
> 
> > 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_domheap_pages in runtime
> > for static domain freeing memory resource, so let's drop the __init
> > flag.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v2 changes:
> > - new commit
> > ---
> >  xen/common/page_alloc.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> With this diffstat the patch subject prefix is somewhat misleading; I first
> thought I could skip this patch.
> 

Oh, sorry. Will change the 'xen/arm' to 'xen'

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2488,7 +2488,13 @@ void free_domheap_pages(struct page_info *pg,
> unsigned int order)
> >              scrub = 1;
> >          }
> >
> > -        free_heap_pages(pg, order, scrub);
> > +#ifdef CONFIG_STATIC_MEMORY
> > +        if ( pg->count_info & PGC_reserved )
> > +            /* Reserved page shall not go back to the heap. */
> > +            free_staticmem_pages(pg, 1 << order, scrub);
> 
> 1UL with, in particular, the function parameter by "unsigned long".
> 
> By calling free_staticmem_pages() at runtime, you make the previous race free
> (because of init-time only) update of .count_info there racy. Making a clone of
> that function just for this difference would likely be excessive, so I'd suggest to
> change the code there to
> 
>         /* In case initializing page of static memory, mark it PGC_reserved. */
>         if ( !(pg[i].count_info & PGC_reserved) )
>             pg[i].count_info |= PGC_reserved;
> 

Learned!

> > +        else
> > +#endif
> > +            free_heap_pages(pg, order, scrub);
> 
> Of course it would be nice to avoid the #ifdef-ary here. May I ask that you
> introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY
> case, such that the construct can become
> 

Sure, will do.

>         if ( !(pg->count_info & PGC_reserved) )
>             free_heap_pages(pg, order, scrub);
>         else
>             /* Reserved page shall not go back to the heap. */
>             free_staticmem_pages(pg, 1 << order, scrub);
> 
> Another question is whether the distinction should be made here in the first
> place. Would it perhaps better belong in free_heap_pages() itself, thus also
> covering other potential call sites? Of course this depends on where, long term,
> reserved pages can / will be used.
> For domains to be truly static, Xen's own allocations to manage the domain
> may also want to come from the reserved set ...
> 

Yes, you're right. I'll defer the distinction to free_heap_pages. And refine the
in-code comment above free_staticmem_pages, in the first place, I was intending
to make it equivalent of free_heap_pages to free static memory.

However as you said, if letting free_heap_pages call free_staticmem_pages, it will 
cover other potential call site. We've already been trying to enable p2m pool on arm,
and in the future, maybe the pages constituting the pool shall not come from heap, but
from reserved set, if the domain is fully static

> > @@ -2636,7 +2642,7 @@ struct domain *get_pg_owner(domid_t domid)
> >
> >  #ifdef CONFIG_STATIC_MEMORY
> >  /* Equivalent of free_heap_pages to free nr_mfns pages of static
> > memory. */ -void __init free_staticmem_pages(struct page_info *pg,
> > unsigned long nr_mfns,
> > +void free_staticmem_pages(struct page_info *pg, unsigned long
> > +nr_mfns,
> >                                   bool need_scrub)
> 
> This line now wants its indentation adjusted.
> 
> Jan