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

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

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, May 17, 2022 2:01 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v4 1/6] xen: do not free reserved memory into heap
> 
> Hi Penny,
> 
> On 10/05/2022 03:27, 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@arm.com>
> > ---
> > 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/common/page_alloc.c | 17 ++++++++++++++---
> >   xen/include/xen/mm.h    |  2 +-
> >   2 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 319029140f..5e569a48a2 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1443,6 +1443,10 @@ static void free_heap_pages(
> >
> >       ASSERT(order <= MAX_ORDER);
> >
> > +    if ( pg->count_info & PGC_reserved )
> 
> NIT: I would suggest to use "unlikely()" here.
> 
> > +        /* Reserved page shall not go back to the heap. */
> > +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> > +
> >       spin_lock(&heap_lock);
> >
> >       for ( i = 0; i < (1 << order); i++ ) @@ -2636,8 +2640,8 @@
> > 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,
> > -                                 bool need_scrub)
> > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub)
> 
> Looking at the implementation of free_staticmem_pages(), the page will be
> scrubbed synchronously.
> 
> If I am not mistaken, static memory is not yet supported so I would be OK to
> continue with synchronous scrubbing. However, this will need to be
> asynchronous before we even consider to security support it.
> 

Yes,  I remembered that asynchronous is still on the to-do list for static memory.

If it doesn't bother too much to you, I would like to ask some help on this issue, ;).
I only knew basic knowledge on the scrubbing, I knew that dirty pages is placed at the
end of list heap(node, zone, order) for scrubbing and "first_dirty" is used to track down
the dirty pages. IMO, Both two parts are restricted to the heap thingy,  not reusable for
static memory, so maybe I need to re-write scrub_free_page for static memory, and also
link the need-to-scrub reserved pages to a new global list e.g.  dirty_resv_list for aync
scrubbing?
 Any suggestions?

> BTW, SUPPORT.md doesn't seem to explicitely say whether static memory is
> supported. Would you be able to send a patch to update it? I think this should
> be tech preview for now.
> 

Sure, will do.

> >   {
> >       mfn_t mfn = page_to_mfn(pg);
> >       unsigned long i;
> > @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info
> *pg, unsigned long nr_mfns,
> >           }
> >
> >           /* In case initializing page of static memory, mark it PGC_reserved. */
> > -        pg[i].count_info |= PGC_reserved;
> > +        if ( !(pg[i].count_info & PGC_reserved) )
> 
> NIT: I understand the flag may have already been set, but I am not convinced if
> it is worth checking it and then set.
> 

Jan suggested that since we remove the __init from free_staticmem_pages, it's now in preemptable
state at runtime, so better be adding this check here. 

> > +            pg[i].count_info |= PGC_reserved;
> 
> 
> >       }
> >   }
> >
> > @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >       return 0;
> >   }
> > +#else
> > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub) {
> > +    ASSERT_UNREACHABLE();
> > +}
> >   #endif
> >
> >   /*
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 3be754da92..9fd95deaec 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -85,10 +85,10 @@ bool scrub_free_pages(void);
> >   } while ( false )
> >   #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
> >
> > -#ifdef CONFIG_STATIC_MEMORY
> >   /* These functions are for static memory */
> >   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> >                             bool need_scrub);
> > +#ifdef CONFIG_STATIC_MEMORY
> >   int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> >                               unsigned int memflags);
> >   #endif
> 
> Cheers,
> 
> --
> Julien Grall