Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 29/06/2022 07:19, Jan Beulich wrote:
> On 29.06.2022 08:08, Penny Zheng wrote:
> > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > Sent: Wednesday, June 29, 2022 1:56 PM
> > >
> > > On 29.06.2022 05:12, Penny Zheng wrote:
> > > > > From: Julien Grall <julien@xxxxxxx>
> > > > > Sent: Monday, June 27, 2022 6:19 PM
> > > > >
> > > > > On 27/06/2022 11:03, Penny Zheng wrote:
> > > > > > > -----Original Message-----
> > > > > > put_static_pages, that is, adding pages to the reserved list, is
> > > > > > only for freeing static pages on runtime. In static page
> > > > > > initialization stage, I also use free_statimem_pages, and in which
> > > > > > stage, I think the domain has not been constructed at all. So I
> > > > > > prefer the freeing of staticmem pages is split into two parts:
> > > > > > free_staticmem_pages and put_static_pages
> > > > > AFAIU, all the pages would have to be allocated via
> > > > > acquire_domstatic_pages(). This call requires the domain to be
> > > > > allocated and setup for handling memory.
> > > > >
> > > > > Therefore, I think the split is unnecessary. This would also have the
> > > > > advantage to remove one loop. Admittly, this is not important when
> > > > > the order 0, but it would become a problem for larger order (you may
> > > > > have to pull the struct page_info multiple time in the cache).
> > > > How about this:
> > > > I create a new func free_domstatic_page, and it will be like:
> > > > "
> > > > static void free_domstatic_page(struct domain *d, struct page_info
> > > > *page) {
> > > >      unsigned int i;
> > > >      bool need_scrub;
> > > >
> > > >      /* NB. May recursively lock from relinquish_memory(). */
> > > >      spin_lock_recursive(&d->page_alloc_lock);
> > > >
> > > >      arch_free_heap_page(d, page);
> > > >
> > > >      /*
> > > >       * Normally we expect a domain to clear pages before freeing them,
> > > >       * if it cares about the secrecy of their contents. However, after
> > > >       * a domain has died we assume responsibility for erasure. We do
> > > >       * scrub regardless if option scrub_domheap is set.
> > > >       */
> > > >      need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
> > > >
> > > >      free_staticmem_pages(page, 1, need_scrub);
> > > >
> > > >      /* Add page on the resv_page_list *after* it has been freed. */
> > > >      put_static_page(d, page);
> > > >
> > > >      drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> > > >
> > > >      spin_unlock_recursive(&d->page_alloc_lock);
> > > >
> > > >      if ( drop_dom_ref )
> > > >          put_domain(d);
> > > > }
> > > > "
> > > >
> > > > In free_domheap_pages, we just call free_domstatic_page:
> > > >
> > > > "
> > > > @@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg,
> > > > unsigned int order)
> > > >
> > > >       ASSERT_ALLOC_CONTEXT();
> > > >
> > > > +    if ( unlikely(pg->count_info & PGC_static) )
> > > > +        return free_domstatic_page(d, pg);
> > > > +
> > > >       if ( unlikely(is_xen_heap_page(pg)) )
> > > >       {
> > > >           /* NB. May recursively lock from relinquish_memory(). */ @@
> > > > -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg,
> > > > unsigned long nr_mfns, "
> > > >
> > > > Then the split could be avoided and we could save the loop as much as
> > > possible.
> > > > Any suggestion?
> > > Looks reasonable at the first glance (will need to see it in proper context for 
> > > a
> > > final opinion), provided e.g. Xen heap pages can never be static.
> > If you don't prefer let free_domheap_pages to call free_domstatic_page, then, 
> > maybe
> > the if-array should happen at put_page
> > "
> > @@ -1622,6 +1622,8 @@ void put_page(struct page_info *page)
> >
> >       if ( unlikely((nx & PGC_count_mask) == 0) )
> >       {
> > +        if ( unlikely(page->count_info & PGC_static) )
At a first glance, this would likely need to be tested against 'nx'.

> > +            free_domstatic_page(page);
> >           free_domheap_page(page);
> >       }
> >   }
> > "
> > Wdyt now?
> Personally I'd prefer this variant, but we'll have to see what Julien or
> the other Arm maintainers think.
I think this is fine so long we are not expecting more places where 
free_domheap_page() may need to be replaced with free_domstatic_page().
I can't think of any at the moment, but I would like Penny to confirm 
what Arm plans to do with static memory.
Cheers,

--
Julien Grall